diff --git a/src/artifacts/ArtifactResourceScope.test.ts b/src/artifacts/ArtifactResourceScope.test.ts index 417e8ee6e7..40be9c20bc 100644 --- a/src/artifacts/ArtifactResourceScope.test.ts +++ b/src/artifacts/ArtifactResourceScope.test.ts @@ -9,7 +9,7 @@ import { ArtifactResourceRetryFailure } from "./ArtifactResourceScope.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { expect, it } from "vitest"; -import { err } from "../domain/result.js"; +import { err, ok } from "../domain/result.js"; import { AnalysisOutputError } from "../domain/analysisErrorCore.js"; import { ArtifactReaderFailure, @@ -138,6 +138,155 @@ it("runs later close hooks when an earlier hook fails", async () => { expect(order).toEqual(["first", "second"]); }); +it("rejects a close hook's undefined reason and retries through the same scope", async () => { + const scope = new ArtifactResourceScope(); + let denied = true; + let failedHookRuns = 0; + let laterHookRuns = 0; + scope.addCloseHook(async () => { + failedHookRuns += 1; + if (denied) throw undefined; + }); + scope.addCloseHook(async () => { + laterHookRuns += 1; + }); + + try { + const close = scope.close(); + expect(scope.close()).toBe(close); + await expect(close).rejects.toBeUndefined(); + expect(failedHookRuns).toBe(1); + expect(laterHookRuns).toBe(1); + + denied = false; + await scope.close(); + expect(failedHookRuns).toBe(2); + expect(laterHookRuns).toBe(2); + } finally { + denied = false; + await scope.close(); + } +}); + +it.each([ + { name: "null", reason: null }, + { name: "false", reason: false }, + { name: "zero", reason: 0 }, + { name: "empty string", reason: "" }, + { name: "Error", reason: new Error("single hook failure") }, +])("preserves a close hook's exact $name rejection", async ({ reason }) => { + const scope = new ArtifactResourceScope(); + let denied = true; + scope.addCloseHook(async () => { + if (denied) throw reason; + }); + try { + await expect(scope.close()).rejects.toBe(reason); + } finally { + denied = false; + await scope.close(); + } +}); + +it("preserves undefined alongside other close hook failures", async () => { + const scope = new ArtifactResourceScope(); + const failure = new Error("third hook failed"); + const reasons = [undefined, null, failure]; + let denied = true; + const attempted: unknown[] = []; + for (const reason of reasons) + scope.addCloseHook(async () => { + attempted.push(reason); + if (denied) throw reason; + }); + + try { + const outcome = await scope.close().then( + () => ok(undefined), + (error: unknown) => err(error), + ); + expect(outcome.ok).toBe(false); + if (outcome.ok || !(outcome.error instanceof AggregateError)) + throw new Error("Expected all hook failures"); + expect(outcome.error.errors).toEqual(reasons); + expect(outcome.error.errors[2]).toBe(failure); + expect(attempted).toEqual(reasons); + } finally { + denied = false; + await scope.close(); + } +}); + +it("retains both a failed owner and an undefined hook failure until each recovers", async () => { + const scope = new ArtifactResourceScope(); + const ownerFailure = new Error("lease still owned"); + let ownerDenied = true; + let hookDenied = true; + let releases = 0; + let hookRuns = 0; + const owner = { + kind: "lease" as const, + resource: "retained lease", + lease: { + async release() { + releases += 1; + if (ownerDenied) throw ownerFailure; + }, + }, + }; + scope.addCloseHook(async () => { + hookRuns += 1; + if (hookDenied) throw undefined; + }); + + try { + const released = await scope.release(owner); + expect(released).toMatchObject({ kind: "failed", cause: ownerFailure }); + const outcome = await scope.close().then( + () => ok(undefined), + (error: unknown) => err(error), + ); + expect(outcome.ok).toBe(false); + if (outcome.ok || !(outcome.error instanceof ArtifactResourceRetryFailure)) + throw new Error("Expected both retained cleanup failures"); + expect(outcome.error.failedOwners).toEqual([owner]); + expect(outcome.error.failedOwners[0]).toBe(owner); + expect(outcome.error.cleanup).toEqual({ + reason: "lease still owned; undefined", + resources: ["retained lease", "close hook"], + }); + const combined = outcome.error.cause; + expect(combined).toBeInstanceOf(AggregateError); + if (!(combined instanceof AggregateError)) + throw new Error("Expected retained cleanup and hook causes"); + expect(combined.errors).toHaveLength(2); + expect(combined.errors[1]).toBeUndefined(); + expect(combined.errors[0]).toMatchObject({ + failedOwners: [owner], + cause: { errors: [ownerFailure] }, + }); + expect(scope.hasPending).toBe(true); + expect(releases).toBe(2); + expect(hookRuns).toBe(1); + + ownerDenied = false; + await expect(scope.close()).rejects.toBeUndefined(); + expect(scope.hasPending).toBe(false); + expect(scope.cleanupResources).toEqual([]); + expect(releases).toBe(3); + expect(hookRuns).toBe(2); + + hookDenied = false; + await scope.close(); + expect(releases).toBe(3); + expect(hookRuns).toBe(3); + } finally { + ownerDenied = false; + hookDenied = false; + await scope.close(); + } +}); + it("retains the same failed reader owner in a retry failure and retries before admitting work", async () => { const scope = new ArtifactResourceScope(); let closeAttempts = 0; diff --git a/src/artifacts/ArtifactResourceScope.ts b/src/artifacts/ArtifactResourceScope.ts index 1b2848e76a..723022c372 100644 --- a/src/artifacts/ArtifactResourceScope.ts +++ b/src/artifacts/ArtifactResourceScope.ts @@ -1,4 +1,5 @@ import type { AnalysisPartialObservation } from "../domain/analysisErrorBase.js"; +import { err, ok, type Result } from "../domain/result.js"; import type { JavaScriptSourceAnalysisPort } from "../domain/javascript/javascriptSourceAnalysis.js"; import type { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; import type { OwnedDirectoryHandle } from "../filesystem/OwnedDirectoryHandle.js"; @@ -228,7 +229,7 @@ export class ArtifactResourceScope { } /** Every registered hook runs. One failure is returned unchanged; several are aggregated. */ - async #runCloseHooks(): Promise { + async #runCloseHooks(): Promise> { const failures: unknown[] = []; for (const hook of this.#closeHooks) { try { @@ -237,15 +238,17 @@ export class ArtifactResourceScope { failures.push(cause); } } - if (failures.length === 0) return undefined; - if (failures.length === 1) return failures[0]; - return new AggregateError( - failures, - failures - .map((cause) => - cause instanceof Error ? cause.message : String(cause), - ) - .join("; "), + if (failures.length === 0) return ok(undefined); + if (failures.length === 1) return err(failures[0]); + return err( + new AggregateError( + failures, + failures + .map((cause) => + cause instanceof Error ? cause.message : String(cause), + ) + .join("; "), + ), ); } @@ -253,14 +256,14 @@ export class ArtifactResourceScope { // A parked analyzer is not a pending owner. Stop it even when another // retained cleanup is still failing. A later close retries both. const cleanup = await this.retryCleanup().then( - () => undefined, - (cause: unknown) => cause, + () => ok(undefined), + (cause: unknown) => err(cause), ); - const hookFailure = await this.#runCloseHooks(); - if (cleanup !== undefined && hookFailure !== undefined) - throw this.#composeCloseFailures(cleanup, hookFailure); - if (hookFailure !== undefined) throw hookFailure; - if (cleanup !== undefined) throw cleanup; + const hooks = await this.#runCloseHooks(); + if (!cleanup.ok && !hooks.ok) + throw this.#composeCloseFailures(cleanup.error, hooks.error); + if (!hooks.ok) throw hooks.error; + if (!cleanup.ok) throw cleanup.error; } /**