Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Remaining before this leaves draft: step 6, the live check through our own clientEverything hermetic is done and quoted above. What is not yet proven is HTTPS connection reuse through the shim's own httplib client against the real edge — every test here runs plain HTTP on loopback, and httplib's TLS reuse path has a peer-closed check the tests cannot exercise without certificates. Blocked on: a signed-in dev session on the test machine. Procedure once signed in (dev, not prod): WALLY_CONSOLE_URL=https://inference.runanywhere.ai/api-dev \
WALLY_CONSOLE_WEB_URL=https://runanywhere-frontend-development.up.railway.app \
wally login # once; then `wally whoami` shows the email
# the shim alone, verbose, built from this branch (build/wally-cxx)
build/wally-cxx claude-code --serve -m glm-5.3-flash --verbose
# in another terminal, two real completions against the printed ANTHROPIC_BASE_URL,
# each timed with curl -w '%{time_starttransfer}', 5 s apartPass criteria:
Numbers go in the PR body under "Against the real edge", then the draft flag comes off. Windows is confirmed by CI on the same push. |
|
Finding from the #81 plan review, for step 6's live measurement — a #80 latency item, not fixed here: cpp-httplib's |
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
ded8a74 to
dd01aeb
Compare
The approach, in plain words (for review without reading the code)The problem. Every request Claude Code (or a JetBrains IDE) sends through wally to the hosted model opened a brand-new secure connection — a TLS handshake measured at about half a second — and an agent makes several requests per turn. Like hanging up and redialling between every sentence of a phone call. What this PR does. Keep the line open. Both translators (the Anthropic shim and the JetBrains proxy) now keep their connections to the model endpoint open between requests and reuse them: one connection per stream in flight, a few idle ones kept for the next request. If a reused connection turns out to be dead (the far side dropped it while you were away), the request is tried once more on a fresh connection — but only when nothing had come back yet; a request that has started answering is never repeated. Why this way (and what we did not do). One shared connection would make parallel calls queue behind a long stream, so it is a small pool, not a singleton. We did not implement TLS session resumption (the HTTP library does not support it) or HTTP/2; keep-alive is the fix that matters. The one accepted risk is stated: "nothing came back" does not prove the server never ran the request, so the single retry can, rarely, run a generation twice — the same heuristic browsers and curl apply. How you can tell it works.
What it does not do / still needs.
|
1 participant
Closes #80. Also covers the same defect in the JetBrains proxy (folded in per the plan review on #80).
What changed, and why
Both loopback translators — the Anthropic shim behind
wally claude-code/claude-desktop, and the OpenAI proxy behindwally clion/rustrover— built anhttplib::Clientper request, so every request opened a TCP connection and completed a TLS handshake before a byte of the request went out. Against the hosted endpoint that handshake was measured at ~541 ms, and an agent makes several requests per turn.src/net/upstream_pool.{h,cpp}keeps keep-alive clients and lends them out per request:httplib::Clientserialises requests on its socket; one shared client would queue an editor's parallel calls behind a stream that lasts minutes. Each request in flight gets its own client; idle ones (up to 4) are kept for the next request.RetryOnFreshConnection): only when the connection was reused, no HTTP status came back, nothing was delivered to the caller, and the error is connection-class. A fresh connection that fails surfaces; a request that has produced output is never repeated. Accepted risk, capped at one and logged: "nothing came back" does not prove the server never ran the request, so a retry can in rare cases run a generation twice — the same heuristic curl and browsers apply to stale keep-alives.Out of scope, stated: TLS session resumption — cpp-httplib 0.46.1 does not implement it, so a reconnect is a full handshake; keep-alive is the fix. HTTP/2 likewise.
Proof
Every test observes behaviour from the fake upstream's side — the peer port a request arrived on — never source text. Each was neutered and watched go red; the runs are quoted in the commit messages.
sequential_requests_reuse_the_upstream_connection(shim and proxy)[62567, 62569]concurrent_requests_use_separate_connectionsstale_reused_connection_is_retried_once_on_a_fresh_one(shim and proxy)200, 502; [62702, 62702]upstream_dying_mid_stream_is_not_retriedreceived_anyignored →[62817, 62817, 62819]retry_rule_only_on_a_stale_reused_connectionCanceled(a reader that left)The half-open fake is raw sockets (httplib's server cannot drop a connection without answering) and POSIX-only; on Windows those two tests skip and the table test still covers the rule. An "editor abandons the stream" test was tried and removed: on loopback the translator's writes to the closed reader kept succeeding for longer than the stream lasted, so it could not observe the abort path and passed for the wrong reason.
Against the real edge
Two requests in one
curlinvocation tohttps://inference.runanywhere.ai/v1/chat/completions(401, no key — the handshake is what is measured):The edge honours keep-alive over HTTP/2; the second request saves ~0.8 s from this location.
Still to do before this leaves draft (the end-to-end test — not done): the same measurement through the shim itself with a signed-in dev session (
wally claude-code --serve -m glm-5.3-flash, two real completions, the shim's verbosereusedline on the second).Validation