Skip to content

fix(github): avoid caching empty release assets#9616

Merged
jdx merged 4 commits into
jdx:mainfrom
risu729:fix/github-empty-release-assets-cache
May 10, 2026
Merged

fix(github): avoid caching empty release assets#9616
jdx merged 4 commits into
jdx:mainfrom
risu729:fix/github-empty-release-assets-cache

Conversation

@risu729
Copy link
Copy Markdown
Contributor

@risu729 risu729 commented May 5, 2026

Fixes #9520.

Summary

  • add a cache initializer path that can return rejected values without storing them
  • skip GitHub release-detail cache writes for releases whose asset list is empty
  • ignore already-cached empty release details and refetch before using them

Tests

  • cargo fmt --check
  • cargo test github::tests
  • cargo test cache::tests

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a conditional caching mechanism in CacheManager through the new get_or_try_init_async_if method. This functionality is applied to GitHub release fetching to prevent the caching of releases with empty asset lists, ensuring that incomplete data is not persisted. Feedback suggests refactoring the cache lookup logic to reduce code duplication and highlights a potential efficiency issue regarding the lack of concurrency synchronization during redundant fetch calls.

Comment thread src/cache.rs Outdated
Comment thread src/cache.rs
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR fixes a bug where GitHub releases with empty asset lists (e.g. due to GitHub propagation delays) were being cached, causing subsequent install attempts to fail without re-fetching. It introduces a conditional caching path on CacheManager and wires it into both get_release and get_release_for_url.

  • Adds get_or_try_init_async_if to CacheManager: like get_or_try_init_async but skips populating the in-memory and on-disk caches when the fetched (or disk-cached) value fails a caller-supplied predicate, while still returning the value to the caller.
  • Updates get_release and get_release_for_url to use the new method with should_cache_release, which rejects releases whose assets list is empty; both functions now also return an owned GithubRelease directly instead of cloning a &T reference.

Confidence Score: 5/5

The change is narrowly scoped to the GitHub release fetch path and is safe to merge.

Both modified functions are exercised by new tests covering the disk-cache bypass, fresh fetch, and in-memory cache hit. The logic in get_or_try_init_async_if correctly avoids setting either OnceCell with a rejected value. The return-type change from Result<&T> (with an explicit .clone()) to Result preserves the public API for all callers.

No files require special attention.

Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/github-empt..." | Re-trigger Greptile

Comment thread src/cache.rs
Comment thread src/cache.rs
@risu729

This comment was marked as outdated.

@risu729 risu729 marked this pull request as ready for review May 5, 2026 19:01
@jdx jdx merged commit 6fed551 into jdx:main May 10, 2026
34 checks passed
@risu729 risu729 deleted the fix/github-empty-release-assets-cache branch May 10, 2026 13:31
mise-en-dev added a commit that referenced this pull request May 11, 2026
### 🚀 Features

- **(cli)** add minimum release age flag to lock and ls-remote by
@risu729 in [#9269](#9269)
- **(config)** add run field for hooks by @risu729 in
[#9718](#9718)
- **(github)** add native oauth token source by @jdx in
[#9654](#9654)
- **(oci)** scope build to project config by default by @jdx in
[#9766](#9766)
- add support for prefixed latest version queries in outdated checks by
@roele in [#9767](#9767)

### 🐛 Bug Fixes

- **(activate)** guard bash chpwd hook under nounset by @risu729 in
[#9716](#9716)
- **(backend)** date-check latest stable fast path by @risu729 in
[#9650](#9650)
- **(config)** parse core tool options consistently by @risu729 in
[#9742](#9742)
- **(exec)** propagate __MISE_DIFF so nested mise recovers pristine PATH
by @jdx in [#9765](#9765)
- **(forgejo)** include prereleases when opted in by @risu729 in
[#9717](#9717)
- **(github)** avoid caching empty release assets by @risu729 in
[#9616](#9616)
- **(java)** resolve lockfile URLs from metadata by @risu729 in
[#9719](#9719)
- **(lock)** cache unavailable github attestations by @risu729 in
[#9741](#9741)
- **(pipx)** preserve options when reinstalling tools by @risu729 in
[#9663](#9663)
- **(python)** skip redundant lockfile provenance verification by
@risu729 in [#9739](#9739)
- **(vfox)** run pre_uninstall hook by @risu729 in
[#9662](#9662)

### 🚜 Refactor

- **(schema)** extract tool options definition by @risu729 in
[#9649](#9649)

### ⚡ Performance

- **(aqua)** bake rkyv aqua package blobs by @risu729 in
[#9535](#9535)

### 📦️ Dependency Updates

- lock file maintenance by @renovate[bot] in
[#9773](#9773)

### 📦 Registry

- add vector
([github:vectordotdev/vector](https://github.com/vectordotdev/vector))
by @kquinsland in [#9761](#9761)
- add oc and openshift-install (http backend) by @konono in
[#9669](#9669)

### New Contributors

- @konono made their first contribution in
[#9669](#9669)
- @kquinsland made their first contribution in
[#9761](#9761)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants