Skip to content

fix(ssh): retire stranded relays and remote tabs after an app update - #21216

Closed
hyein-cbio wants to merge 1 commit into
stablyai:mainfrom
hyein-cbio:fix/ssh-relay-stranded-on-update
Closed

hyein-cbio wants to merge 1 commit into
stablyai:mainfrom
hyein-cbio:fix/ssh-relay-stranded-on-update

Conversation

@hyein-cbio

Copy link
Copy Markdown
Contributor

ELI5

When you update Orca, SSH remote terminals used to keep running in the background even though the new app could no longer talk to them. The next reconnect then started another copy. After enough updates, the remote host filled up with leftover relays and agents. This change treats an update as a clean break: the old unreachable generation is stopped, and those remote tabs are closed so you open new ones.

What Changed

Before: An app update deploys a new relay (the socket path includes the bundle hash). The old relay stays up with its shells and agents. The desktop cannot attach to those PTYs, so it cold-restores agents onto the new relay. Processes accumulate; two agents can share one transcript.

After: If this update changed the relay generation:

  • The old relay is SIGTERM'd when it is the only process holding its socket (no other Orca still connected). Live PTYs go with it.
  • This client's remote terminal tabs for that host are closed. They are not resumed.
  • Open new terminals on that host after updating.

Unchanged: Local tabs. Ordinary disconnect/reconnect to the same relay generation still keeps remote work alive (--grace-time 0). A probe that cannot answer is still unverifiable and is not killed. A relay with another client attached is left running.

The sweep still only runs on POSIX SSH hosts. Windows named-pipe endpoints are not reaped here.

Why

Session ids were already stored. The leak is the version-hashed socket plus a handshake that refuses any other build, plus a sweep that would not signal a relay that still held PTYs.

Preserving live attach across updates would mean a semantic protocol version and keeping the old daemon — the orcad model. That is a much larger change. Retiring the unreachable generation is the small fix: the UI already lost those sessions, so leaving them running only piled up processes.

Linked Issue

Fixes #21215

Visual Proof

No visual change. There is no new UI. After an update, existing SSH terminal tabs for that host close instead of restoring; the rest of the app looks the same.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Covered in unit and POSIX shell-integration tests: classifying a sole-holder superseded relay as reapable even with PTYs, refusing when another holder is present, SIGTERM without the empty-child gate, generation-change detection, lease expiry and tab close on generation change, and retiring before the new build id is remembered if establish fails mid-way.

Not covered here: a full app-update against a live SSH host; Windows named pipes.

Platforms exercised by tests: macOS host running Linux-shaped SSH probe/reap scripts. Windows sweep remains a no-op by existing policy.

AI Disclosure

Review

Cross-platform: the destructive sweep is POSIX-only (lsof / kill -TERM), matching the existing superseded-endpoint path. Windows is unchanged (named pipes, no sweep). macOS/Linux clients both drive the same SSH deploy.

SSH/remote: ordinary same-generation reconnect still reattaches. Loss of contact is still not treated as exited. The new kill is only for a superseded generation that is the sole socket holder.

Agents/integrations: no provider-specific resume path. All SSH agent panes on that host are closed together so we do not --resume onto a duplicate process.

Performance: one extra lsof holder count on the superseded reap command; confirm wait extends to ~10s only for that path, and returns early when the pid is gone.

Security: still SIGTERM, still argv-matches relay.js + this socket, still no kill on unverifiable probes.

UI: no new chrome.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Follow-ups not in this PR: Windows named-pipe reaping; whether expired leases should be marked so a later same-generation connect cannot cold-restore resurrected tabs (host snapshot merge currently uses user-close tombstones only).

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Updating Orca used to leave the previous relay generation running with
its PTYs, then cold-restore agents onto the new relay. The superseded
sweep now SIGTERMs a sole-holder old generation, and the client drops
those remote tabs instead of resuming them.

Fixes stablyai#21215
@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

📝 Walkthrough

Walkthrough

The change retires superseded SSH relay generations when they are the sole socket holder, including relays that still hold PTYs. Session recovery expires their leases and skips PTY reattachment. A retirement event now crosses the preload and renderer IPC layers and closes matching direct-SSH terminal tabs. Relays with another socket holder and unverifiable endpoints remain retained. Tests and reference documentation cover the updated behavior.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: 🟡 Moderate · up to ea71a

A relay cleanup race could terminate the wrong remote process, while a missed notification can leave obsolete terminal tabs open. Both should be fixed before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 20 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: retiring stranded SSH relays and remote tabs after an app update.
Description check ✅ Passed The description covers all required sections, including the user-facing change, rationale, linked issue, visual-proof explanation, testing, scope, risks, and checklist.
Linked Issues check ✅ Passed Issue #21215 requires retirement of an unreachable relay generation after a bundle change, preservation for same-generation reconnects, protection for unverifiable or multi-client relays, POSIX-only h…
Out of Scope Changes check ✅ Passed The changed documentation, relay predicates, takeover commands, generation-reset logic, IPC bridge, renderer action, and tests all support the behavior required by issue #21215. The web preload no-op …
Full details: Docstring Coverage

Explanation

Docstring coverage is 32.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 20 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 927e1650-a517-4c5c-a460-4323316bc375

📥 Commits

Reviewing files that changed from the base of the PR and between de15227 and ea71a63.

📒 Files selected for processing (21)
  • docs/reference/ssh-execution-boundary.md
  • src/main/ssh/ssh-relay-endpoint-incumbent-shell.integration.test.ts
  • src/main/ssh/ssh-relay-endpoint-incumbent.test.ts
  • src/main/ssh/ssh-relay-endpoint-incumbent.ts
  • src/main/ssh/ssh-relay-endpoint-takeover.test.ts
  • src/main/ssh/ssh-relay-endpoint-takeover.ts
  • src/main/ssh/ssh-relay-generation-reset.test.ts
  • src/main/ssh/ssh-relay-generation-reset.ts
  • src/main/ssh/ssh-relay-session-generation-reset.test.ts
  • src/main/ssh/ssh-relay-session.ts
  • src/main/ssh/ssh-relay-superseded-endpoints.test.ts
  • src/main/ssh/ssh-relay-superseded-endpoints.ts
  • src/preload/api/ssh-api.ts
  • src/preload/api/ssh-bridge.ts
  • src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts
  • src/renderer/src/hooks/ipc-events/ssh-relay-generation-retired-ipc-bridge.ts
  • src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts
  • src/renderer/src/store/terminals/direct-ssh-terminal-bindings.ts
  • src/renderer/src/store/terminals/ssh-relay-generation-tab-retirement.test.ts
  • src/renderer/src/store/terminals/terminal-actions.ts
  • src/renderer/src/web/preload-api/web-terminal-api.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment on lines +60 to +61
'holder_count=$(lsof -t -a -U -- "$sock" 2>/dev/null | sort -u | wc -l | tr -d \' \')',
'[ "$holder_count" = "1" ] || { printf \'BUSY\\n\'; exit 0; }'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.md

Repository: 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.ts

Repository: stablyai/orca

Length of output: 20045


Require the sole socket holder to be $pid.

The command validates $pid with ps, but it checks only the number of lsof results before sending SIGTERM. If socket ownership moves to another PID, the command can target a stale PID and leave the current holder running. If the stale PID is reused, it can terminate an unrelated process.

Compare the sole lsof result with $pid before sending SIGTERM.

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

‼️ 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
'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; }'

Comment on lines +2353 to +2357
const win = this.getMainWindow()
if (win && !win.isDestroyed()) {
win.webContents.send('ssh:relay-generation-retired', { targetId: this.targetId })
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

rg -n 'registerAppLifetimeIpcBridge|registerSshRelayGenerationRetiredIpcBridge|useIpcEvents|new SshRelaySession|SshRelaySession|webContents.send' src/main src/renderer/src
sed -n '100,145p' src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts

Repository: stablyai/orca

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- exact channel and bridge references ---'
rg -n -C 4 "ssh:relay-generation-retired|registerAppLifetimeIpcBridge|app-lifetime-ipc-bridge|useIpcEvents" src/renderer/src src/main --glob '!*.map'

printf '%s\n' '--- bridge file ---'
bridge=$(rg -l "ssh:relay-generation-retired" src/renderer/src | head -n 1)
printf 'bridge=%s\n' "$bridge"
[ -n "$bridge" ] && cat -n "$bridge"

printf '%s\n' '--- SSH connect flow ---'
cat -n src/main/ipc/ssh-connect-flow.ts | sed -n '1,230p'

printf '%s\n' '--- relay session retirement and lifecycle references ---'
rg -n -C 8 "retireSupersededRelayGeneration|relay-generation-retired|establish|connect|constructor|configureRelaySessionCallbacks" src/main/ssh/ssh-relay-session.ts src/main/ipc/ssh-relay-session-callbacks.ts src/main/ipc/ssh-active-relay-sessions.ts src/main/ipc/ssh-session-teardown.ts

printf '%s\n' '--- likely renderer startup entrypoints ---'
rg -n -C 5 "registerAppLifetimeIpcBridge|create.*Ipc|useIpcEvents|ipc-events|appLifetime" src/renderer/src --glob '*.{ts,tsx}'

Repository: stablyai/orca

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -e
log=/tmp/coderabbit-shell-logs/shell-output-Rko6rU
printf '%s\n' '--- exact channel matches from prior output ---'
rg -n -C 6 -F "ssh:relay-generation-retired" "$log" | head -n 240

printf '%s\n' '--- exact bridge registration matches from prior output ---'
rg -n -C 6 -F "registerAppLifetimeIpcBridge" "$log" | head -n 240

printf '%s\n' '--- exact listener file candidates ---'
rg -l -F "ssh:relay-generation-retired" src/renderer/src src/main

Repository: stablyai/orca

Length of output: 3150


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- app-lifetime bridge ---'
cat -n src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts

printf '%s\n' '--- renderer IPC event wiring ---'
rg -n -C 5 "app-lifetime-ipc-bridge|AppLifetime|lifetime|useIpcEvents|on[A-Z].*Ipc|ipc.*events" src/renderer/src/hooks src/renderer/src/app-shell src/renderer/src --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 500

printf '%s\n' '--- SSH connect flow ---'
cat -n src/main/ipc/ssh-connect-flow.ts | sed -n '1,230p'

printf '%s\n' '--- exact generation reset and retirement logic ---'
rg -n -C 14 "shouldBroadcastSshRelayGenerationRetired|retireSupersededRelayGeneration|generation" src/main/ssh/ssh-relay-session.ts src/main/ssh/ssh-relay-session-generation-reset.test.ts | head -n 500

Repository: stablyai/orca

Length of output: 50369


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- relay-retired renderer bridge ---'
cat -n src/renderer/src/hooks/ipc-events/ssh-relay-generation-retired-ipc-bridge.ts

printf '%s\n' '--- app shell service and callers ---'
cat -n src/renderer/src/app-shell/use-app-shell-services.ts
rg -n -C 8 "useAppShellServices|AppShell|use-app-shell-services|installAppLifetimeIpcEvents|useIpcEvents" src/renderer/src --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 260

printf '%s\n' '--- SSH connect flow ---'
cat -n src/main/ipc/ssh-connect-flow.ts

printf '%s\n' '--- generation reset source ---'
sed -n '2280,2385p' src/main/ssh/ssh-relay-session.ts
cat -n src/main/ssh/ssh-relay-session-generation-reset.test.ts | sed -n '1,170p'

printf '%s\n' '--- alternate direct SSH cleanup/replay references ---'
rg -n -C 8 "retireDirectSshTerminalsForRelayGeneration|closeTab|direct.*ssh|relay.*generation|generation.*retir|on.*Retir|snapshot" src/renderer/src/hooks/ipc-events src/renderer/src/store/terminals src/renderer/src/store --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 500

Repository: stablyai/orca

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- preload relay-retired binding ---'
rg -n -C 10 "onRelayGenerationRetired|relay-generation-retired|ssh.*on" src/preload src/main src/shared --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 320

printf '%s\n' '--- startup and session-establishment callers ---'
rg -n -C 10 "connectTarget\(|connectTargets|auto.*connect|startup.*ssh|ssh.*startup|eager.*ssh|establish\(" src/main --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 520

printf '%s\n' '--- direct SSH retirement and store implementation ---'
rg -n -C 14 "retireDirectSshTerminalsForRelayGeneration|collectDirectSshTerminalTabIds|DirectSshAuthority|directSsh" src/renderer/src/store/terminals src/renderer/src/store src/renderer/src/hooks/ipc-events --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 700

printf '%s\n' '--- generation retirement tests and replay-like assertions ---'
rg -n -C 12 "generation-retired|retireDirectSsh|relay generation|persisted.*terminal|remote.*lease" src/main src/renderer/src --glob '*.{test,spec}.{ts,tsx}' | head -n 500

Repository: stablyai/orca

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- all connectTarget callers ---'
rg -n -F "connectTarget(" src/main --glob '!**/*.test.*' --glob '!**/*.spec.*'
printf '%s\n' '--- renderer SSH connect callers ---'
rg -n -C 5 "window\.api\.ssh\.connect|api\.ssh\.connect|ssh\.connect" src/renderer/src --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 300

printf '%s\n' '--- direct SSH terminal binding implementation ---'
cat -n src/renderer/src/store/terminals/direct-ssh-terminal-bindings.ts | sed -n '1,210p'

printf '%s\n' '--- related direct SSH state bridge and startup hydration ---'
cat -n src/renderer/src/hooks/ipc-events/direct-ssh-state-ipc-bridge.ts
rg -n -C 10 "retireDirectSshTerminalsForRelayGeneration|collectDirectSshTerminalTabIds|DirectSshAuthority|ssh.*target|targetId" src/renderer/src/store/terminals/direct-ssh-terminal-bindings.ts src/renderer/src/hooks/ipc-events/direct-ssh-state-ipc-bridge.ts src/renderer/src/app-shell/use-app-startup-hydration.ts src/renderer/src/app-shell --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 500

Repository: stablyai/orca

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- all retirement call sites ---'
rg -n -C 18 -F "retireSupersededRelayGeneration(" src/main/ssh/ssh-relay-session.ts

printf '%s\n' '--- establish/reconnect generation decision flow ---'
sed -n '538,685p' src/main/ssh/ssh-relay-session.ts
sed -n '687,880p' src/main/ssh/ssh-relay-session.ts
rg -n -C 16 "isSshRelayGenerationChange|previousRelayBuildId|serverBuildId|generation" src/main/ssh/ssh-relay-session.ts | head -n 420

printf '%s\n' '--- relay-loss callback path ---'
cat -n src/main/ipc/ssh-relay-session-callbacks.ts | sed -n '1,190p'

printf '%s\n' '--- startup hydration ordering ---'
cat -n src/renderer/src/App.tsx | sed -n '30,55p'
cat -n src/renderer/src/app-shell/use-app-startup-hydration.ts | sed -n '210,285p'
cat -n src/renderer/src/startup/startup-ssh-connection-restore.ts | sed -n '1,125p'

Repository: stablyai/orca

Length of output: 50369


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- alternate host-owned direct SSH cleanup paths ---'
rg -n -C 10 "remoteCloseOwnedByHost|localPtyTeardownOwnedExternally|SSH_SESSION_EXPIRED|sessionExpired|closeTab\(" src/renderer/src --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 700

printf '%s\n' '--- direct SSH reconnect routing ---'
rg -n -C 16 "function routeDirectSshConnectedState|routeDirectSshConnectedState|invalidateStaleTerminalBindings|correctUnboundTerminals|prepareAndSync" src/renderer/src/hooks src/renderer/src/ssh src/renderer/src/store --glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 650

printf '%s\n' '--- existing repository evidence for send-before-listener behavior ---'
rg -n -C 8 "webContents\.send.*listener|listener.*webContents\.send|not attached|dropped.*listener" src/main --glob '*.{ts,tsx}' | head -n 160

Repository: stablyai/orca

Length of output: 50370


Make relay-generation retirement replayable. SshRelaySession.establish() and reconnect() call retireSupersededRelayGeneration() when the relay build changes. That method sends ssh:relay-generation-retired once. The renderer registers the listener only from the useIpcEvents React effect. A relay-loss callback can start SshRelaySession.reconnect() while the renderer is loading or reloading. window.api.ssh.onRelayGenerationRetired has no snapshot or replay. If the send occurs before registration, the handler that closes the direct-SSH tabs does not run. Connection-state reconciliation does not close those tabs, so old direct-SSH tabs can remain open.

Persist and replay pending retirements, or buffer the notification at preload level until the app-lifetime bridge is registered. Registering only in the React effect does not cover renderer reloads.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

The superseded sweep's "sole holder" gate cannot detect another attached Orca client, so reapSupersededRelayCommand can SIGTERM a superseded relay that a second (still older) client is actively using and destroy its live remote PTYs. Both isReapableSupersededRelay and the reap-time re-check rely on lsof -t -a -U <path>, which on Linux associates the path only with the bound listener, never with the --connect client bridge (unnamed connect(2) socket). The PR's stated safety property — "a relay with another client still attached is left running" — does not hold. Details inline.

Reviewed changes

  • Generation detection — New src/main/ssh/ssh-relay-generation-reset.ts treats a change in the persisted relay serverBuildId (fullVersion, which also names the install dir) as a generation break.
  • Lease and tab retirement — SshRelaySession.establish/reconnect expire the target's remote PTY leases, clear PTY ownership, skip reattachKnownPtys, and emit ssh:relay-generation-retired; the renderer closes that host's terminal tabs via a new IPC bridge.
  • Superseded sweep reaps relays holding PTYs — reapSupersededRelayCommand drops the empty-child census and instead re-checks the socket holder count before SIGTERM; classifySupersededRelay uses the new isReapableSupersededRelay.
  • IPC surface — New ssh.onRelayGenerationRetired preload method (web client no-op), IPC bridge registration, and lifecycle-test expectations.
  • Docs — docs/reference/ssh-execution-boundary.md reworded from "strands" to "retires".

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +308 to +313
* A superseded generation this client can no longer handshake with. Sole holder means no other
* Orca is attached, so SIGTERM destroys only unreachable work — including live PTYs.
* Same-path takeover must keep using {@link isReapableRelayHusk}; that path still preserves work.
*/
export function isReapableSupersededRelay(incumbent: RelayEndpointIncumbent): boolean {
return isSoleIdentifiedRelayHolder(incumbent)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isSoleIdentifiedRelayHolder is 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 as node relay.js --connect --sock-path <sock> (relay-connect-channel.ts:11 calls createConnection, i.e. connect(2), never bind(2)), so its socket is unnamed and lsof -t -a -U <path> matches only the bound listener. I confirmed locally that a connected client is not returned, and its fd shows as type=STREAM (CONNECTED) with no NAME. With the child census dropped here, a relay another client is actively using is classified reap-candidate and SIGTERM'd with its PTYs — the opposite of the documented guarantee.

Technical details
# "Sole holder" does not mean "no client attached"

## Affected sites
- `src/main/ssh/ssh-relay-endpoint-incumbent.ts:282-290` — `isSoleIdentifiedRelayHolder` keys off `holders.length === 1`
- `src/main/ssh/ssh-relay-endpoint-incumbent.ts:312-313` — `isReapableSupersededRelay` drops `unrecognizedChildCount === 0`, so this is the only guard
- `src/main/ssh/ssh-relay-endpoint-takeover.ts:56-61` — the reap-time re-check has the same blind spot
- `src/main/ssh/ssh-relay-superseded-endpoints.test.ts:148-163` — the test that claims to cover this is mock-only

## Required outcome
- The superseded sweep must not signal a relay that has a client attached; or the PR must state that a superseded generation is reaped unconditionally and drop the "another client attached" claim and test.

## Evidence
- Client side: `src/relay/relay-connect-channel.ts:11` `createConnection({ path: sockPath })`. Daemon side binds: `src/relay/relay-socket-ownership.ts:79` `server.listen(this.sockPath)`.
- Local reproduction: a Node server `listen(path)` plus a client `connect(path)`; `lsof -t -a -U <path>` returned only the server pid. The client's fds were `type=STREAM (CONNECTED)` with no path in the NAME column.
- Argv matching would not have excluded the bridge either: its argv contains `relay.js` and the exact socket, so `matchesRelayArgv` would be true; `holders.length` is the sole discriminator.

## Suggested approach (optional)
- Detect `--connect` bridges explicitly on the host (e.g. `pgrep`/`/proc/*/cmdline` scan for `relay.js` + `--connect` + this socket), or confirm attachment via `ss`/`/proc/net/unix` peer state, before signalling.

## Open questions for the human
- Is a second client ever attached to the same target's relay across app versions? If yes this loses live remote work. If no, the guarantee and its test should be removed rather than asserted.

Comment on lines +56 to +61
: [
// 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; }'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says a client can attach before this signal, but holder_count from lsof -t -a -U counts only the bound listener — an attached --connect bridge never raises it to 2. So this re-check can't catch the race it was written for, and the SIGTERM proceeds against a relay another client is using. The same attached-client detection gap described on isReapableSupersededRelay applies here.

expect(issuedCommands()[2]).not.toContain('unrecognized_kids')
})

it('leaves a superseded relay running when another client still holds the socket', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test asserts the safety property by fabricating HOLDER=3669804 no 0 0; no test runs lsof against a real connected client (the integration test starts a relay with a child but never a --connect bridge). Given the enumeration can't observe a client, this gives false confidence. If the guarantee is kept, add an integration case that starts a relay, attaches a real --connect bridge, probes, and asserts retention.

@hyein-cbio

Copy link
Copy Markdown
Contributor Author

Closing this without merging.

This change treats an app update as a clean break: if the relay generation changed, SIGTERM the old relay when it is the sole socket holder, even if it still has PTYs, and close this client's remote tabs for that host instead of resuming them. That is not the direction main took.

main keeps a superseded relay that still holds work. When the new relay answers "PTY not found", that is not treated as proof the terminal is gone while an older build may still be running it. On POSIX the tab is reattached through that older relay's own connector. If that cannot be done, the terminal is left running and is not cold-restored onto the new relay. That behavior reached main in #24863, from the approach in #25124 and #25170. Merging this PR would kill the processes that path is trying to keep.

This is also not fixed in the v1.4.223 release. The release commit 5272afe is not on main, and that tag does not contain the previous-relay reattach. Leaving #21215 open.

@hyein-cbio hyein-cbio closed this Oct 8, 2026
@hyein-cbio
hyein-cbio deleted the fix/ssh-relay-stranded-on-update branch October 8, 2026 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Updating Orca strands SSH relays and duplicates remote agents

1 participant