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

[wpfleger96]

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.

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is a61239f0d8036aff58176f5c0ce7f080c66e21b7...304d5e6e6c28bbc0b97b950a0c0c0af425f3da62.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 304d5e6e6c28bbc0b97b950a0c0c0af425f3da62 to authorize a new review.
Any previous review applies only to its recorded range.

[wesbillman]

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

Changes requested

Reviewed head 3c428b59537a276ddeed4a2f73ab897f637f54b3 against base 5079c770fe30bb3d8204822ce6c2431eacac6d4b. Source-only review on Blox, with existing exact-head CI evidence; no checkout, build, test execution, or CI rerun.

P2: Use valid channel-bearing event reports in both legacy recovery regressions

The new pre/post-marker tests call e2e_report_pubkey (pre-marker fixture, post-marker fixture). That helper inserts a pubkey report without channel_id. Each test then clears both persisted enforcement fields to simulate an old-writer row, discarding the channel supplied directly to claim_report.

The real recovery worker must re-derive the channel from the report, so it receives None. Before the marker, the kick mutation fails its required-channel check; after the marker, the convergence gate rejects the unresolved context and leaves the action enforcing. Production HTTP rejects kicks on pubkey reports, so these fixtures do not model valid legacy kicks.

This is confirmed by the exact-head PostgreSQL job: legacy_kick_row_pre_marker_recovers_via_worker fails at line 9271 with actual failed; legacy_kick_row_post_marker_recovers_via_worker fails at line 9527 with actual enforcing; both expect succeeded. These newly introduced failures break the PostgreSQL lane and leave the upgrade contract without the intended regression coverage.

Required fix: construct both fixtures from valid event reports, with a stored event author and channel_id on the report, then clear the persisted enforcement pair. Preserve the assertions covering pre/post-marker convergence, cache/subscription eviction, workflow disable, report/action state, and outbox effects. Both corrected regressions should pass in the existing PostgreSQL CI lane; do not relax their success assertions or make production accept invalid pubkey kicks to satisfy them.

Prior feedback and scope

The earlier production NULL/NULL-context defect is addressed for derivable legacy rows: the worker re-derives the context and the convergence gate now accepts that fallback. The failures above establish broken fixtures, not that the production fallback is absent. The moved live-effects test also passes in this CI job. The held re-add fence, same-connection workflow disable, and kick-only recovery branch preserving delete targets have no additional blocker in the reviewed paths.

Cross-pod immediate subscription eviction is not established by this change; that retained limitation and associated wording are non-blocking here. The separate replica-fence/desktop failures and DCO status are not attributed to this patch or added to this finding’s exit criteria.

addressed

[wesbillman]

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

Source re-review clear

Reviewed head 304d5e6e6c28bbc0b97b950a0c0c0af425f3da62 against base a61239f0d8036aff58176f5c0ce7f080c66e21b7. The P2 from review 5269373450 is resolved. No new actionable blocker found. This is a review comment, not approval.

• Fix verified: both legacy-kick regressions now use e2e_event_report_with_author, which supplies the stored event author, report channel, and target membership needed by real recovery. Both still clear the persisted enforcement pair, exercise their distinct pre/post-marker paths through recover_one, and retain the action-success, cache-eviction, subscription-eviction, and workflow-disable assertions. Production was not broadened to accept invalid pubkey-report kicks. Report resolution and outbox insertion were verified in the atomic finalization source; these two tests do not directly assert those rows, and no such assertions were removed.
• Delta and compatibility: accounted for the rebased history with tree comparisons. Eight of the ten PR-owned paths are unchanged since the prior review; changes are the two fixture replacements and a migration-number comment correction. Reviewed relevant main drift and the recovery/finalization contracts. The held re-add fence, legacy fallback, and kick-only worker branch retain their previously reviewed behavior. Cross-pod immediate eviction and existing missing-author limitations remain outside this fix’s exit criteria.
• Validation: PostgreSQL CI job 106449952986 ran merge 980db89f411dcd28b07490974e4e77416e96d20b of this exact head/base: both named legacy recovery regressions PASS, with 406/406 tests passing. The overall check snapshot had 52 successful, 27 skipped, and one cancelled Codex security-review check; that cancellation is not a security pass. Review source work stayed on pinned Blox, with applicable exact-base guidance and independent metadata review; no checkout, local tests, PR-code execution, or CI rerun.