Skip to content

fix(proxy): handle aborted upstream responses#101315

Merged
steipete merged 1 commit into
mainfrom
codex/pr-88052-clean
Jul 7, 2026
Merged

fix(proxy): handle aborted upstream responses#101315
steipete merged 1 commit into
mainfrom
codex/pr-88052-clean

Conversation

@steipete

@steipete steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Related to #82442. The CONNECT client-socket crash reported there was fixed by #82444; this PR covers the distinct HTTP upstream-response stream failure path.

This is the clean maintainer replacement for #88052. GitHub could not reparent the contributor fork branch after fixups, so that PR began showing unrelated main files despite the reviewed two-file tree.

What Problem This Solves

The debug proxy consumed upstream IncomingMessage bodies without an error listener. If an origin closed the connection after sending response headers, Node emitted a response-stream error that could become unhandled and crash the proxy.

The old candidate handler also tried to end the downstream response with the upstream error text. Once a 2xx status and partial body had already been forwarded, that would make a truncated response look complete and append diagnostic text to application data.

Why This Change Was Made

  • Record upstream response-stream failures as inbound capture errors.
  • Before downstream headers are committed, return a complete generic 502 Bad Gateway response with Connection: close.
  • After downstream headers are committed, destroy the downstream response so clients observe an incomplete response rather than a valid partial 2xx body.
  • Reuse the same terminal-response helper for request-level upstream failures, without reflecting upstream error strings to clients.
  • Preserve current body-preview capture behavior and current managed-proxy policy.
  • Preserve @SebTardif's contributor credit in the co-authored maintainer commit.

This follows Node's documented HTTP lifecycle: a premature upstream close after the response begins emits aborted/close/error on the incoming response, while destroying an outgoing response closes its socket. See the Node HTTP documentation.

User Impact

Broken origins no longer crash the local debug proxy. Failures before headers produce a truthful 502; failures after headers terminate the partial response; the proxy remains available for subsequent requests.

Evidence

  • Exact reviewed head: 5ef0700be070b6bc1b5bb01fcc562f6e1d1cc23c.
  • Sanitized AWS Crabbox run run_246cffe1bd2b, lease cbx_71a6b7c8518f, Node v24.15.0, public networking, no Tailscale, no instance profile.
  • Command: pnpm test src/proxy-capture/proxy-server.test.ts src/proxy-capture/proxy-server.managed-proxy.test.ts.
  • Result: 2 test files and 13 tests passed.
  • Real-wire regressions prove a complete generic 502 before headers, an incomplete partial 200 after headers with no appended error text, capture-store error records for both paths, and a successful second request through the same proxy after each failure.
  • Fresh exact-head autoreview will be recorded before merge.

The original contributor reproduction established the missing-listener crash surface. The maintainer rewrite preserves contributor credit and strengthens the client-visible contract and regression proof.

Original contributor PR: #88052.

Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Jul 7, 2026
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Land-ready proof for exact head 5ef0700be070b6bc1b5bb01fcc562f6e1d1cc23c:

  • Clean maintainer replacement for fix(proxy): add error handler on upstream response stream #88052: one co-authored commit and two changed files.
  • Best-fix review: one terminal-response helper returns a bounded generic 502 before headers, destroys partial downstream responses after headers, avoids reflecting upstream error text, and no-ops after the response already ended.
  • Direct Node 24.15 HTTP source/docs inspection confirmed premature IncomingMessage failure and ServerResponse.destroy() socket semantics.
  • Sanitized AWS Crabbox cbx_71a6b7c8518f, run run_246cffe1bd2b, Node 24.15: 13/13 focused proxy and managed-proxy tests passed.
  • Real-wire regressions prove pre-header 502, incomplete post-header response with no injected diagnostics, capture-store error records, and a healthy second request after each failure.
  • Fresh exact-head Codex autoreview: clean, no actionable findings, confidence 0.84.
  • Exact-head hosted CI: run 28838849175, all relevant gates green including lint, types, build, boundaries, QA smoke, and test shards.
  • Repo-native validation: scripts/pr review-validate-artifacts 101315; OPENCLAW_TESTBOX=1 scripts/pr prepare-run 101315.

Known proof gaps: no live external broken-origin service was used; the tests exercise the same real Node socket lifecycle on loopback. No docs or changelog entry is required by the current gates.

@steipete
steipete merged commit ae16704 into main Jul 7, 2026
116 of 119 checks passed
@steipete
steipete deleted the codex/pr-88052-clean branch July 7, 2026 03:24
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 7, 2026
Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer Maintainer-authored PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant