From c66729eca902a21dd70f50ce107fec7598be0f31 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:56:06 +0800 Subject: [PATCH] fix(workers): retain cancellation through final cleanup --- src/browser/WebBundleAnalysisWorker.ts | 19 ++- src/domain/analysisErrorCancellation.ts | 24 ++++ .../analysis/JavaScriptAnalysisWorker.ts | 28 +++-- .../webWorkerFinalizationCancellation.test.ts | 119 ++++++++++++++++++ ...riptWorkerFinalizationCancellation.test.ts | 90 +++++++------ .../workerFailureComposition.test.ts | 47 +++++-- 6 files changed, 267 insertions(+), 60 deletions(-) create mode 100644 src/domain/analysisErrorCancellation.ts create mode 100644 tests/boundary/browser/webWorkerFinalizationCancellation.test.ts diff --git a/src/browser/WebBundleAnalysisWorker.ts b/src/browser/WebBundleAnalysisWorker.ts index 218beffa48..fdc27314a5 100644 --- a/src/browser/WebBundleAnalysisWorker.ts +++ b/src/browser/WebBundleAnalysisWorker.ts @@ -1,3 +1,4 @@ +import { analysisErrorWithCancellation } from "../domain/analysisErrorCancellation.js"; import { WorkerFrameSignal } from "../process/WorkerFrameSignal.js"; import { createHash, randomUUID } from "node:crypto"; import { open, rm } from "node:fs/promises"; @@ -463,13 +464,21 @@ export class WebBundleAnalysisWorker { await rm(join(root, `${id}.result`), { force: true }); } } catch (cause) { - return failed( - this.#cleanupFailure(cause, result.ok ? undefined : result.error), - ); + const previous = result.ok ? undefined : result.error; + const primary = signal?.aborted + ? analysisErrorWithCancellation( + previous, + new AnalysisCancelledError(OPERATION, { cause: signal.reason }), + ) + : previous; + return failed(this.#cleanupFailure(cause, primary)); } - if (signal?.aborted && result.ok) + if (signal?.aborted) return failed( - new AnalysisCancelledError(OPERATION, { cause: signal.reason }), + analysisErrorWithCancellation( + result.ok ? undefined : result.error, + new AnalysisCancelledError(OPERATION, { cause: signal.reason }), + ), ); return result.ok ? result : failed(result.error); } diff --git a/src/domain/analysisErrorCancellation.ts b/src/domain/analysisErrorCancellation.ts new file mode 100644 index 0000000000..da7164bb16 --- /dev/null +++ b/src/domain/analysisErrorCancellation.ts @@ -0,0 +1,24 @@ +import { + primaryAnalysisError, + type AnalysisError, +} from "./analysisErrorBase.js"; +import { AnalysisCancelledError } from "./analysisErrorCore.js"; + +/** Keep the established failure while retaining a later caller cancellation once. */ +export const analysisErrorWithCancellation = ( + failure: AnalysisError | undefined, + cancellation: AnalysisCancelledError, +): AnalysisError => { + if (failure === undefined) return cancellation; + const primary = primaryAnalysisError(failure); + if ( + !(primary instanceof AnalysisCancelledError) && + !primary.secondaryFailures.some( + (secondary) => + secondary instanceof AnalysisCancelledError && + secondary.cause === cancellation.cause, + ) + ) + primary.retainSecondaryFailure(cancellation); + return failure; +}; diff --git a/src/javascript/analysis/JavaScriptAnalysisWorker.ts b/src/javascript/analysis/JavaScriptAnalysisWorker.ts index 58e87c9cf6..7b15baf7a5 100644 --- a/src/javascript/analysis/JavaScriptAnalysisWorker.ts +++ b/src/javascript/analysis/JavaScriptAnalysisWorker.ts @@ -1,3 +1,4 @@ +import { analysisErrorWithCancellation } from "../../domain/analysisErrorCancellation.js"; import { safeParseUtf8Json } from "../../domain/safeJson.js"; import { analysisSettingLimit } from "../../domain/analysisSettingDefinitions.js"; import { WorkerFrameSignal } from "../../process/WorkerFrameSignal.js"; @@ -541,7 +542,17 @@ export class JavaScriptAnalysisWorker { await rm(join(rootPath, `${id}.${suffix}`), { force: true }); } catch (cause: unknown) { this.#runtime.retainForCleanup(); - const error = this.#cleanupError(message(cause)); + const previous = result.ok ? undefined : result.error.error; + const primary = signal?.aborted + ? analysisErrorWithCancellation( + previous, + new AnalysisCancelledError(OPERATION, { + cause: signal.reason, + capturedOutput: this.#captured(), + }), + ) + : previous; + const error = this.#cleanupError(message(cause), primary); this.#runtime.bindCleanupFailure(error); result = err({ javascript, @@ -551,12 +562,15 @@ export class JavaScriptAnalysisWorker { }); } } - if (result.ok && signal?.aborted === true) { - const error = new AnalysisCancelledError(OPERATION, { - cause: signal.reason, - capturedOutput: this.#captured(), - }); - this.#lastFailure = error; + if (signal?.aborted === true) { + const error = analysisErrorWithCancellation( + result.ok ? undefined : result.error.error, + new AnalysisCancelledError(OPERATION, { + cause: signal.reason, + capturedOutput: this.#captured(), + }), + ); + if (result.ok) this.#lastFailure = error; result = err({ error, javascript, module, projection }); } if (this.#applicationProjectionFailure !== undefined) diff --git a/tests/boundary/browser/webWorkerFinalizationCancellation.test.ts b/tests/boundary/browser/webWorkerFinalizationCancellation.test.ts new file mode 100644 index 0000000000..de3a8c800e --- /dev/null +++ b/tests/boundary/browser/webWorkerFinalizationCancellation.test.ts @@ -0,0 +1,119 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +import { expect, it } from "vitest"; + +it.each([ + [false, false], + [true, false], + [false, true], + [true, true], +])( + "retains completed web worker facts when cancellation arrives during final job cleanup (cleanup failure: %s; prior failure: %s)", + async (failCleanup, priorFailure) => { + const loaderUrl = new URL( + "../../../scripts/lib/verifier-source-loader.mjs", + import.meta.url, + ).href; + const workerUrl = new URL( + "../../../src/browser/WebBundleAnalysisWorker.ts", + import.meta.url, + ).href; + const processUrl = new URL( + "../../../src/process/ProviderProcess.ts", + import.meta.url, + ).href; + // Isolate the filesystem seam in a Node process. Both analyses use the real + // analyzer. Failed final removal retains the real file for owning close retry. + const script = ` + import fs from 'node:fs'; + import { createHash } from 'node:crypto'; + import { syncBuiltinESMExports } from 'node:module'; + const { importVerifierSource } = await import(${JSON.stringify(loaderUrl)}); + const controller = new AbortController(); + const reason = new Error('cancel during final worker cleanup'); + const nativeRm = fs.promises.rm; + let cancelOnCleanup = false; + let completedRemovals = 0; + fs.promises.rm = async (path, options) => { + const finalJobRemoval = String(path).includes('rea-web-bundle-analysis-') && String(path).endsWith('.result'); + if (finalJobRemoval && cancelOnCleanup && ${JSON.stringify(failCleanup)}) { + controller.abort(reason); + throw new Error('fixture final removal denied'); + } + await nativeRm(path, options); + if (finalJobRemoval) { + completedRemovals += 1; + if (cancelOnCleanup) controller.abort(reason); + } + }; + syncBuiltinESMExports(); + const { WebBundleAnalysisWorker } = await importVerifierSource(new URL(${JSON.stringify(workerUrl)})); + const { spawnOwnedProviderProcess } = await importVerifierSource(new URL(${JSON.stringify(processUrl)})); + const { createWebTextArtifact } = await importVerifierSource(new URL('../../../src/domain/webContentArtifact.ts', ${JSON.stringify(import.meta.url)})); + const value = 'export const observed = 7;'; + const source = path => ({ script_key: 'scr_' + createHash('sha256').update(value).digest('hex'), + frame_id: 'frame-1', url: 'https://app.example.test/' + path, origin: 'https://app.example.test', + cdp_hash: null, length: Buffer.byteLength(value), is_module: true, language: 'JavaScript', source_map_url: null, + resource_reconciliation: {status: 'unmatched', reason: 'no_exact_sanitized_url'}, + source: {included: true, artifact: createWebTextArtifact(value, 'application/javascript')} }); + let root; + let child; + let launches = 0; + const worker = new WebBundleAnalysisWorker(128, async request => { + root = request.cwd; + const spawned = await spawnOwnedProviderProcess(${JSON.stringify(priorFailure)} ? { + ...request, + arguments: ['-e', 'const send = value => process.stdout.write(JSON.stringify(value) + "\\n"); send({kind:"ready",heap_limit_bytes:require("node:v8").getHeapStatistics().heap_size_limit}); require("node:readline").createInterface({input:process.stdin}).on("line", line => { const request=JSON.parse(line); send({kind:"failure",id:request.id,reason:"output",message:"fixture output failure"}); });'], + } : request); + child = spawned.process; + launches += 1; + return spawned; + }); + let observed; + try { + const control = await worker.analyze(source('control.mjs'), 5000); + + cancelOnCleanup = true; + const cancelled = await worker.analyze(source('cancelled.mjs'), 5000, controller.signal); + const facts = cancelled.ok ? cancelled.value : cancelled.error; + const primary = cancelled.ok ? undefined : (cancelled.error.error.cleanupPrimary ?? cancelled.error.error); + const cancellation = primary?._tag === 'AnalysisCancelledError' ? primary : primary?.secondaryFailures.find(failure => failure._tag === 'AnalysisCancelledError'); + observed = { primaryOutput: primary?._tag === 'AnalysisOutputError', secondaryCount: primary?.secondaryFailures.length ?? 0, controlOk: control.ok, completedRemovals, launches, + cancelled: cancellation !== undefined, + exactReason: cancellation?.cause === reason, + cleanupIncomplete: !cancelled.ok && cancelled.error.error.cleanupIncomplete, + hasFacts: facts.status === 'analyzed' || facts.completed?.status === 'analyzed' }; + } finally { + fs.promises.rm = nativeRm; + syncBuiltinESMExports(); + await worker.close(); + } + const rootRemoved = await fs.promises.stat(root).then(() => false, cause => { + if (cause.code !== 'ENOENT') throw cause; + return true; + }); + process.stdout.write(JSON.stringify({ ...observed, rootRemoved, + childExited: child.exitCode !== null || child.signalCode !== null })); + `; + const { stdout } = await promisify(execFile)( + process.execPath, + ["--input-type=module", "--eval", script], + { timeout: 20_000 }, + ); + const observed: unknown = JSON.parse(stdout); + expect(observed).toEqual({ + primaryOutput: priorFailure, + secondaryCount: priorFailure ? 1 : 0, + controlOk: !priorFailure, + completedRemovals: failCleanup ? 1 : 2, + launches: priorFailure ? 2 : 1, + cancelled: true, + exactReason: true, + cleanupIncomplete: failCleanup, + hasFacts: !priorFailure, + rootRemoved: true, + childExited: true, + }); + }, +); diff --git a/tests/boundary/javascript/javascriptWorkerFinalizationCancellation.test.ts b/tests/boundary/javascript/javascriptWorkerFinalizationCancellation.test.ts index f4b0ae5f5f..7f3f67de7f 100644 --- a/tests/boundary/javascript/javascriptWorkerFinalizationCancellation.test.ts +++ b/tests/boundary/javascript/javascriptWorkerFinalizationCancellation.test.ts @@ -3,22 +3,24 @@ import { promisify } from "node:util"; import { expect, it } from "vitest"; -it("retains completed worker facts when cancellation arrives during final job cleanup", async () => { - const loaderUrl = new URL( - "../../../scripts/lib/verifier-source-loader.mjs", - import.meta.url, - ).href; - const workerUrl = new URL( - "../../../src/javascript/analysis/JavaScriptAnalysisWorker.ts", - import.meta.url, - ).href; - const processUrl = new URL( - "../../../src/process/ProviderProcess.ts", - import.meta.url, - ).href; - // Isolate the filesystem seam in a Node process. Both analyses use the real - // analyzer and remove their actual files before the cancellation is delivered. - const script = ` +it.each([false, true])( + "retains completed worker facts when cancellation arrives during final job cleanup (failure: %s)", + async (failCleanup) => { + const loaderUrl = new URL( + "../../../scripts/lib/verifier-source-loader.mjs", + import.meta.url, + ).href; + const workerUrl = new URL( + "../../../src/javascript/analysis/JavaScriptAnalysisWorker.ts", + import.meta.url, + ).href; + const processUrl = new URL( + "../../../src/process/ProviderProcess.ts", + import.meta.url, + ).href; + // Isolate the filesystem seam in a Node process. Both analyses use the real + // analyzer. Failed final removal retains the real file for owning close retry. + const script = ` import fs from 'node:fs'; import { createHash } from 'node:crypto'; import { syncBuiltinESMExports } from 'node:module'; @@ -29,8 +31,13 @@ it("retains completed worker facts when cancellation arrives during final job cl let cancelOnCleanup = false; let completedRemovals = 0; fs.promises.rm = async (path, options) => { + const finalJobRemoval = String(path).includes('rea-javascript-analysis-') && String(path).endsWith('.records'); + if (finalJobRemoval && cancelOnCleanup && ${JSON.stringify(failCleanup)}) { + controller.abort(reason); + throw new Error('fixture final removal denied'); + } await nativeRm(path, options); - if (String(path).includes('rea-javascript-analysis-') && String(path).endsWith('.records')) { + if (finalJobRemoval) { completedRemovals += 1; if (cancelOnCleanup) controller.abort(reason); } @@ -60,8 +67,9 @@ it("retains completed worker facts when cancellation arrives during final job cl const cancelled = await worker.analyze(source('cancelled.mjs'), { nodes: 100, unknowns: 100 }, controller.signal); const facts = cancelled.ok ? cancelled.value : cancelled.error; observed = { controlOk: control.ok, controlReusable, completedRemovals, launches, - cancelled: !cancelled.ok && cancelled.error.error._tag === 'AnalysisCancelledError', - exactReason: !cancelled.ok && cancelled.error.error.cause === reason, + cancelled: !cancelled.ok && (cancelled.error.error.cleanupPrimary ?? cancelled.error.error)._tag === 'AnalysisCancelledError', + exactReason: !cancelled.ok && (cancelled.error.error.cleanupPrimary ?? cancelled.error.error).cause === reason, + cleanupIncomplete: !cancelled.ok && cancelled.error.error.cleanupIncomplete, parseStatus: facts.javascript?.parse_status, hasModule: facts.module !== null, hasProjection: facts.projection !== null, reusable: worker.canReuse() }; } finally { @@ -76,24 +84,26 @@ it("retains completed worker facts when cancellation arrives during final job cl process.stdout.write(JSON.stringify({ ...observed, rootRemoved, childExited: child.exitCode !== null || child.signalCode !== null })); `; - const { stdout } = await promisify(execFile)( - process.execPath, - ["--input-type=module", "--eval", script], - { timeout: 20_000 }, - ); - const observed: unknown = JSON.parse(stdout); - expect(observed).toEqual({ - controlOk: true, - controlReusable: true, - completedRemovals: 2, - launches: 1, - cancelled: true, - exactReason: true, - parseStatus: "complete", - hasModule: true, - hasProjection: true, - reusable: false, - rootRemoved: true, - childExited: true, - }); -}); + const { stdout } = await promisify(execFile)( + process.execPath, + ["--input-type=module", "--eval", script], + { timeout: 20_000 }, + ); + const observed: unknown = JSON.parse(stdout); + expect(observed).toEqual({ + controlOk: true, + controlReusable: true, + completedRemovals: failCleanup ? 1 : 2, + launches: 1, + cancelled: true, + exactReason: true, + cleanupIncomplete: failCleanup, + parseStatus: "complete", + hasModule: true, + hasProjection: true, + reusable: false, + rootRemoved: true, + childExited: true, + }); + }, +); diff --git a/tests/boundary/javascript/workerFailureComposition.test.ts b/tests/boundary/javascript/workerFailureComposition.test.ts index 94c855b2f7..2013ea765f 100644 --- a/tests/boundary/javascript/workerFailureComposition.test.ts +++ b/tests/boundary/javascript/workerFailureComposition.test.ts @@ -31,13 +31,17 @@ vi.mock("node:fs/promises", async (original) => { return { ...actual, rm: async (...args: Parameters) => { + if ( + String(args[0]).includes("rea-javascript-analysis-") && + String(args[0]).endsWith(".source") + ) + faults.onSourceRemoval?.(); if ( faults.sourceRemoval && String(args[0]).includes("rea-javascript-analysis-") && String(args[0]).endsWith(".source") ) { faults.removals += 1; - faults.onSourceRemoval?.(); throw Object.assign( new Error("controlled source-file removal failure"), { code: "EACCES" }, @@ -77,9 +81,20 @@ afterEach(() => { faults.onSourceRemoval = undefined; }); -it.each(["io", "output"] as const)( - "keeps worker %s failure classification distinct at the protocol boundary", - async (reason) => { +it.each([ + ["io", false, false], + ["output", false, false], + ["output", true, true], + ["output", false, true], +] as const)( + "keeps worker %s failure classification distinct at the protocol boundary (cleanup: %s; cancellation: %s)", + async (reason, deniedCleanup, cancelCleanup) => { + const controller = new AbortController(); + const cancellation = new Error("cancel during failed job cleanup"); + faults.sourceRemoval = deniedCleanup; + faults.onSourceRemoval = cancelCleanup + ? () => controller.abort(cancellation) + : undefined; const worker = new JavaScriptAnalysisWorker( { heapMb: 128, timeoutMs: 30_000 }, async (request) => @@ -92,13 +107,26 @@ it.each(["io", "output"] as const)( }), ); try { - const result = await worker.analyze(file, { nodes: 100, unknowns: 100 }); + const result = await worker.analyze( + file, + { nodes: 100, unknowns: 100 }, + controller.signal, + ); expect(result.ok).toBe(false); if (result.ok) throw new Error("Expected worker failure"); - expect(result.error.error._tag).toBe( + const failure = result.error.error.cleanupPrimary ?? result.error.error; + expect(result.error.error.cleanupIncomplete).toBe(deniedCleanup); + if (cancelCleanup) { + expect(failure.secondaryFailures).toHaveLength(1); + expect(failure.secondaryFailures[0]).toMatchObject({ + _tag: "AnalysisCancelledError", + cause: cancellation, + }); + } + expect(failure._tag).toBe( reason === "io" ? "ProviderAdapterError" : "AnalysisOutputError", ); - const projected = projectAnalysisError(result.error.error); + const projected = projectAnalysisError(failure); expect(projected.code).toBe( reason === "io" ? "execution_failure" : "unreadable_output", ); @@ -112,6 +140,7 @@ it.each(["io", "output"] as const)( expect(projected.message).toContain("storage failed"); } } finally { + faults.sourceRemoval = false; await worker.close(); } }, @@ -124,7 +153,9 @@ it.each([false, true])( faults.sourceRemoval = removalFailure; const controller = new AbortController(); const reason = new Error("cancel during denied source-file cleanup"); - faults.onSourceRemoval = () => controller.abort(reason); + faults.onSourceRemoval = removalFailure + ? () => controller.abort(reason) + : undefined; const sourceMapUrls = [ { declared_url: "app.map",