Repository navigation
fix(api): publish HTTP cache entries without in-place writes - #263
Merged
williammartin merged 2 commits intoSep 11, 2026
Merged
Conversation
williammartin
changed the title
williammartin
force-pushed
the
fix/atomic-api-cache-store
branch
from
September 9, 2026 15:16
206befd to
b0feba3
Compare
Write cache entries to a temp file in the same directory and rename into place so concurrent gh processes and mid-write crashes cannot leave a partial or interleaved cache file. Fixes cli#252
williammartin
force-pushed
the
fix/atomic-api-cache-store
branch
from
September 9, 2026 15:18
b0feba3 to
6e1b7f1
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The retained cache-directory override lost its only regression test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Publishes HTTP cache entries via temporary files and renames to prevent partial or corrupted reads.
Changes:
- Adds atomic-style cache publication with cleanup and body replay.
- Retries transient Windows rename conflicts.
- Adds concurrency, failure, and platform-specific tests.
File summaries
| File | Description |
|---|---|
pkg/api/cache.go |
Stages and renames cache responses. |
pkg/api/cache_test.go |
Adds cache publication tests. |
pkg/api/cache_rename_windows.go |
Implements bounded Windows retries. |
pkg/api/cache_rename_windows_test.go |
Tests Windows rename behavior. |
pkg/api/cache_rename_other.go |
Uses direct rename elsewhere. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
williammartin
requested review from
sergiou87
and removed request for
BagToad and
babakks
September 9, 2026 15:35
niik
approved these changes
Sep 11, 2026
williammartin
force-pushed
the
fix/atomic-api-cache-store
branch
from
September 11, 2026 15:20
6e1b7f1 to
4e421f8
Compare
Member
|
Thanks @baiyuxi930826, I used your work as a base to make some improvements. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Failed body reads can return a silently truncated response to callers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Keep Windows rename retries bounded and platform-specific, simplify response serialization and cleanup, and cover cache publication through public-client and Windows filesystem scenarios. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 6e0a2bb3-4485-45bd-bcbc-f426f9edbb7c
williammartin
force-pushed
the
fix/atomic-api-cache-store
branch
from
September 11, 2026 15:43
4e421f8 to
3707f4d
Compare
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation addresses cross-process cache corruption with focused cleanup, error handling, and platform-specific tests.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #252.
Description
go-gh caches API responses on disk so later requests can reuse them. Previously, saving a response truncated the existing cache file before writing its replacement. An in-process mutex cannot coordinate separate
ghprocesses, so another process could read an incomplete response or overwrite the same file concurrently. A failed or interrupted write could also destroy a usable entry.Write the response to a temporary file in the same directory, close it successfully, and then rename it into place. Failed response writes leave the existing entry untouched. On Windows, briefly retry file-sharing conflicts before giving up on caching; the caller still receives the API response.
How did you test this change?
Not tested manually. I exercised this library change only through automated tests; I did not build and run
ghagainst this branch.Key points
fsyncis added: this cache does not promise durability across power loss or operating-system crashes.Notes for reviewers
Start with
fileStorage.storeinpkg/api/cache.go, then the rename helper and its Windows error classification inpkg/api/cache_rename*.go. The publication and rename tests describe the failure, concurrency, and retry scenarios.Issue #252 supplies the cache-corruption context motivating the change and we also saw it in cli/cli#14394
Authorship and follow-up
Who wrote this:
The original patch is by @baiyuxi930826. Copilot wrote the follow-up implementation and this description under human direction.
Who answers review comments: