Repository navigation
fix: bound daemon birth admission probes - #1064
Conversation
Cap each optional startup process-birth sample to 100 milliseconds and one quarter of the shared remaining lifecycle budget. Skip unusable follow-up samples so authenticated diagnostics retain admission time without weakening recovery witnesses. Cover direct, managed-start, and managed-reuse paths with deterministic deadline, abort, null, throw, and stable-witness cases. Document the availability trade-off and keep lifecycle Codecov ownership current. 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.
🟡 Changes recommended
The updated documentation’s skip condition for birth sampling is inconsistent with the implemented “quarter budget floors to 0ms” behavior (e.g., 1–3.999ms remaining).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a startup-admission failure mode where optional process-birth probing could consume the entire lifecycle deadline before required authenticated diagnostics run, by strictly bounding each birth sample’s time budget and avoiding unnecessary re-sampling when the first probe yields no evidence.
Changes:
- Bound
readAdmissionBirthtomin(100ms, floor(remaining/4)), and skip probing on abort/expired/too-small budgets. - Avoid the “after” birth sample when the “before” sample returned
null, preserving authenticated admission while withholding recovery-witness eligibility. - Add deterministic lifecycle tests, update recovery documentation, and include a patch changeset + Codecov component notes.
File summaries
| File | Description |
|---|---|
src/daemon/lifecycle.ts |
Bounds optional birth probe timeouts and suppresses second sampling when the first sample is unavailable. |
test/daemon/coverage-400-lifecycle-managed.test.ts |
Adds regression tests for capped/omitted birth probes across direct-start, managed-start, and managed-reuse scenarios. |
docs/daemon-restart-recovery.md |
Documents the bounded/optional nature of birth evidence during admission and its impact on recovery witness eligibility. |
codecov.yml |
Updates component commentary to reflect #966 lifecycle ownership notes. |
test/codecov-config.test.ts |
Updates expected component metadata comment for lifecycle ownership notes. |
.changeset/966-bounded-birth-admission.md |
Patch changeset describing the user-facing behavior change. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Cap each optional startup process-birth sample at 100 ms and one quarter of the remaining lifecycle budget, rounded down. Skip unusable budgets and omit the second sample when the first returned no evidence. Preserve authenticated admission and recovery-witness requirements across direct startup, managed startup, and managed reuse.
Motivation / Why
An optional birth probe could consume the complete startup deadline before required authenticated diagnostics ran, rejecting a healthy newly started daemon. With 100 ms remaining, a probe now receives at most 25 ms. Missing birth evidence still prevents recovery authorization, while ordinary authenticated admission can finish.
Type of change
Release-note title
fix: bound daemon birth admission probes
A patch Changeset documents the behavior change.
Testing done
The sample cap depends on the existing synchronous platform helper honoring its timeout. Slower optional birth reads may omit the recovery witness; authentication checks remain required.
Review guide
Start with readAdmissionBirth and the three after-sample guards in src/daemon/lifecycle.ts, then the deterministic lifecycle regressions and user-facing deadline documentation. Codecov changes document existing ownership; classification is unchanged.
Related issues
Closes #966. Part of #968.
Checklist
getLcmConnection()/closeLcmConnection()— no directDatabaseSync(unchanged)PRAGMA journal_mode=WALandPRAGMA foreign_keys=ON(no new connections)implicit any— all types explicitcollectStats()not called in request handlers or hot pathstest/daemon/routes/(no new routes)pnpm exec vitest run <test-files>pnpm run test:ciwith 100% line, branch, function, and statement coveragepnpm-lock.yaml;pnpm install --frozen-lockfilesucceeds (dependencies unchanged)