fix(privacy): declassify takes the write lock before it reads

[Broccolito]

privacy::declassify::declassify() opened its transaction with pool.begin() (SQLite DEFERRED) and its first statement was a read. That pins a WAL read snapshot, so the classification_audit INSERT had to upgrade to a writer — and if any other connection committed in between, SQLite refuses the upgrade with SQLITE_BUSY_SNAPSHOT, for which the busy handler is not consulted. The pool's five-second busy_timeout was never in the path. declassify_session maps every Err to INTERNAL_SERVER_ERROR, so a user whose declassification merely lost a race got a bodyless 500.

Step zero: the defect is live on main (5a404ec)

The new test concurrent_declassifications_survive_racing_writes — 120 declassifications of real private chats, barrier-synced against a second, independent store that never stops committing — on unmodified origin/main:

118 of 120 declassifications failed (a `database is locked`, code 5 or its
SQLITE_BUSY_SNAPSHOT variant 517, here means the transaction read before it wrote)

Three runs on the read-first shape lost 118, 120 and 120 of 120.

The mechanism, measured directly

Two raw SQLite connections (WAL, timeout=5.0), B committing between A's BEGIN DEFERRED/SELECT and A's UPDATE:

read-first UPDATE failed after 0.0000s: OperationalError('database is locked') sqlite_errorcode=517

517 is SQLITE_BUSY | (2<<8) — SQLITE_BUSY_SNAPSHOT — and 0.0000s is the busy handler never being asked.

The fix

The one SessionStorage::delete_session already documents: take the write lock up front. Everything declassify must do before it can write is a read — the provenance decides the grade, and the grade decides whether there is a write at all — so the lock is taken by a statement that matches no row and exists only for its lock. Verified with the exact statement shipped:

• while it is held, a second connection's BEGIN IMMEDIATE blocked for the full 2.2052s timeout → the file write lock really is taken;
• when another writer holds the lock, the statement itself waited 2.1774s and reported code 5, not 517 → the busy handler is back in the path, so contention costs a wait and never an error.

A BEGIN IMMEDIATE on an acquired connection (the ensure_privacy_schema shape) was rejected deliberately: this runs under an axum handler that is dropped when the client disconnects, and sqlx 0.8.0's pool only pings a returned connection (return_to_pool in sqlx-core-0.8.0), so a hand-rolled BEGIN would hand the pool a connection still holding the write lock. A real Transaction rolls back on drop. sqlx 0.8.0 has no Pool::begin_with.

Before / after

	read-first (main)	write-first (this PR)
concurrent_declassifications_survive_racing_writes	118, 120, 120 of 120 lost	0 lost, 8 consecutive runs
concurrent commits landing alongside	—	1139–4245 per run, 0 busy appends

Removing only the new statement and changing nothing else turns both new tests red:

120 of 120 declassifications failed ...
a racing declassification must not error: error returned from database: (code: 5) database is locked

The same-row race improves too

two_declassifications_of_one_chat_serialize_into_one_ledger_row pins the behaviour change. The loser used to hold the same snapshot showing private and surface as a 500; it now parks on the write lock and, by the time it reads, sees public and returns the tidy AlreadyPublic a sequential second call gets. One ledger row either way — the single-ledger-row invariant is now held by serialization rather than by a snapshot conflict. The ⚠ doc paragraph that described the old behaviour as designed is rewritten to say what is now true.

Deliberately not changed

• Point 2 of the suggestion — the route's Err → empty 500 mapping. With the lock taken up front a transient conflict now waits up to the five-second busy_timeout instead of failing instantly, so the class of failure that mapping obscured is gone at the source. Reworking the mapping means editing crates/biorouter-server/src/routes/session.rs, a heavily contended file this week, for a case that no longer occurs; left for a separate change.
• #[serial] coverage in biorouter-server's tests. The suggestion calls it a workaround and it is — the transaction shape was the fix, and the previously-intermittent routes::session::declassify_tests::the_route_needs_more_than_the_secret_key passes here.
• session_id_high_water and anything in fix(session): a deleted chat's side rows go with it, and its id is never minted again #264: untouched.

Gates

• cargo fmt --check -p biorouter clean; ./scripts/clippy-lint.sh → all baseline checks passed.
• cargo test -p biorouter --lib → 4047 passed, 0 failed (includes all 248 privacy:: tests and concurrent_deletes_survive_racing_writes).
• cargo test -p biorouter-server --lib declassify → 7 passed, 0 failed.
• One file touched: crates/biorouter/src/privacy/declassify.rs.

🤖 Generated with Claude Code