From 300905c11be1c25b98fda9e2b71dfa899fc2831c Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:19:12 +0800 Subject: [PATCH] fix(android): retain archive admission ownership through cleanup retries Borrow the archive descriptor from the canonical artifact file workflow. Preserve primary configuration failures, completed archive facts and exact cancellation through final close, retain failed descriptor leases, and join provider and doctor shutdown without nested resource admission. Validation: three baseline failed-close regressions; 65 passing tests across seven files, nine real CLI and nine real MCP tiny APK inspections, check:fast, docs and discovery. Two read-only reviews clear. Bounded equivalent workflow costs recorded without a speedup claim; no actual OS close fault or native Windows/Linux qualification. Closes #4846 --- src/android/JadxConfiguration.ts | 207 +++++++++-- .../android/jadxArchiveLifecycle.test.ts | 321 ++++++++++++++++++ .../process/jadxJavaLifecycle.test.ts | 26 +- 3 files changed, 508 insertions(+), 46 deletions(-) create mode 100644 tests/boundary/android/jadxArchiveLifecycle.test.ts diff --git a/src/android/JadxConfiguration.ts b/src/android/JadxConfiguration.ts index 37b20f4e15..bcc93ad236 100644 --- a/src/android/JadxConfiguration.ts +++ b/src/android/JadxConfiguration.ts @@ -11,12 +11,24 @@ import { systemWindowsNativeAuthority } from "../windows/WindowsNativeLoader.js" import { providerStartupTimeoutSchema } from "../config/providerStartupTimeout.js"; import { AnalysisCancelledError, + AnalysisTimeoutError, AnalysisCapabilityUnavailableError, } from "../domain/analysisErrorCore.js"; import type { AndroidOperation } from "../domain/android/androidAnalysis.js"; import type { JsonValue } from "../domain/jsonValue.js"; import { err, ok, type Result } from "../domain/result.js"; import type { ProviderAvailability } from "../application/AnalysisProvider.js"; +import { AnalysisError } from "../domain/analysisErrorBase.js"; +import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; +import { + ArtifactFileCancelled, + ArtifactFileCleanupFailure, + bindArtifactFileCleanup, + withArtifactRegularFile, +} from "../artifacts/ArtifactFileOperation.js"; +import { analysisClientFailureForOperation } from "../providers/AnalysisClientFailure.js"; import { ZipArtifactReader } from "../artifacts/ZipArtifactReader.js"; import type { DoctorProviderCheck } from "../application/Doctor.js"; import { @@ -275,11 +287,11 @@ const validateJadxConfigurationContents = async ( ): Promise => { throwIfJadxInterrupted(options.signal); await inspectJavaRuntime(configuration.java, environment, options); - await requireEngineArchive(configuration.jar, options.signal); + await requireEngineArchive(configuration.jar, options); }; const projectConfigurationFailure = ( - operation: AndroidOperation, + operation: string, cause: JadxConfigurationFailure, ): AnalysisCapabilityUnavailableError => { const output = cause.diagnostics; @@ -311,49 +323,170 @@ const projectConfigurationFailure = ( const requireEngineArchive = async ( jar: string, - signal?: AbortSignal, + options: JadxJavaProbeOptions, ): Promise => { - // These classes are consumed directly by ReaJadxBridge.java. This checks the - // classpath inventory without loading engine code or starting a JADX session. - const missing = new Set([ - "com/atxx/jhmcp/JadxSession.class", - "com/atxx/jhmcp/SessionHolder.class", - "jadx/api/JadxDecompiler.class", - "com/google/gson/Gson.class", - "io/modelcontextprotocol/kotlin/sdk/server/Server.class", - ]); - const reader = new ZipArtifactReader(jar, "zip", JAR_METADATA_READ_MAX_BYTES); + const operation = options.operation ?? "inspect_jadx_configuration"; try { - for await (const entry of reader.entries(signal)) { - if (entry.kind === "file" && !entry.encrypted) missing.delete(entry.path); - if (missing.size === 0) return; - } - } catch (cause) { - if (signal?.aborted === true) throw cause; - const reason = cause instanceof Error ? cause.message : String(cause); - throw configurationFailure( - "version_unresolved", - `Cannot inspect REA_JADX_MCP_JAR ${jar} as a Java archive: ${reason}. ${JADX_JAR_CONFIGURATION_REMEDIATION}`, - { - jar_path: jar, - phase: "jar-inspection", - archive_error: reason, - maximum_metadata_read_bytes: JAR_METADATA_READ_MAX_BYTES, - }, - "jar", - cause, + await options.resourceOwner.run(() => + withArtifactRegularFile( + jar, + { + retain: ({ lease, resource }) => + options.resourceOwner.retain( + { + close: () => lease.release(), + }, + resource, + ), + }, + options.signal, + async (handle) => { + try { + // Metadata enumeration borrows this descriptor. The file workflow + // alone owns admission, final close, cancellation and failed retry. + const reader = new ZipArtifactReader( + jar, + "zip", + JAR_METADATA_READ_MAX_BYTES, + { handle, initial: await handle.stat() }, + ); + const missing = new Set([ + "com/atxx/jhmcp/JadxSession.class", + "com/atxx/jhmcp/SessionHolder.class", + "jadx/api/JadxDecompiler.class", + "com/google/gson/Gson.class", + "io/modelcontextprotocol/kotlin/sdk/server/Server.class", + ]); + for await (const entry of reader.entries(options.signal)) { + if (entry.kind === "file" && !entry.encrypted) + missing.delete(entry.path); + if (missing.size === 0) return; + } + throw configurationFailure( + "unsupported_version", + `REA_JADX_MCP_JAR ${jar} does not contain the required metadata bridge classes: ${[...missing].join(", ")}. ${JADX_JAR_CONFIGURATION_REMEDIATION}`, + { + jar_path: jar, + phase: "jar-inspection", + missing_classes: [...missing], + }, + "jar", + ); + } catch (cause: unknown) { + if ( + cause instanceof JadxConfigurationFailure || + (cause instanceof ArtifactReaderFailure && + cause.reason === "cancelled") + ) + throw cause; + throw archiveInspectionFailure(jar, cause); + } + }, + ), ); - } finally { - await reader.close(); + } catch (cause: unknown) { + if (cause instanceof ArtifactFileCleanupFailure) { + const physical = cause.fileCleanupFailure; + const cleanup = new ProviderCleanupError( + "jadx", + [jar], + { + phase: "jar-inspection", + ...(physical.outcome.kind === "completed" + ? { archive_observation: { jar_path: jar, required_classes: true } } + : {}), + }, + { operation, cause: physical.cleanupCause }, + ); + bindArtifactFileCleanup(cause, cleanup); + const primary = + physical.outcome.kind === "failed" + ? archiveAnalysisFailure(jar, physical.outcome.cause, options) + : physical.completionCancellation === undefined + ? undefined + : archiveCancellation( + physical.completionCancellation.reason, + options, + { + archive_observation: { + jar_path: jar, + required_classes: true, + }, + }, + ); + if (primary === undefined) throw cleanup; + throw analysisErrorWithCleanupFailure( + primary, + cleanup, + operation, + ).bindCleanupDependencies([primary, cleanup]); + } + if (cause instanceof ArtifactFileCancelled) { + throw archiveCancellation(cause.fileCancellation.reason, options, { + archive_observation: { jar_path: jar, required_classes: true }, + }); + } + if (cause instanceof ArtifactReaderFailure) { + if (cause.reason === "cancelled") + throw archiveCancellation(cause.cancellation?.reason, options); + throw analysisClientFailureForOperation(cause, "jadx", operation); + } + if ( + cause instanceof AnalysisError || + cause instanceof JadxConfigurationFailure + ) + throw cause; + throw archiveInspectionFailure(jar, cause); } - throw configurationFailure( - "unsupported_version", - `REA_JADX_MCP_JAR ${jar} does not contain the required metadata bridge classes: ${[...missing].join(", ")}. ${JADX_JAR_CONFIGURATION_REMEDIATION}`, - { jar_path: jar, phase: "jar-inspection", missing_classes: [...missing] }, +}; + +const archiveInspectionFailure = (jar: string, cause: unknown) => { + const reason = cause instanceof Error ? cause.message : String(cause); + return configurationFailure( + "version_unresolved", + `Cannot inspect REA_JADX_MCP_JAR ${jar} as a Java archive: ${reason}. ${JADX_JAR_CONFIGURATION_REMEDIATION}`, + { + jar_path: jar, + phase: "jar-inspection", + archive_error: reason, + maximum_metadata_read_bytes: JAR_METADATA_READ_MAX_BYTES, + }, "jar", + cause, ); }; +const archiveAnalysisFailure = ( + jar: string, + cause: unknown, + options: JadxJavaProbeOptions, +): AnalysisError => { + if (cause instanceof AnalysisError) return cause; + if (cause instanceof ArtifactReaderFailure && cause.reason === "cancelled") + return archiveCancellation(cause.cancellation?.reason, options); + return projectConfigurationFailure( + options.operation ?? "inspect_jadx_configuration", + cause instanceof JadxConfigurationFailure + ? cause + : archiveInspectionFailure(jar, cause), + ); +}; + +const archiveCancellation = ( + reason: unknown, + options: JadxJavaProbeOptions, + diagnostics?: Readonly>, +) => { + const operation = options.operation ?? "inspect_jadx_configuration"; + return options.operationDeadline?.signal.aborted === true && + reason === options.operationDeadline.signal.reason + ? new AnalysisTimeoutError(operation, options.operationDeadline.timeoutMs, { + cause: reason, + diagnostics, + }) + : new AnalysisCancelledError(operation, { cause: reason, diagnostics }); +}; + const requireSupportedHost = (): void => { if (process.platform === "linux" || process.platform === "darwin") return; if (process.platform === "win32" && process.arch === "x64") { diff --git a/tests/boundary/android/jadxArchiveLifecycle.test.ts b/tests/boundary/android/jadxArchiveLifecycle.test.ts new file mode 100644 index 0000000000..f7e4850f46 --- /dev/null +++ b/tests/boundary/android/jadxArchiveLifecycle.test.ts @@ -0,0 +1,321 @@ +import { inspectJadxProvider } from "../../../src/android/JadxDoctor.js"; +import { createInspectionResources } from "../../fixtures/inspectionResources.js"; +import { stat, writeFile } from "node:fs/promises"; +import { expect, it as test, vi } from "vitest"; +import { createJadxProtocolFixture } from "../../fixtures/android/jadx.js"; +import { writeOrderedZip } from "../../fixtures/artifactEntryOrder.js"; +import { OwnedFileHandle } from "../../../src/filesystem/OwnedFileHandle.js"; +import { AnalysisError } from "../../../src/domain/analysisErrorBase.js"; + +const it = test.skipIf(process.platform === "win32"); + +it.each(["valid", "malformed", "missing-classes"] as const)( + "retains exact archive cleanup and its %s admission outcome through retry", + async (inventory) => { + const f = await createJadxProtocolFixture(); + if (inventory === "malformed") await writeFile(f.jar, "not a zip"); + if (inventory === "missing-classes") + await writeOrderedZip(f.jar, ["unrelated/Library.class"]); + const identity = await stat(f.jar); + const original = OwnedFileHandle.prototype.close; + const handles = new Set(); + let allowClose = false; + const closeFailure = new Error("injected archive descriptor close failure"); + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(async function (this: OwnedFileHandle) { + if (!handles.has(this) && this.handle.fd >= 0) { + const observed = await this.handle.stat(); + if (observed.dev === identity.dev && observed.ino === identity.ino) + handles.add(this); + } + if (handles.has(this) && !allowClose) throw closeFailure; + await original.call(this); + }); + try { + const error: unknown = await f.provider + .inspectAvailability() + .catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(AnalysisError); + if (!(error instanceof AnalysisError)) + throw new Error("Expected retained archive cleanup"); + expect(error.cleanupIncomplete).toBe(true); + expect(error.cleanupResources).toEqual([f.jar]); + expect(handles.size).toBe(1); + expect([...handles][0]?.handle.fd).toBeGreaterThanOrEqual(0); + if (inventory === "valid") { + expect(error.diagnostics).toMatchObject({ + archive_observation: { jar_path: f.jar, required_classes: true }, + }); + } else { + expect(error.cleanupPrimary).toMatchObject({ + _tag: "AnalysisCapabilityUnavailableError", + diagnostics: { + failure_kind: "jar", + failure_code: + inventory === "malformed" + ? "version_unresolved" + : "unsupported_version", + }, + }); + } + const probes = await f.javaProbeCount(); + const refused: unknown = await f.provider + .inspectAvailability() + .catch((cause: unknown) => cause); + expect(refused).toMatchObject({ cleanupIncomplete: true }); + expect(await f.javaProbeCount()).toBe(probes); + const shutdown: unknown = await f.provider + .close() + .catch((cause: unknown) => cause); + expect(shutdown).toMatchObject({ cleanupIncomplete: true }); + expect(f.launches).toEqual([]); + allowClose = true; + await f.provider.close(); + expect([...handles].every((owner) => owner.handle.fd === -1)).toBe(true); + expect(error.cleanupIncomplete).toBe(false); + expect(error.cleanupResources).toEqual([]); + if (refused instanceof AnalysisError) + expect(refused.cleanupIncomplete).toBe(false); + if (shutdown instanceof AnalysisError) + expect(shutdown.cleanupIncomplete).toBe(false); + } finally { + allowClose = true; + close.mockRestore(); + await f.provider.close(); + for (const owner of handles) await original.call(owner); + } + }, +); + +it.each([ + { valid: true, failsClose: false }, + { valid: true, failsClose: true }, + { valid: false, failsClose: false }, + { valid: false, failsClose: true }, +])( + "owns late cancellation during archive close (%j)", + async ({ valid, failsClose }) => { + const f = await createJadxProtocolFixture(); + if (!valid) await writeOrderedZip(f.jar, ["unrelated.class"]); + const caller = new AbortController(); + const reason = { caller: "during final archive close" }; + const identity = await stat(f.jar); + const original = OwnedFileHandle.prototype.close; + let owner: OwnedFileHandle | undefined; + let allowClose = false; + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(async function (this: OwnedFileHandle) { + const observed = await this.handle.stat(); + if (observed.dev === identity.dev && observed.ino === identity.ino) { + owner = this; + caller.abort(reason); + if (failsClose && !allowClose) + throw new Error("injected close failure after cancellation"); + } + await original.call(this); + }); + try { + const outcome: unknown = await f.provider + .inspectAvailability(caller.signal) + .catch((cause: unknown) => cause); + expect(owner).toBeDefined(); + if (!failsClose && !valid) { + expect(outcome).toMatchObject({ + status: "unavailable", + code: "unsupported_version", + }); + } else { + expect(outcome).toBeInstanceOf(AnalysisError); + if (!(outcome instanceof AnalysisError)) + throw new Error("Expected owned lifecycle outcome"); + const primary = failsClose ? outcome.cleanupPrimary : outcome; + if (valid) { + expect(primary).toMatchObject({ _tag: "AnalysisCancelledError" }); + expect(primary?.cause).toBe(reason); + expect(primary?.diagnostics).toMatchObject({ + archive_observation: { required_classes: true }, + }); + } else { + expect(primary).toMatchObject({ + _tag: "AnalysisCapabilityUnavailableError", + diagnostics: { failure_code: "unsupported_version" }, + }); + } + expect(outcome.cleanupIncomplete).toBe(failsClose); + allowClose = true; + await f.provider.close(); + expect(outcome.cleanupIncomplete).toBe(false); + } + expect(f.launches).toEqual([]); + expect(owner?.handle.fd).toBe(-1); + } finally { + allowClose = true; + close.mockRestore(); + await f.provider.close(); + if (owner !== undefined) await original.call(owner); + } + }, +); + +it("resolves mixed archive owners independently without reclosing a released peer", async () => { + const f = await createJadxProtocolFixture(); + const identity = await stat(f.jar); + const original = OwnedFileHandle.prototype.close; + const handles: OwnedFileHandle[] = []; + const attempts = new Map(); + const allowed = new Set(); + let release: () => void = () => {}; + const held = new Promise((resolve) => { + release = resolve; + }); + let entered: () => void = () => {}; + const bothClosing = new Promise((resolve) => { + entered = resolve; + }); + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(async function (this: OwnedFileHandle) { + if (!handles.includes(this) && this.handle.fd >= 0) { + const observed = await this.handle.stat(); + if (observed.dev === identity.dev && observed.ino === identity.ino) + handles.push(this); + } + if (handles.includes(this)) { + attempts.set(this, (attempts.get(this) ?? 0) + 1); + if (handles.length === 2) entered(); + await held; + if (!allowed.has(this)) + throw new Error("injected concurrent archive close failure"); + } + await original.call(this); + }); + const first = f.provider + .inspectAvailability() + .catch((cause: unknown) => cause); + const second = f.provider + .inspectAvailability() + .catch((cause: unknown) => cause); + try { + await bothClosing; + release(); + const errors = await Promise.all([first, second]); + expect( + errors.every( + (error) => error instanceof AnalysisError && error.cleanupIncomplete, + ), + ).toBe(true); + const [released, pending] = handles; + if (released === undefined || pending === undefined) + throw new Error("Expected two exact archive owners"); + allowed.add(released); + const aggregate: unknown = await f.provider + .close() + .catch((cause: unknown) => cause); + expect(aggregate).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: [f.jar], + }); + expect( + errors.filter( + (error) => error instanceof AnalysisError && error.cleanupIncomplete, + ), + ).toHaveLength(1); + expect(released.handle.fd).toBe(-1); + expect(pending.handle.fd).toBeGreaterThanOrEqual(0); + const releasedAttempts = attempts.get(released); + allowed.add(pending); + await f.provider.close(); + expect(attempts.get(released)).toBe(releasedAttempts); + expect( + errors.every( + (error) => error instanceof AnalysisError && !error.cleanupIncomplete, + ), + ).toBe(true); + if (!(aggregate instanceof AnalysisError)) + throw new Error("Expected retained aggregate cleanup"); + expect(aggregate.cleanupIncomplete).toBe(false); + expect(aggregate.cleanupResources).toEqual([]); + expect(f.launches).toEqual([]); + } finally { + release(); + close.mockRestore(); + await Promise.allSettled([first, second]); + await f.provider.close(); + for (const owner of handles) await original.call(owner); + } +}); + +it("joins independent doctor archive finalization and retries its exact failed close", async () => { + const f = await createJadxProtocolFixture(); + const resources = createInspectionResources(); + const identity = await stat(f.jar); + const original = OwnedFileHandle.prototype.close; + let owner: OwnedFileHandle | undefined; + let allowClose = false; + let entered: () => void = () => {}; + const closing = new Promise((resolve) => { + entered = resolve; + }); + let release: () => void = () => {}; + const held = new Promise((resolve) => { + release = resolve; + }); + const close = vi + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(async function (this: OwnedFileHandle) { + const observed = await this.handle.stat(); + if (observed.dev === identity.dev && observed.ino === identity.ino) { + owner = this; + entered(); + await held; + if (!allowClose) + throw new Error("injected independent doctor archive close failure"); + } + await original.call(this); + }); + const inspection = inspectJadxProvider(f.environment, resources.owner).catch( + (cause: unknown) => cause, + ); + let settled = false; + let shutdown: Promise | undefined; + try { + await closing; + shutdown = resources.close().then( + () => { + settled = true; + }, + (cause: unknown) => { + settled = true; + return cause; + }, + ); + await new Promise((resolve) => setImmediate(resolve)); + expect(settled).toBe(false); + release(); + const error = await inspection; + expect(error).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: [f.jar], + }); + expect(await shutdown).toMatchObject({ cleanup: { resources: [f.jar] } }); + expect(owner?.handle.fd).toBeGreaterThanOrEqual(0); + allowClose = true; + await resources.close(); + expect(owner?.handle.fd).toBe(-1); + if (!(error instanceof AnalysisError)) + throw new Error("Expected owned doctor failure"); + expect(error.cleanupIncomplete).toBe(false); + expect(error.cleanupResources).toEqual([]); + expect(f.launches).toEqual([]); + } finally { + release(); + allowClose = true; + close.mockRestore(); + await inspection; + await shutdown; + await resources.close(); + if (owner !== undefined) await original.call(owner); + } +}); diff --git a/tests/boundary/process/jadxJavaLifecycle.test.ts b/tests/boundary/process/jadxJavaLifecycle.test.ts index 7e4608800f..52eb19e0d8 100644 --- a/tests/boundary/process/jadxJavaLifecycle.test.ts +++ b/tests/boundary/process/jadxJavaLifecycle.test.ts @@ -1,4 +1,4 @@ -import { ZipArtifactReader } from "../../../src/artifacts/ZipArtifactReader.js"; +import { OwnedFileHandle } from "../../../src/filesystem/OwnedFileHandle.js"; import { beforeAll, expect, it as test, vi } from "vitest"; import { inspectJadxJava } from "../../../src/android/JadxJavaProbe.js"; import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; @@ -11,7 +11,7 @@ import { prepareProcessOwnershipInspection, } from "../../../src/process/ProcessOwnershipObservation.js"; import { createInspectionResources } from "../../fixtures/inspectionResources.js"; -import { writeFile, readFile } from "node:fs/promises"; +import { writeFile, readFile, stat } from "node:fs/promises"; import { dirname, join } from "node:path"; import { createJadxProtocolFixture } from "../../fixtures/android/jadx.js"; import { inspectJadxProvider } from "../../../src/android/JadxDoctor.js"; @@ -463,15 +463,21 @@ it("joins the whole availability call while its final archive close is held", as const held = new Promise((resolve) => { release = resolve; }); - const original = ZipArtifactReader.prototype.close; + const identity = await stat(f.jar); + const original = OwnedFileHandle.prototype.close; const archive = vi - .spyOn(ZipArtifactReader.prototype, "close") - .mockImplementationOnce(async function (this: ZipArtifactReader) { - started(); - await held; + .spyOn(OwnedFileHandle.prototype, "close") + .mockImplementation(async function (this: OwnedFileHandle) { + const observed = await this.handle.stat(); + if (observed.dev === identity.dev && observed.ino === identity.ino) { + started(); + await held; + } await original.call(this); }); - const availability = f.provider.inspectAvailability(); + const availability = f.provider + .inspectAvailability() + .catch((cause: unknown) => cause); let completed = false; let close: Promise | undefined; try { @@ -483,7 +489,9 @@ it("joins the whole availability call while its final archive close is held", as expect(completed).toBe(false); release(); await close; - expect(await availability).toMatchObject({ status: "available" }); + expect(await availability).toMatchObject({ + _tag: "AnalysisCancelledError", + }); } finally { release(); archive.mockRestore();