Fix audit findings: CB race, silent billing zeros, config/SSE robustness#3
Merged
Conversation
Addresses issues surfaced by the technical audit: - circuit_breaker: fix windowed failure-count race. The old two-atomic design (state + window_start) computed the window-reset decision outside the state CAS, so concurrent failures at a window boundary could each reset the count to 1 and drop one another — pinning the count and keeping a genuinely-broken provider's breaker stuck closed. Anchor the window in the packed timestamp so reset-and-increment commit in a single CAS; delete the now-redundant window_start atomic. Add a regression test. - circuit_breaker: add a record_success fast path so high-throughput success traffic doesn't bounce the breaker cache line on every response. - proxy: emit a warn + ai_usage_parse_errors_total when a managed 2xx response carries no parseable usage, instead of silently emitting a zero-token billing row indistinguishable from a real zero. - config: surface a malformed-TOML parse error at load with the file named, rather than swallowing it and letting it resurface as an opaque Figment error. half_open_permits(1) so the breaker matches the documented "a probe". - state: warn at boot when upstream_verify_cert is disabled (MitM opening valid only for the local test mock). - usage: tolerate CRLF SSE line endings (RFC 8895) — a trailing \r would otherwise fail the JSON parse and silently zero usage. - peek: correct an inaccurate comment about the "stream" pre-filter needle. - docs: keep ARCHITECTURE.md Metrics table in sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Addresses issues surfaced by the technical audit of the gateway.
Correctness / data integrity
circuit_breaker.rs) — the old two-atomic design (state+window_start) computed the window-reset decision outside the state CAS. Concurrent failures landing at a window boundary could each independently reset the count to 1 and clobber one another, pinning the count and leaving a genuinely-broken provider's breaker stuck closed. Fixed by anchoring the failure window in the packed timestamp so reset-and-increment commit in a single CAS; the redundantwindow_startatomic is removed. Newtest_concurrent_windowed_counts_every_failureregression test (200 iterations × 20 threads).proxy.rs) — a managed2xxresponse whoseusageblock fails to parse (provider wire change, new API version) emitted a0-tokenai.usagerow indistinguishable from a real zero. Now incrementsai_usage_parse_errors_totaland logs awarn!for alerting. The zero-token row stays (correct sentinel); the observability gap is closed.usage.rs) —data:lines split on\nretained a trailing\r(valid per RFC 8895), failing the JSON parse and silently zeroing usage. Now trims both ends.Operability / security
upstream_verify_cert=falseboot warning (state.rs) — disabling cert verification is a transparent-MitM opening valid only for the local test mock; now warns loudly at boot so it can't be a silent production misconfig.config.rs) — a syntax error in the operator's config was swallowed and resurfaced as an opaque Figment error; now fails the load with the file named.Performance
record_successfast path (circuit_breaker.rs) — early-return on an already-clean breaker so high-throughput success traffic to one provider doesn't bounce its breaker cache line on every response.Minor
half_open_permits(1)so the breaker matches ARCHITECTURE.md's documented "a probe is admitted"."stream"pre-filter needle (peek.rs).Verification
All CI checks pass locally:
dprint check,cargo fmt --check,cargo clippy --all-targets -D warnings,cargo test --lib(98),cargo test --test '*'(21),cargo build --releasewith-D warnings.🤖 Generated with Claude Code