fix: give GeminiBackend a request timeout so a stuck socket can't hang forever - #370
Conversation
There was a problem hiding this comment.
Re-verified 31 Jul at d7c17c0b. The timeout works; the dependency floor still does not.
I proved the mechanism rather than reading it. Pointing the backend at an unroutable address:
| setting | http_options.timeout |
behaviour |
|---|---|---|
timeout=4.0 |
4000 |
raises ConnectTimeout after 4.0s |
timeout=None |
None |
still hanging when killed at ~70s |
So the original finding is confirmed end to end: timeout=None disables the httpx deadline rather than falling back to a client default, and your fix genuinely closes it. The stall that froze a 3.5h run is a real bug and this is the right fix for it.
The one blocker is unchanged. pyproject.toml:25 is still models = ["google-genai>=0.3", ...]. I checked the actual google_genai-0.3.0 wheel rather than inferring: types.py has no HttpOptions at all. It exists only as a TypedDict in google/genai/_api_client.py, carrying base_url, api_version, headers and response_payload, with no timeout field. In 1.0.0 types.HttpOptions exists as a BaseModel with timeout. So on the floor this package declares, types.HttpOptions(timeout=ms) is an AttributeError, and a legal pip install produces a backend that cannot construct.
One line: google-genai>=1.0.
A second thing worth fixing while you are in there, and it answers @Agastya191's objection properly. tests/test_gateway.py::test_gemini_backend_builds_a_real_client_with_the_timeout does now assert the wiring (_http_options.timeout == 45000), which is exactly what was asked for. But it opens with pytest.importorskip("google.genai"), and google-genai sits in the models extra, not dev (pyproject.toml:27). There is no .github/workflows in this repo. It passed in my environment only because 1.39.1 happens to be installed; on a stock pip install -e .[dev] the test silently skips and the guard is gone precisely where CI would run it.
Either add google-genai to the dev extra, or assert the wiring against a stub so the test does not depend on the optional dependency being present.
Merges clean onto current main, 1081 passed, 7 skipped. Holding only on the floor bump.
Agastya191
left a comment
There was a problem hiding this comment.
Nice fix, duckyquang. The deadline is the right call, _timeout_ms keeping the seconds-facing API while google-genai gets milliseconds is clean, and the docstring note that the raised error is transient so RetryBackend picks it up saves the next reader a trip through the wrapper.
One problem is that none of the three new tests in test_gateway.py observe the http_options actually reaching the client. They assert self.timeout, which is a plain attribute set before the SDK branch and unaffected by the wiring, and test_gemini_backend_builds_a_real_client_with_the_timeout only checks that same attribute plus _client is not None, then importorskips away entirely since google-genai is not in the dev env. I deleted the ms / http_options / genai.Client(http_options=...) block outright and all 19 gateway tests still passed, so the regression this PR exists to prevent is not pinned by anything.
You have options, but I would monkeypatch genai.Client with a recorder and assert it received http_options.timeout == 60000, which tests the wiring rather than the attribute.
188651c to
d7c17c0
Compare
|
@sebasmos thanks for tracing the httpx
Rebased onto main; full suite green locally. Default stays 60s. |
|
Pushed f3ca20b, covers both of you.
Mind another look when you get a sec? |
sebasmos
left a comment
There was a problem hiding this comment.
Approving. Both objections resolved, and I verified the test is not vacuous rather than taking your word for it.
pyproject.toml:25is nowgoogle-genai>=1.0. That was the blocker:types.HttpOptionsdoes not exist in the 0.3 wheel at all, so a legal install built a backend that could not construct.- The
importorskipis gone.test_gemini_backend_wires_the_timeout_into_the_clientnow stubs the google-genai modules and assertsgenai.Clientactually receiveshttp_options.timeout, so it runs on a stockpip install -e .[dev]instead of skipping exactly where CI would live.
Sabotage check. I removed http_options=http_options from the genai.Client(...) call at gateway.py:162, which is the realistic regression, and got:
FAILED tests/test_gateway.py::test_gemini_backend_wires_the_timeout_into_the_client
1 failed, 19 passed, 1 skipped
So the guard catches the thing it exists to catch. My first attempt at that sabotage patched mid-expression and produced a collection error instead, which proved nothing; worth saying because a broken-module "failure" is not evidence of a working test.
Suite merged onto current main: 1118 passed, 7 skipped.
Also good: pinning the timeout=None branch to pass http_options=None rather than HttpOptions(timeout=None) is the right call, since the whole original bug was that None there disables the deadline rather than falling back to a default.
Nice fix. The stall that froze a 3.5h run was real and this closes it properly.
…ng forever (#370) Verified twice, the second time against a main 41 commits past this branch's merge-base, since the first approval was hours old by then: merges clean, 1150 passed and 7 skipped, and sabotage-verified both times, removing http_options=http_options from the Client(...) call fails test_gemini_backend_wires_the_timeout_into_the_client. Both of my original objections were fixed at 6c5af26. The floor is now google-genai>=1.0, which was the real blocker since types.HttpOptions does not exist in the 0.3 wheel, and the importorskip is gone: the wiring test stubs the google-genai modules and asserts genai.Client receives http_options.timeout, so it runs on a stock dev install instead of skipping exactly where CI would. Merging over @Agastya191's CHANGES_REQUESTED of 30 Jul 15:20, deliberately and on the owner's instruction. It predates the 31 Jul 03:54 fix commit that addressed it, and `main` carries no protection rule, so it was never a mechanical gate. Nothing in the code is outstanding; the review state was simply stale. Flagging it here rather than silently, so the history is legible.
Summary
GeminiBackend.completecalledgenerate_contentwith no request deadline, so a wedged socketblocked the call forever. That's what froze the long MedMCQA batch runs — a worker sat at 0% CPU
for ~3.5h with its cache frozen, unrecoverable short of a kill. google-genai takes its timeout in ms
via
HttpOptions, so I added atimeoutparam toGeminiBackend(seconds, default 60) and buildthe client with it: a hung request now raises after the deadline instead of never returning.
The important part is what this unblocks. Every arm script already wraps
GeminiBackendinRetryBackend, which retries on any exception — so they weren't missing a retry, they were stuck ona call that never raised. With a deadline, a wedged request raises and the existing
RetryBackendretries it. So this one change makes the whole battery self-healing against socket hangs, no
per-script edits needed.
Result
Verified against the live API: a 1 ms timeout raises
ConnectTimeoutin 0.03s (no hang), a normal60s call is unaffected. Full suite green (710 passed), including new tests for the ms conversion,
the finite default, and real-client construction with the timeout wired in.
Notes
RetryBackend-wrapped isbuild_backendinrunner.py(thebenchmaxxing runCLI path, on the feat:benchmaxxing runentry point for the stage runners #251 stack, not on main). Worth wrapping oncethis lands and the stack rebases — doing it before the timeout exists would be a no-op, since
RetryBackendcan't retry a call that hangs.timeout=Nonedisables the deadline.