fix(chat): stop losing a user message that arrived mid-turn - #4795
Merged
Conversation
🦋 Changeset detectedLatest commit: b0ecb91 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the two message-loss cases, the router and durability changes, verification results, and known limitations. It does not include the template checklist or an explicit issue closure, but it provides sufficient testing and change details. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A message arriving while a turn was streaming was handed to the turn's push handler and parked in an in-memory array. The router counts a record handed to a handler as terminally decided, so it stopped holding the resume floor behind it and the turn boundary published a cursor past a message that existed only in this process. A crash before the next turn lost it silently. The handler is now attached only when there is a steering config to feed. Without one the record stays queued on the router, which holds the floor until a turn takes it, and both in-memory wire buffers go away. The wait path already takes from the queue before suspending, so a message that arrived mid-turn is still picked up as the next turn without a round trip. The floor also doubles as the wake cursor, so an over-advanced floor parked a waitpoint nothing would complete. It is now recorded on the wait span to make that diagnosable from a trace. Not addressed here: with a steering config, a declined message is still dropped rather than left queued. That path depends on an unresolved question about what declining should mean.
A message arriving mid-turn with a `pendingMessages` config was routed into a turn-local steering queue. If the batch was declined for injection it was discarded with the turn: never injected, never written to the wire buffer, never answered, and nothing raised at either end. Declining is also the default, since a config without `shouldInject` declines every batch, so the documented default behaviour was the losing one. Notification and consumption are now separate. `observe` on the router tells a consumer a record arrived without taking it, so the record stays queued and keeps holding the resume floor, and injection is the point of consumption: `take` removes exactly the records that were injected. A declined batch never reaches that line, so its records stay queued and become later turns, which is what the docs have always promised. `observe` is rejected on an at-arrival route. An observer there would either have to count as a listener, which would stop an unconsumed stop being discarded and bring back the wedged mailbox, or watch records it cannot affect.
The docs described a mid-turn message becoming the next turn only when there were no more step boundaries, and the client-side lifecycle credited the frontend with auto-sending it. Neither matched the behaviour: a message the agent declines to inject is now held on the backend and answered as the next turn, with no client re-send involved, and that covers an explicit `shouldInject: false` as well as a turn that never reaches a boundary. Also spells out that a declined message keeps its place in the queue, so it survives a crash rather than living only in the worker that received it.
ericallam
force-pushed
the
fix/mid-turn-resume-floor
branch
from
August 27, 2026 08:09
4d2027c to
3dd60c2
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`shouldInject` and `prepare` can await, so a record could arrive between the batch being assembled and the injection being applied. That record was not in the batch and the callbacks never saw it, but it was taken and cleared along with them, so a message that should have become a later turn was discarded. The batch is now snapshotted before the callbacks run, and only its entries are taken from the router and removed from the queue. Also scopes the deferral guarantee in the docs and the changeset: it holds only when `pendingMessages` actually reaches `streamText`, via `chat.toStreamTextOptions()` or an explicit `prepareStep`. A config without that wiring has nothing to drain the queue and still loses messages. The resume-floor test now waits for the channel sequence to advance rather than to merely exist, since the first message had already advanced it and the old predicate could capture that sequence instead of the second message's.
`clearRoute` discards a queue outright. That is right for the handover route, whose window closes at a turn boundary, and wrong for any replayable route, where anything queued is still owed to a later boot and the resume floor is deliberately held behind it. Nothing calls it that way today, but the messages queue now holds real unanswered user input rather than being drained into an in-memory buffer, so a future caller would silently discard messages instead of merely losing a window. Enforced rather than left as a comment.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous commit said a `pendingMessages` config without `chat.toStreamTextOptions()` still loses mid-turn messages. That was true before this branch and is not true on it: the arrival path only observes now, so the record stays queued on the channel whatever happens to the steering queue, and the next turn takes it. Injection is the part that needs the spread. Without it nothing injects, so every mid-turn message is answered as the next turn, which is the documented default rather than a loss. Covers the shape a developer reaches by following the docs for `onReceived` alone, with no `shouldInject` and no spread, so nothing can drain the queue.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`observe` notifies before the router decides where a record goes, so a record can be seen by an observer and then consumed by a waiting puller rather than queued. If that happened while `shouldInject` was awaiting, the entry was still injected even though its `take` failed, and the same message would be both injected and answered as a turn of its own. Claiming now happens before the transform, and only claimed entries are injected. A failed claim means something else is already answering that message, so dropping it here is what keeps it processed once. The injection chunk and `onInjected` report the claimed set rather than the offered one, and an entry with no `seqNum` (the accumulator's own queue) is kept, since there is no record to claim. Both mid-turn tests now assert no turn has completed before the second message is sent. A text delta having arrived does not prove the turn is still open, so a message landing late would take the ordinary next-turn path and pass without exercising the mid-turn one.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
An observer is notified before the router decides where a record goes, so a parked puller can consume a record an observer has just been told about, and a later take() for it correctly reports false. That ordering is the precondition for the steering drain injecting an entry it does not own, so it is worth holding in place rather than leaving it to be rediscovered by review. Testable here even though the SDK-side drain is not: it needs no model and no step boundary, only a waiter and an ingest.
Claiming before the transform fixed injecting an entry this drain does not own, but introduced a worse failure on the way: `prepare` is caller code and can throw, and by then the records have left the router, so a failing transform consumed the messages and they were never answered at all. Before the reorder a throw left them queued. `take` now returns the record it removed rather than a boolean, `untake` puts one back in sequence order, and the transform runs inside a try that returns every claim before rethrowing. `untake` ignores a record already queued, so a double return cannot duplicate one. The re-raise is deliberate: the turn should still fail, since the caller's transform failed. What changes is that the messages survive it and are answered by a later turn.
Injection had no unit coverage. A prepareStep boundary only exists on a turn that takes more than one step, and nothing in this package produced one, so every steering test asserted arrival and none could reach the drain. Both bugs found in that code during review were found by reading it, not by running it. `twoStepModel` gives a turn a real boundary: step one calls a tool, the tool blocks on a gate the test holds, and step two answers. Holding the tool open is what makes it deterministic, since a message appended while the gate is shut is queued before `prepareStep` runs with no reliance on stream timing. Three cases, each checked against the commit that introduced the bug it guards rather than only observed to pass: - a mid-turn message is injected and not also answered as its own turn - a message arriving after the batch was assembled is left for a later turn, which fails at 3dd60c2 - a claim is returned when `prepare` throws, which fails at e3e6ad7 The middle one needed two attempts. Asserting the late message eventually gets answered passes on the bug, because without the snapshot its model messages are injected into the first turn, so the text appears either way. A second turn-complete is the real discriminator.
…istory The deployed QA lane finds the two surfaces disagree: a `chat.createSession()` recap in the same run recalls a mid-turn steer, while the managed `chat.agent` loop denies it. This pins the managed half at unit level, which the tracked gap has never had. Turn 2's prompt comes back as the original message and the following one, with the injected one absent, so the message reaches the model inside turn 1 and then leaves no trace in history. Verified as a real assertion failure rather than trusting `it.fails`, which would also pass on a timeout. Not fixed here, and not caused by this branch: nothing in it touches the accumulator. Recorded so the day the managed path starts carrying it is noticed, and so the difference between the surfaces has a repro that needs no deployed environment.
ericallam
marked this pull request as ready for review
August 27, 2026 10:43
nicktrn
approved these changes
Aug 27, 2026
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Merged
3 tasks
ericallam
pushed a commit
that referenced
this pull request
Aug 28, 2026
## Summary 4 new features, 12 improvements, 5 bug fixes. ## Improvements - `trigger.dev deploy` now asks the server whether to build with Depot or the native build server unless `--native-build`, `--depot-build`, or `--local-build` is passed, so the native build server can be rolled out per organization without a CLI change. `--local-bundle` and `--detach` now require `--native-build`. ([#4803](#4803)) - Add an experimental `--local-bundle` deploy flag that runs the install and bundling steps on your machine and uploads only the build output; the image is still built remotely. Useful when your project's install step needs tooling or credentials that only exist locally. ([#4331](#4331)) - Send the CLI version header on all API requests so deployments are attributable to a CLI version ([#4778](#4778)) - A message that arrives mid-turn and is not injected into that turn is now answered as the next turn, instead of being dropped. This is what the `pendingMessages` docs have always described, and it applies to the default too: configuring `pendingMessages` without a `shouldInject` declines every batch, which previously meant every mid-turn message was lost with no error at either end. ([#4795](#4795)) ```ts chat.agent({ id: "my-chat", pendingMessages: { onReceived: ({ message }) => logger.info("arrived mid-turn", { id: message.id }), // Only interrupt once the agent has started calling tools. shouldInject: ({ steps }) => steps.length > 0, }, run: async ({ messages, signal }) => streamText({ model, messages, abortSignal: signal, // Required for injection. Without it nothing injects, and every // mid-turn message is answered as the next turn instead. ...chat.toStreamTextOptions(), }), }); ``` A declined message keeps its place in the queue, so it survives a crash and is answered by whichever run picks the conversation up. An injected one is consumed at the moment it is injected, so it is never also answered as a later turn. - Browser chats now keep the active turn open across page reloads when older completion records are replayed. ([#4643](#4643)) - Add `chat.endAndContinue()` so fully hand-rolled custom chat agents can hand a conversation off to a fresh run on the latest deployed task version while preserving unconsumed Session input. ([#4647](#4647)) - Custom chat agents now validate and parse client data declared with `chat.withClientData({ schema })` before passing it to agent code. ([#4646](#4646)) ## Bug fixes - Fixes a case where a chat could silently lose a message. If a message arrived while the agent was between turns and a stop arrived after it, the cursor the next boot resumed from could point past that message, so it was never answered and no error was raised. This affected `chat.agent`, not just custom agents. ([#4644](#4644)) Fixes a recovered answer being cut off. After a crash the agent replays the message it had not answered yet, but it was replaying the stop that arrived after that message too, so the turn answering it was aborted the moment it began. A stop is now only applied to the turn that was live when it arrived. That holds however the stop got there: sent after the last completed turn, or sent to a chat whose most recent turn was completed by an older version of the SDK. One limitation to know about: the recovered answer is persisted correctly, but a chat page that stayed open across the crash keeps showing the partial answer it had already received. Reload the page to see the full recovered answer. Also fixes a retried send being answered twice. When a send was retried and its idempotency claim was lost, the agent could consume the same message a second time. Custom agent loops can now inspect pending chat input without consuming it, and consume one record at a time, with `chat.messages.hasPending()` and `chat.messages.next()`. Records carry stable identifiers so a redelivery is recognisable. ```ts if (await chat.messages.hasPending()) { const record = await chat.messages.next({ timeoutInSeconds: 0 }); if (record) handle(record.payload); } ``` `hasPending()` answers for messages alone, so a message sitting behind a stop, or behind a record this version of the SDK does not recognise, still reports as pending and is still delivered. Anything the agent has no consumer for is discarded rather than left where it would make every message queued behind it undeliverable. `chat.messages.next()` returning `undefined` means no message became consumable before the timeout. `chat.writeTurnComplete()`'s `sessionInEventId` is the cursor that is safe to resume from, not the sequence of the record the turn answered. It is held back behind any message still waiting to be handled, so a value below the record you just handled is expected. - Fixed a chat agent hanging after an interrupted turn: when a run was killed mid-answer (out of memory, crash, or eviction) and only the one message it was answering was still outstanding, the new run never replied to it. That message is now re-answered on the new run. ([#4768](#4768)) - Fix chat transport discarding the next turn after stopping generation. `skipToTurnComplete` is now reset when a new message or action is sent, so a message sent after `stopGeneration` streams normally instead of leaving the chat stuck in a streaming state. ([#4744](#4744)) - Fixes a message sent while the agent was mid-answer being lost if the run then crashed. The cursor written at the end of each turn could point past a message that had arrived during that turn but had not been answered yet, so the next boot skipped it and no error was raised anywhere. Such a message is now held until a turn actually takes it. ([#4795](#4795)) This also removes the in-memory buffer those messages used to sit in, on both `chat.agent` and `chat.createSession()`, so a message waiting for its turn is durable rather than only present in the worker that received it. ## Server changes These changes affect the self-hosted Docker image and Trigger.dev Cloud: - Self-hosted instances can now disable the admin dashboard and user impersonation entirely. See the self-hosting docs for the new setting. ([#4774](#4774)) - The dashboard has two new themes, Black and White, plus appearance options for stronger colors and underlined links. ([#4547](#4547)) - Deployment logs no longer jump to the bottom while you are reading earlier output. Scroll up to pause auto-scroll, and scroll back down or use the new scroll-to-bottom button in the log header to resume following. ([#4776](#4776)) - Customize the runs list: show, hide, and reorder columns, and add smart columns that pull a value straight out of a run's payload, metadata, or output. Your column choices are saved in the page URL, so you can share a view, bookmark it, or save it straight to your favorites. ([#4652](#4652)) - Stop the browser offering to autofill or save environment variable values as saved credentials. ([#4777](#4777)) - Cut webapp CPU usage by about a quarter on the routes that workers call most, freeing headroom at the same request rate. Detailed event-loop blocking traces are no longer recorded by default, because producing them was itself a large part of that cost. ([#4746](#4746)) - When a runs list or runs.list API request spans too much data to complete, it now returns a clear, actionable error asking you to narrow the time range, instead of failing with a generic error. ([#4773](#4773)) - Improved the performance and reliability of the runs list and the runs.list API, especially for large projects and filtered views. ([#4763](#4763)) - New Vercel connections now get version skew protection turned on automatically, so each run uses the task version its deployment shipped with. Automatic atomic deployments are deprecated and no longer offered when you connect a project, but stay available in your Vercel integration settings. ([#4741](#4741)) - The Staging branch setting now shows an upgrade prompt on plans that don't include a Staging environment, instead of looking editable and then silently doing nothing when saved. ([#4784](#4784)) <details> <summary>Raw changeset output</summary> # Releases ## @trigger.dev/build@4.5.13 ### Patch Changes - Updated dependencies: - `@trigger.dev/core@4.5.13` ## trigger.dev@4.5.13 ### Patch Changes - `trigger.dev deploy` now asks the server whether to build with Depot or the native build server unless `--native-build`, `--depot-build`, or `--local-build` is passed, so the native build server can be rolled out per organization without a CLI change. `--local-bundle` and `--detach` now require `--native-build`. ([#4803](#4803)) - Add an experimental `--local-bundle` deploy flag that runs the install and bundling steps on your machine and uploads only the build output; the image is still built remotely. Useful when your project's install step needs tooling or credentials that only exist locally. ([#4331](#4331)) - Send the CLI version header on all API requests so deployments are attributable to a CLI version ([#4778](#4778)) - Updated dependencies: - `@trigger.dev/core@4.5.13` - `@trigger.dev/build@4.5.13` - `@trigger.dev/schema-to-json@4.5.13` ## @trigger.dev/core@4.5.13 ### Patch Changes - `trigger.dev deploy` now asks the server whether to build with Depot or the native build server unless `--native-build`, `--depot-build`, or `--local-build` is passed, so the native build server can be rolled out per organization without a CLI change. `--local-bundle` and `--detach` now require `--native-build`. ([#4803](#4803)) - Add an experimental `--local-bundle` deploy flag that runs the install and bundling steps on your machine and uploads only the build output; the image is still built remotely. Useful when your project's install step needs tooling or credentials that only exist locally. ([#4331](#4331)) - A message that arrives mid-turn and is not injected into that turn is now answered as the next turn, instead of being dropped. This is what the `pendingMessages` docs have always described, and it applies to the default too: configuring `pendingMessages` without a `shouldInject` declines every batch, which previously meant every mid-turn message was lost with no error at either end. ([#4795](#4795)) ```ts chat.agent({ id: "my-chat", pendingMessages: { onReceived: ({ message }) => logger.info("arrived mid-turn", { id: message.id }), // Only interrupt once the agent has started calling tools. shouldInject: ({ steps }) => steps.length > 0, }, run: async ({ messages, signal }) => streamText({ model, messages, abortSignal: signal, // Required for injection. Without it nothing injects, and every // mid-turn message is answered as the next turn instead. ...chat.toStreamTextOptions(), }), }); ``` A declined message keeps its place in the queue, so it survives a crash and is answered by whichever run picks the conversation up. An injected one is consumed at the moment it is injected, so it is never also answered as a later turn. - Fixes a case where a chat could silently lose a message. If a message arrived while the agent was between turns and a stop arrived after it, the cursor the next boot resumed from could point past that message, so it was never answered and no error was raised. This affected `chat.agent`, not just custom agents. ([#4644](#4644)) Fixes a recovered answer being cut off. After a crash the agent replays the message it had not answered yet, but it was replaying the stop that arrived after that message too, so the turn answering it was aborted the moment it began. A stop is now only applied to the turn that was live when it arrived. That holds however the stop got there: sent after the last completed turn, or sent to a chat whose most recent turn was completed by an older version of the SDK. One limitation to know about: the recovered answer is persisted correctly, but a chat page that stayed open across the crash keeps showing the partial answer it had already received. Reload the page to see the full recovered answer. Also fixes a retried send being answered twice. When a send was retried and its idempotency claim was lost, the agent could consume the same message a second time. Custom agent loops can now inspect pending chat input without consuming it, and consume one record at a time, with `chat.messages.hasPending()` and `chat.messages.next()`. Records carry stable identifiers so a redelivery is recognisable. ```ts if (await chat.messages.hasPending()) { const record = await chat.messages.next({ timeoutInSeconds: 0 }); if (record) handle(record.payload); } ``` `hasPending()` answers for messages alone, so a message sitting behind a stop, or behind a record this version of the SDK does not recognise, still reports as pending and is still delivered. Anything the agent has no consumer for is discarded rather than left where it would make every message queued behind it undeliverable. `chat.messages.next()` returning `undefined` means no message became consumable before the timeout. `chat.writeTurnComplete()`'s `sessionInEventId` is the cursor that is safe to resume from, not the sequence of the record the turn answered. It is held back behind any message still waiting to be handled, so a value below the record you just handled is expected. ## @trigger.dev/python@4.5.13 ### Patch Changes - Updated dependencies: - `@trigger.dev/sdk@4.5.13` - `@trigger.dev/core@4.5.13` - `@trigger.dev/build@4.5.13` ## @trigger.dev/react-hooks@4.5.13 ### Patch Changes - Updated dependencies: - `@trigger.dev/core@4.5.13` ## @trigger.dev/redis-worker@4.5.13 ### Patch Changes - Updated dependencies: - `@trigger.dev/core@4.5.13` ## @trigger.dev/rsc@4.5.13 ### Patch Changes - Updated dependencies: - `@trigger.dev/core@4.5.13` ## @trigger.dev/schema-to-json@4.5.13 ### Patch Changes - Updated dependencies: - `@trigger.dev/core@4.5.13` ## @trigger.dev/sdk@4.5.13 ### Patch Changes - Fixed a chat agent hanging after an interrupted turn: when a run was killed mid-answer (out of memory, crash, or eviction) and only the one message it was answering was still outstanding, the new run never replied to it. That message is now re-answered on the new run. ([#4768](#4768)) - Browser chats now keep the active turn open across page reloads when older completion records are replayed. ([#4643](#4643)) - Add `chat.endAndContinue()` so fully hand-rolled custom chat agents can hand a conversation off to a fresh run on the latest deployed task version while preserving unconsumed Session input. ([#4647](#4647)) - Fix chat transport discarding the next turn after stopping generation. `skipToTurnComplete` is now reset when a new message or action is sent, so a message sent after `stopGeneration` streams normally instead of leaving the chat stuck in a streaming state. ([#4744](#4744)) - Custom chat agents now validate and parse client data declared with `chat.withClientData({ schema })` before passing it to agent code. ([#4646](#4646)) - Fixes a message sent while the agent was mid-answer being lost if the run then crashed. The cursor written at the end of each turn could point past a message that had arrived during that turn but had not been answered yet, so the next boot skipped it and no error was raised anywhere. Such a message is now held until a turn actually takes it. ([#4795](#4795)) This also removes the in-memory buffer those messages used to sit in, on both `chat.agent` and `chat.createSession()`, so a message waiting for its turn is durable rather than only present in the worker that received it. - A message that arrives mid-turn and is not injected into that turn is now answered as the next turn, instead of being dropped. This is what the `pendingMessages` docs have always described, and it applies to the default too: configuring `pendingMessages` without a `shouldInject` declines every batch, which previously meant every mid-turn message was lost with no error at either end. ([#4795](#4795)) ```ts chat.agent({ id: "my-chat", pendingMessages: { onReceived: ({ message }) => logger.info("arrived mid-turn", { id: message.id }), // Only interrupt once the agent has started calling tools. shouldInject: ({ steps }) => steps.length > 0, }, run: async ({ messages, signal }) => streamText({ model, messages, abortSignal: signal, // Required for injection. Without it nothing injects, and every // mid-turn message is answered as the next turn instead. ...chat.toStreamTextOptions(), }), }); ``` A declined message keeps its place in the queue, so it survives a crash and is answered by whichever run picks the conversation up. An injected one is consumed at the moment it is injected, so it is never also answered as a later turn. - Fixes a case where a chat could silently lose a message. If a message arrived while the agent was between turns and a stop arrived after it, the cursor the next boot resumed from could point past that message, so it was never answered and no error was raised. This affected `chat.agent`, not just custom agents. ([#4644](#4644)) Fixes a recovered answer being cut off. After a crash the agent replays the message it had not answered yet, but it was replaying the stop that arrived after that message too, so the turn answering it was aborted the moment it began. A stop is now only applied to the turn that was live when it arrived. That holds however the stop got there: sent after the last completed turn, or sent to a chat whose most recent turn was completed by an older version of the SDK. One limitation to know about: the recovered answer is persisted correctly, but a chat page that stayed open across the crash keeps showing the partial answer it had already received. Reload the page to see the full recovered answer. Also fixes a retried send being answered twice. When a send was retried and its idempotency claim was lost, the agent could consume the same message a second time. Custom agent loops can now inspect pending chat input without consuming it, and consume one record at a time, with `chat.messages.hasPending()` and `chat.messages.next()`. Records carry stable identifiers so a redelivery is recognisable. ```ts if (await chat.messages.hasPending()) { const record = await chat.messages.next({ timeoutInSeconds: 0 }); if (record) handle(record.payload); } ``` `hasPending()` answers for messages alone, so a message sitting behind a stop, or behind a record this version of the SDK does not recognise, still reports as pending and is still delivered. Anything the agent has no consumer for is discarded rather than left where it would make every message queued behind it undeliverable. `chat.messages.next()` returning `undefined` means no message became consumable before the timeout. `chat.writeTurnComplete()`'s `sessionInEventId` is the cursor that is safe to resume from, not the sequence of the record the turn answered. It is held back behind any message still waiting to be handled, so a value below the record you just handled is expected. - Updated dependencies: - `@trigger.dev/core@4.5.13` </details> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
2 participants
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.
Follow-up to #4644, now rebased onto main so the diff is just these three commits.
Summary
Two ways a chat could lose a user message, both pre-existing and both raised while reviewing #4644.
A message arriving while a turn was streaming was handed to that turn's push handler and parked in an in-memory array. The router counts a record handed to a handler as terminally decided, so it stopped holding the resume floor behind it, and the turn boundary published a cursor past a message that existed only in that process. A crash before the next turn lost it, silently. Measured: with the message at sequence 1, the boundary published
session-in-event-id: 1, so a resume skipped it.Separately, a message the agent declined to inject was discarded with the turn. Never injected, never written to the wire buffer, never answered. That was also the documented default, since a
pendingMessagesconfig withoutshouldInjectdeclines every batch.Design
Notification and consumption are now separate concerns on the router.
observereports that a record arrived without taking it, so the record stays queued and keeps holding the floor. It is rejected on anat-arrivalroute: an observer there would either have to count as a listener, which would stop an unconsumed stop being discarded and bring back a wedged mailbox, or watch records it cannot affect.takeremoves exactly one queued record.The managed loop and the
chat.createSession()iterator now only subscribe when there is a steering config to feed, and injection is the point of consumption. A declined batch never reaches the take, so its records stay queued and become later turns. Both in-memory wire buffers are gone, so a message waiting for its turn is durable rather than living in whichever worker received it.The floor doubles as the wake cursor:
awaitWakeregisters with it and the server completes the waitpoint immediately if anything sits after that sequence. An over-advanced floor was therefore also a missed wake. It is now recorded on the wait span so a run that never woke can be diagnosed from its trace.Verification
Both fixes have a red and green pair, each checked against the unmodified source rather than only observed to pass:
Also 8 new router tests for
observeandtake. Suites green at 385 for the SDK and 886 for core.Not addressed
A
pendingMessagesconfig with nochat.toStreamTextOptions()spread still swallows messages, because nothing drains the queue at all. Same shape, different trigger, tracked separately.