Repository navigation
fix(desktop): classify quit-time terminal-host disposal as typed non-500 - #6197
Conversation
During app quit, runQuitCleanup disposes the terminal-host client while a
terminal.createOrAttach mutation can still be in flight. resetConnectionState
rejected every pending request with a generic Error("Connection lost"), which
surfaced as INTERNAL_SERVER_ERROR and got captured by the Sentry tRPC
middleware (DESKTOP-HD) even though the teardown is intentional.
Reject pending requests with TerminalHostClientDisposedError on the explicit
disconnect/dispose path only, and translate it at the tRPC boundary to
PRECONDITION_FAILED with a typed cause. Unexpected socket drops
(handleDisconnect) keep the generic rejection and still report.
Claude-Session: https://claude.ai/code/session_017sk3KSZ5NLrpTedKWRX2k8
📝 WalkthroughWalkthroughThe terminal host client now reports intentional disposal separately from unexpected connection loss. The terminal router applies shared middleware to terminal procedures and maps disposal failures to a ChangesTerminal disposal error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
Greptile SummaryThis PR distinguishes intentional terminal-host teardown from unexpected connection loss and maps quit-time
Confidence Score: 4/5The PR should not merge until intentional disposal is translated consistently for every terminal-host request that can be pending during quit. Explicit disconnect now rejects all pending request methods with the new typed error, while only Files Needing Attention: apps/desktop/src/main/lib/terminal-host/client.ts and apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/terminal-host/client.ts | Adds a typed intentional-disposal rejection, but applies it to every pending request even though router translation currently covers only one request path. |
| apps/desktop/src/lib/trpc/routers/terminal/terminal.ts | Correctly maps disposal during createOrAttach to a non-500 response, while sibling terminal procedures remain able to leak the same typed error as a 500. |
Sequence Diagram
sequenceDiagram
participant UI as Renderer
participant RPC as Terminal tRPC router
participant Client as TerminalHostClient
participant Quit as Quit cleanup
participant Sentry
UI->>RPC: clearScrollback/listSessions request
RPC->>Client: sendRequest()
Note over Client: Request stored in pendingRequests
Quit->>Client: dispose() / disconnect()
Client-->>RPC: TerminalHostClientDisposedError
Note over RPC: Only createOrAttach translates this error
RPC-->>UI: INTERNAL_SERVER_ERROR
RPC->>Sentry: captureException()
Prompt To Fix All With AI
### Issue 1
apps/desktop/src/main/lib/terminal-host/client.ts:819
**Disposal errors still escape as 500s**
If another terminal-host request such as `clearScrollback` or `listDaemonSessions` is awaiting a daemon response during quit, `disconnect()` rejects it with `TerminalHostClientDisposedError`, but only `createOrAttach` translates that error. The unhandled rejection therefore becomes `INTERNAL_SERVER_ERROR` and is still captured by Sentry.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(desktop): classify quit-time termina..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const [id, pending] of this.pendingRequests.entries()) { | ||
| clearTimeout(pending.timeoutId); | ||
| pending.reject(new Error("Connection lost")); | ||
| pending.reject(pendingRequestError ?? new Error("Connection lost")); |
There was a problem hiding this comment.
If another terminal-host request such as clearScrollback or listDaemonSessions is awaiting a daemon response during quit, disconnect() rejects it with TerminalHostClientDisposedError, but only createOrAttach translates that error. The unhandled rejection therefore becomes INTERNAL_SERVER_ERROR and is still captured by Sentry.
Knowledge Base Used: Desktop App (Electron main + renderer)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/terminal-host/client.ts
Line: 819
Comment:
**Disposal errors still escape as 500s**
If another terminal-host request such as `clearScrollback` or `listDaemonSessions` is awaiting a daemon response during quit, `disconnect()` rejects it with `TerminalHostClientDisposedError`, but only `createOrAttach` translates that error. The unhandled rejection therefore becomes `INTERNAL_SERVER_ERROR` and is still captured by Sentry.
**Knowledge Base Used:** [Desktop App (Electron main + renderer)](https://app.greptile.com/personal-org-6810/-/custom-context/knowledge-base/superset-sh/superset/-/docs/desktop-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![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
🤖 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 `@apps/desktop/src/main/lib/terminal-host/client.ts`:
- Around line 784-787: Extend the shared reset/disposal flow around
resetConnectionState, sendRequestOnStream, and ensureConnected to reject every
in-flight connection waiter and stream request with
TerminalHostClientDisposedError when disconnect() disposes the client. Ensure
ensureConnected checks disposal immediately before publishing CONNECTED so
pre-disconnect attempts cannot restore the client, while preserving generic
errors for unexpected socket loss.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 734d1d1e-8ae9-4001-b73d-ef01fa3df07f
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/terminal-host/client.ts
| pendingRequestError, | ||
| }: { | ||
| emitDisconnected: boolean; | ||
| pendingRequestError?: Error; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate disposal to every in-flight connection operation.
resetConnectionState only rejects entries in pendingRequests. sendRequestOnStream and the connection waiters in ensureConnected use separate promises. If disconnect() runs during stream authentication or connection setup, those paths can return Request timeout: hello or Connection failed while waiting. createOrAttach then bypasses the TerminalHostClientDisposedError mapping in apps/desktop/src/lib/trpc/routers/terminal/terminal.ts Lines 189-195.
Add a shared cancellation path for connection waiters and stream requests. Also guard ensureConnected before it publishes CONNECTED, so an attempt started before disconnect() cannot restore a disposed client. Preserve generic errors for unexpected socket loss.
Also applies to: 819-819, 1663-1666
🤖 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 `@apps/desktop/src/main/lib/terminal-host/client.ts` around lines 784 - 787,
Extend the shared reset/disposal flow around resetConnectionState,
sendRequestOnStream, and ensureConnected to reject every in-flight connection
waiter and stream request with TerminalHostClientDisposedError when disconnect()
disposes the client. Ensure ensureConnected checks disposal immediately before
publishing CONNECTED so pre-disconnect attempts cannot restore the client, while
preserving generic errors for unexpected socket loss.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/terminal-host/client.ts">
<violation number="1" location="apps/desktop/src/main/lib/terminal-host/client.ts:819">
P2: resetConnectionState only rejects entries in pendingRequests with the new typed TerminalHostClientDisposedError. sendRequestOnStream and the connection waiters in ensureConnected use separate promises that aren't covered here, so if disconnect() fires during stream auth or connection setup, callers can still see generic timeout/connection-failure errors that bypass the TerminalHostClientDisposedError mapping in createOrAttach and continue to surface as 500s.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Main as Desktop App (Main)
participant Router as Terminal TRPC Router
participant THC as TerminalHostClient
participant Sentry as Sentry Middleware
Note over Router, THC: In-flight Request Scenario
Router->>THC: terminal.createOrAttach()
THC->>THC: Store in pendingRequests
alt Intentional Shutdown (e.g. App Quit)
Main->>THC: disconnect() / dispose()
THC->>THC: CHANGED: resetConnectionState() with TerminalHostClientDisposedError
THC-->>Router: Reject pending request (DisposedError)
Router->>Router: NEW: Catch TerminalHostClientDisposedError
Router-->>Sentry: NEW: Throw TRPCError (PRECONDITION_FAILED)
Note over Sentry: Non-500 response: No Sentry alert triggered
else Unexpected Connection Loss
THC->>THC: Socket drops (handleDisconnect)
THC->>THC: resetConnectionState() with default Error("Connection lost")
THC-->>Router: Reject pending request (Generic Error)
Router-->>Sentry: Throw TRPCError (INTERNAL_SERVER_ERROR)
Note over Sentry: 500 response: Sentry captures & alerts
end
Sentry-->>Router: Response complete
Router-->>Main: Return error to UI/Client
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| for (const [id, pending] of this.pendingRequests.entries()) { | ||
| clearTimeout(pending.timeoutId); | ||
| pending.reject(new Error("Connection lost")); | ||
| pending.reject(pendingRequestError ?? new Error("Connection lost")); |
There was a problem hiding this comment.
P2: resetConnectionState only rejects entries in pendingRequests with the new typed TerminalHostClientDisposedError. sendRequestOnStream and the connection waiters in ensureConnected use separate promises that aren't covered here, so if disconnect() fires during stream auth or connection setup, callers can still see generic timeout/connection-failure errors that bypass the TerminalHostClientDisposedError mapping in createOrAttach and continue to surface as 500s.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/terminal-host/client.ts, line 819:
<comment>resetConnectionState only rejects entries in pendingRequests with the new typed TerminalHostClientDisposedError. sendRequestOnStream and the connection waiters in ensureConnected use separate promises that aren't covered here, so if disconnect() fires during stream auth or connection setup, callers can still see generic timeout/connection-failure errors that bypass the TerminalHostClientDisposedError mapping in createOrAttach and continue to surface as 500s.</comment>
<file context>
@@ -802,7 +816,7 @@ export class TerminalHostClient extends EventEmitter {
for (const [id, pending] of this.pendingRequests.entries()) {
clearTimeout(pending.timeoutId);
- pending.reject(new Error("Connection lost"));
+ pending.reject(pendingRequestError ?? new Error("Connection lost"));
this.pendingRequests.delete(id);
}
</file context>
|
Addressed the greptile/CodeRabbit finding that only |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`:
- Line 237: The terminal handlers around terminalProcedure must preserve
TerminalHostClientDisposedError from terminal.write(input) and terminal.kill()
operations. Re-throw that exact typed error before generic write error handling,
and inspect Promise.allSettled results in the bulk kill handlers so disposal
rejections propagate as typed failures instead of being swallowed.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a3762c4-188c-44ec-b98f-b77a20a61c3e
📒 Files selected for processing (1)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
| }), | ||
|
|
||
| write: publicProcedure | ||
| write: terminalProcedure |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
--glob '*.ts' \
--glob '*.tsx' \
'TerminalHostClientDisposedError|terminal\.write|terminal\.kill|killDaemonSessionsForWorkspace|Promise\.allSettled' \
apps/desktop/srcRepository: superset-sh/superset
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- terminal router slice ---'
sed -n '1,110p;220,275p;300,430p' apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
echo '--- terminal host client disposal slice ---'
sed -n '150,162p;1630,1700p' apps/desktop/src/main/lib/terminal-host/client.ts
echo '--- methods mentioning pendingRequestError or request error setup ---'
rg -n -C 4 'pendingRequestError|uniqueRequestId|requestError|call|send|once|emit|request|write|kill' apps/desktop/src/main/lib/terminal-host/client.ts | head -n 260Repository: superset-sh/superset
Length of output: 19093
Preserve typed terminal disposal rejections in these handlers.
terminalProcedure only preserves TerminalHostClientDisposedError from next(). terminal.write(input) can reject synchronously from the client’s pending request cleanup, so the generic try/catch hides the original error. Re-throw that exact error before the generic write handling. The bulk kill handlers also swallow rejected terminal.kill() calls in Promise.allSettled instead of treating disposal as a typed failure.
🤖 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 `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts` at line 237, The
terminal handlers around terminalProcedure must preserve
TerminalHostClientDisposedError from terminal.write(input) and terminal.kill()
operations. Re-throw that exact typed error before generic write error handling,
and inspect Promise.allSettled results in the bulk kill handlers so disposal
rejections propagate as typed failures instead of being swallowed.
|
Follow-up on the latest CodeRabbit note: the Skipping the bulk-kill half: |
Summary
Refs DESKTOP-HD: a normal app quit racing an in-flight terminal request was being reported as a 500.
During quit,
runQuitCleanup→disposeTerminalHostClient→dispose→disconnect→resetConnectionStaterejected every pending terminal-host request with a genericError("Connection lost"). An in-flightterminal.createOrAttachmutation then surfaced that asINTERNAL_SERVER_ERROR, which the Sentry tRPC middleware captures.Changes
Per the error-handling contract (#6164 — typed causes at throw sites, never a Sentry filter):
TerminalHostClient.disconnect()(the intentional teardown path:dispose/shutdown) now rejects pending requests with a typedTerminalHostClientDisposedErrorinstead of the generic error.terminal.createOrAttachcatch translates that error toPRECONDITION_FAILEDwithcause: { kind: "TERMINAL_HOST_CLIENT_DISPOSED" }, so it never reaches the Sentry middleware as a 500.handleDisconnecton socket drop, connect-failure resets) keeps the genericError("Connection lost")rejection and still reports.Verification
bunx biome checkclean on both changed filescd apps/desktop && bun run typecheckpasseshttps://claude.ai/code/session_017sk3KSZ5NLrpTedKWRX2k8
Summary by cubic
Prevent normal app quit from being reported as a 500 when a terminal request is in flight. Quit-time terminal-host teardown now uses a typed error and maps to a non-500 tRPC code.
TerminalHostClientDisposedError; used on explicitdisconnect/disposeto reject pending requests.terminal.createOrAttach, translate this toPRECONDITION_FAILEDwithcause: { kind: "TERMINAL_HOST_CLIENT_DISPOSED" }; unexpected socket drops keep the generic "Connection lost" and still report.Written for commit 8ed37ff. Summary will update on new commits.
Summary by CodeRabbit