fix: centralize daemon startup ownership and retirement

[thymikee]

Summary

Centralize daemon registration, startup ownership and retirement. Replace the bespoke lock with process-lock exclusion; signal only a proven process lifetime and remove metadata/private state after confirmed exit and protected registration checks.

A monitored child can recover a missed birth probe from matching held-lock/registration records. Recovery and client observation share the identity predicate. Includes retained-guard diagnostics and legacy-concurrency policy. Consolidates six original layers across 41 files. Related to #3116.

Rebased onto the landed session stack at 052cba83c1. Repair markers keep scoped owner checks and clearing of owned expired markers, together with strict parser validation and fail-closed cleanup scanning. Per-call cancellation remains separate from awaited timeout retirement.

Validation

5b12609ef5: pnpm check:affected --run passed 6,579 related tests in 803 files, twelve documentation controls and selected tooling. All 96 focused controls pass. The new hanging restart-health probe control rejects a planted missing-abort-check mutation and proves zero timeout-retirement calls. Earlier birth-recovery regression proof remains at its original head. No quality baseline changed.

Fresh exact-head GitHub CI and review remain pending. User handles merges.

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	5.03 MB	5.04 MB	+7.7 kB
Package (unpacked)	5.03 MB	5.04 MB	+7.7 kB
Package (download)	1.51 MB	1.51 MB	+2.3 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	27.6 ms	27.1 ms	-0.6 ms
CLI --help	86.3 ms	84.5 ms	-1.8 ms

[cubic-dev-ai]

All reported issues were addressed across 6 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[thymikee]

I found one problem at 681b785 that should be fixed before merge. CI is green (14 checks, none failing), and I know of no conflicts.

When no owned startup matches the observed pid and start time, stopAndRetireDaemon returns retained before it calls terminate. The client reads launch.startTime with a timed ps call right after spawn. If that read returns null under load while the daemon's own read succeeds, isLaunchedDaemon fails, ensureLocalDaemon adopts the daemon as reusable, and cleanupDaemonAfterRequest still runs because ownedStateDir is set. The result is ownership-unproven, and the daemon this client launched is never signaled. A successful one-shot replay then comes back as COMMAND_FAILED daemon_retirement_unconfirmed, and a live daemon stays running in a mkdtemp directory with no idle exit. The old code always called stopDaemonProcessForTakeover here, and stopDaemonProcess already gates that stop on pid and start time. The rule should be that missing proof of deletion authority never removes stop authority. On ownership-unproven, please still run the identity-gated retireObservedDaemon with owned=undefined, then return retained with removedStateDir: false. Please add a test where launch.startTime is undefined but the registration carries a real start time, and assert the child exits.

Not blocking, take or leave: the owned-startup match at line 580 restates isLaunchedDaemon in daemon-client-lifecycle.ts, so one launch-identity predicate in the registration owner could serve both sites, and requirePrivateReplayState compares paths that could come from the capability itself; the lifecycle test diff also drops unrelated constraint comments, such as the one about binding fresh before freeing the port, which are worth restoring.

Would making OwnedReplayStateDir a class owned by daemon-registration-owner.ts be simpler? With ES-private fields (#startups, #sealed, #retirement) and launch(args, serverMode) and retire(observed, mode) methods, an instance cannot be forged. That would replace the WeakMap, the symbol brand, the cast and the paths comparison, and stopAndRetireDaemon would keep its #3126 signature while the private path becomes a thin wrapper (join, evidence check, then rmSync under the held acquisition). It needs nothing beyond #3126 landing.

I did not run the mutations the PR describes; I only confirmed the last-startup one by reading the test. The finding above depends on readProcessStartTime returning null on the client right after spawn, which I inferred from the ps timeout in host-process.ts and did not reproduce. I also did not run the import-budget gate.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-04 19:04 UTC

[thymikee]

Reviewed ea4057a. One defect from the earlier review (#3127 (comment)) is still open, and nothing else is. CI is green: 21 checks, none failing. No conflicts.

The start-time problem in https://github.com/callstack/agent-device/blob/ea4057a/src/daemon-registration-owner.ts#L239 is unchanged, and the diff from fa98d0c to ea4057a is empty for daemon-registration-owner.ts and daemon-client-lifecycle.ts. launchDaemonProcess still takes launch.startTime from one synchronous readProcessStartTime(pid) right after spawn. If that read returns null under load, isLaunchedDaemon (daemon-client-lifecycle.ts:803-807) never matches, so the client does not adopt its own healthy daemon. The default route then reports daemon_startup_failed at the deadline for a daemon it started. On one-shot replay, stopAndRetireDaemon (daemon-registration-owner.ts:316-325) returns ownership-unproven, so the live child and its temp directory stay behind. I read this from the code and did not reproduce a null read. The rule to satisfy: while a monitored launch has not exited, its identity is its pid plus the start time the child published. The registration owner holds that identity, and both isLaunchedDaemon and the owned match in stopAndRetireDaemon read it. When launch.startTime is missing and exit is unresolved, can it be taken from the held lock owner record and daemon.json, only when both name launch.pid with the same non-empty start time, and then recorded on the launch? stopDaemonProcess would still re-verify the start time before it signals. Please add two sendToDaemon-level regressions with readProcessStartTime returning null once at launch. The default route must return ready with startedByClient=true. Replay must retire its child and remove the directory. Both must fail at ea4057a and pass after the fix.

The two cubic-dev-ai P2 threads are fixed at this head, so you can resolve them. The release-error one (#3127 (comment)) is covered by process-lock.ts:183 and its test. The auth-token guidance one (#3127 (comment)) is covered by daemon-client-address-hints.ts:22-25 and its test.

I did not run tests or mutations, so the 6,316-test count is unverified. I also did not check whether the process.kill spy in the managed-adb fixture hides a real macOS group-signal outcome on the production adb route. Before merge, the start-time fix and its two regressions need to land.

[thymikee]

The new Repo Guards failure at 20e32800b7 came from the CI merge with main 294dc7d024: #3199 added a recording-stop timeout control using mockIsDaemon and mockStop, which this PR had removed when it moved timeout retirement controls to real registered children. The job log reports both missing symbols.

Rebased onto that relevant main change and corrected the fixture at 0d1c7b3087c2e779d1df2c5387b44e26e4a439c1. The new control now owns a real registered child and asserts no termination/probe dispatch, unchanged registration and retained lock, and a still-live child after timeout. It does not fake successful process signaling. All ten timeout-route controls and check:freerange pass. The final exact-head affected gate is queued after the existing serialized session stage; CI remains separate.

The latest transient launch-start-time review remains open. This update addresses the user's requested review work on #3135/#3140/#3144 and the related new-main CI conflict; it does not claim that missing-birth launch recovery is implemented. The reviewer asks for two sendToDaemon controls, default startup and private replay, with the first process-start-time probe returning null. Positive child-published identity must remain centrally owned; no weakened signaling/deletion proof is proposed.

The user handles all merges; the monitor no longer merges anything.

[thymikee]

The earlier P1 on launch.startTime is still open at 0d1c7b3. The Repo Guards conflict from the #3199 merge is fixed, because this commit removes both mocks.

launchDaemonProcess sets launch.startTime from one synchronous readProcessStartTime(monitored.pid) call right after spawn (https://github.com/callstack/agent-device/blob/0d1c7b3/src/daemon-registration-owner.ts#L237). If that ps read returns null, isLaunchedDaemon never matches, so the client never adopts its own healthy daemon. retireStartupAttempt then passes a null start time, and the owned match in stopAndRetireDaemon throws daemon_private_startup_unowned. On the default route, the client reports daemon_startup_failed at the deadline (about 16 s) for a daemon it launched itself. On one-shot replay, the live child and its private state directory stay behind until the idle reap. The rule the code must satisfy: while a monitored launch has not exited, its identity is its pid plus the start time the child published. The registration owner should record that identity on OwnedStartup.launch. When launch.startTime is missing and the launch has not exited, fill it from the held lock owner and daemon.json, but only when both name launch.pid with the same non-empty start time. isLaunchedDaemon and the owned match in stopAndRetireDaemon should both read the recorded value, and stopDaemonProcess should still re-verify birth before it signals. To validate, please add two sendToDaemon-level regressions where readProcessStartTime returns null once at launch. The default route must return ready with startedByClient=true. The replay route must retire its child (pid not alive) and remove the private state directory. Both must fail at 0d1c7b3.

The two open inline threads on process-lock release logging (#3127 (comment)) and remote hint guidance (#3127 (comment)) do not apply anymore. Both files are unchanged since ea4057a and already cover the points raised, so you can resolve them.

I did not run tests or mutations at 0d1c7b3, so the ten timeout-route controls and check:freerange are unverified by me. The finding above comes from reading the code, and I did not reproduce a null start time right after spawn under load. CI is still pending, and Smoke, Integration and Coverage all run the changed startup and retirement route. No device-facing path changed, so no live device run is needed. Before merge, please land the owner-recorded launch birth fix with its two regressions, then get CI green, Smoke Tests included.

[thymikee]

The monitored-launch birth fix is now published at 36d615abd2. When the initial host probe returns null, launch.readIdentity() can record the child-published birth only while the monitored child has not exited and its held lock and registration name the same PID and non-empty birth. Startup adoption and private retirement use this owner-held identity. Existing kernel birth verification before signaling is unchanged.

Both requested sendToDaemon controls failed on 0d1c7b3: ordinary startup and private replay reported daemon_startup_failed. They now prove startedByClient=true; private replay also proves child exit and directory removal. Three additional controls refuse different PIDs, different births and joined children. The existing missing-proof controls now explicitly omit published birth proof, so they still prove retention rather than treating a recoverable host-probe miss as permanently unowned.

All 69 focused controls pass. The exact-head pnpm check:affected --run gate passes 6,336 related tests in 780 files, twelve documentation controls and selected tooling. The complexity gate initially rejected the added branching; the owning check was simplified, with no baseline relaxation. Fresh GitHub CI remains pending. The user handles all merges.

[cubic-dev-ai]

All reported issues were addressed across 4 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[thymikee]

The earlier findings on 0d1c7b3 are fixed at 36d615a, and I found no new problems in the code. The startup adoption and private retirement route now has regressions for the old behavior, but I did not run them on 0d1c7b3 or on 36d615a. Their failure on the old code comes from reading the earlier isLaunchedDaemon and the mocked null birth probe.

CI is green: 21 checks, none failing at 36d615a, including Smoke, Integration and Coverage, which exercise the changed route. I did not run check:affected or the complexity gate, and I rely on the CI status for both.

The PR has merge conflicts with main, so I could not check how the lifecycle hunks merge. Please rebase onto main and get CI green again on the rebased head before merge, since a rebase could change those hunks.

Not blocking, and you can take or leave these: recoverStartupBirth in daemon-registration-owner.ts and registrationAllowsDaemonObservation in daemon-client-lifecycle.ts both ask whether the held process lock agrees with the registered pid and birth. Would one shared predicate, next to readRegisteredDaemonIdentity in daemon-registration.ts, keep the two answers from drifting?

One open inline thread still applies, the Cubic one on the awaitFile deadline. It reads the stubbed Date.now(), so a missing fixture ends in the generic test timeout. It only affects failure messages, not production.

[thymikee]

Rebased #3127 onto main at ce62bee0cc; the published head is 353fd1bcedba6b1be5c9bdaaa164128339658bd8.

The conflicting transport changes came from #3200. The composed implementation retains its per-call cancellation guard, fallback/retry abort checks, connection close, and typed cancellation outcome. Only a genuine transport timeout enters the central awaited retirement path. Health requester loading and response completion still share the probe budget; a caller abort is classified separately from probe expiry.

I adopted your shared-predicate suggestion: processLockHoldsDaemonIdentity now owns the matching held-lock plus non-empty birth rule, and both startup-birth recovery and client observation use it. Observation's existing absent-lock policy remains explicit at its caller. The shared fixture wait now bounds attempts independently of Date.now, and its inline thread is answered/resolved.

All 121 focused ownership, startup, cancellation, transport and wire controls passed. The exact-head affected gate passed 6,377 related tests in 788 files, twelve documentation controls and selected runnable tooling checks. The complexity audit initially rejected the merged probe; endpoint resolution and timeout/cancellation classification are now separate small functions, with no threshold or baseline changes. Fresh GitHub CI and review are pending. Nothing was merged.

[thymikee]

The PR is ready for human review. At 353fd1b the rebase resolves the earlier conflict, and I found no code problems in the transport, timeout and retirement changes. A caller abort stays a typed cancellation, and only a real transport timeout enters the awaited retirement path.

Not blocking, take or leave both: no test covers a caller abort that lands while the remote instance-mismatch health probe is still running (a 409 mismatch, a hanging health server, then an abort, asserting a canceled-request error and zero handleRequestTimeout calls, in daemon-client-transport.test.ts). Also, the internal timeout that triggers retirement reuses the public daemon_transport_timeout reason that handleRequestTimeout returns at the end; a module-private error class or symbol would keep the two apart.

The earlier Cubic threads are fixed at this head: the startup-race test wait now caps at 200 attempts with a named assert, and request.on('error') returns early once the promise has settled.

CI is green at 353fd1b, with 21 checks and none failing, including the daemon-client unit and integration lanes that exercise this route. There are no conflicts. Nothing is left in code; the next step is your review of the rebased head.

[thymikee]

353fd1b now conflicts with main in src/session-repair-tombstone.ts after the latest merges. The code verdict from my earlier comment is unchanged. I removed the ready-for-human label until the conflict is resolved. Please rebase, and I will check the conflict resolution on the new head.

[thymikee]

Rebased onto the user-landed session stack at main 052cba83c1. Published head: 5b12609ef5b6dd140d5a8b60f09738281e73778b.

The repair-marker conflict is resolved without dropping either contract. The public reader still requires the scoped owner and expiry, and clearing still checks the owner while permitting removal of owned expired markers. The shared parser additionally validates finite reapedAt/expiresAt, source-path type and commit-failure shape. Cleanup scanning still fails closed on unreadable/malformed evidence and only reports unexpired failed commits. Both registration fixtures now pass the scoped owner argument.

I added the suggested restart-health abort control at its cancellation/timeout owner test: one 409 instance mismatch, one hanging health probe, caller abort, typed cancellation, one RPC, and zero timeout-retirement calls. Removing the post-probe abort check makes it fail with daemon-unavailable instead of cancellation; restoration passes. No private error class or discriminator was added; that suggestion remains non-blocking, and the public error shape is unchanged.

All 96 focused controls passed. The exact-head pnpm check:affected --run passed 6,579 related tests in 803 files, twelve documentation controls and all selected runnable tooling checks. Fresh CI and conflict-resolution review remain pending. All five session PR merge commits are verified ancestors of current main. Nothing was merged by this agent.

[thymikee]

The conflict from the earlier review (#3127 (comment)) is now fixed, and I found no problems in the rebased code at 5b12609. The rebase keeps both contracts: the tombstone check in src/session-repair-tombstone.ts still rejects a non-finite expiresAt and reapedAt, and the new abort test covers the case where refuseAbortedRequest is removed. I did not run the tests locally. I judged that the abort test fails without the fix by reading the code: removing refuseAbortedRequest at daemon-client-transport.ts:430 sends the call to the "Remote daemon is unavailable" error. I reviewed only the tombstone hunk of the rebased first commit and the new test commit; commits 2-5 are identical in the range-diff, and the earlier review covered them.

All 21 checks pass at 5b12609, and there are no conflicts. Nothing else stands in the way, so the PR is ready for a maintainer's review.

On the open thread from another reviewer: the cubic-dev-ai P2 thread on the tombstone check is fixed at this head, so you can resolve it: #3127 (comment)