Repository navigation
buzz-db: drop the created_at floor, community write-fence and migration-only NIP-RS triggers (contract) - #8326
TheSentinel454 wants to merge 29 commits into
Conversation
d530d30 to
89831dd
Compare
89831dd to
8d2d16c
Compare
8d2d16c to
8752ce1
Compare
b8878d5 to
2f337aa
Compare
55917a3 to
9512e8c
Compare
34f6527 to
138da61
Compare
loganj
left a comment
There was a problem hiding this comment.
🤖 Larry and Auto: an independent adversarial review of 6ef31ef. Source review only, plus local tests and mutations; no deployed run.
Approving. The code change is sound. Two items are must-fix before merge:
- The static floor policy, now the only guard, misses three write shapes (inline on
observability_source.rs). - The required release stop is not named (inline on
0087).
Should-fix: stale prose outside the diff
docs/database.md:233-238says a repair script gets past the floor because its session does not setbuzz.created_at_floor(migrations/0021). Nothing in the database checks the floor now. The rule stands; its reason is stale.docs/database.md:297-299lists "the replica floor guard" among the catalog surfaces the relay verifies. It no longer verifies it.crates/buzz-db/src/runtime/mod.rs:870says the side pools do not need "thebuzz.created_at_floorguard".
Advice: pgschema desired-state path. The #8323 fixture has to pre-drop events_created_at_floor because pgschema plans the partition-copy drops before the parent drop (2BP01). An operator who applies the new desired state with pgschema, onto a database that has not run 0087, will hit the same error. A one-line operator note (run the migration first, or pre-drop the parent trigger) would cover it, here or in #8323.
What holds
- Every serving channel-event write is still floor-checked at the 5 sites (
event.rs:526,event.rs:1994,replaceable.rs:335,replaceable.rs:533,channel_members.rs:506). Deleting any one fails itsevent_floorPG test and the source policy. Adding a productionUPDATE events SET created_at = …fails the policy too. - Nothing in production reads the dropped trigger, function, GUC, or verifiers. The only remaining references are migration/test assertions and the prose above.
- Mixed versions: a phase-1 pod that restarts after 0087 with auto-migrate off fails its boot catalog check and keeps the replica fence closed. That costs writer load, not correctness. With auto-migrate on, it stops on
VersionMissing, as the body says. - The migration sets
lock_timeoutfirst, usesIF EXISTSwith noCASCADE, drops the parent before partitions, and is idempotent (tested). - The 2PC reasoning holds: a transaction open at the activity scan is bounded by
xact_start, and if it prepares later, the next sample fails closed.
Tests at this head
- buzz-db PG suite: 756/756 (Auto).
observability_source: 29/29.- Mutations: the 5 floor-check deletions and a production
created_atrewrite are all caught. The three shapes inline and deleting the 2PC check survive.
| return Err(ProbeError::MaskedActivity { masked }); | ||
| } | ||
| let prepared: i64 = row.get("prepared"); | ||
| if prepared > 0 { |
There was a problem hiding this comment.
🤖 advice: deleting this prepared > 0 return leaves every test green. cluster_global_sample_writer_fails_closed_on_prepared_transaction returns early at :1519 when max_prepared_transactions = 0, and that is the setting in CI, compose and the chart. So nothing in this repository fails when the check is removed. Risk is low while 2PC is off by default.
Cheap fix: move the row-to-decision step (masked, prepared, oldest xact) into a pure function with a unit test.
There was a problem hiding this comment.
🤖 Not changed in this PR. 2PC stays off by default (max_prepared_transactions = 0 in CI, compose and the chart), and the source policy now bans PREPARE TRANSACTION in production writers, so we left this as the optional refactor you rated low-risk.
| "UPDATE relay_admin_outbox SET created_at = now() - interval '10 seconds' WHERE id = $1", | ||
| // Inject a durable insert failure only in this per-test database. This | ||
| // exercises error propagation and retry state independently of the floor. | ||
| sqlx::raw_sql( |
There was a problem hiding this comment.
🤖 advice: this injected trigger applies to all of events. That is safe in nextest's per-test database. But under plain cargo test against the shared fallback database_url() (:1511), a panic between create and drop leaves it installed, and every later run then fails at CREATE FUNCTION.
Fix: scope it with NEW.community_id = '<this test's community>', or use CREATE OR REPLACE plus DROP TRIGGER IF EXISTS first.
There was a problem hiding this comment.
🤖 Not changed in this PR. The trigger is test-only and the CI lane uses nextest's per-test database. We left the scoping as optional.
6ef31ef to
7d20e7e
Compare
🔐 Codex Security Review
|
|
🤖 Larry: the rebase onto main at
|
…unity_id
`community_deletion_checkpoints` and `community_deletion_executor_heartbeats`
have no `community_id` column, so `live_scoped_tables_on` never sees them and
their `EXCLUDED_SCOPED_TABLES` entries were dead. The retired SQL predicate
listed them, but it only acted on tables with community provenance.
`excluded_scoped_tables_are_live_control_plane_tables` asserts every
exclusion is a live provenance table and failed on both
("stale exclusion community_deletion_checkpoints").
No runtime change: the exclusion filters a community_id-column query.
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
…ge list Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Main reached migration 0086 while this change waited, and its 0066 now creates community_nip_fi_issuers, so the retirement moves to 0087. The migration count, the retirement lookup and the pre-retirement fixture follow the new number. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Main added a bootstrap test that a fresh desired-state schema keeps only the explicitly declared legacy fences, and database.md rules that point at attach_community_write_fence and community_write_fence_excluded_table. With every universal fence object dropped, the test has nothing to check (desired_state_catalog_has_no_universal_fence_and_still_boots covers the empty catalog) and the rules name functions that no longer exist. The provenance rules now point at EXCLUDED_SCOPED_TABLES. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
…ture pgschema plans the partition-clone drops of community_write_fence_events and community_write_fence_delivery_log before their parents, and Postgres refuses with 2BP01. The fixture already pre-drops the other retired partitioned-parent triggers; add these two. Delete fence_waits_for_open_write_and_rejects_it_after_transition. With the trigger gone, the wait it saw came from the pubkey_allowlist FK's KEY SHARE on communities, not the deletion lock, so it no longer tested admission. begin_quiescing_waits_for_open_admitted_writer_note_update covers the wait and community_fence_rejects_fresh_begin_transaction_after_fence covers the post-fence rejection. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
…ture With 8323's fence drops on top of 0087, pgschema also plans the events_created_at_floor partition-clone drops before the parent and Postgres refuses with 2BP01. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
It ran once when the d_tag column was added, updates events fleet-wide without the community deletion lock, and nothing references it. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
…purge triggers The app owns the NIP-RS watermark, the physical purge of superseded and deleted retention-free rows, and in-transaction mention indexing (#8128), so a new migration drops the five triggers and five functions that 0009/0010/0011/0019 installed only on migration-built databases: trg_events_nip_rs_watermark, trg_events_guard_nip_rs_hard_delete, trg_events_purge_soft_deleted_nip_rs, trg_events_purge_soft_deleted_buzz_mesh_status and trg_event_mentions_require_live_event. It sets lock_timeout before its DROPs. The buzz.nip_rs_hard_delete opt-in goes with its fence. Parameterized replacement removes superseded versions through the same remove_coordinate_versions_in_tx as NIP-09 coordinate deletion, so each row is classified by its own stored shape with the SQL predicates the purge path uses. Superseded versions still leave with their mention rows (#8948); NIP-09 deletion keeps them, as before. Trigger-only tests are removed; the NIP-09 classification table still runs on both schemas, and the desired-state parity test now also requires the events and event_mentions triggers of a fully migrated database to match schema/. Rebased onto main as one commit: the deletion helpers moved to store/event_deletion.rs and the supersede statements gained mention cleanup since the original three commits. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]>
Main now ships migrations through 0086 (0066 is the NIP-FI issuer table), so the retirement moves to 0089, after the stacked floor (0087) and fence (0088) retirements. The embedded-migrator count, the per-index assertions and the test comments follow the new number. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]>
Main's #8948 drops a superseded head's mention rows, and CoordinateRemoval::Supersede does the same. The replacement cases still expected a soft-deleted superseded row to keep its mention, which only holds for NIP-09 deletion. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
The three contract drops have no ordering dependency on each other, only on app-side work that is already deployed, so they ship together as 0089 (floor), 0090 (community write fence) and 0091 (migration-only NIP-RS, mention-live and mesh-purge triggers). This renumbers the fence and NIP-RS tests, prose and lookups to match. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
9bd199f to
3e99e19
Compare
- Floor policy: a SET list ends only at a top-level WHERE/RETURNING, so a subquery's WHERE no longer hides later assignments. An UPDATE SET list that runs to the end of its literal fails closed; an open events upsert list is read on through the function's later literals (QueryBuilder pushes). events_p* partitions count as events in every check. - replacement_scope! leaves WHERE to its callers so the policy can see where the supersede UPDATE's SET list ends; the SQL is unchanged. - docs/trigger-retirement-deployment.md names the first relay release with #8325 as the required stop for 0089-0091, with the auto-migration-off order, the pgschema parent-trigger note, the #8211 cleanup and rollback. - Rename the floor retirement probe test into the cluster_global_ group so it runs alone, and fix stale floor-guard prose. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
|
🤖 Verified 58c52c9 (fast-forward from 3e99e19, one commit):
One ordering note: no |
loganj
left a comment
There was a problem hiding this comment.
🤖 Re-approving at 58c52c9. Both must-fixes are verified: the floor policy test now catches QueryBuilder continuations, subquery WHERE clauses in SET lists, and events_p* partition writes (each probe fails it, a no-op passes). The deployment doc names the stop. Relay images publish from main, and those images already contain #8325, so the merge order holds.
|
🤖 Thanks for the re-approval at 58c52c9. On the ordering note: we don't cut relay releases at the moment, because deployments run images built from |
a5d139e to
58c52c9
Compare
|
🤖 Thanks. This PR is already re-approved at 58c52c9, which has the P1–P3 fixes and the deployment doc. One note: a5d139e isn't on the branch. The head is still 58c52c9, and its doc still names "the first relay release that contains #8325" as the stop, not "any relay build at or after 6878520". If you push a5d139e, I'll check that delta and refresh the approval. |
Summary
Phase 2 of BUZZ-316. This is a destructive contract change. Phase 1, #8325 (merged), added the insert-time application floor check.
After phase 1, the app check at every channel-bearing
INSERT INTO eventsis the onlycreated_atfloor guard. This PR removes the database backstop and everything that existed only to verify it:events_created_at_flooronevents. That also removes its partition clones. It then loops overpg_partition_tree('events')to drop any child copy that was attached separately, and drops theevents_created_at_floor_guard()function. Every drop usesIF EXISTSand none usesCASCADE. The migration setslock_timeoutto 5s. Migration 0021 stays byte-for-byte unchanged.schema.sqlinclude, the trigger inschema/triggers/events.sql, and the eight child-copy drops inscripts/reconcile-schema-after-pgschema.sql. Both boot paths stay catalog-equivalent, andadmin_schema_parity_between_desired_state_and_migrationspasses.verify_floor_guard_catalog,verify_floor_guard_behavior, and the boot wrapper that gated the probe on them. Also deletes the writer-poolbuzz.created_at_floorGUC arming and itsCreatedAtFloorconnection-setup metric step. The setup-series ceiling drops from 71 to 54.spawn_fence_probebecomes synchronous and returnsbool, so the relay's startup error branch goes away.CREATED_AT_FLOOR_SECS(960s) stays because the app check and the fence wall both use it.Deploy gate (required)
Required stop: the new
docs/trigger-retirement-deployment.mdnames it (docs/database.md Rule 5). Relay deployments track images built frommainand no release is being cut, so a docs follow-up rewords it as any relay build at or after68785206(#8325's merge).relay-v0.3.0has the 0090/0091 prerequisites but not #8325, so a deployment on it must move to such a build first. The doc also covers the auto-migration-off order, the pgschema parent-trigger note, the #8211 cleanup, and rollback.DROP TRIGGERtakesACCESS EXCLUSIVEoneventsand every partition, so expecteventsreads and writes to stall for up to the 5slock_timeout. A lock timeout aborts the migration transaction and is safe to retry. Under auto-migrate, the pod fails boot, restarts, and retries.Mixed-version behavior:
migrate.autoMigrate: truein the chart): sqlx finds applied migration 89 missing from its embedded set and returnsVersionMissing(89). Boot is fatal and the pod crash-loops. A binary rollback is therefore unsafe once 0089 has run.verify_floor_guard_catalogfails, the fence starts closed, and the relay degrades theReplicaFenceProbestartup step. It keeps serving, with cursor reads on the writer.Tests
crates/buzz-db/src/runtime/tests/floor_retirement.rsadds four PostgreSQL tests. CI's nextest filter picks them up throughpostgres_tests:::All four fail when the change is reverted (0/4 pass) and pass with it (4/4). Four tests that only exercised the trigger or the probes are deleted. The outbox durable-failure regression is kept: it now injects an isolated
BEFORE INSERTfailure instead of arming the floor. The phase-1 floor fixture no longer disables a trigger that no longer exists. It still asserts that no enabled floor trigger can mask the app check.With the trigger gone, the floor source policy (
every_production_channel_event_insert_checks_the_floor_and_never_rewrites_it) is the only guard against a writer that skips the app check or rewritescreated_at/channel_idon an existing row. Phase 1 already makes it scan every crate'ssrc. This PR closes two matcher gaps the trigger used to cover:UPDATE events SET(created_at, …)written with no space before the parenthesis;INSERT INTO events … ON CONFLICT … DO UPDATE SET channel_id = …, which names no table but can re-home an existing row below the floor. EachDO UPDATEis bound to the INSERT it belongs to, so a watermark upsert that setscreated_atnext to a guarded events insert (the replaceable-event writer's shape) is not flagged.Both gaps have negative fixtures, plus controls for a non-floor-column upsert,
DO NOTHING, and the watermark shape.The policy also now fails closed on SQL the per-function literal scan could not attribute to a writer (
event_floor_policy_fails_closed_on_sql_it_cannot_attribute):include_str!is expanded from the file it loads, andconcat!becomes one literal, with an{opaque}placeholder for each non-literal argument. Every delimiter ((),[],{}) and path prefix (::core::concat!) is recognized.SETcolumn that is a placeholder (concat!orformat!); aSETlist of floor columns whoseUPDATEis in another literal; anUPDATE eventsorDO UPDATEwhoseSETlist continues in another literal or argument (empty, or an item with no=, as informat!("UPDATE events SET {}", …)orQueryBuilder::new("UPDATE events SET ").push(…)); anINSERT INTOwith no table after it in the literal, or an events INSERT with a placeholder tail;eventswritten byMERGE INTO/COPY; and event-write SQL in aconst,static, or macro body outside any function. A placeholder in aWHEREor a value is allowed, so the retention deletes (concat!around a predicate macro) and the tenant-severingUPDATE {table} SET community_id = NULLpass.include_str!whose path is not one readable literal and any sqlxquery_file*!macro are violations. Nothing usesquery_file.PREPARE TRANSACTIONin any production literal, including a danglingPREPARE, is a violation (see below).constis judged at item level instead of being credited to the function before it.Prepared transactions. The fence used to bound a prepared (2PC) transaction by
pg_prepared_xacts.prepared. That only held because the deferred trigger checked its rows atPREPARE; the insert-time check runs earlier, at a timepg_prepared_xactsdoes not expose. So the probe no longer foldspreparedinto the wall: any prepared transaction fails the sample closed (ProbeError::PreparedTransactions). No supported writer prepares transactions (now source-policy enforced) andmax_prepared_transactionsdefaults to 0, so in supported deployments this never fires.cluster_global_sample_writer_fails_closed_on_prepared_transactionasserts 2PC is unavailable at the default setting, and asserts the fail-closed error when the server allows it. CI runs only the default branch; the 2PC-enabled branch and a check-removed falsification run are pinned from the Blox gate. The prepared count is cluster-wide, like the activity scan.Verification
Head
6ef31ef113(before the fold). On Blox, fmt, clippy and buzz-db unit pass, and the full buzz-db PostgreSQL lane passes exceptmedia_prebuild_waits_for_old_transactions_during_creation_and_cleanup, which fails only on the Blox host (it passes in CI). The twofloor_retirementtests were fixed after that run, so they were rerun on their own and pass at this head.What got simpler
The change is net −600 lines. Gone are one function, one parent constraint trigger plus its partition clones, two startup probes and their scratch helper, one boot wrapper, the per-connection GUC setup, one metric step, and an async startup branch that could no longer be reached.
Notes
0089, the first free number after main's0088(workflow run cause). The fence drop (0090) and NIP-RS drop (0091) follow it in this PR.floor_retirement.rsnow snapshots at0088and compares after0089alone, then runs the later migrations. The held-writer media prebuild test inmigration.rsnow stops at0088while the writer is held, then rolls the writer back before applying the rest. Otherwise a laterDROP TRIGGERmigration in the stack waits on that writer's lock (55P03). The floor policy'sSETlist ends at a top-levelWHEREorRETURNING(a subquery'sWHEREno longer hides later assignments). AnUPDATESETlist that runs to the end of its literal fails closed, and an open events upsert list is read on through later literals (QueryBuilderpushes).events_p*partitions count aseventsin every check.replacement_scope!now leavesWHEREto its callers so the policy can see the supersedeUPDATE's list end; the SQL is unchanged.buzz-relaygroup_state_postgres_testshitPoolTimedOuton a clean base too.