Preserve overload status and Retry-After before Anthropic streaming - #103
Open
sanchitmonga22 wants to merge 7 commits into
Open
sanchitmonga22 wants to merge 7 commits into
sanchitmonga22 wants to merge 7 commits into
Conversation
The translator built an httplib::Client per request, in the streaming sink
and in the non-streaming handler, so every request opened a TCP connection
and completed a TLS handshake -- measured at ~541 ms against the hosted
endpoint, paid before a byte of the request was sent, on every turn.
A pool now keeps keep-alive clients and lends them out per request
(src/net/upstream_pool.{h,cpp}). A pool rather than one shared client
because an httplib::Client serialises requests on its socket, and an
editor's parallel calls must not queue behind a stream that lasts minutes.
A lease returns to the pool only when its request completed cleanly; an
abandoned stream or a transport error discards the socket.
A request that fails on a REUSED connection with no status, nothing
delivered, and a connection-class error is sent once more on a fresh one
(RetryOnFreshConnection): the stale keep-alive case. A fresh connection
that fails surfaces; a request that produced output is never repeated.
Accepted, capped and logged: "no bytes back" does not prove the server
never ran the request, so a retry can in rare cases run a generation
twice. The bearer is set per lease, not baked into the client, because the
JetBrains proxy renews its token on a 401 (wired in a following commit).
Connect timeout is now 10 s (httplib default 300 s): a black-holed connect
no longer holds the editor's request for five minutes. Read timeout stays
600 s.
Break test (tests/test_wally_anthropic.cpp, fake OpenAI upstream on
loopback recording the peer port of every request):
unmodified shim (093cd44):
[FAIL] sequential_requests_reuse_the_upstream_connection
Expected: same peer port on both upstream requests (one connection)
Actual: [62567, 62569]
[PASS] concurrent_requests_use_separate_connections
Results: 1 passed, 1 failed, 2 total
with this change:
[PASS] sequential_requests_reuse_the_upstream_connection
[PASS] concurrent_requests_use_separate_connections
[PASS] pool_returns_a_clean_lease_and_drops_a_discarded_one
[PASS] pool_outlives_an_outstanding_lease
[PASS] retry_rule_only_on_a_stale_reused_connection
Results: 5 passed, 0 failed, 5 total
ctest: 100% tests passed, 0 tests failed out of 11
…tput rule (#80) Two integration tests on the translator, plus the fake upstreams moved into tests/fake_upstream.h so the JetBrains proxy's suite can share them. A half-open upstream (raw sockets, POSIX-only; the retry rule's table test covers every platform) answers the first request on a connection, keeps it open, then reads the second and closes without answering. That is the stale keep-alive shape the client cannot see before sending -- httplib's is_socket_alive already catches a peer that sent FIN, so a FIN-after-answer server would never reach the retry. The translator must retry once on a fresh connection: the upstream sees three requests, the first two on one connection and the third on another. A reused upstream that dies mid-stream (two frames, then the connection drops) must NOT be retried: the error is connection-class with no status, so only "bytes reached the caller" stops a second generation. The upstream sees exactly two requests. An "editor abandons the stream" variant was written and removed: with the retry rule neutered to always-true the upstream still saw only two requests, i.e. on loopback the translator's writes to the closed reader kept succeeding for longer than the stream lasted, so the test could not observe the abort path and passed for the wrong reason. Abandonment is Error::Canceled, now an explicit row of the table test. Break tests, each neutered in src/anthropic/messages.cpp and restored: retry removed (attempt < 1): [FAIL] stale_reused_connection_is_retried_once_on_a_fresh_one Actual: 200, 502; [62702, 62702] Results: 6 passed, 1 failed, 7 total received_any ignored in the retry decision: [FAIL] upstream_dying_mid_stream_is_not_retried Actual: 200, 200; [62817, 62817, 62819] Results: 7 passed, 1 failed, 8 total restored: Results: 7 passed, 0 failed, 7 total
The proxy behind `wally clion` / `wally rustrover` built an httplib::Client
per request at two sites -- the streaming sink and the non-streaming
handler -- with the same cost the translator paid: a TCP connect and a TLS
handshake before every request. It now takes leases from the same
UpstreamPool.
The streaming sink already tried twice, the second time with a token the
console had just renewed after a 401. That stays, and composes with the
stale-connection retry: each is allowed once, on its own condition -- a 401
body renews the token, a connection-class failure with nothing received on
a REUSED connection takes a fresh one -- and neither can duplicate output,
because nothing reaches the sink until an event stream is recognised. The
bearer is set per lease for exactly this reason: the renewed token must ride
the next attempt.
Break tests (tests/test_wally_ide_proxy.cpp, sharing tests/fake_upstream.h):
PostOnce retry removed (attempt < 1):
[FAIL] proxy_stale_reused_connection_is_retried_once
Actual: 200, 502; [63164, 63164]
Results: 1 passed, 1 failed, 2 total
non-streaming site back to a per-request client:
[FAIL] proxy_sequential_requests_reuse_the_upstream_connection
Actual: [63170, 63172]
[FAIL] proxy_stale_reused_connection_is_retried_once
Actual: 200, 200; [63176, 63178]
Results: 0 passed, 2 failed, 2 total
restored: Results: 2 passed, 0 failed, 2 total
- UpstreamLease move-assignment dropped the client it already held instead of returning it to the pool (or discarding it). Nothing moves-assigns a lease today; it is still the wrong contract, and now it releases the held client exactly as end of scope would before taking the other's. - The proxy tests' FreePort() probe bound a port and dropped the Server object: httplib's stop() only closes the listening socket while running, and ~Server() does not close it at all, so each call leaked a listening socket. The probe now listens, stops, and joins, which closes it. (An assertion that StartProxy received the requested port was tried and removed: with the leaky probe StartProxy still got the port on macOS, so the assertion could not be shown to fail.) - The fake upstream's mode flags are written by the test thread and read by server threads; they are atomics now. ctest: 100% tests passed, 0 tests failed out of 12
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Collaborator
Author
|
Final independent review: PR #103 at Independent checks: full CTest 12/12 passed at implementation commit @Siddhesh2377 merge order: #82 → #103 → #104. Rebase/retarget each dependent PR to |
AmanSwar
force-pushed
the
issue-80-shim-keepalive
branch
from
September 13, 2026 14:33
ded8a74 to
dd01aeb
Compare
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
2 participants
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.
Streaming
/v1/messagesnow waits for upstream HTTP headers before committing downstream SSE. A pre-stream 429 or 503 remains that HTTP status, with the original numeric or HTTP-dateRetry-Afterand a typed Anthropic JSON error. Previously both became HTTP 200 with an SSE error and no cooldown header; nonstream 503 also lost its cooldown.Related to #83. The issue remains open for owner acceptance and deployment testing.
The implementation uses cpp-httplib's response-header callback on the existing pooled connection. One bounded chunk slot connects the upstream reader to the downstream writer, preserving backpressure without buffering the full generation. Failed or abandoned streams discard the socket. A connection failure after receiving HTTP headers is never retried as a stale socket; mid-stream failures retain typed SSE errors and are not replayed.
Validation:
ded8a74): streaming 429 and 503 both returned 200 without Retry-After; nonstream 503 lost Retry-After. Existing seven transport tests passed.ctest --test-dir build --output-on-failure: 12/12 passed. Existing transport tests cover successful streaming, separate concurrent connections, pooled reuse, stale connection retry and no mid-stream replay.scripts/ci/check-agents-sync.shandscripts/test/smoke.sh build/wally-cxxpassed.WALLY_APPLE_MLX_HOST=OFF, explicit OpenSSL root. Optional live/model tests retain their existing internal skips. No real API credentials or paid inference were used. Exact Claude Code retry timing and deployed gateway behavior still require final owner E2E acceptance.Merge sequence: PR #82 → this PR → the separate #84 terminal-stream validation PR. Current base is
issue-80-shim-keepalive; final integration target ismain(this repository has no dev/development branch). After #82 merges, rebase this branch ontomainand retarget, particularly if #82 is squash-merged. Siddhesh owns merging. Coordinate Aman's active #81 cancellation work against this updated transport; this PR does not implement the explicit remote cancel API. No automatic issue-closing keywords are used.