Skip to content

fix(cron): keep scheduler alive when job-store persistence fails - #5376

Open
rickererer wants to merge 1 commit into
HKUDS:mainfrom
rickererer:fix/cron-save-failure-stops-scheduler
Open

fix(cron): keep scheduler alive when job-store persistence fails#5376
rickererer wants to merge 1 commit into
HKUDS:mainfrom
rickererer:fix/cron-save-failure-stops-scheduler

Conversation

@rickererer

Copy link
Copy Markdown

Summary

Fixes a silent failure mode where a single persistence error (e.g. disk full, permission change, locked file) inside CronService._on_timer permanently kills the cron scheduler: _save_store() raises, the exception escapes the try/finally, and _arm_timer() — which sits outside the block — is never called again, so no further ticks are scheduled until the process restarts or a user manually re-arms the timer via add_job/update_job/remove_job/enable_job.

Root cause

nanobot/cron/service.py, _on_timer():

  • _save_store() -> _atomic_write() performs open() / fsync() / os.replace(); any OSError propagates.
  • The finally block only decrements _active_executions; the re-arm (_arm_timer()) lives after the block.
  • tick() creates the asyncio task with no exception handling, so the exception kills _timer_task ("Task exception was never retrieved").
  • The read path is already defended (_load_jobs preserves corrupt stores as .corrupt-<ts> backups); the write path had none.

Changes

  • nanobot/cron/service.py:
    • Move _arm_timer() into the finally block so the next tick is always scheduled, even on unexpected failures.
    • Wrap _save_store() in its own try/except that logs and keeps the in-memory store; the next tick retries the save.
    • Simplify the store is None branch (it no longer needs its own _arm_timer() since finally covers it).
  • tests/cron/test_cron_service.py:
    • Add test_save_store_failure_does_not_kill_scheduler: forces _save_store to raise, then asserts the service is still re-armed and can run a due job on the next healthy tick.

Validation

  • python -m pytest tests/cron/test_cron_service.py -q
  • python -m ruff check nanobot/cron/service.py tests/cron/test_cron_service.py

Behavior notes

  • A transient write failure is logged at error level and retried on the next tick; job state is preserved in memory.
  • No behavior change for the success path (identical save + re-arm ordering).

A single OSError from _save_store() (disk full, permission change, locked
file) escaped _on_timer's try/finally and killed the asyncio timer task,
because _arm_timer() sits outside the block. All scheduled jobs silently
stopped until restart or a manual re-arm via add_job/update_job/remove_job.

Move _arm_timer() into the finally block and guard the whole tick body
(including _load_store, which can persist during agent-binding migrations)
so a transient persistence failure is logged and retried on the next tick
instead of killing the scheduler.

Add test_save_store_failure_does_not_kill_scheduler to cover the failure
path that existing tests (which mock _arm_timer) never exercised.
@chengyongru chengyongru added priority: p2 Normal backlog: minor bug, enhancement, docs, cleanup, edge case, or unvalidated proposal. bug Something isn't working fix test labels Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working fix priority: p2 Normal backlog: minor bug, enhancement, docs, cleanup, edge case, or unvalidated proposal. test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants