diff --git a/src/browser/WebArtifactCleanup.test.ts b/src/browser/WebArtifactCleanup.test.ts new file mode 100644 index 0000000000..34e9328303 --- /dev/null +++ b/src/browser/WebArtifactCleanup.test.ts @@ -0,0 +1,71 @@ +import { expect, it, vi } from "vitest"; +import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; +import { LocalWebScriptArtifacts } from "./assets/LocalWebScriptArtifacts.js"; +import { closeWebArtifactResources } from "./WebArtifactCleanup.js"; + +it("closes a shared script/resource scope once", async () => { + const resources = new ArtifactResourceScope(); + const close = vi.spyOn(resources, "close").mockResolvedValue(); + const scripts = new LocalWebScriptArtifacts( + "trace_web_module_imports", + resources, + ); + + await closeWebArtifactResources("web-module-artifacts", resources, scripts); + + expect(close).toHaveBeenCalledTimes(1); +}); + +it.each([ + ["web-module-artifacts", "trace_web_module_imports", "web module artifacts"], + [ + "web-source-location-artifacts", + "trace_web_source_location", + "web source-location artifacts", + ], +] as const)( + "attempts both cleanup owners and retries them for %s", + async (owner, operation, resource) => { + const resources = new ArtifactResourceScope(); + let resourceCloseCalls = 0; + resources.addCloseHook(async () => { + resourceCloseCalls++; + if (resourceCloseCalls === 1) + throw new Error("artifact close refused once"); + }); + const scripts = new LocalWebScriptArtifacts( + "trace_web_module_imports", + new ArtifactResourceScope(), + ); + let scriptCloseCalls = 0; + vi.spyOn(scripts, "close").mockImplementation(async () => { + scriptCloseCalls++; + if (scriptCloseCalls === 1) throw new Error("script close refused once"); + }); + + await expect( + closeWebArtifactResources(owner, resources, scripts), + ).rejects.toMatchObject({ + operation, + cleanupResources: [resource, "web scripts"], + diagnostics: { + failures: [ + { + message: "artifact close refused once", + resource: [resource], + }, + { + message: "script close refused once", + resource: ["web scripts"], + }, + ], + }, + }); + expect(resourceCloseCalls).toBe(1); + expect(scriptCloseCalls).toBe(1); + + await closeWebArtifactResources(owner, resources, scripts); + expect(resourceCloseCalls).toBe(2); + expect(scriptCloseCalls).toBe(2); + }, +); diff --git a/src/browser/WebArtifactCleanup.ts b/src/browser/WebArtifactCleanup.ts index d5243aea8c..50afae851c 100644 --- a/src/browser/WebArtifactCleanup.ts +++ b/src/browser/WebArtifactCleanup.ts @@ -1,8 +1,51 @@ import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; +import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; +import type { WebScriptArtifactPort } from "../application/WebScriptArtifactPort.js"; +import { LocalWebScriptArtifacts } from "./assets/LocalWebScriptArtifacts.js"; import { AnalysisError } from "../domain/analysisErrorBase.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +type WebArtifactOwner = + | "web-source-location-artifacts" + | "web-module-artifacts"; + +/** Close a composed artifact reader without closing a shared script scope twice. */ +export const closeWebArtifactResources = async ( + owner: WebArtifactOwner, + resources: ArtifactResourceScope, + scripts: WebScriptArtifactPort, +): Promise => { + const sourceLocation = owner === "web-source-location-artifacts"; + const operation = sourceLocation + ? "trace_web_source_location" + : "trace_web_module_imports"; + const resource = sourceLocation + ? "web source-location artifacts" + : "web module artifacts"; + const closeScript = + scripts instanceof LocalWebScriptArtifacts && + scripts.resources === resources + ? undefined + : scripts.close?.bind(scripts); + const results = await Promise.allSettled([ + Promise.resolve().then(() => resources.close()), + ...(closeScript === undefined ? [] : [Promise.resolve().then(closeScript)]), + ]); + const failures = results.flatMap((result, index) => + result.status === "rejected" + ? [ + { + cause: result.reason, + resource: index === 0 ? resource : "web scripts", + }, + ] + : [], + ); + const error = webArtifactCleanupFailures(owner, operation, failures); + if (error !== undefined) throw error; +}; + export const webArtifactCleanupFailures = ( owner: string, operation: string, diff --git a/src/browser/assets/WebSourceLocationArtifacts.ts b/src/browser/assets/WebSourceLocationArtifacts.ts index 24c4d22ddf..f15503d07b 100644 --- a/src/browser/assets/WebSourceLocationArtifacts.ts +++ b/src/browser/assets/WebSourceLocationArtifacts.ts @@ -8,7 +8,7 @@ import { type WebSourceLocationInput, } from "../../domain/webSourceLocation.js"; import { err, ok } from "../../domain/result.js"; -import { webArtifactCleanupFailures } from "../WebArtifactCleanup.js"; +import { closeWebArtifactResources } from "../WebArtifactCleanup.js"; import { LocalWebScriptArtifacts } from "./LocalWebScriptArtifacts.js"; import { decodeWebArtifact, webArtifactReadError } from "./WebArtifactReads.js"; @@ -30,34 +30,11 @@ export class LocalWebSourceLocationArtifacts implements WebSourceLocationArtifac } async close(): Promise { - const closeScript = - this.scripts instanceof LocalWebScriptArtifacts && - this.scripts.resources === this.#resources - ? undefined - : this.scripts.close?.bind(this.scripts); - const results = await Promise.allSettled([ - Promise.resolve().then(() => this.#resources.close()), - ...(closeScript === undefined - ? [] - : [Promise.resolve().then(closeScript)]), - ]); - const failures = results.flatMap((result, index) => - result.status === "rejected" - ? [ - { - cause: result.reason, - resource: - index === 0 ? "web source-location artifacts" : "web scripts", - }, - ] - : [], - ); - const error = webArtifactCleanupFailures( + return closeWebArtifactResources( "web-source-location-artifacts", - "trace_web_source_location", - failures, + this.#resources, + this.scripts, ); - if (error !== undefined) throw error; } /** Preserve actual map identity separately from the selected deployment URL. */ diff --git a/src/browser/modules/WebModuleArtifacts.ts b/src/browser/modules/WebModuleArtifacts.ts index 61cd261e9f..9c9b72e90d 100644 --- a/src/browser/modules/WebModuleArtifacts.ts +++ b/src/browser/modules/WebModuleArtifacts.ts @@ -7,7 +7,7 @@ import type { ExecutionOptions } from "../../application/AnalysisProvider.js"; import { ArtifactResourceScope } from "../../artifacts/ArtifactResourceScope.js"; import { jsonObjectSchema } from "../../domain/jsonValue.js"; import { err, ok } from "../../domain/result.js"; -import { webArtifactCleanupFailures } from "../WebArtifactCleanup.js"; +import { closeWebArtifactResources } from "../WebArtifactCleanup.js"; import type { WebModuleTraceInput } from "../../domain/webModuleTrace.js"; import { LocalWebScriptArtifacts } from "../assets/LocalWebScriptArtifacts.js"; import { @@ -33,33 +33,11 @@ export class LocalWebModuleArtifacts implements WebModuleArtifactPort { } async close(): Promise { - const closeScript = - this.scripts instanceof LocalWebScriptArtifacts && - this.scripts.resources === this.#resources - ? undefined - : this.scripts.close?.bind(this.scripts); - const results = await Promise.allSettled([ - Promise.resolve().then(() => this.#resources.close()), - ...(closeScript === undefined - ? [] - : [Promise.resolve().then(closeScript)]), - ]); - const failures = results.flatMap((result, index) => - result.status === "rejected" - ? [ - { - cause: result.reason, - resource: index === 0 ? "web module artifacts" : "web scripts", - }, - ] - : [], - ); - const error = webArtifactCleanupFailures( + return closeWebArtifactResources( "web-module-artifacts", - "trace_web_module_imports", - failures, + this.#resources, + this.scripts, ); - if (error !== undefined) throw error; } /** Read selected source and optional map without website requests. */