4 Commits

Author	SHA1	Message	Date
Luke Tornquist	52d13efc08	fix(buzz-db): delete workflow and membership children explicitly (#8212) ... ## What Workflow deletion and relay-member removal relied on `ON DELETE CASCADE` to remove child rows. Both paths now delete their own children inside the same transaction, so a later migration can drop those foreign-key actions without changing behavior. **Workflow deletion** (`store/workflow/deletion.rs`, the coordinate path used by NIP-09 deletes of workflow definitions): - Locks the target workflow row with `SELECT … FOR UPDATE`, under the same owner and coordinate predicate as before. A concurrent run or fire insert has to lock that row for its foreign key, so the child deletes see every committed child. - Deletes scheduled fires first. A new fire has to lock the workflow row, and a cron attach locks the fire before the run, so deleting fires before the run lock keeps the lock order consistent and avoids a deadlock. - Then locks the workflow's runs `FOR UPDATE` in `id` order. An approval that names another workflow but references one of these runs key-share locks only the run, so the workflow lock alone doesn't exclude it. - `delete_workflow_with_children` then deletes approvals (for the workflow or any of its runs), then runs, then the workflow, and returns its `channel_id`. **Relay-member removal** (`store/relay_members.rs`): `remove_relay_member` and `remove_relay_member_if_role` share a new `delete_member_with_acceptances` helper. It locks the member row under the existing predicate (any non-owner, or an exact role), deletes `join_policy_acceptances`, then deletes the member, in one transaction. The `RemoveResult` outcomes are unchanged. **Removed:** `delete_workflow` (no callers) and `delete_workflow_for_owner` (test-only), plus their `Db` wrappers. The community-confinement test now drives the production coordinate deletion. ## Tests - `test_support::retire_foreign_key_delete_actions` rewrites every `CASCADE`, `SET NULL` and `SET DEFAULT` foreign key in the test's own per-test database as `NO ACTION`, which is the target schema of the follow-up migration. It asserts that none remain. All triggers stay live, including foreign-key checks and the community write fence. It refuses to run unless `current_database()` has the per-test wrapper's exact shape (`buzz_nt_` plus 24 hex digits), which excludes the run's `_desired` template. It checks the name only. - `workflow_deletion_removes_children_without_cascades` and `member_removal_deletes_acceptances_without_cascades` run the production paths on that schema and require every child row to be gone. One approval references another workflow but this workflow's run, which pins the `run_id` branch. The owner, role-mismatch and not-found outcomes still hold. - `workflow_deletion_waits_for_and_removes_concurrent_run` and `member_removal_waits_for_and_removes_concurrent_acceptance` hold a child insert open in a second transaction, wait until the removal is blocked on a lock, and then commit. The removal must succeed with no children left. - `workflow_deletion_waits_for_concurrent_fire_attach_without_deadlock` holds a fire row the way the cron attach does, then attaches a run to it while the deletion waits. Both must succeed. - `workflow_deletion_waits_for_and_removes_concurrent_cross_workflow_approval` does the same with an approval for another workflow that references this workflow's run. - The bridge test schema now includes `join_policy_acceptances`. ## What got simpler Two delete paths no longer depend on implicit constraint actions, and two unused workflow-delete functions are gone. This is the first step of retiring the `CASCADE` and `SET NULL` foreign keys. **Identified debt (later PRs):** `channel_members` and `personal_read_frontiers` (no hard-delete path found yet for their parents), the deletion `manifest_keys` cascade, the `SET NULL` actions on `executor_heartbeats` and `product_feedback`, and the `users` self-reference. That last one is a composite `(community_id, agent_owner_pubkey)` `ON DELETE SET NULL` that would try to null the `NOT NULL` `community_id`. Also, a scheduled fire for workflow B can link to a run of A (that FK is already `NO ACTION`), which would block A's run delete; the scheduler always attaches a run to its own workflow's fire, so this needs same-workflow enforcement in the cascade work. ## Verification Head `081be4c0d`, tree `ab649487`, on a Linux cloud workstation: fmt, buzz-db and workspace clippy, source policy and buzz-db lib pass; the six new tests pass; buzz-db PostgreSQL passes 490/490 and buzz-relay PostgreSQL 378/378. A sixth mutation, moving the fire delete back after the run lock, makes `workflow_deletion_waits_for_concurrent_fire_attach_without_deadlock` fail with `40P01` (deadlock detected). Previous head: Head `787c7c42d`, tree `6b9cf5d5`, on a Linux cloud workstation: fmt, buzz-db and workspace clippy, source policy and buzz-db lib pass; the new tests pass 5/5; buzz-relay PostgreSQL passes 378/378; buzz-db PostgreSQL passes 488/489, the one failure being `parent_contention_times_out_without_blocking_the_other_table` in `store/partition/`, which this PR doesn't touch and which passes on its own at the same tree. A fifth mutation, removing the run lock, makes the new cross-workflow approval test fail with the reviewer's exact 23503 on `workflow_approvals_community_id_run_id_fkey`. Previous head `00cba2f4a`, tree `2f89fed4`, base `e9269cbdf`. Results from a Linux cloud workstation: - fmt, buzz-db and workspace clippy, the source policy and buzz-db lib all pass. - The buzz-db PostgreSQL suite passes 488/488. - The buzz-relay PostgreSQL suite passes 378/378. Each of four mutations makes exactly one of the new tests fail with the foreign-key violation you'd expect (23503): - swapping the fires and runs deletes - deleting approvals by `workflow_id` only - removing the workflow row lock - removing the member row lock GitHub CI is green at this head. buzz-review-completed --------- Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>	2026-10-14 08:27:26 -04:00
Luke Tornquist	f8bf2c529c	refactor(db): type-level admitted event-write transactions (AdmittedTx) (#8130)	2026-10-08 08:47:07 -04:00
Luke Tornquist	d0fd96fca6	BUZZ-175: Route serving event writes through the tenant lock (#7828) ... ## 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>	2026-10-06 10:24:41 -04:00
Wes	5079c770fe	fix(workflows): make deletion persistent and retryable (#7735) ... ## Summary Fix workflow deletion leaving its definition queryable, and make public deletion acceptance agree atomically with executable/definition state. - Delete the executable workflow and soft-delete its definition under the existing coordinate lock, in the same transaction as kind-5 acceptance and mention indexing. Failed deletion leaves no public accepted event; identical retries can succeed. Legacy already-stored failed requests still repair. - Choose one committed dispatch/audit owner for concurrent identical requests. Completed duplicates remain silent. Preserve owner/community isolation, newer revisions, orphan cleanup, and intentional backdated recreation. - Align Desktop confirmation and architecture documentation with recreation semantics. Add deterministic PostgreSQL, live WebSocket, and Desktop regressions. No schema migration or new retry/delivery subsystem. ## Current validation Head: `40d86ddab0853ffb292316653b22c3fd4f983f0c`. - **Exact clean head:** live relay workflow-deletion E2E **4/4 passed**. All applicable normal pre-push hooks passed: Rust unit suites, full Desktop unit suite, Desktop lint/typecheck, Tauri clippy/tests, and file-size/branch checks. No hook bypass. - **Pre-commit implementation tree, then committed at this head:** full discovered DB/relay PostgreSQL lane **351/351 passed**; built Desktop deletion Playwright smoke **3/3 passed** (success, failure preserving editor/draft, corrected confirmation copy). - **Deterministic mutation evidence:** unchanged previous relay `659b79282` fails both new regressions: concurrent identical requests produce two normal/dispatch-owner responses, and rejected deletion leaves one public event. Both controls pass. Fixed relay passes all four. - **Independent source review:** transaction/authorization/lock-order and regression/cleanup lanes clear. Reviewers did not run tests independently. ## Honest limits / merge gates The broad pre-commit DB/relay/test-client selection passed **1,215/1,216**; the untouched mesh-demo echo test returned HTTP 504 instead of 200. It remains outside this repair and is not counted green. Initial hook attempts also exposed an untouched Desktop inventory ordering assertion and unidentified ACP/agent test failures; standalone package reruns and the final complete hooks passed without changing those packages. Causes are not established. The previous Desktop CI shard failed before tests on a Hermit `just` download (HTTP 500). New-head CI/security review and reviewer disposition remain required. No deployment, fresh interactive Desktop session, full local `just ci`, or multi-replica runtime validation claimed. Postgres/Redis validation used task-owned local containers, fresh identities, and disposable fixtures. ## Compatibility No automatic cleanup of existing workflows: repeating deletion repairs definitions left by the old path. Canonical UUID coordinates and legacy name fallback (`LIMIT 1` for duplicate names) are unchanged. Backdated recreation remains intentional per [Tyler’s decision](https://github.com/block/buzz/pull/7735#issuecomment-5729881295), not an irreversible tombstone. Mention-index failure now rolls back the complete deletion rather than best-effort accepting partial work. Detailed reviewer disposition and evidence: https://github.com/block/buzz/pull/7735#issuecomment-5737367825 --------- Signed-off-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Co-authored-by: Mordecai <f314b6033b6e13df94c8e9e1187d3666b6b9ce54e66ee986e31cdaa909e8dcbc@buzz.block.builderlab.xyz>	2026-09-21 08:37:14 -06:00