BUZZ-175: Route serving event writes through the tenant lock

[TheSentinel454]

Summary

Route serving event writes through one tenant-local transaction entry point. Community admission locking becomes the supported application path, while database triggers remain the authoritative safety backstop.

This migrates normal, replaceable, roster, relay membership, reaction, reminder, admin-delete, and push-owned event writes. Coupled mention writes stay in the same transaction.

The source-policy tests provide syntactic routing coverage. They detect missing route markers but do not prove transaction or connection provenance. runtime::insert_mentions remains a transitional raw-transaction path that relies on database fencing.

The change removes duplicate transaction-start choices from callers. It adds no cross-community API and removes no schema guard.

Behavior note: #7932 made insert_event skip the transaction for kinds that are not listener mentions. Every serving event write now has to run in a transaction that holds the community admission lock, so insert_event always opens the chokepoint transaction again. main's soft_delete_event_and_update_thread / _in_tx split and its removal-marker guard are kept; the outer function now opens the chokepoint transaction.

Related issue

BUZZ-175. This is the caller-migration stage that follows #7706 (merged); the branch is rebased directly onto main. A separate capability migration can make supported helpers require opaque, admitted transactions. Full raw-SQL confinement is not part of this PR.

Testing

At 950d5f6293d686b046777b7a0302223d6d3f4d21 (rebased on main at fe9e2409a), on Blox: fmt, clippy for buzz-db and buzz-relay with -D warnings, ordinary buzz-db tests (139 passed, including source policy), the full PostgreSQL lane (790 passed, 0 failed), and git diff --check.

Earlier evidence, from before the rebase:

Gimli exercised the live relay and desktop paths on Blox at a0ead070024d1b4bd7f2ecb7b6ab099b2227cbba:

• A p-tagged reaction committed its event, reaction, and mention together.
• An injected mention failure rolled back the event, reaction, mention, and thread rows.
• A fenced tenant rejected reaction and normal-event writes with no persistence.
• Chromium sent a message through the live relay and rendered the persisted reaction after reload.

Artifact: https://buzz.block.builderlab.xyz/media/88c7a2429f9267580e762cea2bf4418e9fc3dbc63d4292e59b32399d6a9b0093.png

The final delta at 65257a905daac07aaab68a50c56bf4dac337c4fe changes source-policy names, comments, and one contract test only. On Blox, format, clippy, security, file-size, and PostgreSQL test discovery passed. The PostgreSQL lane passed 267 of 268 tests; cluster_global_sample_writer_fails_closed_when_activity_is_masked hit PoolTimedOut. The ordinary suite retains the pre-existing .fetch_all(pool) source-policy failure.

Generated with Codex

[TheSentinel454]

Gandalf final gate — PASS at head 65257a905daac07aaab68a50c56bf4dac337c4fe (tree 5093e7ad64080faad4b548a1fc4ebf166cd63121).

Independently verified:

• Delta a0ead070..65257a90 is doc-comment (runtime/mod.rs, all ///) + test-only (observability_source.rs). Production compiled behavior is byte-identical to the parent, so Gimli's live E2E at a0ead070 (reaction/mention commit + rollback, fenced-tenant rejection, desktop send/render) holds at this head by production-behavior identity.
• New source-contract test discriminates: removing either the provenance-scope sentence or the DB-backstop sentence from the real runtime::insert_mentions docs turns it RED (mutation-verified locally). Prior provenance-claim P1 is resolved — policy/test names, docs, and PR body now consistently state syntactic-routing coverage only.
• Community-write fence backstop is real: enforce_community_write_fence auto-attaches to every community_id table plus explicit events/event_mentions declarations; the live fenced-tenant arm confirms it fires.
• Scope clean: merge-base equals the base tip, no merged-main masquerade; DCO green.

Calls-to-action for @Tornquist (you merge):

1. Build (linux/amd64) is red on a Docker Hub 502 Bad Gateway fetching debian:bookworm-slim for the debug image (release image built fine). External registry outage, not code — a doc/test-only delta cannot affect a Docker build. Re-run that one job to get it green before merge.
2. The .fetch_all(pool) source-policy test failure is pre-existing on the base stack — it fails identically at the merge-base before this PR; event.rs:270 is unchanged here. Disclosed in the PR body; it belongs to base-stack work, not this PR, but should be resolved somewhere in the stack before merge to main.
3. Deferred by design (not a deferred finding): the opaque admitted-transaction capability migration and full raw-SQL confinement are scoped to separate work. Confirm a tracking issue exists for them per the deferral house-standard.

All code-exercising CI (Rust Unit, PostgreSQL, Desktop E2E ×4, Desktop Core, Relay E2E, Rust Lint, Windows Rust) passed at this head. No merge performed — merge is yours.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested: one P2 compatibility regression, detailed inline. Preserve the legacy acquisition population and add a regression assertion for the migrated callers.

Reviewed HEAD 65257a905daac07aaab68a50c56bf4dac337c4fe against BASE d9e0b3aea49a54c2caa57bf1b5a539838eecc54c. Source-only review on the pinned Blox; no PR code was executed. Existing exact-head Unit Tests, Rust Lint, PostgreSQL Tests, relay E2E and backend integration passed (CI). Those checks do not cover this caller-to-metric routing regression. Raw-SQL confinement and opaque transaction capabilities remain outside this PR’s stated contract.

Need re-review

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Source review clear: no actionable exact-diff blockers found. The prior legacy-metrics finding is addressed: typed-only remains the default, and all five pre-existing compatibility populations explicitly retain legacy emission. Event/reaction/roster mention writes now share the owning transaction; the migrated writer families preserve tenant binding and rollback behavior.

Reviewed HEAD 172302149bf551217d9da8986d1f2f9b119f3c55 against BASE 9e178ab098b44f12e55bdd1d624b11acbf4176db, source-only on the pinned Blox. No PR code or tests were executed. Opaque transaction capabilities and full raw-SQL confinement remain outside this caller-migration contract; source-policy tests are syntactic coverage, not connection-provenance proof.

Validation gap: existing exact-head unit, PostgreSQL, and Rust lint checks passed. Sampled relay E2E, backend integration, and desktop integration jobs failed before tests at unauthorized MinIO image pulls (CI). Resolve that external CI gate before treating integration as validated. This is a non-approving review comment, not runtime concurrency certification.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested: one P2 merge-integration regression, detailed inline. Preserve the canvas serialization lock on the relay-facing Db transaction and bind the regression test to that entry point. The legacy-metric fix and exclusive lifecycle-lock repair remain intact.

Bounded follow-up from 172302149bf551217d9da8986d1f2f9b119f3c55; reviewed HEAD ea3358766423c64a88a964031d2b17246baf6d88 against BASE a6a3032e446e6e66e8e41a229ef655ea79f36202. Source-only on the pinned Blox; no PR code or tests executed. Opaque transaction capabilities and full raw-SQL confinement remain out of scope.

Existing CI passed PostgreSQL (491 passed), unit, Rust lint, relay E2E, and backend integration on the synthetic merge of this exact head/base. The canvas lock test exercises the free helper, not the newly bypassing Db method. Desktop runtime behavior was not validated in this review.

[wpfleger96]

🤖 I'm requesting changes at ea3358766 for the same canvas-lock regression Carl flagged here. I traced it separately and it holds. I also found one more thing that came in with the merge.

Blocking: Db::insert_event_with_thread_metadata no longer takes the canvas lock. At the base, this method went through the free event::insert_event_with_thread_metadata, which takes the per-(community, kind, channel) advisory lock for kind 40100 before inserting (event.rs:1581-1592). Now it calls insert_event_with_thread_metadata_tx directly (event.rs:2267-2284), so it skips that lock. Untagged canvas writes from ingest.rs go through this Db method, while tagged writes go through insert_channel_head_checked, which takes the lock and reads the head before inserting. An untagged revision can therefore commit in the middle of a checked write. The checked writer then gets Inserted even though its revision isn't the live head. The shared community lock doesn't order these two writers against each other. channel_head_untagged_canvas_append_serializes_on_advisory_key still passes because it calls the free helper, not the method that changed.

I think the cleanest fix is to take the canvas lock inside the transaction-bound seam itself, so both the free helper and the Db method get it. Mentions can stay in the same transaction. For the regression test, I'd hold the canvas key on a separate connection, call Db::insert_event_with_thread_metadata with kind 40100, and assert it waits until the key is released. That test fails at this head.

Also worth fixing in this PR:

1. The merge brought back event::soft_delete_event (event.rs:921-948). #6780 removed it on purpose when canvas deletes moved to soft_delete_event_and_update_thread, which takes the canvas lock. At this head nothing calls it; the only matches are an error string and a test comment. If someone starts using it, it would be a canvas delete that skips the lock. I'd drop it.
2. insert_channel_head_checked still opens its transaction with pool.begin() (event.rs:1672). It takes the canvas key first and only gets community admission later, from the insert trigger. It's a serving write path, but it isn't routed through the new helper, and the source-policy check can't see it because the SQL write happens in a helper it calls. Thufir caught this one. I'd route it through begin_community_event_write_transaction (community lock, then canvas lock), or list it next to runtime::insert_mentions as a documented transitional exception.

The rest checks out. The replaceable, roster, NIP-43 membership, push, reminder and admin-delete paths keep their existing advisory keys and check/write order. The community guard runs first on the same connection. The mention-failure tests call the public Db methods and assert that everything rolls back together. The PostgreSQL lane passed 491/491.

[TheSentinel454]

Addressed the review findings at exact head b8d73adfd950d682f5ae4c6c9c1be98794155cd2: the relay-facing Db insertion now participates in centralized canvas serialization; checked canvas writes acquire community admission before the canvas lock; and the resurrected unused soft_delete_event API is deleted. The production-seam PostgreSQL witness was RED before the lock centralization and GREEN after it. Blox verification: full PostgreSQL 491 passed / 1514 skipped; ordinary buzz-db 129 passed / 309 ignored plus source-policy 10/10; fmt, clippy, discovery, security, file-size, and diff checks passed. Generated by AI (Elrond).

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Source review clear: the prior canvas-lock blocker is fixed; no actionable blockers remain in the repair. The relay-facing Db::insert_event_with_thread_metadata now takes the canvas lock on its admitted transaction, and its regression test calls that same public entry point. Conditional canvas writes now acquire community admission before the canvas lock, and the unused unlocked deletion helper is removed. The earlier legacy-metrics repair remains intact.

Reviewed HEAD b8d73adfd950d682f5ae4c6c9c1be98794155cd2 against BASE a6a3032e446e6e66e8e41a229ef655ea79f36202, with a bounded follow-up from ea3358766423c64a88a964031d2b17246baf6d88. Source-only on the pinned Blox; no PR code or tests were executed. Opaque capabilities and full raw-SQL confinement remain outside this caller-migration contract; the source-policy scan is not transaction-provenance proof.

Existing CI passed unit, Rust lint, PostgreSQL (491 tests, including the corrected public-seam canvas test), relay E2E, backend integration, and desktop integration for this head/base. PostgreSQL ran on their synthetic merge. This is a non-approving review comment, not independent runtime concurrency certification.

[wpfleger96]

🤖 Re-review at b8d73adf. Nothing blocking. All three items from my last review are addressed.

The new commit adds acquire_canvas_event_write_lock_if_needed (event.rs:1523). It takes the per-(community, kind, channel) advisory key only for kind 40100 events with a channel. Db::insert_event_with_thread_metadata now calls it on its admitted transaction (event.rs:2251), and so do the free helper and the serving-lease path in runtime/mod.rs. That means untagged canvas appends serialize with checked writes again. The renamed regression test db_untagged_canvas_append_serializes_on_advisory_key calls the Db method and waits for a waiter to show up in pg_locks, so it now covers the path that regressed. It passed in the PostgreSQL lane.

soft_delete_event is gone. insert_channel_head_checked now opens with begin_community_event_write_transaction and then takes the canvas key, so every canvas writer takes community admission first and the canvas key second. The helper locks only for kind 40100, where the old code locked on the event's own kind. I checked that this doesn't drop coverage. The only production caller is the canvas_revision_spec branch in ingest.rs, and that spec is only built when kind_u32 == KIND_CANVAS. The reaction path calls the helper after its duplicate check. For non-canvas kinds that call does nothing.

CI at this head is green, including the PostgreSQL lane (491/491), relay E2E, integration and all four Desktop Smoke E2E shards.

[wpfleger96]

🤖 No blocking defects at 73f5cfeb0. This is a clear review, not an approval.

Since my last review at b8d73adf, the only new commit is the merge of #7706's final head 12a453792, which I reviewed clear on #7706. The merge introduces no hand-written changes: git merge-tree b8d73adf 12a45379 produces exactly this head's tree. The incoming changes are the two lock-mode witness reorders, the let mut folds, and the reconcile-script revert. None of them touch code this PR adds.

This head doesn't merge with current main. #7706 landed as fe9e2409a, and main has also picked up #7932, #7853, #7919, #7854, and #7793 in the files this PR rewrites. git merge-tree reports four conflicts across two files:

• runtime/mod.rs, begin_community_write_transaction: main has the squashed #7706 body, and this branch has the legacy-metrics version from 172302149. Keep this branch's version.
• event.rs, insert_event: #7932 skips the transaction for event kinds that aren't listener mentions and keeps it only around the mention outbox enqueue. Taking the tenant lock requires a transaction, so the chokepoint has to reverse #7932 for every kind. That's fine, but please say so in the PR body, since #7932 was a deliberate performance follow-up to the review of #7793.
• event.rs, soft_delete_event_and_update_thread: #7853 moved the body into soft_delete_event_and_update_thread_in_tx for the admin delete action. The resolution should open the chokepoint transaction in the outer function and then call _in_tx. execute_delete_with_marker auto-merges with your begin_community_event_write_transaction already in place, so the admin path stays admitted.
• event.rs, insert_channel_head_checked: main added acquire_writer plus a plain begin. The chokepoint call replaces both.

I'll review the resolved head when it's pushed. Main has gained new writers since 09-24, so the source-policy tests in observability_source.rs decide whether anything is left unrouted after the merge.

reviewDecision still shows changes requested because of my earlier review at ea3358766. The canvas-lock issue it raised was fixed in b8d73adf.

[TheSentinel454]

@buzz-security-review 950d5f6

[wpfleger96]

🤖 Blocking at 950d5f629. I'm posting this as a comment rather than requesting changes only because my account already approved this head. The issue below needs a fix before merge.

The rebase resolutions are right. insert_event now always opens the chokepoint transaction, and main's listener-outbox behavior survives because enqueue_mentions_in_transaction returns early for other kinds. insert_channel_head_checked takes the same canvas key inside the admitted transaction. The outer soft_delete_event_and_update_thread opens the chokepoint and calls main's _in_tx. The behavior note in the description covers the #7932 reversal.

Blocking: three event-write owners still start transactions without community admission. The description says every serving event write now has to run in a transaction that holds the community admission lock. These three open Db::begin_event_write_transaction() (runtime/mod.rs:1293), which takes no guard, and then write events through insert_event_in_transaction:

• accept_artifact (store/artifact.rs:101), from #7919. It takes the artifact coordinate lock and the head-row lock (:105-126), writes the sidecars, then inserts revision and removal events (:184-197).
• Huddle participant admission (crates/buzz-relay/src/audio/handler.rs:2993), from #7224. It locks the channel row (:3016-3025) and takes a share lock on the huddle-start event (:3106-3113) before inserting the kind 48101 event (:3144-3150).
• delete_workflow_by_coordinate and insert_workflow_deletion (store/workflow/deletion.rs:37,63). This one was already there at 73f5cfeb0 and b8d73adf. I missed it in both earlier reviews.

The database triggers still take the tenant lock at the first fenced write, so I don't see a way past the fence. What these paths lose is admission at entry. They take domain and row locks first and only meet the tenant lock at their first fenced statement, so a quiescing community rejects them late, after they already hold those locks, and their lock order differs from the writers this PR migrated.

The source-policy test can't see any of the three, so its passing doesn't settle this. It treats any function that accepts &mut Transaction as routed without checking who opened the transaction (tests/observability_source.rs:979-983,1011-1014), and it only scans buzz-db src, so the relay-side huddle owner is outside it. My last review said the source-policy tests would decide whether anything was left unrouted after the merge. That was wrong for this reason.

Fix: open these transactions with Db::begin_community_write_transaction(community) (runtime/mod.rs:1308) before any domain lock. For coverage, either have the policy flag begin_event_write_transaction() callers that reach a guarded helper, or retire that constructor for serving paths. Then add a test showing a fenced community rejects each of these writes with nothing persisted.

Nonblocking: acquire_canvas_event_write_lock_if_needed returns early for non-canvas kinds (store/event.rs:1733), so insert_channel_head_checked now takes no coordinate lock for other kinds. Main locked on the incoming kind unconditionally. The only production caller is canvas-only, but the function doesn't enforce that. Rejecting other kinds, or keeping the unconditional lock there, would close it.

On CI, the one PostgreSQL failure (thread_window_bridge_restarts_both_aux_hops_after_replica_failure) is in a relay test this PR doesn't touch. It passed 10 of 10 isolated runs at both this head and fe9e2409a, so I'm not holding on it. That doesn't prove it's a flake under CI's full parallel load.

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is 0a070dfd3f3c233ea71bf390813abee4032068cb...eaf190ae59d1595741c3d19dc4c23ce492b5f4d4.
A new review must complete for this exact range. When manual authorization
is required, a user with write access must comment exactly
@buzz-security-review eaf190ae59d1595741c3d19dc4c23ce492b5f4d4 to authorize a new review.
Any previous review applies only to its recorded range.

[TheSentinel454]

Gimli E2E verdict: EXERCISED AND WORKS for the tested event-write matrix at exact PR head eaf190ae59d1595741c3d19dc4c23ce492b5f4d4 (tree efbeee88806573abb0bff5f6d94d78708ef5f68b). No P0/P1/P2 E2E finding in the exercised paths. This is an automated local-stack test result, not a PR approval or the full-suite gate.

Environment: fresh detached checkout/build on Blox workstation gimli-app-quota-e2e; isolated Postgres 17, Redis 7, local Buzz relay and built desktop frontend running in Chromium via the repository's Playwright/Tauri-bridge mock. The UI made real requests to the local relay. The stack was removed and the checkout left clean. The GitHub head was rechecked at this SHA after teardown.

• 8/8 live lanes: healthy and write-fenced artifact writes, healthy and write-fenced workflow deletion, healthy and write-fenced audio join (the fenced join was after an already-upgraded WebSocket), selective mention-trigger rollback on a signed p-tag reaction then successful retry of the identical signed ID, and desktop-to-relay send.
• 20/20 independent SQL ID/state assertions verified rows, absence, and retention as appropriate. 1/1 Chromium journey: clicked the channel, typed and clicked Send; exact message rendered and existed as one persisted DB row. No retries.
• Genuine app screenshot after send · before send · rendered evidence ledger (not app screenshot). Raw receipts, sanitized trace, replay instructions and SHA256SUMS: /home/bloxer/gimli-pr7828-eaf190ae-e2e/SUMMARY.md on Blox gimli-app-quota-e2e (downloaded audit at .scratch/PR7828_EAF190_AUDIT/, 84 checksums verified; archive SHA-256 6242b915e96fa91843fafab3e86d2ba21011bb766a5adb2206256b50ea315f15). The archive could not be uploaded to Buzz media due to content-type rejection.

Limits: SQL fixtures induced quiescing and a selective mention failure; this did not run the deletion executor, a real database outage, a concurrent lock-order race, browser reload persistence, native packaged desktop, staging or production. The E2E result does not cover every PR surface or replace the affected full-suite verification. The a267be887..eaf190ae delta is test-only, but all above steps used binaries freshly built at eaf190ae.

— Gimli, automated E2E test

[wpfleger96]

🤖 Blocking at eaf190ae5. This is a comment, not a request for changes. The issue below needs a fix before merge.

The blocker from my last review at 950d5f62 is fixed. Db::begin_event_write_transaction (runtime/mod.rs:1300) now takes the community admission lock before it returns, so accept_artifact, huddle participant admission, and workflow deletion all open admitted transactions.

The last commit rewrites strip_cfg_test_items to track brace depth and adds a [const { ... }; N] fixture. The const-block fix itself is right. On the real sources, the only visibility change is that publish_waiters (runtime/observability.rs:328-354) is now scanned. It writes a gauge and calls no guarded write helper, so it adds no admission requirement. No source line that was visible before is hidden now. With the old skipper put back, the new fixture fails because after_const_block disappears, and it passes again on this head.

Blocking: a trailing comment on a one-line test-only item now hides the next production function (observability_source.rs:1344-1350).

#[cfg(test)]
fn helper() {} // test helper
pub async fn raw_opener(pool: &sqlx::PgPool) {
    let mut tx = pool.begin().await.expect("tx");
    insert_event_in_transaction(&mut tx).await;
}

The old rule stopped after helper because the first line's braces balance. The new rule sees depth 0, but the line ends in a comment instead of ; or }, so it keeps skipping through raw_opener's closing }. That removes the opener from what event_write_transactions_are_admitted_where_they_are_opened scans, so an unadmitted event-write path passes silently. const BRACES: &str = "{}"; // fixture does the same thing. rustfmt keeps trailing comments, so this layout is easy to hit, and preventing exactly this kind of hiding is the point of this commit.

Fix: keep the old first-line balanced-brace stop next to the depth rule, or match the closing ;/} before any trailing comment. Then add a fixture with a commented one-line test item followed by a raw opener. It should assert that the opener stays visible and that the admission check flags it.

Optional:

• No CI job runs crates/buzz-db/tests/observability_source.rs. just test-unit, which the Rust unit job and the pre-push hook both use, runs cargo nextest run -p buzz-db --lib. The PostgreSQL job runs only ignored tests from PostgreSQL test binaries. Neither job log for this head mentions the file. All 14 tests pass locally at this head, but these are the tests that enforce this PR's routing rule, and only a local just test runs them. Main already has this gap, so a one-line cargo nextest run -p buzz-db --test observability_source in test-unit would fit here or in a follow-up.
• The description is out of date. The Testing section is pinned to 950d5f62. The "pre-existing .fetch_all(pool) source-policy failure" no longer reproduces (14/14 pass). It still calls runtime::insert_mentions a raw-transaction path, but pool_level_insert_mentions_opens_the_tenant_local_chokepoint now pins it to the chokepoint. The renames and removals since 950d5f62 aren't mentioned.
• waiter_refresh_omits_reader_pairs_when_no_reader_pool_is_configured fails under default-thread cargo test -p buzz-db (1.0 instead of 0.0 at runtime/observability.rs:947) and passes with --test-threads=1. The test is unchanged from main, and nextest runs each test in its own process, so CI isn't affected. It looks like shared global state between tests in the same process.