Repository navigation
fix: preserve diagnostic runtime and failure facts - #1145
Conversation
Run SQLite diagnostic workers with explicit source and compiled flags so the minimum Node 22.12 runtime can execute either asset without inheriting parent arguments or environment. Capture safe SQLite pool counts before bounded reads, retain them only after witness reauthentication, and keep failed route snapshots tied to the configured backend. 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.
🟢 Approval recommended
The changes correctly preserve the intended diagnostic boundaries and are backed by targeted tests and documentation updates without introducing unsafe error serialization or resource-ownership regressions.
Pull request overview
This PR fixes regressions introduced after Feature #619 by preserving three diagnostic “fact boundaries” across failures/timeouts: Node runtime flags needed for TypeScript SQLite workers on early Node 22, independently observed SQLite pool counters across bounded timeouts (when publication still authenticates), and the known configured backend identity in sanitized stats-route fallback responses.
Changes:
- Ensure SQLite diagnostic child processes receive explicit Node
execArgvflags based on whether the worker asset is TypeScript (.ts) or compiled JavaScript (.js), while still using an empty child environment. - Preserve independently observed SQLite pool counters across timeouts only when publication witness re-authenticates, and avoid leaking per-connection paths.
- Keep the configured backend identity in
stats/stats --poolgeneric error fallbacks without serializing exception payloads.
File summaries
| File | Description |
|---|---|
src/db/diagnostic-sqlite.ts |
Adds execArgv selection so TS workers run with required Node flags while keeping isolated env/exec args. |
src/storage/diagnostics.ts |
Captures SQLite pool counters pre-await, returns them only after witness re-check on timeout, and avoids exposing connection paths. |
src/daemon/routes/stats.ts |
Preserves known backend identity in sanitized fallback diagnostics. |
src/daemon/routes/pool-stats.ts |
Preserves known backend identity in sanitized fallback diagnostics. |
test/db/diagnostic-sqlite-lifecycle.test.ts |
Adds assertions covering worker path selection and per-mode execArgv isolation for both fork modes. |
test/storage/diagnostics.test.ts |
Adds coverage for SQLite pool counter retention/discard rules across stalls/timeouts and redaction expectations. |
test/daemon/routes/persistence-read-boundaries.test.ts |
Verifies route fallbacks retain configured backend identity across sanitized failures. |
docs/development.md |
Documents the explicit diagnostic worker launch flags and the empty-env/no-preload boundary. |
docs/cli.md |
Documents SQLite pool counter retention semantics and backend identity preservation on unexpected route failures. |
.changeset/steady-diagnostics-boundaries.md |
Adds a patch changeset describing the diagnostic boundary fixes. |
Review details
- Files reviewed: 10/10 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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Preserve the three distinct diagnostic boundaries reported after Feature #619: worker startup on early Node 22, SQLite pool observations across timeouts, and known backend identity in route error responses.
Motivation / Why
A source worker could exit before sending IPC results because its sanitized launch omitted runtime feature flags. A stalled SQLite diagnostic discarded independently observed pool counts. Unexpected stats-route failures also replaced the known SQLite/PostgreSQL backend with
unavailable.The fixes preserve the child's explicit environment/argument boundary, return pool counters only after publication witnesses authenticate, and keep exception payloads out of responses. These were candidate-only findings at the S5 freeze
fc81b371; Feature #619 subsequently merged in PR #1103, and each defect was verified on the follow-up's main-branch base.Type of change
Release-note title
fix: preserve diagnostic runtime and failure facts
Testing done
Validation: 161 focused acceptance tests and 312 tests across 15 direct integration files passed; typecheck, lint, build/runtime/package verification passed. Real source-worker tests (6/6) and a compiled-worker SQL roundtrip passed under Node 22.22.2. The exact Node 22.12 runtime was unavailable; explicit fork-argument assertions cover its required SQLite/type-stripping flags. Complete coverage remains the fresh exact-head CI gate.
Three independent acceptance rows:
Start review with the worker launch arguments, then the collector's pre-await pool capture and timeout witness guard, and finally the two route fallback branches. Relevant end-user guidance and a patch Changeset accompany the fixes.
Related issues
Closes #1122
Closes #1123
Closes #1124
Checklist
getLcmConnection()/closeLcmConnection()— no directDatabaseSyncPRAGMA journal_mode=WALandPRAGMA foreign_keys=ONimplicit any— all types explicitcollectStats()not called in request handlers or hot pathstest/daemon/routes/pnpm exec vitest run <test-files>pnpm run test:ciwith 100% line, branch, function, and statement coveragepnpm-lock.yaml;pnpm install --frozen-lockfilesucceedsNo new routes, connections, schema writes, or dependencies are introduced. Existing Codecov component ownership remains unchanged. The existing stats handlers continue to call
collectStats(); this fix changes only their generic error response.