fix: clear stale spawn error when sandbox connects#540
Conversation
When a Modal 524 error occurred during spawn but the container actually started, the UI would still show "failed" due to two bugs: 1. onSandboxConnected() only reset the in-memory spawning flag but did not clear last_spawn_error from the DB, so the stale error persisted even after sandbox_status moved to "ready". 2. The web client's subscribed handler unconditionally overrode sandboxStatus to "failed" whenever spawnError was truthy, ignoring the authoritative sandboxStatus from the server. Now onSandboxConnected() clears the spawn error, and the client only applies the spawnError override when sandboxStatus is actually "failed".
📝 WalkthroughWalkthroughThese changes refine spawn error state management across the sandbox lifecycle. The control-plane manager now clears persistent spawn error records when sandbox connection succeeds, while the web hook uses more precise conditional logic to report failed status. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/hooks/use-session-socket.ts (1)
313-316: RedundantsetSessionStateinside the guarded branch.Now that the override only fires when
data.state?.sandboxStatus === "failed", thesetSessionStatecall on line 315 is a no-op:sessionStatewas already initialized fromdata.stateat lines 287‑292 with that samesandboxStatus: "failed". You can drop the state update and keep just theconsole.error, or remove the branch entirely and rely on the generic state hydration above. Functionally correct either way — flagging as a minor cleanup.♻️ Suggested simplification
- if (data.spawnError && data.state?.sandboxStatus === "failed") { - console.error("Sandbox spawn error:", data.spawnError); - setSessionState((prev) => (prev ? { ...prev, sandboxStatus: "failed" } : null)); - } + if (data.spawnError) { + console.error("Sandbox spawn error:", data.spawnError); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/hooks/use-session-socket.ts` around lines 313 - 316, The if-block that checks data.spawnError and data.state?.sandboxStatus === "failed" contains a redundant setSessionState call because sessionState was already initialized from data.state earlier; remove the setSessionState((prev) => (prev ? { ...prev, sandboxStatus: "failed" } : null)) call (or remove the entire if branch and keep only the console.error for data.spawnError) in use-session-socket.ts so you no longer perform a no-op state update; keep references to data.spawnError and the console.error message to preserve logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/hooks/use-session-socket.ts`:
- Around line 313-316: The if-block that checks data.spawnError and
data.state?.sandboxStatus === "failed" contains a redundant setSessionState call
because sessionState was already initialized from data.state earlier; remove the
setSessionState((prev) => (prev ? { ...prev, sandboxStatus: "failed" } : null))
call (or remove the entire if branch and keep only the console.error for
data.spawnError) in use-session-socket.ts so you no longer perform a no-op state
update; keep references to data.spawnError and the console.error message to
preserve logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb1b48e4-be87-4095-a11c-935d4e76a114
📒 Files selected for processing (2)
packages/control-plane/src/sandbox/lifecycle/manager.tspackages/web/src/hooks/use-session-socket.ts
1 participant
Summary
onSandboxConnected()now callssetLastSpawnError(null, null)to clear the stale error from the DB when a sandbox successfully connects, not just the in-memory spawning flag.subscribedmessage handler now only overridessandboxStatusto"failed"whendata.state.sandboxStatusis actually"failed", rather than unconditionally wheneverspawnErroris truthy.These two bugs combined caused the UI to show "failed" even when a sandbox recovered from a transient Modal 524 error and was actively running.
Test plan
sandboxStatusshows "ready" even if a prior spawn attempt failedSummary by CodeRabbit
Bug Fixes