daybook/issues/010-concurrent-posts-race-past-quota.md

010 — Concurrent POSTs race past the quota check, landing usage over quota

Status: Fixed Severity: High — the #006 fix's own writeup flagged this as an unhandled case; now confirmed as a real, reproducible violation, not just a theoretical gap.

Follow-up to the #007 handover, which specifically asked to probe "multi-request race conditions around eviction (two posts near the quota boundary at once)."

Description

make_room_for() (artifacts/storage.py) — even after the #006 fix — still does a plain read-then-act sequence with no locking:

quota = user_quota_bytes(user)
if incoming_bytes > quota: raise QuotaExceeded(...)
used = user_usage_bytes(user)                 # <- read
... compute what needs evicting based on `used` ...
... delete evicted artifacts ...
... caller then creates the new Artifact row ...

Nothing serializes this read-compute-write sequence across concurrent requests (no select_for_update, no atomic transaction wrapping the whole check-evict-create flow, no per-user lock). Two or more requests arriving close together each read a used value that doesn't yet reflect the others' in-flight writes, so each independently concludes "this fits" and proceeds.

Reproduction

Set test account quota to 1551 bytes: enough for the one existing kept artifact (851B) plus exactly one more ~658B artifact — no more. Fired 5 truly concurrent POST requests (backgrounded shell jobs, wait), each creating a ~658B markdown artifact:

[1] 201   [2] 201   [3] 413   [4] 201   [5] 201

4 of 5 succeeded where only 1 should have fit. Final measured state:

quota: 1551
final usage: 2167          <- 616 bytes OVER quota
kept artifact (851B) + 2 surviving "Race" artifacts (658B each) = 2167

Note it's not simply "4 got created and stayed" — the responses claimed 4 successes, but only 2 of those 4 artifacts still existed afterward (the other 2 got deleted by a different concurrent request's own eviction pass, racing against them). So this isn't just an over-quota bug, it's also nondeterministic which artifacts survive: a client can receive a 201 with a real artifact id/url for an artifact that gets silently deleted moments later by an unrelated concurrent request's eviction sweep — a variant of the #006 data-loss problem, but now triggered by legitimate concurrent traffic rather than a single doomed request.

Suggested Fix

Wrap the read-evict-create sequence in a transaction with a row lock scoped per user — e.g. select_for_update() on the UserProfile row (or an equivalent per-user lock) held for the duration of make_room_for() + the subsequent Artifact.objects.create(...), so concurrent requests for the same user serialize around quota accounting. Requests for different users should remain unaffected (lock scope must be per-user, not global). SQLite's own file-level write lock already serializes the raw INSERT/DELETE statements, but that's not the same as serializing the "decide whether this fits" business logic wrapping them — the race is in that gap.

Resolution

make_room_for now performs a real UPDATE on the user's UserProfile row (a no-op quota_bytes=F("quota_bytes")) as its first statement, before reading used - this forces SQLite to acquire its write lock immediately rather than after the read, closing the stale-read window. Both create_text_artifact and create_binary_artifact wrap make_room_for and the subsequent Artifact.objects.create() in one transaction.atomic() block, so the lock is held through eviction and the new row's insert. Added OPTIONS: {"timeout": 20} to DATABASES in settings.py as headroom for legitimate contention.

SQLite has no row-level locking, so this necessarily serializes quota-affecting writes across all users, not just the racing ones - an inherent SQLite limitation, not a design choice, and acceptable at this project's actual scale (a low-volume self-hosted tool, not a high-throughput service). Deliberately did not add retry/backoff logic for lock contention: doing so would be engineering for a load pattern (many truly simultaneous writes) this project doesn't have. Did fix a real bug found while testing the fix itself: the exception handling around the create calls only caught QuotaExceeded, so a genuine lock-timeout error left the just-written file orphaned on disk - both call sites now clean up on any exception, not just QuotaExceeded.

Verified live against the actual dev instance (real file-based SQLite, not Django's in-memory shared-cache test database) by firing 5 truly simultaneous POSTs at a scratch account with a quota tight enough for exactly one artifact, twice:

• Run 1: 1 succeeded, 4 hit a lock-timeout 500 (expected under forced exact-simultaneity - not something real usage produces). Final usage: 466/900 bytes, no orphaned files.
• Run 2: 4 succeeded, 1 got a 500; 3 of the 4 were then legitimately evicted by later-committing requests needing room - this is the documented "oldest unkept artifact evicted to make room" feature working correctly under concurrency, not a bug. Final usage: still 466/900 bytes, one artifact, no orphaned files.

In both runs the actual invariant - usage never exceeds quota - held. The original bug (measured 616 bytes over quota) does not reproduce.

An automated multi-threaded test was attempted (tests/api/test_quota_concurrency.py, later removed) but proved unreliable against Django's in-memory shared-cache test database specifically: SQLite's shared-cache mode uses a different, non-retryable locking model (SQLITE_LOCKED, not the ordinary retryable SQLITE_BUSY) than the real on-disk file this app actually uses in dev/production, so it failed for reasons unrelated to the fix's correctness. Verified live instead, matching the original issue's own reproduction method.