diff --git a/src/browser/WebArtifactCleanup.test.ts b/src/browser/WebArtifactCleanup.test.ts index 34e9328303..9953f62abc 100644 --- a/src/browser/WebArtifactCleanup.test.ts +++ b/src/browser/WebArtifactCleanup.test.ts @@ -1,7 +1,8 @@ import { expect, it, vi } from "vitest"; import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { LocalWebScriptArtifacts } from "./assets/LocalWebScriptArtifacts.js"; -import { closeWebArtifactResources } from "./WebArtifactCleanup.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { WebArtifactCleanup } from "./WebArtifactCleanup.js"; it("closes a shared script/resource scope once", async () => { const resources = new ArtifactResourceScope(); @@ -11,7 +12,11 @@ it("closes a shared script/resource scope once", async () => { resources, ); - await closeWebArtifactResources("web-module-artifacts", resources, scripts); + await new WebArtifactCleanup( + "web-module-artifacts", + resources, + scripts, + ).close(); expect(close).toHaveBeenCalledTimes(1); }); @@ -43,9 +48,8 @@ it.each([ if (scriptCloseCalls === 1) throw new Error("script close refused once"); }); - await expect( - closeWebArtifactResources(owner, resources, scripts), - ).rejects.toMatchObject({ + const cleanup = new WebArtifactCleanup(owner, resources, scripts); + await expect(cleanup.close()).rejects.toMatchObject({ operation, cleanupResources: [resource, "web scripts"], diagnostics: { @@ -64,8 +68,88 @@ it.each([ expect(resourceCloseCalls).toBe(1); expect(scriptCloseCalls).toBe(1); - await closeWebArtifactResources(owner, resources, scripts); + await cleanup.close(); expect(resourceCloseCalls).toBe(2); expect(scriptCloseCalls).toBe(2); }, ); + +it.each(["resources", "scripts"] as const)( + "retries only unresolved %s and resolves prior aggregate errors", + async (failed) => { + const resources = new ArtifactResourceScope(); + const scripts = new LocalWebScriptArtifacts( + "trace_web_module_imports", + new ArtifactResourceScope(), + ); + const resourceClose = vi.spyOn(resources, "close").mockResolvedValue(); + const scriptClose = vi.spyOn(scripts, "close").mockResolvedValue(); + const denied = failed === "resources" ? resourceClose : scriptClose; + denied.mockRejectedValueOnce(new Error("controlled close refusal")); + const cleanup = new WebArtifactCleanup( + "web-module-artifacts", + resources, + scripts, + ); + const first = await cleanup.close().catch((cause: unknown) => cause); + expect(first).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: [ + failed === "resources" ? "web module artifacts" : "web scripts", + ], + }); + await cleanup.close(); + expect(first).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(resourceClose).toHaveBeenCalledTimes(failed === "resources" ? 2 : 1); + expect(scriptClose).toHaveBeenCalledTimes(failed === "scripts" ? 2 : 1); + await cleanup.close(); + expect(denied).toHaveBeenCalledTimes(2); + }, +); + +it("retains exact child labels until every failed owner releases", async () => { + const resources = new ArtifactResourceScope(); + const scripts = new LocalWebScriptArtifacts( + "trace_web_module_imports", + new ArtifactResourceScope(), + ); + const resourceClose = vi.spyOn(resources, "close").mockResolvedValue(); + const scriptClose = vi.spyOn(scripts, "close").mockResolvedValue(); + resourceClose.mockRejectedValueOnce( + new ProviderCleanupError("fixture", ["selected-map-handle"], {}), + ); + scriptClose.mockRejectedValueOnce( + new ProviderCleanupError("fixture", ["captured-script-handle"], {}), + ); + const cleanup = new WebArtifactCleanup( + "web-module-artifacts", + resources, + scripts, + ); + const first = await cleanup.close().catch((cause: unknown) => cause); + expect(first).toMatchObject({ + cleanupResources: ["selected-map-handle", "captured-script-handle"], + }); + scriptClose.mockRejectedValueOnce( + new ProviderCleanupError("fixture", ["captured-script-handle"], {}), + ); + const second = await cleanup.close().catch((cause: unknown) => cause); + expect(first).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["captured-script-handle"], + }); + await cleanup.close(); + expect(first).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(second).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(resourceClose).toHaveBeenCalledTimes(2); + expect(scriptClose).toHaveBeenCalledTimes(3); +}); diff --git a/src/browser/WebArtifactCleanup.ts b/src/browser/WebArtifactCleanup.ts index 50afae851c..7d51e4a755 100644 --- a/src/browser/WebArtifactCleanup.ts +++ b/src/browser/WebArtifactCleanup.ts @@ -1,3 +1,7 @@ +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../artifacts/AnalysisCleanupLease.js"; import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import type { WebScriptArtifactPort } from "../application/WebScriptArtifactPort.js"; @@ -10,43 +14,89 @@ 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 cleanupOwner = (resource: string, close: () => Promise) => { + let retained: readonly string[] = [resource]; + const lease = new AnalysisCleanupLease( + async () => { + try { + await close(); + } catch (cause: unknown) { + retained = + cause instanceof AnalysisError && cause.cleanupResources.length > 0 + ? cause.cleanupResources + : ArtifactReaderFailure.cleanupObservation(cause, resource) + .resources; + throw cause; + } + }, + { + get cleanupResources() { + return retained; + }, + }, ); - const error = webArtifactCleanupFailures(owner, operation, failures); - if (error !== undefined) throw error; + return { resource, lease }; }; -export const webArtifactCleanupFailures = ( +/** Retain each composed artifact owner's release across partial cleanup retries. */ +export class WebArtifactCleanup { + readonly #owners: readonly { + readonly resource: string; + readonly lease: AnalysisCleanupLease; + }[]; + readonly #operation: string; + + constructor( + private readonly owner: WebArtifactOwner, + resources: ArtifactResourceScope, + scripts: WebScriptArtifactPort, + ) { + const sourceLocation = owner === "web-source-location-artifacts"; + this.#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); + this.#owners = [ + cleanupOwner(resource, () => resources.close()), + ...(closeScript === undefined + ? [] + : [cleanupOwner("web scripts", closeScript)]), + ]; + } + + async close(): Promise { + const pending = this.#owners.filter(({ lease }) => !lease.released); + const results = await Promise.allSettled( + pending.map(({ lease }) => lease.release()), + ); + const failures = results.flatMap((result, index) => + result.status === "rejected" + ? [{ cause: result.reason as unknown, ...pending[index]! }] + : [], + ); + const error = webArtifactCleanupFailures( + this.owner, + this.#operation, + failures, + ); + if (error !== undefined) { + new AnalysisCleanupLeaseGroup( + error, + failures.map(({ lease }) => lease), + ); + throw error; + } + } +} + +const webArtifactCleanupFailures = ( owner: string, operation: string, failures: readonly { readonly resource: string; readonly cause: unknown }[], diff --git a/src/browser/assets/WebSourceLocationArtifacts.ts b/src/browser/assets/WebSourceLocationArtifacts.ts index 7be5133ace..93b2657cf5 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 { closeWebArtifactResources } from "../WebArtifactCleanup.js"; +import { WebArtifactCleanup } from "../WebArtifactCleanup.js"; import { LocalWebScriptArtifacts } from "./LocalWebScriptArtifacts.js"; import { decodeWebArtifact, webArtifactReadError } from "./WebArtifactReads.js"; @@ -16,6 +16,7 @@ import { decodeWebArtifact, webArtifactReadError } from "./WebArtifactReads.js"; export class LocalWebSourceLocationArtifacts implements WebSourceLocationArtifactPort { readonly scripts: WebScriptArtifactPort; readonly #resources: ArtifactResourceScope; + readonly #cleanup: WebArtifactCleanup; constructor( scripts?: WebScriptArtifactPort, @@ -27,14 +28,15 @@ export class LocalWebSourceLocationArtifacts implements WebSourceLocationArtifac this.scripts = scripts ?? new LocalWebScriptArtifacts("trace_web_source_location", resources); + this.#cleanup = new WebArtifactCleanup( + "web-source-location-artifacts", + resources, + this.scripts, + ); } async close(): Promise { - return closeWebArtifactResources( - "web-source-location-artifacts", - this.#resources, - this.scripts, - ); + return this.#cleanup.close(); } /** 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 3f9ee086d4..2971589271 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 { closeWebArtifactResources } from "../WebArtifactCleanup.js"; +import { WebArtifactCleanup } from "../WebArtifactCleanup.js"; import type { WebModuleTraceInput } from "../../domain/webModuleTrace.js"; import { LocalWebScriptArtifacts } from "../assets/LocalWebScriptArtifacts.js"; import { @@ -19,6 +19,7 @@ import { export class LocalWebModuleArtifacts implements WebModuleArtifactPort { readonly scripts: WebScriptArtifactPort; readonly #resources: ArtifactResourceScope; + readonly #cleanup: WebArtifactCleanup; constructor( scripts?: WebScriptArtifactPort, @@ -30,14 +31,15 @@ export class LocalWebModuleArtifacts implements WebModuleArtifactPort { this.scripts = scripts ?? new LocalWebScriptArtifacts("trace_web_module_imports", resources); + this.#cleanup = new WebArtifactCleanup( + "web-module-artifacts", + resources, + this.scripts, + ); } async close(): Promise { - return closeWebArtifactResources( - "web-module-artifacts", - this.#resources, - this.scripts, - ); + return this.#cleanup.close(); } /** Read selected source and optional map without website requests. */