Skip to content

fix: share daemon retirement and verify mixed-version cutover - #3132

Closed
thymikee wants to merge 2 commits into
fix/daemon-timeout-retirementfrom
fix/daemon-registration-cutover
Closed

thymikee wants to merge 2 commits into
fix/daemon-timeout-retirementfrom
fix/daemon-registration-cutover

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Manual daemon stop now awaits the shared ownership-checked retirement operation. Confirmed process exit alone cannot report success if registration retirement failed; primary errors retain typed reasons and normalized diagnostics. Existing CLI stop/report shapes are preserved.

Verifies mixed-version daemon exclusion at the actual shared lock path with owned child processes running the previous acquisition algorithm: an already-running old daemon, a new owner delaying metadata, and simultaneous startup. This proof does not expand host-kit's mixed-protocol support policy.

Depends on #3131. Ref #3116. Six files, 483 gross changed lines. ADR 0030 includes the ownership diagram and support boundary.

Validation

Tested 2b3dcfaae5c0422fcb36ea0fece9204bf87bfea4:

  • 52 focused tests, including real graceful/forced manual stops; zero skips.
  • Giving the new implementation a different lock path fails all three cutover controls; concurrent startup produces two winners.
  • Ignoring registration retirement after confirmed exit fails both retained-result controls.
  • pnpm check:quick, Fallow, and pnpm check:affected --base fix/daemon-timeout-retirement --run pass; 34 files/385 related tests.
  • Independent read-only review found a failure-path fixture leak; the bounded admission assertion fixes it and the mutant now fails without hanging. No remaining findings.
  • CI-owned coverage, provider and device checks remain pending.

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.96 MB 4.96 MB -76 B
Package (unpacked) 4.96 MB 4.96 MB -76 B
Package (download) 1.49 MB 1.49 MB -46 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.1 ms 27.0 ms +0.9 ms
CLI --help 81.1 ms 81.1 ms +0.1 ms

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 6 files

Re-trigger cubic

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 1 file (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/__tests__/daemon-registration-owner.test.ts
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

This PR is ready at ba4a5c2. The shared retirement path and the mixed-version cutover look correct from reading the code. I did not run the tests or your mutation controls.

Not blocking, and you can take or leave these: (1) In daemon-stop.ts, a graceful stop can race with another client starting a successor. The retirement then returns 'lock-busy' or 'registration-replaced' and daemon stop throws daemon_retirement_unconfirmed, where base returned success. The rule could be that a manual stop succeeds when the observed daemon's exit is confirmed and its registration no longer names it, with a warning instead of a throw. (2) No test runs daemon stop against spawnLegacyDaemonFixture (daemon-registration-owner.test.ts). A killed old daemon leaves its lock file, host-kit reports 'unproven', and the owner maps that to 'lock-busy', so every later stop fails. Graceful and forced cases against that fixture would cover it, and an 'unproven' timeout may fit 'ownership-unproven' better. (3) The 'registration-replaced' mock in daemon-stop.test.ts carries an error that production never attaches to that outcome. (4) The barrier-wait snippet and the stop body in legacy-daemon-fixture.ts repeat code from registered-daemon-fixture.ts. (5) The ADR line "The client refuses an older registration before signaling or changing it" (0030) holds for startup but not for manual stop, so it could say so.

The legacy check rebuilds the old lock algorithm from source. It does not run a released old daemon binary, so old servers, takeover replies and shutdown teardown are not exercised. This PR is stacked on #3131, so the shared owner module is reviewed there.

Smoke, Repo Guards, Coverage, Integration and cubic are still running, so there is no failure to attribute yet. Integration and Coverage run both changed test files, and they must pass on ba4a5c2 before merge. There are no conflicts.

This review covers ba4a5c2. The newer head 2b3dcfa changes only the ordering in one cutover test in src/tests/daemon-registration-owner.test.ts, so it does not change the result above.

@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

This PR is ready at 2b3dcfa. All 14 checks pass, and the changed tests run in Integration/Coverage and are green. There are no conflicts. I read the code and did not run the cutover tests or the mutation controls. The legacy side rebuilds main's lock algorithm from source, so it does not run a released 0.21.x daemon binary. The real old shutdown and the full legacy info shape are not exercised. I also did not reproduce the successor-race timing below, since it comes from reading the code.

Not blocking, and you can take or leave these. (1) After a graceful daemon stop, if another client starts a successor before retirement takes the lock, the result is 'lock-busy' or 'registration-replaced', and stopDaemon throws daemon_retirement_unconfirmed where main reported stopped:true. A forced stop of a legacy file-lock daemon hits the same path, because its leftover lock file inspects as 'unproven' and maps to 'lock-busy'. A manual stop should succeed when the observed daemon's exit is confirmed and the registration no longer names it, with a warning, and 'unproven' should map to 'ownership-unproven' in retireDaemonRegistration. (2) The ADR line "The client refuses an older registration before signaling or changing it" holds for ensureDaemon but not for manual stop, which SIGTERMs the legacy pid, so it could be scoped to startup and reset. (3) In daemon-stop.test.ts the 'registration-replaced' mock attaches an error that retirementAfterRemoval never produces, so it could be attached only for 'retirement-unconfirmed', and the legacy fixture repeats the barrier snippet from registered-daemon-fixture.ts. The open flake thread on the legacy-file barriers still stands. It would be good to settle it, and the stop-result rule above, before merging after #3131.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 3, 2026
@thymikee
thymikee added this pull request to stack #3146 October 3, 2026 08:46
@thymikee
thymikee force-pushed the fix/daemon-timeout-retirement branch from 2afb5ec to 4fd7029 Compare October 3, 2026 14:40
@thymikee
thymikee force-pushed the fix/daemon-registration-cutover branch from 2b3dcfa to febd631 Compare October 3, 2026 14:40
@thymikee
thymikee force-pushed the fix/daemon-timeout-retirement branch from 4fd7029 to 814fb0c Compare October 3, 2026 16:41
@thymikee
thymikee force-pushed the fix/daemon-registration-cutover branch from febd631 to 31107eb Compare October 3, 2026 16:41
@thymikee
thymikee force-pushed the fix/daemon-timeout-retirement branch from 814fb0c to bcce61b Compare October 3, 2026 17:49
@thymikee
thymikee force-pushed the fix/daemon-registration-cutover branch from 31107eb to 91f9071 Compare October 3, 2026 17:49
@thymikee
thymikee removed this pull request from stack #3146 October 3, 2026 19:38
@thymikee
thymikee force-pushed the fix/daemon-timeout-retirement branch from bcce61b to 50eafe5 Compare October 3, 2026 19:39
@thymikee
thymikee force-pushed the fix/daemon-registration-cutover branch from 91f9071 to 12781ae Compare October 3, 2026 19:39
@thymikee
thymikee added this pull request to stack #3187 October 3, 2026 19:45
@thymikee
thymikee removed this pull request from stack #3187 October 3, 2026 20:58
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Consolidated into #3127 as part of reducing #3116 to seven PRs. The composition preserves the complete pre-consolidation source tree, including tests and later review corrections. This PR is superseded; its review discussion and native evidence remain available. Outstanding findings transfer to the owning keeper in the implementation record.

@thymikee thymikee closed this Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-03 21:18 UTC

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