diff --git a/src/application/binary/BinarySessionLiveness.test.ts b/src/application/binary/BinarySessionLiveness.test.ts index 88915f4c5..92199de5c 100644 --- a/src/application/binary/BinarySessionLiveness.test.ts +++ b/src/application/binary/BinarySessionLiveness.test.ts @@ -116,9 +116,7 @@ describe("BinarySession open liveness", () => { expect(deep.created).toHaveLength(2); }); - it("falls back to the health probe when the child reports no operation health", async () => { - // Without an operation-health hook there is no child-liveness signal, - // so the probe keeps the previous `health` admission behavior. + it("probes instantiated children through health when snapshots are absent", async () => { const { session, deep } = openSession({ operationHealth: false }); expect((await session.open(process.execPath)).ok).toBe(true); expect((await session.execute("address_name", {})).ok).toBe(true); @@ -126,9 +124,33 @@ describe("BinarySession open liveness", () => { const second = await session.open(process.execPath); expect(second.ok).toBe(true); - // The composite's `health` answers ok, so the live client is reused. + // The composite probes this already-instantiated child rather than + // answering health without contacting it. expect(deep.created).toHaveLength(1); expect(deep.created[0]?.closeCalled).toBe(false); + expect(deep.created[0]?.healthCalls).toBe(1); + }); + + it("retires a dead no-snapshot child after its health probe fails", async () => { + const { session, deep } = openSession({ operationHealth: false }); + expect((await session.open(process.execPath)).ok).toBe(true); + expect((await session.execute("address_name", {})).ok).toBe(true); + const dead = deep.created[0]; + dead?.kill(); + + expect((await session.open(process.execPath)).ok).toBe(true); + expect(dead?.healthCalls).toBe(1); + expect(dead?.closeCalled).toBe(true); + expect(deep.created).toHaveLength(1); + expect((await session.execute("address_name", {})).ok).toBe(true); + expect(deep.created).toHaveLength(2); + }); + + it("does not instantiate a child to answer health before first use", async () => { + const { session, deep } = openSession({ operationHealth: false }); + expect((await session.open(process.execPath)).ok).toBe(true); + expect((await session.open(process.execPath)).ok).toBe(true); + expect(deep.created).toEqual([]); }); it("reports cancellation without touching the session when aborted mid-probe", async () => { @@ -167,6 +189,7 @@ describe("BinarySession open liveness", () => { interface KillableChild extends AnalysisClient { kill(state?: "exited" | "unreachable" | "unknown" | "not_started"): void; closeCalled: boolean; + healthCalls: number; } const deepIdentity: ProviderIdentity = { @@ -269,10 +292,12 @@ const killableDeepProvider = (options: { createClient: (): KillableChild => { let alive = true; let closeCalled = false; + let healthCalls = 0; let deadState: "exited" | "unreachable" | "unknown" | "not_started" = "exited"; const client: KillableChild = { execute: (operation) => { + if (operation === "health") healthCalls += 1; return alive ? Promise.resolve( ok( @@ -313,6 +338,9 @@ const killableDeepProvider = (options: { get closeCalled() { return closeCalled; }, + get healthCalls() { + return healthCalls; + }, kill: ( state: | "exited" diff --git a/src/application/binary/CompositeProvider.test.ts b/src/application/binary/CompositeProvider.test.ts new file mode 100644 index 000000000..4e94e0891 --- /dev/null +++ b/src/application/binary/CompositeProvider.test.ts @@ -0,0 +1,129 @@ +import { expect, it, vi } from "vitest"; + +import { + createAnalysisExecution, + type AnalysisClient, + type AnalysisOperation, + type AnalysisProvider, + type CapabilityDescriptor, + type ProviderIdentity, +} from "../AnalysisProvider.js"; +import { ProviderCleanupError } from "../../domain/providerCleanupError.js"; +import { err, ok } from "../../domain/result.js"; +import { CompositeProvider } from "./CompositeProvider.js"; + +const identity = (id: string): ProviderIdentity => ({ + id, + name: id, + version: "1", +}); + +const operationCapability = ( + provider: ProviderIdentity, + operation: Exclude, +): CapabilityDescriptor => ({ + provider, + operation, + available: true, + reason: null, + effects: { + mutatesArtifact: false, + launchesProcess: false, + mayShowUi: false, + mayAccessNetwork: false, + mayWriteFilesystem: false, + changesPermissions: false, + requiresRoot: false, + }, + limitations: [], +}); + +const failingProvider = ( + id: string, + operation: Exclude, + resource: string, +) => { + const providerIdentity = identity(id); + const cleanupError = new ProviderCleanupError(id, [resource], { + reason: `${id} cleanup is unresolved`, + child_marker: `${id}-detail`, + }); + const close = vi.fn(async () => err(cleanupError)); + const provider: AnalysisProvider = { + identity: () => providerIdentity, + capabilities: () => [operationCapability(providerIdentity, operation)], + createClient: (): AnalysisClient => ({ + execute: (name) => + Promise.resolve( + ok(createAnalysisExecution(`${id}:${name}`, providerIdentity)), + ), + close, + }), + }; + return { provider, close, cleanupError }; +}; + +const executableTarget = { + path: "/tmp/fixture", + sha256: "a".repeat(64), + kind: "executable", + format: "elf", + architecture: "x86_64", + availableArchitectures: ["x86_64"], +} as const; + +it("retains every failed child resource across composite cleanup retries", async () => { + const left = failingProvider("provider-left", "address_name", "left-db"); + const right = failingProvider("provider-right", "procedure_info", "right-db"); + const client = new CompositeProvider([ + left.provider, + right.provider, + ]).createClient(executableTarget); + + expect((await client.execute("address_name", {})).ok).toBe(true); + expect((await client.execute("procedure_info", {})).ok).toBe(true); + + for (let attempt = 0; attempt < 2; attempt += 1) { + const closed = await client.close(); + expect(closed.ok).toBe(false); + if (closed.ok) throw new Error("Expected composite cleanup failure"); + expect(closed.error).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["left-db", "right-db"], + diagnostics: { + failures: [ + { provider_id: "provider-left", resources: ["left-db"] }, + { provider_id: "provider-right", resources: ["right-db"] }, + ], + }, + }); + expect(closed.error).toBeInstanceOf(ProviderCleanupError); + expect(closed.error.diagnostics).toMatchObject({ + failures: [ + { + diagnostics: { reason: "provider-left cleanup is unresolved" }, + }, + { + diagnostics: { reason: "provider-right cleanup is unresolved" }, + }, + ], + }); + } + + expect(left.close).toHaveBeenCalledTimes(2); + expect(right.close).toHaveBeenCalledTimes(2); +}); + +it("preserves one child's typed cleanup error unchanged", async () => { + const child = failingProvider("provider-only", "address_name", "one-db"); + const client = new CompositeProvider([child.provider]).createClient( + executableTarget, + ); + expect((await client.execute("address_name", {})).ok).toBe(true); + + const closed = await client.close(); + + expect(closed.ok).toBe(false); + if (closed.ok) throw new Error("Expected child cleanup failure"); + expect(closed.error).toBe(child.cleanupError); +}); diff --git a/src/application/binary/CompositeProvider.ts b/src/application/binary/CompositeProvider.ts index 05184d279..faf3f8705 100644 --- a/src/application/binary/CompositeProvider.ts +++ b/src/application/binary/CompositeProvider.ts @@ -1,4 +1,6 @@ import { AnalysisCapabilityUnavailableError } from "../../domain/analysisErrorCore.js"; +import { ProviderCleanupError } from "../../domain/providerCleanupError.js"; +import { ProviderAdapterError } from "../../domain/providerAdapterError.js"; import { err, ok } from "../../domain/result.js"; import type { BinaryTarget } from "../../domain/binaryTargetTypes.js"; import type { AnalysisProfileCommitment } from "../../domain/analysisProfile.js"; @@ -7,6 +9,7 @@ import type { AnalysisClientContext, AnalysisProvider, CapabilityDescriptor, + ExecutionOptions, ProviderIdentity, } from "../AnalysisProvider.js"; import { closeAnalysisClient } from "./AnalysisClientCleanup.js"; @@ -24,6 +27,88 @@ const compositeProviderIdentity = ( version: null, }); +const closeCompositeClients = + ( + identity: ProviderIdentity, + clients: ReadonlyMap, + ): AnalysisClient["close"] => + async (options) => { + const outcomes = await Promise.all( + [...clients.entries()].map(([provider, client]) => + closeAnalysisClient(client, provider.identity().id, options).then( + (result) => ({ providerId: provider.identity().id, result }), + ), + ), + ); + const failures = outcomes.flatMap(({ providerId, result }) => + result.ok ? [] : [{ providerId, error: result.error }], + ); + if (failures.length === 0) return ok(null); + if (failures.length === 1) { + const failure = failures[0]; + if (failure !== undefined) return err(failure.error); + } + const resources = [ + ...new Set(failures.flatMap(({ error }) => error.cleanupResources)), + ]; + return err( + new ProviderCleanupError( + identity.id, + resources, + { + reason: + "Multiple composed provider clients could not be confirmed closed.", + failures: failures.map(({ providerId, error }) => ({ + provider_id: providerId, + message: error.message, + resources: [...error.cleanupResources], + ...(error.diagnostics === undefined + ? {} + : { diagnostics: error.diagnostics }), + })), + }, + { + cause: new AggregateError( + failures.map(({ error }) => error), + "Composed provider cleanup failed.", + ), + }, + ), + ); + }; + +const executeCompositeHealth = async ( + identity: ProviderIdentity, + clients: ReadonlyMap, + options: ExecutionOptions | undefined, +): ReturnType => { + for (const [provider, client] of clients) { + const health = client.operationHealthSnapshot?.(); + if ( + health !== undefined && + health.state !== "idle" && + health.state !== "busy" + ) + return err( + new ProviderAdapterError(provider.identity().id, "health", { + diagnostics: { + reason: `instantiated provider client is ${health.state}`, + }, + }), + ); + const checked = await client.execute("health", {}, options); + if (!checked.ok) return checked; + } + return ok({ + result: null, + rawResult: null, + provider: identity, + limitations: [], + locations: [], + subject: null, + }); +}; + /** Deterministically route disjoint operations without eagerly starting children. */ export class CompositeProvider implements AnalysisProvider { readonly #identity: ProviderIdentity; @@ -85,14 +170,7 @@ export class CompositeProvider implements AnalysisProvider { clients.set(provider, created); return created; }; - const close: AnalysisClient["close"] = async (options) => { - const outcomes = await Promise.all( - [...clients.entries()].map(([provider, client]) => - closeAnalysisClient(client, provider.identity().id, options), - ), - ); - return outcomes.find((outcome) => !outcome.ok) ?? ok(null); - }; + const close = closeCompositeClients(this.#identity, clients); return { executeIfReady: (operation, parameters, options) => { const provider = this.#providerByOperation.get(operation); @@ -103,18 +181,9 @@ export class CompositeProvider implements AnalysisProvider { Promise.resolve(undefined) ); }, - execute: (operation, parameters, options) => { + execute: async (operation, parameters, options) => { if (operation === "health") - return Promise.resolve( - ok({ - result: null, - rawResult: null, - provider: this.#identity, - limitations: [], - locations: [], - subject: null, - }), - ); + return executeCompositeHealth(this.#identity, clients, options); const provider = this.#providerByOperation.get(operation); return provider === undefined ? Promise.resolve( @@ -141,16 +210,16 @@ export class CompositeProvider implements AnalysisProvider { compareUnicodeCodePoints(left.provider.id, right.provider.id), ), operationHealthSnapshot: () => { - for (const client of clients.values()) { - const snapshot = client.operationHealthSnapshot?.(); - if (snapshot !== undefined && snapshot.state !== "idle") - return snapshot; - } - for (const client of clients.values()) { - const snapshot = client.operationHealthSnapshot?.(); - if (snapshot !== undefined) return snapshot; - } - return undefined; + const snapshots = [...clients.values()].map((client) => + client.operationHealthSnapshot?.(), + ); + if (snapshots.some((snapshot) => snapshot === undefined)) + return undefined; + return ( + snapshots.find( + (snapshot) => snapshot !== undefined && snapshot.state !== "idle", + ) ?? snapshots[0] + ); }, close, };