perf: Reuse the connection to Lago, and fix two silent-failure paths - #25
Open
ancorcruz wants to merge 5 commits into
Open
perf: Reuse the connection to Lago, and fix two silent-failure paths#25ancorcruz wants to merge 5 commits into
ancorcruz wants to merge 5 commits into
Conversation
HTTP/1.1 is persistent by default and urllib3 pools connections — but the module-level `requests.post` builds a Session, uses it, and closes it, so the pool is discarded after every call. The protocol offers keep-alive; that call path declines it. Counted against a local server that answers `Connection: keep-alive`: 8 batches via requests.post -> 8 TCP connections accepted 8 batches via one Session -> 1 TCP connection accepted The cost of each extra connection is a TCP handshake plus a TLS handshake, i.e. about 2 RTT. Measured from a client 137ms from api.getlago.com (us-east-1) that is ~276ms of overhead in front of a request the server answers in ~147ms: fresh connection per batch 440.6 ms median -> 227 evt/s @100/batch reused Session 147.6 ms median -> 677 evt/s @100/batch The 2.98x is path-dependent, not a universal constant — the saving is 2 RTT, so it shrinks toward nothing for a client sitting next to the API and grows for one far from it. The connection count does not move, which is why the tests assert on that rather than on timing. Where it matters most is the isolation path. In the normal path one handshake is amortised over 100 events; when one duplicate transaction_id 422s a batch, `_send_individually` re-sends all 100 alone and pays the handshake PER EVENT — 274ms/event against 2.7ms/event, on the recovery path: isolation after a 422, before 101 requests / 101 connections (~27.7s) isolation after a 422, after 101 requests / 1 connection (~0.3s) Discarding the Session also discards the TLS session ticket, so no abbreviated handshake is possible on reconnect either. Sharing one Session across sender threads is fine — urllib3's connection pool is documented thread-safe, and measured at 1/4/10/20/50 concurrent threads it served every request correctly with zero errors. The caveat is not thread safety but pool sizing: `pool_maxsize` defaults to 10 and `pool_block` to False, so past ten concurrent senders urllib3 opens connections outside the pool and discards them on return — 50 threads against the default pool opened 97 connections and logged 87 "connection pool is full" warnings, i.e. quietly handing back the reuse this commit adds. Whoever adds a second sender should raise `pool_maxsize` to the sender count, not reach for a thread-local Session. Holding a Session means there is now a socket to leak: `requests.post` closed its own per call, so nothing outlived `shutdown()` before this. `LagoSDK.shutdown()` now releases it — after `queue.shutdown()`, because the queue's exit drain still needs to send through it. Measured: 1 open socket after shutdown, now 0. A send after shutdown still works, since a closed Session rebuilds its pool. The two `verify_ssl` tests patched `requests.post` to assert a flag that is still passed through the same way. Once the client stopped calling that function the patch stopped matching, and the tests began issuing live requests to api.getlago.com — they failed, but with Lago's 404 rather than with anything pointing at the real problem. They now patch the session they actually exercise.
`_take_batch` pops events OUT of the buffer before the send is attempted, and `flush()` decided it was done by checking that the buffer was empty. For the duration of a request the batch was in neither the buffer nor Lago, so: flush() returned True while 5 events were in flight and about to FAIL An empty buffer never meant delivered. At process exit the exit drain is a second net and usually still delivers, so this mostly cost nothing there — but `flush()` is documented as the delivery checkpoint, and a caller using it as one (end of a batch-job phase, before logging "billing synced", before a container reports ready-to-terminate) got a false positive with nothing behind it. It also undermines how we report on the SDK: "flush completed cleanly" has been offered as evidence that a sustained-load run delivered every event. It does not establish that. The count of events Lago accepted does. `_in_flight` counts events taken but not yet accounted for, and every path that consumes a batch settles it: `_replay_failed` settles what it puts back (in the same lock acquisition that re-queues it, or flush() could observe a moment where they are counted in neither place), `_send_individually` settles what it resolved rather than re-queued, and both drain loops settle on delivery or on final loss. Three tests cover it: in-flight blocks flush, a delivered batch releases it, and a requeued batch does not leak the counter upward on every retry. Also fixes a stale comment while in here: `_after_in_child` has always emptied the child's buffer, but the comment above it claimed the parent's events were "copied over".
Every wrapper caught adapter failures like this:
except Exception as exc:
logger.warning("lago: anthropic emit failed: %s", exc)
`LagoSDK.emit()` does call `_report_error(exc, "emit")` — but the adapter call
sits OUTSIDE it, on the wrapper's side of the boundary. When a provider changes
its response shape the adapter raises before `emit()` is entered, so the SDK's
own reporting never runs. Across the five wrappers: 14 `except Exception` sites,
14 `logger.warning` calls, zero `on_error` routes.
That is the worst direction for this failure to take. Provider drift does not hit
an occasional call, it hits EVERY call for that provider, and `on_error` is the
channel customers actually watch — it is what the queue already uses to report an
overflow, for exactly this reason. A log line nobody is tailing turns a total
billing outage for one provider into silence.
All 14 sites now report as well as log. `_report_error` already swallows anything
the customer's callback raises, so this cannot break the LLM call it is attached
to — the customer's call returns normally either way.
Every site was checked statically to be inside an `except ... as exc` handler
passing the bound exception, and to use the sdk reference that is actually in
scope there (`self._sdk` inside the anthropic stream managers, `sdk` elsewhere).
A test that stubs HTTP by patching a library function keeps passing as a shape long after it has stopped testing anything: when the code under test moves to a different call path, the patch simply stops matching and the request goes out for real. Not hypothetical — the `verify_ssl` tests did exactly this in an earlier commit, and reported Lago's 404 rather than the mismatch. `--disable-socket` (pytest-socket) is the `disable_net_connect!` equivalent: any unit test that opens a socket now fails saying so. On those same broken tests it turns a confusing `LagoApiError: 404` after 0.97s into `SocketBlockedError` after 0.15s. It also removes the class of CI flake where a green suite depends on the network, and makes it impossible for a test run to reach production Lago. 127.0.0.1 stays allowed, so a test can still stand up a real server when the property under test is about real socket behaviour — `test_connection_reuse.py` counts accepts on a local HTTP/1.1 server rather than asserting that some particular function was called. `responses` is added alongside for stubbing at the requests transport boundary rather than by patching internals. Existing tests are left as they are; it is there for new ones and for migrating the brittle patch sites as they are touched. No cassette/auto-record layer: fixtures in this repo stay explicit, captured by a dev running `tests/unit/adapters/fixtures/capture_*.py` on purpose and reviewed in the diff.
ancorcruz
force-pushed
the
perf/connection-reuse-and-delivery-contract
branch
from
August 26, 2026 09:16
9196c71 to
45b5baa
Compare
vladmarascu
approved these changes
Aug 26, 2026
ancorcruz
force-pushed
the
perf/connection-reuse-and-delivery-contract
branch
from
August 26, 2026 13:45
45b5baa to
9e187c4
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
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.
Context
Every batch opened a new connection.
LagoClient.send_batchcalled the module-levelrequests.post, which builds aSession, uses it, and closes it — discarding the connection pool after every call. HTTP/1.1 is persistent by default and urllib3 pools connections, so keep-alive was available the whole time; the client was declining it. Every batch paid a TCP handshake plus a TLS handshake — about 2 RTT — in front of a request the server answers in ~147 ms. Discarding the Session discarded the TLS session ticket too, so not even an abbreviated handshake was possible on reconnect.That cost is amortised over 100 events in the normal path. It is not amortised at all on the recovery path: when one duplicate
transaction_idmakes Lago reject a whole batch,_send_individuallyre-sends all 100 events one at a time, and the handshake was paid per event — roughly a hundred times the per-event overhead, precisely when the system is already degraded.flush()reported success on events it hadn't delivered._take_batchpops events out of the buffer before attempting the send, andflush()decided it was done by checking that the buffer was empty. For the duration of a POST the batch sat in neither the buffer nor Lago, soflush()returnedTrueon events that were still in flight — and, if that send then failed, on events that had not arrived at all. It is the documented delivery checkpoint, so a caller using it as one (end of a batch-job phase, before logging "billing synced", before a container reports ready-to-terminate) got a false positive with nothing behind it.A provider changing its response shape produced silence.
LagoSDK.emit()reports failures throughon_error, but the adapter that parses a provider response runs outside it, on the wrapper's side of the boundary. An adapter that raised did so beforeemit()was ever entered, so the SDK's own reporting never ran — across the five wrappers, 14except Exceptionsites, 14logger.warningcalls, and zero routes toon_error. Provider drift doesn't hit an occasional call; it hits every call for that provider. The result was a total billing outage that surfaced only as a log line.Underneath all three, the test suite couldn't have caught them regressing: it stubbed HTTP by patching library functions, so a stub silently stops matching the moment the code under test changes call path, and the request goes out to the real network instead.
Description
Impact
flush()with a batch in flightTrueon_errorfiressdk.shutdown()Only the two throughput rows are path-dependent — the saving is two round trips per batch, so it shrinks for a client sitting next to the API and grows for one far from it. The connection count and the three behaviour rows hold everywhere.
Connection reuse
LagoClientnow holds onerequests.Sessionfor its lifetime. Measured against a local server answeringConnection: keep-alive, 8 batches cost 1 connection instead of 8; againstapi.getlago.comfrom 137 ms away, the median batch went from 440.6 ms to 147.6 ms. On the recovery path, a rejected batch that fans out into 100 isolated sends went from 101 connections to 1.The 2.98× figure isn't claimed as a constant, which is why the tests assert on connection counts rather than on timing.
One consequence points the other way and is worth knowing: that fan-out now reaches Lago about 3× faster, with no application-level rate limit in front of it. That argues for bounding the fan-out separately, not against this change.
Holding a Session also creates a socket that can outlive the SDK —
requests.postclosed its own after every call, so nothing lingered before.LagoSDK.shutdown()now releases it, after the queue's exit drain has finished sending through it. A send after shutdown still works, since a closed Session rebuilds its pool.A truthful
flush()The queue now tracks events that have been taken off the buffer but not yet accounted for, and
flush()waits on that as well as on the buffer. ATruenow means delivered, dropped for good, or back on the buffer — never "somewhere in a socket."This is the part worth reviewing closely. Every path that consumes a batch has to settle it exactly once:
_drain_buffer, after the send_replay_failed, in the same lock acquisition_send_individually, for what it resolved rather than re-queued_runA missed settle leaks the counter upward and
flush()never returnsTrueagain; a double settle reopens the original bug. There's a test for each direction.Worth stating for anyone benchmarking the SDK: a
Truefromflush()was never proof of delivery. Showing that a load test lost nothing means counting the events Lago accepted.Adapter failures reach
on_errorAll 14 wrapper sites now report as well as log.
_report_erroralready swallows anything a customer's callback raises, so the LLM call still returns normally either way — the change is only that a billing gap now appears on the channel customers actually watch, the same one the queue already uses for buffer overflow.Each site was verified statically to sit inside an
except ... as exchandler, pass the bound exception, and use thesdkreference genuinely in scope (self._sdkinside the anthropic stream managers,sdkelsewhere).Tests that can't quietly stop testing
pytest-socket's--disable-socketis on by default: a unit test that opens a socket now fails saying so, instead of reaching the network and failing later for an unrelated-looking reason. On the twoverify_ssltests that had started issuing live requests, it turns a confusingLagoApiError: 404after 0.97 s intoSocketBlockedErrorafter 0.15 s. It also removes the class of CI flake where a green suite depends on the network, and makes it impossible for a test run to reach production Lago.127.0.0.1stays allowed so a test can stand up a real server when the property under test is socket behaviour.test_connection_reuse.pydoes exactly that — it counts accepts on a local HTTP/1.1 server rather than asserting that some function was called. Its three cases cover the three decisions made here: that a Session is held, that the recovery path shares it, and that shutdown releases it. All three fail against the previous code.There is deliberately no test for connection recovery. urllib3 reconnects transparently after a clean close — measured, both
Connection: closeand an idle hang-up are handled with zero errors — and the only part that belongs to this SDK, the queue treatingConnectionErroras transient, is already covered. Testing it here would test the HTTP library instead.responsesis added for stubbing at the requests transport boundary rather than by patching internals; existing tests are untouched, it's there for new ones. No cassette or auto-record layer — fixtures stay explicit, captured by a developer runningtests/unit/adapters/fixtures/capture_*.pyon purpose and reviewed in the diff.The recovery path was mutation-tested: six deliberate breakages — isolation disabled, transient sub-failures dropped, FIFO inverted,
on_errorper event, backoff skipped on partial requeue, in-flight counter unsettled — all six caught.608 tests · 91.56% coverage · ruff, format and mypy clean. No public API changes.
Measurement caveat: connection counts are localhost (pool mechanics) plus a single real path to
api.getlago.com(RTT). Neither covers server-side idle timeouts or load-balancer connection limits.