fix(core): fail session deletion when file is missing#26184
fix(core): fail session deletion when file is missing#26184haosenwang1018 wants to merge 1 commit into
Conversation
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 improves the reliability of the session deletion process by ensuring that the system explicitly fails when a session file cannot be located. By moving away from silent success, the application can now correctly handle scenarios where the UI state might be out of sync with the underlying file system, allowing for better error reporting and user feedback. 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 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 counter productive. 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
|
There was a problem hiding this comment.
Code Review
This pull request modifies the deleteSession method in chatRecordingService.ts to throw an error when no matching session files are found, with corresponding updates to the test suite. Feedback highlights that this change breaks the idempotency of the deletion operation and introduces inconsistent behavior, as the method still returns successfully if the underlying directory is missing.
| if (matchingFiles.length === 0) { | ||
| throw new Error(`No session file found for ${sessionIdOrBasename}`); | ||
| } |
There was a problem hiding this comment.
This change breaks the idempotency of the deleteSession method and introduces inconsistent behavior regarding the 'session not found' state.
- Inconsistency: If the
chatsdirectory is missing, the method returns successfully (line 691). However, if the directory exists but no matching session file is found, it now throws an error. This makes the result of deleting a non-existent session dependent on whether other sessions have been created or if the directory was manually initialized. - Idempotency: Standard delete operations are typically idempotent. Throwing an error when a session is already deleted (or never existed) can cause issues for callers or UI logic that expects a successful outcome if the target resource is absent. If a user or script calls delete twice, the second call will now fail.
- Subagent Deletion: Subagent files are stored in subdirectories and do not use the
session-prefix (see line 404). BecausegetMatchingSessionFilesonly searches the top-level directory and filters for the prefix, it will never find subagent files. Attempting to delete a subagent session directly will now result in a hard error instead of a silent no-op, which may prevent the UI from cleaning up stale entries if it allows individual subagent deletion.
Consider whether the 'not found' case should remain a successful no-op to maintain idempotency, or if the search logic should be expanded to be more inclusive before failing.
…(5 PRs) - BerriAI/litellm#26764 merge-after-nits: multipart user_config/tags JSON decode - BerriAI/litellm#26769 request-changes: FuturMix add bundles unrelated deletions - google-gemini/gemini-cli#26184 merge-after-nits: deleteSession fail-loud on missing file - QwenLM/qwen-code#3737 merge-after-nits: drop strip-thoughts helpers, preserve reasoning_content - aaif-goose/goose#8781 needs-discussion: ACP per-connection inline→spawn ordering question
|
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. |
Summary
Fixes #26113.
When session deletion cannot find a matching session file,
ChatRecordingService.deleteSession()currently resolves successfully. The interactive/resumebrowser treats that as a successful deletion and removes the row from UI state even though the.jsonlfile remains on disk.This changes deletion to fail fast when the chats directory exists but no matching session file is found, so callers can surface the error instead of optimistically hiding the session.
Test plan
npm test --workspace @google/gemini-cli-core -- chatRecordingService.test.tsnpm run typecheck --workspace @google/gemini-cli-corenpx prettier --check packages/core/src/services/chatRecordingService.ts packages/core/src/services/chatRecordingService.test.ts