From 3b43d67b4265f68fbdf1c64359fdeb2c98d10c8d Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 13:11:04 -0400 Subject: [PATCH 01/12] fix(runner): wire --timeout into web flow expect timeout --timeout set the Playwright context default action timeout but not the @qawolf/flows expect-wrapper timeout, which stayed pinned at its hardcoded 30s default. Assertion failures (the common case) ignored the flag. Thread the timeout through initFlowRuntime into configureFlowRuntime as webExpectAttributes.defaultExpectTimeoutMs, and reword the flag help to reflect its per-action/assertion semantics. Non-web flows remain unwired (WIZ-10830). Fixes WIZ-10821 --- src/commands/__snapshots__/help.test.ts.snap | 3 +- src/commands/flows/run.register.ts | 2 +- src/domains/runner/initFlowRuntime.test.ts | 49 ++++++++++++++++---- src/domains/runner/initFlowRuntime.ts | 27 +++++++++-- src/domains/runner/runWebFlow.test.ts | 20 ++++++++ src/domains/runner/runWebFlow.ts | 2 +- 6 files changed, 88 insertions(+), 15 deletions(-) diff --git a/src/commands/__snapshots__/help.test.ts.snap b/src/commands/__snapshots__/help.test.ts.snap index 982ee71ed..56ec53f95 100644 --- a/src/commands/__snapshots__/help.test.ts.snap +++ b/src/commands/__snapshots__/help.test.ts.snap @@ -137,7 +137,8 @@ Options: 0) --bail Stop the run after the first failure (default: false) --workers Parallel worker count for web flows (default: 1) - --timeout Per-flow timeout in milliseconds (default: 30000) + --timeout Default timeout for actions and assertions, in + milliseconds (default: 30000) --video Record video: on | off | retain-on-failure (default: "off") --trace Record Playwright trace: on | off | retain-on-failure diff --git a/src/commands/flows/run.register.ts b/src/commands/flows/run.register.ts index b8ef19675..efdb67e51 100644 --- a/src/commands/flows/run.register.ts +++ b/src/commands/flows/run.register.ts @@ -56,7 +56,7 @@ export function registerFlowsRunCommand( ) .option( "--timeout ", - "Per-flow timeout in milliseconds", + "Default timeout for actions and assertions, in milliseconds", parseInteger("--timeout", { min: 0 }), 30_000, ) diff --git a/src/domains/runner/initFlowRuntime.test.ts b/src/domains/runner/initFlowRuntime.test.ts index cb1ab3198..74bb3b3e1 100644 --- a/src/domains/runner/initFlowRuntime.test.ts +++ b/src/domains/runner/initFlowRuntime.test.ts @@ -10,20 +10,47 @@ afterEach(() => { const thisDir = import.meta.dirname; +/** Reads back the expect timeout the runner configured on @qawolf/flows. */ +async function readConfiguredExpectTimeout(): Promise { + const idxUrl = import.meta.resolve("@qawolf/flows"); + const attrsUrl = new URL("./web/expect/attributes.js", idxUrl).href; + const { getWebExpectAttributes } = (await import(attrsUrl)) as { + getWebExpectAttributes: () => { defaultExpectTimeoutMs: number }; + }; + return getWebExpectAttributes().defaultExpectTimeoutMs; +} + describe("initFlowRuntime", () => { it("resolves using the CLI's own @qawolf/flows", async () => { - await initFlowRuntime(path.join(thisDir, "fake.flow.ts")); + await initFlowRuntime(path.join(thisDir, "fake.flow.ts"), { + timeout: 30_000, + }); + }); + + it("configures the @qawolf/flows expect timeout from the passed timeout", async () => { + await initFlowRuntime(path.join(thisDir, "fake.flow.ts"), { + timeout: 5_000, + }); + expect(await readConfiguredExpectTimeout()).toBe(5_000); }); it("returns the same promise for repeated calls from the same directory", () => { - const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts")); - const p2 = initFlowRuntime(path.join(thisDir, "b.flow.ts")); + const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts"), { + timeout: 30_000, + }); + const p2 = initFlowRuntime(path.join(thisDir, "b.flow.ts"), { + timeout: 30_000, + }); expect(p1).toBe(p2); }); it("returns a different promise for a different starting directory", () => { - const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts")); - const p2 = initFlowRuntime(path.join(thisDir, "sub", "b.flow.ts")); + const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts"), { + timeout: 30_000, + }); + const p2 = initFlowRuntime(path.join(thisDir, "sub", "b.flow.ts"), { + timeout: 30_000, + }); expect(p1).not.toBe(p2); // settle both so they don't leak into subsequent tests return Promise.allSettled([p1, p2]); @@ -34,7 +61,9 @@ describe("initFlowRuntime", () => { try { let caught: unknown; try { - await initFlowRuntime(path.join(tmp, "my.flow.ts")); + await initFlowRuntime(path.join(tmp, "my.flow.ts"), { + timeout: 30_000, + }); } catch (e) { caught = e; } @@ -60,7 +89,9 @@ describe("initFlowRuntime", () => { ); let caught: unknown; try { - await initFlowRuntime(path.join(tmp, "my.flow.ts")); + await initFlowRuntime(path.join(tmp, "my.flow.ts"), { + timeout: 30_000, + }); } catch (e) { caught = e; } @@ -82,7 +113,9 @@ describe("initFlowRuntime", () => { await mkdir(path.join(pkgDir, "package.json")); let caught: unknown; try { - await initFlowRuntime(path.join(tmp, "my.flow.ts")); + await initFlowRuntime(path.join(tmp, "my.flow.ts"), { + timeout: 30_000, + }); } catch (e) { caught = e; } diff --git a/src/domains/runner/initFlowRuntime.ts b/src/domains/runner/initFlowRuntime.ts index e433005e2..b5af90a06 100644 --- a/src/domains/runner/initFlowRuntime.ts +++ b/src/domains/runner/initFlowRuntime.ts @@ -6,9 +6,20 @@ import { isNoEntError } from "~/core/errors.js"; type ConfigureFlowRuntime = (opts: { target: unknown; - webExpectAttributes?: unknown; + webExpectAttributes?: { defaultExpectTimeoutMs: number }; }) => Promise; +export type InitFlowRuntimeOptions = { + /** + * Default timeout (ms) for flow actions and assertions. Threaded into + * @qawolf/flows as `defaultExpectTimeoutMs` so the package's `expect` + * wrapper honors `--timeout`; without it the wrapper pins every assertion + * to its hardcoded 30s default. The matching Playwright action timeout is + * applied separately via `context.setDefaultTimeout` at launch. + */ + timeout: number; +}; + async function findFlowsRunnerPath(flowPath: string, fs: Fs): Promise { let dir = path.dirname(flowPath); while (true) { @@ -47,7 +58,11 @@ async function findFlowsRunnerPath(flowPath: string, fs: Fs): Promise { const initCache = new Map>(); -async function doInit(flowPath: string, fs: Fs): Promise { +async function doInit( + flowPath: string, + timeout: number, + fs: Fs, +): Promise { const runnerPath = await findFlowsRunnerPath(flowPath, fs); const mod = (await import(pathToFileURL(runnerPath).href)) as { configureFlowRuntime?: ConfigureFlowRuntime; @@ -63,18 +78,22 @@ async function doInit(flowPath: string, fs: Fs): Promise { runnerName: "node20WithPlaywright", meta: "legacy", }, + webExpectAttributes: { defaultExpectTimeoutMs: timeout }, }); } export function initFlowRuntime( flowPath: string, + options: InitFlowRuntimeOptions, fs: Fs = makeDefaultFs(), ): Promise { const startDir = path.dirname(flowPath); - // Cache key is startDir, not fs — tests reusing the same startDir must call _resetInitCache() between runs. + // Cache key is startDir, not fs — tests reusing the same startDir must call + // _resetInitCache() between runs. Timeout is omitted deliberately: it is a + // single run-global flag, so every flow in a process shares one value. let p = initCache.get(startDir); if (!p) { - p = doInit(flowPath, fs); + p = doInit(flowPath, options.timeout, fs); initCache.set(startDir, p); } return p; diff --git a/src/domains/runner/runWebFlow.test.ts b/src/domains/runner/runWebFlow.test.ts index c01d367a1..675c4b8ac 100644 --- a/src/domains/runner/runWebFlow.test.ts +++ b/src/domains/runner/runWebFlow.test.ts @@ -17,7 +17,27 @@ afterEach(() => { mock.restore(); }); +/** Reads back the expect timeout configured on @qawolf/flows. */ +async function readConfiguredExpectTimeout(): Promise { + const idxUrl = import.meta.resolve("@qawolf/flows"); + const attrsUrl = new URL("./web/expect/attributes.js", idxUrl).href; + const { getWebExpectAttributes } = (await import(attrsUrl)) as { + getWebExpectAttributes: () => { defaultExpectTimeoutMs: number }; + }; + return getWebExpectAttributes().defaultExpectTimeoutMs; +} + describe("runWebFlow", () => { + it("should configure the @qawolf/flows expect timeout from options.timeout", async () => { + await runWebFlow({ + deps: makeWebDeps(), + options: { ...baseOptions, timeout: 7_777 }, + flowPath: fixturePath("pass"), + }); + + expect(await readConfiguredExpectTimeout()).toBe(7_777); + }); + it("should return passed: true when the flow succeeds", async () => { const result = await runWebFlow({ deps: makeWebDeps(), diff --git a/src/domains/runner/runWebFlow.ts b/src/domains/runner/runWebFlow.ts index d04f68d60..880d47a5b 100644 --- a/src/domains/runner/runWebFlow.ts +++ b/src/domains/runner/runWebFlow.ts @@ -40,7 +40,7 @@ export async function runWebFlow({ options: RunWebFlowOptions; flowPath: string; }): Promise { - await initFlowRuntime(flowPath); + await initFlowRuntime(flowPath, { timeout: options.timeout }); const exported = await loadFlowDefault(flowPath); From c4cbfb20d0e158a89318aedff3725224f7667a14 Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 14:13:19 -0400 Subject: [PATCH 02/12] test(runner): reset init cache in expect-timeout readback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The readback test observes a process-global on @qawolf/flows and initFlowRuntime memoizes per flow directory. Sibling runner tests share that directory, so a prior cache entry could skip the reconfigure and leave a stale timeout — failing only under cross-file test ordering in CI. Reset the cache before the assertion to force a fresh configure. --- src/domains/runner/runWebFlow.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/domains/runner/runWebFlow.test.ts b/src/domains/runner/runWebFlow.test.ts index 675c4b8ac..7b9629fa1 100644 --- a/src/domains/runner/runWebFlow.test.ts +++ b/src/domains/runner/runWebFlow.test.ts @@ -7,6 +7,7 @@ import { makeUniformDeps, } from "./web/createWebLaunchContext.fixtures.js"; import { runWebFlow } from "./runWebFlow.js"; +import { _resetInitCache } from "./initFlowRuntime.js"; import { baseOptions, fixturePath, @@ -29,6 +30,11 @@ async function readConfiguredExpectTimeout(): Promise { describe("runWebFlow", () => { it("should configure the @qawolf/flows expect timeout from options.timeout", async () => { + // initFlowRuntime memoizes per flow directory and configures a + // process-global on @qawolf/flows. Other runner tests share this + // directory, so clear the cache to force a fresh configure here. + _resetInitCache(); + await runWebFlow({ deps: makeWebDeps(), options: { ...baseOptions, timeout: 7_777 }, From 5eb8fc1aec9a6b2af94c2e8d8da4134fed24f876 Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 14:35:11 -0400 Subject: [PATCH 03/12] test(runner): reset init cache in afterEach for isolation Clears the @qawolf/flows expect-timeout config the readback test sets, so it cannot leak into later tests in this file (CodeRabbit nitpick). --- src/domains/runner/runWebFlow.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/domains/runner/runWebFlow.test.ts b/src/domains/runner/runWebFlow.test.ts index 7b9629fa1..fe56de1c4 100644 --- a/src/domains/runner/runWebFlow.test.ts +++ b/src/domains/runner/runWebFlow.test.ts @@ -15,6 +15,9 @@ import { } from "./runWebFlow.fixtures.js"; afterEach(() => { + // The expect-timeout readback test configures a process-global on + // @qawolf/flows; clear the init cache so it does not leak into later tests. + _resetInitCache(); mock.restore(); }); From 8ee727aae623806b298c4229027ae136b718c5e4 Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 14:26:02 -0400 Subject: [PATCH 04/12] fix(flows): default run output to gitignored .qawolf/output --- src/commands/__snapshots__/help.test.ts.snap | 2 +- src/commands/flows/run.register.ts | 3 ++- src/core/paths.test.ts | 13 +++++++++++++ src/core/paths.ts | 13 +++++++++++++ src/domains/config/loadConfig.test.ts | 2 +- src/domains/config/schema.ts | 4 +++- src/domains/init/templates.ts | 2 +- 7 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 src/core/paths.test.ts diff --git a/src/commands/__snapshots__/help.test.ts.snap b/src/commands/__snapshots__/help.test.ts.snap index 56ec53f95..1726eb3a2 100644 --- a/src/commands/__snapshots__/help.test.ts.snap +++ b/src/commands/__snapshots__/help.test.ts.snap @@ -148,7 +148,7 @@ Options: --har-content HAR response bodies: omit | full (full uses more memory) (default: "omit") --output-dir Directory for run artifacts (videos, traces, HAR) - (default: "qawolf-output") + (default: ".qawolf/output") --junit [path] Write a JUnit XML report (default: /junit-report.xml) --headed Show the browser window instead of headless (default: diff --git a/src/commands/flows/run.register.ts b/src/commands/flows/run.register.ts index efdb67e51..462c94510 100644 --- a/src/commands/flows/run.register.ts +++ b/src/commands/flows/run.register.ts @@ -9,6 +9,7 @@ import type { TraceMode, VideoMode, } from "~/core/types.js"; +import { defaultOutputDir } from "~/core/paths.js"; import { parseEnum, parseInteger } from "~/domains/runner/runFlagParsers.js"; import type { FlowsRunFlags } from "~/domains/runner/runInternals.js"; @@ -87,7 +88,7 @@ export function registerFlowsRunCommand( .option( "--output-dir ", "Directory for run artifacts (videos, traces, HAR)", - "qawolf-output", + defaultOutputDir, ) .option( "--junit [path]", diff --git a/src/core/paths.test.ts b/src/core/paths.test.ts new file mode 100644 index 000000000..7fd18ed33 --- /dev/null +++ b/src/core/paths.test.ts @@ -0,0 +1,13 @@ +import { describe, expect, it } from "bun:test"; + +import { defaultOutputDir, qawolfDir } from "./paths.js"; + +describe("defaultOutputDir", () => { + it("is nested inside the qawolf directory that init gitignores", () => { + expect(defaultOutputDir.startsWith(`${qawolfDir}/`)).toBe(true); + }); + + it("is a relative path so artifacts land in the project, not an absolute location", () => { + expect(defaultOutputDir.startsWith("/")).toBe(false); + }); +}); diff --git a/src/core/paths.ts b/src/core/paths.ts index bc07e987d..2eb0d697f 100644 --- a/src/core/paths.ts +++ b/src/core/paths.ts @@ -1,5 +1,18 @@ import envPaths from "env-paths"; +/** + * Directory `qawolf init` writes a catch-all `.gitignore` into, so anything + * nested under it is excluded from version control by default. + */ +export const qawolfDir = ".qawolf"; + +/** + * Default directory for `flows run` artifacts (videos, traces, HAR). Nested + * under {@link qawolfDir} so the `.gitignore` written by `qawolf init` keeps + * run artifacts out of version control. + */ +export const defaultOutputDir = `${qawolfDir}/output`; + let _paths: ReturnType | undefined; export function getConfigDir(): string { diff --git a/src/domains/config/loadConfig.test.ts b/src/domains/config/loadConfig.test.ts index 4cb6217dd..555bf81d5 100644 --- a/src/domains/config/loadConfig.test.ts +++ b/src/domains/config/loadConfig.test.ts @@ -30,7 +30,7 @@ describe("loadConfig", () => { it("returns defaults when qawolf.config.ts is missing", async () => { const config = await loadConfig(missingConfig()); expect(config).toEqual({ - outputDir: ".qawolf", + outputDir: ".qawolf/output", timeout: 60_000, retries: 0, bail: false, diff --git a/src/domains/config/schema.ts b/src/domains/config/schema.ts index 549139c8e..c9d20ab0c 100644 --- a/src/domains/config/schema.ts +++ b/src/domains/config/schema.ts @@ -1,9 +1,11 @@ import { z } from "zod"; +import { defaultOutputDir } from "~/core/paths.js"; + const artifactModeSchema = z.enum(["on", "off", "retain-on-failure"]); export const qawolfConfigSchema = z.strictObject({ - outputDir: z.string().default(".qawolf"), + outputDir: z.string().default(defaultOutputDir), timeout: z.number().int().positive().default(60_000), retries: z.number().int().min(0).default(0), bail: z.boolean().default(false), diff --git a/src/domains/init/templates.ts b/src/domains/init/templates.ts index 78cf6c342..853b36b54 100644 --- a/src/domains/init/templates.ts +++ b/src/domains/init/templates.ts @@ -1,5 +1,5 @@ export const qawolfConfigTs = `export default { - outputDir: ".qawolf", + outputDir: ".qawolf/output", timeout: 60_000, retries: 0, video: "retain-on-failure", From 10e50880d74cd44249ac367e266b14c21bb7d43c Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 14:36:31 -0400 Subject: [PATCH 05/12] refactor(init): interpolate defaultOutputDir in config template --- src/domains/init/templates.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/domains/init/templates.ts b/src/domains/init/templates.ts index 853b36b54..90f919a3d 100644 --- a/src/domains/init/templates.ts +++ b/src/domains/init/templates.ts @@ -1,5 +1,7 @@ +import { defaultOutputDir } from "~/core/paths.js"; + export const qawolfConfigTs = `export default { - outputDir: ".qawolf/output", + outputDir: "${defaultOutputDir}", timeout: 60_000, retries: 0, video: "retain-on-failure", From 99e891cfc199504568d4bd9f37bb1a8bb8c6e99b Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 14:49:46 -0400 Subject: [PATCH 06/12] docs(changeset): add changeset for gitignored default output dir --- .changeset/gitignored-default-output-dir.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/gitignored-default-output-dir.md diff --git a/.changeset/gitignored-default-output-dir.md b/.changeset/gitignored-default-output-dir.md new file mode 100644 index 000000000..b6f607b2f --- /dev/null +++ b/.changeset/gitignored-default-output-dir.md @@ -0,0 +1,5 @@ +--- +"@qawolf/cli": patch +--- + +Change the default `flows run` output directory from `qawolf-output` to `.qawolf/output`, so run artifacts (videos, traces, HAR) land under the `.qawolf/` directory that `qawolf init` gitignores. Pass `--output-dir` to override. From 51e8fc3cbbb75fc0ea69d24159869a89164323da Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 14:50:33 -0400 Subject: [PATCH 07/12] chore(changeset): add patch changeset for --timeout web expect fix --- .changeset/timeout-web-flow-expect.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/timeout-web-flow-expect.md diff --git a/.changeset/timeout-web-flow-expect.md b/.changeset/timeout-web-flow-expect.md new file mode 100644 index 000000000..09db3a44b --- /dev/null +++ b/.changeset/timeout-web-flow-expect.md @@ -0,0 +1,5 @@ +--- +"@qawolf/cli": patch +--- + +Wire `flows run --timeout` into the web flow expect timeout. Previously the flag only set Playwright's per-action timeout, so assertion failures still waited the hardcoded 30s default; it now applies to actions and assertions alike. From 80713da735dcbb5b5b3ff4eccf90c03cd807cec8 Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 15:02:10 -0400 Subject: [PATCH 08/12] fix(runner): save HAR and trace artifacts reliably Close contexts fully before browsers in createLaunch cleanup. Playwright flushes HAR/video/trace during context.close(), and the previous concurrent browser.close() could terminate the connection mid-flush, silently dropping artifacts (reproduced as flaky HAR loss). Wire trace recording through the runner: the parsed --trace flag was dropped in buildRunOptions, omitted from RunWebFlowOptions, and tracing.start/stop was never called. Add traceFlowPath/initTrace/ maybeCleanupTrace symmetric to HAR, start tracing after context creation, and stop to the trace path during cleanup. Respect on/off/retain-on-failure. Fixes WIZ-10823 --- .../runner/dispatchViaSubprocess.test.ts | 1 + src/domains/runner/executeWorkerFlow.test.ts | 1 + src/domains/runner/run.manifestStamp.test.ts | 3 + src/domains/runner/runHelpers.test.ts | 5 + src/domains/runner/runHelpers.ts | 1 + src/domains/runner/runWebFlow.fixtures.ts | 1 + src/domains/runner/runWebFlow.trace.test.ts | 101 +++++++++++++++ src/domains/runner/runWebFlow.ts | 11 +- src/domains/runner/runWebFlowUtils.test.ts | 117 ++++++++++++++++++ src/domains/runner/runWebFlowUtils.ts | 31 +++-- .../runner/web/contextSetup.trace.test.ts | 97 +++++++++++++++ src/domains/runner/web/contextSetup.ts | 44 ++++++- 12 files changed, 403 insertions(+), 10 deletions(-) create mode 100644 src/domains/runner/runWebFlow.trace.test.ts create mode 100644 src/domains/runner/runWebFlowUtils.test.ts create mode 100644 src/domains/runner/web/contextSetup.trace.test.ts diff --git a/src/domains/runner/dispatchViaSubprocess.test.ts b/src/domains/runner/dispatchViaSubprocess.test.ts index 13eef15a5..87f83678a 100644 --- a/src/domains/runner/dispatchViaSubprocess.test.ts +++ b/src/domains/runner/dispatchViaSubprocess.test.ts @@ -19,6 +19,7 @@ const webOptions: RunWebFlowOptions = { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: 30_000, har: "off", harContent: "omit", diff --git a/src/domains/runner/executeWorkerFlow.test.ts b/src/domains/runner/executeWorkerFlow.test.ts index b1bac616b..e5179b24d 100644 --- a/src/domains/runner/executeWorkerFlow.test.ts +++ b/src/domains/runner/executeWorkerFlow.test.ts @@ -25,6 +25,7 @@ function makeInput(): WorkerInput { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: 30_000, har: "off", harContent: "omit", diff --git a/src/domains/runner/run.manifestStamp.test.ts b/src/domains/runner/run.manifestStamp.test.ts index e9b84cecb..e9dd697d6 100644 --- a/src/domains/runner/run.manifestStamp.test.ts +++ b/src/domains/runner/run.manifestStamp.test.ts @@ -62,6 +62,7 @@ describe("dispatchFlow manifest stamping", () => { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: defaultFlags().timeout, }, androidOptions: { @@ -104,6 +105,7 @@ describe("dispatchFlow manifest stamping", () => { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: defaultFlags().timeout, }, androidOptions: { @@ -142,6 +144,7 @@ describe("dispatchFlow manifest stamping", () => { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: defaultFlags().timeout, }, androidOptions: { diff --git a/src/domains/runner/runHelpers.test.ts b/src/domains/runner/runHelpers.test.ts index 81c337f58..2c46e1817 100644 --- a/src/domains/runner/runHelpers.test.ts +++ b/src/domains/runner/runHelpers.test.ts @@ -16,4 +16,9 @@ describe("buildRunOptions", () => { const { webOptions } = buildRunOptions({ ...defaultFlags(), headed: true }); expect(webOptions.headed).toBe(true); }); + + it("passes the trace mode through to webOptions", () => { + const { webOptions } = buildRunOptions({ ...defaultFlags(), trace: "on" }); + expect(webOptions.trace).toBe("on"); + }); }); diff --git a/src/domains/runner/runHelpers.ts b/src/domains/runner/runHelpers.ts index 67d1fe342..feac509cb 100644 --- a/src/domains/runner/runHelpers.ts +++ b/src/domains/runner/runHelpers.ts @@ -31,6 +31,7 @@ export function buildRunOptions(flags: FlowsRunFlags): { headed: flags.headed, slowMo: 0, video: flags.video, + trace: flags.trace, timeout: flags.timeout, har: flags.har, harContent: flags.harContent, diff --git a/src/domains/runner/runWebFlow.fixtures.ts b/src/domains/runner/runWebFlow.fixtures.ts index ca750d401..a7468b358 100644 --- a/src/domains/runner/runWebFlow.fixtures.ts +++ b/src/domains/runner/runWebFlow.fixtures.ts @@ -43,6 +43,7 @@ export const baseOptions: RunWebFlowOptions = { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: 30_000, }; diff --git a/src/domains/runner/runWebFlow.trace.test.ts b/src/domains/runner/runWebFlow.trace.test.ts new file mode 100644 index 000000000..8ce5d765f --- /dev/null +++ b/src/domains/runner/runWebFlow.trace.test.ts @@ -0,0 +1,101 @@ +import { afterEach, describe, expect, it, mock } from "bun:test"; +import { + makeBrowser, + makeContext, + makeDep, + makePage, + makeUniformDeps, +} from "./web/createWebLaunchContext.fixtures.js"; +import { runWebFlow } from "./runWebFlow.js"; +import { + baseOptions, + fixturePath, + makeWebDeps, +} from "./runWebFlow.fixtures.js"; + +afterEach(() => { + mock.restore(); +}); + +describe("runWebFlow trace", () => { + it("should not start tracing when trace is off", async () => { + const ctx = makeContext([makePage()]); + const startMock = mock(async () => {}); + ctx.tracing = { start: startMock, stop: async () => {} }; + const deps = makeWebDeps(makeUniformDeps(makeDep(makeBrowser(ctx), ctx))); + + await runWebFlow({ + deps, + options: baseOptions, + flowPath: fixturePath("launch"), + }); + + expect(startMock).not.toHaveBeenCalled(); + }); + + it("should start and stop tracing to the trace path when trace is on", async () => { + const ctx = makeContext([makePage()]); + const startMock = mock(async () => {}); + const stopMock = mock(async (_opts: { path?: string }) => {}); + ctx.tracing = { start: startMock, stop: stopMock }; + const deps = makeWebDeps(makeUniformDeps(makeDep(makeBrowser(ctx), ctx))); + + await runWebFlow({ + deps, + options: { ...baseOptions, trace: "on", outputDir: "/tmp/test-trace" }, + flowPath: fixturePath("launch"), + }); + + expect(startMock).toHaveBeenCalledTimes(1); + const stopArg = stopMock.mock.calls[0]![0]; + expect(stopArg.path).toContain("/trace/launch.zip"); + }); + + it("should delete the trace file on success when trace is retain-on-failure", async () => { + const unlinkMock = mock(async (_p: string) => {}); + const deps = { + ...makeWebDeps(), + fs: { + mkdir: async () => {}, + writeFile: async () => {}, + unlink: unlinkMock, + }, + }; + + await runWebFlow({ + deps, + options: { + ...baseOptions, + trace: "retain-on-failure", + outputDir: "/tmp/test-trace", + }, + flowPath: fixturePath("pass"), + }); + + expect(unlinkMock.mock.calls[0]![0]).toContain("/trace/"); + }); + + it("should not delete the trace file on failure when trace is retain-on-failure", async () => { + const unlinkMock = mock(async (_p: string) => {}); + const deps = { + ...makeWebDeps(), + fs: { + mkdir: async () => {}, + writeFile: async () => {}, + unlink: unlinkMock, + }, + }; + + await runWebFlow({ + deps, + options: { + ...baseOptions, + trace: "retain-on-failure", + outputDir: "/tmp/test-trace", + }, + flowPath: fixturePath("fail"), + }); + + expect(unlinkMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/domains/runner/runWebFlow.ts b/src/domains/runner/runWebFlow.ts index 880d47a5b..bf3ddc5cf 100644 --- a/src/domains/runner/runWebFlow.ts +++ b/src/domains/runner/runWebFlow.ts @@ -23,13 +23,14 @@ import { import { buildContextSetup, initHar, + initTrace, maybeCleanupHar, + maybeCleanupTrace, } from "./web/contextSetup.js"; export type RunWebFlowDeps = RunnerDeps & WebLaunchDeps; -// trace is not yet implemented export type RunWebFlowOptions = RunnerOptions & - Omit; + Omit; export async function runWebFlow({ deps, @@ -53,6 +54,7 @@ export async function runWebFlow({ : (exported as WebFlowDefinition).run; const { harMode, harPath } = await initHar(deps.fs, options, flowName); + const { traceMode, tracePath } = await initTrace(deps.fs, options, flowName); const videoSize = { width: 1280, height: 720 }; const contextSetup = buildContextSetup(videoSize, options, harPath); @@ -74,6 +76,8 @@ export async function runWebFlow({ launchBrowserOpts, signals: deps.signals, timeout: options.timeout, + traceMode, + tracePath, }); const flowDef: FlowDefinition = { @@ -109,5 +113,8 @@ export async function runWebFlow({ if (harPath !== undefined) { await maybeCleanupHar(deps.fs, harPath, passed, harMode); } + if (tracePath !== undefined) { + await maybeCleanupTrace(deps.fs, tracePath, passed, traceMode); + } } } diff --git a/src/domains/runner/runWebFlowUtils.test.ts b/src/domains/runner/runWebFlowUtils.test.ts new file mode 100644 index 000000000..71d29c7ee --- /dev/null +++ b/src/domains/runner/runWebFlowUtils.test.ts @@ -0,0 +1,117 @@ +import { afterEach, describe, expect, it, mock } from "bun:test"; +import { createLaunch } from "./runWebFlowUtils.js"; +import { + makeBrowser, + makeContext, + makeDep, +} from "./web/createWebLaunchContext.fixtures.js"; +import { makeNoopSignals } from "~/shell/signals/createSignalRegistry.fixtures.js"; + +afterEach(() => { + mock.restore(); +}); + +const launchBrowserOpts = { headless: true, slowMo: 0 }; + +describe("createLaunch cleanup", () => { + it("should close browsers only after contexts have fully closed", async () => { + const order: string[] = []; + const ctx = makeContext(); + ctx.close = mock(async () => { + order.push("context:start"); + await Promise.resolve(); + order.push("context:end"); + }); + const browser = makeBrowser(ctx); + browser.close = mock(async () => { + order.push("browser:start"); + }); + const dep = makeDep(browser, ctx); + + const { launch, cleanup } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + }); + + await launch(); + await cleanup(); + + expect(order).toEqual(["context:start", "context:end", "browser:start"]); + }); +}); + +describe("createLaunch tracing", () => { + it("should start tracing after creating the context when trace is on", async () => { + const ctx = makeContext(); + const startMock = mock(async () => {}); + ctx.tracing = { start: startMock, stop: async () => {} }; + const dep = makeDep(makeBrowser(ctx), ctx); + + const { launch } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + traceMode: "on", + tracePath: "/out/trace/flow.zip", + }); + + await launch(); + + expect(startMock).toHaveBeenCalledTimes(1); + }); + + it("should not start tracing when trace is off", async () => { + const ctx = makeContext(); + const startMock = mock(async () => {}); + ctx.tracing = { start: startMock, stop: async () => {} }; + const dep = makeDep(makeBrowser(ctx), ctx); + + const { launch } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + traceMode: "off", + tracePath: undefined, + }); + + await launch(); + + expect(startMock).not.toHaveBeenCalled(); + }); + + it("should stop tracing to the trace path before closing the context", async () => { + const order: string[] = []; + const ctx = makeContext(); + const stopMock = mock(async (_opts: { path?: string }) => { + order.push("trace:stop"); + }); + ctx.tracing = { start: async () => {}, stop: stopMock }; + ctx.close = mock(async () => { + order.push("context:close"); + }); + const dep = makeDep(makeBrowser(ctx), ctx); + + const { launch, cleanup } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + traceMode: "on", + tracePath: "/out/trace/flow.zip", + }); + + await launch(); + await cleanup(); + + expect(stopMock).toHaveBeenCalledWith({ path: "/out/trace/flow.zip" }); + expect(order).toEqual(["trace:stop", "context:close"]); + }); +}); diff --git a/src/domains/runner/runWebFlowUtils.ts b/src/domains/runner/runWebFlowUtils.ts index c13713035..e1010dfe4 100644 --- a/src/domains/runner/runWebFlowUtils.ts +++ b/src/domains/runner/runWebFlowUtils.ts @@ -1,6 +1,7 @@ import crypto from "node:crypto"; import os from "node:os"; import path from "node:path"; +import type { TraceMode } from "~/core/types.js"; import type { SignalRegistry } from "./types.js"; import { runnerMessages } from "~/core/messages/index.js"; import { unsupportedSharedDepNames } from "./unsupportedDepNames.js"; @@ -32,20 +33,28 @@ export function createLaunch({ launchBrowserOpts, signals, timeout, + traceMode = "off", + tracePath, }: { browsers: { chromium: BrowserDep; firefox: BrowserDep; webkit: BrowserDep }; contextSetup: ContextSetupOptions; launchBrowserOpts: BrowserLaunchOptions; signals: SignalRegistry; timeout: number; + traceMode?: TraceMode; + tracePath?: string | undefined; }): { launch: LaunchFn; cleanup: () => Promise } { const openBrowsers: MinimalBrowser[] = []; const openContexts: MinimalBrowserContext[] = []; const unregisters: (() => void)[] = []; + const tracingEnabled = traceMode !== "off"; - const trackContext = (ctx: MinimalBrowserContext) => { + const trackContext = async (ctx: MinimalBrowserContext) => { openContexts.push(ctx); unregisters.push(signals.register(() => ctx.close())); + if (tracingEnabled) { + await ctx.tracing.start({ screenshots: true, snapshots: true }); + } }; const launch: LaunchFn = async (launchOpts) => { @@ -61,7 +70,7 @@ export function createLaunch({ ...contextSetup, }); context.setDefaultTimeout(timeout); - trackContext(context); + await trackContext(context); const page = await context.newPage(); return { browserType: browserName, context, page }; } @@ -71,16 +80,24 @@ export function createLaunch({ unregisters.push(signals.register(() => browser.close())); const context = await browser.newContext(contextSetup); context.setDefaultTimeout(timeout); - trackContext(context); + await trackContext(context); return { browser, browserType: browserName, context }; }; const cleanup = async () => { for (const unreg of unregisters) unreg(); - await Promise.allSettled([ - ...openContexts.map((c) => c.close()), - ...openBrowsers.map((b) => b.close()), - ]); + // Stop tracing before closing the context — Playwright writes the trace + // zip on stop(), and stop() is not valid once the context has closed. + if (tracingEnabled && tracePath !== undefined) { + await Promise.allSettled( + openContexts.map((c) => c.tracing.stop({ path: tracePath })), + ); + } + // Close contexts fully before browsers: Playwright flushes HAR/video/trace + // during context.close(), and a concurrent browser.close() can terminate + // the connection mid-flush, silently dropping those artifacts. + await Promise.allSettled(openContexts.map((c) => c.close())); + await Promise.allSettled(openBrowsers.map((b) => b.close())); }; return { launch, cleanup }; diff --git a/src/domains/runner/web/contextSetup.trace.test.ts b/src/domains/runner/web/contextSetup.trace.test.ts new file mode 100644 index 000000000..4fffdc6fd --- /dev/null +++ b/src/domains/runner/web/contextSetup.trace.test.ts @@ -0,0 +1,97 @@ +import { afterEach, describe, expect, it, mock } from "bun:test"; +import { initTrace, maybeCleanupTrace, traceFlowPath } from "./contextSetup.js"; + +afterEach(() => { + mock.restore(); +}); + +describe("traceFlowPath", () => { + it("should return /trace/.zip", () => { + expect(traceFlowPath("/out", "my-flow")).toBe("/out/trace/my-flow.zip"); + }); +}); + +describe("initTrace", () => { + it("should return traceMode off and tracePath undefined when trace is off", async () => { + const mkdirMock = mock(async () => {}); + const fs = { mkdir: mkdirMock }; + const result = await initTrace(fs, { outputDir: "/out" }, "flow"); + expect(result.traceMode).toBe("off"); + expect(result.tracePath).toBeUndefined(); + expect(mkdirMock).not.toHaveBeenCalled(); + }); + + it("should return traceMode and tracePath and call mkdir when trace is on", async () => { + const mkdirMock = mock(async () => {}); + const fs = { mkdir: mkdirMock }; + const result = await initTrace( + fs, + { trace: "on", outputDir: "/out" }, + "my-flow", + ); + expect(result.traceMode).toBe("on"); + expect(result.tracePath).toBe("/out/trace/my-flow.zip"); + expect(mkdirMock).toHaveBeenCalledWith("/out/trace", { recursive: true }); + }); + + it("should return traceMode and tracePath and call mkdir when trace is retain-on-failure", async () => { + const mkdirMock = mock(async () => {}); + const fs = { mkdir: mkdirMock }; + const result = await initTrace( + fs, + { trace: "retain-on-failure", outputDir: "/out" }, + "my-flow", + ); + expect(result.traceMode).toBe("retain-on-failure"); + expect(result.tracePath).toBe("/out/trace/my-flow.zip"); + expect(mkdirMock).toHaveBeenCalledWith("/out/trace", { recursive: true }); + }); +}); + +describe("maybeCleanupTrace", () => { + it("should call unlink when traceMode is retain-on-failure and flow passed", async () => { + const unlinkMock = mock(async () => {}); + const fs = { unlink: unlinkMock }; + await maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + true, + "retain-on-failure", + ); + expect(unlinkMock).toHaveBeenCalledWith("/out/trace/flow.zip"); + }); + + it("should not call unlink when traceMode is retain-on-failure and flow failed", async () => { + const unlinkMock = mock(async () => {}); + const fs = { unlink: unlinkMock }; + await maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + false, + "retain-on-failure", + ); + expect(unlinkMock).not.toHaveBeenCalled(); + }); + + it("should not call unlink when traceMode is on", async () => { + const unlinkMock = mock(async () => {}); + const fs = { unlink: unlinkMock }; + await maybeCleanupTrace(fs, "/out/trace/flow.zip", true, "on"); + expect(unlinkMock).not.toHaveBeenCalled(); + }); + + it("should not throw when unlink rejects", async () => { + const fs = { + unlink: mock(async () => { + throw new Error("ENOENT"); + }), + }; + const result = maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + true, + "retain-on-failure", + ); + expect(result).resolves.toBeUndefined(); + }); +}); diff --git a/src/domains/runner/web/contextSetup.ts b/src/domains/runner/web/contextSetup.ts index 5c438995d..51efc71de 100644 --- a/src/domains/runner/web/contextSetup.ts +++ b/src/domains/runner/web/contextSetup.ts @@ -1,5 +1,10 @@ import path from "node:path"; -import type { HarContent, HarMode, VideoMode } from "~/core/types.js"; +import type { + HarContent, + HarMode, + TraceMode, + VideoMode, +} from "~/core/types.js"; import type { ContextSetupOptions } from "./types.js"; /** Returns the path where Playwright writes the HAR file for a single flow run. */ @@ -7,6 +12,11 @@ export function harFlowPath(outputDir: string, flowName: string): string { return path.join(outputDir, "har", `${flowName}.har`); } +/** Returns the path where Playwright writes the trace zip for a single flow run. */ +export function traceFlowPath(outputDir: string, flowName: string): string { + return path.join(outputDir, "trace", `${flowName}.zip`); +} + /** * Returns the Playwright `recordHar` context option. * Always sets `mode: "minimal"` (captures URL, status, headers, timing; skips @@ -46,6 +56,38 @@ export async function initHar( return { harMode, harPath }; } +/** Resolves traceMode and tracePath, creating the output directory when needed. */ +export async function initTrace( + fs: { mkdir: (p: string, opts?: { recursive?: boolean }) => Promise }, + options: { trace?: TraceMode; outputDir: string }, + flowName: string, +): Promise<{ traceMode: TraceMode; tracePath: string | undefined }> { + const traceMode = options.trace ?? "off"; + if (traceMode === "off") return { traceMode, tracePath: undefined }; + const tracePath = traceFlowPath(options.outputDir, flowName); + await fs.mkdir(path.dirname(tracePath), { recursive: true }); + return { traceMode, tracePath }; +} + +/** + * Deletes the trace file when `traceMode` is `"retain-on-failure"` and the flow + * passed. No-ops for `"on"` and `"off"` modes. + * Errors from `unlink` are swallowed — cleanup failure must not fail a passing flow. + */ +export async function maybeCleanupTrace( + fs: { unlink: (p: string) => Promise }, + tracePath: string, + passed: boolean, + traceMode: TraceMode, +): Promise { + if (traceMode !== "retain-on-failure" || !passed) return; + try { + await fs.unlink(tracePath); + } catch { + // best-effort; do not surface cleanup errors + } +} + /** Builds the Playwright context setup options for video and HAR recording. */ export function buildContextSetup( videoSize: { width: number; height: number }, From 962192c6f3c2851d610c7ac25b86c1301a878711 Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 15:09:24 -0400 Subject: [PATCH 09/12] docs(runner): clarify trace cleanup semantics and known limits Correct the cleanup comment: context.close() flushes HAR/video, while the trace zip is written by the preceding tracing.stop(). Note that a SIGINT-interrupted run preserves HAR/video but not the trace, since the signal handlers only close the context. Add a TODO WIZ-10839 for the multi-launch last-wins artifact path: multiple launch() calls in one flow stop every context to the same trace path, so only one zip survives (mirrors existing HAR behavior). Surfaced during adversarial review of WIZ-10823. --- src/domains/runner/runWebFlowUtils.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/domains/runner/runWebFlowUtils.ts b/src/domains/runner/runWebFlowUtils.ts index e1010dfe4..f4f2d5bca 100644 --- a/src/domains/runner/runWebFlowUtils.ts +++ b/src/domains/runner/runWebFlowUtils.ts @@ -88,14 +88,19 @@ export function createLaunch({ for (const unreg of unregisters) unreg(); // Stop tracing before closing the context — Playwright writes the trace // zip on stop(), and stop() is not valid once the context has closed. + // TODO WIZ-10839: a flow with multiple launch() calls stops every context + // to the same tracePath, so only one trace zip survives (mirrors HAR). if (tracingEnabled && tracePath !== undefined) { await Promise.allSettled( openContexts.map((c) => c.tracing.stop({ path: tracePath })), ); } - // Close contexts fully before browsers: Playwright flushes HAR/video/trace + // Close contexts fully before browsers: Playwright flushes HAR and video // during context.close(), and a concurrent browser.close() can terminate - // the connection mid-flush, silently dropping those artifacts. + // the connection mid-flush, silently dropping those artifacts. (Trace is + // already written by the tracing.stop() above.) Note: the signal handlers + // registered above only close the context, so a SIGINT-interrupted run + // preserves HAR/video but not the trace. await Promise.allSettled(openContexts.map((c) => c.close())); await Promise.allSettled(openBrowsers.map((b) => b.close())); }; From f58d9cdd660d340a3d5581c8b49b80cc3bb8a758 Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 16:10:05 -0400 Subject: [PATCH 10/12] docs(runner): add changeset for HAR/trace artifact fix --- .changeset/har-trace-artifacts.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/har-trace-artifacts.md diff --git a/.changeset/har-trace-artifacts.md b/.changeset/har-trace-artifacts.md new file mode 100644 index 000000000..4ee6c7930 --- /dev/null +++ b/.changeset/har-trace-artifacts.md @@ -0,0 +1,5 @@ +--- +"@qawolf/cli": patch +--- + +Save HAR and trace artifacts reliably from `flows run`. HAR (and video) could be silently dropped because contexts and browsers were closed concurrently, racing Playwright's artifact flush; contexts now close first. The `--trace` flag is now wired end-to-end and writes a Playwright trace to `/trace/.zip`, honoring `on`, `off`, and `retain-on-failure`. From 5ac70d6d377a4d4ed9573ea9c4b9ae1253f1e7a3 Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 16:23:33 -0400 Subject: [PATCH 11/12] fix(runner): preserve artifact flush order on signal shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createLaunch registered context.close() and browser.close() as separate signal cleanups, but SignalRegistry runs cleanups concurrently — so a SIGINT could race browser.close() against the context's HAR/video flush, the same way the old concurrent cleanup did. Register a single ordered teardown (stop tracing, close contexts, then browsers) used by both normal cleanup and signal shutdown, guarded against double-run. Surfaced by CodeRabbit on PR #178. --- src/domains/runner/runWebFlowUtils.test.ts | 30 ++++++++++++++ src/domains/runner/runWebFlowUtils.ts | 48 ++++++++++++---------- 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/domains/runner/runWebFlowUtils.test.ts b/src/domains/runner/runWebFlowUtils.test.ts index 71d29c7ee..f119b33fc 100644 --- a/src/domains/runner/runWebFlowUtils.test.ts +++ b/src/domains/runner/runWebFlowUtils.test.ts @@ -6,6 +6,7 @@ import { makeDep, } from "./web/createWebLaunchContext.fixtures.js"; import { makeNoopSignals } from "~/shell/signals/createSignalRegistry.fixtures.js"; +import { createSignalRegistry } from "~/shell/signals/createSignalRegistry.js"; afterEach(() => { mock.restore(); @@ -41,6 +42,35 @@ describe("createLaunch cleanup", () => { expect(order).toEqual(["context:start", "context:end", "browser:start"]); }); + + it("should close contexts before browsers on signal-driven shutdown", async () => { + const order: string[] = []; + const ctx = makeContext(); + ctx.close = mock(async () => { + order.push("context:start"); + await Promise.resolve(); + order.push("context:end"); + }); + const browser = makeBrowser(ctx); + browser.close = mock(async () => { + order.push("browser:start"); + }); + const dep = makeDep(browser, ctx); + const signals = createSignalRegistry(); + + const { launch } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals, + timeout: 30_000, + }); + + await launch(); + await signals.shutdown("SIGINT"); + + expect(order).toEqual(["context:start", "context:end", "browser:start"]); + }); }); describe("createLaunch tracing", () => { diff --git a/src/domains/runner/runWebFlowUtils.ts b/src/domains/runner/runWebFlowUtils.ts index f4f2d5bca..07424b809 100644 --- a/src/domains/runner/runWebFlowUtils.ts +++ b/src/domains/runner/runWebFlowUtils.ts @@ -46,12 +46,35 @@ export function createLaunch({ }): { launch: LaunchFn; cleanup: () => Promise } { const openBrowsers: MinimalBrowser[] = []; const openContexts: MinimalBrowserContext[] = []; - const unregisters: (() => void)[] = []; const tracingEnabled = traceMode !== "off"; + let closed = false; + + // Ordered teardown shared by normal cleanup and signal-driven shutdown: + // stop tracing, then close contexts, then close browsers. The order matters + // because Playwright flushes HAR/video during context.close() and writes the + // trace on tracing.stop(); a browser.close() racing either can terminate the + // connection mid-flush and silently drop the artifact. + // TODO WIZ-10839: a flow with multiple launch() calls stops every context to + // the same tracePath, so only one trace zip survives (mirrors HAR). + const closeAll = async () => { + if (closed) return; + closed = true; + if (tracingEnabled && tracePath !== undefined) { + await Promise.allSettled( + openContexts.map((c) => c.tracing.stop({ path: tracePath })), + ); + } + await Promise.allSettled(openContexts.map((c) => c.close())); + await Promise.allSettled(openBrowsers.map((b) => b.close())); + }; + + // Register the ordered teardown once. SignalRegistry runs cleanups + // concurrently, so registering context and browser closes separately would + // let browser.close() race the context flush on a SIGINT-interrupted run. + const unregister = signals.register(closeAll); const trackContext = async (ctx: MinimalBrowserContext) => { openContexts.push(ctx); - unregisters.push(signals.register(() => ctx.close())); if (tracingEnabled) { await ctx.tracing.start({ screenshots: true, snapshots: true }); } @@ -77,7 +100,6 @@ export function createLaunch({ const browser = await bt.launch(launchBrowserOpts); openBrowsers.push(browser); - unregisters.push(signals.register(() => browser.close())); const context = await browser.newContext(contextSetup); context.setDefaultTimeout(timeout); await trackContext(context); @@ -85,24 +107,8 @@ export function createLaunch({ }; const cleanup = async () => { - for (const unreg of unregisters) unreg(); - // Stop tracing before closing the context — Playwright writes the trace - // zip on stop(), and stop() is not valid once the context has closed. - // TODO WIZ-10839: a flow with multiple launch() calls stops every context - // to the same tracePath, so only one trace zip survives (mirrors HAR). - if (tracingEnabled && tracePath !== undefined) { - await Promise.allSettled( - openContexts.map((c) => c.tracing.stop({ path: tracePath })), - ); - } - // Close contexts fully before browsers: Playwright flushes HAR and video - // during context.close(), and a concurrent browser.close() can terminate - // the connection mid-flush, silently dropping those artifacts. (Trace is - // already written by the tracing.stop() above.) Note: the signal handlers - // registered above only close the context, so a SIGINT-interrupted run - // preserves HAR/video but not the trace. - await Promise.allSettled(openContexts.map((c) => c.close())); - await Promise.allSettled(openBrowsers.map((b) => b.close())); + unregister(); + await closeAll(); }; return { launch, cleanup }; From 70ac1e34cc340b6cb830a445c156a1d566f1ba3b Mon Sep 17 00:00:00 2001 From: Simon Ingeson Date: Mon, 15 Jun 2026 16:33:40 -0400 Subject: [PATCH 12/12] feat(runner): warn when retain-on-failure artifact cleanup fails When a passing flow's HAR/trace file cannot be deleted under retain-on-failure, the error was swallowed silently, leaving a stale artifact the user never heard about. Log a warning with the path via the runner logger so it can be removed manually. Cleanup failure still never fails a passing flow. Addresses review feedback from @michael-pr on PR #178. --- src/core/messages/runner.ts | 4 ++++ src/domains/runner/runWebFlow.ts | 5 +++-- src/domains/runner/web/contextSetup.test.ts | 18 ++++++++++++++++++ .../runner/web/contextSetup.trace.test.ts | 18 ++++++++++++++++++ src/domains/runner/web/contextSetup.ts | 18 ++++++++++++------ 5 files changed, 55 insertions(+), 8 deletions(-) diff --git a/src/core/messages/runner.ts b/src/core/messages/runner.ts index ac292a7f3..875f3f00e 100644 --- a/src/core/messages/runner.ts +++ b/src/core/messages/runner.ts @@ -16,6 +16,10 @@ export const runnerMessages = { `retries must be a non-negative integer, got ${String(retries)}`, manifestStampReadFailed: (file: string, message: string) => `failed to read manifest stamp for ${file}: ${message}`, + harCleanupFailed: (file: string, message: string) => + `failed to delete HAR file ${file} (retain-on-failure); remove it manually: ${message}`, + traceCleanupFailed: (file: string, message: string) => + `failed to delete trace file ${file} (retain-on-failure); remove it manually: ${message}`, notSupportedInCli: (name: string) => `${name} is not supported in the CLI runner`, notAvailableLocally: (name: string) => diff --git a/src/domains/runner/runWebFlow.ts b/src/domains/runner/runWebFlow.ts index bf3ddc5cf..6c851a06a 100644 --- a/src/domains/runner/runWebFlow.ts +++ b/src/domains/runner/runWebFlow.ts @@ -110,11 +110,12 @@ export async function runWebFlow({ return result; } finally { await cleanup(); + const warn = (message: string) => deps.logger?.warn(message); if (harPath !== undefined) { - await maybeCleanupHar(deps.fs, harPath, passed, harMode); + await maybeCleanupHar(deps.fs, harPath, passed, harMode, warn); } if (tracePath !== undefined) { - await maybeCleanupTrace(deps.fs, tracePath, passed, traceMode); + await maybeCleanupTrace(deps.fs, tracePath, passed, traceMode, warn); } } } diff --git a/src/domains/runner/web/contextSetup.test.ts b/src/domains/runner/web/contextSetup.test.ts index b57a22119..f5c2cac86 100644 --- a/src/domains/runner/web/contextSetup.test.ts +++ b/src/domains/runner/web/contextSetup.test.ts @@ -82,6 +82,24 @@ describe("maybeCleanupHar", () => { ); expect(result).resolves.toBeUndefined(); }); + + it("should warn with the HAR path when unlink rejects", async () => { + const warn = mock((_msg: string) => {}); + const fs = { + unlink: mock(async () => { + throw new Error("EPERM"); + }), + }; + await maybeCleanupHar( + fs, + "/out/har/flow.har", + true, + "retain-on-failure", + warn, + ); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]![0]).toContain("/out/har/flow.har"); + }); }); describe("initHar", () => { diff --git a/src/domains/runner/web/contextSetup.trace.test.ts b/src/domains/runner/web/contextSetup.trace.test.ts index 4fffdc6fd..c7a856eb1 100644 --- a/src/domains/runner/web/contextSetup.trace.test.ts +++ b/src/domains/runner/web/contextSetup.trace.test.ts @@ -94,4 +94,22 @@ describe("maybeCleanupTrace", () => { ); expect(result).resolves.toBeUndefined(); }); + + it("should warn with the trace path when unlink rejects", async () => { + const warn = mock((_msg: string) => {}); + const fs = { + unlink: mock(async () => { + throw new Error("EPERM"); + }), + }; + await maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + true, + "retain-on-failure", + warn, + ); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]![0]).toContain("/out/trace/flow.zip"); + }); }); diff --git a/src/domains/runner/web/contextSetup.ts b/src/domains/runner/web/contextSetup.ts index 51efc71de..2d33c3ac5 100644 --- a/src/domains/runner/web/contextSetup.ts +++ b/src/domains/runner/web/contextSetup.ts @@ -1,4 +1,6 @@ import path from "node:path"; +import { errorMessage } from "~/core/errors.js"; +import { runnerMessages } from "~/core/messages/index.js"; import type { HarContent, HarMode, @@ -72,19 +74,21 @@ export async function initTrace( /** * Deletes the trace file when `traceMode` is `"retain-on-failure"` and the flow * passed. No-ops for `"on"` and `"off"` modes. - * Errors from `unlink` are swallowed — cleanup failure must not fail a passing flow. + * A failed `unlink` must not fail a passing flow, so the error is swallowed — + * but `warn` is called with the path so the user can remove the file manually. */ export async function maybeCleanupTrace( fs: { unlink: (p: string) => Promise }, tracePath: string, passed: boolean, traceMode: TraceMode, + warn?: (message: string) => void, ): Promise { if (traceMode !== "retain-on-failure" || !passed) return; try { await fs.unlink(tracePath); - } catch { - // best-effort; do not surface cleanup errors + } catch (err) { + warn?.(runnerMessages.traceCleanupFailed(tracePath, errorMessage(err))); } } @@ -118,18 +122,20 @@ export function buildContextSetup( /** * Deletes the HAR file when `harMode` is `"retain-on-failure"` and the flow * passed. No-ops for `"on"` and `"off"` modes. - * Errors from `unlink` are swallowed — cleanup failure must not fail a passing flow. + * A failed `unlink` must not fail a passing flow, so the error is swallowed — + * but `warn` is called with the path so the user can remove the file manually. */ export async function maybeCleanupHar( fs: { unlink: (p: string) => Promise }, harPath: string, passed: boolean, harMode: HarMode, + warn?: (message: string) => void, ): Promise { if (harMode !== "retain-on-failure" || !passed) return; try { await fs.unlink(harPath); - } catch { - // best-effort; do not surface cleanup errors + } catch (err) { + warn?.(runnerMessages.harCleanupFailed(harPath, errorMessage(err))); } }