fix: warn on stderr when discovery cache write fails#191
fix: warn on stderr when discovery cache write fails#191Bortlesboat wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 70361fe The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 error visibility by ensuring that failures during the discovery document cache write operation are no longer silently ignored. Instead, a warning is now printed to stderr, providing users with crucial information to diagnose cache persistence issues, such as full disks or permission problems. Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Bortlesboat
force-pushed
the
fix/discovery-cache-warning
branch
from
0bce00a to
cbaab50
Compare
![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 correctly addresses an issue where a cache write error was silently discarded. Logging the error to stderr is the right approach. However, the current implementation of the eprintln! macro mixes positional and named format arguments, which will cause a compilation error. I've left a specific comment with a suggested fix. With that change, the PR should be good to merge.
jpoehnelt
added
area: discovery
cla: yes
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where an error during the discovery cache write was silently discarded. The change correctly implements the intended behavior by printing a warning to stderr, including the file path and the error message. This will help users diagnose potential cache persistence issues. The implementation is sound and aligns with the project's existing error handling patterns.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 56.21% 57.53% +1.32%
==========================================
Files 38 38
Lines 13853 14185 +332
==========================================
+ Hits 7788 8162 +374
+ Misses 6065 6023 -42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bortlesboat
force-pushed
the
fix/discovery-cache-warning
branch
from
f2f9e74 to
70361fe
Compare
google-cla
bot
added
cla: no
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where an error when writing the discovery document cache was silently ignored. The change introduces a warning message to stderr, which is a good improvement for diagnostics. I've added one comment regarding a potential panic when writing to stderr, suggesting a more robust way to handle the warning in a 'non-fatal' manner.
| if let Err(e) = std::fs::write(&cache_file, &body) { | ||
| // Non-fatal: just warn via stderr-safe approach | ||
| let _ = e; | ||
| eprintln!("warning: failed to write discovery cache to {}: {}", cache_file.display(), e); | ||
| } |
There was a problem hiding this comment.
While eprintln! is a good step up from silently ignoring the error, it can panic if writing to stderr fails (e.g., if the pipe is closed). Given the operation is described as 'non-fatal', a panic would be an undesirable outcome. To create a truly 'stderr-safe' warning that won't panic, it's better to use writeln! with std::io::stderr() and ignore the Result. This ensures that a failure to write the warning doesn't crash the application.
| if let Err(e) = std::fs::write(&cache_file, &body) { | |
| // Non-fatal: just warn via stderr-safe approach | |
| let _ = e; | |
| eprintln!("warning: failed to write discovery cache to {}: {}", cache_file.display(), e); | |
| } | |
| if let Err(e) = std::fs::write(&cache_file, &body) { | |
| use std::io::Write; | |
| let _ = writeln!(std::io::stderr(), "warning: failed to write discovery cache to {}: {}", cache_file.display(), e); | |
| } |
3 participants
Summary
The comment on the cache write says "Non-fatal: just warn via stderr-safe approach" but the error was actually assigned to
_and silently discarded. Now prints a warning to stderr with the file path and error, so users can diagnose cache persistence issues.Changes
src/discovery.rs: Replacelet _ = e;witheprintln!warning that includes the cache file path and error messageTest plan
cargo test,cargo clippy,cargo fmt