From 684199ba0be7822266d57ddb8a40e95f087fdc42 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 11 Oct 2026 05:33:15 +0800 Subject: [PATCH] fix(javascript)!: retain nested artifact readers in caller scopes Require the caller artifact resource scope when reading JavaScript artifacts. Transfer failed nested ASAR cleanup to that scope so later work retries the same owner before admission. Preserve traversal failures and attempt every remaining iterator during unwind. Validation: reconstruction, integrity, content identity, cancellation and nested-reader cleanup boundary checks. BREAKING CHANGE: readJavaScriptArtifactFiles now requires ArtifactResourceScope as its third argument; the optional abort signal is fourth. --- .../JavaScriptArtifactReconstruction.ts | 7 +- .../javascript/JavaScriptArtifactFiles.ts | 40 +++++++--- .../javascriptArtifactContentIdentity.test.ts | 2 +- .../javascriptArtifactReconstruction.test.ts | 6 +- .../javascriptIntegrityPolicy.test.ts | 2 +- .../javascriptNestedReaderCleanup.test.ts | 76 +++++++++++++++++++ .../javascriptArtifactReconstruction.test.ts | 6 +- tests/support/javascriptApplicationScope.ts | 13 ++++ 8 files changed, 133 insertions(+), 19 deletions(-) create mode 100644 tests/boundary/filesystem/javascriptNestedReaderCleanup.test.ts 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 = (