Skip to content

fix(doctor): admit maintenance before database inspection - #141412

Merged
steipete merged 6 commits into
mainfrom
steipete/upg-doctor-docs
Sep 7, 2026
Merged

steipete merged 6 commits into
mainfrom
steipete/upg-doctor-docs

Conversation

@steipete

@steipete steipete commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where operators running openclaw doctor --fix after 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:

it("submits one secret for both authentication modes with visible acceptance and rejection recovery", async () => {

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 fork options, removing the custom timer and abort listener. The remaining growth protects these boundaries:

Owner Net lines Required boundary and simpler alternatives considered
src/commands/doctor-maintenance.ts −33 Removed the duplicate schema preflight; this owner now only establishes service/coordinator/lease authority before inspection.
src/commands/doctor.ts +7 Defers optional session-maintenance and post-upgrade imports until those modes run. Eager imports load their state graphs before normal repair can reject a live owner.
src/flows/doctor-health.ts +25 Absorbs configured-path schema admission from maintenance, preserves diagnostic Doctor's update offer, and reports admission failure through the updater's existing result envelope. This offsets the maintenance deletion; a second detector would restore the original ordering defect.
src/state/openclaw-agent-db-lease.ts +56 Adds read-only admission without schema creation, stale-row deletion, snapshots, or a fresh quarantine-store read. It borrows an existing handle without closing it, bounds/restores its busy policy, and closes only its own connection. The canonical query and liveness predicate are shared. The writable checker must retain transactional maintenance/deletion fencing and stale-lease cleanup, so calling it before admission would mutate the state being protected.
src/infra/sqlite-readonly-worker.ts +35 Bounds synchronous/asynchronous snapshot processes and joins close before cleanup or lease release. execFile does not forward killSignal on 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.ts +14 The parent owns staging before spawn, allowing removal when a killed child never publishes its copy path. Ordinary handles retain retryable boolean cleanup; cancellable maintenance retains throwing cleanup. Combining those policies reproduced a close-contract regression.
src/infra/sqlite-integrity-worker.ts +12 A process can interrupt native SQLite; a Worker thread cannot reliably do so. Native fork options now own timeout/cancellation. The remaining code sends typed IPC input, waits for close, 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.ts +5 Receives private process IPC and binds send/disconnect after checking availability. These replace Worker-only input/result plumbing; the diagnostic envelope remains unchanged.
src/infra/state-migrations.workspace-setup.ts +6 Uses the existing recovery formatter to tell the operator how to retain or restore an unreadable source/claim and retry. Removing the recovery action would reinstate the Doctor/startup dead end.

A 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

  • Reinstating the original Doctor flow and maintenance owner makes all three admission regressions fail: Gateway/state/agent cases spawn 3/3/4 snapshot children before refusal. The repaired flow spawns none. Existing maintenance tests also verify the fence survives until cancelled native work closes.
  • Real stalled snapshot and integrity processes fail on the original code at the fixture's 35-second safety exit and pass after the repair at approximately 30 seconds. A 300 MiB synthetic database took 1.13–3.15 seconds to snapshot and under one second to check; the 30-second fence leaves storage headroom after ownership admission.
  • Shipped-package reproduction: 7.1-2 created shared schema 1; raw 9.2 startup exited 1 on legacy workspace state after changing the SQLite/WAL/SHM family to schema 15 and adding startup-failure/lock artifacts. The fixture's configuration bytes were unchanged.
  • Clean Linux rig, Node 24.19.0: the built candidate refused the live shipped 9.2 Gateway with exit 1 in 0.844 seconds. With writers stopped, it repaired actual 7.1-2 state with exit 0, migrated shared schema 1→16, preserved the legacy audit event, retired workspace/session/exec-approval sources, and booted the candidate Gateway. The native Testbox wrapper exited 0 and stopped its lease; the backing Actions step can show cancellation during delegated cleanup.
  • CI follow-up regression proof: lets the schema repair owner decide current shared-state %s repairs missing/drifted indexes while a missing required table stays absent and fails readiness; reports success when retrying transient read-only snapshot cleanup preserves retryable close. All four fail on the prior pushed production for the intended reasons. The newer-agent-schema cases now exercise runDoctorHealthFlow and retain byte-preservation assertions. The IPC WAL-commit fixture and Windows directory-sync cases retain their real filesystem/database checks.
  • The broad focused run passed 601 tests (two existing skips); the relocated eight-case schema-preflight suite and targeted lint passed. After the absorption refactor, 86 focused lease, deletion, admission, integrity, and deadline tests passed, and the shipped-package rig was rebuilt and rerun. Rebasing onto the merged main UI repair and routing fix(gateway): stop restart loops for required maintenance #141416 preserved both routing changes; all 186 focused integration tests and a fresh branch-mode P0–P2 review passed. Doctor, workspace, SQLite, cancellation, and agent-maintenance tests passed. 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_modules declaration 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

@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation gateway Gateway runtime commands Command implementations size: L maintainer Maintainer-authored PR labels Sep 7, 2026
@clawsweeper clawsweeper Bot added P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 7, 2026, 5:45 PM ET / 21:45 UTC.

ClawSweeper review

What this changes

Doctor 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
Reviewed head: 5a09bb942a27a10f321a3135746b29e02cafd3be

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) Strong production-path recovery evidence, focused regression coverage, and resolved prior findings support a clean review.
Proof confidence 🦞 diamond lobster (5/6) ✨ media proof bonus Sufficient (linked_artifact): The captured Linux rig observations exercise the built Doctor against a live shipped Gateway and stopped legacy state, showing fast refusal followed by successful repair and readiness. Recorded real subprocess deadline checks cover the changed inspection lifecycle; linked Actions logs could not be independently fetched during this review.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (linked_artifact): The captured Linux rig observations exercise the built Doctor against a live shipped Gateway and stopped legacy state, showing fast refusal followed by successful repair and readiness. Recorded real subprocess deadline checks cover the changed inspection lifecycle; linked Actions logs could not be independently fetched during this review.
Evidence reviewed 8 items Current main still needs the repair: The fetched main revision still performs schema preflight before entering maintenance. The introduced branch moves maintenance admission ahead of snapshots; the merged startup-routing repair does not replace this work.
Ownership precedes inspection: Maintenance acquires both lifecycle coordinators and checks live agent leases before returning to the health flow. The diagnostic lease reader uses existing rows without schema creation or stale-row deletion.
Earlier findings resolved: The current health-flow tests exercise newer schemas at canonical, custom, shared, and registered paths with byte-preservation assertions. Missing or drifted indexes reach the existing repair owner, while a missing required table still fails final readiness.
Findings None None.
Security None None.

How this fits together

Doctor 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]
Loading

Before merge

  • Add data-model compatibility proof - Confirm migration or upgrade compatibility proof before merge.
Agent review details

Security

None.

PR surface

Source +127, Tests +271, Docs -945. Total -547 across 23 files.

View PR surface stats
Area Files Added Removed Net
Source 9 286 159 +127
Tests 9 398 127 +271
Docs 5 141 1086 -945
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 23 825 1372 -547

Review metrics

Metric Value Why it matters
Production growth justification production +127 lines; tests +271 lines The body explains the growth through read-only lease admission, subprocess deadlines, and cleanup ownership while removing duplicate preflight logic.

Stored data model

Persistent data-model change detected: migration/backfill/repair: src/commands/doctor-maintenance.ts, migration/backfill/repair: src/commands/doctor.ts, migration/backfill/repair: src/flows/doctor-health.ts, migration/backfill/repair: src/infra/state-migrations.workspace-setup.ts, unknown-data-model-change: src/commands/doctor-maintenance.ts, unknown-data-model-change: src/commands/doctor.ts, and 1 more. Confirm migration or upgrade compatibility proof before merge.

Technical review

Best 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.

Labels

Label justifications:

  • P1: The repair addresses blocked post-upgrade recovery and Doctor waiting on database inspection before reporting active ownership.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (linked_artifact): The captured Linux rig observations exercise the built Doctor against a live shipped Gateway and stopped legacy state, showing fast refusal followed by successful repair and readiness. Recorded real subprocess deadline checks cover the changed inspection lifecycle; linked Actions logs could not be independently fetched during this review.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured Linux rig observations exercise the built Doctor against a live shipped Gateway and stopped legacy state, showing fast refusal followed by successful repair and readiness. Recorded real subprocess deadline checks cover the changed inspection lifecycle; linked Actions logs could not be independently fetched during this review.

Evidence

What I checked:

  • Current main still needs the repair: The fetched main revision still performs schema preflight before entering maintenance. The introduced branch moves maintenance admission ahead of snapshots; the merged startup-routing repair does not replace this work. (src/flows/doctor-health.ts:122, 031e7702ad81)
  • Ownership precedes inspection: Maintenance acquires both lifecycle coordinators and checks live agent leases before returning to the health flow. The diagnostic lease reader uses existing rows without schema creation or stale-row deletion. (src/commands/doctor-maintenance.ts:160, 5a09bb942a27)
  • Earlier findings resolved: The current health-flow tests exercise newer schemas at canonical, custom, shared, and registered paths with byte-preservation assertions. Missing or drifted indexes reach the existing repair owner, while a missing required table still fails final readiness. (src/commands/doctor-maintenance.schema-preflight.test.ts:144, 5a09bb942a27)
  • Cancellation and cleanup retain ownership: Snapshot and integrity subprocesses settle on close after cancellation or timeout. The maintenance caller rechecks its live authority and database owner before resuming schema work; existing tests cover abort, expiry, replacement, and changed database ownership. (src/state/openclaw-agent-db-maintenance.ts:118, 5a09bb942a27)
  • Upgrade compatibility evidence already supplied: The captured body and fix(doctor): admit maintenance before database inspection #141412 (comment) report a built Linux candidate refusing a live shipped 2026.9.2 Gateway in 0.844 seconds, then repairing stopped 2026.7.1-2 state, preserving its audit event, and reaching Gateway readiness. The cited run is https://github.com/openclaw/openclaw/actions/runs/34155452783; the reported tested tree is 8c8d1d5e5eaefc7b8c9b3e28a1eba0b90b50075b, followed by a production-preserving rebase and focused integration validation. Direct Actions inspection was unavailable; this assessment preserves the supplied observations rather than claiming independent execution. (8c8d1d5e5eae)
  • No introduced persistent schema migration: The introduced production diff changes admission, subprocess lifecycle, and diagnostics, not database definitions or schema versions. Removed SQL consists of documentation examples for unsupported reverse migrations; recovery now points to managed compatibility checks and verified backups. Public docs: docs/reference/database-schemas.md. (docs/reference/database-schemas.md:777, 5a09bb942a27)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (7 earlier review cycles)
  • reviewed 2026-09-07T17:51:00.738Z sha 1f2d6b3 :: blocked before merge. :: [P1] Allow repairable schema drift to reach Doctor's repair owner | [P2] Move schema-refusal tests to the new health-flow boundary
  • reviewed 2026-09-07T18:11:23.433Z sha 2dccdfd :: blocked before merge. :: [P1] Allow repairable schema drift to reach Doctor's repair owner | [P2] Move schema-refusal tests to the health-flow boundary
  • reviewed 2026-09-07T18:49:48.180Z sha 850b06e :: needs maintainer review before merge. :: none
  • reviewed 2026-09-07T19:07:22.982Z sha 9719f9f :: blocked before merge. :: none
  • reviewed 2026-09-07T19:14:51.119Z sha f7fb29e :: blocked before merge. :: none
  • reviewed 2026-09-07T20:42:54.172Z sha 7a9cce4dadc406c29bc314a2306a9690202a0f6c :: blocked before merge. :: none
  • reviewed 2026-09-07T21:28:08.251Z sha 5a09bb9 :: blocked before merge. :: none

@steipete
steipete force-pushed the steipete/upg-doctor-docs branch from 1f2d6b3 to 2dccdfd Compare September 7, 2026 18:03
@steipete
steipete marked this pull request as ready for review September 7, 2026 18:04
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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 runDoctorHealthFlow and retain unchanged config/state/agent bytes assertions.

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.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Sep 7, 2026
@steipete
steipete force-pushed the steipete/upg-doctor-docs branch from f7fb29e to 7a9cce4 Compare September 7, 2026 20:36
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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 8c8d1d5e5eaefc7b8c9b3e28a1eba0b90b50075b. The subsequent rebase preserved the production patch and added the merged routing repair; 186 integration tests passed, followed by the complete changed-file gate and clean independent reviews. This PR changes no SQLite schema. Read-only admission of a restored quarantined primary, supported canonical-index repair, and unchanged-file refusal for newer schemas are covered by the focused tests.

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.

@steipete
steipete force-pushed the steipete/upg-doctor-docs branch from 9412032 to 5a09bb9 Compare September 7, 2026 21:21
@steipete
steipete merged commit 662080a into main Sep 7, 2026
185 checks passed
@steipete
steipete deleted the steipete/upg-doctor-docs branch September 7, 2026 21:49
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 8, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations docs Improvements or additions to documentation gateway Gateway runtime maintainer Maintainer-authored PR P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: L status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant