From 005ba47eaa2d588c00f378d7d1195acc96e5cf4b Mon Sep 17 00:00:00 2001 From: Aniruddha Adak Date: Sun, 4 Oct 2026 17:24:32 +0530 Subject: [PATCH 1/2] fix(e2e): cancel a case's interrupt once it has been accounted for The mid-stream interrupt was scheduled and never cancelled: let interruptTimer: NodeJS.Timeout | undefined; if (spec.interrupt && parentTs) { interruptTimer = setTimeout(() => { postAsUser(...) }, spec.interrupt.afterMs); } ... if (interruptTimer === undefined) { /* no-op */ } That trailing block is the tell: the handle was read only to keep the compiler quiet about an unused variable, and nothing ever cleared the timer. afterMs is longer than a lot of replies take, so a case whose bot answers first returns with the timer still armed and the runner moves on to the next case. It then fires into the finished case's thread: a message the runner never sent, a bot turn nobody asked for, and an "interrupt send failed" line appended to an errors array whose CaseResult was already returned and written to the report. The scheduling moves to e2e/interrupt.ts and hands back a cancel(), called as soon as the interrupt phase has read its result - the point after which the interrupt can no longer belong to this case. run.ts calls main() at import, so this is also what makes the behaviour testable at all; the new test drives the fake timers directly. The dead no-op goes with it. --- app/e2e-interrupt.test.ts | 59 +++++++++++++++++++++++++++++++++++++++ e2e/interrupt.ts | 31 ++++++++++++++++++++ e2e/run.ts | 31 +++++++++++--------- 3 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 app/e2e-interrupt.test.ts create mode 100644 e2e/interrupt.ts diff --git a/app/e2e-interrupt.test.ts b/app/e2e-interrupt.test.ts new file mode 100644 index 00000000..843a6a61 --- /dev/null +++ b/app/e2e-interrupt.test.ts @@ -0,0 +1,59 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { scheduleInterrupt } from "../e2e/interrupt.js"; + +describe("the mid-stream interrupt a case schedules", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it("is sent once the delay has passed", () => { + vi.useFakeTimers(); + const send = vi.fn(async () => {}); + + scheduleInterrupt(15_000, send, () => {}); + vi.advanceTimersByTime(15_000); + + expect(send).toHaveBeenCalledTimes(1); + }); + + it("is not sent at all once the case has cancelled it", () => { + /* + * A reply that arrives before afterMs leaves the timer armed, and the runner moves on to the + * next case. It then fires into a thread the runner is no longer watching: a message nobody + * sent, a bot turn nobody asked for, and a failure appended to a result already reported. + */ + vi.useFakeTimers(); + const send = vi.fn(async () => {}); + + const interrupt = scheduleInterrupt(15_000, send, () => {}); + interrupt.cancel(); + vi.advanceTimersByTime(120_000); + + expect(send).not.toHaveBeenCalled(); + }); + + it("reports a send that failed, and still only once", async () => { + const errors: string[] = []; + + scheduleInterrupt( + 1, + () => Promise.reject(new Error("thread_not_found")), + (message) => errors.push(message), + ); + await new Promise((resolve) => setTimeout(resolve, 25)); + + expect(errors).toEqual(["interrupt send failed: thread_not_found"]); + }); + + it("can be cancelled after it has already fired without throwing", async () => { + vi.useFakeTimers(); + const send = vi.fn(async () => {}); + + const interrupt = scheduleInterrupt(10, send, () => {}); + vi.advanceTimersByTime(10); + interrupt.cancel(); + interrupt.cancel(); + + expect(send).toHaveBeenCalledTimes(1); + }); +}); \ No newline at end of file diff --git a/e2e/interrupt.ts b/e2e/interrupt.ts new file mode 100644 index 00000000..8792fd87 --- /dev/null +++ b/e2e/interrupt.ts @@ -0,0 +1,31 @@ +/** + * The mid-stream interrupt an E2E case schedules: a second user message sent `afterMs` into the + * thread, so the bot is interrupted part-way through its first reply. + * + * It is handed back as something the caller has to cancel, because the delay is longer than a lot + * of replies take. A case whose bot answers first would otherwise leave the timer armed, and the + * runner would move on to the next case with it still ticking: the interrupt then arrives in a + * thread nobody is watching any more, starts a bot turn nobody asked for, and appends its own + * failure to a result that has already been reported. + */ +export type ScheduledInterrupt = { + /** Stop the interrupt being sent. Safe to call more than once, and after it has fired. */ + cancel(): void; +}; + +export function scheduleInterrupt( + afterMs: number, + send: () => Promise, + onError: (message: string) => void, +): ScheduledInterrupt { + const timer = setTimeout(() => { + send().catch((error: Error) => + onError(`interrupt send failed: ${error.message}`), + ); + }, afterMs); + return { + cancel() { + clearTimeout(timer); + }, + }; +} \ No newline at end of file diff --git a/e2e/run.ts b/e2e/run.ts index 70c21287..0dc920cc 100644 --- a/e2e/run.ts +++ b/e2e/run.ts @@ -15,6 +15,7 @@ import { mkdirSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { CASES } from "./cases.js"; import type { E2ECase } from "./cases.js"; +import { scheduleInterrupt } from "./interrupt.js"; import { postAsUser, watchForReply, @@ -109,16 +110,17 @@ async function runCase(spec: E2ECase): Promise { let followUpResult: CaseResult | undefined; // Schedule a mid-stream interrupt if the case asks for one. - let interruptTimer: NodeJS.Timeout | undefined; - if (spec.interrupt && parentTs) { - interruptTimer = setTimeout(() => { - postAsUser(TEST_CHANNEL, spec.interrupt!.prompt, { - threadTs: parentTs, - }).catch((e: Error) => - errors.push(`interrupt send failed: ${e.message}`), - ); - }, spec.interrupt.afterMs); - } + const interrupt = + spec.interrupt && parentTs + ? scheduleInterrupt( + spec.interrupt.afterMs, + () => + postAsUser(TEST_CHANNEL, spec.interrupt!.prompt, { + threadTs: parentTs, + }), + (message) => errors.push(message), + ) + : undefined; const onSample = (s: { elapsedMs: number; text: string | undefined }) => { const text = s.text ?? ""; @@ -134,9 +136,6 @@ async function runCase(spec: E2ECase): Promise { }); }; - if (interruptTimer === undefined) { - /* no-op */ - } const result = flatMode ? await watchForChannelReply({ channel: TEST_CHANNEL, @@ -250,6 +249,12 @@ async function runCase(spec: E2ECase): Promise { errors.push(...iErrors); } + // The interrupt has been accounted for, so stop it being sent. A reply that beat afterMs would + // otherwise leave the timer armed for the rest of the run, and it would fire into this thread + // after the case was done: a message nobody sent, a bot turn nobody asked for, and a failure + // appended to a result already written to the report. + interrupt?.cancel(); + // If there's a follow-up, send it as a thread reply (no @mention) into // the same thread the first prompt created. if (spec.followUp && parentTs && finalText) { From d1101d13e3c6035ee5731bd47d0ab05b987baa19 Mon Sep 17 00:00:00 2001 From: Maxim Date: Tue, 13 Oct 2026 23:40:50 +0200 Subject: [PATCH 2/2] fix(e2e): cancel the interrupt when a case throws The main loop catches a case that throws and moves on to the next one, so a failed Slack call between scheduling and cancelling left the interrupt timer armed for the rest of the run. runCase now cancels it in a finally. scheduleInterrupt's send also takes Promise, since postAsUser resolves to the Slack response. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gk4Q5dr4ovWEnCAsKB5drw --- e2e/interrupt.ts | 2 +- e2e/run.ts | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/e2e/interrupt.ts b/e2e/interrupt.ts index 8792fd87..c1294e89 100644 --- a/e2e/interrupt.ts +++ b/e2e/interrupt.ts @@ -15,7 +15,7 @@ export type ScheduledInterrupt = { export function scheduleInterrupt( afterMs: number, - send: () => Promise, + send: () => Promise, onError: (message: string) => void, ): ScheduledInterrupt { const timer = setTimeout(() => { diff --git a/e2e/run.ts b/e2e/run.ts index 0dc920cc..fade63e7 100644 --- a/e2e/run.ts +++ b/e2e/run.ts @@ -16,6 +16,7 @@ import { join } from "node:path"; import { CASES } from "./cases.js"; import type { E2ECase } from "./cases.js"; import { scheduleInterrupt } from "./interrupt.js"; +import type { ScheduledInterrupt } from "./interrupt.js"; import { postAsUser, watchForReply, @@ -87,7 +88,26 @@ function runExpectations( } } +/** + * Runs one case and cancels its interrupt however the case ends. The main loop catches a case + * that throws and moves on to the next one, so without this a failed Slack call between + * scheduling and cancelling would leave the timer armed for the rest of the run. + */ async function runCase(spec: E2ECase): Promise { + let interrupt: ScheduledInterrupt | undefined; + try { + return await runCaseBody(spec, (scheduled) => { + interrupt = scheduled; + }); + } finally { + interrupt?.cancel(); + } +} + +async function runCaseBody( + spec: E2ECase, + onScheduled: (interrupt: ScheduledInterrupt) => void, +): Promise { const errors: string[] = []; const samples: CaseResult["samples"] = []; const t0 = Date.now(); @@ -121,6 +141,7 @@ async function runCase(spec: E2ECase): Promise { (message) => errors.push(message), ) : undefined; + if (interrupt) onScheduled(interrupt); const onSample = (s: { elapsedMs: number; text: string | undefined }) => { const text = s.text ?? "";