Skip to content

fix(filesystem): harden file replacement with EPERM fallback#3610

Open
Chelebii wants to merge 1 commit intomodelcontextprotocol:mainfrom
Chelebii:fix/harden-atomic-replace-file
Open

fix(filesystem): harden file replacement with EPERM fallback#3610
Chelebii wants to merge 1 commit intomodelcontextprotocol:mainfrom
Chelebii:fix/harden-atomic-replace-file

Conversation

@Chelebii
Copy link
Contributor

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

Extract replaceFileFromTemp helper that falls back to fs.cp when
fs.rename fails with EPERM (Windows locked files). Temp file cleanup
is best-effort so a successful write is never masked by a cleanup
failure (e.g. antivirus holding the temp file).

Changes:
- Extract replaceFileFromTemp from inline rename logic in
  writeFileContent and applyFileEdits
- Add EPERM fallback: rename -> cp + best-effort unlink
- Document FILE_SHARE_DELETE limitation and non-atomic fallback in
  JSDoc
- Add tests for EPERM fallback through both writeFileContent and
  applyFileEdits
- Add tests for successful writes despite temp cleanup failure

Fixes modelcontextprotocol#3430
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant