Repository navigation
fix(host-service): classify terminal session failures by kind, not message text - #6432
Merged
Merged
Conversation
…d of blanket 500s
Contributor
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTerminal operations now return structured session errors. A shared mapper converts these errors to specific TRPC responses. Terminal, agent, and terminal-agent routers use the mapper for session creation and terminal operations. ChangesTerminal error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🔵 Low · up to The change correctly narrows terminal session error classification while preserving reporting for daemon startup failures; merge is reasonable with owner awareness that the repository-wide lint check must still pass. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…ssage text Replace the prose-matching classifier with a discriminated union on the producer. Every session producer now returns a TerminalSessionError carrying a `kind`, and the tRPC boundary switches on it exhaustively. Fixes three misclassifications and removes the mechanism behind them: - agents.run and terminal.createSession filed worktree-gone as a 500 - terminal.writeInput filed a wrong-workspace access as NOT_FOUND rather than FORBIDDEN - terminalAgents.getOrCreate filed every creation outcome as a 500 The daemon-readiness timeout now maps to SERVICE_UNAVAILABLE via the `transient` signal the producer already computed, so a healable daemon blip stops paging while a permanent bootstrap failure still reports.
saddlepaddle
changed the title
This branch was previously deployed
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
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.
Problem
Every terminal session failure reaching tRPC was classified by substring-matching the error's English message, and three call sites skipped even that and hard-coded a status.
Two Sentry issues on 1.20.2 are the visible symptom:
HOST-SERVICE-27— launching an agent into a workspace whose worktree was already deleted. Routine lifecycle state, filed as a 500.HOST-SERVICE-2B— session creation on a memory-pressured machine where the pty daemon did not become ready in its budget.Two more misclassifications were found while fixing those, neither of which had a Sentry issue yet:
terminal.writeInputthrew a blanketNOT_FOUNDfor every failure, so a session owned by a different workspace was reported to the caller as "missing" rather thanFORBIDDEN.terminalAgents.getOrCreatethrew a blanketINTERNAL_SERVER_ERRORfor every creation outcome, the same shape as the two above.Root cause
packages/host-service/src/terminal/terminal.tsproduces session errors as{ error: string }with hand-written prose, at ~15 sites across five functions. The type information existed at the producer and was discarded, so the boundary reconstructed it by guessing:Two of those producer sites pass an arbitrary upstream exception message straight through. Any such message containing "not found" would have been silently reclassified as a non-reporting
NOT_FOUND, hiding a real bootstrap failure. That is the failure mode the no-allowlist rule from #6164 exists to prevent, relocated from the middleware into a helper.The producer had also already begun growing structured discriminants for this exact reason —
transientfor daemon reachability, andcode: "session-gone"on the websocket path — and the HTTP attach route already branches ontransientto pick 503 vs 500. Only the tRPC boundary ignored the structure and matched prose.Fix
Give the producer a discriminated union and let each consumer decide its own mapping.
TerminalSessionErrorKind/TerminalSessionErrorare exported from the terminal module. Every producer (writeInputToSession,getOrAdoptSession,writeFramedInputToSession,snapshotSession,createTerminalSessionInternal) returns one, and every existing site sets akind.errorstring field is unchanged, so the HTTP attach route, the websocket path and existing tests keep working untouched.transientis retained for those consumers.toTerminalIoError(message: string)becomestoTerminalSessionError(failure): one exhaustive switch, no string matching. All five tRPC call sites use it.TERMINAL_START_FAILEDis the only kind that staysINTERNAL_SERVER_ERROR, so it is the only one the Sentry middleware reports.DAEMON_UNAVAILABLEmaps toSERVICE_UNAVAILABLE, derived from thetransientflag the producer already computed rather than from the message.HOST-SERVICE-2B's "daemon: timed out after 5000ms" is raised as aDaemonUnavailableError, i.e. the supervisor heals it and the session may still come up under the same id, so it stops paging. A permanent bootstrap failure (missing binary, no socket path, crash circuit open, handshake rejected) staysTERMINAL_START_FAILEDand keeps reporting.The switch is exhaustive by construction: adding a kind without deciding its code is a compile error at the mapper. Verified by temporarily adding a kind:
This supersedes the first commit on this branch, which shared the old string classifier between the two creation call sites. That fixed the symptom but spread the mechanism.
Verification
bunx biome checkon the five changed files:cd packages/host-service && bun run typecheck:bun test src/trpc/router/:No test was added for the mapper: its exhaustiveness is enforced by the compiler, which is a stronger guarantee than a case-by-case test would give.
Refs HOST-SERVICE-27
Refs HOST-SERVICE-2B
Summary by CodeRabbit