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

# 010 — Concurrent `POST`s 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:

```python
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 `POST`s 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.