Skip to content

Commit 5ce7948

Browse files
committed
fix(tui): keep leftover same-name collect under the stall budget
Two concurrent shell_collect calls share one callIdByName slot, so when the mapping-owning sibling resolves first and clears it, the leftover earlier collect loses its name and pins the turn with no wall-clock bound. Track the tool name per call id (callNameById) in register/ unregister and consult it in isStallBoundedInFlightTool, so the leftover still aborts at the stall budget. Fixes CL-8059
1 parent 810fd77 commit 5ce7948

5 files changed

Lines changed: 132 additions & 3 deletions

File tree

‎src/tui/runtime-bridge.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1551,6 +1551,7 @@ export function attachSessionBridge(
15511551
currentToolName: bag.turn.currentToolName,
15521552
activeToolCalls: bag.turn.activeToolCalls,
15531553
callIdByName: bag.turn.callIdByName,
1554+
callNameById: bag.turn.callNameById,
15541555
stallNoticeMs,
15551556
repeating: bag.turn.repeating,
15561557
});

‎src/tui/stall-watchdog.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,41 @@ describe("shouldAbortForStall — execution-watchdog-exempt tools do not pin for
211211
}),
212212
).toBe(false);
213213
});
214+
215+
// Two concurrent collects share one callIdByName slot: the second
216+
// registration overwrites the first, so when the mapping-owning sibling
217+
// resolves first and clears the slot, the leftover earlier collect is
218+
// invisible to the name-keyed check above. The per-id record keeps it
219+
// bounded (CL-8059).
220+
test("concurrent collects with the mapping owner done first still abort at the stall budget", () => {
221+
const leftover = {
222+
...collect,
223+
currentToolName: null,
224+
streamingType: null,
225+
awaitingResponse: false,
226+
activeToolCalls: ["collect-1"],
227+
callIdByName: {},
228+
callNameById: { "collect-1": "shell_collect" },
229+
};
230+
expect(
231+
shouldAbortForStall({ ...leftover, nowMs: STALL_TIMEOUT_MS - 1 }),
232+
).toBe(false);
233+
expect(shouldAbortForStall(leftover)).toBe(true);
234+
});
235+
236+
test("a leftover ordinary tool tracked per-id is not a stall", () => {
237+
expect(
238+
shouldAbortForStall({
239+
...collect,
240+
currentToolName: null,
241+
streamingType: null,
242+
awaitingResponse: false,
243+
activeToolCalls: ["bash-1"],
244+
callIdByName: {},
245+
callNameById: { "bash-1": "bash" },
246+
}),
247+
).toBe(false);
248+
});
214249
});
215250

216251
describe("applyStallRecovery", () => {

‎src/tui/stall-watchdog.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ export interface ShouldAbortForStallArgs {
2525
readonly currentToolName: string | null;
2626
readonly activeToolCalls: readonly string[];
2727
readonly callIdByName: Readonly<Record<string, string>>;
28+
/**
29+
* Tool name for each in-flight real call id. Optional so callers holding
30+
* only the legacy name-keyed record still compile; absent means no
31+
* per-id names are known. See `isStallBoundedInFlightTool`.
32+
*/
33+
readonly callNameById?: Readonly<Record<string, string>>;
2834
}
2935

3036
/**
@@ -62,7 +68,10 @@ function silentPastThreshold(
6268
* bound they hold the turn with no other wall-clock limit. A sibling
6369
* tool.done clears `currentToolName` / `streamingType` while the poll is
6470
* still in `activeToolCalls`, so the bound keys any in-flight stall-bounded
65-
* name, not only the last announced tool.
71+
* name, not only the last announced tool. The name-keyed `callIdByName`
72+
* holds one id per tool, so concurrent same-name siblings overwrite each
73+
* other there; the per-id `callNameById` covers the leftover when the
74+
* mapping-owning sibling resolves first and clears the shared slot.
6675
*/
6776
function isStallBoundedToolName(name: string | null | undefined): boolean {
6877
return (
@@ -79,6 +88,13 @@ function isStallBoundedInFlightTool(args: ShouldAbortForStallArgs): boolean {
7988
return true;
8089
}
8190
}
91+
// Concurrent same-name siblings share one callIdByName slot, so the
92+
// mapping-owning sibling clearing it on tool.done must not release the
93+
// leftover earlier call: its own id still maps to the bounded name here.
94+
const callNameById = args.callNameById ?? {};
95+
for (const id of args.activeToolCalls) {
96+
if (isStallBoundedToolName(callNameById[id])) return true;
97+
}
8298
// Name-only announcements track the call under its name until a real id
8399
// arrives, so the placeholder itself is the stall-bounded name.
84100
return args.activeToolCalls.some(isStallBoundedToolName);

‎src/tui/turn-state.test.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,43 @@ describe("turnStateFromEvent", () => {
155155
expect(bothDone.activeToolCalls).toHaveLength(0);
156156
});
157157

158+
test("concurrent same-name collects stay tracked when the mapping owner finishes first", () => {
159+
// Regression for CL-8059: two concurrent shell_collect calls share one
160+
// callIdByName slot, so the second registration overwrites the first.
161+
// When the mapping-owning sibling resolves first and clears that slot,
162+
// the leftover earlier collect must keep its own name record.
163+
const running = fold([
164+
{ type: "inference.start" },
165+
{
166+
type: "tool.start",
167+
data: { call: { id: "collect-1", name: "shell_collect" } },
168+
},
169+
{
170+
type: "tool.start",
171+
data: { call: { id: "collect-2", name: "shell_collect" } },
172+
},
173+
]);
174+
expect(running.activeToolCalls).toHaveLength(2);
175+
176+
const oneDone = turnStateFromEvent(
177+
running,
178+
{ type: "tool.done", data: { result: { callId: "collect-2" } } },
179+
200,
180+
);
181+
expect(oneDone.activeToolCalls).toEqual(["collect-1"]);
182+
expect(oneDone.callNameById).toEqual({
183+
"collect-1": "shell_collect",
184+
});
185+
186+
const bothDone = turnStateFromEvent(
187+
oneDone,
188+
{ type: "tool.done", data: { result: { callId: "collect-1" } } },
189+
201,
190+
);
191+
expect(bothDone.activeToolCalls).toHaveLength(0);
192+
expect(bothDone.callNameById).toEqual({});
193+
});
194+
158195
test("tool.done only sets awaitingResponse once every parallel call has finished", () => {
159196
// Regression for CL-5661: with a fan-out of two outstanding calls, the
160197
// first tool.done must not claim the turn is idle while the second call

‎src/tui/turn-state.ts‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,15 @@ export interface TurnState {
9393
* onto one `activeToolCalls` entry. See `registerActiveCall`.
9494
*/
9595
readonly callIdByName: Readonly<Record<string, string>>;
96+
/**
97+
* Tool name for each in-flight real call id, so two concurrent calls to
98+
* the same tool each keep their own record. `callIdByName` holds only one
99+
* id per name — the latest registration wins — so when the mapping-owning
100+
* sibling resolves first and clears that slot, the leftover earlier call
101+
* would otherwise lose its name and (for stall-bounded polls like
102+
* `shell_collect`) its stall budget. See `registerActiveCall`.
103+
*/
104+
readonly callNameById: Readonly<Record<string, string>>;
96105
/**
97106
* Tail of the text/thinking output streamed in the current uninterrupted
98107
* streaming cycle. A tool call ends the cycle and clears it: a model
@@ -147,6 +156,7 @@ export function initialTurnState(nowMs: number): TurnState {
147156
quota: null,
148157
activeToolCalls: [],
149158
callIdByName: {},
159+
callNameById: {},
150160
streamText: "",
151161
repeating: false,
152162
repeatingSinceTokenCount: null,
@@ -189,6 +199,7 @@ export function turnStateOnSubmit(state: TurnState, nowMs: number): TurnState {
189199
lastActivityAt: nowMs,
190200
activeToolCalls: [],
191201
callIdByName: {},
202+
callNameById: {},
192203
streamText: "",
193204
repeating: false,
194205
repeatingSinceTokenCount: null,
@@ -385,6 +396,23 @@ interface CallTracking {
385396
* the original name-keyed tracking collapsed them identically.
386397
*/
387398
readonly callIdByName: Readonly<Record<string, string>>;
399+
/**
400+
* Tool name for each in-flight real call id. Unlike `callIdByName` this
401+
* keeps one entry per call, so concurrent same-name siblings do not
402+
* overwrite each other — the entry that survives is keyed by the
403+
* leftover call's own id, not the shared name.
404+
*/
405+
readonly callNameById: Readonly<Record<string, string>>;
406+
}
407+
408+
function withoutCallNameById(
409+
callNameById: Readonly<Record<string, string>>,
410+
id: string,
411+
): Readonly<Record<string, string>> {
412+
if (!(id in callNameById)) return callNameById;
413+
return Object.fromEntries(
414+
Object.entries(callNameById).filter(([callId]) => callId !== id),
415+
);
388416
}
389417

390418
/**
@@ -396,13 +424,17 @@ function registerActiveCall(
396424
tracking: CallTracking,
397425
identity: CallIdentity,
398426
): CallTracking {
399-
const { activeToolCalls, callIdByName } = tracking;
427+
const { activeToolCalls, callIdByName, callNameById } = tracking;
400428

401429
if (identity.id !== undefined) {
402430
const nextCallIdByName =
403431
identity.name !== undefined
404432
? { ...callIdByName, [identity.name]: identity.id }
405433
: callIdByName;
434+
const nextCallNameById =
435+
identity.name !== undefined
436+
? { ...callNameById, [identity.id]: identity.name }
437+
: callNameById;
406438
// A provisional entry may already be tracking this call under its name —
407439
// promote it onto the real id in place instead of adding a duplicate.
408440
const withoutPlaceholder =
@@ -412,6 +444,7 @@ function registerActiveCall(
412444
return {
413445
activeToolCalls: withActiveCall(withoutPlaceholder, identity.id),
414446
callIdByName: nextCallIdByName,
447+
callNameById: nextCallNameById,
415448
};
416449
}
417450

@@ -420,12 +453,14 @@ function registerActiveCall(
420453
return {
421454
activeToolCalls: withActiveCall(activeToolCalls, id),
422455
callIdByName,
456+
callNameById,
423457
};
424458
}
425459

426460
return {
427461
activeToolCalls: withActiveCall(activeToolCalls, "tool"),
428462
callIdByName,
463+
callNameById,
429464
};
430465
}
431466

@@ -455,12 +490,14 @@ function unregisterActiveCall(
455490
tracking: CallTracking,
456491
identity: CallIdentity,
457492
): CallTracking {
458-
const { activeToolCalls, callIdByName } = tracking;
493+
const { activeToolCalls, callIdByName, callNameById } = tracking;
459494

460495
if (identity.id !== undefined) {
461496
// Clear the mapping once its call resolves, or a later call reusing the
462497
// same tool name would resolve straight to this now-finished id instead
463498
// of tracking its own — reproducing the leak this function exists to fix.
499+
// The per-id record clears only the finished call's own entry, so a
500+
// concurrent same-name sibling keeps its name (CL-8059).
464501
const resolvedName =
465502
identity.name ?? nameForCallId(callIdByName, identity.id);
466503
const nextCallIdByName =
@@ -470,6 +507,7 @@ function unregisterActiveCall(
470507
return {
471508
activeToolCalls: withoutActiveCall(activeToolCalls, identity.id),
472509
callIdByName: nextCallIdByName,
510+
callNameById: withoutCallNameById(callNameById, identity.id),
473511
};
474512
}
475513

@@ -478,12 +516,14 @@ function unregisterActiveCall(
478516
return {
479517
activeToolCalls: withoutActiveCall(activeToolCalls, id),
480518
callIdByName: withoutCallIdByName(callIdByName, identity.name),
519+
callNameById: withoutCallNameById(callNameById, id),
481520
};
482521
}
483522

484523
return {
485524
activeToolCalls: withoutActiveCall(activeToolCalls, "tool"),
486525
callIdByName,
526+
callNameById,
487527
};
488528
}
489529

0 commit comments

Comments
 (0)