fix(daemon): race condition when shutdownAction is none on first up - #784
Conversation
The inactivity daemon decided whether to auto-stop a workspace from a global --shutdown-action flag baked in at install time. On the first up that flag was empty: installDaemon runs before the devcontainer config is resolved, and the host has no LastDevContainerConfig to send yet. The daemon therefore ignored shutdownAction "none" and stopped the workspace once activity went stale, failing "shutdownAction none suppresses inactivity timeout". Resolve shutdownAction per-workspace at patrol time instead: - persist the build-resolved devcontainer config back to workspace.json - daemon reads the per-workspace shutdownAction, falling back to the flag Also fix writeWorkspaceInfo marshaling the original instead of the clone, which persisted CLIOptions despite the intent to drop them.
✅ Deploy Preview for images-devsy-sh canceled.
|
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughResolved devcontainer configuration is persisted during workspace startup. The daemon now uses the workspace’s resolved shutdown action, falling back to its install-time action when unavailable. Persistence tests cover round-tripping, validation, CLI option sanitization, and non-mutation. ChangesWorkspace shutdown configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UpCmd
participant devsyUp
participant AgentPersistence
participant workspace.json
participant Daemon
UpCmd->>devsyUp: start workspace
devsyUp-->>UpCmd: resolved devcontainer configuration
UpCmd->>AgentPersistence: persist workspace configuration
AgentPersistence->>workspace.json: write sanitized workspace info
Daemon->>workspace.json: read workspace info
Daemon->>Daemon: use resolved or fallback shutdown action
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for devsydev canceled.
|
skevetter
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/internal/agent_daemon_test.go (1)
93-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the existing-config/empty-action fallback shape.
workspaceWithShutdownAction("")returns without creatingLastDevContainerConfig, so this only tests the nil-config fallback. Add a case where the config wrapper exists butShutdownAction == ""to cover the separate production branch for older or partialworkspace.jsonfiles.Also applies to: 112-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/internal/agent_daemon_test.go` around lines 93 - 101, Extend the tests around workspaceWithShutdownAction to include a workspace whose LastDevContainerConfig and nested Config exist while ShutdownAction is empty. Exercise the production fallback path for an existing configuration with an empty action, separately from the current nil-config case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/internal/agentworkspace/up.go`:
- Around line 754-765: Update persistResolvedConfig and its startup caller so
PersistAgentWorkspaceInfo failures are propagated or retried and prevent a
successful startup result from being forwarded. Preserve the existing early
returns and successful persistence behavior, and add coverage for the
failed-persistence path to ensure stale shutdown policy is not accepted.
---
Nitpick comments:
In `@cmd/internal/agent_daemon_test.go`:
- Around line 93-101: Extend the tests around workspaceWithShutdownAction to
include a workspace whose LastDevContainerConfig and nested Config exist while
ShutdownAction is empty. Exercise the production fallback path for an existing
configuration with an empty action, separately from the current nil-config case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fa7b303-5549-4208-b61c-d9dbc4482733
📒 Files selected for processing (5)
cmd/internal/agent_daemon.gocmd/internal/agent_daemon_test.gocmd/internal/agentworkspace/up.gopkg/agent/agent.gopkg/agent/persist_test.go
| func persistResolvedConfig( | ||
| workspaceInfo *provider.AgentWorkspaceInfo, | ||
| result *config2.Result, | ||
| ) { | ||
| if result == nil || result.Error != "" || result.DevContainerConfigWithPath == nil { | ||
| return | ||
| } | ||
|
|
||
| workspaceInfo.LastDevContainerConfig = result.DevContainerConfigWithPath | ||
| if err := agent.PersistAgentWorkspaceInfo(workspaceInfo); err != nil { | ||
| log.Errorf("persist resolved devcontainer config: %v", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not silently accept failed shutdown-policy persistence.
When PersistAgentWorkspaceInfo fails, this helper only logs and the caller still forwards a successful startup result. The daemon can then fall back to a stale or install-time action, reintroducing the shutdownAction: none failure when workspace.json was not updated. Retry or propagate this failure before reporting startup success, and test the failure path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/internal/agentworkspace/up.go` around lines 754 - 765, Update
persistResolvedConfig and its startup caller so PersistAgentWorkspaceInfo
failures are propagated or retried and prevent a successful startup result from
being forwarded. Preserve the existing early returns and successful persistence
behavior, and add coverage for the failed-persistence path to ensure stale
shutdown policy is not accepted.
1 participant
Summary
Fixes a race in the machine-provider inactivity daemon where
shutdownAction: nonewas ignored on the firstup, so the workspace was auto-stopped despite the setting. This surfaced as the flaky e2e failuretest shutdownAction none suppresses inactivity timeout(devsy stop failed— the workspace was alreadyStopped).Root cause
The daemon decided whether to auto-stop from a global
--shutdown-actionflag baked in at install time (checkAndShutdown).installDaemonsources that flag fromworkspaceInfo.LastDevContainerConfig, but it runs duringinitialize()beforerunner.Upresolves the devcontainer config. On the first upLastDevContainerConfigisnil(it's only persisted host-side after a successful up), so the daemon started with an empty shutdownAction, ignorednone, and ran the provider's shutdown once activity went stale.Fix
Resolve
shutdownActionper-workspace, at patrol time instead of from a one-shot global flag:workspace.json(persistResolvedConfig+ newagent.PersistAgentWorkspaceInfo).LastDevContainerConfig.Config.ShutdownAction(effectiveShutdownAction), falling back to the install-time flag when absent. This is dynamic (picked up next patrol, no systemd restart) and also correct for multiple workspaces under one daemon.Also fixes a latent bug in
writeWorkspaceInfo: it clearedCLIOptionson a clone but marshaled the original, persisting them anyway.Tests
TestEffectiveShutdownAction— per-workspace precedence and fallback paths.TestPersistAgentWorkspaceInfo_*— resolved config round-trips through the daemon's parse path;CLIOptionsare not persisted.CodeRabbit local review: no findings.
Summary by CodeRabbit
Bug Fixes
Tests