From c264d9e776d01a23c699a2b502c88afd12cd3c39 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 22 Oct 2026 06:28:55 +0000 Subject: [PATCH 1/9] reproduces issue 14967 Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/atomicWrite.test.ts | 76 +++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index c8d9dc116668d..19bef8a85cb62 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -130,6 +130,82 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { assert.deepStrictEqual(yield* fs.readDirectory(root), ["settings.json"]); }), ); + + it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( + "keeps the permissions of the file it replaces", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + yield* fs.writeFileString(filePath, "before"); + yield* fs.chmod(filePath, 0o600); + + yield* writeFileStringAtomically({ filePath, contents: "after" }); + + assert.strictEqual(yield* fs.readFileString(filePath), "after"); + assert.strictEqual(((yield* fs.stat(filePath)).mode & 0o777).toString(8), "600"); + }), + ); + + it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( + "creates a new file readable only by its owner", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + + yield* writeFileStringAtomically({ filePath, contents: "fresh" }); + + assert.strictEqual(((yield* fs.stat(filePath)).mode & 0o777).toString(8), "600"); + }), + ); + + it.effect("syncs the contents before the rename and the directory after it", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + const events: Array = []; + const recording = FileSystem.make({ + ...fs, + open: (openedPath, options) => + fs.open(openedPath, options).pipe( + Effect.map((file) => ({ + ...file, + sync: file.sync.pipe( + Effect.tap(() => + Effect.sync(() => { + events.push(openedPath === root ? "sync directory" : "sync temp file"); + }), + ), + ), + })), + ), + rename: (from, to) => + fs + .rename(from, to) + .pipe( + Effect.tap(() => + Effect.sync(() => + events.push(from === filePath ? "rename target" : "rename temp file"), + ), + ), + ), + }); + + yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( + Effect.provideService(FileSystem.FileSystem, recording), + ); + + assert.strictEqual(yield* fs.readFileString(filePath), "after"); + assert.deepStrictEqual(events, ["sync temp file", "rename temp file", "sync directory"]); + }), + ); }); // Retries stat the real target between waits, and that I/O finishes outside the From 7ebc0f149fc2dcedf072b0aa57db829108f46b3d Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 22 Oct 2026 08:13:39 +0000 Subject: [PATCH 2/9] fix(shared): atomic saves keep file permissions and sync to disk Saving settings replaced a 0600 file with one at the umask default (0664). The atomic writer now keeps the replaced file's permission bits, creates new files as 0600, syncs the temp file before the rename, and syncs the directory after it on platforms that can open one. Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/atomicWrite.test.ts | 43 ++++++++++++++++++------- packages/shared/src/atomicWrite.ts | 32 +++++++++++++++++- 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index 19bef8a85cb62..74c876db7857e 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -175,16 +175,20 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { ...fs, open: (openedPath, options) => fs.open(openedPath, options).pipe( - Effect.map((file) => ({ - ...file, - sync: file.sync.pipe( - Effect.tap(() => - Effect.sync(() => { - events.push(openedPath === root ? "sync directory" : "sync temp file"); - }), - ), - ), - })), + // The real handle keeps its methods on its prototype, so it is extended, not spread. + Effect.map((file): FileSystem.File => + Object.create(file, { + sync: { + value: file.sync.pipe( + Effect.tap(() => + Effect.sync(() => { + events.push(openedPath === root ? "sync directory" : "sync temp file"); + }), + ), + ), + }, + }), + ), ), rename: (from, to) => fs @@ -203,7 +207,13 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { ); assert.strictEqual(yield* fs.readFileString(filePath), "after"); - assert.deepStrictEqual(events, ["sync temp file", "rename temp file", "sync directory"]); + // Windows cannot open a directory to sync it. + assert.deepStrictEqual( + events, + HostProcess.Platform.defaultValue() === "win32" + ? ["sync temp file", "rename temp file"] + : ["sync temp file", "rename temp file", "sync directory"], + ); }), ); }); @@ -281,7 +291,18 @@ it.effect("succeeds when the write lands but its temp directory cannot be remove ), makeDirectory: () => Effect.void, makeTempDirectory: () => Effect.succeed("/home/settings.json.abc123"), + stat: (path) => + Effect.fail( + PlatformError.systemError({ + _tag: "NotFound", + module: "FileSystem", + method: "stat", + pathOrDescriptor: path, + }), + ), + chmod: () => Effect.void, writeFileString: () => Effect.void, + open: () => Effect.succeed({ sync: Effect.void } as FileSystem.File), rename: (_from, to) => Effect.sync(() => void renamed.push(to)), remove: (path) => Effect.sync(() => void removed.push(path)).pipe( diff --git a/packages/shared/src/atomicWrite.ts b/packages/shared/src/atomicWrite.ts index 709614d86fdea..ae6a7c85693cf 100644 --- a/packages/shared/src/atomicWrite.ts +++ b/packages/shared/src/atomicWrite.ts @@ -22,6 +22,18 @@ export const isWindowsRenameLock = (error: PlatformError.PlatformError) => { const LOCKED_RENAME_RETRIES = 20; const LOCKED_RENAME_RETRY_DELAY = "100 millis"; +// A save keeps the permissions of the file it replaces, so a private file stays +// private, and a new file is readable only by its owner. chmod rather than an open +// mode, because the umask narrows the latter. +const replacedMode = (fs: FileSystem.FileSystem, targetPath: string) => + fs.stat(targetPath).pipe( + Effect.map((info) => info.mode & 0o777), + Effect.catchIf( + (error) => error.reason._tag === "NotFound", + () => Effect.succeed(0o600), + ), + ); + /** * A symlinked target is resolved first so the link survives and its destination is * rewritten, since renaming over the link itself would swap it for a regular file. @@ -52,7 +64,25 @@ export const writeFileStringAtomically = (input: { const tempPath = path.join(tempDirectory, "contents.tmp"); yield* fs.writeFileString(tempPath, input.contents); - if (platform !== "win32") return yield* fs.rename(tempPath, targetPath); + // Windows modes carry only the read-only bit, which the rename below handles. + if (platform !== "win32") yield* fs.chmod(tempPath, yield* replacedMode(fs, targetPath)); + // Without this a crash soon after the rename can leave the target empty. A sync + // flushes the file, not just its handle, and the handle closes before the rename, + // which Windows refuses while it is open. + yield* fs.open(tempPath, { flag: "r+" }).pipe( + Effect.flatMap((file) => file.sync), + Effect.scoped, + ); + if (platform !== "win32") { + yield* fs.rename(tempPath, targetPath); + // Persists the rename. Windows cannot open a directory to sync it, and a + // filesystem that refuses must not fail a write that already landed. + return yield* fs.open(targetDirectory, { flag: "r" }).pipe( + Effect.flatMap((directory) => directory.sync), + Effect.scoped, + Effect.ignore({ log: true }), + ); + } // A retry must not replace a save another process landed while this one // waited, so it gives up once the target is no longer the file seen here. From 80cf07e3bc8b8e412837a749ec156b073b4cede6 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 22 Oct 2026 08:21:21 +0000 Subject: [PATCH 3/9] fix(shared): read-only targets save again and failed directory syncs surface The preserved mode was applied before the temp file was reopened for its sync, so a 0444, 0400, 0200, or 0000 target failed to save even in a writable directory. The handle now opens first, the mode is set while it is open, and one sync flushes contents and mode before the rename. A directory sync that fails after the rename now fails the save; only EINVAL, ENOTSUP, and ENOSYS, which mean the filesystem cannot sync a directory, are logged and allowed. Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/atomicWrite.test.ts | 156 +++++++++++++----------- packages/shared/src/atomicWrite.ts | 27 ++-- 2 files changed, 105 insertions(+), 78 deletions(-) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index 74c876db7857e..c34a919c2c74e 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -131,22 +131,28 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { }), ); - it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( - "keeps the permissions of the file it replaces", - () => - Effect.gen(function* () { - const fs = yield* FileSystem.FileSystem; - const path = yield* Path.Path; - const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); - const filePath = path.join(root, "settings.json"); - yield* fs.writeFileString(filePath, "before"); - yield* fs.chmod(filePath, 0o600); + // Read-only and write-only targets replace fine in a writable directory, so the + // narrowed mode lands only after the temp file is open for writing. + it.effect.each( + HostProcess.Platform.defaultValue() === "win32" + ? [] + : ["644", "600", "444", "400", "200", "000"], + )("keeps the %s permissions of the file it replaces", (octal) => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + const mode = Number.parseInt(octal, 8); + yield* fs.writeFileString(filePath, "before"); + yield* fs.chmod(filePath, mode); - yield* writeFileStringAtomically({ filePath, contents: "after" }); + yield* writeFileStringAtomically({ filePath, contents: "after" }); - assert.strictEqual(yield* fs.readFileString(filePath), "after"); - assert.strictEqual(((yield* fs.stat(filePath)).mode & 0o777).toString(8), "600"); - }), + assert.strictEqual((yield* fs.stat(filePath)).mode & 0o777, mode); + yield* fs.chmod(filePath, 0o600); + assert.strictEqual(yield* fs.readFileString(filePath), "after"); + }), ); it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( @@ -216,6 +222,72 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { ); }), ); + it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( + "fails when the directory cannot be synced after the rename", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + const syncFailure = PlatformError.systemError({ + _tag: "Unknown", + module: "FileSystem", + method: "sync", + pathOrDescriptor: root, + cause: Object.assign(new Error("EIO, fsync"), { code: "EIO" }), + }); + const failingDirectorySync = FileSystem.make({ + ...fs, + open: (openedPath, options) => + fs + .open(openedPath, options) + .pipe( + Effect.map((file): FileSystem.File => + openedPath === root + ? Object.create(file, { sync: { value: Effect.fail(syncFailure) } }) + : file, + ), + ), + }); + + const failure = yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( + Effect.provideService(FileSystem.FileSystem, failingDirectorySync), + Effect.flip, + ); + + assert.strictEqual(failure, syncFailure); + assert.strictEqual(yield* fs.readFileString(filePath), "after"); + }), + ); + + it.effect("succeeds when the write lands but its temp directory cannot be removed", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + const unremovable = FileSystem.make({ + ...fs, + remove: (removedPath) => + Effect.fail( + PlatformError.systemError({ + _tag: "PermissionDenied", + module: "FileSystem", + method: "remove", + pathOrDescriptor: removedPath, + }), + ), + }); + + yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( + Effect.provideService(FileSystem.FileSystem, unremovable), + ); + + assert.strictEqual(yield* fs.readFileString(filePath), "after"); + assert.strictEqual((yield* fs.readDirectory(root)).length, 2); + }), + ); }); // Retries stat the real target between waits, and that I/O finishes outside the @@ -274,59 +346,3 @@ it.effect("surfaces an unreadable link instead of writing over it", () => ), ), ); - -it.effect("succeeds when the write lands but its temp directory cannot be removed", () => - Effect.gen(function* () { - const renamed: Array = []; - const removed: Array = []; - const fileSystem = FileSystem.layerNoop({ - readLink: (path) => - Effect.fail( - PlatformError.systemError({ - _tag: "NotFound", - module: "FileSystem", - method: "readLink", - pathOrDescriptor: path, - }), - ), - makeDirectory: () => Effect.void, - makeTempDirectory: () => Effect.succeed("/home/settings.json.abc123"), - stat: (path) => - Effect.fail( - PlatformError.systemError({ - _tag: "NotFound", - module: "FileSystem", - method: "stat", - pathOrDescriptor: path, - }), - ), - chmod: () => Effect.void, - writeFileString: () => Effect.void, - open: () => Effect.succeed({ sync: Effect.void } as FileSystem.File), - rename: (_from, to) => Effect.sync(() => void renamed.push(to)), - remove: (path) => - Effect.sync(() => void removed.push(path)).pipe( - Effect.andThen( - Effect.fail( - PlatformError.systemError({ - _tag: "PermissionDenied", - module: "FileSystem", - method: "remove", - pathOrDescriptor: path, - }), - ), - ), - ), - }); - - const result = yield* Effect.exit( - writeFileStringAtomically({ filePath: "/home/settings.json", contents: "after" }).pipe( - Effect.provide(Layer.mergeAll(Path.layer, fileSystem)), - ), - ); - - assert.deepStrictEqual(result, Exit.void); - assert.deepStrictEqual(renamed, ["/home/settings.json"]); - assert.deepStrictEqual(removed, ["/home/settings.json.abc123"]); - }), -); diff --git a/packages/shared/src/atomicWrite.ts b/packages/shared/src/atomicWrite.ts index ae6a7c85693cf..4234fab1ac6fc 100644 --- a/packages/shared/src/atomicWrite.ts +++ b/packages/shared/src/atomicWrite.ts @@ -34,6 +34,11 @@ const replacedMode = (fs: FileSystem.FileSystem, targetPath: string) => ), ); +const isUnsupportedSync = (error: PlatformError.PlatformError) => { + const code = (error.reason.cause as NodeJS.ErrnoException | undefined)?.code; + return code === "EINVAL" || code === "ENOTSUP" || code === "ENOSYS"; +}; + /** * A symlinked target is resolved first so the link survives and its destination is * rewritten, since renaming over the link itself would swap it for a regular file. @@ -64,23 +69,29 @@ export const writeFileStringAtomically = (input: { const tempPath = path.join(tempDirectory, "contents.tmp"); yield* fs.writeFileString(tempPath, input.contents); - // Windows modes carry only the read-only bit, which the rename below handles. - if (platform !== "win32") yield* fs.chmod(tempPath, yield* replacedMode(fs, targetPath)); - // Without this a crash soon after the rename can leave the target empty. A sync - // flushes the file, not just its handle, and the handle closes before the rename, - // which Windows refuses while it is open. + // The handle opens before the chmod, which may drop write access, and closes + // before the rename, which Windows refuses while it is open. The sync flushes + // the contents and the mode, or a crash soon after the rename can leave the + // target empty. Windows modes carry only the read-only bit, which the rename + // below handles. yield* fs.open(tempPath, { flag: "r+" }).pipe( + Effect.tap(() => + platform === "win32" + ? Effect.void + : replacedMode(fs, targetPath).pipe(Effect.flatMap((mode) => fs.chmod(tempPath, mode))), + ), Effect.flatMap((file) => file.sync), Effect.scoped, ); if (platform !== "win32") { yield* fs.rename(tempPath, targetPath); - // Persists the rename. Windows cannot open a directory to sync it, and a - // filesystem that refuses must not fail a write that already landed. + // Persists the rename. Windows cannot open a directory to sync it. Only a + // filesystem that cannot sync a directory at all is let through: the rename + // is visible, but a failed sync means it may not survive a crash. return yield* fs.open(targetDirectory, { flag: "r" }).pipe( Effect.flatMap((directory) => directory.sync), Effect.scoped, - Effect.ignore({ log: true }), + Effect.catchIf(isUnsupportedSync, (error) => Effect.logWarning(error)), ); } From c93d54e71359065b1c54acbe9ba26a78c06ce8f6 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sat, 24 Oct 2026 02:27:59 +0000 Subject: [PATCH 4/9] fix(shared): an unopenable directory fails a save before the rename A directory without read permission (0300) let the rename land and then failed to open the directory for its sync, so the save reported an error after the target had already changed. The directory handle now opens before the temp file is written, so that failure leaves the original file intact. Sync errors after the rename still fail the save; EINVAL, ENOTSUP, and ENOSYS are still allowed. Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/atomicWrite.test.ts | 79 +++++++++++++++++-------- packages/shared/src/atomicWrite.ts | 17 +++--- 2 files changed, 65 insertions(+), 31 deletions(-) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index c34a919c2c74e..93c0e46b1b069 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -222,42 +222,73 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { ); }), ); + // EIO means the rename may not survive a crash. ENOTSUP means the filesystem + // cannot sync a directory at all, so the save stands. + it.effect.each( + HostProcess.Platform.defaultValue() === "win32" + ? [] + : [ + { code: "EIO", fails: true }, + { code: "ENOTSUP", fails: false }, + ], + )("handles a directory sync failing with $code after the rename", ({ code, fails }) => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + const syncFailure = PlatformError.systemError({ + _tag: "Unknown", + module: "FileSystem", + method: "sync", + pathOrDescriptor: root, + cause: Object.assign(new Error(`${code}, fsync`), { code }), + }); + const failingDirectorySync = FileSystem.make({ + ...fs, + open: (openedPath, options) => + fs + .open(openedPath, options) + .pipe( + Effect.map((file): FileSystem.File => + openedPath === root + ? Object.create(file, { sync: { value: Effect.fail(syncFailure) } }) + : file, + ), + ), + }); + + const exit = yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( + Effect.provideService(FileSystem.FileSystem, failingDirectorySync), + Effect.exit, + ); + + assert.deepStrictEqual(exit, fails ? Exit.fail(syncFailure) : Exit.void); + assert.strictEqual(yield* fs.readFileString(filePath), "after"); + }), + ); + + // Write and search but no read: the directory cannot be opened to sync it, which + // has to fail the save before the rename rather than after it. it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( - "fails when the directory cannot be synced after the rename", + "leaves the target intact when its directory cannot be opened for the sync", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; const path = yield* Path.Path; const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); const filePath = path.join(root, "settings.json"); - const syncFailure = PlatformError.systemError({ - _tag: "Unknown", - module: "FileSystem", - method: "sync", - pathOrDescriptor: root, - cause: Object.assign(new Error("EIO, fsync"), { code: "EIO" }), - }); - const failingDirectorySync = FileSystem.make({ - ...fs, - open: (openedPath, options) => - fs - .open(openedPath, options) - .pipe( - Effect.map((file): FileSystem.File => - openedPath === root - ? Object.create(file, { sync: { value: Effect.fail(syncFailure) } }) - : file, - ), - ), - }); + yield* fs.writeFileString(filePath, "before"); + yield* Effect.acquireRelease(fs.chmod(root, 0o300), () => + fs.chmod(root, 0o700).pipe(Effect.orDie), + ); const failure = yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( - Effect.provideService(FileSystem.FileSystem, failingDirectorySync), Effect.flip, ); - assert.strictEqual(failure, syncFailure); - assert.strictEqual(yield* fs.readFileString(filePath), "after"); + assert.strictEqual(failure.reason._tag, "PermissionDenied"); + assert.strictEqual(yield* fs.readFileString(filePath), "before"); }), ); diff --git a/packages/shared/src/atomicWrite.ts b/packages/shared/src/atomicWrite.ts index 4234fab1ac6fc..9c89300c58517 100644 --- a/packages/shared/src/atomicWrite.ts +++ b/packages/shared/src/atomicWrite.ts @@ -56,6 +56,11 @@ export const writeFileStringAtomically = (input: { const targetDirectory = path.dirname(targetPath); yield* fs.makeDirectory(targetDirectory, { recursive: true }); + // Opened before anything is written, so a directory that cannot be opened + // (one without read permission) fails the save while the target is intact. + // Windows cannot open a directory to sync it. + const directory = + platform === "win32" ? undefined : yield* fs.open(targetDirectory, { flag: "r" }); // The temp directory is cleanup, not part of the write: failing to remove // it (a virus scanner holding it on Windows) must not fail a write that // already landed. @@ -83,14 +88,12 @@ export const writeFileStringAtomically = (input: { Effect.flatMap((file) => file.sync), Effect.scoped, ); - if (platform !== "win32") { + if (directory !== undefined) { yield* fs.rename(tempPath, targetPath); - // Persists the rename. Windows cannot open a directory to sync it. Only a - // filesystem that cannot sync a directory at all is let through: the rename - // is visible, but a failed sync means it may not survive a crash. - return yield* fs.open(targetDirectory, { flag: "r" }).pipe( - Effect.flatMap((directory) => directory.sync), - Effect.scoped, + // Persists the rename. Only a filesystem that cannot sync a directory at all + // is let through: the rename is visible, but a failed sync means it may not + // survive a crash. + return yield* directory.sync.pipe( Effect.catchIf(isUnsupportedSync, (error) => Effect.logWarning(error)), ); } From 80f3935e01284c43364f83e7cbeec9d3a55b4250 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sat, 24 Oct 2026 02:44:00 +0000 Subject: [PATCH 5/9] test(shared): inject the directory open failure instead of relying on chmod Root ignores a chmod that drops read access, so the test never saw PermissionDenied when run as root. Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/atomicWrite.test.ts | 28 +++++++++++++++++-------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index 93c0e46b1b069..2227aa22db076 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -268,8 +268,9 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { }), ); - // Write and search but no read: the directory cannot be opened to sync it, which - // has to fail the save before the rename rather than after it. + // A directory that cannot be opened to sync it has to fail the save before the + // rename rather than after it. The failure is injected because root ignores a + // chmod that drops read access. it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( "leaves the target intact when its directory cannot be opened for the sync", () => @@ -279,15 +280,24 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); const filePath = path.join(root, "settings.json"); yield* fs.writeFileString(filePath, "before"); - yield* Effect.acquireRelease(fs.chmod(root, 0o300), () => - fs.chmod(root, 0o700).pipe(Effect.orDie), + const openFailure = PlatformError.systemError({ + _tag: "PermissionDenied", + module: "FileSystem", + method: "open", + pathOrDescriptor: root, + }); + const unopenableDirectory = FileSystem.make({ + ...fs, + open: (openedPath, options) => + openedPath === root ? Effect.fail(openFailure) : fs.open(openedPath, options), + }); + + const exit = yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( + Effect.provideService(FileSystem.FileSystem, unopenableDirectory), + Effect.exit, ); - const failure = yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( - Effect.flip, - ); - - assert.strictEqual(failure.reason._tag, "PermissionDenied"); + assert.deepStrictEqual(exit, Exit.fail(openFailure)); assert.strictEqual(yield* fs.readFileString(filePath), "before"); }), ); From 2aa9cc7e913b73cd23824c231b706c31ca9424b7 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sat, 24 Oct 2026 03:12:55 +0000 Subject: [PATCH 6/9] test(shared): assert the atomic write attempts its temp directory cleanup once Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/atomicWrite.test.ts | 26 ++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index 2227aa22db076..c02595e26cdd6 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -308,17 +308,21 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { const path = yield* Path.Path; const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); const filePath = path.join(root, "settings.json"); + const removeAttempts: Array = []; const unremovable = FileSystem.make({ ...fs, remove: (removedPath) => - Effect.fail( - PlatformError.systemError({ - _tag: "PermissionDenied", - module: "FileSystem", - method: "remove", - pathOrDescriptor: removedPath, - }), - ), + Effect.suspend(() => { + removeAttempts.push(removedPath); + return Effect.fail( + PlatformError.systemError({ + _tag: "PermissionDenied", + module: "FileSystem", + method: "remove", + pathOrDescriptor: removedPath, + }), + ); + }), }); yield* writeFileStringAtomically({ filePath, contents: "after" }).pipe( @@ -326,7 +330,11 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { ); assert.strictEqual(yield* fs.readFileString(filePath), "after"); - assert.strictEqual((yield* fs.readDirectory(root)).length, 2); + assert.strictEqual(removeAttempts.length, 1); + assert.deepStrictEqual( + (yield* fs.readDirectory(root)).toSorted(), + [path.basename(removeAttempts[0]!), "settings.json"].toSorted(), + ); }), ); }); From 2b6c568321894edf27bc0de78d9c81d58b7dc962 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 11 Nov 2026 01:30:15 +0000 Subject: [PATCH 7/9] fix(shared): atomic saves write through one handle so a narrow umask still saves --- packages/shared/src/atomicWrite.test.ts | 23 +++++++++++++++++++++++ packages/shared/src/atomicWrite.ts | 6 +++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index 62ff3d2e9972f..a522644d26082 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -197,6 +197,29 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { }), ); + // Root ignores the missing read bit, so only a regular user sees the failure. + it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32" || process.getuid?.() === 0)( + "saves when the umask removes owner read access", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-atomic-write-" }); + const filePath = path.join(root, "settings.json"); + yield* fs.writeFileString(filePath, "before"); + yield* fs.chmod(filePath, 0o600); + yield* Effect.acquireRelease( + Effect.sync(() => process.umask(0o400)), + (previous) => Effect.sync(() => process.umask(previous)), + ); + + yield* writeFileStringAtomically({ filePath, contents: "after" }); + + assert.strictEqual(yield* fs.readFileString(filePath), "after"); + assert.strictEqual((yield* fs.stat(filePath)).mode & 0o777, 0o600); + }), + ); + it.effect("syncs the contents before the rename and the directory after it", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; diff --git a/packages/shared/src/atomicWrite.ts b/packages/shared/src/atomicWrite.ts index 16dc8e53f6e58..9d8f0d5f5e9b2 100644 --- a/packages/shared/src/atomicWrite.ts +++ b/packages/shared/src/atomicWrite.ts @@ -59,10 +59,10 @@ export const writeFileStringAtomically = (input: { ); const tempPath = path.join(tempDirectory, "contents.tmp"); - yield* fs.writeFileString(tempPath, input.contents); - // Open before the chmod can drop write access and close before the Windows rename. + // One write-only handle: a reopen fails when the umask or the chmod drops access. // Without the sync, a crash soon after the rename can leave the target empty. - yield* fs.open(tempPath, { flag: "r+" }).pipe( + yield* fs.open(tempPath, { flag: "w" }).pipe( + Effect.tap((file) => file.writeAll(new TextEncoder().encode(input.contents))), Effect.tap(() => platform === "win32" ? Effect.void From 0f3c1ae40a58e97e72fc37c4867d2b33e08c7d1d Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 11 Nov 2026 01:45:31 +0000 Subject: [PATCH 8/9] fix(shared): saves in a non-listable directory skip the directory sync instead of failing --- packages/shared/src/atomicWrite.test.ts | 6 +++--- packages/shared/src/atomicWrite.ts | 17 ++++++++++------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/shared/src/atomicWrite.test.ts b/packages/shared/src/atomicWrite.test.ts index a522644d26082..15a6c5d329526 100644 --- a/packages/shared/src/atomicWrite.test.ts +++ b/packages/shared/src/atomicWrite.test.ts @@ -320,7 +320,7 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { // Injected because root ignores a chmod that drops read access. it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( - "leaves the target intact when its directory cannot be opened for the sync", + "saves when its directory cannot be opened for the sync", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; @@ -345,8 +345,8 @@ it.layer(NodeServices.layer)("writeFileStringAtomically", (it) => { Effect.exit, ); - assert.deepStrictEqual(exit, Exit.fail(openFailure)); - assert.strictEqual(yield* fs.readFileString(filePath), "before"); + assert.deepStrictEqual(exit, Exit.void); + assert.strictEqual(yield* fs.readFileString(filePath), "after"); }), ); diff --git a/packages/shared/src/atomicWrite.ts b/packages/shared/src/atomicWrite.ts index 9d8f0d5f5e9b2..b8d286923d2ba 100644 --- a/packages/shared/src/atomicWrite.ts +++ b/packages/shared/src/atomicWrite.ts @@ -46,9 +46,6 @@ export const writeFileStringAtomically = (input: { const targetDirectory = path.dirname(targetPath); yield* fs.makeDirectory(targetDirectory, { recursive: true }); - // Opened up front so an unreadable directory fails the save before the target changes. - const directory = - platform === "win32" ? undefined : yield* fs.open(targetDirectory, { flag: "r" }); // Cleanup failure must not fail a write that already landed. const tempDirectory = yield* Effect.acquireRelease( fs.makeTempDirectory({ @@ -71,11 +68,17 @@ export const writeFileStringAtomically = (input: { Effect.flatMap((file) => file.sync), Effect.scoped, ); - if (directory !== undefined) { + if (platform !== "win32") { yield* fs.rename(tempPath, targetPath); - // Persists the rename; only filesystems that cannot sync directories are let through. - return yield* directory.sync.pipe( - Effect.catchIf(isUnsupportedSync, (error) => Effect.logWarning(error)), + // Persists the rename. A directory without read access cannot be opened to sync, and + // replacing in one needs only write and search access, so that save stands. + return yield* fs.open(targetDirectory, { flag: "r" }).pipe( + Effect.flatMap((directory) => directory.sync), + Effect.scoped, + Effect.catchIf( + (error) => error.reason._tag === "PermissionDenied" || isUnsupportedSync(error), + (error) => Effect.logWarning(error), + ), ); } From d5345dea3346274bd52dd3474f276ba69079614b Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 11 Nov 2026 09:52:37 +0000 Subject: [PATCH 9/9] test(server): tear the anonymous id write at the file handle atomic saves now use AI-assisted; verified with the tests and tools listed above. --- apps/server/src/telemetry/Identify.test.ts | 33 +++++++++++++--------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/apps/server/src/telemetry/Identify.test.ts b/apps/server/src/telemetry/Identify.test.ts index 6a0951875106f..b888e07641d81 100644 --- a/apps/server/src/telemetry/Identify.test.ts +++ b/apps/server/src/telemetry/Identify.test.ts @@ -105,21 +105,26 @@ it.layer(Layer.mergeAll(NodeServices.layer, NodeCrypto.layer))("telemetry identi const path = yield* Path.Path; const tornWrites = FileSystem.FileSystem.of({ ...fileSystem, - writeFileString: (filePath, data, options) => - fileSystem - .writeFileString(filePath, data.slice(0, Math.floor(data.length / 2)), options) - .pipe( - Effect.andThen( - Effect.fail( - PlatformError.systemError({ - _tag: "WriteZero", - module: "FileSystem", - method: "writeFileString", - pathOrDescriptor: filePath, - }), - ), - ), + open: (filePath, options) => + fileSystem.open(filePath, options).pipe( + Effect.map((file): FileSystem.File => + Object.assign(Object.create(file), { + writeAll: (buffer: Uint8Array) => + file.writeAll(buffer.subarray(0, Math.floor(buffer.length / 2))).pipe( + Effect.andThen( + Effect.fail( + PlatformError.systemError({ + _tag: "WriteZero", + module: "FileSystem", + method: "writeAll", + pathOrDescriptor: filePath, + }), + ), + ), + ), + }), ), + ), }); const identity = yield* Identify.getTelemetryIdentityForHome(