From 2da7cac273c69ec61c850c1829366e3c6599d873 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 29 Oct 2026 17:06:26 +1100 Subject: [PATCH] test(db): cover the kind:9000 archive race and tighten the archive-race waits Address the review of af4730a54 on #8594. The lock order holds; these are the comment, coverage and doc gaps it found. - `put_member_waits_for_a_concurrent_archive` now drives `put_members_with_event` against a second uncommitted archive. The kind:9000 add waits for it, then returns `Refused` with one `AccessDenied("channel is archived")` and stores no event. It gets its own archive because a write queued behind the join's membership lock would wait whether or not its own call site took the row lock. Reverting that call site to the membership lock alone fails the test (verified by mutation): the add never waits. - The re-add refusal in the same test is pinned to `"channel is archived"`, not any `AccessDenied`. - `auto_join_skips_a_default_channel_archived_while_it_joins` waits for a `Lock` wait on the `SELECT archived_at FROM channels ... FOR NO KEY UPDATE` statement, not for any session the archive blocks. - The `CHANNEL_MEMBERSHIP_LOCK_NAMESPACE` rationale says admissions also lock the channel row (`lock_channel_for_member_put`) so an archive is ordered against them, and that only removals take just this key. - NOSTR.md "Default channels": the auto-join paragraph is rewrapped, and its dangling "It" now says "The relay". Verified: fmt; clippy -D warnings --all-targets on buzz-db and buzz-relay; the buzz-db `channel_members` Postgres tests (32 passed) and the four buzz-relay auto_join Postgres tests. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matt Toohey --- NOSTR.md | 22 ++--- crates/buzz-db/src/store/channel_members.rs | 99 ++++++++++++++++++--- crates/buzz-relay/src/api/invites.rs | 9 +- 3 files changed, 103 insertions(+), 27 deletions(-) diff --git a/NOSTR.md b/NOSTR.md index 0973bba412..4047e2248f 100644 --- a/NOSTR.md +++ b/NOSTR.md @@ -222,17 +222,17 @@ body of kind:9021, so the channel's `join_role` applies and the channel gets its `member_joined` kind:40099, kind:39002 and kind:44100. There is no cap on how many channels that is: whatever carries the kind:39000 tag is joined, and the work a new member costs grows with the number of channels an owner or -admin flags. It does this once, -at that moment, and never again on login or reconnect, so a later kind:9022 -leave stands. A channel that fails to join is logged and counted -(`buzz_auto_join_failures_total`), not retried: the kind:39000 tag is the -record a client reconciles against. That includes a channel archived or made -private while the join runs: the membership write decides both again under -the channel's locks, so the member is not added and nothing is announced. `buzz-admin add-member` writes the -roster row only and does not join default channels. Auto-join fires only at -those roster-insert sites, so a community that does not require relay -membership advertises the kind:39000 tag but auto-joins nobody; a client that -wants the same result there self-joins (kind:9021) on the tag. +admin flags. The relay does this once, at that moment, and never again on +login or reconnect, so a later kind:9022 leave stands. A channel that fails +to join is logged and counted (`buzz_auto_join_failures_total`), not retried: +the kind:39000 tag is the record a client reconciles against. That includes a +channel archived or made private while the join runs: the membership write +decides both again under the channel's locks, so the member is not added and +nothing is announced. `buzz-admin add-member` writes the roster row only and +does not join default channels. Auto-join fires only at those roster-insert +sites, so a community that does not require relay membership advertises the +kind:39000 tag but auto-joins nobody; a client that wants the same result +there self-joins (kind:9021) on the tag. ### Group Discovery diff --git a/crates/buzz-db/src/store/channel_members.rs b/crates/buzz-db/src/store/channel_members.rs index ca95fb88fa..272c0cbf14 100644 --- a/crates/buzz-db/src/store/channel_members.rs +++ b/crates/buzz-db/src/store/channel_members.rs @@ -41,9 +41,11 @@ pub struct MemberRecord { /// concurrent demotions (or a demotion racing a removal) to each observe two /// owners, each pass, and together leave zero — the exact governance loss the /// guards exist to prevent. An advisory key rather than `SELECT ... FOR UPDATE` -/// on the channel row: membership is its own contention domain and must not -/// serialize against unrelated channel metadata writers (`update_channel`, -/// `set_topic`, the TTL transition). Distinct key domain from +/// on the channel row, so a removal, which takes only this key, does not +/// serialize against the row's writers that leave admission alone +/// (`set_topic`, `set_canvas`, the TTL transition). Admissions also lock the +/// row, after this key (see [`lock_channel_for_member_put`]), because the row +/// is what orders an archive against them. Distinct key domain from /// `buzz_channel_ttl:`. const CHANNEL_MEMBERSHIP_LOCK_NAMESPACE: &str = "buzz_channel_membership:"; @@ -3885,22 +3887,28 @@ mod postgres_tests { /// An archive and an admission are ordered by the channel row lock, and an /// archived channel admits nobody new. A join that waits on an uncommitted /// archive is refused once it commits, with no row, and re-adding a - /// removed member is refused too. The other order: an archive waits for - /// an admission that holds the locks. + /// removed member is refused too. A kind:9000 add that waits on an + /// uncommitted archive is refused the same way, and its event is not + /// stored. The other order: an archive waits for an admission that holds + /// the locks. /// The archive here is the TTL reaper's single UPDATE, which takes no - /// advisory lock. Mutations: drop the row lock from - /// `lock_channel_for_member_put`, or the archived refusal from - /// `put_members_on` → RED. + /// advisory lock. The join and the kind:9000 add each wait on their own + /// archive, so neither queues behind the other's membership lock. + /// Mutations: drop the row lock from `lock_channel_for_member_put`, take + /// only the membership lock in `put_members_with_event`, or drop the + /// archived refusal from `put_members_on` → RED. #[tokio::test] #[ignore = "requires Postgres"] async fn put_member_waits_for_a_concurrent_archive() { let pool = setup_pool().await; let community_id = make_test_community(&pool).await; let community = CommunityId::from_uuid(community_id); - let owner = random_pubkey(); + let owner_keys = Keys::generate(); + let owner = owner_keys.public_key().to_bytes().to_vec(); let joiner = random_pubkey(); let removed = random_pubkey(); - for pk in [&owner, &joiner, &removed] { + let invitee = random_pubkey(); + for pk in [&owner, &joiner, &removed, &invitee] { ensure_user(&pool, community, pk) .await .expect("ensure user"); @@ -3956,16 +3964,81 @@ mod postgres_tests { let readd = put_member(&pool, community, channel.id, &removed, None, Some(&owner)) .await .expect_err("an archived channel does not reactivate a removed member"); - assert!(matches!(readd, DbError::AccessDenied(_)), "{readd:?}"); + assert!( + matches!(&readd, DbError::AccessDenied(reason) if reason == "channel is archived"), + "{readd:?}" + ); + + // A kind:9000 add stores its event in the same transaction as the + // write, so it takes the same locks: it waits on an uncommitted + // archive, then is refused and stores nothing. + crate::channel::unarchive_channel(&pool, community, channel.id) + .await + .expect("unarchive before the kind:9000 race"); + let mut archive = pool.begin().await.expect("begin second archive"); + sqlx::query( + "UPDATE channels SET archived_at = NOW() \ + WHERE community_id = $1 AND id = $2 AND archived_at IS NULL", + ) + .bind(community_id) + .bind(channel.id) + .execute(&mut *archive) + .await + .expect("archive again"); + let add = nostr::EventBuilder::new(nostr::Kind::Custom(9000), "") + .tags([ + nostr::Tag::parse(["h", &channel.id.to_string()]).unwrap(), + nostr::Tag::parse(["p", &hex::encode(&invitee)]).unwrap(), + ]) + .sign_with_keys(&owner_keys) + .expect("sign 9000"); + let add_pool = crate::test_support::named_pool("put-members-event-archive-race").await; + let add_write = tokio::spawn({ + let channel_id = channel.id; + let add = add.clone(); + let invitee = invitee.clone(); + async move { + let targets = [MemberPut { + pubkey: &invitee, + requested: None, + }]; + put_members_with_event(&add_pool, None, community, channel_id, &add, &targets, None) + .await + } + }); + crate::test_support::wait_for_lock_wait(&pool, "put-members-event-archive-race").await; + assert!( + !add_write.is_finished(), + "the kind:9000 add must wait for the archive" + ); + archive.commit().await.expect("commit second archive"); + let write = add_write + .await + .expect("kind:9000 task") + .expect("kind:9000 write"); + assert!( + matches!(&write, MemberPutEventWrite::Refused(errors) + if matches!(errors.as_slice(), + [DbError::AccessDenied(reason)] if reason == "channel is archived")), + "expected an archived refusal" + ); + assert!( + crate::event::get_event_by_id(&pool, community, add.id.as_bytes()) + .await + .expect("read event") + .is_none(), + "a refused kind:9000 is not stored" + ); + let mut roles = Vec::new(); - for pk in [&owner, &joiner, &removed] { + for pk in [&owner, &joiner, &removed, &invitee] { roles.push( get_member_role(&pool, community, channel.id, pk) .await .expect("read role"), ); } - assert_eq!(roles, [Some("owner".to_string()), None, None]); + assert_eq!(roles, [Some("owner".to_string()), None, None, None]); // The other order: an archive waits for an admission that holds the // locks, and the member it admitted stays a member. diff --git a/crates/buzz-relay/src/api/invites.rs b/crates/buzz-relay/src/api/invites.rs index 7d8cc7648d..c770448bdd 100644 --- a/crates/buzz-relay/src/api/invites.rs +++ b/crates/buzz-relay/src/api/invites.rs @@ -1532,13 +1532,16 @@ mod postgres_tests { let joiner_hex = joiner_hex.clone(); async move { auto_join_default_channels(&tenant, &state, &joiner_hex).await } }); - // Wait until the join is blocked behind the archive, so it has - // already listed the channel and passed the join body's check. + // Wait until the join is blocked on the channel row lock in + // `put_member`, behind the archive, so it has already listed the + // channel and passed the join body's check. let deadline = std::time::Instant::now() + Duration::from_secs(10); loop { let waiting: bool = sqlx::query_scalar( "SELECT EXISTS (SELECT 1 FROM pg_stat_activity \ - WHERE $1 = ANY(pg_blocking_pids(pid)))", + WHERE wait_event_type = 'Lock' \ + AND $1 = ANY(pg_blocking_pids(pid)) \ + AND query LIKE 'SELECT archived_at FROM channels %FOR NO KEY UPDATE')", ) .bind(archive_pid) .fetch_one(&pool)