fix(storage): fail closed when the spend journal cannot be trusted - #552
Merged
Conversation
Journal replay swallowed every I/O and parse error and returned an empty SpendData, so a corrupt, truncated, or unreadable journal started the process at $0 spent and silently reopened an exhausted budget. That is the LiteLLM fail-open ADR-0030 exists to prevent, reached through the file layer rather than Redis. Replay is now fallible. A genuinely absent journal still means 'nothing spent'; a journal that exists but cannot be trusted is fatal at startup and a denial on the per-tenant request path.
Destynova2
enabled auto-merge (squash)
September 1, 2026 10:38
Destynova2
added a commit
that referenced
this pull request
Sep 1, 2026
…able (#556) Fixes the red `Coverage` job currently blocking #547, #552 and #555. ## What broke `cargo llvm-cov` runs every integration test in **one process**; nextest gives each test its own. So a latent bug stayed invisible under the default suite and only surfaced in the Coverage job, on every open PR at once — including a docs-only one, which is what made it look like flaky infra rather than a real defect. The bug: `install_metrics_recorder` failure was propagated with `?`. `metrics::set_global_recorder` is a process-wide one-shot, so the **second** server spawned in a test binary failed to boot, and the test that waits for `/health` timed out: ``` server did not become healthy at http://127.0.0.1:40099/health ``` Adding a second server-spawning test (the conformance gate, #554) is what tipped it over. ## Why this is a real bug, not a test-only annoyance ADR-0030 classifies metrics as an **observing** dependency: failure must degrade, never block. The ADR even warns about this direction explicitly — "an audit or metrics failure that could block a request would itself be a contract violation in the other direction". The code did the opposite: an observability failure took down the serving path. Notably the ADR's metrics row cited *no enforcing code*, unlike every other row. That empty cell was the tell. It now points at the call site that actually honours it. Startup logs and continues. `/metrics` still renders from `metrics_handle` either way; what is lost is the recording of new samples, not the ability to serve. ## Verification Reproduced and fixed with the exact CI command, not a proxy: - `cargo llvm-cov --lcov` on `main`: **FAILS** (`responses_e2e_round_trip`, server never healthy). - Same command with this fix: **272 passed**. - `cargo test --test lib -- responses_e2e conformance` (both servers, one process): fails before, passes after. Plus `cargo nextest run` 1730 passed, clippy `-D warnings` clean. The regression assertion lives in `every_metric_family_is_described_without_otel`, the one test that legitimately owns the global recorder — a standalone test would race it for the single install slot.
Contributor
Mutation testing (PR diff sample)Informational — never blocks merge. Full matrix runs on main.
Legend: clean (no survivors), missed (inspect artifact), timed-out (25 min cap reached). Artifact: mutants-pr-results-00b5d8d2eb8f9ed97384b1829b1da1cc8bf11e10. |
Destynova2
added a commit
that referenced
this pull request
Sep 2, 2026
## 🤖 New release * `grob`: 0.36.101 -> 0.36.102 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [0.36.102](v0.36.101...v0.36.102) - 2026-09-02 ### Fixed - *(storage)* fail closed when the spend journal cannot be trusted ([#552](#552)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
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.
Closes #485.
ADR-0030 classifies the budget as an authorizing dependency: if it cannot be evaluated, the request must be blocked. The ADR's own audit note said the implementation already satisfied this "on every row", because the spend counter is in-memory and has no Redis to fail open on.
That in-memory counter is seeded by replaying
spend/YYYY-MM.jsonlat startup, and the replay swallowed every error:So a truncated journal (crash mid-write), a corrupt line, or a permissions problem made grob start at $0 spent and silently reopen an already-exhausted budget. Same failure mode LiteLLM shipped, reached through the file layer instead of Redis, and invisible: no error, just a counter that reads zero.
Change
replay_current,replay_for_tenantandreplay_all_tenantsnow returnResult. The distinction that matters is absent vs untrustworthy:NotFoundstaysOk(default)— a fresh install has genuinely spent nothing.GrobStore::openpropagates it, so a damaged journal refuses startup instead of serving on bad numbers. On the per-tenant path, which can hit the journal at request time on a cache miss,check_tenant_budgetmaps the failure to aBudgetErrorand denies.load_spendwas split: the global read is a pure cache hit and stays infallible, so only the read that can actually fail carries aResult.Verification
src/storage/fault_injection_tests.rs(6 tests) injects each failure. Checked againstorigin/mainin a scratch worktree: 2 fail there (corrupt_spend_journal_blocks_startup,corrupt_journal_denies_tenant_spend_read), confirming they catch the real bug and are not tautological. The other 4 pass on both sides, pinning the boundaries so the fix does not overshoot: a missing journal must still start clean, spend must survive a restart, and observing dependencies must never block a read.The pre-existing
malformed_lines_skippedtest asserted the fail-open ("skip the broken line, carry on") and is deliberately inverted tomalformed_line_is_an_error.Full suite: 1733 passed, clippy
-D warningsclean, 24 doc tests pass. ADR-0030's audit note and consequences updated to record the gap and its closure.