mirror of
https://github.com/block/buzz.git
synced 2026-10-27 04:30:52 +00:00
ce41be9c4b
## What
Ensures admin kick live side effects (subscription eviction + workflow
disable) fire correctly on every path — fresh HTTP, crash recovery, and
upgrade recovery from pre-migration rows — without a race window where a
concurrent legitimate re-add can be revoked.
### Finding 1 — Lock held through effects
Replace `verify_member_still_removed` (which acquired + released the
advisory lock before returning) with `membership_removal_fence`, a RAII
guard that keeps the `pg_advisory_xact_lock` alive until the caller
drops it.
Update `apply_kick_live_side_effects` to use the fence on every path
(fresh and recovery), holding it through eviction and workflow-disable.
Remove the `is_recovery` parameter entirely.
**The bug:** the old code released the lock before returning a bool; a
concurrent `add_member` could commit between the check and the effects,
silently evicting a re-added member.
**New guarantee:** any `add_member` either serializes before the fence
(fence observes `removed_at IS NULL`, skips effects) or waits until
after effects complete.
### Finding 2 — Persisted-context path scoped to kick only
Gate the persisted-context branch in `admin_action_worker.rs` on `action
== "kick"`. Non-kick actions (ban, timeout, delete) always re-derive
from the report.
**The bug:** any action with `enforcement_target_pubkey` set took the
persisted-context branch, which hard-coded `target_event_id = None`. A
stranded delete either failed pre-marker recovery ("delete requires
target_event_id") or finalized post-marker without the tombstone outbox
row.
### Finding 3 — Legacy kick row upgrade recovery (Carl's
CHANGES_REQUESTED)
Migration 0047 (renumbered from 0045 to avoid collision with two main
merges: 0045_retain_push_revocation_tombstones and
0046_storage_accounting_snapshots) adds
`enforcement_target_pubkey`/`enforcement_channel_id` without
backfilling.
**The bug:** the convergence gate read only the persisted columns and
rejected NULL/NULL rows as corrupt, causing every recovery retry to fail
forever. A cancellation cannot release a post-mutation action, so these
rows remain permanently stuck.
**Fix:** when persisted columns are absent, fall back to the function
parameters (which the recovery worker re-derives from the report row).
Error only when both sources are absent (genuinely unresolvable). This
preserves new-writer safety while allowing pre-migration rows to
converge.
An old writer in a rolling deployment can also create NULL rows after
the DDL applies; the same fallback handles these.
### Finding 4 — Test moved to PG lane
`kick_live_side_effects_clears_membership_cache_and_evicts_subscription`
was in the infra-free unit lane but called `membership_removal_fence`,
which requires a real Postgres connection. Moved to the PG fixture lane
(`#[ignore = "requires Postgres"]`) with a real member row so the fence
query sees the expected `removed_at` state.
## Tests
- **`crash_recovery_redrive_fires_kick_live_side_effects`** (existing):
re-drive via real recovery worker path, asserts all three side effects.
-
**`crash_recovery_after_readd_preserves_membership_subscriptions_and_workflows`**
(updated): uses real `add_member` (not direct SQL) — re-add completes
before fence is acquired.
- **`membership_removal_fence_blocks_concurrent_add_member`**
(existing): holds the fence explicitly, spawns `add_member`, asserts it
blocks until the fence is dropped.
- **`stranded_delete_pre_marker_recovers_via_worker`** (existing):
stranded delete (crash before mutation) converges via `recover_one` to
succeeded.
- **`legacy_kick_row_pre_marker_recovers_via_worker`** (new): old-writer
shaped row (NULL columns, no step_marker) converges through the full
state machine via recovery worker. Falsifiable: reverting the
convergence-gate fallback leaves the action stuck in enforcing.
- **`legacy_kick_row_post_marker_recovers_via_worker`** (new): same but
with step_marker set (post-marker crash). Falsifiable: same revert.
-
**`kick_live_side_effects_clears_membership_cache_and_evicts_subscription`**
(moved to PG lane): verifies cache eviction and subscription removal
against a real member row.
## CI
`Rust / Rust Lint` and `Windows Rust` failures in the prior run were
main-inherited (same lanes fail on main at the prior base). The branch
is now at main's current head.
---------
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
Co-authored-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
18 lines
937 B
SQL
18 lines
937 B
SQL
-- Persist the authoritative enforcement target with the action row so that
|
|
-- crash-recovery can fire live side effects without re-deriving from mutable
|
|
-- sources (event rows, report rows) that may have changed or been purged.
|
|
--
|
|
-- enforcement_target_pubkey: the resolved target pubkey bytes at claim time,
|
|
-- when the action targets a pubkey (kick/ban/timeout). NULL for event/blob
|
|
-- targets where no pubkey is derived.
|
|
-- enforcement_channel_id: the channel the enforcement targets. Populated for
|
|
-- kick actions. NULL for community-wide actions.
|
|
--
|
|
-- Both columns mirror the values passed to claim_report's target_pubkey and
|
|
-- channel_id parameters. They are written once at claim time and never updated.
|
|
|
|
ALTER TABLE relay_admin_actions
|
|
ADD COLUMN enforcement_target_pubkey BYTEA
|
|
CHECK (enforcement_target_pubkey IS NULL OR length(enforcement_target_pubkey) = 32),
|
|
ADD COLUMN enforcement_channel_id UUID;
|