fix(tasks): recover terminal lost cron rows#86088
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 7:57 AM ET / 11:57 UTC. Summary PR surface: Source +17, Tests +172. Total +189 across 4 files. Reproducibility: yes. source-reproducible. Current main and Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrowed recovery after task/session-state owner acceptance, or require live gateway cron proof first if seeded CLI proof is not enough. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible. Current main and Is this the best way to solve the issue? Yes, subject to owner acceptance. The patch reuses the existing durable cron recovery source of truth and narrows terminal-row recovery to the backing-session-missing false-positive path; pruning would discard durable completion truth. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against db2488b6e3d6. Label changesLabel justifications:
Evidence reviewedPR surface: Source +17, Tests +172. Total +189 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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 PR egg ✨ Hatched: 🥚 common Brave Review Wisp Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
martingarramon
left a comment
There was a problem hiding this comment.
Explicitly carrying the error key, then deleting it when recovery.error is undefined, distinguishes "clear this field" from "omit this field". The old conditional spread couldn't express that, so recovered tasks could retain stale lost-state error values.
CI: Real behavior proof gate is failing. Rename Verification: → ## Real behavior proof and paste verbose vitest output (each test name + result), not just the command, to clear it.
Question on isRecoverableLostCronTask: the !error branch returns true when task.error is absent or empty — not only when it contains "backing session missing". Since markTaskLostById takes error as optional, this branch isn't dead code. Given the durable-completion check, is it intentionally safe to recover errorless terminal lost cron tasks, or should this gate still require the "backing session missing" substring?
6b2b511 to
742e16f
Compare
martingarramon
left a comment
There was a problem hiding this comment.
LGTM on the behavior fix. The previous !error || error.includes(...) path made every errorless lost task recoverable. Requiring the explicit backing session missing signal is a tighter recovery condition, and the new test covers the important boundary: a lost cron task without an error string is not recovered. Real behavior proof is still failing, so that proof section needs to pass before merge.
742e16f to
39e5e07
Compare
d19d1e6 to
384e1fd
Compare
384e1fd to
445af4a
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
445af4a to
c6b0355
Compare
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
c6b0355 to
bfcad5e
Compare
|
Merged via squash.
|
* fix(tasks): recover terminal lost cron rows * fix(tasks): require backing-session lost recovery signal * fix(tasks): satisfy has-own lint rule --------- Co-authored-by: Altay <[email protected]> (cherry picked from commit be94853)
* fix(tasks): recover terminal lost cron rows * fix(tasks): require backing-session lost recovery signal * fix(tasks): satisfy has-own lint rule --------- Co-authored-by: Altay <[email protected]>
* fix(tasks): recover terminal lost cron rows * fix(tasks): require backing-session lost recovery signal * fix(tasks): satisfy has-own lint rule --------- Co-authored-by: Altay <[email protected]>
* fix(tasks): recover terminal lost cron rows * fix(tasks): require backing-session lost recovery signal * fix(tasks): satisfy has-own lint rule --------- Co-authored-by: Altay <[email protected]>
* fix(tasks): recover terminal lost cron rows * fix(tasks): require backing-session lost recovery signal * fix(tasks): satisfy has-own lint rule --------- Co-authored-by: Altay <[email protected]>
* fix(tasks): recover terminal lost cron rows * fix(tasks): require backing-session lost recovery signal * fix(tasks): satisfy has-own lint rule --------- Co-authored-by: Altay <[email protected]> (cherry picked from commit be94853)
Replaces closed #75036 with the same task-registry fix on a narrow branch.
This keeps only the terminal lost cron recovery change and focused regression coverage. The previous PR branch had accumulated unrelated generated/runtime changes, so this branch was rebuilt from current main.
Real behavior proof
Behavior or issue addressed: Terminal
lostcron task rows that were marked lost by a backing-session-missing false positive are recovered from durable cron run logs, and stale terminal errors are cleared when recovery has no error. Errorless terminallostrows and rows with unrelated lost errors are not recovered.Real environment tested: Local OpenClaw source checkout on macOS, branch
fix/tasks-recover-terminal-lost-cron-narrow, head445af4a, Node/opt/homebrew/opt/node/bin/node. The CLI proof used an isolatedOPENCLAW_HOMEandOPENCLAW_STATE_DIRunder/private/tmp, with bundled plugins disabled. Commands were run from the current PR head through the source CLI entrypoint:node --import tsx src/entry.ts.Exact steps or command run after this patch:
runtime: "cron"status: "lost"error: "backing session missing"runId: "cron:proof-terminal-lost-cron:1764072000000"action: "finished"status: "ok"summary: "done"runAtMs: 1764072000000durationMs: 1250node --import tsx src/entry.ts tasks list --jsonnode --import tsx src/entry.ts tasks maintenance --jsonnode --import tsx src/entry.ts tasks maintenance --apply --jsonnode --import tsx src/entry.ts tasks list --jsonSeed output, with the temp path redacted:
{ "stateDir": "<isolated-state>", "taskId": "task-proof-terminal-lost-cron", "jobId": "proof-terminal-lost-cron", "runId": "cron:proof-terminal-lost-cron:1764072000000", "seededStatus": "lost", "seededError": "backing session missing", "cronRunLog": "<isolated-state>/cron/runs/proof-terminal-lost-cron.jsonl" }Before maintenance,
tasks list --jsonshowed the durable row was terminal lost and still had the backing-session error:{ "count": 1, "tasks": [ { "taskId": "task-proof-terminal-lost-cron", "runtime": "cron", "sourceId": "proof-terminal-lost-cron", "runId": "cron:proof-terminal-lost-cron:1764072000000", "status": "lost", "endedAt": 1764072060000, "lastEventAt": 1764072060000, "error": "backing session missing", "terminalSummary": "stale lost summary" } ] }Preview mode projected exactly one recovered task without mutating by command intent:
{ "mode": "preview", "maintenance": { "tasks": { "reconciled": 0, "recovered": 1, "cleanupStamped": 0, "pruned": 0 } }, "tasks": { "total": 1, "terminal": 1, "byStatus": { "succeeded": 1, "lost": 0 }, "byRuntime": { "cron": 1 } } }Apply mode recovered the persisted task:
{ "mode": "apply", "maintenance": { "tasks": { "reconciled": 0, "recovered": 1, "cleanupStamped": 0, "pruned": 0 } }, "tasks": { "total": 1, "terminal": 1, "byStatus": { "succeeded": 1, "lost": 0 }, "byRuntime": { "cron": 1 } } }After
--apply,tasks list --jsonshowed the row persisted assucceeded, used the durable run-log finish time, set the durable summary, and cleared the staleerrorfield:{ "count": 1, "tasks": [ { "taskId": "task-proof-terminal-lost-cron", "runtime": "cron", "sourceId": "proof-terminal-lost-cron", "runId": "cron:proof-terminal-lost-cron:1764072000000", "status": "succeeded", "endedAt": 1764072001250, "lastEventAt": 1764072001250, "terminalSummary": "done" } ] }Focused regression output from the same branch still covers the important boundaries:
Observed result after fix: The current-head CLI proof recovered the isolated terminal lost cron task from durable cron logs, changed persisted status from
losttosucceeded, rewroteendedAt/lastEventAtto the durable finish timestamp, setterminalSummarytodone, and cleared the staleerrorfield.What was not tested: I did not run a live scheduled cron job through a full gateway process in this PR. The proof uses an isolated seeded registry plus durable cron run log and then runs the real current-head
tasks maintenanceCLI preview/apply/list flow against that state.Review follow-up:
backing session missingsignal.lostcron rows are not recovered.CHANGELOG.mdedit per review; release-note context remains in this PR body for squash/release tooling.