Skip to content

Commit edab293

Browse files
committed
r51 batch: three review fixes across history locking and branch summary
- historyService: every chat.jsonl mutation (appends, updateHistory, deletes, truncations, boundary persistence, migration) now runs under the cross-process history write lock via withRecoveredHistoryWriteResultLock; reads stay lock-free (atomic-rename replacement) - historyService: append paths refresh the cached sequence counter from durable history under the lock so foreign backends' rows can never receive duplicate sequences (delete/truncate keep their own post-mutation counter recompute) - branchSummary: the deadline drain (reader.cancel + consume) is bounded by BRANCH_SUMMARY_CANCEL_DRAIN_MS so a provider wedged in its cancel path cannot hold edit-resend or workspace removal
1 parent c311c6c commit edab293

5 files changed

Lines changed: 188 additions & 55 deletions

File tree

src/constants/branchSummary.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,15 @@ export const BRANCH_SUMMARY_MAX_OUTPUT_TOKENS = 512;
4242
*/
4343
export const BRANCH_SUMMARY_TIMEOUT_MS = 6_000;
4444

45+
/**
46+
* Bounded cleanup window for draining a deadline-cancelled summary stream
47+
* (reader.cancel + consumer settlement). Cancellation normally settles in
48+
* milliseconds; a provider wedged in its own cancel path must not hold the
49+
* synchronous edit-resend wait or workspace removal past the deadline the
50+
* drain exists to serve — after this window the consumer is detached.
51+
*/
52+
export const BRANCH_SUMMARY_CANCEL_DRAIN_MS = 2_000;
53+
4554
/**
4655
* Hard cap on characters accumulated from the summary stream. Purely
4756
* defensive: BRANCH_SUMMARY_MAX_OUTPUT_TOKENS already bounds well-behaved

src/node/services/branchSummary.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -800,6 +800,39 @@ describe("maybeAppendAbandonedBranchSummary", () => {
800800
}
801801
});
802802

803+
test("a provider wedged in its cancel path cannot hold the deadline drain (r51)", async () => {
804+
const { historyService, cleanup } = await createTestHistoryService();
805+
try {
806+
// Never produces chunks AND never settles its cancel: the deadline
807+
// drain (reader.cancel + consume) must be bounded, or the synchronous
808+
// edit-resend wait blocks indefinitely on exactly the wedged provider
809+
// the deadline exists to cap.
810+
const wedgedCancel = new MockLanguageModelV3({
811+
doStream: () =>
812+
Promise.resolve({
813+
stream: new ReadableStream<LanguageModelV3StreamPart>({
814+
pull: () => new Promise<never>(() => undefined),
815+
cancel: () => new Promise<never>(() => undefined),
816+
}),
817+
}),
818+
});
819+
const startedAt = Date.now();
820+
const appended = await maybeAppendAbandonedBranchSummary({
821+
historyService,
822+
aiService: fakeAiService(wedgedCancel),
823+
workspaceId: "ws-wedged-cancel",
824+
abandonedMessages: meatyExchange("wedged-cancel"),
825+
experiments: RLM_ON,
826+
timeoutMs: 100,
827+
});
828+
expect(appended).toBeNull();
829+
// Bounded: deadline + drain window, well under the suite cap.
830+
expect(Date.now() - startedAt).toBeLessThan(5_000);
831+
} finally {
832+
await cleanup();
833+
}
834+
});
835+
803836
test("wedged model creation is cut off by the shared deadline (r50)", async () => {
804837
const { historyService, cleanup } = await createTestHistoryService();
805838
try {

src/node/services/branchSummary.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import { getErrorMessage } from "@/common/utils/errors";
2929
import { estimateMuxMessageTokens } from "@/common/utils/messages/keepRecentTail";
3030
import { acquireProcessFileLock } from "@/node/utils/concurrency/fileLock";
3131
import {
32+
BRANCH_SUMMARY_CANCEL_DRAIN_MS,
3233
BRANCH_SUMMARY_MAX_ACCUMULATED_CHARS,
3334
BRANCH_SUMMARY_MAX_OUTPUT_TOKENS,
3435
BRANCH_SUMMARY_MAX_TRANSCRIPT_CHARS,
@@ -471,14 +472,25 @@ async function generateAbandonedBranchSummaryText(input: {
471472
// Actively cancel the losing consumer: a wedged provider leaves it
472473
// pinned in read() (the loop's aborted check only runs when a delta
473474
// arrives), and cancel resolves that pending read so the reader is
474-
// released promptly. AWAITED, then the consume task drained (r50,
475-
// mirroring the refine runner's deadline path): returning while
475+
// released promptly. Drained before cleanup (r50): returning while
476476
// cancellation is still in flight would run the finally's
477477
// runLanguageModelCleanup underneath a provider whose asynchronous
478478
// stream teardown had not settled, keeping network/runtime resources
479-
// alive past workspace removal.
480-
await reader.cancel().catch(() => undefined);
481-
await consume;
479+
// alive past workspace removal. The drain itself is BOUNDED (r51):
480+
// a provider wedged in its own cancel path would otherwise hold the
481+
// synchronous edit-resend wait or workspace removal indefinitely —
482+
// exactly the wedged-provider case the deadline exists to cap. After
483+
// the window the consumer is detached; nothing observable depends on
484+
// it (the salvage snapshot below is taken from `accumulated`, and
485+
// the raced-away task can only settle into an abandoned stream).
486+
const drained = (async () => {
487+
await reader.cancel().catch(() => undefined);
488+
await consume;
489+
})();
490+
await Promise.race([
491+
drained,
492+
new Promise<void>((resolve) => setTimeout(resolve, BRANCH_SUMMARY_CANCEL_DRAIN_MS)),
493+
]);
482494
// Deadline hit. Salvage whole sentences already streamed — a missed
483495
// deadline should still buy a (shorter) summary when tokens flowed.
484496
const salvaged = trimSummaryToBoundary(accumulated);

src/node/services/historyService.test.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ describe("HistoryService", () => {
384384
// replacement — built from contents read before the foreign append —
385385
// would silently delete the foreign row.
386386
const foreign = await acquireProcessFileLock({
387-
lockPath: path.join(config.getSessionDir(workspaceId), "history-append.lock"),
387+
lockPath: path.join(config.getSessionDir(workspaceId), "history.lock"),
388388
timeoutMs: 5_000,
389389
label: "test foreign backend",
390390
});
@@ -406,6 +406,39 @@ describe("HistoryService", () => {
406406
const messages = await collectFullHistory(service, workspaceId);
407407
expect(messages.map((m) => m.id)).toEqual(["msg1", "payload-1", "trigger-1"]);
408408
});
409+
410+
it("advances the sequence counter past foreign rows under the write lock (r51)", async () => {
411+
const workspaceId = "workspace1";
412+
// Cache a counter in this instance (msg1 takes sequence 0, counter -> 1).
413+
const seeded = await service.appendToHistory(
414+
workspaceId,
415+
createMuxMessage("msg1", "user", "Hello")
416+
);
417+
expect(seeded.success).toBe(true);
418+
// A foreign backend (XUM_ALLOW_MULTIPLE_INSTANCES=1) appends a row with
419+
// a higher sequence from its own counter.
420+
const foreignLine = messageLine(
421+
workspaceId,
422+
createMuxMessage("foreign-1", "assistant", "foreign row", { historySequence: 7 })
423+
);
424+
await fs.appendFile(
425+
path.join(config.getSessionDir(workspaceId), "chat.jsonl"),
426+
foreignLine + "\n"
427+
);
428+
// Without the in-lock counter refresh this batch would assign stale
429+
// sequences from the cached counter; updateHistory replaces the FIRST
430+
// row matching a sequence, so a duplicate would let a later stream
431+
// finalization overwrite an unrelated foreign row.
432+
const result = await service.appendManyToHistory(workspaceId, [
433+
createMuxMessage("payload-1", "assistant", "family payload"),
434+
createMuxMessage("trigger-1", "user", "family trigger"),
435+
]);
436+
expect(result.success).toBe(true);
437+
const messages = await collectFullHistory(service, workspaceId);
438+
const seqById = new Map(messages.map((m) => [m.id, m.metadata?.historySequence]));
439+
expect(seqById.get("payload-1")).toBe(8);
440+
expect(seqById.get("trigger-1")).toBe(9);
441+
});
409442
});
410443

411444
describe("updateHistory", () => {

0 commit comments

Comments
 (0)