Repository navigation
perf: cut pricing refresh traffic with gzip and ETag revalidation - #1565
wishworldbetter wants to merge 7 commits into
Conversation
The pricing fetch downloaded the full LiteLLM table uncompressed on every run: ureq was built without its gzip feature, so no Accept-Encoding was sent (1.67MB on the wire), and nothing revalidated, so polling callers (statuslines, collectors) paid that full price every time. - Enable ureq's gzip feature: the same fetch now transfers ~82KB. - Keep each response body on disk next to its ETag (best-effort, in the spirit of the statusline cache) and send If-None-Match on later runs: an unchanged document is a bodyless 304, ~zero transfer. GitHub serves the pricing table with a strong ETag, so this is the common case. - Serve the cached body when revalidation fails, so a network hiccup degrades to yesterday's validated table instead of embedded-only data. Measured on a live setup polling once a minute: 1764KB per run before, 162KB cold / ~1KB warm (304) after. Co-Authored-By: Claude Fable 5 <[email protected]>
|
This PR was auto-closed. Only contributors approved with Maintainers review auto-closed issues and reopen worthwhile ones. Issues that do not meet the quality bar in CONTRIBUTING.md may not be reopened or receive a reply. If a maintainer replies See CONTRIBUTING.md. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pricing fetch enables gzip and uses disk-backed ETag caching. It bounds decompressed and cached reads, writes cache entries atomically, and commits cache writes only after valid pricing data loads. ChangesPricing fetch cache
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR reduces pricing refresh traffic and improves offline resilience, but a corrupted cached response can persist across revalidations and repeatedly cause fallback behavior. The change is mergeable with explicit owner awareness and follow-up to validate or evict unusable cached bodies. Sequence Diagram(s)sequenceDiagram
participant PricingLoader
participant fetch_json
participant PricingEndpoint
participant CacheEntry
PricingLoader->>fetch_json: Request pricing JSON
fetch_json->>CacheEntry: Read cached body and ETag
fetch_json->>PricingEndpoint: Send conditional request
PricingEndpoint-->>fetch_json: Return response body or 304
fetch_json-->>PricingLoader: Return FetchedJson
PricingLoader->>PricingLoader: Parse and validate pricing data
PricingLoader->>CacheEntry: Commit validated body and ETag
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
rust/Cargo.toml— enables ureq'sgzipfeature soAccept-Encoding: gzipis sent and compressed responses are transparently decodedrust/crates/ccusage/src/http.rs— adds an on-disk ETag cache under$TMPDIR/ccusage-http-cache/withIf-None-Matchrevalidation, 304 handling, and network-failure fallback to the last validated body; includes FNV-1a hashing for collision-resistant filesystem-safe stems and 3 unit tests
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
The body and ETag lived in two files written back-to-back, so two concurrent runs (a statusline plus a polling collector) could interleave and pair one run's body with the other's ETag — revalidation would then vouch for a body that does not match the validator until the next real upstream change. Store the pair in a single file (first line ETag, rest body), written to a per-process temp file and renamed into place: one atomic rename swaps both together, so readers see the old pair or the new pair, never a mix. Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
> ✅ No new issues found.
Reviewed changes
- Single-file cache with atomic rename — replaced the separate
.body/.etagfiles with one.cachefile (ETag as first line, body follows). Writes go through a per-process temp file (stem.tmp<PID>) andrenameinto place, so concurrent readers observe either the old or the new pair atomically, never a body paired with another run's ETag. - Truncated-entry guard — added a test case for missing newline: a cache file with no
\nreturnsNoneinstead of being used for revalidation. - Expanded atomic test —
cache_write_replaces_pair_atomically_and_cleans_upverifies rewrite correctness, multi-line body round-trip, and that no temp files linger after a successful write.
<!--
Pullfrog review metadata. These findings were written against 2c80ac2;
if commits have landed on pricing-http-cache since, treat every specific bug, file, or
line callout as POTENTIALLY STALE and re-diff before acting on it.
- Mode: IncrementalReview (delta against prior pullfrog review)
- Files reviewed: 3
- Commits reviewed: 2
- Base: main (eda5e50)
- Head: pricing-http-cache (2c80ac2)
- Reviewed commits:
- Prior pullfrog review: 80e189e (#1565 (review))
-->
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![cubic-dev-ai[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Findings from an adversarial review of the previous two commits: - ureq's `.limit()` sits below the gzip decoder, so with gzip enabled it only capped the compressed stream — a hostile origin could balloon memory ~1000x through it. Read via `.reader()` with `take()` so the bound applies to what actually lands in memory. - A 4xx other than 429 now surfaces as an error instead of silently serving the stale cache forever: if the URL moves or dies, the embedded snapshot (refreshed with every release) wins over a cache frozen at the last successful fetch. 5xx/429/transport errors still serve the last validated body, now with a WARN at log level 4+. - A body that dies mid-read (timeout, truncated stream, oversize) gets the same stale fallback as a failed request, as the comment promised. - Temp file names include a per-call counter besides the pid, and a failed write cleans its half-written temp file up. - The cache moved from the temp dir to XDG_CACHE_HOME/~/.cache: /tmp is world-shared on Linux, so another local user could plant entries or symlink the predictable temp path. - Docs: the gzip variant of the pricing table carries a weak ETag, not the strong one served for identity. Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Cache directory moved to XDG_CACHE_HOME — switched from
/tmpto~/.cache/ccusage/http-cache(viaXDG_CACHE_HOMEorHOME), with temp dir as fallback;cache_dir_under()rejects relative paths per the basedir spec - 4xx status-code differentiation — non-429 4xx codes are now hard errors surfaced to the embedded-snapshot fallback; 5xx/429/transport errors still fall back to the last validated cached body
- Stale-cache logging —
log_stale_cache_use()mirrors the pricing refresh WARN pattern, gated onccusage_core::log_level() >= 4 - Decompressed body-size bounding — replaced ureq's
.limit()(compressed-stream cap only) withreader().take(PRICING_FETCH_MAX_BYTES + 1)to guard against gzip decompression bombs - Body-read failure fallback — mid-read errors (timeout, truncation, oversize) now fall back to the cached response instead of failing
- Per-call temp file naming — added
AtomicU64sequence counter so two threads in the same process racing on the same URL use distinct temp files - Write-failure cleanup — half-written temp files (e.g. disk full) are now removed
- New test —
cache_dir_prefers_xdg_then_home_then_tempcovers relative-path rejection and fallback order
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ore caching Addresses both findings from the cubic review: - The permanent-failure branch is now an allowlist (404, 410) instead of "every 4xx but 429": 408, 421, 425 and friends can be passing conditions, so they serve the last validated cached copy like 5xx and transport errors do, instead of dropping to the embedded snapshot. - A 200 body must parse as JSON before it may replace the cached copy. The stale fallback is only sound while the copy on disk is known-good; a hijacked or errored 200 (captive portal, CDN error page) returning non-JSON with an ETag would otherwise evict the last good table. The invalid body is still handed to the caller unchanged. Co-Authored-By: Claude Fable 5 <[email protected]>
![coderabbitai[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/crates/ccusage/src/http.rs (1)
65-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd fixture-backed tests for revalidation behavior.
The current tests do not call
fetch_json_with_cache_dir. Add loader tests that verifyIf-None-Matchis sent, HTTP 304 returns the cached body, and transient request or read failures preserve the cached body. Run the applicablejusttest recipe after adding them.As per coding guidelines, “Prefer fixture-backed parser and loader tests for Rust code,” and “Use
justas the single entry point for repository tasks and consultjust --listfor available recipes.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/ccusage/src/http.rs` around lines 65 - 95, Add fixture-backed loader tests that exercise fetch_json_with_cache_dir, covering If-None-Match request headers, HTTP 304 returning the cached body, and transient request or response-read failures falling back to the cached body. Reuse the existing HTTP fixture/test infrastructure and add the tests near the current loader tests, then run the applicable recipe discovered via just --list.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/crates/ccusage/src/http.rs`:
- Around line 137-145: Update CacheEntry::read to validate the cached body with
looks_like_json before accepting an entry, including the valid-ETag path. Reject
and bypass malformed or truncated cache contents so 304 and fetch-failure
handling can use the next fallback source, while preserving valid cached
responses.
---
Nitpick comments:
In `@rust/crates/ccusage/src/http.rs`:
- Around line 65-95: Add fixture-backed loader tests that exercise
fetch_json_with_cache_dir, covering If-None-Match request headers, HTTP 304
returning the cached body, and transient request or response-read failures
falling back to the cached body. Reuse the existing HTTP fixture/test
infrastructure and add the tests near the current loader tests, then run the
applicable recipe discovered via just --list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fbc4c770-93c6-49c3-b8e8-553761dc3ed8
📒 Files selected for processing (1)
rust/crates/ccusage/src/http.rs
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Narrowed permanent HTTP failures to 404/410 only — replaced the catch-all non-429 4xx error branch with
is_permanent_http_failure(), so 400, 401, 403, 408, 421, and 425 now fall back to the cached body instead of surfacing hard errors. A 404 or 410 still signals a dead URL and escalates to the embedded snapshot. - JSON-gated cache writes — added
looks_like_json()guard so only responses that parse as valid JSON can evict the on-disk cache; a hijacked 200 (captive portal, CDN error page) still reaches the caller but can't corrupt the cache. - Two new tests —
permanent_failure_is_a_short_allowlistexhaustively checks the classification table across 12 status codes, andonly_valid_json_may_replace_the_cached_copycovers valid objects/arrays alongside HTML, truncated JSON, and empty strings.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
ccusage
@ccusage/ccusage-darwin-arm64
@ccusage/ccusage-darwin-x64
@ccusage/ccusage-linux-arm64
@ccusage/ccusage-linux-x64
@ccusage/ccusage-win32-x64
commit: |
|
@wishworldbetter Thanks for the PR! I ran two independent review passes over this branch and want to share both the findings and a direction decision before you invest more time in fixes. Overall judgmentThe core of this PR is worth merging: today every non-offline run re-downloads the full ~1.7MB LiteLLM JSON, so gzip plus an ETag/304 path is a real win. However, most of the review findings below stem from one added feature: serving the stale cached body as a fallback when the network fails. That fallback competes with the embedded pricing snapshot we already ship (which is the intended failure/offline fallback) and drags in a lot of subtle cache-semantics problems — for example:
Rather than patching each of these, I'd like to reduce the scope:
That removes the findings above by design, keeps the failure behavior identical to what users have now, and shrinks the diff to a reviewable size. Findings that still apply after the scope cut
If you'd rather I take over the scope reduction, happy to do it — otherwise, looking forward to the update. Thanks again! |
|
@pullfrog do you want to work on it? |
|
Implemented and committed the requested PR #1565 review direction as The working tree is clean, but publishing is blocked by repository permissions. Both push attempts failed with: Task list (5/5 completed)
|
Implements the maintainer review direction on ccusage#1565: the cache no longer stands in for the network, it only answers If-None-Match revalidation. - Any request failure, non-200/304 status, or body-read failure now returns an error so the embedded pricing snapshot takes over, exactly as main behaves without a cache. The stale-on-error fallback and its permanent/transient status split are gone. - The cache write is deferred to the pricing loaders: fetch_json returns a FetchedJson whose commit() runs only after LiteLLM or models.dev schema parsing accepts the body, so a JSON-shaped error envelope can no longer evict the last validated pair, and the duplicate looks_like_json parse is gone. - Caching is disabled entirely when no absolute per-user cache directory can be resolved instead of falling back to a world-shared temp dir, and the home lookup goes through ccusage_core::home::home_dir so Windows profiles resolve. - Cache-file reads are bounded like the network read. - Tests use ccusage-test-support fixtures, platform-safe absolute paths, and a scripted local HTTP server covering the 200-commit-304 cycle, failure with a cached copy present, and the cache-less fetch. Co-Authored-By: Claude Fable 5 <[email protected]>
|
@ryoppippi The scope reduction is done in 24d79fb: the cache now only answers If-None-Match revalidation, and every fetch failure surfaces as an error so the embedded snapshot takes over exactly as on The six remaining findings are addressed as well:
Verified locally against the pinned toolchain: all 587 workspace tests pass, |
There was a problem hiding this comment.
Important
The new deferred validation can persist a syntactically valid but unusable models.dev response as a successful cache entry.
Reviewed changes
The incremental delta was reviewed against the prior Pullfrog review, including the cache's 304-only behavior and the deferred commit plumbing shared by LiteLLM and models.dev.
- Scoped cache reuse — changed fetch failures to surface to embedded pricing while retaining the disk body only for successful 304 revalidation.
- Deferred cache writes — returned
FetchedJsonvalues from the pricing fetcher and committed new bodies only after the pricing loaders accepted them. - Expanded loader coverage — added tests for commit verdicts, cache-less operation, revalidation headers, and failed refreshes.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Luna (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A models.dev body that parses but prices nothing — `{}`, or providers
whose models all lack usable costs — was retained as the process-wide
map and committed to the HTTP cache, so a later 304 kept serving the
empty document until upstream changed the ETag. Require at least one
loaded entry before retaining the map or running the commit hook,
matching how the LiteLLM loader already gates its commit on
loaded_count. Raised by pullfrog and cubic on ccusage#1565.
Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="rust/crates/ccusage-core/src/pricing.rs">
<violation number="1" location="rust/crates/ccusage-core/src/pricing.rs:1472">
P2: When an existing cache contains `{}`, a 304 repeatedly feeds that body into this rejection and returns `None` without invalidating its ETag. Invalidate the unusable cache entry or force an unconditional retry after rejecting a 304 body.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // the process and committing it would keep a 304 serving it across runs. | ||
| if map | ||
| .load_models_dev_json_missing(fetched.body()) | ||
| .unwrap_or(0) |
There was a problem hiding this comment.
P2: When an existing cache contains {}, a 304 repeatedly feeds that body into this rejection and returns None without invalidating its ETag. Invalidate the unusable cache entry or force an unconditional retry after rejecting a 304 body.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At rust/crates/ccusage-core/src/pricing.rs, line 1472:
<comment>When an existing cache contains `{}`, a 304 repeatedly feeds that body into this rejection and returns `None` without invalidating its ETag. Invalidate the unusable cache entry or force an unconditional retry after rejecting a 304 body.</comment>
<file context>
@@ -1464,7 +1464,14 @@ where
+ // the process and committing it would keep a 304 serving it across runs.
+ if map
+ .load_models_dev_json_missing(fetched.body())
+ .unwrap_or(0)
+ == 0
+ {
</file context>
There was a problem hiding this comment.
Important
The new zero-entry guard can permanently poison an existing models.dev cache entry. Please invalidate or bypass unusable 304 bodies before merging.
Reviewed changes
The incremental review covers the new models.dev zero-entry validation and its regression test added after the prior Pullfrog review.
- Rejected empty models.dev refreshes — treated documents that load zero usable pricing entries as failed refreshes, preventing them from being retained in process or committed to disk.
- Added zero-entry regression coverage — verified that
{}is rejected and does not invoke the deferred cache commit hook.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Luna (free via Pullfrog for OSS) ｜ 𝕏
| // empty object parses, but retaining it would pin an unpriceable map for | ||
| // the process and committing it would keep a 304 serving it across runs. | ||
| if map | ||
| .load_models_dev_json_missing(fetched.body()) |
There was a problem hiding this comment.
When {} was already written by an older loader, this new zero-entry check rejects it, but the 304 path in http.rs keeps returning the cached body and ETag. Every refresh therefore repeats the 304 and returns to the embedded fallback without ever issuing an unconditional request, so the cache remains poisoned across runs. Invalidate the unusable entry or retry once without If-None-Match after rejecting the 304 body.
Technical details
# Recover from unusable 304 cache bodies
## Affected sites
- `rust/crates/ccusage-core/src/pricing.rs:1471` — the new zero-entry gate rejects a cached `{}` document but does not communicate that the HTTP cache entry must be discarded.
- `rust/crates/ccusage/src/http.rs:76-80` — a `304` returns the cached body without changing or invalidating its ETag.
## Required outcome
- When a `304` body fails the models.dev usable-entry validation, the next attempt must be able to fetch a fresh `200` response instead of sending the same validator forever.
## Suggested approach
- Invalidate the unusable cache pair or retry the request once without `If-None-Match` when the cached body is rejected.
ryoppippi
left a comment
ryoppippi
left a comment
There was a problem hiding this comment.
Thanks for incorporating the scope reduction from the earlier discussion. I found one remaining correctness issue before this can merge.
[P2] Invalidate or bypass a cached body that fails validation after a 304
fetch_json_with_cache_dir returns FetchedJson::new(cached.body) when the server responds with 304. The pricing loaders now correctly reject zero usable entries, including {}, and do not call commit, but there is no way to invalidate the existing cache or ETag after that rejection. As a result, a cache written by an earlier revision—or a malformed/truncated cache that passes the cache-file framing checks—can loop forever:
If-None-Match -> 304 -> same invalid body -> reject
The next process repeats the same sequence, so models.dev pricing never recovers until the upstream ETag changes.
Please either add a rejection/invalidation hook so the loader can remove the cache after rejecting a 304 body, or retry once without If-None-Match when the cached 304 body fails validation. Please also add a regression test that seeds an existing {} cache, returns 304, and verifies that the next request bypasses the ETag and can refresh with a 200 body.
The gzip/ETag/304 direction and the removal of the stale-on-error fallback look good; this is the remaining blocker from my side.
|
Superseded by #1672, which implements the safer ETag refresh and cache validation path. Closing this stale alternative. |
ryoppippi
added
the
triage:resolved
|
Historical audit: this pull request was auto-closed by the legacy contributor gate. That closure did not assess technical importance. Audit result: resolved. A later merged change or the current |
2 participants
Closes #1564
What
The LiteLLM pricing fetch downloaded the full table uncompressed on every non-offline run:
ureqwas built without its gzip feature, so noAccept-Encodingwas sent (1.7 MB on the wire), and nothing revalidated, so polling callers (statuslines, usage collectors) paid that full price every run.gzipfeature — the same fetch now transfers ~82 KB.If-None-Matchon later runs: an unchanged document is a bodyless 304, ~zero transfer. GitHub serves the pricing table with a strong ETag, so this is the common case.The models.dev fetch goes through the same fetcher and gets the same treatment for free.
Measured
Real setup polling once a minute (curl cross-check: identity 1.7 MB / gzip 82 KB / If-None-Match match → HTTP 304, empty body):
Tested
cargo test -p ccusage53/53, fmt and clippy clean.$TMPDIR/ccusage-http-cache/*.body/.etag; warm run leaves the pair untouched (304 path) and produces identical report output.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Cuts pricing refresh bandwidth with gzip and ETag revalidation and scopes the HTTP cache to 304-only use. Previously we downloaded ~1.7MB uncompressed every run and could serve stale cache on errors; now gzip + If-None-Match cut transfer, and any fetch failure surfaces so the embedded snapshot takes over.
ureqgzipand bounds decompressed reads; sends If-None-Match and serves the cached body only on HTTP 304; any non-200/304 status or read error returns an error (no stale-on-error fallback).Written for commit 3031019. Summary will update on new commits.
Summary by CodeRabbit
Performance
Reliability