diff --git a/src/artifacts/DirectoryArtifactReader.ts b/src/artifacts/DirectoryArtifactReader.ts index 123a1cb092..69beb8e1d1 100644 --- a/src/artifacts/DirectoryArtifactReader.ts +++ b/src/artifacts/DirectoryArtifactReader.ts @@ -1,23 +1,32 @@ -import { constants } from "node:fs"; -import { lstat, open, opendir, realpath } from "node:fs/promises"; +import { constants, type Dir, type Stats } from "node:fs"; +import { + lstat, + open, + opendir, + realpath, + type FileHandle, +} from "node:fs/promises"; import { isAbsolute, join, relative, sep } from "node:path"; -import type { Readable } from "node:stream"; +import { Readable } from "node:stream"; import { ArtifactReaderFailure, type ArtifactEntry, type ArtifactReader, } from "./ArtifactReader.js"; +import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; +import { readFileHandleChunks } from "../filesystem/readFileHandleChunks.js"; /** Non-symlink-following directory reader. */ export class DirectoryArtifactReader implements ArtifactReader { readonly format = "directory" as const; - readonly #rootPromise: Promise; + #rootPromise: Promise | undefined; readonly #rootPath: string; + readonly #directories = new Set(); + readonly #files = new Map(); constructor(root: string) { this.#rootPath = root; - this.#rootPromise = realpath(root); } /** Enumerate entries, optionally restricting descent while retaining directories. */ @@ -25,9 +34,7 @@ export class DirectoryArtifactReader implements ArtifactReader { signal?: AbortSignal, descendInto?: (path: string) => boolean, ): AsyncIterable { - const root = await this.#rootPromise.catch((cause: unknown) => { - throw directoryIoFailure("resolve artifact root", this.#rootPath, cause); - }); + const root = await this.#resolveRoot(); const pending = [root]; while (pending.length > 0) { abortIfNeeded(signal); @@ -36,62 +43,100 @@ export class DirectoryArtifactReader implements ArtifactReader { await withDirectoryIoContext("inspect directory", directory, () => assertContainedDirectory(root, directory), ); - const handle = await opendir(directory).catch((cause: unknown) => { - throw directoryIoFailure("open directory", directory, cause); - }); - const directories: string[] = []; - try { - for await (const child of handle) { - abortIfNeeded(signal); - const absolute = join(directory, child.name); - const metadata = await lstat(absolute).catch((cause: unknown) => { - throw directoryIoFailure("inspect entry", absolute, cause); - }); - const path = relative(root, absolute).split(sep).join("/"); - const kind = metadata.isSymbolicLink() - ? "symlink" - : metadata.isDirectory() - ? "directory" - : metadata.isFile() - ? "file" - : undefined; - if (kind === undefined) continue; - yield { - path, - kind, - declaredSize: kind === "file" ? metadata.size : null, - compressedSize: null, - executable: (metadata.mode & 0o111) !== 0, - encrypted: false, - byteOffset: null, - declaredSha256: null, - unpacked: false, - limitations: - kind === "symlink" - ? ["Symlink target was not followed or disclosed."] - : [], - adapterKey: absolute, - ...(kind === "file" - ? { - sourceIdentity: { device: metadata.dev, inode: metadata.ino }, - } - : {}), - }; - if (kind === "directory" && (descendInto?.(path) ?? true)) - directories.push(absolute); - } - } catch (cause: unknown) { - if (cause instanceof ArtifactReaderFailure) throw cause; - throw directoryIoFailure("read directory", directory, cause); - } finally { - // best-effort cleanup: directory-handle close must not mask traversal. - await handle.close().catch(() => undefined); - } + for await (const entry of this.#readDirectory( + directory, + pending, + signal, + descendInto, + )) + yield entry; await withDirectoryIoContext("inspect directory", directory, () => assertContainedDirectory(root, directory), ); - directories.reverse(); - pending.push(...directories); + } + } + + async #resolveRoot(): Promise { + return (this.#rootPromise ??= realpath(this.#rootPath)).catch( + (cause: unknown) => { + throw directoryIoFailure( + "resolve artifact root", + this.#rootPath, + cause, + ); + }, + ); + } + + async *#readDirectory( + directory: string, + pending: string[], + signal?: AbortSignal, + descendInto?: (path: string) => boolean, + ): AsyncIterable { + const root = await this.#resolveRoot(); + const handle = new OwnedDirectoryHandle( + await opendir(directory).catch((cause: unknown) => { + throw directoryIoFailure("open directory", directory, cause); + }), + ); + this.#directories.add(handle); + const childDirectories: string[] = []; + let traversal: + | { readonly kind: "complete" } + | { readonly kind: "failed"; readonly cause: unknown } = { + kind: "complete", + }; + try { + for (;;) { + const child = await handle.handle.read(); + if (child === null) break; + abortIfNeeded(signal); + const absolute = join(directory, child.name); + const metadata = await lstat(absolute).catch((cause: unknown) => { + throw directoryIoFailure("inspect entry", absolute, cause); + }); + const path = relative(root, absolute).split(sep).join("/"); + const entry = artifactEntry(path, absolute, metadata); + if (entry === undefined) continue; + yield entry; + if (entry.kind === "directory" && (descendInto?.(path) ?? true)) + childDirectories.push(absolute); + } + } catch (cause: unknown) { + traversal = { + kind: "failed", + cause: + cause instanceof ArtifactReaderFailure + ? cause + : directoryIoFailure("read directory", directory, cause), + }; + } finally { + await this.#finishDirectory(handle, directory, traversal); + } + if (traversal.kind === "failed") throw traversal.cause; + childDirectories.reverse(); + pending.push(...childDirectories); + } + + async #finishDirectory( + handle: OwnedDirectoryHandle, + path: string, + traversal: + | { readonly kind: "complete" } + | { readonly kind: "failed"; readonly cause: unknown }, + ): Promise { + try { + await handle.close(); + this.#directories.delete(handle); + } catch (cause: unknown) { + const closeFailure = directoryIoFailure("close directory", path, cause); + if (traversal.kind === "failed") + throw ArtifactReaderFailure.withCleanup( + traversal.cause, + ArtifactReaderFailure.cleanupObservation(closeFailure, path), + ); + throw closeFailure; } } @@ -102,14 +147,16 @@ export class DirectoryArtifactReader implements ArtifactReader { "format", "Only regular files can be opened", ); - const handle = await open( + const fileHandle = await open( entry.adapterKey, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK, ).catch((cause: unknown) => { throw directoryIoFailure("open entry", entry.adapterKey, cause); }); + const owner = new OwnedFileHandle(fileHandle); + this.#files.set(owner, entry.adapterKey); try { - const observed = await handle.stat().catch((cause: unknown) => { + const observed = await owner.handle.stat().catch((cause: unknown) => { throw directoryIoFailure( "inspect opened entry", entry.adapterKey, @@ -127,16 +174,76 @@ export class DirectoryArtifactReader implements ArtifactReader { `Directory entry changed before open: ${entry.path}`, ); } - return handle.createReadStream({ autoClose: true }); + abortIfNeeded(signal); + const source = Readable.from( + mapFileReadCancellation(owner.handle, signal), + { objectMode: false }, + ); + source.once("close", () => { + void this.#closeFile(owner).catch(() => undefined); + }); + return source; } catch (cause: unknown) { - // best-effort cleanup: file-handle close must not mask the open failure. - await handle.close().catch(() => undefined); + try { + await this.#closeFile(owner); + } catch (closeCause: unknown) { + throw ArtifactReaderFailure.withCleanup( + cause, + ArtifactReaderFailure.cleanupObservation( + directoryIoFailure( + "close opened entry", + entry.adapterKey, + closeCause, + ), + entry.adapterKey, + ), + ); + } throw cause; } } - close(): Promise { - return Promise.resolve(); + async close(): Promise { + const failures: unknown[] = []; + for (const [owner, path] of this.#files) { + try { + await this.#closeFile(owner); + } catch (cause: unknown) { + failures.push(directoryIoFailure("close opened entry", path, cause)); + } + } + for (const owner of this.#directories) { + try { + await owner.close(); + this.#directories.delete(owner); + } catch (cause: unknown) { + failures.push( + directoryIoFailure("close directory", owner.handle.path, cause), + ); + } + } + if (failures.length > 0) { + const reason = failures.map(failureMessage).join("; "); + throw new ArtifactReaderFailure( + "io", + `Directory artifact cleanup failed: ${reason}`, + { + cause: new AggregateError(failures), + cleanup: { + reason, + resources: [ + ...this.#files.values(), + ...[...this.#directories].map((owner) => owner.handle.path), + ], + }, + }, + ); + } + } + + async #closeFile(owner: OwnedFileHandle): Promise { + await owner.close(); + this.#files.delete(owner); } provenance(): readonly [] { @@ -144,6 +251,74 @@ export class DirectoryArtifactReader implements ArtifactReader { } } +const artifactEntry = ( + path: string, + absolute: string, + metadata: Stats, +): ArtifactEntry | undefined => { + const kind = metadata.isSymbolicLink() + ? "symlink" + : metadata.isDirectory() + ? "directory" + : metadata.isFile() + ? "file" + : undefined; + if (kind === undefined) return undefined; + return { + path, + kind, + declaredSize: kind === "file" ? metadata.size : null, + compressedSize: null, + executable: (metadata.mode & 0o111) !== 0, + encrypted: false, + byteOffset: null, + declaredSha256: null, + unpacked: false, + limitations: + kind === "symlink" + ? ["Symlink target was not followed or disclosed."] + : [], + adapterKey: absolute, + ...(kind === "file" + ? { + sourceIdentity: { device: metadata.dev, inode: metadata.ino }, + } + : {}), + }; +}; + +type DirectoryCloseState = + | { readonly kind: "open" } + | { readonly kind: "closing"; readonly attempt: Promise } + | { readonly kind: "closed" } + | { readonly kind: "failed"; readonly cause: unknown }; + +/** Dir.close invalidates Node's private handle before its native close settles. */ +class OwnedDirectoryHandle { + #state: DirectoryCloseState = { kind: "open" }; + + constructor(readonly handle: Dir) {} + + 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); + const attempt = Promise.resolve().then(async () => { + try { + await this.handle.close(); + this.#state = { kind: "closed" }; + } catch (cause: unknown) { + // Dir exposes no public validity signal; a second close can only lie + // with ERR_DIR_CLOSED after Node has invalidated the first owner. + this.#state = { kind: "failed", cause }; + throw cause; + } + }); + this.#state = { kind: "closing", attempt }; + return attempt; + } +} + const directoryIoFailure = ( operation: string, path: string, @@ -157,13 +332,38 @@ const directoryIoFailure = ( typeof cause.code === "string" ? ` (${cause.code})` : ""; + const detail = failureMessage(cause); return new ArtifactReaderFailure( "io", - `Could not ${operation} at ${path}${code}`, + `Could not ${operation} at ${path}${code}: ${detail}`, { cause }, ); }; +const failureMessage = (cause: unknown): string => + cause instanceof Error ? cause.message : String(cause); + +const mapFileReadCancellation = async function* ( + handle: FileHandle, + signal?: AbortSignal, +): AsyncGenerator { + try { + for await (const chunk of readFileHandleChunks(handle, { signal })) { + abortIfNeeded(signal); + yield chunk; + } + abortIfNeeded(signal); + } catch (cause: unknown) { + if (signal?.aborted === true) + throw new ArtifactReaderFailure( + "cancelled", + "Directory artifact read cancelled", + { cause }, + ); + throw cause; + } +}; + const withDirectoryIoContext = async ( operation: string, path: string, diff --git a/src/artifacts/MachOSliceArtifactReader.ts b/src/artifacts/MachOSliceArtifactReader.ts index 540fe17263..e21e975549 100644 --- a/src/artifacts/MachOSliceArtifactReader.ts +++ b/src/artifacts/MachOSliceArtifactReader.ts @@ -12,6 +12,7 @@ import { type LipoArchitecture, } from "../native/parsers/lipo.js"; import type { MachoSlice } from "../domain/apple/dylibResolution.js"; +import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; import { readMachoImage, type ReadAt } from "./apple/MachoLoadCommandReader.js"; import { ArtifactReaderFailure, @@ -22,6 +23,8 @@ import { } from "./ArtifactReader.js"; import { streamChunkToBuffer } from "./StreamBytes.js"; import { + NonRegularFileReadError, + RegularFileAdmissionFailure, openRegularFile, sameRegularFileState, type StableRegularFileDescriptor, @@ -33,6 +36,7 @@ export class MachOSliceArtifactReader implements ArtifactReader { readonly format = "file" as const; #command: ArtifactCommand | undefined; #source: StableRegularFileDescriptor | undefined; + #ownedSource: OwnedFileHandle | undefined; #pendingSource: Promise | undefined; readonly #entries = new Map(); readonly #ownsSource: boolean; @@ -283,23 +287,51 @@ export class MachOSliceArtifactReader implements ArtifactReader { } async #acquireSource(): Promise { - const handle = await openRegularFile(this.path, { symlinks: "reject" }); + let handle: FileHandle; + try { + handle = await openRegularFile(this.path, { symlinks: "reject" }); + } catch (cause: unknown) { + if (!(cause instanceof RegularFileAdmissionFailure)) throw cause; + this.#ownedSource = cause.owner; + const primary = + cause.cause instanceof NonRegularFileReadError + ? new ArtifactReaderFailure( + "format", + `Universal Mach-O source is not a regular file: ${this.path}`, + { cause: cause.cause }, + ) + : cause.cause; + throw ArtifactReaderFailure.withCleanup( + primary, + ArtifactReaderFailure.cleanupObservation(cause.cleanupCause, this.path), + ); + } + const owner = new OwnedFileHandle(handle); + this.#ownedSource = owner; try { const initial = await handle.stat(); const source = { handle, initial }; this.#source = source; return source; } catch (cause: unknown) { - await handle.close().catch(() => undefined); + try { + await owner.close(); + this.#ownedSource = undefined; + } catch (cleanupCause: unknown) { + throw ArtifactReaderFailure.withCleanup( + cause, + ArtifactReaderFailure.cleanupObservation(cleanupCause, this.path), + ); + } throw cause; } } async #closeSource(): Promise { await this.#pendingSource?.catch(() => undefined); - if (!this.#ownsSource || this.#source === undefined) return; - const source = this.#source; - await source.handle.close(); + if (!this.#ownsSource || this.#ownedSource === undefined) return; + await this.#ownedSource.close(); + this.#ownedSource = undefined; this.#source = undefined; } diff --git a/src/artifacts/ZipArtifactReader.ts b/src/artifacts/ZipArtifactReader.ts index ca3988a70d..fbffd76d8d 100644 --- a/src/artifacts/ZipArtifactReader.ts +++ b/src/artifacts/ZipArtifactReader.ts @@ -12,8 +12,10 @@ import { type ArtifactReader, } from "./ArtifactReader.js"; import type { ZipPackageFormat } from "../domain/zipPackageFormat.js"; +import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; import { NonRegularFileReadError, + RegularFileAdmissionFailure, openRegularFile, sameRegularFileState, type StableRegularFileDescriptor, @@ -21,6 +23,7 @@ import { class NodeFileReader extends Reader { #handle: FileHandle | undefined; + #owner: OwnedFileHandle | undefined; #initPromise: Promise | undefined; readonly #ownsHandle: boolean; @@ -40,10 +43,30 @@ class NodeFileReader extends Reader { async #initialize(): Promise { try { - this.#handle = + const handle = this.admitted?.handle ?? (await openRegularFile(this.path, { symlinks: "reject" })); + this.#handle = handle; + if (this.#ownsHandle) this.#owner = new OwnedFileHandle(handle); } catch (cause: unknown) { + if (cause instanceof RegularFileAdmissionFailure) { + this.#owner = cause.owner; + const primary = + cause.cause instanceof NonRegularFileReadError + ? new ArtifactReaderFailure( + "format", + `ZIP source is not a regular file: ${this.path}`, + { cause: cause.cause }, + ) + : cause.cause; + throw ArtifactReaderFailure.withCleanup( + primary, + ArtifactReaderFailure.cleanupObservation( + cause.cleanupCause, + this.path, + ), + ); + } if (cause instanceof NonRegularFileReadError) throw new ArtifactReaderFailure( "format", @@ -88,8 +111,9 @@ class NodeFileReader extends Reader { async closeHandle(): Promise { if (!this.#ownsHandle) return; await this.#initPromise?.catch(() => undefined); - if (this.#handle === undefined) return; - await this.#handle.close(); + if (this.#owner === undefined) return; + await this.#owner.close(); + this.#owner = undefined; this.#handle = undefined; } } diff --git a/tests/boundary/filesystem/directoryArtifactReaderFifoRace.test.ts b/tests/boundary/filesystem/directoryArtifactReaderFifoRace.test.ts index 443de83f20..7e504bd22a 100644 --- a/tests/boundary/filesystem/directoryArtifactReaderFifoRace.test.ts +++ b/tests/boundary/filesystem/directoryArtifactReaderFifoRace.test.ts @@ -1,7 +1,9 @@ +import { execFile } from "node:child_process"; import { writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { randomUUID } from "node:crypto"; +import { promisify } from "node:util"; import { fileURLToPath } from "node:url"; import { expect, it } from "vitest"; @@ -12,6 +14,8 @@ import { } from "../../../src/process/ProviderProcess.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +const run = promisify(execFile); + it.skipIf(process.platform === "win32")( "rejects a FIFO replacement without blocking the owner process", async () => { @@ -51,3 +55,46 @@ it.skipIf(process.platform === "win32")( } }, ); + +it.skipIf(process.platform === "win32")( + "skips unsupported FIFO entries and continues through the directory", + async () => { + const root = await createTestTempDirectory("rea-directory-entry-fifo-"); + await writeFile(join(root, "module.js"), "export {}\n"); + await run("mkfifo", [join(root, "unsupported.fifo")]); + const probe = fileURLToPath( + new URL( + "../../fixtures/directoryReaderUnsupportedEntryProbe.mjs", + import.meta.url, + ), + ); + const readerModule = fileURLToPath( + new URL( + "../../../dist/artifacts/DirectoryArtifactReader.js", + import.meta.url, + ), + ); + const launch = await spawnOwnedProviderProcess({ + command: process.execPath, + arguments: [probe, readerModule, root], + runId: `rea-directory-entry-fifo-${randomUUID()}`, + cwd: tmpdir(), + hostEnvironment: process.env, + }); + const supervisor = new ProviderProcessSupervisor({ + ...launch, + ownsProcessLifetime: true, + }); + + try { + expect(await supervisor.waitForOutputClose(2_000)).toBe(true); + expect(supervisor.snapshot().stdout.text.trim()).toBe('["module.js"]'); + } finally { + const stopped = await supervisor.stop({ + terminationGraceMs: 100, + killGraceMs: 500, + }); + expect(stopped.status).not.toBe("incomplete"); + } + }, +); diff --git a/tests/boundary/filesystem/directoryArtifactReaderOwnership.test.ts b/tests/boundary/filesystem/directoryArtifactReaderOwnership.test.ts new file mode 100644 index 0000000000..4145e7bca2 --- /dev/null +++ b/tests/boundary/filesystem/directoryArtifactReaderOwnership.test.ts @@ -0,0 +1,216 @@ +import { writeFile, type FileHandle } from "node:fs/promises"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it, vi } from "vitest"; + +const controls = vi.hoisted(() => ({ + wrapDirectory: undefined as + | ((handle: { close(): Promise; readonly path: string }) => void) + | undefined, + wrapFile: undefined as + | ((handle: { close(): Promise; readonly fd: number }) => void) + | undefined, +})); + +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + opendir: async (...args: Parameters) => { + const handle = await actual.opendir(...args); + controls.wrapDirectory?.(handle); + return handle; + }, + open: async (...args: Parameters) => { + const handle = await actual.open(...args); + controls.wrapFile?.(handle); + return handle; + }, + }; +}); + +import { ArtifactReaderFailure } from "../../../src/artifacts/ArtifactReader.js"; +import { DirectoryArtifactReader } from "../../../src/artifacts/DirectoryArtifactReader.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +const makeFixture = async () => { + const root = await createTestTempDirectory("rea-directory-owner-"); + const file = join(root, "sample.txt"); + await writeFile(file, "directory content\n"); + return { root, file, reader: new DirectoryArtifactReader(root) }; +}; + +afterEach(() => { + controls.wrapDirectory = undefined; + controls.wrapFile = undefined; + vi.restoreAllMocks(); +}); + +describe("DirectoryArtifactReader resource ownership", () => { + it("closes the exact directory owner on early iterator return", async () => { + const { reader } = await makeFixture(); + let closeCalls = 0; + controls.wrapDirectory = (handle) => { + const close = handle.close.bind(handle); + handle.close = async () => { + closeCalls += 1; + await close(); + }; + }; + + try { + const entries = reader.entries()[Symbol.asyncIterator](); + expect((await entries.next()).done).toBe(false); + await entries.return?.(); + expect(closeCalls).toBe(1); + await reader.close(); + expect(closeCalls).toBe(1); + } finally { + await reader.close(); + } + }); + + it("retains an invalidated directory owner after native close rejection", async () => { + const { reader } = await makeFixture(); + const nativeFailure = new Error("injected native directory close failure"); + let closeCalls = 0; + controls.wrapDirectory = (handle) => { + const close = handle.close.bind(handle); + handle.close = async () => { + closeCalls += 1; + await close(); + throw nativeFailure; + }; + }; + + try { + await expect(async () => { + for await (const _entry of reader.entries()) { + // Read through EOF so the explicit owner close runs. + } + }).rejects.toMatchObject({ + reason: "io", + message: expect.stringContaining("close directory"), + }); + + const cleanupFailure = await reader + .close() + .catch((cause: unknown) => cause); + expect(cleanupFailure).toBeInstanceOf(ArtifactReaderFailure); + expect((cleanupFailure as Error).message).toContain("cleanup failed"); + expect(closeCalls).toBe(1); + const aggregate = (cleanupFailure as Error & { cause: AggregateError }) + .cause; + expect( + aggregate.errors.some( + (cause) => cause instanceof Error && cause.cause === nativeFailure, + ), + ).toBe(true); + } finally { + await reader.close().catch(() => undefined); + } + }); + + it("preserves cancellation while retaining a failed directory close", async () => { + const { root, reader } = await makeFixture(); + await writeFile(join(root, "second.txt"), "second entry\n"); + const closeFailure = new Error("injected close after cancellation"); + let closeCalls = 0; + controls.wrapDirectory = (handle) => { + const close = handle.close.bind(handle); + handle.close = async () => { + closeCalls += 1; + await close(); + throw closeFailure; + }; + }; + + try { + const controller = new AbortController(); + const entries = reader.entries(controller.signal)[Symbol.asyncIterator](); + expect((await entries.next()).done).toBe(false); + controller.abort(); + const failure = await entries.next().catch((cause: unknown) => cause); + expect(failure).toMatchObject({ reason: "cancelled" }); + expect((failure as ArtifactReaderFailure).cleanup?.resources).toEqual([ + root, + ]); + expect(closeCalls).toBe(1); + await expect(reader.close()).rejects.toMatchObject({ reason: "io" }); + expect(closeCalls).toBe(1); + } finally { + await reader.close().catch(() => undefined); + } + }); +}); + +describe("DirectoryArtifactReader file handle ownership", () => { + it("retries an uninvalidated file close through its retained owner", async () => { + const { reader } = await makeFixture(); + let fileCloseCalls = 0; + let openedHandle: FileHandle | undefined; + controls.wrapFile = (handle) => { + if (handle.fd < 0) return; + openedHandle = handle as FileHandle; + const close = handle.close.bind(handle); + handle.close = async () => { + fileCloseCalls += 1; + if (fileCloseCalls === 1) throw new Error("before native close"); + await close(); + }; + }; + + try { + let entry; + for await (const observed of reader.entries()) { + if (observed.path === "sample.txt") entry = observed; + } + if (entry === undefined) + throw new Error("fixture file was not inventoried"); + const stream = await reader.open(entry); + stream.destroy(); + await new Promise((resolve) => stream.once("close", resolve)); + await new Promise((resolve) => setImmediate(resolve)); + expect(fileCloseCalls).toBe(1); + expect(openedHandle?.fd).toBeGreaterThanOrEqual(0); + await reader.close(); + expect(fileCloseCalls).toBe(2); + expect(openedHandle?.fd).toBe(-1); + } finally { + await reader.close().catch(() => undefined); + } + }); + + it("maps an aborted positional read to typed cancellation and owner close", async () => { + const { file, reader } = await makeFixture(); + await writeFile(file, Buffer.alloc(256 * 1024, 0x5a)); + try { + let entry; + for await (const observed of reader.entries()) { + if (observed.path === "sample.txt") entry = observed; + } + if (entry === undefined) + throw new Error("fixture file was not inventoried"); + const controller = new AbortController(); + const stream = await reader.open(entry, controller.signal); + const closed = new Promise((resolve) => + stream.once("close", resolve), + ); + const outcome = new Promise((resolve, reject) => { + stream.once("error", resolve); + stream.once("end", () => reject(new Error("read was not cancelled"))); + }); + stream.once("data", () => controller.abort()); + const failure = await outcome; + expect(failure).toMatchObject({ + name: "ArtifactReaderFailure", + reason: "cancelled", + }); + await closed; + await new Promise((resolve) => setImmediate(resolve)); + await reader.close(); + } finally { + await reader.close().catch(() => undefined); + } + }); +}); diff --git a/tests/boundary/filesystem/ownedArtifactReaderCleanup.test.ts b/tests/boundary/filesystem/ownedArtifactReaderCleanup.test.ts new file mode 100644 index 0000000000..c9bc919ff9 --- /dev/null +++ b/tests/boundary/filesystem/ownedArtifactReaderCleanup.test.ts @@ -0,0 +1,140 @@ +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 { ArtifactResourceScope } from "../../../src/artifacts/ArtifactResourceScope.js"; +import { MachOSliceArtifactReader } from "../../../src/artifacts/MachOSliceArtifactReader.js"; +import { ZipArtifactReader } from "../../../src/artifacts/ZipArtifactReader.js"; +import { ok } from "../../../src/domain/result.js"; +import type { NativeCommandRunner } from "../../../src/native/CommandRunner.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; + }, + }; +}); + +const runner: NativeCommandRunner = { + run: async () => + ok({ + tool: "lipo", + executable: "/usr/bin/lipo", + executableSha256: "1".repeat(64), + toolVersion: null, + versionReason: "fixture", + arguments: [], + stdout: "", + stderr: "", + stdoutBytes: 0, + stderrBytes: 0, + exitCode: 0, + signal: null, + }), +}; + +const readers = [ + ["ZIP", (path: string) => new ZipArtifactReader(path, "zip")], + [ + "Mach-O slice", + (path: string) => new MachOSliceArtifactReader(path, {}, runner), + ], +] as const; + +it.each(readers)( + "retains %s reader admission and stat owners", + async (_, createReader) => { + const root = await createTestTempDirectory("rea-owned-reader-cleanup-"); + const path = join(root, "source"); + await writeFile(path, "fixture source"); + + for (const stage of ["admission", "post-open-stat"] as const) { + const statFailure = Object.assign( + new Error(`primary ${stage} stat failure`), + { code: "EIO", errno: -5, syscall: "fstat" }, + ); + const closeFailure = new Error( + `close failed after invalidation during ${stage}`, + ); + let descriptor: FileHandle | undefined; + let statCalls = 0; + let closeAttempts = 0; + let failOnStat = stage === "admission" ? 1 : 2; + injected.opened = (openedPath, handle) => { + if (openedPath !== path || descriptor !== undefined) return; + descriptor = handle; + const stat = handle.stat.bind(handle); + const close = handle.close.bind(handle); + vi.spyOn(handle, "stat").mockImplementation(async (...args) => { + statCalls += 1; + if (statCalls === failOnStat) { + failOnStat = -1; + throw statFailure; + } + return stat(...args); + }); + vi.spyOn(handle, "close").mockImplementation(async () => { + closeAttempts += 1; + await close(); + throw closeFailure; + }); + }; + + const reader = createReader(path); + const scope = new ArtifactResourceScope(); + const owner = { kind: "reader" as const, resource: path, reader }; + onTestFinished(async () => { + injected.opened = undefined; + await scope.close().catch(() => undefined); + vi.restoreAllMocks(); + }); + + const primary = await (async () => { + try { + for await (const _entry of reader.entries()) { + // Both injected failures happen before the reader can yield an entry. + } + } catch (cause: unknown) { + return cause; + } + throw new Error("Expected reader initialization to fail"); + })(); + expect(primary).toBeInstanceOf(Error); + expect(causeChainContains(primary, statFailure)).toBe(true); + + const release = await scope.release(owner); + expect(release).toEqual({ kind: "failed", cause: closeFailure }); + expect(descriptor?.fd).toBe(-1); + await expect(scope.close()).rejects.toMatchObject({ + reason: "unavailable", + cleanup: { reason: closeFailure.message, resources: [path] }, + }); + // Native close invalidated the fd before rejecting. Retrying the same owner + // must remain failed without another syscall or a false successful release. + expect(closeAttempts).toBe(1); + } + }, +); + +const causeChainContains = (cause: unknown, expected: unknown): boolean => { + let current: unknown = cause; + while (current instanceof Error) { + if (current === expected) return true; + current = current.cause; + } + return false; +}; diff --git a/tests/fixtures/directoryReaderUnsupportedEntryProbe.mjs b/tests/fixtures/directoryReaderUnsupportedEntryProbe.mjs new file mode 100644 index 0000000000..cc66c6e11d --- /dev/null +++ b/tests/fixtures/directoryReaderUnsupportedEntryProbe.mjs @@ -0,0 +1,15 @@ +import { pathToFileURL } from "node:url"; + +const [modulePath, root] = process.argv.slice(2); +if (modulePath === undefined || root === undefined) + throw new Error("Directory reader probe requires module and root paths"); + +const { DirectoryArtifactReader } = await import(pathToFileURL(modulePath)); +const reader = new DirectoryArtifactReader(root); +const paths = []; +try { + for await (const entry of reader.entries()) paths.push(entry.path); +} finally { + await reader.close(); +} +process.stdout.write(`${JSON.stringify(paths)}\n`);