fix(policy): add EBUSY fallback and TOML parse recovery (#19919) (#21541)

Signed-off-by: krishdef7 <gargkrish06@gmail.com>
Co-authored-by: Sikandar <ma5161310@gmail.com>
Co-authored-by: Tommaso Sciortino <sciortino@gmail.com>

packages/core/src/policy/config.ts

@@ -9,6 +9,7 @@ import * as path from 'node:path';
 import * as crypto from 'node:crypto';
 import { fileURLToPath } from 'node:url';
 import { Storage } from '../config/storage.js';
+import { debugLogger } from '../utils/debugLogger.js';
 import {
   ApprovalMode,
   type PolicyEngineConfig,
@@ -28,7 +29,6 @@ import {
 } from '../confirmation-bus/types.js';
 import { type MessageBus } from '../confirmation-bus/message-bus.js';
 import { coreEvents } from '../utils/events.js';
-import { debugLogger } from '../utils/debugLogger.js';
 import { SHELL_TOOL_NAMES } from '../utils/shell-utils.js';
 import {
   SHELL_TOOL_NAME,
@@ -794,6 +794,7 @@ export function createPolicyUpdater(
 
       if (message.persist) {
         persistenceQueue = persistenceQueue.then(async () => {
+          let tmpFile: string | undefined;
           try {
             const policyFile =
               message.persistScope === 'workspace'
@@ -814,11 +815,27 @@ export function createPolicyUpdater(
                 existingData = parsed as { rule?: TomlRule[] };
               }
             } catch (error) {
-              if (!isNodeError(error) || error.code !== 'ENOENT') {
-                debugLogger.warn(
-                  `Failed to parse ${policyFile}, overwriting with new policy.`,
-                  error,
+              if (isNodeError(error) && error.code === 'ENOENT') {
+                // File doesn't exist yet, start fresh
+              } else if (!isNodeError(error)) {
+                // TOML parse error — back up corrupted file and recover
+                coreEvents.emitFeedback(
+                  'warning',
+                  `Syntax error found in policy file. Backing up corrupted file to ${policyFile}.bak and starting fresh.`,
                 );
+                if (
+                  !(
+                    await fs.lstat(policyFile).catch(() => null)
+                  )?.isSymbolicLink()
+                ) {
+                  await fs
+                    .copyFile(policyFile, `${policyFile}.bak`)
+                    .catch(() => {});
+                }
+                existingData = {};
+              } else {
+                // Real filesystem error (e.g. EACCES) — throw to prevent silent failure
+                throw error;
               }
             }
 
@@ -866,7 +883,7 @@ export function createPolicyUpdater(
             // Using a unique suffix avoids race conditions where concurrent processes
             // overwrite each other's temporary files, leading to ENOENT errors on rename.
             const tmpSuffix = crypto.randomBytes(8).toString('hex');
-            const tmpFile = `${policyFile}.${tmpSuffix}.tmp`;
+            tmpFile = `${policyFile}.${tmpSuffix}.tmp`;
 
             let handle: fs.FileHandle | undefined;
             try {
@@ -876,11 +893,37 @@ export function createPolicyUpdater(
             } finally {
               await handle?.close();
             }
-            await fs.rename(tmpFile, policyFile);
+            try {
+              await fs.rename(tmpFile, policyFile);
+            } catch (renameError) {
+              // Cross-device rename fails with EXDEV on some Linux mount configurations.
+              // Fall back to copy + unlink which works across filesystems.
+              if (
+                isNodeError(renameError) &&
+                (renameError.code === 'EXDEV' || renameError.code === 'EBUSY')
+              ) {
+                if (
+                  (
+                    await fs.lstat(policyFile).catch(() => null)
+                  )?.isSymbolicLink()
+                )
+                  throw renameError;
+                await fs.copyFile(tmpFile, policyFile);
+                await fs.unlink(tmpFile).catch(() => {});
+              } else {
+                throw renameError;
+              }
+            }
           } catch (error) {
+            // Clean up orphaned tmp file if it was created
+            if (tmpFile) {
+              await fs.unlink(tmpFile).catch(() => {});
+            }
+            const reason =
+              error instanceof Error ? error.message : String(error);
             coreEvents.emitFeedback(
               'error',
-              `Failed to persist policy for ${toolName}`,
+              `Failed to persist policy for ${toolName}: ${reason}`,
               error,
             );
           }

packages/core/src/policy/persistence.test.ts

@@ -5,6 +5,7 @@
  */
 
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import * as fs from 'node:fs/promises';
 import * as path from 'node:path';
 import {
   createPolicyUpdater,
@@ -16,10 +17,20 @@ import { MessageBusType } from '../confirmation-bus/types.js';
 import { Storage, AUTO_SAVED_POLICY_FILENAME } from '../config/storage.js';
 import { ApprovalMode } from './types.js';
 import { vol, fs as memfs } from 'memfs';
+import { coreEvents } from '../utils/events.js';
 
 // Use memfs for all fs operations in this test
 vi.mock('node:fs/promises', () => import('memfs').then((m) => m.fs.promises));
 
+/**
+ * Creates a Node.js-style error with a `code` property.
+ */
+function makeNodeError(message: string, code: string): NodeJS.ErrnoException {
+  const err = new Error(message) as NodeJS.ErrnoException;
+  err.code = code;
+  return err;
+}
+
 vi.mock('../config/storage.js');
 
 describe('createPolicyUpdater', () => {
@@ -57,8 +68,6 @@ describe('createPolicyUpdater', () => {
       persist: true,
     });
 
-    // Policy updater handles persistence asynchronously in a promise queue.
-    // We use advanceTimersByTimeAsync to yield to the microtask queue.
     await vi.advanceTimersByTimeAsync(100);
 
     const fileExists = memfs.existsSync(policyFile);
@@ -243,6 +252,147 @@ decision = "deny"
     expect(content).toContain('toolName = "test_tool"');
   });
 
+  it('should include error details in feedback message on persistence failure', async () => {
+    createPolicyUpdater(policyEngine, messageBus, mockStorage);
+
+    const workspacePoliciesDir = '/mock/project/.gemini/policies';
+    const policyFile = path.join(
+      workspacePoliciesDir,
+      AUTO_SAVED_POLICY_FILENAME,
+    );
+    vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue(
+      workspacePoliciesDir,
+    );
+    vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
+    vi.spyOn(fs, 'mkdir').mockRejectedValue(new Error('Permission denied'));
+
+    const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback');
+
+    await messageBus.publish({
+      type: MessageBusType.UPDATE_POLICY,
+      toolName: 'test_tool',
+      persist: true,
+    });
+
+    await vi.runAllTimersAsync();
+    expect(feedbackSpy).toHaveBeenCalledWith(
+      'error',
+      expect.stringContaining('Permission denied'),
+      expect.any(Error),
+    );
+  });
+
+  it('should clean up tmp file on write failure', async () => {
+    createPolicyUpdater(policyEngine, messageBus, mockStorage);
+
+    const workspacePoliciesDir = '/mock/project/.gemini/policies';
+    const policyFile = path.join(
+      workspacePoliciesDir,
+      AUTO_SAVED_POLICY_FILENAME,
+    );
+    vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue(
+      workspacePoliciesDir,
+    );
+    vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
+    vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined as never);
+    vi.spyOn(fs, 'readFile').mockRejectedValue(
+      makeNodeError('ENOENT: no such file or directory', 'ENOENT'),
+    );
+
+    const mockFileHandle = {
+      writeFile: vi.fn().mockRejectedValue(new Error('Disk full')),
+      close: vi.fn().mockResolvedValue(undefined),
+    };
+    vi.spyOn(fs, 'open').mockResolvedValue(mockFileHandle as never);
+    vi.spyOn(fs, 'unlink').mockResolvedValue(undefined as never);
+
+    await messageBus.publish({
+      type: MessageBusType.UPDATE_POLICY,
+      toolName: 'test_tool',
+      persist: true,
+    });
+
+    await vi.runAllTimersAsync();
+    expect(fs.unlink).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/));
+  });
+
+  it('should abort persistence on non-ENOENT read errors', async () => {
+    createPolicyUpdater(policyEngine, messageBus, mockStorage);
+
+    const workspacePoliciesDir = '/mock/project/.gemini/policies';
+    const policyFile = path.join(
+      workspacePoliciesDir,
+      AUTO_SAVED_POLICY_FILENAME,
+    );
+    vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue(
+      workspacePoliciesDir,
+    );
+    vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
+    vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined as never);
+    vi.spyOn(fs, 'readFile').mockRejectedValue(
+      makeNodeError('Permission denied', 'EACCES'),
+    );
+    const openSpy = vi.spyOn(fs, 'open');
+
+    const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback');
+
+    await messageBus.publish({
+      type: MessageBusType.UPDATE_POLICY,
+      toolName: 'test_tool',
+      persist: true,
+    });
+
+    await vi.runAllTimersAsync();
+    expect(openSpy).not.toHaveBeenCalled();
+    expect(feedbackSpy).toHaveBeenCalledWith(
+      'error',
+      expect.stringContaining('Permission denied'),
+      expect.any(Error),
+    );
+  });
+
+  it('should fall back to copy+unlink when rename fails with EXDEV', async () => {
+    createPolicyUpdater(policyEngine, messageBus, mockStorage);
+
+    const workspacePoliciesDir = '/mock/project/.gemini/policies';
+    const policyFile = path.join(
+      workspacePoliciesDir,
+      AUTO_SAVED_POLICY_FILENAME,
+    );
+    vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue(
+      workspacePoliciesDir,
+    );
+    vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
+    vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined as never);
+    vi.spyOn(fs, 'readFile').mockRejectedValue(
+      makeNodeError('ENOENT: no such file or directory', 'ENOENT'),
+    );
+
+    const mockFileHandle = {
+      writeFile: vi.fn().mockResolvedValue(undefined),
+      close: vi.fn().mockResolvedValue(undefined),
+    };
+    vi.spyOn(fs, 'open').mockResolvedValue(mockFileHandle as never);
+    vi.spyOn(fs, 'rename').mockRejectedValue(
+      makeNodeError('EXDEV: cross-device link not permitted', 'EXDEV'),
+    );
+    vi.spyOn(fs, 'copyFile').mockResolvedValue(undefined as never);
+    vi.spyOn(fs, 'unlink').mockResolvedValue(undefined as never);
+
+    await messageBus.publish({
+      type: MessageBusType.UPDATE_POLICY,
+      toolName: 'test_tool',
+      persist: true,
+    });
+
+    await vi.runAllTimersAsync();
+    expect(fs.copyFile).toHaveBeenCalledWith(
+      expect.stringMatching(/\.tmp$/),
+      policyFile,
+    );
+    expect(fs.unlink).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/));
+  });
+
   it('should include modes if provided', async () => {
     createPolicyUpdater(policyEngine, messageBus, mockStorage);
 
@@ -295,4 +445,78 @@ modes = [ "autoEdit", "yolo" ]
     expect(ruleCount).toBe(1);
     expect(content).toContain('modes = [ "default", "autoEdit", "yolo" ]');
   });
+
+  it('should fall back to copy+unlink when rename fails with EBUSY', async () => {
+    createPolicyUpdater(policyEngine, messageBus, mockStorage);
+
+    const workspacePoliciesDir = '/mock/project/.gemini/policies';
+    const policyFile = path.join(
+      workspacePoliciesDir,
+      AUTO_SAVED_POLICY_FILENAME,
+    );
+    vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue(
+      workspacePoliciesDir,
+    );
+    vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
+    vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined);
+    vi.spyOn(fs, 'readFile').mockRejectedValue(
+      makeNodeError('ENOENT: no such file or directory', 'ENOENT'),
+    );
+
+    const mockFileHandle = {
+      writeFile: vi.fn().mockResolvedValue(undefined),
+      close: vi.fn().mockResolvedValue(undefined),
+    };
+    vi.spyOn(fs, 'open').mockResolvedValue(
+      mockFileHandle as unknown as fs.FileHandle,
+    );
+    vi.spyOn(fs, 'rename').mockRejectedValue(
+      makeNodeError('EBUSY: resource busy or locked', 'EBUSY'),
+    );
+    vi.spyOn(fs, 'copyFile').mockResolvedValue(undefined);
+    vi.spyOn(fs, 'unlink').mockResolvedValue(undefined);
+
+    await messageBus.publish({
+      type: MessageBusType.UPDATE_POLICY,
+      toolName: 'test_tool',
+      persist: true,
+    });
+
+    await vi.runAllTimersAsync();
+    expect(fs.copyFile).toHaveBeenCalledWith(
+      expect.stringMatching(/\.tmp$/),
+      policyFile,
+    );
+    expect(fs.unlink).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/));
+  });
+
+  it('should back up corrupted TOML file and recover', async () => {
+    createPolicyUpdater(policyEngine, messageBus, mockStorage);
+
+    const policyFile = '/mock/user/.gemini/policies/auto-saved.toml';
+    vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
+
+    const dir = path.dirname(policyFile);
+    memfs.mkdirSync(dir, { recursive: true });
+    memfs.writeFileSync(policyFile, 'this is not valid toml ][[[');
+
+    const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback');
+
+    await messageBus.publish({
+      type: MessageBusType.UPDATE_POLICY,
+      toolName: 'test_tool',
+      persist: true,
+    });
+
+    await vi.advanceTimersByTimeAsync(100);
+
+    expect(feedbackSpy).toHaveBeenCalledWith(
+      'warning',
+      expect.stringContaining('.bak'),
+    );
+
+    expect(memfs.existsSync(policyFile)).toBe(true);
+    const content = memfs.readFileSync(policyFile, 'utf-8') as string;
+    expect(content).toContain('toolName = "test_tool"');
+  });
 });

packages/core/src/policy/policy-updater.test.ts

@@ -121,7 +121,11 @@ describe('createPolicyUpdater', () => {
 
   it('should persist mcpName to TOML', async () => {
     createPolicyUpdater(policyEngine, messageBus, mockStorage);
-    vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' });
+    vi.mocked(fs.readFile).mockRejectedValue(
+      Object.assign(new Error('ENOENT: no such file or directory'), {
+        code: 'ENOENT',
+      }),
+    );
     vi.mocked(fs.mkdir).mockResolvedValue(undefined);
 
     const mockFileHandle = {
@@ -142,9 +146,9 @@ describe('createPolicyUpdater', () => {
     });
 
     // Wait for the async listener to complete
-    await new Promise((resolve) => setTimeout(resolve, 0));
-
-    expect(fs.open).toHaveBeenCalled();
+    await vi.waitFor(() => {
+      expect(fs.open).toHaveBeenCalled();
+    });
     const [content] = mockFileHandle.writeFile.mock.calls[0] as [
       string,
       string,
@@ -199,7 +203,11 @@ describe('createPolicyUpdater', () => {
 
   it('should persist multiple rules correctly to TOML', async () => {
     createPolicyUpdater(policyEngine, messageBus, mockStorage);
-    vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' });
+    const enoentError = Object.assign(
+      new Error('ENOENT: no such file or directory'),
+      { code: 'ENOENT' },
+    );
+    vi.mocked(fs.readFile).mockRejectedValue(enoentError);
     vi.mocked(fs.mkdir).mockResolvedValue(undefined);
 
     const mockFileHandle = {
@@ -219,17 +227,17 @@ describe('createPolicyUpdater', () => {
     });
 
     // Wait for the async listener to complete
-    await new Promise((resolve) => setTimeout(resolve, 0));
+    await vi.waitFor(() => {
+      expect(fs.open).toHaveBeenCalled();
+      const [content] = mockFileHandle.writeFile.mock.calls[0] as [
+        string,
+        string,
+      ];
+      const parsed = toml.parse(content) as unknown as ParsedPolicy;
 
-    expect(fs.open).toHaveBeenCalled();
-    const [content] = mockFileHandle.writeFile.mock.calls[0] as [
-      string,
-      string,
-    ];
-    const parsed = toml.parse(content) as unknown as ParsedPolicy;
-
-    expect(parsed.rule).toHaveLength(1);
-    expect(parsed.rule![0].commandPrefix).toEqual(['echo', 'ls']);
+      expect(parsed.rule).toHaveLength(1);
+      expect(parsed.rule![0].commandPrefix).toEqual(['echo', 'ls']);
+    });
   });
 
   it('should reject unsafe regex patterns', async () => {