fix: sweep only failed chips when the user aborts

The abort listener also fires on navigating away while the run continues
server side. Sweeping pending chips there stranded a steer the server had
already acked and sent it a second time. selectLocalSteersForQueue takes
a statuses argument so abort sweeps failed only.
This commit is contained in:
Marco Beretta 2026-07-26 16:32:30 +02:00
parent 50e64a142e
commit 82cfe41033
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
5 changed files with 73 additions and 23 deletions

View file

@ -82,12 +82,16 @@ function Queue({ steering, conversationId, onEditToComposer, onRestoreToComposer
<span
className="shrink-0 text-xs text-text-secondary"
title={localize('com_ui_queued_attachment_count', { 0: String(fileCount) })}
aria-label={localize('com_ui_queued_attachment_count', { 0: String(fileCount) })}
>
{localize(
fileCount === 1 ? 'com_ui_attachment_count_one' : 'com_ui_attachment_count',
{ count: fileCount },
)}
<span className="sr-only">
{localize('com_ui_queued_attachment_count', { 0: String(fileCount) })}
</span>
<span aria-hidden="true">
{localize(
fileCount === 1 ? 'com_ui_attachment_count_one' : 'com_ui_attachment_count',
{ count: fileCount },
)}
</span>
</span>
)}
<button

View file

@ -93,6 +93,11 @@ describe('Queue', () => {
renderQueue([queued({ files: [{ file_id: 'f1' }, { file_id: 'f2' }] as never })]);
const attachmentLabel = screen.getByText('com_ui_attachment_count:2');
expect(attachmentLabel).toBeInTheDocument();
expect(attachmentLabel).toHaveAttribute('title', 'com_ui_queued_attachment_count:2');
expect(attachmentLabel.parentElement).toHaveAttribute(
'title',
'com_ui_queued_attachment_count:2',
);
expect(attachmentLabel.parentElement).not.toHaveAttribute('aria-label');
expect(screen.getByText('com_ui_queued_attachment_count:2')).toHaveClass('sr-only');
});
});

View file

@ -3870,6 +3870,14 @@ describe('useResumableSSE', () => {
* the selection logic directly (this is the exact piece of logic finding 3
* was about which statuses survive a run end) rather than faking a
* RecoilRoot-backed integration around the rest of this large hook's mocks.
*
* The `statuses` parameter pins a second contract: the `final` handler and
* the two error/404 terminals call `convertLocalSteersToQueued` with no
* override (default `pending || failed`, the run is genuinely over), while
* the intentional-close `abort` listener passes `{ statuses: ['failed'] }`
* because that listener also fires on navigation away while the run
* CONTINUES server-side a server-ACK'd `pending` chip must be left alone
* there, not swept into the queue as a duplicate of the server's own injection.
*/
describe('selectLocalSteersForQueue', () => {
const chip = (over: Partial<PendingSteer> = {}): PendingSteer => ({
@ -3880,7 +3888,7 @@ describe('selectLocalSteersForQueue', () => {
...over,
});
it('includes pending and failed chips, excluding sending', () => {
it('final/error-path selection (default statuses) includes pending and failed chips, excluding sending', () => {
const chips = [
chip({ steerId: 'p1', status: 'pending' }),
chip({ steerId: 'f1', status: 'failed' }),
@ -3889,6 +3897,21 @@ describe('selectLocalSteersForQueue', () => {
expect(selectLocalSteersForQueue(chips).map((steer) => steer.steerId)).toEqual(['p1', 'f1']);
});
it('abort-path selection (statuses: ["failed"]) sweeps only failed chips, leaving pending alone', () => {
// The abort listener fires on navigation away while the run CONTINUES
// server-side: a `pending` chip already has a server id and will be
// injected by the server regardless, so sweeping it here too would make
// `useQueueDrain` resend the same words as a duplicate turn at run end.
const chips = [
chip({ steerId: 'p1', status: 'pending' }),
chip({ steerId: 'f1', status: 'failed' }),
chip({ steerId: 'sending-1', status: 'sending' }),
];
expect(selectLocalSteersForQueue(chips, ['failed']).map((steer) => steer.steerId)).toEqual([
'f1',
]);
});
it('converts a failed local chip present at a run-end path into a queueable item', () => {
// The leak finding 3 was about: a `failed` chip carries a local-* id the
// server never reports, so `data.pendingSteers`/the abort response can

View file

@ -607,29 +607,39 @@ const mergeResumeMessages = (
return [...nextMessages, userMessage, responseMessage];
};
/** Default sweep for a run that has genuinely ended (final/error/404): a
* `pending` chip behind a server id that no `on_steer_applied` will ever
* confirm, or a `failed` chip that never reached the server at all. Either
* one left behind survives past this run end and renders under whatever
* comes next `PendingSteers` reads the same atom keyed only by
* conversation, not by run. */
const RUN_ENDED_STATUSES: readonly PendingSteer['status'][] = ['pending', 'failed'];
/**
* Local chips with no injection-boundary event left to resolve them: `pending`
* behind a server id that no `on_steer_applied` will ever confirm (the run
* ended), or `failed` and never having reached the server at all. Either one
* left behind survives past this run end and renders under whatever comes
* next `PendingSteers` reads the same atom keyed only by conversation, not
* by run.
* Local chips with no injection-boundary event left to resolve them.
* `statuses` defaults to `RUN_ENDED_STATUSES` for terminals where the run is
* actually over; pass `['failed']` for a path (like intentional abort) where
* the run may still be live server-side and a server-ACK'd `pending` chip
* must be left alone the server injects it regardless, and sweeping it here
* would resend the same words as a duplicate queued turn.
*
* `sending` chips are deliberately excluded: they have their own in-flight
* POST whose `onSuccess`/`onError` will settle them, and sweeping them here
* too would race that callback a late ACK's re-add in
* `sending` chips are never included: they have their own in-flight POST
* whose `onSuccess`/`onError` will settle them, and sweeping them here too
* would race that callback a late ACK's re-add in
* `resolveAcknowledgedSteer` could then double-queue the same words.
*/
export function selectLocalSteersForQueue(
chips: PendingSteer[],
statuses: readonly PendingSteer['status'][] = RUN_ENDED_STATUSES,
/** Ids the caller has already routed elsewhere this run end; matched against
* both ids a chip can be known by, since either may be the one excluded. */
excluded: ReadonlySet<string> = new Set(),
): TPendingSteer[] {
const allowed = new Set(statuses);
return chips
.filter(
(steer) =>
(steer.status === 'pending' || steer.status === 'failed') &&
allowed.has(steer.status) &&
!excluded.has(steer.steerId) &&
(steer.clientSteerId == null || !excluded.has(steer.clientSteerId)),
)
@ -977,15 +987,18 @@ export default function useResumableSSE(
/** Sweeps this conversation's local chips (see `selectLocalSteersForQueue`)
* into queued follow-ups. Error events carry no `pendingSteers` payload of
* their own (the server drops its copy on failure) this is the only
* source of truth for those runs; the `final` and intentional-`abort`
* paths call it alongside their own server-reported list as a backstop for
* `failed` chips, which never rode that list at all. */
* source of truth for those runs; the `final` and error/404 terminals call
* it (default `statuses`, i.e. `pending || failed`) alongside their own
* server-reported list as a backstop for `failed` chips, which never rode
* that list at all. The intentional-abort path passes `statuses: ['failed']`
* since the run may still be live server-side there (see its call site). */
const convertLocalSteersToQueued = useRecoilCallback(
({ snapshot }) =>
(
conversationId: string,
options?: {
claimParked?: boolean;
statuses?: readonly PendingSteer['status'][];
excludeSteerIds?: Iterable<string>;
generationProtocolVersion?: GenerationProtocolVersion;
},
@ -993,6 +1006,7 @@ export default function useResumableSSE(
const chips = snapshot.getLoadable(store.pendingSteersByConvoId(conversationId)).getValue();
const settled = selectLocalSteersForQueue(
chips,
options?.statuses,
new Set(options?.excludeSteerIds ?? []),
);
if (settled.length > 0) {
@ -3106,9 +3120,15 @@ export default function useResumableSSE(
resetLive({ ...currentSubmission, userMessage });
// No final/error event fires on this path, so it's the only place left
// to sweep a local `failed` chip — otherwise it survives this close and
// renders (with a live Retry) under whatever run starts next.
// renders (with a live Retry) under whatever run starts next. `pending`
// chips are deliberately left alone here: this listener also fires on
// navigation away while the run CONTINUES server-side, so a
// server-ACK'd `pending` steer is not stranded — the server injects it
// regardless, and sweeping it into the queue too would resend the same
// words as a duplicate turn once `useQueueDrain` fires at run end.
convertLocalSteersToQueued(
currentSubmission.conversation?.conversationId ?? currentStreamId,
{ statuses: ['failed'] },
);
});

View file

@ -1957,8 +1957,6 @@
"com_ui_stateful_sessions": "Stateful code sessions",
"com_ui_status_prefix": "Status:",
"com_ui_steer": "Steer",
"com_ui_steer_already_applied": "That steering message already reached the agent, so it was left in the response",
"com_ui_steer_cancel": "Cancel steering message",
"com_ui_steer_cancel_failed": "Could not cancel the steering message — it may still reach the agent",
"com_ui_steer_failed": "Steering failed",
"com_ui_steer_failed_inline": "Couldn't add to this reply",