1 commit

Author	SHA1	Message	Date
Tejas Chopra	a29d2015e5	fix(proxy): restore the buffered-CCR heartbeat behind a grace window (#3091) ... ## Description Closes #3079 #2997 removed the buffered-CCR keepalive preamble from both provider handlers. That preamble was added by #2479 to close #2465, so `main` is back to the condition #2465 described while #2465 stays closed. Confirmed against the tags: ``` v0.35.0 (released): keepalive_deadline = loop.time() + 1.0 + b'event: ping...' main (-> 0.36.0): neither ``` Since #2997 is queued in #3067, 0.36.0 would ship this. The justification left in the code does not hold. It reads *"clients budget minutes for a turn (Claude Code sends `x-stainless-timeout: 600`), so waiting is free"* — but `x-stainless-timeout` is the **total request** budget, and #2465 was about the **stream idle** watchdog, a separate timer. Total-budget headroom says nothing about idle-budget headroom, and not every client sends 600. The reporter's buffered turns routinely run 15-25s, all of it silent. The status-ordering half of #2997 is correct and is kept. What was wrong was treating the two properties as a trade. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) ## Changes Made - New `headroom/proxy/buffered_ccr_response.py` holding one implementation of the buffered-CCR ASGI wrapper. Both handlers previously carried ~100 duplicated lines each, which is how the OpenAI twin drifted from the Anthropic one; now only the error wire format differs. - A buffered turn holds out for `buffered_ccr_grace_seconds` before committing. Inside the window nothing is sent and the result is relayed untouched, so a fast 4xx — or a 429/529 that resolves once `_retry_request` has honored `Retry-After` — keeps its real status and headers. That is #2997's property. - Past the window the response is committed as SSE and a heartbeat starts, so a first byte always precedes the client's idle watchdog. That is #2479's property. - A failure landing after the commit can no longer carry an HTTP status, so it is translated into the provider's own **typed** SSE error (`rate_limit_error`, `overloaded_error`, ...) carrying the upstream's own message where there is one, rather than a generic `api_error`. **This is the part worth reviewing.** #2997 was right that early commits broke client backoff — but that was a consequence of degrading every post-commit failure to a bare `api_error`, not of committing itself. - New `buffered_ccr_grace_seconds` on `ProxyConfig`, default 5s, env `HEADROOM_BUFFERED_CCR_GRACE_SECONDS`. Setting it to `0` restores current `main` behavior exactly. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] New tests added for new functionality - [x] Manual testing performed **#2997's own tests pass unchanged.** `test_buffered_ccr_preserves_late_failure_status_and_headers` and `test_buffered_ccr_withholds_output_until_delayed_upstream_resolves` resolve at 1.1s, comfortably inside the 5s window, so everything #2997 bought for the cases it tested is intact. New `tests/test_buffered_ccr_grace_window.py` pins both constraints @JerrettDavis asked for on #2959 — a late failure keeping its real status and headers, and a slow success producing a first byte before the ceiling — plus the typed-error mapping, the zero-grace escape hatch, and the OpenAI wire format. ### Test Output ```text $ pytest tests/test_buffered_ccr_grace_window.py -q 9 passed in 2.01s $ pytest tests/test_proxy/test_anthropic_streaming_ccr_retrieve.py tests/test_ccr_buffered_stream_signed_thinking.py -q 16 passed, 1 warning in 8.19s $ pytest tests/ -q 3 failed, 11157 passed, 581 skipped in 334.97s (0:05:34) Same 3 failures as a clean-main baseline run on this machine: tests/test_graceful_shutdown.py::test_run_server_installs_cancelled_error_filter tests/test_learn/test_integration.py::TestCodexIntegration::test_full_pipeline tests/test_release_workflows.py::test_no_native_tls_in_wheel_build_tree (missing local `cargo` toolchain; a stale tool-name fixture; a logging test that loses to global-state pollution in a full run — all present on main.) $ ruff check . && ruff format --check . All checks passed! ``` ## Real Behavior Proof - Environment: this branch, the wrapper driven directly over ASGI with a stubbed buffered operation standing in for upstream; macOS arm64, Python 3.12. - Exact command / steps: drove three scenarios — a 429 resolving at 0.05s with a 5s window; a success released only after the window with a 0.1s window; a 429 resolving at 0.3s with a 0.05s window — recording every ASGI message sent. - Observed result: (1) client receives **HTTP 429** with `retry-after: 30` and zero bytes beforehand; (2) first byte (`200 text/event-stream` + ping) arrives **before** the upstream resolves, body follows intact; (3) committed 200, then `event: error` typed `rate_limit_error` carrying the upstream's own message rather than a generic one. - Not tested: a live client idle-timeout against a real 15-25s turn. #3079 notes this depends on whether the client's idle timer starts at request send or at first response byte — the grace window makes Headroom correct under either reading, but confirming the original symptom is gone needs the reporter's fleet. ## Runtime Rollout Safety - Rollout-managed feature(s): none — this is a fix to an existing code path, not behind a rollout channel. - Minimum rollout channel: n/a (ships to stable with the fix). - Stable/default behavior changed: yes. A buffered-CCR turn slower than 5s now emits SSE headers plus keepalives instead of staying silent. Turns resolving under 5s are byte-identical to current `main`. - Kill switch / disable path: `HEADROOM_BUFFERED_CCR_GRACE_SECONDS=0` restores current `main` behavior exactly (never commit early, no heartbeat). Covered by `test_a_zero_grace_window_never_commits_early`. - Unsafe override required: no. - Qualification impact: none — no qualification-gated surface is touched. - Rollback path: revert this commit, or set the env var to `0` without a redeploy of code. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I did **not** edit `CHANGELOG.md` ## Additional Notes Documentation update is marked N/A: the new env var is documented in the module docstring and the `ProxyConfig` field comment, matching how `HEADROOM_ANTHROPIC_BUFFERED_REQUEST_TIMEOUT_SECONDS` is handled. Type checking (`mypy headroom`) was not run separately; `ruff` is the gate this repo's CI enforces. Related: #2465, #2479, #2959, #2968, #2997, #3067. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Tejas Chopra <tejas@Tejass-MacBook-Pro.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-17 10:52:25 -07:00