fix(cron): keep isolated run status ok when delivery fails#95419
fix(cron): keep isolated run status ok when delivery fails#95419Alix-007 wants to merge 9 commits into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 5:18 AM ET / 09:18 UTC. Summary PR surface: Source +59, Tests +167. Total +226 across 6 files. Reproducibility: yes. Source inspection shows current main can propagate a delivery-dispatch Review metrics: 2 noteworthy metrics.
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
Maintainer decision needed
Security Review detailsBest possible solution: Land the narrow decoupling only after maintainers accept Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main can propagate a delivery-dispatch Is this the best way to solve the issue? Yes as a narrow bug fix if maintainers accept the status contract. The safer product alternative is the broader first-class warning status tracked by #49190 and #80139. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0de5d37f92fc. Label changesLabel justifications:
Evidence reviewedPR surface: Source +59, Tests +167. Total +226 across 6 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
Review history (13 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review — Added a 'Semantic change — for maintainer review' section: only the (non-fatal success + delivery-error + non-aborted) path is reclassified to ok + deliveryStatus:not-delivered; genuine execution failures still record error and trigger backoff/alerts, and delivery failure stays visible via the deliveryStatus column. No schema/provider/channel changes. Fixes #94058. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review — Fixed a genuine regression the cron-isolated-agent check caught: the #94058 status-decoupling branch was too broad and reclassified deliberate keyless delivery-target denials (errorKind: delivery-target) as ok. Narrowed the condition so those stay error, while transient delivery failures still become ok + deliveryStatus:not-delivered. Local: delivery-awareness 3 pass, full cron/isolated-agent suite 395 tests pass. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
CI note after re-checking the failing Those files are unrelated to this cron finalizer PR. The cron-specific regression was already narrowed in the latest push so deliberate |
b570ae5 to
cbe8648
Compare
0ad8b53 to
8a68409
Compare
A successful isolated cron agentTurn whose post-run delivery phase returns an error collapsed the execution status into the delivery error, so the outer scheduled run persisted status=error even though the isolated session ended successfully. Decouple execution status from delivery outcome in finalizeCronRun: when the agent payload is non-fatal and the run was not aborted, a delivery dispatch error now keeps status=ok and records the failure separately via delivered/deliveryAttempted plus delivery diagnostics, which drive the already-decoupled run-log deliveryStatus (not-delivered). Aborted runs and fatal agent payloads keep their error status. Fixes openclaw#94058
The openclaw#94058 fix kept isolated turn status=ok when post-run delivery fails, but the broad `status === "error"` check also caught the openclaw#91613 keyless implicit last-target refusal, which is a deliberate delivery-target guard (errorKind "delivery-target") and must stay status=error. Exclude that errorKind so genuine delivery dispatch failures still become ok while target-guard refusals remain errors.
A successful isolated agent turn keeps status=ok when post-run delivery fails (openclaw#94058), but the ok/not-delivered resolveRunOutcome branch only forwarded delivery diagnostics and dropped the delivery dispatch error. That left lastDeliveryError empty and the finished-event deliveryError field blank, so CLI/UI/API run logs could not show why delivery did not land for an otherwise successful run. Thread the delivery error on a dedicated CronRunOutcome.deliveryError field from resolveRunOutcome through executeDetachedCronJob and applyJobResult into resolveDeliveryState, so it persists as lastDeliveryError and reaches the finished event (and thus the run log) without conflating it with a run-level error (lastError stays empty on a successful run). Add a service/run-log readback test proving the delivery error survives the cron boundary into a persisted run-log entry, and extend the isolated-turn regression to assert the dedicated deliveryError field.
8a68409 to
d73b9b3
Compare
…irty # Conflicts: # src/cron/service/ops.ts
|
Rebased onto current upstream/main and resolved the manual-run conflict by preserving both behaviors: trigger no-fire/manual-run handling from main, and the isolated-run deliveryError path from this PR. Local validation on the new head:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Landed the completed current-main replacement as #105215 / The landed version preserves @Alix-007's contributor commits and completes the invariant across required and best-effort delivery, target-resolution refusal, startup catch-up, manual/removed-job terminal events, diagnostics, persistence, and run-log readback. The original fork history was too stale to update safely without unrelated drift, so the reviewed commits were transplanted to current main and hardened there. Thank you, @Alix-007. |
Summary
Scheduled isolated
agentTurncron jobs could finish a successful isolated session yet have the outer scheduled run persisted withstatus=errorwhen the post-run delivery phase failed. This patch keeps executionstatus=okand records the delivery failure separately, so cron run logs reportstatus=okwithdeliveryStatus=not-deliveredinstead of a false execution failure.Root cause
In
finalizeCronRun(src/cron/isolated-agent/run.ts), when the agent payload was non-fatal (!hasFatalErrorPayload), the dispatch outcome was returned verbatim:A
dispatchCronDeliveryresult ofstatus: "error"(unresolved target / outbound send failure) therefore collapsed into the isolated agent execution status.applyJobResultthen setlastRunStatus=error, incrementedconsecutiveErrors, and could trigger backoff / failure alerts for a session that actually succeeded.The storage layer already keeps
status(CronRunStatus) anddeliveryStatus(CronDeliveryStatus) as separate columns; the bug was purely thatrun.tscontaminatedstatusbefore persistence.Changes
finalizeCronRun: when the agent payload is non-fatal and the run was not aborted, a delivery dispatchstatus: "error"now routes throughresolveRunOutcomeso executionstatusstaysok. Delivery failure is recorded viadelivered/deliveryAttemptedand delivery diagnostics, which drive the decoupled run-logdeliveryStatus(not-delivered). Aborted runs and fatal agent payloads keep theirerrorstatus.resolveRunOutcome: carries optionaldeliveryDiagnosticsso the delivery error remains operator-visible in run diagnostics even whenstatus=ok.status=okwith delivery failure metadata.Scope is limited to cron finalization; no provider/channel code is touched.
Decoupling rationale (status vs deliveryStatus)
statusanswers "did the isolated agent turn execute successfully" anddeliveryStatusanswers "did the result reach its destination". These are orthogonal: a turn can succeed and the announce/direct send can still fail. Collapsing them made every transient delivery error look like an agent failure and drove backoff/alerting. Keeping them decoupled means a delivery failure is surfaced (deliveryStatus=not-delivered, delivery diagnostics) without falsely failing the execution — avoiding any cross-cutting product-decision change to the scheduler's error semantics.Semantic change — for maintainer review
This PR changes one observable behavior — the run-log
statusfor the "execution succeeded, delivery failed" path — so I want to lay out the semantics and blast radius explicitly rather than asking you to infer them. This is the only judgement call in the change; the code is otherwise a verbatim re-route.1. Why
status/deliveryStatusdecoupling is the correct semantics. An isolatedagentTurnis a self-contained unit of work: it either executes successfully and produces a final assistant report, or it does not. Post-run delivery (announce / direct send) is a separate, downstream phase that depends on transient externals (channel resolution, outbound network, provider availability). Today a successful, completed isolated session is retroactively re-labelledstatus=errorpurely because the announce send failed afterwards — i.e. work that finished is recorded as failed work. The storage layer already models these as two independent columns (CronRunStatus+CronDeliveryStatus); this PR just stopsrun.tsfrom contaminating the first with the second. The user who filed #94058 reached the same conclusion independently — their stated expected behavior is exactly "preserve the successful agent execution outcome and separately record the delivery failure phase."2. Blast radius on backoff / failure-alert is deliberately narrow. Only one path changes: agent payload non-fatal (
!hasFatalErrorPayload) anddeliveryResult.result.status === "error"and the run was not aborted (!params.isAborted()). That path goes fromstatus=errortostatus=ok+deliveryStatus=not-delivered. Everything else is untouched:hasFatalErrorPayload) still recordsstatus=error→consecutiveErrors++→ backoff / failure alerts fire exactly as before.status=error.status=ok+ delivered.So real execution failures continue to drive backoff and alerting; only the false positives (succeeded-but-undelivered) stop doing so. The delivery failure is not swallowed — it remains operator-visible via
delivered=false,deliveryAttempted, thedeliveryStatus=not-deliveredcolumn, and the merged delivery diagnostics.3. Compatibility note (the
merge-risk: compatibilitylabel). The risk to weigh is operators/tooling that currently key offstatus=errorto detect undelivered runs. After this change those runs surface asstatus=ok+deliveryStatus=not-delivered, so any such consumer should readdeliveryStatus(the column already exists for this purpose) rather than overloadingstatus. There is no schema/migration change and no provider/channel code touched. I have kept the code change to the minimum re-route and surfaced the trade-off here so the call on the error-semantics is yours to make — happy to adjust framing or gate the behavior if you'd prefer a more conservative landing.Real behavior proof
Behavior addressed: A successful isolated
agentTurnwhose post-run delivery fails was persisted to the cron run log withstatus=error, making a successful session look like a failed run (and feeding error backoff/alerting).Real environment tested: Local OpenClaw source checkout on Linux at
upstream/mainhead7217477553f64701d2ba36e344ca1cd6be8ba54f, dependencies hydrated, commands run from the repo root.Exact steps or command run after this patch: Drove the REAL
CronServicefinalization (applyJobResult+resolveDeliveryState) for a successful isolated turn with a failing post-run delivery, persisted the resultingfinishedrun record through the REALappendCronRunLoginto a REAL on-disk SQLite database (OPENCLAW_STATE_DIR), then READ IT BACK across the SQLite disk boundary via the REALreadCronRunLogEntriesSync. The injected finalization outcome is exactly what patchedrun.tsreturns, validated independently by the new vitest regression test.Evidence after fix: SQLite disk readback (console-intercept disabled):
Negative control (pre-fix behavior reproduced): Reverting only
run.tsand re-running the regression test reproduces the bug (realrun.tsoutput), and the same SQLite readback flow persists the collapsed status:Validation commands (all green after fix):
node scripts/run-vitest.mjs src/cron/isolated-agent/run.message-tool-policy.test.ts src/cron/isolated-agent/run.meta-error-status.test.ts src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts src/cron/service.persists-delivered-status.test.ts src/cron/service.runs-one-shot-main-job-disables-it.test.ts-> 166 passednode scripts/run-tsgo.mjs -p tsconfig.core.json --incremental false-> exit 0oxfmt --checkon changed files -> all formattedoxlinton changed files -> exit 0Label: proof: live SQLite run-log readback across disk (status=ok / deliveryStatus=not-delivered) + negative control reproducing the pre-fix collapsed status=error.
AI-assisted.
Fixes #94058