fix(filesystem): harden atomicReplaceFile fallback from PR #3296

[olaservo]

Context

PR #3296 adds an atomicReplaceFile() helper that falls back to fs.cp() + fs.unlink() when fs.rename() fails with EPERM on Windows (locked files). The fix is sound for the primary use case (VS Code), but review identified several hardening opportunities.

Ref: #3199, #3296

Follow-up Items

1. Make temp file cleanup best-effort inside atomicReplaceFile

If fs.cp() succeeds but fs.unlink(tempPath) fails (e.g., antivirus briefly locked the temp file), the error propagates even though the write succeeded. The user sees an error despite the file being written correctly.

Fix: Wrap fs.unlink(tempPath) in a try/catch inside the EPERM branch:

if ((error as NodeJS.ErrnoException).code === 'EPERM') {
  await fs.cp(tempPath, targetPath, { force: true });
  try {
    await fs.unlink(tempPath);
  } catch {
    // Best-effort cleanup; target was already written successfully
  }
}

2. Document FILE_SHARE_DELETE limitation and non-atomic fallback

fs.cp with force: true internally calls unlink(dest) before copyFile (Node.js source). This means the fallback only works when the locking process uses FILE_SHARE_DELETE (e.g., VS Code). Editors that lock without FILE_SHARE_DELETE will still see EPERM.

Also, the existing security comments at both call sites claim atomicity, but the fs.cp fallback is not atomic (TOCTOU window between unlink and copyFile). Risk is low in the EPERM context but should be documented.

Fix: Update JSDoc on atomicReplaceFile to note these limitations.

3. Add EPERM fallback test for applyFileEdits

PR #3296 adds EPERM tests only for writeFileContent. The applyFileEdits function also uses atomicReplaceFile but has no EPERM test coverage.

4. Consider renaming atomicReplaceFile

The function name is misleading since the fs.cp fallback is not atomic. replaceFile or replaceFileFromTemp would be more accurate.

Tests

Three tests are needed in src/filesystem/__tests__/lib.test.ts. These should be added after #3296 merges since they depend on atomicReplaceFile.

Test 1: EPERM fallback through applyFileEdits

Mirrors the existing writeFileContent EPERM test but exercises the applyFileEdits call site.

// In describe('applyFileEdits')
it('falls back to fs.cp when fs.rename fails with EPERM during file edit', async () => {
  const epermError = new Error('EPERM') as NodeJS.ErrnoException;
  epermError.code = 'EPERM';

  mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n');
  mockFs.writeFile.mockResolvedValue(undefined);
  mockFs.rename.mockRejectedValueOnce(epermError);
  mockFs.cp.mockResolvedValueOnce(undefined);
  mockFs.unlink.mockResolvedValueOnce(undefined);

  const edits = [{ oldText: 'line2', newText: 'modified line2' }];
  const result = await applyFileEdits('/test/file.txt', edits, false);

  // Should have tried rename first, then fallen back to cp + unlink
  expect(mockFs.rename).toHaveBeenCalledWith(
    expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
    '/test/file.txt'
  );
  expect(mockFs.cp).toHaveBeenCalledWith(
    expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
    '/test/file.txt',
    { force: true }
  );
  expect(mockFs.unlink).toHaveBeenCalledWith(
    expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/)
  );
  // Edit should still produce a valid diff
  expect(result).toContain('modified line2');
});

Test 2: Successful write despite temp cleanup failure (writeFileContent)

This test fails today and passes after the best-effort fix (item 1).

// In describe('writeFileContent')
it('succeeds when fs.cp works but temp file unlink fails', async () => {
  const eexistError = new Error('EEXIST') as NodeJS.ErrnoException;
  eexistError.code = 'EEXIST';
  const epermError = new Error('EPERM') as NodeJS.ErrnoException;
  epermError.code = 'EPERM';
  const unlinkError = new Error('EBUSY') as NodeJS.ErrnoException;
  unlinkError.code = 'EBUSY';

  mockFs.writeFile
    .mockRejectedValueOnce(eexistError)   // First write fails (file exists)
    .mockResolvedValueOnce(undefined);     // Temp file write succeeds
  mockFs.rename.mockRejectedValueOnce(epermError);  // Rename fails (locked)
  mockFs.cp.mockResolvedValueOnce(undefined);       // cp succeeds — file written!
  mockFs.unlink.mockRejectedValueOnce(unlinkError); // Temp cleanup fails (e.g. antivirus)

  // Should NOT throw — the target file was written successfully
  await expect(writeFileContent('/test/file.txt', 'new content'))
    .resolves.toBeUndefined();

  expect(mockFs.cp).toHaveBeenCalledWith(
    expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
    '/test/file.txt',
    { force: true }
  );
});

Test 3: Successful edit despite temp cleanup failure (applyFileEdits)

Same scenario through the other call site.

// In describe('applyFileEdits')
it('succeeds when fs.cp works but temp file unlink fails during file edit', async () => {
  const epermError = new Error('EPERM') as NodeJS.ErrnoException;
  epermError.code = 'EPERM';
  const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException;
  ebusyError.code = 'EBUSY';

  mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n');
  mockFs.writeFile.mockResolvedValue(undefined);
  mockFs.rename.mockRejectedValueOnce(epermError);   // Rename fails (locked)
  mockFs.cp.mockResolvedValueOnce(undefined);          // cp succeeds — file written!
  mockFs.unlink.mockRejectedValueOnce(ebusyError);    // Temp cleanup fails

  const edits = [{ oldText: 'line2', newText: 'modified line2' }];

  // Should NOT throw — the target file was written successfully
  const result = await applyFileEdits('/test/file.txt', edits, false);

  expect(result).toContain('modified line2');
  expect(mockFs.cp).toHaveBeenCalledWith(
    expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
    '/test/file.txt',
    { force: true }
  );
});

Acceptance Criteria

• Temp file cleanup is best-effort (no false errors on successful writes)
• JSDoc documents FILE_SHARE_DELETE requirement and non-atomic fallback
• applyFileEdits has EPERM fallback test coverage
• Function name accurately reflects behavior
• Tests 1-3 above pass