fix(node): don't fire abort signal on normal request completion

[dcitron]

Fixes #152

Problem

The Node adapter was listening to req.once("end", abort) which fires when the request body is done reading, not when the client disconnects. This caused the abort signal to fire on every single request.

Solution

• Remove req.once("end", abort) listener
• Listen to res.on("close") instead
• Only fire abort if !res.writableEnded (connection closed before response completed)

This matches how @hono/node-server handles abort signals.

Testing

Added tests in test/node.test.ts to verify:

• Successful GET requests don't fire abort
• Successful POST requests don't fire abort
• Client disconnects do fire abort

Summary by CodeRabbit

• Bug Fixes

  • Improved abort handling so request lifecycles follow response closure, adding a fallback when no response is present for more reliable aborts and fewer missed or spurious aborts.
  • Ensured abort paths use a centralized controller for consistent error and completion behavior.

• Tests

  • Added tests for request-signal behavior: successful GET/POST and client-disconnect abort handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces request end-based abort handling with response close-based handling in the Node adapter, stores the NodeServerResponse on the request instance, routes aborts through the stored AbortController, and adds tests validating GET, POST, and client-disconnect abort behaviors plus a test adjustment for expected abort counts.

Changes

Cohort / File(s)	Summary
Node adapter abort handling src/adapters/_node/request.ts	Adds a private #res field initialized from constructor context; replaces req.once("end", ...) with res.on("close", ...); on close, aborts via the stored AbortController only if the response errored or hasn't finished sending; includes fallback abort when no response object exists.
Abort count test refactor test/_tests.ts	Changes default expectedAbortCount to 1, sets it to fetchCount for Deno, removes Bun-specific override and adjusts test expectations accordingly.
Request signal behavior tests test/node-adapters.test.ts	Adds "request signal" suite with tests for GET (no abort), POST (no abort and server receives body), and client disconnect (abort fires and server-side stream handles/force-closes).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Inspect src/adapters/_node/request.ts for correct wiring of #res, correct detection of res.writableEnded, and proper use of the stored AbortController to avoid double-abort or missed aborts.
• Validate test/node-adapters.test.ts reliably simulates client disconnects and asserts signal behavior across environments.
• Confirm test/_tests.ts runtime-specific logic matches CI/runtime expectations (Deno vs others).

Possibly related issues

• Node adapter aborts requests after the socket closes, triggering Undici “Response body … disturbed” crash #142 — Likely related: both modify src/adapters/_node/request.ts to drive aborts from response close and route through an AbortController.
• #152 — Addressed: replaces end-based abort with res.on("close") plus res.writableEnded check so abort fires only on client disconnects or errors, not on every successful request.

Poem

🐰 I nibble bugs beneath the moon,

I hop where streams and signals tune,
I wait for res to close its door,
Not every end, but errors more —
Hooray, the tests applaud the rune.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: preventing the abort signal from firing on normal request completion in the Node adapter.
Linked Issues check	✅ Passed	The pull request implements all coding requirements from issue #152: removes req.once('end', abort), listens to res.on('close'), checks res.writableEnded before aborting, and adds comprehensive tests for successful requests and client disconnects.
Out of Scope Changes check	✅ Passed	All changes are directly within scope: modifications to request abort handling in the Node adapter, test logic adjustments for abort counting, and new test cases validating the fix.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25db24c and 7e008c1.

📒 Files selected for processing (1)

• test/node-adapters.test.ts (1 hunks)

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

• test/node-adapters.test.ts

---

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: 0

🧹 Nitpick comments (3)

src/adapters/_node/request.ts (1)

59-69: Abort wiring now matches desired semantics; consider the res-optional case

This refactor correctly moves abort signaling off of req.once("end") and onto req.once("error") + res.on("close") with the !res.writableEnded guard, which aligns with the “only on error / client disconnect before response completion” requirement from issue #152 and the PR description. Nice fix.

One thing to double‑check: NodeRequestContext.res is still typed as optional, but _abortController now relies entirely on res for client‑disconnect detection (when there is no request error). If NodeRequest is ever instantiated with only { req } (no res), request.signal will no longer abort on client disconnects in those code paths.

You might want to either:

• Make res effectively required for all Node server entrypoints that rely on request.signal, or
• Add a fallback for the !res case, e.g. attach req.once("aborted", abort) or req.once("close", abort) when no res is provided.

That keeps the new semantics while preserving correctness in any request‑only contexts.

Also applies to: 112-135

test/_tests.ts (1)

145-148: Abort count expectation looks fine; avoid noisy console logging in tests

The new expectedAbortCount logic (default 1, with the Deno special case) matches the current suite behavior and the updated abort semantics.

The console.log(opts.runtime) inside the assertion, though, will run on every test execution and can clutter CI logs. Consider either removing it or gating it behind a debug flag (or leaving the commented‑out detailed log if you need ad‑hoc diagnostics).

Also applies to: 157-158

test/node-adapters.test.ts (1)

178-284: Request‑signal behavior tests are good; tighten sync to avoid time‑based flakiness

These three tests nicely pin down the new semantics:

• No abort on successful GET.
• No abort on successful POST, and the body is fully readable.
• Abort fires when the client disconnects mid‑stream.

To make them more robust long‑term:

• Instead of fixed setTimeout(50/100) sleeps, consider awaiting concrete signals:
  • For the disconnect case, store the fetch promise and await its rejection after controller.abort(); that guarantees the client‑side abort has fully propagated before you assert abortFired.
  • For the “no abort” cases, an extra microtask (setImmediate / queueMicrotask) or awaiting server.close() after the response may be enough, and avoids hardcoded timing.

That should reduce the chance of rare timing‑related flakes while keeping the semantics identical.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a6139d and a69ea09.

📒 Files selected for processing (3)

• src/adapters/_node/request.ts (3 hunks)
• test/_tests.ts (2 hunks)
• test/node-adapters.test.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/adapters/_node/request.ts (2)

src/types.ts (1)

• NodeServerResponse (352-354)

test/bench-node/_run.mjs (1)

• res (47-51)

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/adapters/_node/request.ts (1)

112-143: Abort wiring via res.close / req.close now matches the intended semantics; consider a minor listener tweak

The new _abortController setup correctly:

• Uses a stable abortController reference in the closure.
• Aborts on req errors.
• For normal adapter usage (with res), aborts only when res emits close and !res.writableEnded, avoiding the previous “abort on successful completion” behavior.
• Falls back to req.once("close") with a !req.complete check when no res is available, which is a reasonable best-effort for request-only contexts.

This aligns with the PR objective of firing abort only on client disconnects or errors rather than at the end of a successful request.

One small improvement you might consider (non-blocking):

• Use once("close") for the response as well, to make the one-shot nature of the handler explicit and avoid any risk of multiple invocations if close were ever re-emitted:

-        if (res) {
-          // Primary path: detect client disconnect via response close
-          res.on("close", () => {
+        if (res) {
+          // Primary path: detect client disconnect via response close
+          res.once("close", () => {
             if (req.errored) {
               abort(req.errored);
             } else if (!res.writableEnded) {
               abort();
             }
           });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a69ea09 and 5e290d4.

📒 Files selected for processing (3)

• src/adapters/_node/request.ts (3 hunks)
• test/_tests.ts (1 hunks)
• test/node-adapters.test.ts (1 hunks)

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

• test/_tests.ts
• test/node-adapters.test.ts

🧰 Additional context used

🧬 Code graph analysis (1)

src/adapters/_node/request.ts (2)

src/types.ts (1)

• NodeServerResponse (352-354)

test/bench-node/_run.mjs (1)

• res (47-51)

🔇 Additional comments (1)

src/adapters/_node/request.ts (1)

55-73: Storing #res on the request to support abort semantics looks correct

Wiring ctx.res into the private #res field and exposing it via runtime.node gives the abort logic access to the response lifecycle without changing the public surface. There are no obvious correctness or lifetime issues with this addition.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/adapters/_node/request.ts (1)

112-145: Core fix looks correct.

The logic properly distinguishes between:

1. Normal request completion (res.writableEnded === true) → no abort
2. Client disconnect before response completes (!res.writableEnded) → abort
3. Request errors → abort with error

This aligns with the expected behavior described in the issue.

Minor nit: The optional chaining on line 120 is redundant since abortController is guaranteed to be defined at this point (assigned on line 117, from the controller created on line 114).

 const abort = (err?: any) => {
-  abortController?.abort?.(err);
+  abortController.abort(err);
 };

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e290d4 and 25db24c.

📒 Files selected for processing (1)

• src/adapters/_node/request.ts (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/adapters/_node/request.ts (1)

src/types.ts (1)

• NodeServerResponse (352-354)

🔇 Additional comments (2)

src/adapters/_node/request.ts (2)

59-68: LGTM!

Clean addition of the response reference. The optional typing correctly reflects that res may not be present in all contexts (e.g., request-only scenarios).

---

134-142: Fallback path is sensible.

Using req.complete to detect incomplete request body reception is the right check for request-only contexts. This ensures abort fires only when the client disconnects mid-request rather than on normal completion.

When would this fallback path actually be exercised? Consider adding a brief inline comment explaining which scenarios result in a NodeRequestContext without a res object, to help future maintainers understand when this code path is taken.

[autofix-ci]

Hi! I'm autofix.ci, a bot that automatically fixes trivial issues such as code formatting in pull requests.

I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:

1. Allow edits by maintainers for your pull request, and then re-trigger CI (for example by pushing a new commit).
2. Manually fix the issues identified for your pull request (see the GitHub Actions output for details on what I would like to change).