Extends the atomic_write suite with three write-path gaps:
- an app cannot exceed PICLOUD_APP_DOCS_MAX_ROWS (create-only, update
net-zero, delete frees a slot) via a real PostgresDocsWriter;
- concurrent writers cannot push a group past its docs total-bytes quota
(16 racers against a 450-byte ceiling under pg_advisory_xact_lock);
- a files create with a broken outbox leaves no row AND no blob on disk,
while a files delete with a broken outbox keeps both the row and the
readable blob (the disk-ordering invariant).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Turning CI on surfaced `atomic_write` as intermittently failing under load —
self-inflicted. Its fault injection is `CREATE TRIGGER ... ON outbox`, which takes
an ACCESS EXCLUSIVE lock on a table every other suite is concurrently inserting
into. The header even claimed it "cannot affect any test running in parallel";
that was wrong — scoping the trigger by app_id bounds which ROWS it rejects, not
the table lock installing it takes. Shipping that alongside "run the DB tests in
CI" would have poisoned the signal.
The fix is the one already used for the e2e suites: a private database per test.
That logic (build a migrated template once, clone it per test via
`CREATE DATABASE ... TEMPLATE`) was duplicated in `picloud/tests/common`, and this
would have been a third copy — so it moves into a shared `picloud-test-support`
dev-dependency crate, and `picloud/tests/common` now re-exports it. The invariant
it encodes: manager-core's claim loops are global by design, so a test must not
share a database with anything that runs them OR with anything that does DDL.
Moved onto it:
- `atomic_write` — the DDL fault injection above; its per-test cleanup is now
deleted (the database is thrown away).
- `outbox_reclaim` — asserts `reclaim_stale_claims` returns exactly 0, a count
over the WHOLE outbox table. On the shared DB any stale row from any other suite
(or a killed picloud that died holding a claim) would fail it permanently, until
someone cleared the table by hand.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Self-review of this branch caught a data-loss bug I introduced with the
per-app / group files ceilings: the quota check ran AFTER the blob write.
`write_atomic_at` renames the new bytes over the FINAL path, so by the time
the ceiling refused an update the previous bytes were already gone. The
per-app path then unlinked the blob (row survives, file destroyed — every
read 404s); the group path left the new bytes in place under the old row's
checksum (every read fails `Corrupted`). Either way a user permanently lost
a file merely by exceeding a quota — a strictly worse outcome than the
unchecked-update bypass the ceiling was added to close.
The check now precedes the write on create AND update, per-app and group. As
a bonus this stops an over-quota caller driving unbounded write+unlink disk
churn: the ceiling now bounds I/O, not just stored bytes. The blob still goes
down inside the transaction, under the advisory lock, so a rollback unlinks
it and nothing can reference it in between.
The original test passed against the bug: it asserted the refused update did
not change the stored byte TOTAL — true, while the blob was already
destroyed. The regression test now reads the file back through the
checksum-verifying `FsFilesRepo::get`, and was confirmed to fail with
`Corrupted` against the old ordering.
Also in the KV writer (same file): drop the redundant pre-read on the hottest
write path. `set`/`set_if` did a SELECT purely to learn whether the write
would add a row, when the upsert already returns the previous value. Check
after the insert instead (`>` not `>=`) and let the transaction roll back on
refusal — identical outcome, one round-trip fewer, and an update pays nothing
at all. `kv_repo::get_on` and `FsFilesRepo::final_path` are now unused and
deleted; `FilesRepo::delete` delegates to the `delete_meta_on` + `unlink_blob`
helpers rather than re-implementing them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An app's OWN store had no ceiling of any kind — only the group-SHARED
collections did, which is the inversion of what you'd expect. And
`authz::script_gate` returns `Ok(())` when there is no principal, so a
PUBLIC UNAUTHENTICATED route could reach `files::collection(..).create(..)`
and write blobs in a loop until the disk filled. `PICLOUD_FILES_MAX_FILE_SIZE_BYTES`
caps ONE file at 100 MB; nothing capped the count. Same for KV keys and docs
against Postgres.
Ceilings: `PICLOUD_APP_KV_MAX_ROWS` / `PICLOUD_APP_DOCS_MAX_ROWS` (100k) and
`PICLOUD_APP_FILES_MAX_TOTAL_BYTES` (10 GiB).
They are enforced DIFFERENTLY from the group ones, on purpose — porting the
group design as-is would have been a serious regression:
* KV/docs check a ROW count, on INSERT only, with NO lock. `kv::set` is the
hottest write path in the system; taking a per-app advisory lock on every
set would serialize an app's entire data plane, and a `SUM(...)` scan would
make write cost grow with the app's stored size. What the lock buys is
small — unlocked overshoot is bounded by write concurrency (32) against a
ceiling of 100k, ~0.03%. These are anti-DoS rails, not billing. An update
adds no row and is bounded by the per-value cap, so it pays nothing at all.
* No per-app BYTE ceiling for KV/docs: every value is already capped at
`PICLOUD_KV_MAX_VALUE_BYTES`, so `max_rows x max_value_bytes` is ALREADY a
finite bound. A second scan would buy nothing.
* FILES are the exception and DO lock + sum: one blob may be 100 MB, so 32
racing uploads could overshoot by gigabytes of real disk, and a file write
is heavy enough that the lock and the SUM are lost in the noise. Checked on
create AND update — an update that skipped the ceiling would be a free
bypass (the same bug just fixed for group files: grow a 1-byte file to
100 MB, repeat).
`group_quota` is renamed `quota`: it now serves both owners, and the module
doc is where the two enforcement strategies are contrasted.
Pinned by tests/atomic_write.rs: the key ceiling refuses a new key while
still allowing an update at the ceiling (rejecting updates would brick an app
the moment it filled up — worse than the DoS the rail exists to stop), and 10
concurrent uploads cannot exceed the disk ceiling, nor can an update grow past it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Audit #6 and #8 for the last of the three stores, plus a bypass found on
the way.
**#6.** create/update/delete wrote the metadata row, then emitted
best-effort, so an outbox failure left a committed file whose trigger never
fired. `atomic_write::FilesWriter` commits the metadata row and the fan-out
together.
Files are the one store where the ordering is subtle, because the BYTES live
on disk and cannot join a transaction:
* create/update — blob first, then commit metadata + fan-out. A rollback
unlinks the blob. (A crash at that exact point still orphans it; that
hazard predates this change — the repo already wrote the blob and then
inserted the row in a separate, failable statement — and the orphan is
inert, referenced by nothing.)
* delete — commit the metadata removal + fan-out FIRST, then unlink. The
reverse order would destroy the bytes of a row that a rollback keeps,
leaving a file that can never be read.
**#8.** `GroupFilesService::create` read `total_bytes` on one connection and
wrote on another, so concurrent uploads each saw the same pre-write total and
together overshot the ceiling. This is the worst instance of the race in the
codebase: the ceiling is DISK (10 GiB by default) and one file may be 100 MB,
so a racing fleet overshoots by gigabytes. `PostgresGroupFilesWriter` takes
the per-group advisory lock (on its own `files` key) across the check and the
write.
**The bypass.** `GroupFilesService::update` checked NO quota at all — so a
1-byte file could be updated to a 100 MB one without the ceiling ever being
consulted, repeatedly, for unbounded disk. It now checks the projected total
(the replaced file's bytes subtracted in SQL, so a same-size-or-smaller
update near the cap still goes through).
Also drive-by: `queue_e2e` asserted the ack the instant the marker appeared,
but the marker is written DURING the handler and the ack happens after it
returns — a zero-tolerance race. It polls now. (This does not fix the
suite's flakiness, which reproduces on the pre-pass commit too.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Audit #6 and #8, applied to the docs store — the same two bugs KV had, in
the same two places.
Per-app docs (#6): create/update/delete wrote the row, then emitted
best-effort. An outbox failure left a committed doc whose trigger never
fired. They now go through `atomic_write::DocsWriter`, whose Postgres impl
writes and fans out on one connection in one transaction.
Group docs (#8): the service read `count_rows`/`projected_total_bytes` on
pooled connections and wrote on another, so concurrent creators each saw the
same pre-write count and together overshot the ceiling.
`PostgresGroupDocsWriter` takes a per-group advisory lock — on its OWN
`docs`-namespaced key, so it serializes against other docs writers to that
group but not against KV writers, which draw on a separate ceiling.
The bespoke `check_total_bytes` (with its own copy of the upper-bound fast
path) is gone; group docs now shares `group_quota::check_group_write` with
group KV, so the row ceiling, the projected-bytes ceiling, and the fast path
that skips the O(n) SUM scan have exactly one implementation between them.
tests/atomic_write.rs gains the docs row-quota race (16 concurrent creators
against a ceiling of 4).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Audit #8. `GroupKvServiceImpl` read the group's usage (`count_rows` /
`projected_total_bytes`) on one pooled connection and wrote on another. N
concurrent writers therefore each observed the same pre-write total, each
concluded it had room, and together sailed past the ceiling. The code
called this out as "best-effort ... quotas are safety rails, not exact
accounting" — but the overshoot is not small. With a ceiling of 5 and 20
concurrent writers, 19 rows land.
Route group-KV mutations through `atomic_write::GroupKvWriter`, whose
Postgres impl runs the quota reads, the row write, and the shared-trigger
fan-out on ONE connection in ONE transaction — and, crucially, takes a
per-group `pg_advisory_xact_lock` first.
The lock is the fix, not the transaction. Putting the check inside the
writing tx is necessary but NOT sufficient: under READ COMMITTED each
transaction's `COUNT(*)`/`SUM(...)` still sees a snapshot without the other
writers' uncommitted rows, so they all still pass. Serializing the
check-then-write per group is what makes a writer see its predecessor's row.
The lock key is namespaced per kind (kv/docs/…) so writes drawing on
different ceilings don't contend. A delete takes no lock — it only frees
space.
The quota POLICY (row ceiling, projected-bytes ceiling, and the
upper-bound fast path that skips the O(n) SUM scan for a group nowhere near
its cap) moves to one place, `group_quota::check_group_write`, over a small
`GroupUsage` trait. Both backends — the transactional one reading through
`&mut *tx`, and the in-memory one the unit tests use — share it, so there is
exactly one copy of the decision.
`set_if` gains a correctness nicety on the way: the row ceiling now keys on
whether the write ADDS a row, so a compare-and-swap against an absent key
with a `Some` precondition (which cannot swap, and so consumes nothing) is
no longer charged for one.
tests/atomic_write.rs pins both ceilings against 20/16 concurrent writers.
Both tests fail without the lock (19 rows stored against a ceiling of 5).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Audit #6. `KvServiceImpl` wrote the row, waited for it to commit, then
asked the emitter to resolve matching triggers and insert outbox rows —
a second transaction on a second connection. If that second step failed,
the row was permanently in the store with its trigger having never
fired: invisible to the caller (the write "succeeded"), unrecoverable by
any retry, and only ever logged. The code said as much:
// Audit finding (Medium): this is non-transactional with the data
// write — the row above has already committed by the time emit()
// runs, so an emit failure means triggers silently don't fire.
Introduce `atomic_write::KvWriter` — the mutating half of the service (the
write AND the fan-out it produces), so the two can share a transaction:
* `PostgresKvWriter` opens a tx, writes via `kv_repo::*_on(&mut *tx, …)`,
runs the fan-out on the SAME connection via `emit_on(&mut *tx, …)`, and
commits. An emit failure now rolls the write back and surfaces to the
script, which is the honest outcome — the caller learns the write did
not happen instead of silently getting a store that disagrees with its
triggers.
* `BestEffortKvWriter` keeps the old write-then-log-on-emit-failure
semantics for the in-memory unit tests (no Postgres).
Both sit behind one trait, so the service body has a single code path and
the choice is a constructor detail (`with_atomic_writes(pool)` in the host).
Pool-deadlock rule, documented on the module: everything inside the tx runs
on the tx's connection. A writer that held a tx and then reached for a
second pooled connection could starve — the pool is sized to the execution
concurrency cap, so N executions each wanting 2 connections deadlock. The
fan-out takes `&mut *tx` and never touches a repo.
`tests/atomic_write.rs` pins it by injecting an outbox failure (a Postgres
BEFORE-INSERT trigger scoped to the test's own app_id, so parallel tests are
unaffected): the set errors and the key is NOT in the store; likewise a
delete rolls back rather than dropping a key nothing downstream hears about.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
