feat(pubsub): add reason field to UnsubcriptionEvent#590
Open
Faolain wants to merge 1 commit intodao-xyz:masterfrom
Open
feat(pubsub): add reason field to UnsubcriptionEvent#590Faolain wants to merge 1 commit intodao-xyz:masterfrom
Faolain wants to merge 1 commit intodao-xyz:masterfrom
Conversation
Consumers of the "unsubscribe" event currently cannot distinguish between a peer going offline (removeSubscriptions) and a peer explicitly unsubscribing (Unsubscribe message). Attach a .reason string to the event detail so callers can react appropriately. Values: "peer-unreachable" | "remote-unsubscribe" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Why
I have an app I've been making and that my app's
UnifiedP2PProvider.tsx(line 1386-1391) listens for pubsub unsubscribe events and logs them withlogP2PDebugin order to understand what's going on with flakiness:Without the .reason field, the app couldn't distinguish between:
"peer-unreachable"— connection dropped (relay died, network issue)"remote-unsubscribe"— peer explicitly left the topicThis was important because the app had a @peerbit/stream patch - now merged that skips peer removal when another connection is still alive. Without knowing why a peer unsubscribed, the app couldn't make correct reconnection decisions. Was the peer gone, or did they just leave one topic? The debug logging (gated by
__APP_P2P_DEBUG) was critical for diagnosing these issues during development. It still not only remains crucial for debugging due to increased visibility but also general app functionality.Summary
The
UnsubcriptionEventdispatched byDirectSubcurrently provides no way to distinguish why an unsubscription occurred. There are two fundamentally different unsubscription paths:removeSubscriptions())Unsubscribemessage for specific topicsConsumers of the
"unsubscribe"event receive the same event shape for both cases, making it impossible to react appropriately (e.g., showing "peer went offline" vs. "peer left the topic" in a UI, or deciding whether to attempt reconnection).Problem
Both dispatch sites create a bare
UnsubcriptionEventwith no distinguishing metadata:The
UnsubcriptionEventclass only hasfromandtopicsfields. There is noreasonortypediscriminator.Downstream impact
I discovered this while building a browser-based P2P application using Peerbit. My connection management layer needs to distinguish between:
Without a reason field, the only workaround is to cross-reference unsubscribe events with connection state, which is racy and unreliable.
Fix
Attach a
.reasonstring property to theUnsubcriptionEventbefore dispatching:The
as anycast is used because theUnsubcriptionEventclass in@peerbit/pubsub-interfacedoesn't currently declare areasonfield. A cleaner upstream approach would be to addreason?: stringto theUnsubcriptionEventclass definition — I kept this minimal to avoid cross-package changes, but happy to updatepubsub-interfaceas well if preferred.Possible values
"peer-unreachable"removeSubscriptions()"remote-unsubscribe"Testing
Included two new test cases in
test/bug2-unsubscribe-reason.spec.ts:Peer disconnect (peer-unreachable): Two peers subscribe to a topic. Peer 0 stops. Verifies that the unsubscribe event on peer 1 has
reason === "peer-unreachable".AssertionError: expected undefined to equal 'peer-unreachable'Explicit unsubscribe (remote-unsubscribe): Peer 0 subscribes then explicitly calls
unsubscribe(). Verifies that the unsubscribe event on peer 1 hasreason === "remote-unsubscribe".AssertionError: expected undefined to equal 'remote-unsubscribe'Both tests use real
DirectSubinstances viaTestSession, matching existing test patterns.Notes
.reasonare unaffected.reasonfield to be a first-class property onUnsubcriptionEvent(in@peerbit/pubsub-interface), I'm happy to update that package as well.