fix(daemon): retire empty current-generation daemons#9277
Conversation
📝 WalkthroughWalkthroughThis change introduces protocol v24 daemon endpoint identities and launch-scoped PID records, including strict readiness validation and ownership-safe cleanup. It adds timeout-aware client connections, identity matching across sockets, clean-disconnect retirement, idle shutdown fencing, respawn leases, and legacy protocol compatibility. Daemon launch, initialization, adapter retry, server shutdown, CLI parsing, unit tests, and end-to-end lifecycle fixtures are updated accordingly. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/daemon/daemon-health.ts (1)
320-329: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail closed on malformed process identity values.
pid,startedAtMs, andlaunchNonceare less strictly validated than endpoint identities. For example, a non-positive start time can become a start-time mismatch and classify a matching live command/nonce asabsent, permitting cleanup and replacement. Require a positive safe PID/start time and non-empty nonce, or classify the identity asunknown.Proposed fix
-if (typeof parsed.pid === 'number' && Number.isFinite(parsed.pid)) { +if (Number.isSafeInteger(parsed.pid) && (parsed.pid as number) > 0) { return { pid: parsed.pid, startedAtMs: typeof parsed.startedAtMs === 'number' && - Number.isFinite(parsed.startedAtMs) + Number.isFinite(parsed.startedAtMs) && + parsed.startedAtMs > 0 ? parsed.startedAtMs : null, entryPath: typeof parsed.entryPath === 'string' ? parsed.entryPath : null, appVersion: typeof parsed.appVersion === 'string' ? parsed.appVersion : null, - ...(typeof parsed.launchNonce === 'string' ? { launchNonce: parsed.launchNonce } : {}) + ...(typeof parsed.launchNonce === 'string' && parsed.launchNonce.length > 0 + ? { launchNonce: parsed.launchNonce } + : {}) } }-if (args.expectedStartedAtMs === null || !args.launchNonce) { +if ( + args.expectedStartedAtMs === null || + !Number.isFinite(args.expectedStartedAtMs) || + args.expectedStartedAtMs <= 0 || + !args.launchNonce +) { return 'unknown' }Also applies to: 506-523
src/main/daemon/daemon-init.ts (1)
1184-1206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not report nonexistent legacy generations as failures.
When both socket and PID file are absent,
readFileSyncthrowsENOENT,legacyDaemonProcessState()returnsunknown, and the protocol is added tofailedProtocols. A clean install therefore marks protocols 1–22 failed, keeping reconciliation and generation audit incomplete. Skip protocols with no endpoint artifacts; reservefailedProtocolsfor actual unverifiable candidates.
🟠 Major comments (25)
tests/e2e/helpers/daemon-generation-processes.ts-5-5 (1)
5-5: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not accept adjacent process starts as the same incarnation.
A PID reused within the 1.1–2 second tolerance is treated as the recorded process and may then be killed by cleanup. These comparisons use the same OS-derived creation timestamp, so compare it exactly; if its resolution is insufficient, use a stronger platform identity rather than widening the match.
Minimal safety fix
-const PROCESS_IDENTITY_TOLERANCE_MS = process.platform === 'win32' ? 2_000 : 1_100- Math.abs(current.startedAtMs - identity.startedAtMs) <= PROCESS_IDENTITY_TOLERANCE_MS + current.startedAtMs === identity.startedAtMs- Math.abs(currentRoot.startedAtMs - root.startedAtMs) > PROCESS_IDENTITY_TOLERANCE_MS + currentRoot.startedAtMs !== root.startedAtMsAlso applies to: 107-113, 149-153
tests/e2e/helpers/daemon-generation-fixtures.ts-273-283 (1)
273-283: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not replace the v23 daemon’s owned PID record.
When
launchNonceexists, the PID path was already supplied to the daemon. Overwriting it after readiness can race idle cleanup, recreate a stale artifact after removal, and validates synthetic data instead of the production writer. Only synthesize records for legacy fixtures.Proposed fix
- writeFileSync( - pidPath, - `${JSON.stringify({ - pid: daemonIdentity.pid, - startedAtMs: daemonIdentity.startedAtMs, - entryPath, - fixtureNonce: randomUUID(), - ...(launchNonce ? { launchNonce } : {}) - })}\n`, - { mode: 0o600 } - ) + if (launchNonce === null) { + writeFileSync( + pidPath, + `${JSON.stringify({ + pid: daemonIdentity.pid, + startedAtMs: daemonIdentity.startedAtMs, + entryPath, + fixtureNonce: randomUUID() + })}\n`, + { mode: 0o600 } + ) + }src/main/daemon/retirement-pending-claim-reconciliation.ts-21-27 (1)
21-27: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winList each daemon generation once, not once per claim.
Claims sharing a
protocolVersionrequest the identical provider inventory. With the 4,096-item bound, this can serialize thousands of redundant daemon calls during startup maintenance. Group claims by protocol, list once per provenance, then reconcile all matching session IDs from that snapshot.src/main/daemon/production-launcher.ts-41-48 (1)
41-48: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail the launch when v23 process identity cannot be published.
Missing/invalid
startedAtMsproduces anullPID record, while missingchild.pidsilently skips it. The v23 cleanup path then fails closed, leaving an unreplaceable daemon. A PID-record write failure also rejects without terminating the already-ready detached child.Proposed fix
const startedAtMs = await waitForReady(child) -if (pidPath && launchNonce && child.pid) { - writeFileSync( - pidPath, - serializeDaemonPidFile({ pid: child.pid, startedAtMs, entryPath, launchNonce }), - { mode: 0o600 } - ) +if (pidPath && launchNonce) { + if (!child.pid || startedAtMs === null) { + await shutdownChild(child) + throw new Error('Daemon ready message did not include a complete process identity') + } + try { + writeFileSync( + pidPath, + serializeDaemonPidFile({ pid: child.pid, startedAtMs, entryPath, launchNonce }), + { mode: 0o600 } + ) + } catch (error) { + await shutdownChild(child) + throw error + } }Also applies to: 60-60, 92-95
src/main/daemon/daemon-spawner.ts-174-181 (1)
174-181: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly discard the claim for a confirmed
EEXISTreplacement.Any failed copy followed by
canonicalExists() === trueis treated as success. If the failed operation created a partial destination before throwing, the caller deletes the sole intact claim and preserves corrupted canonical data. Retain the claim for ENOSPC/EIO and other failures.Proposed fix
- } catch { + } catch (error) { // Why: only a confirmed replacement makes the unique claim redundant; an // I/O failure with no canonical file must retain the sole recoverable record. - return canonicalExists(canonicalPath) + return ( + (error as NodeJS.ErrnoException).code === 'EEXIST' && + canonicalExists(canonicalPath) + ) }Add a test where
copyExclusivecreates the canonical path and then throws ENOSPC; the result must remainfalse.src/main/daemon/daemon-ownership-raw-extractor.ts-123-128 (1)
123-128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not let provenance labels hide local bindings.
remoteaccepts local-shaped IDs, whilelocal-fallbackdoes not require the ID to be inlegacyProtectedSessionIds. Either inconsistency returns a complete extraction that omits the bound local session.Proposed fix
+ const legacyProtected = new Set(ownership.legacyProtectedSessionIds) for (const binding of workspace.bindings) { const provenance = ownership.bindingProvenanceByPtyId[binding.sessionId] - if (!provenance || !bindingMatchesProvenance(binding, provenance, claimByPhysicalId)) { + if ( + !provenance || + !bindingMatchesProvenance(binding, provenance, claimByPhysicalId, legacyProtected) + ) { return null } } function bindingMatchesProvenance( binding: RawTerminalBinding, provenance: TerminalBindingProvenance, - claimByPhysicalId: ReadonlyMap<string, DaemonSessionClaim> + claimByPhysicalId: ReadonlyMap<string, DaemonSessionClaim>, + legacyProtected: ReadonlySet<string> ): boolean { const idClass = classifyRawTerminalSessionId(binding.sessionId) if (provenance.kind === 'remote') { - return !hasClaimForSession(claimByPhysicalId, binding.sessionId) + return idClass === 'remote' && !hasClaimForSession(claimByPhysicalId, binding.sessionId) } if (idClass !== 'local') { return false } if (provenance.kind === 'local-fallback') { - return !hasClaimForSession(claimByPhysicalId, binding.sessionId) + return ( + legacyProtected.has(binding.sessionId) && + !hasClaimForSession(claimByPhysicalId, binding.sessionId) + ) }Also applies to: 159-175
src/main/daemon/daemon-ownership-raw-field-classification.ts-1-7 (1)
1-7: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow the known
remoteSessionIdsByTabIdfield.Line 18 classifies
remoteSessionIdsByTabIdas ownership-like, but it is absent from this allowlist. Consequently,parseRawLocalWorkspacerejects every workspace containing metadata that it explicitly validates later asunsupported-field.Proposed fix
const KNOWN_WORKSPACE_ID_FIELDS = new Set([ 'activeConnectionIdsAtShutdown', 'defaultTerminalTabsAppliedByWorktreeId', + 'remoteSessionIdsByTabId', 'sleepingAgentSessionsByPaneKey', 'terminalLayoutsByTabId' ])src/main/daemon/daemon-reap-candidate-journal.ts-197-205 (1)
197-205: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate stale evidence when ordinary journal replacement fails.
If
replaceAndReadBackfails after entries were removed or reset, the old mature journal remains readable next launch. Apply the same invalidation safeguard already used bycommitResetMarker.Proposed fix
if (!(await this.persistence.replaceAndReadBack(encoded, this.maxBytes))) { + await this.persistence.invalidate(this.maxBytes) return this.result('incomplete', 'persistence-error', true, 0, []) }src/main/daemon/daemon-ownership-raw-remote-and-legacy.ts-106-114 (1)
106-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject migration rows whose source contradicts the PTY ID.
An entry with
source: 'ssh'currently accepts a local-shaped ID, but Line 45 ignores that row rather than protecting it. Corrupt state can therefore be reported complete while omitting a local session from ownership.Proposed fix
function isValidMigrationRow(value: unknown): value is { ptyId: string; source: 'local' | 'ssh' } { - return ( - isRecord(value) && - isId(value.ptyId) && - (value.source === 'local' || value.source === 'ssh') && - value.reason === 'legacy-numeric-pane-key' && - Number.isFinite(value.updatedAt) && - (value.source === 'ssh' || classifyRawTerminalSessionId(value.ptyId) === 'local') - ) + if ( + !isRecord(value) || + !isId(value.ptyId) || + (value.source !== 'local' && value.source !== 'ssh') + ) { + return false + } + const classification = classifyRawTerminalSessionId(value.ptyId) + return ( + value.reason === 'legacy-numeric-pane-key' && + Number.isFinite(value.updatedAt) && + ((value.source === 'local' && classification === 'local') || + (value.source === 'ssh' && classification === 'remote')) + ) }src/main/daemon/daemon-session-process-incarnation-windows.ts-114-123 (1)
114-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReport missing Windows rows as
not-observed.A successful CIM query that omits a requested PID is authoritative absence, but Line 119 conflates it with a present row lacking
CreationDate. This prevents Windows probes from ever reporting dead/missing processes.Proposed fix
if (entries.length > 1) { return { pid, state: 'ambiguous' } } - if (entries.length === 0 || entries[0].creationDate === null) { + if (entries.length === 0) { + return { pid, state: 'not-observed' } + } + if (entries[0].creationDate === null) { return { pid, state: 'unknown' } }src/main/daemon/daemon-ownership-raw-snapshot.ts-220-240 (1)
220-240: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse a verified committed backup when the primary is unavailable.
Although Lines 136–145 capture five backups, this branch always reports
state-backup-unverifiableand never callsparseValidDaemonOwnershipCommiton them. A missing/corrupt primary therefore leaves ownership permanently incomplete even when an authoritative committed backup exists, contrary to the recovery contract in the plan.src/main/daemon/daemon-ownership-raw-snapshot.ts-125-133 (1)
125-133: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftKeep large snapshot parsing and hashing off the main thread.
At the allowed 64 MiB limit,
JSON.stringify, SHA-256 hashing,JSON.parse, and extraction all run synchronously. This can stall terminal I/O well beyond the documented 50 ms event-loop ceiling. Stream/incrementally hash sources and move parsing/extraction to a worker or bounded incremental pipeline.Also applies to: 228-266
src/main/daemon/daemon-session-claims.ts-139-145 (1)
139-145: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBuild the provenance map with a null prototype.
A persisted PTY ID of
__proto__changes this object’s prototype instead of creating an own entry. Downstream property lookup can then accept inherited provenance and incorrectly treat malformed ownership as complete.Proposed fix
- const bindingProvenanceByPtyId: Record<string, TerminalBindingProvenance> = {} + const bindingProvenanceByPtyId: Record<string, TerminalBindingProvenance> = + Object.create(null)src/main/persistence.ts-6169-6175 (1)
6169-6175: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftScope ownership mutations to
(sessionId, protocolVersion).Line 6172 removes every claim sharing the PTY ID, Lines 6420-6443 transition all matching protocols, and Line 6481 deletes whichever provenance currently occupies the ID. Since same session IDs can coexist across protocols, a v23 bind or stop can erase or mutate v22 ownership.
Proposed direction
- existing.sessionId === args.ptyId || + (existing.sessionId === args.ptyId && + existing.protocolVersion === protocolVersion) || (existing.ownerKind === 'pane' && existing.workspaceKey === args.worktreeId && existing.ownerId === args.leafId)Resolve the stored
local-daemonprovenance intransitionExistingDaemonSessionClaimand only transition claims with itsprotocolVersion. Likewise, deletebindingProvenanceByPtyId[sessionId]only when the stored provenance exactly matches the verified-stop provenance.Also applies to: 6411-6443, 6475-6485
src/main/orca-profiles/profile-project-transfer-recovery.ts-112-158 (1)
112-158: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRevalidate the source before clearing the target receipt.
Lines 138-143 use captured source absence as the commit proof, but Line 155 only checks the target. If the source is active or rewritten after scanning, recovery can clear the receipt after the repo has been reintroduced, losing the recovery fence and leaving duplicate ownership. Skip active-source receipts and require every source capture to remain current.
Proposed fix
if ( lineage.role !== 'target-lineage' || lineage.targetProfileId !== targetProfile.id || + lineage.sourceProfileId === activeProfileId || !knownProfileIds.has(lineage.sourceProfileId) ) { return false } ... - if (!captureStillCurrent(rawProfiles.get(targetProfile.id), targetProfile.id, userDataPath)) { + const sourcesStillCurrent = recoverable.every((lineage) => + captureStillCurrent( + rawProfiles.get(lineage.sourceProfileId), + lineage.sourceProfileId, + userDataPath + ) + ) + if ( + !sourcesStillCurrent || + !captureStillCurrent(rawProfiles.get(targetProfile.id), targetProfile.id, userDataPath) + ) { continue }src/main/daemon/daemon-ownership-commit-migration.ts-57-66 (1)
57-66: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFence the migration against concurrent profile writes.
The file can change after
readBoundedMigrationSourcereturns. A laterrenameSyncthen replaces the newer profile with the stale captured snapshot, losing unrelated state. Perform migration under the same exclusive writer lock/fence used by profile persistence.src/main/daemon/daemon-ownership-commit-migration.ts-40-48 (1)
40-48: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate ownership schema independently of commit integrity.
A checksum proves consistency, not that the persisted ownership authority has a valid schema. The current write/read boundary can promote malformed ownership into trusted transfer state.
src/main/daemon/daemon-ownership-commit-migration.ts#L40-L48: validate the raw state and any present ownership field before backfilling a commit.src/main/orca-profiles/profile-project-state-file.ts#L62-L80: reject malformed presentdaemonSessionOwnershipbefore reconstructingTransferProfileState.src/main/orca-profiles/profile-project-terminal-binding-ids.ts-34-48 (1)
34-48: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve binding IDs shared with retained projects.
all.delete(id)removes an ID whenever the moved project references it, even if another repository also references the same PTY. Source cleanup can consequently discard provenance or legacy protection for a still-retained binding. Collect bindings directly from owner keys not belonging torepoIdinstead of subtracting sets.src/main/orca-profiles/profile-project-transfer-lineage.ts-28-38 (1)
28-38: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed on ambiguous source-pending receipts.
Selecting the oldest candidate can resume the wrong operation when multiple source receipts match the request. Apply the same operation/target identity ambiguity check used by
findTargetTransferLineagebefore returning a candidate.src/main/providers/pty-natural-exit-reconciliation.ts-200-205 (1)
200-205: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCanonicalize provenance keys.
JSON.stringifypreserves property insertion order, so structurally identical provenance objects can map to different keys. That can make cancellation or explicit-stop fences miss a pending exit, causing duplicate cleanup or delivery.Build the key from each provenance variant’s fields in a fixed order, and add a regression test using differently ordered objects.
src/main/daemon/client.ts-301-306 (1)
301-306: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire endpoint identity for protocol v23.
An absent
daemonIdentitybecomesnull, and two null identities compare equal, so v23 can connect without authenticating PID/start-time/nonce. Reject null identities for protocol 23+ while retaining legacy compatibility.Proposed fix
const identity = parseDaemonEndpointIdentity(response.daemonIdentity) -if (response.daemonIdentity !== undefined && identity === null) { +if ( + identity === null && + (this.protocolVersion >= 23 || response.daemonIdentity !== undefined) +) { finish(new DaemonProtocolError('Invalid daemon identity')) return }Also applies to: 440-450
src/main/daemon/daemon-init.ts-803-817 (1)
803-817: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep legacy listeners bound while launching the replacement.
unbindLocalProviderListeners()runs beforeensureRunning()and legacy rediscovery. Data or exits from preserved legacy PTYs during those awaits are dropped. Build and discover the replacement first, then unbind, dispose old subscriptions, swap, and rebind synchronously.Also applies to: 843-850, 853-868
src/main/ipc/pty.ts-607-617 (1)
607-617: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not finalize when ownership removal rejects the stop.
When
mayForgetRouteis false, this still returns true. Callers then finalize shutdown and deliver an exit despite the durable binding refusing route removal, potentially clearing replacement state.Proposed fix
const mayForgetRoute = store?.removePtyBindingOwnershipAfterVerifiedStop({ sessionId: id, provenance: expectedProvenance }) ?? true - if (mayForgetRoute) { - if (provider.forgetPtyRouteAfterVerifiedStop?.(id, expectedProvenance) === false) { - return false - } + if (!mayForgetRoute) { + return false + } + if (provider.forgetPtyRouteAfterVerifiedStop?.(id, expectedProvenance) === false) { + return false } return truesrc/main/ipc/pty.ts-2962-2966 (1)
2962-2966: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the verified exit when cleanup throws.
The pending proof is cancelled before durable cleanup. If cleanup throws, reconciliation cannot retry and the exited binding remains stuck.
Proposed fix
if (payload.verifiedAbsent) { - naturalPtyExitReconciliation.cancel(payload.id, payload.provenance) - if (finishVerifiedPtyBindingStop(localProvider, payload.id, store, payload.provenance)) { + let didFinish: boolean + try { + didFinish = finishVerifiedPtyBindingStop( + localProvider, + payload.id, + store, + payload.provenance + ) + } catch { + naturalPtyExitReconciliation.enqueue(exit) + return + } + naturalPtyExitReconciliation.cancel(payload.id, payload.provenance) + if (didFinish) { deliverConfirmedNaturalExit(exit) }src/main/daemon/degraded-daemon-pty-provider.ts-272-278 (1)
272-278: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoll back the local fence when daemon fencing fails.
If
current.beginRestartFence()rejects,restartFencedremains true and future spawn/attach operations stay blocked.Proposed fix
async beginRestartFence(): Promise<string[]> { this.restartFenced = true - if (this.createOrAttachInFlight > 0) { - await new Promise<void>((resolve) => this.createOrAttachDrainWaiters.add(resolve)) + try { + if (this.createOrAttachInFlight > 0) { + await new Promise<void>((resolve) => this.createOrAttachDrainWaiters.add(resolve)) + } + return await this.current.beginRestartFence() + } catch (error) { + this.restartFenced = false + this.current.cancelRestartFence() + throw error } - return this.current.beginRestartFence() }
🟡 Minor comments (3)
tests/e2e/helpers/daemon-generation-fixtures.ts-265-272 (1)
265-272: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a reported start identity when validation is enabled.
A missing or non-numeric
startedAtMssilently bypasses validation. Only the official legacy fixture disables this contract check.Proposed fix
const reportedStart = ready.startedAtMs - if ( - validateReportedStart && - typeof reportedStart === 'number' && - Math.abs(reportedStart - daemonIdentity.startedAtMs) > 2_000 - ) { - throw new Error(`${label} daemon self-reported a different process incarnation`) + if (validateReportedStart) { + if ( + typeof reportedStart !== 'number' || + !Number.isFinite(reportedStart) || + Math.abs(reportedStart - daemonIdentity.startedAtMs) > 2_000 + ) { + throw new Error(`${label} daemon self-reported a different process incarnation`) + } }tests/e2e/daemon-generation-reaping.spec.ts-200-217 (1)
200-217: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the child-exit listener race.
The child can exit after Lines 204-206 but before the listener is registered. Since
exitis not replayed, this then waits until the 10-second timeout.Proposed fix
return await new Promise((resolve, reject) => { const timer = setTimeout( () => reject(new Error('Timed out waiting for fixture daemon exit')), timeoutMs ) - child.once('exit', (code, signal) => { + const onExit = (code: number | null, signal: NodeJS.Signals | null): void => { clearTimeout(timer) resolve({ code, signal }) - }) + } + child.once('exit', onExit) + if (child.exitCode !== null || child.signalCode !== null) { + child.off('exit', onExit) + clearTimeout(timer) + resolve({ code: child.exitCode, signal: child.signalCode }) + } })src/main/daemon/degraded-daemon-provider-events.ts-27-40 (1)
27-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSnapshot listener arrays before fanout.
A listener that unsubscribes itself mutates the live array and can skip the next data or exit listener. Mirror the synthetic-exit implementation.
Proposed fix
- for (const listener of this.dataListeners) { + for (const listener of this.dataListeners.slice()) { listener(payload) } ... - for (const listener of this.exitListeners) { + for (const listener of this.exitListeners.slice()) { listener({
🧹 Nitpick comments (4)
tests/e2e/helpers/daemon-generation-processes.ts (1)
216-224: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEnumerate the process table once per aggregate check.
This currently runs a full
ps/CIM query for every identity. A single polling predicate can therefore takeidentityCount × 3 seconds, defeating the surrounding three- and five-second deadlines.Proposed refactor
export async function anyRecordedProcessIsAlive( identities: RecordedProcessIdentity[] ): Promise<boolean> { - for (const identity of identities) { - if (await processIdentityIsAlive(identity)) { - return true - } - } - return false + const rows = await readProcessRows() + return identities.some((identity) => { + const current = rows.find((row) => row.pid === identity.pid) + return current?.startedAtMs === identity.startedAtMs + }) }Source: Coding guidelines
src/main/daemon/retirement-pending-claim-reconciliation.test.ts (1)
52-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the verified stale route is forgotten.
This case should verify
forgetPtyRouteAfterVerifiedStopreceivespending-sessionand the protocol-22 provenance; otherwise the route-cleanup half of successful reconciliation can regress unnoticed.Proposed assertion
expect(store.clearVerifiedRetirementPendingDaemonClaim).toHaveBeenCalledOnce() +expect(provider.forgetPtyRouteAfterVerifiedStop).toHaveBeenCalledWith('pending-session', { + kind: 'local-daemon', + protocolVersion: 22 +})src/main/daemon/daemon-session-process-incarnation-posix.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstruct
/procpaths with Node path utilities.Use
path.posix.joinfor both the boot-ID and PID-stat paths rather than embedding separators.As per coding guidelines, “Use
path.joinor Electron/Node path utilities for file paths; never assume/or\.”Also applies to: 45-45
Source: Coding guidelines
src/main/daemon/daemon-server.ts (1)
783-802: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the
shutdownswitch clause.
const serverClosecurrently occupies the switch-wide lexical scope and triggers Biome’snoSwitchDeclarationserror. Wrap this case in braces.Proposed fix
- case 'shutdown': + case 'shutdown': { this.log.log('shutdown', { reason: 'rpc', killSessions: request.payload.killSessions === true }) const serverClose = this.beginOrdinaryShutdownFence() // ... return {} + }Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c86cd00d-b379-4085-b8ac-765e3d95b3a0
📒 Files selected for processing (106)
config/reliability-gates.jsoncdocs/reference/plans/2026-07-17-stale-terminal-daemon-generation-reaping.mdsrc/main/daemon/client.test.tssrc/main/daemon/client.tssrc/main/daemon/daemon-entry.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-foreground-confirmation-protocol.test.tssrc/main/daemon/daemon-generation-audit.test.tssrc/main/daemon/daemon-generation-audit.tssrc/main/daemon/daemon-generation-inventory.tssrc/main/daemon/daemon-health.test.tssrc/main/daemon/daemon-health.tssrc/main/daemon/daemon-hello-protocol.tssrc/main/daemon/daemon-idle-shutdown.test.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-ownership-commit-migration.test.tssrc/main/daemon/daemon-ownership-commit-migration.tssrc/main/daemon/daemon-ownership-commit.tssrc/main/daemon/daemon-ownership-profile-index.tssrc/main/daemon/daemon-ownership-raw-extractor.test.tssrc/main/daemon/daemon-ownership-raw-extractor.tssrc/main/daemon/daemon-ownership-raw-field-classification.tssrc/main/daemon/daemon-ownership-raw-filesystem.tssrc/main/daemon/daemon-ownership-raw-remote-and-legacy.tssrc/main/daemon/daemon-ownership-raw-snapshot.test.tssrc/main/daemon/daemon-ownership-raw-snapshot.tssrc/main/daemon/daemon-ownership-raw-workspace.tssrc/main/daemon/daemon-pty-adapter.test.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-pty-router.test.tssrc/main/daemon/daemon-pty-router.tssrc/main/daemon/daemon-reap-candidate-journal-file.tssrc/main/daemon/daemon-reap-candidate-journal-schema.tssrc/main/daemon/daemon-reap-candidate-journal-storage.test.tssrc/main/daemon/daemon-reap-candidate-journal.test.tssrc/main/daemon/daemon-reap-candidate-journal.tssrc/main/daemon/daemon-reconciliation-coordinator.test.tssrc/main/daemon/daemon-reconciliation-coordinator.tssrc/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-session-claims.tssrc/main/daemon/daemon-session-process-incarnation-posix.tssrc/main/daemon/daemon-session-process-incarnation-windows.tssrc/main/daemon/daemon-session-process-incarnation.test.tssrc/main/daemon/daemon-session-process-incarnation.tssrc/main/daemon/daemon-spawner.test.tssrc/main/daemon/daemon-spawner.tssrc/main/daemon/degraded-daemon-fallback-shutdown.tssrc/main/daemon/degraded-daemon-provider-events.tssrc/main/daemon/degraded-daemon-pty-provider.test.tssrc/main/daemon/degraded-daemon-pty-provider.tssrc/main/daemon/degraded-daemon-session-routes.tssrc/main/daemon/production-launcher.test.tssrc/main/daemon/production-launcher.tssrc/main/daemon/retirement-pending-claim-reconciliation.test.tssrc/main/daemon/retirement-pending-claim-reconciliation.tssrc/main/daemon/types.tssrc/main/index.tssrc/main/ipc/orca-profile-project-transfer-handler.tssrc/main/ipc/orca-profiles.test.tssrc/main/ipc/orca-profiles.tssrc/main/ipc/pty.test.tssrc/main/ipc/pty.tssrc/main/orca-profiles/profile-cloud-index.tssrc/main/orca-profiles/profile-cloud-service.test.tssrc/main/orca-profiles/profile-index-initialization.tssrc/main/orca-profiles/profile-index-store.test.tssrc/main/orca-profiles/profile-index-store.tssrc/main/orca-profiles/profile-ownership-authority-seed.tssrc/main/orca-profiles/profile-project-daemon-ownership-transfer.test.tssrc/main/orca-profiles/profile-project-daemon-ownership-transfer.tssrc/main/orca-profiles/profile-project-session-state.tssrc/main/orca-profiles/profile-project-session-transfer.tssrc/main/orca-profiles/profile-project-source-removal.tssrc/main/orca-profiles/profile-project-state-file.tssrc/main/orca-profiles/profile-project-terminal-binding-ids.tssrc/main/orca-profiles/profile-project-transfer-lineage-raw.test.tssrc/main/orca-profiles/profile-project-transfer-lineage.tssrc/main/orca-profiles/profile-project-transfer-payload.tssrc/main/orca-profiles/profile-project-transfer-recovery.test.tssrc/main/orca-profiles/profile-project-transfer-recovery.tssrc/main/orca-profiles/profile-project-transfer-topology.tssrc/main/orca-profiles/profile-project-transfer.test.tssrc/main/orca-profiles/profile-project-transfer.tssrc/main/persistence.test.tssrc/main/persistence.tssrc/main/providers/local-pty-provider.tssrc/main/providers/pty-binding-provider.tssrc/main/providers/pty-exit-payload.tssrc/main/providers/pty-natural-exit-reconciliation.test.tssrc/main/providers/pty-natural-exit-reconciliation.tssrc/main/providers/types.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/shared/constants.tssrc/shared/daemon-session-ownership.tssrc/shared/orca-profiles.tssrc/shared/types.tstests/e2e/daemon-generation-reaping.spec.tstests/e2e/fixtures/daemon-generation-entry.tstests/e2e/fixtures/daemon-generation-marker.cjstests/e2e/helpers/daemon-generation-fixtures.tstests/e2e/helpers/daemon-generation-processes.tstests/e2e/helpers/daemon-generation-v21-release.ts
7c91590 to
4fff463
Compare
Jinwoo-H
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75a984d9-e4a0-4e8a-ae87-be379c63dbba
📒 Files selected for processing (22)
docs/reference/plans/2026-07-18-daemon-lifecycle-retirement.mdsrc/main/daemon/client.test.tssrc/main/daemon/client.tssrc/main/daemon/daemon-entry.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-foreground-confirmation-protocol.test.tssrc/main/daemon/daemon-hello-protocol.tssrc/main/daemon/daemon-idle-shutdown.test.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-self-retirement-respawn.test.tssrc/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-spawner.test.tssrc/main/daemon/daemon-spawner.tssrc/main/daemon/production-launcher.test.tssrc/main/daemon/production-launcher.tssrc/main/daemon/types.tstests/e2e/daemon-lifecycle-retirement.spec.tstests/e2e/fixtures/daemon-lifecycle-entry.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/daemon/production-launcher.test.ts
- src/main/daemon/daemon-spawner.test.ts
- src/main/daemon/daemon-main.ts
- src/main/daemon/daemon-server.test.ts
- src/main/daemon/daemon-entry.ts
- src/main/daemon/daemon-spawner.ts
4fff463 to
1569c55
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/daemon/daemon-init.ts (1)
273-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider sharing the child-shutdown helper with
src/main/daemon/production-launcher.ts.
terminateLaunchedDaemonChildandshutdownChildduplicate the SIGTERM/SIGKILL cleanup path andESRCHhandling, so the two launchers can drift over time.src/main/daemon/production-launcher.ts (1)
16-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPartial
pidPath/launchNoncesilently skips identity publication.The fork-args gate (
pidPath && launchNonce, Line 31) and the PID-write gate (Line 49) both require both values. If a caller ever passes only one (e.g. a future bug in thedaemon-spawner.tscall site), the launcher silently skips ownership-record creation and CLI flags with no error — unlikedaemon-entry.ts'sparseArgs, which explicitly throws when only one of--pid-record/--launch-nonceis supplied. Consider asserting this invariant on the launcher side too, so a caller mistake fails loudly instead of producing a daemon with no ownership metadata.🛡️ Suggested guard
const entryPath = opts.getDaemonEntryPath() + + if ((pidPath && !launchNonce) || (!pidPath && launchNonce)) { + throw new Error('pidPath and launchNonce must be provided together') + }Please confirm the
daemon-spawner.tscall site always passes both together (or neither) for this launcher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a57b3bc7-f40a-4ffa-8bb0-6d26a812dbec
📒 Files selected for processing (21)
docs/reference/plans/2026-07-18-daemon-lifecycle-retirement.mdsrc/main/daemon/client.test.tssrc/main/daemon/client.tssrc/main/daemon/daemon-entry.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-foreground-confirmation-protocol.test.tssrc/main/daemon/daemon-hello-protocol.tssrc/main/daemon/daemon-idle-shutdown.test.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-self-retirement-respawn.test.tssrc/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-spawner.test.tssrc/main/daemon/daemon-spawner.tssrc/main/daemon/production-launcher.test.tssrc/main/daemon/production-launcher.tssrc/main/daemon/types.tstests/e2e/daemon-lifecycle-retirement.spec.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/main/daemon/daemon-foreground-confirmation-protocol.test.ts
- src/main/daemon/daemon-entry.test.ts
- src/main/daemon/daemon-hello-protocol.ts
- src/main/daemon/daemon-spawner.test.ts
- src/main/daemon/daemon-main.ts
- src/main/daemon/types.ts
- src/main/daemon/daemon-entry.ts
- src/main/daemon/daemon-server.test.ts
- src/main/daemon/daemon-spawner.ts
- src/main/daemon/client.test.ts
- src/main/daemon/client.ts
- src/main/daemon/daemon-init.test.ts
1569c55 to
c8a98b7
Compare
Co-authored-by: Orca <help@stably.ai>
Co-authored-by: Orca <help@stably.ai>
Co-authored-by: Orca <help@stably.ai>
Co-authored-by: Orca <help@stably.ai>
c8a98b7 to
cdc89e1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/daemon/production-launcher.ts (1)
138-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared child-termination protocol into one helper. Both launchers implement the same SIGTERM→5s grace→SIGKILL→1s force-exit sequence (with identical timings) plus a copy of
isNoSuchProcessError. Keeping two copies of this safety-critical teardown in sync is error-prone; a fix to one (timing, exit-listener handling, ESRCH tolerance) could silently miss the other.
src/main/daemon/production-launcher.ts#L138-L221: replaceshutdownChild/isNoSuchProcessErrorwith the shared helper (its 5s/1s intervals are hardcoded here vs. named constants in daemon-init).src/main/daemon/daemon-init.ts#L273-L339: replaceterminateLaunchedDaemonChild/isNoSuchProcessErrorwith the same shared helper, passing the grace/force constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc2113a2-9336-46c7-845a-580109955431
📒 Files selected for processing (23)
config/scripts/windows-daemon-workspace-close-repro.mjsdocs/reference/plans/2026-07-18-daemon-lifecycle-retirement.mdsrc/main/daemon/client.test.tssrc/main/daemon/client.tssrc/main/daemon/daemon-entry.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-foreground-confirmation-protocol.test.tssrc/main/daemon/daemon-hello-protocol.tssrc/main/daemon/daemon-idle-shutdown.test.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-self-retirement-respawn.test.tssrc/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-spawner.test.tssrc/main/daemon/daemon-spawner.tssrc/main/daemon/production-launcher.test.tssrc/main/daemon/production-launcher.tssrc/main/daemon/types.tstests/e2e/daemon-lifecycle-retirement.spec.tstests/e2e/fixtures/daemon-lifecycle-entry.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/main/daemon/daemon-foreground-confirmation-protocol.test.ts
- src/main/daemon/daemon-entry.test.ts
- src/main/daemon/daemon-hello-protocol.ts
- tests/e2e/fixtures/daemon-lifecycle-entry.ts
- src/main/daemon/types.ts
- src/main/daemon/client.test.ts
- src/main/daemon/daemon-entry.ts
- src/main/daemon/daemon-spawner.test.ts
- src/main/daemon/daemon-self-retirement-respawn.test.ts
- src/main/daemon/production-launcher.test.ts
- src/main/daemon/daemon-server.test.ts
- src/main/daemon/daemon-spawner.ts
- src/main/daemon/client.ts
- src/main/daemon/daemon-idle-shutdown.test.ts
- src/main/daemon/daemon-pty-adapter.ts
2 participants
ELI5
Orca intentionally leaves terminal daemons alive when the app disconnects so live terminals can
reattach after an update or crash. The missing piece was that a new daemon with no terminals also
stayed alive forever.
This change lets a new v24 daemon retire itself only when it can atomically prove it has no terminal,
no other client, no unknown connection, and no terminal creation in progress. Live terminals keep
the existing survival behavior.
Before / after
Root cause
Daemon endpoints are versioned by protocol, so upgrades can create a new generation while retaining
older generations for live PTY reattachment. The daemon had no authenticated, race-safe empty
retirement operation, and app startup did not have enough evidence to safely kill a process.
What changes
mainassigned v23 to the macOS login-shell changewhile this work was in progress, so v23 remains a legacy generation.
per-launch nonce. Both authenticated sockets must report the same identity.
shutdownIfIdle. It synchronously closes admission before acknowledging onlywhen the authenticated requester is the sole complete client, every transport is accounted for,
no create/attach is in flight, and there are zero sessions.
and unknown transports block retirement; an exactly empty daemon exits immediately.
control+stream adoption cancels it permanently, so it is never a terminal inactivity timeout.
replacement can block retirement but cannot erase its evidence.
with bounded control/stream handshakes and deterministic failure cleanup.
there is no retirement gap while a relaunched app adopts the daemon.
observed an authenticated disconnect.
retain an explicit total budget and generation fence against late socket resurrection.
There is no app-startup kill, no legacy-daemon retirement, and no inference from worktree/layout
absence.
Scope split and archived work
The larger ownership persistence, cross-profile audit, candidate journal, transfer recovery, and
enforcement prototype has been removed from this PR. It remains recoverable at:
Jinwoo-H/issue-9138-full-ownership-audit-snapshot7c915909bd26670b8af36aa683cff85077395dd1The narrowed design incorporates the safety constraints from AmethystLiang's reviewed design:
#9138 (comment)
Performance
This PR adds no polling, process scan, profile enumeration, ownership checksum, candidate journal,
per-byte hashing, or steady-state persistence write. Lifecycle bookkeeping runs on connection,
disconnection, terminal admission/exit, and quit events. Startup adds one authenticated two-socket
lease and one unref'ed pre-adoption watchdog; clean quit adds one bounded RPC. There is no runtime
disconnect timer after adoption.
The local machine was too busy for publication-grade absolute benchmarks (load averages 17-38 with
unrelated browser, simulator, VM, and Orca work). A paired same-host regression screen bracketed five
mainv23 samples with ten v24 branch samples:mainv23listSessionsRPCAll medians were within about 5%, but individual throughput/event-loop samples varied widely under
host load. Treat this as evidence against a large regression, not a precise delta.
Validation
ownership safety, connection/respawn races, process termination, and test fidelity.
independent review tracks after timeout and rejection-path findings were fixed.
after its final authenticated client disconnected and removed only its owned PID/token/socket
artifacts.
git diff --checkpassed.cdc89e1294: repository verify, packaged Windows crash-survival,Windows terminal restart regressions, and Ubuntu/Windows native smoke including the built-daemon
ConPTY workspace-close repro.
The local shell used Node 26.5.0 while the repository requests Node 24. Existing Vite chunk/CSS,
Swift deprecation, and CLI symlink warnings remained; the build completed successfully. CI provides
the supported Node and platform matrix.
Tradeoffs
relaunch an empty daemon instead of reusing it, adding bounded reconnect/startup latency but never
terminating a live session.
retirement contract. This PR prevents new v24 empty-generation buildup but does not clean existing
legacy generations.
Screenshots
No visual change.
AI Review Report
Three independent reviewers completed approximately 18 review loops until clean. Valid findings were
fixed and regression-tested, including shutdown reply flushing, startup child cleanup, adoption
lease handoff/disposal races, second-client preservation, monotonic retirement across partial
clients, complete-pair-only terminal admission, exact PID-record cleanup, launcher ownership-pair
validation, and E2E teardown.
Security Audit
as one daemon connection.
make emptiness uncertain.
Notes
The broader ownership-audit prototype remains archived at the branch and commit above; it is not
part of this PR. Full repository lint still stops on the pre-existing unrelated exhaustiveness error
in
src/renderer/src/components/skills/skill-freshness-group.tsx:101; all changed files pass focusedlint.
Refs #9138
Made with Orca 🐋