From 964decd989acfa84c4bfc3295362c4c46877fc20 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 18 Oct 2026 01:59:10 +0800 Subject: [PATCH] fix(providers): preserve live cleanup ownership through retries Share exact command/helper cleanup adoption across Java probes. Preserve complete live resource labels and explicit unknowns through inspection leases, and retain unresolved ownership when a cleanup capability is unavailable. Validation: focused cleanup, provider/doctor and Kuna consumer checks; shared Ghidra lifecycle and public doctor regressions. Native Windows/Linux provider behavior remains unverified. --- src/application/AnalysisProvider.ts | 9 +- src/application/Doctor.ts | 2 +- .../binary/AnalysisProviderRegistry.ts | 2 +- src/artifacts/AnalysisCleanupLease.test.ts | 31 +++++++ src/artifacts/AnalysisCleanupLease.ts | 45 ++++++++-- src/artifacts/ArtifactReader.ts | 7 +- src/artifacts/ArtifactResourceScope.ts | 7 +- src/ghidra/GhidraJavaProbe.ts | 62 ++----------- src/ghidra/GhidraProvider.ts | 2 +- src/kuna/KunaInstallation.ts | 12 ++- src/kuna/KunaSessionClient.ts | 3 +- src/providers/AnalysisClientCreation.ts | 2 +- src/providers/OwnedCommandCleanup.ts | 86 ++++++++++++++++++ .../providers/ownedCommandCleanup.test.ts | 87 +++++++++++++++++++ tests/fixtures/inspectionResources.ts | 5 +- 15 files changed, 288 insertions(+), 74 deletions(-) create mode 100644 src/providers/OwnedCommandCleanup.ts create mode 100644 tests/boundary/providers/ownedCommandCleanup.test.ts diff --git a/src/application/AnalysisProvider.ts b/src/application/AnalysisProvider.ts index bbcb8f768f..662b5e8fc8 100644 --- a/src/application/AnalysisProvider.ts +++ b/src/application/AnalysisProvider.ts @@ -205,7 +205,14 @@ export interface AnalysisClientContext { /** One lifecycle owner for descriptor cleanup and abortable inspection work. */ export interface FileCloseResourceOwner { - retain(owner: { close(): Promise }, resource: string): void; + retain( + owner: { + close(): Promise; + /** Complete labels authored by this exact owner; the display label is only a fallback. */ + readonly cleanupResources?: readonly string[] | undefined; + }, + resource: string, + ): void; run(operation: () => Promise): Promise; } diff --git a/src/application/Doctor.ts b/src/application/Doctor.ts index 538abf427b..1204391e17 100644 --- a/src/application/Doctor.ts +++ b/src/application/Doctor.ts @@ -510,7 +510,7 @@ export const systemDoctorHost = ( resources.retain({ kind: "lease", resource, - lease: new AnalysisCleanupLease(() => owner.close(), [resource]), + lease: AnalysisCleanupLease.forCloseOwner(owner, resource), }), run: (operation) => resources.run(operation), }; diff --git a/src/application/binary/AnalysisProviderRegistry.ts b/src/application/binary/AnalysisProviderRegistry.ts index cc6f67feba..1e4291f759 100644 --- a/src/application/binary/AnalysisProviderRegistry.ts +++ b/src/application/binary/AnalysisProviderRegistry.ts @@ -114,7 +114,7 @@ export class AnalysisProviderRegistry { this.#profileFiles.retain({ kind: "lease", resource, - lease: new AnalysisCleanupLease(() => owner.close(), [resource]), + lease: AnalysisCleanupLease.forCloseOwner(owner, resource), }), run: (operation) => this.#profileFiles.run(operation), }; diff --git a/src/artifacts/AnalysisCleanupLease.test.ts b/src/artifacts/AnalysisCleanupLease.test.ts index a5a0e3bc68..16ea53580f 100644 --- a/src/artifacts/AnalysisCleanupLease.test.ts +++ b/src/artifacts/AnalysisCleanupLease.test.ts @@ -111,3 +111,34 @@ it("resolves every live retry error bound to one lease", async () => { expect(firstError.cause).toBe(firstCause); expect(secondError.cause).toBe(secondCause); }); + +it("keeps explicit unknown labels separate from an absent owner label", () => { + const unknown = AnalysisCleanupLease.forCloseOwner( + { + close: async () => {}, + get cleanupResources() { + return undefined; + }, + }, + "display-only", + ); + const absent = AnalysisCleanupLease.forCloseOwner( + { close: async () => {} }, + "owned-path", + ); + expect(unknown.cleanupResources).toBeUndefined(); + expect(absent.cleanupResources).toEqual(["owned-path"]); +}); + +it.each([false, true])( + "preserves unknown resource labels in pending lease groups (known peer: %s)", + (knownPeer) => { + const unknown = new AnalysisCleanupLease(async () => {}); + const error = new ProviderCleanupError("fixture", [], {}); + const peers = knownPeer + ? [new AnalysisCleanupLease(async () => {}, ["known"]), unknown] + : [unknown]; + const group = new AnalysisCleanupLeaseGroup(error, peers); + expect(group.cleanupResources).toBeUndefined(); + }, +); diff --git a/src/artifacts/AnalysisCleanupLease.ts b/src/artifacts/AnalysisCleanupLease.ts index 90d363a57e..d2dc6d9170 100644 --- a/src/artifacts/AnalysisCleanupLease.ts +++ b/src/artifacts/AnalysisCleanupLease.ts @@ -1,4 +1,7 @@ -import type { AnalysisError } from "../domain/analysisErrorBase.js"; +import type { + AnalysisError, + AnalysisCleanupResourceOwner, +} from "../domain/analysisErrorBase.js"; /** Link one resource's confirmed release to its originating analysis error. */ export class AnalysisCleanupLease { @@ -11,14 +14,46 @@ export class AnalysisCleanupLease { return this.#released; } - readonly cleanupResources: readonly string[] | undefined; + readonly #resourceOwner: AnalysisCleanupResourceOwner | undefined; + readonly #resources: readonly string[] | undefined; + + get cleanupResources(): readonly string[] | undefined { + return this.#resourceOwner === undefined + ? this.#resources + : this.#resourceOwner.cleanupResources; + } + + /** Adopt exact close authority while retaining its live labels across retries. */ + static forCloseOwner( + owner: { + close(): Promise; + readonly cleanupResources?: readonly string[] | undefined; + }, + resource: string, + ): AnalysisCleanupLease { + return new AnalysisCleanupLease( + () => owner.close(), + !("cleanupResources" in owner) + ? [resource] + : { + get cleanupResources() { + return owner.cleanupResources; + }, + }, + ); + } constructor( private readonly cleanup: () => Promise, - resources?: readonly string[], + resources?: readonly string[] | AnalysisCleanupResourceOwner, ) { - this.cleanupResources = - resources === undefined ? undefined : [...resources]; + if (resources !== undefined && "cleanupResources" in resources) { + this.#resourceOwner = resources; + this.#resources = undefined; + } else { + this.#resourceOwner = undefined; + this.#resources = resources === undefined ? undefined : [...resources]; + } } async release(): Promise { diff --git a/src/artifacts/ArtifactReader.ts b/src/artifacts/ArtifactReader.ts index 7b2edcbb92..c88e696120 100644 --- a/src/artifacts/ArtifactReader.ts +++ b/src/artifacts/ArtifactReader.ts @@ -205,11 +205,14 @@ export class ArtifactReaderFailure extends Error { /** Give otherwise opaque cleanup failures a concrete owned resource. */ static cleanupObservation( cause: unknown, - resource: string, + resource: string | readonly string[], ): AnalysisCleanupObservation { if (cause instanceof ArtifactReaderFailure && cause.cleanup !== undefined) return cause.cleanup; - return { reason: errorMessage(cause), resources: [resource] }; + return { + reason: errorMessage(cause), + resources: typeof resource === "string" ? [resource] : [...resource], + }; } } diff --git a/src/artifacts/ArtifactResourceScope.ts b/src/artifacts/ArtifactResourceScope.ts index d726474aa2..af28aa7912 100644 --- a/src/artifacts/ArtifactResourceScope.ts +++ b/src/artifacts/ArtifactResourceScope.ts @@ -179,7 +179,12 @@ export class ArtifactResourceScope { } if (failures.length === 0) return; const observations = failures.map(({ owner, cause }) => - ArtifactReaderFailure.cleanupObservation(cause, owner.resource), + ArtifactReaderFailure.cleanupObservation( + cause, + owner.kind === "lease" && owner.lease instanceof AnalysisCleanupLease + ? (owner.lease.cleanupResources ?? owner.resource) + : owner.resource, + ), ); const resources = [ ...new Set(observations.flatMap(({ resources }) => resources)), diff --git a/src/ghidra/GhidraJavaProbe.ts b/src/ghidra/GhidraJavaProbe.ts index e843fa6361..2785d7b1a7 100644 --- a/src/ghidra/GhidraJavaProbe.ts +++ b/src/ghidra/GhidraJavaProbe.ts @@ -1,12 +1,11 @@ +import { retainOwnedCommandCleanup } from "../providers/OwnedCommandCleanup.js"; import { analysisClientFailureForOperation } from "../providers/AnalysisClientFailure.js"; import { randomUUID } from "node:crypto"; import { AnalysisError } from "../domain/analysisErrorBase.js"; import { AnalysisCancelledError } from "../domain/analysisErrorCore.js"; import { ProviderAdapterError } from "../domain/providerAdapterError.js"; -import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; import { jsonValueSchema } from "../domain/jsonValue.js"; -import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import type { FileCloseResourceOwner } from "../application/AnalysisProvider.js"; import { DarwinProcessOwnershipInspectionError } from "../process/DarwinProcessRunTokenReader.js"; import { @@ -207,14 +206,16 @@ const observeGhidraJava = async ( Object.defineProperty(interpreted.error, "cancellation", { value: commandFailure.cancellation, }); + const cleanups = retainOwnedCommandCleanup( + cause, + options.resourceOwner, + "ghidra", + "ghidra:java", + ); let primary: AnalysisError | undefined; if (interpreted !== undefined && !interpreted.ok) { // A completed, invalid report remains the primary fact across cleanup-time cancellation. - if ( - commandFailure?.cleanupOwner === undefined && - preparation?.cleanupFailure === undefined - ) - return interpreted; + if (cleanups.length === 0) return interpreted; primary = probeFailureError(interpreted.error, cause); } else if ( commandFailure?.reason === "cancelled" && @@ -264,53 +265,6 @@ const observeGhidraJava = async ( } else if (cause instanceof AnalysisError) { primary = cause; } - const cleanups: AnalysisError[] = []; - if (preparation?.cleanupFailure !== undefined) { - const cleanup = preparation.cleanupFailure; - const lease = new AnalysisCleanupLease(async () => { - if (!cleanup.cleanupIncomplete) return; - if (preparation.cleanupOwner === undefined) throw cleanup; - await preparation.cleanupOwner.close(); - if (cleanup.cleanupIncomplete) throw cleanup; - }, cleanup.cleanupResources); - options.resourceOwner.retain( - { close: () => lease.release() }, - cleanup.cleanupResources[0] ?? - "Java prerequisite ownership preparation", - ); - cleanups.push(cleanup); - } - if (commandFailure?.cleanupOwner !== undefined) { - const supervisor = commandFailure.cleanupOwner; - const cleanup = new ProviderCleanupError( - "ghidra", - commandFailure.resources, - { - reason: - commandFailure.cleanupFailure ?? - "Java prerequisite cleanup remains incomplete", - ...(commandFailure.cleanupResult === undefined - ? {} - : { - process_cleanup: jsonValueSchema.parse( - commandFailure.cleanupResult, - ), - }), - }, - { cause, operation: "ghidra:java" }, - ); - const lease = new AnalysisCleanupLease(async () => { - const stopped = await supervisor.stop(); - if (stopped.status === "incomplete") throw cleanup; - }, commandFailure.resources); - cleanup.bindCleanupResourceOwner(lease); - lease.bindCleanupError(cleanup); - options.resourceOwner.retain( - { close: () => lease.release() }, - commandFailure.resources[0] ?? "Java prerequisite process", - ); - cleanups.push(cleanup); - } if (primary === undefined) { const code = commandFailure?.reason === "timeout" diff --git a/src/ghidra/GhidraProvider.ts b/src/ghidra/GhidraProvider.ts index d5a25b36d0..1776eddbcc 100644 --- a/src/ghidra/GhidraProvider.ts +++ b/src/ghidra/GhidraProvider.ts @@ -113,7 +113,7 @@ export class GhidraProvider implements AnalysisProviderCandidate { this.#inspectionResources.retain({ kind: "lease", resource, - lease: new AnalysisCleanupLease(() => owner.close(), [resource]), + lease: AnalysisCleanupLease.forCloseOwner(owner, resource), }), run: (operation) => this.#inspectionResources.run(operation), }; diff --git a/src/kuna/KunaInstallation.ts b/src/kuna/KunaInstallation.ts index af5580c1d5..5c6604627d 100644 --- a/src/kuna/KunaInstallation.ts +++ b/src/kuna/KunaInstallation.ts @@ -92,9 +92,17 @@ export const createKunaFileCloseScope = ( return { owner: { retain: (owner, resource) => { - const lease = new AnalysisCleanupLease(() => owner.close(), [resource]); + const lease = AnalysisCleanupLease.forCloseOwner(owner, resource); leases.set(owner, lease); - parent.retain({ close: () => lease.release() }, resource); + parent.retain( + { + close: () => lease.release(), + get cleanupResources() { + return lease.cleanupResources ?? [resource]; + }, + }, + resource, + ); }, run: (operation) => parent.run(operation), }, diff --git a/src/kuna/KunaSessionClient.ts b/src/kuna/KunaSessionClient.ts index 5a75c9bada..cc75416f4d 100644 --- a/src/kuna/KunaSessionClient.ts +++ b/src/kuna/KunaSessionClient.ts @@ -1,3 +1,4 @@ +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import { z } from "zod"; import { @@ -1382,7 +1383,7 @@ export class KunaSessionClient implements AnalysisClient { this.#fileCloseResources.retain({ kind: "lease", resource, - lease: { release: () => owner.close() }, + lease: AnalysisCleanupLease.forCloseOwner(owner, resource), }), run: (operation: () => Promise) => this.#fileCloseResources.run(operation), diff --git a/src/providers/AnalysisClientCreation.ts b/src/providers/AnalysisClientCreation.ts index fb520e5577..6d09e5f340 100644 --- a/src/providers/AnalysisClientCreation.ts +++ b/src/providers/AnalysisClientCreation.ts @@ -26,7 +26,7 @@ export class AnalysisClientCreation { this.#inspectionResources.retain({ kind: "lease", resource, - lease: new AnalysisCleanupLease(() => owner.close(), [resource]), + lease: AnalysisCleanupLease.forCloseOwner(owner, resource), }), run: (operation) => this.#inspectionResources.run(operation), }; diff --git a/src/providers/OwnedCommandCleanup.ts b/src/providers/OwnedCommandCleanup.ts new file mode 100644 index 0000000000..0da5bc978b --- /dev/null +++ b/src/providers/OwnedCommandCleanup.ts @@ -0,0 +1,86 @@ +import { jsonValueSchema } from "../domain/jsonValue.js"; +import type { AnalysisError } from "../domain/analysisErrorBase.js"; +import type { FileCloseResourceOwner } from "../application/AnalysisProvider.js"; +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { DarwinProcessOwnershipInspectionError } from "../process/DarwinProcessRunTokenReader.js"; +import { OwnedCommandFailure } from "../process/OwnedCommand.js"; + +/** Transfer exact helper and command cleanup capabilities to the caller's scope. */ +export const retainOwnedCommandCleanup = ( + cause: unknown, + resourceOwner: FileCloseResourceOwner, + providerId: string, + operation: string, +): readonly AnalysisError[] => { + const commandFailure = + cause instanceof OwnedCommandFailure ? cause : undefined; + const preparation = + commandFailure?.preparationFailure ?? + (cause instanceof DarwinProcessOwnershipInspectionError + ? cause + : undefined); + const cleanups: AnalysisError[] = []; + if (preparation?.cleanupFailure !== undefined) { + const cleanup = preparation.cleanupFailure; + const lease = new AnalysisCleanupLease(async () => { + if (!cleanup.cleanupIncomplete) return; + if (preparation.cleanupOwner === undefined) throw cleanup; + await preparation.cleanupOwner.close(); + if (cleanup.cleanupIncomplete) throw cleanup; + }, cleanup); + resourceOwner.retain( + { + close: () => lease.release(), + get cleanupResources() { + return cleanup.cleanupResources; + }, + }, + cleanup.cleanupResources[0] ?? + "Provider prerequisite ownership preparation", + ); + cleanups.push(cleanup); + } + if ( + commandFailure !== undefined && + (commandFailure.cleanupOwner !== undefined || + commandFailure.cleanupFailure !== null) + ) { + const supervisor = commandFailure.cleanupOwner; + const cleanup = new ProviderCleanupError( + providerId, + commandFailure.resources, + { + reason: + commandFailure.cleanupFailure ?? + "Provider prerequisite cleanup remains incomplete", + ...(commandFailure.cleanupResult === undefined + ? {} + : { + process_cleanup: jsonValueSchema.parse( + commandFailure.cleanupResult, + ), + }), + }, + { cause, operation }, + ); + const lease = new AnalysisCleanupLease(async () => { + if (supervisor === undefined) throw cleanup; + const stopped = await supervisor.stop(); + if (stopped.status === "incomplete") throw cleanup; + }, commandFailure.resources); + cleanup.bindCleanupResourceOwner(lease); + lease.bindCleanupError(cleanup); + resourceOwner.retain( + { + close: () => lease.release(), + get cleanupResources() { + return cleanup.cleanupResources; + }, + }, + commandFailure.resources[0] ?? "Provider prerequisite process", + ); + cleanups.push(cleanup); + } + return cleanups; +}; diff --git a/tests/boundary/providers/ownedCommandCleanup.test.ts b/tests/boundary/providers/ownedCommandCleanup.test.ts new file mode 100644 index 0000000000..589d434ba3 --- /dev/null +++ b/tests/boundary/providers/ownedCommandCleanup.test.ts @@ -0,0 +1,87 @@ +import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; +import { DarwinProcessOwnershipInspectionError } from "../../../src/process/DarwinProcessRunTokenReader.js"; +import { expect, it } from "vitest"; +import { OwnedCommandFailure } from "../../../src/process/OwnedCommand.js"; +import { retainOwnedCommandCleanup } from "../../../src/providers/OwnedCommandCleanup.js"; +import { createInspectionResources } from "../../fixtures/inspectionResources.js"; +import { ArtifactResourceRetryFailure } from "../../../src/artifacts/ArtifactResourceScope.js"; + +it("retains every owned label without inventing a missing cleanup capability", async () => { + // Model unavailable capabilities only; these labels name no real resources. + const resources = createInspectionResources(); + const labels = ["controlled-run", "controlled-leader", "controlled-group"]; + const cause = new OwnedCommandFailure( + "process", + "cleanup unresolved", + null, + "cleanup capability unavailable", + undefined, + labels, + ); + const [cleanup] = retainOwnedCommandCleanup( + cause, + resources.owner, + "jadx", + "inspect_jadx_configuration", + ); + expect(cleanup?.cleanupResources).toEqual(labels); + for (let attempt = 0; attempt < 2; attempt++) { + const error: unknown = await resources + .close() + .catch((failure: unknown) => failure); + expect(error).toBeInstanceOf(ArtifactResourceRetryFailure); + if (!(error instanceof ArtifactResourceRetryFailure)) + throw new Error("Expected retained ownership"); + expect(error.cleanup?.resources).toEqual(labels); + expect(cleanup?.cleanupIncomplete).toBe(true); + } +}); + +it("reports only live helper peers across partial cleanup and retry", async () => { + const resources = createInspectionResources(); + let labels = ["helper-one", "helper-two"]; + const cleanup = new ProviderCleanupError("process-ownership", labels, { + reason: "mixed helper cleanup", + }).bindCleanupResourceOwner({ + get cleanupResources() { + return labels; + }, + }); + const preparation = new DarwinProcessOwnershipInspectionError( + "controlled preparation cleanup", + { + cleanupFailure: cleanup, + cleanupOwner: { + close: async () => { + if (labels.length > 1) { + labels = ["helper-two"]; + throw cleanup; + } + labels = []; + cleanup.resolveCleanup(); + }, + }, + }, + ); + retainOwnedCommandCleanup( + preparation, + resources.owner, + "jadx", + "inspect_jadx_configuration", + ); + try { + const failed: unknown = await resources + .close() + .catch((cause: unknown) => cause); + expect(failed).toBeInstanceOf(ArtifactResourceRetryFailure); + if (!(failed instanceof ArtifactResourceRetryFailure)) + throw new Error("Expected mixed cleanup"); + expect(failed.cleanup?.resources).toEqual(["helper-two"]); + await resources.close(); + expect(cleanup.cleanupIncomplete).toBe(false); + } finally { + labels = []; + cleanup.resolveCleanup(); + await resources.close(); + } +}); diff --git a/tests/fixtures/inspectionResources.ts b/tests/fixtures/inspectionResources.ts index cf56b912e0..952b3e43b4 100644 --- a/tests/fixtures/inspectionResources.ts +++ b/tests/fixtures/inspectionResources.ts @@ -10,10 +10,7 @@ export const createInspectionResources = () => { scope.retain({ kind: "lease", resource, - lease: new AnalysisCleanupLease( - () => resourceOwner.close(), - [resource], - ), + lease: AnalysisCleanupLease.forCloseOwner(resourceOwner, resource), }), run: (operation) => scope.run(operation), };