From f438a620a601cf5ed489cc476d93066573f8c4f2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 01:26:56 -0400 Subject: [PATCH 1/6] fix(filesystem): move a file across filesystems by copying it (#5188) rename() cannot cross a device boundary, so move_file failed with EXDEV between allowed directories on different mounts. On EXDEV, moveFile now falls back to moveFileAcrossDevices, which opens the source and creates the destination relative to their anchored directories (O_EXCL, no symlinks followed), copies the content, mode and times, then removes the source only while it is still the file copied. A failed step removes the copy and leaves the source in place. A directory or symlink is refused with a clear error rather than copied. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .changeset/5188-filesystem-move-exdev.md | 5 + src/filesystem/README.md | 7 + src/filesystem/__tests__/lib.test.ts | 34 ++++ .../__tests__/pinned-directory.test.ts | 178 ++++++++++++++++++ src/filesystem/__tests__/server-tools.test.ts | 66 +++++++ src/filesystem/lib.ts | 11 +- src/filesystem/pinned-directory.ts | 177 ++++++++++++++++- 7 files changed, 476 insertions(+), 2 deletions(-) create mode 100644 .changeset/5188-filesystem-move-exdev.md diff --git a/.changeset/5188-filesystem-move-exdev.md b/.changeset/5188-filesystem-move-exdev.md new file mode 100644 index 0000000000..3826f11fc1 --- /dev/null +++ b/.changeset/5188-filesystem-move-exdev.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-filesystem": patch +--- + +`move_file` now moves a regular file between allowed directories on different filesystems (mounts, disks or shares), where it used to fail with `EXDEV`: it copies the file to a newly created destination, keeping its permissions and modification time, then removes the source, and removes a partial copy if anything fails. A directory or a symbolic link still cannot be moved across filesystems, and now fails with an error that says so. diff --git a/src/filesystem/README.md b/src/filesystem/README.md index 9400f40ab2..f5adc3520f 100644 --- a/src/filesystem/README.md +++ b/src/filesystem/README.md @@ -161,6 +161,13 @@ The server's directory access control follows this flow: - `source` (string) - `destination` (string) - Fails if destination exists + - Across filesystems (the source and destination on different mounts or + disks), a regular file is moved by copying it to a newly created + destination, with its permissions and modification time, and then removing + the source. If the copy fails, the partial destination is removed and the + source is left in place. A directory or a symbolic link cannot be moved + across filesystems and fails with an error; move a directory's files one + at a time instead - **search_files** - Recursively search for files/directories that match or do not match patterns diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index 886bc25af5..15ee98abf1 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -703,6 +703,40 @@ describe("Lib Functions", () => { expect(mockPinned.renameIntoDirectory).not.toHaveBeenCalled(); }); + + it("copies and removes instead when the rename crosses a device (EXDEV)", async () => { + mockFs.lstat.mockRejectedValueOnce( + Object.assign(new Error("not found"), { code: "ENOENT" }), + ); + mockPinned.renameIntoDirectory.mockImplementationOnce(() => { + throw Object.assign(new Error("EXDEV: cross-device link"), { + code: "EXDEV", + }); + }); + + await moveFile("/test/source.txt", "/mnt/other/dest.txt"); + + expect(mockPinned.moveFileAcrossDevices).toHaveBeenCalledWith( + "/test/source.txt", + "/mnt/other/dest.txt", + ); + }); + + it("reports a rename failure other than EXDEV as is, without copying", async () => { + mockFs.lstat.mockRejectedValueOnce( + Object.assign(new Error("not found"), { code: "ENOENT" }), + ); + mockPinned.renameIntoDirectory.mockImplementationOnce(() => { + throw Object.assign(new Error("EACCES: permission denied"), { + code: "EACCES", + }); + }); + + await expect( + moveFile("/test/source.txt", "/test/dest.txt"), + ).rejects.toThrow("EACCES: permission denied"); + expect(mockPinned.moveFileAcrossDevices).not.toHaveBeenCalled(); + }); }); }); diff --git a/src/filesystem/__tests__/pinned-directory.test.ts b/src/filesystem/__tests__/pinned-directory.test.ts index 43f83df7d0..7c5f35109a 100644 --- a/src/filesystem/__tests__/pinned-directory.test.ts +++ b/src/filesystem/__tests__/pinned-directory.test.ts @@ -13,6 +13,7 @@ import { createFileInDirectory, isChildDirectory, isSameDirectory, + moveFileAcrossDevices, overwriteFileInDirectory, renameIntoDirectory, } from "../pinned-directory.js"; @@ -429,6 +430,183 @@ describe("renameIntoDirectory", () => { ); }); +// Each test here runs on one device: the helper is what moveFile falls back +// to on EXDEV, and it never calls rename, so one device exercises it fully. +describe("moveFileAcrossDevices", () => { + const fail = (code: string) => + ((...args: unknown[]) => { + (args[args.length - 1] as (error: Error) => void)(errno(code)); + }) as never; + + it("copies the content, mode and times, then removes the source", async () => { + const source = path.join(dir, "a.bin"); + const destination = path.join(outside, "b.bin"); + // More than one 64 KiB chunk. + const content = Buffer.alloc(200 * 1024, 7); + fs.writeFileSync(source, content); + fs.chmodSync(source, 0o640); + fs.utimesSync(source, new Date(1_000_000_000), new Date(1_000_000_000)); + await moveFileAcrossDevices(source, destination); + expect(fs.readFileSync(destination).equals(content)).toBe(true); + expect(fs.existsSync(source)).toBe(false); + const moved = fs.statSync(destination); + if (process.platform !== "win32") { + expect(moved.mode & 0o777).toBe(0o640); + } + expect(moved.mtimeMs).toBe(1_000_000_000); + expect(process.cwd()).toBe(cwd); + }); + + it("moves an empty file", async () => { + const source = path.join(dir, "empty"); + fs.writeFileSync(source, ""); + await moveFileAcrossDevices(source, path.join(outside, "empty")); + expect(fs.readFileSync(path.join(outside, "empty"), "utf-8")).toBe(""); + expect(fs.existsSync(source)).toBe(false); + }); + + it("refuses a directory, and leaves it in place", async () => { + const source = path.join(dir, "d"); + fs.mkdirSync(source); + await expect( + moveFileAcrossDevices(source, path.join(outside, "d")), + ).rejects.toThrow(`Cannot move a directory across filesystems: ${source}.`); + expect(fs.statSync(source).isDirectory()).toBe(true); + expect(fs.readdirSync(outside)).toEqual([]); + }); + + it.skipIf(!symlinks)( + "refuses a symlink rather than following or recreating it", + async () => { + const target = path.join(dir, "t.txt"); + fs.writeFileSync(target, "t"); + const source = path.join(dir, "l.txt"); + fs.symlinkSync(target, source); + await expect( + moveFileAcrossDevices(source, path.join(outside, "l.txt")), + ).rejects.toThrow( + `Cannot move across filesystems: not a regular file: ${source}`, + ); + expect(fs.lstatSync(source).isSymbolicLink()).toBe(true); + expect(fs.readdirSync(outside)).toEqual([]); + }, + ); + + it("refuses an existing destination with EEXIST, and keeps both", async () => { + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "a"); + fs.writeFileSync(destination, "b"); + await expect(moveFileAcrossDevices(source, destination)).rejects.toThrow( + expect.objectContaining({ code: "EEXIST" }), + ); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + expect(fs.readFileSync(destination, "utf-8")).toBe("b"); + }); + + it.skipIf(!symlinks)( + "refuses a destination directory that resolves somewhere else", + async () => { + fs.symlinkSync(outside, path.join(dir, "link")); + const source = path.join(dir, "a.txt"); + fs.writeFileSync(source, "a"); + await expect( + moveFileAcrossDevices(source, path.join(dir, "link", "b.txt")), + ).rejects.toThrow("Access denied"); + expect(fs.readdirSync(outside)).toEqual([]); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + }, + ); + + it.each([ + ["another file", { ino: -1 }], + ["not a regular file", { isFile: () => false }], + ])( + "refuses, and closes the source, when it was replaced while being opened (%s)", + async (_, replaced) => { + const source = path.join(dir, "a.txt"); + fs.writeFileSync(source, "a"); + const realFstat = fs.fstatSync; + vi.spyOn(fs, "fstatSync").mockImplementationOnce((fd) => + Object.assign(realFstat(fd), replaced), + ); + const closeSync = vi.spyOn(fs, "closeSync"); + await expect( + moveFileAcrossDevices(source, path.join(outside, "b.txt")), + ).rejects.toThrow( + `Refusing to move: ${source} was replaced while it was being opened`, + ); + expect(closeSync).toHaveBeenCalled(); + expect(fs.readdirSync(outside)).toEqual([]); + }, + ); + + it.each(["read", "writeFile", "fchmod", "futimes"] as const)( + "removes the partial copy, and keeps the source, when %s fails", + async (step) => { + const source = path.join(dir, "a.txt"); + fs.writeFileSync(source, "a"); + vi.spyOn(fs, step).mockImplementationOnce(fail("EIO")); + await expect( + moveFileAcrossDevices(source, path.join(outside, "b.txt")), + ).rejects.toThrow("EIO"); + expect(fs.readdirSync(outside)).toEqual([]); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + }, + ); + + it("reports the copy's failure when removing the copy fails too", async () => { + const source = path.join(dir, "a.txt"); + fs.writeFileSync(source, "a"); + vi.spyOn(fs, "read").mockImplementationOnce(fail("EIO")); + vi.spyOn(fs, "unlinkSync").mockImplementationOnce(() => { + throw errno("EBUSY"); + }); + await expect( + moveFileAcrossDevices(source, path.join(outside, "b.txt")), + ).rejects.toThrow("EIO"); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + }); + + it("removes the copy, and keeps the source, when the source cannot be removed", async () => { + const source = path.join(dir, "a.txt"); + fs.writeFileSync(source, "a"); + const realUnlink = fs.unlinkSync; + vi.spyOn(fs, "unlinkSync") + .mockImplementationOnce(() => { + throw errno("EACCES"); + }) + .mockImplementation(realUnlink); + await expect( + moveFileAcrossDevices(source, path.join(outside, "b.txt")), + ).rejects.toThrow("EACCES"); + expect(fs.readdirSync(outside)).toEqual([]); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + }); + + it("removes the copy, and leaves the new file, when the source was replaced during the copy", async () => { + const source = path.join(dir, "a.txt"); + fs.writeFileSync(source, "a"); + const realFchmod = fs.fchmod; + vi.spyOn(fs, "fchmod").mockImplementationOnce((( + fd: number, + mode: number, + callback: (error: NodeJS.ErrnoException | null) => void, + ) => { + fs.unlinkSync(source); + fs.writeFileSync(source, "replacement"); + realFchmod(fd, mode, callback); + }) as typeof fs.fchmod); + await expect( + moveFileAcrossDevices(source, path.join(outside, "b.txt")), + ).rejects.toThrow( + `Refusing to remove: ${source} was replaced during the move`, + ); + expect(fs.readdirSync(outside)).toEqual([]); + expect(fs.readFileSync(source, "utf-8")).toBe("replacement"); + }); +}); + describe("createDirectoryInPlace", () => { it("creates missing parents, and restores the working directory", () => { createDirectoryInPlace(path.join(dir, "a", "b", "c")); diff --git a/src/filesystem/__tests__/server-tools.test.ts b/src/filesystem/__tests__/server-tools.test.ts index 676bfeb588..20bded6aeb 100644 --- a/src/filesystem/__tests__/server-tools.test.ts +++ b/src/filesystem/__tests__/server-tools.test.ts @@ -1383,6 +1383,72 @@ describe("move_file", () => { expect(textOf(result)).toMatch(/^ENOENT/); }); + // #5188: rename cannot cross a device boundary. A second mount cannot be + // staged in a test, so the rename is made to fail as it would across one. + describe("across filesystems (rename fails with EXDEV)", () => { + const crossDevice = () => + vi.spyOn(fsSync, "renameSync").mockImplementation(() => { + throw Object.assign( + new Error("EXDEV: cross-device link not permitted"), + { + code: "EXDEV", + }, + ); + }); + + it("moves a file by copying it and removing the source", async () => { + const source = path.join(dir, "a.txt"); + const destination = path.join(dir, "sub", "b.txt"); + await fs.mkdir(path.join(dir, "sub")); + await fs.writeFile(source, "across"); + const rename = crossDevice(); + const result = await call(client, "move_file", { source, destination }); + expect(rename).toHaveBeenCalled(); + expect(result.isError).toBeFalsy(); + expect(textOf(result)).toBe( + `Successfully moved ${source} to ${destination}`, + ); + expect(await fs.readFile(destination, "utf-8")).toBe("across"); + await expect(fs.stat(source)).rejects.toThrow(); + }); + + it("refuses a directory with a clear error, and leaves it in place", async () => { + const source = path.join(dir, "d1"); + await fs.mkdir(source); + await fs.writeFile(path.join(source, "f"), "x"); + crossDevice(); + const result = await call(client, "move_file", { + source, + destination: path.join(dir, "d2"), + }); + expect(result.isError).toBe(true); + expect(textOf(result)).toBe( + `Cannot move a directory across filesystems: ${source}. ` + + "Create the directory at the destination and move its files one at a time.", + ); + expect(await fs.readFile(path.join(source, "f"), "utf-8")).toBe("x"); + await expect(fs.stat(path.join(dir, "d2"))).rejects.toThrow(); + }); + + it("leaves no partial destination when the copy fails", async () => { + const source = path.join(dir, "a.txt"); + await fs.writeFile(source, "keep"); + crossDevice(); + vi.spyOn(fsSync, "read").mockImplementationOnce(((...args: unknown[]) => { + (args[args.length - 1] as (error: Error) => void)( + Object.assign(new Error("EIO: i/o error, read"), { code: "EIO" }), + ); + }) as typeof fsSync.read); + const result = await call(client, "move_file", { + source, + destination: path.join(dir, "b.txt"), + }); + expect(textOf(result)).toBe("EIO: i/o error, read"); + expect(await fs.readdir(dir)).toEqual(["a.txt"]); + expect(await fs.readFile(source, "utf-8")).toBe("keep"); + }); + }); + it("reports a destination lstat failure other than ENOENT as is", async () => { await fs.writeFile(path.join(dir, "a.txt"), "a"); vi.spyOn(fs, "lstat").mockRejectedValueOnce(new Error("EACCES lstat")); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 75915768ec..775145e708 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -9,6 +9,7 @@ import { isPathWithinAllowedDirectories } from "./path-validation.js"; import { createDirectoryInPlace, createFileInDirectory, + moveFileAcrossDevices, overwriteFileInDirectory, renameIntoDirectory, } from "./pinned-directory.js"; @@ -633,7 +634,15 @@ export async function moveFile( } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") { // The destination's directory is pinned (see pinned-directory.ts). - renameIntoDirectory(sourcePath, destinationPath); + try { + renameIntoDirectory(sourcePath, destinationPath); + } catch (renameError) { + // rename cannot cross a device boundary: copy, then remove (#5188). + if ((renameError as NodeJS.ErrnoException).code !== "EXDEV") { + throw renameError; + } + await moveFileAcrossDevices(sourcePath, destinationPath); + } return; } throw error; diff --git a/src/filesystem/pinned-directory.ts b/src/filesystem/pinned-directory.ts index 9cb15ec9f6..fa9c71552d 100644 --- a/src/filesystem/pinned-directory.ts +++ b/src/filesystem/pinned-directory.ts @@ -12,7 +12,8 @@ // and only then act on the final component relative to it. The working // directory holds the directory's inode, so swapping the path afterwards // cannot move the operation. Every check happens before a byte is written, -// so nothing ever needs rolling back. +// so nothing ever needs rolling back, except in the cross-device move, which +// copies and so removes its copy if a later step fails. // // On Windows the working directory is a string and relative paths resolve // against it, so reading it back proves nothing; there the leaf is lstat'd @@ -291,6 +292,180 @@ export function renameIntoDirectory( ); } +// The cross-device move below (#5188). rename() cannot cross a device +// boundary, so a move between allowed directories on different mounts copies +// the file and then removes the source. Every name it touches is opened, +// created or removed relative to its anchored directory, like the writes +// above, and checked to be the entry it expects. + +// Read without following a symlink, and without blocking on a FIFO swapped +// in after the lstat (a regular file ignores O_NONBLOCK). Both are undefined +// on Windows, where the lstat identity checks stand in. +function readFlags(): number { + return fs.constants.O_RDONLY | noFollow() | (fs.constants.O_NONBLOCK ?? 0); +} + +function readChunk(fd: number, buffer: Buffer): Promise { + return new Promise((resolve, reject) => + fs.read(fd, buffer, 0, buffer.length, null, (error, bytesRead) => + error ? reject(error) : resolve(bytesRead), + ), + ); +} + +function writeChunk(fd: number, chunk: Buffer): Promise { + return new Promise((resolve, reject) => + fs.writeFile(fd, chunk, (error) => (error ? reject(error) : resolve())), + ); +} + +function keepModeAndTimes(fd: number, source: fs.Stats): Promise { + return new Promise((resolve, reject) => + fs.fchmod(fd, source.mode & 0o777, (chmodError) => + chmodError + ? reject(chmodError) + : fs.futimes(fd, source.atime, source.mtime, (timesError) => + timesError ? reject(timesError) : resolve(), + ), + ), + ); +} + +/** + * Open the regular file at `sourcePath` for reading, relative to its anchored + * directory. A directory, a symlink or anything else that is not a regular + * file is refused: a symlink is not followed or recreated, since its target + * could name anything, and a directory is not copied recursively. + */ +function openSourceInDirectory(sourcePath: string): { + fd: number; + opened: fs.Stats; +} { + const name = path.basename(sourcePath); + return inDirectory(path.dirname(sourcePath), () => { + const found = fs.lstatSync(name); + if (found.isDirectory()) { + throw new Error( + `Cannot move a directory across filesystems: ${sourcePath}. ` + + "Create the directory at the destination and move its files one at a time.", + ); + } + if (!found.isFile()) { + throw new Error( + `Cannot move across filesystems: not a regular file: ${sourcePath}`, + ); + } + const fd = fs.openSync(name, readFlags()); + try { + const opened = fs.fstatSync(fd); + if (!opened.isFile() || !isSameEntry(opened, found)) { + throw new Error( + `Refusing to move: ${sourcePath} was replaced while it was being opened`, + ); + } + return { fd, opened }; + } catch (error) { + fs.closeSync(fd); + throw error; + } + }); +} + +/** + * Remove `filePath`, relative to its anchored directory, only while it is + * still the entry `expected` describes. + */ +function removeFromDirectory( + filePath: string, + expected: { dev: number; ino: number }, +): void { + const name = path.basename(filePath); + inDirectory(path.dirname(filePath), () => { + if (!isSameEntry(fs.lstatSync(name), expected)) { + throw new Error( + `Refusing to remove: ${filePath} was replaced during the move`, + ); + } + fs.unlinkSync(name); + }); +} + +/** Remove a copy this move created, keeping the error that made it necessary. */ +function discardCopy( + destinationPath: string, + created: { dev: number; ino: number }, +): void { + try { + removeFromDirectory(destinationPath, created); + } catch { + // Left in place; the error that made the move fail is the one to report. + } +} + +/** + * Create `destinationPath`, which must not exist, as a copy of the file open + * at `sourceFd`, with its mode and times. A copy that fails is removed. + */ +async function copyIntoNewFile( + sourceFd: number, + source: fs.Stats, + destinationPath: string, +): Promise { + // O_EXCL: an existing file or symlink fails with EEXIST, never followed. + const { fd, opened } = openInDirectory( + destinationPath, + fs.constants.O_WRONLY | + fs.constants.O_CREAT | + fs.constants.O_EXCL | + noFollow(), + false, + ); + try { + try { + const buffer = Buffer.alloc(64 * 1024); + for (;;) { + const bytesRead = await readChunk(sourceFd, buffer); + if (bytesRead === 0) { + break; + } + await writeChunk(fd, buffer.subarray(0, bytesRead)); + } + await keepModeAndTimes(fd, source); + } finally { + await close(fd); + } + } catch (error) { + discardCopy(destinationPath, opened); + throw error; + } + return opened; +} + +/** + * Move the regular file at `sourcePath` to `destinationPath`, which must not + * exist, where the two are on different devices: copy it into a newly created + * file, then remove the source. If any step fails, the copy is removed and the + * source is left where it was. + */ +export async function moveFileAcrossDevices( + sourcePath: string, + destinationPath: string, +): Promise { + const { fd, opened: source } = openSourceInDirectory(sourcePath); + let created: fs.Stats; + try { + created = await copyIntoNewFile(fd, source, destinationPath); + } finally { + await close(fd); + } + try { + removeFromDirectory(sourcePath, source); + } catch (error) { + discardCopy(destinationPath, created); + throw error; + } +} + /** * Create `directory` and any missing parents, like mkdir -p: anchor at the * deepest ancestor that exists, then step down one component at a time, From 5c65ee68c0076c7bd2262f55b8f9c8ffb4df5653 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 01:40:28 -0400 Subject: [PATCH 2/6] docs(filesystem): say only a directory is refused across filesystems A symlink source is resolved by validatePath before the move, so its target is what moves, on one device or across two; the copy refuses a symlink only when one is swapped in after validation. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .changeset/5188-filesystem-move-exdev.md | 2 +- src/filesystem/README.md | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.changeset/5188-filesystem-move-exdev.md b/.changeset/5188-filesystem-move-exdev.md index 3826f11fc1..c6f35e65c9 100644 --- a/.changeset/5188-filesystem-move-exdev.md +++ b/.changeset/5188-filesystem-move-exdev.md @@ -2,4 +2,4 @@ "@modelcontextprotocol/server-filesystem": patch --- -`move_file` now moves a regular file between allowed directories on different filesystems (mounts, disks or shares), where it used to fail with `EXDEV`: it copies the file to a newly created destination, keeping its permissions and modification time, then removes the source, and removes a partial copy if anything fails. A directory or a symbolic link still cannot be moved across filesystems, and now fails with an error that says so. +`move_file` now moves a regular file between allowed directories on different filesystems (mounts, disks or shares), where it used to fail with `EXDEV`: it copies the file to a newly created destination, keeping its permissions and modification time, then removes the source, and removes a partial copy if anything fails. A directory still cannot be moved across filesystems, and now fails with an error that says so. diff --git a/src/filesystem/README.md b/src/filesystem/README.md index f5adc3520f..dbb89834a9 100644 --- a/src/filesystem/README.md +++ b/src/filesystem/README.md @@ -165,9 +165,8 @@ The server's directory access control follows this flow: disks), a regular file is moved by copying it to a newly created destination, with its permissions and modification time, and then removing the source. If the copy fails, the partial destination is removed and the - source is left in place. A directory or a symbolic link cannot be moved - across filesystems and fails with an error; move a directory's files one - at a time instead + source is left in place. A directory cannot be moved across filesystems + and fails with an error; move its files one at a time instead - **search_files** - Recursively search for files/directories that match or do not match patterns From 790d7cd0435e78535f496ba1e895d0aab92c4ada Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 01:48:40 -0400 Subject: [PATCH 3/6] fix(filesystem): create the copy owner-only, and keep the source unless the copy is in place Review round 1 on #5322: - the copy is created 0600 and takes the source's mode only once it is complete (openInDirectory gains an optional creation mode, default unchanged), so a private file is never readable at the destination - before removing the source, the copy must still be at the destination (anchored lstat identity); if it was removed or replaced, the source stays and the move fails - a failure closing the source after a complete copy now removes the copy too, so a retry is not blocked by a leftover destination Rebased onto #5325 (#5154): the cross-device move's identity checks take BigInt stats in isSameEntry's (pathStat, handleStat) order, and the copy's identity is read again from its descriptor after the write, since isSameEntry's Windows dev-0 fallback compares size and mtime. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../__tests__/pinned-directory.test.ts | 153 +++++++++++++++++- src/filesystem/pinned-directory.ts | 95 +++++++---- 2 files changed, 217 insertions(+), 31 deletions(-) diff --git a/src/filesystem/__tests__/pinned-directory.test.ts b/src/filesystem/__tests__/pinned-directory.test.ts index 7c5f35109a..7c3cf40cd4 100644 --- a/src/filesystem/__tests__/pinned-directory.test.ts +++ b/src/filesystem/__tests__/pinned-directory.test.ts @@ -519,7 +519,7 @@ describe("moveFileAcrossDevices", () => { ); it.each([ - ["another file", { ino: -1 }], + ["another file", { ino: -1n }], ["not a regular file", { isFile: () => false }], ])( "refuses, and closes the source, when it was replaced while being opened (%s)", @@ -527,9 +527,14 @@ describe("moveFileAcrossDevices", () => { const source = path.join(dir, "a.txt"); fs.writeFileSync(source, "a"); const realFstat = fs.fstatSync; - vi.spyOn(fs, "fstatSync").mockImplementationOnce((fd) => - Object.assign(realFstat(fd), replaced), - ); + vi.spyOn(fs, "fstatSync").mockImplementationOnce((( + fd: number, + options?: fs.StatOptions, + ) => + Object.assign( + realFstat(fd, options), + replaced, + )) as typeof fs.fstatSync); const closeSync = vi.spyOn(fs, "closeSync"); await expect( moveFileAcrossDevices(source, path.join(outside, "b.txt")), @@ -555,6 +560,146 @@ describe("moveFileAcrossDevices", () => { }, ); + it.skipIf(process.platform === "win32")( + "creates the copy owner-only while it is being written", + async () => { + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "a"); + fs.chmodSync(source, 0o644); + const modes: number[] = []; + const realWriteFile = fs.writeFile; + vi.spyOn(fs, "writeFile").mockImplementationOnce((( + ...args: unknown[] + ) => { + modes.push(fs.statSync(destination).mode & 0o777); + return (realWriteFile as (...a: unknown[]) => void)(...args); + }) as typeof fs.writeFile); + await moveFileAcrossDevices(source, destination); + expect(modes).toEqual([0o600]); + expect(fs.statSync(destination).mode & 0o777).toBe(0o644); + }, + ); + + it.each([ + ["removed", (destination: string) => fs.unlinkSync(destination)], + [ + "replaced", + (destination: string) => { + fs.unlinkSync(destination); + fs.writeFileSync(destination, "someone else's"); + }, + ], + ])( + "keeps the source when the copy was %s during the move", + async (_, interfere) => { + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "a"); + const realFchmod = fs.fchmod; + vi.spyOn(fs, "fchmod").mockImplementationOnce((( + fd: number, + mode: number, + callback: (error: NodeJS.ErrnoException | null) => void, + ) => { + interfere(destination); + realFchmod(fd, mode, callback); + }) as typeof fs.fchmod); + await expect(moveFileAcrossDevices(source, destination)).rejects.toThrow( + `Refusing to remove the source: ${destination} was removed or replaced during the move`, + ); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + }, + ); + + it("reports a failure to check the copy, and keeps the source", async () => { + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "a"); + const realLstat = fs.lstatSync; + // The first lstat of the copy is the create's own check; fail the second. + let checks = 0; + vi.spyOn(fs, "lstatSync").mockImplementation((( + p: fs.PathLike, + options?: fs.StatSyncOptions, + ) => { + if (process.cwd() === outside && p === "b.txt" && ++checks === 2) { + throw errno("EIO"); + } + return realLstat(p, options); + }) as typeof fs.lstatSync); + await expect(moveFileAcrossDevices(source, destination)).rejects.toThrow( + "EIO", + ); + // The third is discarding the copy, which goes ahead. + expect(checks).toBe(3); + expect(fs.readdirSync(outside)).toEqual([]); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + }); + + it("removes the copy, and keeps the source, when closing the source fails", async () => { + const source = path.join(dir, "a.txt"); + fs.writeFileSync(source, "a"); + const realClose = fs.close; + let closes = 0; + vi.spyOn(fs, "close").mockImplementation((( + fd: number, + callback: (error: NodeJS.ErrnoException | null) => void, + ) => { + // The copy's descriptor closes first, then the source's. + closes += 1; + realClose(fd, (error) => callback(closes === 2 ? errno("EIO") : error)); + }) as typeof fs.close); + await expect( + moveFileAcrossDevices(source, path.join(outside, "b.txt")), + ).rejects.toThrow("EIO"); + expect(fs.readdirSync(outside)).toEqual([]); + expect(fs.readFileSync(source, "utf-8")).toBe("a"); + }); + + it("checks the copy against its identity after the write, where Windows compares size and time", async () => { + // libuv < 1.51 on Windows reports dev 0 on a path stat, so isSameEntry + // compares size and mtime there: the identity from the create (empty) + // would no longer match the finished copy. + Object.defineProperty(process, "platform", { value: "win32" }); + const realLstat = fs.lstatSync; + vi.spyOn(fs, "lstatSync").mockImplementation((( + p: fs.PathLike, + options?: fs.StatSyncOptions, + ) => { + const stats = realLstat(p, options); + return options?.bigint ? Object.assign(stats!, { dev: 0n }) : stats; + }) as typeof fs.lstatSync); + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "content"); + await moveFileAcrossDevices(source, destination); + expect(fs.readFileSync(destination, "utf-8")).toBe("content"); + expect(fs.existsSync(source)).toBe(false); + }); + + it("falls back to the create's identity when the copy cannot be stat'd before closing", async () => { + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "a"); + const realFstat = fs.fstatSync; + // The source's open and the copy's create stat first; fail the third. + let calls = 0; + vi.spyOn(fs, "fstatSync").mockImplementation((( + fd: number, + options?: fs.StatOptions, + ) => { + if (++calls === 3) { + throw errno("EIO"); + } + return realFstat(fd, options); + }) as typeof fs.fstatSync); + await moveFileAcrossDevices(source, destination); + expect(calls).toBe(3); + expect(fs.readFileSync(destination, "utf-8")).toBe("a"); + expect(fs.existsSync(source)).toBe(false); + }); + it("reports the copy's failure when removing the copy fails too", async () => { const source = path.join(dir, "a.txt"); fs.writeFileSync(source, "a"); diff --git a/src/filesystem/pinned-directory.ts b/src/filesystem/pinned-directory.ts index fa9c71552d..5609a1691a 100644 --- a/src/filesystem/pinned-directory.ts +++ b/src/filesystem/pinned-directory.ts @@ -208,13 +208,14 @@ function openInDirectory( filePath: string, flags: number, refuseLink: boolean, + mode: number = 0o666, ): { fd: number; opened: fs.BigIntStats } { const name = path.basename(filePath); return inDirectory(path.dirname(filePath), () => { if (refuseLink && fs.lstatSync(name).isSymbolicLink()) { throw new Error(`Refusing to write: not a regular file: ${filePath}`); } - const fd = fs.openSync(name, flags, 0o666); + const fd = fs.openSync(name, flags, mode); try { const opened = fs.fstatSync(fd, { bigint: true }); if (!isSameEntry(fs.lstatSync(name, { bigint: true }), opened)) { @@ -319,9 +320,9 @@ function writeChunk(fd: number, chunk: Buffer): Promise { ); } -function keepModeAndTimes(fd: number, source: fs.Stats): Promise { +function keepModeAndTimes(fd: number, source: fs.BigIntStats): Promise { return new Promise((resolve, reject) => - fs.fchmod(fd, source.mode & 0o777, (chmodError) => + fs.fchmod(fd, Number(source.mode & 0o777n), (chmodError) => chmodError ? reject(chmodError) : fs.futimes(fd, source.atime, source.mtime, (timesError) => @@ -339,11 +340,11 @@ function keepModeAndTimes(fd: number, source: fs.Stats): Promise { */ function openSourceInDirectory(sourcePath: string): { fd: number; - opened: fs.Stats; + opened: fs.BigIntStats; } { const name = path.basename(sourcePath); return inDirectory(path.dirname(sourcePath), () => { - const found = fs.lstatSync(name); + const found = fs.lstatSync(name, { bigint: true }); if (found.isDirectory()) { throw new Error( `Cannot move a directory across filesystems: ${sourcePath}. ` + @@ -357,8 +358,8 @@ function openSourceInDirectory(sourcePath: string): { } const fd = fs.openSync(name, readFlags()); try { - const opened = fs.fstatSync(fd); - if (!opened.isFile() || !isSameEntry(opened, found)) { + const opened = fs.fstatSync(fd, { bigint: true }); + if (!opened.isFile() || !isSameEntry(found, opened)) { throw new Error( `Refusing to move: ${sourcePath} was replaced while it was being opened`, ); @@ -372,16 +373,38 @@ function openSourceInDirectory(sourcePath: string): { } /** - * Remove `filePath`, relative to its anchored directory, only while it is - * still the entry `expected` describes. + * Check that `filePath`, relative to its anchored directory, is still the + * entry `expected` describes, and fail with `message` if it is not. */ -function removeFromDirectory( +function confirmInDirectory( filePath: string, - expected: { dev: number; ino: number }, + expected: EntryIdentity, + message: string, ): void { const name = path.basename(filePath); inDirectory(path.dirname(filePath), () => { - if (!isSameEntry(fs.lstatSync(name), expected)) { + let found: fs.BigIntStats | undefined; + try { + found = fs.lstatSync(name, { bigint: true }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error; + } + } + if (!found || !isSameEntry(found, expected)) { + throw new Error(message); + } + }); +} + +/** + * Remove `filePath`, relative to its anchored directory, only while it is + * still the entry `expected` describes. + */ +function removeFromDirectory(filePath: string, expected: EntryIdentity): void { + const name = path.basename(filePath); + inDirectory(path.dirname(filePath), () => { + if (!isSameEntry(fs.lstatSync(name, { bigint: true }), expected)) { throw new Error( `Refusing to remove: ${filePath} was replaced during the move`, ); @@ -391,10 +414,7 @@ function removeFromDirectory( } /** Remove a copy this move created, keeping the error that made it necessary. */ -function discardCopy( - destinationPath: string, - created: { dev: number; ino: number }, -): void { +function discardCopy(destinationPath: string, created: EntryIdentity): void { try { removeFromDirectory(destinationPath, created); } catch { @@ -408,9 +428,9 @@ function discardCopy( */ async function copyIntoNewFile( sourceFd: number, - source: fs.Stats, + source: fs.BigIntStats, destinationPath: string, -): Promise { +): Promise { // O_EXCL: an existing file or symlink fails with EEXIST, never followed. const { fd, opened } = openInDirectory( destinationPath, @@ -419,7 +439,14 @@ async function copyIntoNewFile( fs.constants.O_EXCL | noFollow(), false, + // Owner-only until the copy is complete and takes the source's mode, so + // a private file is never readable at the destination in between. + 0o600, ); + // Where isSameEntry falls back to size and modification time (see there), + // the identity from the create goes stale as the copy is written, so it is + // read again from the descriptor before that closes. + let identity: EntryIdentity = opened; try { try { const buffer = Buffer.alloc(64 * 1024); @@ -432,13 +459,18 @@ async function copyIntoNewFile( } await keepModeAndTimes(fd, source); } finally { + try { + identity = fs.fstatSync(fd, { bigint: true }); + } catch { + // Keep the identity from the create; the close reports the fault. + } await close(fd); } } catch (error) { - discardCopy(destinationPath, opened); + discardCopy(destinationPath, identity); throw error; } - return opened; + return identity; } /** @@ -452,16 +484,25 @@ export async function moveFileAcrossDevices( destinationPath: string, ): Promise { const { fd, opened: source } = openSourceInDirectory(sourcePath); - let created: fs.Stats; - try { - created = await copyIntoNewFile(fd, source, destinationPath); - } finally { - await close(fd); - } + // Set once the copy is complete; until then copyIntoNewFile cleans up. + let created: EntryIdentity | undefined; try { + try { + created = await copyIntoNewFile(fd, source, destinationPath); + } finally { + await close(fd); + } + // The source goes only while the copy is still where the move put it. + confirmInDirectory( + destinationPath, + created, + `Refusing to remove the source: ${destinationPath} was removed or replaced during the move`, + ); removeFromDirectory(sourcePath, source); } catch (error) { - discardCopy(destinationPath, created); + if (created) { + discardCopy(destinationPath, created); + } throw error; } } From 1860bb120b56a2f038a825d222138b385f94bc4d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 08:52:53 -0400 Subject: [PATCH 4/6] fix(filesystem): keep a source written to in place during a cross-device move Review round 3 on #5322: the source's device and inode still match after another writer edits it in place, so the move would delete the edited source while the destination held the content read before the edit. Before the source is removed, its size, mtime and ctime (ns) must also match the stat taken when it was opened; otherwise the copy is removed and the source kept. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../__tests__/pinned-directory.test.ts | 23 +++++++++++++++++++ src/filesystem/pinned-directory.ts | 23 ++++++++++++++++--- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/filesystem/__tests__/pinned-directory.test.ts b/src/filesystem/__tests__/pinned-directory.test.ts index 7c3cf40cd4..256db9c819 100644 --- a/src/filesystem/__tests__/pinned-directory.test.ts +++ b/src/filesystem/__tests__/pinned-directory.test.ts @@ -700,6 +700,29 @@ describe("moveFileAcrossDevices", () => { expect(fs.existsSync(source)).toBe(false); }); + it("removes the copy, and keeps the source, when the source was written to in place during the move", async () => { + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "a"); + const before = fs.lstatSync(source).ino; + const realFchmod = fs.fchmod; + // fchmod runs once the copy has read to the end of the source. + vi.spyOn(fs, "fchmod").mockImplementationOnce((( + fd: number, + mode: number, + callback: (error: NodeJS.ErrnoException | null) => void, + ) => { + fs.appendFileSync(source, " and more"); + realFchmod(fd, mode, callback); + }) as typeof fs.fchmod); + await expect(moveFileAcrossDevices(source, destination)).rejects.toThrow( + `Refusing to remove: ${source} changed during the move`, + ); + expect(fs.lstatSync(source).ino).toBe(before); + expect(fs.readFileSync(source, "utf-8")).toBe("a and more"); + expect(fs.readdirSync(outside)).toEqual([]); + }); + it("reports the copy's failure when removing the copy fails too", async () => { const source = path.join(dir, "a.txt"); fs.writeFileSync(source, "a"); diff --git a/src/filesystem/pinned-directory.ts b/src/filesystem/pinned-directory.ts index 5609a1691a..f0e4bfd081 100644 --- a/src/filesystem/pinned-directory.ts +++ b/src/filesystem/pinned-directory.ts @@ -401,14 +401,31 @@ function confirmInDirectory( * Remove `filePath`, relative to its anchored directory, only while it is * still the entry `expected` describes. */ -function removeFromDirectory(filePath: string, expected: EntryIdentity): void { +function removeFromDirectory( + filePath: string, + expected: EntryIdentity, + snapshot?: Pick, +): void { const name = path.basename(filePath); inDirectory(path.dirname(filePath), () => { - if (!isSameEntry(fs.lstatSync(name, { bigint: true }), expected)) { + const found = fs.lstatSync(name, { bigint: true }); + if (!isSameEntry(found, expected)) { throw new Error( `Refusing to remove: ${filePath} was replaced during the move`, ); } + // The same file, written to in place after it was copied: removing it + // would lose what was written, so the copy is the one that goes. + if ( + snapshot && + (found.size !== snapshot.size || + found.mtimeNs !== snapshot.mtimeNs || + found.ctimeNs !== snapshot.ctimeNs) + ) { + throw new Error( + `Refusing to remove: ${filePath} changed during the move`, + ); + } fs.unlinkSync(name); }); } @@ -498,7 +515,7 @@ export async function moveFileAcrossDevices( created, `Refusing to remove the source: ${destinationPath} was removed or replaced during the move`, ); - removeFromDirectory(sourcePath, source); + removeFromDirectory(sourcePath, source, source); } catch (error) { if (created) { discardCopy(destinationPath, created); From 215815d1e3d0c2bedbe6038f82675db4bb36ac84 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 09:02:44 -0400 Subject: [PATCH 5/6] fix(filesystem): keep sub-millisecond times on a cross-device copy Review round 4 on #5322: futimes was given the source's Date times, which keep only milliseconds. Pass seconds derived from atimeNs and mtimeNs instead. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/filesystem/__tests__/pinned-directory.test.ts | 13 +++++++++++++ src/filesystem/pinned-directory.ts | 13 +++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/filesystem/__tests__/pinned-directory.test.ts b/src/filesystem/__tests__/pinned-directory.test.ts index 256db9c819..4bb24000ad 100644 --- a/src/filesystem/__tests__/pinned-directory.test.ts +++ b/src/filesystem/__tests__/pinned-directory.test.ts @@ -457,6 +457,19 @@ describe("moveFileAcrossDevices", () => { expect(process.cwd()).toBe(cwd); }); + it("keeps the modification time below a millisecond", async () => { + const source = path.join(dir, "a.txt"); + const destination = path.join(outside, "b.txt"); + fs.writeFileSync(source, "a"); + // 0.5 ms past a whole millisecond: a Date would drop it. + fs.utimesSync(source, 1_000_000_000.1235, 1_000_000_000.1235); + const before = fs.statSync(source, { bigint: true }).mtimeNs; + await moveFileAcrossDevices(source, destination); + const after = fs.statSync(destination, { bigint: true }).mtimeNs; + const drift = after > before ? after - before : before - after; + expect(drift < 1000n).toBe(true); + }); + it("moves an empty file", async () => { const source = path.join(dir, "empty"); fs.writeFileSync(source, ""); diff --git a/src/filesystem/pinned-directory.ts b/src/filesystem/pinned-directory.ts index f0e4bfd081..e8d562ca4d 100644 --- a/src/filesystem/pinned-directory.ts +++ b/src/filesystem/pinned-directory.ts @@ -320,13 +320,22 @@ function writeChunk(fd: number, chunk: Buffer): Promise { ); } +function seconds(nanoseconds: bigint): number { + return Number(nanoseconds) / 1e9; +} + function keepModeAndTimes(fd: number, source: fs.BigIntStats): Promise { return new Promise((resolve, reject) => fs.fchmod(fd, Number(source.mode & 0o777n), (chmodError) => chmodError ? reject(chmodError) - : fs.futimes(fd, source.atime, source.mtime, (timesError) => - timesError ? reject(timesError) : resolve(), + : // In seconds from the nanosecond fields: a Date keeps only + // milliseconds. + fs.futimes( + fd, + seconds(source.atimeNs), + seconds(source.mtimeNs), + (timesError) => (timesError ? reject(timesError) : resolve()), ), ), ); From 7fabe2ad3659f8f0ca5a61176c9066b5588d70da Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 27 Oct 2026 09:10:32 -0400 Subject: [PATCH 6/6] fix(filesystem): keep a pre-1970 time on a cross-device copy Review round 5 on #5322: fs.futimes takes a negative number to mean "now", so the seconds passed since round 4 lost any time before 1970. Pass them as a numeric string, which it parses as given. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/filesystem/__tests__/pinned-directory.test.ts | 10 +++++++--- src/filesystem/pinned-directory.ts | 6 ++++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/filesystem/__tests__/pinned-directory.test.ts b/src/filesystem/__tests__/pinned-directory.test.ts index 4bb24000ad..a395187025 100644 --- a/src/filesystem/__tests__/pinned-directory.test.ts +++ b/src/filesystem/__tests__/pinned-directory.test.ts @@ -457,12 +457,16 @@ describe("moveFileAcrossDevices", () => { expect(process.cwd()).toBe(cwd); }); - it("keeps the modification time below a millisecond", async () => { + it.each([ + // 0.5 ms past a whole millisecond: a Date would drop it. + ["below a millisecond", 1_000_000_000.1235], + // Before 1970: futimes takes a negative number to mean "now". + ["before 1970", new Date(-157_766_400_250)], + ])("keeps the modification time %s", async (_, time) => { const source = path.join(dir, "a.txt"); const destination = path.join(outside, "b.txt"); fs.writeFileSync(source, "a"); - // 0.5 ms past a whole millisecond: a Date would drop it. - fs.utimesSync(source, 1_000_000_000.1235, 1_000_000_000.1235); + fs.utimesSync(source, time, time); const before = fs.statSync(source, { bigint: true }).mtimeNs; await moveFileAcrossDevices(source, destination); const after = fs.statSync(destination, { bigint: true }).mtimeNs; diff --git a/src/filesystem/pinned-directory.ts b/src/filesystem/pinned-directory.ts index e8d562ca4d..8b2e63ad6d 100644 --- a/src/filesystem/pinned-directory.ts +++ b/src/filesystem/pinned-directory.ts @@ -320,8 +320,10 @@ function writeChunk(fd: number, chunk: Buffer): Promise { ); } -function seconds(nanoseconds: bigint): number { - return Number(nanoseconds) / 1e9; +// A numeric string, since fs.futimes takes a negative number to mean "now", +// which would lose a time before 1970; a string it parses as given. +function seconds(nanoseconds: bigint): string { + return String(Number(nanoseconds) / 1e9); } function keepModeAndTimes(fd: number, source: fs.BigIntStats): Promise {