Repository navigation
fix(cli): make persistent state writes failure-safe - #29402
Oscar-Williams wants to merge 5 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
📊 PR Size: size/L
|
gemini-cli
Bot
added
priority/p1
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 enhances the reliability of the CLI's persistent state management by introducing failure-safe write operations. By utilizing atomic file system operations and implementing a robust backup and recovery workflow, the changes protect the application's state from data loss caused by unexpected interruptions or file corruption. 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]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances PersistentState with atomic file saving, backup creation, and automatic recovery from corrupt state files. It also adds comprehensive unit and integration tests to validate these behaviors. The reviewer feedback suggests refactoring the synchronous file system operations in the save mechanism to use the asynchronous fs.promises API to prevent blocking the event loop, and updating the corresponding tests to match this asynchronous pattern.
| temporaryPath = `${filePath}.${randomUUID()}.tmp`; | ||
| fs.writeFileSync(temporaryPath, JSON.stringify(this.cache, null, 2), { | ||
| encoding: 'utf-8', | ||
| flag: 'wx', | ||
| }); | ||
|
|
||
| // A writable descriptor is required for fsyncSync on Windows. | ||
| fileDescriptor = fs.openSync(temporaryPath, 'r+'); | ||
| fs.fsyncSync(fileDescriptor); | ||
| fs.closeSync(fileDescriptor); | ||
| fileDescriptor = undefined; |
There was a problem hiding this comment.
Opening the temporary file twice introduces unnecessary I/O overhead and is prone to race conditions on Windows. To ensure robustness and non-blocking behavior, use the asynchronous fs.promises API. Open the file handle once with 'wx', write to it, sync, and close it using the promise-based file handle methods.
temporaryPath = `${filePath}.${randomUUID()}.tmp`;
const fileHandle = await fs.promises.open(temporaryPath, 'wx');
await fileHandle.writeFile(JSON.stringify(this.cache, null, 2), 'utf-8');
await fileHandle.sync();
await fileHandle.close();References
- Use asynchronous file system operations instead of synchronous ones to avoid blocking the event loop.
| it('should save state through a temporary file and publish it atomically', () => { | ||
| vi.mocked(fs.existsSync).mockReturnValue(false); | ||
| persistentState.set('defaultBannerShownCount', { banner1: 1 }); | ||
|
|
||
| expect(fs.mkdirSync).toHaveBeenCalledWith(path.normalize(mockDir), { | ||
| recursive: true, | ||
| }); | ||
| expect(fs.writeFileSync).toHaveBeenCalledWith( | ||
| mockFilePath, | ||
| expect.stringMatching(/state\.json\..+\.tmp$/), | ||
| JSON.stringify({ defaultBannerShownCount: { banner1: 1 } }, null, 2), | ||
| { encoding: 'utf-8', flag: 'wx' }, | ||
| ); | ||
| expect(fs.openSync).toHaveBeenCalledWith( | ||
| expect.stringMatching(/state\.json\..+\.tmp$/), | ||
| 'r+', | ||
| ); | ||
| expect(fs.fsyncSync).toHaveBeenCalled(); | ||
| expect(fs.closeSync).toHaveBeenCalled(); | ||
| expect(fs.renameSync).toHaveBeenCalledWith( | ||
| expect.stringMatching(/state\.json\..+\.tmp$/), | ||
| mockFilePath, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Update the test expectations to match the asynchronous file handle pattern. Since this is a test environment, synchronous mocks are acceptable for setup.
it('should save state through a temporary file and publish it atomically', async () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
const mockHandle = { writeFile: vi.fn(), sync: vi.fn(), close: vi.fn() };
vi.mocked(fs.promises.open).mockResolvedValue(mockHandle as any);
persistentState.set('defaultBannerShownCount', { banner1: 1 });
expect(fs.promises.open).toHaveBeenCalledWith(
expect.stringMatching(/state\.json\..+\.tmp$/),
'wx',
);
expect(mockHandle.writeFile).toHaveBeenCalledWith(
JSON.stringify({ defaultBannerShownCount: { banner1: 1 } }, null, 2),
'utf-8',
);
expect(mockHandle.sync).toHaveBeenCalled();
expect(mockHandle.close).toHaveBeenCalled();
});References
- For test environment setup, it is acceptable to use synchronous operations within an async function.
|
Addressed the Gemini Code Assist feedback in commit
Focused CLI tests: 22 passed. CLI typecheck and build pass on Windows. Please re-review the updated head. |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request updates the PersistentState class to perform atomic file writes using temporary files, maintain a backup of the previous state, and automatically recover from corrupted state files by restoring from the backup. It also serializes consecutive saves using a promise queue, changing the set method to return a Promise<void>, which requires updating its usage across various UI components and hooks. The review feedback identifies a critical issue where a restored backup state could be silently lost if the CLI process exits before a new save is triggered, and suggests performing a synchronous save immediately after recovery to ensure data durability.
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| this.cache = JSON.parse(content); | ||
| } catch (error) { | ||
| this.cache = this.recoverFromCorruptState(filePath, error); | ||
| } |
There was a problem hiding this comment.
If state.json is corrupt, recoverFromCorruptState successfully renames it to state.json.corrupt and restores the backup state into this.cache in memory. However, if the CLI process exits without calling set() (which triggers save()), the restored state is never written back to state.json. On the next run, state.json will be missing entirely, and load() will initialize this.cache to {} because it only attempts recovery if state.json exists. This results in the restored backup state being silently lost on the subsequent run.
To fix this, we should trigger a synchronous save of the restored backup state back to state.json immediately after recovery. In error handling contexts like this, synchronous file I/O is preferred over asynchronous methods to ensure data durability, especially since the CLI process might terminate unexpectedly.
| try { | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | |
| this.cache = JSON.parse(content); | |
| } catch (error) { | |
| this.cache = this.recoverFromCorruptState(filePath, error); | |
| } | |
| try { | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | |
| this.cache = JSON.parse(content); | |
| } catch (error) { | |
| this.cache = this.recoverFromCorruptState(filePath, error); | |
| this.saveSync(); | |
| } |
References
- In contexts like test logging or error handling, prefer synchronous file I/O over asynchronous methods to ensure data durability and log capture when the process might terminate unexpectedly.
- The use of synchronous file I/O from
persistentStateis acceptable within React hooks for this CLI application.
|
Addressed the follow-up recovery durability finding in commit When a corrupt Focused CLI tests remain 22 passed; CLI typecheck and build pass on Windows. Please re-review the updated head. |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors PersistentState to perform atomic file writes asynchronously using a temporary file, fsync, and rename operations. It introduces a backup system (.bak), corruption recovery (.corrupt), and a promise queue to serialize consecutive saves, updating the set method to return a Promise<void>. UI components and hooks have been updated to handle the promise, and new integration and unit tests have been added. The reviewer identified a potential race condition on Windows in the synchronous fallback saveSync() method, where reopening a recently closed file to sync it could fail due to file locking, and suggested writing directly to the opened file descriptor instead.
| fs.writeFileSync(temporaryPath, JSON.stringify(this.cache, null, 2), { | ||
| encoding: 'utf-8', | ||
| flag: 'wx', | ||
| }); | ||
|
|
||
| fileDescriptor = fs.openSync(temporaryPath, 'r+'); | ||
| fs.fsyncSync(fileDescriptor); | ||
| fs.closeSync(fileDescriptor); | ||
| fileDescriptor = undefined; |
There was a problem hiding this comment.
In saveSync(), writing the file using fs.writeFileSync and then immediately reopening it with fs.openSync(..., 'r+') to perform fsyncSync creates a race condition on Windows. On Windows, background processes like anti-virus scanners or search indexers often open and lock newly created/modified files immediately after they are closed. Reopening the file immediately after writeFileSync closes it can result in transient EACCES (Permission Denied) errors. To prevent this, open the file descriptor once with 'wx', write the content, fsyncSync it, and then close it.
| fs.writeFileSync(temporaryPath, JSON.stringify(this.cache, null, 2), { | |
| encoding: 'utf-8', | |
| flag: 'wx', | |
| }); | |
| fileDescriptor = fs.openSync(temporaryPath, 'r+'); | |
| fs.fsyncSync(fileDescriptor); | |
| fs.closeSync(fileDescriptor); | |
| fileDescriptor = undefined; | |
| fileDescriptor = fs.openSync(temporaryPath, 'wx'); | |
| fs.writeSync(fileDescriptor, JSON.stringify(this.cache, null, 2)); | |
| fs.fsyncSync(fileDescriptor); | |
| fs.closeSync(fileDescriptor); | |
| fileDescriptor = undefined; |
References
- The use of synchronous file I/O from persistentState is acceptable within React hooks for this CLI application, contrary to general advice against blocking operations.
|
Addressed the Windows recovery-path review in commit
Focused CLI tests remain 22 passed; CLI typecheck and build pass on Windows. Please re-review the updated head. |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the PersistentState class to ensure atomic, asynchronous state persistence. State updates are now written to a temporary file, synced, and atomically renamed to the target file, with the previous state preserved as a backup (.bak). Additionally, a corruption recovery mechanism has been introduced to restore from the backup if the main state file becomes corrupted, and consecutive saves are serialized using a promise queue. Consequently, the set method now returns a Promise<void>, and its call sites across the codebase have been updated to handle this change. Comprehensive unit and integration tests have also been added to verify these filesystem behaviors. I have no feedback to provide as there are no review comments to address.
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
|
The current head addresses the file-handle and recovery-durability review, and the CLA check is now passing. #29307 still lacks |
|
This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
2 participants
Summary
Make
PersistentStatewrites failure-safe so an interrupted save cannot replacestate.jsonwith truncated JSON and silently clear the CLI's persistent state.Details
fsyncit, then publish with an atomic rename.fs.promisesfile handle per save and serialize queued saves to keep the event loop responsive while preserving call order.state.json.bakbefore replacement.state.json.corruptand restore a valid backup when available.load()returns, so the recovery survives an immediate process exit.wx, writes through that descriptor, fsyncs it, and then closes it before the atomic rename.commentJsonand other write paths outside this focused fix; they are separate follow-ups.Related Issues
Fixes #29307
How to Validate
From
packages/cli:The focused suite covers the miss-to-save path, asynchronous file-handle writes, serialized saves, real filesystem backup creation, corrupt-file preservation, durable backup recovery, and adjacent JSON writes.
npm run typecheck --workspace @google/gemini-cliandnpm run build --workspace @google/gemini-clialso pass on Windows.The focused
persistentState.test.tsfile passes 7/7; its recovery case asserts the synchronous republish and the single-descriptoropenSync('wx')→writeSync→fsyncSync→closeSyncsequence.AI assistance was used for implementation and automated validation; no manual contributor test is claimed.
Pre-Merge Checklist