diff --git a/src/javascript/sourceMaps/SourceMapCommand.test.ts b/src/javascript/sourceMaps/SourceMapCommand.test.ts index cdca518743..ae2b338408 100644 --- a/src/javascript/sourceMaps/SourceMapCommand.test.ts +++ b/src/javascript/sourceMaps/SourceMapCommand.test.ts @@ -1,5 +1,5 @@ import { access } from "node:fs/promises"; -import { expect, it, vi } from "vitest"; +import { afterEach, expect, it, vi } from "vitest"; import { AnalysisCancelledError, AnalysisResourceConstraintError, @@ -14,8 +14,18 @@ import { WEB_SOURCE_MAP_LIMITS } from "../../domain/webSourceLocation.js"; import { runSourceMapCommand, SOURCE_MAP_CODEC_MEMORY_LIMIT_BYTES, + type SourceMapCleanupRetention, } from "./SourceMapCommand.js"; +const unexpectedCleanup: SourceMapCleanupRetention = {}; +afterEach(async () => { + const owner = unexpectedCleanup.owner; + const failure = await owner?.close(); + if (failure !== undefined) throw failure; + unexpectedCleanup.owner = undefined; + expect(owner).toBeUndefined(); +}); + const runChild = ( signal: AbortSignal | undefined, script: string, @@ -44,6 +54,7 @@ const runChild = ( return child; }, }, + unexpectedCleanup, ); const exitsAfterHoldingOutput = @@ -65,6 +76,7 @@ it("preserves the exact object reason for an already-aborted caller", async () = throw new Error("must not launch"); }, }, + unexpectedCleanup, ); expect(result.ok).toBe(false); if (result.ok) throw new Error("expected cancellation"); @@ -72,6 +84,67 @@ it("preserves the exact object reason for an already-aborted caller", async () = expect(result.error.cause).toBe(reason); }); +it.each([null, { caller: "late source-map cancellation" }])( + "retains a late caller reason %j when runtime cleanup first fails", + async (reason) => { + const root = await PrivateRuntimeRoot.create({ + prefix: "rea-source-map-late-cleanup-test-", + }); + const controller = new AbortController(); + let cleanupAttempts = 0; + const retainedCleanup: SourceMapCleanupRetention = {}; + try { + const result = await runSourceMapCommand( + "{}", + "/probe/source.map", + { signal: controller.signal }, + { + prepareOwnership: async () => {}, + createRuntime: async () => ({ + path: root.path, + close: async () => { + cleanupAttempts += 1; + if (cleanupAttempts === 1) { + controller.abort(reason); + throw new Error("controlled late cleanup failure"); + } + await root.close(); + }, + }), + launcher: async (options) => + spawnOwnedProviderProcess({ + ...options, + arguments: ["-e", 'process.stdout.write("complete")'], + }), + }, + retainedCleanup, + ); + expect(result.ok).toBe(false); + if (result.ok) + throw new Error("expected cancellation with cleanup failure"); + expect(result.error.cleanupPrimary).toBeInstanceOf( + AnalysisCancelledError, + ); + expect(result.error.cleanupPrimary?.cause).toBe(reason); + expect(result.error.cleanupResources).toEqual([root.path]); + expect(result.error.cleanupIncomplete).toBe(true); + expect(retainedCleanup.owner).toBeDefined(); + await retainedCleanup.owner?.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cleanupResources).toEqual([]); + expect(result.error.cleanupPrimary?.cause).toBe(reason); + } finally { + await retainedCleanup.owner?.close(); + await root.close(); + } + }, +); + +it("returns completed codec output after successful cleanup", async () => { + const result = await runChild(undefined, 'process.stdout.write("complete")'); + expect(result).toEqual({ ok: true, value: "complete" }); +}); + it("preserves exact caller reason when cancellation precedes process exit", async () => { const reason = { caller: "active source-map cancellation" }; const controller = new AbortController(); @@ -222,6 +295,7 @@ it("preserves an observed process-memory limit over the codec exit status", asyn return spawned; }, }, + unexpectedCleanup, ); } finally { waitSpy.mockRestore(); @@ -266,6 +340,7 @@ it("keeps a timeout distinct from caller cancellation", async () => { throw new Error("launcher settled after deadline"); }, }, + unexpectedCleanup, ); expect(result.ok).toBe(false); if (result.ok) throw new Error("expected timeout"); @@ -301,6 +376,7 @@ it("applies a non-default decode deadline and releases its runtime", async () => throw new Error("launcher settled after deadline"); }, }, + unexpectedCleanup, 250, ); expect(result.ok).toBe(false); diff --git a/src/javascript/sourceMaps/SourceMapCommand.ts b/src/javascript/sourceMaps/SourceMapCommand.ts index 3cf7485560..3da5b9eef6 100644 --- a/src/javascript/sourceMaps/SourceMapCommand.ts +++ b/src/javascript/sourceMaps/SourceMapCommand.ts @@ -4,6 +4,7 @@ import { join } from "node:path"; import { fileURLToPath } from "node:url"; import type { ExecutionOptions } from "../../application/AnalysisProvider.js"; import { AnalysisError } from "../../domain/analysisErrorBase.js"; +import { analysisErrorWithCleanupFailure } from "../../domain/analysisErrorCleanup.js"; import { AnalysisCancelledError, AnalysisOutputError, @@ -62,9 +63,12 @@ export interface SourceMapDecoderDependencies { } export interface SourceMapCleanupOwner { - close( - previousError: string | null, - ): Promise; + close(): Promise; +} + +/** The caller retains retry authority independently of the returned error. */ +export interface SourceMapCleanupRetention { + owner?: SourceMapCleanupOwner | undefined; } type CodecPhysicalObservation = @@ -88,7 +92,6 @@ type CodecOutcome = export class SourceMapCleanupFailure extends ProviderCleanupError { constructor( - readonly cleanupOwner: SourceMapCleanupOwner, resources: readonly string[], diagnostics: Readonly>, ) { @@ -102,6 +105,7 @@ export const runSourceMapCommand = async ( path: string, options: ExecutionOptions | undefined, dependencies: SourceMapDecoderDependencies, + retainedCleanup: SourceMapCleanupRetention, decodeTimeoutMs: number = DEFAULT_WEB_SOURCE_MAP_DECODE_TIMEOUT_MS, ): Promise> => { try { @@ -141,13 +145,26 @@ export const runSourceMapCommand = async ( command.dispose(); deadline.dispose(); } - const cleanup = await command.close(result.ok ? null : result.error.message); - if (cleanup !== undefined) return err(cleanup); - return result.ok && options?.signal?.aborted - ? err( - new AnalysisCancelledError(OPERATION, { cause: options.signal.reason }), - ) - : result; + const cleanup = await command.close(); + const completed = + result.ok && options?.signal?.aborted + ? err( + new AnalysisCancelledError(OPERATION, { + cause: options.signal.reason, + }), + ) + : result; + if (cleanup === undefined) return completed; + retainedCleanup.owner = command; + return completed.ok + ? err(cleanup) + : err( + analysisErrorWithCleanupFailure( + completed.error, + cleanup, + OPERATION, + ).bindCleanupDependencies([cleanup], command), + ); }; class OwnedCodec implements SourceMapCleanupOwner { @@ -471,9 +488,7 @@ class OwnedCodec implements SourceMapCleanupOwner { ]; } - async close( - previousError: string | null, - ): Promise { + async close(): Promise { const failures: string[] = []; let processReleased = this.#supervisor === undefined; if (this.#supervisor !== undefined) { @@ -497,10 +512,9 @@ class OwnedCodec implements SourceMapCleanupOwner { this.#cleanupAnchor = undefined; return undefined; } - const failure = new SourceMapCleanupFailure(this, this.cleanupResources, { + const failure = new SourceMapCleanupFailure(this.cleanupResources, { map_path: this.path, cleanup_failures: failures, - previous_error: previousError, codec_pid: this.#supervisor?.launch.process.pid ?? null, }).bindCleanupResourceOwner(this); if (this.#cleanupAnchor === undefined) this.#cleanupAnchor = failure; diff --git a/src/javascript/sourceMaps/SourceMapDecoder.test.ts b/src/javascript/sourceMaps/SourceMapDecoder.test.ts index dc062ac01b..9236716b0f 100644 --- a/src/javascript/sourceMaps/SourceMapDecoder.test.ts +++ b/src/javascript/sourceMaps/SourceMapDecoder.test.ts @@ -67,19 +67,30 @@ it("retains failed cleanup ownership and retries it before another decode", asyn expect(failed.ok).toBe(false); if (failed.ok) throw new Error("expected physical failure with cleanup failure"); - expect(failed.error).toBeInstanceOf(SourceMapCleanupFailure); - if (!(failed.error instanceof SourceMapCleanupFailure)) - throw new Error("expected source-map cleanup ownership"); - expect(failed.error.cleanupOwner).toBeDefined(); + const primary = failed.error.cleanupPrimary; + expect(primary).toMatchObject({ + _tag: "ProviderAdapterError", + diagnostics: { + exit_code: 7, + stderr: "physical exit 7", + }, + }); + expect(primary?.capturedOutput?.stderr).toBe("physical exit 7"); + expect(failed.error.capturedOutput).toEqual(primary?.capturedOutput); + expect(failed.error.cause).toBeInstanceOf(AggregateError); + expect((failed.error.cause as AggregateError).errors[0]).toBe(primary); expect(failed.error.cleanupResources).toEqual([root.path]); expect(failed.error.diagnostics).toMatchObject({ - cleanup_failures: ["injected root cleanup failure"], - }); - expect(failed.error.diagnostics).toMatchObject({ - previous_error: expect.stringContaining( - "source-map-decoder adapter failed", - ), + primary_error: { code: "execution_failure" }, + cleanup_error: { + details: { + diagnostics: { + cleanup_failures: ["injected root cleanup failure"], + }, + }, + }, }); + expect(projectAnalysisError(failed.error).code).toBe("cleanup_incomplete"); await expect(access(root.path)).resolves.toBeUndefined(); const invalidInput = { ...input, text: null } as unknown as typeof input; @@ -92,11 +103,16 @@ it("retains failed cleanup ownership and retries it before another decode", asyn await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); expect(failed.error.cleanupIncomplete).toBe(false); expect(failed.error.cleanupResources).toEqual([]); - expect(projectAnalysisError(failed.error).code).not.toBe( - "cleanup_incomplete", - ); + expect(projectAnalysisError(failed.error).code).toBe("execution_failure"); + expect(failed.error.cleanupPrimary).toBe(primary); expect(failed.error.diagnostics).toMatchObject({ - cleanup_failures: ["injected root cleanup failure"], + cleanup_error: { + details: { + diagnostics: { + cleanup_failures: ["injected root cleanup failure"], + }, + }, + }, }); } finally { await decoder.close(); @@ -108,6 +124,7 @@ it("settles every source-map cleanup failure when final close releases the exact prefix: "rea-source-map-final-cleanup-test-", }); let attempts = 0; + const launchCause = new Error("controlled launch failure"); const decoder = new SourceMapDecoder({ prepareOwnership: async () => {}, createRuntime: async () => ({ @@ -120,12 +137,16 @@ it("settles every source-map cleanup failure when final close releases the exact }, }), launcher: async () => { - throw new Error("controlled launch failure"); + throw launchCause; }, }); try { const initial = await decoder.trace(input); if (initial.ok) throw new Error("expected initial cleanup failure"); + expect(initial.error.cleanupPrimary?.cause).toBe(launchCause); + expect((initial.error.cause as AggregateError).errors[0]).toBe( + initial.error.cleanupPrimary, + ); const retry = await decoder.trace(input); if (retry.ok) throw new Error("expected retry cleanup failure"); for (const failure of [initial.error, retry.error]) { @@ -139,11 +160,22 @@ it("settles every source-map cleanup failure when final close releases the exact for (const failure of [initial.error, retry.error]) { expect(failure.cleanupIncomplete).toBe(false); expect(failure.cleanupResources).toEqual([]); - expect(projectAnalysisError(failure).code).not.toBe("cleanup_incomplete"); - expect(failure.diagnostics?.cleanup_failures).toEqual([ - expect.stringContaining("runtime cleanup attempt"), - ]); + expect(projectAnalysisError(failure).code).toBe("execution_failure"); } + expect(initial.error.cleanupPrimary?.cause).toBe(launchCause); + expect(initial.error.diagnostics).toMatchObject({ + cleanup_error: { + details: { + diagnostics: { + cleanup_failures: ["runtime cleanup attempt 1 failed"], + }, + }, + }, + }); + expect(retry.error).toBeInstanceOf(SourceMapCleanupFailure); + expect(retry.error.diagnostics?.cleanup_failures).toEqual([ + "runtime cleanup attempt 2 failed", + ]); } finally { await decoder.close(); } diff --git a/src/javascript/sourceMaps/SourceMapDecoder.ts b/src/javascript/sourceMaps/SourceMapDecoder.ts index 4159330ea1..08eca6a1c5 100644 --- a/src/javascript/sourceMaps/SourceMapDecoder.ts +++ b/src/javascript/sourceMaps/SourceMapDecoder.ts @@ -22,9 +22,9 @@ import { } from "./SourceMapCodecProtocol.js"; import { SOURCE_MAP_CODEC_MEMORY_LIMIT_BYTES, - SourceMapCleanupFailure, runSourceMapCommand, type SourceMapCleanupOwner, + type SourceMapCleanupRetention, type SourceMapDecoderDependencies, } from "./SourceMapCommand.js"; import { ProviderCleanupError } from "../../domain/providerCleanupError.js"; @@ -114,18 +114,18 @@ export class SourceMapDecoder implements WebSourceMapPort { new AnalysisCancelledError(OPERATION, { cause: options.signal.reason }), ); const serialized = encoded.join(""); + const retainedCleanup: SourceMapCleanupRetention = {}; const response = await runSourceMapCommand( serialized, input.path, options, this.dependencies, + retainedCleanup, input.decodeTimeoutMs ?? DEFAULT_WEB_SOURCE_MAP_DECODE_TIMEOUT_MS, ); - if (!response.ok) { - if (response.error instanceof SourceMapCleanupFailure) - this.#pendingCleanup = response.error.cleanupOwner; - return response; - } + if (retainedCleanup.owner !== undefined) + this.#pendingCleanup = retainedCleanup.owner; + if (!response.ok) return response; let value: unknown; try { value = JSON.parse(response.value); @@ -174,7 +174,7 @@ export class SourceMapDecoder implements WebSourceMapPort { async #retryCleanup(): Promise { const pending = this.#pendingCleanup; if (pending === undefined) return undefined; - const failure = await pending.close(null); + const failure = await pending.close(); if (failure !== undefined) return failure; this.#pendingCleanup = undefined; return undefined; diff --git a/tests/boundary/javascript/sourceMapPreparation.test.ts b/tests/boundary/javascript/sourceMapPreparation.test.ts index 5d9b01ff05..904462079f 100644 --- a/tests/boundary/javascript/sourceMapPreparation.test.ts +++ b/tests/boundary/javascript/sourceMapPreparation.test.ts @@ -2,7 +2,10 @@ import { expect, it, vi } from "vitest"; import { AnalysisCancelledError } from "../../../src/domain/analysisErrorCore.js"; import { ProviderAdapterError } from "../../../src/domain/providerAdapterError.js"; import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; -import { runSourceMapCommand } from "../../../src/javascript/sourceMaps/SourceMapCommand.js"; +import { + runSourceMapCommand, + type SourceMapCleanupRetention, +} from "../../../src/javascript/sourceMaps/SourceMapCommand.js"; it.each([ null, @@ -11,6 +14,7 @@ it.each([ "preserves opaque cancellation during ownership preparation: %s", async (reason) => { const controller = new AbortController(); + const retainedCleanup: SourceMapCleanupRetention = {}; const createRuntime = vi.fn(async () => { throw new Error("Cancelled preparation must not acquire a runtime"); }); @@ -29,7 +33,9 @@ it.each([ createRuntime, launcher, }, + retainedCleanup, ); + expect(retainedCleanup.owner).toBeUndefined(); if (result.ok) throw new Error("Preparation must return cancellation"); expect(result.error).toBeInstanceOf(AnalysisCancelledError); expect(result.error.cause).toBe(reason); @@ -46,19 +52,27 @@ it.each([ it("retains diagnostics for an unknown ownership preparation failure", async () => { const cause = new Error("unexpected ownership failure"); + const retainedCleanup: SourceMapCleanupRetention = {}; const createRuntime = vi.fn(async () => { throw new Error("Failed preparation must not acquire a runtime"); }); const launcher = vi.fn(async () => { throw new Error("Failed preparation must not launch a codec"); }); - const result = await runSourceMapCommand("{}", "/selected.map", undefined, { - prepareOwnership: async () => { - throw cause; + const result = await runSourceMapCommand( + "{}", + "/selected.map", + undefined, + { + prepareOwnership: async () => { + throw cause; + }, + createRuntime, + launcher, }, - createRuntime, - launcher, - }); + retainedCleanup, + ); + expect(retainedCleanup.owner).toBeUndefined(); if (result.ok) throw new Error("Preparation must return its failure"); expect(result.error).toBeInstanceOf(ProviderAdapterError); expect(result.error.cause).toBe(cause); diff --git a/tests/boundary/javascript/sourceMapRuntimeAvailability.test.ts b/tests/boundary/javascript/sourceMapRuntimeAvailability.test.ts index 753a3bd470..19970876f7 100644 --- a/tests/boundary/javascript/sourceMapRuntimeAvailability.test.ts +++ b/tests/boundary/javascript/sourceMapRuntimeAvailability.test.ts @@ -1,6 +1,9 @@ import { expect, it } from "vitest"; import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; -import { runSourceMapCommand } from "../../../src/javascript/sourceMaps/SourceMapCommand.js"; +import { + runSourceMapCommand, + type SourceMapCleanupRetention, +} from "../../../src/javascript/sourceMaps/SourceMapCommand.js"; import { PrivateRuntimeRootUnavailableError } from "../../../src/process/PrivateRuntimeRootUnavailableError.js"; it.each([false, true])( @@ -8,16 +11,24 @@ it.each([false, true])( async (retryable) => { const reason = "Native runtime self-test failed with a sharing violation"; let launches = 0; - const result = await runSourceMapCommand("{}", "/selected.map", undefined, { - prepareOwnership: async () => {}, - createRuntime: async () => { - throw new PrivateRuntimeRootUnavailableError(reason, { retryable }); + const retainedCleanup: SourceMapCleanupRetention = {}; + const result = await runSourceMapCommand( + "{}", + "/selected.map", + undefined, + { + prepareOwnership: async () => {}, + createRuntime: async () => { + throw new PrivateRuntimeRootUnavailableError(reason, { retryable }); + }, + launcher: async () => { + launches += 1; + throw new Error("A refused runtime must not launch a codec"); + }, }, - launcher: async () => { - launches += 1; - throw new Error("A refused runtime must not launch a codec"); - }, - }); + retainedCleanup, + ); + expect(retainedCleanup.owner).toBeUndefined(); expect(launches).toBe(0); if (result.ok) throw new Error("Unavailable runtime must refuse decoding"); expect(projectAnalysisError(result.error)).toMatchObject({