Leases: on PostgreSQL two processes can both take an expired scheduler, backup or sync lease #1130

New issue

Closed

opened 2026-10-31 03:04:14 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-31 03:04:14 +00:00

Owner

What exists today

Three leases are taken with cache.add on the default cache, which is Django's DatabaseCache unless the operator sets POSTULO_CACHE_URL (config/settings/base.py:213-215):

core/scheduler.py:91-109 only_one_pass, called from notifications/management/commands/send_due_reminders.py:198.
core/backups.py:245-261 take, called by the scheduled backup and by the Back up now errand (core/slow.py:137).
plugins/syncing.py:81-113 sync_connection, called by the scheduler (plugins/syncing.py:197) and by Sync now (plugins/views.py:580).

Django 6.1.2's DatabaseCache.add goes through _base_set("add", ...) (.venv/Lib/site-packages/django/core/cache/backends/db.py). Inside transaction.atomic it runs a plain SELECT cache_key, expires ... WHERE cache_key = %s (no FOR UPDATE), and if a row exists with expires < now it runs UPDATE ... SET value, expires WHERE cache_key = %s and returns True. The UPDATE does not repeat the expires < now condition.

On PostgreSQL under READ COMMITTED, two processes A and B that both run the SELECT before either commits both see the expired row. A's UPDATE takes the row lock; B's UPDATE waits, and when A commits PostgreSQL re-evaluates B's WHERE cache_key = %s on the new row version, which still matches, so B overwrites A's token and also returns True. Both run. A then finds at its end that the key holds B's token and leaves it, so the two runs overlap fully.

The other cases are safe: with no row, both INSERT and the loser's IntegrityError is caught (except DatabaseError: return False); with a live row, the INSERT fails the same way. On SQLite the SELECT and UPDATE run inside BEGIN IMMEDIATE (config/sqlite.py:52), so they are serialised and the race cannot happen. Redis and Memcached have an atomic add.

An expired row remains whenever a holder outlived its lease or died without the finally (a killed worker, an OOM, a restart past the grace period): a sync past LEASE_SECONDS (15 min, plugins/syncing.py:44), a reminders pass past every * 2, a backup killed mid-way (R3). The race then needs two contenders inside the same few milliseconds, which the scheduler and a person pressing Sync now or Back up now can be. Rare, but it is the exact overlap #586 and #576 set out to prevent (duplicated DAV contacts in #586).

No measurement: there is no local PostgreSQL in the brief's limits; the evidence is the code path above.

What would change

A small lease table of Postulo's own (core/leases.py, a model with key unique, token, expires) with three operations that are atomic on both databases:

take: INSERT; on IntegrityError, UPDATE ... SET token, expires WHERE key = %s AND expires < now, and the caller holds the lease if and only if the row count is 1. On PostgreSQL the waiting UPDATE re-checks expires < now on the committed row and matches nothing.
release: DELETE WHERE key = %s AND token = %s, which is also the atomic compare-and-delete that scheduler.only_one_pass and sync_connection say the cache lacks (the remaining gap noted in #576 and #988).
renew: UPDATE ... SET expires WHERE key = %s AND token = %s, which R3 needs.

The three callers switch to it; the lease no longer depends on which cache the operator chose. Library check: django-pglocks and PostgreSQL advisory locks are PostgreSQL only and are held by a connection, which a lease across a long task is not; the cache libraries offer nothing that works on SQLite and PostgreSQL alike. A dozen lines of ORM is the smaller dependency.

Tests: one per operation, plus a PostgreSQL-only test in the CI Postgres job that runs two take calls on two connections over an expired row and expects one winner.

What it saves or protects

small: protects the "one at a time" promise of the scheduler pass, the backup and each sync connection on PostgreSQL with the default database cache; the window is narrow (milliseconds, and only over an expired row)

Accessibility

none

Decisions

Own table, or keep the cache and add select_for_update? Recommended: own table. DatabaseCache cannot be changed, and an operator's Redis cache would make the lease vanish on a cache flush.
Same issue as R3? Recommended: no, separate issues; R3's renewal uses the renew operation added here, so R3 lands after this one.

Found by the efficiency and reliability audit of 2026-10-30 (finding R2; index postulo/postulo#1108). Checked against main at daee30b15 (2026-10-30).

## What exists today Three leases are taken with `cache.add` on the default cache, which is Django's `DatabaseCache` unless the operator sets `POSTULO_CACHE_URL` (`config/settings/base.py:213-215`): - `core/scheduler.py:91-109` `only_one_pass`, called from `notifications/management/commands/send_due_reminders.py:198`. - `core/backups.py:245-261` `take`, called by the scheduled backup and by the *Back up now* errand (`core/slow.py:137`). - `plugins/syncing.py:81-113` `sync_connection`, called by the scheduler (`plugins/syncing.py:197`) and by *Sync now* (`plugins/views.py:580`). Django 6.1.2's `DatabaseCache.add` goes through `_base_set("add", ...)` (`.venv/Lib/site-packages/django/core/cache/backends/db.py`). Inside `transaction.atomic` it runs a plain `SELECT cache_key, expires ... WHERE cache_key = %s` (no `FOR UPDATE`), and if a row exists with `expires < now` it runs `UPDATE ... SET value, expires WHERE cache_key = %s` and returns `True`. The `UPDATE` does not repeat the `expires < now` condition. On PostgreSQL under READ COMMITTED, two processes A and B that both run the `SELECT` before either commits both see the expired row. A's `UPDATE` takes the row lock; B's `UPDATE` waits, and when A commits PostgreSQL re-evaluates B's `WHERE cache_key = %s` on the new row version, which still matches, so B overwrites A's token and also returns `True`. Both run. A then finds at its end that the key holds B's token and leaves it, so the two runs overlap fully. The other cases are safe: with no row, both `INSERT` and the loser's `IntegrityError` is caught (`except DatabaseError: return False`); with a live row, the `INSERT` fails the same way. On SQLite the `SELECT` and `UPDATE` run inside `BEGIN IMMEDIATE` (`config/sqlite.py:52`), so they are serialised and the race cannot happen. Redis and Memcached have an atomic add. An expired row remains whenever a holder outlived its lease or died without the `finally` (a killed worker, an OOM, a restart past the grace period): a sync past `LEASE_SECONDS` (15 min, `plugins/syncing.py:44`), a reminders pass past `every * 2`, a backup killed mid-way (R3). The race then needs two contenders inside the same few milliseconds, which the scheduler and a person pressing *Sync now* or *Back up now* can be. Rare, but it is the exact overlap #586 and #576 set out to prevent (duplicated DAV contacts in #586). No measurement: there is no local PostgreSQL in the brief's limits; the evidence is the code path above. ## What would change A small lease table of Postulo's own (`core/leases.py`, a model with `key` unique, `token`, `expires`) with three operations that are atomic on both databases: - take: `INSERT`; on `IntegrityError`, `UPDATE ... SET token, expires WHERE key = %s AND expires < now`, and the caller holds the lease if and only if the row count is 1. On PostgreSQL the waiting `UPDATE` re-checks `expires < now` on the committed row and matches nothing. - release: `DELETE WHERE key = %s AND token = %s`, which is also the atomic compare-and-delete that `scheduler.only_one_pass` and `sync_connection` say the cache lacks (the remaining gap noted in #576 and #988). - renew: `UPDATE ... SET expires WHERE key = %s AND token = %s`, which R3 needs. The three callers switch to it; the lease no longer depends on which cache the operator chose. Library check: `django-pglocks` and PostgreSQL advisory locks are PostgreSQL only and are held by a connection, which a lease across a long task is not; the cache libraries offer nothing that works on SQLite and PostgreSQL alike. A dozen lines of ORM is the smaller dependency. Tests: one per operation, plus a PostgreSQL-only test in the CI Postgres job that runs two `take` calls on two connections over an expired row and expects one winner. ## What it saves or protects small: protects the "one at a time" promise of the scheduler pass, the backup and each sync connection on PostgreSQL with the default database cache; the window is narrow (milliseconds, and only over an expired row) ## Accessibility none ## Decisions - **Own table, or keep the cache and add `select_for_update`?** Recommended: own table. `DatabaseCache` cannot be changed, and an operator's Redis cache would make the lease vanish on a cache flush. - **Same issue as R3?** Recommended: no, separate issues; R3's renewal uses the `renew` operation added here, so R3 lands after this one. --- Found by the efficiency and reliability audit of 2026-10-30 (finding R2; index postulo/postulo#1108). Checked against `main` at `daee30b15` (2026-10-30).

tiagoagueda added this to the 0.6.0 milestone

2026-10-31 03:04:14 +00:00

tiagoagueda added the

performance

bug

labels

2026-10-31 03:04:14 +00:00

tiagoagueda referenced this issue

2026-10-31 03:04:40 +00:00

Efficiency and reliability audit of 2026-10-30: index #1108

tiagoagueda referenced this issue from a commit

2026-11-10 13:37:55 +00:00