fix: report fatal sandbox errors to the session and its caller - #1599
Open
duboff wants to merge 1 commit into
Open
fix: report fatal sandbox errors to the session and its caller#1599duboff wants to merge 1 commit into
duboff wants to merge 1 commit into
Conversation
The supervisor already posts {error, fatal: true} when a sandbox cannot
continue, but nothing downstream acts on it.
The error event extended messageSandboxEventBaseSchema, so messageId was
required — and a fatal boot error happens before any message exists, so
the event was rejected at the parse layer. fatal was not in the schema at
all. Even parsed, only execution_complete moved session status or called
notifyComplete, so a repository that would not clone or an image that
would not start left the session sitting active with no messages, and
whoever asked for the work heard nothing.
Settling reuses the queue's existing failMessage, which already
synthesizes the terminal event, records it, projects it and notifies the
caller for stop, cancel and the stuck-processing timeout. That path only
ever sees processing; a prompt that fails during boot is still pending.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughFatal sandbox errors can now fail unfinished prompts and transition the session to ChangesSandbox error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change reports fatal sandbox startup failures to sessions and callers instead of leaving work pending; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Sandbox
participant SessionSandboxEventProcessor
participant SessionMessageQueue
participant statusService
Sandbox->>SessionSandboxEventProcessor: Send fatal error event
SessionSandboxEventProcessor->>SessionSandboxEventProcessor: Persist and broadcast event
SessionSandboxEventProcessor->>SessionMessageQueue: Fail unfinished messages
SessionSandboxEventProcessor->>statusService: Transition session to failed
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A sandbox that dies before it can run leaves its session sitting
activewith no messages, and never tells whoever asked for the work. Three things line up to produce that:supervisor.pyalready posts{"error": message, "fatal": true}via_report_fatal_error, but theerrorvariant ofsandboxEventSchemaextendedmessageSandboxEventBaseSchema, somessageIdwas required. A fatal boot error happens before any message exists, so the one event that most needs to get through is exactly the one that cannot.fatalwas not in the schema at all.execution_completetransitions, so the session staysactiveindefinitely. At any scale this is unpleasant to operate: a total sandbox outage is indistinguishable from a pile of merely-slow sessions.notifyCompleteis only reached fromexecution_complete, so the prompt stayspendingforever and the Slack/Linear/GitHub bot that requested the session hears nothing back. On a PR that means the bot's acknowledgement reaction reads as "still thinking" rather than "died before it began".Changes
shared: on theerrorevent, makemessageIdoptional and addfatal.processSandboxEventalready falls back to the processing message whenmessageIdis absent.message-queue: addfailUnfinishedMessages, next to the existingfailMessagethat already synthesizes the terminal event, records it, projects it and notifies the caller for stop, cancel and the stuck-processing timeout. That path only ever inspects theprocessingmessage; a prompt that fails during boot is stillpending, which is why it was never covered.sandbox-events: on a fatal error, settle in-flight prompts and transition the session tofailed, after persist and broadcast so the reason is on record first.web: anerrorevent can now arrive without amessageId, so it cannot be keyed to a parent Task call — leave it at the top level rather than mis-nesting it.Notes
execution_completeis safe:recordMessageCompletiononly writes while the row still holds the expected status, so whichever event arrives second finds nothing to settle and delivers no second callback.errorstays advisory — status and in-flight prompts are untouched — which the tests pin.Test plan
npm run typecheckandnpm run lintcleancontrol-plane3171 tests pass, including new coverage for settling apendingprompt, settling aprocessingprompt against its own status, the no-op case, and the non-fatal case leaving everything aloneshared686,slack-bot420,github-bot135,linear-bot224 passweb1213 pass; the single failure inuse-session-rename.test.tsxreproduces on an unmodifiedmainand is unrelated to this changeSummary by CodeRabbit