Skip to content

Commit a0f0911

Browse files
fix(tui): quit-abort ordering, phantom folds, reset generation guard (#1130)
Abort before shutdown on quit; gate onFolded/telemetry on abort; generation counter for stale settles; reloadIfIdle reset guard. CL-8570, CL-8571.
1 parent 9ff0a32 commit a0f0911

7 files changed

Lines changed: 409 additions & 5 deletions

File tree

‎src/session/compaction-lifecycle.test.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,4 +361,52 @@ describe("createCompactionLifecycle", () => {
361361
expect(seen).toHaveLength(1);
362362
expect(seen[0]?.aborted).toBe(true);
363363
});
364+
365+
test("a stale settle cannot clear a newer compact's flag or emit its end", async () => {
366+
const ends: string[] = [];
367+
const lifecycle = createCompactionLifecycle({
368+
onCompactionStart: () => ends.push("start"),
369+
onCompactionEnd: (info) => ends.push(`end:${info.aborted}`),
370+
});
371+
let releaseStale!: (result: StrategyResult<ConversationTurn[]>) => void;
372+
let releaseCurrent!: (result: StrategyResult<ConversationTurn[]>) => void;
373+
let calls = 0;
374+
const inner: Compactor = {
375+
name: "inner",
376+
version: "0",
377+
apply: () => {
378+
calls += 1;
379+
if (calls === 1) {
380+
return new Promise<StrategyResult<ConversationTurn[]>>((resolve) => {
381+
releaseStale = resolve;
382+
});
383+
}
384+
return new Promise<StrategyResult<ConversationTurn[]>>((resolve) => {
385+
releaseCurrent = resolve;
386+
});
387+
},
388+
};
389+
const wrapped = lifecycle.wrapCompactor(inner);
390+
const input = turns(8);
391+
const stale = wrapped.apply(input, ctx);
392+
expect(lifecycle.isCompacting()).toBe(true);
393+
// A rebuild lands mid-compact: the replacement agent gets a fresh signal
394+
// while the old compact is still in flight.
395+
lifecycle.reset();
396+
expect(lifecycle.isCompacting()).toBe(false);
397+
const current = wrapped.apply(input, ctx);
398+
expect(lifecycle.isCompacting()).toBe(true);
399+
// The stale compact settles late: the flag stays up for the newer compact
400+
// and no end event fires for the already-reset generation.
401+
releaseStale(okResult(input));
402+
await stale;
403+
expect(lifecycle.isCompacting()).toBe(true);
404+
expect(ends).toEqual(["start", "start"]);
405+
// The newer compact still settles normally.
406+
releaseCurrent(okResult(input));
407+
const settled = await current;
408+
expect(settled.record.reason).toBe("folded");
409+
expect(lifecycle.isCompacting()).toBe(false);
410+
expect(ends).toEqual(["start", "start", "end:false"]);
411+
});
364412
});

‎src/session/compaction-lifecycle.ts‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,13 @@ export function createCompactionLifecycle(
8888
): CompactionLifecycle {
8989
let controller = new AbortController();
9090
let compacting = false;
91+
// Settle generation: every started compact and every reset mints a new
92+
// generation, and a settle only clears the flag / emits the end event when
93+
// its generation is still current. Without this, a stale settle (an aborted
94+
// compact whose loser promise resolves after abort + reset + a newer
95+
// compact started) would clear the newer compact's in-flight flag and emit
96+
// a duplicate end event for a compact that is already over.
97+
let generation = 0;
9198

9299
return {
93100
getSignal: () => controller.signal,
@@ -98,6 +105,7 @@ export function createCompactionLifecycle(
98105
reset: (): void => {
99106
controller = new AbortController();
100107
compacting = false;
108+
generation += 1;
101109
},
102110
wrapCompactor: (inner: Compactor): Compactor => ({
103111
name: inner.name,
@@ -108,6 +116,8 @@ export function createCompactionLifecycle(
108116
// compact): skip the inner run entirely, without lifecycle events —
109117
// the interrupting path already told the operator what happened.
110118
if (signal.aborted) return abortedResult(inner, turns);
119+
generation += 1;
120+
const settledGeneration = generation;
111121
compacting = true;
112122
events.onCompactionStart?.();
113123
let aborted = false;
@@ -139,8 +149,13 @@ export function createCompactionLifecycle(
139149
signal.removeEventListener("abort", onAbort);
140150
}
141151
} finally {
142-
compacting = false;
143-
events.onCompactionEnd?.({ aborted });
152+
// A stale settle (abort + reset + a newer compact started while
153+
// this one was still in flight) must not clear the newer compact's
154+
// flag or emit an end event for a compact that is already over.
155+
if (settledGeneration === generation) {
156+
compacting = false;
157+
events.onCompactionEnd?.({ aborted });
158+
}
144159
}
145160
},
146161
}),

‎src/session/runtime-assembly.test.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import {
3131
} from "./runtime-assembly.js";
3232
import type { SubAgentSourcesConfig } from "./runtime-assembly.js";
3333
import type { Settings } from "../config/settings.js";
34+
import type { Telemetry } from "../telemetry/index.js";
3435
import { generateSessionId, initSessionDir, sessionDir } from "./index.js";
3536
import type { PluginModule } from "../plugins/loader.js";
3637

@@ -522,6 +523,53 @@ describe("createSessionPruningCompactor", () => {
522523
await noop.apply(few as never, { state: {} as never, trigger: "test" });
523524
expect(silent).toEqual([]);
524525
});
526+
527+
test("a discarded fold emits no telemetry and no onFolded", async () => {
528+
const captured: { event: string }[] = [];
529+
const folds: { turnsBefore: number; turnsAfter: number }[] = [];
530+
const telemetry: Telemetry = {
531+
enabled: true,
532+
installationId: "test",
533+
capture: (event) => {
534+
captured.push({ event });
535+
},
536+
captureIntentional: () => false,
537+
flush: async () => undefined,
538+
discard: () => undefined,
539+
};
540+
// Bound to the lifecycle signal in production: true once the outer abort
541+
// race has discarded (or will discard) this run's output.
542+
let aborted = false;
543+
const compactor = createSessionPruningCompactor({
544+
summarize: async () => "summary",
545+
telemetry,
546+
onFolded: (info) => folds.push(info),
547+
isAborted: () => aborted,
548+
});
549+
const now = Date.now();
550+
const many = Array.from({ length: 8 }, (_, i) => ({
551+
role: i % 2 === 0 ? "user" : "assistant",
552+
content: [{ type: "text", text: `t${i}` }],
553+
timestamp: now,
554+
}));
555+
// A live fold still reports…
556+
const folded = await compactor.apply(many as never, {
557+
state: {} as never,
558+
trigger: "test",
559+
});
560+
expect(folds).toHaveLength(1);
561+
expect(captured.map((entry) => entry.event)).toEqual(["compaction"]);
562+
// …but a fold the lifecycle discarded reports nothing, while the output
563+
// itself still passes through untouched (fold semantics unchanged).
564+
aborted = true;
565+
const discarded = await compactor.apply(many as never, {
566+
state: {} as never,
567+
trigger: "test",
568+
});
569+
expect(discarded.output).toEqual(folded.output);
570+
expect(folds).toHaveLength(1);
571+
expect(captured.map((entry) => entry.event)).toEqual(["compaction"]);
572+
});
525573
});
526574

527575
describe("buildCompactionContinuationMessage", () => {

‎src/session/runtime-assembly.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ import {
5757
} from "./compactor.js";
5858
import type { SummaryContext } from "./summarizer.js";
5959
import { NOOP_TELEMETRY, type Telemetry } from "../telemetry/index.js";
60+
import { COMPACTION_ABORTED_REASON } from "./compaction-lifecycle.js";
6061

6162
// ---------------------------------------------------------------------------
6263
// 1. Sub-agent provider literal
@@ -380,6 +381,13 @@ export interface SessionPruningCompactorArgs {
380381
telemetry?: Telemetry;
381382
/** Fires only when turns were actually folded away — not on no-ops. */
382383
onFolded?: (info: { turnsBefore: number; turnsAfter: number }) => void;
384+
/**
385+
* True when the lifecycle has discarded (or will discard) the in-flight
386+
* compact — e.g. bound to the session compaction lifecycle's signal. A
387+
* fold the outer abort race threw away must report nothing: no telemetry,
388+
* no onFolded side effects for work that never landed.
389+
*/
390+
isAborted?: () => boolean;
383391
}
384392

385393
/** Shared pruning-compactor defaults for the main session agent. */
@@ -399,6 +407,16 @@ export function createSessionPruningCompactor(
399407
const turnsBefore = turns.length;
400408
const startedAt = Date.now();
401409
const result = await compactor.apply(turns, ctx);
410+
// A discarded compact reports nothing. When the lifecycle abort wins
411+
// the outer race, this inner run may still complete with a genuine
412+
// fold — but the reactor threw that output away, so emitting telemetry
413+
// or onFolded would describe work that never landed (phantom fold).
414+
if (
415+
args.isAborted?.() === true ||
416+
result.record.reason === COMPACTION_ABORTED_REASON
417+
) {
418+
return result;
419+
}
402420
// summarizedTurnCount is only set on the branch that actually folded
403421
// turns away. The other branch is a no-op (or image aging alone), and
404422
// reporting it as compaction would drag the duration and turn-count

0 commit comments

Comments
 (0)