[kyeh-amp]

Summary

An unread or stalled HTTP response could leave a pooled connection in Request-sent state, making the next request or retry fail. Incomplete responses now retire their connections; completed 200 responses keep HTTP/1.1 reuse.

HTTP response
  200 -> read while checked out -> reuse connection
  non-200 -> close unread body -> retry with fresh connection
  read failure -> close partial response -> retry with fresh connection

Julie Kwok's original fix from PR #83 is included as a separate commit with her authorship preserved.

What Changed

• Remote fetch: On a non-200 response, RemoteEvaluationClient now closes the connection without reading the error body. A failed body read on a 200 response also closes it. The pool then gets a fresh connection back, so a retry no longer fails with ResponseNotReady: Request-sent. The user is now serialized before a connection is taken from the pool, so a serialization error can no longer leak a pool slot.
• Flag config and cohort download: FlagConfigApiV2 closes the connection when reading the body fails. _get_cohort_members_request is now a context manager that keeps the connection checked out while the caller reads the response. On release it closes the connection for any non-200, unread, or failed response (202, 413, 429, and others).
• Tests: tests/pooled_response_test.py runs each client against a real local HTTP server. It covers an error followed by a retry, stalled error and success bodies, keep-alive reuse on success, release of the pool slot after a serialization failure, a stalled flag config body, and a cohort 202 followed by success. The cohort unit test mocks now return context managers.

# before: on a non-200, the body stayed unread and the open connection went back to the pool
if response.status != 200:
    raise FetchException(...)

# after: retire the connection
if response.status != 200:
    conn.close()
    raise FetchException(...)

Risk Assessment

✅ Low: The change is bounded to the three pooled HTTP paths. It closes the stalled-body, unread-cohort-body and to_json checkout-leak gaps. Real local-server regression tests cover these failures, and none of the traced paths returns a poisoned connection to the pool.

Testing

• 36 targeted tests passed at the pushed commit: pooled-response regressions, cohort download, flag config, and remote client tests.
• The local HTTP/1.1 server uses one pool slot, Content-Length, one retry, and zero backoff. It covers ordinary 503 responses, stalled partial bodies, successful keep-alive reuse, flag config, and cohort download.

Revision	503 body then 200	Partial 503 body stalls, then 200
Base	ResponseNotReady: Request-sent	ResponseNotReady: Request-sent
Contributor commit	Succeeds	ResponseNotReady: Request-sent
This branch	Succeeds	Succeeds

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 2 infos

• ℹ️ src/amplitude_experiment/cohort/cohort_download_api.py:88 - The finally condition response.status != 200 or not response.isclosed() closes the connection on every 204 (not modified). 204 is the normal steady-state answer when polling a cohort, so each poll now does a new TCP/TLS handshake. The result is still correct, but it costs extra work. A 204 has no body, so the caller could call response.read() (cheap, zero length) and keep the connection. Using only the isclosed() check would then keep the connection for 204, and also for any fully read non-200 body. This is a small optional optimization, not a blocker.
• ℹ️ src/amplitude_experiment/flag/flag_config_api.py:39 - On non-200, flag config still reads the full error body so it can include it in the exception message. A stall now closes the connection on timeout, which fixes gap 3. A large or slowly trickling error body can still hold the poller until each recv times out. The remote client closes the connection on non-200 instead. The difference looks deliberate, because the body is part of the error message.

✅ **Test** - passed

✅ No issues found.

• PYTHONPATH=src python -m unittest -v tests.pooled_response_test tests.cohort.cohort_download_api_test tests.flag.flag_config_api_test tests.remote.client_test at HEAD 9d4fa91 (36 tests, all pass)
• Ran the new tests/pooled_response_test.py against git archive exports of base 77cf5b7 and contributor commit 0e602da. 6 of 8 tests fail at each revision. Most failures show ResponseNotReady: Request-sent.
• Manual end-user check: called the public RemoteEvaluationClient.fetch_v2 against a local HTTP server. Case 1 returns 503 then 200. Case 2 sends 1 byte of a 503 body, stalls, then returns 200. Settings: fetch_timeout_millis=100, fetch_retries=1, pool size 1. Ran it at 77cf5b7, 0e602da and HEAD.

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

---

Note

Low Risk
Scoped to three pooled HTTP clients with explicit close-on-bad-response behavior; regression tests cover retry and keep-alive paths.

Overview
Fixes pooled HTTP/1.1 connections being returned while still in Request-sent (unread or stalled body), which broke retries with ResponseNotReady.

Remote evaluation closes the connection on non-200 without reading the error body, and on failed body reads for 200. User JSON is built before acquiring a pool slot so a to_json failure cannot leak a checkout.

Flag config closes the connection when reading the response body fails (e.g. stall/timeout).

Cohort download turns _get_cohort_members_request into a context manager: the connection stays checked out until the caller finishes, then release closes it for non-200, partially read, or failed reads; fully consumed 200 responses can still be reused.

Adds tests/pooled_response_test.py (local HTTP server) for error→retry, stalled bodies, keep-alive on success, flag/cohort paths, and updates cohort unit mocks to yield context managers.

^{Reviewed by Cursor Bugbot for commit 8cb4238. Bugbot is set up for automated code reviews on this repo. Configure here.}

[zhukaihan]

🤖 Codex review

Reviewed the full diff, connection-pool lifecycle, remote evaluation, flag polling, cohort downloads, and regression tests. No blocking issues found. Closing error/partial-read connections before release prevents reuse of a poisoned HTTP connection; retaining the cohort lease through response consumption fixes the concurrent-reuse window. Serialization also now happens before acquiring a pool slot.

The real HTTP/1.1 tests cover retries, partial-body timeouts, single-slot recovery, and successful keep-alive reuse. All eight reported checks passed, including Python 3.8/3.12/3.14; the inspected Python 3.14 log reports 208 tests, OK with four skipped. I did not run tests locally.

[linear-code]

STADXP-648