Repository navigation
fix(ssh): retire stranded relays and remote tabs after an app update #21216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,10 @@ | ||||||||||
| /** | ||||||||||
| * Deciding whether a relay socket path is ours to take, and acting on the answer. | ||||||||||
| * | ||||||||||
| * The only destructive action available here is a SIGTERM to a relay that has been proven — | ||||||||||
| * by argv, by socket-holder enumeration, and by a child census re-run on the host immediately | ||||||||||
| * before the signal — to hold nothing at all. Everything else is left running. | ||||||||||
| * Per docs/reference/ssh-execution-boundary.md, a relay we merely failed to reach is | ||||||||||
| * `unverifiable`, and `unverifiable` never authorizes a kill or a rebind. | ||||||||||
| * Same-path takeover may SIGTERM only a relay proven empty (argv, sole holder, no unaccounted | ||||||||||
| * children). The superseded-generation sweep uses a separate command that still re-checks argv | ||||||||||
| * but may signal a daemon that still holds PTYs, because that generation is unreachable after | ||||||||||
| * an app update. A relay we merely failed to reach is `unverifiable` and never authorized. | ||||||||||
| */ | ||||||||||
| import type { SshConnection } from './ssh-connection' | ||||||||||
| import { shellEscape } from './ssh-connection-utils' | ||||||||||
|
|
@@ -32,28 +31,40 @@ import type { RemoteHostPlatform } from './ssh-remote-platform' | |||||||||
| export type RelayHuskReapResult = 'reaped' | 'reap-unconfirmed' | 'retained-live-work' | ||||||||||
|
|
||||||||||
| const REAP_CONFIRM_ATTEMPTS = 15 | ||||||||||
| // Relay dispose waits up to IMMEDIATE_PTY_EXIT_TIMEOUT_MS (8s) for PTY SIGKILL to land. | ||||||||||
| const SUPERSEDED_REAP_CONFIRM_ATTEMPTS = 50 | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Signal one relay, re-verifying identity and emptiness inside the same command. | ||||||||||
| * | ||||||||||
| * The re-verification is not belt-and-braces: a client can attach and spawn a PTY between the | ||||||||||
| * probe and the signal, and pids are reused. `MISMATCH`/`BUSY` abort without signalling. | ||||||||||
| */ | ||||||||||
| export function reapEmptyRelayHuskCommand(pid: number, sockPath: string): string { | ||||||||||
| function reapRelayDaemonCommand( | ||||||||||
| pid: number, | ||||||||||
| sockPath: string, | ||||||||||
| options?: { requireEmptyUnrecognizedChildren?: boolean; confirmAttempts?: number } | ||||||||||
| ): string { | ||||||||||
| const requireEmpty = options?.requireEmptyUnrecognizedChildren !== false | ||||||||||
| const confirmAttempts = options?.confirmAttempts ?? REAP_CONFIRM_ATTEMPTS | ||||||||||
| return [ | ||||||||||
| `pid=${shellEscape(String(pid))}`, | ||||||||||
| `sock=${shellEscape(sockPath)}`, | ||||||||||
| 'args=$(ps -o args= -p "$pid" 2>/dev/null | tr "\\n" " ")', | ||||||||||
| 'case "$args" in *relay.js*"$sock"*) ;; *) printf \'MISMATCH\\n\'; exit 0 ;; esac', | ||||||||||
| // Why the same census as the probe: `unknown` (no pgrep) and any child this host could | ||||||||||
| // not account for as a relay service both land on BUSY, so nothing is signalled. | ||||||||||
| ...relayDaemonChildCensusShell(), | ||||||||||
| `[ "$${RELAY_UNRECOGNIZED_CHILD_COUNT_VAR}" = "0" ] || { printf 'BUSY\\n'; exit 0; }`, | ||||||||||
| ...(requireEmpty | ||||||||||
| ? [ | ||||||||||
| // Why the same census as the probe: `unknown` (no pgrep) and any child this host could | ||||||||||
| // not account for as a relay service both land on BUSY, so nothing is signalled. | ||||||||||
| ...relayDaemonChildCensusShell(), | ||||||||||
| `[ "$${RELAY_UNRECOGNIZED_CHILD_COUNT_VAR}" = "0" ] || { printf 'BUSY\\n'; exit 0; }` | ||||||||||
| ] | ||||||||||
| : [ | ||||||||||
| // Sole-holder was a probe-time fact. Another client can attach before this signal. | ||||||||||
| // Why -a: lsof ORs selectors without it and reports unrelated unix-socket holders. | ||||||||||
| "command -v lsof >/dev/null 2>&1 || { printf 'BUSY\\n'; exit 0; }", | ||||||||||
| 'holder_count=$(lsof -t -a -U -- "$sock" 2>/dev/null | sort -u | wc -l | tr -d \' \')', | ||||||||||
| '[ "$holder_count" = "1" ] || { printf \'BUSY\\n\'; exit 0; }' | ||||||||||
|
Comment on lines
+60
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '31,100p' src/main/ssh/ssh-relay-endpoint-takeover.ts
sed -n '270,320p' src/main/ssh/ssh-relay-endpoint-incumbent.ts
rg -n 'sole socket holder|sole holder|reapSupersededRelayCommand|BUSY|MISMATCH' src/main/ssh docs/reference/ssh-execution-boundary.mdRepository: stablyai/orca Length of output: 16284 🏁 Script executed: sed -n '120,185p' src/main/ssh/ssh-relay-endpoint-takeover.ts
sed -n '1,90p' src/main/ssh/ssh-relay-superseded-endpoints.ts
sed -n '1,220p' src/main/ssh/ssh-relay-endpoint-incumbent.ts
sed -n '200,295p' src/main/ssh/ssh-relay-endpoint-incumbent-shell.integration.test.ts
sed -n '200,245p' src/main/ssh/ssh-relay-endpoint-takeover.test.tsRepository: stablyai/orca Length of output: 20045 Require the sole socket holder to be The command validates Compare the sole Proposed fix- 'holder_count=$(lsof -t -a -U -- "$sock" 2>/dev/null | sort -u | wc -l | tr -d \' \')',
- '[ "$holder_count" = "1" ] || { printf \'BUSY\\n\'; exit 0; }'
+ 'holder_pid=$(lsof -t -a -U -- "$sock" 2>/dev/null | sort -u)',
+ '[ "$holder_pid" = "$pid" ] || { printf \'BUSY\\n\'; exit 0; }'📝 Committable suggestion
Suggested change
Comment on lines
+56
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment says a client can attach before this signal, but |
||||||||||
| ]), | ||||||||||
| // SIGTERM only: the relay's own handler disposes and unlinks. SIGKILL would leave the | ||||||||||
| // socket inode behind and skip that shutdown path for no gain on an empty daemon. | ||||||||||
| 'kill -TERM "$pid" 2>/dev/null || true', | ||||||||||
| 'i=0', | ||||||||||
| `while [ $i -lt ${REAP_CONFIRM_ATTEMPTS} ]; do`, | ||||||||||
| `while [ $i -lt ${confirmAttempts} ]; do`, | ||||||||||
| ' kill -0 "$pid" 2>/dev/null || { printf \'GONE\\n\'; exit 0; }', | ||||||||||
| ' sleep 0.2', | ||||||||||
| ' i=$((i+1))', | ||||||||||
|
|
@@ -62,6 +73,24 @@ export function reapEmptyRelayHuskCommand(pid: number, sockPath: string): string | |||||||||
| ].join('\n') | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Signal one empty relay, re-verifying identity and emptiness inside the same command. | ||||||||||
| * | ||||||||||
| * The re-verification is not belt-and-braces: a client can attach and spawn a PTY between the | ||||||||||
| * probe and the signal, and pids are reused. `MISMATCH`/`BUSY` abort without signalling. | ||||||||||
| */ | ||||||||||
| export function reapEmptyRelayHuskCommand(pid: number, sockPath: string): string { | ||||||||||
| return reapRelayDaemonCommand(pid, sockPath) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** SIGTERM a superseded generation even when it still holds PTYs. Argv is still re-checked. */ | ||||||||||
| export function reapSupersededRelayCommand(pid: number, sockPath: string): string { | ||||||||||
| return reapRelayDaemonCommand(pid, sockPath, { | ||||||||||
| requireEmptyUnrecognizedChildren: false, | ||||||||||
| confirmAttempts: SUPERSEDED_REAP_CONFIRM_ATTEMPTS | ||||||||||
| }) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export function interpretRelayHuskReapOutput(output: string): RelayHuskReapResult { | ||||||||||
| const state = output.trim().split('\n').pop()?.trim() | ||||||||||
| if (state === 'GONE') { | ||||||||||
|
|
@@ -75,21 +104,21 @@ export function interpretRelayHuskReapOutput(output: string): RelayHuskReapResul | |||||||||
| return 'reap-unconfirmed' | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export async function reapEmptyRelayHusk( | ||||||||||
| async function signalRelayDaemon( | ||||||||||
| conn: SshConnection, | ||||||||||
| incumbent: RelayEndpointIncumbent, | ||||||||||
| command: string, | ||||||||||
| options?: { signal?: AbortSignal } | ||||||||||
| ): Promise<RelayHuskReapResult> { | ||||||||||
| const holder = incumbent.holders[0] | ||||||||||
| if (!holder) { | ||||||||||
| return 'retained-live-work' | ||||||||||
| } | ||||||||||
| try { | ||||||||||
| const output = await execCommand( | ||||||||||
| conn, | ||||||||||
| reapEmptyRelayHuskCommand(holder.pid, incumbent.sockPath), | ||||||||||
| { wrapCommand: true, signal: options?.signal } | ||||||||||
| ) | ||||||||||
| const output = await execCommand(conn, command, { | ||||||||||
| wrapCommand: true, | ||||||||||
| signal: options?.signal | ||||||||||
| }) | ||||||||||
| return interpretRelayHuskReapOutput(output) | ||||||||||
| } catch (err) { | ||||||||||
| if (isUnconfirmedSshCommandTermination(err)) { | ||||||||||
|
|
@@ -99,6 +128,40 @@ export async function reapEmptyRelayHusk( | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export async function reapEmptyRelayHusk( | ||||||||||
| conn: SshConnection, | ||||||||||
| incumbent: RelayEndpointIncumbent, | ||||||||||
| options?: { signal?: AbortSignal } | ||||||||||
| ): Promise<RelayHuskReapResult> { | ||||||||||
| const holder = incumbent.holders[0] | ||||||||||
| if (!holder) { | ||||||||||
| return 'retained-live-work' | ||||||||||
| } | ||||||||||
| return signalRelayDaemon( | ||||||||||
| conn, | ||||||||||
| incumbent, | ||||||||||
| reapEmptyRelayHuskCommand(holder.pid, incumbent.sockPath), | ||||||||||
| options | ||||||||||
| ) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export async function reapSupersededRelay( | ||||||||||
| conn: SshConnection, | ||||||||||
| incumbent: RelayEndpointIncumbent, | ||||||||||
| options?: { signal?: AbortSignal } | ||||||||||
| ): Promise<RelayHuskReapResult> { | ||||||||||
| const holder = incumbent.holders[0] | ||||||||||
| if (!holder) { | ||||||||||
| return 'retained-live-work' | ||||||||||
| } | ||||||||||
| return signalRelayDaemon( | ||||||||||
| conn, | ||||||||||
| incumbent, | ||||||||||
| reapSupersededRelayCommand(holder.pid, incumbent.sockPath), | ||||||||||
| options | ||||||||||
| ) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Called when `--connect` to an existing socket failed and the caller is about to launch a | ||||||||||
| * replacement at the same path. Resolves when the launch may proceed; throws | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { describe, expect, it, vi } from 'vitest' | ||
| import { | ||
| isSshRelayGenerationChange, | ||
| retireSshRelayGenerationLeases, | ||
| shouldBroadcastSshRelayGenerationRetired | ||
| } from './ssh-relay-generation-reset' | ||
|
|
||
| describe('isSshRelayGenerationChange', () => { | ||
| it('is false until both sides have a build id', () => { | ||
| expect(isSshRelayGenerationChange(undefined, '0.1.0+new')).toBe(false) | ||
| expect(isSshRelayGenerationChange('0.1.0+old', undefined)).toBe(false) | ||
| expect(isSshRelayGenerationChange(undefined, undefined)).toBe(false) | ||
| }) | ||
|
|
||
| it('is false when the relay bundle did not move', () => { | ||
| expect(isSshRelayGenerationChange('0.1.0+same', '0.1.0+same')).toBe(false) | ||
| }) | ||
|
|
||
| it('is true only when this client last spoke a different bundle', () => { | ||
| expect(isSshRelayGenerationChange('0.1.0+old', '0.1.0+new')).toBe(true) | ||
| }) | ||
| }) | ||
|
|
||
| describe('retireSshRelayGenerationLeases', () => { | ||
| it('expires every lease on the target so the new generation cannot reattach them', () => { | ||
| const store = { markSshRemotePtyLeases: vi.fn() } | ||
| retireSshRelayGenerationLeases(store, 'ssh-1') | ||
| expect(store.markSshRemotePtyLeases).toHaveBeenCalledWith('ssh-1', 'expired') | ||
| }) | ||
| }) | ||
|
|
||
| describe('shouldBroadcastSshRelayGenerationRetired', () => { | ||
| it('does not broadcast for runtime-owned targets', () => { | ||
| expect(shouldBroadcastSshRelayGenerationRetired('runtime-ssh-vm-1')).toBe(false) | ||
| }) | ||
|
|
||
| it('broadcasts for a user SSH target', () => { | ||
| expect(shouldBroadcastSshRelayGenerationRetired('ssh-1')).toBe(true) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { isRuntimeOwnedSshTargetId } from '../../shared/execution-host' | ||
|
|
||
| /** True when this connect landed on a different relay bundle than the last successful one. */ | ||
| export function isSshRelayGenerationChange( | ||
| previousBuildId: string | undefined, | ||
| nextBuildId: string | undefined | ||
| ): boolean { | ||
| return Boolean(previousBuildId && nextBuildId && previousBuildId !== nextBuildId) | ||
| } | ||
|
|
||
| /** Drop leases so the new generation cannot reattach or cold-restore the unreachable PTYs. */ | ||
| export function retireSshRelayGenerationLeases( | ||
| store: { markSshRemotePtyLeases: (targetId: string, state: 'expired') => void }, | ||
| targetId: string | ||
| ): void { | ||
| store.markSshRemotePtyLeases(targetId, 'expired') | ||
| } | ||
|
|
||
| export function shouldBroadcastSshRelayGenerationRetired(targetId: string): boolean { | ||
| return !isRuntimeOwnedSshTargetId(targetId) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSoleIdentifiedRelayHolderis the only guard left on this path, and it assumes an attached client appears as a second socket holder. On Linux that is not true: a client attaches asnode relay.js --connect --sock-path <sock>(relay-connect-channel.ts:11callscreateConnection, i.e.connect(2), neverbind(2)), so its socket is unnamed andlsof -t -a -U <path>matches only the bound listener. I confirmed locally that a connected client is not returned, and its fd shows astype=STREAM (CONNECTED)with no NAME. With the child census dropped here, a relay another client is actively using is classifiedreap-candidateand SIGTERM'd with its PTYs — the opposite of the documented guarantee.Technical details