Repository navigation
Preserve legacy daemon ownership when liveness is unavailable - #16900
brennanb2025 wants to merge 4 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLegacy daemon cleanup now removes ownership files only when the PID record is valid and the recorded process is confirmed exited. The PID file is removed only if its PID and launch nonce still match the record. The token file is removed only after PID-file removal succeeds. Tests cover process-check outcomes, unreadable and malformed records, and files republished during the process check. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Legacy cleanup can delete a replacement daemon’s token, preventing its sessions from being adopted. Protect token ownership before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new checks reduce the risk of losing access to running terminals when a process check fails. A pre-existing timing gap around credential deletion remains, and behavior across every deployment condition has not been verified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
|
Fixed the static-analysis failure at head Decision: the redundant
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eea5cad2-b4f1-4576-8378-7019189081d9
📒 Files selected for processing (3)
src/main/daemon/daemon-incarnation-evidence-types.tssrc/main/daemon/daemon-init-provider-installation.test.tssrc/main/daemon/daemon-init.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
Independent adversarial review — head
|
| # | Arm mutated back toward coercion | Result | Pinned? |
|---|---|---|---|
| A1 | gate switch: unverifiable falls through to deletion (daemon-init.ts:1385) |
new EACCES test fails | ✅ |
| A2 | read-failure catch → exited (daemon-init.ts:1355) |
new EACCES test fails | ✅ |
| A3 | !parsed (unparseable record) → exited (daemon-init.ts:1358) |
all 14 pass | ❌ |
| A4 | signal unavailable → exited (daemon-init.ts:1368) |
all 14 pass | ❌ |
| A5 | signal permission_denied → exited (daemon-init.ts:1364) |
all 14 pass | ❌ |
| A6 | occupied → exited |
pre-existing alive test fails | ✅ |
| A7 | missing → live |
pre-existing ESRCH test fails | ✅ |
Request before merge: pin A3/A4/A5 (unparseable record preserved; kill-probe non-ESRCH/EPERM error preserved; EPERM counts as live). Each is a small variant of the existing fixtures (parseDaemonPidFileMock.mockReturnValue(null); kill spy throwing EINVAL-coded and EPERM-coded errors). These are precisely the coercion-resistance arms this PR exists to add; today a refactor can silently reintroduce failure-becomes-fact on 3 of 5 routes with CI green.
2. Surviving-coercion attempts (the destructive site)
Deletion requires the full conjunction: socket probe false and pid file read succeeds and record parses and process.kill(pid, 0) throws a literal ESRCH for the recorded pid. Every constructed failure lands on preserve or a third path:
| Scenario | Outcome |
|---|---|
kill probe throws non-EPERM/non-ESRCH (EINVAL, no-code throw, string throw) |
unavailable → preserved (verified hasErrorCode requires 'code' in error) |
partially populated record ({"pid":123} only; metadata nulls) |
probed on pid alone → signal verdict; never deleted on partial metadata. startedAtMs is ignored — no pid-recycle demotion (pre-existing; recycled pid reads occupied → preserved, same as pre-fix) |
| signal probe "times out" | structurally impossible — kill(pid,0) is a synchronous syscall; the socket probe timeout only gates entry and post-fix no longer authorizes deletion by itself |
| pid file exists but unreadable (EACCES/EIO) | preserved (pinned by the new test) |
| runtime dir on a disconnected network share (ENOTCONN/ETIMEDOUT/EIO, or autofs-style ENOENT) | read throws → preserved |
| corrupt record from an older protocol (parse → null) | preserved — forever (see finding F4) |
| SSH host with contact lost | this site never crosses the SSH boundary: it runs in the client's main process against the local runtime dir; remote daemons are owned by the relay host. Loss of remote contact cannot reach this unlink. A network-mounted runtime dir degrades to the row above → preserved |
weird pids from a hostile/torn record: pid:0, empty file (Number('') = 0), negative pid |
kill(0,0)/kill(-1,0) probe the caller's own process group → no throw → occupied → "live" → preserved (empirically verified). False evidence label, safe direction; behavior-identical to pre-fix (F5) |
fractional pid ({"pid":123.5}) |
ERR_INVALID_ARG_TYPE → unavailable → preserved (empirically verified) |
I could not construct a coercion that deletes ownership. The one deletion route that survives a wrong outcome is a parsed record that names the wrong (already-exited) pid while the real daemon is alive on a different pid with a transiently failing socket — that is record-content corruption the client cannot distinguish from truth, and ESRCH on the recorded pid is positive evidence about the record's own claim. Acceptable.
3. Exhaustiveness ruling (the deleted default arms)
Empirical results, both directions:
- Producer switch (daemon-init.ts:1362, over
ProcessSignalEvidence): double-protected. Adding a 5th evidence variant failspnpm tc:nodewith TS2366 at the function (explicit return type, all arms return) and failsoxlint --type-awareswitch-exhaustiveness-check. - Deletion-gate switch (daemon-init.ts:1385, over
verdict.status): NOT compiler-protected. Adding a 4th verdict status (suspended) →pnpm tc:nodepasses clean — the premise that return-type annotations make a fourth verdict fail to compile is false for this switch (it is a statement switch; fall-through is legal control flow, and its fall-through direction is the unlink loop). The line is held solely bytypescript/switch-exhaustiveness-checkaterrorwithallowDefaultCaseForExhaustiveSwitch: falseinconfig/oxlint-code-quality-type-aware.json, whichpr.ymlruns over the fullsrctree on every PR (audit:code-quality:type-aware,--deny-warnings) — verified it flags 1385 for the added status. Note the deletion was also mandatory: that same rule setting errors on adefaultarm of an exhaustive switch, so thesatisfies neverarms could not have been kept. - Runtime origin:
ProcessLivenessVerdicthas exactly 3 references repo-wide (type decl, the import, the producer's return type). It never crosses a wire, RPC, or persisted record; the only persisted input (the pid file) is funneled throughparseDaemonPidFile, and unrecognized content becomesunverifiable, never an unrecognized verdict. No runtime producer can hand this switch an unhandled value today.
Ruling: not a regression — protection moved from tsc to a repo-wide error-level CI lint. Residual risk worth one line of hardening (P2): the guarded direction is inverted for a destructive site — if that lint rule is ever relaxed, the unprotected fall-through deletes. if (verdict.status !== 'exited') continue expresses the same logic with no switch, fails safe (new statuses preserve) under every toolchain, and drops 4 lines.
4. Sibling-PR interactions (#16908 / #16924 / #16932 — all still open)
- Fix Windows daemon host pruning on unverifiable liveness #16908: inserts the byte-identical
ProcessLivenessVerdictinto the same location ofdaemon-incarnation-evidence-types.ts, and addsinspectProcessLiveness(pid)indaemon-process-inspection.tswhose body is verbatim the signal→verdict switch this PR inlines at daemon-init.ts:1361-1370 (identicalreasonstring included).git merge-treeof the two heads: clean, no conflicts, single copy of the type (treec67d2145…). No ordering constraint — but whichever lands second should replace daemon-init's inline switch with the sharedinspectProcessLiveness(reuse-before-reimplementing; also removes one of the two exhaustiveness surfaces). Fix Windows daemon host pruning on unverifiable liveness #16908's corrupt-record quarantine is also the finished shape for F4 below. - fix(daemon): make pid-record publish atomic via claim-then-link #16924: touches only publish-claim scratch (
.claim-*patterns; its own tests pin "records, tokens, and swap/hold claims stay"). Disjoint from this PR's canonical legacy pid/token unlinks; current protocol is 36, this loop covers 1–35. No interaction, no ordering constraint. - Never read an unanswerable hasPty probe as proven PTY absence #16932: disjoint files entirely (
orca-runtime.ts,pty-liveness-verdict.ts). Philosophically aligned (null→unverifiable). No interaction.
5. Consumer derivation (who observes what changed)
legacyDaemonProcessMayBeAlive was module-private with one caller; its replacement likewise. createLegacyDaemonAdapters has exactly one caller (daemon-init.ts:915). The observable change is preserved-vs-deleted legacy files, whose readers are all type-check-blind to this PR — each verified:
collectPinnedDaemonVersions(daemon-host-relocation.ts:309) scans alldaemon-v\d+.pidfiles → could pin Windows host dirs against pruning. Verified benign: every newly-preserved case (unreadable / unparseable / kill-unavailable) fails its own read→parse→isDaemonPidAlivegates identically, so no new pins; the parseable-alive case preserved files pre-fix too.killStaleDaemonlegacy invocation (daemon-init.ts:1306, explicitprotocolVersion) — runs only on explicit preserved-handle shutdown; same read/parse gates fail for the newly-preserved cases, so no new PowerShell identity spawns are reachable from preservation.getDaemonLaunchIdentity/isDaemonStaleForCurrentBundle/ TCC attribution (daemon-init.ts:434-471): current protocol only.- Wire/renderer/mobile: none — no RPC, stream, persisted-schema, or preload surface is touched (repo-wide grep), so there is no wire claim to prove against a shipped release; the only persisted artifact involved is the pre-existing pid-file format, read-only and unchanged.
6. Non-blocking findings (P2)
- F1 (tests, requested pre-merge): pin ablation arms A3/A4/A5 (§1).
- F2 (hardening): invert the deletion gate to
if (verdict.status !== 'exited') continueso the destructive action is opt-in rather than lint-guarded fall-through (§3). - F3 (observability): every unverifiable preserve is silent, and
unverifiable.reasonis computed but never consumed. The sibling standard (Fix Windows daemon host pruning on unverifiable liveness #16908) warns on every unverifiable prune-skip; oneconsole.warnwith the reason at thecontinuewould make field diagnosis of "why do these v12 files never go away" possible. - F4 (eternal preservation): a parse-null legacy record is now preserved forever with no healing path — nothing ever rewrites or reclaims a retired-protocol record. Verified benign (no pid → no identity-check or pinning cost; harm is stray files), but the finished shape is Fix Windows daemon host pruning on unverifiable liveness #16908's quarantine-to-
.corruptgated on a salvaged-pid ESRCH probe; natural follow-up once both land. - F5 (evidence hygiene):
inspectProcessSignallacks thepid <= 0guard its siblingqueryWindowsProcess(same file, :76-78) already has;kill(0,0)/kill(-1,0)probe the caller's own process group and reportoccupied— a false "live" from a torn/hostile record (empty pid file parses to pid 0). Direction is preserve, behavior matches pre-fix, but the label is wrong evidence; guard →unavailable. Pre-existing module, in scope only because this PR adds the first consumer fed by a persisted file. - F6 (post-merge dedup): replace the inline signal→verdict switch with Fix Windows daemon host pruning on unverifiable liveness #16908's
inspectProcessLivenessonce both are in (§4).
7. Performance (by hand; the $perf skill is disable-model-invocation for dispatched workers)
Once per startup, ≤35 legacy protocols: the verdict path performs the same one read + one parse + one kill(0) the pre-fix code did — zero new syscalls, no polling, no subprocess (no PowerShell reachable here), no retained state. The EACCES route is now cheaper (skips 2 unlink attempts). Steady-state cost of eternally-preserved records: ≤35 × (open+parse+kill) synchronous µs-scale calls per startup vs. one failed open pre-fix — negligible. Budget claim in the PR body holds.
8. Superseding bar
From scratch we would write: the same tri-state verdict with ESRCH-only deletion (this PR's core is the right design and matches the boundary doc's reference implementation), but calling the shared mapper (F6), gating deletion with !== 'exited' (F2), warning on preserve (F3), guarding non-positive pids (F5), and pinning every arm (F1). None of that changes the merge calculus except F1; the rest are follow-ups.
Review environment note: lint verification here used direct oxlint --type-aware with config/oxlint-code-quality-type-aware.json, matching the audit:code-quality:type-aware step pr.yml runs.
Review response — head
|
| # | Arm mutated back toward coercion | Result | Killed by |
|---|---|---|---|
| A1 | gate: unverifiable authorizes deletion |
RED (3 failed) | EACCES + unparseable + unavailable tests |
| A2 | read-failure catch → exited |
RED | EACCES test |
| A3 | unparseable record → exited |
RED | new unparseable test |
| A4 | signal unavailable → exited |
RED | new unavailable test |
| A5 | permission_denied → exited |
RED | new EPERM test |
| A6 | occupied → exited |
RED | pre-existing alive test |
| A7 | missing → live |
RED | pre-existing ESRCH test |
| A8 | new fail-safe guard reverted to deny-list (old fall-through semantics) | RED | new hypothetical-verdict test |
All 8 arms now go red in isolation (was 4/7).
Blocker 2 — deletion gate no longer lint-only, fails safe by construction
The reviewer refuted the exhaustiveness premise for the statement switch at the deletion gate: pnpm tc:node passes with a 4th verdict status, and the fall-through direction was the unlink loop. Fixed by inverting the gate: the unlink loop moved behind reclaimLegacyDaemonOwnershipFiles(verdict, stalePaths), which requires a positively matched 'exited' — any unhandled future verdict status returns early and preserves ownership. No default arm exists anywhere, the lint rule is untouched (switch-exhaustiveness-check still at error; re-verified live on this config by injecting a non-exhaustive switch → it flags and exits 1), and the producer switch keeps its TS2366 + lint double protection. The new test pins the fail-safe direction by feeding a deliberately out-of-contract { status: 'suspended' } verdict and asserting zero unlinks (ablation A8 proves it kills a reversion to the old semantics).
Verification at this SHA
pnpm test src/main/daemon— 146 files, 1679 tests passpnpm tc:nodeclean;oxlintclean;oxlint --type-aware --config config/oxlint-code-quality-type-aware.json src/main/daemon --deny-warningsexit 0- Push verified via
git ls-remote: branch head is3492fc859598aaa2eaef5908b881eae81000e969
Follow-up (noted in the PR body, not acted on here): #16908 carries a byte-identical verdict type plus inspectProcessLiveness duplicating this PR's inline switch — whichever lands second should dedup.
The prune switch shared #16900's delete-gate shape: an unhandled future verdict status fell through into rmSync, protected only by the lint exhaustiveness rule. Deletion is now opted into by a positively matched 'exited' via reclaimUnownedDaemonHostDir; a pinning test feeds an out-of-contract verdict and asserts the host dir survives. Pid salvage from corrupt records now requires the digit run to be terminated by a following non-digit byte. A tear inside the digits leaves a truncated prefix that is a different pid: probing it either quarantined a record on an unrelated process's death or, when the prefix collided with an immortal pid (Windows System pid 4), re-created the permanent prune veto for that record. Unterminated digits mean the writer died mid-write, so the record quarantines without consulting any probe.
The monitor entered the completion path on children !== 'live' after the unverifiable early-return — a fall-through, not a positive 'exited' match (the polarity shape fixed in #16900/#16908). A fourth verdict arm admitted by a future evidence funnel would have silently read as exit evidence. Completion now requires children === 'exited'; anything outside the known vocabulary re-arms like a failed inspection. Pinned by an out-of-contract monitor test (fails on the previous head) and a funnel-level normalization unit test that keeps the normalize hunk independently covered.
The monitor entered the completion path on children !== 'live' after the unverifiable early-return — a fall-through, not a positive 'exited' match (the polarity shape fixed in #16900/#16908). A fourth verdict arm admitted by a future evidence funnel would have silently read as exit evidence. Completion now requires children === 'exited'; anything outside the known vocabulary re-arms like a failed inspection. Pinned by an out-of-contract monitor test (fails on the previous head) and a funnel-level normalization unit test that keeps the normalize hunk independently covered.
…cabulary guard The two hardening arms were mutually redundant in coverage: reverting either alone left the suite green. The out-of-vocabulary guard is now pinned on its own; this pins the other half. Given the vocabulary guard, every sample reaching the exit gate already carries 'live' or 'exited'. On a host that builds its wire result with buildPtyProcessInspectionWireResult the legacy `hasChildProcesses` boolean is exactly `children.verdict === 'live'`, so reading the boolean and requiring a positive 'exited' agree — the arm is invisible there. They part on the shape the reader is written to survive: a peer this client cannot vouch for, whose two fields disagree. readPtyProcessInspectionEvidence never cross-checks the legacy scalar against the evidence union, so `hasChildProcesses: false` can arrive beside a positively observed `children: 'live'`. That boolean's `false` is the lossy collapse conflating "no children" with "could not tell" (#16900/#16908). The evidence union is authoritative, so 'live' must refuse even though the legacy field waves it through. Reverting the positive-exited arm alone turns two of these red with a false `process-exit` completion; reverting the vocabulary guard alone leaves all three green.
Review loop — patched, not supersededHead reviewed: Direct answer — can an unverifiable or unavailable liveness answer still reach the The fix is structural, confirmed
The TOCTOU compare-gate's premise also checks out: Two problems found and fixed1. Four gates were not individually pinned — the same weakness found in #16926. The 2. A record naming no process was reported Ablation table — every gate reverted individually
A6 is structurally unpinnable and left as-is. Evidence
Not verifiedNo Windows host was available, so the Windows leg is unvalidated. Dev-mode CDP is blind to packaged-build regressions. |
…eld contradicts The exit gate used to read `if (result.hasChildProcesses)` and refuse on `true`. Moving it onto the evidence union dropped the legacy field, so the gate now refuses only on a children verdict other than 'exited'. That is more conservative in one direction — the direction the positive-exited arm pins — and less conservative in its mirror: `hasChildProcesses: true` beside `children: 'exited'` is a shape the boolean-only line refused and the verdict-only gate dispatches a process-exit completion for. Only the boolean's `false` pole is the lossy collapse this batch removes; it conflates "no children" with "could not ask". Its `true` pole is a positive observation on every producer — buildPtyProcessInspectionWireResult sets it from `children.verdict === 'live'`, processHasChildren from a pgrep that exited 0 with output, and the daemon host from an observed non-shell foreground. Dropping a positive liveness observation to honour a disagreeing 'exited' is the #16900/#16908 polarity bug pointing the other way, so the gate now refuses whenever either half reports live children. No conforming host is affected: readPtyProcessInspectionEvidence's legacy fallback maps `true` to 'live', which the verdict term already refuses, and the wire builder never emits the disagreeing pair. Only a peer this client cannot vouch for can, which is the same premise the mirror arm is pinned on. Pinned by three monitor tests through the real reader with no mock (all three fail on the previous head, each with a false process-exit completion). The four-way ablation keeps the arms independent: reverting only this term reddens its three tests and leaves the vocabulary-guard and positive-exited pins green.
The mirror of the children-axis rule, one axis over. Recognition used to read
`recognizeAgentProcess(result.foregroundProcess)`. Moving it onto the evidence
union made it read `evidence.foreground.processName`, so a sample whose union
reports `{observed, processName: null}` beside a legacy `foregroundProcess:
'codex'` no longer recognizes an agent. Its foreground verdict is still inside
the vocabulary, so the out-of-vocabulary guard cannot refuse it either, and the
sample falls through to the exit gate and dispatches a process-exit completion
for an agent the host still names in the foreground.
Only the `null` pole of the legacy scalar is the lossy collapse this batch
removes; it conflates "no agent in the foreground" with "could not ask". A
recognized non-null name is a positive observation on every producer:
buildPtyProcessInspectionWireResult copies it straight off an `observed`
foreground verdict, and the ps/pgrep probes only set it from a command line the
host actually read back. Discarding a positive liveness observation to honour a
union reporting no name is the #16900/#16908 polarity bug on the foreground
axis, so the gate now refuses whenever either half still names an agent.
The refusal is added at the exit gate rather than by widening recognition, so
the arm can only ever withhold a dispatch, never introduce one: reading the
legacy name into handleRecognizedProcess would fire a process-replacement
completion on a disagreeing name, which is less conservative, not more.
No conforming host is affected. To reach the gate the foreground verdict is
already 'observed', so buildPtyProcessInspectionWireResult's legacy scalar is
byte-identical to the union's processName the gate already found unrecognized,
and a host predating the field gets that same scalar through the reader's legacy
fallback. Only a peer this client cannot vouch for can disagree, the same
premise the children-axis arm is pinned on.
Pinned by four monitor tests through the real reader with no mock; all four fail
on the previous head, each with an actual false process-exit completion for
'codex'. The four-way ablation keeps the arms independent: reverting only this
term reddens its four tests and leaves the vocabulary-guard, positive-exited and
legacy-live-children pins green.
* Fix Windows daemon host prune liveness contract * Scope host prune evidence per version * Drop redundant default cases from exhaustive liveness switches All three switches consume ProcessLivenessVerdict/ProcessSignalEvidence values constructed in-process by inspectProcessSignal/inspectProcessLiveness; the union is never deserialized from a wire, RPC, or persisted record, so the defaults are genuinely unreachable. * Cover prune liveness gates and quarantine corrupt pid records * Make the prune delete-gate fail safe and refuse truncated pid salvage The prune switch shared #16900's delete-gate shape: an unhandled future verdict status fell through into rmSync, protected only by the lint exhaustiveness rule. Deletion is now opted into by a positively matched 'exited' via reclaimUnownedDaemonHostDir; a pinning test feeds an out-of-contract verdict and asserts the host dir survives. Pid salvage from corrupt records now requires the digit run to be terminated by a following non-digit byte. A tear inside the digits leaves a truncated prefix that is a different pid: probing it either quarantined a record on an unrelated process's death or, when the prefix collided with an immortal pid (Windows System pid 4), re-created the permanent prune veto for that record. Unterminated digits mean the writer died mid-write, so the record quarantines without consulting any probe. * fix(daemon): stop an in-flight pid publish from being read as a dead version publishDaemonPidFile creates the record before writing it (writeFileSync with flag 'wx'), so a concurrent launch can read a live daemon's record as empty. A two-process probe observed the empty window on 7 of 2273 reads. An empty record was not treated as corrupt at all: the parser's legacy bare-integer fallback coerces it to pid 0 (Number('') === 0) with appVersion null, so the scan skipped it as a pre-relocation daemon, left its version unpinned, and the prune reclaimed a running daemon's host image -- the exact destructive outcome this change exists to prevent, reached without any 'unverifiable' verdict. A pid that is not a positive integer names no process (process.kill(0, 0) probes the caller's own process group), so it is now a veto rather than a skip. Quarantine additionally refuses any record written in the last minute: an in-flight publish is by definition fresh, while a record left corrupt by a dead writer ages past the floor and is quarantined on a later launch. Fixed locally rather than in parseDaemonPidFile, whose null result also drives an unlink in daemon-stale-kill. Each gate is pinned by a test that fails individually when it is reverted. --------- Co-authored-by: Brennan Benson <brennanb2025@users.noreply.github.com>
`native-smoke (windows-latest)` fails ~4.5% of runs on `preserves a multiline argument through the compiled remote launcher` with "Test timed out in 15000ms" — on unrelated PRs, for reasons that have nothing to do with them. Across 176 sampled attempts it is the only red that job produced, and it hit seven different PRs in two days: #16900, #16904, #16915, #16955 (twice), #16979, #17014, #17085. The test is six process creations: powershell.exe forks csc.exe, then the freshly compiled orca.exe forks node.exe, twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job. Comparing the 80 attempts where it ran under 3s against the 12 where it ran over 12s, its own median goes 2198ms -> 15917ms (7.2x) while the same file's powershell-only test moves 556 -> 686ms (1.2x), the cmd.exe and Git Bash process tests in the neighbouring file move 1.4x, and the other 35 files put together move 1.5x. Measured across those 176 attempts: 1881ms to 35438ms, p50 4264ms, correlation +0.881 with the job's total Vitest duration. 8 of 176 (4.5%) exceeded the 15s cap; 2 of 176 (1.1%) also exceeded the shared 30s testTimeout, so deleting the override and inheriting the config is not enough on its own. 60s clears all 176 with 1.7x headroom on the worst. This is slow, not hung. Every body here is synchronous spawnSync, so Vitest cannot interrupt one — the timer fires only after the body returns and the reported duration is real elapsed time. That is why a failure reads `× ... 22464ms` under `Test timed out in 15000ms`. The work finished; the stopwatch was short. Seven reruns at one identical head measured 2053 / 4680 / 5551 / 8732 / 13506 / 14868 / 21937ms — the last of those would have been red on code that had not changed. The 15s came from #8897, which raised this test off Vitest's built-in 5s default because the job then ran bare `pnpm vitest run`. #8909 landed 3h27m later and pointed the job at config/vitest.config.ts, which is the real fix for that. The constant stayed behind and has been the binding budget ever since.
`native-smoke (windows-latest)` fails ~4.5% of runs on `preserves a multiline argument through the compiled remote launcher` with "Test timed out in 15000ms" — on unrelated PRs, for reasons that have nothing to do with them. Across 176 sampled attempts it is the only red that job produced, and it hit seven different PRs in two days: #16900, #16904, #16915, #16955 (twice), #16979, #17014, #17085. The test is six process creations: powershell.exe forks csc.exe, then the freshly compiled orca.exe forks node.exe, twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job. Comparing the 80 attempts where it ran under 3s against the 12 where it ran over 12s, its own median goes 2198ms -> 15917ms (7.2x) while the same file's powershell-only test moves 556 -> 686ms (1.2x), the cmd.exe and Git Bash process tests in the neighbouring file move 1.4x, and the other 35 files put together move 1.5x. Measured across those 176 attempts: 1881ms to 35438ms, p50 4264ms, correlation +0.881 with the job's total Vitest duration. 8 of 176 (4.5%) exceeded the 15s cap; 2 of 176 (1.1%) also exceeded the shared 30s testTimeout, so deleting the override and inheriting the config is not enough on its own. 60s clears all 176 with 1.7x headroom on the worst. This is slow, not hung. Every body here is synchronous spawnSync, so Vitest cannot interrupt one — the timer fires only after the body returns and the reported duration is real elapsed time. That is why a failure reads `× ... 22464ms` under `Test timed out in 15000ms`. The work finished; the stopwatch was short. Seven reruns at one identical head measured 2053 / 4680 / 5551 / 8732 / 13506 / 14868 / 21937ms — the last of those would have been red on code that had not changed. The 15s came from #8897, which raised this test off Vitest's built-in 5s default because the job then ran bare `pnpm vitest run`. #8909 landed 3h27m later and pointed the job at config/vitest.config.ts, which is the real fix for that. The constant stayed behind and has been the binding budget ever since.
) * Fix Windows daemon host prune liveness contract * Scope host prune evidence per version * Drop redundant default cases from exhaustive liveness switches All three switches consume ProcessLivenessVerdict/ProcessSignalEvidence values constructed in-process by inspectProcessSignal/inspectProcessLiveness; the union is never deserialized from a wire, RPC, or persisted record, so the defaults are genuinely unreachable. * Cover prune liveness gates and quarantine corrupt pid records * Make the prune delete-gate fail safe and refuse truncated pid salvage The prune switch shared stablyai#16900's delete-gate shape: an unhandled future verdict status fell through into rmSync, protected only by the lint exhaustiveness rule. Deletion is now opted into by a positively matched 'exited' via reclaimUnownedDaemonHostDir; a pinning test feeds an out-of-contract verdict and asserts the host dir survives. Pid salvage from corrupt records now requires the digit run to be terminated by a following non-digit byte. A tear inside the digits leaves a truncated prefix that is a different pid: probing it either quarantined a record on an unrelated process's death or, when the prefix collided with an immortal pid (Windows System pid 4), re-created the permanent prune veto for that record. Unterminated digits mean the writer died mid-write, so the record quarantines without consulting any probe. * fix(daemon): stop an in-flight pid publish from being read as a dead version publishDaemonPidFile creates the record before writing it (writeFileSync with flag 'wx'), so a concurrent launch can read a live daemon's record as empty. A two-process probe observed the empty window on 7 of 2273 reads. An empty record was not treated as corrupt at all: the parser's legacy bare-integer fallback coerces it to pid 0 (Number('') === 0) with appVersion null, so the scan skipped it as a pre-relocation daemon, left its version unpinned, and the prune reclaimed a running daemon's host image -- the exact destructive outcome this change exists to prevent, reached without any 'unverifiable' verdict. A pid that is not a positive integer names no process (process.kill(0, 0) probes the caller's own process group), so it is now a veto rather than a skip. Quarantine additionally refuses any record written in the last minute: an in-flight publish is by definition fresh, while a record left corrupt by a dead writer ages past the floor and is quarantined on a later launch. Fixed locally rather than in parseDaemonPidFile, whose null result also drives an unlink in daemon-stale-kill. Each gate is pinned by a test that fails individually when it is reverted. --------- Co-authored-by: Brennan Benson <brennanb2025@users.noreply.github.com> (cherry picked from commit 7abdf03)
9231f9a to
039b428
Compare
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e948786-33b6-4d79-8241-0f281c1f4105
📒 Files selected for processing (2)
src/main/daemon/daemon-init-provider-installation.test.tssrc/main/daemon/daemon-legacy-adapters.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| }) | ||
|
|
||
| it('preserves ownership files for any verdict that is not positively exited', async () => { | ||
| const mod = await importFresh() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the unused mod bindings.
Lint and typecheck reject these declarations because no test reads mod. Keep the importFresh() call, but do not assign its result.
src/main/daemon/daemon-init-provider-installation.test.ts#L391-L391: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L423-L423: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L438-L438: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L454-L454: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L506-L506: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L519-L519: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L533-L533: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L545-L545: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L562-L562: replaceconst mod = await importFresh()withawait importFresh().src/main/daemon/daemon-init-provider-installation.test.ts#L579-L579: replaceconst mod = await importFresh()withawait importFresh().
Proposed fix
- const mod = await importFresh()
+ await importFresh()📝 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.
| const mod = await importFresh() | |
| await importFresh() |
🧰 Tools
🪛 GitHub Check: static analysis
[failure] 391-391: eslint(no-unused-vars)
src/main/daemon/daemon-init-provider-installation.test.ts:391:11: Variable 'mod' is declared but never used. Unused variables should start with a '_'.
🪛 GitHub Check: typecheck
[failure] 391-391:
'mod' is declared but its value is never read.
📍 Affects 1 file
src/main/daemon/daemon-init-provider-installation.test.ts#L391-L391(this comment)src/main/daemon/daemon-init-provider-installation.test.ts#L423-L423src/main/daemon/daemon-init-provider-installation.test.ts#L438-L438src/main/daemon/daemon-init-provider-installation.test.ts#L454-L454src/main/daemon/daemon-init-provider-installation.test.ts#L506-L506src/main/daemon/daemon-init-provider-installation.test.ts#L519-L519src/main/daemon/daemon-init-provider-installation.test.ts#L533-L533src/main/daemon/daemon-init-provider-installation.test.ts#L545-L545src/main/daemon/daemon-init-provider-installation.test.ts#L562-L562src/main/daemon/daemon-init-provider-installation.test.ts#L579-L579
Source: Linters/SAST tools
| return | ||
| } | ||
| try { | ||
| unlinkSync(pidPath) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Prevent the compare-to-unlink race.
A new daemon can replace the stale PID record after line 84 compares it and before line 88 unlinks it. This code then removes the new daemon’s PID record. The later token unlink can also remove its token. Serialize publication and reclamation with the same ownership protocol, or use an atomic claim-and-delete operation. Do not unlink a path after an unprotected content check.
As per coding guidelines, “No actor removes a name it did not create.”
Source: Coding guidelines
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return | ||
| } | ||
| try { | ||
| unlinkSync(tokenPath) |
There was a problem hiding this comment.
If a previous-version daemon republishes its PID and token after this cleanup unlinks the proven-dead PID, the unconditional token unlink removes the replacement daemon's credential. Clients then read an empty token and receive an Invalid token rejection, leaving the replacement daemon's PTYs unadoptable.
Knowledge Base Used:
`native-smoke (windows-latest)` fails ~4.5% of runs on `preserves a multiline argument through the compiled remote launcher` with "Test timed out in 15000ms" — on unrelated PRs, for reasons that have nothing to do with them. Across 176 sampled attempts it is the only red that job produced, and it hit seven different PRs in two days: #16900, #16904, #16915, #16955 (twice), #16979, #17014, #17085. The test is six process creations: powershell.exe forks csc.exe, then the freshly compiled orca.exe forks node.exe, twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job. Comparing the 80 attempts where it ran under 3s against the 12 where it ran over 12s, its own median goes 2198ms -> 15917ms (7.2x) while the same file's powershell-only test moves 556 -> 686ms (1.2x), the cmd.exe and Git Bash process tests in the neighbouring file move 1.4x, and the other 35 files put together move 1.5x. Measured across those 176 attempts: 1881ms to 35438ms, p50 4264ms, correlation +0.881 with the job's total Vitest duration. 8 of 176 (4.5%) exceeded the 15s cap; 2 of 176 (1.1%) also exceeded the shared 30s testTimeout, so deleting the override and inheriting the config is not enough on its own. 60s clears all 176 with 1.7x headroom on the worst. This is slow, not hung. Every body here is synchronous spawnSync, so Vitest cannot interrupt one — the timer fires only after the body returns and the reported duration is real elapsed time. That is why a failure reads `× ... 22464ms` under `Test timed out in 15000ms`. The work finished; the stopwatch was short. Seven reruns at one identical head measured 2053 / 4680 / 5551 / 8732 / 13506 / 14868 / 21937ms — the last of those would have been red on code that had not changed. The 15s came from #8897, which raised this test off Vitest's built-in 5s default because the job then ran bare `pnpm vitest run`. #8909 landed 3h27m later and pointed the job at config/vitest.config.ts, which is the real fix for that. The constant stayed behind and has been the binding budget ever since.
* fix(ssh): fence stale kills and retired pane replay * fix(ssh): support cancellable interactive authentication * fix(ssh): await remote catalog before snapshot adoption * fix(pty): contain Windows ConPTY input failures * fix(power): avoid redundant macOS display blocking * perf(editor): narrow markdown override subscriptions * fix(quick-open): close directory handles after reads * refactor(linux): remove unused proc socket scanner * fix(usage): apply flat Sonnet 4.6 pricing * ci: prime Node next native test cache * docs(skills): resolve snapshot cleanup data path * fix(ssh): recover install locks after host reboot * test(ssh): recognize boot-aware install locks * test(ssh): prove previous-boot lock recovery live * test(wire): pin pre-metadata release coverage * fix(terminal): preserve remote tab ownership through recovery races * test(runtime): fence replaced terminal handles in agent guard * fix(ssh): preserve remote snapshot authority across polls * fix(pty): contain late ConPTY output EPIPE * test(pty): register Windows exit watcher before kill * fix: close SSH and tab readiness race gaps * fix(tabs): retain headless order and placeholder titles * fix(build): avoid parallel electron-vite config race * test(windows): avoid MSYS temp path rewriting * test(windows): avoid killing exited PTY * fix(pty): avoid late ConPTY input teardown race * fix(terminal): sync reconnect error ownership after commit * fix(runtime): use canonical worktree identity comparison * test(ssh): assert complete cold-hydration baseline * test(windows): invoke quoted retention fixture via PowerShell * test(windows): read ConPTY grid through mode con * fix(terminal): publish PTY replacements atomically * fix(terminal): infer stale identity on reattach * fix(terminal): fence stale pane PTY callbacks * fix(terminal): fence stale pane binds after rebind * fix(terminal): reject stale pane transport callbacks * fix(terminal): fence mirrored reattach spawn callbacks * fix(terminal): replace stale pane PTYs on remount * fix(ci): size the Windows launcher-compile test budget from measurement `native-smoke (windows-latest)` fails ~4.5% of runs on `preserves a multiline argument through the compiled remote launcher` with "Test timed out in 15000ms" — on unrelated PRs, for reasons that have nothing to do with them. Across 176 sampled attempts it is the only red that job produced, and it hit seven different PRs in two days: #16900, #16904, #16915, #16955 (twice), #16979, #17014, #17085. The test is six process creations: powershell.exe forks csc.exe, then the freshly compiled orca.exe forks node.exe, twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job. Comparing the 80 attempts where it ran under 3s against the 12 where it ran over 12s, its own median goes 2198ms -> 15917ms (7.2x) while the same file's powershell-only test moves 556 -> 686ms (1.2x), the cmd.exe and Git Bash process tests in the neighbouring file move 1.4x, and the other 35 files put together move 1.5x. Measured across those 176 attempts: 1881ms to 35438ms, p50 4264ms, correlation +0.881 with the job's total Vitest duration. 8 of 176 (4.5%) exceeded the 15s cap; 2 of 176 (1.1%) also exceeded the shared 30s testTimeout, so deleting the override and inheriting the config is not enough on its own. 60s clears all 176 with 1.7x headroom on the worst. This is slow, not hung. Every body here is synchronous spawnSync, so Vitest cannot interrupt one — the timer fires only after the body returns and the reported duration is real elapsed time. That is why a failure reads `× ... 22464ms` under `Test timed out in 15000ms`. The work finished; the stopwatch was short. Seven reruns at one identical head measured 2053 / 4680 / 5551 / 8732 / 13506 / 14868 / 21937ms — the last of those would have been red on code that had not changed. The 15s came from #8897, which raised this test off Vitest's built-in 5s default because the job then ran bare `pnpm vitest run`. #8909 landed 3h27m later and pointed the job at config/vitest.config.ts, which is the real fix for that. The constant stayed behind and has been the binding budget ever since. * fix(terminal): fence stale remount reattach ownership * fix(terminal): reconcile mounted pane identity after replacement * fix(terminal): fence stale reattach fallback ownership * fix(terminal): fence deferred SSH reattach ownership * fix(terminal): fence stale split pane ownership callbacks * fix(terminal): keep stale spawns from consuming startup --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
Reuse the shipped inspectProcessLiveness instead of a local signal switch, and remove the dead daemon's pid record through the existing rename-claimed unlinkOwnedDaemonPidFile so a record republished in the meantime is left alone. Tests move to real files in a temp runtime dir.
…ases The real-file suite covers them, and with the pid record now removed through the mocked fenced unlink their pid assertions could not fail.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7d4d0d87-2117-40a9-b10a-de022992928e
📒 Files selected for processing (4)
src/main/daemon/daemon-init-provider-installation.test.tssrc/main/daemon/daemon-init-test-harness.tssrc/main/daemon/daemon-legacy-adapters.test.tssrc/main/daemon/daemon-legacy-adapters.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| return | ||
| } | ||
| try { | ||
| unlinkSync(tokenPath) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Preserve a replacement daemon’s token.
If another daemon publishes a token after the old PID file is removed, unlinkSync(tokenPath) deletes the new token. The PID-file ownership check does not protect tokenPath. Claim and check the old token before removing it, or coordinate publication and cleanup across both files. The earlier review identified this token-removal race; the PID-file fence does not close it.
Review statusThe problem. When you upgrade Orca while terminals from the previous version are still running, the new version checks the old background terminal service at startup. If that check could not tell whether the old service was alive (its record unreadable, a permission error, the connection refused), Orca treated "can't tell" as "dead" and deleted the credentials needed to reach it. The old service kept running with your terminals, and the agents in them, but Orca could never reconnect to it again, not even from Settings > Manage Sessions. What you see differently. Orca now deletes those files only after proving the old service has exited. If it can't tell, it leaves them alone, and the next start reconnects and lists those sessions again. How. It uses the one existing process check ( Fixed in review: rebased onto the shipped liveness check; replaced the hand-rolled "same record" compare with the existing fenced unlink; moved the tests to real files in a temp folder, where 9 fail against main's code and the republish test fails without the fence; removed two duplicate startup-harness tests. Deferred: files main used to delete are now kept when an exit isn't proven (at most 35 old versions × 2 small, unused files); the token delete itself is not fenced (racing it needs an older Orca running on the same profile at the same moment); no test yet for the oldest record formats (bare integer, no launch id). Verified: 1 review round with no P0–P2 issues; readiness checklist passed; tc:node clean; targeted tests green. Live on a Mac with main vs this PR: a real previous-version service held a terminal, and the upgraded Orca started while its socket and record were unreadable. On main, the token and record were deleted and the next start no longer listed the old session, although it was still running ("Invalid token"). On this PR, both files were kept and the next start listed the old session again.
Not verified: a real EPERM from the process check (can't be produced for your own processes without admin rights; covered by unit tests), the proven-exit cleanup path live (unit tests only), and Windows/Linux/SSH. Known, not caused by this PR: when an old version was unreachable at startup, the tab may have started a fresh shell under the same session id. So once the old version is reachable again, Manage Sessions can show two identical rows, and Kill currently ends the FIRST session with that id, which may be the other one (the second screenshot shows the two rows). main already shows this in its most common case; on the case this PR fixes, main instead hides the old session permanently. The follow-up fixes both halves: it keeps an unreachable old version in the session list as "unverifiable", so no duplicate is created, and it makes Kill target the clicked row's exact identity (helper version and incarnation), routed through the session resolver. CI: all 33 checks pass at Part of STA-2018. This PR is step 1 of 4:
|
) * Fix Windows daemon host prune liveness contract * Scope host prune evidence per version * Drop redundant default cases from exhaustive liveness switches All three switches consume ProcessLivenessVerdict/ProcessSignalEvidence values constructed in-process by inspectProcessSignal/inspectProcessLiveness; the union is never deserialized from a wire, RPC, or persisted record, so the defaults are genuinely unreachable. * Cover prune liveness gates and quarantine corrupt pid records * Make the prune delete-gate fail safe and refuse truncated pid salvage The prune switch shared stablyai#16900's delete-gate shape: an unhandled future verdict status fell through into rmSync, protected only by the lint exhaustiveness rule. Deletion is now opted into by a positively matched 'exited' via reclaimUnownedDaemonHostDir; a pinning test feeds an out-of-contract verdict and asserts the host dir survives. Pid salvage from corrupt records now requires the digit run to be terminated by a following non-digit byte. A tear inside the digits leaves a truncated prefix that is a different pid: probing it either quarantined a record on an unrelated process's death or, when the prefix collided with an immortal pid (Windows System pid 4), re-created the permanent prune veto for that record. Unterminated digits mean the writer died mid-write, so the record quarantines without consulting any probe. * fix(daemon): stop an in-flight pid publish from being read as a dead version publishDaemonPidFile creates the record before writing it (writeFileSync with flag 'wx'), so a concurrent launch can read a live daemon's record as empty. A two-process probe observed the empty window on 7 of 2273 reads. An empty record was not treated as corrupt at all: the parser's legacy bare-integer fallback coerces it to pid 0 (Number('') === 0) with appVersion null, so the scan skipped it as a pre-relocation daemon, left its version unpinned, and the prune reclaimed a running daemon's host image -- the exact destructive outcome this change exists to prevent, reached without any 'unverifiable' verdict. A pid that is not a positive integer names no process (process.kill(0, 0) probes the caller's own process group), so it is now a veto rather than a skip. Quarantine additionally refuses any record written in the last minute: an in-flight publish is by definition fresh, while a record left corrupt by a dead writer ages past the floor and is quarantined on a later launch. Fixed locally rather than in parseDaemonPidFile, whose null result also drives an unlink in daemon-stale-kill. Each gate is pinned by a test that fails individually when it is reverted. --------- Co-authored-by: Brennan Benson <brennanb2025@users.noreply.github.com>
* fix(ssh): fence stale kills and retired pane replay * fix(ssh): support cancellable interactive authentication * fix(ssh): await remote catalog before snapshot adoption * fix(pty): contain Windows ConPTY input failures * fix(power): avoid redundant macOS display blocking * perf(editor): narrow markdown override subscriptions * fix(quick-open): close directory handles after reads * refactor(linux): remove unused proc socket scanner * fix(usage): apply flat Sonnet 4.6 pricing * ci: prime Node next native test cache * docs(skills): resolve snapshot cleanup data path * fix(ssh): recover install locks after host reboot * test(ssh): recognize boot-aware install locks * test(ssh): prove previous-boot lock recovery live * test(wire): pin pre-metadata release coverage * fix(terminal): preserve remote tab ownership through recovery races * test(runtime): fence replaced terminal handles in agent guard * fix(ssh): preserve remote snapshot authority across polls * fix(pty): contain late ConPTY output EPIPE * test(pty): register Windows exit watcher before kill * fix: close SSH and tab readiness race gaps * fix(tabs): retain headless order and placeholder titles * fix(build): avoid parallel electron-vite config race * test(windows): avoid MSYS temp path rewriting * test(windows): avoid killing exited PTY * fix(pty): avoid late ConPTY input teardown race * fix(terminal): sync reconnect error ownership after commit * fix(runtime): use canonical worktree identity comparison * test(ssh): assert complete cold-hydration baseline * test(windows): invoke quoted retention fixture via PowerShell * test(windows): read ConPTY grid through mode con * fix(terminal): publish PTY replacements atomically * fix(terminal): infer stale identity on reattach * fix(terminal): fence stale pane PTY callbacks * fix(terminal): fence stale pane binds after rebind * fix(terminal): reject stale pane transport callbacks * fix(terminal): fence mirrored reattach spawn callbacks * fix(terminal): replace stale pane PTYs on remount * fix(ci): size the Windows launcher-compile test budget from measurement `native-smoke (windows-latest)` fails ~4.5% of runs on `preserves a multiline argument through the compiled remote launcher` with "Test timed out in 15000ms" — on unrelated PRs, for reasons that have nothing to do with them. Across 176 sampled attempts it is the only red that job produced, and it hit seven different PRs in two days: stablyai#16900, stablyai#16904, stablyai#16915, stablyai#16955 (twice), stablyai#16979, stablyai#17014, stablyai#17085. The test is six process creations: powershell.exe forks csc.exe, then the freshly compiled orca.exe forks node.exe, twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job. Comparing the 80 attempts where it ran under 3s against the 12 where it ran over 12s, its own median goes 2198ms -> 15917ms (7.2x) while the same file's powershell-only test moves 556 -> 686ms (1.2x), the cmd.exe and Git Bash process tests in the neighbouring file move 1.4x, and the other 35 files put together move 1.5x. Measured across those 176 attempts: 1881ms to 35438ms, p50 4264ms, correlation +0.881 with the job's total Vitest duration. 8 of 176 (4.5%) exceeded the 15s cap; 2 of 176 (1.1%) also exceeded the shared 30s testTimeout, so deleting the override and inheriting the config is not enough on its own. 60s clears all 176 with 1.7x headroom on the worst. This is slow, not hung. Every body here is synchronous spawnSync, so Vitest cannot interrupt one — the timer fires only after the body returns and the reported duration is real elapsed time. That is why a failure reads `× ... 22464ms` under `Test timed out in 15000ms`. The work finished; the stopwatch was short. Seven reruns at one identical head measured 2053 / 4680 / 5551 / 8732 / 13506 / 14868 / 21937ms — the last of those would have been red on code that had not changed. The 15s came from stablyai#8897, which raised this test off Vitest's built-in 5s default because the job then ran bare `pnpm vitest run`. stablyai#8909 landed 3h27m later and pointed the job at config/vitest.config.ts, which is the real fix for that. The constant stayed behind and has been the binding budget ever since. * fix(terminal): fence stale remount reattach ownership * fix(terminal): reconcile mounted pane identity after replacement * fix(terminal): fence stale reattach fallback ownership * fix(terminal): fence deferred SSH reattach ownership * fix(terminal): fence stale split pane ownership callbacks * fix(terminal): keep stale spawns from consuming startup --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
ELI5
Orca runs your terminals inside a background helper process. When you upgrade Orca, the new version checks on helpers left over from the previous version and tidies away the files of helpers that are really gone. The bug: if Orca could not tell whether an old helper was still running (its record was unreadable, or the OS refused to answer), it treated "I don't know" as "it's dead" and deleted the credentials needed to reach it. The old helper kept running with your terminals, and any agents in them, but Orca could never reconnect to it again. Those sessions vanished from Settings > Manage Sessions for good, with nothing to click to get them back.
Now Orca deletes those files only after it has proved the old helper exited. "I can't tell" means "leave it alone", so the next start reconnects and the sessions show up again.
What Changed
Where it bites: you upgrade Orca to a version that talks to its helper differently, while terminals from the previous version are still running. At startup the quick connection check on the old helper fails (for example on a busy machine), and the follow-up check on its process can't give an answer: the record is unreadable or half-written, or the OS returns "permission denied". This affects every platform. It does not affect SSH or WSL terminals, the current version's helper, or anything else when the old helper really has exited.
Mechanism (
src/main/daemon/daemon-legacy-adapters.ts):inspectProcessLiveness. Only "no such process" counts as exited; "permission denied" counts as alive; anything else, or a record that can't be read or doesn't name a real process, means "can't tell".unlinkOwnedDaemonPidFile. That function first claims the file by renaming it, then checks that it still names the same process and launch before deleting, so a record republished in between is restored untouched.Why
This is less code than a private liveness switch plus a hand-written "re-read and compare" check. It reuses the two mechanisms the rest of the helper code already relies on: one liveness check instead of three, and the same fenced delete the stale-helper cleanup uses. That fenced delete also closes the gap a re-read-then-delete check leaves between its compare and its delete.
Accepted trade-offs:
Known, not caused by this PR: once an old helper is reachable again, Manage Sessions can show two identical rows. While the old version was unreachable at startup, the tab may have started a fresh shell under the same session id on the new helper. Kill currently ends the first session with that id, which may be the other one. main already shows the same two rows in its most common case (the connection check times out but the process check succeeds, so main keeps the files too). On the case this PR fixes, main instead hides the old session permanently. The follow-up fixes both halves: it keeps an unreachable old version in the session list as "unverifiable", so no duplicate is created, and it makes Kill target the clicked row's exact identity (helper version and incarnation), routed through the session resolver.
This PR is step 1 of 4 in the plan for old helper versions piling up (STA-2018):
Linked Issue
Part of STA-2018.
Visual Proof
Live on a Mac, main vs this PR. A real previous-version helper was holding a live terminal. The upgraded Orca started while that helper's socket and record were unreadable, then restarted with access restored.
main: the token and record were deleted, and the old session is gone from Manage Sessions, although its helper and shell are still running (the log shows "Invalid token"):
This PR: both files were kept, and the old session is listed again. The second, identical row is the known duplicate described above.
Testing
I manually tested these changes (live, macOS, main vs PR; details above)
Automated tests added/updated
New
src/main/daemon/daemon-legacy-adapters.test.tsdrives the real startup discovery against real files in a temporary folder. It covers: proven exit deletes both files; "permission denied", other errors and error-code-less failures keep both; an unreadable, empty or unparseable record keeps both and never probes pid 0; a record and token republished during the check are left alone.Against main's code, 9 of these fail. With the identity fence replaced by a plain delete, the republish test fails.
node node_modules/typescript/bin/tsc --noEmit -p config/tsconfig.node.jsonis clean. The targeted vitest files pass.Not shown live: a real "permission denied" from the process check (it can't be produced for your own processes without admin rights; unit tests cover it), the proven-exit cleanup path (unit tests), and Windows/Linux. The check is a signal-0 probe on every platform and spawns no process; on Windows it keeps the same meaning ("no such process" vs "access denied").
Review
Author: @BrennanKB5
Agent skill upstream boundary
Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)