fix(core): make tool file writes atomic and serialize same-path writes

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<number>();
+    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']);
+  });
+});

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<ReturnType<typeof fs.stat>>);
+
+      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<ReturnType<typeof fs.stat>>);
+      // 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$/);
     });
   });
 });

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<void>;
 }
 
+/** 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<void> {
-    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<number | undefined> {
+    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<void> {
+    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));
+      }
+    }
   }
 }

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',
+      ]);
+    });
   });
 });

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<string> {
-    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<void> {