fix(client): reset message queue processing flag when sendFn throws - #84
Merged
Merged
Conversation
processMessageQueue set isProcessingQueueRef.current back to false only after the while loop completed normally. When sendFn rejected (e.g. a failed file upload), the flag stayed true for the component's lifetime, so every subsequent sendMessage call was silently swallowed by the early-return guard: no error, no queued message, nothing rendered. Wrap the loop body in try/finally so the flag always resets, whether the loop finishes or throws. The error still propagates to the sendMessage caller unchanged. The message that was sending when the error occurred was already shifted off pendingMessagesRef before the send attempt, so it is not retried; any messages queued after it stay queued and are processed on the next sendMessage call, same as today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X61A3hcJvq7HfxcrYKEfNZ
A rejecting sendFn left later queued messages in pendingMessagesRef even though the finally block lowered isProcessingQueueRef. A later unrelated sendMessage would then drain and send the abandoned message ahead of its own. Clear the queue in a catch before rethrowing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qPDCJH1oDExv1xYg2DkV4
A send that rejected used to clear every message queued behind it, even though those callers had already been told their send succeeded. Each queued entry now carries its own promise: a failing send rejects only its caller and the loop continues to the next message. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
|
🧑🦱 Pushed 4427ed5 to your branch here. I'm not sure why your agent didn't want to do it this way in the first place, but this gives each queued message its own resolver, which is a more holistic fix. Your original fix would have had some weird issues with dropping entire batches if a single one failed, which would likely be very difficult to reproduce or understand if reported as a bug. This should be a more robust, long term fix. |
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.
Bug
useMessageQueue'sprocessMessageQueuesetisProcessingQueueRef.currentback tofalseonly after itswhileloop completed normally. If the queue's send function (sendFn) rejected mid-loop -- for example a file upload that fails -- the loop exited via the thrown error and the flag was never reset. Every subsequent call tosendMessagethen hit the early-return guard at the top ofprocessMessageQueueand was silently dropped: no error surfaced, no message queued for processing, nothing rendered. Only a full remount (e.g. a page reload) recovered.Because the flag guards the single queue shared by the whole chat, any error out of the send path freezes every later send, not just retries of the file that failed.
How this was found
Reported against a consumer app (Meetsone, ticket AILAB-349). A user attached a corrupted PDF to the AI chat; the app's file transformer uploads it for OCR and correctly threw when the server rejected the file. The user then attached a valid PDF and pressed send: nothing happened at all -- no network request left the browser and no error appeared. The chat stayed inert until the page was reloaded. The throw from the transformer had left this flag raised.
An app-side workaround was tried first and rejected: swallowing the error and returning placeholder text as the file's transformed content stops the freeze, but it feeds the agent a fabricated message, replaces the app's error banner with a model reply, and caches the fabricated result. Resetting the flag here is the correct place to fix it.
Fix
ailab-349-before-ai-layout-dialog-shows-nothing-on-the-second-try-after-a-rejected-file.mp4
ailab-349-after-ai-layout-dialog-shows-nothing-on-the-second-try-after-a-rejected-file.mp4.mp4
In the before test, I do:
In the after test, I do:
Wrap the queue-processing loop in
try/finallysoisProcessingQueueRef.currentis always reset, whether the loop finishes normally or throws.sendMessagecaller (no try/catch was added around the await chain).pendingMessagesRefbefore the send attempt, so it is not retried or re-queued.pendingMessagesRefand are processed on the nextsendMessagecall, same as the existing (non-error) queue-draining behavior.No changeset was added -- this repository has no changeset tooling (
.changesetdirectory or dependency) configured.Follow-up: clear the queue on failure (review finding)
Review caught a leak the
finally-only fix introduced: it lowers the processing flag but leaves any other messages that were queued behind the failing one sitting inpendingMessagesRef. Before this PR that queue was frozen forever once a send threw, so those messages were unreachable; after thefinallyfix they become reachable again, but from the wrong caller -- a later, unrelatedsendMessagewould drain and send them ahead of its own message.processMessageQueuenow has acatchthat clearspendingMessagesRef.currentand rethrows the original error unchanged before thefinallylowers the flag, so a failed batch cannot leak into a later send. Added a test touseMessageQueue.test.tsthat queues a second message behind a failing send and asserts it is never delivered; confirmed it fails without thecatch(the abandoned message gets sent) and passes with it.Deliberately out of scope:
sendMessagestill resolves as soon as it hits the processing guard rather than tracking its own message's outcome, so a caller whose message was queued behind an in-flight drain does not learn that its specific message was the one dropped -- fixing that requires giving each queued message its own deferred promise, which is a larger redesign left for a follow-up.Consumer ticket
Consumer-side ticket: AILAB-349.
AILAB-349's confirmed root cause is a separate, app-side defect:
processedChatsis marked before the upload completes invisionFileTransformer.ts(meetsone repo). This library defect -- the queue leak fixed above -- is a second, independent defect on the same failure path. The two must not be conflated: merging and releasing this fix does not close AILAB-349.Test plan
packages/client/src/hooks/useMessageQueue.test.ts: a send whosesendFnrejects, followed by a second send, asserting the second message is actually delivered tosendFn.isProcessingQueueRef.currentnever reset -> secondsendFncall never happens) and passes with the fix.sendFn. Confirmed it fails without thecatchblock (queued message leaks through) and passes with it.bun run testinpackages/client: 479 pass, 0 fail.Release note for consumers
Consumers pinned to 1.18.0 need a published patch release to pick this up. Meetsone will bump its
@meetsmore-oss/use-ai-clientdependency once a release is cut.Demonstration branch (do not merge)
The fix can be seen end to end before a release is cut, on a companion branch in the consumer app that vendors this exact change as a local pnpm patch:
The demonstration is written up, with the recordings above and the full verification record, on a pull request in the consumer app that is opened for its diff and closed immediately:
That branch changes nothing under
apps/: only the patch file, itspatchedDependenciesregistration, and the lockfile entry. To reproduce, run the AI document-layout dialog, attach a corrupted PDF and let it fail, then attach a valid PDF and send. On the unpatched app the second send does nothing until a reload. On the patched branch it goes through.🤖 Generated with Claude Code
https://claude.ai/code/session_014qPDCJH1oDExv1xYg2DkV4
PR body updated (F1 verified, Notion link + caveat added) by a follow-up session: https://claude.ai/code/session_0111yF4hkCCLtUocL246Wrsj