From 832be4b82efebc35ae8a479309ca5873806af747 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 22 Oct 2026 18:13:21 +0000 Subject: [PATCH] fix(android): retain a source descriptor when admission close fails openRegularFile can reject after the descriptor opens. If that close also fails, keep the owner on the snapshot cleanup map instead of dropping it when adoption never runs. Co-authored-by: morluto --- src/android/AndroidTargetSnapshot.ts | 43 +++++++++++- .../process/androidSnapshotClose.test.ts | 68 ++++++++++++++++++- 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/src/android/AndroidTargetSnapshot.ts b/src/android/AndroidTargetSnapshot.ts index 1f6cf26435..28c685c811 100644 --- a/src/android/AndroidTargetSnapshot.ts +++ b/src/android/AndroidTargetSnapshot.ts @@ -16,6 +16,7 @@ import { } from "../filesystem/OwnedRegularFile.js"; import { openRegularFile, + RegularFileAdmissionFailure, assertRegularFileActive, sameRegularFileState, } from "../filesystem/RegularFile.js"; @@ -173,7 +174,7 @@ const copyAndroidSnapshot = async ( // copy outcome and stays retained until that owner is retried; the // partial file is removed only after those close attempts. const source = adoptSnapshotHandle( - await openRegularFile(sourcePath, { symlinks: "follow", signal }), + await openSnapshotSource(sourcePath, signal, retain(sourcePath)), ); return await withOwnedFileHandle( sourcePath, @@ -275,6 +276,44 @@ const copyAndroidSnapshot = async ( } }; +/** + * Admission can reject after the descriptor opens. Keep that owner when its + * own close also rejects; the success path still adopts one close owner. + */ +const openSnapshotSource = async ( + path: string, + signal: AbortSignal | undefined, + retention: { + readonly retainFailedClose?: (owner: OwnedFileHandle) => void; + }, +): Promise => { + try { + return await openRegularFile(path, { symlinks: "follow", signal }); + } catch (cause: unknown) { + if (!(cause instanceof RegularFileAdmissionFailure)) throw cause; + throw new RegularFileCleanupFailure( + path, + cause.owner, + { kind: "failed", cause: cause.cause }, + cause.cleanupCause, + retainAdmissionOwner(retention.retainFailedClose, cause.owner), + ); + } +}; + +const retainAdmissionOwner = ( + retain: ((owner: OwnedFileHandle) => void) | undefined, + owner: OwnedFileHandle, +): RegularFileCleanupFailure["retention"] => { + if (retain === undefined) return { kind: "not_requested" }; + try { + retain(owner); + return { kind: "callback_returned" }; + } catch (cause: unknown) { + return { kind: "callback_threw", cause }; + } +}; + /** Keep a stable-file diagnosis when descriptor cleanup fails around it. */ const projectSnapshotFailure = ( cause: unknown, @@ -300,7 +339,7 @@ const projectSnapshotFailure = ( }; const verifySourceState = async ( - handle: Awaited>, + handle: FileHandle, path: string, initial: Stats, observedBytes: number, diff --git a/tests/boundary/process/androidSnapshotClose.test.ts b/tests/boundary/process/androidSnapshotClose.test.ts index bebb0d40dc..ac0545a9a9 100644 --- a/tests/boundary/process/androidSnapshotClose.test.ts +++ b/tests/boundary/process/androidSnapshotClose.test.ts @@ -24,6 +24,7 @@ const state = vi.hoisted(() => ({ change: false, fail: false, failSource: false, + failAdmission: false, source: "", closeCalls: 0, sourceCloseCalls: 0, @@ -54,13 +55,29 @@ vi.mock("node:fs/promises", async (importOriginal) => { ) { state.sourceOpens += 1; const close = handle.close.bind(handle); - const record = { handle, close, snapshot: false }; + const record = { + handle, + close, + snapshot: false, + admit: state.failAdmission && state.sourceOpens > 1, + }; state.latestSource = record; state.sourceHandles.push(record); + if (record.admit) { + handle.stat = async () => { + throw Object.assign(new Error("controlled admission stat EIO"), { + code: "EIO", + }); + }; + } handle.close = async () => { state.sourceCloseCalls += 1; // Only the descriptor still open when the snapshot destination is // created belongs to the copy. Later admission reads must close. + if (record.admit && state.failAdmission) + throw Object.assign(new Error("controlled admission close EIO"), { + code: "EIO", + }); if (record.snapshot && state.failSource) throw Object.assign(new Error("controlled source close EIO"), { code: "EIO", @@ -408,6 +425,55 @@ test("keeps the changed-file diagnosis when both snapshot closes fail", async () } }); +test("retains the source descriptor when snapshot admission cleanup fails", async () => { + const fixture = await createJadxProtocolFixture(); + state.source = fixture.apk; + state.change = false; + state.fail = false; + state.failSource = false; + state.failAdmission = true; + state.enabled = true; + state.closeCalls = 0; + state.sourceCloseCalls = 0; + state.opens = 0; + state.sourceOpens = 0; + state.handles = []; + state.sourceHandles = []; + state.latestSource = undefined; + try { + const result = await fixture.service.execute("inspect_android_package", { + path: fixture.apk, + }); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(projectAnalysisError(result.error).code).toBe("cleanup_incomplete"); + expect(result.error.cleanupIncomplete).toBe(true); + expect(result.error.cleanupResources).toContain(fixture.apk); + expect(state.opens).toBe(0); + const source = state.sourceHandles.find( + (candidate) => candidate.handle.fd >= 0, + ); + if (source === undefined) throw new Error("missing source descriptor"); + expect(source.handle.fd).toBeGreaterThanOrEqual(0); + await expect(fixture.provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); + expect(source.handle.fd).toBeGreaterThanOrEqual(0); + + state.failAdmission = false; + await fixture.provider.close(); + expect(source.handle.fd).toBe(-1); + expect(result.error.cleanupIncomplete).toBe(false); + } finally { + state.enabled = false; + state.failAdmission = false; + await fixture.provider.close().catch(() => undefined); + for (const owned of [...state.handles, ...state.sourceHandles]) + if (owned.handle.fd >= 0) await owned.close().catch(() => undefined); + await verifyJadxFixtureCleanup(fixture.launches); + } +}); + test("classifies a nested snapshot close as cancellation", () => { const reason = { requested: "snapshot-close" }; const copy = {