fix(hub): deliver message-received events to all:true SSE connections#507
Merged
tiann merged 1 commit intotiann:mainfrom Apr 22, 2026
Merged
Conversation
The message-received branch of shouldSend() in hub/src/sse/sseManager.ts checks only connection.sessionId === event.sessionId, ignoring the connection.all flag. As a result, any SSE connection subscribed with all: true (to observe events across every session in the namespace) silently never receives message-received events, even though every other event type below this branch honors connection.all. Closes tiann#506
There was a problem hiding this comment.
Findings
- [Minor] Missing regression test for the repaired
all:truepath — The routing change inhub/src/sse/sseManager.ts:159looks correct, buthub/src/sse/sseManager.test.ts:6still has no case asserting that amessage-receivedevent reaches anall: truesubscription in the same namespace. That leaves this exact bug easy to reintroduce unnoticed.
Suggested fix:it('delivers message-received events to all subscriptions in the same namespace', () => { const manager = new SSEManager(0, new VisibilityTracker()) const received: SyncEvent[] = [] manager.subscribe({ id: 'all', namespace: 'alpha', all: true, send: (event) => { received.push(event) }, sendHeartbeat: () => {} }) manager.broadcast({ type: 'message-received', namespace: 'alpha', sessionId: 'session-1', message: { id: 'message-1', seq: 1, localId: null, createdAt: 0, content: { type: 'event', data: { type: 'ready' } } } }) expect(received).toHaveLength(1) })
Summary
Review mode: initial. 1 minor issue found: the fix is reasonable, but the repaired all:true delivery path is still untested.
Testing
Not run (bun not available in this runner)
HAPI Bot
|
|
||
| if (event.type === 'message-received') { | ||
| return connection.sessionId === event.sessionId | ||
| return connection.all || connection.sessionId === event.sessionId |
There was a problem hiding this comment.
[MINOR] This branch fix looks right, but there is still no regression test covering message-received delivery for an all: true SSE subscription. The current suite in hub/src/sse/sseManager.test.ts exercises other routing paths only, so this exact bug can slip back unnoticed.
Suggested fix:
it('delivers message-received events to all subscriptions in the same namespace', () => {
const manager = new SSEManager(0, new VisibilityTracker())
const received: SyncEvent[] = []
manager.subscribe({
id: 'all',
namespace: 'alpha',
all: true,
send: (event) => {
received.push(event)
},
sendHeartbeat: () => {}
})
manager.broadcast({
type: 'message-received',
namespace: 'alpha',
sessionId: 'session-1',
message: {
id: 'message-1',
seq: 1,
localId: null,
createdAt: 0,
content: {
type: 'event',
data: { type: 'ready' }
}
}
})
expect(received).toHaveLength(1)
})
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
message-receivedbranch ofshouldSend()inhub/src/sse/sseManager.tschecks onlyconnection.sessionId === event.sessionId, ignoring theconnection.allflag. As a result, any SSE connection subscribed withall: truesilently never receivesmessage-receivedevents, even though every other event type below this branch honorsconnection.all.Change
if (event.type === 'message-received') { - return connection.sessionId === event.sessionId + return connection.all || connection.sessionId === event.sessionId }One-line fix — the branch now behaves consistently with the rest of the dispatch logic further down in the same function.
Impact
Any feature that relies on an
all:trueSSE subscription to observe messages across sessions (cross-session unread indicators, global notification logic, etc.) starts working again. No behavioral change for session-scoped connections.Closes #506
Test plan
shouldSend()all honorconnection.all, confirming this was an inconsistency rather than an intentional restrictionmessage-receivedevent filtering exist inhub/