Skip to content

Commit d54bcaa

Browse files
authored
fix(chat): stop losing a user message that arrived mid-turn (#4795)
Follow-up to [#4644](#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 `pendingMessages` config without `shouldInject` declines every batch. ## Design Notification and consumption are now separate concerns on the router. `observe` reports that a record arrived without taking it, so the record stays queued and keeps holding the floor. It 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 a wedged mailbox, or watch records it cannot affect. `take` removes 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: `awaitWake` registers 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: - the resume cursor test fails on the parent branch and passes here - the declined-message test fails without the second commit and passes with it Also 8 new router tests for `observe` and `take`. Suites green at 385 for the SDK and 886 for core. ## Not addressed A `pendingMessages` config with no `chat.toStreamTextOptions()` spread still swallows messages, because nothing drains the queue at all. Same shape, different trigger, tracked separately.
1 parent 1065251 commit d54bcaa

12 files changed

Lines changed: 1167 additions & 153 deletions

File tree

.changeset/quiet-floors-hold.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@trigger.dev/sdk": patch
3+
---
4+
5+
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.
6+
7+
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.

.changeset/spry-steers-defer.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
---
2+
"@trigger.dev/sdk": patch
3+
"@trigger.dev/core": patch
4+
---
5+
6+
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.
7+
8+
```ts
9+
chat.agent({
10+
id: "my-chat",
11+
pendingMessages: {
12+
onReceived: ({ message }) => logger.info("arrived mid-turn", { id: message.id }),
13+
// Only interrupt once the agent has started calling tools.
14+
shouldInject: ({ steps }) => steps.length > 0,
15+
},
16+
run: async ({ messages, signal }) =>
17+
streamText({
18+
model,
19+
messages,
20+
abortSignal: signal,
21+
// Required for injection. Without it nothing injects, and every
22+
// mid-turn message is answered as the next turn instead.
23+
...chat.toStreamTextOptions(),
24+
}),
25+
});
26+
```
27+
28+
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.

docs/ai-chat/client-protocol.mdx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -953,9 +953,12 @@ You can send messages while the agent is still streaming a response. These are *
953953
954954
The wire format is identical to a normal `kind: "message"` send — same `.in` channel, single `message` field. The difference is timing. What happens depends on the agent's `pendingMessages` configuration:
955955
956-
- **With `pendingMessages.shouldInject`**: the message is injected into the model's context at the next `prepareStep` boundary. The agent sees it and can adjust its behavior mid-response.
956+
- **With `pendingMessages.shouldInject` returning `true`**: the message is injected into the model's context at the next `prepareStep` boundary. The agent sees it and can adjust its behavior mid-response.
957+
- **With a `pendingMessages` config that declines it**, either because `shouldInject` returned `false` or because it is absent: the message stays queued on the backend and is answered as the next turn.
957958
- **Without `pendingMessages` config**: the message queues for the next turn.
958959
960+
In every case the message is answered. A declined message keeps its place in the queue, so it also survives a crash and is picked up by whichever run continues the conversation.
961+
959962
See [Pending Messages](/ai-chat/pending-messages) for how to configure the agent side.
960963
961964
<Note>

docs/ai-chat/pending-messages.mdx

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@ When an AI agent is executing tool calls, users may want to send a message that
1010

1111
By default (without `pendingMessages`), a message sent while the agent is responding never interrupts the in-flight response: it's buffered and processed as its own turn once the current turn completes, with multiple messages running sequentially in arrival order.
1212

13-
The `pendingMessages` option enables steering instead, injecting user messages between tool-call steps via the AI SDK's `prepareStep`. Messages that arrive during streaming are queued and injected at the next step boundary. If there are no more step boundaries (single-step response or final text generation), the message becomes the next turn automatically.
13+
The `pendingMessages` option enables steering instead, injecting user messages between tool-call steps via the AI SDK's `prepareStep`. Messages that arrive during streaming are queued and injected at the next step boundary. A message that is not injected becomes the next turn instead, whether that is because `shouldInject` returned `false` or because there were no more step boundaries (single-step response or final text generation). The backend handles that, so no client-side re-send is involved.
14+
15+
Injection is what needs wiring: the `pendingMessages` options only reach `streamText` if you spread `chat.toStreamTextOptions()` (or pass `prepareStep`). Without that, nothing injects, so every mid-turn message is answered as the next turn. Deferral does not depend on it.
1416

1517
## How it works
1618

@@ -20,7 +22,7 @@ The `pendingMessages` option enables steering instead, injecting user messages b
2022
4. At the next `prepareStep` boundary (between tool-call steps), `shouldInject` is called
2123
5. If it returns `true`, the message is injected into the LLM's context
2224
6. A `data-pending-message-injected` stream chunk confirms injection to the frontend
23-
7. If `prepareStep` never fires (no tool calls), the message becomes the next turn
25+
7. If `shouldInject` returns `false`, or `prepareStep` never fires (no tool calls), the message stays queued on the backend and is answered as the next turn
2426

2527
## Backend: chat.agent
2628

@@ -310,7 +312,7 @@ function Chat({ chatId }: { chatId: string }) {
310312

311313
### Message lifecycle
312314

313-
- **Steering messages** are sent via `transport.sendPendingMessage()` immediately. They appear as purple pending bubbles. If injected, they disappear from the overlay and render inline at the injection point. If not injected (no more step boundaries), they auto-send as the next turn when the response finishes.
315+
- **Steering messages** are sent via `transport.sendPendingMessage()` immediately. They appear as purple pending bubbles. If injected, they disappear from the overlay and render inline at the injection point. If not injected, the backend answers them as the next turn once the response finishes; the client does not need to re-send them.
314316

315317
- **Queued messages** stay client-side until the turn completes, then auto-send as the next turn via `sendMessage()`. They can be promoted to steering mid-stream by clicking "Steer instead".
316318

docs/ai-chat/reference.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,7 @@ Options for the `pendingMessages` field. See [Pending Messages](/ai-chat/pending
396396

397397
| Option | Type | Required | Description |
398398
| -------------- | --------------------------------------------------------------------------------- | -------- | ----------------------------------------------------------------------------------------- |
399-
| `shouldInject` | `(event: PendingMessagesBatchEvent) => boolean \| Promise<boolean>` | No | Decide whether to inject the batch between tool-call steps. If absent, no injection. |
399+
| `shouldInject` | `(event: PendingMessagesBatchEvent) => boolean \| Promise<boolean>` | No | Decide whether to inject the batch between tool-call steps. If absent, nothing is injected and the messages are answered as the next turn. Only consulted when `chat.toStreamTextOptions()` (or `prepareStep`) reaches `streamText`; without that nothing is injected and every mid-turn message becomes the next turn. |
400400
| `prepare` | `(event: PendingMessagesBatchEvent) => ModelMessage[] \| Promise<ModelMessage[]>` | No | Transform the batch before injection. Default: convert each via `convertToModelMessages`. |
401401
| `onReceived` | `(event: PendingMessageReceivedEvent) => void \| Promise<void>` | No | Called when a message arrives during streaming (per-message). |
402402
| `onInjected` | `(event: PendingMessagesInjectedEvent) => void \| Promise<void>` | No | Called after a batch is injected via prepareStep. |

packages/core/src/v3/sessionStreams/router.test.ts

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,3 +407,169 @@ describe("SessionChannelRouter: exactly-once across a crash", () => {
407407
}
408408
});
409409
});
410+
411+
describe("SessionChannelRouter: observe", () => {
412+
it("notifies without consuming, so the record still queues and holds the floor", () => {
413+
const r = router();
414+
const seen: number[] = [];
415+
r.observe("messages", (record) => seen.push(record.seqNum));
416+
417+
r.ingest(rec(0, "message"));
418+
419+
expect(seen).toEqual([0]);
420+
expect(r.hasPending("messages")).toBe(true);
421+
expect(r.resumeFloor()).toBeUndefined();
422+
});
423+
424+
it("does not satisfy a queue route's handler delivery", () => {
425+
const r = router();
426+
const observed: number[] = [];
427+
const handled: number[] = [];
428+
r.observe("messages", (record) => observed.push(record.seqNum));
429+
430+
r.ingest(rec(0, "message"));
431+
expect(handled).toEqual([]);
432+
433+
r.on("messages", (record) => handled.push(record.seqNum));
434+
expect(handled).toEqual([0]);
435+
expect(observed).toEqual([0]);
436+
});
437+
438+
it("rejects an at-arrival route, so a stop with only an observer is still discarded", () => {
439+
const r = router();
440+
expect(() => r.observe("stop", () => {})).toThrow(/at-arrival/);
441+
});
442+
443+
it("does not re-offer records that were already queued when it attached", () => {
444+
const r = router();
445+
r.ingest(rec(0, "message"));
446+
447+
const seen: number[] = [];
448+
r.observe("messages", (record) => seen.push(record.seqNum));
449+
expect(seen).toEqual([]);
450+
451+
r.ingest(rec(1, "message"));
452+
expect(seen).toEqual([1]);
453+
});
454+
455+
it("stops notifying after off()", () => {
456+
const r = router();
457+
const seen: number[] = [];
458+
const sub = r.observe("messages", (record) => seen.push(record.seqNum));
459+
460+
r.ingest(rec(0, "message"));
461+
sub.off();
462+
r.ingest(rec(1, "message"));
463+
464+
expect(seen).toEqual([0]);
465+
});
466+
});
467+
468+
describe("SessionChannelRouter: take", () => {
469+
it("removes one queued record by sequence and releases the floor", () => {
470+
const r = router();
471+
r.ingest(rec(0, "message"));
472+
r.ingest(rec(1, "message"));
473+
474+
expect(r.take("messages", 0)?.seqNum).toBe(0);
475+
expect(r.pendingCount("messages")).toBe(1);
476+
expect(r.peek("messages")?.seqNum).toBe(1);
477+
});
478+
479+
it("reports false for a record that is no longer queued", () => {
480+
const r = router();
481+
r.ingest(rec(0, "message"));
482+
483+
expect(r.take("messages", 0)?.seqNum).toBe(0);
484+
expect(r.take("messages", 0)).toBeUndefined();
485+
expect(r.take("messages", 99)).toBeUndefined();
486+
});
487+
488+
it("leaves an untaken observed record to be delivered as normal", async () => {
489+
const r = router();
490+
r.observe("messages", () => {});
491+
r.ingest(rec(0, "message"));
492+
r.ingest(rec(1, "message"));
493+
494+
r.take("messages", 0);
495+
496+
const next = await r.next("messages", { timeoutMs: 0 });
497+
expect(next?.seqNum).toBe(1);
498+
});
499+
});
500+
501+
describe("SessionChannelRouter: clearRoute guard", () => {
502+
it("refuses to clear a replayable route", () => {
503+
const r = router();
504+
r.ingest(rec(0, "message"));
505+
506+
expect(() => r.clearRoute("messages")).toThrow(/replayable/);
507+
expect(r.hasPending("messages")).toBe(true);
508+
});
509+
510+
it("still clears a non-replayable route", () => {
511+
const r = router();
512+
r.ingest(rec(0, "handover"));
513+
expect(r.hasPending("handover")).toBe(true);
514+
515+
r.clearRoute("handover");
516+
expect(r.hasPending("handover")).toBe(false);
517+
});
518+
});
519+
520+
describe("SessionChannelRouter: observe versus a waiting consumer", () => {
521+
it("notifies the observer even when a parked puller takes the record", async () => {
522+
const r = router();
523+
const seen: number[] = [];
524+
r.observe("messages", (record) => seen.push(record.seqNum));
525+
526+
const pull = r.next("messages");
527+
r.ingest(rec(0, "message"));
528+
const taken = await pull;
529+
530+
expect(seen).toEqual([0]);
531+
expect(taken?.seqNum).toBe(0);
532+
expect(r.hasPending("messages")).toBe(false);
533+
});
534+
535+
it("reports a failed take for a record a puller already consumed", async () => {
536+
const r = router();
537+
const seen: number[] = [];
538+
r.observe("messages", (record) => seen.push(record.seqNum));
539+
540+
const pull = r.next("messages");
541+
r.ingest(rec(0, "message"));
542+
await pull;
543+
544+
expect(seen).toEqual([0]);
545+
expect(r.take("messages", 0)).toBeUndefined();
546+
});
547+
});
548+
549+
describe("SessionChannelRouter: untake", () => {
550+
it("puts a claimed record back in sequence order", async () => {
551+
const r = router();
552+
r.ingest(rec(0, "message"));
553+
r.ingest(rec(2, "message"));
554+
555+
const taken = r.take("messages", 0)!;
556+
expect(r.peek("messages")?.seqNum).toBe(2);
557+
558+
r.untake("messages", taken);
559+
560+
expect(r.pendingCount("messages")).toBe(2);
561+
expect(r.peek("messages")?.seqNum).toBe(0);
562+
expect(r.resumeFloor()).toBeUndefined();
563+
});
564+
565+
it("is idempotent, so a double return cannot duplicate a record", () => {
566+
const r = router();
567+
r.ingest(rec(0, "message"));
568+
const taken = r.take("messages", 0)!;
569+
570+
r.untake("messages", taken);
571+
r.untake("messages", taken);
572+
573+
expect(r.pendingCount("messages")).toBe(1);
574+
});
575+
});

packages/core/src/v3/sessionStreams/router.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ class RouteState {
101101
readonly queue: SessionStreamRecord[] = [];
102102
readonly waiters: QueueWaiter[] = [];
103103
readonly handlers = new Set<RouteHandler>();
104+
readonly observers = new Set<RouteHandler>();
104105

105106
constructor(readonly route: SessionRoute) {}
106107

@@ -239,6 +240,14 @@ export class SessionChannelRouter {
239240
return this.#drop(record, "no-handler", routeName);
240241
}
241242

243+
for (const observer of state.observers) {
244+
try {
245+
observer(record);
246+
} catch {
247+
void 0;
248+
}
249+
}
250+
242251
const waiter = state.waiters.shift();
243252
if (waiter) {
244253
if (waiter.timer) clearTimeout(waiter.timer);
@@ -356,6 +365,70 @@ export class SessionChannelRouter {
356365
};
357366
}
358367

368+
/**
369+
* Watch a route without consuming from it.
370+
*
371+
* Notification and consumption are separate concerns: an observer is told
372+
* that a record arrived and the record still queues, so the resume floor
373+
* stays held behind it until something actually takes it. A push handler
374+
* registered with {@link on} is the opposite, and a record handed to one
375+
* counts as terminally decided.
376+
*
377+
* Only meaningful on a replayable route. On an `at-arrival` route an observer
378+
* would either have to count as a listener, which would stop an unconsumed
379+
* record being discarded, or watch records it cannot affect, so it is
380+
* rejected rather than given one of those two meanings.
381+
*
382+
* Records already queued are not re-offered: an observer reports arrivals
383+
* from the moment it attaches, so re-offering would fire twice for a record
384+
* that arrived before a later consumer attached.
385+
*/
386+
observe(name: string, observer: RouteHandler): { off: () => void } {
387+
const state = this.#stateOrThrow(name);
388+
if (state.route.delivery === "at-arrival") {
389+
throw new Error(
390+
`Route "${name}" is at-arrival, which cannot be observed: an observer must not decide whether a record is discarded, and cannot be offered one that already was`
391+
);
392+
}
393+
state.observers.add(observer);
394+
return {
395+
off: () => {
396+
state.observers.delete(observer);
397+
},
398+
};
399+
}
400+
401+
/**
402+
* Remove one queued record, identified by sequence.
403+
*
404+
* For a consumer that decided to take a record it had only observed. Returns
405+
* whether it was still queued, so a caller can tell a real take from a record
406+
* something else had already consumed.
407+
*/
408+
take(name: string, seqNum: number): SessionStreamRecord | undefined {
409+
const state = this.#stateOrThrow(name);
410+
const index = state.queue.findIndex((record) => record.seqNum === seqNum);
411+
if (index === -1) return undefined;
412+
return state.queue.splice(index, 1)[0];
413+
}
414+
415+
/**
416+
* Put a taken record back, in sequence order.
417+
*
418+
* For a consumer that claimed a record and then could not use it. Returning
419+
* it leaves the route as though the claim never happened, so the record is
420+
* delivered later and goes back to holding the resume floor. Without this a
421+
* failed claim-then-use is indistinguishable from a delivery, and the record
422+
* is lost.
423+
*/
424+
untake(name: string, record: SessionStreamRecord): void {
425+
const state = this.#stateOrThrow(name);
426+
if (state.queue.some((queued) => queued.seqNum === record.seqNum)) return;
427+
const at = state.queue.findIndex((queued) => queued.seqNum > record.seqNum);
428+
if (at === -1) state.queue.push(record);
429+
else state.queue.splice(at, 0, record);
430+
}
431+
359432
/** Whether an `at-arrival` route currently has anywhere to deliver. */
360433
hasHandler(name: string): boolean {
361434
return this.#stateOrThrow(name).handlers.size > 0;
@@ -417,6 +490,11 @@ export class SessionChannelRouter {
417490
*/
418491
clearRoute(name: string): void {
419492
const state = this.#stateOrThrow(name);
493+
if (state.route.replayable) {
494+
throw new Error(
495+
`Route "${name}" is replayable, so its queue cannot be cleared: anything queued on it is still owed to a later boot, and discarding it would lose records the resume floor is holding back`
496+
);
497+
}
420498
state.queue.length = 0;
421499
for (const waiter of state.waiters) {
422500
if (waiter.timer) clearTimeout(waiter.timer);
@@ -430,6 +508,7 @@ export class SessionChannelRouter {
430508
for (const state of this.#routes.values()) {
431509
state.queue.length = 0;
432510
state.handlers.clear();
511+
state.observers.clear();
433512
for (const waiter of state.waiters) {
434513
if (waiter.timer) clearTimeout(waiter.timer);
435514
waiter.resolve(undefined);

0 commit comments

Comments
 (0)