Repository navigation
fix(db): admit fleet sweeps per tenant instead of in-SQL fence predicate - #8318
Conversation
b331316 to
2c15121
Compare
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Thanks Luke. No blocking findings at 2c151217.
All seven former community_write_allowed() sites are still fenced. Six mutate through a community-scoped AdmittedTx. guard_community_write takes the shared deletion lock and then reads lifecycle in a separate statement, so under the writer pool's READ COMMITTED requirement it sees the same fresh post-lock state the SQL predicate did. Push enqueue already guards its transaction before the push lock and insert, so dropping the predicate there is safe. Every mutation is constrained to the admitted community_id, including both sides of the claim join. Counts and reaped channel rows only include tenants that committed. Channel follow-ups are built from those committed rows, so a failed tenant can't create follow-ups or suppress follow-ups for tenants that did commit.
The race tests hold the real exclusive community_deletion_lock_key, wait until the production sweep is blocked on it, fence the tenant in that same transaction, then commit. They would fail if admission were removed, and also if the lock were kept but the lifecycle re-read skipped. CI at this head (run 38589716836, PostgreSQL job 982/982) has named PASS lines for all 11 new admission tests, the 18 tests on the shared fixture, and enqueue_admits_active_tenant_and_rejects_quiescing_tenant. serving_table_writes_expose_syntactic_chokepoint_or_guarded_tx_routes passes in Rust Unit Tests.
Optional:
- Invite cleanup can still be blocked by one tenant.
reap_expired_relay_invitesselects the globally oldest 1,000 expired invites before grouping by community (relay_invite.rs:186-207). If one tenant owns that whole page and its delete keeps failing, later tenants never enter the loop. I couldn't find a persistent tenant-local failure for this delete in the current schema, and base aborted the whole page on any failure, so this is still an improvement. The description's "one failing tenant can't block the tenants after it" is broader than what the code does, though. Suggested wording: "A tenant error does not abort processing of other tenants already in the selected candidate page; invite cleanup keeps its oldest-first 1,000-row selection." A cursor or per-tenant cap, plus a full-page regression test, would close it fully. - Systemic errors are also treated as tenant errors. The four periodic sweeps continue on every error, including pool acquire timeouts and connection loss. During a database outage each remaining tenant waits out the 3s writer acquire timeout and logs a warning. Invite cleanup runs inside the usage-metrics leader tick (
buzz-relay/src/main.rs~2472), so a failover can stall that tick for minutes. Consider continuing only on tenant-local errors and returning on pool or IO errors. - Claim stops at a locked head (pre-existing).
claim_due_match_batch_with_loaderreturnsNoneonce an admitted tenant claims zero rows (push.rs:929-937). If every due row in the oldest active tenant is row-locked, younger tenants with due rows aren't tried on that call. Base behaved the same way, but the new loop could skip empty tenants within its 16-candidate bound. - Description. The Verification section cites older heads (
b3313167,e0229c8a, 979/979). The current-head PostgreSQL job is 982/982 and also passes both writer-pool tests listed as stalled. The "admission tests via plaincargo test" row sits after a paragraph, so it doesn't render as part of the table.buzz-review-completedisn't in the description yet. - Scope of "unblocks dropping the fence functions and triggers." No application mutation calls
community_write_allowed()anymore. However,assert_community_write_allowed()is still called when a serving-write lease is acquired (store/deletion.rs:3068). Catalog validation still requires both functions (store/deletion.rs:1195-1198), and the trigger and compatibility SQL paths also still use them. Consider narrowing that line to the boolean predicate, so the later drop change accounts for these remaining dependencies.
…k base * origin/elrond/fence-sweeps-tenant-admitted: fix(db): pin sweep admission with race tests; continue past failing tenants fix(db): admit fleet sweeps per tenant instead of in-SQL fence predicate
987ace8 to
b587512
Compare
Remove every production application call to community_write_allowed(). Fleet sweeps (relay invites, scheduled workflow fires, ephemeral channels, exhausted push matches), the due push-match claim and the startup d_tag backfill now read candidate communities, open one AdmittedTx per tenant, and run the original mutation scoped to that community. Refused tenants are skipped; batching, ordering and FOR UPDATE SKIP LOCKED are preserved. The redundant predicate in the post-commit push enqueue is deleted (the transaction is already guarded). The ephemeral channel sweep returns committed rows on a later-tenant failure so relay follow-ups still run. Triggers and SQL fence functions are unchanged; a later migration drops them (BUZZ-315 part B). Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
…enants Address review on the tenant-admitted fleet sweeps: - Add race tests for invite reap, ephemeral channel reap and push-match claim that fence a tenant while the sweep is blocked in admission, so admission (not the candidate read) is proven to authorize the write. - Drop the unbounded channel-reap candidate pre-filter; keep the LIMIT-bounded invite and claim pre-filters with a comment. - Periodic sweeps (invite reap, workflow fire prune, channel reap, exhausted match reap) warn and continue past a failing tenant; its rows stay eligible for the next tick. Removes DbError::EphemeralReapPartial and its relay arm. Claim, enqueue and startup backfill still propagate. - Unique per-call admission_test_<uuid> shadow schema; fault-injection objects live inside it. - Recognise begin_community_sweep_transaction as a tenant-local chokepoint and drop the backfill_d_tags source-policy exception. - Tighten the enqueue rejection assertion, fix a stale comment, rename a test. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
The four periodic sweeps (ephemeral channel reap, exhausted match reap, relay invite reap, workflow fire prune) continue past a failing tenant so one tenant's error cannot hold back the others. A pool timeout, closed pool, or socket error is not tenant-local, though: continuing paid one acquire timeout and one warning per remaining tenant in the page. Route all four through one sweep_admitted_communities helper that skips fenced tenants, continues past tenant-local errors, and stops at the first connection-level error (DbError::is_connection_unavailable). Rows already committed are still returned, so reaped channels keep their system events. Also name the sweep chokepoint marker in observability_source (review P2). Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
b587512 to
5d37fbd
Compare
## What This is the expand step for retiring the 32 universal `community_write_fence_*` triggers and their SQL functions (BUZZ-315). Relay pods auto-migrate at boot (`BUZZ_AUTO_MIGRATE`). The drop migration therefore runs while the previous release is still serving, and that release must not depend on the objects it drops. Today the app depends on them in four places, and this PR removes all four from its own code paths. Seven in-SQL `community_write_allowed` predicates in fleet sweeps and push (`push_enqueue.rs`, `channel.rs`, `event.rs`, `push.rs`, `relay_invite.rs`, `workflow.rs`) are #8318's scope. The release is independent of the fence functions only with both this PR and #8318. **It changes no schema.** The objects stay and keep working until the follow-up drop PR. | Old dependency | Now | |---|---| | Startup and readiness (`validate_serving_catalog_on`) require `community_write_allowed`, `assert_community_write_allowed`, `enforce_community_write_fence` and all 32 triggers | No longer required. Still required: the tenant tables, `community_deletion_lock_key` and the enabled community tombstone trigger. | | Serving-lease admission runs `SELECT assert_community_write_allowed($1)` | Uses `guard_community_write`, the same shared lock and state check as ordinary serving writes, in the same transaction as the lease `INSERT`. Fenced or missing communities still map to `AccessDenied`. | | Live scoped-table discovery calls `community_write_fence_excluded_table()` | Uses a Rust `EXCLUDED_SCOPED_TABLES` list. A unit test pins it equal to the SQL list while both exist. | | The deletion executor compares the live `fenced_tables` against the frozen inventory | Only tenant-table equality is compared. Inventories **still record** live fences, so an older executor that validates an inventory frozen by this release still matches. The digest still covers the full frozen manifest. | `lock_community_deletion_shared` now refuses transactions that are not READ COMMITTED, so every lock-then-read admission gets the check: `guard_community_write`, serving-lease finalization (`guard_transaction_with_serving_lease`), lease renewal/verification, owner preparation and owner-mutation admission. The old SQL assertion did this check for the lease path, and the trigger backed up the rest. The isolation level comes back from the existing `pg_advisory_xact_lock_shared` statement, so the hot path makes no extra round trip. The refusal comes before any lifecycle-state read. ## Mixed versions | Combination | Why it is safe | |---|---| | Previous release, this DB | No schema change, so all old checks still see the 32 fences. | | This release, DB with fence objects | Extra objects are tolerated, and the trigger backstop still applies. | | This release **plus #8318**, DB without fence objects | Serving, readiness, lease admission and deletion proven by `migration_schema_fence_objects_are_optional_for_serving_and_deletion`, which drops them inline. Without #8318, serving and deletion still work, but the push matcher, exhausted-job reaper, invite retention, ephemeral reaper and startup d_tag backfill fail with `42883 community_write_allowed(uuid) does not exist` (seen in deploy-stack E2E). | | Older executor, inventory frozen by this release | `fenced_tables` is still populated from the live catalog (32). | This PR does not depend on #8318 to merge and can ship alongside it, but it is not drop-safe alone. The drop PR (#8323) needs #8219, #8318 and this PR deployed on every pod, and every rollback image after the drop must include #8318. ## Tests - `store::deletion::postgres_tests::migration_schema_fence_objects_are_optional_for_serving_and_deletion` migrates to 0060. It then runs the following with the fence objects present, then again after dropping them inline: - serving startup - readiness - the deletion catalog check - `inventory_schema` - lease acquire/release - `AccessDenied` for quiescing, fenced, tombstoned and missing communities - `store::deletion::postgres_tests::deletion_lock_admission_rejects_pinned_snapshot_isolation` checks that REPEATABLE READ and SERIALIZABLE are refused by both `guard_community_write` and `guard_transaction_with_serving_lease`. The lease case uses a never-acquired lease, so if the isolation check is skipped it fails as stale instead. - `buzz-deletion` `postgres_tests::approved_legacy_fence_inventory_accepts_current_catalog` checks that a legacy digest with 32 fences validates against a live catalog with none, and that an unknown tenant table is still rejected. - `store::deletion::tests::scoped_table_exclusions_match_sql_during_transition` checks exclusion-list parity. - `store::deletion::postgres_tests::serving_lease_acquire_waits_for_fence_lock_then_refuses` holds the exclusive deletion lock and checks that lease acquisition stays pending. It then commits `fenced` and requires `AccessDenied` with no lease row persisted. Removing the lease path's `guard_community_write` call fails this test, while the other four lease/admission tests above still pass. - `buzz-deletion` `serving_guard_heartbeats_long_operation_through_quiescing` now requires `AccessDenied` for a lease acquired during quiescing, rather than any error. Each test was confirmed to fail when its fix is removed (rc 100, 0/1): the isolation refusal, the three validators restored to the base version, the fence-list comparison restored, and one name dropped from the Rust list. ## What got simpler The lease path loses its special SQL assertion and its error-code translation and reuses the ordinary app guard. The serving and deletion catalog checks each lose one requirement. No new abstraction. The `fenced_tables` reader stays only for mixed-version executors; the drop PR deletes it. **Identified debt (pre-existing, outside this diff):** `runtime::postgres_tests::writer_pool_rejects_non_read_committed_database_default` hangs on clean `177551dda` too. ## Verification (Blox, tree identical to this commit) | Gate | Result | |---|---| | `cargo fmt --all -- --check` | pass | | `cargo clippy --workspace --all-targets -- -D warnings` | pass | | `cargo nextest run -p buzz-db --lib` | 167/167 | | `scripts/test-postgres-test-discovery.sh` | pass | | `scripts/postgres-test-run.sh -p` buzz-admin / audit / deletion / push-gateway / relay / search / workflow | all rc 0 (4, 7, 12, 11, 402, 20, 2) | | `scripts/postgres-test-run.sh -p buzz-db` | 510 pass, 1 fail: the pre-existing hang above, interrupted | | Round 2 (this head): `-p buzz-db` / `-p buzz-deletion` | 511 pass + the same pre-existing hang interrupted / 12/12 | The new PG tests are selected by the `postgres-ci` filter `(test(/postgres_tests::/) or binary(/^postgres_/)) and not test(/(^|::)external_infra[^:]*::/)`. buzz-review-completed --------- Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
…i-bindings * origin/main: fix(db): validate channel roster snapshots in the app writer (#8316) fix(mobile): support DeviceHub in mobile-dev (#8404) fix(db): admit fleet sweeps per tenant instead of in-SQL fence predicate (#8318) buzz-db: tolerate absent universal write-fence objects (expand) (#8322) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…ip-fi-resolver * duncan/bbpublic-nip-fi-bindings: fix(db): serialize NIP-FI backfill with operator binds fix(db): validate channel roster snapshots in the app writer (#8316) fix(mobile): support DeviceHub in mobile-dev (#8404) fix(db): admit fleet sweeps per tenant instead of in-SQL fence predicate (#8318) buzz-db: tolerate absent universal write-fence objects (expand) (#8322) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
What
Removes every production call to the SQL fence predicate
community_write_allowed()from application SQL, so no application mutation depends on the boolean predicate anymore. This is one prerequisite for BUZZ-315 part B; #8322 (tolerating absent fence objects) is a separate one. The triggers and SQL functions stay in this PR because mixed-version pods still depend on them.The predicate isn't inlined. It's a VOLATILE plpgsql function that takes the deletion lock and then reads lifecycle state on a fresh snapshot, and an inline expression would read state from the statement's start snapshot. A separate earlier check would race deletion. So each sweep now:
AdmittedTxper community (begin_community_sweep_transaction, a thin wrapper overbegin_community_write_transactionthat turns an admission refusal into "skip this tenant"; the four periodic sweeps go throughsweep_admitted_communities, which also owns their error handling),community_id = $1and commits.runtime/push_enqueue.rsguard_transaction.store/relay_invite.rsreap_expired_relay_invitesstore/workflow.rsprune_scheduled_workflow_fires_beforestore/channel.rsreap_expired_ephemeral_channelsLIMIT, so admission alone decides).store/event.rsbackfill_d_tagsstore/push.rsclaim_due_match_batch_with_loaderFOR UPDATE OF q SKIP LOCKEDand batch limit. It skips at most 16 tenants that fence between the read and admission.store/push.rsreap_exhausted_matchesTenant failures. The four periodic sweeps (invite reap, workflow fire prune, channel reap, exhausted match reap) log a warning and continue when one tenant fails. That tenant's rows are left unmodified and stay eligible, so the next tick retries them. A tenant error doesn't abort processing of the other tenants already in the selected candidate page. Invite cleanup keeps its oldest-first 1,000-row selection, so if one tenant owns that entire page and its delete keeps failing, tenants outside the page wait. No persistent tenant-local failure for that delete is known in the current schema, and base aborted the whole page on any error. The returned count or rows cover the tenants that committed. Claim, enqueue and the startup backfill still return the error to the caller.
Database outages. A connection-level error (pool acquire timeout, closed pool, socket I/O;
DbError::is_connection_unavailable) isn't tenant-local, so it stops the periodic sweep at that tenant instead of moving on. Without this, an outage that began after the candidate read cost one writer acquire timeout and one warning per remaining tenant, and invite cleanup runs inside the usage-metrics leader tick. Rows already committed are still returned, so reaped channels keep their follow-ups. Remaining tenants retry next tick.Known limit (pre-existing, tracked separately):
claim_due_match_batch_with_loaderreturnsNoneonce an admitted tenant claims zero rows. If every due row in the oldest active tenant is row-locked, younger tenants aren't tried on that call. Base behaved the same way. This PR doesn't change it.Tests
All tests use a one-session fixture (
test_support::admission_fixture) withsession_replication_role = replica, so the trigger backstop is off and only application admission protects the fenced tenants. The fixture shadowscommunity_write_allowedwith a function that raises, in a per-calladmission_test_<uuid>schema.channel_reap_returns_committed_rows_after_earlier_tenant_errorandperiodic_reaps_continue_after_tenant_error_and_retry_retained_rowsinject a per-tenant failure. They check that later tenants commit, that the failed tenant's rows are retained, and that the next call processes them.fleet_sweep_stops_at_first_unavailable_connectionruns the sweep helper against a closed pool with three candidates and asserts only the first is attempted.fleet_sweep_returns_committed_results_when_database_drops_mid_page(PostgreSQL) commits the first tenant, closes the pool, and asserts the first tenant's result is returned and the sweep stops at the second of three candidates.connection_unavailable_covers_only_pool_and_io_failurespins the classification; the per-tenant failure tests above confirm a database-raised error still continues.Falsification, each with the tests failing as expected:
legacy SQL fence predicate called)guard_community_writereturnsOk(())after taking the lockcontinues instead of stopping on a connection-level errorfleet_sweep_stops_at_first_unavailable_connectionandfleet_sweep_returns_committed_results_when_database_drops_mid_pagefailreturn Vec::new())fleet_sweep_returns_committed_results_when_database_drops_mid_pagefailsserving_table_writes_expose_syntactic_chokepoint_or_guarded_tx_routesfailsThe PostgreSQL tests are under
postgres_testsmodules with#[ignore = "requires Postgres"], which puts them in the.config/nextest.tomlPostgreSQL filter. CI evidence is under Verification.Verification
CI at head
5d37fbda(run 38925996275) is green. The PostgreSQL job (job 117062895683) passed 1017/1017. That includes named PASS lines for all 30admission_postgres_tests,fleet_sweep_returns_committed_results_when_database_drops_mid_page, and both writer-pool tests listed below. Unit Tests (job 117058136482) has PASS lines forconnection_unavailable_covers_only_pool_and_io_failuresandfleet_sweep_stops_at_first_unavailable_connection.Local gates ran on a Linux cloud workstation against private PostgreSQL 16 and Redis. The tree was
735170df, which is this head rebased onmainc0ab789, so the gates built the merge result.cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo nextest run -p buzz-db --lib --testsscripts/test-postgres-test-discovery.shgroup_state_postgres_testsfail withPoolTimedOuton unmodifiedmainc0ab789too (see below)continue×2,return Vec::new())These failures happen only on the workstation, and each one passes in CI's PostgreSQL job at this head:
writer_pool_metrics_stop_after_session_timeout_setup_failurestalls. It's an unchanged writer-pool configuration test, and it stalled the same way at earlier heads.group_state_postgres_testsfail at the same point,ensure community: Sqlx(PoolTimedOut), on unmodifiedmainc0ab789. This PR doesn't touchbuzz-relay.What got simpler
community_write_allowed()anymore. Dropping the fence objects still has other dependencies: serving-write lease acquisition callsassert_community_write_allowed()(store/deletion.rs:3068), catalog validation requires both functions (store/deletion.rs:1195-1198), and the triggers and compatibility SQL still use them. Those belong to buzz-db: tolerate absent universal write-fence objects (expand) #8322 and part B, not this PR.targetCTE is gone, and every sweep uses the same admission-and-skip helper.sweep_admitted_communities) for admission, skip, continue-on-tenant-error and stop-on-outage, instead of four copies of it.backfill_d_tagsno longer needs a reviewed exception.