Repository navigation
fix: converge explicit restart publication admission - #1079
Conversation
Authenticate publication contention independently before and after daemon replacement, and retry only the blocked assertions within bounded active-time budgets. Keep manager mutation and ensure work single-shot, preserve the existing child convergence window, and fail closed on PID, token, birth, health, or runtime identity drift. Add hermetic boundary coverage and operator guidance. Signed-off-by: Bernardo Donadio <[email protected]>
Bring the Bug 950 candidate onto origin/main at b0df28e before review. Preserve the merged bounded lifecycle birth probes and combine their Codecov ownership notes with restart publication convergence. Signed-off-by: Bernardo Donadio <[email protected]>
Integrate origin/main at 2dace53 after the earlier publication candidate merge. Retain the upstream systemd parent-fixture stabilization before final review. Signed-off-by: Bernardo Donadio <[email protected]>
Read restart publication PID and token evidence through the existing bounded, nonblocking regular-file helper while preserving scoped ownership and PID tri-state behavior. Keep ordinary post-contention assertion errors intact after deadline expiry, and document the terminal typed-error and complete six-second contention window. Signed-off-by: Bernardo Donadio <[email protected]>
Integrate origin/main at d3c3b12 because the coordinator-checkpoint update overlaps backend-publication documentation and Codecov ownership metadata. Preserve both upstream ownership notes and Bug 950 restart behavior. Signed-off-by: Bernardo Donadio <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes critical daemon lifecycle/publication-convergence control flow and identity capture semantics, which warrants final human review despite strong hermetic coverage.
Pull request overview
This PR fixes a reliability race where an explicit restartDaemon can collide with backend-publication passive sweeps at its assertion boundaries. It introduces bounded, identity-authenticated publication admission retries for restart-specific assertions (without replaying manager mutations), and documents the resulting timing/identity guarantees.
Changes:
- Add restart-scoped publication convergence wrappers that only retry
PrivateMutationLockContentionErrorafter capturing authenticated daemon identity (PID/token/birth/health/runtime fields). - Extend lifecycle options to support a distinct wrapper for the initial publication assertion and a shared replacement retry budget across nested/outer restart assertions.
- Add comprehensive hermetic tests for restart-publication convergence boundaries and update documentation/Codecov component notes plus a changeset.
File summaries
| File | Description |
|---|---|
src/daemon/lifecycle.ts |
Adds restart-scoped publication evidence capture and retry wrappers; wires them into restartDaemon and ensureDaemon initial assertion flow. |
test/daemon/lifecycle-restart-publication.test.ts |
New hermetic test suite covering PID/token/birth drift, manager PID absence window, abort/deadline behavior, and “no replay” restart semantics. |
docs/backend-publication.md |
Documents the three restart assertion boundaries, bounded retry budgets, and fail-closed identity requirements. |
codecov.yml |
Updates component narrative to include #950 in the lifecycle/service-manager ownership description. |
test/codecov-config.test.ts |
Keeps Codecov ownership expectations aligned with the updated component narrative. |
.changeset/steady-restart-publication.md |
Adds a patch changeset describing the user-visible reliability behavior change. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d3962dff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Derive the publication-capture UID from the injected lifecycle authority or the local process UID, and apply it to every bounded PID and token evidence read. Cover production fallback, explicit zero, missing-getuid compatibility, and injected descriptor-owner mismatches without changing manager UID behavior. Signed-off-by: Bernardo Donadio <[email protected]>
There was a problem hiding this comment.
🔵 Needs a closer look
It makes substantial changes to the daemon lifecycle restart/admission control flow and retry semantics in a critical reliability/security boundary that warrants final human review despite strong tests.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Explicit daemon restarts can collide with a passive publication sweep before restart, immediately after the manager starts the replacement, or after replacement admission. Retry only those publication assertions after independently authenticating the daemon responsible for the lock.
Motivation / Why
A replacement can begin its first passive sweep before publishing its PID file. The nested assertion must use the supervisor's authenticated manager PID during this interval, while the outer assertions independently authenticate owned daemon state. The lock-owner record remains a comparison check and cannot establish daemon identity.
The current and replacement daemon have separate bounded retry allowances. Manager operations run once, and the existing post-start admission behavior remains intact.
Type of change
Release-note title
fix: converge explicit restart publication admission
Testing done
6fda028b5478c973f42c082d705e8a5da17d6bdd: 12 direct lifecycle test files / 954 tests passed; lifecycle coverage is 100% statements, branches, functions, and lines.Related issues
Closes #950. Part of #968. Preserves #865 post-start admission behavior.
The general legacy PID/token readers remain tracked separately in #1073; this change uses bounded reads only for the new restart-publication capture path.
Checklist
getLcmConnection()/closeLcmConnection()— no DB access addedPRAGMA journal_mode=WALandPRAGMA foreign_keys=ON— no connections addedimplicit any— all types explicitcollectStats()not called in request handlers or hot pathstest/daemon/routes/— no routes addedpnpm exec vitest run <test-files>pnpm run test:ciwith 100% line, branch, function, and statement coveragepnpm-lock.yaml;pnpm install --frozen-lockfilesucceeds — no dependency changesLimits
The current and replacement identity allowances add up to four seconds of assertion wait; the existing independent child-final allowance can add another two seconds. Unavailable or staged health and unprovable identity remain failures.
Review guidance
Start with restart assertion identity capture and the initial-only nested wrapper. Then inspect the hermetic restart-publication tests for the absent PID window, token-disclosure boundary, deadline and cancellation behavior, and manager call counts.