feat: add stream close reason to social service v2 subscription updates#439
feat: add stream close reason to social service v2 subscription updates#439LautaroPetaccio wants to merge 2 commits into
Conversation
Adds a SubscriptionStreamClosed message (reason enum + optional detail) and an optional stream_closed field to all six streamed update messages, so the server can send a final message informing the client why a subscription stream is being closed. When stream_closed is present, the message carries no update data and the stream ends right after it. Wire- and JSON-compatible: new fields only (verified with buf breaking).
Test this pull request
|
A close notice reaches the client as the final stream message, which requires the connection to still be alive. That only holds for the duplicate-subscription rejection; shutdown and stale-cleanup closes happen after the socket is already gone, so those reasons could never be delivered. Trim the enum to UNKNOWN and DUPLICATE_SUBSCRIPTION.
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: feat: add stream close reason to social service v2 subscription updates
Summary
Adds application-level close notices to the social service v2 streaming protocol. New SubscriptionStreamClosedReason enum (UNKNOWN=0, DUPLICATE_SUBSCRIPTION=1), SubscriptionStreamClosed message, and an optional stream_closed field on all 6 streamed update messages. Clean, well-documented, additive-only changes.
Architecture ✅
- Design is sound.
stream_closedplaced as a standalone optional field outside existingoneofgroups (e.g., outsideoneof updateinFriendshipUpdate). This is the correct protobuf pattern — it keeps backward compatibility and lets the server and client enforce mutual exclusivity by convention. - Enum discipline is good. Only listing deliverable reasons (UNKNOWN + DUPLICATE_SUBSCRIPTION) rather than dead-code enum values that can never reach the client is a pragmatic choice. The enum remains extensible.
- Field numbers verified. All new field numbers are the next consecutive integer after existing fields — no conflicts:
FriendshipUpdate.stream_closed = 7(after oneof 1-6)FriendConnectivityUpdate.stream_closed = 3(after 1-2)BlockUpdate.stream_closed = 3(after 1-2)CommunityMemberConnectivityUpdate.stream_closed = 4(after 1-3)PrivateVoiceChatUpdate.stream_closed = 6(after 1-5)CommunityVoiceChatUpdate.stream_closed = 10(after 1-9)
Backward Compatibility ✅
- All changes are additive: new optional fields, new messages, new enum.
buf breakingpasses against main.- Old clients (TS, C#, Rust) will silently ignore the unknown field on the wire — they see a zero-value update followed by a normal stream end, which is benign.
Consumer Impact ✅
Consumers found: social-service-ea (server, PR #432), unity-explorer (C#), godot-explorer (Rust), bevy-explorer (Rust, vendored), social-rpc-client-js (TS). All will gracefully degrade without code changes — they simply won't surface the close reason until they opt in. No breakage.
Security ✅
No security issues. The message field contains compile-time event names (public API names), not user input. No injection, auth bypass, or information disclosure vectors.
Style ✅
- PR title follows
feat: <summary>convention. - Branch name
feat/subscription-stream-close-reasonfollows<type>/<summary>. - Proto comments are thorough and explain the contract clearly.
Verdict: APPROVE
No P0 or P1 findings. Well-designed, additive, backward-compatible protocol change with solid documentation.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U1140EA3W>) via Slack
Problem
When the social service closes a subscription stream, the client only sees the stream end — the
@dcl/rpcclose-stream frame has no reason field, so there is no way to tell the client why.Solution
Application-level close notice in the social service v2 protocol, sent as the final message of a stream:
SubscriptionStreamClosedmessage: aSubscriptionStreamClosedReasonplus an optional human-readablemessagefor logging/debugging.optional SubscriptionStreamClosed stream_closedfield on all six streamed update messages (FriendshipUpdate,FriendConnectivityUpdate,BlockUpdate,CommunityMemberConnectivityUpdate,PrivateVoiceChatUpdate,CommunityVoiceChatUpdate).When
stream_closedis present the message carries no update data and the stream ends right after it. Uniform across all six streams.Reason enum — only what's deliverable
A close notice can only reach the client while its connection is still alive (it travels as the final stream element, which the client must still be able to ACK). That holds for the duplicate-subscription rejection and essentially nothing else: shutdown and stale-cleanup closes happen after the socket is already gone, so those reasons could never be delivered. Rather than ship enum values that are dead on arrival, the enum keeps only
UNKNOWN(proto3 zero default) andDUPLICATE_SUBSCRIPTION. It stays extensible — a future deliverable reason can be added.Compatibility
buf breakingagainstmain.Verification
make buf-lint,make buf-build,make buf-breakingpass.scripts/test.sh) pass.🤖 Generated with Claude Code