Repository navigation
fix(doctor): admit maintenance before database inspection - #141412
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 7, 2026, 5:45 PM ET / 21:45 UTC. ClawSweeper reviewWhat this changesDoctor checks maintenance ownership before database inspection, bounds SQLite subprocess execution, preserves retryable cleanup, and clarifies workspace and downgrade recovery. Merge readiness⛔ Blocked before merge - 1 item remains This remains a useful, distinct fix. Earlier findings are resolved, and the recorded shipped-package recovery evidence satisfies the previous compatibility concern. Priority: P1 Review scores
Verification
How this fits togetherDoctor diagnoses and repairs an installation’s configuration and databases. Its maintenance flow coordinates with the Gateway and agent writers before inspection, migration, readiness checks, and service restart. flowchart TD
A[Doctor repair request] --> B[Verify managed service ownership]
B --> C[Acquire coordination and check writers]
C -->|Another owner active| D[Refuse with recovery guidance]
C -->|Admitted| E[Bounded database inspection]
E --> F[Existing migration owners]
F --> G[Validate readiness and release ownership]
G --> H[Restore managed service when appropriate]
Before merge
Agent review detailsSecurityNone. PR surfaceSource +127, Tests +271, Docs -945. Total -547 across 23 files. View PR surface stats
Review metrics
Stored data modelPersistent data-model change detected: Technical reviewBest possible solution: Keep Doctor’s existing migration and readiness owners behind one ownership-first maintenance flow with bounded, fully joined inspection subprocesses. Do we have a high-confidence way to reproduce the issue? Yes: current-main source places snapshots before maintenance admission, and the supplied before/after observations exercise live-owner refusal and stopped-state recovery. This read-only review did not execute the reproduction. Is this the best way to solve the issue? Yes: the patch fixes admission ordering at its owner, retains existing migration contracts, and addresses the earlier schema-repair and cleanup regressions without adding parallel migration controls. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 031e7702ad81. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (7 earlier review cycles)
|
1f2d6b3 to
2dccdfd
Compare
|
Addressed both findings and rank-up moves. Doctor now lets recognized canonical-index drift reach its existing schema repair owner while retaining header/readability/newer-version refusal and final strict readiness. The missing-index and drifted-index cases repair and complete; a missing required table remains absent and fails readiness. The four newer-agent-schema refusal cases now run through The schema/cleanup regressions failed on the prior pushed production for the expected reasons and pass after the repair. The broad focused run passed 601 tests with two existing skips; the relocated eight-case schema-preflight suite also passed. CI fixture repairs preserve independent WAL visibility, Windows fsync policy, and retryable read-only cleanup. |
f7fb29e to
7a9cce4
Compare
|
Compatibility proof confirmed for this PR. The refreshed built-runtime run is https://github.com/openclaw/openclaw/actions/runs/34155452783 (Node 24.19.0): Doctor refused the live shipped 2026.9.2 Gateway in 0.844 seconds, then repaired actual shipped 2026.7.1-2 state with the writers stopped. It migrated shared schema 1→16, preserved the audit event, retired the workspace/session/exec-approval sources, exited 0, and the candidate Gateway reached readiness. The home marker reported zero changed files; the wrapper exited 0 and stopped its Testbox. The tested tree was This confirms the compatibility evidence already acknowledged in the latest review. The separate CI failure is the stale QA scenario title/criteria introduced on main by #141514; it is being handled at its ownership boundary. Prepare and merge remain gated on green exact-head CI. |
7a9cce4 to
9412032
Compare
9412032 to
5a09bb9
Compare
|
Merged via squash.
|
…41412) * fix(doctor): admit maintenance before database inspection * fix(doctor): preserve schema repair and retryable snapshot cleanup * test(state): avoid returning from Promise executor * docs(updating): link the startup preflight recovery guarantee * refactor(doctor): share lease reads and native child cancellation * test(doctor): verify owner and recovery in one error
What Problem This Solves
Fixes an issue where operators running
openclaw doctor --fixafter an upgrade could wait on database snapshots before learning that the Gateway or an agent still owned the state. An unreadable legacy workspace file also lacked an actionable recovery step, leaving startup blocked after Doctor returned.Why This Change Was Made
Doctor now verifies or stops its managed service, acquires Gateway and state coordination, and checks agent leases before taking schema snapshots. The diagnostic lease reader neither migrates state nor consults a persisted quarantine receipt that Doctor needs to repair. A live Gateway owns its coordinator for its lifetime, so admission refuses it immediately. Optional diagnostic imports follow admission.
Recognized canonical-index drift reaches the schema repair owner; strict readiness validation runs after repair. Ordinary read-only close remains retryable after a transient snapshot cleanup failure.
Snapshot and initial integrity children have a 30-second execution fence. Timeout and cancellation kill and join the child before releasing ownership, and the parent cleans unpublished snapshot copies. Integrity uses a process because a Worker thread cannot reliably interrupt native SQLite. The recovery docs consolidate downgrade guidance, distinguish retained migration originals from package-transaction backups, and remove reverse-schema SQL recipes. Unreadable workspace sources remain intact and name backup or retained-file recovery.
Main's QA catalog breakage from #141514 was reproduced and repaired during landing. Our YAML correction passed all 20 native scenario-runner tests, then #141555 landed an equivalent fix. This branch now includes that upstream repair and drops the redundant YAML hunk. The effective selector matches this exact current line in
ui/src/e2e/control-ui-credentials.e2e.test.ts:114:The scenario retains its token/password-mode ID and coverage key; its title and criteria describe the single Gateway secret field supplying both connect auth values. The existing alignment test failed before the correction (one failure, 19 passes).
Production footprint
An absorption pass removed 9 production lines; the current net is +127. Both lease checks now use one typed lease query and the same PID/start-time predicate. Integrity timeout/cancellation use Node's native
forkoptions, removing the custom timer and abort listener. The remaining growth protects these boundaries:src/commands/doctor-maintenance.tssrc/commands/doctor.tssrc/flows/doctor-health.tssrc/state/openclaw-agent-db-lease.tssrc/infra/sqlite-readonly-worker.tsclosebefore cleanup or lease release.execFiledoes not forwardkillSignalon AbortSignal cancellation, so its explicit SIGKILL listener is necessary. Native timeout options and the error formatter are shared with integrity inspection.src/infra/sqlite-readonly-location.tssrc/infra/sqlite-integrity-worker.tsforkoptions now own timeout/cancellation. The remaining code sends typed IPC input, waits forclose, and preserves file identity plus native error details. Reusing the stdout snapshot transport would require adapters for BigInt file identities and the existing error envelope.src/infra/sqlite-integrity.worker.tssrc/infra/state-migrations.workspace-setup.tsA custom common subprocess wrapper was considered. It would need separate adapters for captured stdout/stderr and advanced IPC, while still retaining each transport's failure rules. The shared boundary is Node's process lifecycle; adding that wrapper would obscure ownership without reducing the required work.
User Impact
Explicit repair refuses live ownership before copying databases and reports which service or process must stop. Supported startup-blocking migrations run without prompts with
--fix --non-interactive; malformed or conflicting inputs require the manual action in the diagnostic. Stalled snapshot or asynchronous integrity inspection ends with a database-specific recovery message.Evidence
lets the schema repair owner decide current shared-state %srepairs missing/drifted indexes while a missing required table stays absent and fails readiness;reports success when retrying transient read-only snapshot cleanuppreserves retryable close. All four fail on the prior pushed production for the intended reasons. The newer-agent-schema cases now exerciserunDoctorHealthFlowand retain byte-preservation assertions. The IPC WAL-commit fixture and Windows directory-sync cases retain their real filesystem/database checks.pnpm build,node scripts/check-changed.mjs, documentation anchor validation (9,510 links; zero broken),git diff --check, and fresh P0–P2 autoreview passed. The refreshed rig home-marker assertion and the earlier local real-state guard both reported zero changed files.The fresh physical-checkout gate completed all typechecks and lint with exit 0 and stopped its Testbox. The local nested checkout had correctly refused ancestor
node_modulesdeclaration inputs; the guard was preserved and validation moved to an isolated checkout.Current main already authorizes all four named migration families in explicit non-interactive repair. The combined rig verifies that behavior; no redundant migration switches were added. No schema, configuration option, environment variable, dependency, or validation baseline was added or changed.
Related: #133984 #139105 #119583 #138892 #134896
Reported by @Rocky_Chuck: https://x.com/Rocky_Chuck/status/2096970154622267418