Skip to content

fix(filesystem): preserve inode and birthtime on write/edit (#4512) #4579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
knoal wants to merge 1 commit into
base: main
Choose a base branch
from
+250 −119
Open

fix(filesystem): preserve inode and birthtime on write/edit (#4512) #4579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions src/filesystem/__tests__/birthtime_real.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@

import { describe, it, expect, beforeEach } from 'vitest';
import { writeFileContent, applyFileEdits, setAllowedDirectories } from '../lib.js';
import { stat, mkdtemp, writeFile as fsWriteFile, copyFile, link, unlink } from 'fs/promises';
import { tmpdir } from 'os';
import { join } from 'path';

describe('Real filesystem birthtime preservation (#4512)', () => {
let testDir: string;
let testPath: string;
let hardlinkPath: string;

beforeEach(async () => {
testDir = await mkdtemp(join(tmpdir(), 'mcp-birthtime-'));
testPath = join(testDir, 'file.txt');
hardlinkPath = join(testDir, 'hardlink.txt');
setAllowedDirectories([testDir]);
});

it('writeFileContent preserves birthtime and inode', async () => {
// Create initial file
await writeFileContent(testPath, 'initial');
const before = await stat(testPath);
await new Promise(r => setTimeout(r, 1500));

// Write again — pre-fix this would destroy birthtime and inode
await writeFileContent(testPath, 'updated');

const after = await stat(testPath);
expect(after.birthtimeMs).toBe(before.birthtimeMs);
expect(after.ino).toBe(before.ino);
expect(after.nlink).toBe(before.nlink);
});

it('writeFileContent preserves hard links', async () => {
await writeFileContent(testPath, 'initial');
// Create a hard link to the same inode
await link(testPath, hardlinkPath);
const before = await stat(testPath);
expect(before.nlink).toBe(2);

await new Promise(r => setTimeout(r, 1500));
await writeFileContent(testPath, 'updated');

const after = await stat(testPath);
expect(after.nlink).toBe(2); // Hard link should still be valid
expect(after.ino).toBe(before.ino);

// The hardlinked file should still be readable
const { readFile } = await import('fs/promises');
const hardContent = await readFile(hardlinkPath, 'utf-8');
expect(hardContent).toBe('updated'); // Both names point to same inode with new content
});

it('applyFileEdits preserves birthtime and inode', async () => {
await writeFileContent(testPath, 'line1\nline2\nline3\n');
const before = await stat(testPath);
await new Promise(r => setTimeout(r, 1500));

await applyFileEdits(testPath, [{ oldText: 'line2', newText: 'modified' }], false);

const after = await stat(testPath);
expect(after.birthtimeMs).toBe(before.birthtimeMs);
expect(after.ino).toBe(before.ino);
});
});
201 changes: 108 additions & 93 deletions src/filesystem/__tests__/lib.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,11 +303,44 @@ describe('Lib Functions', () => {
describe('writeFileContent', () => {
it('writes file content', async () => {
mockFs.writeFile.mockResolvedValueOnce(undefined);

await writeFileContent('/test/file.txt', 'new content');

expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' });
});

// Regression test for issue #4512: when the file already exists, the
// 'wx' flag fails with EEXIST and the code falls back to writeInPlace.
// The fix replaces temp-file+rename with fs.open + handle.writeFile
// to preserve inode, birthtime, hard links, and inode-based file
// watchers.
it('writes to an existing file in place, preserving birthtime', async () => {
// First call (with 'wx' flag) fails with EEXIST — file exists.
// Second call path is the new writeInPlace which uses fs.open.
const eexistError = Object.assign(new Error('EEXIST'), { code: 'EEXIST' });
mockFs.writeFile.mockRejectedValueOnce(eexistError);
const mockHandle = {
writeFile: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
stat: vi.fn().mockResolvedValue({ isFile: () => true } as any),
};
mockFs.open.mockResolvedValue(mockHandle as any);

await writeFileContent('/test/file.txt', 'new content');

// The fix: use fs.open + handle.writeFile in place. No temp file.
expect(mockFs.open).toHaveBeenCalledWith(
'/test/file.txt',
expect.any(Number) // O_WRONLY | O_CREAT | O_TRUNC
);
expect(mockHandle.writeFile).toHaveBeenCalledWith('new content', { encoding: 'utf-8' });
expect(mockHandle.close).toHaveBeenCalled();
// The pre-fix code would have called writeFile(tempPath) + rename.
// After the fix, mockFs.writeFile should be called only once (the
// initial 'wx' attempt), not twice.
expect(mockFs.writeFile).toHaveBeenCalledTimes(1);
expect(mockFs.rename).not.toHaveBeenCalled();
});
});

});
Expand Down Expand Up @@ -416,54 +449,60 @@ describe('Lib Functions', () => {
beforeEach(() => {
mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n');
mockFs.writeFile.mockResolvedValue(undefined);
// The new writeInPlace() uses fs.open + handle.writeFile + handle.close.
// The tests need to provide a mock FileHandle with those methods.
const mockHandle = {
writeFile: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
stat: vi.fn().mockResolvedValue({ isFile: () => true } as any),
};
mockFs.open.mockResolvedValue(mockHandle as any);
});

it('applies simple text replacement', async () => {
const edits = [
{ oldText: 'line2', newText: 'modified line2' }
];

mockFs.rename.mockResolvedValueOnce(undefined);


const result = await applyFileEdits('/test/file.txt', edits, false);

expect(result).toContain('modified line2');
// Should write to temporary file then rename
expect(mockFs.writeFile).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
'line1\nmodified line2\nline3\n',
'utf-8'
);
expect(mockFs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
'/test/file.txt'
// The fix: writes via fs.open + handle.writeFile in place,
// preserving inode/birthtime/hard links (see issue #4512).
expect(mockFs.open).toHaveBeenCalledWith(
'/test/file.txt',
expect.any(Number) // O_WRONLY | O_CREAT | O_TRUNC on POSIX
);
const handle = await mockFs.open.mock.results[0].value;
expect(handle.writeFile).toHaveBeenCalledWith('line1\nmodified line2\nline3\n', { encoding: 'utf-8' });
expect(handle.close).toHaveBeenCalled();
// Should NOT use temp file + rename (the old broken pattern).
expect(mockFs.rename).not.toHaveBeenCalled();
});

it('treats dollar signs in replacement text literally', async () => {
const edits = [
{ oldText: 'line2', newText: "price=$$; match=$&; before=$`; after=$'" }
];

mockFs.rename.mockResolvedValueOnce(undefined);

await applyFileEdits('/test/file.txt', edits, false);

expect(mockFs.writeFile).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
const handle = await mockFs.open.mock.results[0].value;
expect(handle.writeFile).toHaveBeenCalledWith(
"line1\nprice=$$; match=$&; before=$`; after=$'\nline3\n",
'utf-8'
{ encoding: 'utf-8' }
);
});

it('handles dry run mode', async () => {
const edits = [
{ oldText: 'line2', newText: 'modified line2' }
];

const result = await applyFileEdits('/test/file.txt', edits, true);

expect(result).toContain('modified line2');
expect(mockFs.open).not.toHaveBeenCalled();
expect(mockFs.writeFile).not.toHaveBeenCalled();
});

Expand All @@ -472,122 +511,98 @@ describe('Lib Functions', () => {
{ oldText: 'line1', newText: 'first line' },
{ oldText: 'line3', newText: 'third line' }
];
mockFs.rename.mockResolvedValueOnce(undefined);
await applyFileEdits('/test/file.txt', edits, false);

expect(mockFs.writeFile).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),

const result = await applyFileEdits('/test/file.txt', edits, false);

expect(result).toContain('first line');
expect(mockFs.open).toHaveBeenCalled();
const handle = await mockFs.open.mock.results[0].value;
expect(handle.writeFile).toHaveBeenCalledWith(
'first line\nline2\nthird line\n',
'utf-8'
);
expect(mockFs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
'/test/file.txt'
{ encoding: 'utf-8' }
);
});

it('handles whitespace-flexible matching', async () => {
mockFs.readFile.mockResolvedValue(' line1\n line2\n line3\n');

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

mockFs.rename.mockResolvedValueOnce(undefined);

await applyFileEdits('/test/file.txt', edits, false);

expect(mockFs.writeFile).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),

const result = await applyFileEdits('/test/file.txt', edits, false);

expect(result).toContain('modified line2');
const handle = await mockFs.open.mock.results[0].value;
expect(handle.writeFile).toHaveBeenCalledWith(
' line1\n modified line2\n line3\n',
'utf-8'
);
expect(mockFs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
'/test/file.txt'
{ encoding: 'utf-8' }
);
});

it('throws error for non-matching edits', async () => {
const edits = [
{ oldText: 'nonexistent line', newText: 'replacement' }
];

await expect(applyFileEdits('/test/file.txt', edits, false))
.rejects.toThrow('Could not find exact match for edit');
});

it('handles complex multi-line edits with indentation', async () => {
mockFs.readFile.mockResolvedValue('function test() {\n console.log("hello");\n return true;\n}');

const edits = [
{
oldText: ' console.log("hello");\n return true;',
newText: ' console.log("world");\n console.log("test");\n return false;'
{
oldText: ' console.log("hello");\n return true;',
newText: ' console.log("world");\n console.log("test");\n return false;'
}
];

mockFs.rename.mockResolvedValueOnce(undefined);

await applyFileEdits('/test/file.js', edits, false);

expect(mockFs.writeFile).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/),

const result = await applyFileEdits('/test/file.js', edits, false);

expect(result).toContain('console.log("world")');
const handle = await mockFs.open.mock.results[0].value;
expect(handle.writeFile).toHaveBeenCalledWith(
'function test() {\n console.log("world");\n console.log("test");\n return false;\n}',
'utf-8'
);
expect(mockFs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/),
'/test/file.js'
{ encoding: 'utf-8' }
);
});

it('handles edits with different indentation patterns', async () => {
mockFs.readFile.mockResolvedValue(' if (condition) {\n doSomething();\n }');

const edits = [
{
oldText: 'doSomething();',
newText: 'doSomethingElse();\n doAnotherThing();'
{
oldText: 'doSomething();',
newText: 'doSomethingElse();\n doAnotherThing();'
}
];

mockFs.rename.mockResolvedValueOnce(undefined);

await applyFileEdits('/test/file.js', edits, false);

expect(mockFs.writeFile).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/),

const result = await applyFileEdits('/test/file.js', edits, false);

expect(result).toContain('doSomethingElse');
const handle = await mockFs.open.mock.results[0].value;
expect(handle.writeFile).toHaveBeenCalledWith(
' if (condition) {\n doSomethingElse();\n doAnotherThing();\n }',
'utf-8'
);
expect(mockFs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/),
'/test/file.js'
{ encoding: 'utf-8' }
);
});

it('handles CRLF line endings in file content', async () => {
mockFs.readFile.mockResolvedValue('line1\r\nline2\r\nline3\r\n');

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

mockFs.rename.mockResolvedValueOnce(undefined);

await applyFileEdits('/test/file.txt', edits, false);

expect(mockFs.writeFile).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),

const result = await applyFileEdits('/test/file.txt', edits, false);

expect(result).toContain('modified line2');
const handle = await mockFs.open.mock.results[0].value;
expect(handle.writeFile).toHaveBeenCalledWith(
'line1\nmodified line2\nline3\n',
'utf-8'
);
expect(mockFs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
'/test/file.txt'
{ encoding: 'utf-8' }
);
});
});
Expand Down
Loading
Loading