diff --git a/docs/tool-design.md b/docs/tool-design.md index f1d4ecaee6..303137375d 100644 --- a/docs/tool-design.md +++ b/docs/tool-design.md @@ -116,6 +116,13 @@ artifact can execute successfully. ## Own the lifecycle +`SafeOutputTree` publishes only after file readback, nested-directory fsync +from children to parents, and root-parent fsync on hosts that support directory +syncing. Windows skips directory fsync. Failed writes retain a separate pending +identity admission only until identity is captured; rollback retries retain +unremoved file/directory identities and open handles independently, so confirmed +unlink progress cannot be re-admitted through a closed descriptor. + For tools that acquire resources, identify the owner and the observations that establish each transition. Include temporary files and one-shot commands, as well as persistent engine sessions. Use the existing provider contracts; diff --git a/src/artifacts/SafeOutputTree.ts b/src/artifacts/SafeOutputTree.ts index d113c42f2b..4d5dac7c96 100644 --- a/src/artifacts/SafeOutputTree.ts +++ b/src/artifacts/SafeOutputTree.ts @@ -66,7 +66,8 @@ export class SafeOutputTree { readonly #nestedDirectories = new Map(); readonly #unknownDirectories = new Set(); readonly #ownedFiles = new Map(); - readonly #openFiles = new Map(); + /** Exclusive files awaiting identity admission; admitted identity and handles have separate owners. */ + readonly #pendingFiles = new Map(); readonly #openHandles = new Set(); #published = false; #createdRoot = true; @@ -211,13 +212,14 @@ export class SafeOutputTree { ); }); const ownedHandle = this.#trackHandle(handle); - this.#openFiles.set(destination, ownedHandle); + this.#pendingFiles.set(destination, ownedHandle); const fileIdentity = await readFileIdentity( ownedHandle.handle, destination, ); await assertFilePathIdentity(destination, fileIdentity); this.#ownedFiles.set(destination, fileIdentity); + this.#pendingFiles.delete(destination); if (this.platform !== "win32") { await ownedHandle.handle.chmod(0o600); if (((await ownedHandle.handle.stat()).mode & 0o777) !== 0o600) @@ -255,7 +257,7 @@ export class SafeOutputTree { await this.#assertLineage(lineage); await assertFilePathIdentity(destination, fileIdentity); await ownedHandle.handle.sync(); - await this.#closeFileHandle(destination, ownedHandle); + await this.#closeHandle(ownedHandle); const readbackHandle = this.#trackHandle( await open(destination, constants.O_RDONLY | constants.O_NOFOLLOW), ); @@ -284,7 +286,7 @@ export class SafeOutputTree { } } - /** Sync the owned output tree and prevent further writes through this instance. */ + /** Sync files' containing directories from children to root, then publish the root in its parent. */ async commit(signal?: AbortSignal): Promise { this.#assertWritable(); abortIfNeeded(signal); @@ -302,7 +304,7 @@ export class SafeOutputTree { constants.O_RDONLY | constants.O_DIRECTORY, ), ); - let output: OwnedFileHandle | undefined; + const handles: OwnedFileHandle[] = [parent]; let sync: | { readonly kind: "synced" } | { @@ -310,18 +312,21 @@ export class SafeOutputTree { readonly cause: unknown; } = { kind: "synced" }; try { - output = this.#trackHandle( - await open( - this.#outputRoot, - constants.O_RDONLY | constants.O_DIRECTORY, - ), - ); - await assertHandleIdentity( - output.handle, - this.#requireRootIdentity(), - this.#outputRoot, - ); - await output.handle.sync(); + // A directory's entries become durable before its parent publishes the + // child directory name. Keep only one nested descriptor open at a time. + const directories = [...this.#nestedDirectories].reverse(); + directories.push([this.#outputRoot, this.#requireRootIdentity()]); + for (const [path, identity] of directories) { + abortIfNeeded(signal); + const directory = this.#trackHandle( + await open(path, constants.O_RDONLY | constants.O_DIRECTORY), + ); + handles.push(directory); + await assertHandleIdentity(directory.handle, identity, path); + await directory.handle.sync(); + await this.#closeHandle(directory); + handles.pop(); + } await parent.handle.sync(); await this.#assertExactEntries(); abortIfNeeded(signal); @@ -330,9 +335,7 @@ export class SafeOutputTree { sync = { kind: "failed", cause }; } const closeResults = await Promise.allSettled( - [parent, output] - .filter(isDefined) - .map((handle) => this.#closeHandle(handle)), + handles.map((handle) => this.#closeHandle(handle)), ); const closeFailures = closeResults .filter(isRejected) @@ -609,25 +612,19 @@ export class SafeOutputTree { throw new AggregateError(failures, "Output tree handles remain open"); } - async #closeFileHandle(path: string, owner: OwnedFileHandle): Promise { - await this.#closeHandle(owner); - this.#openFiles.delete(path); - } - async #admitPendingFiles(): Promise { - for (const [path, owner] of this.#openFiles) { + for (const [path, owner] of this.#pendingFiles) { let identity = this.#ownedFiles.get(path); if (identity === undefined) { identity = await readFileIdentity(owner.handle, path); await assertFilePathIdentity(path, identity); this.#ownedFiles.set(path, identity); } + this.#pendingFiles.delete(path); } } } -const isDefined = (value: T | undefined): value is T => value !== undefined; - const isRejected = ( result: PromiseSettledResult, ): result is PromiseRejectedResult => result.status === "rejected"; diff --git a/tests/boundary/filesystem/safeOutputTreePublication.test.ts b/tests/boundary/filesystem/safeOutputTreePublication.test.ts new file mode 100644 index 0000000000..127619ecef --- /dev/null +++ b/tests/boundary/filesystem/safeOutputTreePublication.test.ts @@ -0,0 +1,117 @@ +import { createHash } from "node:crypto"; +import { access, chmod, readFile, readdir, rm } from "node:fs/promises"; +import { dirname, join } from "node:path"; +import { Readable } from "node:stream"; +import { expect, it, vi } from "vitest"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +const observation = vi.hoisted(() => ({ + synced: [] as string[], + refuseSync: undefined as string | undefined, +})); +vi.mock("node:fs/promises", async (importOriginal) => { + const filesystem = await importOriginal(); + return { + ...filesystem, + open: async (...args: Parameters) => { + const handle = await filesystem.open(...args); + const sync = handle.sync.bind(handle); + handle.sync = async () => { + const path = String(args[0]); + if (path === observation.refuseSync) + throw new Error("controlled nested directory sync failure"); + await sync(); + observation.synced.push(path); + }; + return handle; + }, + }; +}); +import { SafeOutputTree } from "../../../src/artifacts/SafeOutputTree.js"; + +it.skipIf(process.platform === "win32")( + "syncs nested directory entries before their parents and refuses publication when one cannot sync", + async () => { + const root = await createTestTempDirectory("rea-tree-directory-sync-"); + const bytes = Buffer.from("published bytes"); + for (const fail of [false, true]) { + const output = join(root, fail ? "failed" : "success"); + const tree = await SafeOutputTree.create(output); + try { + await tree.write("a/b/payload.txt", Readable.from([bytes]), { + bytes: bytes.length, + sha256: createHash("sha256").update(bytes).digest("hex"), + }); + observation.synced.length = 0; + observation.refuseSync = fail ? join(output, "a") : undefined; + if (fail) { + await expect(tree.commit()).rejects.toMatchObject({ + message: "Could not durably sync extraction output", + cause: { message: "controlled nested directory sync failure" }, + }); + expect(tree.published).toBe(false); + expect(observation.synced).toEqual([join(output, "a", "b")]); + observation.refuseSync = undefined; + expect(await tree.rollback()).toEqual({ + status: "complete", + residualPaths: [], + }); + await expect(access(output)).rejects.toMatchObject({ + code: "ENOENT", + }); + } else { + await tree.commit(); + expect(tree.published).toBe(true); + expect(observation.synced).toEqual([ + join(output, "a", "b"), + join(output, "a"), + output, + dirname(output), + ]); + expect(await readFile(join(output, "a", "b", "payload.txt"))).toEqual( + bytes, + ); + } + } finally { + observation.refuseSync = undefined; + await tree.closeHandles(); + } + } + }, +); + +it + .skipIf(process.platform === "win32" || process.getuid?.() === 0) + .each([false, true])( + "resumes rollback after root removal denial without readmitting a closed file (failed write: %s)", + async (failedWrite) => { + const root = await createTestTempDirectory("rea-tree-removal-retry-"); + const output = join(root, "output"); + const tree = await SafeOutputTree.create(output); + const bytes = Buffer.from("x"); + try { + const write = tree.write("partial.txt", Readable.from([bytes]), { + bytes: failedWrite ? 2 : 1, + sha256: createHash("sha256").update(bytes).digest("hex"), + }); + if (failedWrite) + await expect(write).rejects.toMatchObject({ reason: "integrity" }); + else await write; + await chmod(root, 0o500); + await expect(tree.rollback()).rejects.toMatchObject({ + cause: { code: "EACCES", syscall: "rmdir" }, + }); + expect(await readdir(output)).toEqual([]); + await chmod(root, 0o700); + expect(await tree.rollback()).toEqual({ + status: "complete", + residualPaths: [], + }); + await expect(access(output)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + await chmod(root, 0o700); + await tree.closeHandles(); + await rm(root, { recursive: true, force: true }); + } + }, +);