[kaluli123123]

ELI5

When Orca reconnects to a remote terminal, it checks which process owns the connection. If that check gets stuck, Orca now stops the check and its helper processes instead of waiting indefinitely. It keeps any evidence of a live owner, so a slow check cannot erase the reason to protect existing terminals.

What Changed

• Give the POSIX lsof subprocess five seconds, followed by up to 1.5 seconds to confirm its child and pipes have closed and no running work remains in its owned group. The connect probe and per-holder child census keep their existing behavior; this is not a five-second deadline for the entire reconnect.
• Preserve complete numeric holder records even when enumeration warns, fails, or times out. Incomplete enumeration cannot authorize reaping or prove absence.
• Propagate unconfirmed probe cleanup through deployment and superseded-endpoint cleanup, preventing replacement launch or subsequent version GC while cleanup is unresolved.
• Register termination handlers before spawning; recognize positively observed zombie-only groups as stopped. A bounded process-state census must also confirm cleanup of its own helper group.
• Leave the Windows named-pipe probe as a no-op.

Why

macOS lsof can stall and fork filesystem helpers. Killing only its parent leaves those helpers behind. Discarding partial output can also lose positive evidence of a socket holder. The supervisor follows Orca's existing ownership model and signals only the process group it created for the probe.

Linked Issue

Fixes #18081

Visual Proof

N/A — SSH process observation and cleanup; no renderer layout or interaction change.

Testing

• I manually tested these changes locally
• Automated tests added/updated

The final focused run passed 114 assertions across incumbent parsing, real generated shell/socket/process execution, takeover, deployment, and superseded cleanup. New regressions cover hung parent/helper cleanup, partial holder output, malformed/truncated PID records, and suppression of replacement launch and background GC when cleanup is unconfirmed. Removing the relevant fix made the timeout, holder, helper, and GC regression controls fail.

Actual macOS /usr/sbin/lsof holder enumeration and a fixture-owned stopped native helper were exercised. The old timeout left the native helper alive; supervision removed it. Native Windows verification through Orca's paired awin environment ran the exact source bundle: normal and aborted Windows probes made zero SSH calls, while a Linux-descriptor negative control reached the refusing connection once.

Actual Ubuntu-24.04 WSL2 verification through the same awin environment passed five supervisor cases with native Linux Node and /usr/bin/lsof: parent/helper cleanup, partial warning output, completed empty enumeration, output overflow, and a fixture-stopped real native helper. Both wrapper and native helpers were absent after the timeout. Temporary runtimes and fixtures were removed; no distro packages or user checkouts were changed.

Final awin checks additionally exercised a real unreaped zombie, all three startup signals, and a census-cleanup failure after the original probe disappears. The last case remains unconfirmed even though the original group is gone. Native Windows no-op/negative-control checks also passed at the final commit; all owned processes and fixtures were cleaned.

pnpm tc:node and pnpm run check:code-quality:changed pass. Focused tests use ORCA_BACKGROUND_LAUNCH=1. No full build or full test-suite pass is claimed.

AI Disclosure

Codex assisted with implementation, independent review, and verification.

Review

Independent review identified and reproduced partial-holder loss and native helper leakage; both received fixes and regression checks. No user relay was killed or unlinked by verification. Test faults affected fixture-owned processes only.

Agent skill upstream boundary

• Not applicable; no upstream skill-installer source or material is copied or translated.

Notes

Ordinary unverifiable relay ownership still permits the existing daemon-guarded launch. Positive live evidence prevents replacement. Unconfirmed probe-process cleanup is a separate error that stops the attempt. There is no remote wire protocol change, and clients continue to execute the probe on the owning POSIX host.

Checklist

• This PR is focused on bounded, ownership-safe relay observation
• Explained the change, including ELI5
• Visual proof is N/A with reason
• Self-reviewed for correctness and performance
• Cross-platform and SSH ownership considered
• Full lint/typecheck/test/build or equivalent CI coverage

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The change adds a bounded Node-based lsof probe with cleanup-status reporting and partial holder enumeration. Incumbent parsing now raises RelayProbeCleanupUnconfirmedError when cleanup is not confirmed. Relay deployment and superseded-endpoint sweeping propagate this error and avoid replacement launches or cleanup commands. Tests cover probe timing, malformed output, partial results, confirmed and unconfirmed SSH termination, and deployment behavior.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: 🟡 Moderate · up to d1376

Cancelling a relay probe during startup can leave lsof running beyond its intended bound. Install the handlers before spawning lsof prior to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 11 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR meets the coding requirements in issue #18081. RELAY_LSOF_PROBE_JS bounds lsof at five seconds and allows up to 1.5 seconds for process-group cleanup. The probe marks incomplete or failed e…
Out of Scope Changes check	✅ Passed	The changes stay within issue #18081. The shared POSIX probe, incumbent parsing, takeover safeguards, deployment handling, superseded-endpoint handling, and related tests all support bounded lsof pr…
Title check	✅ Passed	The title clearly and concisely describes the main change: bounding the SSH relay incumbent lsof probe.
Description check	✅ Passed	The description is complete and aligned with the template. It explains the change and rationale, links issue #18081, documents testing and platform coverage, includes AI disclosure, addresses review a…

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 951ada48-8d4b-4b66-aca1-87d064051914

📥 Commits

Reviewing files that changed from the base of the PR and between 9cda5a9 and 4b61df834badea9fe360543a370dc687600b4d8b.

📒 Files selected for processing (4)

• src/main/ssh/ssh-relay-deploy.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

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

[pullfrog]

ℹ️ Safe, correct latency bound — one inline note on its scope, plus a test nit.

Reviewed changes

• 5s probe timeout — probeRelayEndpointIncumbent now passes a dedicated timeoutMs: 5_000 to execCommand instead of relying on the 30s default, capping the read-only incumbent probe (and the per-endpoint superseded-relay sweep) at 5s.
• Preserved safety semantics — confirmed failures still map to unverifiable, and unconfirmed SSH terminations still rethrow so the caller aborts the relaunch rather than racing a detached launch.
• Test hardening — new tests pin the 5s timeout, the unverifiable-on-confirmed-failure path, and the rethrow-on-unconfirmed path; the takeover and deploy suites now assert an unconfirmed probe issues no --detached / rm -f / kill.

ℹ️ Nitpicks

• The test named keeps a confirmed timeout or rejection unverifiable and unenumerable actually only exercises a plain Error with no sshChannelCloseConfirmed field. Both undefined and true fall through isUnconfirmedSshCommandTermination, so the branch is covered, but the name overstates what is constructed — consider passing an error with sshChannelCloseConfirmed: true if a confirmed close is meant to be modeled explicitly.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

• In-script lsof bound — probeRelayEndpointIncumbent no longer passes a whole-command timeoutMs; instead a new ES5 LSOF_PROBE_JS wraps lsof -t -a -U "$sock" in spawnSync({ timeout: 5000, killSignal: "SIGKILL" }), printing unavailable on any error, signal, or non-0/1 exit status.
• Connect-probe evidence preserved — the LISTEN= line still prints before the lsof step, so a slow lsof no longer discards the fast accepted → live verdict.
• Stricter lsof failure semantics — an lsof runtime failure now maps to HOLDERS_SOURCE=unavailable (unverifiable), where the old inline loop would have printed an enumerable-but-empty holder set and risked an exited verdict.
• Test hardening — the confirmed-failure test now models sshChannelCloseConfirmed: true, and new relayEndpointIncumbentProbeCommand tests pin the lsof-only bound and the connect-before-lsof ordering.

Verified locally: the four touched suites pass (74 tests), including the real-shell ssh-relay-endpoint-incumbent-shell.integration.test.ts that exercises the new spawnSync path against a live socket.

  ^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[pullfrog]

✅ No new issues found.

Reviewed changes

Since the prior review, the branch merged origin/main and reconciled the probe's child-census step with the already-merged relay-daemon-service-children.ts refactor (#13614):

• Child-census reconciliation — relayEndpointIncumbentProbeCommand now emits the daemon's child census through relayDaemonChildCensusShell() and prints a four-field HOLDER= line (pid match kids unrecognized_kids), replacing the inline three-field pgrep census. parseHolder, RelayEndpointHolder.unrecognizedChildCount, and isReapableRelayHusk (unrecognizedChildCount === 0) all align with the mainline census contract.
• lsof bounding unchanged — the in-script LSOF_PROBE_JS bound and the connect-before-lsof ordering are untouched by the merge.

Verified locally: 85 tests pass across the four touched suites, including the real-shell integration suite that exercises the spawnSync lsof path and the four-field holder census.

  ^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

Important

One must-address issue: the new process-level regression test fails deterministically on Linux, because the probe's cleanup confirmation treats an orphaned zombie helper as a live process group and reports cleanup-unconfirmed, which aborts the relay relaunch.

Reviewed changes

Since the prior pullfrog review, the branch replaced the inline spawnSync lsof bound with a supervised child process and added cleanup-uncertainty propagation:

• New supervised lsof probe (ssh-relay-lsof-probe.ts) — RELAY_LSOF_PROBE_JS spawns lsof in its own process group, SIGKILLs that group on a 5s deadline, and reports lsof / unavailable / cleanup-unconfirmed.
• Partial holder evidence preserved — the shell now emits HOLDER= lines even when enumeration warns, fails, or times out, so a positive holder still yields live/holder-process with holdersEnumerable: false.
• Cleanup uncertainty propagated — RelayProbeCleanupUnconfirmedError is thrown by the parser, rethrown by probeRelayEndpointIncumbent, rethrown through launchRelay, and gates the post-launch gcOldRelayVersions chain in ssh-relay-deploy.ts.
• Test reorganization — the generated probe is exercised against a fixture lsof (hung parent, hung helper, partial output, malformed PIDs), and takeover/deploy assertions were tightened to the \bkill\s / --detached|rm -f|kill forms.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 2f5ec264-f1b2-4aea-93ea-3fb8edfc6fee

📥 Commits

Reviewing files that changed from the base of the PR and between 5465f3c and d137606.

📒 Files selected for processing (3)

• src/main/ssh/ssh-relay-endpoint-incumbent.test.ts
• src/main/ssh/ssh-relay-endpoint-incumbent.ts
• src/shared/child-process/posix-lsof-probe.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• src/main/ssh/ssh-relay-endpoint-incumbent.ts
• src/main/ssh/ssh-relay-endpoint-incumbent.test.ts

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

[pullfrog]

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review, the branch added zombie-aware cleanup confirmation and probe startup-signal handling to the supervised lsof probe:

• Zombie-aware cleanup confirmation — RELAY_LSOF_PROBE_JS now runs a bounded ps -axo pgid=,state= census when the lsof process group still appears present after its direct child closes; a group whose only members are zombies (Z…) is confirmed clean, eliminating the cleanup-unconfirmed verdict that aborted relaunch on Linux hosts whose PID 1 does not reap orphans promptly.
• Census lifecycle supervision — finish clears the cleanup timer and SIGKILLs any in-flight census group; the census is bounded at 1000ms / 8MB and retries live members every 25ms until the 1500ms cleanup deadline forces an unconfirmed verdict.
• Startup signal handling — SIGTERM/SIGHUP/SIGINT handlers are registered before lsof spawns, so a signal delivered during startup routes through group cleanup instead of terminating the probe under the default disposition.
• Test updates — new posix-lsof-probe.test.ts drives the supervisor through a --require preload (zombie-only evidence, startup signals, live / failed / hung / oversized census); ssh-relay-incumbent-process.test.ts derives liveness from ps -o state= rather than kill(pid, 0) and renames the helper case to stops … since the probe cannot reap an orphan.
• Probe relocation — RELAY_LSOF_PROBE_JS moved from src/main/ssh/ssh-relay-lsof-probe.ts to src/shared/child-process/posix-lsof-probe.ts (d137606), no behavior change.

The prior must-address finding (zombie helper mistaken for a live group) is resolved: the zombie-only case now confirms clean, and the affected test passes.

ℹ️ Nitpicks

• The eight keeps cleanup unconfirmed for %s cases assert only the fallback marker that both the pre-fix and current implementations emit, so they exercise the new census failure handlers but do not fail if readGroupStates were removed. accepts zombie-only group evidence is the case that actually guards the fix; asserting the census was consulted (e.g. via the existing PS_PID side effect) in the parameterized cases would make a revert fail for the intended reason.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review, RELAY_LSOF_PROBE_JS now suppresses lsof filesystem warnings so they no longer discard an otherwise-complete holder enumeration:

• Warnings no longer force unavailable — the spawn is now spawn('lsof', ['-w', '-t', '-a', '-U', process.argv[1]]). A can't stat() <mount> warning previously set stderrSeen, which made the marker unavailable → HOLDERS_SOURCE=unavailable → holdersEnumerable=false, so the exited verdict (and the superseded-endpoint sweep's reap path) was unreachable on hosts with un-stat-able mounts. Genuine failures still set unavailable through a non-0/non-1 exit, signal, or stream/child error.
• Regression coverage — a new posix-lsof-probe.test.ts case drives a fixture lsof that writes a warning to stderr unless -w is present, asserting the marker stays lsof and the holder pids survive; the existing selector assertion was updated for the new flag.

Verified locally: the focused suite passes (14 tests), and reverting -w makes the new case fail for the intended reason (expected 'unavailable' to be 'lsof'). The prior must-address finding (zombie helper mistaken for a live group) remains resolved.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review, the branch reverted the lsof warning-suppression refinement:

• Reverted -w — RELAY_LSOF_PROBE_JS spawns lsof as ['-t', '-a', '-U', process.argv[1]] again, and posix-lsof-probe.test.ts drops the warning-emitting fixture case along with the now-unused lsof fixture override in runProbe.
• Restored prior state — a tree diff from the reverted commit's parent (bfd53b9f8f) to head is empty across both touched paths, so the probe matches the revision previously reviewed clean.

ℹ️ Reverting -w restores conservative holder classification

This is informational, not a request for changes. With -w removed, a host where lsof writes a filesystem warning (can't stat() <mount>) again classifies the enumeration as unavailable (HOLDERS_SOURCE=unavailable, holdersEnumerable=false) even when it enumerated completely. Holder pids are still emitted, so positive holder evidence continues to yield live/holder-process; the only lost reachability is the empty-enumeration exited verdict (still permitted by mayLaunchOverRelayEndpoint) and the superseded sweep's stale-endpoint-removed reap on those hosts. Failing safe, and defensible if the intent is to stop suppressing diagnostics that can indicate incomplete enumeration.

Technical details

# Reverted `-w` reclassifies warning-bearing enumerations as unavailable

## Affected sites
- `src/shared/child-process/posix-lsof-probe.ts:115` — spawn omits `-w`.
- `src/shared/child-process/posix-lsof-probe.ts:126` — any non-empty stderr sets `stderrSeen`.
- `src/shared/child-process/posix-lsof-probe.ts:39` — `stderrSeen` downgrades the marker to `unavailable`.

## Required outcome
- None required if the revert is intentional; this records the behavioral delta so a later run does not mistake it for an unexplained regression.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}