perf(daemon): reap idle legacy daemons and self-exit at zero sessions#7615
perf(daemon): reap idle legacy daemons and self-exit at zero sessions#7615brennanb2025 wants to merge 7 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds idle self-exit support for the daemon. A new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/daemon/daemon-init.ts (1)
777-782: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment exceeds the one-to-two-line guideline.
This new "Why" comment spans 6 lines. As per coding guidelines,
**/*.{ts,tsx,js,jsx}comments explaining non-obvious behavior should be "short — one or two lines, capturing only the non-obvious reason."✏️ Suggested trim
- // Why: an alive legacy daemon with zero live sessions has nothing left to - // preserve, but adoption would pin its node + node-pty processes forever — - // every protocol bump re-adopts it on the next launch. Reap it through the - // existing shutdown/cleanup path instead. A failed session-count query - // (null) fail-safes to adoption exactly as before, so a daemon whose - // sessions cannot be verified is never killed. + // Why: an idle alive legacy daemon would otherwise be re-adopted forever; + // a failed query fail-safes to adoption instead of killing an unverified daemon.Source: Coding guidelines
src/main/daemon/daemon-init.test.ts (1)
780-818: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: unused
requestMockreturn value.Since
ensureConnectedthrows beforerequestis ever called in this scenario, the{ sessions: [] }return value onrequestMockis dead — harmless but slightly misleading about what's actually being exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 52ff9718-5830-4f83-9f6e-1ff0eb9accf4
📒 Files selected for processing (10)
src/main/daemon/daemon-entry.tssrc/main/daemon/daemon-idle-exit.test.tssrc/main/daemon/daemon-idle-exit.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-main.test.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/terminal-host.ts
|
Priority: P1 — from the 2026-07-06 perf audit. Issue: the PTY daemon never exited at zero sessions, and alive legacy-protocol daemons (v1–v17) were re-adopted on every launch with no session-count check — so idle node + node-pty processes accumulate forever on long-lived installs (part of the machine-wide memory-sprawl finding: one audited machine sat at 99.6% RAM). This PR: reaps 0-session legacy daemons at adoption (fail-safe to adoption on query failure) and adds an unref'd 10-min self-exit at 0 sessions + 0 clients with fire-time re-check — a daemon holding any session is never touched. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/daemon/daemon-server.test.ts (1)
594-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest title claims "hello" refusal but only exercises
routeRequest.The test name says it "refuses hello and createOrAttach once shutdown has begun," but the body only calls
daemon.routeRequest(...)directly (which exercises theshuttingDownguard inrouteRequest, Line 335 of daemon-server.ts). It never sends a realhellomessage throughhandleConnection/handleFirstMessageto exercise theshuttingDowncheck at Line 205 of daemon-server.ts. If that path isn't covered elsewhere, this leaves the hello-refusal behavior added in this PR untested.♻️ Suggested addition to cover the hello path
it('refuses hello and createOrAttach once shutdown has begun', async () => { ... await server.shutdown() + + const rawSocket = connect(socketPath) + const rawClosed = new Promise<void>((resolve) => rawSocket.once('close', resolve)) + rawSocket.on('error', () => {}) + await rawClosed // socket is destroyed immediately since server is shutting down
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4962419-554c-4662-9ee2-2ba53473a89f
📒 Files selected for processing (4)
src/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/terminal-host.test.tssrc/main/daemon/terminal-host.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/daemon/terminal-host.ts
261f3da to
09c7bde
Compare
Legacy adoption now queries live-session count first: an alive legacy daemon reporting 0 live sessions is shut down through the existing cleanupDaemonForProtocol path instead of being adopted (and re-adopted forever on every protocol bump). A failed session-count query fail-safes to adoption exactly as before, so unverifiable daemons are never killed. The current-protocol daemon now arms a single unref'd 10-minute idle countdown whenever it holds 0 sessions and 0 connected clients, cancels it on any session creation or client connect (event-driven, no polling), re-checks idleness at fire time as a fail-safe, and then shuts down cleanly before exiting the process. Co-authored-by: Orca <help@stably.ai>
…lo socket An accepted socket that had not completed hello was invisible to the client map: an idle-exit fired in that window could tear down the server while the late hello still registered a client and spawned a live PTY — which process.exit(0) would then silently kill once server.close() drained. The same untracked socket could also hold server.close() (and the idle-exit callback) open forever. shuttingDown is now set synchronously at the top of shutdown(): new connections and late hellos are destroyed, RPCs are refused, all accepted sockets (tracked or pre-hello) are destroyed, TerminalHost refuses createOrAttach after dispose, and a failed idle shutdown still exits instead of stranding a half-shut daemon. Co-authored-by: Orca <help@stably.ai>
The detached daemon runs with stdio 'ignore', so the previous console.warn on idle exit went nowhere. Log idle-exit / idle-exit-shutdown-error events via DaemonFileLog and close the log before process.exit(0) so the exit reason lands in diagnostic bundles. Co-authored-by: Orca <help@stably.ai>
…tChange wiring Seed the session before server.start() arms the countdown so the invariant is proven by the timer never arming, instead of racing a cancel against a 75ms grace on a loaded machine. Add a deterministic TerminalHost onSessionCountChange wiring test covering create, natural-exit reap, and dispose (mutation-verified: deleting the create-side notify fails it). Co-authored-by: Orca <help@stably.ai>
…he adapter respawns The idle-exit shutdown barrier destroys sockets that connected but had not completed hello. The client surfaces that as EPIPE on the hello write or 'Connection closed before hello response' on a clean close — neither was classified daemon-gone, so withDaemonRetry surfaced the error to the caller (one failed terminal create) instead of respawning. Hello rejections reply ok:false with different messages and still never trigger a respawn. Found by the PR review loop; regression test fails without the fix. Co-authored-by: Orca <help@stably.ai>
…ion-killing test The shuttingDown guards in handleConnection and handleFirstMessage had no test that fails when either is deleted — the existing coverage drove routeRequest directly or never sent a hello. Raise only the barrier flag while the listener still accepts, then assert a pre-admitted socket's late valid hello and a brand-new connection are both destroyed unregistered. Verified: deleting either guard fails this test. Co-authored-by: Orca <help@stably.ai>
09c7bde to
23b98e6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/daemon/terminal-host.ts (1)
72-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing
onSessionCountChangenotification after dead session cleanup.Lines 72-75 remove a dead session from the map without calling
this.onSessionCountChange?.(). IfspawnSubprocess(line 81) subsequently throws, the session count drops to zero with no notification fired, leaving the idle-exit timer unaware and the daemon alive indefinitely — exactly the accumulation this PR aims to prevent.In the normal path the omission is masked because line 114 fires the callback after the new session is added. But on spawn failure,
reapSessionalso won't fire (the session was already deleted), so no callback ever signals the count change.🛡️ Proposed fix
if (existing) { existing.dispose() this.sessions.delete(opts.sessionId) + this.onSessionCountChange?.() }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2466c81-6323-4f53-bf81-0a23f8d9ace3
📒 Files selected for processing (14)
src/main/daemon/daemon-entry.tssrc/main/daemon/daemon-idle-exit.test.tssrc/main/daemon/daemon-idle-exit.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-main.test.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-pty-adapter.test.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/terminal-host-contract.tssrc/main/daemon/terminal-host.test.tssrc/main/daemon/terminal-host.ts
✅ Files skipped from review due to trivial changes (2)
- src/main/daemon/daemon-entry.ts
- src/main/daemon/terminal-host-contract.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/daemon/daemon-main.test.ts
- src/main/daemon/daemon-main.ts
- src/main/daemon/daemon-pty-adapter.ts
- src/main/daemon/daemon-pty-adapter.test.ts
- src/main/daemon/terminal-host.test.ts
- src/main/daemon/daemon-server.test.ts
- src/main/daemon/daemon-idle-exit.ts
- src/main/daemon/daemon-idle-exit.test.ts
- src/main/daemon/daemon-server.ts
Problem
The PTY daemon never idle-exits: it only terminates on SIGTERM/SIGINT or an explicit
shutdownRPC. Worse, every app launch adopts ALIVE legacy-protocol daemons (v1–v17 sockets) with no session-count check and never shuts them down — a legacy daemon with zero sessions is re-adopted forever, each one pinning a node + node-pty process in memory indefinitely. Protocol bumps guarantee these accumulate on long-lived installs (verified on a machine at 99.6% RAM from Orca-managed processes).Fix
daemon-init.ts): before adopting an alive legacy daemon, query its live-session count.0live sessions → issue shutdown through the existingcleanupDaemonForProtocolpath (shutdown RPC,killStaleDaemonfallback, socket/pid file cleanup) and skip adoption. Any live session, or a failed query (null), fail-safes to adoption exactly as before — a daemon whose sessions cannot be verified is never killed.daemon-idle-exit.ts,daemon-server.ts,daemon-entry.ts): a single unref'd countdown arms whenever the daemon holds 0 sessions AND 0 connected clients, and cancels on any session creation or client connect. Strictly event-driven (server start, client hello, control-socket close, session-map transitions via a newTerminalHost.onSessionCountChangehook) — no polling. At fire time the idle predicate is re-checked as a fail-safe, then the server shuts down cleanly beforeprocess.exit(0). Shutdown is a hard barrier: ashuttingDownflag set synchronously at the top ofshutdown()refuses new connections, late hellos, and RPCs, destroys every accepted socket (including pre-hello ones invisible to the client map), andTerminalHostrejectscreateOrAttachafter dispose — so an idle exit can never race an accepted-but-unhandshaken socket into a live PTY on a half-shut server, and a failed shutdown still exits instead of stranding a zombie. Grace is 10 minutes: the daemon's whole purpose is holding sessions across app restarts/updates, so at 0 sessions the grace only needs to outlast an update/restart reconnect window.daemon-pty-adapter.ts): a client whose socket is accepted just before the barrier rises but destroyed before its hello completes surfacesEPIPEon the hello write orConnection closed before hello responseon a clean close. Both are now classified daemon-gone, sowithDaemonRetryrespawns and retries instead of failing that terminal create. Hello rejections (bad token/version) replyok:falsewith different messages and still never trigger a respawn.ELI5
Orca keeps your terminals alive in a small helper program (the "daemon") that runs outside the app — that's why a running agent survives when Orca restarts or updates. The problem: a helper with nothing left to do never went away. Once all its terminals were closed it just sat there forever, and every time Orca bumped the helper's internal protocol version, the next launch would find the previous helper and keep that one alive too. Long-time users slowly collected a pile of do-nothing helpers, each pinning memory.
This PR does two things. First, at launch, if Orca finds an old-generation helper holding zero terminals, it now tells it to shut down instead of adopting it. Second, the current helper now shuts itself down after sitting completely empty — no terminals, no app connected — for 10 minutes.
Both are deliberately cautious. If the helper owns even one live terminal, or Orca can't verify what it owns (it doesn't answer, the query fails), nothing is touched — it's kept exactly as before. An empty helper holds nothing a user could lose, and the next launch just starts a fresh one, which is the same path Orca already uses when a helper crashes. A hard shutdown barrier also guarantees a helper can never half-die: once it starts shutting down it refuses all new connections and finishes exiting, so the app can never end up talking to a dying helper.
Design alternatives considered
Known residual coverage notes: the pre-v18 backlog needs one future app launch to be reaped; a legacy daemon adopted with sessions that later drain mid-run is only reaped at the next launch (it runs old code with no self-exit); crash-orphaned daemons with live sessions are out of scope here (see #4639).
Reliability proof (terminal-session-reliability contract)
startup-upgrade.persisted-session-corpus/ daemon startup reconcile; manifest gateterminal-provider.daemon-startup-degraded-contract("preserve valid live daemon sessions, reap only true orphans").daemon-init.test.ts: alive legacy daemon with 0 live sessions (list contains a dead entry, proving theisAlivefilter gates reaping) →shutdown {killSessions:true}issued via the v9-protocol client, no adapter constructed, no PID-fallback kill; alive legacy daemon with a live session → adopted unchanged, no shutdown issued; session-count query failure → adopted unchanged (fail-safe).daemon-idle-exit.test.ts(fake timers, fully deterministic): fires only after the full grace; cancels on non-idle evaluate; re-arms; keeps the original deadline on redundant evaluates; fire-time re-check refuses to expire if idleness was lost without a cancel (fail-safe); dispose blocks future arming; default grace is 10 min; countdown is unref'd (hasRef() === false).daemon-server.test.ts(real sockets): idle daemon with 0 clients/0 sessions shuts down and callsonIdleExit; client connect deterministically cancels the countdown and disconnect re-arms it; a session seeded beforestart()prevents the countdown from ever arming, survives a 3x-grace real-time wait, and the daemon exits only after that session ends.daemon-server.test.ts(shutdown barrier): a pre-hello socket open at idle-fire time is destroyed and the daemon still exits (pre-fix this scenario hangsserver.close()forever); aftershutdown(),createOrAttachis refused and spawns nothing; and a mutation-killing test raises only theshuttingDownflag while the listener still accepts, then proves a pre-admitted socket's late valid hello and a brand-new connection are both destroyed unregistered (deleting either barrier guard fails it).daemon-pty-adapter.test.ts: a barrier server that destroys pre-hello sockets makes the adapter respawn and retry (fails without the daemon-gone classification fix).terminal-host.test.ts:createOrAttachafterdispose()rejects without spawning a subprocess;onSessionCountChangefires on create, natural-exit reap, and dispose (mutation-verified: deleting the create-side notify fails it).daemon-main.test.ts: idle-exit options pass throughstartDaemonto the server.listSessionsround-trip per ALIVE legacy daemon insidecreateLegacyDaemonAdapters, which already runs socket probes per legacy version and executes concurrently with window load behind the existing abort guard. A dedicated perf audit confirmed: the app's persistent daemon control socket means a running Orca always counts as a connected client, so idle exit can only ever fire on a truly orphaned daemon; there is no exit/respawn churn loop (nothing auto-reconnects; a post-reap spawn pays one ordinary cold fork). Full daemon directory suite: 49 files, 703 tests passed (2 skipped pre-existing).pnpm typecheckclean.check:reliability-gatespassed (30 gates).probeSocket/cleanupDaemonForProtocolhelpers, which already gateexistsSync/unlinkSyncbehindprocess.platform !== 'win32'— no new path handling was added, so named-pipe behavior is identical by construction (covered-by-existing-helpers, no new Windows-only branch to test).killStaleDaemonverifies pid + process start time before acting, so a recycled pid is safe. The stale pid/token files are cleaned by the existing dead-socket cleanup on next launch.listSessionsRPC fail the query and are adopted (fail-safe), same as today.shouldPreserveDaemonWithLiveSessionspattern (only reachable with two different-version app instances launching simultaneously) — not a new risk class.terminal-provider.daemon-startup-degraded-contractis an experimental gate whose executable corpus is tracked on the pending reliability stack; these tests strengthen the same invariant family from this PR's files.liveSessionCount === null→ adopt; fire-timeisIdle()re-check) are the load-bearing guards.Post-rebase re-review (2026-07-09, second pass)
Rebased onto latest
main(v1.4.131-rc.2 era) to clear conflicts with#7538(Windows terminal update-survival) and#7849(daemon file log + startup CI hard-fail), which rewrote parts of the same files. Semantic interactions re-reviewed: no fights — the idle countdown cannot arm while the app holds its persistent daemon client connection, and the#7849boot smoke SIGTERMs well inside the grace window.#7849'sDaemonFileLog(idle-exitandidle-exit-shutdown-errorevents indaemon-server.ts,daemonLog.close()beforeprocess.exit(0)indaemon-entry.ts) instead ofconsole.warn, which goes nowhere in the detached stdio-ignore daemon. The exit reason now lands in diagnostic bundles.isDaemonGoneErrordid not classify a pre-hello socket teardown, so the barrier race would fail one terminal create instead of respawning (see Fix Add Shift+Enter to insert newline in terminal #3). Applied with a real-socket regression test; a follow-up mutation-killing test pins thehandleConnection/handleFirstMessagebarrier guards themselves. Final round: two independent reviewers on the complete diff, both returned clean with no blocking findings.server.start()arms the countdown, so the invariant is proven by the timer never arming instead of racing a cancel against a 75 ms grace on a loaded machine; added a deterministicTerminalHost.onSessionCountChangewiring test covering create, natural-exit reap, and dispose (mutation-verified).daemon-entry.js(grace patched to 5 s in a scratch copy of the bundle only; source untouched): 12/12 checks — idle-from-birth exit at exactly grace withidle-exit+daemon-log-closedin the log and the socket unlinked; a connected client blocks exit for 3x grace and disconnect re-arms it; a live PTY session with zero clients blocks exit (the data-loss invariant) and the daemon exits only after that session ends; a pre-hello socket is destroyed by the shutdown barrier and cannot hangserver.close().[daemon] Shutting down idle legacy daemon (protocol v17) with no live sessionsin the main-process log, the v17 process gone from the process table, the v17 socket unlinked) while a working terminal came up on a fresh v18 daemon (full-app screenshot captured locally undervalidation-screenshots/, untracked); re-planted the v17 daemon holding a live PTY session → app launch adopted it untouched (daemon and its session shell both still alive, no reap log line).pnpm typecheckclean; daemon suite 49 files / 703 tests passed;check:reliability-gates30 gates pass;#7849'sdaemon-boot-smoke.mjsPASS against the built bundle. The onepnpm lintfailure is pre-existing onmainfrom#7684(notification-authorization-status.ts, untouched here).Evidence commands
pnpm vitest run --config config/vitest.config.ts src/main/daemon→ 48 files passed (1 skipped pre-existing), 703 tests passed (2 skipped pre-existing)pnpm typecheck→ cleanpnpm run check:reliability-gates→ 30 gates passnode config/scripts/daemon-boot-smoke.mjs→ PASS (real PTY served end-to-end under plain Node)Made with Orca 🐋