Share one subscribe per identifier - #6
Merged
Merged
Conversation
Rails keeps one subscription per identifier per connection. A second subscribe for an identifier it already has is ignored outright (ActionCable::Connection::Subscriptions#add returns early), so no confirmation ever comes back for it. The client already let several Subscribe calls share an identifier, fanning messages out to each and sending unsubscribe only from the last one. But a second Subscribe still sent its own subscribe command and waited for a confirmation the server would never send, while the guarantor resent it every WithSubscribeRetry forever. The fake transport confirmed duplicates, so the tests hid it. The subscribe command, its verdict, and the retries until then now belong to the identifier rather than to each Subscription, through a registration the client keeps per identifier. A Subscribe that finds one already there meets one of three cases: - Confirmed on the connection in hand: nothing is sent, and the new subscription is confirmed on the spot, OnConnected reporting no reconnect. - In flight, or waiting for the next welcome: nothing is sent, and the new subscription waits for that subscribe's verdict. A confirmation confirms every holder; a rejection gives every holder ErrRejected. Cancelling a waiting holder's context forgets only that holder. - No registration: a subscribe goes out, as before. A welcome still resubscribes exactly one command per identifier, and the guarantor now retries per identifier rather than per holder. The fake connection ignores a subscribe for an identifier it has already heard, pending or confirmed, the way Rails does, and the retry test drops the first subscribe on the floor to stand in for one that reached the server too early.
A Subscribe whose context ended while its subscribe was in flight, or just after it was confirmed, forgot the subscription without a word to the server. Rails kept it, and since it ignores a second subscribe for an identifier it already has, the next Subscribe for that identifier was never answered and hung. Giving up on the last holder of an identifier the server has heard a subscribe for now sends an unsubscribe, best effort: the connection may be gone, and then there is nothing left to tell. A holder giving up while others remain still sends nothing, and an identifier the server never heard of on this connection has nothing to undo.
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation and reconnect races can leave orphaned server subscriptions, dispatch stale callbacks, or block canceled calls.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Shares one server subscription among local subscriptions with the same identifier.
Changes:
- Adds shared registration state and message fan-out.
- Coordinates confirmation, rejection, retry, and unsubscribe behavior.
- Expands documentation and tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
client.go |
Implements shared registrations and lifecycle handling. |
subscription.go |
Adds registration state and updates callbacks. |
client_test.go |
Tests sharing, reconnects, cancellation, and rejection. |
fake_transport_test.go |
Models Rails duplicate-subscribe behavior. |
README.md |
Documents shared subscription semantics. |
Review details
Suppressed comments (1)
subscription.go:123
- Confirmation is now unconditional per holder after the registration snapshot. During reconnect,
confirmcan capture the holders, then a concurrentUnsubscribecan remove and close one before this call; that removed holder still receivesOnConnected(true). The former per-subscription pending check suppressed this stale callback. Preserve an active/pending state per holder, or revalidate membership before dispatching confirmation.
func (s *Subscription) confirm(reconnected bool) {
s.confirmOnce.Do(func() { close(s.confirmed) })
if s.onConnected != nil {
s.callbacks.dispatch(func() { s.onConnected(reconnected) })
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
welcome and the guarantor listed the identifiers to resubscribe under one lock, let go of it, then wrote each subscribe under another. An Unsubscribe landing in between could get its command out ahead of the subscribe for the same identifier. The server would then hold a subscription the client had forgotten, and since Rails ignores a subscribe for an identifier it already has, the next Subscribe for it would wait forever. Both now hold writeMu from listing the identifiers until the last subscribe is written, so any other command is ordered after the burst.
confirm lists a registration's holders, lets go of the lock, and then tells each. A holder that unsubscribed in between was told anyway, and its OnConnected ran after Unsubscribe had returned. The per-subscription pending flag used to catch that; now confirm checks the subscription is still open. abandon sent its unsubscribe under context.Background, so a Write that never returned would hold a cancelled Subscribe hostage. It goes out on the client's own context now, which ends with the client and with the connection the write is stuck on.
monorkin
added a commit
to basecamp/hey-cli
that referenced
this pull request
Sep 8, 2026
The client reports its own lifecycle since basecamp/actioncable-go#6 and #7, so the code here that worked that out for itself can go. - A Connect that gives up stops the client, so Dial no longer closes one behind its own error. Dial bounds the opening itself rather than leaving a caller who passed no deadline inside the retry loop. - A dial's credentials are no longer fetched twice. The error from building the upgrade request comes back with the one Connect returns, so a token that can't be built is reported rather than retried out of sight. - Client.Err says whether the shared TUI connection stopped and why, in place of calling Connect again and reading ErrAlreadyConnected as a sign of life. It also stops a rejected subscription from condemning a connection the other watches are still using. - Subscription.Err says why the messages dried up, in place of the flag OnRejected used to set, and the watch now reports what it was. - Unsubscribe takes no context, so the goodbye no longer needs one built to outlive the watch that just ended. The Screener's separate reconnect channel stays. Its relay closes on the watch's context while the subscription behind it is still registered, so a late callback would still write to a closed channel — that never had anything to do with the dispatcher draining after a stop, and the comment said otherwise.
monorkin
added a commit
to basecamp/hey-cli
that referenced
this pull request
Sep 8, 2026
The client reports its own lifecycle since basecamp/actioncable-go#6 and #7, so the code here that worked that out for itself can go. - A Connect that gives up stops the client, so Dial no longer closes one behind its own error. Dial bounds the opening itself rather than leaving a caller who passed no deadline inside the retry loop. - A dial's credentials are no longer fetched twice. The error from building the upgrade request comes back with the one Connect returns, so a token that can't be built is reported rather than retried out of sight. - Client.Err says whether the shared TUI connection stopped and why, in place of calling Connect again and reading ErrAlreadyConnected as a sign of life. It also stops a rejected subscription from condemning a connection the other watches are still using. - Subscription.Err says why the messages dried up, in place of the flag OnRejected used to set, and the watch now reports what it was. - Unsubscribe takes no context, so the goodbye no longer needs one built to outlive the watch that just ended. The Screener's separate reconnect channel stays. Its relay closes on the watch's context while the subscription behind it is still registered, so a late callback would still write to a closed channel — that never had anything to do with the dispatcher draining after a stop, and the comment said otherwise.
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.
No description provided.