fix(acp): don't start a fresh chat before resuming, it poisons the session file - #28744
fix(acp): don't start a fresh chat before resuming, it poisons the session file#28744PranavMishra28 wants to merge 2 commits into
Conversation
…ssion file Closes google-gemini#28693. `loadSession` called `geminiClient.initialize()` immediately before `resumeChat()`. `initialize()` starts a chat with no resumed session data, so it initializes chat recording for a *fresh* conversation and writes a metadata header plus a `$set` checkpoint. The recording filename is keyed on the UTC minute and the session id prefix (`session-<UTC-minute>-<id[0:8]>.jsonl`), so a load issued in the same minute the session was created resolves to the session's own file. The checkpoint buries the existing conversation in the fold, `hasResumableContent` flips false, the session drops out of the listing, and the load fails with "No previous sessions found for this project" — after durably destroying the session's resumability, so every later load for that id fails too. No crash is required; it reproduces after a clean exit. Automated clients that reconnect immediately land in this window, while a human retrying minutes later lands in a different minute and sees it work, which is why it looked intermittent. `resumeChat` already does everything `initialize()` does, including `updateTelemetryTokenCount()`, so the call was redundant as well as destructive and is simply removed. Tests: `resumes without first starting a fresh chat for the same session id` asserts the client is resumed directly and never initialized for a fresh chat. It fails on the parent commit with "expected spy to not be called at all, but actually been called 1 times". Full ACP suite 92 passed across 10 files, eslint --max-warnings 0 and prettier clean, npm run typecheck exit 0. Developed with Claude Code; reviewed and tested by Pranav before marking ready for review.
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 a critical issue where loading a chat session could inadvertently corrupt the session file, rendering it unresumable. By removing a redundant initialization step that incorrectly triggered fresh-conversation logic, the fix ensures that sessions are resumed directly without modifying their internal structure. This change improves the reliability of session persistence and prevents intermittent failures during rapid session reconnection. 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
|
|
📊 PR Size: size/M
|
There was a problem hiding this comment.
Code Review
This pull request removes a redundant and destructive call to geminiClient.initialize() prior to calling geminiClient.resumeChat() in AcpSessionManager. The redundant call was causing issues by starting a fresh chat and overwriting session metadata when resuming a session within the same minute. A detailed explanatory comment has been added to the code, and a new unit test has been introduced in acpResume.test.ts to verify that initialize() is not called during session resumption. There are no review comments, and I have no additional feedback to provide.
|
correction on my own PR: this removal is necessary but it is not sufficient, and I want that on the record before anyone merges it thinking the path is closed.
my test could not catch it because the reason it still matters: I opened #28767 for the underlying cause — once that lands, the ACP fix is a small follow-up: resolve the session before |
|
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. |
|
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. |
Partially addresses #28693 — see the correction below; this removes one of two fresh-chat starts on the load path, not both.
Problem
loadSessioncalledgeminiClient.initialize()immediately beforeresumeChat():initialize()isthis.chat = await this.startChat()with noresumedSessionData, soChatRecordingService.initialize(undefined)takes the fresh-conversation branch and writes a metadata header plus a$setcheckpoint.The recording filename is keyed on the UTC minute and the session id prefix —
session-<UTC-minute>-<id[0:8]>.jsonl(chatRecordingService.ts:495-519). So when the load happens in the same UTC minute the session was created, that append lands in the session's own conversation file. The checkpoint buries the conversation in the fold,hasResumableContentflips false, the session is filtered out of the listing, and the load fails withNo previous sessions found for this project— after durably destroying the session's resumability, so every later load for that id fails too.No crash is required; it reproduces after a clean exit. Automated clients that reconnect immediately land in this window, while a human retrying minutes later lands in a different minute and sees loads succeed — which is why it read as intermittent or environment-dependent.
Credit to @realtonyyoung for the repro and for re-root-causing it; I'd earlier ruled out four candidate mechanisms on the issue and the load path was what was left.
Approach
Remove the
initialize()call.resumeChatalready does everything it does — it setsthis.chatviastartChat(history, resumedSessionData)and callsupdateTelemetryTokenCount()— so the call was redundant as well as destructive. One line out, plus a comment recording why it must not come back.This deliberately does not change the filename scheme. Minute-precision plus an 8-char id prefix is a real collision surface, but widening it would change where existing sessions are looked up, and it isn't needed here: nothing should be writing a fresh-conversation header during a load in the first place.
How tested
resumes without first starting a fresh chat for the same session idasserts the client is resumed directly with the resolved session data and never initialized for a fresh chat. It fails on the parent commit withexpected "spy" to not be called at all, but actually been called 1 times.eslint --max-warnings 0andprettier --checkclean;npm run typecheckexit 0. The repo'spre-commithook ran clean on the staged files.Developed with Claude Code; reviewed and tested by Pranav before marking ready for review.