From f76c5d8f49e3238c7956def88e6a6a700ba41ad0 Mon Sep 17 00:00:00 2001 From: Ranjan G Date: Tue, 8 Sep 2026 00:25:46 +0530 Subject: [PATCH 1/2] fix(core): make tool file writes atomic and serialize same-path writes Parallel tool execution lets two file operations target the same path at once. Four consequences, each reproduced on main before fixing: - fs.writeFile truncates the destination and then streams it back in 512KB chunks, so a concurrent reader observes a partially written file. A 16MB write exposes dozens of truncated intermediate sizes. - Two parallel edits both read the original content, and whichever writes second discards the other's edit while both tool calls report success to the model. Editing "alpha" and "beta" in one file concurrently leaves "alpha\nBETA\n". - Two parallel write_file calls to the same new path both report creating it, so the second one's diff claims the file was empty beforehand. - Concurrent createFileSnapshot calls interleave as add, add, commit, commit. Since add('.') stages the whole worktree, one snapshot folds the other's files into its commit. StandardFileSystemService.writeTextFile now writes to a uniquely named sibling temp file and renames it into place, so an observer sees either the old file or the new one. It copies the destination's permissions onto the temp file first, so replacing a 0600 file does not widen it, and retries the rename on transient Windows EBUSY/EPERM. A new in-process per-path async mutex serializes the read-modify-write in EditTool and WriteFileTool, keyed on the already realpath-resolved target, and serializes stage/status/commit in GitService. Known limits, left for follow-up work: - The mutex is in-process only. It does not coordinate with a second Gemini CLI process or an external editor; that needs an on-disk lock. - SandboxedFileSystemService still writes through its __write sandbox command and is unchanged, so sandboxed writes are not yet atomic. Fixes #29078 --- .../services/fileSystemService.atomic.test.ts | 98 +++++++++++++++++++ .../src/services/fileSystemService.test.ts | 28 ++++-- .../core/src/services/fileSystemService.ts | 66 ++++++++++++- packages/core/src/services/gitService.test.ts | 32 ++++++ packages/core/src/services/gitService.ts | 38 ++++--- packages/core/src/tools/edit.test.ts | 35 +++++++ packages/core/src/tools/edit.ts | 16 +++ packages/core/src/tools/write-file.test.ts | 30 ++++++ packages/core/src/tools/write-file.ts | 13 +++ packages/core/src/utils/pathMutex.test.ts | 80 +++++++++++++++ packages/core/src/utils/pathMutex.ts | 65 ++++++++++++ 11 files changed, 478 insertions(+), 23 deletions(-) create mode 100644 packages/core/src/services/fileSystemService.atomic.test.ts create mode 100644 packages/core/src/utils/pathMutex.test.ts create mode 100644 packages/core/src/utils/pathMutex.ts diff --git a/packages/core/src/services/fileSystemService.atomic.test.ts b/packages/core/src/services/fileSystemService.atomic.test.ts new file mode 100644 index 00000000000..4e308aa35da --- /dev/null +++ b/packages/core/src/services/fileSystemService.atomic.test.ts @@ -0,0 +1,98 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import fs from 'node:fs'; +import fsp from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { StandardFileSystemService } from './fileSystemService.js'; + +/** + * These tests exercise the real filesystem on purpose: the behaviour under + * test is what a concurrent observer can see on disk while a write is in + * flight, which a mocked `fs` cannot express. + */ +describe('StandardFileSystemService atomicity', () => { + let dir: string; + let service: StandardFileSystemService; + + beforeEach(async () => { + dir = await fsp.mkdtemp(path.join(os.tmpdir(), 'gemini-atomic-write-')); + service = new StandardFileSystemService(); + }); + + afterEach(async () => { + await fsp.rm(dir, { recursive: true, force: true }); + }); + + it('never exposes a partially written file to a concurrent observer', async () => { + // Large enough that the underlying write is split into several chunks; + // old and new are the same length so any other size is a partial state. + const SIZE = 16 * 1024 * 1024; + const filePath = path.join(dir, 'large.txt'); + await fsp.writeFile(filePath, 'o'.repeat(SIZE), 'utf-8'); + + // The write runs on the libuv threadpool, so a *synchronous* loop on the + // main thread is what actually catches the destination mid-write. An + // async reader tends to be scheduled only before or after it. + const sizes = new Set(); + let settled = false; + const write = service + .writeTextFile(filePath, 'n'.repeat(SIZE)) + .finally(() => { + settled = true; + }); + + let spins = 0; + while (!settled && spins < 2_000_000) { + try { + sizes.add(fs.statSync(filePath).size); + } catch { + // The destination may briefly not exist while being replaced. + sizes.add(-1); + } + spins++; + if (spins % 200 === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + } + await write; + + // Guard against a vacuous pass where the observer never ran. + expect(sizes.size).toBeGreaterThan(0); + + const partialStates = [...sizes].filter((size) => size !== SIZE); + expect(partialStates).toEqual([]); + }); + + it('writes the requested content', async () => { + const filePath = path.join(dir, 'content.txt'); + + await service.writeTextFile(filePath, 'hello'); + + await expect(fsp.readFile(filePath, 'utf-8')).resolves.toBe('hello'); + }); + + it('preserves the permissions of an existing file', async () => { + const filePath = path.join(dir, 'secret.txt'); + await fsp.writeFile(filePath, 'before', { mode: 0o600 }); + await fsp.chmod(filePath, 0o600); + + await service.writeTextFile(filePath, 'after'); + + const stats = await fsp.stat(filePath); + expect(stats.mode & 0o777).toBe(0o600); + }); + + it('leaves no temporary files behind on success', async () => { + const filePath = path.join(dir, 'clean.txt'); + + await service.writeTextFile(filePath, 'done'); + + await expect(fsp.readdir(dir)).resolves.toEqual(['clean.txt']); + }); +}); diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 4ca5c3329ef..9eb571c7342 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -44,16 +44,32 @@ describe('StandardFileSystemService', () => { }); describe('writeTextFile', () => { - it('should write file content using fs', async () => { + it('should write to a sibling temp file and rename it into place', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); + vi.mocked(fs.rename).mockResolvedValue(); + vi.mocked(fs.stat).mockRejectedValue(new Error('ENOENT')); await fileSystem.writeTextFile('/test/file.txt', 'Hello, World!'); - expect(fs.writeFile).toHaveBeenCalledWith( - '/test/file.txt', - 'Hello, World!', - 'utf-8', - ); + const [tmpPath, content, encoding] = vi.mocked(fs.writeFile).mock + .calls[0] as [string, string, string]; + expect(content).toBe('Hello, World!'); + expect(encoding).toBe('utf-8'); + expect(tmpPath).toMatch(/^\/test\/file\.txt\..*\.tmp$/); + expect(fs.rename).toHaveBeenCalledWith(tmpPath, '/test/file.txt'); + }); + + it('should remove the temp file when the write fails', async () => { + vi.mocked(fs.writeFile).mockRejectedValue(new Error('ENOSPC')); + vi.mocked(fs.rm).mockResolvedValue(); + + await expect( + fileSystem.writeTextFile('/test/file.txt', 'Hello, World!'), + ).rejects.toThrow('ENOSPC'); + + expect(fs.rename).not.toHaveBeenCalled(); + const [removed] = vi.mocked(fs.rm).mock.calls[0] as [string]; + expect(removed).toMatch(/^\/test\/file\.txt\..*\.tmp$/); }); }); }); diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 946c227ab6a..05e6b19aa47 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -4,7 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { randomUUID } from 'node:crypto'; import fs from 'node:fs/promises'; +import { isNodeError } from '../utils/errors.js'; /** * Interface for file system operations that may be delegated to different implementations @@ -27,6 +29,9 @@ export interface FileSystemService { writeTextFile(filePath: string, content: string): Promise; } +/** Rename retries, for transient Windows lock errors. */ +const RENAME_MAX_RETRIES = 5; + /** * Standard file system implementation */ @@ -35,7 +40,66 @@ export class StandardFileSystemService implements FileSystemService { return fs.readFile(filePath, 'utf-8'); } + /** + * Writes `content` to `filePath` atomically. + * + * A plain `fs.writeFile` truncates the destination and then streams the + * content in chunks, so anything reading the file concurrently can observe + * a truncated prefix. Writing to a sibling temp file and renaming it into + * place means an observer sees either the old file or the new one. + */ async writeTextFile(filePath: string, content: string): Promise { - await fs.writeFile(filePath, content, 'utf-8'); + // The temp file must share a directory with the destination so that the + // rename stays within one filesystem, and must be uniquely named so that + // concurrent writers do not clobber each other's temp file. + const tmpPath = `${filePath}.${randomUUID()}.tmp`; + + try { + await fs.writeFile(tmpPath, content, 'utf-8'); + + // A fresh temp file does not inherit the destination's permissions, so + // copy them over before the rename. Without this, replacing a 0600 file + // would silently widen it to the default mode. + const existingMode = await this.getFileMode(filePath); + if (existingMode !== undefined) { + await fs.chmod(tmpPath, existingMode); + } + + await this.renameWithRetry(tmpPath, filePath); + } catch (error) { + await fs.rm(tmpPath, { force: true }).catch(() => { + // Best effort: the original error is the one worth reporting. + }); + throw error; + } + } + + private async getFileMode(filePath: string): Promise { + try { + const stats = await fs.stat(filePath); + return stats.mode & 0o777; + } catch { + // New file, or a destination we cannot stat; keep the default mode. + return undefined; + } + } + + private async renameWithRetry(from: string, to: string): Promise { + for (let attempt = 0; attempt < RENAME_MAX_RETRIES; attempt++) { + try { + await fs.rename(from, to); + return; + } catch (error: unknown) { + // Windows can transiently refuse a rename while another process has + // the destination open (antivirus, editors, watchers). + const code = isNodeError(error) ? error.code : ''; + const isRetryable = code === 'EBUSY' || code === 'EPERM'; + if (!isRetryable || attempt === RENAME_MAX_RETRIES - 1) { + throw error; + } + const delayMs = Math.pow(2, attempt) * 50; + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } + } } } diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index 0c355cd4812..7ab2faf4a73 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -490,5 +490,37 @@ describe('GitService', () => { expect(hoistedMockRaw).toHaveBeenCalledWith('rev-parse', 'HEAD'); expect(commitHash).toBe('current-head-hash'); }); + + it('does not interleave staging and committing across concurrent snapshots', async () => { + const events: string[] = []; + hoistedMockAdd.mockImplementation(async () => { + events.push('add:start'); + await new Promise((resolve) => setTimeout(resolve, 5)); + events.push('add:end'); + }); + hoistedMockStatus.mockResolvedValue({ isClean: () => false }); + hoistedMockCommit.mockImplementation(async (message: string) => { + events.push(`commit:${message}`); + return { commit: `hash-${message}` }; + }); + + const service = new GitService(projectRoot, storage); + await Promise.all([ + service.createFileSnapshot('A'), + service.createFileSnapshot('B'), + ]); + + // `add('.')` stages the whole working tree, so a second snapshot that + // stages while the first has not committed yet folds the first + // snapshot's files into its own commit. + expect(events).toEqual([ + 'add:start', + 'add:end', + 'commit:A', + 'add:start', + 'add:end', + 'commit:B', + ]); + }); }); }); diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index 3d9fb284fdb..0bb79b7a97c 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -7,6 +7,7 @@ import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import { isNodeError } from '../utils/errors.js'; +import { withPathLock } from '../utils/pathMutex.js'; import { spawnAsync } from '../utils/shell-utils.js'; import { simpleGit, @@ -194,23 +195,28 @@ export class GitService { } async createFileSnapshot(message: string): Promise { - try { - const repo = this.shadowGitRepository; - await repo.add('.'); - const status = await repo.status(); - if (status.isClean()) { - // If no changes are staged, return the current HEAD commit hash - return await this.getCurrentCommitHash(); + // `add('.')` stages the entire working tree, so two snapshots running at + // once fold each other's files into whichever commit lands first. Serialize + // stage -> status -> commit per shadow repository. + return withPathLock(`git-snapshot:${this.projectRoot}`, async () => { + try { + const repo = this.shadowGitRepository; + await repo.add('.'); + const status = await repo.status(); + if (status.isClean()) { + // If no changes are staged, return the current HEAD commit hash + return await this.getCurrentCommitHash(); + } + const commitResult = await repo.commit(message, { + '--no-verify': null, + }); + return commitResult.commit; + } catch (error) { + throw new Error( + `Failed to create checkpoint snapshot: ${error instanceof Error ? error.message : 'Unknown error'}. Checkpointing may not be working properly.`, + ); } - const commitResult = await repo.commit(message, { - '--no-verify': null, - }); - return commitResult.commit; - } catch (error) { - throw new Error( - `Failed to create checkpoint snapshot: ${error instanceof Error ? error.message : 'Unknown error'}. Checkpointing may not be working properly.`, - ); - } + }); } async restoreProjectFromSnapshot(commitHash: string): Promise { diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 90bcfc6a6ba..f67ae05cbe4 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1496,4 +1496,39 @@ function doIt() { fs.rmSync(plansDir, { recursive: true, force: true }); }); }); + + describe('concurrent edits to the same file', () => { + it('applies both edits rather than losing one', async () => { + const filePath = path.join(rootDir, 'shared.txt'); + fs.writeFileSync(filePath, 'alpha\nbeta\n', 'utf8'); + + const first = tool.build({ + file_path: filePath, + instruction: 'Uppercase alpha', + old_string: 'alpha', + new_string: 'ALPHA', + }); + const second = tool.build({ + file_path: filePath, + instruction: 'Uppercase beta', + old_string: 'beta', + new_string: 'BETA', + }); + + const signal = new AbortController().signal; + const results = await Promise.all([ + first.execute({ abortSignal: signal }), + second.execute({ abortSignal: signal }), + ]); + + for (const result of results) { + expect(result.error).toBeUndefined(); + } + + // Both tool calls reported success, so neither edit may be missing. + const finalContent = fs.readFileSync(filePath, 'utf8'); + expect(finalContent).toContain('ALPHA'); + expect(finalContent).toContain('BETA'); + }); + }); }); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 9f5a735c10b..87699d1685b 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -34,6 +34,7 @@ import { resolveToRealPath, } from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; +import { withPathLock } from '../utils/pathMutex.js'; import { correctPath } from '../utils/pathCorrector.js'; import type { Config } from '../config/config.js'; import { CoreToolCallStatus } from '../scheduler/types.js'; @@ -911,6 +912,21 @@ class EditToolInvocation }; } + // Serialize the whole read-modify-write against other writers of this + // path. Two edits scheduled in parallel (common with sub-agents) would + // otherwise both read the original content, and whichever wrote second + // would silently discard the other's edit while still reporting success. + return withPathLock(this.resolvedPath, () => this.applyEdit(signal)); + } + + /** + * Computes and applies the edit. + * + * Must be called while holding the path lock for `this.resolvedPath`, so + * that the read in `calculateEdit` and the subsequent write cannot be + * interleaved with another writer of the same file. + */ + private async applyEdit(signal: AbortSignal): Promise { let editData: CalculatedEdit; try { editData = await this.calculateEdit(this.params, signal); diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 7e2c3f86c8e..54993d8a30e 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -944,6 +944,36 @@ describe('WriteFileTool', () => { }); }); + describe('concurrent writes to the same file', () => { + it('does not report two creations of the same new file', async () => { + const abortSignal = new AbortController().signal; + const filePath = path.join(rootDir, 'concurrent_new_file.txt'); + mockEnsureCorrectFileContent.mockImplementation( + async (content: string) => content, + ); + + const first = tool.build({ file_path: filePath, content: 'first' }); + const second = tool.build({ file_path: filePath, content: 'second' }); + + const results = await Promise.all([ + first.execute({ abortSignal }), + second.execute({ abortSignal }), + ]); + + // Whichever call lands second must observe the file the other created, + // otherwise both report a creation and the second one's diff claims the + // file was empty beforehand. + const created = results.filter((r) => + /Successfully created and wrote to new file/.test(String(r.llmContent)), + ); + const overwrote = results.filter((r) => + /Successfully overwrote file/.test(String(r.llmContent)), + ); + expect(created).toHaveLength(1); + expect(overwrote).toHaveLength(1); + }); + }); + describe('workspace boundary validation', () => { it('should validate paths are within workspace root', () => { const params = { diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 92e005b3bb7..08127b42d5b 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -62,6 +62,7 @@ import { resolveModel, } from '../config/models.js'; import { discoverJitContext, appendJitContext } from './jit-context.js'; +import { withPathLock } from '../utils/pathMutex.js'; /** * Parameters for the WriteFile tool @@ -379,6 +380,18 @@ class WriteFileToolInvocation extends BaseToolInvocation< }; } + // Serialize against other writers of this path, so that the existence + // check and content read that produce the diff cannot be interleaved with + // another write to the same file. + return withPathLock(this.resolvedPath, () => this.applyWrite(abortSignal)); + } + + /** + * Writes the file. + * + * Must be called while holding the path lock for `this.resolvedPath`. + */ + private async applyWrite(abortSignal: AbortSignal): Promise { const { content, ai_proposed_content, modified_by_user } = this.params; const correctedContentResult = await getCorrectedFileContent( this.config, diff --git a/packages/core/src/utils/pathMutex.test.ts b/packages/core/src/utils/pathMutex.test.ts new file mode 100644 index 00000000000..c7106025cbb --- /dev/null +++ b/packages/core/src/utils/pathMutex.test.ts @@ -0,0 +1,80 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { pendingPathLockCount, withPathLock } from './pathMutex.js'; + +describe('withPathLock', () => { + it('serializes callbacks that target the same key', async () => { + const events: string[] = []; + + const first = withPathLock('/tmp/same.txt', async () => { + events.push('first:enter'); + await new Promise((resolve) => setTimeout(resolve, 10)); + events.push('first:exit'); + }); + + const second = withPathLock('/tmp/same.txt', async () => { + events.push('second:enter'); + await new Promise((resolve) => setTimeout(resolve, 0)); + events.push('second:exit'); + }); + + await Promise.all([first, second]); + + expect(events).toEqual([ + 'first:enter', + 'first:exit', + 'second:enter', + 'second:exit', + ]); + }); + + it('runs callbacks for different keys concurrently', async () => { + let bothEntered = false; + let aEntered = false; + let bEntered = false; + + const a = withPathLock('/tmp/a.txt', async () => { + aEntered = true; + await new Promise((resolve) => setTimeout(resolve, 10)); + if (bEntered) bothEntered = true; + }); + + const b = withPathLock('/tmp/b.txt', async () => { + bEntered = true; + await new Promise((resolve) => setTimeout(resolve, 10)); + if (aEntered) bothEntered = true; + }); + + await Promise.all([a, b]); + + expect(bothEntered).toBe(true); + }); + + it('returns the value produced by the callback', async () => { + await expect(withPathLock('/tmp/value.txt', async () => 42)).resolves.toBe( + 42, + ); + }); + + it('releases the lock when a callback rejects', async () => { + await expect( + withPathLock('/tmp/throws.txt', async () => { + throw new Error('boom'); + }), + ).rejects.toThrow('boom'); + + await expect( + withPathLock('/tmp/throws.txt', async () => 'recovered'), + ).resolves.toBe('recovered'); + }); + + it('does not retain lock state for keys that are no longer in use', async () => { + await withPathLock('/tmp/transient.txt', async () => undefined); + expect(pendingPathLockCount()).toBe(0); + }); +}); diff --git a/packages/core/src/utils/pathMutex.ts b/packages/core/src/utils/pathMutex.ts new file mode 100644 index 00000000000..5781f830607 --- /dev/null +++ b/packages/core/src/utils/pathMutex.ts @@ -0,0 +1,65 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * In-process, per-path mutex used to serialize read-modify-write sequences + * against the same file. + * + * Parallel tool execution (notably sub-agents) can schedule two writes to the + * same path concurrently. Without serialization the two sequences interleave + * and one update is silently lost. + * + * Scope: this coordinates callers inside a single process only. It does not + * guard against a second Gemini CLI process, an editor, or any other program + * writing the same file; that would require an on-disk lock. + * + * Callers are expected to pass an already-resolved absolute path so that two + * spellings of the same file map to the same lock. + */ +const chains = new Map>(); + +/** + * Runs `fn` with exclusive access to `key`, relative to other `withPathLock` + * callers using the same key. + * + * @param key - The resolved path (or other identifier) to lock. + * @param fn - The critical section. + * @returns Whatever `fn` resolves to. + */ +export async function withPathLock( + key: string, + fn: () => Promise, +): Promise { + // The stored chain is deliberately non-rejecting (see below), so waiting for + // our turn cannot fail just because the previous lock holder threw. + const previous = chains.get(key) ?? Promise.resolve(); + const run = previous.then(fn); + + // Store a settled-either-way view of our run, so a throwing critical section + // neither blocks the next waiter nor raises an unhandled rejection. + const chained = run.then( + () => undefined, + () => undefined, + ); + chains.set(key, chained); + + try { + return await run; + } finally { + // Drop the entry once we are the last waiter, so the map does not grow + // without bound across a long session. + if (chains.get(key) === chained) { + chains.delete(key); + } + } +} + +/** + * Number of paths currently holding lock state. Exposed for tests. + */ +export function pendingPathLockCount(): number { + return chains.size; +} From 44ae691369d32d698e9c3a83b26887f61a08b947 Mon Sep 17 00:00:00 2001 From: Ranjan G Date: Tue, 8 Sep 2026 22:58:52 +0530 Subject: [PATCH 2/2] fix(core): make permission preservation best-effort in atomic write Review feedback on #29244: fs.chmod sat inside the try block whose catch removes the temp file and rethrows, so on filesystems that reject chmod (FAT32, exFAT, some NFS/CIFS mounts) or in restricted sandboxes raising EPERM/ENOTSUP, the whole write failed and the content was lost. The previous in-place fs.writeFile never called chmod, so that was a regression introduced by the atomic write. The chmod is now best-effort. The destination's mode is also passed when the temp file is created, which keeps the mode correct in the common case and closes the window where a replacement for a 0600 file was briefly readable through the default mode. Creation mode is masked by umask so it can only be more restrictive; the chmod that follows corrects that when it is permitted. Two regression tests: the temp file is created with the destination's mode, and a rejecting chmod does not fail the write or delete the temp file. --- .../src/services/fileSystemService.test.ts | 44 +++++++++++++++++-- .../core/src/services/fileSystemService.ts | 27 +++++++++--- 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 9eb571c7342..46f4c127e79 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -51,14 +51,52 @@ describe('StandardFileSystemService', () => { await fileSystem.writeTextFile('/test/file.txt', 'Hello, World!'); - const [tmpPath, content, encoding] = vi.mocked(fs.writeFile).mock - .calls[0] as [string, string, string]; + const [tmpPath, content, options] = vi.mocked(fs.writeFile).mock + .calls[0] as [string, string, { encoding: string }]; expect(content).toBe('Hello, World!'); - expect(encoding).toBe('utf-8'); + expect(options.encoding).toBe('utf-8'); expect(tmpPath).toMatch(/^\/test\/file\.txt\..*\.tmp$/); expect(fs.rename).toHaveBeenCalledWith(tmpPath, '/test/file.txt'); }); + it('should create the temp file with the destination permissions', async () => { + vi.mocked(fs.writeFile).mockResolvedValue(); + vi.mocked(fs.rename).mockResolvedValue(); + vi.mocked(fs.chmod).mockResolvedValue(); + vi.mocked(fs.stat).mockResolvedValue({ + mode: 0o600, + } as unknown as Awaited>); + + await fileSystem.writeTextFile('/test/secret.txt', 'Hello, World!'); + + // Creating the temp file already restricted means the content is never + // briefly readable through a wider default mode. + const [, , options] = vi.mocked(fs.writeFile).mock.calls[0]; + expect(options).toEqual({ encoding: 'utf-8', mode: 0o600 }); + }); + + it('should still write the file when chmod is not permitted', async () => { + vi.mocked(fs.writeFile).mockResolvedValue(); + vi.mocked(fs.rename).mockResolvedValue(); + vi.mocked(fs.rm).mockResolvedValue(); + vi.mocked(fs.stat).mockResolvedValue({ + mode: 0o600, + } as unknown as Awaited>); + // FAT32/exFAT, some NFS/CIFS mounts and restricted sandboxes reject chmod. + vi.mocked(fs.chmod).mockRejectedValue( + Object.assign(new Error('operation not supported'), { + code: 'ENOTSUP', + }), + ); + + await expect( + fileSystem.writeTextFile('/test/file.txt', 'Hello, World!'), + ).resolves.toBeUndefined(); + + expect(fs.rename).toHaveBeenCalled(); + expect(fs.rm).not.toHaveBeenCalled(); + }); + it('should remove the temp file when the write fails', async () => { vi.mocked(fs.writeFile).mockRejectedValue(new Error('ENOSPC')); vi.mocked(fs.rm).mockResolvedValue(); diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 05e6b19aa47..7377dc08673 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -54,15 +54,30 @@ export class StandardFileSystemService implements FileSystemService { // concurrent writers do not clobber each other's temp file. const tmpPath = `${filePath}.${randomUUID()}.tmp`; + // A fresh temp file does not inherit the destination's permissions, so + // without this, replacing a 0600 file would silently widen it to the + // default mode. + const existingMode = await this.getFileMode(filePath); + try { - await fs.writeFile(tmpPath, content, 'utf-8'); + // Create the temp file already carrying the destination's mode, so the + // content is never briefly readable through a wider default mode. The + // mode is masked by umask, so this can only be more restrictive. + await fs.writeFile(tmpPath, content, { + encoding: 'utf-8', + ...(existingMode !== undefined ? { mode: existingMode } : {}), + }); - // A fresh temp file does not inherit the destination's permissions, so - // copy them over before the rename. Without this, replacing a 0600 file - // would silently widen it to the default mode. - const existingMode = await this.getFileMode(filePath); if (existingMode !== undefined) { - await fs.chmod(tmpPath, existingMode); + try { + // Correct any narrowing that umask applied above. Best effort: some + // filesystems (FAT32, exFAT, a few NFS/CIFS mounts) and restricted + // sandboxes reject chmod with EPERM/ENOTSUP, and permissions must + // not be the reason a write fails. + await fs.chmod(tmpPath, existingMode); + } catch { + // Keep whatever mode the temp file was created with. + } } await this.renameWithRetry(tmpPath, filePath);