fix(core): make tool file writes atomic and serialize same-path writes - #29244
fix(core): make tool file writes atomic and serialize same-path writes#29244ranjan-del wants to merge 2 commits into
Conversation
Parallel tool execution lets two file operations target the same path at
once. Four consequences, each reproduced on main before fixing:
- fs.writeFile truncates the destination and then streams it back in
512KB chunks, so a concurrent reader observes a partially written
file. A 16MB write exposes dozens of truncated intermediate sizes.
- Two parallel edits both read the original content, and whichever
writes second discards the other's edit while both tool calls report
success to the model. Editing "alpha" and "beta" in one file
concurrently leaves "alpha\nBETA\n".
- Two parallel write_file calls to the same new path both report
creating it, so the second one's diff claims the file was empty
beforehand.
- Concurrent createFileSnapshot calls interleave as add, add, commit,
commit. Since add('.') stages the whole worktree, one snapshot folds
the other's files into its commit.
StandardFileSystemService.writeTextFile now writes to a uniquely named
sibling temp file and renames it into place, so an observer sees either
the old file or the new one. It copies the destination's permissions
onto the temp file first, so replacing a 0600 file does not widen it,
and retries the rename on transient Windows EBUSY/EPERM.
A new in-process per-path async mutex serializes the read-modify-write
in EditTool and WriteFileTool, keyed on the already realpath-resolved
target, and serializes stage/status/commit in GitService.
Known limits, left for follow-up work:
- The mutex is in-process only. It does not coordinate with a second
Gemini CLI process or an external editor; that needs an on-disk lock.
- SandboxedFileSystemService still writes through its __write sandbox
command and is unchanged, so sandboxed writes are not yet atomic.
Fixes google-gemini#29078
|
📊 PR Size: size/L
|
gemini-cli
Bot
added
priority/p1
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical race conditions in file system and Git operations caused by parallel tool execution. By introducing atomic write patterns and an in-process path-based mutex, it ensures that concurrent file operations are properly serialized, preventing data loss and inconsistent states. These changes improve the reliability of file edits and checkpointing mechanisms without introducing breaking changes to the existing interface. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces concurrency control and atomicity across file operations and git snapshots. It implements an in-process per-path mutex (withPathLock) to serialize concurrent edits, writes, and git snapshots targeting the same paths, preventing race conditions and data loss. Additionally, StandardFileSystemService.writeTextFile is updated to write atomically via a temporary file and rename, including permission preservation and rename retries for Windows compatibility. The feedback suggests wrapping the fs.chmod call in a try-catch block to make permission preservation best-effort, preventing failures on filesystems or environments that do not support permission changes.
Note: Security Review did not run due to the size of the PR.
| const existingMode = await this.getFileMode(filePath); | ||
| if (existingMode !== undefined) { | ||
| await fs.chmod(tmpPath, existingMode); | ||
| } |
There was a problem hiding this comment.
On certain filesystems (such as FAT32, exFAT, or network mounts like NFS/CIFS) or in highly restricted sandbox/container environments, fs.chmod can throw errors like EPERM or ENOTSUP. If fs.chmod fails, the entire write operation will fail, which is a regression compared to non-atomic writes. Wrapping fs.chmod in a try-catch block to make permission preservation best-effort ensures maximum compatibility across diverse environments.
const existingMode = await this.getFileMode(filePath);
if (existingMode !== undefined) {
try {
await fs.chmod(tmpPath, existingMode);
} catch {
// Best effort: some filesystems or restricted environments do not support chmod.
}
}There was a problem hiding this comment.
Confirmed and fixed in 44ae691.
I could not test FAT32 or an NFS mount directly, so I verified it the way I could: forcing chmod to reject with ENOTSUP made writeTextFile reject rather than resolve, and the temp file was deleted. So the write was lost, and since the previous in-place fs.writeFile never called chmod, that was a regression introduced by this PR.
The chmod is now best-effort as you suggested. I also went slightly further, because making it best-effort on its own leaves a second problem: if chmod is the only thing setting the mode and it silently fails, a replacement for a 0600 file lands at the default mode. And even when chmod succeeds, the temp file exists briefly at the default mode with the secret content already in it.
So the destination's mode is now also passed when the temp file is created:
const existingMode = await this.getFileMode(filePath);
await fs.writeFile(tmpPath, content, {
encoding: 'utf-8',
...(existingMode !== undefined ? { mode: existingMode } : {}),
});
if (existingMode !== undefined) {
try {
await fs.chmod(tmpPath, existingMode);
} catch {
// Keep whatever mode the temp file was created with.
}
}Creation mode is masked by umask, so it can only end up more restrictive, never wider. The chmod then corrects that narrowing where it is permitted, and where it is not, we are left more restrictive rather than more permissive, which is the safer direction to fail in.
Two regression tests added: the temp file is created carrying the destination's mode, and a rejecting chmod neither fails the write nor deletes the temp file.
Review feedback on google-gemini#29244: fs.chmod sat inside the try block whose catch removes the temp file and rethrows, so on filesystems that reject chmod (FAT32, exFAT, some NFS/CIFS mounts) or in restricted sandboxes raising EPERM/ENOTSUP, the whole write failed and the content was lost. The previous in-place fs.writeFile never called chmod, so that was a regression introduced by the atomic write. The chmod is now best-effort. The destination's mode is also passed when the temp file is created, which keeps the mode correct in the common case and closes the window where a replacement for a 0600 file was briefly readable through the default mode. Creation mode is masked by umask so it can only be more restrictive; the chmod that follows corrects that when it is permitted. Two regression tests: the temp file is created with the destination's mode, and a rejecting chmod does not fail the write or delete the temp file.
GitHub Actions
Waiting
1 participant
Summary
Parallel tool execution lets two file operations target the same path at once.
Today that silently loses edits: two concurrent
replacecalls on one file bothread the original content, and whichever writes second discards the other's edit
while both tool calls report success to the model. Neither the user nor the
agent can tell it happened.
This makes tool writes atomic and serializes the read-modify-write per path, and
covers the other three races in #29078. Every one was reproduced on
mainfirst,with a regression test added for each.
Details
Four distinct problems, and what each fix does.
1. Writes were not atomic.
fs.writeFileopens withO_TRUNCand thenstreams the content back in 512KB chunks, so a concurrent reader sees a
truncated prefix. A 64MB write exposed 129 distinct partial sizes
(
0, 524288, 1048576, ...).StandardFileSystemService.writeTextFilenow writes to a uniquely named siblingtemp file and renames it into place, so an observer sees either the old file or
the new one. Two details worth flagging for review:
A fresh temp file does not inherit permissions, so without this, replacing a
0600file would silently widen it.EBUSY/EPERM, which Windows raises transiently whileanother process holds the destination open. This mirrors the existing
backoff in
config/projectRegistry.ts.2. Lost update in
replace.EditTool.executereads viacalculateEditand writes later in the same method, with
awaitpoints in between, so twoinvocations interleave as read, read, write, write. Editing
alphaandbetain one file concurrently left
alpha\nBETA\n.Fixed with a new in-process per-path async mutex (
utils/pathMutex.ts). Thebody of
executemoved intoapplyEdit, which runs inside the lock, so thesecond edit reads the first one's result and both land. The key is the
already realpath-resolved target, so two spellings of one file share a lock.
3.
write_fileexistence check raced. Two concurrent calls to the same newpath both reported "Successfully created and wrote to new file", so the second
one's diff claimed the file was empty beforehand. Same fix,
applyWriteinsidethe path lock.
4. Checkpoint snapshots interleaved.
GitService.createFileSnapshotrunsadd('.'),status(),commit(). Concurrent calls interleaved asadd, add, commit, commit, and becauseadd('.')stages the whole worktree,one snapshot folded the other's files into its commit. Now serialized per
shadow repository.
Deliberately out of scope
Happy to extend this PR or open follow-ups, whichever you prefer:
CLI process or an external editor writing the same file. That needs an on-disk
lock (
proper-lockfileor anO_EXCLlockfile), which felt like a separatedecision.
SandboxedFileSystemServiceis unchanged. It writes through a__writesandbox command, so sandboxed writes are still not atomic. Fixing it means
changing the sandbox helper, not just this service.
No user-facing command, flag, or output changes, so no
/docsupdate. Nobreaking changes:
FileSystemServicekeeps its existing two-method interface.Related Issues
Fixes #29078
How to Validate
Reproduce the headline bug on
main(before this change):Then with this branch, all four regression tests pass:
The atomicity test is worth a look, because the obvious version of it does not
work. An async reader polling during the write is only ever scheduled before or
after it, since the write runs on the libuv threadpool. It needs a synchronous
statSyncloop on the main thread to actually catch the destination mid-write.src/services/fileSystemService.atomic.test.tsdoes that, and guards against avacuous pass by asserting the observer ran at least once.
Full gate:
Note for reviewers running this locally: if your checkout is not a trusted
workspace, 9 tests in
packages/cli/src/gemini.test.tsxfail withFatalUntrustedWorkspaceErrorregardless of this change. SetGEMINI_CLI_TRUST_WORKSPACE=trueand they pass. I confirmed those same 9 failon a pristine stashed tree.
Pre-Merge Checklist
user-facing surface change
watched failing first
Validated on macOS with
npm runonly. I do not have Windows, Linux, Docker,Podman or Seatbelt available here. The Windows-relevant part of this change is
the
EBUSY/EPERMrename retry, which I could not exercise on a real Windowshost, so it deserves a careful look or a CI run on Windows before merge.
Why this is a draft
#29078 does not carry the
help wantedlabel, and CONTRIBUTING askscontributors to discuss the issue and wait for maintainer feedback first. I had
the fix working while investigating, so I am opening this as a draft rather than
sitting on it. Happy to close it, split it up, or take it in a different
direction if a maintainer would rather shape the approach first.