fix(http): surface truncated response body in shared request_json helper#39
fix(http): surface truncated response body in shared request_json helper#39seanhanca wants to merge 1 commit into
Conversation
Generalizes PR #38 (which fixed only byoc.py _create_byoc_payment) to the shared HTTP helper in orchestrator.py, which powers discovery, orchestrator selection, LV2V start, and signer session/refresh via request_json/get_json/ post_json. Root cause: http.client.IncompleteRead subclasses HTTPException — NOT HTTPError/URLError/OSError — so a truncated response (endpoint advertises a Content-Length but closes the connection early) slips past every existing handler and surfaces opaquely as "IncompleteRead(85, 108)", discarding the partial body that carries the real error (e.g. {"error":{"message":"..."}}). - request_json: catch IncompleteRead on the success read and re-raise the captured partial body inside LivepeerGatewayError with byte counts. - _http_error_body: fall back to e.partial when the error-body read is itself truncated, so _extract_error_message can still recover the real message. Error-legibility hardening only: happy-path behaviour is unchanged. Does not migrate byoc.py's inline urlopen call sites (deferred, larger scope). Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds handling for http.client.IncompleteRead in orchestrator.py: extends error-body extraction to return partial bytes and adds a request_json exception branch raising LivepeerGatewayError with truncation details. Adds a new test module covering both success and error truncated-response scenarios. ChangesTruncated HTTP Response Handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_request_json_truncation.py (1)
70-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a simpler raise idiom for
err.read.The generator-throw trick
(_ for _ in ()).throw(...)works but is harder to read at a glance than a plain function.♻️ Optional refactor
- err.read = lambda: (_ for _ in ()).throw( - http.client.IncompleteRead(partial, 40) - ) + def _raise_incomplete(): + raise http.client.IncompleteRead(partial, 40) + err.read = _raise_incomplete🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_request_json_truncation.py` around lines 70 - 72, The `err.read` test stub uses a generator-throw trick that is harder to read than necessary; simplify it in the `test_request_json_truncation` setup by replacing the inline lambda with a plain helper function or equivalent direct callable that raises `http.client.IncompleteRead`, keeping the same behavior while making the intent clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_request_json_truncation.py`:
- Around line 70-72: The `err.read` test stub uses a generator-throw trick that
is harder to read than necessary; simplify it in the
`test_request_json_truncation` setup by replacing the inline lambda with a plain
helper function or equivalent direct callable that raises
`http.client.IncompleteRead`, keeping the same behavior while making the intent
clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b227df23-d492-46ab-bd2f-d10970ee8637
📒 Files selected for processing (2)
src/livepeer_gateway/orchestrator.pytests/test_request_json_truncation.py
What & why
Generalizes the diagnostic fix from #38 to the shared HTTP helper.
#38 hardened only
byoc.py::_create_byoc_payment. The same error-swallowing weakness exists across the gateway because of a Python exception-hierarchy fact:So when an endpoint advertises a
Content-Lengthbut closes the connection early, urllib raisesIncompleteRead, which slips past every existingexcept HTTPError/except URLError/except OSErrorhandler and either propagates raw or is caught by a genericexcept Exceptionthat only prints the opaqueIncompleteRead(85, 108)— discardinge.partial, the partial body that carries the real error (e.g.{"error":{"message":"..."}}).This PR hardens the shared helper
orchestrator.py::request_json(and its_http_error_bodycompanion), which is the single choke point for most of the gateway's JSON HTTP traffic.Change (
src/livepeer_gateway/orchestrator.py)import http.client.request_json: add anexcept http.client.IncompleteReadon the success-body read that decodese.partial, reports bytes-read/expected, and re-raises inside the existingLivepeerGatewayErrorwith the partial body — mirroring the pattern established in diag(byoc): surface real signer error behind opaque IncompleteRead(85, 108) #38._http_error_body: when the error-body read (e.read()) is itself truncated, fall back toe.partialinstead of returning"", so_extract_error_messagecan still recover{"error":{"message":"..."}}.Error-legibility hardening only — happy-path behaviour is unchanged (~15 LOC + tests).
Call sites now covered (via
request_json/get_json/post_json)orchestrator.discover_orchestrators(get_json)lv2v.start_lv2vandscope.py(post_json)remote_signersession + refresh (post_json,_extract_error_message)/paymenterror-body path — benefits from the_http_error_bodyfallbackOut of scope (deferred, larger)
Does not migrate
byoc.py's remaining inlineurlopencall sites (_sign_byoc_job,submit_byoc_job,submit_training_job,refresh_training_payment,list_capabilities,get_training_status) onto the shared helper. Those still roll their own reads and carry the same weakness; tracked as a follow-up because it's a mechanical refactor that also moves their test mock points.Root-cause reference
Verified:
isinstance(http.client.IncompleteRead(b'x', 5), (HTTPError, URLError, OSError))isFalse; its MRO isIncompleteRead → HTTPException → Exception.Test plan
python -m py_compile src/livepeer_gateway/orchestrator.pytests/test_request_json_truncation.py(2 tests): truncated success body and truncated error body throughrequest_json→ assert the raisedLivepeerGatewayErrorcontains the partial body + byte counts, and that the real error text ({"error":{"message":...}}) is legible.pytest tests/→ 11 passed.Relates to #38.
cc @j0sh for review (repo CODEOWNER).
Made with Cursor
Summary by CodeRabbit