Skip to content

fix(daemon): preserve recorded ownership through stop and cleanup - #3806

Merged
thymikee merged 4 commits into
mainfrom
fix/daemon-recorded-owner-safety
Oct 19, 2026
Merged

thymikee merged 4 commits into
mainfrom
fix/daemon-recorded-owner-safety

Conversation

@thymikee

@thymikee thymikee commented Oct 19, 2026 •

Copy link
Copy Markdown
Member

Summary

Stop, takeover, drain, and cleanup use ownership and lifetime from one registration read. Central retirement refuses a live embedded or unrecognized owner without a signal. It retires a provably exited owner, so a crashed host cannot block managed startup. Legacy disk records without ownership remain detached.

The review repair removes premature client guards, maps retained owner reasons once, consolidates registration projections, removes an unused parameter, and corrects the embedded-stop fixture. Help and docs distinguish unreadable plain-stop metadata (daemon-record-unreadable) from the early --clean refusal (daemon-clean-not-detached).

This prerequisite for #3797 supports #3747 and #3757. It changes 36 files, with 2,212 gross lines. The public host API remains later work.

agent-device daemon stop --state-dir /tmp/agent-device

Validation

Head e574ba365abbce8766bbed17ff2bae48ae8dd0f4, merge base 8e9361a789. The managed-command regression fails at old b7df117 for dead embedded/unrecognized owners and passes at the repair, with no signal to the old owner.

pnpm check:affected --run --head e574ba36: 36 runnable checks passed. Receipt run-k8c1I5: exactCommit=true, checkoutChanged=false. GitHub-owned skips are not passes. This lifecycle change needs no device run.

Independent code and smaller-design review approved f5b44dfa. Independent sentence review approved the final prose at e574ba36; no findings remain. Required CI remains pending.

@thymikee
thymikee added this pull request to stack #3807 October 19, 2026 06:56
@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-19 14:07 UTC

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.42 MB 5.42 MB +4.9 kB
Package (unpacked) 5.42 MB 5.42 MB +4.9 kB
Package (download) 1.64 MB 1.64 MB +1.5 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 40.6 ms 40.7 ms +0.1 ms
CLI --help 73.6 ms 72.9 ms -0.8 ms

@thymikee

Copy link
Copy Markdown
Member Author

The ownership change has one problem that needs a fix before merge, at b7df117. All checks pass, including the device smoke jobs that reuse the shared daemon through the changed code.

readReusableLocalDaemon checks for an embedded or unknown owner itself and throws before it calls retireDaemonForTakeover. So the central rule in stopAndRetireDaemon never runs on this route. That rule proves the owner has exited, sends no signal, and retires the dead owner's record. Take a host that crashed and left daemon.json and daemon.lock behind (state held, pid dead). Every managed CLI command then fails with daemon-owner-embedded and the hint "Close the host that started this runtime", for a host that no longer exists. A dead unknown record fails the same way with daemon-owner-unrecognized. Meanwhile daemon stop and the replaceUnreachable path in ensureDaemon recover the same record, so two routes disagree about one ownership fact. Once an embedded host (#3757) crashes, the state directory stays wedged for every implicit CLI or Node command. This also contradicts the PR's "dead-owner recovery remains available". The rule to enforce: a client refuses a takeover only when stopAndRetireDaemon reports an owner refusal (retained reason embedded-owner or owner-unproven), and ownership is judged once, inside stopAndRetireDaemon, for every takeover route (readReusableLocalDaemon, replaceUnreachable, quiesceOrRetireUnresponsive, awaitDrainedDaemon and forceRetireDaemon). Please delete the two guards, and map those retained reasons to daemonNotRetirableError / daemonRecordOwnerUnrecognizedError in retireDaemonForTakeover. That also gives replaceUnreachable the owner-specific reason instead of the generic retirement-unconfirmed one. To prove it, add a test with a dead embedded record of an incompatible version, then run a managed command and assert it starts a fresh daemon and sends no signal. The embedded-lifecycle tests today only use live owners.

Not blocking, and you can take or leave these: the test at daemon.test.ts:389 stubs stopRecordedDaemon to a graceful stop, while the real one throws daemon-owner-embedded for that record, so it describes #3797 and not this PR; stopRecordedDaemon takes host but never reads it, and the lifecycle-mode rename, the mode parameter and the one-element OWNER_REFUSALS could wait for #3797's second mode; readCleanupIdentity and the conversion at daemon-client-stop.ts:27 do the same DaemonStopIdentity to ObservedDaemonIdentity step, and readDaemonStopIdentity repeats readRegistration, so one reader in daemon-registration.ts would serve both; and stopRecordedDaemon rebuilds the owner errors from string literals when retirement.reason and the existing reason constants could map them once inside daemonRetirementError.

Could the PR be simpler? Keep the central gate in stopAndRetireDaemon and the runtime ownership option. Delete the guards in readReusableLocalDaemon and resolveTimeoutRecovery. Map the retained owner reasons once. Drop the mode rename and the unused host until #3797 adds a second mode. That removes roughly 150 production lines and the divergence above, and it needs nothing else to change first.

I found the problem by reading the code (observeLocalDaemon, resolveDaemonTakeover, then the guard), not by running tests. No production writer sets ownership: 'embedded' at this head, so that path is reachable today only through tests or #3757. The unknown variant is reachable now through a hand-edited record or one from a newer build. I did not re-run your gate receipt or the omitted-owner test on the old code. No conflicts are known. Before merge, drop the two guards and add the dead-owner recovery test on the managed route.

@thymikee

Copy link
Copy Markdown
Member Author

The managed-takeover finding is valid at b7df117. The client guards reject a dead embedded or unknown owner before the central retirement path can prove its exit. My earlier ready report missed this issue comment.

A repair is in progress. It will remove the premature owner checks, retain the central no-signal rule, and preserve the typed owner refusal reasons. The regression will exercise managed startup with a dead registered host. The worker will also assess the optional simplifications against both stack layers.

After #3806 is repaired, #3797 will be rebased and validated on it. Both layers need fresh independent review and CI before I report them ready again.

readReusableLocalDaemon rejected an embedded or unknown owner before the
central retirement could look at it, so a record whose owner had already
exited wedged every managed command instead of recovering. Delete the two
client guards and the resolveTimeoutRecovery ownership guard: ownership is
judged inside stopAndRetireDaemon on every takeover route
(readReusableLocalDaemon, replaceUnreachable, quiesceOrRetireUnresponsive,
awaitDrainedDaemon and forceRetireDaemon alike). Map a retained owner
reason to the existing typed errors once, in daemonRetirementError for the
stop route and daemonTakeoverRetirementError for a takeover, and prove the
managed route recovers a crashed embedded or unrecognized owner without
signaling it.

#3806 (comment)
@thymikee
thymikee force-pushed the fix/daemon-recorded-owner-safety branch from b7df117 to f5b44df Compare October 19, 2026 13:27
@thymikee

Copy link
Copy Markdown
Member Author

Round 2 pushed at f5b44dfa (rebased on current main; the prior head was b7df117c).

Fixed as judged: readReusableLocalDaemon and resolveTimeoutRecovery no longer pre-judge owners — takeover, stop, drain, and timeout recovery all ask stopAndRetireDaemon, and the retained embedded-owner/owner-unproven reasons map once to the typed errors (daemonRetirementError, daemonTakeoverRetirementError). The unused host param is gone; all four record projections now share one reader.

Regression evidence: new managed-command rows create a real joined child, publish an embedded/unknown/detached record, SIGKILL it, and run a managed request. At b7df117 the embedded row fails daemon-owner-embedded and the unrecognized row fails daemon-owner-unrecognized from the removed guards; at f5b44dfa all three retire without signaling and start a fresh daemon (isolated base checkout per test-experiments).

Declined with reason: mode rename/OWNER_REFUSALS centralization — it lands on #3797's stop route; folding it here pushes the dependent layer past its size budget.

Gate: check:affected --run --head f5b44dfa receipt in-thread (passed, exactCommit, checkout unchanged). #3797 rebased to 23a68a41, range-diff clean, finalizer/close/f0ea5fe deltas preserved. Independent review and CI still required.

@thymikee

Copy link
Copy Markdown
Member Author

The PR is ready. The new managed-route regression and the stop and cleanup changes at f5b44df fix the ownership problem from the earlier review at b7df117, and I found nothing that needs a change.

Not blocking: emitDaemonTakeoverNotice in daemon-client-startup.ts now prints "Replacing daemon (pid X, vY) ..." before the retirement runs, so a live embedded or unrecognized runtime of another version still gets the notice and then a daemon-owner-embedded or daemon-owner-unrecognized refusal. You could print the notice after retirement succeeds, or word it as an attempt. The inline reason === 'embedded-owner' || reason === 'owner-unproven' test in daemon-stop.ts repeats isOwnerRefusal from daemon-registration-owner.ts, so exporting that and using it here would keep one source. Take or leave both.

CI has not finished: 12 of 18 checks are still queued or running, and none has failed. Integration Tests and Coverage cover the changed route, including daemon-client startup and ensure, smoke-daemon-clean and the embedded-lifecycle tests. I did not run the new managed-route regression on b7df117, so "it fails there" rests on reading the old guard and its observation path. No conflicts are known. Before merge, Integration Tests, Coverage and the smoke jobs must go green on f5b44df.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 19, 2026
…cord

daemon stop --clean checks the record's owner before the stop route reads it, so an unreadable
record fails with daemon-clean-not-detached, not daemon-record-unreadable. The help and docs said
the latter unconditionally; qualify it with the flag that decides it.

#3806 (comment)
@thymikee

thymikee commented Oct 19, 2026 •

Copy link
Copy Markdown
Member Author

Independent different-provider review is complete for #3806 at e574ba365abbce8766bbed17ff2bae48ae8dd0f4. Codex/Sol approved the OpenCode/Apex author's ownership repair at f5b44dfa, then approved the final three-line help/documentation correction at e574ba36. High confidence; no findings remain.

The blocking dead-owner recovery defect is fixed. Managed takeover reaches central retirement; proven-dead embedded and unknown records recover without signals. Live-owner protection remains intact. The fixture, unused host parameter, duplicated registration projections, and string error reconstruction are fixed. Single-mode scaffolding remains a valid optional deferral.

The final sentence review confirms that plain unreadable stop reports daemon-record-unreadable, while --clean refuses embedded, unknown and unreadable records with daemon-clean-not-detached. Runtime implementation and test blobs are unchanged from the reviewed repair. The gross diff remains 2,212 lines.

The exact local receipt run-k8c1I5 passed with exactCommit=true and checkoutChanged=false. All three comment surfaces were read. No open inline threads remain. CI is still running; the PR is not ready yet.

@thymikee

thymikee commented Oct 19, 2026 •

Copy link
Copy Markdown
Member Author

P3 fixed at e574ba36 (docs-only commit on the approved f5b44dfa; that approval and receipt stay attributed to f5b44dfa). Help and both docs now qualify daemon-record-unreadable as the no---clean outcome and name daemon-clean-not-detached for --clean against host-owned, unrecognized or unreadable records — matching the CLI's existing refusal table.

Gate at exact e574ba36: passed, exactCommit=true, checkoutChanged=false (run-k8c1I5). Gross unchanged: 2,212.

#3797 rebased to d2f2eb7d on this head; runtime implementation and test blobs unchanged from reviewed 23a68a41 (range-diff: three doc lines carry the same qualification into B's overriding sentences). Gate at exact d2f2eb7d: passed, exactCommit=true, checkoutChanged=false (run-gr2nMV). Gross 2,497. Both PRs mergeable. No merges; independent review of the prose/interaction delta and CI still required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant