Skip to content

fix(scheduler): reject past one-time 'at' cron schedules - #1526

Open
lifelmy wants to merge 17 commits into
TokenRhythm:mainfrom
lifelmy:agent-tasks/1516
Open

fix(scheduler): reject past one-time 'at' cron schedules#1526
lifelmy wants to merge 17 commits into
TokenRhythm:mainfrom
lifelmy:agent-tasks/1516

Conversation

@lifelmy

@lifelmy lifelmy commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #1516.

A one-time at cron schedule whose timestamp is already in the past is currently accepted. On the next scheduler tick the job fires, and because at jobs are delete_after_run, the one-shot job is immediately removed. The net effect is that the payload runs right away with no future occurrence and then disappears — exactly the surprising behavior reported in the issue (CLI exits 0, Web UI shows "Schedule created" then immediately runs the reminder, and the job vanishes from the list).

This change validates the at timestamp against the current time and rejects a past value before persisting, so the caller gets a clear error instead of a silent immediate run.

What changed

  • Added _reject_past_at() in scheduler/ops.py, which parses the ISO-8601 value (reusing parse_iso_at) and raises a field-named ValueError (schedule.at is in the past: ...) when it is earlier than now.
  • Wired it into the two structured-schedule entry points that create/repoint an at job: SchedulerOps.add and SchedulerOps.update. The check runs before next_run_at is set, so nothing is stored on rejection.
  • Recurring cron and every schedules are untouched; only one-time at is affected.

The error is raised as ValueError, which the RPC layer and the admin cron tool already translate into their respective field-named / ToolError messages, matching how the existing timezone and interval validations surface.

I deliberately kept this to a hard rejection rather than an opt-in catch-up flag, since the issue notes catch-up is not an existing intentional feature here. If maintainers would prefer an explicit opt-in (e.g. allowPast) instead, I'm happy to adjust.

Tests

Added regression tests in tests/test_scheduler/test_ops_strict_schedule.py:

  • test_ops_add_at_rejects_past_timestamp — a past at on add raises and persists nothing.
  • test_ops_update_at_rejects_past_timestamp — repointing an existing job to a past at is rejected.

Verified locally:

  • .venv/bin/python -m pytest tests/test_scheduler tests/test_gateway/test_rpc_cron_strict_schedule.py tests/test_tools/test_admin_cron_strict.py → 221 passed.
  • ruff check src tests → all checks passed.

Third-party origins

none.

I use a coding assistant to help implement, and I review and take responsibility for the final change.

A one-time 'at' schedule with a timestamp already in the past is accepted,
fires on the next scheduler tick, and the one-shot job is then deleted — so
the payload runs immediately with no future occurrence and disappears. That
is almost never what a caller scheduling a one-time reminder intends.

Validate the 'at' timestamp against the current time in SchedulerOps.add and
SchedulerOps.update (the two structured-schedule entry points) and raise a
field-named ValueError before persisting, matching the existing timezone and
interval validation style. Recurring cron/every schedules are unaffected.

Fixes TokenRhythm#1516
@lifelmy
lifelmy marked this pull request as ready for review September 2, 2026 11:17
@lifelmy

lifelmy commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed this PR onto the latest upstream/main (3f92428e); current head is a1762a25. The merge completed without manual conflict edits and preserved the strict past-schedule rejection.

Verification on the refreshed head:

  • .venv/bin/pytest -q tests/test_scheduler/test_ops_strict_schedule.py -> 17 passed
  • focused ruff check -> passed
  • git diff --check upstream/main...HEAD -> passed

GitHub reports the branch as mergeable; the newly triggered CI jobs are still running.

@lifelmy

lifelmy commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed this PR onto the latest upstream/main (4535029); new head is 9558a1e. The branch was 1 commit behind and merged cleanly without manual conflict edits. Verification: 17 strict-schedule tests passed; focused ruff passed; git diff --check passed.

@lifelmy

lifelmy commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Task A refresh (2026-09-05): fetched latest upstream/main at 770ff387c6, merged it without conflicts, and pushed head e3faf53a9a. The two new base commits touch provider ensemble/runtime and WebUI toast-host paths; the scheduler fix remains unchanged.

Verification on the refreshed head:

  • uv run pytest -q tests/test_scheduler/test_ops_strict_schedule.py -> 17 passed
  • focused uv run ruff check -> passed
  • git diff --check upstream/main...HEAD -> passed

No new comments/reviews or unresolved review threads.

@lifelmy

lifelmy commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed this branch onto latest upstream/main (1663b1a); new head is 37231ee. The one-commit base update merged without conflicts, and the strict one-time schedule diff remains limited to scheduler ops plus its regression tests. Verification: 17 strict-schedule tests passed, focused Ruff passed, and git diff --check passed.

@lifelmy

lifelmy commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Task A refresh: merged latest upstream/main (846bbc6b) without conflicts; new head is 7af664e42. The strict one-time schedule behavior remains covered. Verification: scheduler/parser/RPC strict-schedule suites passed (44 tests), focused Ruff passed, and git diff --check upstream/main...HEAD passed.

@lifelmy

lifelmy commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Task A refresh: merged latest upstream/main (085bfed00d) without conflicts; new head is 735b225602. The upstream change consolidates settings and durable turn admission, while this PR’s strict rejection of past one-time schedules remains intact. Verification: PYENV_VERSION=3.11.11 uv run pytest -q tests/test_scheduler/test_ops_strict_schedule.py (17 passed), focused Ruff passed, and git diff --check upstream/main...HEAD passed.

@lifelmy

lifelmy commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed onto current upstream/main (cfa8e4b); merge completed without conflicts and preserved the scheduler change. New head: 172d5c4. Verification: .venv/bin/pytest -q tests/test_scheduler/test_ops_strict_schedule.py (17 passed); git diff --check refs/task-a/upstream/main...HEAD (clean). Fresh CI is running.

@lifelmy

lifelmy commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Task A refresh: merged latest upstream/main (bf7fa0e1e) without conflicts; new head is 4ae1088e5. The upstream progressive-tool-disclosure change does not overlap the scheduler fix. Verification: .venv/bin/pytest -q tests/test_scheduler/test_ops_strict_schedule.py (17 passed), focused Ruff passed, and git diff --check upstream/main...HEAD passed.

@lifelmy

lifelmy commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Task A refresh: merged latest upstream/main (05a6b8619) into the branch without conflicts; new head is a2d0ee1622a59b1bcb32fbcced1b1fbd60c4e5ef. The branch was 1 commit behind; the merge preserved the strict one-time at schedule rejection change.

Verification: .venv/bin/pytest -q tests/test_scheduler/test_ops_strict_schedule.py → 17 passed; git diff --check upstream/main clean. No new review feedback; unresolved threads = 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Past one-time Cron schedules are accepted and executed immediately

1 participant