diff --git a/src/adb/AdbProvider.ts b/src/adb/AdbProvider.ts index 8c81573c43..399f271a51 100644 --- a/src/adb/AdbProvider.ts +++ b/src/adb/AdbProvider.ts @@ -378,6 +378,8 @@ interface PendingAdbDirectoryCleanup { | "install_adb_package"; readonly path: string; readonly primary: AnalysisError; + readonly reportedError: AnalysisError; + readonly prerequisite?: AnalysisError; readonly remove: () => Promise; } @@ -385,6 +387,7 @@ interface PendingAdbFileCleanup { readonly operation: string; readonly path: string; readonly owner: OwnedFileHandle; + readonly reportedError: AnalysisError; } interface OwnedDirectoryIdentity { @@ -583,8 +586,6 @@ export class AdbProvider { try { return await hashStableFile(path, signal); } catch (cause: unknown) { - if (cause instanceof OwnedRegularFileCleanupFailure) - this.#pendingFileCleanup.add({ operation, path, owner: cause.owner }); if (cause instanceof OwnedRegularFileCleanupFailure) { const primary = signal?.aborted ? new AnalysisCancelledError(operation, { cause: cause.cause }) @@ -594,10 +595,11 @@ export class AdbProvider { cause.cause instanceof Error ? cause.cause.message : String(cause.cause), - diagnostics: (cause instanceof StableFileHashCleanupFailure && + diagnostics: + cause instanceof StableFileHashCleanupFailure && cause.completedHash !== undefined ? { completed_hash: cause.completedHash } - : {}), + : {}, }); const cleanup = new ProviderCleanupError( ADB_PROVIDER_IDENTITY.id, @@ -614,7 +616,18 @@ export class AdbProvider { }, { operation, cause: cause.cleanupCause }, ); - throw analysisErrorWithCleanupFailure(primary, cleanup, operation); + const reported = analysisErrorWithCleanupFailure( + primary, + cleanup, + operation, + ); + this.#pendingFileCleanup.add({ + operation, + path, + owner: cause.owner, + reportedError: reported, + }); + throw reported; } if (signal?.aborted === true) throw new AnalysisCancelledError(operation, { cause }); @@ -676,6 +689,7 @@ export class AdbProvider { try { await owner.owner.close(); this.#pendingFileCleanup.delete(owner); + owner.reportedError.resolveCleanup(); } catch (cause: unknown) { failures.push( new ProviderCleanupError( @@ -688,9 +702,21 @@ export class AdbProvider { } } for (const owner of this.#pendingDirectoryCleanup) { + if (owner.prerequisite?.cleanupIncomplete === true) { + failures.push( + new ProviderCleanupError( + ADB_PROVIDER_IDENTITY.id, + [owner.path], + { reason: "A dependent owned resource still needs cleanup" }, + { operation: owner.operation, cause: owner.prerequisite }, + ), + ); + continue; + } try { await owner.remove(); this.#pendingDirectoryCleanup.delete(owner); + owner.reportedError.resolveCleanup(); } catch (cause: unknown) { const cleanup = new ProviderCleanupError( ADB_PROVIDER_IDENTITY.id, @@ -793,7 +819,13 @@ export class AdbProvider { } if (root !== undefined) { + const prerequisite = + !result.ok && result.error.cleanupIncomplete ? result.error : undefined; const remove = async () => { + if (prerequisite?.cleanupIncomplete === true) + throw new Error( + "A staged file still has an unresolved cleanup owner", + ); await verifyOwnedDirectory(root!.path, identity); await root!.close(); }; @@ -841,14 +873,15 @@ export class AdbProvider { operation, path: root.path, primary, + reportedError: + primary instanceof AnalysisCancelledError + ? primary + : analysisErrorWithCleanupFailure(primary, cleanup, operation), + ...(prerequisite === undefined ? {} : { prerequisite }), remove, }; this.#pendingDirectoryCleanup.add(cleanupOwner); - return err( - primary instanceof AnalysisCancelledError - ? primary - : analysisErrorWithCleanupFailure(primary, cleanup, operation), - ); + return err(cleanupOwner.reportedError); } } @@ -1650,14 +1683,6 @@ export class AdbProvider { { cause }, ); if (!packageDirectoryCreated) return err(primary); - const owner: PendingAdbDirectoryCleanup = { - operation, - path: packageDirectory, - primary, - remove: () => - removeOwnedDirectory(packageDirectory, packageDirectoryIdentity), - }; - this.#pendingDirectoryCleanup.add(owner); const cleanup = new ProviderCleanupError( ADB_PROVIDER_IDENTITY.id, [packageDirectory], @@ -1666,31 +1691,49 @@ export class AdbProvider { }, { operation, cause }, ); - return err(analysisErrorWithCleanupFailure(primary, cleanup, operation)); + const reported = analysisErrorWithCleanupFailure( + primary, + cleanup, + operation, + ); + this.#pendingDirectoryCleanup.add({ + operation, + path: packageDirectory, + primary, + reportedError: reported, + remove: () => + removeOwnedDirectory(packageDirectory, packageDirectoryIdentity), + }); + return err(reported); } const failOwnedPackage = async ( primary: AnalysisError, ): Promise> => { - const owner: PendingAdbDirectoryCleanup = { - operation, - path: packageDirectory, - primary, - remove: () => - removeOwnedDirectory(packageDirectory, packageDirectoryIdentity), - }; + const remove = () => + removeOwnedDirectory(packageDirectory, packageDirectoryIdentity); try { - await owner.remove(); + await remove(); } catch (cause: unknown) { - this.#pendingDirectoryCleanup.add(owner); const cleanup = new ProviderCleanupError( ADB_PROVIDER_IDENTITY.id, [packageDirectory], { reason: cause instanceof Error ? cause.message : String(cause) }, { operation, cause }, ); - return err( - analysisErrorWithCleanupFailure(primary, cleanup, operation), + const reported = analysisErrorWithCleanupFailure( + primary, + cleanup, + operation, ); + this.#pendingDirectoryCleanup.add({ + operation, + path: packageDirectory, + primary, + reportedError: reported, + prerequisite: primary, + remove, + }); + return err(reported); } return err(primary); }; @@ -1717,8 +1760,8 @@ export class AdbProvider { stage: "resolve_paths" | "pull" | "digest"; message: string; }[] = []; - const cancelledPackagePull = ( - error: AnalysisCancelledError, + const partialPackagePull = ( + error: AnalysisError, localPath: string, devicePath: string, stage: "pull" | "digest", @@ -1739,8 +1782,8 @@ export class AdbProvider { stage, message: stage === "pull" - ? `Pull cancelled; any incomplete file is retained at ${localPath}. Remaining APKs were not attempted.` - : `Digesting the pulled file was cancelled; the file is retained at ${localPath}. Remaining APKs were not attempted.`, + ? `${error instanceof AnalysisCancelledError ? "Pull cancelled" : "Pull failed"}; any incomplete file is retained at ${localPath}. Remaining APKs were not attempted.` + : `${error instanceof AnalysisCancelledError ? "Digesting the pulled file was cancelled" : "Digesting the pulled file failed"}; the file is retained at ${localPath}. Remaining APKs were not attempted.`, }, ], coverage: "partial", @@ -1760,7 +1803,7 @@ export class AdbProvider { ); if (!pulled.ok) { if (pulled.error instanceof AnalysisCancelledError) - return cancelledPackagePull( + return partialPackagePull( pulled.error, localPath, entry.device_path, @@ -1785,8 +1828,11 @@ export class AdbProvider { role_basis: "file_name", }); } catch (cause: unknown) { - if (cause instanceof AnalysisCancelledError) - return cancelledPackagePull( + if ( + cause instanceof AnalysisCancelledError || + (cause instanceof AnalysisError && cause.cleanupIncomplete) + ) + return partialPackagePull( cause, localPath, entry.device_path, @@ -1888,7 +1934,10 @@ export class AdbProvider { ), ); } catch (cause) { - if (cause instanceof AnalysisCancelledError) { + if ( + cause instanceof AnalysisCancelledError || + (cause instanceof AnalysisError && cause.cleanupIncomplete) + ) { cause.retainPartialObservation({ provider_id: "adb", operation, @@ -2629,7 +2678,10 @@ export class AdbProvider { }), ); } catch (cause) { - if (cause instanceof AnalysisCancelledError) { + if ( + cause instanceof AnalysisCancelledError || + (cause instanceof AnalysisError && cause.cleanupIncomplete) + ) { if (retainedObservation !== undefined) cause.retainPartialObservation(retainedObservation); return err(cause); @@ -2658,24 +2710,25 @@ export class AdbProvider { : collectedResult.error; if (retainedObservation !== undefined) primary.retainPartialObservation(retainedObservation); + const reported = analysisErrorWithCleanupFailure( + primary, + new ProviderCleanupError( + ADB_PROVIDER_IDENTITY.id, + [stagingDirectory], + { reason: cause instanceof Error ? cause.message : String(cause) }, + { operation, cause }, + ), + operation, + ); this.#pendingDirectoryCleanup.add({ operation, path: stagingDirectory, primary, + reportedError: reported, + prerequisite: primary, remove: () => removeOwnedDirectory(stagingDirectory, stagingIdentity), }); - return err( - analysisErrorWithCleanupFailure( - primary, - new ProviderCleanupError( - ADB_PROVIDER_IDENTITY.id, - [stagingDirectory], - { reason: cause instanceof Error ? cause.message : String(cause) }, - { operation, cause }, - ), - operation, - ), - ); + return err(reported); } return collectedResult; } diff --git a/tests/boundary/providers/adb/adbHashCleanupConsumers.test.ts b/tests/boundary/providers/adb/adbHashCleanupConsumers.test.ts new file mode 100644 index 0000000000..d4ac4e9e89 --- /dev/null +++ b/tests/boundary/providers/adb/adbHashCleanupConsumers.test.ts @@ -0,0 +1,148 @@ +import { readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { afterEach, expect, onTestFinished, test, vi } from "vitest"; + +import { AdbProvider } from "../../../../src/adb/AdbProvider.js"; +import { OwnedFileHandle } from "../../../../src/filesystem/OwnedFileHandle.js"; +import { + execFileBytes, + execFileOutput, +} from "../../../../src/process/ExecFileOutput.js"; +import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js"; +import { createTestTempDirectory } from "../../../fixtures/temporaryDirectory.js"; + +vi.mock( + "../../../../src/process/ExecFileOutput.js", + async (importOriginal) => ({ + ...(await importOriginal< + typeof import("../../../../src/process/ExecFileOutput.js") + >()), + execFileBytes: vi.fn(), + execFileOutput: vi.fn(), + }), +); + +afterEach(() => vi.restoreAllMocks()); + +const fixture = async () => { + const root = await createTestTempDirectory("rea-adb-hash-consumer-"); + const binary = join(root, "adb"); + vi.mocked(execFileOutput).mockImplementation(async () => { + const stdout = `Android Debug Bridge version 1.0.41\nVersion 34.0.5-stub\nInstalled as ${binary}\n`; + return { + stdout, + stderr: "", + stdoutBytes: Buffer.byteLength(stdout), + stderrBytes: 0, + }; + }); + vi.mocked(execFileBytes).mockImplementation(async (_binary, args) => { + const last = args.at(-1)!; + if (args.includes("pull")) { + await writeFile(last, "pulled bytes"); + return { stdout: Buffer.alloc(0), stderr: Buffer.alloc(0) }; + } + if (args.includes("path") && args.includes("pm")) + return { + stdout: Buffer.from("package:/data/app/example/base.apk\n"), + stderr: Buffer.alloc(0), + }; + if (args.includes("bugreport")) { + const staging = last; + const archive = join(staging, "report.zip"); + await writeFile(archive, "pulled bytes"); + return { + stdout: Buffer.from(`Bug report copied to ${archive}\n`), + stderr: Buffer.alloc(0), + }; + } + return { stdout: Buffer.alloc(0), stderr: Buffer.alloc(0) }; + }); + const provider = new AdbProvider({ environment: { REA_ADB_PATH: binary } }); + onTestFinished(() => provider.close()); + const failFirstClose = () => { + const originalClose = OwnedFileHandle.prototype.close; + let failed = false; + vi.spyOn(OwnedFileHandle.prototype, "close").mockImplementation( + async function (this: OwnedFileHandle) { + if (!failed) { + failed = true; + throw new Error("hash descriptor close denied"); + } + return originalClose.call(this); + }, + ); + }; + return { root, provider, failFirstClose }; +}; + +test.each([ + { + label: "single-file pull", + retainedPath: (root: string) => join(root, "payload.bin"), + request: (root: string) => ({ + operation: "pull_adb_file" as const, + input: { + serial: "device", + device_path: "/sdcard/payload.bin", + output_directory: root, + }, + }), + expected: { digest_status: "unknown", device_path: "/sdcard/payload.bin" }, + }, + { + label: "package pull", + retainedPath: (root: string) => join(root, "example", "base.apk"), + request: (root: string) => ({ + operation: "pull_adb_package" as const, + input: { + serial: "device", + package: "example", + output_directory: root, + }, + }), + expected: { + package_name: "example", + coverage: "partial", + failures: [ + { device_path: "/data/app/example/base.apk", stage: "digest" }, + ], + }, + }, + { + label: "bugreport collection", + retainedPath: (root: string) => join(root, "report.zip"), + request: (root: string) => ({ + operation: "collect_adb_bugreport" as const, + input: { serial: "device", output_directory: root }, + }), + expected: { digest_status: "unknown", serial: "device" }, + }, +])( + "preserves the linked hash cleanup error through $label", + async ({ request, expected, retainedPath }) => { + const f = await fixture(); + f.failFirstClose(); + const result = await f.provider.execute(request(f.root)); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected hash descriptor cleanup failure"); + const returnedError = result.error; + const originalCause = returnedError.cause; + expect(returnedError.cleanupIncomplete).toBe(true); + expect(returnedError.diagnostics).toMatchObject({ + primary_error: { + details: { diagnostics: { completed_hash: { bytes: 12 } } }, + }, + }); + expect( + projectAnalysisError(returnedError).details?.partial_observation, + ).toMatchObject({ result: expected }); + + await f.provider.close(); + expect(returnedError.cleanupIncomplete).toBe(false); + expect(returnedError.cause).toBe(originalCause); + await expect(readFile(retainedPath(f.root))).resolves.toEqual( + Buffer.from("pulled bytes"), + ); + }, +); diff --git a/tests/boundary/providers/adb/adbInputSnapshotOwnership.test.ts b/tests/boundary/providers/adb/adbInputSnapshotOwnership.test.ts index d958f80227..2dc8d312c1 100644 --- a/tests/boundary/providers/adb/adbInputSnapshotOwnership.test.ts +++ b/tests/boundary/providers/adb/adbInputSnapshotOwnership.test.ts @@ -11,6 +11,8 @@ import { basename, dirname, join } from "node:path"; import { afterEach, expect, onTestFinished, test, vi } from "vitest"; import { AdbProvider } from "../../../../src/adb/AdbProvider.js"; +import { AnalysisError } from "../../../../src/domain/analysisErrorBase.js"; +import { OwnedFileHandle } from "../../../../src/filesystem/OwnedFileHandle.js"; import { execFileBytes, execFileOutput, @@ -117,6 +119,7 @@ test("retains failed snapshot cleanup ownership and retries it on close", async const result = await f.provider.execute(f.request); expect(result.ok).toBe(false); if (result.ok) throw new Error("expected snapshot cleanup failure"); + expect(result.error.cleanupIncomplete).toBe(true); expect(result.error.cleanupResources).toContain(dirname(f.snapshotPath())); const evidence = parseEvidence(result.error.partialObservation); expect(evidence).toMatchObject({ @@ -134,12 +137,102 @@ test("retains failed snapshot cleanup ownership and retries it on close", async await readFile(f.source), ); + vi.mocked(rm).mockImplementationOnce(async (path, options) => { + if (basename(String(path)).startsWith("rea-adb-input-")) + throw new Error("snapshot cleanup retry denied"); + return actualFs.rm(path, options); + }); + await expect(f.provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); + expect(result.error.cleanupIncomplete).toBe(true); + await f.provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); await expect(lstat(dirname(f.snapshotPath()))).rejects.toMatchObject({ code: "ENOENT", }); }); +test("resolves a hash cleanup error only after the exact retained descriptor closes", async () => { + const f = await fixture(); + const actualFs = + await vi.importActual( + "node:fs/promises", + ); + let stagedPath: string | undefined; + vi.mocked(copyFile).mockImplementationOnce( + async (source, destination, mode) => { + stagedPath = String(destination); + await actualFs.copyFile(source, destination, mode); + }, + ); + const closeFailure = new Error("digest descriptor close denied"); + const owners: OwnedFileHandle[] = []; + let denialsRemaining = 2; + const originalClose = OwnedFileHandle.prototype.close; + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(async function (this: OwnedFileHandle) { + owners.push(this); + if (denialsRemaining > 0) { + denialsRemaining -= 1; + throw closeFailure; + } + return originalClose.call(this); + }); + try { + const result = await f.provider.execute(f.request); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected digest cleanup failure"); + const reported = result.error; + const originalCause = reported.cause; + expect(reported.cleanupIncomplete).toBe(true); + expect(reported.cause).toBeInstanceOf(AggregateError); + const hashFailure = (reported.cause as AggregateError).errors[0]; + if (!(hashFailure instanceof AnalysisError)) + throw new Error("Missing original typed hash failure"); + expect(hashFailure).toMatchObject({ + diagnostics: { + primary_error: { + details: { + diagnostics: { + completed_hash: { bytes: 17, sha256: f.digest }, + }, + }, + }, + }, + }); + expect(owners).toHaveLength(1); + if (stagedPath === undefined) throw new Error("Missing staged input path"); + + await expect(f.provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + cleanupResources: [stagedPath, dirname(stagedPath)], + }); + expect(reported.cleanupIncomplete).toBe(true); + expect(reported.cause).toBe(originalCause); + await expect(readFile(stagedPath)).resolves.toEqual( + await readFile(f.source), + ); + + await f.provider.close(); + expect(owners).toHaveLength(3); + expect(owners[0]).toBe(owners[1]); + expect(owners[1]).toBe(owners[2]); + expect(reported.cleanupIncomplete).toBe(false); + expect(hashFailure.cleanupIncomplete).toBe(false); + expect(reported.cause).toBe(originalCause); + await expect(lstat(dirname(stagedPath))).rejects.toMatchObject({ + code: "ENOENT", + }); + } finally { + denialsRemaining = 0; + close.mockRestore(); + await f.provider.close(); + } +}); + test.each([false, true])( "preserves cancellation and push receipt during final snapshot cleanup (cleanup failure: %s)", async (failCleanup) => { @@ -163,6 +256,7 @@ test.each([false, true])( expect(result.ok).toBe(false); if (result.ok) throw new Error("expected cancellation"); expect(result.error._tag).toBe("AnalysisCancelledError"); + const originalCause = result.error.cause; const evidence = parseEvidence(result.error.partialObservation); expect(evidence).toMatchObject({ provider: { id: "adb" }, @@ -176,13 +270,16 @@ test.each([false, true])( device_path: "/data/local/tmp/payload.bin", }, }); - if (failCleanup) + if (failCleanup) { + expect(result.error.cleanupIncomplete).toBe(true); expect(result.error.cleanupResources).toContain( dirname(f.snapshotPath()), ); - else expect(result.error.cleanupIncomplete).toBe(false); + } else expect(result.error.cleanupIncomplete).toBe(false); expect(await readFile(f.source)).toEqual(Buffer.from("immutable payload")); await f.provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cause).toBe(originalCause); }, );