From ccf7d379b94b82b2122a9b15d8efcc0951c3b650 Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Mon, 9 Nov 2026 00:20:06 +0100 Subject: [PATCH 1/2] test: the resume tests wait for the run they resume, not a fixed 30 ms The driver starts a resumed run without awaiting it (a run can outlast a tool call). The tests waited 30 ms for it; on a loaded CI runner the resumed run started later, in the next test, where the fake process counted it as that test's first run and so did not fail it: "no failed run to resume", or one spawn too many or too few. Seen on #127 and #129, never on main by luck. - The fake run fails a plain run and passes a resumed one, decided by its arguments, not by a count every test shares. - A test that expects the resumed run waits until it is spawned and has ended (`resumedRun`), however long that takes. With a 100 ms delay put in front of every resumed run, the old file fails 3 of 9 and this one passes 9 of 9. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv --- .../test/resume-failed-run.test.ts | 39 +++++++++++++++---- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/packages/sidecar-toolkit/test/resume-failed-run.test.ts b/packages/sidecar-toolkit/test/resume-failed-run.test.ts index 2da9b48..4af601d 100644 --- a/packages/sidecar-toolkit/test/resume-failed-run.test.ts +++ b/packages/sidecar-toolkit/test/resume-failed-run.test.ts @@ -11,15 +11,22 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import * as vscode from 'vscode'; import { resetRegisteredCommands } from './vscode-mock'; -/** Each spawn: the first fails (leaving a record and a three-step trace), later ones succeed. */ +/** + * Each spawn: a plain run fails (leaving a record and a three-step trace), a + * resumed one succeeds. Decided by the arguments, not by a count: a count is + * shared by every test in the file, so a resumed run that started late counted + * as the next test's first run, which then did not fail. + */ const spawns: string[][] = []; +let openChildren = 0; vi.mock('../src/process-control.js', () => ({ spawnWorkflowProcess: (invocation: { args: string[] }) => { spawns.push(invocation.args); + openChildren++; const child: any = new EventEmitter(); child.stdout = new EventEmitter(); child.stderr = new EventEmitter(); - const fails = spawns.length === 1; + const fails = !invocation.args.includes('--resume-from'); if (fails) { const outDir = invocation.args[invocation.args.indexOf('--out-dir') + 1]; const runDir = path.join(outDir, 'r1'); @@ -37,7 +44,10 @@ vi.mock('../src/process-control.js', () => ({ steps: [1, 2, 3].map(step => ({ step, actorInstanceName: 'a', journalSeq: step })) })); } - setTimeout(() => child.emit('close', fails ? 1 : 0, null), 0); + setTimeout(() => { + child.emit('close', fails ? 1 : 0, null); + openChildren--; + }, 0); return child; }, requestWorkflowStop: () => true @@ -82,6 +92,7 @@ const originals: any = {}; beforeEach(() => { resetRegisteredCommands(); spawns.length = 0; + openChildren = 0; asked = []; originals.info = (vscode.window as any).showInformationMessage; originals.error = (vscode.window as any).showErrorMessage; @@ -106,6 +117,18 @@ async function failRun(): Promise { const settle = () => new Promise(resolve => setTimeout(resolve, 30)); +/** + * The resume starts its run without awaiting it (a run can outlast a tool + * call), so a test that expects the resumed run waits for it to be spawned and + * to end, however long a loaded machine takes: a fixed pause let it start in + * the next test. + */ +async function resumedRun(): Promise { + await vi.waitFor(() => expect(spawns).toHaveLength(2), { timeout: 5000 }); + await vi.waitFor(() => expect(openChildren).toBe(0), { timeout: 5000 }); + await settle(); +} + describe('resuming a failed run when the agent proposes it', () => { it('asks the person, then resumes from the last completed step', async () => { makeDriver(); @@ -113,7 +136,7 @@ describe('resuming a failed run when the agent proposes it', () => { answer = 'Resume'; const result = await vscode.commands.executeCommand(resumeFailedRunCommandId(RUN_CMD), { file }); - await settle(); + await resumedRun(); expect(asked.some(q => q.includes('Resume the run of top from where it failed (x (in m))'))).toBe(true); expect(result).toContain('resuming from where it failed'); @@ -141,7 +164,7 @@ describe('resuming a failed run when the agent proposes it', () => { answer = 'Resume'; await vscode.commands.executeCommand(resumeFailedRunCommandId(RUN_CMD), { file: '/w/layers/nested.py' }); - await settle(); + await resumedRun(); expect(spawns).toHaveLength(2); }); @@ -151,7 +174,7 @@ describe('resuming a failed run when the agent proposes it', () => { const file = await failRun(); answer = 'Resume'; await vscode.commands.executeCommand(resumeFailedRunCommandId(RUN_CMD), { file }); - await settle(); // the resumed run succeeds + await resumedRun(); // the resumed run succeeds const again = await vscode.commands.executeCommand(resumeFailedRunCommandId(RUN_CMD), { file }); expect(again).toContain('no failed run to resume'); @@ -175,7 +198,7 @@ describe('asked in the chat', () => { const file = await failRun(); const result = await vscode.commands.executeCommand(resumeFailedRunCommandId(RUN_CMD), { file }); - await settle(); + await resumedRun(); expect(confirms).toHaveLength(1); expect(confirms[0].sourceUri).toContain('top.py'); @@ -199,7 +222,7 @@ describe('asked in the chat', () => { const file = await failRun(); answer = 'Resume'; await vscode.commands.executeCommand(resumeFailedRunCommandId(RUN_CMD), { file }); - await settle(); + await resumedRun(); expect(asked.some(q => q.startsWith("The chat's fix"))).toBe(true); expect(spawns).toHaveLength(2); }); From 391743d7e17720fc0ce79233d3e5536f05538cab Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Mon, 9 Nov 2026 00:23:27 +0100 Subject: [PATCH 2/2] fix: what the review of the resume-test race turned up The child's `close` is not the end of a run: the driver still refreshes the diagram, forgets the failed run and says "Workflow run finished." after it, and that tail could still land in the next test. A test that expects the resumed run now waits for that last word, under the default test timeout so a slow machine fails by name. The temporary directories each run makes are removed after each test, and the "not now" test drops a pause that waited for nothing (the resume returns before anything is spawned). With 100 ms added before every resumed run and 150 ms after every child's close (not committed), main's file fails 2 of 9 and this one passes 9 of 9. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv --- .../test/resume-failed-run.test.ts | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/packages/sidecar-toolkit/test/resume-failed-run.test.ts b/packages/sidecar-toolkit/test/resume-failed-run.test.ts index 4af601d..2c5c690 100644 --- a/packages/sidecar-toolkit/test/resume-failed-run.test.ts +++ b/packages/sidecar-toolkit/test/resume-failed-run.test.ts @@ -18,11 +18,9 @@ import { resetRegisteredCommands } from './vscode-mock'; * as the next test's first run, which then did not fail. */ const spawns: string[][] = []; -let openChildren = 0; vi.mock('../src/process-control.js', () => ({ spawnWorkflowProcess: (invocation: { args: string[] }) => { spawns.push(invocation.args); - openChildren++; const child: any = new EventEmitter(); child.stdout = new EventEmitter(); child.stderr = new EventEmitter(); @@ -44,10 +42,7 @@ vi.mock('../src/process-control.js', () => ({ steps: [1, 2, 3].map(step => ({ step, actorInstanceName: 'a', journalSeq: step })) })); } - setTimeout(() => { - child.emit('close', fails ? 1 : 0, null); - openChildren--; - }, 0); + setTimeout(() => child.emit('close', fails ? 1 : 0, null), 0); return child; }, requestWorkflowStop: () => true @@ -92,7 +87,6 @@ const originals: any = {}; beforeEach(() => { resetRegisteredCommands(); spawns.length = 0; - openChildren = 0; asked = []; originals.info = (vscode.window as any).showInformationMessage; originals.error = (vscode.window as any).showErrorMessage; @@ -105,28 +99,33 @@ beforeEach(() => { afterEach(() => { (vscode.window as any).showInformationMessage = originals.info; (vscode.window as any).showErrorMessage = originals.error; + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } }); +const tempDirs: string[] = []; + async function failRun(): Promise { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'resume-failed-')); + tempDirs.push(dir); const file = path.join(dir, 'top.py'); fs.writeFileSync(file, '# top\n'); await vscode.commands.executeCommand(RUN_CMD, { sourceUri: `file://${file}`, workflowName: 'top' }); return file; } -const settle = () => new Promise(resolve => setTimeout(resolve, 30)); - /** * The resume starts its run without awaiting it (a run can outlast a tool - * call), so a test that expects the resumed run waits for it to be spawned and - * to end, however long a loaded machine takes: a fixed pause let it start in - * the next test. + * call), so a test that expects the resumed run waits for the run's own last + * word, "Workflow run finished.", said after the diagram refresh and after the + * failed run is forgotten. A fixed pause let the run end in the next test; the + * child's `close` is not the end either, the driver still has work after it. + * Under the default 5 s test timeout, so a slow machine fails here, by name. */ async function resumedRun(): Promise { - await vi.waitFor(() => expect(spawns).toHaveLength(2), { timeout: 5000 }); - await vi.waitFor(() => expect(openChildren).toBe(0), { timeout: 5000 }); - await settle(); + await vi.waitFor(() => expect(asked).toContain('Workflow run finished.'), { timeout: 4000 }); + await new Promise(resolve => setTimeout(resolve, 0)); } describe('resuming a failed run when the agent proposes it', () => { @@ -152,7 +151,6 @@ describe('resuming a failed run when the agent proposes it', () => { answer = 'Not now'; const result = await vscode.commands.executeCommand(resumeFailedRunCommandId(RUN_CMD), { file }); - await settle(); expect(result).toContain('chose not to resume'); expect(spawns).toHaveLength(1);