Repository navigation
fix(desktop): re-mint terminal JWT on every reconnect - #5408
Open
saddlepaddle wants to merge 1 commit into
Open
saddlepaddle wants to merge 1 commit into
saddlepaddle wants to merge 1 commit into
Conversation
Terminal WebSocket attaches bake a 1h JWT into the connection URL at render time; every reconnect path (reconnectNow, scheduleReconnect, handleResume) replayed that same URL verbatim, so once the token passed its 1h exp the relay rejected the attach with 401 while all other host-service calls (which mint a fresh token per request) kept working. The aggressive sleep/wake + focus/visibility auto-reconnect added in #5135 turned this latent staleness into a fleet-wide ~71% terminal-401 storm. Store a token-less base URL in the transport and append a freshly-minted token at each dial (socket open and the affinity pre-flight) via a getWsToken provider threaded through the registry, mirroring how web's buildUrl() and desktop tRPC headers already refresh per request.
Contributor
📝 WalkthroughWalkthroughTerminal WebSocket connect/reconnect now supports an optional ChangesToken-aware terminal reconnection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalPane
participant TerminalRuntimeRegistry
participant TerminalTransport
participant WebSocket
TerminalPane->>TerminalRuntimeRegistry: connect/reconnect(terminalId, wsUrl, getWsTokenRef.current)
TerminalRuntimeRegistry->>TerminalRuntimeRegistry: stripUrlToken(wsUrl) vs currentUrl
alt URL unchanged (token-only diff)
TerminalRuntimeRegistry-->>TerminalPane: reuse existing connection
else URL changed
TerminalRuntimeRegistry->>TerminalTransport: connect(transport, terminal, wsUrl, getToken)
TerminalTransport->>TerminalTransport: buildDialUrl() with fresh token
TerminalTransport->>WebSocket: open(dialUrl)
end
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 |
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 2a35f66 on Jul 1, 2026 6:08pm UTC. |
Contributor
Greptile SummaryFixes a fleet-wide 401 storm on terminal WebSocket reconnects by stopping the relay-rejecting pattern of replaying a stale JWT baked into
Confidence Score: 4/5The fix is sound and directly addresses the root cause; the main risk is a subtle token-provider lifetime issue in TerminalPane that could resurface stale-token behavior under workspace-client rebuilds. The transport-layer changes are clean and correct — TerminalPane.tsx — the
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Core fix: strips token from currentUrl, introduces _getToken provider on the transport, and mints fresh JWTs via buildDialUrl() at every dial including the pre-flight affinity request. disconnect() and disposeTransport() omit clearing _getToken, leaving a dangling closure reference after teardown. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts | Threads the optional getToken provider through connect()/reconnect() and updates the idempotency guard to compare against the stripped URL so a token-only change doesn't tear down a live shell. Changes are minimal and correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | Wires getWsToken from useWorkspaceClient() into both connect() and reconnect() via a ref. Passes getWsTokenRef.current (a function snapshot) instead of a stable wrapper that delegates through the ref, so transport._getToken can become stale if the workspace client is rebuilt without a URL change. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant TP as TerminalPane
participant TRR as TerminalRuntimeRegistry
participant TWT as TerminalWsTransport
participant Relay as Relay/Edge
Note over TP: Mount effect
TP->>TRR: connect(terminalId, wsUrl, instanceId, getWsTokenRef.current)
TRR->>TWT: connect(transport, terminal, wsUrl, getToken)
TWT->>TWT: stripUrlToken(wsUrl) → baseUrl
TWT->>TWT: "transport._getToken = getToken"
TWT->>TWT: "transport.currentUrl = baseUrl"
TWT->>TWT: buildDialUrl() → appends fresh JWT to baseUrl
TWT->>Relay: primeRelayAffinity(buildDialUrl())
Relay-->>TWT: affinity set
TWT->>Relay: new WebSocket(buildDialUrl()) ← fresh token
Note over TWT: 1h later, JWT expires...
Note over TWT: Sleep/wake triggers reconnectNow()
TWT->>TWT: connect(transport, terminal, transport.currentUrl)
TWT->>TWT: "getToken=undefined → _getToken unchanged"
TWT->>TWT: buildDialUrl() → transport._getToken() → fresh JWT
TWT->>Relay: new WebSocket(buildDialUrl()) ← NEW fresh token
Note over TP: baseWebsocketUrl changes
TP->>TRR: reconnect(terminalId, wsUrl, instanceId, getWsTokenRef.current)
TRR->>TRR: stripUrlToken(wsUrl) vs transport.currentUrl
alt URL unchanged
TRR-->>TP: return (no teardown)
else URL changed
TRR->>TWT: connect(transport, terminal, wsUrl, getToken)
TWT->>TWT: "transport._getToken = getToken (updated)"
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 TP as TerminalPane
participant TRR as TerminalRuntimeRegistry
participant TWT as TerminalWsTransport
participant Relay as Relay/Edge
Note over TP: Mount effect
TP->>TRR: connect(terminalId, wsUrl, instanceId, getWsTokenRef.current)
TRR->>TWT: connect(transport, terminal, wsUrl, getToken)
TWT->>TWT: stripUrlToken(wsUrl) → baseUrl
TWT->>TWT: "transport._getToken = getToken"
TWT->>TWT: "transport.currentUrl = baseUrl"
TWT->>TWT: buildDialUrl() → appends fresh JWT to baseUrl
TWT->>Relay: primeRelayAffinity(buildDialUrl())
Relay-->>TWT: affinity set
TWT->>Relay: new WebSocket(buildDialUrl()) ← fresh token
Note over TWT: 1h later, JWT expires...
Note over TWT: Sleep/wake triggers reconnectNow()
TWT->>TWT: connect(transport, terminal, transport.currentUrl)
TWT->>TWT: "getToken=undefined → _getToken unchanged"
TWT->>TWT: buildDialUrl() → transport._getToken() → fresh JWT
TWT->>Relay: new WebSocket(buildDialUrl()) ← NEW fresh token
Note over TP: baseWebsocketUrl changes
TP->>TRR: reconnect(terminalId, wsUrl, instanceId, getWsTokenRef.current)
TRR->>TRR: stripUrlToken(wsUrl) vs transport.currentUrl
alt URL unchanged
TRR-->>TP: return (no teardown)
else URL changed
TRR->>TWT: connect(transport, terminal, wsUrl, getToken)
TWT->>TWT: "transport._getToken = getToken (updated)"
end
Comments Outside Diff (2)
-
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts, line 554-561 (link)disconnect()zeros outcurrentUrland_terminalbut leaves_getTokenset. A transport that has been disconnected and then passed to a freshconnect()call without agetTokenargument will silently reuse the old provider. Clearing it on disconnect makes the field's semantics consistent with the rest of the teardown and avoids an unintended closure reference keeping the old workspace client alive.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: 554-561 Comment: `disconnect()` zeros out `currentUrl` and `_terminal` but leaves `_getToken` set. A transport that has been disconnected and then passed to a fresh `connect()` call without a `getToken` argument will silently reuse the old provider. Clearing it on disconnect makes the field's semantics consistent with the rest of the teardown and avoids an unintended closure reference keeping the old workspace client alive. How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts, line 594-602 (link)disposeTransport()clears every internal field (_terminal,currentUrl, all timers, all listener sets) but not_getToken. This leaves a closure reference alive until the transport object is GC'd. Adding anullassignment keeps the teardown symmetrical withdisconnect()and makes the dispose semantics complete.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: 594-602 Comment: `disposeTransport()` clears every internal field (`_terminal`, `currentUrl`, all timers, all listener sets) but not `_getToken`. This leaves a closure reference alive until the transport object is GC'd. Adding a `null` assignment keeps the teardown symmetrical with `disconnect()` and makes the dispose semantics complete. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:155-156
**Stale `getToken` provider after workspace-client rebuild**
`getWsTokenRef.current` is passed by value at call time, so `transport._getToken` captures the `getWsToken` function instance from the mount render. If `useWorkspaceClient()` ever returns a new `getWsToken` reference that closes over refreshed credentials (e.g., after a workspace-client rebuild without a URL change), auto-reconnects (`reconnectNow`, `scheduleReconnect`) will still call the old function. The reconnect effect only updates `_getToken` when `stripUrlToken(wsUrl)` changes — a pure credential-context change with a stable URL leaves `_getToken` pointing at the stale provider, which is exactly the class of bug this PR is fixing.
Passing a stable wrapper that delegates through the ref prevents this — use `useCallback(() => getWsTokenRef.current(), [])` and pass that stable function to both `connect()` and `reconnect()`. `transport._getToken` then stores one stable function for the transport's lifetime, and every dial calls through to whatever `getWsToken` is current at reconnect time.
### Issue 2 of 3
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:554-561
`disconnect()` zeros out `currentUrl` and `_terminal` but leaves `_getToken` set. A transport that has been disconnected and then passed to a fresh `connect()` call without a `getToken` argument will silently reuse the old provider. Clearing it on disconnect makes the field's semantics consistent with the rest of the teardown and avoids an unintended closure reference keeping the old workspace client alive.
```suggestion
transport.currentUrl = null;
transport._terminal = null;
transport._getToken = null;
transport._reconnectAttempt = 0;
setTerminalTitle(transport, undefined);
setConnectionState(transport, "disconnected");
transport.onDataDisposable?.dispose();
transport.onDataDisposable = null;
}
```
### Issue 3 of 3
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:594-602
`disposeTransport()` clears every internal field (`_terminal`, `currentUrl`, all timers, all listener sets) but not `_getToken`. This leaves a closure reference alive until the transport object is GC'd. Adding a `null` assignment keeps the teardown symmetrical with `disconnect()` and makes the dispose semantics complete.
```suggestion
transport._writeCoalescer?.dispose();
transport._writeCoalescer = null;
transport.currentUrl = null;
transport._terminal = null;
transport._getToken = null;
transport._reconnectAttempt = 0;
setTerminalTitle(transport, undefined);
transport.onDataDisposable?.dispose();
transport.onDataDisposable = null;
transport.stateListeners.clear();
```
Reviews (1): Last reviewed commit: "fix(desktop): re-mint terminal JWT on ev..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+155
to
156
| getWsTokenRef.current, | ||
| ); |
Contributor
There was a problem hiding this comment.
getToken provider after workspace-client rebuild
getWsTokenRef.current is passed by value at call time, so transport._getToken captures the getWsToken function instance from the mount render. If useWorkspaceClient() ever returns a new getWsToken reference that closes over refreshed credentials (e.g., after a workspace-client rebuild without a URL change), auto-reconnects (reconnectNow, scheduleReconnect) will still call the old function. The reconnect effect only updates _getToken when stripUrlToken(wsUrl) changes — a pure credential-context change with a stable URL leaves _getToken pointing at the stale provider, which is exactly the class of bug this PR is fixing.
Passing a stable wrapper that delegates through the ref prevents this — use useCallback(() => getWsTokenRef.current(), []) and pass that stable function to both connect() and reconnect(). transport._getToken then stores one stable function for the transport's lifetime, and every dial calls through to whatever getWsToken is current at reconnect time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Line: 155-156
Comment:
**Stale `getToken` provider after workspace-client rebuild**
`getWsTokenRef.current` is passed by value at call time, so `transport._getToken` captures the `getWsToken` function instance from the mount render. If `useWorkspaceClient()` ever returns a new `getWsToken` reference that closes over refreshed credentials (e.g., after a workspace-client rebuild without a URL change), auto-reconnects (`reconnectNow`, `scheduleReconnect`) will still call the old function. The reconnect effect only updates `_getToken` when `stripUrlToken(wsUrl)` changes — a pure credential-context change with a stable URL leaves `_getToken` pointing at the stale provider, which is exactly the class of bug this PR is fixing.
Passing a stable wrapper that delegates through the ref prevents this — use `useCallback(() => getWsTokenRef.current(), [])` and pass that stable function to both `connect()` and `reconnect()`. `transport._getToken` then stores one stable function for the transport's lifetime, and every dial calls through to whatever `getWsToken` is current at reconnect time.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/renderer/lib/terminal/terminal-ws-transport.ts`:
- Around line 433-434: Guard token minting on the pre-flight relay path by
moving the buildDialUrl() call out of the eager primeRelayAffinity(...) argument
in terminal-ws-transport.ts; if _getToken can throw, it should be handled
through the existing openSocket flow so /hosts/ connections still reach the
reconnect/error path instead of staying stuck in "connecting". Update the
needsPreFlight branch to defer URL construction until openSocket runs, and keep
the logic centered around primeRelayAffinity and openSocket so the pre-flight
setup still works without minting the token too early.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 89-93: The token provider in TerminalPane is being captured as the
current function instance, which can become stale when the workspace client
changes. In TerminalPane’s useRef/useWorkspaceClient setup (and the same
reconnect logic in the other affected spots), pass a stable wrapper that reads
getWsTokenRef.current at dial time instead of storing getWsToken directly. This
ensures reconnects always use the latest token provider even when the base URL
stays the same.
🪄 Autofix (Beta)
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
Run ID: 8da40048-fdb7-49fe-beec-8937e5cd0496
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Comment on lines
433
to
+434
| if (needsPreFlight) { | ||
| void primeRelayAffinity(actualUrl).then(openSocket); | ||
| void primeRelayAffinity(buildDialUrl()).then(openSocket); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard token minting on the pre-flight path.
buildDialUrl() is protected inside openSocket, but Line 434 evaluates it before primeRelayAffinity(...). If _getToken throws, /hosts/ connections can remain stuck in "connecting" without reaching the logged reconnect path.
Proposed fix
if (needsPreFlight) {
- void primeRelayAffinity(buildDialUrl()).then(openSocket);
+ void Promise.resolve()
+ .then(() => primeRelayAffinity(buildDialUrl()))
+ .catch(() => undefined)
+ .then(openSocket);
} else {
openSocket();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (needsPreFlight) { | |
| void primeRelayAffinity(actualUrl).then(openSocket); | |
| void primeRelayAffinity(buildDialUrl()).then(openSocket); | |
| if (needsPreFlight) { | |
| void Promise.resolve() | |
| .then(() => primeRelayAffinity(buildDialUrl())) | |
| .catch(() => undefined) | |
| .then(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
433 - 434, Guard token minting on the pre-flight relay path by moving the
buildDialUrl() call out of the eager primeRelayAffinity(...) argument in
terminal-ws-transport.ts; if _getToken can throw, it should be handled through
the existing openSocket flow so /hosts/ connections still reach the
reconnect/error path instead of staying stuck in "connecting". Update the
needsPreFlight branch to defer URL construction until openSocket runs, and keep
the logic centered around primeRelayAffinity and openSocket so the pre-flight
setup still works without minting the token too early.
Comment on lines
+89
to
+93
| // The transport re-mints the token on every (re)dial, so the JWT stays fresh | ||
| // across reconnects instead of replaying the one baked into websocketUrl. | ||
| const { getWsToken } = useWorkspaceClient(); | ||
| const getWsTokenRef = useRef(getWsToken); | ||
| getWsTokenRef.current = getWsToken; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Pass a stable token-provider wrapper, not the current function value.
getWsTokenRef.current is evaluated when the effect runs, so the transport stores that function instance. If the workspace client/provider changes without a base URL change, reconnect dials can keep using the stale token provider. Store a stable wrapper that dereferences the ref at dial time.
Proposed fix
const { getWsToken } = useWorkspaceClient();
const getWsTokenRef = useRef(getWsToken);
getWsTokenRef.current = getWsToken;
+const getFreshWsToken = useCallback(() => getWsTokenRef.current(), []); terminalRuntimeRegistry.connect(
terminalId,
websocketUrlRef.current,
terminalInstanceId,
- getWsTokenRef.current,
+ getFreshWsToken,
); terminalRuntimeRegistry.reconnect(
terminalId,
websocketUrlRef.current,
terminalInstanceId,
- getWsTokenRef.current,
+ getFreshWsToken,
);Also applies to: 151-155, 210-214
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
around lines 89 - 93, The token provider in TerminalPane is being captured as
the current function instance, which can become stale when the workspace client
changes. In TerminalPane’s useRef/useWorkspaceClient setup (and the same
reconnect logic in the other affected spots), pass a stable wrapper that reads
getWsTokenRef.current at dial time instead of storing getWsToken directly. This
ensures reconnects always use the latest token provider even when the base URL
stays the same.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 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/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:155">
P1: `getWsTokenRef.current` is evaluated at effect-run time, so the transport captures that specific function instance. If `useWorkspaceClient()` later returns a new `getWsToken` (e.g., after a workspace-client rebuild) without a base URL change, the effect won't re-run and reconnects will keep calling the stale provider — reintroducing the token-staleness issue this PR addresses.
Pass a stable wrapper that dereferences the ref at dial time:
```ts
const getFreshWsToken = useCallback(() => getWsTokenRef.current(), []);
Then pass getFreshWsToken to both connect() and reconnect().
if (needsPreFlight) {
void Promise.resolve()
.then(() => primeRelayAffinity(buildDialUrl()))
.catch(() => undefined)
.then(openSocket);
} else {
openSocket();
}
```</violation>
</file>Architecture diagram
sequenceDiagram
participant TP as TerminalPane (React)
participant RR as TerminalRuntimeRegistry
participant TR as TerminalWsTransport
participant WSC as useWorkspaceClient
participant Relay as Relay Edge
Note over TP,Relay: NEW: Fresh JWT per (re)dial flow
TP->>WSC: getWsToken()
WSC-->>TP: token provider
Note over TP: stores getWsTokenRef<br/>for stable reference
TP->>RR: connect(terminalId, wsUrl, instanceId, getWsToken)
RR->>TR: connect(transport, terminal, wsUrl, getToken)
Note over TR: stripUrlToken(wsUrl)<br/>→ baseUrl (token removed)
Note over TR: transport.currentUrl = baseUrl<br/>transport._getToken = getToken
alt First connect (needs pre-flight for /hosts/ paths)
TR->>TR: buildDialUrl() → mint fresh token
TR->>Relay: primeRelayAffinity(buildDialUrl())
Relay-->>TR: affinity established
TR->>TR: buildDialUrl() → mint another fresh token
TR->>Relay: NEW: WebSocket(buildDialUrl())
Note over TR: token from _getToken(),<br/>not from cached URL
Relay-->>TR: WebSocket open
else Direct connect (non-/hosts/ path)
TR->>Relay: NEW: WebSocket(buildDialUrl())
Relay-->>TR: WebSocket open
end
TR-->>RR: connected
RR-->>TP: ok
Note over TP,Relay: RECONNECT (e.g. sleep/wake)
TP->>RR: reconnect(terminalId, wsUrl, instanceId, getWsToken)
Note over RR: stripUrlToken(wsUrl) == currentUrl?<br/>→ Yes, only token changed
RR->>TR: connect(transport, terminal, wsUrl, getToken)
Note over TR: close old socket,<br/>cancel reconnect timer
TR->>TR: buildDialUrl() → mint fresh token
Note over TR: CHANGED: token NOT from baked URL<br/>→ fresh from _getToken()
alt Needs pre-flight
TR->>Relay: primeRelayAffinity(buildDialUrl())
Relay-->>TR: affinity established
TR->>Relay: WebSocket(buildDialUrl()) with fresh token
else Direct
TR->>Relay: WebSocket(buildDialUrl()) with fresh token
end
alt Token valid (freshly minted)
Relay-->>TR: WebSocket open (200)
else Token expired / invalid
Relay-->>TR: 401 reject
Note over TR: old behavior: replayed baked token → 401<br/>NEW: fresh token → usually 200
end
Note over TR: on reconnect: transport._hasReceivedBytes<br/>→ append replay=0 query param<br/>to baseUrl before dial
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| terminalId, | ||
| websocketUrlRef.current, | ||
| terminalInstanceId, | ||
| getWsTokenRef.current, |
Contributor
There was a problem hiding this comment.
P1: getWsTokenRef.current is evaluated at effect-run time, so the transport captures that specific function instance. If useWorkspaceClient() later returns a new getWsToken (e.g., after a workspace-client rebuild) without a base URL change, the effect won't re-run and reconnects will keep calling the stale provider — reintroducing the token-staleness issue this PR addresses.
Pass a stable wrapper that dereferences the ref at dial time:
const getFreshWsToken = useCallback(() => getWsTokenRef.current(), []);Then pass getFreshWsToken to both connect() and reconnect().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx, line 155:
<comment>`getWsTokenRef.current` is evaluated at effect-run time, so the transport captures that specific function instance. If `useWorkspaceClient()` later returns a new `getWsToken` (e.g., after a workspace-client rebuild) without a base URL change, the effect won't re-run and reconnects will keep calling the stale provider — reintroducing the token-staleness issue this PR addresses.
Pass a stable wrapper that dereferences the ref at dial time:
```ts
const getFreshWsToken = useCallback(() => getWsTokenRef.current(), []);
Then pass getFreshWsToken to both connect() and reconnect().
</file context>
</details>
| } | ||
| if (needsPreFlight) { | ||
| void primeRelayAffinity(actualUrl).then(openSocket); | ||
| void primeRelayAffinity(buildDialUrl()).then(openSocket); |
Contributor
There was a problem hiding this comment.
P2: If _getToken throws synchronously inside buildDialUrl(), the exception propagates out of this expression unhandled — the transport remains stuck in "connecting" state with no error recovery or reconnect attempt. Consider wrapping buildDialUrl() in a .then() chain or a try/catch so that a token-minting failure still falls through to the reconnect/error path:
if (needsPreFlight) {
void Promise.resolve()
.then(() => primeRelayAffinity(buildDialUrl()))
.catch(() => undefined)
.then(openSocket);
} else {
openSocket();
}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 434:
<comment>If `_getToken` throws synchronously inside `buildDialUrl()`, the exception propagates out of this expression unhandled — the transport remains stuck in `"connecting"` state with no error recovery or reconnect attempt. Consider wrapping `buildDialUrl()` in a `.then()` chain or a try/catch so that a token-minting failure still falls through to the reconnect/error path:
```ts
if (needsPreFlight) {
void Promise.resolve()
.then(() => primeRelayAffinity(buildDialUrl()))
.catch(() => undefined)
.then(openSocket);
} else {
openSocket();
}
```</comment>
<file context>
@@ -397,12 +426,12 @@ export function connect(
}
if (needsPreFlight) {
- void primeRelayAffinity(actualUrl).then(openSocket);
+ void primeRelayAffinity(buildDialUrl()).then(openSocket);
} else {
openSocket();
</file context>
Suggested change
| void primeRelayAffinity(buildDialUrl()).then(openSocket); | |
| void Promise.resolve() | |
| .then(() => primeRelayAffinity(buildDialUrl())) | |
| .catch(() => undefined) | |
| .then(openSocket); |
This branch has not been 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
Remote-workspace terminals fail to attach while everything else (files, PRs, git, ports) works. Relay logs show
/hosts/<org>:<machine>/terminal/<id>?token=…returning HTTP 401 in ~0–1 ms at the relay edge, at a ~71% rate across multiple users' hosts (128× 401 vs 10× 200 on a live sample). The same host gets both 200 and 401 → a per-token rejection, not a per-host/version issue.Root cause
The terminal WebSocket URL has a 1-hour better-auth JWT baked into it at React render time (
useWorkspaceWsUrl→getWsToken()), and the desktop transport stores that full URL intransport.currentUrl. Every reconnect path —reconnectNow,scheduleReconnect,handleResume— redialstransport.currentUrlverbatim, never re-minting the token. Once the JWT passes itsexp, the relay (validating via better-auth JWKS) rejects the attach with 401. tRPC/filesystem calls are unaffected because they mint a fresh token per request.The module-level token refresh (
setJwtevery 50 min) updates a plain variable that doesn't trigger a re-render, so the baked URL never recomputes.Trigger: the sleep/wake + focus/visibility auto-reconnect machinery added in #5135 (and hardened in #5137) redials aggressively using the token-baked URL, converting a previously-latent staleness bug into a fleet-wide 401 storm. Surfacing via the recent canary / 1.12.6 builds. The relay validator and JWT signing config are unchanged — this is not a server/secret problem.
Fix
Stop persisting the token in
currentUrl; mint it fresh per dial:terminal-ws-transport.ts: store a token-less base URL (stripUrlToken) and append a freshly-mintedtokenat each dial — both the WS open and the fly-affinity pre-flight — via a new optionalgetTokenprovider onconnect().terminal-runtime-registry.ts: threadgetTokenthroughconnect()/reconnect(); compare against the stripped URL so a token refresh alone doesn't tear down a live shell.TerminalPane.tsx: passgetWsToken(fromuseWorkspaceClient()) as the provider.This mirrors what web's
TerminalConnection.buildUrl()and desktop's tRPC headers already do (fresh token per request).Verification
bun test terminal-ws-transport.test.ts→ 7 passbiome check→ clean@codemirrordedup error inCodeEditor.tsxis unrelated and reproduces onmainwithout this change).Immediate mitigation (no release needed)
Affected users can fully quit & reopen the desktop app (or reopen the terminal) to force a fresh token.
Summary by cubic
Re-mint the terminal JWT on every reconnect in the desktop app to stop 401s after token expiry. Store a token‑less base URL and add a fresh token for each dial (including pre-flight).
currentUrl; append a fresh token per (re)dial interminal-ws-transport(WS open and relay-affinity pre-flight).getTokentoconnect(); thread throughterminal-runtime-registryso reconnects use new tokens.getWsTokenfromuseWorkspaceClient()inTerminalPane(@superset/workspace-client).Written for commit 2a35f66. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes