Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions pkg/api/message/listener.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@ import (
)

type listener struct {
ctx context.Context
ch chan<- []*envelopes.OriginatorEnvelope
ctx context.Context
ch chan<- []*envelopes.OriginatorEnvelope
// topicsMu guards topics, which is fixed for a server-stream listener but mutated in
// place by a mutableSubscription (the XIP-83 bidi Subscribe). The worker reads it when
// reaping the listener (closeListener), so concurrent mutation needs the lock.
topicsMu sync.Mutex
closed bool
topics map[string]struct{}
originators map[uint32]struct{}
Expand Down Expand Up @@ -101,7 +105,7 @@ func (lm *listenersMap[K]) removeListener(keys map[K]struct{}, l *listener) {
for key := range keys {
value, ok := lm.data.Load(key)
if !ok || value == nil {
return // Key doesn't exist, nothing to do
continue // This key is already gone (e.g. reaped by the worker); keep unregistering the rest
}
set := value.(*listenerSet)
set.removeListener(l)
Expand Down
80 changes: 80 additions & 0 deletions pkg/api/message/mutable_subscription.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package message

import (
"context"

"github.com/xmtp/xmtpd/pkg/envelopes"
)

// mutableSubscription is an in-place-mutable topic subscription on the subscribeWorker, used by
// the XIP-83 bidirectional Subscribe RPC. Unlike subscribeWorker.listen (a fixed query), its
// topic set is grown and shrunk over the life of the stream via addTopics / removeTopics. It is
// never global: an empty topic set delivers nothing (a fresh stream that has subscribed to
// nothing yet), in contrast to newListener, which treats an empty query as "all envelopes".
//
// All mutation methods are intended to be called from the single owning goroutine (the Subscribe
// handler's writer loop). The worker's reap path (closeListener) may also touch the underlying
// listener's topic set concurrently; both sides take listener.topicsMu, so that is safe.
type mutableSubscription struct {
worker *subscribeWorker
l *listener
// ch is the receive end of the listener channel. The worker pushes envelope batches here;
// it CLOSES the channel when it reaps the listener (ctx done or the consumer fell behind),
// which the writer loop observes as "torn down, reconnect from cursors".
ch <-chan []*envelopes.OriginatorEnvelope
}

// newMutableSubscription registers an empty, non-global topic subscription on the worker and
// returns a handle the caller mutates over the stream's lifetime. Call close when done.
func (s *subscribeWorker) newMutableSubscription(ctx context.Context) *mutableSubscription {
ch := make(chan []*envelopes.OriginatorEnvelope, subscriptionBufferSize)
l := &listener{
ctx: ctx,
ch: ch,
topics: make(map[string]struct{}),
originators: make(map[uint32]struct{}),
isGlobal: false, // empty topic set => delivers nothing, NOT all-envelopes
}
return &mutableSubscription{worker: s, l: l, ch: ch}
}

// addTopics begins delivering the given topic keys to this subscription. Idempotent: a key
// already subscribed is a no-op. A no-op once the worker has reaped the listener.
func (m *mutableSubscription) addTopics(keys map[string]struct{}) {
if len(keys) == 0 {
return
}
m.l.topicsMu.Lock()
defer m.l.topicsMu.Unlock()
if m.l.closed {
return
}
m.worker.topicListeners.addListener(keys, m.l)
for k := range keys {
m.l.topics[k] = struct{}{}
}
}
Comment on lines +47 to +56

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical message/mutable_subscription.go:47

mutableSubscription.addTopics checks m.l.closed while holding topicsMu, but subscribeWorker.closeListener sets l.closed = true without taking that lock. A concurrent mutate can pass the stale closed check and re-add the listener to topicListeners after its channel is closed, causing dispatchToListeners to panic on send.

 func (m *mutableSubscription) addTopics(keys map[string]struct{}) {
 	if len(keys) == 0 {
 		return
 	}
-	m.l.topicsMu.Lock()
-	defer m.l.topicsMu.Unlock()
-	if m.l.closed {
+	m.worker.topicListeners.mu.RLock()
+	defer m.worker.topicListeners.mu.RUnlock()
+	m.l.topicsMu.Lock()
+	defer m.l.topicsMu.Unlock()
+	if m.l.closed {
 		return
 	}
 	m.worker.topicListeners.addListener(keys, m.l)
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @pkg/api/message/mutable_subscription.go around lines 47-56:

`mutableSubscription.addTopics` checks `m.l.closed` while holding `topicsMu`, but `subscribeWorker.closeListener` sets `l.closed = true` without taking that lock. A concurrent mutate can pass the stale `closed` check and re-add the listener to `topicListeners` after its channel is closed, causing `dispatchToListeners` to panic on send.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. closeListener now sets l.closed under l.topicsMu, so a concurrent mutableSubscription.addTopics observes it atomically with its addListener call and can no longer re-register a listener after its channel is closed. Added a -race regression test (TestMutableSubscriptionAddRaceWithClose) that hammers addTopics against the reap.


// removeTopics stops delivering the given topic keys. Idempotent.
func (m *mutableSubscription) removeTopics(keys map[string]struct{}) {
if len(keys) == 0 {
return
}
m.l.topicsMu.Lock()
defer m.l.topicsMu.Unlock()
m.worker.topicListeners.removeListener(keys, m.l)
for k := range keys {
delete(m.l.topics, k)
}
}

// close unregisters the subscription from every topic it still holds. Safe to call even after
// the worker has already reaped the listener (removeListener is idempotent).
func (m *mutableSubscription) close() {
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
m.l.topicsMu.Lock()
defer m.l.topicsMu.Unlock()
if len(m.l.topics) > 0 {
m.worker.topicListeners.removeListener(m.l.topics, m.l)
m.l.topics = make(map[string]struct{})
}
}
Loading
Loading