diff --git a/src/application/javascript/JavaScriptArtifactReconstruction.ts b/src/application/javascript/JavaScriptArtifactReconstruction.ts index 7b3972c363..00253da3fb 100644 --- a/src/application/javascript/JavaScriptArtifactReconstruction.ts +++ b/src/application/javascript/JavaScriptArtifactReconstruction.ts @@ -96,7 +96,12 @@ export const reconstructJavaScriptArtifact = async ( "read_javascript_artifacts", "Reading inventoried JavaScript application sources", ); - const files = await readJavaScriptArtifactFiles(reader, snapshot, signal); + const files = await readJavaScriptArtifactFiles( + reader, + snapshot, + scope, + signal, + ); await reportPhase( "parse_javascript_sources", `Parsing and projecting ${String(files.files.length)} application source files`, diff --git a/src/artifacts/javascript/JavaScriptArtifactFiles.ts b/src/artifacts/javascript/JavaScriptArtifactFiles.ts index cfe1e811bf..6ed6b29e71 100644 --- a/src/artifacts/javascript/JavaScriptArtifactFiles.ts +++ b/src/artifacts/javascript/JavaScriptArtifactFiles.ts @@ -4,6 +4,7 @@ import type { Readable } from "node:stream"; import { compareUnicodeCodePoints } from "../../domain/unicodeCodePointOrder.js"; import { AsarArtifactReader } from "../AsarArtifactReader.js"; +import type { ArtifactResourceScope } from "../ArtifactResourceScope.js"; import { ArtifactPathRegistry, normalizeArtifactPath, @@ -39,6 +40,7 @@ interface ExpectedContainer { } interface ReadContext { + readonly resources: ArtifactResourceScope; readonly expected: ReadonlyMap; readonly expectedContainers: ReadonlyMap; readonly registry: ArtifactPathRegistry; @@ -59,6 +61,7 @@ interface ReadTextInput { export const readJavaScriptArtifactFiles = async ( reader: ArtifactReader, snapshot: ArtifactInventorySnapshot, + resources: ArtifactResourceScope, signal?: AbortSignal, ): Promise => { if (reader instanceof AsarArtifactReader) { @@ -79,6 +82,7 @@ export const readJavaScriptArtifactFiles = async ( const inventory = expectedInventory(snapshot); const expected = inventory.files; const context: ReadContext = { + resources, expected, expectedContainers: inventory.containers, registry: new ArtifactPathRegistry(), @@ -122,7 +126,10 @@ const visitReader = async ( iterator: reader.entries(context.signal)[Symbol.asyncIterator](), }, ]; - const ownedReaders: ArtifactReader[] = []; + const ownedReaders: { + readonly reader: ArtifactReader; + readonly path: string; + }[] = []; let failure: { readonly cause: unknown } | undefined; try { while (stack.length > 0) { @@ -174,7 +181,7 @@ const visitReader = async ( ); context.containers.push(inventory); const nested = new AsarArtifactReader(entry.adapterKey); - ownedReaders.push(nested); + ownedReaders.push({ reader: nested, path }); stack.push({ reader: nested, prefix: path, @@ -205,24 +212,33 @@ const visitReader = async ( } catch (cause: unknown) { failure = { cause }; } finally { - for (const { iterator } of stack) { + for (const { iterator, prefix, reader } of stack.reverse()) { try { await iterator.return?.(); } catch (cause: unknown) { - failure ??= { cause }; + failure = { + cause: ArtifactReaderFailure.withCleanup( + failure === undefined ? cause : failure.cause, + ArtifactReaderFailure.cleanupObservation( + cause, + `JavaScript artifact traversal for ${prefix || reader.format}`, + ), + ), + }; } } for (const owned of ownedReaders.reverse()) { - try { - await owned.close(); - } catch (cause: unknown) { + const resource = `nested ASAR reader for ${owned.path}`; + const cleanup = await context.resources.release({ + kind: "reader", + reader: owned.reader, + resource, + }); + if (cleanup.kind === "failed") { failure = { cause: ArtifactReaderFailure.withCleanup( - failure?.cause ?? cause, - ArtifactReaderFailure.cleanupObservation( - cause, - `nested ASAR reader for ${prefix}`, - ), + failure === undefined ? cleanup.cause : failure.cause, + ArtifactReaderFailure.cleanupObservation(cleanup.cause, resource), ), }; } diff --git a/tests/boundary/filesystem/javascriptArtifactContentIdentity.test.ts b/tests/boundary/filesystem/javascriptArtifactContentIdentity.test.ts index 47e1b5ce48..16eecbb6ca 100644 --- a/tests/boundary/filesystem/javascriptArtifactContentIdentity.test.ts +++ b/tests/boundary/filesystem/javascriptArtifactContentIdentity.test.ts @@ -5,7 +5,7 @@ import { createPackage } from "@electron/asar"; import { describe, expect, it } from "vitest"; import { scanArtifactInventory } from "../../fixtures/artifactInventory.js"; -import { readJavaScriptArtifactFiles } from "../../../src/artifacts/javascript/JavaScriptArtifactFiles.js"; +import { readJavaScriptArtifactFiles } from "../../support/javascriptApplicationScope.js"; import { AsarArtifactReader } from "../../../src/artifacts/AsarArtifactReader.js"; import { DirectoryArtifactReader } from "../../../src/artifacts/DirectoryArtifactReader.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; diff --git a/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts b/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts index 5ecbb0fdad..27b953ef9d 100644 --- a/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts +++ b/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts @@ -6,8 +6,10 @@ import { expect, it } from "vitest"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; -import { readJavaScriptArtifactFiles } from "../../../src/artifacts/javascript/JavaScriptArtifactFiles.js"; -import { reconstructJavaScriptArtifact } from "../../support/javascriptApplicationScope.js"; +import { + reconstructJavaScriptArtifact, + readJavaScriptArtifactFiles, +} from "../../support/javascriptApplicationScope.js"; import { scanArtifactInventory } from "../../fixtures/artifactInventory.js"; import { type ArtifactEntry, diff --git a/tests/boundary/filesystem/javascriptIntegrityPolicy.test.ts b/tests/boundary/filesystem/javascriptIntegrityPolicy.test.ts index 61e120e182..47b77114ac 100644 --- a/tests/boundary/filesystem/javascriptIntegrityPolicy.test.ts +++ b/tests/boundary/filesystem/javascriptIntegrityPolicy.test.ts @@ -13,7 +13,7 @@ import { analyzeJavaScriptApplication } from "../../support/javascriptApplicatio import { javascriptApplicationAnalysisResultSchema } from "../../../src/domain/javascript/javascriptApplicationAnalysis.js"; import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; import { createJavaScriptArtifactReader } from "../../../src/artifacts/javascript/JavaScriptArtifactReader.js"; -import { readJavaScriptArtifactFiles } from "../../../src/artifacts/javascript/JavaScriptArtifactFiles.js"; +import { readJavaScriptArtifactFiles } from "../../support/javascriptApplicationScope.js"; import { scanCanonicalArtifactInventory } from "../../fixtures/artifactInventory.js"; import { createStrippedAsarAddon } from "../../fixtures/strippedAsarAddon.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; diff --git a/tests/boundary/filesystem/javascriptNestedReaderCleanup.test.ts b/tests/boundary/filesystem/javascriptNestedReaderCleanup.test.ts new file mode 100644 index 0000000000..1f86afd179 --- /dev/null +++ b/tests/boundary/filesystem/javascriptNestedReaderCleanup.test.ts @@ -0,0 +1,76 @@ +import { mkdir, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { createPackage } from "@electron/asar"; +import { expect, it, onTestFinished, vi } from "vitest"; + +import { AsarArtifactReader } from "../../../src/artifacts/AsarArtifactReader.js"; +import { ArtifactResourceScope } from "../../../src/artifacts/ArtifactResourceScope.js"; +import { ArtifactReaderFailure } from "../../../src/artifacts/ArtifactReader.js"; +import { reconstructJavaScriptArtifact } from "../../../src/application/javascript/JavaScriptArtifactReconstruction.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +it("retains a failed nested reader through blocked work and scope cleanup", async () => { + const root = await createTestTempDirectory("rea-js-nested-reader-cleanup-"); + const bundle = join(root, "bundle"); + const contents = join(root, "asar-contents"); + const archive = join(bundle, "app.asar"); + await Promise.all([mkdir(bundle), mkdir(contents)]); + await writeFile(join(contents, "package.json"), '{"main":"index.js"}\n'); + await writeFile(join(contents, "index.js"), "module.exports = 1;\n"); + await createPackage(contents, archive); + + const scope = new ArtifactResourceScope(); + const originalClose = AsarArtifactReader.prototype.close; + const closedReaders: AsarArtifactReader[] = []; + const closeFailure = new Error("nested ASAR close failed"); + let blockReadPhaseClose = true; + onTestFinished(async () => { + blockReadPhaseClose = false; + await scope.close().catch(() => undefined); + vi.restoreAllMocks(); + }); + vi.spyOn(AsarArtifactReader.prototype, "close").mockImplementation( + async function (this: AsarArtifactReader) { + closedReaders.push(this); + // Inventory closes its nested reader first. Fail the next, read-phase + // reader close after its native handle remains available for scope retry. + if (this === closedReaders[1] && blockReadPhaseClose) throw closeFailure; + await originalClose.call(this); + }, + ); + + const input = { input_path: bundle }; + const firstFailure = await reconstructJavaScriptArtifact(input, scope).catch( + (cause: unknown) => cause, + ); + expect(firstFailure).toBeInstanceOf(ArtifactReaderFailure); + expect(firstFailure).toMatchObject({ + cleanup: { + reason: closeFailure.message, + resources: [expect.stringContaining("app.asar")], + }, + }); + expect(closedReaders).toHaveLength(2); + const retainedReader = closedReaders[1]; + expect(retainedReader).toBeDefined(); + expect(retainedReader).not.toBe(closedReaders[0]); + + const blockedWork = await reconstructJavaScriptArtifact(input, scope).catch( + (cause: unknown) => cause, + ); + expect(blockedWork).toMatchObject({ + reason: "unavailable", + cleanup: { + reason: closeFailure.message, + resources: [expect.stringContaining("app.asar")], + }, + }); + expect(closedReaders).toHaveLength(3); + expect(closedReaders[2]).toBe(retainedReader); + + blockReadPhaseClose = false; + await scope.close(); + expect(closedReaders).toHaveLength(4); + expect(closedReaders[3]).toBe(retainedReader); +}); diff --git a/tests/composition/javascript-applications/javascriptArtifactReconstruction.test.ts b/tests/composition/javascript-applications/javascriptArtifactReconstruction.test.ts index 846eee6c3c..bb41d5c0b7 100644 --- a/tests/composition/javascript-applications/javascriptArtifactReconstruction.test.ts +++ b/tests/composition/javascript-applications/javascriptArtifactReconstruction.test.ts @@ -11,7 +11,10 @@ import { expect, it } from "vitest"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; -import { reconstructJavaScriptArtifact } from "../../support/javascriptApplicationScope.js"; +import { + reconstructJavaScriptArtifact, + readJavaScriptArtifactFiles, +} from "../../support/javascriptApplicationScope.js"; import { analyzeJavaScriptArtifactFiles, analyzeAndProjectJavaScriptArtifactFiles, @@ -22,7 +25,6 @@ import { createJavaScriptSemanticGraphProjection, } from "../../../src/application/javascript/JavaScriptSemanticGraphBuilder.js"; import { createJavaScriptArtifactReader } from "../../../src/artifacts/javascript/JavaScriptArtifactReader.js"; -import { readJavaScriptArtifactFiles } from "../../../src/artifacts/javascript/JavaScriptArtifactFiles.js"; import { scanCanonicalArtifactInventory, scanArtifactInventory, diff --git a/tests/support/javascriptApplicationScope.ts b/tests/support/javascriptApplicationScope.ts index 830f803681..c9c920f02b 100644 --- a/tests/support/javascriptApplicationScope.ts +++ b/tests/support/javascriptApplicationScope.ts @@ -3,6 +3,19 @@ import type { ExecutionOptions } from "../../src/application/AnalysisProvider.js import { analyzeJavaScriptApplication as analyzeOwned } from "../../src/application/javascript/JavaScriptApplicationService.js"; import { reconstructJavaScriptArtifact as reconstructOwned } from "../../src/application/javascript/JavaScriptArtifactReconstruction.js"; import type { ProgressReporter } from "../../src/application/ProgressReporter.js"; +import { readJavaScriptArtifactFiles as readOwned } from "../../src/artifacts/javascript/JavaScriptArtifactFiles.js"; +import type { ArtifactReader } from "../../src/artifacts/ArtifactReader.js"; +import type { ArtifactInventorySnapshot } from "../../src/domain/artifactInventorySnapshot.js"; + +/** Read a fixture's nested containers with an awaited cleanup owner. */ +export const readJavaScriptArtifactFiles = ( + reader: ArtifactReader, + snapshot: ArtifactInventorySnapshot, + signal?: AbortSignal, +) => + withArtifactResourceScope((scope) => + readOwned(reader, snapshot, scope, signal), + ); /** Run a JavaScript analysis with an awaited, test-owned artifact scope. */ export const analyzeJavaScriptApplication = (