fix(relay): fire kick live side effects at convergence; persist target; fence re-add race with held lock (#7298)

## 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>
This commit is contained in:
Will Pfleger
2026-09-21 21:52:54 +03:00
committed by GitHub
parent 6a86531ac9
commit ce41be9c4b
10 changed files with 2613 additions and 45 deletions

View File

@@ -1806,6 +1806,14 @@ CREATE TABLE relay_admin_actions (
-- retries and lets the recovery worker claim/re-drive stranded actions.
action_lease_token UUID,
action_lease_expires_at TIMESTAMPTZ,
-- Authoritative enforcement target (migration 0047): persisted at claim time
-- so crash-recovery can fire live side effects without re-deriving from mutable
-- sources. enforcement_target_pubkey is the resolved target pubkey bytes for
-- kick/ban/timeout actions; NULL for event/blob targets. enforcement_channel_id
-- is the channel targeted by kick actions; NULL for community-wide actions.
enforcement_target_pubkey BYTEA
CHECK (enforcement_target_pubkey IS NULL OR length(enforcement_target_pubkey) = 32),
enforcement_channel_id UUID,
created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
updated_at TIMESTAMPTZ NOT NULL DEFAULT now(),
-- Report-scoped idempotency: one action per (report, request_id).