fix(core,cli): repair /compress session reload and quota-fallback tool response loss - #28672
Conversation
ChatRecordingService.initialize() re-read the session file from disk when resuming and threw 'Failed to load resumed session data from file' if the read returned null, discarding the in-memory conversation it had just been handed. Because tryCompressChat() re-initializes the chat with the live session as resumed data, any reload hiccup (missing file, corrupt metadata line, or a transient I/O error) surfaced to the user as 'Failed to compress chat history: Failed to initialize chat: ...' and made /compress unusable. Fall back to the supplied in-memory conversation instead of throwing, and rewrite a clean session file from it (atomically, via temp file + rename) so later appends and future loads succeed.
When a quota error switched the active model mid-session, handleCompletedTools() marked the tools as submitted and returned early without ever recording their functionResponse parts. The model turn holding the matching functionCall was already in history, so this left a dangling tool call that violates the Gemini API invariant requiring every functionCall to be followed by its functionResponse. The malformed history persisted (the quota flag only resets on the next non-continuation query), so subsequent requests were sent with an unpaired call and the model continued/autocompleted the user's next message instead of answering it. Record the responses before returning, mirroring the two cancellation branches. The turn still does not auto-continue on the fallback model.
|
📊 PR Size: size/L
|
Two refinements to the fixes in this PR: - chatRecordingService: the recovery rewrite now moves the unreadable session file aside instead of overwriting it. The reload may have failed only transiently (a lock or I/O blip) on a file that is actually intact, so its bytes are kept rather than destroyed. - useGeminiStream: move the quota-fallback early return above the steering hint block. The hint is built to be sent to the model, but this path returns without submitting, so consuming it there both dropped the user's steering text and wrote it into history ahead of the functionResponse parts. Leaving it unconsumed keeps the recorded turn to just the tool responses and lets the hint ride along with the next real submit.
gemini-cli
Bot
added
area/core
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 two critical stability issues in the Gemini CLI. The first fix improves session management during compression by ensuring that transient disk read errors do not cause the application to crash, instead opting to recover using the existing in-memory state. The second fix ensures that when the CLI switches models due to quota limits, it properly persists tool results to maintain the integrity of the conversation history, preventing the model from autocompleting user input. 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 improves robustness in two areas: it ensures tool responses are recorded in history when a model switch occurs due to a quota error in useGeminiStream, and it implements a fallback to in-memory conversations in ChatRecordingService when session files are unreadable, writing a clean file atomically and backing up the corrupt file. The review feedback suggests using asynchronous file system operations instead of synchronous ones to prevent blocking the event loop, and adding a try...finally block to clean up temporary files in case of write or rename failures.
| const tempFile = `${this.conversationFile}.tmp-${process.pid}`; | ||
| fs.writeFileSync(tempFile, content); | ||
| fs.renameSync(tempFile, this.conversationFile); |
There was a problem hiding this comment.
When writing to a temporary file and then renaming it atomically, if the write or rename fails, the temporary file will be left orphaned on disk. Wrapping the write and rename operations in a try...finally block to clean up the temporary file ensures that no orphaned .tmp-* files are left behind. Additionally, use asynchronous file system operations (e.g., fs.promises.writeFile) instead of synchronous ones to avoid blocking the event loop.
const tempFile = this.conversationFile + '.tmp-' + process.pid;
try {
await fs.promises.writeFile(tempFile, content);
await fs.promises.rename(tempFile, this.conversationFile);
} finally {
try {
await fs.promises.unlink(tempFile);
} catch {
// Ignore cleanup errors to avoid masking the original error
}
}References
- Use asynchronous file system operations (e.g.,
fs.promises.readFile) instead of synchronous ones (e.g.,fs.readFileSync) to avoid blocking the event loop.
If writeFileSync succeeded but renameSync did not, the .tmp-* file was left orphaned next to the session file. Remove it on the failure path and rethrow, so the original error still surfaces. Cleanup is done in catch rather than finally: after a successful rename the temp path no longer exists, so unlinking unconditionally would issue a pointless syscall and swallow an ENOENT on every healthy write. Kept synchronous to match the rest of this service. appendRecord writes every message with fs.appendFileSync, and introducing an await between the write and the rename would let a sync append land on a file that is about to be replaced.
2 participants
Summary
Two independent bug fixes.
1.
/compressfails and stays broken. Running/compress(or hitting automatic compression) could fail withFailed to compress chat history: Failed to initialize chat: Failed to load resumed session data from file.2. Hitting a quota limit corrupts the conversation. After a quota/rate-limit fallback, the model starts finishing your sentences instead of answering your next message.
Fix 2 is the higher-priority one: it silently poisons a conversation for the rest of its life.
Details
Some background that makes both fixes easier to follow:
Fix 1 —
/compress: use the conversation we already haveWhat happened. Compression summarizes old messages to free up space, then re-initializes the chat. That re-initialization hands the recorder two things: the conversation already loaded in memory, and the path to the session file.
The recorder used the in-memory copy for two small fields, then ignored it and re-read the whole conversation off disk anyway. If that re-read came back empty — file missing, a corrupted first line, or a momentary disk/read error — it threw an error and gave up, even though a perfectly good copy was sitting right there in its own argument.
That error travelled up and became the
Failed to compress chat historymessage the user sees.The fix. Use the in-memory copy instead of giving up, and write a fresh, readable session file from it so future saves and loads work again.
Safety. The unreadable file is moved aside (to
<name>.unreadable-<timestamp>), never overwritten. Of the three ways the read can come back empty, two are safe to replace (file missing; corrupt first line, which would fail forever anyway) — but the third, a momentary read error, may leave a perfectly intact file. So we keep its contents rather than destroy them. The new file is written to a temp path and renamed into place, so an interrupted write can't leave a half-written file.When this broke. This is a regression. The older code handled an unreadable file gracefully (it fell back to an empty record and carried on). #23749 replaced that with a hard error, landing in a code path that had relied on the forgiving behaviour since #15714.
Fix 2 — Quota fallback: don't throw away the tool result
What happened. When a quota error makes the CLI switch to a backup model, it deliberately stops the turn rather than silently continuing on a different model. That part is intentional.
The problem: it stopped without saving the tool's result. The model's tool call was already in the conversation, so this left a call with no matching result — exactly the pairing the API requires. Nothing retried it, and the flag that caused it only resets on your next fresh (non-continuation) message, so every request after that carried the broken pair. The visible symptom is the model continuing/autocompleting your next message instead of replying to it.
The fix. Save the tool result before stopping. The turn still does not auto-continue on the backup model — this only repairs the record.
One ordering detail. The stop now happens before the "steering hint" step (text you type while a tool is still running). That hint is built to be sent to the model, but this path never sends anything. Handling it here would both discard what you typed and write it into the conversation ahead of the tool result. Leaving it alone keeps the saved turn to just the tool result, and your hint goes out with the next message you actually send.
When this broke. Long-standing, not new: the early stop was added in #3662 with no result-saving. It was simply rare to hit until quota fallbacks became common.
Related Issues
None.
How to Validate
Both fixes ship with regression tests. The Fix 2 test was confirmed to fail without the production change and pass with it.
Relevant test names:
should fall back to the in-memory conversation when the file cannot be reloadedshould preserve an unreadable session file instead of destroying itshould record tool responses in history when the model was switched due to a quota errorManual check of
/compress:npm run build/compress.Chat history compressed from N to M tokens., and the next message still works — noFailed to load resumed session data from file.I ran this end-to-end in a real terminal against a local build:
/compressreportedChat history compressed from 14903 to 4615 tokensand the following turn responded normally.Regression check. No new failures. The
packages/core/src/{services,core}suites are already red onmainat this commit (pre-existing timeouts ingeminiChat.test.tsandlogger.test.ts); this branch does not add to them:main(baseline)Pre-Merge Checklist