Keep chat subscribers alive after a rejected changeApplied callback - #314
Open
hidcc wants to merge 2 commits into
Open
Keep chat subscribers alive after a rejected changeApplied callback#314hidcc wants to merge 2 commits into
hidcc wants to merge 2 commits into
Conversation
A rejected changeApplied callback removed the subscriber from #chatSubscribers even though the RPC connection was still healthy, leaving the page connected but permanently unsubscribed: the chat UI silently stopped updating mid-turn until a refresh re-subscribed it. Log the rejection and keep the subscription instead, in both the live broadcast path and the subscribeToChat replay path. Dead clients are still removed by their onRpcBroken handler. Adds a regression test that fails on the previous behavior. Fixes cloudflare#305 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test lives on the regression-test-305 branch, referenced from the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
Author
|
recheck |
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.
Fixes #305.
What does this change?
Since #275, a rejected
changeAppliedcallback removes the subscriber from#chatSubscribers(and thesubscribeToChatreplay path callsunsubscribeon rejection), even though one rejected callback does not mean the RPC connection is broken. The page stays connected but permanently unsubscribed, so the chat UI silently stops updating during an agent turn until a refresh re-subscribes it.This patch makes both
changeAppliedrejection handlers (the live broadcast inemitChatChangeAppliedand the retained-row replay insubscribeToChat) log the rejection and keep the subscription, matching the mitigation described in #305. Dead clients are still removed through the existingsubscriber.onRpcBroken(() => unsubscribe())handler.Why is this obviously correct and trivially verifiable?
The patch only changes what happens after a
changeAppliedpromise rejects: previously the subscriber was dropped (silently, with the error swallowed); now the error is logged with the existing logger convention and the subscriber is kept. No control flow before the.catchchanges, and disconnect cleanup is unchanged (onRpcBroken→unsubscribe, visible a few lines above the replay site).Verified locally: a regression test (a subscriber whose
changeAppliedrejects once must still receive the next row) fails onmainand passes with this patch — kept out of this PR to stay within the line limit, available athidcc:regression-test-305. The fullchat-changes.test.tssuite passes, andvp lint/tsc -p packages/workshop-backendare clean.Checklist
Checking every item does not guarantee acceptance. Maintainers determine whether
a pull request meets the contribution policy.
🤖 Generated with Claude Code