From ba34fb292c6533535911e0db6a8df4a2256e8062 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Thu, 3 Sep 2026 18:12:36 -0400 Subject: [PATCH 01/13] fix(relay): fire kick live side effects at convergence, persist target, fence re-add race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes closing the live-effects gap on the admin kick path: 1. Convergence placement: move apply_kick_live_side_effects from inside the is_none() mutation block to a shared convergence point after it. The fresh HTTP path (Committed), concurrent-driver path (AlreadyCommitted → reload), and crash-recovery path all pass through the convergence point. 2. Persist enforcement context: add enforcement_target_pubkey / enforcement_channel_id to relay_admin_actions (migration 0045). These columns are written once at claim time and read at recovery, so recovery no longer re-derives the kick target from mutable report/event rows that may have been purged. Missing persisted context at convergence is now an invariant error (not warn+skip). AdminActionRecord gains both fields; claim_report and row_to_action updated. Recovery worker reads persisted fields and falls back to report re-derivation only for pre-migration rows where both columns are NULL. 3. Re-add race fence: add verify_member_still_removed in buzz-db, which acquires the same pg_advisory_xact_lock as add_member and checks removed_at IS NOT NULL. apply_kick_live_side_effects accepts an is_recovery flag; on the recovery path eviction and workflow-disable are gated on this check. Cache invalidation remains unconditional (stale-positive is always safe to drop). Tests: crash-recovery seam test moved from report_resolution.rs generic mod tests to api/admin/mod.rs postgres_tests (discoverable by check-postgres-test-discovery.py), rewritten to use shared e2e helpers, extended to assert workflow disable, and the re-add race test added. Purge-test seeding/assertions extended to cover all three live side effects (not just finalize state). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/runtime/migration.rs | 2 +- crates/buzz-db/src/store/channel_members.rs | 70 +++ .../buzz-db/src/store/relay_admin_actions.rs | 18 +- crates/buzz-relay/src/api/admin/mod.rs | 566 +++++++++++++++++- .../src/handlers/admin_action_worker.rs | 58 +- .../src/handlers/report_resolution.rs | 173 +++++- .../buzz-relay/src/handlers/side_effects.rs | 79 +++ migrations/0045_relay_admin_action_target.sql | 17 + 8 files changed, 944 insertions(+), 39 deletions(-) create mode 100644 migrations/0045_relay_admin_action_target.sql diff --git a/crates/buzz-db/src/runtime/migration.rs b/crates/buzz-db/src/runtime/migration.rs index 59015125042..ecd48bef598 100644 --- a/crates/buzz-db/src/runtime/migration.rs +++ b/crates/buzz-db/src/runtime/migration.rs @@ -702,7 +702,7 @@ mod postgres_tests { let mut migrations: Vec<_> = MIGRATOR.iter().collect(); migrations.sort_by_key(|migration| migration.version); - assert_eq!(migrations.len(), 44); + assert_eq!(migrations.len(), 45); assert_eq!(migrations[0].version, 1); assert_eq!(&*migrations[0].description, "initial schema"); assert!(migrations[0] diff --git a/crates/buzz-db/src/store/channel_members.rs b/crates/buzz-db/src/store/channel_members.rs index 8280ca01f82..0227a0a3984 100644 --- a/crates/buzz-db/src/store/channel_members.rs +++ b/crates/buzz-db/src/store/channel_members.rs @@ -687,6 +687,59 @@ pub async fn is_member( Ok(cnt > 0) } +/// Verify that a member is still removed under the channel-membership lock, +/// serializing with any concurrent [`add_member`] call on the same channel. +/// +/// Returns `true` when `removed_at IS NOT NULL` for `(community_id, channel_id, +/// pubkey)` — i.e., the removal that crash-recovery is re-applying is still the +/// current state and no re-add has reversed it. +/// +/// Uses the same `pg_advisory_xact_lock` key as [`add_member`] so that a +/// concurrent re-add either sees this check complete (and then sets `removed_at +/// = NULL` after recovery finishes) or serializes before it (in which case this +/// check observes the re-add and returns `false`, preventing stale eviction). +/// +/// Designed for use in crash-recovery before firing subscription eviction and +/// workflow disablement: cache invalidation is always safe (it only drops a +/// stale positive), but eviction and workflow-disable must not target a member +/// who was legitimately re-added after the kick committed. +pub async fn verify_member_still_removed( + pool: &PgPool, + community_id: CommunityId, + channel_id: Uuid, + pubkey: &[u8], +) -> Result { + let connection = crate::observability::acquire_writer( + pool, + crate::observability::WriterOperation::Authorization, + ) + .await?; + let mut tx = sqlx::Transaction::begin(connection, None).await?; + acquire_channel_membership_lock(&mut tx, community_id, channel_id).await?; + + let row = sqlx::query( + "SELECT removed_at FROM channel_members \ + WHERE community_id = $1 AND channel_id = $2 AND pubkey = $3", + ) + .bind(community_id.as_uuid()) + .bind(channel_id) + .bind(pubkey) + .fetch_optional(&mut *tx) + .await?; + + // Row absent → never joined or hard-deleted; treat as removed (safe to skip + // eviction for someone who isn't and wasn't a member). + let still_removed = match row { + None => true, + Some(r) => { + let removed_at: Option> = r.try_get("removed_at")?; + removed_at.is_some() + } + }; + tx.rollback().await?; + Ok(still_removed) +} + /// Return which of the given (channel, pubkey) combinations are active /// memberships, restricted to non-deleted channels — one statement for any /// batch size (T2b). Semantics per pair match [`is_member`]. @@ -1364,6 +1417,23 @@ impl Db { is_member(&self.pool, community_id, channel_id, pubkey).await } + /// Returns `true` when the member row still has `removed_at IS NOT NULL`, + /// holding the channel-membership advisory lock so the check serializes + /// with concurrent [`add_member`] calls. + /// + /// Use this in crash-recovery before firing subscription eviction and + /// workflow disablement to guard against the kick-commit → re-add → recover + /// race (IMPORTANT 3). + #[datastore_span(name = "verify_member_still_removed", system = "postgresql")] + pub async fn verify_member_still_removed( + &self, + community_id: CommunityId, + channel_id: Uuid, + pubkey: &[u8], + ) -> Result { + verify_member_still_removed(&self.pool, community_id, channel_id, pubkey).await + } + /// Return the active (channel, pubkey) membership pairs among the given /// sets, in one statement. #[datastore_span(name = "membership_pairs", system = "postgresql")] diff --git a/crates/buzz-db/src/store/relay_admin_actions.rs b/crates/buzz-db/src/store/relay_admin_actions.rs index 7662077911d..3401a1fcb80 100644 --- a/crates/buzz-db/src/store/relay_admin_actions.rs +++ b/crates/buzz-db/src/store/relay_admin_actions.rs @@ -47,6 +47,12 @@ pub struct AdminActionRecord { pub cancelled_by: Option>, /// Error from the last failure, if any. pub error_message: Option, + /// Authoritative target pubkey persisted at claim time (kick/ban/timeout pubkey targets). + /// `None` for event/blob targets or actions with no pubkey target. + pub enforcement_target_pubkey: Option>, + /// Authoritative channel persisted at claim time (kick actions). + /// `None` for community-wide actions. + pub enforcement_channel_id: Option, /// Row creation time. pub created_at: DateTime, /// Row last-updated time. @@ -241,6 +247,7 @@ pub async fn claim_report( r#" SELECT id, report_id, report_community_id, request_id, actor_pubkey, actor_role, action, reason, timeout_until, state, step_marker, cancelled_by, error_message, + enforcement_target_pubkey, enforcement_channel_id, created_at, updated_at FROM relay_admin_actions WHERE report_community_id = $1 AND report_id = $2 AND request_id = $3 @@ -269,10 +276,11 @@ pub async fn claim_report( r#" INSERT INTO relay_admin_actions ( report_id, report_community_id, request_id, actor_pubkey, actor_role, - action, reason, timeout_until, state - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, 'pending') + action, reason, timeout_until, state, enforcement_target_pubkey, enforcement_channel_id + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, 'pending', $9, $10) RETURNING id, report_id, report_community_id, request_id, actor_pubkey, actor_role, action, reason, timeout_until, state, step_marker, cancelled_by, error_message, + enforcement_target_pubkey, enforcement_channel_id, created_at, updated_at "#, ) @@ -284,6 +292,8 @@ pub async fn claim_report( .bind(action) .bind(reason) .bind(timeout_until) + .bind(target_pubkey) + .bind(channel_id) .fetch_one(&mut *tx) .await?; @@ -1242,6 +1252,7 @@ pub async fn get_action(pool: &PgPool, action_id: Uuid) -> Result Result { step_marker: row.try_get("step_marker")?, cancelled_by: row.try_get("cancelled_by")?, error_message: row.try_get("error_message")?, + enforcement_target_pubkey: row.try_get("enforcement_target_pubkey")?, + enforcement_channel_id: row.try_get("enforcement_channel_id")?, created_at: row.try_get("created_at")?, updated_at: row.try_get("updated_at")?, }) diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 19f2153b95b..eaa855628b3 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -6029,17 +6029,15 @@ mod postgres_tests { } #[tokio::test] - #[ignore = "requires Postgres — stranded kick re-drive converges after mid-flight event purge"] + #[ignore = "requires Postgres — stranded kick re-drive converges and fires live side effects after mid-flight event purge"] async fn worker_redrive_of_event_kick_converges_after_event_purged_mid_flight() { // Criterion 3 + Paul's mid-flight edge: a kick on an event report claims, // commits its mutation+marker, then the event row is HARD-purged before a - // stranded re-drive. The worker re-derives from the (now author-less) - // report row; because the target is not persisted, the pubkey re-derives - // to None. The action is already past `mutation_committed`, so the driver - // skips the mutation and finalizes: action → succeeded, report → resolved. - // The kick already landed (member removed at commit time); only the - // system_message artifact (which needs the target pubkey) is dropped — - // the action does NOT strand permanently. + // stranded re-drive. Because target_pubkey and channel_id are now persisted + // in relay_admin_actions at claim time (migration 0045), recovery no longer + // re-derives them from the mutable event/report rows. All three live side + // effects fire on re-drive: cache invalidation, subscription eviction, and + // workflow disablement. The action converges to succeeded and report → resolved. let pool = e2e_pool().await; let (community_id, _host) = e2e_community(&pool, "worker-midflight-purge").await; let author = vec![0x71u8; 32]; @@ -6118,6 +6116,53 @@ mod postgres_tests { "kick must commit its mutation + marker before the crash" ); + // Seed in-process state that simulates a live session after the crash: + // a stale membership cache entry and an active channel subscription. + // These are what the crash-recovery live side effects must clear. + let cid = buzz_core::CommunityId::from_uuid(community_id); + let tenant = e2e_tenant(community_id, "worker-midflight-purge.example"); + state + .membership_cache + .insert((cid, channel_id, author.clone()), true); + let conn_id = uuid::Uuid::new_v4(); + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); + state.conn_manager.register( + conn_id, + tx, + ctrl_tx, + None, + tokio_util::sync::CancellationToken::new(), + cid, + std::sync::Arc::new(std::sync::atomic::AtomicU8::new(0)), + std::sync::Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())), + 3, + ); + state + .conn_manager + .set_authenticated_pubkey(conn_id, author.clone()); + state.sub_registry.register_channels_scoped( + cid, + conn_id, + "purge-test-sub".to_string(), + vec![nostr::Filter::new()], + vec![channel_id], + ); + + // Seed an enabled workflow owned by the kicked user so we can verify disable. + let workflow_id = state + .db + .create_workflow( + cid, + Some(channel_id), + &author, + "purge-test-workflow", + r#"{"kind":"workflow"}"#, + &[0u8; 32], + ) + .await + .expect("create workflow"); + // Mid-flight disappearance: HARD-purge the stored event (community purge), // then expire the lease so the recovery worker can re-claim. sqlx::query("DELETE FROM events WHERE community_id = $1 AND id = $2") @@ -6135,7 +6180,9 @@ mod postgres_tests { .await .expect("expire lease"); - // Re-derive now yields no author — the exact divergence Paul flagged. + // Verify: re-deriving from the now-author-less report yields None (event + // is purged), but recovery does NOT depend on re-derivation — it reads + // the persisted enforcement_target_pubkey / enforcement_channel_id columns. let report_after = state .db .admin_get_report(report_id) @@ -6145,7 +6192,10 @@ mod postgres_tests { let (target_after, _e) = crate::handlers::report_resolution::derive_enforcement_target(&report_after) .expect("derive after purge"); - assert_eq!(target_after, None, "author unresolvable after purge"); + assert_eq!( + target_after, None, + "confirm: re-derivation path yields None after purge (recovery must NOT use this path)" + ); // Re-drive through the REAL recovery worker entry point. let batch = buzz_db::relay_admin_actions::claim_stranded_action_batch( @@ -6162,8 +6212,41 @@ mod postgres_tests { .expect("stranded action must appear in batch"); crate::handlers::admin_action_worker::recover_one(&state, claim).await; + // ── Assertions: all three live side effects fired ───────────────────── // Convergence: action succeeded (marker was already committed), report - // resolved. No permanent strand despite the vanished target. + // resolved. Persisted target context means the event purge no longer + // prevents live side effects from firing. + + // 1. Membership cache cleared. + assert!( + state + .membership_cache + .get(&(cid, channel_id, author.clone())) + .is_none(), + "crash-recovery must clear membership cache after purged-event kick re-drive" + ); + + // 2. Channel subscription evicted. + assert!( + !state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "crash-recovery must evict channel subscription after purged-event kick re-drive" + ); + + // 3. Workflow disabled. + let wf = state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get_workflow"); + assert!( + !wf.enabled, + "crash-recovery must disable target's workflows after purged-event kick re-drive" + ); + + // 4. DB state. let final_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) .await .expect("get_action") @@ -7602,4 +7685,465 @@ mod postgres_tests { "row claim token must belong to worker B (stale A token must not rewrite)" ); } + + // ── Kick live side effects: crash-recovery and re-add race ──────────────── + + /// Recovery seam: commit kick marker, simulate crash, re-drive via the real + /// recovery worker entry point and assert all three live side effects fire: + /// membership cache cleared, channel subscription evicted, workflow disabled. + /// + /// The test is falsifiable: deleting the convergence-point call to + /// `apply_kick_live_side_effects` in `drive_enforcement` would leave the + /// helper-direct test green but fail this test (recovery would finalize + /// without clearing cache, evicting, or disabling). + #[tokio::test] + #[ignore = "requires Postgres — crash recovery re-drive fires all three kick live side effects"] + async fn crash_recovery_redrive_fires_kick_live_side_effects() { + let pool = e2e_pool().await; + let (community_id, host) = e2e_community(&pool, "kick-crash-recovery").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0xAAu8; 32]; + let actor = vec![0xBBu8; 32]; + + // Seed channel + member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'recovery-test-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + + // Claim → enforcing → lease → kick + commit marker (simulates process + // that committed the kick but crashed before live side effects ran). + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + uuid::Uuid::new_v4(), + &actor, + "operator", + "kick", + None, + None, + "resolve:kick", + "relay_operator", + Some(&target), + None, + Some(channel_id), + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + let _ = buzz_db::relay_admin_actions::begin_enforcing(&pool, action_id) + .await + .expect("begin_enforcing"); + let lease_until = chrono::Utc::now() + chrono::Duration::seconds(60); + let lease_token = + match buzz_db::relay_admin_actions::acquire_action_lease(&pool, action_id, lease_until) + .await + .expect("acquire lease") + { + buzz_db::relay_admin_actions::LeaseResult::Acquired(t) => t, + other => panic!("expected Acquired, got {other:?}"), + }; + let kick_result = buzz_db::relay_admin_actions::execute_kick_with_marker( + &pool, + action_id, + lease_token, + cid, + channel_id, + &target, + &actor, + ) + .await + .expect("execute_kick_with_marker"); + assert!( + matches!( + kick_result, + buzz_db::relay_admin_actions::KickWithMarkerResult::Removed + ), + "kick must commit before the simulated crash" + ); + + // Expire the lease so the recovery worker can re-claim. + sqlx::query( + "UPDATE relay_admin_actions SET action_lease_expires_at = $2, action_lease_token = NULL WHERE id = $1", + ) + .bind(action_id) + .bind(chrono::Utc::now() - chrono::Duration::seconds(300)) + .execute(&pool) + .await + .expect("expire lease"); + + // Build state with real DB. Seed stale in-process entries. + let state = state_from_pool(pool.clone()).await; + let tenant = e2e_tenant(community_id, &host); + + state + .membership_cache + .insert((cid, channel_id, target.clone()), true); + + let conn_id = uuid::Uuid::new_v4(); + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); + state.conn_manager.register( + conn_id, + tx, + ctrl_tx, + None, + tokio_util::sync::CancellationToken::new(), + cid, + std::sync::Arc::new(std::sync::atomic::AtomicU8::new(0)), + std::sync::Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())), + 3, + ); + state + .conn_manager + .set_authenticated_pubkey(conn_id, target.clone()); + state.sub_registry.register_channels_scoped( + cid, + conn_id, + "recovery-seam-sub".to_string(), + vec![nostr::Filter::new()], + vec![channel_id], + ); + + // Seed an enabled owned workflow; disable must be asserted after recovery. + let workflow_id = state + .db + .create_workflow( + cid, + Some(channel_id), + &target, + "recovery-test-workflow", + r#"{"kind":"workflow"}"#, + &[0u8; 32], + ) + .await + .expect("create workflow"); + + // Pre-conditions. + assert!( + state + .membership_cache + .get(&(cid, channel_id, target.clone())) + .is_some(), + "pre-condition: membership cache entry must exist before recovery" + ); + assert!( + state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "pre-condition: subscription must be registered before recovery" + ); + assert!( + state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "pre-condition: workflow must be enabled before recovery" + ); + + // Re-drive via the real recovery worker path. + let batch = buzz_db::relay_admin_actions::claim_stranded_action_batch( + &pool, + "e2e-kick-recovery", + chrono::Utc::now() + chrono::Duration::seconds(120), + 1000, + ) + .await + .expect("claim_stranded_action_batch"); + let claim = batch + .into_iter() + .find(|c| c.record.id == action_id) + .expect("stranded action must appear in batch"); + crate::handlers::admin_action_worker::recover_one(&state, claim).await; + + // ── Assertions ─────────────────────────────────────────────────────── + + // 1. Membership cache cleared. + assert!( + state + .membership_cache + .get(&(cid, channel_id, target.clone())) + .is_none(), + "crash-recovery re-drive must clear the membership cache" + ); + + // 2. Channel subscription evicted. + assert!( + !state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "crash-recovery re-drive must evict the kicked user's channel subscription" + ); + + // 3. Workflow disabled. + assert!( + !state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "crash-recovery re-drive must disable the kicked user's workflows" + ); + + // 4. Action reached succeeded. + let final_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("record must still exist"); + assert_eq!( + final_rec.state, "succeeded", + "action must reach succeeded after crash-recovery re-drive" + ); + } + + /// Re-add race: kick commits → crash → `add_member` re-adds → recovery fires. + /// + /// Asserts that after a post-kick re-add, the recovery worker does NOT evict + /// subscriptions or disable workflows for the now-valid member. Cache + /// invalidation (stale positive) is always safe and may still fire. + /// + /// This test is falsifiable: removing the `verify_member_still_removed` fence + /// from `apply_kick_live_side_effects` would cause this test to fail because + /// the re-added member's subscription and workflow would be wrongly revoked. + #[tokio::test] + #[ignore = "requires Postgres — post-kick re-add survives crash recovery"] + async fn crash_recovery_after_readd_preserves_membership_subscriptions_and_workflows() { + let pool = e2e_pool().await; + let (community_id, host) = e2e_community(&pool, "kick-readd-race").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0xCCu8; 32]; + let actor = vec![0xDDu8; 32]; + + // Seed channel + member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'readd-race-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + + // Claim → enforcing → kick + commit marker. + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + uuid::Uuid::new_v4(), + &actor, + "operator", + "kick", + None, + None, + "resolve:kick", + "relay_operator", + Some(&target), + None, + Some(channel_id), + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + let _ = buzz_db::relay_admin_actions::begin_enforcing(&pool, action_id) + .await + .expect("begin_enforcing"); + let lease_until = chrono::Utc::now() + chrono::Duration::seconds(60); + let lease_token = + match buzz_db::relay_admin_actions::acquire_action_lease(&pool, action_id, lease_until) + .await + .expect("acquire lease") + { + buzz_db::relay_admin_actions::LeaseResult::Acquired(t) => t, + other => panic!("expected Acquired, got {other:?}"), + }; + buzz_db::relay_admin_actions::execute_kick_with_marker( + &pool, + action_id, + lease_token, + cid, + channel_id, + &target, + &actor, + ) + .await + .expect("execute_kick_with_marker"); + + // Expire lease so recovery worker can reclaim. + sqlx::query( + "UPDATE relay_admin_actions SET action_lease_expires_at = $2, action_lease_token = NULL WHERE id = $1", + ) + .bind(action_id) + .bind(chrono::Utc::now() - chrono::Duration::seconds(300)) + .execute(&pool) + .await + .expect("expire lease"); + + // Re-add the member (simulates `add_member` re-granting access after kick). + sqlx::query( + "UPDATE channel_members SET removed_at = NULL, removed_by = NULL \ + WHERE community_id = $1 AND channel_id = $2 AND pubkey = $3", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("re-add member"); + + // Verify pre-condition: member is active again. + let still_member = state_from_pool(pool.clone()) + .await + .db + .is_member(cid, channel_id, &target) + .await + .expect("is_member"); + assert!( + still_member, + "pre-condition: member must be active after re-add" + ); + + // Build state with real DB. Register a live subscription and workflow + // for the re-added member — recovery must NOT revoke these. + let state = state_from_pool(pool.clone()).await; + let tenant = e2e_tenant(community_id, &host); + + let conn_id = uuid::Uuid::new_v4(); + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); + state.conn_manager.register( + conn_id, + tx, + ctrl_tx, + None, + tokio_util::sync::CancellationToken::new(), + cid, + std::sync::Arc::new(std::sync::atomic::AtomicU8::new(0)), + std::sync::Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())), + 3, + ); + state + .conn_manager + .set_authenticated_pubkey(conn_id, target.clone()); + state.sub_registry.register_channels_scoped( + cid, + conn_id, + "readd-race-sub".to_string(), + vec![nostr::Filter::new()], + vec![channel_id], + ); + + let workflow_id = state + .db + .create_workflow( + cid, + Some(channel_id), + &target, + "readd-race-workflow", + r#"{"kind":"workflow"}"#, + &[0u8; 32], + ) + .await + .expect("create workflow"); + + // Re-drive via the real recovery worker. + let batch = buzz_db::relay_admin_actions::claim_stranded_action_batch( + &pool, + "e2e-readd-race", + chrono::Utc::now() + chrono::Duration::seconds(120), + 1000, + ) + .await + .expect("claim_stranded_action_batch"); + let claim = batch + .into_iter() + .find(|c| c.record.id == action_id) + .expect("stranded action must appear in batch"); + crate::handlers::admin_action_worker::recover_one(&state, claim).await; + + // ── Assertions: re-added member's live session must survive ────────── + + // 1. Subscription NOT evicted: re-added member still has a live session. + assert!( + state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "recovery must NOT evict the subscription of a re-added member" + ); + + // 2. Workflow NOT disabled: re-added member's workflow remains active. + assert!( + state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "recovery must NOT disable workflows of a re-added member" + ); + + // 3. DB membership active after re-add (not further altered by recovery). + assert!( + state + .db + .is_member(cid, channel_id, &target) + .await + .expect("is_member"), + "recovery must NOT remove the re-added membership row" + ); + + // 4. Action still converges to succeeded (the kick itself already landed). + let final_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("exists"); + assert_eq!( + final_rec.state, "succeeded", + "action must reach succeeded even when re-add fence suppresses eviction" + ); + } } diff --git a/crates/buzz-relay/src/handlers/admin_action_worker.rs b/crates/buzz-relay/src/handlers/admin_action_worker.rs index 75f98417b4b..524aa5a2991 100644 --- a/crates/buzz-relay/src/handlers/admin_action_worker.rs +++ b/crates/buzz-relay/src/handlers/admin_action_worker.rs @@ -101,26 +101,43 @@ pub(crate) async fn recover_one(state: &Arc, claim: StrandedActionClai "Action recovery worker re-driving stranded action" ); - // Decode the target from the report row. - let report = match state.db.admin_get_report(rec.report_id).await { - Ok(Some(r)) => r, - Ok(None) => { - warn!(action_id = %action_id, "Action recovery: report not found"); - return; - } - Err(e) => { - warn!(action_id = %action_id, "Action recovery: report lookup failed: {e}"); - return; - } - }; - - let (target_pubkey_opt, target_event_id_opt) = - match crate::handlers::report_resolution::derive_enforcement_target_pub(&report) { - Ok(pair) => pair, - Err(e) => { - warn!(action_id = %action_id, "Action recovery: target derive failed: {e:?}"); - return; - } + // Use the target context persisted at claim time. These columns were added + // in migration 0045; existing rows that pre-date the migration will have + // NULL here, in which case we fall back to report re-derivation so that + // pre-migration stranded actions still converge. + let (target_pubkey_opt, target_event_id_opt, channel_id) = + if rec.enforcement_target_pubkey.is_some() || rec.enforcement_channel_id.is_some() { + // Persisted context available — use it unconditionally. + ( + rec.enforcement_target_pubkey.clone(), + None::>, + rec.enforcement_channel_id, + ) + } else { + // Pre-migration row or non-pubkey-targeted action: re-derive from report. + let report = match state.db.admin_get_report(rec.report_id).await { + Ok(Some(r)) => r, + Ok(None) => { + warn!(action_id = %action_id, "Action recovery: report not found"); + return; + } + Err(e) => { + warn!(action_id = %action_id, "Action recovery: report lookup failed: {e}"); + return; + } + }; + let (pk, eid) = + match crate::handlers::report_resolution::derive_enforcement_target_pub(&report) { + Ok(pair) => pair, + Err(e) => { + warn!( + action_id = %action_id, + "Action recovery: target derive failed: {e:?}" + ); + return; + } + }; + (pk, eid, report.report.channel_id) }; let timeout_until = rec.timeout_until; @@ -128,7 +145,6 @@ pub(crate) async fn recover_one(state: &Arc, claim: StrandedActionClai let reason = rec.reason.clone(); let actor_pubkey = rec.actor_pubkey.clone(); let report_id = rec.report_id; - let channel_id = report.report.channel_id; match crate::handlers::report_resolution::drive_enforcement_pub( state, diff --git a/crates/buzz-relay/src/handlers/report_resolution.rs b/crates/buzz-relay/src/handlers/report_resolution.rs index d004903bd84..97148a53c2e 100644 --- a/crates/buzz-relay/src/handlers/report_resolution.rs +++ b/crates/buzz-relay/src/handlers/report_resolution.rs @@ -379,7 +379,7 @@ struct EnforcementCtx<'a> { #[allow(clippy::too_many_arguments)] async fn drive_enforcement( state: &Arc, - _tenant: &TenantContext, + tenant: &TenantContext, community_id: buzz_core::tenant::CommunityId, report_id: Uuid, action: &str, @@ -451,6 +451,7 @@ async fn drive_enforcement( } // Run mutation only if step marker is not yet committed. + let step_marker_was_set = rec.step_marker.is_some(); if rec.step_marker.is_none() { // Acquire exclusive action lease before running the mutation. Two // concurrent HTTP retries with the same request_id would both reach @@ -564,8 +565,9 @@ async fn drive_enforcement( match mutation_result { Ok(MutationOutcome::AlreadyCommitted) => { - // step_marker already set by a concurrent driver; - // reload and advance to finalization. + // step_marker already set by a concurrent driver. Reload and + // advance to finalization; live side effects fire at the + // convergence point below (after the is_none block). rec = state .db .get_admin_action(action_id) @@ -589,7 +591,8 @@ async fn drive_enforcement( ))); } Ok(MutationOutcome::Committed) => { - // Marker committed. Fall through to finalization below. + // Marker committed. Fall through to the convergence point + // below for live side effects and finalization. } Err(e) => { if failure_lease_lost { @@ -607,6 +610,50 @@ async fn drive_enforcement( } } } + // ── Convergence point ──────────────────────────────────────────────── + // Reached on every path where the step marker is (or was just) committed: + // the fresh HTTP path (Committed above), a concurrent-driver path + // (AlreadyCommitted → reload → loop reaches here with marker set), and + // the crash-recovery path (process died after DB commit but before live + // effects; recovery worker re-enters here directly with marker set). + // + // Live side effects for kick use the target context persisted at claim + // time (enforcement_target_pubkey / enforcement_channel_id) rather than + // the function parameters, which on the recovery path are re-derived from + // mutable sources that may have changed or been purged since the kick + // committed. Missing persisted context is an invariant failure: the + // INSERT that claimed the action required both values and stored them; if + // they are absent the row is corrupt and we must not silently succeed. + // + // On the crash-recovery path only (marker was already set when we entered + // this loop iteration), eviction and workflow-disable are gated behind + // verify_member_still_removed so a kick-commit → re-add → recover race + // does not revoke a legitimately restored membership. Cache invalidation + // is unconditional because stale-positive is always safe to drop. + if action == "kick" { + match ( + rec.enforcement_target_pubkey.as_deref(), + rec.enforcement_channel_id, + ) { + (Some(target), Some(ch)) => { + crate::handlers::side_effects::apply_kick_live_side_effects( + tenant, + state, + ch, + target, + step_marker_was_set, + ) + .await; + } + _ => { + return Err(ResolutionError::Internal(format!( + "kick action {action_id} reached convergence with missing \ + enforcement_target_pubkey or enforcement_channel_id — \ + action row is corrupt; refusing to finalize as succeeded" + ))); + } + } + } // Finalize: action → succeeded, report → resolved, outbox rows created. // Requires step_marker = 'mutation_committed' AND active_action_id = this action. let finalized = state @@ -1023,4 +1070,122 @@ mod tests { "worker re-derive must match the HTTP claim derivation exactly" ); } + + /// Verify that `apply_kick_live_side_effects` drops the membership cache + /// entry and evicts the live channel subscription for the kicked user. + /// + /// Setup: + /// 1. Seed the membership cache with `true` so the cache claims the target + /// is still a member. + /// 2. Register a connection authenticated as the target pubkey and add a + /// channel-scoped subscription for them. + /// 3. Call `apply_kick_live_side_effects`. + /// + /// Assertions: + /// - The membership cache entry is gone (cache returns `None`). + /// - The channel subscription index no longer lists the connection. + /// + /// Redis-dependent work inside the helper (cross-pod cache invalidation + /// publish, pubsub topic release) hits an intentionally unreachable endpoint + /// and is silently dropped — this mirrors the production "best-effort" + /// contract and does not affect the in-process assertions. + #[tokio::test] + async fn kick_live_side_effects_clears_membership_cache_and_evicts_subscription() { + use buzz_core::tenant::CommunityId; + use std::sync::atomic::AtomicU8; + use std::sync::Arc; + use tokio::sync::Mutex; + use tokio_util::sync::CancellationToken; + use uuid::Uuid; + + let state = crate::state::tests::test_state().await; + + let community_id = CommunityId::from_uuid(Uuid::from_u128(0xCAFE_BABE)); + let channel_id = Uuid::from_u128(0x1234_5678); + let target_pubkey: Vec = vec![0xABu8; 32]; + let tenant = buzz_core::tenant::TenantContext::resolved(community_id, "kick-test.example"); + + // 1. Seed the membership cache — simulates a cache hit that would keep + // the kicked user appearing as a member after the DB write. + state + .membership_cache + .insert((community_id, channel_id, target_pubkey.clone()), true); + + // Confirm the entry is visible before the side effects run. + assert!( + state + .membership_cache + .get(&(community_id, channel_id, target_pubkey.clone())) + .is_some(), + "pre-condition: membership cache entry must exist before kick" + ); + + // 2. Register a connection authenticated as the target pubkey and add a + // channel-scoped subscription so eviction has something to remove. + let conn_id = Uuid::new_v4(); + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); + state.conn_manager.register( + conn_id, + tx, + ctrl_tx, + None, + CancellationToken::new(), + community_id, + Arc::new(AtomicU8::new(0)), + Arc::new(Mutex::new(std::collections::HashMap::new())), + 3, + ); + state + .conn_manager + .set_authenticated_pubkey(conn_id, target_pubkey.clone()); + + let sub_id = "kick-test-sub".to_string(); + state.sub_registry.register_channels_scoped( + community_id, + conn_id, + sub_id, + // One unconstrained filter (no `kinds`) hits the wildcard index, + // making the subscription visible in channel_subscriber_conns_scoped. + vec![nostr::Filter::new()], + vec![channel_id], + ); + + // Confirm subscription is visible before side effects run. + assert!( + state + .sub_registry + .channel_subscriber_conns_scoped(community_id, channel_id) + .contains(&conn_id), + "pre-condition: subscription must be registered before kick" + ); + + // 3. Fire kick live side effects. + crate::handlers::side_effects::apply_kick_live_side_effects( + &tenant, + &state, + channel_id, + &target_pubkey, + false, // fresh path: member still removed, no re-add guard needed + ) + .await; + + // Assert: membership cache entry is gone. + assert!( + state + .membership_cache + .get(&(community_id, channel_id, target_pubkey.clone())) + .is_none(), + "membership cache must not contain a stale entry after kick side effects" + ); + + // Assert: channel subscription is no longer indexed for this connection. + assert!( + !state + .sub_registry + .channel_subscriber_conns_scoped(community_id, channel_id) + .contains(&conn_id), + "kicked user's channel subscription must be evicted after kick side effects" + ); + } } diff --git a/crates/buzz-relay/src/handlers/side_effects.rs b/crates/buzz-relay/src/handlers/side_effects.rs index 8183fe98d80..3d1e3c19093 100644 --- a/crates/buzz-relay/src/handlers/side_effects.rs +++ b/crates/buzz-relay/src/handlers/side_effects.rs @@ -36,6 +36,85 @@ pub fn is_side_effect_kind(kind: u32) -> bool { matches!(kind, 0 | 5 | 9000..=9022 | KIND_GIT_REPO_ANNOUNCEMENT | KIND_AGENT_PROFILE | 41001..=41003 | 40099) } +/// Apply the three live side effects that must follow a successful admin kick: +/// +/// 1. `invalidate_membership` — drops the 10 s membership-cache entry locally +/// AND cross-pod, so subsequent REQ/fan-out checks see the removal immediately. +/// 2. `evict_live_channel_subscriptions` — closes every open channel subscription +/// for the kicked pubkey so they stop receiving messages from this channel. +/// 3. `disable_departed_member_workflows` — durably disables any workflows the +/// kicked user owned in this channel (SEC-006). +/// +/// These are identical to what `handle_remove_user` (kind 9001 path) fires after +/// removing a member. Without them a kicked user's live WebSocket session retains +/// full channel access: the `channel_members.removed_at` write is invisible to +/// the running subscription and the membership cache until natural expiry, which +/// is exactly the "kick reported success but user still in channel" symptom. +/// +/// Failures are logged, not propagated: the kick DB mutation has already +/// committed and the report is finalized. Best-effort live enforcement matches +/// the notice-delivery contract: the enforcement is the promise; live revocation +/// is the courtesy. +/// +/// `is_recovery` must be `true` when called on the crash-recovery path (i.e., the +/// step marker was already set before this drive iteration began). On that path, +/// eviction and workflow-disable are fenced behind a `verify_member_still_removed` +/// check that serializes with any concurrent `add_member` call; if the member was +/// legitimately re-added after the original kick committed the side effects are +/// suppressed (cache invalidation still fires — stale-positive is always safe to +/// drop). On the fresh path (`is_recovery = false`) the removal just committed in +/// the same transaction and no re-add can have happened yet, so the guard is skipped. +pub(crate) async fn apply_kick_live_side_effects( + tenant: &TenantContext, + state: &Arc, + channel_id: Uuid, + target_pubkey: &[u8], + is_recovery: bool, +) { + // Cache invalidation is unconditionally safe — drops a stale positive. + state.invalidate_membership(tenant, channel_id, target_pubkey); + + if is_recovery { + // Fence eviction and workflow-disable against a post-kick re-add. + // Acquire the membership lock and verify removed_at IS NOT NULL. + match state + .db + .verify_member_still_removed(tenant.community(), channel_id, target_pubkey) + .await + { + Ok(true) => { + // Still removed — safe to proceed. + evict_live_channel_subscriptions(tenant, state, channel_id, target_pubkey).await; + disable_departed_member_workflows(tenant, state, channel_id, target_pubkey).await; + } + Ok(false) => { + // Member was re-added after the kick committed; skip eviction and + // workflow-disable so the re-add is not silently undone. + tracing::info!( + channel = %channel_id, + target = %hex::encode(target_pubkey), + "crash-recovery kick: member re-added since kick committed; \ + skipping eviction and workflow-disable" + ); + } + Err(e) => { + // DB error in the fence check. Err on the side of not revoking + // a potentially valid membership: skip eviction/disable and log. + warn!( + channel = %channel_id, + target = %hex::encode(target_pubkey), + error = %e, + "crash-recovery kick: verify_member_still_removed failed; \ + skipping eviction and workflow-disable" + ); + } + } + } else { + evict_live_channel_subscriptions(tenant, state, channel_id, target_pubkey).await; + disable_departed_member_workflows(tenant, state, channel_id, target_pubkey).await; + } +} + async fn evict_live_channel_subscriptions( tenant: &TenantContext, state: &Arc, diff --git a/migrations/0045_relay_admin_action_target.sql b/migrations/0045_relay_admin_action_target.sql new file mode 100644 index 00000000000..41ec7ce3385 --- /dev/null +++ b/migrations/0045_relay_admin_action_target.sql @@ -0,0 +1,17 @@ +-- 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; From 1d0db4299e031c9d7180aaa5834a62d1413a8d4d Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Thu, 3 Sep 2026 18:54:44 -0400 Subject: [PATCH 02/13] fix(relay): hold membership lock through kick effects, gate persisted-context to kick only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 — Lock not 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 advisory transaction 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 so no concurrent add_member can commit between the removed-at check and the destructive effects. Remove the is_recovery parameter — the fence is the single correct path. Add membership_removal_fence + MembershipRemovalFence to buzz-db and expose them via Db::membership_removal_fence. Update race test to use real add_member (not direct SQL), falsifying the lock serialization guarantee. Add membership_removal_fence_blocks_concurrent_add_member test demonstrating add_member blocks while the fence is held and completes once it is released. Finding 2 — Persisted-context branch breaks stranded delete recovery: Gate the persisted-context path in admin_action_worker.rs on action == "kick" only. Non-kick actions (ban, timeout, delete) always re-derive from the report so delete recovery still has the required target_event_id. The erroneous branch was forcing target_event_id = None for any action with enforcement_target_pubkey set, causing pre-marker delete recovery to fail with "delete requires target_event_id" and post-marker recovery to skip the tombstone outbox row. Add stranded_delete_pre_marker_recovers_via_worker test pinning the regression: a stranded delete (crash before mutation) must converge via recover_one to succeeded with the event soft-deleted and tombstone + reporter_notice rows. Finding 3 — False compatibility comment: Replace the misleading "pre-migration rows re-derive to save legacy kicks" comment with an accurate description: pre-migration kick rows cannot finalize (convergence requires rec.enforcement_*); the re-derive path is correct only for non-kick actions. CI: Rust Lint and Windows Rust failures are main-inherited (same failures on PR #7291 merged to main as b4cc53a26); local clippy is clean. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/store/channel_members.rs | 110 +++++- crates/buzz-relay/src/api/admin/mod.rs | 353 ++++++++++++++++-- .../src/handlers/admin_action_worker.rs | 81 ++-- .../src/handlers/report_resolution.rs | 19 +- .../buzz-relay/src/handlers/side_effects.rs | 74 ++-- 5 files changed, 525 insertions(+), 112 deletions(-) diff --git a/crates/buzz-db/src/store/channel_members.rs b/crates/buzz-db/src/store/channel_members.rs index 0227a0a3984..08eb8d6bd3a 100644 --- a/crates/buzz-db/src/store/channel_members.rs +++ b/crates/buzz-db/src/store/channel_members.rs @@ -699,10 +699,15 @@ pub async fn is_member( /// = NULL` after recovery finishes) or serializes before it (in which case this /// check observes the re-add and returns `false`, preventing stale eviction). /// -/// Designed for use in crash-recovery before firing subscription eviction and -/// workflow disablement: cache invalidation is always safe (it only drops a -/// stale positive), but eviction and workflow-disable must not target a member -/// who was legitimately re-added after the kick committed. +/// Designed for use before firing subscription eviction and workflow +/// disablement: cache invalidation is always safe (it only drops a stale +/// positive), but eviction and workflow-disable must not target a member who +/// was legitimately re-added after the kick committed. +/// +/// **Use [`membership_removal_fence`] instead of this function** when the caller +/// needs to hold the advisory lock through the destructive effects themselves. +/// This function releases the lock immediately after the read; concurrent +/// `add_member` calls can commit between the return and the caller's effects. pub async fn verify_member_still_removed( pool: &PgPool, community_id: CommunityId, @@ -740,6 +745,77 @@ pub async fn verify_member_still_removed( Ok(still_removed) } +/// A guard that holds the per-channel membership advisory lock for the +/// duration of kick live side effects. +/// +/// Acquired via [`membership_removal_fence`]. The lock is released when the +/// guard is dropped (the inner transaction rolls back). Callers must NOT drop +/// the guard until after subscription eviction and workflow-disable complete, +/// so that no concurrent [`add_member`] can commit between the `still_removed` +/// observation and the destructive effects. +pub struct MembershipRemovalFence { + /// `true` when the member row still has `removed_at IS NOT NULL` — i.e., no + /// re-add has reversed the kick since it committed. When `false`, the caller + /// must NOT fire eviction or workflow-disable. + pub still_removed: bool, + // Keeps the advisory transaction lock alive. Rolled back (lock released) + // on drop — no domain writes happen here. + _tx: Transaction<'static, Postgres>, +} + +/// Acquire the per-channel membership advisory lock and check whether the +/// kicked member is still removed, returning a [`MembershipRemovalFence`] that +/// keeps the lock alive until the guard is dropped. +/// +/// By holding the lock from the moment of the `removed_at` check until after +/// the caller's subscription eviction and workflow-disable complete, this +/// prevents the window where a concurrent [`add_member`] could commit between +/// the check and the effects (the race that [`verify_member_still_removed`] +/// leaves open, since it releases the lock before returning). +/// +/// The guard is read-only: no domain writes occur inside this transaction. +/// Once the caller has finished its effects, dropping the guard releases the +/// lock (the transaction is implicitly rolled back). +pub async fn membership_removal_fence( + pool: &PgPool, + community_id: CommunityId, + channel_id: Uuid, + pubkey: &[u8], +) -> Result { + let connection = crate::observability::acquire_writer( + pool, + crate::observability::WriterOperation::Authorization, + ) + .await?; + let mut tx = sqlx::Transaction::begin(connection, None).await?; + acquire_channel_membership_lock(&mut tx, community_id, channel_id).await?; + + let row = sqlx::query( + "SELECT removed_at FROM channel_members \ + WHERE community_id = $1 AND channel_id = $2 AND pubkey = $3", + ) + .bind(community_id.as_uuid()) + .bind(channel_id) + .bind(pubkey) + .fetch_optional(&mut *tx) + .await?; + + // Row absent → never joined or hard-deleted; treat as removed (safe to + // skip eviction for someone who isn't and wasn't a member). + let still_removed = match row { + None => true, + Some(r) => { + let removed_at: Option> = r.try_get("removed_at")?; + removed_at.is_some() + } + }; + + Ok(MembershipRemovalFence { + still_removed, + _tx: tx, + }) +} + /// Return which of the given (channel, pubkey) combinations are active /// memberships, restricted to non-deleted channels — one statement for any /// batch size (T2b). Semantics per pair match [`is_member`]. @@ -1421,9 +1497,9 @@ impl Db { /// holding the channel-membership advisory lock so the check serializes /// with concurrent [`add_member`] calls. /// - /// Use this in crash-recovery before firing subscription eviction and - /// workflow disablement to guard against the kick-commit → re-add → recover - /// race (IMPORTANT 3). + /// **Note:** this function releases the lock before returning. Use + /// [`Db::membership_removal_fence`] when the advisory lock must remain + /// held through subscription eviction and workflow-disable. #[datastore_span(name = "verify_member_still_removed", system = "postgresql")] pub async fn verify_member_still_removed( &self, @@ -1434,6 +1510,26 @@ impl Db { verify_member_still_removed(&self.pool, community_id, channel_id, pubkey).await } + /// Acquire the per-channel membership advisory lock and return a + /// [`MembershipRemovalFence`] that keeps the lock alive until it is + /// dropped, allowing the caller to run eviction and workflow-disable + /// while serializing against concurrent [`add_member`] calls. + /// + /// The guard's `still_removed` field indicates whether the kick is still + /// the current state (i.e., no re-add has reversed it). When `false` the + /// caller must skip eviction and workflow-disable. Dropping the guard + /// releases the advisory lock (the transaction is implicitly rolled back — + /// this is a read-only fence, no domain writes occur). + #[datastore_span(name = "membership_removal_fence", system = "postgresql")] + pub async fn membership_removal_fence( + &self, + community_id: CommunityId, + channel_id: Uuid, + pubkey: &[u8], + ) -> Result { + membership_removal_fence(&self.pool, community_id, channel_id, pubkey).await + } + /// Return the active (channel, pubkey) membership pairs among the given /// sets, in one statement. #[datastore_span(name = "membership_pairs", system = "postgresql")] diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index eaa855628b3..cc6e130bde2 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -5180,6 +5180,207 @@ mod postgres_tests { ); } + // ── 1c. pre-marker stranded delete: crash before mutation, recovery re-drives ── + + /// Stranded delete: crash BEFORE the mutation+marker committed. + /// + /// Recovery worker must re-run the delete mutation, commit the step marker, + /// and finalize — resulting in the event being soft-deleted and both tombstone + /// and reporter_notice outbox rows created. + /// + /// This test guards against the Finding-2 regression introduced at ba34fb292: + /// if the persisted-context branch (`enforcement_target_pubkey` / `channel_id` + /// set) forces `target_event_id = None` for non-kick actions, delete recovery + /// fails pre-mutation with "delete requires target_event_id". Gate on action=="kick" + /// ensures non-kick actions always re-derive from the report. + #[tokio::test] + #[ignore = "requires Postgres — pre-marker stranded delete recovers correctly"] + async fn stranded_delete_pre_marker_recovers_via_worker() { + let pool = e2e_pool().await; + let (community_id, _host) = e2e_community(&pool, "stranded-delete-pre-marker").await; + let target_event_id: Vec = { + let u = uuid::Uuid::new_v4(); + u.as_bytes() + .iter() + .chain(u.as_bytes().iter()) + .copied() + .collect() + }; + let actor = vec![6u8; 32]; + let author = vec![7u8; 32]; + let cid = buzz_core::CommunityId::from_uuid(community_id); + + // Create a channel and seed the target event. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'stranded-del-pre-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + + let sig = vec![0u8; 64]; + sqlx::query( + r#"INSERT INTO events (community_id, id, pubkey, created_at, kind, tags, content, sig, received_at, channel_id) + VALUES ($1, $2, $3, now(), 1, '[]', 'test', $4, now(), $5)"#, + ) + .bind(community_id) + .bind(target_event_id.as_slice()) + .bind(&author) + .bind(sig.as_slice()) + .bind(channel_id) + .execute(&pool) + .await + .expect("insert event"); + + // Create an event report. + let reporter = vec![0u8; 32]; + let report_event_raw: Vec = { + let u = uuid::Uuid::new_v4(); + u.as_bytes() + .iter() + .chain(u.as_bytes().iter()) + .copied() + .collect() + }; + let report_id: uuid::Uuid = sqlx::query_scalar( + r#"INSERT INTO moderation_reports + (community_id, report_event_id, reporter_pubkey, target_kind, target_event_id, + channel_id, report_type) + VALUES ($1, $2, $3, 'event', $4, $5, 'spam') RETURNING id"#, + ) + .bind(community_id) + .bind(report_event_raw.as_slice()) + .bind(&reporter) + .bind(target_event_id.as_slice()) + .bind(channel_id) + .fetch_one(&pool) + .await + .expect("insert event report"); + + // Claim DELETE action but do NOT run the mutation or commit the step marker. + // This simulates a crash immediately after claim — pre-mutation state. + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + uuid::Uuid::new_v4(), + &actor, + "operator", + "delete", + None, + None, + "resolve:delete", + "relay_operator", + None, + Some(target_event_id.as_slice()), + Some(channel_id), + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + + // Advance to enforcing but do not run mutation (crash point = post-claim, pre-mutation). + let _ = buzz_db::relay_admin_actions::begin_enforcing(&pool, action_id) + .await + .expect("begin_enforcing"); + + // Action is in enforcing state with no step_marker — stranded pre-mutation. + let rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("action exists"); + assert_eq!(rec.state, "enforcing"); + assert!(rec.step_marker.is_none(), "must have no step_marker yet"); + + // Also verify the event is NOT yet deleted. + let deleted_before: Option>> = + sqlx::query_scalar("SELECT deleted_at FROM events WHERE community_id = $1 AND id = $2") + .bind(community_id) + .bind(target_event_id.as_slice()) + .fetch_optional(&pool) + .await + .expect("fetch deleted_at before"); + assert!( + deleted_before.flatten().is_none(), + "event must not be deleted before recovery" + ); + + // Expire the lease so the stranded batch can claim it. + sqlx::query( + "UPDATE relay_admin_actions SET action_lease_expires_at = $2, action_lease_token = NULL WHERE id = $1", + ) + .bind(action_id) + .bind(chrono::Utc::now() - chrono::Duration::seconds(300)) + .execute(&pool) + .await + .expect("expire lease"); + + // Re-drive via the real recovery worker. + let batch = buzz_db::relay_admin_actions::claim_stranded_action_batch( + &pool, + "e2e-stranded-del-pre", + chrono::Utc::now() + chrono::Duration::seconds(120), + 1000, + ) + .await + .expect("claim_stranded_action_batch"); + let claim = batch + .into_iter() + .find(|c| c.record.id == action_id) + .expect("stranded action must appear in batch"); + + let state = state_from_pool(pool.clone()).await; + crate::handlers::admin_action_worker::recover_one(&state, claim).await; + + // Action must have converged to succeeded. + let final_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("exists"); + assert_eq!( + final_rec.state, "succeeded", + "stranded pre-marker delete must converge to succeeded" + ); + + // Event must now be soft-deleted. + let deleted_after: Option>> = + sqlx::query_scalar("SELECT deleted_at FROM events WHERE community_id = $1 AND id = $2") + .bind(community_id) + .bind(target_event_id.as_slice()) + .fetch_optional(&pool) + .await + .expect("fetch deleted_at after"); + assert!( + deleted_after.flatten().is_some(), + "event must be soft-deleted after stranded pre-marker delete recovery" + ); + + // Tombstone + reporter_notice outbox rows must exist. + let outbox_rows: Vec = sqlx::query_scalar( + "SELECT task_type FROM relay_admin_outbox WHERE action_id = $1 ORDER BY task_type", + ) + .bind(action_id) + .fetch_all(&pool) + .await + .expect("fetch outbox rows"); + assert!( + outbox_rows.iter().any(|t| t == "tombstone"), + "tombstone outbox row must exist after delete recovery; got: {outbox_rows:?}" + ); + assert!( + outbox_rows.iter().any(|t| t == "reporter_notice"), + "reporter_notice outbox row must exist; got: {outbox_rows:?}" + ); + } + // ── 1b. timeout affected-user notice: worker renders the authoritative term ─ #[tokio::test] @@ -7925,13 +8126,19 @@ mod postgres_tests { /// Re-add race: kick commits → crash → `add_member` re-adds → recovery fires. /// - /// Asserts that after a post-kick re-add, the recovery worker does NOT evict - /// subscriptions or disable workflows for the now-valid member. Cache - /// invalidation (stale positive) is always safe and may still fire. + /// Scenario: the member was legitimately re-added (via `add_member`) BEFORE + /// the recovery worker acquires the membership fence. When the fence is + /// acquired, `removed_at IS NULL` — the re-add already committed. Recovery + /// must NOT evict subscriptions or disable workflows for the now-valid member. + /// + /// This test is falsifiable: removing the `membership_removal_fence` check + /// from `apply_kick_live_side_effects` would cause the recovery to always + /// fire effects, wrongly evicting the re-added member's subscription and + /// disabling their workflow. /// - /// This test is falsifiable: removing the `verify_member_still_removed` fence - /// from `apply_kick_live_side_effects` would cause this test to fail because - /// the re-added member's subscription and workflow would be wrongly revoked. + /// Uses real `add_member` (not direct SQL) to exercise the advisory-lock + /// serialization: `add_member` sets `removed_at = NULL` inside the lock and + /// commits before the fence is acquired, so the fence observes the re-add. #[tokio::test] #[ignore = "requires Postgres — post-kick re-add survives crash recovery"] async fn crash_recovery_after_readd_preserves_membership_subscriptions_and_workflows() { @@ -8022,23 +8229,22 @@ mod postgres_tests { .await .expect("expire lease"); - // Re-add the member (simulates `add_member` re-granting access after kick). - sqlx::query( - "UPDATE channel_members SET removed_at = NULL, removed_by = NULL \ - WHERE community_id = $1 AND channel_id = $2 AND pubkey = $3", + // Re-add the member via the real `add_member` (not direct SQL). + // This acquires and releases the membership advisory lock, setting + // removed_at = NULL before the recovery worker's fence is acquired. + buzz_db::channel_members::add_member( + &pool, + cid, + channel_id, + &target, + buzz_db::channel::MemberRole::Member, + None, ) - .bind(community_id) - .bind(channel_id) - .bind(&target) - .execute(&pool) .await - .expect("re-add member"); + .expect("re-add member via add_member"); // Verify pre-condition: member is active again. - let still_member = state_from_pool(pool.clone()) - .await - .db - .is_member(cid, channel_id, &target) + let still_member = buzz_db::channel_members::is_member(&pool, cid, channel_id, &target) .await .expect("is_member"); assert!( @@ -8106,7 +8312,7 @@ mod postgres_tests { // ── Assertions: re-added member's live session must survive ────────── - // 1. Subscription NOT evicted: re-added member still has a live session. + // 1. Subscription NOT evicted: fence observed re-add, skipped eviction. assert!( state .sub_registry @@ -8115,7 +8321,7 @@ mod postgres_tests { "recovery must NOT evict the subscription of a re-added member" ); - // 2. Workflow NOT disabled: re-added member's workflow remains active. + // 2. Workflow NOT disabled: fence observed re-add, skipped disable. assert!( state .db @@ -8146,4 +8352,109 @@ mod postgres_tests { "action must reach succeeded even when re-add fence suppresses eviction" ); } + + /// Fence ordering: the membership advisory lock is held through eviction and + /// workflow-disable, so `add_member` cannot commit between the removed-at + /// check and the destructive effects. + /// + /// This test verifies the other direction of the race: the fence is acquired + /// BEFORE any concurrent `add_member`. While the fence is held, an `add_member` + /// in a separate task must block and cannot commit until the fence is released. + /// + /// This is falsifiable: if `membership_removal_fence` released the transaction + /// lock before returning (as the old `verify_member_still_removed` did), the + /// `add_member` task would complete while effects are still running. + #[tokio::test] + #[ignore = "requires Postgres — membership_removal_fence holds advisory lock through effects"] + async fn membership_removal_fence_blocks_concurrent_add_member() { + let pool = e2e_pool().await; + let (community_id, _host) = e2e_community(&pool, "fence-ordering").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0xEEu8; 32]; + let actor = vec![0xFFu8; 32]; + + // Seed channel + member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'fence-ordering-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + // Kick: set removed_at (simulate kick committed). + sqlx::query( + "UPDATE channel_members SET removed_at = now(), removed_by = $4 \ + WHERE community_id = $1 AND channel_id = $2 AND pubkey = $3", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .bind(&actor) + .execute(&pool) + .await + .expect("kick member"); + + // Acquire the fence — this holds the membership advisory lock. + let fence = + buzz_db::channel_members::membership_removal_fence(&pool, cid, channel_id, &target) + .await + .expect("acquire fence"); + assert!(fence.still_removed, "pre-condition: member must be removed"); + + // Spawn `add_member` in a separate task. It must NOT complete while the + // fence is held — the advisory lock blocks it. + let pool2 = pool.clone(); + let target_clone = target.clone(); + let mut add_task = tokio::spawn(async move { + buzz_db::channel_members::add_member( + &pool2, + cid, + channel_id, + &target_clone, + buzz_db::channel::MemberRole::Member, + None, + ) + .await + }); + + // `add_member` must block: the fence holds the same advisory lock. + let blocked = + tokio::time::timeout(std::time::Duration::from_millis(500), &mut add_task).await; + assert!( + blocked.is_err(), + "add_member must not complete while the membership fence is held" + ); + + // Release the fence — the advisory lock is dropped when `fence` is dropped. + drop(fence); + + // Now `add_member` can proceed. + tokio::time::timeout(std::time::Duration::from_secs(5), add_task) + .await + .expect("add_member must proceed after fence is released") + .expect("add_member task panicked") + .expect("add_member must succeed after fence is released"); + + // Confirm member is active again. + assert!( + buzz_db::channel_members::is_member(&pool, cid, channel_id, &target) + .await + .expect("is_member"), + "member must be active after add_member succeeds" + ); + } } diff --git a/crates/buzz-relay/src/handlers/admin_action_worker.rs b/crates/buzz-relay/src/handlers/admin_action_worker.rs index 524aa5a2991..364bcfbf44a 100644 --- a/crates/buzz-relay/src/handlers/admin_action_worker.rs +++ b/crates/buzz-relay/src/handlers/admin_action_worker.rs @@ -101,44 +101,57 @@ pub(crate) async fn recover_one(state: &Arc, claim: StrandedActionClai "Action recovery worker re-driving stranded action" ); - // Use the target context persisted at claim time. These columns were added - // in migration 0045; existing rows that pre-date the migration will have - // NULL here, in which case we fall back to report re-derivation so that - // pre-migration stranded actions still converge. - let (target_pubkey_opt, target_event_id_opt, channel_id) = - if rec.enforcement_target_pubkey.is_some() || rec.enforcement_channel_id.is_some() { - // Persisted context available — use it unconditionally. - ( - rec.enforcement_target_pubkey.clone(), - None::>, - rec.enforcement_channel_id, - ) - } else { - // Pre-migration row or non-pubkey-targeted action: re-derive from report. - let report = match state.db.admin_get_report(rec.report_id).await { - Ok(Some(r)) => r, - Ok(None) => { - warn!(action_id = %action_id, "Action recovery: report not found"); - return; - } + // Use the target context persisted at claim time for kick actions only. + // Migration 0045 added enforcement_target_pubkey/enforcement_channel_id + // for kicks; other actions (ban, timeout, delete) do not set these columns + // and must re-derive from the report row on every recovery. Applying the + // persisted-context branch to non-kick actions breaks delete recovery: + // delete requires target_event_id (not NULL'd here for kick only) and any + // persisted pubkey from a prior kick on the same report would force + // target_event_id=None, causing pre-marker delete recovery to fail + // ("delete requires target_event_id") or post-marker recovery to skip the + // tombstone outbox row. Gate strictly on action=="kick". + let (target_pubkey_opt, target_event_id_opt, channel_id) = if rec.action == "kick" + && (rec.enforcement_target_pubkey.is_some() || rec.enforcement_channel_id.is_some()) + { + // Persisted kick context available — use it unconditionally. + ( + rec.enforcement_target_pubkey.clone(), + None::>, + rec.enforcement_channel_id, + ) + } else { + // Non-kick action, or pre-migration kick row with both columns NULL. + // Non-kick actions: re-derive from the report — all other action types + // (ban, timeout, delete) do not persist context and must always derive. + // Pre-migration kick rows (both NULL): re-derive also, but note these + // rows cannot finalize — convergence requires rec.enforcement_target_pubkey + // and rec.enforcement_channel_id (invariant error if absent). Pre-migration + // stranded kicks should be effectively zero at deploy time. + let report = match state.db.admin_get_report(rec.report_id).await { + Ok(Some(r)) => r, + Ok(None) => { + warn!(action_id = %action_id, "Action recovery: report not found"); + return; + } + Err(e) => { + warn!(action_id = %action_id, "Action recovery: report lookup failed: {e}"); + return; + } + }; + let (pk, eid) = + match crate::handlers::report_resolution::derive_enforcement_target_pub(&report) { + Ok(pair) => pair, Err(e) => { - warn!(action_id = %action_id, "Action recovery: report lookup failed: {e}"); + warn!( + action_id = %action_id, + "Action recovery: target derive failed: {e:?}" + ); return; } }; - let (pk, eid) = - match crate::handlers::report_resolution::derive_enforcement_target_pub(&report) { - Ok(pair) => pair, - Err(e) => { - warn!( - action_id = %action_id, - "Action recovery: target derive failed: {e:?}" - ); - return; - } - }; - (pk, eid, report.report.channel_id) - }; + (pk, eid, report.report.channel_id) + }; let timeout_until = rec.timeout_until; let action = rec.action.clone(); diff --git a/crates/buzz-relay/src/handlers/report_resolution.rs b/crates/buzz-relay/src/handlers/report_resolution.rs index 97148a53c2e..eb5c0f7e0a4 100644 --- a/crates/buzz-relay/src/handlers/report_resolution.rs +++ b/crates/buzz-relay/src/handlers/report_resolution.rs @@ -451,7 +451,6 @@ async fn drive_enforcement( } // Run mutation only if step marker is not yet committed. - let step_marker_was_set = rec.step_marker.is_some(); if rec.step_marker.is_none() { // Acquire exclusive action lease before running the mutation. Two // concurrent HTTP retries with the same request_id would both reach @@ -625,11 +624,12 @@ async fn drive_enforcement( // INSERT that claimed the action required both values and stored them; if // they are absent the row is corrupt and we must not silently succeed. // - // On the crash-recovery path only (marker was already set when we entered - // this loop iteration), eviction and workflow-disable are gated behind - // verify_member_still_removed so a kick-commit → re-add → recover race - // does not revoke a legitimately restored membership. Cache invalidation - // is unconditional because stale-positive is always safe to drop. + // Eviction and workflow-disable are fenced behind membership_removal_fence + // (which holds the per-channel advisory lock through both effects) so a + // kick-commit → re-add → re-drive race does not revoke a legitimately + // restored membership. Cache invalidation is unconditional because + // stale-positive is always safe to drop. The fence applies on every path + // (fresh and recovery) for a single consistent ordering guarantee. if action == "kick" { match ( rec.enforcement_target_pubkey.as_deref(), @@ -637,11 +637,7 @@ async fn drive_enforcement( ) { (Some(target), Some(ch)) => { crate::handlers::side_effects::apply_kick_live_side_effects( - tenant, - state, - ch, - target, - step_marker_was_set, + tenant, state, ch, target, ) .await; } @@ -1166,7 +1162,6 @@ mod tests { &state, channel_id, &target_pubkey, - false, // fresh path: member still removed, no re-add guard needed ) .await; diff --git a/crates/buzz-relay/src/handlers/side_effects.rs b/crates/buzz-relay/src/handlers/side_effects.rs index 3d1e3c19093..dcef0bcd17b 100644 --- a/crates/buzz-relay/src/handlers/side_effects.rs +++ b/crates/buzz-relay/src/handlers/side_effects.rs @@ -56,62 +56,60 @@ pub fn is_side_effect_kind(kind: u32) -> bool { /// the notice-delivery contract: the enforcement is the promise; live revocation /// is the courtesy. /// -/// `is_recovery` must be `true` when called on the crash-recovery path (i.e., the -/// step marker was already set before this drive iteration began). On that path, -/// eviction and workflow-disable are fenced behind a `verify_member_still_removed` -/// check that serializes with any concurrent `add_member` call; if the member was -/// legitimately re-added after the original kick committed the side effects are -/// suppressed (cache invalidation still fires — stale-positive is always safe to -/// drop). On the fresh path (`is_recovery = false`) the removal just committed in -/// the same transaction and no re-add can have happened yet, so the guard is skipped. +/// **Fencing:** eviction and workflow-disable are gated behind a +/// `membership_removal_fence` that acquires the same `pg_advisory_xact_lock` as +/// `add_member` and holds it through both destructive effects. This prevents the +/// window where a concurrent re-add could commit between the `removed_at` check +/// and the effects: any `add_member` either serializes before the fence (the +/// fence then observes `removed_at IS NULL` and skips effects) or waits until +/// after the effects complete (the re-add then succeeds cleanly). Cache +/// invalidation fires unconditionally before the fence — stale-positive is +/// always safe to drop. pub(crate) async fn apply_kick_live_side_effects( tenant: &TenantContext, state: &Arc, channel_id: Uuid, target_pubkey: &[u8], - is_recovery: bool, ) { // Cache invalidation is unconditionally safe — drops a stale positive. state.invalidate_membership(tenant, channel_id, target_pubkey); - if is_recovery { - // Fence eviction and workflow-disable against a post-kick re-add. - // Acquire the membership lock and verify removed_at IS NOT NULL. - match state - .db - .verify_member_still_removed(tenant.community(), channel_id, target_pubkey) - .await - { - Ok(true) => { - // Still removed — safe to proceed. + // Acquire the membership advisory lock and check that the member is still + // removed. The fence guard keeps the lock alive through eviction and + // workflow-disable so no concurrent add_member can commit in that window. + match state + .db + .membership_removal_fence(tenant.community(), channel_id, target_pubkey) + .await + { + Ok(fence) => { + if fence.still_removed { + // Still removed — fire effects while the lock is held. evict_live_channel_subscriptions(tenant, state, channel_id, target_pubkey).await; disable_departed_member_workflows(tenant, state, channel_id, target_pubkey).await; - } - Ok(false) => { - // Member was re-added after the kick committed; skip eviction and - // workflow-disable so the re-add is not silently undone. + // Lock released here when `fence` is dropped. + } else { + // Member was re-added before this driver acquired the fence; + // skip eviction and workflow-disable so the re-add is not undone. tracing::info!( channel = %channel_id, target = %hex::encode(target_pubkey), - "crash-recovery kick: member re-added since kick committed; \ - skipping eviction and workflow-disable" - ); - } - Err(e) => { - // DB error in the fence check. Err on the side of not revoking - // a potentially valid membership: skip eviction/disable and log. - warn!( - channel = %channel_id, - target = %hex::encode(target_pubkey), - error = %e, - "crash-recovery kick: verify_member_still_removed failed; \ + "kick live effects: member re-added before fence acquired; \ skipping eviction and workflow-disable" ); } } - } else { - evict_live_channel_subscriptions(tenant, state, channel_id, target_pubkey).await; - disable_departed_member_workflows(tenant, state, channel_id, target_pubkey).await; + Err(e) => { + // DB error acquiring the fence. Err on the side of not revoking a + // potentially valid membership: skip eviction/disable and log. + warn!( + channel = %channel_id, + target = %hex::encode(target_pubkey), + error = %e, + "kick live effects: membership_removal_fence failed; \ + skipping eviction and workflow-disable" + ); + } } } From 6c9a43b9a2975c9e20bf4aefd7f1e40c50d31570 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Thu, 3 Sep 2026 19:41:48 -0400 Subject: [PATCH 03/13] fix(relay): fix 3 unused-variable warnings and add enforcement columns to schema.sql MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix three test-only lint errors: prefix let tenant → let _tenant at lines 6324, 8000, 8258 (added in ba34fb292, not carried forward by 1d0db4299). Add enforcement_target_pubkey and enforcement_channel_id to relay_admin_actions in schema/schema.sql. The postgres-test framework builds the per-test DB from schema.sql via pgschema, not from SQLx migrations. Migration 0045 adds these columns, but the desired-state schema was not updated, causing all relay_admin_actions tests to fail with 42703 "column does not exist". Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-relay/src/api/admin/mod.rs | 6 +++--- schema/schema.sql | 8 ++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index cc6e130bde2..61c856aac86 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -6321,7 +6321,7 @@ mod postgres_tests { // a stale membership cache entry and an active channel subscription. // These are what the crash-recovery live side effects must clear. let cid = buzz_core::CommunityId::from_uuid(community_id); - let tenant = e2e_tenant(community_id, "worker-midflight-purge.example"); + let _tenant = e2e_tenant(community_id, "worker-midflight-purge.example"); state .membership_cache .insert((cid, channel_id, author.clone()), true); @@ -7997,7 +7997,7 @@ mod postgres_tests { // Build state with real DB. Seed stale in-process entries. let state = state_from_pool(pool.clone()).await; - let tenant = e2e_tenant(community_id, &host); + let _tenant = e2e_tenant(community_id, &host); state .membership_cache @@ -8255,7 +8255,7 @@ mod postgres_tests { // Build state with real DB. Register a live subscription and workflow // for the re-added member — recovery must NOT revoke these. let state = state_from_pool(pool.clone()).await; - let tenant = e2e_tenant(community_id, &host); + let _tenant = e2e_tenant(community_id, &host); let conn_id = uuid::Uuid::new_v4(); let (tx, _rx) = tokio::sync::mpsc::channel(1); diff --git a/schema/schema.sql b/schema/schema.sql index 09508125622..a04f621cb56 100644 --- a/schema/schema.sql +++ b/schema/schema.sql @@ -1803,6 +1803,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 0045): 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). From 765f045520701216aa43ee91a51f8593a54bb463 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Fri, 4 Sep 2026 10:03:50 -0400 Subject: [PATCH 04/13] fix(relay): seed user rows for workflow FK and run migrations to 45 in parity test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin_schema_parity_between_desired_state_and_migrations test was running migrations only to version 39, while schema.sql (the desired-state source) now includes the enforcement_target_pubkey and enforcement_channel_id columns added by migration 0045. Advance run_to(39) to run_to(45) so the migrated probe DB matches the desired-state probe DB column for column. The three kick-recovery tests (crash_recovery_redrive_fires_kick_live_side_effects, crash_recovery_after_readd_preserves_membership_subscriptions_and_workflows, and worker_redrive_of_event_kick_converges_after_event_purged_mid_flight) all call create_workflow for the kicked user. The desired-state DB enforces the workflows FK (community_id, owner_pubkey) → users (community_id, pubkey), but none of the test setup paths creates a users row for the workflow owner. Seed the row with ON CONFLICT DO NOTHING before each create_workflow call. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/runtime/migration.rs | 4 ++-- crates/buzz-relay/src/api/admin/mod.rs | 32 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/buzz-db/src/runtime/migration.rs b/crates/buzz-db/src/runtime/migration.rs index ecd48bef598..119e7301d19 100644 --- a/crates/buzz-db/src/runtime/migration.rs +++ b/crates/buzz-db/src/runtime/migration.rs @@ -2253,9 +2253,9 @@ mod postgres_tests { .await .expect("connect migrated probe database"); MIGRATOR - .run_to(39, &migrated) + .run_to(45, &migrated) .await - .expect("apply migrations 1-39"); + .expect("apply migrations 1-45"); for table in [ "relay_admin_actions", diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 61c856aac86..d0b65420db3 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -6351,6 +6351,16 @@ mod postgres_tests { ); // Seed an enabled workflow owned by the kicked user so we can verify disable. + // Seed a user row first: required by the workflows FK + // (community_id, owner_pubkey) → users (community_id, pubkey). + sqlx::query( + "INSERT INTO users (community_id, pubkey) VALUES ($1, $2) ON CONFLICT DO NOTHING", + ) + .bind(community_id) + .bind(&author) + .execute(&pool) + .await + .expect("seed user row for workflow owner"); let workflow_id = state .db .create_workflow( @@ -8028,6 +8038,17 @@ mod postgres_tests { vec![channel_id], ); + // Seed a user row for the target: required by the workflows FK + // (community_id, owner_pubkey) → users (community_id, pubkey). + sqlx::query( + "INSERT INTO users (community_id, pubkey) VALUES ($1, $2) ON CONFLICT DO NOTHING", + ) + .bind(community_id) + .bind(&target) + .execute(&pool) + .await + .expect("seed user row for workflow owner"); + // Seed an enabled owned workflow; disable must be asserted after recovery. let workflow_id = state .db @@ -8282,6 +8303,17 @@ mod postgres_tests { vec![channel_id], ); + // Seed a user row for the target: required by the workflows FK + // (community_id, owner_pubkey) → users (community_id, pubkey). + sqlx::query( + "INSERT INTO users (community_id, pubkey) VALUES ($1, $2) ON CONFLICT DO NOTHING", + ) + .bind(community_id) + .bind(&target) + .execute(&pool) + .await + .expect("seed user row for workflow owner"); + let workflow_id = state .db .create_workflow( From 9f2236604022a97d1e7d8ef2284846df88a60fc3 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Fri, 4 Sep 2026 10:36:25 -0400 Subject: [PATCH 05/13] fix(relay): run workflow-disable on fence connection, add pool-size-1 regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workflow-disable UPDATE (SEC-006) previously called disable_workflows_for_owner_in_channel(&pool, ...) inside apply_kick_live_side_effects, acquiring a second pool connection while the MembershipRemovalFence already held one. With a pool of size N, N concurrent kicks self-deadlock: each kick waits for a connection its own fence is holding. The 3 s sqlx acquire timeout silently skips the durable revocation — the per-fire authority gate remains, but the disable is lost. Fix: add disable_workflows_for_owner_in_channel_on_conn(&mut PgConnection) to buzz-db/store/workflow.rs and MembershipRemovalFence::commit_disabling_workflows to channel_members.rs. The fence's commit path runs the UPDATE on its own transaction connection, then commits — no second pool connection needed. The advisory lock is not released until after the disable is durable. The inline remove paths (kind 9001 / kind 9022 via handle_group_remove_member and handle_group_leave) do not hold a fence, so they continue using disable_departed_member_workflows with a pool connection; this function is restored with a clarifying doc comment distinguishing the two paths. Regression test pool_size_1_fence_commit_disabling_workflows_completes_without_deadlock uses a max_connections=1 pool to make the old two-connection path deterministically deadlock, and asserts the workflow row is durably disabled after the fix. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/store/channel_members.rs | 50 ++++++- crates/buzz-db/src/store/workflow.rs | 31 ++++- crates/buzz-relay/src/api/admin/mod.rs | 122 ++++++++++++++++++ .../buzz-relay/src/handlers/side_effects.rs | 56 +++++++- 4 files changed, 244 insertions(+), 15 deletions(-) diff --git a/crates/buzz-db/src/store/channel_members.rs b/crates/buzz-db/src/store/channel_members.rs index 08eb8d6bd3a..6d78ad6eb87 100644 --- a/crates/buzz-db/src/store/channel_members.rs +++ b/crates/buzz-db/src/store/channel_members.rs @@ -753,14 +753,53 @@ pub async fn verify_member_still_removed( /// the guard until after subscription eviction and workflow-disable complete, /// so that no concurrent [`add_member`] can commit between the `still_removed` /// observation and the destructive effects. +/// +/// After firing all live side effects, call +/// [`commit_disabling_workflows`][Self::commit_disabling_workflows] to run +/// the durable workflow-disable UPDATE on this guard's own connection and +/// commit the transaction (releasing the lock). If the guard is dropped +/// without committing, the inner transaction rolls back — the advisory lock +/// is released, but no domain writes persist. pub struct MembershipRemovalFence { /// `true` when the member row still has `removed_at IS NOT NULL` — i.e., no /// re-add has reversed the kick since it committed. When `false`, the caller /// must NOT fire eviction or workflow-disable. pub still_removed: bool, - // Keeps the advisory transaction lock alive. Rolled back (lock released) - // on drop — no domain writes happen here. - _tx: Transaction<'static, Postgres>, + // Holds the advisory transaction lock. The caller commits via + // `commit_disabling_workflows`; on drop without commit the tx rolls back. + tx: Transaction<'static, Postgres>, +} + +impl MembershipRemovalFence { + /// Run the workflow-disable UPDATE on this guard's own connection, then + /// commit the transaction (releasing the advisory lock). + /// + /// By running the UPDATE inside the same connection that holds the lock, + /// no additional pool connection is needed — preventing the self-deadlock + /// that would arise from a pool-size-N scenario where every kick holds one + /// connection while trying to acquire a second for the disable write. + /// + /// The commit makes the disable durable before the lock is released, so + /// there is no window between "workflows disabled" and "lock released." + /// + /// On failure the transaction is rolled back (the advisory lock is still + /// released), and the error is returned to the caller to handle (log/skip). + pub async fn commit_disabling_workflows( + mut self, + community_id: CommunityId, + channel_id: Uuid, + owner_pubkey: &[u8], + ) -> crate::Result { + let affected = crate::workflow::disable_workflows_for_owner_in_channel_on_conn( + &mut self.tx, + community_id, + channel_id, + owner_pubkey, + ) + .await?; + self.tx.commit().await?; + Ok(affected) + } } /// Acquire the per-channel membership advisory lock and check whether the @@ -810,10 +849,7 @@ pub async fn membership_removal_fence( } }; - Ok(MembershipRemovalFence { - still_removed, - _tx: tx, - }) + Ok(MembershipRemovalFence { still_removed, tx }) } /// Return which of the given (channel, pubkey) combinations are active diff --git a/crates/buzz-db/src/store/workflow.rs b/crates/buzz-db/src/store/workflow.rs index 3ceed9ea32e..e13795cde44 100644 --- a/crates/buzz-db/src/store/workflow.rs +++ b/crates/buzz-db/src/store/workflow.rs @@ -12,7 +12,7 @@ use std::str::FromStr; use chrono::{DateTime, Utc}; use sha2::{Digest, Sha256}; -use sqlx::{PgPool, Row}; +use sqlx::{PgConnection, PgPool, Row}; use uuid::Uuid; use buzz_core::CommunityId; @@ -743,6 +743,35 @@ pub async fn disable_workflows_for_owner_in_channel( Ok(affected) } +/// Connection-local variant: run the same workflow-disable UPDATE on the +/// provided connection rather than acquiring a new one from the pool. +/// +/// Used by [`buzz_db::channel_members::MembershipRemovalFence::commit_disabling_workflows`] +/// so the disable executes on the fence's own connection, avoiding a second +/// pool acquisition while the fence holds one connection. +pub async fn disable_workflows_for_owner_in_channel_on_conn( + conn: &mut PgConnection, + community_id: CommunityId, + channel_id: Uuid, + owner_pubkey: &[u8], +) -> Result { + let affected = sqlx::query( + r#" + UPDATE workflows + SET enabled = FALSE + WHERE community_id = $1 AND channel_id = $2 AND owner_pubkey = $3 AND enabled = TRUE + "#, + ) + .bind(community_id.as_uuid()) + .bind(channel_id) + .bind(owner_pubkey) + .execute(&mut *conn) + .await? + .rows_affected(); + + Ok(affected) +} + /// Delete a workflow and all its runs/approvals (CASCADE). /// /// NOTE: see the cache-invalidation note on [`update_workflow`]. The relay's diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index d0b65420db3..7f648969038 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -8489,4 +8489,126 @@ mod postgres_tests { "member must be active after add_member succeeds" ); } + + /// Pool-size-1 regression: `commit_disabling_workflows` runs the UPDATE on + /// the fence's own connection so no second pool connection is needed. + /// + /// This test would deadlock (or time out and skip the disable) on the + /// previous two-connection implementation: the fence holds the one available + /// connection while `disable_workflows_for_owner_in_channel(&pool, …)` waits + /// for another. With the fix, only the fence's connection is used for both + /// the lock and the UPDATE, so the test must complete and the workflow row + /// must be durably disabled. + #[tokio::test] + #[ignore = "requires Postgres — pool-size-1 fence workflow-disable does not self-deadlock"] + async fn pool_size_1_fence_commit_disabling_workflows_completes_without_deadlock() { + // Single-connection pool: every additional acquire blocks until the + // current holder releases. This makes the old two-connection path + // deterministically self-deadlock. + let url = database_url(); + let pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .connect(&url) + .await + .expect("connect with pool-size-1"); + + let (community_id, host) = e2e_community(&pool, "fence-pool1").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0x11u8; 32]; + let actor = vec![0x22u8; 32]; + + // Seed channel + member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'fence-pool1-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + // Simulate kick committed: set removed_at. + sqlx::query( + "UPDATE channel_members SET removed_at = now(), removed_by = $4 \ + WHERE community_id = $1 AND channel_id = $2 AND pubkey = $3", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .bind(&actor) + .execute(&pool) + .await + .expect("kick member"); + + // Seed a user row and an enabled workflow for the target. + sqlx::query( + "INSERT INTO users (community_id, pubkey) VALUES ($1, $2) ON CONFLICT DO NOTHING", + ) + .bind(community_id) + .bind(&target) + .execute(&pool) + .await + .expect("seed user"); + let state = state_from_pool(pool.clone()).await; + let workflow_id = state + .db + .create_workflow( + cid, + Some(channel_id), + &target, + "pool1-fence-workflow", + r#"{"kind":"workflow"}"#, + &[0u8; 32], + ) + .await + .expect("create workflow"); + + // Pre-condition: workflow is enabled. + assert!( + state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "pre-condition: workflow must be enabled" + ); + + // The pool has max_connections=1. The fence acquires that one connection. + // On the old code, commit_disabling_workflows would try to acquire a + // SECOND connection from the pool here and deadlock (the fence still holds + // the first). With the fix, the UPDATE runs on the fence's own connection. + let tenant = buzz_core::tenant::TenantContext::resolved(cid, host.clone()); + tokio::time::timeout( + std::time::Duration::from_secs(10), + crate::handlers::side_effects::apply_kick_live_side_effects( + &tenant, &state, channel_id, &target, + ), + ) + .await + .expect("apply_kick_live_side_effects must complete without deadlock on pool-size-1"); + + // Post-condition: workflow is disabled — the UPDATE committed. + assert!( + !state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow after effects") + .enabled, + "workflow must be durably disabled after kick live effects" + ); + } } diff --git a/crates/buzz-relay/src/handlers/side_effects.rs b/crates/buzz-relay/src/handlers/side_effects.rs index dcef0bcd17b..f0d026bb5d1 100644 --- a/crates/buzz-relay/src/handlers/side_effects.rs +++ b/crates/buzz-relay/src/handlers/side_effects.rs @@ -65,6 +65,13 @@ pub fn is_side_effect_kind(kind: u32) -> bool { /// after the effects complete (the re-add then succeeds cleanly). Cache /// invalidation fires unconditionally before the fence — stale-positive is /// always safe to drop. +/// +/// The workflow-disable UPDATE (SEC-006) runs on the fence's own connection +/// via [`buzz_db::channel_members::MembershipRemovalFence::commit_disabling_workflows`] +/// rather than acquiring a second pool connection. This prevents a self-deadlock +/// when N concurrent kicks ≥ pool size: each kick holds one connection while +/// the disable would otherwise wait for a second — a cycle the 3 s acquire +/// timeout would "resolve" by silently skipping the durable revocation. pub(crate) async fn apply_kick_live_side_effects( tenant: &TenantContext, state: &Arc, @@ -85,9 +92,37 @@ pub(crate) async fn apply_kick_live_side_effects( Ok(fence) => { if fence.still_removed { // Still removed — fire effects while the lock is held. + // Eviction first (no DB write needed), then commit the fence + // with the workflow-disable UPDATE on the same connection so no + // second pool connection is needed (pool-exhaustion deadlock + // prevention — see module doc). evict_live_channel_subscriptions(tenant, state, channel_id, target_pubkey).await; - disable_departed_member_workflows(tenant, state, channel_id, target_pubkey).await; - // Lock released here when `fence` is dropped. + match fence + .commit_disabling_workflows(tenant.community(), channel_id, target_pubkey) + .await + { + Ok(0) => {} + Ok(n) => { + tracing::info!( + channel = %channel_id, + owner = %hex::encode(target_pubkey), + disabled = n, + "Disabled departed member's workflows" + ); + state + .workflow_engine + .invalidate_channel_workflows(tenant.community(), channel_id); + } + Err(e) => { + warn!( + channel = %channel_id, + owner = %hex::encode(target_pubkey), + error = %e, + "Failed to disable departed member's workflows — \ + per-fire authority gate still denies" + ); + } + } } else { // Member was re-added before this driver acquired the fence; // skip eviction and workflow-disable so the re-add is not undone. @@ -130,12 +165,19 @@ async fn evict_live_channel_subscriptions( /// Durably disable a departing member's workflows in the channel (SEC-006). /// +/// Used by the inline remove paths (kind 9001 / kind 9022) where the member +/// removal DB write has already committed and the caller does not hold a fence +/// connection. For the fenced kick path (`apply_kick_live_side_effects`) use +/// [`buzz_db::channel_members::MembershipRemovalFence::commit_disabling_workflows`] +/// instead, which runs the UPDATE on the fence's own connection to avoid the +/// pool-exhaustion self-deadlock. +/// /// A workflow runs with its owner's standing authority; once the owner is no -/// longer a member (removed via kind 9001 or left via kind 9022) their -/// workflows must stop firing on every path — event triggers, the scheduler, -/// manual triggers, and the webhook endpoint all honor `enabled = FALSE`. -/// The per-fire authority gate in `buzz-workflow` is the fail-closed backstop; -/// this makes the revocation durable and immediately visible. +/// longer a member their workflows must stop firing on every path — event +/// triggers, the scheduler, manual triggers, and the webhook endpoint all +/// honor `enabled = FALSE`. The per-fire authority gate in `buzz-workflow` is +/// the fail-closed backstop; this makes the revocation durable and immediately +/// visible. /// /// Failures are logged, not propagated: membership removal has already been /// committed, and the per-fire gate still denies a removed owner even if this From 5d336ac8f3d1998f76e3071708393099b1be24c8 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Fri, 4 Sep 2026 11:36:12 -0400 Subject: [PATCH 06/13] fix(relay): propagate live-effects failure before kick finalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apply_kick_live_side_effects now returns Result<(), anyhow::Error>. Fence-acquire, workflow-disable UPDATE, and commit failures are returned to the caller rather than swallowed. drive_enforcement maps the error to ResolutionError::Internal and returns before finalize_action_success, so mutation_committed remains recoverable and the worker can retry. Successful convergence paths (committed disable, re-added skip) still return Ok(()). Cache invalidation still fires unconditionally before the fence (it's always safe to drop a stale positive). Failure-path regression test added (kick_live_effects_failure_does_not_ finalize_as_succeeded): builds state with a closed pool so the fence acquire fails, then asserts drive_enforcement_pub returns Err and the action stays in enforcing/mutation_committed — not succeeded. Also fixes stale comments: membership_removal_fence wrapper and free function docs no longer describe the guard as read-only/no-domain-writes, and apply_kick_live_side_effects doc no longer claims the report is finalized before this function returns. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/store/channel_members.rs | 20 ++- crates/buzz-relay/src/api/admin/mod.rs | 159 +++++++++++++++++- .../src/handlers/report_resolution.rs | 11 +- .../buzz-relay/src/handlers/side_effects.rs | 115 +++++++------ 4 files changed, 239 insertions(+), 66 deletions(-) diff --git a/crates/buzz-db/src/store/channel_members.rs b/crates/buzz-db/src/store/channel_members.rs index 6d78ad6eb87..7a246e04698 100644 --- a/crates/buzz-db/src/store/channel_members.rs +++ b/crates/buzz-db/src/store/channel_members.rs @@ -812,9 +812,11 @@ impl MembershipRemovalFence { /// the check and the effects (the race that [`verify_member_still_removed`] /// leaves open, since it releases the lock before returning). /// -/// The guard is read-only: no domain writes occur inside this transaction. +/// The guard is read-only before [`Self::commit_disabling_workflows`] is called. /// Once the caller has finished its effects, dropping the guard releases the -/// lock (the transaction is implicitly rolled back). +/// lock (the transaction is implicitly rolled back if not committed). To +/// durably write the workflow-disable and release the lock in one step, call +/// [`Self::commit_disabling_workflows`]. pub async fn membership_removal_fence( pool: &PgPool, community_id: CommunityId, @@ -1548,14 +1550,16 @@ impl Db { /// Acquire the per-channel membership advisory lock and return a /// [`MembershipRemovalFence`] that keeps the lock alive until it is - /// dropped, allowing the caller to run eviction and workflow-disable - /// while serializing against concurrent [`add_member`] calls. + /// dropped or committed via [`MembershipRemovalFence::commit_disabling_workflows`]. /// /// The guard's `still_removed` field indicates whether the kick is still - /// the current state (i.e., no re-add has reversed it). When `false` the - /// caller must skip eviction and workflow-disable. Dropping the guard - /// releases the advisory lock (the transaction is implicitly rolled back — - /// this is a read-only fence, no domain writes occur). + /// the current state (i.e., no re-add has reversed it). When `true`, the + /// caller fires eviction and then calls + /// [`MembershipRemovalFence::commit_disabling_workflows`] to durably + /// write the workflow-disable and commit the transaction (releasing the + /// lock). When `false`, the member was re-added and effects must be skipped. + /// Dropping the guard without committing releases the advisory lock (the + /// transaction is implicitly rolled back). #[datastore_span(name = "membership_removal_fence", system = "postgresql")] pub async fn membership_removal_fence( &self, diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 7f648969038..57707690e56 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -8598,7 +8598,8 @@ mod postgres_tests { ), ) .await - .expect("apply_kick_live_side_effects must complete without deadlock on pool-size-1"); + .expect("apply_kick_live_side_effects must complete without deadlock on pool-size-1") + .expect("apply_kick_live_side_effects must succeed on pool-size-1"); // Post-condition: workflow is disabled — the UPDATE committed. assert!( @@ -8611,4 +8612,160 @@ mod postgres_tests { "workflow must be durably disabled after kick live effects" ); } + + /// Failure-path regression: when `apply_kick_live_side_effects` fails, the + /// action must NOT be finalized as `succeeded`. + /// + /// Without the error-propagation fix the caller (drive_enforcement) would + /// ignore the `Err` and proceed to `finalize_action_success`, falsely + /// recording the kick as `succeeded` while the SEC-006 workflow-disable + /// never committed. + /// + /// This test sets `mutation_committed` directly (simulating the state after + /// a crash or concurrent re-drive), then calls `drive_enforcement_pub` with + /// a pool that has been closed so the fence acquisition fails immediately. + /// The expected outcome is `Err(Internal)` and the action remains at + /// `mutation_committed` in `enforcing` state — retryable, not succeeded. + #[tokio::test] + #[ignore = "requires Postgres — live-effects failure must not finalize action as succeeded"] + async fn kick_live_effects_failure_does_not_finalize_as_succeeded() { + // Use a separate pool to pre-stage the DB rows, then build the state + // from a pool that is immediately closed to force the fence to fail. + let pool = e2e_pool().await; + let (community_id, host) = e2e_community(&pool, "kick-effects-fail").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0xCCu8; 32]; + let actor = vec![0xDDu8; 32]; + + // Seed channel + kicked member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'effects-fail-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + + // Claim → enforcing → lease → execute kick (commits removal + marker). + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + uuid::Uuid::new_v4(), + &actor, + "operator", + "kick", + None, + None, + "resolve:kick", + "relay_operator", + Some(&target), + None, + Some(channel_id), + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + let _ = buzz_db::relay_admin_actions::begin_enforcing(&pool, action_id) + .await + .expect("begin_enforcing"); + let lease_until = chrono::Utc::now() + chrono::Duration::seconds(120); + let lease_token = + match buzz_db::relay_admin_actions::acquire_action_lease(&pool, action_id, lease_until) + .await + .expect("acquire lease") + { + buzz_db::relay_admin_actions::LeaseResult::Acquired(t) => t, + other => panic!("expected Acquired, got {other:?}"), + }; + buzz_db::relay_admin_actions::execute_kick_with_marker( + &pool, + action_id, + lease_token, + cid, + channel_id, + &target, + &actor, + ) + .await + .expect("execute_kick_with_marker"); + + // Verify pre-condition: mutation_committed set, action still enforcing. + let rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("action exists"); + assert_eq!(rec.step_marker.as_deref(), Some("mutation_committed")); + assert_eq!(rec.state, "enforcing"); + + // Build state from a pool that is immediately closed so every DB + // operation in the live-effects path will fail — specifically the + // membership_removal_fence acquire. + let dying_pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .connect(&database_url()) + .await + .expect("connect dying pool"); + let state = state_from_pool(dying_pool.clone()).await; + dying_pool.close().await; + + let tenant = e2e_tenant(community_id, &host); + let result = crate::handlers::report_resolution::drive_enforcement_pub( + &state, + &tenant, + cid, + report_id, + "kick", + None, + None, + &actor, + Some(&target), + None, + Some(channel_id), + &rec, + None, + ) + .await; + + // drive_enforcement_pub must return Err, not Ok. + assert!( + result.is_err(), + "drive_enforcement_pub must return Err when live effects fail, got Ok" + ); + + // Action must NOT have been finalized as succeeded — it must remain + // in enforcing with mutation_committed so the worker can retry. + // (Use the original pool, which is still open, to check.) + let after_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action after") + .expect("record must still exist"); + assert_eq!( + after_rec.state, "enforcing", + "action must remain in enforcing state, not succeeded, when live effects fail" + ); + assert_eq!( + after_rec.step_marker.as_deref(), + Some("mutation_committed"), + "mutation_committed marker must be preserved for retry when live effects fail" + ); + } } diff --git a/crates/buzz-relay/src/handlers/report_resolution.rs b/crates/buzz-relay/src/handlers/report_resolution.rs index eb5c0f7e0a4..1d806a4c79a 100644 --- a/crates/buzz-relay/src/handlers/report_resolution.rs +++ b/crates/buzz-relay/src/handlers/report_resolution.rs @@ -639,7 +639,13 @@ async fn drive_enforcement( crate::handlers::side_effects::apply_kick_live_side_effects( tenant, state, ch, target, ) - .await; + .await + .map_err(|e| { + ResolutionError::Internal(format!( + "kick action {action_id} live side effects failed \ + (mutation_committed marker is recoverable; worker will retry): {e}" + )) + })?; } _ => { return Err(ResolutionError::Internal(format!( @@ -1163,7 +1169,8 @@ mod tests { channel_id, &target_pubkey, ) - .await; + .await + .expect("kick live side effects must succeed in test"); // Assert: membership cache entry is gone. assert!( diff --git a/crates/buzz-relay/src/handlers/side_effects.rs b/crates/buzz-relay/src/handlers/side_effects.rs index ac76b09e719..6c275e6129b 100644 --- a/crates/buzz-relay/src/handlers/side_effects.rs +++ b/crates/buzz-relay/src/handlers/side_effects.rs @@ -52,10 +52,15 @@ pub fn is_side_effect_kind(kind: u32) -> bool { /// the running subscription and the membership cache until natural expiry, which /// is exactly the "kick reported success but user still in channel" symptom. /// -/// Failures are logged, not propagated: the kick DB mutation has already -/// committed and the report is finalized. Best-effort live enforcement matches -/// the notice-delivery contract: the enforcement is the promise; live revocation -/// is the courtesy. +/// # Return value +/// +/// Returns `Ok(())` on successful convergence — both after effects committed +/// and after an intentional re-added skip (the kick is still correctly enforced; +/// the membership was legitimately restored). Returns `Err` when fence +/// acquisition, the workflow-disable UPDATE, or the transaction commit fail — +/// the caller must propagate the error rather than finalizing the action as +/// succeeded, so the `mutation_committed` marker remains recoverable for a later +/// retry. /// /// **Fencing:** eviction and workflow-disable are gated behind a /// `membership_removal_fence` that acquires the same `pg_advisory_xact_lock` as @@ -78,66 +83,18 @@ pub(crate) async fn apply_kick_live_side_effects( state: &Arc, channel_id: Uuid, target_pubkey: &[u8], -) { +) -> Result<(), anyhow::Error> { // Cache invalidation is unconditionally safe — drops a stale positive. state.invalidate_membership(tenant, channel_id, target_pubkey); // Acquire the membership advisory lock and check that the member is still // removed. The fence guard keeps the lock alive through eviction and // workflow-disable so no concurrent add_member can commit in that window. - match state + let fence = state .db .membership_removal_fence(tenant.community(), channel_id, target_pubkey) .await - { - Ok(fence) => { - if fence.still_removed { - // Still removed — fire effects while the lock is held. - // Eviction first (no DB write needed), then commit the fence - // with the workflow-disable UPDATE on the same connection so no - // second pool connection is needed (pool-exhaustion deadlock - // prevention — see module doc). - evict_live_channel_subscriptions(tenant, state, channel_id, target_pubkey).await; - match fence - .commit_disabling_workflows(tenant.community(), channel_id, target_pubkey) - .await - { - Ok(0) => {} - Ok(n) => { - tracing::info!( - channel = %channel_id, - owner = %hex::encode(target_pubkey), - disabled = n, - "Disabled departed member's workflows" - ); - state - .workflow_engine - .invalidate_channel_workflows(tenant.community(), channel_id); - } - Err(e) => { - warn!( - channel = %channel_id, - owner = %hex::encode(target_pubkey), - error = %e, - "Failed to disable departed member's workflows — \ - per-fire authority gate still denies" - ); - } - } - } else { - // Member was re-added before this driver acquired the fence; - // skip eviction and workflow-disable so the re-add is not undone. - tracing::info!( - channel = %channel_id, - target = %hex::encode(target_pubkey), - "kick live effects: member re-added before fence acquired; \ - skipping eviction and workflow-disable" - ); - } - } - Err(e) => { - // DB error acquiring the fence. Err on the side of not revoking a - // potentially valid membership: skip eviction/disable and log. + .map_err(|e| { warn!( channel = %channel_id, target = %hex::encode(target_pubkey), @@ -145,8 +102,56 @@ pub(crate) async fn apply_kick_live_side_effects( "kick live effects: membership_removal_fence failed; \ skipping eviction and workflow-disable" ); + e + })?; + + if !fence.still_removed { + // Member was re-added before this driver acquired the fence. + // Skip eviction and workflow-disable so the re-add is not undone. + // This is successful convergence: the kick is enforced; membership was + // legitimately restored. + tracing::info!( + channel = %channel_id, + target = %hex::encode(target_pubkey), + "kick live effects: member re-added before fence acquired; \ + skipping eviction and workflow-disable" + ); + return Ok(()); + } + + // Still removed — fire effects while the lock is held. + // Eviction first (no DB write needed), then commit the fence with the + // workflow-disable UPDATE on the same connection so no second pool + // connection is needed (pool-exhaustion deadlock prevention — see module doc). + evict_live_channel_subscriptions(tenant, state, channel_id, target_pubkey).await; + match fence + .commit_disabling_workflows(tenant.community(), channel_id, target_pubkey) + .await + { + Ok(0) => {} + Ok(n) => { + tracing::info!( + channel = %channel_id, + owner = %hex::encode(target_pubkey), + disabled = n, + "Disabled departed member's workflows" + ); + state + .workflow_engine + .invalidate_channel_workflows(tenant.community(), channel_id); + } + Err(e) => { + warn!( + channel = %channel_id, + owner = %hex::encode(target_pubkey), + error = %e, + "Failed to disable departed member's workflows; \ + kick live effects returning error so caller can retry" + ); + return Err(e.into()); } } + Ok(()) } async fn evict_live_channel_subscriptions( From f8daef09dfc2b54c961766b61bd700ba04fa91be Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Fri, 4 Sep 2026 11:59:37 -0400 Subject: [PATCH 07/13] test(relay): bind workflow-disable tx-failure arm and add recovery retry assertion The prior failure-path test exercised only the fence-acquire arm (closed pool, pre-effects): it did not bind the case where the fence acquires its connection and the workflow-disable UPDATE itself fails, which is the seam SEC-006 was written to protect. Add `kick_live_effects_disable_tx_failure_does_not_finalize_and_retries_to_success`: - Seeds a user and an enabled workflow for the kick target. - Injects a scoped BEFORE UPDATE trigger on workflows that raises for the seeded workflow id only, so the UPDATE inside commit_disabling_workflows fails after fence acquisition and member eviction succeed. - Phase 1: asserts drive_enforcement_pub returns Err, action remains enforcing/mutation_committed, and workflow is still enabled. - Drops the trigger, expires the lease, claims via claim_stranded_action_batch, and calls the real recover_one. - Phase 2: asserts action is succeeded and workflow is durably disabled. Rename the original closed-pool test to make its scope explicit (fence-acquire arm) and add a doc cross-reference to the new test. Both tests are Postgres-lane discoverable (#[ignore], api::admin::postgres_tests). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-relay/src/api/admin/mod.rs | 310 ++++++++++++++++++++++++- 1 file changed, 299 insertions(+), 11 deletions(-) diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 57707690e56..89a19d13467 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -8613,19 +8613,307 @@ mod postgres_tests { ); } - /// Failure-path regression: when `apply_kick_live_side_effects` fails, the - /// action must NOT be finalized as `succeeded`. + /// Workflow-disable transaction failure regression: the specific seam that + /// SEC-006 protects. /// - /// Without the error-propagation fix the caller (drive_enforcement) would - /// ignore the `Err` and proceed to `finalize_action_success`, falsely - /// recording the kick as `succeeded` while the SEC-006 workflow-disable - /// never committed. + /// A `BEFORE UPDATE` trigger on `workflows` is installed to raise an + /// exception when the workflow-disable UPDATE fires (after fence acquisition + /// and member eviction have already succeeded). The trigger is scoped to + /// one workflow row so it cannot affect unrelated tests running concurrently. /// - /// This test sets `mutation_committed` directly (simulating the state after - /// a crash or concurrent re-drive), then calls `drive_enforcement_pub` with - /// a pool that has been closed so the fence acquisition fails immediately. - /// The expected outcome is `Err(Internal)` and the action remains at - /// `mutation_committed` in `enforcing` state — retryable, not succeeded. + /// Phase 1 — fault injected: + /// `drive_enforcement_pub` must return `Err` and leave the action at + /// `enforcing / mutation_committed`. The workflow must remain enabled + /// (disable rolled back). + /// + /// Phase 2 — fault removed, lease expired, recovery worker re-drives: + /// `claim_stranded_action_batch` must surface the action, `recover_one` + /// must drive it to `succeeded`, and the workflow must be durably disabled. + /// + /// This test was introduced because the original failure-path test only + /// exercised the fence-acquisition arm (closed pool, pre-effects). The + /// transaction-failure arm — the actual finding — was structurally correct + /// but unbound. + #[tokio::test] + #[ignore = "requires Postgres — workflow-disable tx failure must not finalize kick as succeeded, and recovery worker must converge"] + async fn kick_live_effects_disable_tx_failure_does_not_finalize_and_retries_to_success() { + let pool = e2e_pool().await; + let (community_id, host) = e2e_community(&pool, "wf-tx-fail").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0xEEu8; 32]; + let actor = vec![0xFFu8; 32]; + + // Seed channel + member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'wf-tx-fail-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + // Seed user + enabled workflow owned by the kick target. + sqlx::query( + "INSERT INTO users (community_id, pubkey) VALUES ($1, $2) ON CONFLICT DO NOTHING", + ) + .bind(community_id) + .bind(&target) + .execute(&pool) + .await + .expect("seed user"); + let state = state_from_pool(pool.clone()).await; + let workflow_id = state + .db + .create_workflow( + cid, + Some(channel_id), + &target, + "wf-tx-fail-workflow", + r#"{"kind":"workflow"}"#, + &[0u8; 32], + ) + .await + .expect("create workflow"); + + // Confirm pre-condition: workflow enabled. + assert!( + state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "pre-condition: workflow must be enabled" + ); + + // Claim → enforcing → lease → execute kick mutation + marker. + let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + uuid::Uuid::new_v4(), + &actor, + "operator", + "kick", + None, + None, + "resolve:kick", + "relay_operator", + Some(&target), + None, + Some(channel_id), + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + let _ = buzz_db::relay_admin_actions::begin_enforcing(&pool, action_id) + .await + .expect("begin_enforcing"); + let lease_until = chrono::Utc::now() + chrono::Duration::seconds(120); + let lease_token = + match buzz_db::relay_admin_actions::acquire_action_lease(&pool, action_id, lease_until) + .await + .expect("acquire lease") + { + buzz_db::relay_admin_actions::LeaseResult::Acquired(t) => t, + other => panic!("expected Acquired, got {other:?}"), + }; + buzz_db::relay_admin_actions::execute_kick_with_marker( + &pool, + action_id, + lease_token, + cid, + channel_id, + &target, + &actor, + ) + .await + .expect("execute_kick_with_marker"); + + // Pre-condition: mutation_committed set, action still enforcing. + let rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("action exists"); + assert_eq!(rec.step_marker.as_deref(), Some("mutation_committed")); + assert_eq!(rec.state, "enforcing"); + + // ── Phase 1: inject a trigger that raises on workflow-disable UPDATE ── + // + // The trigger function raises immediately for the seeded workflow row so + // the UPDATE inside `commit_disabling_workflows` fails after the fence + // acquires its connection and the member row is already removed. It is + // DROP-ped before the retry, so recovery sees a clean database. + let fn_name = format!("raise_for_wf_{}", workflow_id.simple()); + let trigger_name = format!("trg_raise_for_wf_{}", workflow_id.simple()); + + sqlx::query(sqlx::AssertSqlSafe(format!( + r#" + CREATE OR REPLACE FUNCTION {fn_name}() + RETURNS TRIGGER LANGUAGE plpgsql AS $$ + BEGIN + IF NEW.id = '{workflow_id}' THEN + RAISE EXCEPTION 'injected fault: workflow-disable tx failure for test'; + END IF; + RETURN NEW; + END; + $$ + "# + ))) + .execute(&pool) + .await + .expect("create fault-injection function"); + + sqlx::query(sqlx::AssertSqlSafe(format!( + r#" + CREATE TRIGGER {trigger_name} + BEFORE UPDATE ON workflows + FOR EACH ROW EXECUTE FUNCTION {fn_name}() + "# + ))) + .execute(&pool) + .await + .expect("create fault-injection trigger"); + + // Drive enforcement — the workflow-disable UPDATE will raise, which + // means `commit_disabling_workflows` returns Err, which propagates + // through `apply_kick_live_side_effects` and out of `drive_enforcement` + // BEFORE `finalize_action_success`. + let tenant = e2e_tenant(community_id, &host); + let result = crate::handlers::report_resolution::drive_enforcement_pub( + &state, + &tenant, + cid, + report_id, + "kick", + None, + None, + &actor, + Some(&target), + None, + Some(channel_id), + &rec, + None, + ) + .await; + + assert!( + result.is_err(), + "drive_enforcement_pub must return Err when workflow-disable tx fails, got Ok" + ); + + // Action must remain enforcing / mutation_committed — not succeeded. + let after_fault = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action after fault") + .expect("record must still exist"); + assert_eq!( + after_fault.state, "enforcing", + "action must remain enforcing when disable tx fails, not finalize as succeeded" + ); + assert_eq!( + after_fault.step_marker.as_deref(), + Some("mutation_committed"), + "mutation_committed marker must be preserved for retry" + ); + + // Workflow must still be enabled — the UPDATE rolled back. + assert!( + state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow after fault") + .enabled, + "workflow must remain enabled after rolled-back disable tx" + ); + + // ── Phase 2: remove fault, expire lease, recovery worker converges ──── + + sqlx::query(sqlx::AssertSqlSafe(format!( + "DROP TRIGGER IF EXISTS {trigger_name} ON workflows" + ))) + .execute(&pool) + .await + .expect("drop fault trigger"); + sqlx::query(sqlx::AssertSqlSafe(format!( + "DROP FUNCTION IF EXISTS {fn_name}()" + ))) + .execute(&pool) + .await + .expect("drop fault function"); + + // Expire the lease so claim_stranded_action_batch can reclaim it. + sqlx::query( + "UPDATE relay_admin_actions SET action_lease_expires_at = $2, action_lease_token = NULL WHERE id = $1", + ) + .bind(action_id) + .bind(chrono::Utc::now() - chrono::Duration::seconds(300)) + .execute(&pool) + .await + .expect("expire lease"); + + let batch = buzz_db::relay_admin_actions::claim_stranded_action_batch( + &pool, + "e2e-wf-tx-fail-worker", + chrono::Utc::now() + chrono::Duration::seconds(120), + 1000, + ) + .await + .expect("claim_stranded_action_batch"); + let claim = batch + .into_iter() + .find(|c| c.record.id == action_id) + .expect("stranded action must appear in batch after lease expiry"); + + crate::handlers::admin_action_worker::recover_one(&state, claim).await; + + // Recovery must have succeeded and disabled the workflow durably. + let final_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action after recover_one") + .expect("record still exists"); + assert_eq!( + final_rec.state, "succeeded", + "action must be succeeded after recovery worker re-drives with fault removed" + ); + + let wf_after = state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow after recovery"); + assert!( + !wf_after.enabled, + "workflow must be durably disabled after successful recovery re-drive" + ); + } + + /// Failure-path regression (fence-acquire arm): when `apply_kick_live_side_effects` + /// fails because fence acquisition itself fails, the action must NOT be + /// finalized as `succeeded`. + /// + /// This test closes the pool before `drive_enforcement_pub` so the fence + /// acquire fails immediately, binding the propagation arm for that failure + /// mode. For the disable-UPDATE/commit-failure arm, see + /// `kick_live_effects_disable_tx_failure_does_not_finalize_and_retries_to_success`. #[tokio::test] #[ignore = "requires Postgres — live-effects failure must not finalize action as succeeded"] async fn kick_live_effects_failure_does_not_finalize_as_succeeded() { From 3038b78607f7f2c71ebbb087729864c1ae8ca3bc Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Fri, 4 Sep 2026 12:25:35 -0400 Subject: [PATCH 08/13] fix(db): add enforcement_target_pubkey and enforcement_channel_id to claim_stranded_action_batch RETURNING projection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claim_stranded_action_batch's UPDATE … RETURNING omitted the two columns introduced by this PR, but row_to_action unconditionally reads both with try_get. Every other caller (claim_report at line 283, get_action at line 1255, and the request-id lookup at line 1277) includes them in its RETURNING clause. The gap caused every stranded-action batch containing a claimable row to fail with ColumnNotFound, so the production recovery worker would retry the same batch forever without recovering any action. Add enforcement_target_pubkey and enforcement_channel_id to the projection immediately before created_at / updated_at, matching the ordering at the other sites. The new kick_live_effects_disable_tx_failure_does_not_finalize_and_retries_to_success PostgreSQL test exposed this: phase 2 (claim_stranded_action_batch → recover_one → succeeded + workflow disabled) now passes locally. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/store/relay_admin_actions.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/buzz-db/src/store/relay_admin_actions.rs b/crates/buzz-db/src/store/relay_admin_actions.rs index 3401a1fcb80..f50ab385b47 100644 --- a/crates/buzz-db/src/store/relay_admin_actions.rs +++ b/crates/buzz-db/src/store/relay_admin_actions.rs @@ -1532,7 +1532,8 @@ pub async fn claim_stranded_action_batch( AND (action_lease_expires_at IS NULL OR action_lease_expires_at < now()) RETURNING id, report_id, report_community_id, request_id, actor_pubkey, actor_role, action, reason, timeout_until, state, step_marker, - cancelled_by, error_message, created_at, updated_at + cancelled_by, error_message, enforcement_target_pubkey, + enforcement_channel_id, created_at, updated_at "#, ) .bind(id) From e602b3931bf754468aaf1f15dc70fa4af6ea1129 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Fri, 4 Sep 2026 14:49:13 -0400 Subject: [PATCH 09/13] docs(buzz-relay): clarify pod-local scope of evict_live_channel_subscriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior docstring for apply_kick_live_side_effects said eviction "closes every open channel subscription," which implied cluster-wide coverage. The helper actually iterates conn_manager on the local pod only; a kicked user connected to a different pod keeps their inert subscription state. Rescope the doc to match the real behavior: - eviction = local-pod connections only - cluster-wide correctness = cross-pod CacheInvalidation::Membership (invalidate_membership publishes to all pods) + filter_fanout_by_access re-checks is_member_cached before every delivery, blocking the removed user on every pod regardless of which pod they are connected to - note that the same pod-local primitive serves remove/leave; cross-pod CLOSED frame delivery is a known future improvement No code changes — doc/behavior consistency fix per review comment. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-relay/src/handlers/side_effects.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/buzz-relay/src/handlers/side_effects.rs b/crates/buzz-relay/src/handlers/side_effects.rs index 6c275e6129b..c6fb339667e 100644 --- a/crates/buzz-relay/src/handlers/side_effects.rs +++ b/crates/buzz-relay/src/handlers/side_effects.rs @@ -39,10 +39,18 @@ pub fn is_side_effect_kind(kind: u32) -> bool { /// Apply the three live side effects that must follow a successful admin kick: /// -/// 1. `invalidate_membership` — drops the 10 s membership-cache entry locally -/// AND cross-pod, so subsequent REQ/fan-out checks see the removal immediately. -/// 2. `evict_live_channel_subscriptions` — closes every open channel subscription -/// for the kicked pubkey so they stop receiving messages from this channel. +/// 1. `invalidate_membership` — drops the 10 s membership-cache entry on this pod +/// AND publishes a cross-pod `CacheInvalidation::Membership` message so every +/// other relay instance also drops the entry. Subsequent REQ / fan-out +/// `is_member_cached` calls on any pod see the removal immediately. +/// 2. `evict_live_channel_subscriptions` — closes open channel subscriptions for +/// the kicked pubkey **on this pod only**. A session connected to a different +/// pod keeps its inert subscription state, but will not receive messages from +/// this channel: the cross-pod membership invalidation (step 1) ensures that +/// `filter_fanout_by_access` re-checks membership before delivery and denies +/// the removed user on every pod. The same pod-local eviction primitive is +/// used by the remove (kind 9001) and leave (kind 9022) paths; cross-pod +/// remote `CLOSED` frame delivery is not yet implemented (tracked separately). /// 3. `disable_departed_member_workflows` — durably disables any workflows the /// kicked user owned in this channel (SEC-006). /// From 733ff6eeb1d3a3adad529743d2da412103686f39 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Mon, 21 Sep 2026 11:49:17 -0400 Subject: [PATCH 10/13] fix(relay): recover kick actions created before migration 0047 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migration 0047 adds enforcement_target_pubkey/enforcement_channel_id to relay_admin_actions without backfilling existing rows. The old claim writer left those columns NULL, so any pending/enforcing kick action that pre-dates the migration has NULL/NULL context. The convergence gate in drive_enforcement read only the persisted columns and rejected NULL rows as corrupt, causing every recovery retry to fail forever. A cancellation cannot release a post-mutation action, so these rows would remain permanently stuck. Fix: when persisted columns are absent, fall back to the function parameters (which the recovery worker already re-derives from the report row). Error only when both sources are absent (genuinely unresolvable). This preserves the invariant that a new-writer row always uses persisted context, while allowing pre-migration rows to converge through the same recovery path. Also: - Rename migration 0045 → 0047 to avoid collision with two main migrations (0045_retain_push_revocation_tombstones, 0046_storage_accounting_snapshots) that merged during branch drift. - Move kick_live_side_effects_clears_membership_cache_and_evicts_subscription from the infra-free unit test lane to the PG fixture lane; the function calls membership_removal_fence which requires a real Postgres connection. - Add two new PG upgrade-recovery tests: legacy_kick_row_pre_marker and legacy_kick_row_post_marker, seeded through the old-writer shape (NULL enforcement columns), each falsifiable against the pre-fix convergence gate. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-relay/src/api/admin/mod.rs | 651 +++++++++++++++++- .../src/handlers/admin_action_worker.rs | 16 +- .../src/handlers/report_resolution.rs | 154 +---- 3 files changed, 682 insertions(+), 139 deletions(-) diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 89a19d13467..1586dbfc60b 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -6235,7 +6235,7 @@ mod postgres_tests { // Criterion 3 + Paul's mid-flight edge: a kick on an event report claims, // commits its mutation+marker, then the event row is HARD-purged before a // stranded re-drive. Because target_pubkey and channel_id are now persisted - // in relay_admin_actions at claim time (migration 0045), recovery no longer + // in relay_admin_actions at claim time (migration 0047), recovery no longer // re-derives them from the mutable event/report rows. All three live side // effects fire on re-drive: cache invalidation, subscription eviction, and // workflow disablement. The action converges to succeeded and report → resolved. @@ -9056,4 +9056,653 @@ mod postgres_tests { "mutation_committed marker must be preserved for retry when live effects fail" ); } + + // ── Pre-migration kick row upgrade-recovery (pre-marker) ───────────────── + + /// A kick action created by the old writer (before migration 0047 applied) + /// has NULL enforcement_target_pubkey and enforcement_channel_id. After the + /// migration the recovery worker finds this row pre-marker (no mutation + /// committed yet), re-derives the target from the report, and drives the full + /// state machine: kick executes, marker is set, live side effects fire, and + /// the action finalizes to succeeded. + /// + /// This test is falsifiable: reverting the convergence-gate fallback in + /// `drive_enforcement` (removing the `.or(target_pubkey)` / `.or(channel_id)` + /// lines) causes the gate to return the "unresolvable target" error instead + /// of calling `apply_kick_live_side_effects`, leaving the action stuck in + /// enforcing/mutation_committed. + #[tokio::test] + #[ignore = "requires Postgres — pre-migration NULL kick row converges via recovery worker (pre-marker path)"] + async fn legacy_kick_row_pre_marker_recovers_via_worker() { + let pool = e2e_pool().await; + let (community_id, _host) = e2e_community(&pool, "legacy-kick-pre-marker").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0xE1u8; 32]; + let actor = vec![0xE2u8; 32]; + + // Seed channel and member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'legacy-pre-marker-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + + // Claim via the normal path (populates enforcement columns), then clear + // them to simulate the old writer that did not know about migration 0047. + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + uuid::Uuid::new_v4(), + &actor, + "operator", + "kick", + None, + None, + "resolve:kick", + "relay_operator", + Some(&target), + None, + Some(channel_id), + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + + // Simulate old-writer shape: NULL out the persisted columns. + sqlx::query( + "UPDATE relay_admin_actions \ + SET enforcement_target_pubkey = NULL, enforcement_channel_id = NULL \ + WHERE id = $1", + ) + .bind(action_id) + .execute(&pool) + .await + .expect("null out enforcement columns (old-writer simulation)"); + + // No step_marker — pre-marker crash path. Expire the lease. + sqlx::query( + "UPDATE relay_admin_actions \ + SET action_lease_expires_at = $2, action_lease_token = NULL \ + WHERE id = $1", + ) + .bind(action_id) + .bind(chrono::Utc::now() - chrono::Duration::seconds(300)) + .execute(&pool) + .await + .expect("expire lease"); + + // Seed user row for workflow FK. + sqlx::query( + "INSERT INTO users (community_id, pubkey) VALUES ($1, $2) ON CONFLICT DO NOTHING", + ) + .bind(community_id) + .bind(&target) + .execute(&pool) + .await + .expect("seed user row"); + + // Build state with real DB; seed stale in-process entries. + let state = state_from_pool(pool.clone()).await; + + state + .membership_cache + .insert((cid, channel_id, target.clone()), true); + + let conn_id = uuid::Uuid::new_v4(); + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); + state.conn_manager.register( + conn_id, + tx, + ctrl_tx, + None, + tokio_util::sync::CancellationToken::new(), + cid, + std::sync::Arc::new(std::sync::atomic::AtomicU8::new(0)), + std::sync::Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())), + 3, + ); + state + .conn_manager + .set_authenticated_pubkey(conn_id, target.clone()); + state.sub_registry.register_channels_scoped( + cid, + conn_id, + "legacy-pre-marker-sub".to_string(), + vec![nostr::Filter::new()], + vec![channel_id], + ); + + let workflow_id = state + .db + .create_workflow( + cid, + Some(channel_id), + &target, + "legacy-pre-marker-workflow", + r#"{"kind":"workflow"}"#, + &[0u8; 32], + ) + .await + .expect("create workflow"); + + // Pre-conditions. + assert!( + state + .membership_cache + .get(&(cid, channel_id, target.clone())) + .is_some(), + "pre-condition: membership cache entry must exist" + ); + assert!( + state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "pre-condition: subscription must be registered" + ); + assert!( + state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "pre-condition: workflow must be enabled" + ); + + // Verify that the row has NULL enforcement columns before recovery. + let before = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action before") + .expect("row must exist"); + assert!( + before.enforcement_target_pubkey.is_none(), + "pre-condition: enforcement_target_pubkey must be NULL (old-writer simulation)" + ); + assert!( + before.enforcement_channel_id.is_none(), + "pre-condition: enforcement_channel_id must be NULL (old-writer simulation)" + ); + + // Re-drive via the recovery worker. + let batch = buzz_db::relay_admin_actions::claim_stranded_action_batch( + &pool, + "e2e-legacy-kick-pre-marker", + chrono::Utc::now() + chrono::Duration::seconds(120), + 1000, + ) + .await + .expect("claim_stranded_action_batch"); + let claim = batch + .into_iter() + .find(|c| c.record.id == action_id) + .expect("stranded action must appear in batch"); + crate::handlers::admin_action_worker::recover_one(&state, claim).await; + + // ── Assertions ─────────────────────────────────────────────────────── + + // 1. Action converged to succeeded. + let final_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("record must still exist"); + assert_eq!( + final_rec.state, "succeeded", + "legacy kick row (pre-marker) must converge to succeeded via recovery worker" + ); + + // 2. Membership cache cleared. + assert!( + state + .membership_cache + .get(&(cid, channel_id, target.clone())) + .is_none(), + "recovery must clear the membership cache for legacy kick row" + ); + + // 3. Channel subscription evicted. + assert!( + !state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "recovery must evict the kicked user's channel subscription for legacy kick row" + ); + + // 4. Workflow disabled. + assert!( + !state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "recovery must disable the kicked user's workflows for legacy kick row" + ); + } + + // ── Pre-migration kick row upgrade-recovery (post-marker) ──────────────── + + /// Same scenario as the pre-marker test, but the old writer committed both + /// the kick mutation AND the step_marker before crashing. After migration 0047 + /// the recovery worker picks up the stranded post-marker row, falls back to + /// the re-derived target, fires live side effects, and finalizes to succeeded. + /// + /// This test is falsifiable: reverting the convergence-gate fallback causes + /// the gate to see NULL persisted columns with no function-parameter fallback + /// and return the "unresolvable target" error, leaving the action stuck + /// forever in enforcing/mutation_committed. + #[tokio::test] + #[ignore = "requires Postgres — pre-migration NULL kick row converges via recovery worker (post-marker path)"] + async fn legacy_kick_row_post_marker_recovers_via_worker() { + let pool = e2e_pool().await; + let (community_id, _host) = e2e_community(&pool, "legacy-kick-post-marker").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let target = vec![0xE3u8; 32]; + let actor = vec![0xE4u8; 32]; + + // Seed channel and member. + let channel_id = uuid::Uuid::new_v4(); + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'legacy-post-marker-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target) + .execute(&pool) + .await + .expect("add member"); + + let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + + // Claim and execute the kick (mutation + marker), then NULL out the + // enforcement columns to simulate the old-writer shape. + let action_id = match buzz_db::relay_admin_actions::claim_report( + &pool, + cid, + report_id, + uuid::Uuid::new_v4(), + &actor, + "operator", + "kick", + None, + None, + "resolve:kick", + "relay_operator", + Some(&target), + None, + Some(channel_id), + ) + .await + .expect("claim") + { + buzz_db::relay_admin_actions::ClaimResult::Claimed(a) => a.id, + other => panic!("expected Claimed, got {other:?}"), + }; + + let _ = buzz_db::relay_admin_actions::begin_enforcing(&pool, action_id) + .await + .expect("begin_enforcing"); + let lease_until = chrono::Utc::now() + chrono::Duration::seconds(60); + let lease_token = + match buzz_db::relay_admin_actions::acquire_action_lease(&pool, action_id, lease_until) + .await + .expect("acquire lease") + { + buzz_db::relay_admin_actions::LeaseResult::Acquired(t) => t, + other => panic!("expected Acquired, got {other:?}"), + }; + + // Commit kick + step_marker — the post-marker crash point. + let kick_result = buzz_db::relay_admin_actions::execute_kick_with_marker( + &pool, + action_id, + lease_token, + cid, + channel_id, + &target, + &actor, + ) + .await + .expect("execute_kick_with_marker"); + assert!( + matches!( + kick_result, + buzz_db::relay_admin_actions::KickWithMarkerResult::Removed + ), + "kick must commit before simulated crash" + ); + + // Simulate old-writer shape: NULL out the enforcement columns. + sqlx::query( + "UPDATE relay_admin_actions \ + SET enforcement_target_pubkey = NULL, enforcement_channel_id = NULL \ + WHERE id = $1", + ) + .bind(action_id) + .execute(&pool) + .await + .expect("null out enforcement columns (old-writer simulation)"); + + // Expire the lease so the recovery worker can re-claim. + sqlx::query( + "UPDATE relay_admin_actions \ + SET action_lease_expires_at = $2, action_lease_token = NULL \ + WHERE id = $1", + ) + .bind(action_id) + .bind(chrono::Utc::now() - chrono::Duration::seconds(300)) + .execute(&pool) + .await + .expect("expire lease"); + + // Seed user row for workflow FK. + sqlx::query( + "INSERT INTO users (community_id, pubkey) VALUES ($1, $2) ON CONFLICT DO NOTHING", + ) + .bind(community_id) + .bind(&target) + .execute(&pool) + .await + .expect("seed user row"); + + // Build state with real DB; seed stale in-process entries. + let state = state_from_pool(pool.clone()).await; + + state + .membership_cache + .insert((cid, channel_id, target.clone()), true); + + let conn_id = uuid::Uuid::new_v4(); + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); + state.conn_manager.register( + conn_id, + tx, + ctrl_tx, + None, + tokio_util::sync::CancellationToken::new(), + cid, + std::sync::Arc::new(std::sync::atomic::AtomicU8::new(0)), + std::sync::Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())), + 3, + ); + state + .conn_manager + .set_authenticated_pubkey(conn_id, target.clone()); + state.sub_registry.register_channels_scoped( + cid, + conn_id, + "legacy-post-marker-sub".to_string(), + vec![nostr::Filter::new()], + vec![channel_id], + ); + + let workflow_id = state + .db + .create_workflow( + cid, + Some(channel_id), + &target, + "legacy-post-marker-workflow", + r#"{"kind":"workflow"}"#, + &[0u8; 32], + ) + .await + .expect("create workflow"); + + // Verify that the row is post-marker with NULL enforcement columns. + let before = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action before") + .expect("row must exist"); + assert_eq!( + before.step_marker.as_deref(), + Some("mutation_committed"), + "pre-condition: step_marker must be set (post-marker path)" + ); + assert!( + before.enforcement_target_pubkey.is_none(), + "pre-condition: enforcement_target_pubkey must be NULL (old-writer simulation)" + ); + assert!( + before.enforcement_channel_id.is_none(), + "pre-condition: enforcement_channel_id must be NULL (old-writer simulation)" + ); + + // Re-drive via the recovery worker. + let batch = buzz_db::relay_admin_actions::claim_stranded_action_batch( + &pool, + "e2e-legacy-kick-post-marker", + chrono::Utc::now() + chrono::Duration::seconds(120), + 1000, + ) + .await + .expect("claim_stranded_action_batch"); + let claim = batch + .into_iter() + .find(|c| c.record.id == action_id) + .expect("stranded action must appear in batch"); + crate::handlers::admin_action_worker::recover_one(&state, claim).await; + + // ── Assertions ─────────────────────────────────────────────────────── + + // 1. Action converged to succeeded. + let final_rec = buzz_db::relay_admin_actions::get_action(&pool, action_id) + .await + .expect("get_action") + .expect("record must still exist"); + assert_eq!( + final_rec.state, "succeeded", + "legacy kick row (post-marker) must converge to succeeded via recovery worker" + ); + + // 2. Membership cache cleared. + assert!( + state + .membership_cache + .get(&(cid, channel_id, target.clone())) + .is_none(), + "recovery must clear the membership cache for legacy kick row" + ); + + // 3. Channel subscription evicted. + assert!( + !state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "recovery must evict the kicked user's channel subscription for legacy kick row" + ); + + // 4. Workflow disabled. + assert!( + !state + .db + .get_workflow(cid, workflow_id) + .await + .expect("get workflow") + .enabled, + "recovery must disable the kicked user's workflows for legacy kick row" + ); + } + + // ── kick_live_side_effects: membership cache + subscription eviction ────── + + /// Verify that `apply_kick_live_side_effects` clears the membership cache + /// entry and evicts the live channel subscription for the kicked user. + /// + /// Moved from `handlers::report_resolution` tests (which used `test_state()` + /// with a lazy PG pool) to the PG fixture lane, because + /// `apply_kick_live_side_effects` → `membership_removal_fence` requires a + /// real Postgres connection. + /// + /// Setup: + /// 1. Seed the membership cache with `true` so the cache claims the target + /// is still a member. + /// 2. Register a connection authenticated as the target pubkey and add a + /// channel-scoped subscription for them. + /// 3. Call `apply_kick_live_side_effects`. + /// + /// Assertions: + /// - The membership cache entry is gone (cache returns `None`). + /// - The channel subscription index no longer lists the connection. + /// + /// Redis-dependent work inside the helper (cross-pod cache invalidation + /// publish, pubsub topic release) hits an intentionally unreachable endpoint + /// and is silently dropped — this mirrors the production "best-effort" + /// contract and does not affect the in-process assertions. + /// + /// This test is falsifiable: replacing `membership_removal_fence` with an + /// always-fire eviction path (bypassing the `still_removed` gate) would + /// leave this test green, but + /// `crash_recovery_after_readd_preserves_membership_subscriptions_and_workflows` + /// covers the fence semantics. + #[tokio::test] + #[ignore = "requires Postgres — kick live side effects clear cache and evict subscription"] + async fn kick_live_side_effects_clears_membership_cache_and_evicts_subscription() { + let pool = e2e_pool().await; + let (community_id, host) = e2e_community(&pool, "kick-side-effects-unit").await; + let cid = buzz_core::CommunityId::from_uuid(community_id); + let channel_id = uuid::Uuid::new_v4(); + let target_pubkey: Vec = vec![0xABu8; 32]; + let actor: Vec = vec![0xACu8; 32]; + let tenant = buzz_core::tenant::TenantContext::resolved(cid, host); + + // Create channel and seed the target as a member so the fence query + // finds a removed_at IS NULL row (kick has already committed in DB but + // we need a member row for the fence to read). + sqlx::query( + r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) + VALUES ($1, $2, 'side-effects-unit-ch', 'stream', 'open', $3)"#, + ) + .bind(channel_id) + .bind(community_id) + .bind(&actor) + .execute(&pool) + .await + .expect("create channel"); + // Insert already-removed member row (removed_at set) — simulates state + // after a kick mutation committed but before side effects ran. + sqlx::query( + "INSERT INTO channel_members (community_id, channel_id, pubkey, role, removed_at) \ + VALUES ($1, $2, $3, 'member', now())", + ) + .bind(community_id) + .bind(channel_id) + .bind(&target_pubkey) + .execute(&pool) + .await + .expect("insert removed member row"); + + let state = state_from_pool(pool.clone()).await; + + // 1. Seed the membership cache. + state + .membership_cache + .insert((cid, channel_id, target_pubkey.clone()), true); + + assert!( + state + .membership_cache + .get(&(cid, channel_id, target_pubkey.clone())) + .is_some(), + "pre-condition: membership cache entry must exist before kick side effects" + ); + + // 2. Register a connection and a channel-scoped subscription. + let conn_id = uuid::Uuid::new_v4(); + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); + state.conn_manager.register( + conn_id, + tx, + ctrl_tx, + None, + tokio_util::sync::CancellationToken::new(), + cid, + std::sync::Arc::new(std::sync::atomic::AtomicU8::new(0)), + std::sync::Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())), + 3, + ); + state + .conn_manager + .set_authenticated_pubkey(conn_id, target_pubkey.clone()); + + state.sub_registry.register_channels_scoped( + cid, + conn_id, + "kick-side-effects-sub".to_string(), + vec![nostr::Filter::new()], + vec![channel_id], + ); + + assert!( + state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "pre-condition: subscription must be registered before kick side effects" + ); + + // 3. Fire kick live side effects. + crate::handlers::side_effects::apply_kick_live_side_effects( + &tenant, + &state, + channel_id, + &target_pubkey, + ) + .await + .expect("kick live side effects must succeed in test"); + + // Assert: membership cache entry is gone. + assert!( + state + .membership_cache + .get(&(cid, channel_id, target_pubkey.clone())) + .is_none(), + "membership cache must not contain a stale entry after kick side effects" + ); + + // Assert: channel subscription is no longer indexed for this connection. + assert!( + !state + .sub_registry + .channel_subscriber_conns_scoped(cid, channel_id) + .contains(&conn_id), + "kicked user's channel subscription must be evicted after kick side effects" + ); + } } diff --git a/crates/buzz-relay/src/handlers/admin_action_worker.rs b/crates/buzz-relay/src/handlers/admin_action_worker.rs index 364bcfbf44a..42c3647b2cc 100644 --- a/crates/buzz-relay/src/handlers/admin_action_worker.rs +++ b/crates/buzz-relay/src/handlers/admin_action_worker.rs @@ -102,7 +102,7 @@ pub(crate) async fn recover_one(state: &Arc, claim: StrandedActionClai ); // Use the target context persisted at claim time for kick actions only. - // Migration 0045 added enforcement_target_pubkey/enforcement_channel_id + // Migration 0047 added enforcement_target_pubkey/enforcement_channel_id // for kicks; other actions (ban, timeout, delete) do not set these columns // and must re-derive from the report row on every recovery. Applying the // persisted-context branch to non-kick actions breaks delete recovery: @@ -111,6 +111,11 @@ pub(crate) async fn recover_one(state: &Arc, claim: StrandedActionClai // target_event_id=None, causing pre-marker delete recovery to fail // ("delete requires target_event_id") or post-marker recovery to skip the // tombstone outbox row. Gate strictly on action=="kick". + // + // For pre-migration kick rows (both columns NULL), we still re-derive and + // pass the result as function parameters; the convergence gate in + // drive_enforcement accepts those as a legacy-context fallback so these + // stranded kicks can finalize without the persisted columns being populated. let (target_pubkey_opt, target_event_id_opt, channel_id) = if rec.action == "kick" && (rec.enforcement_target_pubkey.is_some() || rec.enforcement_channel_id.is_some()) { @@ -122,12 +127,9 @@ pub(crate) async fn recover_one(state: &Arc, claim: StrandedActionClai ) } else { // Non-kick action, or pre-migration kick row with both columns NULL. - // Non-kick actions: re-derive from the report — all other action types - // (ban, timeout, delete) do not persist context and must always derive. - // Pre-migration kick rows (both NULL): re-derive also, but note these - // rows cannot finalize — convergence requires rec.enforcement_target_pubkey - // and rec.enforcement_channel_id (invariant error if absent). Pre-migration - // stranded kicks should be effectively zero at deploy time. + // Non-kick actions: always re-derive from the report. + // Pre-migration kick rows: re-derive so the convergence gate can use the + // result as a legacy-context fallback (see report_resolution.rs). let report = match state.db.admin_get_report(rec.report_id).await { Ok(Some(r)) => r, Ok(None) => { diff --git a/crates/buzz-relay/src/handlers/report_resolution.rs b/crates/buzz-relay/src/handlers/report_resolution.rs index 1d806a4c79a..8b77eb77462 100644 --- a/crates/buzz-relay/src/handlers/report_resolution.rs +++ b/crates/buzz-relay/src/handlers/report_resolution.rs @@ -617,12 +617,15 @@ async fn drive_enforcement( // effects; recovery worker re-enters here directly with marker set). // // Live side effects for kick use the target context persisted at claim - // time (enforcement_target_pubkey / enforcement_channel_id) rather than - // the function parameters, which on the recovery path are re-derived from - // mutable sources that may have changed or been purged since the kick - // committed. Missing persisted context is an invariant failure: the - // INSERT that claimed the action required both values and stored them; if - // they are absent the row is corrupt and we must not silently succeed. + // time (enforcement_target_pubkey / enforcement_channel_id) when present. + // Migration 0047 added these columns without backfilling, so rows written + // before the migration have NULL/NULL. On the recovery path the worker + // re-derives the target from the report and passes it as the function + // parameters; we accept those as a legacy-context fallback so pre-migration + // stranded kicks can still converge. Both persisted context (preferred) + // and derived-parameter fallback must satisfy the non-NULL/non-NULL + // requirement; if neither can supply the target the row is genuinely + // unresolvable and we must not silently succeed. // // Eviction and workflow-disable are fenced behind membership_removal_fence // (which holds the per-channel advisory lock through both effects) so a @@ -631,10 +634,13 @@ async fn drive_enforcement( // stale-positive is always safe to drop. The fence applies on every path // (fresh and recovery) for a single consistent ordering guarantee. if action == "kick" { - match ( - rec.enforcement_target_pubkey.as_deref(), - rec.enforcement_channel_id, - ) { + // Prefer the persisted context (accurate at claim time, immune to + // later report/member mutations). Fall back to the function parameters + // when the row pre-dates migration 0047 and those columns are NULL. + let kick_target = rec.enforcement_target_pubkey.as_deref().or(target_pubkey); + let kick_channel = rec.enforcement_channel_id.or(channel_id); + + match (kick_target, kick_channel) { (Some(target), Some(ch)) => { crate::handlers::side_effects::apply_kick_live_side_effects( tenant, state, ch, target, @@ -648,10 +654,14 @@ async fn drive_enforcement( })?; } _ => { + // Both persisted columns and function parameters are absent. + // The recovery worker could not resolve a target from the + // report, so this action cannot be finalized safely. return Err(ResolutionError::Internal(format!( - "kick action {action_id} reached convergence with missing \ - enforcement_target_pubkey or enforcement_channel_id — \ - action row is corrupt; refusing to finalize as succeeded" + "kick action {action_id} reached convergence with unresolvable \ + target: enforcement_target_pubkey and enforcement_channel_id are \ + absent from both the row and the derived function parameters — \ + cannot finalize; manual intervention required" ))); } } @@ -1072,122 +1082,4 @@ mod tests { "worker re-derive must match the HTTP claim derivation exactly" ); } - - /// Verify that `apply_kick_live_side_effects` drops the membership cache - /// entry and evicts the live channel subscription for the kicked user. - /// - /// Setup: - /// 1. Seed the membership cache with `true` so the cache claims the target - /// is still a member. - /// 2. Register a connection authenticated as the target pubkey and add a - /// channel-scoped subscription for them. - /// 3. Call `apply_kick_live_side_effects`. - /// - /// Assertions: - /// - The membership cache entry is gone (cache returns `None`). - /// - The channel subscription index no longer lists the connection. - /// - /// Redis-dependent work inside the helper (cross-pod cache invalidation - /// publish, pubsub topic release) hits an intentionally unreachable endpoint - /// and is silently dropped — this mirrors the production "best-effort" - /// contract and does not affect the in-process assertions. - #[tokio::test] - async fn kick_live_side_effects_clears_membership_cache_and_evicts_subscription() { - use buzz_core::tenant::CommunityId; - use std::sync::atomic::AtomicU8; - use std::sync::Arc; - use tokio::sync::Mutex; - use tokio_util::sync::CancellationToken; - use uuid::Uuid; - - let state = crate::state::tests::test_state().await; - - let community_id = CommunityId::from_uuid(Uuid::from_u128(0xCAFE_BABE)); - let channel_id = Uuid::from_u128(0x1234_5678); - let target_pubkey: Vec = vec![0xABu8; 32]; - let tenant = buzz_core::tenant::TenantContext::resolved(community_id, "kick-test.example"); - - // 1. Seed the membership cache — simulates a cache hit that would keep - // the kicked user appearing as a member after the DB write. - state - .membership_cache - .insert((community_id, channel_id, target_pubkey.clone()), true); - - // Confirm the entry is visible before the side effects run. - assert!( - state - .membership_cache - .get(&(community_id, channel_id, target_pubkey.clone())) - .is_some(), - "pre-condition: membership cache entry must exist before kick" - ); - - // 2. Register a connection authenticated as the target pubkey and add a - // channel-scoped subscription so eviction has something to remove. - let conn_id = Uuid::new_v4(); - let (tx, _rx) = tokio::sync::mpsc::channel(1); - let (ctrl_tx, _ctrl_rx) = tokio::sync::mpsc::channel(1); - state.conn_manager.register( - conn_id, - tx, - ctrl_tx, - None, - CancellationToken::new(), - community_id, - Arc::new(AtomicU8::new(0)), - Arc::new(Mutex::new(std::collections::HashMap::new())), - 3, - ); - state - .conn_manager - .set_authenticated_pubkey(conn_id, target_pubkey.clone()); - - let sub_id = "kick-test-sub".to_string(); - state.sub_registry.register_channels_scoped( - community_id, - conn_id, - sub_id, - // One unconstrained filter (no `kinds`) hits the wildcard index, - // making the subscription visible in channel_subscriber_conns_scoped. - vec![nostr::Filter::new()], - vec![channel_id], - ); - - // Confirm subscription is visible before side effects run. - assert!( - state - .sub_registry - .channel_subscriber_conns_scoped(community_id, channel_id) - .contains(&conn_id), - "pre-condition: subscription must be registered before kick" - ); - - // 3. Fire kick live side effects. - crate::handlers::side_effects::apply_kick_live_side_effects( - &tenant, - &state, - channel_id, - &target_pubkey, - ) - .await - .expect("kick live side effects must succeed in test"); - - // Assert: membership cache entry is gone. - assert!( - state - .membership_cache - .get(&(community_id, channel_id, target_pubkey.clone())) - .is_none(), - "membership cache must not contain a stale entry after kick side effects" - ); - - // Assert: channel subscription is no longer indexed for this connection. - assert!( - !state - .sub_registry - .channel_subscriber_conns_scoped(community_id, channel_id) - .contains(&conn_id), - "kicked user's channel subscription must be evicted after kick side effects" - ); - } } From cef249d6b8b0308ab084ccdb1b7b6d2087874343 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Mon, 21 Sep 2026 12:29:06 -0400 Subject: [PATCH 11/13] test(relay): rebuild legacy-kick fixtures with event reports that carry channel_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both legacy-kick recovery tests previously seeded via e2e_report_pubkey(), which inserts a pubkey report without channel_id. The recovery worker re-derives channel_id from report.report.channel_id — None for pubkey reports — so kick's 'requires channel_id' guard rejected both fixtures before the convergence gate could be exercised. Fix: seed through e2e_event_report_with_author() instead. The helper inserts a channel, adds the author as a member, inserts the target event, and creates an event report with the channel FK populated. derive_enforcement_target_pub returns Some(author) via the events JOIN. Both fixtures now produce a complete enforcement context (pubkey + channel_id) when the convergence gate falls back to function parameters — exactly what the fix needs to exercise. Also fix schema.sql:1809 comment: 'migration 0045' → 'migration 0047'. Falsifiability re-established: removing the .or(target_pubkey) and .or(channel_id) fallbacks causes both tests to fail with the action remaining in 'enforcing' state (unresolvable-target error path). Confirmed locally: GREEN at fixed head, RED with fallbacks reverted. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-relay/src/api/admin/mod.rs | 60 ++++++-------------------- schema/schema.sql | 2 +- 2 files changed, 15 insertions(+), 47 deletions(-) diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 1586dbfc60b..e9175d92ae5 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -9080,29 +9080,13 @@ mod postgres_tests { let target = vec![0xE1u8; 32]; let actor = vec![0xE2u8; 32]; - // Seed channel and member. - let channel_id = uuid::Uuid::new_v4(); - sqlx::query( - r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) - VALUES ($1, $2, 'legacy-pre-marker-ch', 'stream', 'open', $3)"#, - ) - .bind(channel_id) - .bind(community_id) - .bind(&actor) - .execute(&pool) - .await - .expect("create channel"); - sqlx::query( - "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", - ) - .bind(community_id) - .bind(channel_id) - .bind(&target) - .execute(&pool) - .await - .expect("add member"); - - let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + // Seed an event report with author=target so: + // 1. report.report.channel_id is non-NULL (kick requires it) + // 2. derive_enforcement_target_pub returns Some(target) via the event + // author join, giving the convergence gate a real pubkey fallback + // 3. target is already a channel member (helper seeds it) + let (report_id, channel_id, _) = + e2e_event_report_with_author(&pool, community_id, &target).await; // Claim via the normal path (populates enforcement columns), then clear // them to simulate the old writer that did not know about migration 0047. @@ -9323,29 +9307,13 @@ mod postgres_tests { let target = vec![0xE3u8; 32]; let actor = vec![0xE4u8; 32]; - // Seed channel and member. - let channel_id = uuid::Uuid::new_v4(); - sqlx::query( - r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) - VALUES ($1, $2, 'legacy-post-marker-ch', 'stream', 'open', $3)"#, - ) - .bind(channel_id) - .bind(community_id) - .bind(&actor) - .execute(&pool) - .await - .expect("create channel"); - sqlx::query( - "INSERT INTO channel_members (community_id, channel_id, pubkey, role) VALUES ($1, $2, $3, 'member')", - ) - .bind(community_id) - .bind(channel_id) - .bind(&target) - .execute(&pool) - .await - .expect("add member"); - - let report_id = e2e_report_pubkey(&pool, community_id, &target).await; + // Seed an event report with author=target so: + // 1. report.report.channel_id is non-NULL (kick requires it) + // 2. derive_enforcement_target_pub returns Some(target) via the event + // author join, giving the convergence gate a real pubkey fallback + // 3. target is already a channel member (helper seeds it) + let (report_id, channel_id, _) = + e2e_event_report_with_author(&pool, community_id, &target).await; // Claim and execute the kick (mutation + marker), then NULL out the // enforcement columns to simulate the old-writer shape. diff --git a/schema/schema.sql b/schema/schema.sql index 9fdc892becd..21d0d2ea8ea 100644 --- a/schema/schema.sql +++ b/schema/schema.sql @@ -1806,7 +1806,7 @@ 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 0045): persisted at claim time + -- 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 From 7bb0305195ac166573487248c013471cacdd6dac Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Mon, 21 Sep 2026 12:50:06 -0400 Subject: [PATCH 12/13] fix(db): rename migration 0045_relay_admin_action_target to 0047 The branch introduced this migration as 0045, but main merged two migrations with the same prefix while the branch was open (0045_retain_push_revocation_tombstones, 0046_storage_accounting_snapshots). The merge replayed the collision. Rename to 0047 to restore uniqueness and correct ordering. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- ...admin_action_target.sql => 0047_relay_admin_action_target.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename migrations/{0045_relay_admin_action_target.sql => 0047_relay_admin_action_target.sql} (100%) diff --git a/migrations/0045_relay_admin_action_target.sql b/migrations/0047_relay_admin_action_target.sql similarity index 100% rename from migrations/0045_relay_admin_action_target.sql rename to migrations/0047_relay_admin_action_target.sql From 304d5e6e6c28bbc0b97b950a0c0c0af425f3da62 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Mon, 21 Sep 2026 13:25:25 -0400 Subject: [PATCH 13/13] test(db): fix admin_schema_parity test to run_to(47) The DCO rebase regressed this test from run_to(47) to run_to(45). Migration 0047_relay_admin_action_target is the highest migration in this branch, so the parity test must run through 47 to validate the admin table columns and index shapes added by that migration. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-db/src/runtime/migration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/buzz-db/src/runtime/migration.rs b/crates/buzz-db/src/runtime/migration.rs index 2c9944fb9eb..97c619d6edb 100644 --- a/crates/buzz-db/src/runtime/migration.rs +++ b/crates/buzz-db/src/runtime/migration.rs @@ -2259,9 +2259,9 @@ mod postgres_tests { .await .expect("connect migrated probe database"); MIGRATOR - .run_to(45, &migrated) + .run_to(47, &migrated) .await - .expect("apply migrations 1-45"); + .expect("apply migrations 1-47"); for table in [ "relay_admin_actions",