diff --git a/src/artifacts/ArtifactResourceScope.ts b/src/artifacts/ArtifactResourceScope.ts index b7da3fd835..8e86e14bdb 100644 --- a/src/artifacts/ArtifactResourceScope.ts +++ b/src/artifacts/ArtifactResourceScope.ts @@ -1,4 +1,4 @@ -import type { FileHandle } from "node:fs/promises"; +import type { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; import type { ArtifactReader } from "./ArtifactReader.js"; import { ArtifactReaderFailure } from "./ArtifactReader.js"; @@ -14,7 +14,7 @@ export type ArtifactResourceOwner = | { readonly kind: "file-handle"; readonly resource: string; - readonly handle: FileHandle; + readonly handle: OwnedFileHandle; } | { readonly kind: "output-tree"; diff --git a/src/artifacts/OwnedFileHandle.test.ts b/src/artifacts/OwnedFileHandle.test.ts new file mode 100644 index 0000000000..aeb6649d5a --- /dev/null +++ b/src/artifacts/OwnedFileHandle.test.ts @@ -0,0 +1,57 @@ +import { expect, it, vi } from "vitest"; + +import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; + +it("shares one in-flight successful close between concurrent owners", async () => { + let resolveClose: (() => void) | undefined; + const file = { + fd: 17, + close: vi.fn( + () => + new Promise((resolve) => { + file.fd = -1; + resolveClose = resolve; + }), + ), + }; + const owner = new OwnedFileHandle(file); + + const first = owner.close(); + const second = owner.close(); + await Promise.resolve(); + expect(file.close).toHaveBeenCalledTimes(1); + resolveClose?.(); + + await expect(Promise.all([first, second])).resolves.toEqual([ + undefined, + undefined, + ]); + await expect(owner.close()).resolves.toBeUndefined(); + expect(file.close).toHaveBeenCalledTimes(1); +}); + +it("shares one in-flight failed close and keeps its unconfirmed result sticky", async () => { + const failure = new Error("native close failed"); + let rejectClose: (() => void) | undefined; + const file = { + fd: 17, + close: vi.fn( + () => + new Promise((_resolve, reject) => { + file.fd = -1; + rejectClose = () => reject(failure); + }), + ), + }; + const owner = new OwnedFileHandle(file); + + const first = owner.close(); + const second = owner.close(); + await Promise.resolve(); + expect(file.close).toHaveBeenCalledTimes(1); + rejectClose?.(); + + await expect(Promise.all([first, second])).rejects.toBe(failure); + await expect(owner.close()).rejects.toBe(failure); + expect(file.close).toHaveBeenCalledTimes(1); +}); diff --git a/src/artifacts/RegularFileAdmission.test.ts b/src/artifacts/RegularFileAdmission.test.ts new file mode 100644 index 0000000000..818e75ef17 --- /dev/null +++ b/src/artifacts/RegularFileAdmission.test.ts @@ -0,0 +1,68 @@ +import type { Stats } from "node:fs"; +import type { FileHandle } from "node:fs/promises"; +import { afterEach, expect, it, vi } from "vitest"; + +import { + NonRegularFileReadError, + openRegularFile, + RegularFileAdmissionFailure, +} from "../filesystem/RegularFile.js"; + +const { mockOpen } = vi.hoisted(() => ({ mockOpen: vi.fn() })); + +vi.mock("node:fs/promises", async (importOriginal) => ({ + ...(await importOriginal()), + open: mockOpen, +})); + +afterEach(() => mockOpen.mockReset()); + +it.each(["cancellation after open", "non-regular metadata", "stat failure"])( + "retains the exact owner when close fails after %s", + async (failureCase) => { + const closeFailure = new Error("native close failed"); + const statFailure = new Error("descriptor stat failed"); + const cancellation = new Error("cancelled after open"); + const controller = new AbortController(); + const file = { + fd: 17, + stat: vi.fn(async () => { + if (failureCase === "cancellation after open") { + controller.abort(cancellation); + return { isFile: () => true } as Stats; + } + if (failureCase === "non-regular metadata") + return { + isFile: () => false, + isDirectory: () => false, + } as Stats; + throw statFailure; + }), + close: vi.fn(async () => { + file.fd = -1; + throw closeFailure; + }), + }; + mockOpen.mockResolvedValue(file as unknown as FileHandle); + + const cause = await openRegularFile("/selected/input", { + symlinks: "reject", + signal: controller.signal, + }).catch((failure: unknown) => failure); + + expect(cause).toBeInstanceOf(RegularFileAdmissionFailure); + if (!(cause instanceof RegularFileAdmissionFailure)) + throw new Error("Expected an admission failure that retains its owner"); + expect(cause.owner.handle).toBe(file); + expect(cause.cleanupCause).toBe(closeFailure); + expect(file.close).toHaveBeenCalledTimes(1); + await expect(cause.owner.close()).rejects.toBe(closeFailure); + expect(file.close).toHaveBeenCalledTimes(1); + + if (failureCase === "non-regular metadata") + expect(cause.cause).toBeInstanceOf(NonRegularFileReadError); + else if (failureCase === "stat failure") + expect(cause.cause).toBe(statFailure); + else expect(cause.cause).toBe(cancellation); + }, +); diff --git a/src/artifacts/inventory/classify.ts b/src/artifacts/inventory/classify.ts index 7932c519eb..6534fcfdf2 100644 --- a/src/artifacts/inventory/classify.ts +++ b/src/artifacts/inventory/classify.ts @@ -1,11 +1,8 @@ -import { - NonRegularFileReadError, - openRegularFile, - sameRegularFileState, -} from "../../filesystem/RegularFile.js"; +import { sameRegularFileState } from "../../filesystem/RegularFile.js"; import type { FileHandle } from "node:fs/promises"; import type { Stats } from "node:fs"; import type { StableRegularFileDescriptor } from "../../filesystem/RegularFile.js"; +import type { OwnedFileHandle } from "../../filesystem/OwnedFileHandle.js"; import { classifyArtifactContent } from "./ArtifactGraphConstruction.js"; import { ARTIFACT_CLASSIFICATION_PREFIX_BYTES } from "../ArtifactHash.js"; @@ -17,7 +14,10 @@ import { import { ArtifactReaderFailure } from "../ArtifactReader.js"; import { type ArtifactResourceOwner } from "../ArtifactResourceScope.js"; import type { HashResult } from "../ArtifactHash.js"; -import { hashStableRootArtifactHandle } from "./hashStableRootArtifact.js"; +import { + hashStableRootArtifactHandle, + openRootArtifact, +} from "./hashStableRootArtifact.js"; import type { ArtifactInventoryOptions } from "./types.js"; interface RootClassification { @@ -26,9 +26,14 @@ interface RootClassification { } type RootInventoryClassification = RootClassification & { - readonly rootSource: StableRegularFileDescriptor | undefined; + readonly rootSource: RootInventorySource | undefined; }; +/** The admitted descriptor and the single owner that can confirm its close. */ +export interface RootInventorySource extends StableRegularFileDescriptor { + readonly owner: OwnedFileHandle; +} + /** Classify and hash one file root through the same stable open descriptor. */ export const classifyAndHashRoot = async ( path: string, @@ -47,7 +52,7 @@ export const classifyAndHashRoot = async ( if (rootSource !== undefined) { const owner: ArtifactResourceOwner = { kind: "file-handle", - handle: rootSource.handle, + handle: rootSource.owner, resource: `root artifact descriptor for ${path}`, }; const cleanupAttempt = await options.resourceScope.release(owner); @@ -73,10 +78,15 @@ export const classifyAndHashRootForInventory = async ( ): Promise => { if (directory) return { format: "directory", digest: null, rootSource: undefined }; - const handle = await openRootFile(path, options.signal); + const ownedHandle = await openRootArtifact( + path, + options.resourceScope, + options.signal, + ); + const handle = ownedHandle.handle; const owner: ArtifactResourceOwner = { kind: "file-handle", - handle, + handle: ownedHandle, resource: `root artifact descriptor for ${path}`, }; let outcome: @@ -105,7 +115,9 @@ export const classifyAndHashRootForInventory = async ( value: { format, digest, - rootSource: retainSource ? { handle, initial } : undefined, + rootSource: retainSource + ? { handle, initial, owner: ownedHandle } + : undefined, }, }; } catch (cause: unknown) { @@ -133,23 +145,6 @@ const isZipFormat = (format: ArtifactOccurrence["artifact_format"]): boolean => format === "msix" || format === "appx"; -const openRootFile = async ( - path: string, - signal?: AbortSignal, -): Promise => { - try { - return await openRegularFile(path, { symlinks: "reject", signal }); - } catch (cause: unknown) { - if (cause instanceof NonRegularFileReadError) - throw new ArtifactReaderFailure( - "format", - `Artifact root is not a regular file: ${path}`, - { cause }, - ); - throw cause; - } -}; - const classifyRootFormat = async ( path: string, handle: FileHandle, diff --git a/src/artifacts/inventory/hashStableRootArtifact.ts b/src/artifacts/inventory/hashStableRootArtifact.ts index cd3cc302c9..40a8a5fd6d 100644 --- a/src/artifacts/inventory/hashStableRootArtifact.ts +++ b/src/artifacts/inventory/hashStableRootArtifact.ts @@ -1,10 +1,13 @@ import { lstat } from "node:fs/promises"; import type { Stats } from "node:fs"; import type { FileHandle } from "node:fs/promises"; +import { readFileHandleChunks } from "../../filesystem/readFileHandleChunks.js"; +import { OwnedFileHandle } from "../../filesystem/OwnedFileHandle.js"; import { ArtifactReaderFailure } from "../ArtifactReader.js"; import { NonRegularFileReadError, + RegularFileAdmissionFailure, openRegularFile, sameRegularFileState, } from "../../filesystem/RegularFile.js"; @@ -13,33 +16,79 @@ import { hashReadable, type HashResult, } from "../ArtifactHash.js"; +import type { ArtifactResourceScope } from "../ArtifactResourceScope.js"; + +/** Admit a root descriptor without losing ownership when validation cleanup fails. */ +export const openRootArtifact = async ( + path: string, + resourceScope: ArtifactResourceScope, + signal?: AbortSignal, +): Promise => { + try { + return new OwnedFileHandle( + await openRegularFile(path, { symlinks: "reject", signal }), + ); + } catch (cause: unknown) { + const primary = + cause instanceof RegularFileAdmissionFailure ? cause.cause : cause; + const failure = + primary instanceof NonRegularFileReadError + ? new ArtifactReaderFailure( + "format", + `Artifact root is not a regular file: ${path}`, + { cause: primary }, + ) + : primary; + if (cause instanceof RegularFileAdmissionFailure) { + const resource = `root artifact descriptor for ${path}`; + const cleanup = await resourceScope.release({ + kind: "file-handle", + resource, + handle: cause.owner, + }); + if (cleanup.kind === "failed") + throw ArtifactReaderFailure.withCleanup( + failure, + ArtifactReaderFailure.cleanupObservation(cleanup.cause, resource), + ); + } + throw failure; + } +}; /** Hash one stable regular root file from the same descriptor that was checked. */ export const hashStableRootArtifact = async ( path: string, + resourceScope: ArtifactResourceScope, signal?: AbortSignal, ): Promise => { - let handle: FileHandle; - try { - handle = await openRegularFile(path, { - symlinks: "reject", - signal, - }); - } catch (cause: unknown) { - if (cause instanceof NonRegularFileReadError) - throw new ArtifactReaderFailure( - "format", - `Artifact root is not a regular file: ${path}`, - { cause }, - ); - throw cause; - } + const owner = await openRootArtifact(path, resourceScope, signal); + const handle = owner.handle; + let outcome: + | { readonly kind: "completed"; readonly value: HashResult } + | { readonly kind: "failed"; readonly cause: unknown }; try { const initial = await handle.stat(); - return await hashStableRootArtifactHandle(path, handle, initial, signal); - } finally { - await handle.close(); + outcome = { + kind: "completed", + value: await hashStableRootArtifactHandle(path, handle, initial, signal), + }; + } catch (cause: unknown) { + outcome = { kind: "failed", cause }; } + const resource = `root artifact descriptor for ${path}`; + const cleanup = await resourceScope.release({ + kind: "file-handle", + resource, + handle: owner, + }); + if (cleanup.kind === "failed") + throw ArtifactReaderFailure.withCleanup( + outcome.kind === "failed" ? outcome.cause : cleanup.cause, + ArtifactReaderFailure.cleanupObservation(cleanup.cause, resource), + ); + if (outcome.kind === "failed") throw outcome.cause; + return outcome.value; }; /** Hash and revalidate the same open descriptor used for root classification. */ @@ -52,7 +101,7 @@ export const hashStableRootArtifactHandle = async ( const digest = await hashReadable( // Read at most the admitted extent plus one byte to detect growth without // following an actively appended file indefinitely. - handle.createReadStream({ autoClose: false, start: 0, end: initial.size }), + readFileHandleChunks(handle, { start: 0, end: initial.size }), signal, ); abortIfNeeded(signal); diff --git a/src/artifacts/inventory/scanCanonical.ts b/src/artifacts/inventory/scanCanonical.ts index 2d36bcd74e..0985773114 100644 --- a/src/artifacts/inventory/scanCanonical.ts +++ b/src/artifacts/inventory/scanCanonical.ts @@ -5,7 +5,7 @@ import { AsarArtifactReader } from "../AsarArtifactReader.js"; import { lstat } from "node:fs/promises"; import type { Stats } from "node:fs"; -import type { StableRegularFileDescriptor } from "../../filesystem/RegularFile.js"; +import type { RootInventorySource } from "./classify.js"; import { ArtifactReaderFailure, @@ -107,6 +107,7 @@ export const scanCanonicalArtifactInventoryInScope = async ( rootFormat, rootDigest, rootSource, + resourceScope: options.resourceScope, reader, signal: options.signal, nodes, @@ -146,7 +147,7 @@ const cleanupInventoryOwners = async ({ }: { readonly path: string; readonly options: ArtifactInventoryOptions; - readonly rootSource: StableRegularFileDescriptor | undefined; + readonly rootSource: RootInventorySource | undefined; readonly ownedReaders: readonly ArtifactReader[]; readonly outcome: InventoryScanOutcome; }): Promise => { @@ -161,7 +162,7 @@ const cleanupInventoryOwners = async ({ if (rootSource !== undefined) owners.push({ kind: "file-handle", - handle: rootSource.handle, + handle: rootSource.owner, resource: `root artifact descriptor for ${path}`, }); for (const owner of owners) { @@ -182,11 +183,12 @@ const cleanupInventoryOwners = async ({ }; interface SnapshotBuildInput { + readonly resourceScope: ArtifactInventoryOptions["resourceScope"]; readonly path: string; readonly metadata: Stats; readonly rootFormat: ArtifactOccurrence["artifact_format"]; readonly rootDigest: HashResult | null; - readonly rootSource: StableRegularFileDescriptor | undefined; + readonly rootSource: RootInventorySource | undefined; readonly reader: ArtifactReader | undefined; readonly signal: AbortSignal | undefined; readonly nodes: Map; @@ -243,7 +245,13 @@ const buildInventorySnapshot = async ( ordinal, })); - await verifyRootDigest(path, rootDigest, rootSource, signal); + await verifyRootDigest( + path, + rootDigest, + rootSource, + input.resourceScope, + signal, + ); const graphSha256 = artifactGraphDigest({ nodes: orderedNodes, @@ -318,13 +326,14 @@ const buildIntegrityContradictions = ( const verifyRootDigest = async ( path: string, rootDigest: HashResult | null, - rootSource: StableRegularFileDescriptor | undefined, + rootSource: RootInventorySource | undefined, + resourceScope: ArtifactInventoryOptions["resourceScope"], signal: AbortSignal | undefined, ): Promise => { if (rootDigest === null) return; const verified = rootSource === undefined - ? await hashStableRootArtifact(path, signal) + ? await hashStableRootArtifact(path, resourceScope, signal) : await hashStableRootArtifactHandle( path, rootSource.handle, diff --git a/src/filesystem/OwnedFileHandle.ts b/src/filesystem/OwnedFileHandle.ts new file mode 100644 index 0000000000..8c1b7ede4e --- /dev/null +++ b/src/filesystem/OwnedFileHandle.ts @@ -0,0 +1,45 @@ +import type { FileHandle } from "node:fs/promises"; + +type CloseState = + | { readonly kind: "open" } + | { readonly kind: "closing"; readonly attempt: Promise } + | { readonly kind: "closed" } + | { readonly kind: "failed"; readonly cause: unknown }; + +/** One file handle whose close result must remain truthful across retries. */ +export class OwnedFileHandle< + Handle extends Pick = FileHandle, +> { + #state: CloseState = { kind: "open" }; + + constructor(readonly handle: Handle) {} + + /** Close the owned handle, retaining an unconfirmed close as a sticky failure. */ + close(): Promise { + if (this.#state.kind === "closed") return Promise.resolve(); + if (this.#state.kind === "closing") return this.#state.attempt; + if (this.#state.kind === "failed") return Promise.reject(this.#state.cause); + if (this.handle.fd < 0) { + const cause = new Error( + "File handle was invalidated before this owner confirmed its close", + ); + this.#state = { kind: "failed", cause }; + return Promise.reject(cause); + } + + const attempt = Promise.resolve().then(() => this.#attemptClose()); + this.#state = { kind: "closing", attempt }; + return attempt; + } + + async #attemptClose(): Promise { + try { + await this.handle.close(); + this.#state = { kind: "closed" }; + } catch (cause: unknown) { + this.#state = + this.handle.fd < 0 ? { kind: "failed", cause } : { kind: "open" }; + throw cause; + } + } +} diff --git a/src/filesystem/RegularFile.ts b/src/filesystem/RegularFile.ts index 69edcad312..a5c2391cfc 100644 --- a/src/filesystem/RegularFile.ts +++ b/src/filesystem/RegularFile.ts @@ -2,6 +2,8 @@ import { constants } from "node:fs"; import type { BigIntStats, Stats } from "node:fs"; import { open, type FileHandle } from "node:fs/promises"; +import { OwnedFileHandle } from "./OwnedFileHandle.js"; + /** Identifies a selected path that cannot be read as one regular file. */ export class NonRegularFileReadError extends Error { readonly code: "EISDIR" | "ENOTFILE"; @@ -16,6 +18,24 @@ export class NonRegularFileReadError extends Error { } } +/** Admission failed and the opened descriptor could not be confirmed closed. */ +export class RegularFileAdmissionFailure extends Error { + override readonly cause: unknown; + + constructor( + readonly path: string, + admissionCause: unknown, + readonly cleanupCause: unknown, + readonly owner: OwnedFileHandle, + ) { + super( + `Selected input failed admission and its descriptor close failed: ${path}`, + ); + this.name = "RegularFileAdmissionFailure"; + this.cause = admissionCause; + } +} + /** A regular input no longer matches the extent or identity admitted for reading. */ export class RegularFileChangedError extends Error { readonly code = "ESTALE"; @@ -47,6 +67,7 @@ export const openRegularFile = async ( (constants.O_NOCTTY ?? 0) | (options.symlinks === "reject" ? (constants.O_NOFOLLOW ?? 0) : 0), ); + const owner = new OwnedFileHandle(handle); try { options.signal?.throwIfAborted(); const metadata = await handle.stat(); @@ -55,7 +76,11 @@ export const openRegularFile = async ( throw new NonRegularFileReadError(path, metadata.isDirectory()); return handle; } catch (cause: unknown) { - await handle.close().catch(() => undefined); + try { + await owner.close(); + } catch (cleanupCause: unknown) { + throw new RegularFileAdmissionFailure(path, cause, cleanupCause, owner); + } throw cause; } }; diff --git a/src/filesystem/readFileHandleChunks.ts b/src/filesystem/readFileHandleChunks.ts new file mode 100644 index 0000000000..f0e205891c --- /dev/null +++ b/src/filesystem/readFileHandleChunks.ts @@ -0,0 +1,46 @@ +import type { FileHandle } from "node:fs/promises"; +import { Readable } from "node:stream"; + +/** Stream positional reads while leaving descriptor cleanup to its owner. */ +export const readFileHandleChunks = ( + handle: Pick, + options: { + readonly start?: number; + readonly end?: number; + readonly signal?: AbortSignal | undefined; + } = {}, +): Readable => { + const start = options.start ?? 0; + const end = options.end ?? Infinity; + if ( + !Number.isSafeInteger(start) || + start < 0 || + (end !== Infinity && (!Number.isSafeInteger(end) || end < start)) + ) + throw new RangeError("Invalid file read range"); + return Readable.from( + (async function* () { + let position = start; + while (position <= end) { + options.signal?.throwIfAborted(); + const chunk = Buffer.allocUnsafe( + Math.min(64 * 1024, end - position + 1), + ); + const { bytesRead } = await handle.read( + chunk, + 0, + chunk.length, + position, + ); + options.signal?.throwIfAborted(); + if (bytesRead === 0) return; + position += bytesRead; + yield chunk.subarray(0, bytesRead); + } + })(), + { + objectMode: false, + ...(options.signal === undefined ? {} : { signal: options.signal }), + }, + ); +}; diff --git a/tests/boundary/filesystem/artifactInventoryDescriptorCleanup.test.ts b/tests/boundary/filesystem/artifactInventoryDescriptorCleanup.test.ts new file mode 100644 index 0000000000..d89714ceb1 --- /dev/null +++ b/tests/boundary/filesystem/artifactInventoryDescriptorCleanup.test.ts @@ -0,0 +1,92 @@ +import type { FileHandle } from "node:fs/promises"; +import { writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { expect, it, onTestFinished, vi } from "vitest"; + +import { ArtifactProvider } from "../../../src/artifacts/ArtifactProvider.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +const injected = vi.hoisted(() => ({ + opened: undefined as + | ((path: unknown, handle: FileHandle) => void) + | undefined, +})); + +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + open: async (...args: Parameters) => { + const handle = await actual.open(...args); + injected.opened?.(args[0], handle); + return handle; + }, + }; +}); + +it.each([false, true])( + "preserves root admission failure and close authority (invalidated=%s)", + async (invalidated) => { + const root = await createTestTempDirectory("rea-root-admission-cleanup-"); + const path = join(root, "payload.bin"); + await writeFile(path, "selected bytes"); + const client = new ArtifactProvider({}).createClient({ + path, + sourcePath: path, + sha256: "0".repeat(64), + kind: "artifact", + format: "javascript", + }); + let descriptor: FileHandle | undefined; + let closeAttempts = 0; + let blocked = true; + injected.opened = (openedPath, handle) => { + if (openedPath !== path || descriptor !== undefined) return; + descriptor = handle; + const close = handle.close.bind(handle); + vi.spyOn(handle, "stat").mockRejectedValueOnce( + Object.assign(new Error("primary admission stat failure"), { + code: "EIO", + errno: -5, + syscall: "fstat", + }), + ); + vi.spyOn(handle, "close").mockImplementation(async () => { + closeAttempts += 1; + if (invalidated) await close(); + if (blocked) throw new Error("descriptor cleanup outcome unavailable"); + await close(); + }); + }; + onTestFinished(async () => { + injected.opened = undefined; + blocked = false; + await client.close(); + vi.restoreAllMocks(); + }); + + const inventory = await client.execute("inventory_artifact", {}); + expect(inventory).toMatchObject({ + ok: false, + error: { + reason: "io", + detail: expect.stringContaining("primary admission stat failure"), + cleanup: { + resources: [expect.stringContaining(path)], + reason: "descriptor cleanup outcome unavailable", + }, + }, + }); + expect(descriptor).toBeDefined(); + expect((await client.close()).ok).toBe(false); + const attemptsBeforeRecovery = closeAttempts; + blocked = false; + const recovery = await client.close(); + expect(recovery.ok).toBe(!invalidated); + expect(closeAttempts).toBe( + invalidated ? attemptsBeforeRecovery : attemptsBeforeRecovery + 1, + ); + expect(descriptor?.fd).toBe(-1); + if (invalidated) expect(closeAttempts).toBe(1); + }, +);