From 890dfa49f60a66e819e86ba29f3728f2e1e1827b Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Sun, 11 Oct 2026 08:05:08 +0800 Subject: [PATCH] fix(javascript): preserve cleanup failures during preparation cancellation (#1654) --- .../analysis/JavaScriptAnalysisWorker.ts | 4 +- .../javascriptApplicationCancellation.test.ts | 36 ++++++++----- .../javascriptSourceAnalysisWorker.test.ts | 51 +++++++++++++++++++ 3 files changed, 78 insertions(+), 13 deletions(-) rename src/application/javascript/JavaScriptApplicationCancellation.test.ts => tests/boundary/javascript/javascriptApplicationCancellation.test.ts (81%) diff --git a/src/javascript/analysis/JavaScriptAnalysisWorker.ts b/src/javascript/analysis/JavaScriptAnalysisWorker.ts index c88e9e091c..e679ee80a1 100644 --- a/src/javascript/analysis/JavaScriptAnalysisWorker.ts +++ b/src/javascript/analysis/JavaScriptAnalysisWorker.ts @@ -637,7 +637,9 @@ export class JavaScriptAnalysisWorker { // Ownership preparation is abortable before the worker deadline exists. if ( deadline?.interruption === "cancelled" || - (deadline === undefined && signal?.aborted === true) + (deadline === undefined && + signal?.aborted === true && + !(cause instanceof ProviderCleanupError)) ) return new AnalysisCancelledError(OPERATION, { capturedOutput: this.#captured(), diff --git a/src/application/javascript/JavaScriptApplicationCancellation.test.ts b/tests/boundary/javascript/javascriptApplicationCancellation.test.ts similarity index 81% rename from src/application/javascript/JavaScriptApplicationCancellation.test.ts rename to tests/boundary/javascript/javascriptApplicationCancellation.test.ts index aa2b7b9454..8bc5f2aa3f 100644 --- a/src/application/javascript/JavaScriptApplicationCancellation.test.ts +++ b/tests/boundary/javascript/javascriptApplicationCancellation.test.ts @@ -2,11 +2,11 @@ import { setImmediate } from "node:timers"; import { writeFile } from "node:fs/promises"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; -import { createTestTempDirectory } from "../../../tests/fixtures/temporaryDirectory.js"; -import { projectAnalysisError } from "../../domain/analysisErrorProjection.js"; -import { analyzeJavaScriptApplication } from "../../../tests/support/javascriptApplicationScope.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; +import { analyzeJavaScriptApplication } from "../../support/javascriptApplicationScope.js"; -describe("JavaScript analysis cancellation before publication", () => { +describe("JavaScript application cancellation", () => { it("interrupts semantic graph commitment before result validation", async () => { const root = await createTestTempDirectory("rea-js-cancel-semantic-"); await writeFile( @@ -33,7 +33,9 @@ describe("JavaScript analysis cancellation before publication", () => { }, ); if (result.ok) - throw new Error("Cancelled commitment must not publish Evidence"); + throw new Error( + "Cancelled commitment must not publish a successful analysis", + ); expect(projectAnalysisError(result.error)).toMatchObject({ code: "cancelled", details: { reason: "cancelled" }, @@ -42,7 +44,7 @@ describe("JavaScript analysis cancellation before publication", () => { expect(phases).not.toContain("validate_javascript_application_result"); }); - it("handles control turns and cancellation inside one large source file (#1462)", async () => { + it("honors control turns and cancellation while preparing one large source for owned analysis (#1462)", async () => { const root = await createTestTempDirectory("rea-js-cancel-in-file-"); await writeFile( join(root, "index.js"), @@ -74,7 +76,7 @@ describe("JavaScript analysis cancellation before publication", () => { }, }, ); - if (result.ok) throw new Error("Cancelled analysis must not publish"); + if (result.ok) throw new Error("Cancelled analysis must not succeed"); expect(projectAnalysisError(result.error)).toMatchObject({ code: "cancelled", }); @@ -118,11 +120,21 @@ describe("JavaScript analysis cancellation before publication", () => { }, ); expect(requested).toBe(true); - if (result.ok) - throw new Error("Cancelled analysis must not publish Evidence"); - expect(projectAnalysisError(result.error)).toMatchObject({ - code: "cancelled", - }); + if (result.ok) throw new Error("Cancelled analysis must return a failure"); + const failure = projectAnalysisError(result.error); + expect(failure).toMatchObject({ code: "cancelled" }); + if (phase === "parse_javascript_source") + expect(failure).toMatchObject({ + details: { + operation: "analyze_javascript_application", + cleanup: "complete", + partial_observation: { + normalized_result: { + statistics: { relevant_files: 1, parse_failures: 0 }, + }, + }, + }, + }); expect(terminal).toEqual([]); }); diff --git a/tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts b/tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts index 46987c8779..2482675766 100644 --- a/tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts +++ b/tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts @@ -1,6 +1,9 @@ import { createHash } from "node:crypto"; import { expect, it } from "vitest"; import { JavaScriptAnalysisWorker } from "../../../src/javascript/analysis/JavaScriptAnalysisWorker.js"; +import { cleanupOwnedProcessGroup } from "../../../src/process/ProcessOwnership.js"; +import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; +import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; import { spawnOwnedProviderProcess } from "../../../src/process/ProviderProcess.js"; import type { JavaScriptArtifactFile } from "../../../src/domain/javascript/javascriptArtifactFiles.js"; @@ -91,3 +94,51 @@ it("reports an actual startup deadline and closes its owned resources before sub await next.close(); } }); + +it("preserves a failed owned cleanup retry when the next caller cancels before acquisition", async () => { + const controller = new AbortController(); + let releaseAllowed = false; + let cleanupAttempts = 0; + const worker = new JavaScriptAnalysisWorker( + { heapMb: 128, timeoutMs: 30_000 }, + async (request) => { + const launched = await spawnOwnedProviderProcess(request); + return { + ...launched, + async cleanup() { + cleanupAttempts += 1; + if (!releaseAllowed) { + if (cleanupAttempts > 1) controller.abort(); + throw new Error("Fixture owned cleanup is unavailable"); + } + return launched.cleanup === undefined + ? cleanupOwnedProcessGroup(launched.ownership) + : launched.cleanup(); + }, + }; + }, + ); + const next = new JavaScriptAnalysisWorker({ heapMb: 128, timeoutMs: 30_000 }); + try { + const file = source("app.mjs", "export const observed = 7;"); + const first = await worker.analyze(file, 100); + if (!first.ok) throw first.error.error; + await expect(worker.close()).rejects.toBeInstanceOf(ProviderCleanupError); + const interrupted = await next.analyze(file, 100, controller.signal); + if (interrupted.ok) + throw new Error("Unreleased ownership must block subsequent analysis"); + expect(controller.signal.aborted).toBe(true); + expect(projectAnalysisError(interrupted.error.error)).toMatchObject({ + code: "cleanup_incomplete", + }); + releaseAllowed = true; + await worker.close(); + const recovered = await next.analyze(file, 100); + if (!recovered.ok) throw recovered.error.error; + expect(recovered.value.javascript.parse_status).toBe("complete"); + } finally { + releaseAllowed = true; + await worker.close(); + await next.close(); + } +});