fix(web-incoming): close downstream stream when upstream SSE aborts

[gkoos]

resolves #84

Problem

When an SSE stream is terminated abruptly by the upstream server (e.g., backend crash or forced socket close), the downstream response remains open. SSE clients cannot detect the disconnection and appear stuck waiting for data.

Root Cause

The HTTP stream pipeline in web-incoming handles normal response end events but does not explicitly propagate upstream aborted or error events to the downstream response. Without this propagation, client-side streams can remain open despite the upstream being closed.

Solution

Added response lifecycle handlers in web-incomings.ts

Summary by CodeRabbit

• Bug Fixes

  • Only forwards request bodies on redirects when a body is present and the method is preserved.
  • Improved cleanup and error handling for proxied responses so downstream connections are reliably torn down on upstream aborts/errors.

• Tests

  • Added an SSE test to verify downstream connections close correctly when upstream sockets are aborted.

[coderabbitai]

📝 Walkthrough

Walkthrough

Updated proxy middleware to handle upstream stream aborts and errors by adding proxyRes close/error handlers that conditionally destroy the downstream response and emit server errors only when listeners exist; added an SSE test that verifies downstream closure when the upstream socket is destroyed.

Changes

Cohort / File(s)	Summary
Middleware src/middleware/web-incoming.ts	Added proxyRes.on("close") to destroy the client res when upstream response is incomplete and client is not destroyed; added proxyRes.on("error") to destroy client res with the error and conditionally server.emit("error", ...) only if listeners exist. Redirect replay behavior unchanged for other flows.
Tests test/http-proxy.test.ts	New SSE test: upstream SSE endpoint sends an initial :ok chunk then destroys its socket; test asserts the proxied downstream receives the initial chunk and that the downstream response close fires (no request error) within timeout.

Sequence Diagram(s)

sequenceDiagram
    rect rgba(66,135,245,0.5)
    participant Client
    end
    rect rgba(45,185,115,0.5)
    participant Proxy
    end
    rect rgba(245,90,90,0.5)
    participant Upstream
    end
    rect rgba(120,120,120,0.5)
    participant Server
    end

    Client->>Proxy: HTTP request (SSE)
    Proxy->>Upstream: forward request
    Upstream-->>Proxy: streaming response (proxyRes)
    Proxy->>Client: pipe streaming data
    Upstream->>Proxy: socket destroyed / aborts
    Proxy->>Proxy: proxyRes "close"/"error" handlers run
    Proxy-->>Client: destroy/end client response (notify closure)
    Proxy->>Server: emit "error" (only if server has listeners)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰
I hopped where bytes and breezes met,
The upstream sighed — the socket left.
I nudged the client, soft and clear,
"The stream is done, the end is near."
A tidy close — hop, cheer, repeat. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and concisely describes the main change: fixing downstream stream closure when upstream SSE connections abort.
Linked Issues check	✅ Passed	The PR directly addresses issue #84 by adding upstream abort/error event handlers to propagate disconnections to downstream SSE clients, meeting the core requirement.
Out of Scope Changes check	✅ Passed	All changes are scoped to the stated objective: adding lifecycle handlers in web-incoming.ts and corresponding test coverage for upstream SSE abort scenarios.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/middleware/web-incoming.ts (1)

278-286: Inconsistent error handling pattern within the same file.

This handler checks listenerCount("error") > 0 before emitting, but createErrorHandler at lines 141-145 emits errors unconditionally when no callback is provided. This creates inconsistent behavior—some upstream errors will be silently swallowed here while others from createErrorHandler will throw if unhandled.

Consider aligning with the existing pattern for consistency, or update both to use the guarded approach. The relevant code snippets show ws-incoming.ts also emits unconditionally.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/middleware/web-incoming.ts` around lines 278 - 286, The file currently
guards emitting errors in the proxyRes.on("error") handler (checking
server.listenerCount("error") > 0) but createErrorHandler emits errors
unconditionally; to make behavior consistent, change createErrorHandler to only
call server.emit("error", ...) when server.listenerCount("error") > 0 (same
guard used in the proxyRes.on handler) so both the createErrorHandler function
and the proxyRes.on("error") block use the same guarded emit pattern.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/middleware/web-incoming.ts`:
- Around line 273-277: Replace the deprecated proxyRes.on("aborted", ...)
handler with a proxyRes.on("close", ...) handler that checks if the upstream
response terminated prematurely by testing !proxyRes.complete and then destroys
the downstream res if it isn't already destroyed; locate the current listener
attached to proxyRes (the variable named proxyRes in this middleware) and change
the event and condition accordingly (remove the "aborted" listener and use
"close" + !proxyRes.complete to detect premature termination).

---

Nitpick comments:
In `@src/middleware/web-incoming.ts`:
- Around line 278-286: The file currently guards emitting errors in the
proxyRes.on("error") handler (checking server.listenerCount("error") > 0) but
createErrorHandler emits errors unconditionally; to make behavior consistent,
change createErrorHandler to only call server.emit("error", ...) when
server.listenerCount("error") > 0 (same guard used in the proxyRes.on handler)
so both the createErrorHandler function and the proxyRes.on("error") block use
the same guarded emit pattern.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 88b9b518-fef8-4797-8003-18fb7b85bb3c

📥 Commits

Reviewing files that changed from the base of the PR and between 4b03756 and 04ec458.

📒 Files selected for processing (2)

• src/middleware/web-incoming.ts
• test/http-proxy.test.ts

[pi0]

Core fix is solid — proxyRes.on("close") with !proxyRes.complete is the correct modern pattern for detecting premature upstream termination, and the proxyRes.on("error") handler properly destroys the downstream response.

Pushed one fix: reverted the createErrorHandler change that guarded server.emit("error") with listenerCount > 0. That change was unrelated to the SSE fix and would silently swallow errors when no listener exists (breaking Node.js EventEmitter semantics).

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.19%. Comparing base (4b03756) to head (9a7906b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/middleware/web-incoming.ts	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
- Coverage   96.47%   96.19%   -0.29%     
==========================================
  Files           8        8              
  Lines         596      604       +8     
  Branches      222      225       +3     
==========================================
+ Hits          575      581       +6     
- Misses         21       23       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

🧹 Nitpick comments (1)

src/middleware/web-incoming.ts (1)

278-286: Add null check for server for consistency with the rest of the file.

Lines 253, 266, and 288 guard server access with if (server), but line 283 accesses server.listenerCount directly. While server is always passed in practice, adding the guard maintains consistency and defensive coding.

♻️ Suggested fix

       proxyRes.on("error", function (err) {
         if (!res.destroyed) {
           res.destroy(err);
         }

-        if (server.listenerCount("error") > 0) {
+        if (server && server.listenerCount("error") > 0) {
           server.emit("error", err, req, res, currentUrl);
         }
       });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/middleware/web-incoming.ts` around lines 278 - 286, The proxyRes "error"
handler currently calls server.listenerCount and server.emit without checking
server; add a guard (if (server)) around the block that checks
server.listenerCount and emits the error so it matches the other guards in this
file — update the proxyRes.on("error", ...) handler to call res.destroy(err) as
before, then only call server.listenerCount(...) and server.emit(...) when
server is truthy to avoid potential null/undefined access.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/middleware/web-incoming.ts`:
- Around line 278-286: The proxyRes "error" handler currently calls
server.listenerCount and server.emit without checking server; add a guard (if
(server)) around the block that checks server.listenerCount and emits the error
so it matches the other guards in this file — update the proxyRes.on("error",
...) handler to call res.destroy(err) as before, then only call
server.listenerCount(...) and server.emit(...) when server is truthy to avoid
potential null/undefined access.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: df3afbda-63e1-44f3-9de3-6c935131335b

📥 Commits

Reviewing files that changed from the base of the PR and between 93b262f and 9a7906b.

📒 Files selected for processing (2)

• src/middleware/web-incoming.ts
• test/http-proxy.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• test/http-proxy.test.ts