Repository navigation
feat(desktop): surface why a terminal can't connect + emit failure telemetry - #5546
Merged
Merged
Conversation
…lemetry
The terminal pane showed a bare "disconnected" with no cause, because the
browser WebSocket API hides the upgrade's HTTP status (a relay 502/503 only
surfaces as close code 1006). Two changes:
1. Classify the failure using the /_whoowns preflight (which the client already
fires) — 503 = host offline, 401/403 = unauthorized, 200-but-WS-drops =
relay routing issue (the cross-region fingerprint), null = relay
unreachable. The give-up log now says which.
2. Emit a `terminal_connect_failed` PostHog event on give-up with
{endpoint, close_code, preflight_status, tunnel_region, reconnect_attempts,
category} so this failure mode is queryable instead of silent.
primeRelayAffinity now returns the probe {status, region} (was void; both
callers ignore the value). Pure classifier + probe parsing are unit-tested
(mutation-checked).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
📝 WalkthroughWalkthroughThis PR adds structured relay affinity probing, maps probe outcomes to terminal failure categories, and uses those classifications when relay-routed terminal connections exhaust reconnect attempts. ChangesRelay affinity probe and terminal diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalTransport
participant primeRelayAffinity
participant RelayWhoowns
participant classifyTerminalFailure
participant PostHog
TerminalTransport->>primeRelayAffinity: primeRelayAffinity(wsUrl)
primeRelayAffinity->>RelayWhoowns: GET /_whoowns
RelayWhoowns-->>primeRelayAffinity: status, region
primeRelayAffinity-->>TerminalTransport: RelayAffinityProbe or null
TerminalTransport->>classifyTerminalFailure: classifyTerminalFailure(_lastProbe, isRelayHostUrl)
classifyTerminalFailure-->>TerminalTransport: category + message
TerminalTransport->>PostHog: capture(terminal_connect_failed, ...)
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 17770b9 on Jul 9, 2026 7:50pm UTC. |
Contributor
Greptile SummaryThis PR surfaces actionable failure reasons when a relay-routed terminal can't connect, and emits
Confidence Score: 4/5Safe to merge. The probe already fired on every connect; storing and reading its result only in the give-up branch is a pure additive change with no impact on the reconnect lifecycle. The behavioral change is additive — the probe already fired on every connect and the result was thrown away; now it is stored and read only in the give-up branch. Both flagged items are minor quality improvements: 502 falling to the unknown catchall produces a slightly less helpful user message, and the relay parameter leak is harmless in practice. No auth, data-integrity, or crash risk is introduced. terminalConnectionDiagnostics.ts — the 502 gap and the probe-URL parameter suggestion are both there; otherwise all files are straightforward.
|
| Filename | Overview |
|---|---|
| packages/workspace-client/src/lib/primeRelayAffinity.ts | Refactored to return RelayAffinityProbe |
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Adds _lastProbe field to transport, wires probe result through connect → close handler give-up path, and fires PostHog telemetry on max-reconnect give-up. Logic for when _lastProbe is set vs. stale is correct. One minor note: actualUrl (which may carry ?replay=0) is forwarded to primeRelayAffinity, causing the probe to include an irrelevant parameter. |
| apps/desktop/src/renderer/lib/terminal/terminalConnectionDiagnostics.ts | New pure classifier mapping probe status to failure category and user message. Handles null/503/401/403/200 correctly; 502 falls to the unknown catchall with a generic message, which may be worth handling explicitly. |
| apps/desktop/src/renderer/lib/terminal/terminalConnectionDiagnostics.test.ts | Good unit tests covering all classifier branches including the local-terminal short-circuit, null probe, 503, 401/403, 200+stream-blocked (with region assertion), and unknown fallback. |
| packages/workspace-client/src/lib/primeRelayAffinity.test.ts | New tests validate non-/hosts early return (no fetch), token preservation, status+region parsing on 200, 503 returned as-is, and null on network error. Comprehensive coverage of the refactored function. |
| packages/workspace-client/src/index.ts | Exports the new RelayAffinityProbe type alongside the existing primeRelayAffinity export. Straightforward additive change. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as Terminal Pane
participant T as Transport
participant P as primeRelayAffinity
participant R as Relay (_whoowns)
participant WS as WebSocket
UI->>T: connect(wsUrl)
T->>T: "_lastProbe = null"
alt isRelayHostUrl
T->>P: primeRelayAffinity(actualUrl)
P->>R: "GET /hosts/id/_whoowns?token=..."
alt 200 (host online)
R-->>P: "{ok: true, region: iad}"
P-->>T: "{status:200, region:iad}"
else 503 (host offline)
R-->>P: "{error: Host not connected}"
P-->>T: "{status:503, region:null}"
else network error / timeout
R-->>P: (error)
P-->>T: null
end
T->>T: "_lastProbe = probe"
T->>WS: new WebSocket(actualUrl)
else local terminal
T->>WS: new WebSocket(actualUrl)
end
WS-->>T: "close event (unexpected, code != 1000)"
T->>T: check willReconnect
alt "_reconnectAttempt < MAX_RECONNECT_ATTEMPTS"
T->>T: log warn Reconnecting...
T->>T: scheduleReconnect (backoff)
note over T: repeat up to 10x, refreshing _lastProbe each time
else "gave up (attempt >= MAX)"
T->>T: classifyTerminalFailure(_lastProbe, isHostUrl)
note over T: 503->host-offline, 401/403->unauthorized, 200+drop->stream-blocked, null->relay-unreachable, other->unknown
T->>T: log error with diagnosis.message
T->>T: posthog.capture terminal_connect_failed
T->>UI: show error in pane log
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as Terminal Pane
participant T as Transport
participant P as primeRelayAffinity
participant R as Relay (_whoowns)
participant WS as WebSocket
UI->>T: connect(wsUrl)
T->>T: "_lastProbe = null"
alt isRelayHostUrl
T->>P: primeRelayAffinity(actualUrl)
P->>R: "GET /hosts/id/_whoowns?token=..."
alt 200 (host online)
R-->>P: "{ok: true, region: iad}"
P-->>T: "{status:200, region:iad}"
else 503 (host offline)
R-->>P: "{error: Host not connected}"
P-->>T: "{status:503, region:null}"
else network error / timeout
R-->>P: (error)
P-->>T: null
end
T->>T: "_lastProbe = probe"
T->>WS: new WebSocket(actualUrl)
else local terminal
T->>WS: new WebSocket(actualUrl)
end
WS-->>T: "close event (unexpected, code != 1000)"
T->>T: check willReconnect
alt "_reconnectAttempt < MAX_RECONNECT_ATTEMPTS"
T->>T: log warn Reconnecting...
T->>T: scheduleReconnect (backoff)
note over T: repeat up to 10x, refreshing _lastProbe each time
else "gave up (attempt >= MAX)"
T->>T: classifyTerminalFailure(_lastProbe, isHostUrl)
note over T: 503->host-offline, 401/403->unauthorized, 200+drop->stream-blocked, null->relay-unreachable, other->unknown
T->>T: log error with diagnosis.message
T->>T: posthog.capture terminal_connect_failed
T->>UI: show error in pane log
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/lib/terminal/terminalConnectionDiagnostics.ts:44-49
The classifier handles 503 (host offline) but drops 502 into the `unknown` catchall with a generic "relay status 502" message. A relay 502 means it received a bad/no response from the upstream host service — semantically close to 503 and actionable. Users hitting a temporary host-service crash would see a cryptic status code rather than a helpful hint.
```suggestion
if (probe.status === 502 || probe.status === 503) {
return {
category: "host-offline",
message: "This host is offline (not connected to the relay).",
};
}
```
### Issue 2 of 2
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:425-430
The preflight probe is fired against `actualUrl`, which may include `?replay=0` when the terminal has previously received bytes. `primeRelayAffinity` preserves all search params, so the `_whoowns` request becomes `/_whoowns?replay=0&token=...`. The relay likely ignores `replay`, but the probe should be authenticating with only the token — passing the WS-specific hint to an HTTP probe endpoint is unintentional.
```suggestion
if (isRelayHostUrl(actualUrl)) {
transport._lastProbe = null;
void primeRelayAffinity(wsUrl).then((probe) => {
transport._lastProbe = probe;
openSocket();
});
```
Reviews (1): Last reviewed commit: "feat(desktop): surface why a terminal ca..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+44
to
+49
| if (probe.status === 503) { | ||
| return { | ||
| category: "host-offline", | ||
| message: "This host is offline (not connected to the relay).", | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
unknown catchall with a generic "relay status 502" message. A relay 502 means it received a bad/no response from the upstream host service — semantically close to 503 and actionable. Users hitting a temporary host-service crash would see a cryptic status code rather than a helpful hint.
Suggested change
| if (probe.status === 503) { | |
| return { | |
| category: "host-offline", | |
| message: "This host is offline (not connected to the relay).", | |
| }; | |
| } | |
| if (probe.status === 502 || probe.status === 503) { | |
| return { | |
| category: "host-offline", | |
| message: "This host is offline (not connected to the relay).", | |
| }; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminalConnectionDiagnostics.ts
Line: 44-49
Comment:
The classifier handles 503 (host offline) but drops 502 into the `unknown` catchall with a generic "relay status 502" message. A relay 502 means it received a bad/no response from the upstream host service — semantically close to 503 and actionable. Users hitting a temporary host-service crash would see a cryptic status code rather than a helpful hint.
```suggestion
if (probe.status === 502 || probe.status === 503) {
return {
category: "host-offline",
message: "This host is offline (not connected to the relay).",
};
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+425
to
+430
| if (isRelayHostUrl(actualUrl)) { | ||
| transport._lastProbe = null; | ||
| void primeRelayAffinity(actualUrl).then((probe) => { | ||
| transport._lastProbe = probe; | ||
| openSocket(); | ||
| }); |
Contributor
There was a problem hiding this comment.
actualUrl, which may include ?replay=0 when the terminal has previously received bytes. primeRelayAffinity preserves all search params, so the _whoowns request becomes /_whoowns?replay=0&token=.... The relay likely ignores replay, but the probe should be authenticating with only the token — passing the WS-specific hint to an HTTP probe endpoint is unintentional.
Suggested change
| if (isRelayHostUrl(actualUrl)) { | |
| transport._lastProbe = null; | |
| void primeRelayAffinity(actualUrl).then((probe) => { | |
| transport._lastProbe = probe; | |
| openSocket(); | |
| }); | |
| if (isRelayHostUrl(actualUrl)) { | |
| transport._lastProbe = null; | |
| void primeRelayAffinity(wsUrl).then((probe) => { | |
| transport._lastProbe = probe; | |
| openSocket(); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Line: 425-430
Comment:
The preflight probe is fired against `actualUrl`, which may include `?replay=0` when the terminal has previously received bytes. `primeRelayAffinity` preserves all search params, so the `_whoowns` request becomes `/_whoowns?replay=0&token=...`. The relay likely ignores `replay`, but the probe should be authenticating with only the token — passing the WS-specific hint to an HTTP probe endpoint is unintentional.
```suggestion
if (isRelayHostUrl(actualUrl)) {
transport._lastProbe = null;
void primeRelayAffinity(wsUrl).then((probe) => {
transport._lastProbe = probe;
openSocket();
});
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts (1)
425-432: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden
_lastProbemanagement against stale data and unexpected rejections.Three related gaps in the preflight wiring:
Stale probe for non-relay URLs:
_lastProbeis only reset inside theisRelayHostUrlbranch. When switching from a relay URL to a local URL, the old probe persists. The close handler's give-up path correctly classifies asunknown, but the PostHog event still sendspreflight_statusandtunnel_regionfrom the stale probe — misleading telemetry.Race on rapid URL switch: If
connect()is called with a new URL while a previous probe is in flight, the stale probe's.then()callback unconditionally setstransport._lastProbe, overwriting the correct probe for the current URL.No rejection handler:
primeRelayAffinityis designed to never reject, but if it does,openSocket()is never called and the transport is stuck in"connecting"indefinitely.♻️ Proposed fix: reset unconditionally, guard probe assignment, add rejection handler
+ transport._lastProbe = null; if (isRelayHostUrl(actualUrl)) { - transport._lastProbe = null; - void primeRelayAffinity(actualUrl).then((probe) => { - transport._lastProbe = probe; - openSocket(); - }); + void primeRelayAffinity(actualUrl).then( + (probe) => { + if (transport.currentUrl === wsUrl) { + transport._lastProbe = probe; + } + openSocket(); + }, + () => openSocket(), + ); } else { openSocket(); }🤖 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/renderer/lib/terminal/terminal-ws-transport.ts` around lines 425 - 432, The `_lastProbe` handling in `connect()`/`openSocket()` is vulnerable to stale relay data, races, and hung connections. Reset `transport._lastProbe` unconditionally before any URL-specific branch, and in the `primeRelayAffinity(actualUrl)` promise chain guard the `.then()` assignment so it only applies if the probe still matches the current connection attempt. Also add a rejection handler that still calls `openSocket()` so a failed `primeRelayAffinity` cannot leave the transport stuck in `"connecting"`.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts`:
- Around line 425-432: The `_lastProbe` handling in `connect()`/`openSocket()`
is vulnerable to stale relay data, races, and hung connections. Reset
`transport._lastProbe` unconditionally before any URL-specific branch, and in
the `primeRelayAffinity(actualUrl)` promise chain guard the `.then()` assignment
so it only applies if the probe still matches the current connection attempt.
Also add a rejection handler that still calls `openSocket()` so a failed
`primeRelayAffinity` cannot leave the transport stuck in `"connecting"`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c3dd4cc-a6fb-4135-8079-cb83940e2871
📒 Files selected for processing (6)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tsapps/desktop/src/renderer/lib/terminal/terminalConnectionDiagnostics.test.tsapps/desktop/src/renderer/lib/terminal/terminalConnectionDiagnostics.tspackages/workspace-client/src/index.tspackages/workspace-client/src/lib/primeRelayAffinity.test.tspackages/workspace-client/src/lib/primeRelayAffinity.ts
The 'WebSocket API hides the upgrade HTTP status' rationale was restated in four places; keep it where the signal originates (primeRelayAffinity) and make the transport comments terse references.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found and verified against the latest diff
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/renderer/lib/terminal/terminal-ws-transport.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:423">
P2: Failure diagnosis can be attributed to the wrong host when connect calls race between URLs. The preflight callback stores `_lastProbe` unconditionally before the URL/state guard, so stale probe data survives even when that socket open is skipped.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+423
to
+424
| transport._lastProbe = probe; | ||
| openSocket(); |
Contributor
There was a problem hiding this comment.
P2: Failure diagnosis can be attributed to the wrong host when connect calls race between URLs. The preflight callback stores _lastProbe unconditionally before the URL/state guard, so stale probe data survives even when that socket open is skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts, line 428:
<comment>Failure diagnosis can be attributed to the wrong host when connect calls race between URLs. The preflight callback stores `_lastProbe` unconditionally before the URL/state guard, so stale probe data survives even when that socket open is skipped.</comment>
<file context>
@@ -395,14 +420,14 @@ export function connect(
+ if (isRelayHostUrl(actualUrl)) {
+ transport._lastProbe = null;
+ void primeRelayAffinity(actualUrl).then((probe) => {
+ transport._lastProbe = probe;
+ openSocket();
+ });
</file context>
Suggested change
| transport._lastProbe = probe; | |
| openSocket(); | |
| if ( | |
| transport.currentUrl !== wsUrl || | |
| transport.connectionState !== "connecting" | |
| ) { | |
| return; | |
| } | |
| transport._lastProbe = probe; | |
| openSocket(); |
…2/504 - Preflight the `_whoowns` probe against `wsUrl` so the WS-only `replay` hint isn't sent to the HTTP probe endpoint (greptile). - Map a 502/504 preflight status to a 'temporary relay failure' message rather than the raw status, without over-claiming the host is offline.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Kitenite
added a commit
that referenced
this pull request
Jul 9, 2026
…n:test (#5553) #5546 added packages/workspace-client/src/lib/primeRelayAffinity.test.ts (the first bun:test in this package), but the tsconfig had no bun-types, so `tsc --noEmit` failed with 'Cannot find module bun:test' and turned main's Typecheck red. Add bun-types (tsconfig types + devDep, matching apps/relay) and cast the test's fetch mocks through unknown for Bun's stricter fetch type.
This was referenced Jul 10, 2026
Merged
This was referenced Jul 12, 2026
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.
Why
When a terminal pane can't connect, it just shows "disconnected" with no cause — which is exactly what made the recent cross-region relay bug hard to spot (control plane worked, only the terminal WS 502'd). The root difficulty: the browser WebSocket API hides the upgrade's HTTP status — a relay 502/503 only surfaces as close code
1006, so the client can't see why.What
Both changes lean on the
_whoownspreflight the client already fires before every terminal WS (its response status was thrown away until now).1. Classify the failure (surface the why). On give-up, the pane log now says the cause instead of a generic close:
2. Emit
terminal_connect_failedtelemetry on give-up (PostHog), so this failure mode is queryable/alertable instead of silent:{ endpoint, close_code, close_reason, preflight_status, tunnel_region, reconnect_attempts, category }(endpoint has the token stripped).Notes
primeRelayAffinitynow returns{ status, region } | null(wasvoid); both existing callers ignore the value, so it's backward-compatible.Follow-on ideas from the same discussion (not in this PR): a bounded error state with Retry/Copy-diagnostics buttons, and a cross-region synthetic check in the relay (today's
synthetic.tsonly exercises same-region).Summary by cubic
Surface why a terminal can’t connect and emit telemetry on final failure. We classify errors using the relay
_whoownspreflight (now probed with thewsUrl), show a clear message in the pane, and send a PostHog event for alerting.New Features
terminal_connect_failedon give-up with endpoint, close_code, close_reason, preflight_status, tunnel_region, reconnect_attempts, category./hosts/...); local terminals unchanged.Refactors
@superset/workspace-clientprimeRelayAffinitynow returns{ status, region } | null(and exportsRelayAffinityProbe) and is used to classify failures; added unit tests for probe parsing and classification._whoownswith the originalwsUrl(notactualUrl); deduplicated diagnostics comments and centralized rationale inprimeRelayAffinity.Written for commit 17770b9. Summary will update on new commits.
Summary by CodeRabbit