fix(cron): retry transient errors before next natural cron slot (#85888)#85896
fix(cron): retry transient errors before next natural cron slot (#85888)#85896yetval wants to merge 1 commit into
Conversation
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. This PR is still necessary: current main still delays recurring error retries to the later natural slot/backoff floor, while the branch targets that path with focused tests, docs/config help updates, and sufficient runtime proof. I found no blocking correctness finding, but the recurring retry upgrade semantics still need an explicit maintainer call before merge. Canonical path: Close this PR as superseded by #74068. So I’m closing this here and keeping the remaining discussion on #74068. Review detailsBest possible solution: Close this PR as superseded by #74068. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main schedules recurring errors at the later natural slot/backoff floor, so an infrequent daily cron with a retryable 503 can be delayed until the next natural slot; the PR body also includes after-fix terminal proof for the changed path. Is this the best way to solve the issue? Yes, if maintainers accept recurring Security review: Security review cleared: The diff is limited to cron scheduler logic, config comments/help, docs, and regression tests; no concrete security or supply-chain concern was found. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 0ba6b235349a. |
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Brave Crabkin Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
Addresses ClawSweeper P1 review on openclaw#85896. When retry budget is available but croner cannot resolve the natural next slot for a cron schedule, clear nextRunAtMs (matching the existing resolveCronNextRunWithLowerBound guard) instead of refiring on bare backoff, which would reverse the unresolved-next-run protection from openclaw#66019. Adds regression covering retryable overloaded_error with computeNextRunAtMs returning undefined for a daily cron job.
Addresses second ClawSweeper P1 on openclaw#85896. Previous patch used Math.min(naturalNext, backoffNext) while in retry budget, which let high-frequency recurring schedules (e.g. an every-1s job with a 5s backoff) re-fire at their natural slot before the configured backoff elapsed, weakening the existing retry-storm protection. Switch to using backoffNext directly while in retry budget. This still unblocks the daily-cron retry path from openclaw#85888 (backoffNext lands within the same day, naturalNext is ~24h away) and now also preserves the backoff floor for short-period recurring jobs. Adds regression covering an every-1s job: nextRunAtMs must be >= endedAt + everyMs (not the immediate natural slot), demonstrating the backoff floor.
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Addresses ClawSweeper P2 on openclaw#85896. The retry-budget branch in timer.ts now consults cron.retry.retryOn and cron.retry.maxAttempts for recurring schedules, so the config contract needs to reflect that recurring jobs also honor this knob. Updates the type comments on CronRetryOn and CronConfig.retry, and rewrites the Recurring retry accordion in docs/automation/cron-jobs.md to describe the new behavior: same-day backoff retry within budget, fallback to natural slot when exhausted, backoff floor preserved.
Addresses second-round ClawSweeper P2 on openclaw#85896. After broadening cron.retry to recurring jobs in the prior commits, the gateway configuration reference at docs/gateway/configuration-reference.md and the FIELD_HELP entries in src/config/schema.help.ts still described cron.retry and cron.retry.maxAttempts as one-shot-only. Update both surfaces to match the runtime contract: while a recurring job's error matches retryOn and consecutiveErrors stays within maxAttempts the next run is scheduled at the backoff slot rather than the natural cron/every slot; once exhausted, the job falls back to its natural slot.
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…l and backoff Addresses third-round ClawSweeper P2 on openclaw#85896. Prior docs commit said "falls back to its natural slot" on exhausted retry budget, but the runtime keeps using resolveCronNextRunWithLowerBound (cron) and Math.max(naturalNext, backoffNext) (every) for the exhausted branch, which schedules no sooner than both the natural slot and the backoff floor. Update configuration-reference.md, automation/cron-jobs.md, schema.help.ts, and types.cron.ts to match.
Addresses ClawSweeper P2 wording nit on openclaw#85896. The configuration-reference.md maxAttempts summary still said a recurring job "falls back to its natural schedule" while the paragraph below described "later of natural and backoff". Make the summary line and the exported CronRetryConfig.maxAttempts type comment use the same later-of phrasing so docs, help, and types are all consistent.
…penclaw#85888) The recurring cron error path in applyJobResult scheduled the next run as max(naturalNext, backoffNext), so on daily schedules the ~24h-away natural slot always dominated the short backoff. Transient provider failures (e.g. MiniMax overloaded_error 503 during the early-morning Asia/Shanghai window) were silently pushed to the next day instead of being retried, while manual reruns at the same time succeeded. While a recurring job's error matches cron.retry.retryOn and consecutiveErrors stays within cron.retry.maxAttempts, schedule the next run at the configured backoff slot rather than the natural cron/`every` slot. Preserve the unresolved-next-run guard for cron schedules whose naturalNext is undefined. Once retries are exhausted or the error is permanent, fall back to max(naturalNext, backoffNext) so high-frequency schedules cannot bypass the configured backoff after exhausting the retry budget. Updates the public surface to match the new contract: - docs/gateway/configuration-reference.md (cron.retry, maxAttempts) - docs/automation/cron-jobs.md (Recurring retry accordion) - src/config/schema.help.ts (FIELD_HELP) - src/config/types.cron.ts (CronRetryOn, CronRetryConfig.maxAttempts, CronConfig.retry) Adds regression coverage: - daily cron retry lands within the same day on retryable error - high-frequency every-1s job preserves backoff floor on retryable error - cron schedule with unresolved naturalNext clears nextRunAtMs on retryable error - exhausted retry budget falls back to the natural slot Closes openclaw#85888
acc4e5f to
1c8a66c
Compare
|
@clawsweeper re-review Added live runtime proof to the PR body: real-Linux Fedora 43 / Node 22 / real
Captured live log lines and redacted tmp paths included in the PR body. |
|
🦞🧹 Reason: re-review requires an open issue or PR. |
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
ClawSweeper applied the proposed close for this PR.
|
Summary
Math.max(naturalNext, backoffNext)for the next scheduled run. For daily schedules (e.g.0 5 * * *),naturalNextis ~24h ahead and always dominated the short backoff, so retries for transient provider errors were effectively suppressed.overloaded_error(503) during 05:00-07:30 Asia/Shanghai never re-fired the same day, while manual reruns at 08:30+ (after the busy window) succeeded because they bypassed the broken scheduler path.resolveCronExecutionRetryHint, schedule atbackoffNext. OnceconsecutiveErrors > retryConfig.maxAttemptsor the error is permanent, fall back tomax(naturalNext, backoffNext)so high-frequency recurring schedules cannot bypass the configured backoff after retry-budget exhaustion. When croner cannot resolve the natural next slot for a cron schedule, the retry path clearsnextRunAtMsto preserve the unresolved-next-run guard from [Bug] Cron spin loop via MIN_REFIRE_GAP_MS fallback when computeJobNextRunAtMs returns undefined (incomplete fix for #17821) #66019.Closes #85888
Files
src/cron/service/timer.ts— split recurring error path into "within retry budget" (usebackoffNext, preserve unresolved-next-run guard) vs. "exhausted / permanent" (usemax(naturalNext, backoffNext)).src/cron/service/timer.regression.test.ts— four regressions: (1) same-day backoff retry on retryable daily-cron error, (2) high-frequencyevery-1spreserves backoff floor, (3) cron unresolved-next-run guard, (4) exhausted budget falls back to natural slot.docs/gateway/configuration-reference.md,docs/automation/cron-jobs.md,src/config/schema.help.ts,src/config/types.cron.ts— aligncron.retrycontract surfaces with the new recurring-job behavior.Real behavior proof
Behavior or issue addressed: Scheduled recurring cron jobs that hit a retryable provider error (e.g. MiniMax
overloaded_error503) on an infrequent schedule (#85888 reporter: daily0 5 * * *Asia/Shanghai during 05:00-07:30 CST) had their next run pushed to the next natural slot (~24h away) instead of being retried with backoff, so the failure was effectively swallowed for that day while manual reruns at 08:30+ succeeded. After this patch, while the error is incron.retry.retryOnandconsecutiveErrorsis still withincron.retry.maxAttempts, the next run is scheduled at the configured backoff slot rather than the natural cron/everyslot; once retries are exhausted the job falls back tomax(naturalNext, backoffNext)so high-frequency schedules cannot bypass backoff.Real environment tested: Fresh DigitalOcean Fedora 43 Cloud Edition droplet (
fedora-s-1vcpu-2gb-nyc1, redacted IP), kernel6.17.1-300.fc43.x86_64,node v22.22.2,pnpm 9.15.9. Patched OpenClaw checkout at PR head1c8a66cfe3f47e9dd2a17eaa2c0e93295a9c9d32. Realnode:fs/promiseswrites under/tmp/cron-proof-85888-<redacted>/jobs.json. Real productionapplyJobResult, realcreateCronServiceState, realcomputeJobNextRunAtMs, realresolveCronExecutionRetryHint– none of the scheduler code under test is mocked. Theproof-85888.mtsscript is executed via the project's owntsx(./node_modules/.bin/tsx) directly from the patched repo, outside the vitest test harness.Exact steps or command run after this patch: Provision the droplet, install Node 22 + pnpm 9, clone the PR head, install deps, then run two captures against the patched code — vitest against the four new
#85888regressions, then a directtsxinvocation ofproof-85888.mtsthat exercises the productionapplyJobResultoutside the test harness on real fs.Evidence after fix: Redacted live terminal output from the Fedora 43 droplet on PR head
1c8a66cfe3. Token, IP, and tmp paths redacted in-place; everything else is the actual stdout from the patched scheduler running on real Linux. Two captures: vitest runner output for the new regressions (real fs viasetupCronRegressionFixtures+ real Node 22 + real Linux), and a directtsx-invokedproof-85888.mtsruntime script that imports the productionapplyJobResultand writes a realjobs.jsonto/tmp.Direct runtime proof (
proof-85888.mts— realapplyJobResultinvoked outside the test harness, real fs writes under/tmp, realcreateCronServiceState, no mocks of the scheduler under test):Observed result after fix:
0 5 * * *Asia/Shanghai,overloaded_error503): on currentmainthe runtime would pushnextRunAtMsto2026-05-24T21:00:00.000Z(15.95h away, next-day inAsia/Shanghai); on this PR head it lands at2026-05-24T05:03:14.264Z(30s after endedAt). Same-day retry achieved.scheduledMode=backoff,retryBudgetAvailable=true.every-1scadence and a 30s backoff, the patched runtime respects the backoff floor and schedules atendedAt + 30000msinstead of the +800ms natural slot. Retry-storm protection preserved.scheduledMode=backoff.consecutiveErrors > maxAttempts,scheduledModeflips tonatural_floorandnextRunAtMsresolves to the next natural cron slot (max(naturalNext, backoffNext)semantics), so a high-frequency schedule cannot bypass backoff after the retry budget is gone.retryBudgetAvailable=false.cron: applying error backofflog line on a real Node 22 process with real fs writes (/tmp/cron-proof-85888-<redacted>/jobs.json).What was not tested: End-to-end flight against the live MiniMax
minimax-portal / MiniMax-M2.7provider on the issue reporter's tenant. I do not have credentials for that tenant. The retryable-error contract is exercised here by passing the exactFailoverError: The AI service is temporarily overloaded (overloaded_error). status=503text the issue logs show; the upstreamresolveCronExecutionRetryHintandresolveFailoverReasonFromErrorclassifiers are production code and are not mocked in the proof above, so the patched scheduling decision is being made on the sameretryable/overloadedclassification path that production code would take when MiniMax returns the same error.Test plan
node scripts/run-vitest.mjs src/cron/service/timer.regression.test.ts -t "85888"— 4/4 PR regressions pass on Fedora 43 / Node 22tsx proof-85888.mtsagainst the patched scheduler — three scenarios match expected behaviorcron/retry-hint.test.ts+cron/service.jobs.test.tsin CI