diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0f377691ca..9ac4080785 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -274,7 +274,10 @@ The general source suite (`npm run test:fast`, `npm run test:changed`, and On a Windows checkout, some source tests fail because fixtures use POSIX paths, `#!/bin/sh` tool stubs, or POSIX rename semantics. Compare failures against the base revision, or run the suite on Linux or WSL, before attributing them to a -change. See [testing](docs/testing.md#developer-commands) for command scopes +change. The embedded-artifact parent-swap tests exercise the selected-path +replacement when the rename succeeds; if Windows refuses it while the admitted +file is open, they instead verify the refusal and cleanup. See +[testing](docs/testing.md#developer-commands) for command scopes and lane selection. `git submodule update --init --recursive` fails at the pinned Unblob submodule diff --git a/src/artifacts/GodotPckArtifactReader.test.ts b/src/artifacts/GodotPckArtifactReader.test.ts index 3e57c8acb3..4340130faf 100644 --- a/src/artifacts/GodotPckArtifactReader.test.ts +++ b/src/artifacts/GodotPckArtifactReader.test.ts @@ -7,7 +7,7 @@ import { pckU64, type GodotPckMember, } from "../../tests/fixtures/godotPck.js"; -import { mkdir, rename, writeFile } from "node:fs/promises"; +import { mkdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { text } from "node:stream/consumers"; import { once } from "node:events"; @@ -15,6 +15,7 @@ import { once } from "node:events"; import { describe, expect, it } from "vitest"; import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; +import { renameOpenParent } from "../../tests/fixtures/renameOpenParent.js"; import { openRegularFile } from "../filesystem/RegularFile.js"; import type { ArtifactEntry } from "./ArtifactReader.js"; @@ -381,7 +382,7 @@ describe("GodotPckArtifactReader member streams", () => { }); it.each([false, true])( - "reads located pack facts from the admitted descriptor after path replacement (embedded: %s)", + "reads located pack facts from the admitted descriptor after an attempted path replacement (embedded: %s)", async (embedded) => { const root = await createTestTempDirectory("rea-godot-borrowed-source-"); const selected = join(root, "selected"); @@ -399,12 +400,14 @@ describe("GodotPckArtifactReader member streams", () => { if (location === undefined) throw new Error("expected a pack location"); // Replace the selected directory without changing the admitted inode's // metadata. Moving the file itself would change its validated ctime. - await rename(selected, join(root, "original")); - await mkdir(selected); - await writeFile( - path, - buildGodotPck(3, [{ path: "other.gd", data: "" }]), - ); + const swap = await renameOpenParent(selected, join(root, "original")); + if (swap.kind === "renamed") { + await mkdir(selected); + await writeFile( + path, + buildGodotPck(3, [{ path: "other.gd", data: "" }]), + ); + } const reader = new GodotPckArtifactReader(path, { kind: "godot-pck", source: descriptor, diff --git a/src/artifacts/extraction/EmbeddedArtifactExtraction.test.ts b/src/artifacts/extraction/EmbeddedArtifactExtraction.test.ts index 229499d76a..8742984684 100644 --- a/src/artifacts/extraction/EmbeddedArtifactExtraction.test.ts +++ b/src/artifacts/extraction/EmbeddedArtifactExtraction.test.ts @@ -1,7 +1,6 @@ import { appendFile, mkdir, - rename, readFile, stat, symlink, @@ -12,6 +11,7 @@ import { createHash } from "node:crypto"; import { join } from "node:path"; import { expect, it, vi } from "vitest"; import { createTestTempDirectory } from "../../../tests/fixtures/temporaryDirectory.js"; +import { renameOpenParent } from "../../../tests/fixtures/renameOpenParent.js"; import { buildGodotPck } from "../../../tests/fixtures/godotPck.js"; import { ArtifactResourceScope } from "../ArtifactResourceScope.js"; import { GodotPckArtifactReader } from "../GodotPckArtifactReader.js"; @@ -301,9 +301,14 @@ it.each(cases)( }, ); -it.each(cases)( - "rejects a replacement at the admitted $format pathname", - async ({ inputFormat, reader, bytes }) => { +it.each( + cases.flatMap((item) => [ + { ...item, simulateWindowsRefusal: false }, + { ...item, simulateWindowsRefusal: true }, + ]), +)( + "handles attempted replacement at the admitted $format pathname (simulated refusal: $simulateWindowsRefusal)", + async ({ inputFormat, reader, bytes, simulateWindowsRefusal }) => { const directory = await createTestTempDirectory( "rea-embedded-extraction-replacement-", ); @@ -311,34 +316,66 @@ it.each(cases)( await mkdir(selectedDirectory); const path = join(selectedDirectory, "source"); const output = join(directory, "out"); - await writeFile(path, bytes()); + const originalBytes = bytes(); + await writeFile(path, originalBytes); const scope = new ArtifactResourceScope(); const originalClose = reader.prototype.close; let closes = 0; + let deniedRename: unknown; const close = vi .spyOn(reader.prototype, "close") .mockImplementation(async function (this: { close(): Promise }) { await originalClose.call(this); if (++closes === 1) { // Moving the parent leaves the admitted regular file's metadata intact. - await rename(selectedDirectory, join(directory, "admitted")); + const swap = await renameOpenParent( + selectedDirectory, + join(directory, "admitted"), + simulateWindowsRefusal + ? { + platform: "win32", + rename: async () => { + throw Object.assign(new Error("injected EPERM"), { + code: "EPERM", + }); + }, + } + : {}, + ); + if (swap.kind === "blocked") { + deniedRename = swap.cause; + throw swap.cause; + } await mkdir(selectedDirectory); await writeFile(path, Buffer.from("replacement")); } }); try { - await expect( - extractArtifact({ - inputPath: path, - inputFormat, - outputRoot: output, - environment: {}, - integrityPolicy: "fail", - resourceScope: scope, - }), - ).rejects.toMatchObject({ reason: "integrity" }); + const failure = await extractArtifact({ + inputPath: path, + inputFormat, + outputRoot: output, + environment: {}, + integrityPolicy: "fail", + resourceScope: scope, + }).then( + () => undefined, + (cause: unknown) => cause, + ); + if (deniedRename === undefined) + expect(failure).toMatchObject({ reason: "integrity" }); + else + expect(failure).toMatchObject({ + reason: "io", + message: expect.stringContaining("EPERM"), + }); await expect(stat(output)).rejects.toMatchObject({ code: "ENOENT" }); expect(closes).toBe(1); + if (deniedRename !== undefined) { + expect((await readFile(path)).equals(originalBytes)).toBe(true); + expect(scope.hasPending).toBe(true); + await scope.close(); + } expect(scope.hasPending).toBe(false); } finally { await scope.close(); diff --git a/tests/fixtures/renameOpenParent.ts b/tests/fixtures/renameOpenParent.ts new file mode 100644 index 0000000000..e90f075e06 --- /dev/null +++ b/tests/fixtures/renameOpenParent.ts @@ -0,0 +1,25 @@ +import { rename } from "node:fs/promises"; + +/** Attempt the inode-preserving parent swap; Windows may refuse it while the child is open. */ +export const renameOpenParent = async ( + from: string, + to: string, + options: { + readonly platform?: NodeJS.Platform; + readonly rename?: typeof rename; + } = {}, +): Promise<{ kind: "renamed" } | { kind: "blocked"; cause: unknown }> => { + try { + await (options.rename ?? rename)(from, to); + return { kind: "renamed" }; + } catch (cause: unknown) { + if ( + (options.platform ?? process.platform) === "win32" && + cause instanceof Error && + "code" in cause && + cause.code === "EPERM" + ) + return { kind: "blocked", cause }; + throw cause; + } +};