Scheduler: a pass that outlives its lease deletes the next pass's lease, so a third pass can start #576

New issue

Closed

opened 2026-10-01 10:47:46 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:47:46 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the scheduler's lease and what each step of a pass claims.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at notifications/management/commands/send_due_reminders.py:189 (with scheduler.only_one_pass(every * 2)); the other locations have not moved.

The pass lease that #221 added releases itself unconditionally. A pass that runs longer than the lease's lifetime ends by deleting the lease that a later pass has since taken, which lets yet another pass start beside that one. Reminders, webhooks and store copies claim their own rows, so they are not sent twice, but sync connections claim nothing, so overlapping passes can run the same connection's sync at the same time.

Where: core/scheduler.py:90-98 (only_one_pass); notifications/management/commands/send_due_reminders.py:187 (with scheduler.only_one_pass(every * 2)); plugins/syncing.py:70-109 (sync_connection, due_connections).
What happens:
1. only_one_pass takes the lease with cache.add(LEASE_KEY, timezone.now().isoformat(), max(seconds, 60)), 600 seconds with the default --every 300, and in finally runs cache.delete(LEASE_KEY) whenever it took it, without checking that the key still holds its own value.
2. Pass A runs longer than 600 seconds: a slow sync already started is never cut off (the budget only decides whether to begin another), and up to 50 webhook deliveries and one notifier call per reminder can each wait on a timeout.
3. The key expires; pass B (the loop's next pass, or cron beside --loop, which the Compose files mention as alternatives) takes it.
4. Pass A finishes and deletes the key. Pass C finds no lease and runs beside B.
5. due_connections() selects by synced_at, which sync_connection writes only when the sync has finished, so B and C both run every connection that is due, concurrently.
Why it matters: the lease exists so that "two schedulers never overlap" (#221), and it stops holding under exactly the load that makes passes long. Two concurrent runs of one sync connection do the network work twice and race on the plugin's SyncLink rows; a sync that creates records or links on first sight can create them twice.
Fix: store a unique token (for example uuid4().hex) as the lease's value and release it only if the key still holds that token; the database cache has no atomic compare-and-delete, so a small SchedulerLease row updated with filter(owner=token).delete() is the dependable form. Separately, give a sync a claim of its own (stamp synced_at, or a syncing_since column, with a conditional UPDATE before calling the plugin, as reminders already do).

Related to #221, which added the lease and the per-row claims.

Done when

A test in tests/test_scheduler.py that enters only_one_pass, replaces the key's value as a later pass would, leaves the block, and asserts the later pass's key is still present.
A test in tests/test_scheduler.py (or the sync tests) that calls run_syncs while another run holds the connection's claim, and asserts the plugin's sync is called once.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the scheduler's lease and what each step of a pass claims. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `notifications/management/commands/send_due_reminders.py:189` (`with scheduler.only_one_pass(every * 2)`); the other locations have not moved. The pass lease that #221 added releases itself unconditionally. A pass that runs longer than the lease's lifetime ends by deleting the lease that a later pass has since taken, which lets yet another pass start beside that one. Reminders, webhooks and store copies claim their own rows, so they are not sent twice, but sync connections claim nothing, so overlapping passes can run the same connection's sync at the same time. - **Where:** `core/scheduler.py:90-98` (`only_one_pass`); `notifications/management/commands/send_due_reminders.py:187` (`with scheduler.only_one_pass(every * 2)`); `plugins/syncing.py:70-109` (`sync_connection`, `due_connections`). - **What happens:** 1. `only_one_pass` takes the lease with `cache.add(LEASE_KEY, timezone.now().isoformat(), max(seconds, 60))`, 600 seconds with the default `--every 300`, and in `finally` runs `cache.delete(LEASE_KEY)` whenever it took it, without checking that the key still holds its own value. 2. Pass A runs longer than 600 seconds: a slow sync already started is never cut off (the budget only decides whether to begin another), and up to 50 webhook deliveries and one notifier call per reminder can each wait on a timeout. 3. The key expires; pass B (the loop's next pass, or cron beside `--loop`, which the Compose files mention as alternatives) takes it. 4. Pass A finishes and deletes the key. Pass C finds no lease and runs beside B. 5. `due_connections()` selects by `synced_at`, which `sync_connection` writes only when the sync has finished, so B and C both run every connection that is due, concurrently. - **Why it matters:** the lease exists so that "two schedulers never overlap" (#221), and it stops holding under exactly the load that makes passes long. Two concurrent runs of one sync connection do the network work twice and race on the plugin's `SyncLink` rows; a sync that creates records or links on first sight can create them twice. - **Fix:** store a unique token (for example `uuid4().hex`) as the lease's value and release it only if the key still holds that token; the database cache has no atomic compare-and-delete, so a small `SchedulerLease` row updated with `filter(owner=token).delete()` is the dependable form. Separately, give a sync a claim of its own (stamp `synced_at`, or a `syncing_since` column, with a conditional `UPDATE` before calling the plugin, as reminders already do). Related to #221, which added the lease and the per-row claims. ## Done when - A test in `tests/test_scheduler.py` that enters `only_one_pass`, replaces the key's value as a later pass would, leaves the block, and asserts the later pass's key is still present. - A test in `tests/test_scheduler.py` (or the sync tests) that calls `run_syncs` while another run holds the connection's claim, and asserts the plugin's `sync` is called once.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:47:46 +00:00

tiagoagueda added the

bug

label

2026-10-01 10:47:46 +00:00

tiagoagueda referenced this issue from a commit

2026-10-04 02:35:50 +00:00

Give each scheduler pass its own lease token and claim a sync before running it

tiagoagueda closed this issue

2026-10-04 02:35:50 +00:00

tiagoagueda referenced this issue