Skip to content

fix: preserve daemon ownership through startup and cleanup - #3133

Open
thymikee wants to merge 2 commits into
fix/daemon-registration-cutoverfrom
fix/daemon-review-hardening
Open

thymikee wants to merge 2 commits into
fix/daemon-registration-cutoverfrom
fix/daemon-review-hardening

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes seven review findings in the daemon ownership stack. A startup contender waits for a live winner to become ready before applying takeover policy. Stop, exit confirmation, registration parsing and lock ownership share one native-PID validator; invalid identity cannot authorize deletion. Test cleanup joins both its observed child and a registered successor before deleting their directory.

Timeout diagnostics report retained state from the retirement result. Fixture waits observe their own child's exit and clear their timers. Fixture directories remain owned by runner cleanup until journal writes settle. Startup deadline tests advance directly to the deadline boundary, preserving the 15-second budget while avoiding coverage-dependent polling timeouts. Manual stop loads retirement lazily, preserving CLI startup closure.

Depends on #3132; addresses comments on #3124, #3130 and #3131. Ref #3116. Twenty-one files, 379 gross changed lines.

Validation

Tested 5411c0bb96a070eac1cdcb6266d88d60912fdfbf:

  • 113 focused tests pass with zero skips, including actual socket/HTTP requests and owned children.
  • Removing native-PID range validation causes six failures, including live-child state retention. Removing startup readiness, successor joining or truthful timeout reporting each fails its regression control.
  • CLI import closure passes; independent read-only review has no remaining findings.
  • Quick checks and Fallow pass. Exact-head affected checks pass: 768 files, 6,241 tests; zero skips.
  • CI-owned coverage, provider and device checks remain pending. The coverage failures on the lower startup layers are addressed by the deadline test correction here.

Review in cubic

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.94 MB 4.94 MB +611 B
Package (unpacked) 4.94 MB 4.94 MB +611 B
Package (download) 1.48 MB 1.48 MB +202 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 18.2 ms 18.3 ms +0.1 ms
CLI --help 53.9 ms 53.8 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.

All reported issues were addressed across 21 files

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

Re-trigger cubic

Comment thread src/__tests__/test-utils/registered-daemon-fixture.ts
Comment thread test/integration/support/daemon-test-cleanup.ts
Comment thread test/integration/support/daemon-test-cleanup.ts
Comment thread packages/host-kit/src/internal/owner-identity-liveness.test.ts
Comment thread src/daemon-client/daemon-client-timeout.ts
Comment thread test/integration/support/daemon-test-cleanup.ts
Comment thread src/daemon-client/__tests__/daemon-client-startup-race.test.ts
Comment thread vitest.config.ts
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The code at 5411c0b looks correct to me, but I could not run two of the checks that matter here, so I am holding the call until they are covered. I did not run the startup-race readiness control, so I only infer that removing the gate fails it, from LIVE_DAEMON_PROBE_RETRIES and the fixture's 12 not-ready probes. I also did not run the CLI import-closure gate for the lazy stopAndRetireDaemon import. Please run both and share the output: the readiness control should fail with the gate removed and pass with it in place, and the import-closure gate should pass.

CI is green: 14 checks, 0 not passing, and the daemon-client startup, timeout and host-kit owner liveness routes this diff touches are covered by unit-core and coverage, which pass. No conflicts. The eight resolved threads about daemonPreservedAfterTimeout misreporting a force-killed daemon as preserved are fixed only in dependent #3138 (0780e5e), which I did not review, so they still apply at this head. Please land #3138 in the same stack before this reaches main.

Not blocking, and fine to take or leave: isProcessAlive in packages/host-kit/src/internal/host-process.ts still returns false for pids above 0x7fffffff, so callers like owned-process-reaper.ts keep the out-of-range hole this PR closes elsewhere, and a follow-up could make an invalid pid never read as death at that owner. Also, readIdentity in test/integration/support/daemon-test-cleanup.ts reads daemon.json twice, and one status read would do.

Could the PID guard be smaller if the invalid state could not be represented? For example, host-kit could parse every pid once into a branded ProcessPid (with isProcessPid as the only constructor), and OwnerIdentity.pid and DaemonProcessIdentity.pid could use that type. The guards in stopDaemonProcess, waitForDaemonExit and classifyOwnerLiveness would then hold by type, and readDaemonInfo could reject the record instead of mapping an invalid pid to 0, which feeds takeover and retirement today. That is not required for this stacked fix.

One open question for later: does the real daemon publish daemon.json before /health is ready on every transport? If not, a contender with a non-auto transport preference could still reach takeover after the gate passes on the other transport. That is a policy choice from #3130, not part of this change.

This branch has not been deployed

No deployments
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