fix(filesystem): harden file replacement with EPERM fallback

[Chelebii]

Problem

When \s.rename\ fails with \EPERM\ on Windows (e.g. VS Code or antivirus holding a file lock), both \writeFileContent\ and \�pplyFileEdits\ propagate the error even though the write could succeed via a fallback path. Additionally, if the fallback copy succeeds but temp file cleanup fails (antivirus briefly locks the temp), the user sees an error despite the target file being written correctly.

Ref: #3430, #3199

What changed

• Extracted
  eplaceFileFromTemp\ helper from the inline rename logic in \writeFileContent\ and \�pplyFileEdits, eliminating code duplication.
• Added EPERM fallback: when \s.rename\ fails with \EPERM, falls back to \s.cp({ force: true })\ + best-effort \s.unlink\ of the temp file.
• Best-effort temp cleanup: if \s.cp\ succeeds but \s.unlink(tempPath)\ fails, the error is swallowed — the target was already written successfully.
• JSDoc documentation: documents \FILE_SHARE_DELETE\ limitation and the non-atomic nature of the \s.cp\ fallback.
• 5 new tests covering:
  • EPERM fallback through \writeFileContent\
  • EPERM fallback through \�pplyFileEdits\
  • Successful write despite temp cleanup failure (\writeFileContent)
  • Successful edit despite temp cleanup failure (\�pplyFileEdits)
  • Existing behavior preserved (non-EPERM errors still propagate)

Validation

\
npx vitest run
✓ tests/lib.test.ts (49 tests)
✓ tests/path-utils.test.ts (29 tests)
✓ tests/roots-utils.test.ts (3 tests)
✓ tests/path-validation.test.ts (53 tests)
✓ tests/directory-tree.test.ts (7 tests)
✓ tests/structured-content.test.ts (5 tests)
✓ tests/startup-validation.test.ts (4 tests)

Test Files 7 passed (7)
Tests 150 passed (150)
\\

\ sc --noEmit\ — 0 errors.

Checklist (from #3430)

• Temp file cleanup is best-effort (no false errors on successful writes)
• JSDoc documents \FILE_SHARE_DELETE\ requirement and non-atomic fallback
• \�pplyFileEdits\ has EPERM fallback test coverage
• Function name accurately reflects behavior (
  eplaceFileFromTemp)
• All acceptance tests pass

Fixes #3430

[Chelebii]

Follow-up: all checks are now passing after the lockfile refresh commit (c22fad2). The earlier Python failure was caused by src/fetch/uv.lock being out of sync with src/fetch/pyproject.toml, and that is now resolved.\n\nAt this point the PR looks blocked only on maintainer review / merge, not on CI.

[Chelebii]

@codex review focus on bugs, edge cases and missing tests

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 49s —— View job

---

Code Review

• Read changed files (lib.ts, lib.test.ts)
• Analyze implementation correctness and edge cases
• Check test coverage
• Post review feedback

---

Overall this is a clean, well-motivated change. The refactoring reduces duplication, the JSDoc is excellent, and the new tests cover the primary scenarios. There are a couple of real issues worth addressing before merge.

---

Bug: orphaned temp file when fs.cp itself fails

src/filesystem/lib.ts lines 166–181 — when fs.rename fails with EPERM and the fallback fs.cp also throws (e.g. disk full, target directory removed), the temp file is never cleaned up:

if ((renameError as NodeJS.ErrnoException).code === 'EPERM') {
  await fs.cp(tempPath, targetPath, { force: true }); // throws → temp file orphaned
  try { await fs.unlink(tempPath); } catch {}          // never reached
}

The caller (writeFileContent, applyFileEdits) has no finally block to clean up tempPath, so the *.hex.tmp file is left on disk. The fix is to catch the cp error, attempt cleanup, then re-throw:

if ((renameError as NodeJS.ErrnoException).code === 'EPERM') {
  try {
    await fs.cp(tempPath, targetPath, { force: true });
  } catch (cpError) {
    try { await fs.unlink(tempPath); } catch {}
    throw cpError;
  }
  try { await fs.unlink(tempPath); } catch {}
}

Fix this →

---

Minor regression: writeFile(tempPath) failure no longer attempts cleanup

Previously, the temp-file write and rename were in a single try block, so even a failed writeFile(tempPath, ...) triggered unlink(tempPath). With the refactor, if fs.writeFile(tempPath, ...) throws (e.g. quota exceeded mid-write), replaceFileFromTemp is never called and the partial temp file is orphaned. The old behaviour:

// OLD — unlink is attempted even if writeFile fails
try {
  await fs.writeFile(tempPath, content, 'utf-8');
  await fs.rename(tempPath, filePath);
} catch {
  try { await fs.unlink(tempPath); } catch {}
  throw;
}

A simple fix is a try/finally (or try/catch) around the write+replace pair in both call sites:

try {
  await fs.writeFile(tempPath, content, 'utf-8');
  await replaceFileFromTemp(tempPath, filePath);
} catch (err) {
  try { await fs.unlink(tempPath); } catch {}
  throw err;
}

Fix this →

---

Missing test: fs.cp failure in EPERM path

There is no test verifying that a fs.cp error (after an EPERM rename) propagates to the caller and that the temp file is cleaned up. Given the orphan-file bug above, a test like this would catch regressions:

it('cleans up temp and re-throws when fs.cp fails after EPERM', async () => {
  const epermError = Object.assign(new Error('EPERM'), { code: 'EPERM' });
  const enospcError = Object.assign(new Error('ENOSPC'), { code: 'ENOSPC' });

  mockFs.writeFile.mockResolvedValueOnce(undefined);
  mockFs.rename.mockRejectedValueOnce(epermError);
  mockFs.cp.mockRejectedValueOnce(enospcError);
  mockFs.unlink.mockResolvedValueOnce(undefined);

  await expect(writeFileContent('/tmp/file.txt', 'x')).rejects.toThrow('ENOSPC');
  expect(mockFs.unlink).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/));
});

---

Minor notes (no action required)

• EACCES on Linux/macOS: File-lock scenarios on Unix manifest as EACCES, not EPERM. The fallback won't help there, but the JSDoc clearly scopes this to the Windows FILE_SHARE_DELETE case, so this is acceptable as-is.
• fs.cp vs fs.copyFile: For a single file, fs.copyFile is more direct, but fs.cp({ force: true }) is fine for Node 22 and handles the overwrite semantic correctly.
• The code deduplication is clean and the JSDoc comments are thorough and accurate.

---

[Chelebii]

Addressed the Claude review feedback in b014d43.

What changed:

• clean up the temp file if the EPERM fallback fs.cp path fails
• restore best-effort temp cleanup when the temp-file writeFile step fails
• add regression coverage for both the fs.cp failure path and temp-write cleanup

Validation:

• npm run test --workspace @modelcontextprotocol/server-filesystem -- __tests__/lib.test.ts
• npm run build --workspace @modelcontextprotocol/server-filesystem

[LuuOW]

Technical audit: Verified MCP server implementation for consistency with current SDK patterns.

[hxaxd]

@Chelebii @olaservo, thank you both for the work and detailed investigation here. Is #3610 still the preferred path for resolving #3199/#3430? I noticed that the later review on #3296 recommends trying writeFile first before falling back to fs.cp for better symlink safety. If that change is still desired and additional help would be useful, I’d be happy to contribute a follow-up or prepare a fresh PR—whichever you prefer.

[Chelebii]

Thanks for checking in. #3610 is still open and the current CI checks are passing. I agree that trying writeFile before falling back to fs.cp would provide better symlink safety.

Your help with this change would be appreciated. A follow-up contribution to #3610 would probably be preferable, unless the maintainers would rather have a fresh PR. @olaservo, what do you think?

[hxaxd]

@Chelebii Thanks, I’d be happy to help. Are you planning to implement the writeFile-before-fs.cp change yourself? If not, would you be comfortable giving me access to the branch so I can make the change directly?

[Chelebii]

Thanks — I’m not planning to implement it myself. Rather than granting direct write access, could you make the change in your fork and open a PR against Chelebii/servers:fix/harden-atomic-replace-file? I’ll review and merge it into the branch, which will automatically update #3610. Please include tests covering symlink behavior and the fallback path.

[hxaxd]

@Chelebii I opened Chelebii#2 with the writeFile-before-fs.cp fallback and coverage for both the direct-overwrite and fallback paths. The focused filesystem tests and build pass. Thanks!

[Chelebii]

Follow-up: merged @hxaxd's contribution from Chelebii#2 into this branch as f78ab42. This changes the EPERM fallback order to rename → direct writeFile → fs.cp, reducing the symlink race exposure while retaining the final Windows fallback. It also adds coverage for both writeFileContent and applyFileEdits.

Validation:

• full filesystem suite: 153 tests passed
• filesystem TypeScript build passed
• upstream CI: 16/16 checks passed

Thanks @hxaxd for the contribution.