fix(cron): persist failure alert delivery status#95129
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 7:47 AM ET / 11:47 UTC. Summary PR surface: Source +95, Tests +176. Total +271 across 7 files. Reproducibility: yes. source-level: current main records failure-notification status before the configured per-job failureAlert request result can return, matching the linked production report. 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 status fix once maintainers accept the bounded wait; use a separate post-finish status update path if finished events must never wait on alert transport. Do we have a high-confidence way to reproduce the issue? Yes, source-level: current main records failure-notification status before the configured per-job failureAlert request result can return, matching the linked production report. Is this the best way to solve the issue? Yes, if maintainers accept the bounded wait; otherwise the safer design is a post-finish persistence update that records alert status without delaying finished events. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d1cbe29f3d52. Label changesLabel justifications:
Evidence reviewedPR surface: Source +95, Tests +176. Total +271 across 7 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
Bot
added
rating: 🦪 silver shellfish
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
1 participant
Summary
failureAlertdelivery status from the actual alert request before emitting the cronfinishedevent/run log.delivered, send failure asnot-delivered, and bounded send timeout asunknowninstead of leaving errored runs atnot-requested.Fixes #92058.
Verification
node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/cron/service.persists-delivered-status.test.ts src/cron/service/failure-alerts.ts src/cron/service/ops.ts src/cron/service/state.ts src/cron/service/timer.ts src/gateway/server-cron-notifications.ts src/gateway/server-cron-notifications.test.ts --type-aware --report-unused-disable-directives-severity error --threads=1git diff --check upstream/main..HEADnode scripts/run-vitest.mjs src/cron/service.persists-delivered-status.test.ts src/cron/service.failure-alert.test.ts src/cron/service/timer.regression.test.ts src/gateway/server-cron-notifications.test.ts packages/gateway-protocol/src/cron-validators.test.ts src/cli/cron-cli.test.tspnpm tsgo:corepnpm tsgo:core:test.agents/skills/autoreview/scripts/autoreview --mode branch --base upstream/main(autoreview clean: no accepted/actionable findings reported)Real behavior proof
Behavior addressed: Errored cron runs with configured
failureAlert.after = 1no longer persist the failure-notification status asnot-requestedwhen the alert path is actually requested.Real environment tested: Local OpenClaw source checkout on macOS with an isolated
OPENCLAW_STATE_DIR; the proof usedCronService.run(job.id, "force"), the realrunCronCommandJobchild-process command runner, the productionsendGatewayCronFailureAlertgateway webhook path,fetchWithSsrFGuard, cron store persistence, and SQLite cron run-log readback.Exact steps or command run after this patch:
OPENCLAW_STATE_DIR="$(mktemp -d /tmp/openclaw-95129-state.XXXXXX)" node --import tsx --input-type=module <proof script>plus the focused test command listed above.Evidence after fix: Real proof output from the
CronService.run(force)command-failure run. Temp paths are redacted; the raw command result shows the actual child process exited with code 7, and the finished event/job state/run log show the resulting failure-alert delivery status persisted asnot-delivered.{ "scenario": "CronService.run(force) real command failure with per-job failureAlert.after=1", "head": "40dd4c72e73a12372f30157d863e0253f9d100f0", "stateDir": "<isolated-temp-state>", "storePath": "<isolated-temp-store>/cron/jobs.json", "job": { "name": "issue-92058-real-command-failure-alert-proof", "payloadKind": "command", "command": [ "/bin/sh", "-lc", "printf proof-command-started; exit 7" ], "delivery": { "mode": "none" }, "failureAlert": { "after": 1, "mode": "webhook", "channel": "telegram", "to": "http://127.0.0.1:9/openclaw-cron-proof", "cooldownMs": 0 } }, "rawCommandResult": { "status": "error", "error": "command exited with code 7", "summary": "proof-command-started", "diagnosticSummary": "proof-command-started", "exitCode": 7 }, "runResult": { "ok": true, "ran": true }, "finishedEvent": { "action": "finished", "status": "error", "deliveryStatus": "not-requested", "failureNotificationDelivery": { "delivered": false, "status": "not-delivered", "error": "Blocked hostname or private/internal/special-use IP address" } }, "persistedJobState": { "lastRunStatus": "error", "lastStatus": "error", "consecutiveErrors": 1, "lastDeliveryStatus": "not-requested", "lastFailureNotificationDelivered": false, "lastFailureNotificationDeliveryStatus": "not-delivered", "lastFailureNotificationDeliveryError": "Blocked hostname or private/internal/special-use IP address", "lastFailureAlertAtMsType": "number" }, "persistedRunLog": { "action": "finished", "status": "error", "deliveryStatus": "not-requested", "failureNotificationDelivery": { "status": "not-delivered", "delivered": false, "error": "Blocked hostname or private/internal/special-use IP address" }, "runId": "cron:<job-id>:<timestamp>" }, "warningMessages": [ "cron: job run returned error status", "cron: failure alert webhook blocked by SSRF guard" ] }Observed result after fix: For a failed job with
failureAlert.after = 1and no primary delivery, the job state/event/run log keep primarylastDeliveryStatusasnot-requestedwhilelastFailureNotificationDeliveryStatusrecords the actual alert path result. In the real guarded-webhook proof above, the persisted result isnot-delivered; the regression tests also coverdeliveredon success andunknownon bounded timeout.What was not tested: No packaged/Docker E2E lane or live external messaging channel was run. The live proof used a real local cron command failure and the production guarded webhook path from source, and channel delivery remains covered by the focused cron/gateway tests.