Skip to content

perf(daemon): reap idle legacy daemons and self-exit at zero sessions#7615

Open
brennanb2025 wants to merge 7 commits into
mainfrom
brennanb2025/perf-daemon-idle-exit
Open

perf(daemon): reap idle legacy daemons and self-exit at zero sessions#7615
brennanb2025 wants to merge 7 commits into
mainfrom
brennanb2025/perf-daemon-idle-exit

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Problem

The PTY daemon never idle-exits: it only terminates on SIGTERM/SIGINT or an explicit shutdown RPC. 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

  1. Legacy reaping at adoption (daemon-init.ts): before adopting an alive legacy daemon, query its live-session count. 0 live sessions → issue shutdown through the existing cleanupDaemonForProtocol path (shutdown RPC, killStaleDaemon fallback, 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.
  2. Idle self-exit for the current-protocol daemon (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 new TerminalHost.onSessionCountChange hook) — no polling. At fire time the idle predicate is re-checked as a fail-safe, then the server shuts down cleanly before process.exit(0). Shutdown is a hard barrier: a shuttingDown flag set synchronously at the top of shutdown() refuses new connections, late hellos, and RPCs, destroys every accepted socket (including pre-hello ones invisible to the client map), and TerminalHost rejects createOrAttach after 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.
  3. Barrier-race recovery in the app (daemon-pty-adapter.ts): a client whose socket is accepted just before the barrier rises but destroyed before its hello completes surfaces EPIPE on the hello write or Connection closed before hello response on a clean close. Both are now classified daemon-gone, so withDaemonRetry respawns and retries instead of failing that terminal create. Hello rejections (bad token/version) reply ok:false with 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

  • Exit the instant the last terminal closes (no grace): rejected — the daemon exists to survive app restarts/updates for warm reattach; a grace window is the standard tradeoff for reattach-oriented daemons, and 10 minutes comfortably outlasts an update cycle while bounding idle memory.
  • Parent-process watchdog (kill daemon when the app dies): orthogonal — the daemon must outlive the app by design. A watchdog addresses a different failure mode (crash-orphaned daemon with live sessions), which is exactly open PR fix(daemon): add parent process watchdog to kill orphaned PTY sessions on crash #4639's territory and stays complementary to this PR.
  • Launch-time sweep only (no self-exit): insufficient — a user who never relaunches Orca would keep an empty daemon forever. Self-exit covers end-of-life without depending on a future launch; the launch-time reap covers the pre-v18 backlog that predates self-exit code.
  • Open PR Clean up empty legacy daemons #4429 implements the same legacy-reap half of this PR (and corroborates the leak profile); this PR is a superset and Clean up empty legacy daemons #4429 can be closed when this lands.

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)

  • Touched class: startup-upgrade.persisted-session-corpus / daemon startup reconcile; manifest gate terminal-provider.daemon-startup-degraded-contract ("preserve valid live daemon sessions, reap only true orphans").
  • Invariant: a daemon that reports live sessions, or whose session state cannot be verified, is never shut down; the idle countdown can never fire while any tracked session (alive, terminating, or exited-but-unreaped) or any connected client exists; and once shutdown begins, no accepted socket can become a client and no RPC can create a session (idle exit cannot pass the point of no return while any accepted socket could still become live work).
  • Failure source: July 2026 perf audit — idle daemon accumulation across protocol bumps; no end-of-life reaping for legacy daemons.
  • Product change type: perf hardening + runtime hardening, with new deterministic gate tests.
  • Oracle / correctness evidence:
    • daemon-init.test.ts: alive legacy daemon with 0 live sessions (list contains a dead entry, proving the isAlive filter 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 calls onIdleExit; client connect deterministically cancels the countdown and disconnect re-arms it; a session seeded before start() 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 hangs server.close() forever); after shutdown(), createOrAttach is refused and spawns nothing; and a mutation-killing test raises only the shuttingDown flag 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: createOrAttach after dispose() rejects without spawning a subprocess; onSessionCountChange fires on create, natural-exit reap, and dispose (mutation-verified: deleting the create-side notify fails it).
    • daemon-main.test.ts: idle-exit options pass through startDaemon to the server.
  • No-regression evidence: all logic is event-driven (no polling loops, no hidden-pane wakeups); the only startup-path addition is one listSessions round-trip per ALIVE legacy daemon inside createLegacyDaemonAdapters, 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 typecheck clean. check:reliability-gates passed (30 gates).
  • Provider/platform coverage: daemon provider covered by the tests above; local/SSH/WSL/remote-runtime/mobile unaffected (this daemon is local-machine infrastructure; SSH/relay terminals do not route through it). macOS/Linux covered by the unix-socket paths; Windows named pipes use the same probeSocket/cleanupDaemonForProtocol helpers, which already gate existsSync/unlinkSync behind process.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).
  • Residual gaps:
    • The daemon cannot unlink its own pid file on idle exit (it doesn't know the path); the next launch's killStaleDaemon verifies 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.
    • Very old legacy daemons that predate the listSessions RPC fail the query and are adopted (fail-safe), same as today.
    • The adapter regression test covers the destroyed-pre-hello-socket race as a class; whether a given run surfaces the EPIPE or the clean-close variant depends on write-flush timing (both are classified, so the test is deterministic either way).
    • Legacy reap has the same theoretical adopt-vs-kill TOCTOU as the existing shouldPreserveDaemonWithLiveSessions pattern (only reachable with two different-version app instances launching simultaneously) — not a new risk class.
    • Manifest not modified: terminal-provider.daemon-startup-degraded-contract is 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.
  • Promotion status: none → the new tests run in the standard vitest suite; no gate promoted.
  • Rollback/demotion rule: any report of a daemon exiting while it owned sessions, or of a legacy daemon with live sessions being reaped, demotes this to revert-first — the fail-safe paths (liveSessionCount === null → adopt; fire-time isIdle() 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 #7849 boot smoke SIGTERMs well inside the grace window.

  • Conflict-resolution improvement: idle-exit diagnostics route through #7849's DaemonFileLog (idle-exit and idle-exit-shutdown-error events in daemon-server.ts, daemonLog.close() before process.exit(0) in daemon-entry.ts) instead of console.warn, which goes nowhere in the detached stdio-ignore daemon. The exit reason now lands in diagnostic bundles.
  • Review-loop fix: the two-reviewer review/fix loop surfaced that isDaemonGoneError did 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 the handleConnection/handleFirstMessage barrier guards themselves. Final round: two independent reviewers on the complete diff, both returned clean with no blocking findings.
  • Test-only hardening: deflaked the "never fires while a session is alive" test — the session is now seeded before 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 deterministic TerminalHost.onSessionCountChange wiring test covering create, natural-exit reap, and dispose (mutation-verified).
  • End-to-end validation against the real built 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 with idle-exit + daemon-log-closed in 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 hang server.close().
  • Real-app Electron validation (dev build, scratch user-data dir): planted a live protocol-v17 daemon with zero sessions → app launch reaped it (three independent signals: [daemon] Shutting down idle legacy daemon (protocol v17) with no live sessions in 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 under validation-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).
  • Post-rebase evidence: pnpm typecheck clean; daemon suite 49 files / 703 tests passed; check:reliability-gates 30 gates pass; #7849's daemon-boot-smoke.mjs PASS against the built bundle. The one pnpm lint failure is pre-existing on main from #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 → clean
  • pnpm run check:reliability-gates → 30 gates pass
  • node config/scripts/daemon-boot-smoke.mjs → PASS (real PTY served end-to-end under plain Node)

Made with Orca 🐋

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

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

Use the following commands to manage reviews:

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

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds idle self-exit support for the daemon. A new DaemonIdleExit controller manages the grace-period timer, TerminalHost reports session-count changes, and DaemonServer uses those signals to evaluate idleness during startup, client activity, and shutdown. daemon-main.ts and daemon-entry.ts forward and handle the idle-exit callbacks. Tests cover the timer controller, server behavior, startup wiring, and terminal host notifications. The legacy daemon adoption path now checks live session count and shuts down alive legacy daemons with zero live sessions.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed, but it does not follow the repository's required template sections or checklist structure. Rewrite it to match the template with Summary, Screenshots, Testing checklist, AI Review Report, Security Audit, and Notes sections.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: reaping idle legacy daemons and self-exiting the daemon when idle.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/main/daemon/daemon-init.ts (1)

777-782: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Comment 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 value

Minor: unused requestMock return value.

Since ensureConnected throws before request is ever called in this scenario, the { sessions: [] } return value on requestMock is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9a3bf12 and 592ef53.

📒 Files selected for processing (10)
  • src/main/daemon/daemon-entry.ts
  • src/main/daemon/daemon-idle-exit.test.ts
  • src/main/daemon/daemon-idle-exit.ts
  • src/main/daemon/daemon-init.test.ts
  • src/main/daemon/daemon-init.ts
  • src/main/daemon/daemon-main.test.ts
  • src/main/daemon/daemon-main.ts
  • src/main/daemon/daemon-server.test.ts
  • src/main/daemon/daemon-server.ts
  • src/main/daemon/terminal-host.ts

Comment thread src/main/daemon/daemon-init.ts
Comment thread src/main/daemon/daemon-server.ts
@brennanb2025

Copy link
Copy Markdown
Contributor Author

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.

@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-server.test.ts (1)

594-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test 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 the shuttingDown guard in routeRequest, Line 335 of daemon-server.ts). It never sends a real hello message through handleConnection/handleFirstMessage to exercise the shuttingDown check 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

📥 Commits

Reviewing files that changed from the base of the PR and between 592ef53 and 14faf53.

📒 Files selected for processing (4)
  • src/main/daemon/daemon-server.test.ts
  • src/main/daemon/daemon-server.ts
  • src/main/daemon/terminal-host.test.ts
  • src/main/daemon/terminal-host.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/daemon/terminal-host.ts

@brennanb2025
brennanb2025 force-pushed the brennanb2025/perf-daemon-idle-exit branch 2 times, most recently from 261f3da to 09c7bde Compare July 10, 2026 00:25
brennanb2025 and others added 7 commits July 10, 2026 17:36
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>
@brennanb2025
brennanb2025 force-pushed the brennanb2025/perf-daemon-idle-exit branch from 09c7bde to 23b98e6 Compare July 11, 2026 00:43

@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.

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 win

Missing onSessionCountChange notification after dead session cleanup.

Lines 72-75 remove a dead session from the map without calling this.onSessionCountChange?.(). If spawnSubprocess (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, reapSession also 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

📥 Commits

Reviewing files that changed from the base of the PR and between 09c7bde and 23b98e6.

📒 Files selected for processing (14)
  • src/main/daemon/daemon-entry.ts
  • src/main/daemon/daemon-idle-exit.test.ts
  • src/main/daemon/daemon-idle-exit.ts
  • src/main/daemon/daemon-init.test.ts
  • src/main/daemon/daemon-init.ts
  • src/main/daemon/daemon-main.test.ts
  • src/main/daemon/daemon-main.ts
  • src/main/daemon/daemon-pty-adapter.test.ts
  • src/main/daemon/daemon-pty-adapter.ts
  • src/main/daemon/daemon-server.test.ts
  • src/main/daemon/daemon-server.ts
  • src/main/daemon/terminal-host-contract.ts
  • src/main/daemon/terminal-host.test.ts
  • src/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

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