Skip to content

fix(orcad): replace a stale terminal daemon once it drains on Linux and WSL hosts - #31235

Merged
nwparker merged 2 commits into
mainfrom
nwparker/orcad-recycle-idle-stale-daemon
Oct 20, 2026
Merged

nwparker merged 2 commits into
mainfrom
nwparker/orcad-recycle-idle-stale-daemon

Conversation

@nwparker

@nwparker nwparker commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

#31135 fixed the Orca server on SSH machines so files it creates are no longer private-only. The program that keeps your terminals alive (the terminal daemon) was usually started by the old server, so it still has the old "make everything private" setting. Every terminal it starts inherits that setting. Orca already swaps out an outdated daemon when the server starts, but only if no terminal is open at that moment. If you had a terminal open, the old daemon stayed. On Linux and WSL machines it then stayed forever, even after you closed every terminal. Now, once the old daemon has no terminals left, Orca swaps it out right before it starts your next terminal. It never touches a daemon that still has terminals running.

What Changed

Before: a remote orcad could replace an outdated terminal daemon in two places:

  • At startup (prepareDaemonReplacement, stale_bundle branch): only if the daemon had exactly zero live sessions. This already worked on every platform.
  • Before each new terminal (replaceStaleBundleDaemonBeforeNewPty): this is the path that catches a daemon that was busy at startup and has since drained. It was enabled only where resolvePackagedDarwinAppVersion() returned a version, which means macOS only.

So on Linux and WSL hosts, a daemon forked by a pre-#31135 orcad (umask 077) kept hosting every new terminal after the upgrade if any terminal was open when orcad restarted. Those terminals created owner-only files until the daemon happened to die.

After:

  • DaemonLaunchPolicy gets an optional recycleStaleDaemonOnEveryPlatform, and startOrcadDaemon sets it.
  • resolveStaleDaemonRecycleVersion(onEveryPlatform) replaces resolvePackagedDarwinAppVersion. It returns the app version on every platform when that flag is set, and keeps the darwin-only rule for the desktop.
  • The value is resolved once in initDaemonPtyProvider and stored on the DaemonSpawner. restartDaemon reuses that spawner, so the choice survives a restart.

The swap rules don't change. The existing replaceStaleBundleDaemonOnce swaps only when:

  • the PID record is verified and its appVersion differs from orcad's ORCA_VERSION (an unverifiable PID record counts as not stale), and
  • the daemon's own listSessions answers with zero alive sessions, and the adapter tracks none either.

If the session count can't be read, the daemon is kept, so an unverifiable state never leads to a swap. A daemon with any live session is kept. Nothing ever forces a restart, and there is no UI.

Why

  • Signal: version stamp, not umask probing. The daemon already reports which orcad version forked it: appVersion sits in its PID record and in the hello identity. Every orcad build that contains fix(orcad): run the remote daemon at the login umask so worktrees it creates are not owner-only #31135 has a newer version than every daemon forked before it, so "stale version" is a reliable superset of "pre-fix daemon". Reading /proc/<pid>/status Umask: would only work on Linux, could not run on macOS, and would add a second, parallel staleness rule. A version check covers every platform with the record the code already verifies.
  • Reuse, not a new path. The drain-then-replace logic, its unverifiable handling and its telemetry (stale_bundle) already exist and run on macOS desktops. This PR only lets orcad switch it on.
  • Scope kept to orcad. The darwin gate exists on the desktop because a stale desktop daemon breaks macOS privacy (TCC) attribution. Desktop Linux and Windows keep their current behaviour.
  • Side benefit: any version-stale orcad daemon is now refreshed once it is idle. That includes one still running code from a slot that version cleanup has since removed. On a Windows orcad host this is the same swap the startup check already does.
  • Platforms:

Linked Issue

Follow-up to #31135 (no separate issue).

Visual Proof

N/A: there is no UI. The change decides which daemon process hosts the next new terminal on a remote host.

Testing

  • Automated tests added/updated
  • orcad-daemon-supervision.test.ts: orcad's policy opts in, including on linux. 3 tests fail on main.
  • daemon-init-provider-installation.test.ts:
    • With the opt-in, the adapter and spawner get the version on every platform. This fails on main on any host.
    • Without packaging, the result stays null.
  • daemon-init-restart-sequence.test.ts: the opt-in survives restartDaemon. This fails on main on Linux CI.
  • The existing daemon-pty-adapter-daemon-recovery.test.ts already covers the swap itself: it swaps only at zero sessions and keeps the daemon when the count is unverifiable or positive.
  • pnpm test src/main/daemon src/main/orcad with one worker: 3621 passed. 23 tests fail locally in shell-ready, repro-13767 and orcad-forked-child-paths, and the same 23 fail with main's sources: they depend on the developer's real shell and on macOS /private path resolution.
  • The full local static-analysis job passes, including the orcad terminal smoke. The exception is the Node 18 process-host smoke: Node 18 isn't installed locally, and this PR doesn't touch that package.
  • Docker managed-orcad lane (e2e.yml dispatch, ssh on managed orcad (Docker)), covering terminal journeys, retained upgrade state, idle exit and terminal root owner: passed on 14f8382 (run 39502482172). Passed again on the final head 30cbc26 (run 39504705041).
  • Verified live on a real host: no. Reproducing it needs two orcad versions on one host with a terminal open across the upgrade. That is covered by the existing adapter tests and the Docker lane, not a manual run.

Review

  • Codex review (pinned base 3be5039): clean.
  • Independent Opus review: no P0 to P2 findings, and a re-check of the final head is also clean. P3 items:
    • Pinning the restart test to linux so it fails on base on every host: done.
    • Removing a redundant supervision test: done.
    • Changing the comment wording from "older" to "another version's": done.
    • Not changed, because each is already true on macOS today:
      • The adapter option is still named packagedAppVersion.
      • A stale daemon that is still busy logs a "preserving" warning on each new terminal.
      • If the crash-loop throttle is exhausted, the recycle can fail that terminal.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Wire compatibility: nothing on the wire changes. The check reads the PID record that already exists, and older daemons with no appVersion were already treated as stale.
  • Performance: the check runs only when a new terminal is created, and its result is cached by PID-record contents.

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)

…nd WSL hosts

A daemon forked by an older orcad (before #31135, at umask 077) kept
serving new terminals after an upgrade if it was busy at startup, because
the runtime stale-bundle recycle was macOS-only. orcad now opts in on every
platform; the replacement still waits for exactly zero live sessions and
treats an unverifiable count as keep.
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

The daemon launch policy now supports stale-daemon recycling on every platform. Packaged apps use their app version when this option is enabled or when running on macOS; unpackaged apps and packaged non-macOS apps without the option use no version. The resolved version is stored by the spawner and passed to initial and replacement adapters. Orcad startup enables the option, and tests cover policy resolution, restart behavior, and startup configuration.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 30cbc

The change is mergeable with a bounded test-coverage gap: the new tests may not catch a Linux opt-in regression when run on macOS.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 11 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: replacing stale terminal daemons after they drain on Linux and WSL hosts.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description is complete and follows the required structure. It clearly explains the user impact, implementation, rationale, testing, platform considerations, review status, and checklist results. …
Full details: Description check

Explanation

The description is complete and follows the required structure. It clearly explains the user impact, implementation, rationale, testing, platform considerations, review status, and checklist results. The Linked Issue section references #31135 as a follow-up rather than using a Fixes link, but this is a minor template deviation and does not prevent review.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

🧹 Nitpick comments: 1
src/main/daemon/daemon-init-restart-sequence.test.ts: 1

260-260: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Pin both opt-in tests to Linux before provider initialization.

initDaemonPtyProvider() resolves the recycle version during initialization. The restart test changes process.platform afterward, and the provider-installation test does not override it. On macOS, both tests can pass through the existing macOS behavior even if recycleStaleDaemonOnEveryPlatform is ignored.

Set Linux before initialization in both tests. Restore the original platform in a finally block that covers initialization and restart. This improves cross-platform coverage; it does not indicate a current production failure.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5b41582d-3ed6-4fce-aec6-6f6b4688d697
📥 Commits

Reviewing files that changed from the base of the PR and between 706ea95 and 30cbc26.

📒 Files selected for processing: 11
  • src/main/daemon/daemon-init-dependency-mocks.ts
  • src/main/daemon/daemon-init-mock-types.ts
  • src/main/daemon/daemon-init-provider-installation.test.ts
  • src/main/daemon/daemon-init-restart-sequence.test.ts
  • src/main/daemon/daemon-launch-paths.ts
  • src/main/daemon/daemon-out-of-process-launcher.ts
  • src/main/daemon/daemon-provider-init.ts
  • src/main/daemon/daemon-provider-restart.ts
  • src/main/daemon/daemon-spawner.ts
  • src/main/orcad/orcad-daemon-supervision.test.ts
  • src/main/orcad/orcad-daemon-supervision.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@nwparker
nwparker merged commit 9823949 into main Oct 20, 2026
65 checks passed
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.

1 participant