fix(server): subscription hardening — read-error subscription preservation + owner_id fence (PR #22 follow-ups)#24
Merged
Conversation
…review Two non-blocking items Odin flagged while reviewing the concurrency work: 1. A transient read error in SubscribeAuthorizedStable no longer tears down the channel's live subscriptions. RemoveChannel on an indeterminate read dropped legit subscribers a concurrent connect had installed, stranding already- connected members until reconnect. It now returns the error and leaves the set intact; the caller broadcasts nothing and the committed row self-heals on the client's next channel-list fetch. RemoveChannel is kept only on the genuine !exists path. 2. Migration 019 extends the per-server event fence on the servers table from DELETE-only to also cover owner_id UPDATE. An ownership transfer changes channel-visibility authorization; no endpoint updates owner_id today, so the trigger fires never until one is added -- at which point the create-vs-authz race is already closed by construction. Future-proofing, zero current cost. Mutation-verified pins for both. Full Go -race suite + gofmt/vet/lint green.
Owner
Author
|
LGTM at I reviewed the complete PR #24 diff and found no blocking issues. Verified:
Validation:
PR #24 is ready to merge. |
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.
Closes the two non-blocking NITs surfaced during PR #22's review.
1. Transient read error no longer drops live subscriptions
SubscribeAuthorizedStablecalledRemoveChannelon any read error, dropping the channel's entire subscriber set — including legit members a concurrent connect had installed — stranding already-connected clients until reconnect. It now returns the error and leaves the set intact (the caller broadcasts nothing; the committed row self-heals on the client's next channel-list fetch).RemoveChannelis kept only on the genuine!exists(deleted) path.2. Migration 019 — fence owner_id updates
authorizedMemberIDsreadsservers.owner_id, but migration 017 fenced the servers table on DELETE only. An ownership transfer (UPDATE servers SET owner_id) would change channel-visibility authorization without taking the exclusive per-server lock. No endpoint updates owner_id today, so the trigger fires never until one is added — at which point the create-vs-authz race is already closed. Future-proofing, zero current cost.Tests
Mutation-verified pins for both:
TestSubscribeAuthorizedStableReadErrorPreservesSubscriptions(realtime) andTestMigration019FencesServerOwnerUpdate(up/down trigger-event assertion). Full Go-racesuite + gofmt/vet/golangci-lint green.