Skip to content

fix(proxy): abort paid upstream on client disconnect for chat path - #279

Merged
VickyXAI merged 1 commit into
BlockRunAI:mainfrom
erhnysr:fix/chat-path-client-disconnect-abort
Aug 30, 2026
Merged

fix(proxy): abort paid upstream on client disconnect for chat path#279
VickyXAI merged 1 commit into
BlockRunAI:mainfrom
erhnysr:fix/chat-path-client-disconnect-abort

Conversation

@erhnysr

@erhnysr erhnysr commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Describe your changes

The media handlers all abort their upstream on client disconnect (#251, fixed for /v1/images/image2image in v0.12.252 and for audio + chat-path /img2img in v0.12.253). The main /v1/chat/completions path — the highest-traffic one — was the one handler still missing it.

proxyRequest wired its disconnect abort to req.on("close"):

const onClientClose = () => {
  if (!globalController.signal.aborted) { ... globalController.abort(); }
};
req.on("close", onClientClose);

On Node (verified on v24.15.0) an IncomingMessage emits "close" when the request-body readable finishes, not when the client hangs up. The body is fully drained near the top of proxyRequest, so that event had already fired by the time the listener was attached ~1,500 lines below — onClientClose never ran. globalController was therefore only ever aborted by the request timeout, and a caller that hung up mid-request left the paid x402 upstream running to completion for a response nobody would receive — the exact orphan-upstream problem #251 fixed for the media paths.

Change

Move the abort to res.on("close") (guarded by !res.writableEnded), mirroring every media handler — e.g. /v1/images/image2image:

res.on("close", () => {
  if (!res.writableEnded) clientAbort.abort();
});

The four removeListener cleanup sites are migrated from req to res to match.

Reproduction

New regression test src/proxy.chat-abort.test.ts (same harness/style as src/proxy.img2img-abort.test.ts): the client POSTs /v1/chat/completions, the upstream mock holds the socket open, and the client destroy()s mid-request. The mock's res.on("close")-with-!writableEnded counter observes the abort.

  • Before the fix: upstreamAborted stays 0 — the upstream is orphaned (surfaces as Response finished with error: Premature closeAll 1 models failed (HTTP 500)).
  • After the fix: the upstream socket observes the abort (upstreamAborted === 1), and the test also completes far faster since the abort fires immediately instead of waiting out the premature-close path.

Full suite: 64 files / 724 tests passing.

Checklist

Summary by CodeRabbit

  • Bug Fixes

    • Client disconnections during chat completions now properly cancel ongoing paid upstream requests.
    • Prevents unnecessary processing and costs after a client connection is closed.
  • Tests

    • Added regression coverage to verify upstream requests are aborted exactly once when clients disconnect.
  • Chores

    • Released version 0.12.254.

…0.12.254)

The media handlers all abort their upstream on client disconnect (BlockRunAI#251,
v0.12.252-253), but the main /v1/chat/completions path never did.
proxyRequest wired its abort to req.on("close"), and on Node (verified on
v24.15.0) an IncomingMessage emits "close" when the request-body readable
finishes, not when the client hangs up. The body is fully drained near the
top of proxyRequest, so that event had already passed by the time the
listener was attached far below -- onClientClose never ran and a caller that
hung up mid-request left the paid x402 upstream running to completion for a
response nobody would receive.

Move the abort to res.on("close") (guarded by !res.writableEnded), mirroring
every media handler, and migrate the four removeListener cleanup sites to
match. Covered by src/proxy.chat-abort.test.ts (upstream socket observes the
abort after the client destroys its request).
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a7744b62-a619-4cb2-882c-4c337ba9f880

📥 Commits

Reviewing files that changed from the base of the PR and between 477413a and fe8e871.

⛔ Files ignored due to path filters (5)
  • dist/cli.js is excluded by !**/dist/**
  • dist/cli.js.map is excluded by !**/dist/**, !**/*.map
  • dist/index.js is excluded by !**/dist/**
  • dist/index.js.map is excluded by !**/dist/**, !**/*.map
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • CHANGELOG.md
  • package.json
  • src/proxy.chat-abort.test.ts
  • src/proxy.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The proxy now listens for response closure to abort unfinished paid chat upstream requests. Cleanup paths use the response listener. An end-to-end regression test verifies upstream closure after client disconnect. The package version and changelog were updated to v0.12.254.

Changes

Chat request cancellation

Layer / File(s) Summary
Response-close abort handling
src/proxy.ts
proxyRequest attaches onClientClose to res.on("close") and removes the listener from the response across cleanup paths.
Regression coverage and release update
src/proxy.chat-abort.test.ts, package.json, CHANGELOG.md
The regression test verifies upstream socket closure after client disconnect. The package version and changelog move to 0.12.254.

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

Merge Risk: ⚪ Minimal · up to fe8e8

The change localizes client-disconnect cancellation to the chat request path and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant proxyRequest
  participant UpstreamHTTPServer
  Client->>proxyRequest: Send chat completion request
  proxyRequest->>UpstreamHTTPServer: Forward paid request
  Client--xproxyRequest: Close connection
  proxyRequest->>UpstreamHTTPServer: Abort unfinished request
Loading

Suggested reviewers: 1bcmax, killerqueen-z, 0xcheetah1

🚥 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 and concisely describes the main change: aborting paid upstream requests on client disconnects in the proxy chat path.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (2 skipped: 2 …
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (2 skipped: 2 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

VickyXAI pushed a commit that referenced this pull request Aug 30, 2026
…lient cancels as 300s timeouts (v0.12.254)

Follow-ups on #279 (@erhnysr), landed in the same release:

- proxyRequest now checks res.destroyed right after attaching the
  res "close" listener. Context compression and the balance check run
  between draining the body and the attach; a client that hung up in
  that window had already emitted "close", so the listener alone never
  fired and the paid upstream still ran.
- A client cancel used to reach the shared abort exits and surface as
  "Request timed out after 300000ms" -> onError + balance cache
  invalidation. Introduce ClientDisconnectedError, return quietly from
  the catch, keep the cache. Regression test asserts onError is silent
  and gates the disconnect on the upstream being reached (no fixed
  500ms timer).
@VickyXAI
VickyXAI merged commit fe8e871 into BlockRunAI:main Aug 30, 2026
1 check passed
@VickyXAI

Copy link
Copy Markdown
Contributor

Merged as-is via fast-forward (fe8e871) and shipped in v0.12.254 with two small follow-ups on top (bd13340): a res.destroyed check right after the listener attach (covers a hang-up during the context-compression / balance-check awaits, where close had already fired), and a ClientDisconnectedError so a client cancel no longer surfaces as Request timed out after 300000ms + a balance-cache invalidation. Your test now also asserts onError stays silent and gates the disconnect on the upstream being reached instead of a fixed timer. Great diagnosis — thank you!

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.

2 participants