Skip to content

fix(http): surface truncated response body in shared request_json helper#39

Open
seanhanca wants to merge 1 commit into
mainfrom
fix/incomplete-read-shared-http-helper
Open

fix(http): surface truncated response body in shared request_json helper#39
seanhanca wants to merge 1 commit into
mainfrom
fix/incomplete-read-shared-http-helper

Conversation

@seanhanca

@seanhanca seanhanca commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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:

http.client.IncompleteRead subclasses HTTPExceptionNOT HTTPError / URLError / OSError.

So when an endpoint advertises a Content-Length but closes the connection early, urllib raises IncompleteRead, which slips past every existing except HTTPError / except URLError / except OSError handler and either propagates raw or is caught by a generic except Exception that only prints the opaque IncompleteRead(85, 108)discarding e.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_body companion), which is the single choke point for most of the gateway's JSON HTTP traffic.

Change (src/livepeer_gateway/orchestrator.py)

  • Add import http.client.
  • request_json: add an except http.client.IncompleteRead on the success-body read that decodes e.partial, reports bytes-read/expected, and re-raises inside the existing LivepeerGatewayError with 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 to e.partial instead of returning "", so _extract_error_message can 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)

  • Discoveryorchestrator.discover_orchestrators (get_json)
  • Orchestrator selection — discovery-backed selection path
  • LV2V startlv2v.start_lv2v and scope.py (post_json)
  • Signer session / refreshremote_signer session + refresh (post_json, _extract_error_message)
  • Signer /payment error-body path — benefits from the _http_error_body fallback

Out of scope (deferred, larger)

Does not migrate byoc.py's remaining inline urlopen call 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)) is False; its MRO is IncompleteRead → HTTPException → Exception.

Test plan

  • python -m py_compile src/livepeer_gateway/orchestrator.py
  • Added tests/test_request_json_truncation.py (2 tests): truncated success body and truncated error body through request_json → assert the raised LivepeerGatewayError contains the partial body + byte counts, and that the real error text ({"error":{"message":...}}) is legible.
  • Full suite green: pytest tests/ → 11 passed.

Relates to #38.

cc @j0sh for review (repo CODEOWNER).

Made with Cursor

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of truncated HTTP responses so requests now return clearer errors instead of failing with empty or vague messages.
    • Error details now include partial response text and byte-count information when a server closes the connection too early.
    • Better preserved HTTP error body content when only part of the response is available.

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>
@seanhanca seanhanca requested a review from j0sh as a code owner July 9, 2026 16:35
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Truncated HTTP Response Handling

Layer / File(s) Summary
IncompleteRead handling in orchestrator
src/livepeer_gateway/orchestrator.py
Imports http.client, extends _http_error_body to return decoded partial bytes on IncompleteRead, and adds an except branch in request_json that raises LivepeerGatewayError with partial/expected byte counts and a partial-body snippet.
Tests for truncated response handling
tests/test_request_json_truncation.py
New test module with a helper class simulating IncompleteRead on .read(), and tests asserting LivepeerGatewayError correctly surfaces truncation details for both successful and error HTTP responses.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: improving request_json to surface truncated response bodies and error details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/incomplete-read-shared-http-helper

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/test_request_json_truncation.py (1)

70-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider 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

📥 Commits

Reviewing files that changed from the base of the PR and between cc31b56 and 21186a5.

📒 Files selected for processing (2)
  • src/livepeer_gateway/orchestrator.py
  • tests/test_request_json_truncation.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant