From 98ec00f4068bab894639d0d1d83ac0bfc428bcbf Mon Sep 17 00:00:00 2001 From: Dery Ferdika <114888155+DeryFerd@users.noreply.github.com> Date: Sat, 24 Oct 2026 14:08:10 +0700 Subject: [PATCH] fix(setup): preserve cleanup ownership and resource reports (#6920) * fix(setup): report host-owned cleanup resources alongside the primary's SetupHost.close documents that it reports the resources a setup host still owns, but its contract returned only a string. runSetup therefore built the ProviderCleanupError from the primary failure's cleanupResources alone, so a resource the host failed to release reached the projection only inside a free-text reason while the error asks the caller to review the reported local resources. Return a SetupCloseReport instead. closeSetupResources absorbs the structured resources of any cause that reports them, through ArtifactReaderFailure.cleanupObservation, and leaves opaque failures message-only so no resource name is invented. runSetup then unions the host's resources with the primary's before building the typed cleanup error. Two boundary tests read the close result directly: the skill-cleanup case now asserts the retained backup path in the report reason rather than in a bare string, and the shared ownership helper takes an opaque close result because it only ever awaits undefined from either host it is given. Validation: RED then green in src/application/Setup.recovery.test.ts, where the new case previously received only the primary resource; 116 tests across the seven Setup and cleanup-ownership suites pass; tsc --noEmit over the changed set, its setup callers, and both boundary tests exits 0; oxlint reports 0 errors; oxfmt --check is clean; verify:module-boundaries passes. The two failures in src/setupEnvironment.test.ts reproduce identically on pristine bded9c628 with this change stashed. * fix(setup): preserve cleanup ownership through host reports * fix(setup): bind finalization reports to cleanup owners * fix(setup): retain live resources in returned cleanup views * fix(setup): preserve completed cleanup and exact installer resources * fix(setup): retain delegated cleanup across authority changes * fix(setup): compose callback and source cleanup authorities * fix(setup): retain producer cleanup failure reasons * fix(cli): render structured setup cleanup resources * fix(setup): separate failure evidence from cleanup ownership * fix(setup): normalize close failures and unify lease binding * fix(setup): preserve direct lease resource attribution --------- Co-authored-by: gensokyo-dev <342580529+gensokyo-dev@users.noreply.github.com> --- docs/installation.md | 12 + src/application/ClientConfigurationCleanup.ts | 8 +- src/application/LinuxHopper.test.ts | 5 +- src/application/LinuxHopper.ts | 10 +- src/application/MacHopper.test.ts | 30 +- src/application/MacHopper.ts | 49 +- src/application/RegularFileRead.ts | 5 + src/application/Setup.recovery.test.ts | 251 +++++++++- src/application/Setup.ts | 56 ++- src/application/SetupCleanupOwner.test.ts | 439 ++++++++++++++++++ src/application/SetupCleanupOwner.ts | 254 ++++++++++ src/application/SetupHost.ts | 226 +++++---- src/application/SetupTypes.ts | 12 +- src/artifacts/AnalysisCleanupLease.test.ts | 23 + src/artifacts/AnalysisCleanupLease.ts | 28 +- src/cli/interactiveSetup.ts | 2 + src/domain/analysisErrorBase.ts | 16 + tests/boundary/cli/cliSetup.test.ts | 38 ++ .../macHopperInstallOwnership.test.ts | 49 +- .../setupClientConfigCleanupOwnership.test.ts | 10 +- .../setupLinuxCleanupOwnership.test.ts | 132 ++++++ .../setupSkillCleanupOwnership.test.ts | 62 ++- .../setupConfigurationRollback.test.ts | 14 +- 23 files changed, 1543 insertions(+), 188 deletions(-) create mode 100644 src/application/SetupCleanupOwner.test.ts create mode 100644 src/application/SetupCleanupOwner.ts create mode 100644 tests/boundary/filesystem/setupLinuxCleanupOwnership.test.ts diff --git a/docs/installation.md b/docs/installation.md index 3881ba13eb..a3f60e6bb3 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -745,6 +745,18 @@ may conflict with Hopper's versioned `zlib` dependency. Resolve the package conflict yourself, install Hopper's official package manually, or use another provider such as Ghidra; rerun `rea setup` after installing Hopper. +If setup cannot finish releasing its resources, a normally returned structured +result reports `needs_human` and includes `cleanup.reason` with the identified +paths in `cleanup.resources`. A thrown typed failure reports the same retained +paths in `details.resources`, preserving the primary failure separately. Reasons +that do not identify a resource remain explicit without guessing paths from text. +For in-process callers retaining REA's setup host, a successful retry of its close +updates the original failure's cleanup state and removes released resource labels. +Custom `SetupHost` implementations must bind returned or thrown cleanup errors +to their resource owners to provide the same live recovery. An ownerless error +remains an unconfirmed snapshot: `runSetup` does not observe later direct calls +to the caller's port or replace that port's methods. + The supported Linux demo forwards additional launches into its existing application, even across private displays. REA therefore reserves one Linux Hopper application per user across cooperating REA processes. A competing diff --git a/src/application/ClientConfigurationCleanup.ts b/src/application/ClientConfigurationCleanup.ts index 09e7ca4f4e..cbcfe9315d 100644 --- a/src/application/ClientConfigurationCleanup.ts +++ b/src/application/ClientConfigurationCleanup.ts @@ -60,9 +60,11 @@ export const retryClientConfigurationCleanup = async ( ): Promise<{ readonly primary: unknown; readonly failures: readonly RegularFileCleanupFailure[]; + readonly cleanupLeases: readonly AnalysisCleanupLease[]; readonly cleanup?: AnalysisCleanupObservation; }> => { const failures: RegularFileCleanupFailure[] = []; + const cleanupLeases = new Set(); const observations: AnalysisCleanupObservation[] = []; let current = failure; let primary: unknown; @@ -71,6 +73,7 @@ export const retryClientConfigurationCleanup = async ( if (current instanceof RegularFileCleanupFailure) { const retried = await retryRegularFileCleanupFailures(current, resources); failures.push(...retried.failures); + for (const lease of retried.cleanupLeases) cleanupLeases.add(lease); if (retried.cleanup !== undefined) observations.push(retried.cleanup); primary = retried.primary; const last = retried.failures.at(-1)!; @@ -84,13 +87,15 @@ export const retryClientConfigurationCleanup = async ( resource: current.owner.resource, lease: current.cleanupLease, }); - if (released.kind === "failed") + if (released.kind === "failed") { + cleanupLeases.add(released.lease); observations.push( ArtifactReaderFailure.cleanupObservation( released.cause, current.owner.resource, ), ); + } primary = current.outcome.kind === "failed" ? current.outcome.cause @@ -104,6 +109,7 @@ export const retryClientConfigurationCleanup = async ( return { primary, failures, + cleanupLeases: [...cleanupLeases], ...(observations.length === 0 ? {} : { diff --git a/src/application/LinuxHopper.test.ts b/src/application/LinuxHopper.test.ts index d9f056455e..b43d048c9c 100644 --- a/src/application/LinuxHopper.test.ts +++ b/src/application/LinuxHopper.test.ts @@ -504,7 +504,10 @@ describe("Linux Hopper temporary cleanup ownership", () => { attempts += 1; throw new Error("still busy"); }), - ).toContain("/tmp/rea-hopper"); + ).toMatchObject({ + reason: expect.stringContaining("/tmp/rea-hopper"), + resources: ["/tmp/rea-hopper"], + }); expect( await owner.retry(async () => { attempts += 1; diff --git a/src/application/LinuxHopper.ts b/src/application/LinuxHopper.ts index 5de3a4d923..a401a4c13a 100644 --- a/src/application/LinuxHopper.ts +++ b/src/application/LinuxHopper.ts @@ -1,3 +1,4 @@ +import type { AnalysisCleanupObservation } from "../domain/analysisErrorBase.js"; import type { HopperInstallFailureReason } from "./SetupInstallFailure.js"; import { access, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -268,7 +269,7 @@ export class LinuxHopperTemporaryCleanupOwner { async retry( cleanup: (path: string) => Promise, - ): Promise { + ): Promise { const failures: string[] = []; for (const path of this.pending.keys()) { try { @@ -281,7 +282,12 @@ export class LinuxHopperTemporaryCleanupOwner { ); } } - return failures.length === 0 ? undefined : failures.join("; "); + return failures.length === 0 + ? undefined + : { + reason: failures.join("; "), + resources: [...this.pending.keys()], + }; } } diff --git a/src/application/MacHopper.test.ts b/src/application/MacHopper.test.ts index fe056bd0a2..2eef63ed87 100644 --- a/src/application/MacHopper.test.ts +++ b/src/application/MacHopper.test.ts @@ -4,7 +4,6 @@ import { describe, expect, it } from "vitest"; import { installMacHopper, - installMacHopperBundle, type MacHopperBundleVerification, type MacHopperBundleInstallResult, type MacHopperInstallHost, @@ -233,3 +232,32 @@ it("reports an unresolved destination probe without claiming an existing install expect(host.installs).toBe(0); expect(host.unmounts).toBe(0); }); + +it("retains the exact DMG temporary root with its retry capability", async () => { + const host = new FakeMacHopperHost(); + const retained: { + close: () => Promise; + resources: readonly string[]; + }[] = []; + let allowCleanup = false; + const result = await installMacHopper( + {}, + { + ...host, + cleanup: async () => { + if (!allowCleanup) throw new Error("DMG root busy"); + }, + retainCleanup: (close, resources) => { + retained.push({ close, resources }); + }, + }, + ); + expect(result).toMatchObject({ + status: "installed", + cleanupFailure: expect.stringContaining("DMG root busy"), + }); + expect(retained).toHaveLength(1); + expect(retained[0]?.resources).toEqual(["/tmp/rea-hopper-test"]); + allowCleanup = true; + await expect(retained[0]!.close()).resolves.toBeUndefined(); +}); diff --git a/src/application/MacHopper.ts b/src/application/MacHopper.ts index af3ecdfd5e..a38276aaaf 100644 --- a/src/application/MacHopper.ts +++ b/src/application/MacHopper.ts @@ -15,6 +15,7 @@ import { promisify } from "node:util"; import { z } from "zod"; import { homeDirectoryFromEnvironment } from "../config/homeDirectory.js"; +import type { AnalysisCleanupObservation } from "../domain/analysisErrorBase.js"; import { HOPPER_PUBLISHER } from "../domain/hopperPublisher.js"; import { macDeveloperIdRequirement } from "../process/MacCodeSignature.js"; import { macHopperUserApplicationPath } from "./MacHopperPaths.js"; @@ -120,7 +121,10 @@ export interface MacHopperInstallHost { openApplication(path: string): Promise; cleanup(path: string): Promise; /** Retain the cleanup capability until the owning setup host retries it. */ - retainCleanup?(cleanup: () => Promise): void; + retainCleanup?( + cleanup: () => Promise, + resources: readonly string[], + ): void; destination(): string; } @@ -228,7 +232,7 @@ export const installMacHopper = async ( await cleanup(); } catch (cause: unknown) { cleanupFailure = `Hopper DMG cleanup failed: ${failureMessage(cause)}`; - host.retainCleanup?.(cleanup); + host.retainCleanup?.(cleanup, [temporaryPath]); } } @@ -250,7 +254,10 @@ export const installMacHopperBundle = async ( destination: string, replaceExisting: boolean, operations: MacHopperBundleInstallOperations, - retainCleanup: (cleanup: () => Promise) => void, + retainCleanup: ( + cleanup: () => Promise, + resources: readonly string[], + ) => void, ): Promise => { const stagingRoot = await operations.createStagingRoot( posix.dirname(destination), @@ -297,7 +304,7 @@ export const installMacHopperBundle = async ( originalBackedUp = false; } await operations.removeStagingRoot(stagingRoot); - }); + }, [stagingRoot]); return { status: "failed", reason: failureReason, @@ -310,7 +317,10 @@ export const installMacHopperBundle = async ( await operations.removeStagingRoot(stagingRoot); } catch (cleanupCause: unknown) { const cleanupFailure = `Hopper staging root cleanup failed: ${failureMessage(cleanupCause)}`; - retainCleanup(() => operations.removeStagingRoot(stagingRoot)); + retainCleanup( + () => operations.removeStagingRoot(stagingRoot), + [stagingRoot], + ); diagnostics.push(cleanupFailure); return { status: "failed", @@ -331,7 +341,10 @@ export const installMacHopperBundle = async ( return { status: "installed" }; } catch (cause: unknown) { const cleanupFailure = `Hopper staging root cleanup failed after installation committed: ${failureMessage(cause)}`; - retainCleanup(() => operations.removeStagingRoot(stagingRoot)); + retainCleanup( + () => operations.removeStagingRoot(stagingRoot), + [stagingRoot], + ); return { status: "installed", cleanupFailure }; } }; @@ -340,8 +353,10 @@ export const installMacHopperBundle = async ( export const systemMacHopperInstallHost = ( homeDirectory: string, environment: Readonly, -): MacHopperInstallHost & { close(): Promise } => { - const pendingCleanup = new Set<() => Promise>(); +): MacHopperInstallHost & { + close(): Promise; +} => { + const pendingCleanup = new Map<() => Promise, readonly string[]>(); return { download: downloadHopperPackage, createTemporaryDirectory: () => mkdtemp(join(tmpdir(), "rea-hopper-mac-")), @@ -427,24 +442,30 @@ export const systemMacHopperInstallHost = ( rename, removeStagingRoot: (path) => rm(path, { recursive: true }), }, - (cleanup) => pendingCleanup.add(cleanup), + (cleanup, resources) => pendingCleanup.set(cleanup, resources), ), async openApplication(path) { await execFileAsync("open", [path], { env: environment }); }, cleanup: (path) => rm(path, { recursive: true, force: true }), - retainCleanup: (cleanup) => pendingCleanup.add(cleanup), + retainCleanup: (cleanup, resources) => + pendingCleanup.set(cleanup, resources), async close() { - let failure: string | undefined; - for (const cleanup of pendingCleanup) { + const failures: string[] = []; + for (const cleanup of pendingCleanup.keys()) { try { await cleanup(); pendingCleanup.delete(cleanup); } catch (cause: unknown) { - failure ??= `Hopper installer cleanup remains incomplete: ${failureMessage(cause)}`; + failures.push(failureMessage(cause)); } } - return failure; + return failures.length === 0 + ? undefined + : { + reason: `Hopper installer cleanup remains incomplete: ${failures.join("; ")}`, + resources: [...new Set([...pendingCleanup.values()].flat())], + }; }, destination: () => macHopperUserApplicationPath(homeDirectory), }; diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts index 7921353ac4..fba17dce50 100644 --- a/src/application/RegularFileRead.ts +++ b/src/application/RegularFileRead.ts @@ -140,14 +140,18 @@ export const retryRegularFileCleanupFailures = async ( ): Promise<{ readonly primary: unknown; readonly failures: readonly RegularFileCleanupFailure[]; + readonly cleanupLeases: readonly AnalysisCleanupLease[]; readonly cleanup?: AnalysisCleanupObservation; }> => { const failures: RegularFileCleanupFailure[] = []; + const cleanupLeases = new Set(); const observations: AnalysisCleanupObservation[] = []; let current = failure; let primary: unknown; for (;;) { const retried = await retryRegularFileCleanup(current, resources); + if (retried.cleanupLease !== undefined) + cleanupLeases.add(retried.cleanupLease); failures.push(retried.failure ?? current); if (retried.cleanup !== undefined) observations.push(retried.cleanup); primary = @@ -165,6 +169,7 @@ export const retryRegularFileCleanupFailures = async ( return { primary, failures, + cleanupLeases: [...cleanupLeases], ...(observations.length === 0 ? {} : { diff --git a/src/application/Setup.recovery.test.ts b/src/application/Setup.recovery.test.ts index dc5bfd6b5c..2891bb7663 100644 --- a/src/application/Setup.recovery.test.ts +++ b/src/application/Setup.recovery.test.ts @@ -11,6 +11,8 @@ import { type LinuxDistribution, } from "./LinuxHopper.js"; import type { SetupProgressEvent } from "./SetupTypes.js"; +import { setupCleanupError, SetupCleanupOwner } from "./SetupCleanupOwner.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; describe("setup workflow", () => { it("rejects Node 25 before applying setup actions", async () => { @@ -182,8 +184,11 @@ describe("setup failure cleanup projection", () => { }, }); class SetupHostWithCleanupFailure extends FakeSetupHost { - close = async (): Promise => - "Setup could not release its client configuration resources."; + close = async () => + setupCleanupError(undefined, { + reason: "Setup could not release its client configuration resources.", + resources: [], + }); } const host = new SetupHostWithCleanupFailure(); host.clients = [{ name: "codex", configPath }]; @@ -215,7 +220,7 @@ describe("setup failure cleanup projection", () => { }, cleanup_error: { code: "cleanup_incomplete", - details: { resources: [configPath] }, + details: { resources: [] }, }, }, }, @@ -234,6 +239,175 @@ describe("setup failure cleanup projection", () => { }, }); }); + + it.each(["report", "throw"] as const)( + "reports host-owned cleanup resources alongside the primary's: %s", + async (outcome) => { + const configPath = "/fixture/home/vscode/settings.json"; + const retainedPath = "/fixture/home/retained/handle.bin"; + const primary = new ConfigurationError( + "Client config could not be read", + { + settings: [ + { + setting: configPath, + constraint: "The selected config could not be read.", + }, + ], + cleanup: { + reason: "The admitted config handle remains open.", + resources: [configPath], + }, + }, + ); + class HostWithRetainedResource extends FakeSetupHost { + close = async () => { + const reason = `Artifact cleanup remains incomplete: ${retainedPath}`; + if (outcome === "throw") + throw new ProviderCleanupError("setup", [retainedPath], { reason }); + return setupCleanupError(undefined, { + reason, + resources: [retainedPath], + }); + }; + } + const host = new HostWithRetainedResource(); + host.clients = [{ name: "codex", configPath }]; + host.inspectClientConfiguration = async () => { + throw primary; + }; + + const failure = await runSetup( + { ...options(true), clientIds: ["codex"] }, + host, + ).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toBeInstanceOf(AnalysisError); + if (!(failure instanceof AnalysisError)) + throw new Error("Expected setup to preserve a typed analysis failure"); + + const projected = projectAnalysisError(failure); + // The host still owns a resource the primary failure never saw; the + // projection must list it, not bury it in a free-text reason. + expect(projected).toMatchObject({ + code: "cleanup_incomplete", + details: { + resources: [configPath, retainedPath], + diagnostics: { + primary_error: { details: { resources: [configPath] } }, + cleanup_error: { details: { resources: [retainedPath] } }, + }, + }, + }); + }, + ); +}); + +describe("setup opaque failure cleanup", () => { + it("retains the typed host cleanup report alongside an opaque primary", async () => { + const primary = new Error("Controlled setup workflow failure"); + const retainedPath = "/fixture/home/retained/handle.bin"; + class HostWithRetainedResource extends FakeSetupHost { + close = async () => + setupCleanupError(undefined, { + reason: "Controlled host close failure", + resources: [retainedPath], + }); + } + const host = new HostWithRetainedResource(); + host.initialSetupState = async () => { + throw primary; + }; + const failure = await runSetup(options(true), host).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toBeInstanceOf(AggregateError); + if (!(failure instanceof AggregateError)) + throw new Error("Expected setup and cleanup failures to remain together"); + expect(failure.cause).toBe(primary); + expect(failure.errors[0]).toBe(primary); + const cleanup: unknown = failure.errors[1]; + expect(cleanup).toBeInstanceOf(ProviderCleanupError); + if (!(cleanup instanceof ProviderCleanupError)) + throw new Error("Expected typed cleanup facts in the aggregate"); + expect(projectAnalysisError(cleanup)).toMatchObject({ + code: "cleanup_incomplete", + details: { resources: [retainedPath] }, + }); + }); +}); + +describe("setup returned result cleanup", () => { + it.each([ + { state: "ready", outcome: "return" }, + { state: "ready", outcome: "throw" }, + { state: "needs_human", outcome: "return" }, + { state: "needs_human", outcome: "throw" }, + ] as const)( + "preserves $state result facts and reports retained resources: $outcome", + async ({ state, outcome }) => { + let held = true; + const cleanup = new SetupCleanupOwner([ + { + close: async () => + held + ? { + reason: "Controlled host cleanup", + resources: ["/fixture/retained"], + } + : undefined, + }, + ]); + class HostWithCleanup extends FakeSetupHost { + close = async () => { + const failure = await cleanup.close(); + if (outcome === "throw" && failure !== undefined) throw failure; + return failure; + }; + } + const host = new HostWithCleanup(); + const control = new FakeSetupHost(); + for (const selected of [host, control]) { + selected.hopper = "/Applications/Hopper"; + selected.doctorHealthy = state === "ready"; + selected.scopedDoctorHealthy = state === "ready"; + if (state === "needs_human") selected.nodeVersion = "25.0.0"; + } + const input = { + ...options(true), + installSkill: false, + proposeHopper: false, + clientIds: [], + }; + const baseline = await runSetup(input, control); + expect(baseline.status).toBe(state); + const result = await runSetup(input, host); + expect(result).toMatchObject({ + status: "needs_human", + code: baseline.code ?? "cleanup_failed", + plannedActions: baseline.plannedActions, + appliedActions: baseline.appliedActions, + clients: baseline.clients, + doctor: baseline.doctor, + cleanup: { + reason: "Controlled host cleanup", + resources: ["/fixture/retained"], + }, + }); + expect(JSON.parse(JSON.stringify(result))).toMatchObject({ + cleanup: { resources: ["/fixture/retained"] }, + }); + const retainedReport = result.cleanup; + held = false; + expect(await host.close()).toBeUndefined(); + expect(result.cleanup).toBeUndefined(); + expect(retainedReport?.resources).toEqual([]); + expect(JSON.parse(JSON.stringify(result))).not.toHaveProperty("cleanup"); + }, + ); }); describe("setup scoped readiness", () => { @@ -529,3 +703,74 @@ describe("setup Hopper install eligibility", () => { }, ); }); + +it("leaves ownerless caller-host failures unconfirmed after an unobserved direct retry", async () => { + const failure = setupCleanupError(undefined, { + reason: "Unknown cleanup authority", + resources: ["/fixture/unconfirmed"], + }); + let held = true; + class Host extends FakeSetupHost { + close = async () => { + if (held) throw failure; + return undefined; + }; + } + const host = new Host(); + const result = await runSetup(options(true), host); + const report = result.cleanup; + held = false; + expect(await host.close()).toBeUndefined(); + expect(failure.hasCleanupOwner).toBe(false); + expect(failure.cleanupIncomplete).toBe(true); + expect(report?.resources).toEqual(["/fixture/unconfirmed"]); + expect(result.cleanup).toBeDefined(); +}); + +it.each(["return", "throw"] as const)( + "does not report already confirmed host cleanup: %s", + async (outcome) => { + let held = true; + const cleanup = new SetupCleanupOwner([ + { close: async () => (held ? "held" : undefined) }, + ]); + const resolved = await cleanup.close(); + held = false; + await cleanup.close(); + class Host extends FakeSetupHost { + close = async () => { + if (outcome === "throw") throw resolved; + return resolved; + }; + } + const input = options(true); + const control = await runSetup(input, new FakeSetupHost()); + expect(await runSetup(input, new Host())).toEqual(control); + }, +); + +it.each(["return", "throw"] as const)( + "reports ordinary typed host close failures consistently: %s", + async (outcome) => { + const failure = new ConfigurationError( + "Controlled close configuration failure", + ); + class Host extends FakeSetupHost { + close = async () => { + if (outcome === "throw") throw failure; + return failure; + }; + } + const input = options(true); + const control = await runSetup(input, new FakeSetupHost()); + const result = await runSetup(input, new Host()); + expect(result).toMatchObject({ + status: "needs_human", + code: control.code ?? "cleanup_failed", + cleanup: { resources: [] }, + }); + expect(result.cleanup?.reason).toContain(failure.message); + expect(failure.cleanupIncomplete).toBe(false); + expect(failure.hasCleanupOwner).toBe(false); + }, +); diff --git a/src/application/Setup.ts b/src/application/Setup.ts index b92fd3323b..08a482c67e 100644 --- a/src/application/Setup.ts +++ b/src/application/Setup.ts @@ -27,9 +27,12 @@ import { hostRemediation, systemSetupHost, } from "./SetupHost.js"; -import { AnalysisError } from "../domain/analysisErrorBase.js"; +import { + AnalysisError, + type AnalysisCleanupObservation, +} from "../domain/analysisErrorBase.js"; +import { setupCleanupError, setupCleanupReason } from "./SetupCleanupOwner.js"; import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; -import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import type { SetupSkillInstallError } from "./SetupSkill.js"; import type { SetupHopperInstallFailure } from "./SetupInstallFailure.js"; @@ -49,42 +52,47 @@ export const runSetup = async ( primaryFailure = cause; } - let cleanupFailure: string | undefined; + let cleanup: AnalysisError | undefined; try { - cleanupFailure = await host.close?.(); + const report = await host.close?.(); + cleanup = report === undefined ? undefined : setupCleanupError(report); } catch (cause: unknown) { - cleanupFailure = `Setup resource cleanup failed: ${failureMessage(cause)}`; + cleanup = setupCleanupError(cause); } - + if (cleanup !== undefined && !cleanup.cleanupIncomplete) cleanup = undefined; if (threw) { - if (cleanupFailure !== undefined && primaryFailure instanceof AnalysisError) + if (cleanup === undefined) throw primaryFailure; + if (primaryFailure instanceof AnalysisError) throw analysisErrorWithCleanupFailure( primaryFailure, - new ProviderCleanupError( - "setup", - primaryFailure.cleanupResources, - { reason: cleanupFailure }, - { operation: "setup" }, - ), + cleanup, "setup", - ); - if (cleanupFailure !== undefined) - throw new AggregateError( - [primaryFailure, new Error(cleanupFailure)], - "Setup and resource cleanup failed.", - { cause: primaryFailure }, - ); - throw primaryFailure; + ).bindCleanupDependencies([primaryFailure, cleanup]); + throw new AggregateError( + [primaryFailure, cleanup], + "Setup and resource cleanup failed.", + { cause: primaryFailure }, + ); } if (result === undefined) throw new Error("Setup returned no result."); - if (cleanupFailure === undefined) return result; + if (cleanup === undefined) return result; + const cleanupReason = setupCleanupReason(cleanup); + const cleanupReport: AnalysisCleanupObservation = { + reason: cleanupReason, + get resources() { + return cleanup.cleanupResources; + }, + }; return { ...result, status: "needs_human", code: result.code ?? "cleanup_failed", + get cleanup() { + return cleanup.cleanupIncomplete ? cleanupReport : undefined; + }, remediation: result.remediation - ? `${result.remediation} ${cleanupFailure}` - : cleanupFailure, + ? `${result.remediation} ${cleanupReason}` + : cleanupReason, }; }; diff --git a/src/application/SetupCleanupOwner.test.ts b/src/application/SetupCleanupOwner.test.ts new file mode 100644 index 0000000000..b31407ca19 --- /dev/null +++ b/src/application/SetupCleanupOwner.test.ts @@ -0,0 +1,439 @@ +import { expect, it } from "vitest"; + +import { SetupCleanupOwner } from "./SetupCleanupOwner.js"; +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../artifacts/AnalysisCleanupLease.js"; +import { AnalysisError } from "../domain/analysisErrorBase.js"; +import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; + +const errorSources = (error: unknown): ReadonlySet => { + const pending = [error]; + const seen = new Set(); + while (pending.length > 0) { + const value = pending.pop(); + if (seen.has(value)) continue; + seen.add(value); + if (value instanceof Error) pending.push(value.cause); + if (value instanceof AggregateError) pending.push(...value.errors); + if (value instanceof AnalysisError) + pending.push(value.cleanupPrimary, ...value.secondaryFailures); + } + return seen; +}; + +it("updates every report through mixed cleanup outcomes and exact retries", async () => { + let firstHeld = true; + let secondHeld = true; + let firstCalls = 0; + const owner = new SetupCleanupOwner([ + { + close: async () => { + if (++firstCalls > 2) + throw new Error("Completed child must not close again"); + return firstHeld + ? { reason: "first held", resources: ["/first"] } + : undefined; + }, + }, + { + close: async () => + secondHeld + ? { reason: "second held", resources: ["/second"] } + : undefined, + }, + ]); + const first = await owner.close(); + expect(first?.cleanupResources).toEqual(["/first", "/second"]); + firstHeld = false; + const second = await owner.close(); + expect(first?.cleanupIncomplete).toBe(true); + expect(first?.cleanupResources).toEqual(["/second"]); + expect(second?.cleanupResources).toEqual(["/second"]); + secondHeld = false; + expect(await owner.close()).toBeUndefined(); + expect(first?.cleanupIncomplete).toBe(false); + expect(first?.cleanupResources).toEqual([]); + expect(second?.cleanupIncomplete).toBe(false); + expect(firstCalls).toBe(2); +}); + +it("keeps opaque cleanup unresolved without inventing paths or retaining released labels", async () => { + let fileHeld = true; + let opaqueHeld = true; + const owner = new SetupCleanupOwner([ + { + close: async () => + fileHeld ? { reason: "held file", resources: ["/owned"] } : undefined, + }, + { + close: async () => { + if (opaqueHeld) throw new Error("opaque /not-an-owned-path"); + }, + }, + ]); + const report = await owner.close(); + expect(report?.cleanupResources).toEqual(["/owned"]); + fileHeld = false; + await owner.close(); + expect(report?.cleanupIncomplete).toBe(true); + expect(report?.cleanupResources).toEqual([]); + opaqueHeld = false; + expect(await owner.close()).toBeUndefined(); + expect(report?.cleanupIncomplete).toBe(false); +}); + +it("starts child finalization synchronously and shares concurrent closes", async () => { + let calls = 0; + let complete: (() => void) | undefined; + const gate = new Promise((resolve) => { + complete = resolve; + }); + const owner = new SetupCleanupOwner([ + { + close: () => { + calls++; + return gate; + }, + }, + ]); + const first = owner.close(); + expect(calls).toBe(1); + expect(owner.close()).toBe(first); + complete?.(); + expect(await first).toBeUndefined(); + expect(await owner.close()).toBeUndefined(); + expect(calls).toBe(1); +}); + +it("adopts later cleanup without reopening a completed generation", async () => { + let held = true; + const task = { + close: async () => + held ? { reason: "held", resources: ["/same-label"] } : undefined, + }; + const owner = new SetupCleanupOwner([task]); + const previous = await owner.close(); + held = false; + expect(await owner.close()).toBeUndefined(); + held = true; + owner.admit(task); + const current = await owner.close(); + expect(previous?.cleanupIncomplete).toBe(false); + expect(previous?.cleanupResources).toEqual([]); + expect(current?.cleanupIncomplete).toBe(true); + expect(current?.cleanupResources).toEqual(["/same-label"]); + held = false; + expect(await owner.close()).toBeUndefined(); +}); + +it("keeps finalization cancellation unresolved and still attempts the other child", async () => { + let interrupted = true; + let otherAttempts = 0; + const owner = new SetupCleanupOwner([ + { + close: async () => { + if (interrupted) + throw new DOMException("close interrupted", "AbortError"); + }, + }, + { + close: async () => { + otherAttempts++; + }, + }, + ]); + const report = await owner.close(); + expect(report?.cleanupIncomplete).toBe(true); + expect(otherAttempts).toBe(1); + interrupted = false; + expect(await owner.close()).toBeUndefined(); + expect(report?.cleanupIncomplete).toBe(false); +}); + +it.each(["group", "direct"])( + "observes exact concurrent recovery before aggregate binding: %s", + async (binding) => { + const source = new ProviderCleanupError("fixture", ["/shared"], {}); + const lease = new AnalysisCleanupLease(async () => {}, ["/shared"]); + if (binding === "direct") lease.bindCleanupError(source); + else new AnalysisCleanupLeaseGroup(source, [lease]); + let finish: (() => void) | undefined; + const gate = new Promise((resolve) => { + finish = resolve; + }); + let sourceCalls = 0; + const owner = new SetupCleanupOwner([ + { + close: async () => { + sourceCalls++; + throw source; + }, + }, + { close: () => gate }, + ]); + const closing = owner.close(); + await lease.release(); + finish?.(); + expect(await closing).toBeUndefined(); + expect(await owner.close()).toBeUndefined(); + expect(sourceCalls).toBe(1); + }, +); + +it.each(["pending", "already recovered"] as const)( + "keeps delegated cleanup pending when the next report has an independent owner: %s", + async (timing) => { + const source = new ProviderCleanupError("fixture", ["/independent"], {}); + const sourceLease = new AnalysisCleanupLease(async () => {}, [ + "/independent", + ]); + new AnalysisCleanupLeaseGroup(source, [sourceLease]); + let calls = 0; + const owner = new SetupCleanupOwner([ + { + close: async () => { + calls++; + if (calls === 1) + return { + reason: "delegated work pending", + resources: ["/delegated"], + }; + if (calls === 2) return source; + return undefined; + }, + }, + ]); + const first = await owner.close(); + if (timing === "already recovered") await sourceLease.release(); + const second = await owner.close(); + await sourceLease.release(); + expect(source.cleanupIncomplete).toBe(false); + expect(first?.cleanupIncomplete).toBe(true); + expect(second?.cleanupIncomplete).toBe(true); + expect(first?.cleanupResources).toContain("/delegated"); + expect(second?.cleanupResources).toContain("/delegated"); + expect(first?.cleanupResources).toEqual(["/delegated"]); + expect(second?.cleanupResources).toEqual(["/delegated"]); + expect(await owner.close()).toBeUndefined(); + expect(calls).toBe(3); + expect(first?.cleanupIncomplete).toBe(false); + expect(second?.cleanupIncomplete).toBe(false); + }, +); + +it("keeps an immutable ownerless source snapshot while its owned report recovers", async () => { + const source = new ProviderCleanupError("fixture", ["/delegated"], {}); + Object.freeze(source); + let held = true; + const owner = new SetupCleanupOwner([ + { + close: async () => (held ? source : undefined), + }, + ]); + const first = await owner.close(); + expect(first?.cleanupResources).toEqual(["/delegated"]); + const second = await owner.close(); + expect(second?.cleanupResources).toEqual(["/delegated"]); + held = false; + expect(await owner.close()).toBeUndefined(); + expect(source.hasCleanupOwner).toBe(false); + expect(source.cleanupIncomplete).toBe(true); + expect(first?.cleanupIncomplete).toBe(false); + expect(second?.cleanupIncomplete).toBe(false); +}); + +it("keeps both authorities pending when the callback succeeds before its independent source", async () => { + const source = new ProviderCleanupError("fixture", ["/independent"], {}); + const sourceLease = new AnalysisCleanupLease(async () => {}, [ + "/independent", + ]); + new AnalysisCleanupLeaseGroup(source, [sourceLease]); + let calls = 0; + const owner = new SetupCleanupOwner([ + { + close: async () => { + calls++; + if (calls === 1) + return { + reason: "delegated work pending", + resources: ["/delegated"], + }; + if (calls === 2) return source; + return undefined; + }, + }, + ]); + const first = await owner.close(); + const second = await owner.close(); + const third = await owner.close(); + expect(source.cleanupIncomplete).toBe(true); + for (const report of [first, second, third]) { + expect(report?.cleanupIncomplete).toBe(true); + expect(report?.cleanupResources).toEqual(["/independent"]); + } + expect(await owner.close()).toBeDefined(); + expect(calls).toBe(3); + await sourceLease.release(); + expect(await owner.close()).toBeUndefined(); + expect(calls).toBe(3); + for (const report of [first, second, third]) + expect(report?.cleanupIncomplete).toBe(false); +}); + +it("starts a fresh report when a prior source resolves inside a retry before a new source fails", async () => { + const firstSource = new ProviderCleanupError( + "fixture", + ["/first-source"], + {}, + ); + const secondSource = new ProviderCleanupError( + "fixture", + ["/second-source"], + {}, + ); + const firstLease = new AnalysisCleanupLease(async () => {}, [ + "/first-source", + ]); + const secondLease = new AnalysisCleanupLease(async () => {}, [ + "/second-source", + ]); + new AnalysisCleanupLeaseGroup(firstSource, [firstLease]); + new AnalysisCleanupLeaseGroup(secondSource, [secondLease]); + let calls = 0; + const owner = new SetupCleanupOwner([ + { + close: async () => { + calls++; + if (calls === 1) return firstSource; + await firstLease.release(); + return secondSource; + }, + }, + ]); + const first = await owner.close(); + const second = await owner.close(); + expect(first?.cleanupIncomplete).toBe(false); + expect(second?.cleanupIncomplete).toBe(true); + expect(second?.cleanupResources).toEqual(["/second-source"]); + await secondLease.release(); + expect(await owner.close()).toBeUndefined(); + expect(calls).toBe(2); +}); + +it.each(["artifact", "analysis"] as const)( + "preserves a rejected %s producer's detailed cleanup reason", + async (kind) => { + const observation = { + reason: "Unlink denied because the owned file remains locked", + resources: ["/fixture/owned"], + }; + const failure = + kind === "artifact" + ? new ArtifactReaderFailure( + "unavailable", + "Retained resources: /fixture/owned", + { cleanup: observation }, + ) + : new ProviderCleanupError( + "fixture", + observation.resources, + {}, + { cleanup: observation }, + ); + const owner = new SetupCleanupOwner([ + { + rejectedPrefix: "Publisher cleanup failed", + close: async () => { + throw failure; + }, + }, + ]); + const report = await owner.close(); + expect(report?.cleanup?.reason).toBe( + "Publisher cleanup failed: " + observation.reason, + ); + expect(report?.cleanupResources).toEqual(observation.resources); + }, +); + +it("retains the latest typed retry cause independently of its completion owner", async () => { + const firstCause = new ProviderCleanupError("fixture", ["/owned"], { + stage: "first", + }); + const secondCause = new ProviderCleanupError( + "fixture", + ["/owned"], + { stage: "retry" }, + { + capturedOutput: { + stdout: "retry-specific output", + stderr: "", + truncated: false, + }, + }, + ); + let calls = 0; + const owner = new SetupCleanupOwner([ + { + close: async () => { + if (++calls === 1) throw firstCause; + if (calls === 2) throw secondCause; + }, + }, + ]); + const first = await owner.close(); + const second = await owner.close(); + expect(errorSources(second).has(secondCause)).toBe(true); + expect(secondCause.capturedOutput?.stdout).toBe("retry-specific output"); + expect(await owner.close()).toBeUndefined(); + expect(first?.cleanupIncomplete).toBe(false); + expect(second?.cleanupIncomplete).toBe(false); +}); + +it("does not share callback ownership through a reused ownerless source error", async () => { + const source = new ProviderCleanupError("fixture", ["/same-file"], {}); + let firstHeld = true; + let secondHeld = true; + const owner = new SetupCleanupOwner([ + { close: async () => (firstHeld ? source : undefined) }, + { close: async () => (secondHeld ? source : undefined) }, + ]); + const first = await owner.close(); + firstHeld = false; + const second = await owner.close(); + expect(second?.cleanupIncomplete).toBe(true); + expect(second?.cleanupResources).toEqual(["/same-file"]); + secondHeld = false; + expect(await owner.close()).toBeUndefined(); + expect(first?.cleanupIncomplete).toBe(false); + expect(second?.cleanupIncomplete).toBe(false); + expect(source.hasCleanupOwner).toBe(false); + expect(source.cleanupIncomplete).toBe(true); +}); + +it("retains earlier independent owner evidence alongside the latest retry cause", async () => { + const firstSource = new ProviderCleanupError("fixture", ["/first"], { + stage: "first", + }); + const secondSource = new ProviderCleanupError("fixture", ["/second"], { + stage: "retry", + }); + const firstLease = new AnalysisCleanupLease(async () => {}, ["/first"]); + const secondLease = new AnalysisCleanupLease(async () => {}, ["/second"]); + new AnalysisCleanupLeaseGroup(firstSource, [firstLease]); + new AnalysisCleanupLeaseGroup(secondSource, [secondLease]); + let calls = 0; + const owner = new SetupCleanupOwner([ + { close: async () => (++calls === 1 ? firstSource : secondSource) }, + ]); + await owner.close(); + const latest = await owner.close(); + expect(errorSources(latest).has(firstSource)).toBe(true); + expect(errorSources(latest).has(secondSource)).toBe(true); + await firstLease.release(); + await secondLease.release(); + expect(await owner.close()).toBeUndefined(); +}); diff --git a/src/application/SetupCleanupOwner.ts b/src/application/SetupCleanupOwner.ts new file mode 100644 index 0000000000..46d5a34c94 --- /dev/null +++ b/src/application/SetupCleanupOwner.ts @@ -0,0 +1,254 @@ +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../artifacts/AnalysisCleanupLease.js"; +import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; +import { + AnalysisError, + type AnalysisCleanupObservation, +} from "../domain/analysisErrorBase.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { bindRegularFileCleanup } from "./RegularFileRead.js"; + +interface SetupCleanupTask { + readonly rejectedPrefix?: string; + close(): Promise; +} + +/** Construct a report from facts already parsed by its owning boundary. */ +const setupCleanupReport = ( + cause: unknown, + observation: AnalysisCleanupObservation, +): AnalysisError => + new ProviderCleanupError( + "setup", + observation.resources, + { reason: observation.reason }, + { cause, cleanup: observation, operation: "setup" }, + ); + +/** Preserve explicit cleanup capabilities when a host rejects instead of reporting. */ +export const setupCleanupError = ( + cause: unknown, + observation = ArtifactReaderFailure.cleanupObservation(cause, []), +): AnalysisError => { + if ( + cause instanceof AnalysisError && + (cause.cleanupIncomplete || + cause.hasCleanupOwner || + cause.cleanup !== undefined || + cause instanceof ProviderCleanupError) + ) + return cause; + const error = setupCleanupReport(cause, observation); + bindRegularFileCleanup(cause, error); + return error; +}; + +export const setupCleanupReason = (error: AnalysisError): string => + error.cleanup?.reason ?? + (typeof error.diagnostics?.reason === "string" + ? error.diagnostics.reason + : error.message); + +/** One child callback, its conjunctive completion owner, and latest failure provenance. */ +class SetupCleanupChild { + #generation = {}; + #attempted: object | undefined; + #completed: object | undefined; + #failure: { readonly owner: AnalysisError; cause: AnalysisError } | undefined; + #reportedErrors = new WeakSet(); + #delegated: AnalysisError | undefined; + #reason = ""; + #resources: readonly string[] = []; + #attempt: ReturnType = Promise.resolve(); + #lease = this.#newLease(); + + constructor(private readonly task: SetupCleanupTask) {} + + admit(): void { + this.#generation = {}; + } + get hasNewGeneration(): boolean { + return this.#attempted !== this.#generation; + } + + get failure(): + | { + readonly owner: AnalysisError; + readonly cause: AnalysisError; + readonly reason: string; + } + | undefined { + if (this.#failure !== undefined && !this.#failure.owner.cleanupIncomplete) { + this.#completed = this.#attempted; + this.#failure = undefined; + this.#reportedErrors = new WeakSet(); + } + return this.#failure === undefined + ? undefined + : { ...this.#failure, reason: this.#reason }; + } + + close(): Promise { + // A source owner may have completed between rejection and this retry. + void this.failure; + if (this.#completed === this.#generation) return Promise.resolve(); + if (this.#lease.released || this.#completed !== undefined) { + this.#lease = this.#newLease(); + this.#completed = undefined; + this.#delegated = undefined; + } + const generation = this.#generation; + this.#attempted = generation; + // Seal terminal child scopes before returning from close. + try { + this.#attempt = this.task.close(); + } catch (cause: unknown) { + this.#attempt = Promise.reject(cause); + } + return this.#lease.release().then(() => { + this.#completed = generation; + }); + } + + #newLease(): AnalysisCleanupLease { + return new AnalysisCleanupLease(() => this.#execute(), this); + } + + get cleanupResources(): readonly string[] { + return this.#resources; + } + + async #execute(): Promise { + let failure: AnalysisError; + try { + const report = await this.#attempt; + if (report === undefined) return; + failure = + report instanceof AnalysisError + ? setupCleanupError(report) + : setupCleanupError( + undefined, + typeof report === "string" + ? { reason: report, resources: [] } + : report, + ); + this.#reason = setupCleanupReason(failure); + } catch (cause: unknown) { + failure = setupCleanupError(cause); + this.#reason = + (this.task.rejectedPrefix === undefined + ? "" + : this.task.rejectedPrefix + ": ") + setupCleanupReason(failure); + } + // Keep this attempt's diagnostics separate from the owner followed by + // every earlier report from this generation. + if (this.#failure !== undefined) this.#failure.cause = failure; + let dependency = failure; + if (failure.cleanupIncomplete && !failure.hasCleanupOwner) { + this.#resources = [...failure.cleanupResources]; + if (this.#delegated === undefined) { + // Ownerless source errors remain evidence snapshots. A private report + // binds this callback without changing shared or immutable input. + this.#delegated = setupCleanupReport(failure, { + reason: this.#reason, + resources: this.#resources, + }); + new AnalysisCleanupLeaseGroup(this.#delegated, [this.#lease]); + } + // Every attempt shares this exact callback lease and one dependency. + dependency = this.#delegated; + } + if (dependency !== undefined && dependency.cleanupIncomplete) { + if (!this.#reportedErrors.has(dependency)) { + if ( + this.#failure === undefined || + !this.#failure.owner.cleanupIncomplete + ) { + this.#failure = { + owner: setupCleanupReport( + new AggregateError( + [dependency], + "Setup child cleanup authorities", + ), + { reason: this.#reason, resources: [] }, + ), + cause: failure, + }; + this.#reportedErrors = new WeakSet(); + } else this.#failure.owner.retainSecondaryFailure(dependency); + this.#failure.owner.bindCleanupDependencies([dependency]); + this.#reportedErrors.add(dependency); + } + } + // A source's release cannot confirm an earlier delegated callback, and a + // successful callback cannot release an independently owned source. + if (failure.cleanupIncomplete) throw failure; + if (this.#delegated?.cleanupIncomplete) throw this.#delegated; + } +} + +/** Preserve completed peers and follow live source errors through exact retries. */ +export class SetupCleanupOwner { + readonly #children: ReadonlyMap; + #closing: Promise | undefined; + + constructor(tasks: readonly SetupCleanupTask[]) { + this.#children = new Map( + tasks.map((task) => [task, new SetupCleanupChild(task)]), + ); + } + + /** Admit actual new cleanup work, never merely another retry of a failed peer. */ + admit(task: SetupCleanupTask): void { + const child = this.#children.get(task); + if (child === undefined) + throw new TypeError("Cleanup task is not owned by this setup host"); + child.admit(); + } + + close(): Promise { + if (this.#closing !== undefined) return this.#closing; + const attempt = this.#attempt().finally(() => { + if (this.#closing === attempt) this.#closing = undefined; + }); + this.#closing = attempt; + return attempt; + } + + async #attempt(): Promise { + const children = [...this.#children.values()]; + // An active producer can admit its final cleanup outcome while draining. + do { + await Promise.allSettled(children.map((child) => child.close())); + } while (children.some((child) => child.hasNewGeneration)); + const failures = children.flatMap((child) => { + const failure = child.failure; + return failure === undefined ? [] : [failure]; + }); + if (failures.length === 0) return undefined; + const observation: AnalysisCleanupObservation = { + reason: failures.map((failure) => failure.reason).join("; "), + resources: [ + ...new Set( + failures.flatMap((failure) => failure.owner.cleanupResources), + ), + ], + }; + const error = setupCleanupReport( + new AggregateError( + failures.map((failure) => failure.cause), + "Setup child cleanup failures", + { + cause: new AggregateError( + failures.map((failure) => failure.owner), + "Setup cleanup ownership reports", + ), + }, + ), + observation, + ).bindCleanupDependencies(failures.map((failure) => failure.owner)); + return error.cleanupIncomplete ? error : undefined; + } +} diff --git a/src/application/SetupHost.ts b/src/application/SetupHost.ts index 7ad354cb8b..3990b35272 100644 --- a/src/application/SetupHost.ts +++ b/src/application/SetupHost.ts @@ -24,7 +24,6 @@ import { installLinuxHopper, LinuxHopperTemporaryCleanupOwner, systemLinuxHopperInstallHost, - type LinuxHopperInstallHost, } from "./LinuxHopper.js"; import { installMacHopper, systemMacHopperInstallHost } from "./MacHopper.js"; import { linuxHopperReleaseCacheDirectory } from "./LinuxHopperCacheDirectory.js"; @@ -46,7 +45,10 @@ import { inspectClientConfiguration, } from "./SetupClientConfiguration.js"; import { failedConfigurationMessage } from "./SetupClients.js"; -import { setupInstallFailure } from "./SetupInstallFailure.js"; +import { + setupInstallFailure, + type SetupHopperInstallResult, +} from "./SetupInstallFailure.js"; import { RegularFileCleanupFailure } from "../filesystem/OwnedRegularFile.js"; import { explicitSetupRegistrationEnvironment, @@ -62,6 +64,12 @@ import type { SetupProviderEnvironment, } from "./SetupTypes.js"; import type { DoctorScope } from "./Doctor.js"; +import { SetupCleanupOwner } from "./SetupCleanupOwner.js"; +import { + AnalysisCleanupLeaseGroup, + type AnalysisCleanupLease, +} from "../artifacts/AnalysisCleanupLease.js"; +import { bindRegularFileCleanup } from "./RegularFileRead.js"; import { CURRENT_ENTRYPOINT_PATH } from "../process/CurrentEntrypointPath.js"; import { analysisProviderSelectorSchema } from "../contracts/providerSelection.js"; @@ -151,6 +159,30 @@ export const systemSetupHost = ( linuxDistribution, ); const pendingLinuxHopperCleanup = new LinuxHopperTemporaryCleanupOwner(); + const macCleanup = { + rejectedPrefix: "Hopper host cleanup failed", + close: async () => macHopperHost?.close?.(), + }; + const temporaryCleanup = { + rejectedPrefix: "Linux Hopper cleanup retry failed", + close: () => pendingLinuxHopperCleanup.retry(linuxHopperHost.cleanup), + }; + const publisherCleanup = { + rejectedPrefix: "Linux Hopper publisher cleanup failed", + close: async () => { + await linuxHopperHost.close?.(); + }, + }; + const cleanup = new SetupCleanupOwner([ + macCleanup, + { + rejectedPrefix: "Doctor host cleanup failed", + close: async () => doctorHost.close?.(), + }, + temporaryCleanup, + publisherCleanup, + { close: () => resources.close() }, + ]); return { platform, homeDirectory, @@ -166,10 +198,7 @@ export const systemSetupHost = ( nodeVersion: process.versions.node, macosVersion: () => doctorHost.macosVersion(), linuxDistribution, - close: () => - closeSetupResources(resources, macHopperHost, doctorHost, () => - closeLinuxHopperInstallHost(linuxHopperHost, pendingLinuxHopperCleanup), - ), + close: () => cleanup.close(), initialSetupState: async ( scope?: DoctorScope, ): Promise => { @@ -205,62 +234,74 @@ export const systemSetupHost = ( doctor: diagnosis, }; }, - installHopper: async (replaceExisting) => { - if (platform === "linux") { - const result = await installLinuxHopper(linuxHopperHost); - if (result.cleanupFailure !== undefined) - pendingLinuxHopperCleanup.retain(result.cleanupFailure); - if (result.status === "installed") - return { status: "installed", launcherPath: result.launcherPath }; - const failure = setupInstallFailure( - result.reason, - result.packageFailure, - "linux", - ); - const detail = - result.verificationFailure?.message ?? - result.pythonRuntimeFailure?.message ?? - (result.operationFailure === undefined - ? undefined - : `${result.operationFailure.operation}${result.operationFailure.path === undefined ? "" : ` at ${result.operationFailure.path}`}: ${result.operationFailure.systemCode === undefined ? "" : `${result.operationFailure.systemCode}: `}${result.operationFailure.detail}`); - return detail !== undefined - ? { - ...failure, - remediation: `${detail} ${failure.remediation}`, + installHopper: (replaceExisting) => + resources.run(async () => { + const publisher = platform === "linux" ? publisherCleanup : macCleanup; + cleanup.admit(publisher); + try { + if (platform === "linux") { + const result = await installLinuxHopper(linuxHopperHost); + if (result.cleanupFailure !== undefined) { + pendingLinuxHopperCleanup.retain(result.cleanupFailure); + cleanup.admit(temporaryCleanup); } - : failure; - } - if (macHopperHost === undefined) - return setupInstallFailure("unsupported_host"); - const result = await installMacHopper({ replaceExisting }, macHopperHost); - if (result.status === "installed") - return { - status: "installed", - launcherPath: result.launcherPath, - ...(result.openFailure === undefined - ? {} - : { - warning: `Hopper was installed, but macOS could not open it: ${result.openFailure}. Open ${result.launcherPath} manually to complete first-run setup.`, - }), - ...(result.cleanupFailure === undefined - ? {} - : { cleanupFailure: result.cleanupFailure }), - }; - const failure = setupInstallFailure(result.reason); - const diagnostic = setupInstallerDiagnostic(result.diagnostic); - return { - ...failure, - remediation: [ - failure.remediation, - ...(diagnostic === undefined - ? [] - : [`Installer detail: ${diagnostic}`]), - ...(result.cleanupFailure === undefined - ? [] - : [result.cleanupFailure]), - ].join(" "), - }; - }, + if (result.status === "installed") + return { status: "installed", launcherPath: result.launcherPath }; + const failure = setupInstallFailure( + result.reason, + result.packageFailure, + "linux", + ); + const detail = + result.verificationFailure?.message ?? + result.pythonRuntimeFailure?.message ?? + (result.operationFailure === undefined + ? undefined + : `${result.operationFailure.operation}${result.operationFailure.path === undefined ? "" : ` at ${result.operationFailure.path}`}: ${result.operationFailure.systemCode === undefined ? "" : `${result.operationFailure.systemCode}: `}${result.operationFailure.detail}`); + return detail !== undefined + ? { + ...failure, + remediation: `${detail} ${failure.remediation}`, + } + : failure; + } + if (macHopperHost === undefined) + return setupInstallFailure("unsupported_host"); + const result = await installMacHopper( + { replaceExisting }, + macHopperHost, + ); + if (result.status === "installed") + return { + status: "installed", + launcherPath: result.launcherPath, + ...(result.openFailure === undefined + ? {} + : { + warning: `Hopper was installed, but macOS could not open it: ${result.openFailure}. Open ${result.launcherPath} manually to complete first-run setup.`, + }), + ...(result.cleanupFailure === undefined + ? {} + : { cleanupFailure: result.cleanupFailure }), + }; + const failure = setupInstallFailure(result.reason); + const diagnostic = setupInstallerDiagnostic(result.diagnostic); + return { + ...failure, + remediation: [ + failure.remediation, + ...(diagnostic === undefined + ? [] + : [`Installer detail: ${diagnostic}`]), + ...(result.cleanupFailure === undefined + ? [] + : [result.cleanupFailure]), + ].join(" "), + }; + } finally { + cleanup.admit(publisher); + } + }), detectedClients: () => detectClients(homeDirectory, platform, environment), supportedClients: () => Promise.resolve(supportedClients(homeDirectory, platform, environment)), @@ -360,42 +401,6 @@ const clientConfigurationOperations = ( ), }); -const closeSetupResources = async ( - resources: ArtifactResourceScope, - macHopperHost: ReturnType | undefined, - doctorHost: DoctorHost, - retryLinuxHopperCleanup: () => Promise, -): Promise => { - const [resourceCleanup, hopperCleanup, doctorCleanup, linuxCleanup] = - await Promise.allSettled([ - resources.close(), - Promise.resolve().then(() => macHopperHost?.close()), - Promise.resolve().then(() => doctorHost.close?.()), - retryLinuxHopperCleanup(), - ]); - const failures: string[] = []; - if (hopperCleanup.status === "rejected") - failures.push( - `Hopper host cleanup failed: ${errorMessage(hopperCleanup.reason)}`, - ); - else if (hopperCleanup.value !== undefined) - failures.push(hopperCleanup.value); - if (doctorCleanup.status === "rejected") - failures.push( - `Doctor host cleanup failed: ${errorMessage(doctorCleanup.reason)}`, - ); - else if (doctorCleanup.value !== undefined) - failures.push(doctorCleanup.value); - if (linuxCleanup.status === "rejected") - failures.push( - `Linux Hopper cleanup retry failed: ${errorMessage(linuxCleanup.reason)}`, - ); - else if (linuxCleanup.value !== undefined) failures.push(linuxCleanup.value); - if (resourceCleanup.status === "rejected") - failures.push(errorMessage(resourceCleanup.reason)); - return failures.length === 0 ? undefined : failures.join("; "); -}; - const runClientConfigurationOperation = async ( resources: ArtifactResourceScope, operation: () => Promise, @@ -419,6 +424,7 @@ const runClientConfigurationOperation = async ( `Could not safely access client configuration at ${cause.path}: ${errorMessage(cleanup.primary)}.${rollbackMessage}`, cause, cleanup.cleanup, + cleanup.cleanupLeases, ); } }); @@ -435,33 +441,21 @@ const configurationCleanupError = ( cleanup: | { readonly reason: string; readonly resources: readonly string[] } | undefined, -): ConfigurationError => - new ConfigurationError(message, { + leases?: readonly AnalysisCleanupLease[], +): ConfigurationError => { + const error = new ConfigurationError(message, { cause, settings: [{ setting: "client configuration", constraint: message }], ...(cleanup === undefined ? {} : { cleanup }), }); + if (leases !== undefined) new AnalysisCleanupLeaseGroup(error, leases); + else bindRegularFileCleanup(cause, error); + return error; +}; const errorMessage = (cause: unknown): string => cause instanceof Error ? cause.message : String(cause); -const closeLinuxHopperInstallHost = async ( - host: LinuxHopperInstallHost, - temporaryOwner: LinuxHopperTemporaryCleanupOwner, -): Promise => { - const reasons: string[] = []; - const temporary = await temporaryOwner.retry(host.cleanup); - if (temporary !== undefined) reasons.push(temporary); - try { - await host.close?.(); - } catch (cause: unknown) { - reasons.push( - `Linux Hopper publisher cleanup failed: ${errorMessage(cause)}`, - ); - } - return reasons.length === 0 ? undefined : reasons.join("; "); -}; - /** Detect supported agents from their config files or stable installation markers. */ export const detectClients = async ( home: string, diff --git a/src/application/SetupTypes.ts b/src/application/SetupTypes.ts index 7b9e3fba7b..a817425bd7 100644 --- a/src/application/SetupTypes.ts +++ b/src/application/SetupTypes.ts @@ -8,6 +8,10 @@ import type { SetupSkillInstallError, } from "./SetupSkill.js"; import type { Result } from "../domain/result.js"; +import type { + AnalysisCleanupObservation, + AnalysisError, +} from "../domain/analysisErrorBase.js"; import type { SetupClient } from "./SupportedClients.js"; import type { SetupFailureCode, @@ -106,8 +110,10 @@ export interface SetupHost { linuxDistribution(): Promise; initialSetupState(scope?: DoctorScope): Promise; installHopper(replaceExisting: boolean): Promise; - /** Retry and report any cleanup resources still owned by this setup host. */ - close?(): Promise; + /** Retry retained owners. Returned or thrown errors with explicit cleanup + * owners follow live release state; ownerless failures remain unconfirmed. + * runSetup does not observe later direct calls to a caller-supplied host. */ + close?(): Promise; detectedClients(): Promise; supportedClients(): Promise; configureClient( @@ -148,6 +154,8 @@ export interface SetupResult { readonly clientStates: readonly SetupClientState[]; readonly remediation?: string; readonly code?: SetupFailureCode; + /** Identified resources still retained after finalization; absent after confirmed release. */ + readonly cleanup?: AnalysisCleanupObservation | undefined; } /** Whether the requested setup actions still need approval or human repair. */ diff --git a/src/artifacts/AnalysisCleanupLease.test.ts b/src/artifacts/AnalysisCleanupLease.test.ts index cf94eb23ea..5c82c19c06 100644 --- a/src/artifacts/AnalysisCleanupLease.test.ts +++ b/src/artifacts/AnalysisCleanupLease.test.ts @@ -23,7 +23,9 @@ it.each(["before release", "after release"])( if (binding === "before release") lease.bindCleanupError(error); await lease.release(); if (binding === "after release") lease.bindCleanupError(error); + expect(error.hasCleanupOwner).toBe(true); expect(error.cleanupIncomplete).toBe(false); + expect(error.cleanupResources).toEqual(["owned resource"]); expect(error.cause).toBe(cause); await lease.release(); expect(releases).toBe(1); @@ -178,3 +180,24 @@ it("shares an in-flight release and permits one new attempt after failure", asyn for (const settle of pending) settle(); } }); + +it("preserves a producer's live attribution when binding direct completion", async () => { + const error = new ProviderCleanupError("fixture", ["initial resource"], {}); + let resources = ["first resource", "second resource"]; + error.bindCleanupResourceOwner({ + get cleanupResources() { + return resources; + }, + }); + const lease = new AnalysisCleanupLease(async () => { + resources = []; + }); + lease.bindCleanupError(error); + expect(error.hasCleanupOwner).toBe(true); + expect(error.cleanupResources).toEqual(["first resource", "second resource"]); + resources = ["second resource"]; + expect(error.cleanupResources).toEqual(["second resource"]); + await lease.release(); + expect(error.cleanupIncomplete).toBe(false); + expect(error.cleanupResources).toEqual([]); +}); diff --git a/src/artifacts/AnalysisCleanupLease.ts b/src/artifacts/AnalysisCleanupLease.ts index 80049cfc60..df79a6c85f 100644 --- a/src/artifacts/AnalysisCleanupLease.ts +++ b/src/artifacts/AnalysisCleanupLease.ts @@ -7,7 +7,6 @@ import type { export class AnalysisCleanupLease { #released = false; #releasePromise: Promise | undefined; - #errors: WeakRef[] = []; readonly #groups = new Set(); /** Whether this exact owner has confirmed release. */ @@ -64,7 +63,6 @@ export class AnalysisCleanupLease { .then(async () => { await this.cleanup(); this.#released = true; - this.#resolveBoundErrors(); for (const group of this.#groups) group.ownerReleased(this); this.#groups.clear(); }) @@ -77,14 +75,9 @@ export class AnalysisCleanupLease { /** Binding can follow a concurrent retry that already confirmed release. */ bindCleanupError(error: AnalysisError): void { - if (this.#released) { - error.resolveCleanup(); - return; - } - this.#errors = [ - ...this.#errors.filter((reference) => reference.deref() !== undefined), - new WeakRef(error), - ]; + new AnalysisCleanupLeaseGroup(error, [this], { + preserveResourceAttribution: true, + }); } /** Bind one aggregate error to every exact lease in a failed retry set. */ @@ -97,11 +90,6 @@ export class AnalysisCleanupLease { if (!retained.hasLiveError) this.#groups.delete(retained); this.#groups.add(group); } - - #resolveBoundErrors(): void { - for (const reference of this.#errors) reference.deref()?.resolveCleanup(); - this.#errors = []; - } } /** Conjunctive cleanup binding for an error covering multiple exact owners. */ @@ -109,11 +97,17 @@ export class AnalysisCleanupLeaseGroup { readonly #error: WeakRef; readonly #pending: Set; - constructor(error: AnalysisError, owners: readonly AnalysisCleanupLease[]) { + constructor( + error: AnalysisError, + owners: readonly AnalysisCleanupLease[], + options: { readonly preserveResourceAttribution?: boolean } = {}, + ) { this.#error = new WeakRef(error); this.#pending = new Set(owners); if (this.#pending.size === 0) return; - error.bindCleanupResourceOwner(this); + if (options.preserveResourceAttribution === true) + error.bindCleanupCompletionOwner(this); + else error.bindCleanupResourceOwner(this); for (const owner of this.#pending) owner.bindCleanupGroup(this); } diff --git a/src/cli/interactiveSetup.ts b/src/cli/interactiveSetup.ts index 6d19ae6eb5..9e73025e69 100644 --- a/src/cli/interactiveSetup.ts +++ b/src/cli/interactiveSetup.ts @@ -110,6 +110,8 @@ export const renderInteractiveSetupResult = ( } if (result.status === "planned" || result.status === "cancelled") return; writeLine(`! ${result.remediation ?? "Setup needs attention."}`); + for (const resource of result.cleanup?.resources ?? []) + writeLine(` Pending cleanup: ${resource}`); outro( `Run \`${cliInvocation(environment)} doctor\` for the remaining checks.`, promptStreams, diff --git a/src/domain/analysisErrorBase.ts b/src/domain/analysisErrorBase.ts index 573a93e1ad..d754ae2008 100644 --- a/src/domain/analysisErrorBase.ts +++ b/src/domain/analysisErrorBase.ts @@ -82,6 +82,7 @@ export abstract class AnalysisError extends Error { cleanupIncomplete: boolean; #ownedCleanupResources: readonly string[]; #cleanupResourceOwner: AnalysisCleanupResourceOwner | undefined; + #cleanupCompletionOwner: AnalysisCleanupResourceOwner | undefined; #secondaryFailures: AnalysisError[] = []; constructor(message: string, options?: AnalysisErrorOptions) { super(message, options); @@ -132,12 +133,27 @@ export abstract class AnalysisError extends Error { ]; } + /** Whether cleanup follows an explicit owner or dependent errors. */ + get hasCleanupOwner(): boolean { + return ( + this.#cleanupResourceOwner !== undefined || + this.#cleanupCompletionOwner !== undefined || + this.#cleanupDependencies !== undefined + ); + } + /** Bind attribution only to the producer that owns the complete cleanup set. */ bindCleanupResourceOwner(owner: AnalysisCleanupResourceOwner): this { this.#cleanupResourceOwner = owner; return this; } + /** Observe exact completion without replacing the producer's resource facts. */ + bindCleanupCompletionOwner(owner: AnalysisCleanupResourceOwner): this { + this.#cleanupCompletionOwner = owner; + return this; + } + /** Initialize resource facts supplied by a more specific error producer. */ protected setCleanupResources(resources: readonly string[]): void { this.#ownedCleanupResources = [...resources]; diff --git a/tests/boundary/cli/cliSetup.test.ts b/tests/boundary/cli/cliSetup.test.ts index 00e45c5b17..7574b4e26d 100644 --- a/tests/boundary/cli/cliSetup.test.ts +++ b/tests/boundary/cli/cliSetup.test.ts @@ -17,6 +17,7 @@ import { describe, expect, it } from "vitest"; import { z } from "zod"; import { parse as parseToml } from "smol-toml"; +import type { SetupResult } from "../../../src/application/SetupTypes.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { cliTest } from "../../support/cli/cliFixture.js"; @@ -492,6 +493,43 @@ describe("interactive setup completion", () => { ); }, ); + it("prints retained cleanup resources without relying on paths in remediation", async () => { + const resources = ["/fixture/owned backup", "/fixture/临时目录"]; + const result: SetupResult = { + status: "needs_human", + code: "cleanup_failed", + plannedActions: [], + appliedActions: [], + clients: {}, + clientStates: [], + doctor: { + healthy: true, + environment_healthy: true, + availableProviders: [], + scope: { + mode: "explicit", + clients: [], + providers: [], + skill: false, + target: null, + }, + }, + remediation: "Owned descriptor remains busy", + cleanup: { reason: "Owned descriptor remains busy", resources }, + }; + const { stderr } = await execute( + process.execPath, + [ + "--input-type=module", + "--eval", + 'import { renderInteractiveSetupResult } from "./dist/cli/interactiveSetup.js";\n' + + `renderInteractiveSetupResult(${JSON.stringify(result)}, {});`, + ], + { cwd: process.cwd(), env: { ...process.env, NO_COLOR: "1" } }, + ); + expect(stderr).toContain(result.remediation); + for (const resource of resources) expect(stderr).toContain(resource); + }); }); interface JourneyStep { diff --git a/tests/boundary/filesystem/macHopperInstallOwnership.test.ts b/tests/boundary/filesystem/macHopperInstallOwnership.test.ts index 04b47c2fc9..41549a9c83 100644 --- a/tests/boundary/filesystem/macHopperInstallOwnership.test.ts +++ b/tests/boundary/filesystem/macHopperInstallOwnership.test.ts @@ -15,8 +15,11 @@ import { afterEach, describe, expect, it } from "vitest"; import { installMacHopperBundle, + systemMacHopperInstallHost, type MacHopperBundleInstallOperations, } from "../../../src/application/MacHopper.js"; +import { SetupCleanupOwner } from "../../../src/application/SetupCleanupOwner.js"; +import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; const roots: string[] = []; @@ -176,7 +179,10 @@ describe("macOS Hopper replacement ownership", () => { }, (path) => (stagingRoot = path), ), - (cleanup) => retained.push(cleanup), + (cleanup, resources) => { + expect(resources).toEqual([stagingRoot]); + retained.push(cleanup); + }, ); expect(result).toMatchObject({ @@ -233,7 +239,10 @@ describe("macOS Hopper replacement ownership", () => { }, (path) => (stagingRoot = path), ), - (cleanup) => rollbackRetained.push(cleanup), + (cleanup, resources) => { + expect(resources).toEqual([stagingRoot]); + rollbackRetained.push(cleanup); + }, ); expect(result).toMatchObject({ @@ -273,3 +282,39 @@ describe("macOS Hopper replacement ownership", () => { await expect(access(stagingRoot)).rejects.toMatchObject({ code: "ENOENT" }); }); }); + +it("reports only retained Mac cleanup roots and clears the live report after exact retries", async () => { + const home = await temporaryRoot(); + const temporary = join(home, "owned-dmg"); + const stage = join(home, "owned-stage"); + await Promise.all([mkdir(temporary), mkdir(stage)]); + const host = systemMacHopperInstallHost(home, { HOME: home }); + let allowTemporary = false; + let allowStage = false; + let temporaryCalls = 0; + host.retainCleanup?.(async () => { + temporaryCalls++; + if (!allowTemporary) throw new Error("DMG root busy"); + await rm(temporary, { recursive: true }); + }, [temporary]); + host.retainCleanup?.(async () => { + if (!allowStage) throw new Error("staging root busy"); + await rm(stage, { recursive: true }); + }, [stage]); + const cleanup = new SetupCleanupOwner([{ close: () => host.close() }]); + const failure = await cleanup.close(); + expect(failure).toBeDefined(); + expect(projectAnalysisError(failure!).details?.resources).toEqual([ + temporary, + stage, + ]); + allowTemporary = true; + expect((await cleanup.close())?.cleanupResources).toEqual([stage]); + expect(failure?.cleanupResources).toEqual([stage]); + await expect(access(temporary)).rejects.toMatchObject({ code: "ENOENT" }); + allowStage = true; + expect(await cleanup.close()).toBeUndefined(); + expect(failure?.cleanupIncomplete).toBe(false); + expect(temporaryCalls).toBe(2); + await expect(access(stage)).rejects.toMatchObject({ code: "ENOENT" }); +}); diff --git a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts index a780ef3f51..9f819d53e8 100644 --- a/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts +++ b/tests/boundary/filesystem/setupClientConfigCleanupOwnership.test.ts @@ -89,7 +89,7 @@ const preparedVsCodeSetupHost = async (prefix: string) => { /** Release the held read handle and verify host cleanup retries that same owner. */ const expectCloseReclaimsHandle = async ( - host: { close?: () => Promise }, + host: { close?: () => Promise }, closeAttempts: readonly object[], release: () => void, ) => { @@ -102,7 +102,7 @@ const expectCloseReclaimsHandle = async ( /** Give up on the assertion path: release the handle and let cleanup close it. */ const cleanupHeldHandle = async ( - host: { close?: () => Promise }, + host: { close?: () => Promise }, release: () => void, ) => { release(); @@ -257,6 +257,10 @@ describe("setup client configuration cleanup ownership", () => { (cause: unknown) => cause, ); expect(failure).toBeInstanceOf(ConfigurationError); + if (!(failure instanceof ConfigurationError)) + throw new Error( + "Expected the configuration failure to retain cleanup authority", + ); if (encoding === "malformed UTF-8") expect(failure).toMatchObject({ cause: { @@ -279,6 +283,8 @@ describe("setup client configuration cleanup ownership", () => { expect(await readFile(client.configPath)).toEqual(original); await expectCloseReclaimsHandle(host, closeAttempts, release); + expect(failure.cleanupIncomplete).toBe(false); + expect(failure.cleanupResources).toEqual([]); } finally { await cleanupHeldHandle(host, release); } diff --git a/tests/boundary/filesystem/setupLinuxCleanupOwnership.test.ts b/tests/boundary/filesystem/setupLinuxCleanupOwnership.test.ts new file mode 100644 index 0000000000..09c7890e22 --- /dev/null +++ b/tests/boundary/filesystem/setupLinuxCleanupOwnership.test.ts @@ -0,0 +1,132 @@ +import { mkdir, rm, stat } from "node:fs/promises"; +import { join } from "node:path"; +import { afterEach, expect, it, vi } from "vitest"; + +import { + installLinuxHopper, + systemLinuxHopperInstallHost, +} from "../../../src/application/LinuxHopper.js"; +import { systemSetupHost } from "../../../src/application/SetupHost.js"; +import { runSetup } from "../../../src/application/Setup.js"; +import { createDoctorHostFixture } from "../../../src/application/Doctor.fixture.js"; +import { AnalysisError } from "../../../src/domain/analysisErrorBase.js"; +import { ConfigurationError } from "../../../src/domain/configurationErrors.js"; +import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +vi.mock("../../../src/application/LinuxHopper.js", async (load) => { + const actual = + await load(); + return { + ...actual, + installLinuxHopper: vi.fn(actual.installLinuxHopper), + systemLinuxHopperInstallHost: vi.fn(actual.systemLinuxHopperInstallHost), + }; +}); + +afterEach(() => vi.resetAllMocks()); + +it.each(["settled", "late"] as const)( + "retains and releases the actual temporary directory through setup's Linux cleanup task: %s", + async (timing) => { + const home = await createTestTempDirectory("rea-setup-linux-cleanup-"); + const temporary = join(home, "owned-installer-staging"); + await mkdir(temporary); + let allowCleanup = false; + const actual = await vi.importActual< + typeof import("../../../src/application/LinuxHopper.js") + >("../../../src/application/LinuxHopper.js"); + const child = actual.systemLinuxHopperInstallHost( + { HOME: home }, + async () => undefined, + ); + vi.mocked(systemLinuxHopperInstallHost).mockReturnValue({ + ...child, + cleanup: async (path) => { + expect(path).toBe(temporary); + if (!allowCleanup) + throw new Error( + "Controlled owned temporary directory cleanup failure", + ); + await rm(path, { recursive: true }); + }, + }); + // Only the installer outcome is simulated; no package/download action runs. + let completeInstall: (() => void) | undefined; + const installGate = new Promise((resolve) => { + completeInstall = resolve; + }); + vi.mocked(installLinuxHopper).mockImplementation(async () => { + if (timing === "late") await installGate; + return { + status: "failed", + reason: "cleanup_failed", + cleanupFailure: { + path: temporary, + reason: "Controlled cleanup failure", + }, + }; + }); + const closeDoctor = vi.fn(async () => undefined); + const host = systemSetupHost( + createDoctorHostFixture({ + platform: "linux", + homeDirectory: home, + close: closeDoctor, + }), + { HOME: home }, + ); + try { + const installing = host.installHopper(false); + const closing = timing === "late" ? host.close?.() : undefined; + completeInstall?.(); + expect(await installing).toMatchObject({ status: "failed" }); + if (closing !== undefined) + expect((await closing)?.cleanupResources).toEqual([temporary]); + const primary = new ConfigurationError("Controlled state failure", { + settings: [{ setting: "fixture", constraint: "controlled" }], + }); + const failure = await runSetup( + { approved: true, structured: true, installHopper: false }, + { + ...host, + initialSetupState: async () => { + throw primary; + }, + }, + ).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toBeInstanceOf(AnalysisError); + if (!(failure instanceof AnalysisError)) + throw new Error("Expected typed cleanup failure"); + expect(projectAnalysisError(failure)).toMatchObject({ + code: "cleanup_incomplete", + details: { resources: [temporary] }, + }); + expect((await stat(temporary)).isDirectory()).toBe(true); + allowCleanup = true; + expect(await host.close?.()).toBeUndefined(); + expect(failure.cleanupIncomplete).toBe(false); + expect(closeDoctor).toHaveBeenCalledTimes(1); + expect(projectAnalysisError(failure)).toMatchObject( + projectAnalysisError(primary), + ); + expect(projectAnalysisError(failure).details).not.toHaveProperty( + "cleanup", + ); + expect(projectAnalysisError(failure).details).not.toHaveProperty( + "resources", + ); + expect(projectAnalysisError(failure).details).toHaveProperty( + "cleanup_history", + ); + await expect(stat(temporary)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + completeInstall?.(); + allowCleanup = true; + await host.close?.(); + } + }, +); diff --git a/tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts b/tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts index 77e988bca9..a7ac0e4dbd 100644 --- a/tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts +++ b/tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts @@ -22,6 +22,10 @@ import { } from "../../../src/application/SetupSkill.js"; import { RegularFileCleanupFailure } from "../../../src/filesystem/OwnedRegularFile.js"; import { systemSetupHost } from "../../../src/application/SetupHost.js"; +import { runSetup } from "../../../src/application/Setup.js"; +import { AnalysisError } from "../../../src/domain/analysisErrorBase.js"; +import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; +import { ConfigurationError } from "../../../src/domain/configurationErrors.js"; import { runUninstall, systemUninstallHost, @@ -68,7 +72,7 @@ const failFirstCloses = (path: string) => { }; }; -describe("skill regular-file cleanup ownership", () => { +describe("setup skill backup cleanup ownership", () => { it.each(["first", "recovery", "verification"] as const)( "retains a failed %s backup read until setup host disposal", async (stage) => { @@ -106,10 +110,62 @@ describe("skill regular-file cleanup ownership", () => { expect(openMock).toHaveBeenCalledTimes(opensBeforeAdmission); expect(new Set(close.attempts).size).toBe(1); expect(await actualFs.readFile(manifest, "utf8")).toBe(original); - expect(await host.close?.()).toContain(backup); + const report = await host.close?.(); + expect(report?.cleanup?.reason).toBe( + "test holds the skill handle open", + ); + expect(report?.cleanupResources).toEqual([backup]); + const primary = new ConfigurationError( + "Controlled setup state failure", + { + settings: [ + { setting: "fixture", constraint: "Controlled primary failure" }, + ], + }, + ); + const failure = await runSetup( + { approved: true, installHopper: false, structured: true }, + { + ...host, + initialSetupState: async () => { + throw primary; + }, + }, + ).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toBeInstanceOf(AnalysisError); + if (!(failure instanceof AnalysisError)) + throw new Error("Expected the typed setup and host cleanup failure"); + expect(projectAnalysisError(failure)).toMatchObject({ + code: "cleanup_incomplete", + details: { + resources: [backup], + diagnostics: { + cleanup_error: { details: { resources: [backup] } }, + }, + }, + }); + expect(failure.diagnostics?.primary_error).toEqual( + projectAnalysisError(primary), + ); const attemptsBeforeClose = close.attempts.length; close.release(); expect(await host.close?.()).toBeUndefined(); + expect(failure.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(failure)).toMatchObject( + projectAnalysisError(primary), + ); + expect(projectAnalysisError(failure).details).not.toHaveProperty( + "cleanup", + ); + expect(projectAnalysisError(failure).details).not.toHaveProperty( + "resources", + ); + expect(projectAnalysisError(failure).details).toHaveProperty( + "cleanup_history", + ); expect(close.attempts.length).toBeGreaterThan(attemptsBeforeClose); expect(close.attempts[0]?.fd).toBe(-1); } finally { @@ -120,7 +176,9 @@ describe("skill regular-file cleanup ownership", () => { } }, ); +}); +describe("skill regular-file cleanup ownership", () => { it("retains a failed setup skill read until setup host disposal", async () => { const home = await createTestTempDirectory("rea-setup-skill-close-"); const [destination] = skillDestinations(home, [], {}, "linux"); diff --git a/tests/process-global/setupConfigurationRollback.test.ts b/tests/process-global/setupConfigurationRollback.test.ts index e2182c03c1..36fc48f259 100644 --- a/tests/process-global/setupConfigurationRollback.test.ts +++ b/tests/process-global/setupConfigurationRollback.test.ts @@ -151,6 +151,7 @@ import { RegularFileCleanupFailure } from "../../src/filesystem/OwnedRegularFile import { OwnedFileHandle } from "../../src/filesystem/OwnedFileHandle.js"; import type { DoctorHost } from "../../src/application/Doctor.js"; import { systemSetupHost } from "../../src/application/SetupHost.js"; +import { AnalysisError } from "../../src/domain/analysisErrorBase.js"; import { systemUninstallHost } from "../../src/application/Uninstall.js"; import { ConfigurationError } from "../../src/domain/configurationErrors.js"; import { createTestTempDirectory } from "../fixtures/temporaryDirectory.js"; @@ -1315,7 +1316,16 @@ it.each(["setup", "uninstall"] as const)( ).status, ).toBe("failed"); } - expect(await host.close?.()).toContain("cleanup"); + const cleanupFailure = await host.close?.(); + if (operation === "setup") { + expect(cleanupFailure).toBeInstanceOf(AnalysisError); + const retainedRoots = (await fs.readdir(root)) + .filter((name) => name.startsWith(".rea-config-")) + .map((name) => join(root, name)); + expect((cleanupFailure as AnalysisError).cleanupResources).toEqual( + retainedRoots, + ); + } else expect(cleanupFailure).toContain("cleanup"); expect( (await fs.readdir(root)).filter((name) => name.startsWith(".rea-config-"), @@ -1323,6 +1333,8 @@ it.each(["setup", "uninstall"] as const)( ).toHaveLength(1); state.rejectStageRemoval = false; expect(await host.close?.()).toBeUndefined(); + if (cleanupFailure instanceof AnalysisError) + expect(cleanupFailure.cleanupIncomplete).toBe(false); expect( (await fs.readdir(root)).filter((name) => name.startsWith(".rea-config-"),