fix(doctor): recover legacy cron archive across devices by masatohoshino · Pull Request #98217 · openclaw/openclaw

masatohoshino · 2026-06-30T16:31:11Z

What Problem This Solves

Fixes an issue where self-hosters running openclaw doctor --fix see "Cron store migrated to SQLite" reported as finished, yet the legacy cron/jobs.json (and cron/jobs-state.json) file is never archived — so every later openclaw doctor run re-detects the same legacy storage and re-offers the migration. This happens when archiving the legacy file fails: the rename used to swallow every error, so a cross-device move (EXDEV, the common case when the cron store sits on a Docker bind mount) or any other rename failure silently no-op'd while doctor still claimed success.

Why This Change Was Made

archiveLegacyCronFile kept the fast fs.rename path, but added a copy+unlink fallback for cross-device (EXDEV) moves so the legacy file is actually archived, and now returns a closed result instead of discarding errors. archiveLegacyCronStoreForMigration aggregates per-file outcomes, and doctor reports honestly: the existing "Cron store migrated to SQLite" success only when the legacy files are archived, otherwise a warning naming the leftover file, the reason, and the openclaw doctor --fix retry. The scope is deliberately narrow — recover the archive failure and stop the false success. SQLite migration semantics and scheduler behavior are unchanged; no new config or runtime surfaces.

User Impact

Docker / bind-mount self-hosters: legacy cron JSON/state files are archived even when the move crosses a device boundary, so doctor --fix no longer loops on the same migration every run.
When archiving genuinely cannot complete (e.g. a busy or read-only mount), doctor now warns clearly — "Migrated cron jobs to SQLite but could not archive the legacy cron file at <path>: <reason>. Remove it manually or rerun openclaw doctor --fix to retry." — instead of falsely reporting the migration as done.
No change when the cron store and its archive path are on the same filesystem: the rename fast path is untouched.

Evidence

Base upstream/main 6cb82eaab8. Focused suites, types, lint, and format all green, plus two real-kernel captures: (A) a real cross-device EXDEV rename proving the copy+unlink recovery mechanism, and (B) the real exported archive function returning an honest failure on a real kernel rename error. Both run against the real OS with no mocks; the production wrapper's EXDEV branch itself is covered by the deterministic unit tests below.

(A) Real cross-device EXDEV + copy+unlink recovery (real kernel, no mocks). Genuine fs.rename from tmpfs (/dev/shm) to ext4 returns the real EXDEV errno that the code branches on; the same copy+unlink the fix uses then recovers the move:

[A] src st_dev = 28 (tmpfs)
[A] dst st_dev = 66306 (ext4)
[A] rename: REAL kernel error code=EXDEV errno=-18 syscall=rename
[A] copy+unlink fallback: OK; src removed=true dst created=true

(B) Real production function against a bind-mounted store file (real kernel EBUSY, no mocks). This exercises the honest-failure half on the real code path. The legacy store file is turned into its own mountpoint with mount --bind inside an unprivileged user+mount namespace — the same kernel primitive Docker uses for a single-file -v host/jobs.json:container/cron/jobs.json bind mount. Renaming a bind-mounted file to a sibling returns EBUSY, so the unmodified exported archiveLegacyCronStoreForMigration (run via node --import tsx) gets a genuine kernel rename failure and reports it instead of claiming success — the legacy file remains, nothing is mislabeled as migrated:

+ mount --bind /dev/shm/.../cron/jobs.json /dev/shm/.../cron/jobs.json
--- live mount (store file is now its own mountpoint) ---
/dev/shm/.../cron/jobs.json  tmpfs  tmpfs[/.../cron/jobs.json]
--- run real exported archiveLegacyCronStoreForMigration ---
[capture] result          = {"ok":false,"failures":[{"path":".../cron/jobs.json",
                             "reason":"EBUSY: resource busy or locked, rename '.../jobs.json' -> '.../jobs.json.migrated'"}]}
[capture] legacy existed  = true
[capture] legacy remains  = true
[capture] migrated created= false

EBUSY here (bind-mounted file) and EXDEV (cross-device / Docker Desktop FUSE) are both real archive-failure modes; the fix recovers EXDEV via copy+unlink and reports everything else honestly rather than swallowing it.

Deterministic unit tests (src/commands/doctor/cron/index.test.ts) drive the real maybeRepairLegacyCronStore repair path, spying only fs.rename/fs.copyFile to inject the cross-device failures:

falls back to copy+unlink when renaming the legacy cron store fails with EXDEV — archive completes via copy+unlink, original removed, success reported, and a second doctor pass no longer re-detects the store (loop broken).
warns honestly without a false success when archiving the legacy cron store fails entirely — rename EXDEV + copy EACCES; legacy file remains, a warning surfaces, no "Cron store migrated to SQLite" success note.
archives the legacy cron store and its state file on a successful rename — rename happy path unchanged; both files archived, no warning.

$ node scripts/run-vitest.mjs src/commands/doctor/cron/index.test.ts       -> Tests 33 passed (33)
$ node scripts/run-vitest.mjs src/cron/store.test.ts src/commands/doctor/cron/  -> 41 + 38 + 34 passed
$ node scripts/run-tsgo.mjs -p tsconfig.core.json ...                       -> exit 0 (core prod types)
$ node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json ...    -> exit 0 (core test types)
$ scripts/run-oxlint.mjs <changed files>                                    -> exit 0
$ oxfmt --check <changed files>                                             -> all files correctly formatted

A same-directory EXDEV (what Docker Desktop's virtiofs/gRPC-FUSE bind mounts return for in-place rename) needs a FUSE/virtiofs mount that this sandbox cannot provide (no FUSE dev headers). The deterministic injected-EXDEV test is therefore the strongest available proof that the production wrapper takes the copy+unlink branch; capture (A) independently proves that branch's mechanism against the real kernel, and capture (B) proves the honest-failure half on the real exported function with a real kernel rename error. Together they cover every link without overclaiming a live FUSE reproduction.

clawsweeper · 2026-06-30T16:34:25Z

Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 1:33 AM ET / 05:33 UTC.

Summary
The PR changes doctor legacy cron migration to report archive failures, recover EXDEV moves with copy/unlink, roll back failed cleanup, and persist source receipts so retries do not duplicate or resurrect migrated cron jobs.

PR surface: Source +562, Tests +556. Total +1118 across 7 files.

Reproducibility: yes. from source: current main catches legacy cron archive rename failures and still emits the migration success note, matching the repeated doctor --fix loop described in the PR. I did not run a live repro in this read-only review, but the PR includes real-kernel and Crabbox proof.

Review metrics: 1 noteworthy metric.

Migration Cleanup Contract: 1 archive outcome contract changed, 1 durable receipt path added. These are the compatibility-sensitive pieces that affect doctor retry behavior and legacy-file cleanup before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/commands/doctor/cron/index.test.ts, migration/backfill/repair: src/commands/doctor/cron/index.ts, migration/backfill/repair: src/commands/doctor/cron/legacy-store-migration.ts, migration/backfill/repair: src/commands/doctor/cron/migration-ledger.ts, migration/backfill/repair: src/commands/doctor/cron/repair-plan.ts, migration/backfill/repair: src/commands/doctor/cron/store-migration.ts, and 6 more. Migration or upgrade compatibility proof is recorded; maintainers should verify it before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

none.

Risk before merge

[P1] Merging intentionally changes upgrade-time cleanup: legacy cron JSON/state files that earlier releases could leave behind may now be copied, verified, and unlinked during doctor cleanup.
[P1] The new receipt path changes retry semantics for leftover legacy sources, so maintainers should explicitly accept that completed imports are not re-imported after a failed archive retry.

Maintainer options:

Land With Accepted Migration Semantics (recommended)
Accept the upgrade-time cleanup behavior after confirming maintainers want doctor to remove verified legacy cron files and suppress duplicate re-imports with receipts.
Ask For One More Packaged Upgrade Capture
Request an additional packaged Docker or bind-mount doctor --fix capture only if maintainers want stronger upgrade proof beyond the current real-kernel and Crabbox evidence.
Pause For Migration Ownership Review
Pause the PR if maintainers are not ready to let doctor own copy/unlink archival and receipt-based retry handling for legacy cron files.

Next step before merge

[P2] The next action is maintainer review of the compatibility-sensitive migration cleanup semantics and normal exact-head merge gates, not an automated repair job.

Security
Cleared: No concrete security or supply-chain concern was found; the diff is confined to cron doctor migration code/tests and live comments confirm no package, dependency, manifest, or lockfile changes.

Review details

Best possible solution:

Keep runtime cron storage SQLite-only and let doctor own legacy JSON cleanup with explicit archive outcomes, durable receipts, rollback coverage, and maintainer-accepted upgrade semantics.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main catches legacy cron archive rename failures and still emits the migration success note, matching the repeated doctor --fix loop described in the PR. I did not run a live repro in this read-only review, but the PR includes real-kernel and Crabbox proof.

Is this the best way to solve the issue?

Yes, subject to maintainer acceptance of the compatibility-sensitive cleanup semantics. The fix belongs in the doctor legacy cron migration path and preserves the repository rule that runtime cron storage remains SQLite-only.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c230ab3c924e.

Label changes

Label justifications:

P2: This is a normal-priority doctor migration bugfix for self-hosted legacy cron stores with limited blast radius.
merge-risk: 🚨 compatibility: The PR changes upgrade-time cleanup and retry semantics for persisted legacy cron JSON/state files.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body and maintainer comment include after-fix real-kernel/live-output evidence for EXDEV recovery, EBUSY honest failure, and a real doctor --fix bind-mount scenario.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body and maintainer comment include after-fix real-kernel/live-output evidence for EXDEV recovery, EBUSY honest failure, and a real doctor --fix bind-mount scenario.

Evidence reviewed

PR surface:

Source +562, Tests +556. Total +1118 across 7 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	6	613	51	+562
Tests	1	556	0	+556
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	7	1169	51	+1118

What I checked:

Repository policy read: Root AGENTS.md was read fully and applied to the PR review, especially the read-only review, migration, SQLite-only runtime, compatibility, and security-review guidance. (AGENTS.md:1, c230ab3c924e)
Current main swallows archive failures: Current main archives legacy cron files by calling fs.rename and catching every error, so EXDEV/EBUSY can leave the legacy file in place without surfacing failure. (src/commands/doctor/cron/legacy-store-migration.ts:32, c230ab3c924e)
Current main reports success unconditionally: Current main calls archiveLegacyCronStoreForMigration and then adds the Cron store migrated success note whenever a legacy store is detected. (src/commands/doctor/cron/index.ts:197, c230ab3c924e)
PR head handles archive outcomes: The PR head makes archive outcomes explicit, treats non-EXDEV rename failures as failures, and uses copy/unlink only for EXDEV with checksum, fsync, and rollback handling. (src/commands/doctor/cron/legacy-store-migration.ts:283, 40edb7f0579a)
PR head records migration receipts: The PR head checks whether a legacy cron source was already imported, skips re-importing completed receipts, and still retries archiving the leftover source. (src/commands/doctor/cron/index.ts:121, 40edb7f0579a)
Receipt storage uses existing schema: The PR writes to existing migration_runs and migration_sources tables rather than adding a new schema surface in this branch. (src/state/openclaw-state-schema.sql:1184, c230ab3c924e)

Likely related people:

steipete: GitHub path history shows multiple recent cron SQLite and doctor refactors by steipete, and this PR's maintainer hardening commits add the receipt/rollback behavior under review. (role: recent cron persistence and migration contributor; confidence: high; commits: 0ac61072b8ab, a84819a63953, 005da57957c9; files: src/commands/doctor/cron/legacy-store-migration.ts, src/commands/doctor/cron/index.ts, src/commands/doctor/cron/migration-ledger.ts)
MonkeyLeeT: GitHub history shows the auto-migrate legacy cron store work that made doctor responsible for this migration path. (role: feature-path introducer; confidence: medium; commits: 21aa297434e4; files: src/commands/doctor/cron/index.ts)
liuhao1024: Recent history on the same doctor cron entrypoint fixed false-positive legacy cron store warnings, adjacent to this PR's truthful doctor reporting behavior. (role: recent adjacent doctor cron contributor; confidence: medium; commits: 274b7b1d9f5d; files: src/commands/doctor/cron/index.ts, src/commands/doctor/cron/index.test.ts)
vincentkoc: Recent history on cron store persistence includes cleanup of the cron store API surface adjacent to the save path this PR extends. (role: recent adjacent cron store contributor; confidence: low; commits: 31941f3e924e; files: src/cron/store.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works

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.

github-actions · 2026-07-01T04:59:09Z

Dependency graph guard cleared

This PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh /allow-dependencies-change comment after the guard blocks that new head SHA.

Current SHA: 40edb7f0579a6739a4441175ef93db13ec900b24

steipete · 2026-07-01T05:01:12Z

/allow-dependencies-change exact head 11e77bf; GraphQL parent artifact only

The GraphQL-authored commit retains the contributor head f9c71d314e25f4208bd6336bcb966a530bc6ace6 as its parent, so the dependency guard sees unrelated intervening main history. Its tree 6ef81225e519315a78a310b6eb07f70226114102 exactly matches the reviewed current-main-based tree and changes no dependency, manifest, lockfile, or package files.

The real origin/main...reviewed-head scoped dependency pin guard passed on the seven owner files.

steipete · 2026-07-01T05:09:34Z

Maintainer hardening is complete on exact head 40edb7f0579a6739a4441175ef93db13ec900b24 (reviewed tree 13f418bbe178e95a214cb5ea50f38dda303cf1c7). origin/main at c230ab3c924e18c49b44f95be8101054bd4f9c81 is an ancestor, and the four rebased commits are patch-identical to the reviewed series.

What changed:

EXDEV archive recovery now exclusively claims the destination, copies and verifies the imported bytes, preserves mode/mtime, fsyncs the file and directories, and removes partial output on failure.
Doctor reports cleanup failures truthfully and rolls back multi-file state/archive work where possible.
Cron-row replacement and an immutable source receipt commit in one SQLite transaction, so a failed cleanup retry does not duplicate id-less jobs or resurrect runtime-deleted one-shot jobs.
Runtime ownership remains SQLite-only; there is no JSON runtime fallback.

Proof:

node scripts/run-vitest.mjs src/commands/doctor/cron/index.test.ts src/commands/doctor/cron/store-migration.test.ts — 47 doctor + 23 store-migration tests passed on Blacksmith Testbox tbx_01kwdz3ydbx3bbxx9gs2txq1za, Actions run 28493639133.
Scoped pnpm check:changed -- <7 touched files> — core/core-test typechecks, lint (0 warnings/errors), import cycles, database-first guard, and adjacent guards passed on the same Testbox.
Real noninteractive openclaw doctor --fix bind-mount scenario — EXDEV copy path, truthful EBUSY retention, unmounted retry, clean third run, no .migrated.2, and SQLite runtime readback passed on AWS Crabbox lease cbx_4d44a760a58a, run run_3a9f3bbd39a7.
Fresh autoreview after all fixes: no accepted/actionable findings; best bounded fix, confidence 0.94.
Fresh exact-head hosted gates passed: CI 28495614366 and Workflow Sanity 28495614368. Security proof is also green on this SHA: CodeQL 28495614357, CodeQL Critical Quality 28495614323, and OpenGrep PR Diff 28495614326.
Dependency Guard 28495613629 passed natively with no override. The current-main diff changes only the seven cron files and no dependency, package, manifest, or lockfile files.

The earlier broad test:changed attempt was polluted by a sparse full-sync selecting unrelated absent Mantis files. The authoritative exact-head CI full matrix subsequently passed with zero failures. No docs or changelog change is required.

steipete · 2026-07-01T05:16:41Z

/allow-dependencies-change exact head e504c45; old GraphQL ancestry artifact only

Remote tree 27b729bc226eace472d6d02dbbdcab6c905e505d exactly equals local current-main-rebased prep 0d9683adc14d800539924bc1f4c040af6c269899.

git range-diff d68ba5ed..9f3b900a ad59492d..0d9683ad shows all four commits are patch-identical. The real origin/main...0d9683ad diff changes only these seven cron files and contains no dependency, package, manifest, or lockfile changes:

src/commands/doctor/cron/index.test.ts
src/commands/doctor/cron/index.ts
src/commands/doctor/cron/legacy-store-migration.ts
src/commands/doctor/cron/migration-ledger.ts
src/commands/doctor/cron/repair-plan.ts
src/commands/doctor/cron/store-migration.ts
src/cron/store.ts

The dependency-guard failure is solely caused by the old GraphQL ancestry merge base 6cb82eaab8650bcd0fb9cb4d8ae0d60fcf341b3c.

steipete · 2026-07-01T05:39:16Z

Merged via squash.

Prepared head SHA: 40edb7f0579a6739a4441175ef93db13ec900b24
Landed commit: 2c7e989686bac28bfe0f7a27d34a56158c65674b

openclaw-barnacle Bot added commands Command implementations size: S labels Jun 30, 2026

steipete self-assigned this Jul 1, 2026

steipete requested a review from a team as a code owner July 1, 2026 04:57

masatohoshino and others added 4 commits June 30, 2026 22:25

fix(doctor): recover legacy cron archive across devices

0d45b31

fix(doctor): harden legacy cron archive retries

16dd07c

fix(doctor): canonicalize shipped cron retry shape

9100d9b

fix(doctor): persist legacy cron migration receipts

40edb7f

github-actions Bot mentioned this pull request Jul 1, 2026

📡 Upstream Digest — 2026-07-01 08:04 UTC curtismercier/openclaw-mods#1180

Open

clawsweeper Bot mentioned this pull request Jul 2, 2026

Expose legacy cron store doctor lint findings #99211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(doctor): recover legacy cron archive across devices#98217

fix(doctor): recover legacy cron archive across devices#98217
steipete merged 4 commits into
openclaw:mainfrom
masatohoshino:fix/cron-legacy-archive-exdev

masatohoshino commented Jun 30, 2026

Uh oh!

clawsweeper Bot commented Jun 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

steipete commented Jul 1, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

masatohoshino commented Jun 30, 2026

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

clawsweeper Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency graph guard cleared

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

steipete commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented Jun 30, 2026 •

edited

Loading

github-actions Bot commented Jul 1, 2026 •

edited

Loading

steipete commented Jul 1, 2026 •

edited

Loading