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..46f4c127e79 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -44,16 +44,70 @@ 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, options] = vi.mocked(fs.writeFile).mock + .calls[0] as [string, string, { encoding: string }]; + expect(content).toBe('Hello, World!'); + 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(); + + 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..7377dc08673 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,81 @@ 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`; + + // 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 { + // 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 } : {}), + }); + + if (existingMode !== undefined) { + 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); + } 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; +}