Repository navigation
BUZZ-175: Add application-owned writer lock foundations - #7706
Conversation
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
…tract Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
🔐 Codex Security Review
|
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: no actionable introduced defect found at d9e0b3aea49a54c2caa57bf1b5a539838eecc54c against 4ab4f786085a23fe6126529861840eff6048ceee. Two nonblocking test-assertion improvements are noted inline.
Reviewed community admission/deletion ordering, probe lock → sample → activity scan → heartbeat commit → ring publication, rollback/error paths, and mixed-version compatibility. This head adds single-community, opt-in helpers; production deletion behavior and existing trigger/GUC backstops remain unchanged. The PR description should reflect that narrower scope.
Existing PostgreSQL CI, verified against this head/base pair, passed 428/428 tests including all five added witnesses. Review was source-only; no additional test, mutation, or deployment execution. No blocking changes requested.
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: tornquist <tornquist@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P1 deletion-fence regression at 9e178ab098b44f12e55bdd1d624b11acbf4176db, against base 4ab4f786085a23fe6126529861840eff6048ceee. The new writer coexistence and exact-denial assertions address the prior suggestions, but the accompanying production change removes lifecycle exclusivity. Restore that barrier and cover the production transition without an incidental foreign-key row lock before merge.
Reviewed writer admission through quiescing/fencing/abort, trigger and serving-lease backstops, and replica probe transaction/ring publication. Source-only on the pinned Blox; no checkout, build, tests, or PR-code execution. Existing exact-head PostgreSQL CI passed; Desktop Smoke E2E (4) failed and was not diagnosed here.
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: tornquist <tornquist@squareup.com>
…undation-local * origin/main: (50 commits) feat(desktop): relay admin console for the /api/admin/v1 operator surface (#4768) fix(mobile): keep retired sections manager out of successor cache (#7873) Select one feature flag provider at compile time (#7677) chore(release): release Buzz Desktop version 0.5.25 (#7867) fix(ci): consume the published MinIO image (#7870) fix(mobile): converge sidebar managers on relay head with resume re-read (#7806) fix(ci): bootstrap the reusable MinIO image in GHCR (#7869) Discover alternate Buzz ACP commands (#6948) fix(hooks): surface nextest failures and stale pnpm deps in pre-push (#7850) feat(acp): run one prepared task from a file or stdin (#7851) Fix mobile heart and warning emoji with native font fallback (#7842) chore(mesh): upgrade MeshLLM to 0.76.2 (#7559) feat(agents): humanize uncurated Databricks model ids with a label grammar (#7844) fix: route databricks claude fqns to anthropic messages (#7829) feat(relay): add opt-in newest-first thread windows (#7823) refactor: move agent Git bootstrap into ACP harness (#7819) Use worker snapshots for relay storage metrics (#7845) fix(hooks): strip repo-local git env from pre-push test lanes (#7841) fix(mobile-infra): render push grant lifetimes as decimal in chart 0.3.2 (#7820) fix(agent): preserve Databricks Opus UC reasoning and tool continuation (#7840) ... Signed-off-by: tornquist <tornquist@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: the prior P1 lifecycle-lock regression is repaired at a6a3032e446e6e66e8e41a229ef655ea79f36202, against base 8f8c4dfadecb2298a9140af5d323487aaad844ab. No new blocking defect found in this bounded corrective follow-up.
lock_community_deletion is exclusive again; quiescing, fencing, and abort use it while admitted writers retain the shared counterpart. The new regression exercises real begin_quiescing against an open update of an existing allowlist row’s non-key note, avoiding the earlier incidental parent-FK lock. The 50 ms unfinished-task assertion is timing-based, not deterministic mutation proof; that remains a nonblocking test limitation.
Reviewed the repair and necessary seven-path merge integration. The APIs remain opt-in, existing trigger/GUC backstops remain authoritative, and replica proof publication still follows heartbeat commit. Unrelated merged upstream changes were excluded.
Existing PostgreSQL CI passed 486/486 tests, including the new regression. Its checkout was synthetic merge 2a5f42225a11f4169c3fa6753fb5a2cdb4562b2e, containing this head merged into 676e8c43825fa478850a3a320b1367d3288cce1a, not the exact pinned base pair. Source-only review on Blox; no checkout, tests, mutation run, or PR-code execution performed by this review. No blocking changes requested; this is not an approval.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 No blocking defects at a6a3032e4. This is a clear review, not an approval.
This agrees with the earlier automated review at this head. The lifecycle helper is exclusive again, so begin_quiescing, fence and abort all wait for admitted writers holding the shared key. Abort now locks in the order advisory lock, community row, request row, which matches the other lifecycle transitions. sample_writer takes the exclusive floor lock before it samples S, and it commits the heartbeat before probe_once records the ring entry. So a failed or dropped probe never publishes an uncommitted token. The PostgreSQL lane passed 486/486, including all six new tests and the updated abort/quiesce test.
A few small things I think are worth fixing in this PR:
- Two lock-mode witnesses can't detect a missing lock.
runtime/tests.rs:3125-3154andstore/deletion.rs:4146-4177keepshared_contenderopen while they try the exclusive lock. That contender blocks the exclusive attempt on its own, so both tests would still pass if the helper took no lock at all. They catch shared-vs-exclusive but not shared-vs-absent. The probe-wait andbegin_quiescing_waits_for_open_admitted_writer_note_updatetests do cover a missing lock, so this is about test precision, not a coverage hole. Rolling backshared_contenderbefore the exclusive attempt fixes both. - The PR body overstates the scope. It says this adds "stable ordering for multi-community writes". The code adds a single-community wrapper, and the new ARCHITECTURE.md section says a supported serving transaction never spans communities. That also answers your multi-community question on
runtime/mod.rs. I'd narrow the summary bullet to match. - Something to design for when callers migrate: a compliant writer holding the shared floor lock for a long time stalls the probe, and an exclusive waiter queued in Postgres also blocks new shared requesters behind it. So one slow writer can hold up every other compliant channel-event writer until the writer's
lock_timeout(5s by default) fails the probe. Nothing callsbegin_replica_floor_locked_event_write_transactionyet, so nothing is affected today. When the first caller lands, it's worth bounding how long writers hold the shared lock, or giving the probe a shortSET LOCAL lock_timeoutso a missed sample doesn't block writers. - Nits:
- The only change left in
scripts/reconcile-schema-after-pgschema.sqldeletes a blank line. The body says that unrelated edit was removed, so I'd drop the file from the diff entirely. - A blank line now separates the doc comment from its test at
replica_fence.rs:1101-1102. let mut sampling = sampling;(replica_fence.rs:1065) andlet mut probing = probing;(runtime/tests.rs:3194) can be folded into thelet muton the spawn.
- The only change left in
The 50/100ms "still waiting" assertions remain timing-based, as the earlier review noted. That's fine for this slice.
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
|
@buzz-security-review 12a4537 |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 No blocking defects at 12a453792. This is a clear review, not an approval.
The three commits since my last review at a6a3032e4 address its notes:
- Both lock-mode witnesses now roll back
shared_contenderbefore trying the exclusive lock (runtime/tests.rs:3135-3138,store/deletion.rs:4158-4161). The writer is now the only lock holder when the exclusive attempt runs, so both tests fail if the helper takes no lock. scripts/reconcile-schema-after-pgschema.sqlis out of the diff, the stray blank line before the masked-activity test is gone, and bothlet mutrebinds are folded into the spawns.- The PR body now describes a single-community write lock, which matches the code and ARCHITECTURE.md.
The branch is based on main from 09-24, and main has since added owner deletion admission and automatic preparation in deletion.rs. It still merges cleanly. The reordered abort (advisory lock, then community row, then request row) uses the same order as main's new complete_owner_preparation (shared advisory lock, community row, owners, request row), so the new path doesn't bring back the deadlock this PR fixes.
When this was published, the PostgreSQL lane that runs the changed #[ignore] tests was still queued. The failed Codex Security Review job was cancelled during its own output validation, so it says nothing about this diff.
|
Re: review at Thanks. Items 1, 2 and 4 are fixed in |
Brings 16 upstream block/buzz commits (a14107a) into the fork integration branch: ACP mention/edit steering (block#6131, block#6132), quiet-host recovery wakes (block#7459), relay NIP-FI shadow mode (block#8034, block#8062), writer lock foundations (block#7706), Goose MCP handshake (block#8037), Claude model names (block#8053), summarized thinking (block#8051) and mobile iOS changes. Merged cleanly without textual conflicts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Arnoldinh0 <arnaudlafosse92100@gmail.com>
## Summary
Route every serving event write through one community-admitted
transaction entry point. `Db::begin_event_write_transaction(community)`
is now the only `Db` constructor for event-write transactions: it takes
the shared community admission lock before returning, so callers that
use it cannot take domain or row locks ahead of tenant admission, and a
quiescing community rejects the write at entry. Database triggers and
commit-time fences remain the authoritative backstop.
This migrates normal, replaceable, roster, relay membership, reaction,
reminder, admin-delete, push-owned, artifact-accept, huddle-join
(48101), workflow-deletion, and command-executor event writes. Coupled
mention writes stay in the same transaction; pool-level
`insert_mentions` now opens its own admitted transaction.
**Review fixes (since `950d5f6`):**
- The unguarded event-write constructor is gone. Removed with it: the
deprecated `begin_transaction` alias, the test-only replica-floor
constructor, and `begin_community_write_transaction` (now identical to
the single constructor). Artifact accept, the huddle join, and both
workflow-deletion paths now admit before any domain or row lock;
`persist_command_event` no longer hand-rolls `guard_transaction`;
read-only `query_artifacts` no longer opens an event-write transaction.
- `insert_channel_head_checked` is now `insert_canvas_head_checked`: it
rejects non-canvas kinds before opening a transaction and always takes
the coordinate lock.
- Source policy now also scans `buzz-relay`: any function that hands a
transaction to an event-write helper must have opened it through the
admitted constructor. The scan drops each `#[cfg(test)]` item instead of
stopping at the first one: it tracks brace depth and ends the item on a
line ending in `;` or `}` (ignoring a trailing `//` comment, even after
a string that contains `//`), with a column-0 `}` as fallback. Fixtures
pin a unit struct, a multi-line static, a `const { … }` block, a struct
with fields, a module, and one-line items with trailing comments,
including after `"https://…"` and `"ws://…"` strings; a raw opener after
each must still be flagged. Known limit: braces inside string or char
literals or comments are still counted, and only `//` trailing comments
are recognized, so a test-only item with unbalanced braces in a literal
or comment, or with a trailing `/* */`, would hide what follows; none
exists today. The scan checks that admission is present; the per-path
PostgreSQL tests pin that it comes before domain locks.
- `just test-unit` (and the non-nextest fallback in
`scripts/run-tests.sh`) now runs `buzz-db --test observability_source`.
Before this, no CI job ran these source-policy tests: the unit lane ran
`buzz-db --lib` only and the PostgreSQL lane runs only ignored tests.
- Renamed or removed since `950d5f6`: `begin_transaction`,
`begin_community_write_transaction`, and the replica-floor test
constructor are gone; `insert_channel_head_checked` →
`insert_canvas_head_checked`; test helper `quiesce_community_for_tests`
now delegates to `main`'s `set_deletion_state`, which reads the real
`deletion_fence_generation` instead of hard-coding `'0'`;
`runtime::insert_mentions` is pinned to the admitted constructor by
`pool_level_insert_mentions_opens_the_tenant_local_chokepoint`.
**What got simpler:** four transaction constructors collapse to one, and
the hand-rolled admission guard is deleted. The compiler rejects callers
of the removed constructors, but a transaction opened from `Db::pool()`
can still reach the public `*_in_transaction` helpers; only the
source-policy scan and the database fences catch that until BUZZ-254.
**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 admitted 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 admitted
transaction.
### Related issue
[BUZZ-175](https://linear.app/squareup/issue/BUZZ-175). Caller-migration
stage following #7706 (merged); rebased onto `main` at `5173fad66`.
Follow-up: [BUZZ-254](https://linear.app/squareup/issue/BUZZ-254) makes
event-write helpers require a type-level admitted transaction.
**Known debt, out of scope:** `create_channel`, `update_channel`, and
`insert_thread_metadata` write channel and thread rows (not events)
outside the admitted constructor.
### Testing
At `efe4a94623dd69973780a3c39f16dd972cf7cb76` on a cloud workstation:
fmt; `buzz-db` clippy (all targets, `-D warnings`);
`observability_source` 14/14; `just --dry-run test-unit` includes the
new step. Reverting just the end-of-item check to an earlier rule makes
a trailing-comment fixture fail.
At `eaf190ae59d1595741c3d19dc4c23ce492b5f4d4` (the commit after it only
changes this test file, `Justfile`, and `scripts/run-tests.sh`): full
GitHub CI green; on the workstation, clippy for `buzz-db` and
`buzz-relay`, ordinary `buzz-db` 154/154, `just test-unit` green, and
the PostgreSQL lane 805 of 806 (exception below). An end-to-end run
against a local relay and the built desktop app covered artifact accept,
huddle join, and workflow deletion for both a healthy and a quiescing
community, plus p-tag reaction rollback and replay of the same signed
event.
New tests: one fenced-community test per migrated path (artifact accept,
huddle join, both workflow deletions) puts the community into
`quiescing`, holds the lock the path takes next, and asserts rejection
at entry with nothing stored; a DB-free test pins the non-canvas
rejection.
Exceptions:
-
`runtime::postgres_tests::writer_pool_rejects_non_read_committed_database_default`
hangs on that workstation's local Postgres 16 at both this head and an
earlier `main` base, `f0eb5575f` (2/2 runs each). The test, `Db::new`,
and the pool setup are unchanged by this PR, and the test passes in
GitHub CI. Live traces put the hang in the client, not the database: the
1 s acquire timeout stops firing after sqlx's `after_connect` rejection
path under tokio's `current_thread` runtime. No database lock waits were
observed.
- Two `observability_source` assertions were already failing on `main`,
unnoticed because no CI job ran that test binary: a stale
`fetch_optional` pin after #8005, and lease-seam attribution that now
lives in the `bounded_serving_lease_sql` helper. Both are corrected
here, and the binary now runs in `just test-unit`.
Generated with Codex
---------
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
## Summary Buzz is moving database invariants from triggers and foreign keys into application-owned transaction protocols. This PR adds the first transition foundations while preserving all current database backstops. - Add a shared community-write lock for single-community writer transactions, and make deletion lifecycle transitions take the matching exclusive lock. A supported serving transaction never spans communities. - Add shared event-writer and exclusive replica-probe lock ordering around the existing heartbeat publication model. - Document the supported writer contract, reviewed backfill path, and transition gates. - Keep one replica-floor publication concept and remove an unrelated reconciliation-script edit found during review. This narrows the change rather than adding parallel protocols. The blast radius is limited to new opt-in writer helpers plus lock acquisition in deletion and replica probing. Existing trigger and GUC enforcement remains authoritative, so callers can migrate in later pull requests. ### Related issue [BUZZ-175](https://linear.app/squareup/issue/BUZZ-175). This is the first implementation slice from the OSC migration plan. ### Testing No manual testing. CI covers the Rust checks and database tests added here. Generated with [Codex](https://openai.com/codex) --------- Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
) ## Summary Route every serving event write through one community-admitted transaction entry point. `Db::begin_event_write_transaction(community)` is now the only `Db` constructor for event-write transactions: it takes the shared community admission lock before returning, so callers that use it cannot take domain or row locks ahead of tenant admission, and a quiescing community rejects the write at entry. Database triggers and commit-time fences remain the authoritative backstop. This migrates normal, replaceable, roster, relay membership, reaction, reminder, admin-delete, push-owned, artifact-accept, huddle-join (48101), workflow-deletion, and command-executor event writes. Coupled mention writes stay in the same transaction; pool-level `insert_mentions` now opens its own admitted transaction. **Review fixes (since `950d5f6`):** - The unguarded event-write constructor is gone. Removed with it: the deprecated `begin_transaction` alias, the test-only replica-floor constructor, and `begin_community_write_transaction` (now identical to the single constructor). Artifact accept, the huddle join, and both workflow-deletion paths now admit before any domain or row lock; `persist_command_event` no longer hand-rolls `guard_transaction`; read-only `query_artifacts` no longer opens an event-write transaction. - `insert_channel_head_checked` is now `insert_canvas_head_checked`: it rejects non-canvas kinds before opening a transaction and always takes the coordinate lock. - Source policy now also scans `buzz-relay`: any function that hands a transaction to an event-write helper must have opened it through the admitted constructor. The scan drops each `#[cfg(test)]` item instead of stopping at the first one: it tracks brace depth and ends the item on a line ending in `;` or `}` (ignoring a trailing `//` comment, even after a string that contains `//`), with a column-0 `}` as fallback. Fixtures pin a unit struct, a multi-line static, a `const { … }` block, a struct with fields, a module, and one-line items with trailing comments, including after `"https://…"` and `"ws://…"` strings; a raw opener after each must still be flagged. Known limit: braces inside string or char literals or comments are still counted, and only `//` trailing comments are recognized, so a test-only item with unbalanced braces in a literal or comment, or with a trailing `/* */`, would hide what follows; none exists today. The scan checks that admission is present; the per-path PostgreSQL tests pin that it comes before domain locks. - `just test-unit` (and the non-nextest fallback in `scripts/run-tests.sh`) now runs `buzz-db --test observability_source`. Before this, no CI job ran these source-policy tests: the unit lane ran `buzz-db --lib` only and the PostgreSQL lane runs only ignored tests. - Renamed or removed since `950d5f6`: `begin_transaction`, `begin_community_write_transaction`, and the replica-floor test constructor are gone; `insert_channel_head_checked` → `insert_canvas_head_checked`; test helper `quiesce_community_for_tests` now delegates to `main`'s `set_deletion_state`, which reads the real `deletion_fence_generation` instead of hard-coding `'0'`; `runtime::insert_mentions` is pinned to the admitted constructor by `pool_level_insert_mentions_opens_the_tenant_local_chokepoint`. **What got simpler:** four transaction constructors collapse to one, and the hand-rolled admission guard is deleted. The compiler rejects callers of the removed constructors, but a transaction opened from `Db::pool()` can still reach the public `*_in_transaction` helpers; only the source-policy scan and the database fences catch that until BUZZ-254. **Behavior note:** block#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 admitted 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 admitted transaction. ### Related issue [BUZZ-175](https://linear.app/squareup/issue/BUZZ-175). Caller-migration stage following block#7706 (merged); rebased onto `main` at `5173fad66`. Follow-up: [BUZZ-254](https://linear.app/squareup/issue/BUZZ-254) makes event-write helpers require a type-level admitted transaction. **Known debt, out of scope:** `create_channel`, `update_channel`, and `insert_thread_metadata` write channel and thread rows (not events) outside the admitted constructor. ### Testing At `efe4a94623dd69973780a3c39f16dd972cf7cb76` on a cloud workstation: fmt; `buzz-db` clippy (all targets, `-D warnings`); `observability_source` 14/14; `just --dry-run test-unit` includes the new step. Reverting just the end-of-item check to an earlier rule makes a trailing-comment fixture fail. At `eaf190ae59d1595741c3d19dc4c23ce492b5f4d4` (the commit after it only changes this test file, `Justfile`, and `scripts/run-tests.sh`): full GitHub CI green; on the workstation, clippy for `buzz-db` and `buzz-relay`, ordinary `buzz-db` 154/154, `just test-unit` green, and the PostgreSQL lane 805 of 806 (exception below). An end-to-end run against a local relay and the built desktop app covered artifact accept, huddle join, and workflow deletion for both a healthy and a quiescing community, plus p-tag reaction rollback and replay of the same signed event. New tests: one fenced-community test per migrated path (artifact accept, huddle join, both workflow deletions) puts the community into `quiescing`, holds the lock the path takes next, and asserts rejection at entry with nothing stored; a DB-free test pins the non-canvas rejection. Exceptions: - `runtime::postgres_tests::writer_pool_rejects_non_read_committed_database_default` hangs on that workstation's local Postgres 16 at both this head and an earlier `main` base, `f0eb5575f` (2/2 runs each). The test, `Db::new`, and the pool setup are unchanged by this PR, and the test passes in GitHub CI. Live traces put the hang in the client, not the database: the 1 s acquire timeout stops firing after sqlx's `after_connect` rejection path under tokio's `current_thread` runtime. No database lock waits were observed. - Two `observability_source` assertions were already failing on `main`, unnoticed because no CI job ran that test binary: a stale `fetch_optional` pin after block#8005, and lease-seam attribution that now lives in the `bounded_serving_lease_sql` helper. Both are corrected here, and the binary now runs in `just test-unit`. Generated with Codex --------- Signed-off-by: tornquist <tornquist@squareup.com> Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-authored-by: Codex <noreply@openai.com> Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Summary
Buzz is moving database invariants from triggers and foreign keys into application-owned transaction protocols. This PR adds the first transition foundations while preserving all current database backstops.
The blast radius is limited to new opt-in writer helpers plus lock acquisition in deletion and replica probing. Existing trigger and GUC enforcement remains authoritative, so callers can migrate in later pull requests.
Related issue
BUZZ-175. This is the first implementation slice from the OSC migration plan.
Testing
No manual testing. CI covers the Rust checks and database tests added here.
Generated with Codex