mirror of
https://github.com/block/buzz.git
synced 2026-11-06 17:31:19 +00:00
4c7c15a9c2
## Summary Partition maintenance runs one transaction per table in a pass. When a table's attempt failed on the server, the transaction was dropped as soon as the error arrived, which can be before the backend has aborted and released its schema-scoped advisory lock. The next table could then find the lock held and report `skipped_locked`, which is the `parent_contention_times_out_without_blocking_the_other_table` flake and a mis-reported outcome in production. `apply_change` now owns the transaction and commits or rolls it back before it returns, so the next table starts only after the previous one's locks are free. ### fix(db): finish each table's maintenance transaction before the next starts #### Problem `store::partition::maintenance::tests::postgres_tests::parent_contention_times_out_without_blocking_the_other_table` failed once on main at maintenance_tests.rs:480 (`assert_advanced`). `events` reported `lock_timeout` as expected, but `delivery_log` was left on its old catch-all with no error. - main: https://github.com/block/buzz/actions/runs/41669713108/job/126035790322 Production has the same race. When one table's attempt fails on the server (lock_timeout, statement_timeout, deadlock), the next table in the same pass can report `skipped_locked` and skip its DDL. The next interval repairs it, but that pass reports the wrong outcome and metric. #### Cause `delivery_log` can only stay un-advanced without an error through `skipped_locked`: its `pg_try_advisory_xact_lock` returned false. The key is per schema, so the holder was `events`'s own failed transaction. PostgreSQL sends a statement's ErrorResponse (`EmitErrorReport` flushes it) before the backend calls `AbortCurrentTransaction`, which releases its locks (postgres.c, PG 16 and 17). sqlx returns the error as soon as the ErrorResponse arrives. `apply_change`'s `?` then dropped the transaction without waiting for that backend again, and `maintain_audited` started `delivery_log` on another pooled connection. The abort normally finishes within microseconds. If the `events` backend is descheduled in that gap, `delivery_log`'s probe finds the lock still held. The first suspect was that a dropped transaction only queues its ROLLBACK, which the pool's return task flushes later. That does not explain this failure. A backend left "idle in transaction (aborted)" already holds no locks, and keeping the queued ROLLBACK unflushed for 500 ms did not reproduce it. It does apply to client-side errors, where the transaction is still live on the server. #### Fix `apply_change` now owns the transaction. It begins it, runs the attempt in a new `attempt_change`, commits a `Committed` result and rolls back every other result, errors included. The early-return arms no longer roll back by hand. ROLLBACK first drains the failed statement's ReadyForQuery, which the backend sends only after its abort, so `apply_change` returns only once the lock is free. If the rollback fails, it is logged and the original error is returned. The test is unchanged. #### Validation Local Linux workstation, PostgreSQL 17.11 (docker compose): - Deterministic: gdb stopped the `events` backend at `AbortCurrentTransaction`, after its error reached the client, for 1 s. Without the fix: 6/6 failed at :480 with `delivery_log` reporting `skipped_locked`, the CI signature. With the fix: 6/6 passed. - Natural contention: 24 parallel loops of the test, with Postgres pinned to 2 CPUs shared with 4 busy loops. Without the fix: 6/1440 failed, all at :480 with `delivery_log` `skipped_locked`. With the fix: 0/1440. - `cargo test -p buzz-db --lib store::partition -- --include-ignored --test-threads=1`: 77/77 passed. Run in parallel against one shared local database, another test's database-wide event trigger also fails `cluster_global_parent_readers_follow_monthly_creation_and_catch_all_advancement` on main. CI gives each test its own database. - The test alone: 30/30 passed. `cargo test -p buzz-db --lib`, `cargo clippy -p buzz-db --all-targets -- -D warnings` and `cargo fmt --check` are clean. No regression test was added. Only a debugger can hold the window open, and the only remaining client-side error that leaves the transaction live is a concurrent-drop audit error, itself timing-dependent. **Rebased onto origin/main `64d12e297` (this branch):** Linux workstation, private `postgres:16` container with the desired-state schema applied as in the PostgreSQL lane (pgschema plus the reconcile script). - `cargo test -p buzz-db`: 191 lib, 21 `observability_source` and 5 doc tests passed. The 673 Postgres-backed tests are `#[ignore]`d there. - `cargo test -p buzz-db --lib store::partition -- --ignored --test-threads=1`: 56/56 Postgres-backed partition tests passed. The other 21 of the 77 counted above aren't Postgres-backed and ran with the lib tests. - `parent_contention_times_out_without_blocking_the_other_table` alone: 20/20 passed. - `cargo clippy -p buzz-db --all-targets -- -D warnings`, `cargo fmt -p buzz-db -- --check` and `git diff --check` are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: mtoohey <mtoohey@squareup.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>