Skip to content

Commit 22be8bb

Browse files
tlongwell-blocknpub12gtutshhh76rx0jx697f32f9tffd4hhp3hx58fp4x6u4uemkm7sqf8f757npub1jh9wn95s0472h86ahapupaf7m6kx4v9sx2n0atj2hltcfer8k06s5n3pyf
authored
fix(relay): avoid subscription lock inversion (#3413)
## Summary - drop the `subs` DashMap guard before mutating subscription indexes - snapshot fan-out candidate vectors so index guards are dropped before looking up `subs` - add concurrent fan-out/replacement regression coverage ## Why `fan_out_scoped` previously held an index guard while `push_match` acquired `subs`, while CLOSE and same-ID replacement held `subs` while removing from an index. The reverse ordering made an AB/BA deadlock reachable and could synchronously park all Tokio workers. ## Validation - `rustup run 1.95.0 cargo test -p buzz-relay` — 769 library tests passed, 33 ignored; 11 binary tests passed; doc tests passed - push hooks with pinned Rust 1.95 — branch-skew, repository Rust suites, and desktop Tauri suite passed - `git diff --check` ## Residual risk Fan-out now clones bounded candidate vectors before matching. This adds allocation/copy cost proportional to the indexed candidate set, in exchange for eliminating nested DashMap guards. This fixes the concrete lock cycle but does not prove every observed production wedge had this cause. --------- Signed-off-by: npub12gtutshhh76rx0jx697f32f9tffd4hhp3hx58fp4x6u4uemkm7sqf8f757 <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Signed-off-by: npub1jh9wn95s0472h86ahapupaf7m6kx4v9sx2n0atj2hltcfer8k06s5n3pyf <95cae996907d7cab9f5dbf43c0f53edeac6ab0b032a6feae4abfd784e467b3f5@buzz.block.builderlab.xyz> Co-authored-by: npub12gtutshhh76rx0jx697f32f9tffd4hhp3hx58fp4x6u4uemkm7sqf8f757 <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Co-authored-by: npub1jh9wn95s0472h86ahapupaf7m6kx4v9sx2n0atj2hltcfer8k06s5n3pyf <95cae996907d7cab9f5dbf43c0f53edeac6ab0b032a6feae4abfd784e467b3f5@buzz.block.builderlab.xyz>
1 parent 90e058e commit 22be8bb

1 file changed

Lines changed: 236 additions & 27 deletions

File tree

crates/buzz-relay/src/subscription.rs

Lines changed: 236 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -164,17 +164,30 @@ impl SubscriptionRegistry {
164164
conn_id: ConnId,
165165
sub_id: &str,
166166
) -> Option<RemovedSubscription> {
167-
if let Some(mut conn_subs) = self.subs.get_mut(&conn_id) {
168-
if let Some((filters, community_id, channel_id)) = conn_subs.remove(sub_id) {
169-
self.remove_from_index(conn_id, sub_id, &filters, community_id, channel_id);
170-
metrics::gauge!("buzz_subscriptions_active").decrement(1.0);
171-
return Some(RemovedSubscription {
172-
community_id,
173-
channel_id,
174-
});
175-
}
176-
}
177-
None
167+
self.remove_subscription_inner(conn_id, sub_id, || {})
168+
}
169+
170+
fn remove_subscription_inner<F>(
171+
&self,
172+
conn_id: ConnId,
173+
sub_id: &str,
174+
after_remove: F,
175+
) -> Option<RemovedSubscription>
176+
where
177+
F: FnOnce(),
178+
{
179+
let mut conn_subs = self.subs.get_mut(&conn_id)?;
180+
let (filters, community_id, channel_id) = conn_subs.remove(sub_id)?;
181+
182+
after_remove();
183+
self.remove_from_index(conn_id, sub_id, &filters, community_id, channel_id);
184+
drop(conn_subs);
185+
186+
metrics::gauge!("buzz_subscriptions_active").decrement(1.0);
187+
Some(RemovedSubscription {
188+
community_id,
189+
channel_id,
190+
})
178191
}
179192

180193
/// Remove all subscriptions for a connection and clean up index entries.
@@ -275,15 +288,37 @@ impl SubscriptionRegistry {
275288
channel_id,
276289
kind: event.event.kind,
277290
};
278-
if let Some(candidates) = self.channel_kind_index.get(&(community_id, key)) {
279-
for (conn_id, sub_id) in candidates.iter() {
280-
self.push_match(*conn_id, sub_id, event, &mut results, &mut seen);
291+
if let Some(candidates) = self
292+
.channel_kind_index
293+
.get(&(community_id, key))
294+
.map(|entry| entry.value().clone())
295+
{
296+
for (conn_id, sub_id) in candidates {
297+
self.push_match(
298+
conn_id,
299+
&sub_id,
300+
community_id,
301+
event,
302+
&mut results,
303+
&mut seen,
304+
);
281305
}
282306
}
283307
// Also check wildcard (channel-only, kindless) index.
284-
if let Some(wildcards) = self.channel_wildcard_index.get(&(community_id, channel_id)) {
285-
for (conn_id, sub_id) in wildcards.iter() {
286-
self.push_match(*conn_id, sub_id, event, &mut results, &mut seen);
308+
if let Some(wildcards) = self
309+
.channel_wildcard_index
310+
.get(&(community_id, channel_id))
311+
.map(|entry| entry.value().clone())
312+
{
313+
for (conn_id, sub_id) in wildcards {
314+
self.push_match(
315+
conn_id,
316+
&sub_id,
317+
community_id,
318+
event,
319+
&mut results,
320+
&mut seen,
321+
);
287322
}
288323
}
289324
} else {
@@ -296,24 +331,54 @@ impl SubscriptionRegistry {
296331
kind: event.event.kind,
297332
p,
298333
};
299-
if let Some(candidates) = self.global_p_kind_index.get(&key) {
300-
for (conn_id, sub_id) in candidates.iter() {
301-
self.push_match(*conn_id, sub_id, event, &mut results, &mut seen);
334+
if let Some(candidates) = self
335+
.global_p_kind_index
336+
.get(&key)
337+
.map(|entry| entry.value().clone())
338+
{
339+
for (conn_id, sub_id) in candidates {
340+
self.push_match(
341+
conn_id,
342+
&sub_id,
343+
community_id,
344+
event,
345+
&mut results,
346+
&mut seen,
347+
);
302348
}
303349
}
304350
}
305351
if let Some(candidates) = self
306352
.global_kind_index
307353
.get(&(community_id, event.event.kind))
354+
.map(|entry| entry.value().clone())
308355
{
309-
for (conn_id, sub_id) in candidates.iter() {
310-
self.push_match(*conn_id, sub_id, event, &mut results, &mut seen);
356+
for (conn_id, sub_id) in candidates {
357+
self.push_match(
358+
conn_id,
359+
&sub_id,
360+
community_id,
361+
event,
362+
&mut results,
363+
&mut seen,
364+
);
311365
}
312366
}
313367
// Also check global wildcard (kindless global subs).
314-
if let Some(wildcards) = self.global_wildcard_index.get(&community_id) {
315-
for (conn_id, sub_id) in wildcards.iter() {
316-
self.push_match(*conn_id, sub_id, event, &mut results, &mut seen);
368+
if let Some(wildcards) = self
369+
.global_wildcard_index
370+
.get(&community_id)
371+
.map(|entry| entry.value().clone())
372+
{
373+
for (conn_id, sub_id) in wildcards {
374+
self.push_match(
375+
conn_id,
376+
&sub_id,
377+
community_id,
378+
event,
379+
&mut results,
380+
&mut seen,
381+
);
317382
}
318383
}
319384
}
@@ -370,13 +435,20 @@ impl SubscriptionRegistry {
370435
&self,
371436
conn_id: ConnId,
372437
sub_id: &str,
438+
community_id: CommunityId,
373439
event: &StoredEvent,
374440
results: &mut Vec<(ConnId, SubId)>,
375441
seen: &mut HashSet<(ConnId, SubId)>,
376442
) {
377443
if let Some(conn_subs) = self.subs.get(&conn_id) {
378-
if let Some((filters, _, _)) = conn_subs.get(sub_id) {
379-
if filters_match(filters, event) {
444+
if let Some((filters, sub_community_id, sub_channel_id)) = conn_subs.get(sub_id) {
445+
// Candidate snapshots can become stale while a same-ID replacement
446+
// moves the subscription. Re-check its authoritative scope before
447+
// matching so an old index entry cannot deliver across scopes.
448+
if *sub_community_id == community_id
449+
&& *sub_channel_id == event.channel_id
450+
&& filters_match(filters, event)
451+
{
380452
let entry = (conn_id, sub_id.to_string());
381453
if seen.insert(entry.clone()) {
382454
results.push(entry);
@@ -576,6 +648,8 @@ mod tests {
576648
use buzz_core::StoredEvent;
577649
use chrono::Utc;
578650
use nostr::{EventBuilder, Keys, Kind, Tag};
651+
use std::sync::Arc;
652+
use std::time::{Duration, Instant};
579653

580654
fn make_stored_event(kind: Kind, channel_id: Option<Uuid>) -> StoredEvent {
581655
let keys = Keys::generate();
@@ -629,6 +703,141 @@ mod tests {
629703
assert!(matches.is_empty());
630704
}
631705

706+
#[test]
707+
fn test_subscription_removal_cannot_delete_replacement_index() {
708+
let registry = Arc::new(SubscriptionRegistry::new());
709+
let conn_id = Uuid::new_v4();
710+
let channel_id = Uuid::new_v4();
711+
let sub_id = "same-id".to_string();
712+
let filters = vec![Filter::new().kind(Kind::TextNote)];
713+
registry.register(conn_id, sub_id.clone(), filters.clone(), Some(channel_id));
714+
715+
let (removed_tx, removed_rx) = std::sync::mpsc::sync_channel(0);
716+
let (resume_tx, resume_rx) = std::sync::mpsc::sync_channel(0);
717+
let remove_registry = Arc::clone(&registry);
718+
let remove_sub_id = sub_id.clone();
719+
let remove = std::thread::spawn(move || {
720+
remove_registry.remove_subscription_inner(conn_id, &remove_sub_id, || {
721+
removed_tx.send(()).expect("signal authoritative removal");
722+
resume_rx.recv().expect("resume index cleanup");
723+
})
724+
});
725+
726+
removed_rx.recv().expect("old subscription removed");
727+
728+
let (registered_tx, registered_rx) = std::sync::mpsc::sync_channel(0);
729+
let register_registry = Arc::clone(&registry);
730+
let register_sub_id = sub_id.clone();
731+
let register = std::thread::spawn(move || {
732+
register_registry.register(conn_id, register_sub_id, filters, Some(channel_id));
733+
registered_tx
734+
.send(())
735+
.expect("signal replacement registration");
736+
});
737+
738+
let replacement_finished_early = registered_rx
739+
.recv_timeout(Duration::from_millis(100))
740+
.is_ok();
741+
resume_tx.send(()).expect("resume old cleanup");
742+
remove.join().expect("removal thread completes");
743+
if !replacement_finished_early {
744+
registered_rx
745+
.recv_timeout(Duration::from_secs(1))
746+
.expect("replacement registration completes");
747+
}
748+
register.join().expect("registration thread completes");
749+
assert!(
750+
!replacement_finished_early,
751+
"replacement must wait until old index cleanup is complete"
752+
);
753+
754+
let event = make_stored_event(Kind::TextNote, Some(channel_id));
755+
assert_eq!(
756+
registry.fan_out(&event),
757+
vec![(conn_id, sub_id)],
758+
"replacement must remain reachable through its index"
759+
);
760+
}
761+
762+
#[test]
763+
fn test_stale_candidate_snapshot_does_not_cross_subscription_scope() {
764+
let registry = SubscriptionRegistry::new();
765+
let conn_id = Uuid::new_v4();
766+
let channel_a = Uuid::new_v4();
767+
let channel_b = Uuid::new_v4();
768+
let sub_id = "same-id".to_string();
769+
let filters = vec![Filter::new().kind(Kind::TextNote)];
770+
registry.register(conn_id, sub_id.clone(), filters.clone(), Some(channel_a));
771+
772+
// Reproduce fan-out's unlocked candidate snapshot, then move the same
773+
// subscription ID before the authoritative subscription lookup.
774+
let key = IndexKey {
775+
channel_id: channel_a,
776+
kind: Kind::TextNote,
777+
};
778+
let candidates = registry
779+
.channel_kind_index
780+
.get(&(test_community(), key))
781+
.expect("channel A candidate exists")
782+
.value()
783+
.clone();
784+
registry.register(conn_id, sub_id, filters, Some(channel_b));
785+
786+
let event = make_stored_event(Kind::TextNote, Some(channel_a));
787+
let mut results = Vec::new();
788+
let mut seen = HashSet::new();
789+
for (candidate_conn_id, candidate_sub_id) in candidates {
790+
registry.push_match(
791+
candidate_conn_id,
792+
&candidate_sub_id,
793+
test_community(),
794+
&event,
795+
&mut results,
796+
&mut seen,
797+
);
798+
}
799+
800+
assert!(
801+
results.is_empty(),
802+
"replacement on channel B received channel A event through stale snapshot"
803+
);
804+
}
805+
806+
#[test]
807+
fn test_fan_out_concurrent_with_subscription_replacement_completes() {
808+
let registry = Arc::new(SubscriptionRegistry::new());
809+
let conn_id = Uuid::new_v4();
810+
let channel_id = Uuid::new_v4();
811+
let sub_id = "sub1".to_string();
812+
let filters = vec![Filter::new().kind(Kind::TextNote)];
813+
registry.register(conn_id, sub_id.clone(), filters.clone(), Some(channel_id));
814+
let event = Arc::new(make_stored_event(Kind::TextNote, Some(channel_id)));
815+
let deadline = Instant::now() + Duration::from_secs(2);
816+
817+
let fan_out_registry = Arc::clone(&registry);
818+
let fan_out_event = Arc::clone(&event);
819+
let fan_out = std::thread::spawn(move || {
820+
while Instant::now() < deadline {
821+
let _ = fan_out_registry.fan_out(&fan_out_event);
822+
}
823+
});
824+
825+
let replace_registry = Arc::clone(&registry);
826+
let replace = std::thread::spawn(move || {
827+
while Instant::now() < deadline {
828+
replace_registry.register(
829+
conn_id,
830+
sub_id.clone(),
831+
filters.clone(),
832+
Some(channel_id),
833+
);
834+
}
835+
});
836+
837+
fan_out.join().expect("fan-out thread completes");
838+
replace.join().expect("replacement thread completes");
839+
}
840+
632841
#[test]
633842
fn test_subscription_registry_remove_connection() {
634843
let registry = SubscriptionRegistry::new();

0 commit comments

Comments
 (0)