fix(sdk): reset skipToTurnComplete when a new chat turn starts - #4744
fix(sdk): reset skipToTurnComplete when a new chat turn starts#4744wuweiweiwu wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 9902a08 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 |
|
Hi @wuweiweiwu, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
WalkthroughThe chat transport resets 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Thanks for your contribution! We require all external PRs to be opened in draft status first so you can address CodeRabbit review comments and ensure CI passes before requesting a review. Please re-open this PR as a draft. See CONTRIBUTING.md for details. |
|
Thanks for your contribution! We require all external PRs to be opened in draft status first so you can address CodeRabbit review comments and ensure CI passes before requesting a review. Please re-open this PR as a draft. See CONTRIBUTING.md for details. |
|
Thanks for your contribution! We require all external PRs to be opened in draft status first so you can address CodeRabbit review comments and ensure CI passes before requesting a review. Please re-open this PR as a draft. See CONTRIBUTING.md for details. |
Co-authored-by: Wei-Wei Wu <wei-wei@momentic.ai>
|
Added regression tests in The |
| // A stop that never saw its TURN_COMPLETE leaves the flag set, and the new | ||
| // turn would be skipped record by record. | ||
| state.skipToTurnComplete = false; |
There was a problem hiding this comment.
🔍 Stopped-turn leftover chunks no longer skipped
Clearing skipToTurnComplete (chat.ts:875, chat.ts:1290) means the new subscription streams every record from its resume cursor. The sinceInSeq guard at chat.ts:1994 filters only stale TURN_COMPLETE records, not leftover text-deltas from the stopped turn. If any remain on .out below the new send, they can now surface in the new turn's UI. Depends on server stop semantics; the author's test streamed cleanly.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Real trade-off, and it is intentional. Some notes for a maintainer who knows the server stop semantics:
- The skip flag exists to drop the tail of a stopped turn, and it clears only on that turn's
TURN_COMPLETE. If the stopped turn never writes one, the flag survives every later turn, so the chat is stuck inisStreaminguntil a reload. That is the bug this PR fixes. - Leftover deltas cannot be filtered by cursor today:
session-in-event-idrides onturn-completerecords only (writeTurnCompleteChunkinai.ts), so thesinceInSeqguard cannot see data records. A per-record turn marker on.outwould be needed for exact filtering. - Scope of the residue: only records written between the abort and the moment the stop lands on the agent, and only if the stopped turn never completes. The cost is a few extra text-deltas in the next turn instead of a chat that no longer streams at all.
If stop does write a TURN_COMPLETE for the stopped turn, the flag clears on the next subscription anyway and this reset changes nothing. Happy to follow a different approach if you want the residue filtered server-side.
✅ Checklist
Testing
Reproduced with
useTriggerChatTransport+useChatand the stop pattern from the ai-chat frontend docs:transport.stopGeneration(chatId), thenuseChat'sstop().Before this change the second turn never renders: no parts arrive,
statusstaysstreaming, and the session staysisStreaming: true, so a stop button stays on screen until the page is reloaded. The run itself is fine and everything persists, so a reload shows the full response.Cause:
stopGenerationsetsstate.skipToTurnComplete = true, and the read loop only clears that when it sees aTURN_COMPLETErecord. The abort closes the reader before that record arrives, so the flag survives into the next turn and every record of that turn is skipped, including its ownTURN_COMPLETE.After this change the same sequence streams the second turn normally. Verified against 4.5.11 and 4.5.12 (both affected) with the equivalent patch applied to the built SDK.
Changelog
Reset
skipToTurnCompletewhen a new chat turn or action is sent, so a message sent afterstopGenerationstreams normally instead of leaving the chat stuck in a streaming state.