Skip to content

fix(deduplicate): settle the deduplicated request when it is aborted - #5673

Open
pacocartones wants to merge 1 commit into
nodejs:mainfrom
pacocartones:fix/deduplicate-abort-settle
Open

fix(deduplicate): settle the deduplicated request when it is aborted#5673
pacocartones wants to merge 1 commit into
nodejs:mainfrom
pacocartones:fix/deduplicate-abort-settle

Conversation

@pacocartones

Copy link
Copy Markdown
Contributor

This relates to...

No open issue found for this; reported directly with a reproduction.

Rationale

With interceptors.deduplicate(), a request that joins an in-flight identical
request (the "waiting" request) never settles if it is aborted: its promise
neither resolves nor rejects, so callers hang forever (an await never returns,
Promise.all never completes). Two reachable variants, both reproduced against
current main (86b6299):

  1. Aborted after joining. The signal fires while the primary request is
    still in flight. The abort reaches the waiting request's synthetic
    controller (abort(reason) in lib/handler/deduplication-handler.js), which
    only flips flags and marks the handler done — nothing ever calls the waiting
    handler's onResponseError, so the caller is never notified.
  2. Already-aborted signal at join time. onRequestStart aborts the
    synthetic controller synchronously; addWaitingHandler sees
    controller.aborted, marks the handler done and returns true (deduplicated
    "successfully") — again without ever erroring the request.

The primary request is unaffected in both cases and completes normally; only
the joined caller hangs.

Changes

  • The synthetic waiting controller's abort(reason) now settles the waiting
    request: it notifies the handler via onResponseError with the abort reason
    (a RequestAbortedError when no reason was given), guarded so a repeated
    abort() notifies at most once. This covers both variants, since the
    already-aborted case also flows through controller.abort().
  • #errorWaitingHandler now delegates the notification to
    controller.abort(err) instead of calling onResponseError itself, so each
    waiting handler is still errored exactly once.
       abort: (reason) => {
+        if (state.aborted) {
+          return
+        }
+
         state.aborted = true
         state.reason = reason ?? null
         waitingHandler.done = true
         waitingHandler.pendingTrailers = null
         waitingHandler.bufferedChunks = []
         waitingHandler.bufferedBytes = 0
+
+        try {
+          handler.onResponseError?.(waitingHandler.controller, state.reason ?? new RequestAbortedError())
+        } catch {
+          // Ignore errors from waiting handlers
+        }
       }

Out of scope (deliberately not changed): aborting the primary request
propagates the error to all joined requests via onResponseError instead of
re-dispatching them independently. That pre-existing behavior is debatable but
works as designed and settles every caller; this PR only fixes the requests
that never settled.

Features

N/A

Bug Fixes

  • deduplicate(): a deduplicated (joined) request whose signal aborts now
    rejects with the AbortError instead of hanging forever; same for a request
    joined with an already-aborted signal.

Breaking Changes and Deprecations

None.

Verification

Every command below was run and its output captured verbatim. The record is
reproducible — the exact commands are included so you can re-run them yourself.

red — main (86b6299) without the fix (must fail: the joined request never
settles, the 1000 ms race in the test wins)

$ git checkout origin/main -- lib/handler/deduplication-handler.js
$ node --test --test-name-pattern="abort" test/interceptors/deduplicate.js
✖ aborting a deduplicated request settles it with an AbortError (1029.1597ms)
✖ a deduplicated request with an already aborted signal rejects with an AbortError (1019.5229ms)
✖ Deduplicate Interceptor (2051.4565ms)
ℹ tests 2
ℹ pass 0
ℹ fail 2
✖ failing tests:
✖ aborting a deduplicated request settles it with an AbortError (1029.1597ms)
  + actual - expected
    actual: undefined,
    expected: 'AbortError',
✖ a deduplicated request with an already aborted signal rejects with an AbortError (1019.5229ms)
  + actual - expected
    actual: undefined,
    expected: 'AbortError',

(actual: undefined because the outcome is the literal 'timed-out' from the
Promise.race watchdog: the request did not settle within 1000 ms while the
primary request to the same origin completed in ~100 ms.)

green — with the fix (must pass)

$ node --test --test-name-pattern="abort" test/interceptors/deduplicate.js
▶ Deduplicate Interceptor
  ✔ aborting a deduplicated request settles it with an AbortError (179.9118ms)
  ✔ a deduplicated request with an already aborted signal rejects with an AbortError (126.6933ms)
✔ Deduplicate Interceptor (309.7645ms)
ℹ tests 2
ℹ pass 2
ℹ fail 0

standalone reproduction against a live server (probe with explicit
settled-tracking, not just "no answer within Xs"): on main the joined request
is still unsettled 3000 ms after the primary resolved; with the fix it rejects
with AbortError.

# main:
V1 lider: RESOLVED: status=200 body="data-/v1"
V1 unida-abortada: HUNG (still unsettled after 3000ms)
V2 lider: RESOLVED: status=200 body="data-/v2"
V2 unida-pre-abortada: HUNG (still unsettled after 3000ms)
CTL lider: RESOLVED: body="data-/ctl"
CTL unida-sin-abort: RESOLVED: status=200 body="data-/ctl"

# with the fix:
V1 unida-abortada: REJECTED: AbortError: This operation was aborted
V2 unida-pre-abortada: REJECTED: AbortError: This operation was aborted
CTL unida-sin-abort: RESOLVED: status=200 body="data-/ctl"

(V1 = joined request aborted after joining; V2 = joined with an
already-aborted signal; CTL = joined without abort, resolves normally.)

area suites with the fix (must pass)

$ node --test test/interceptors/deduplicate.js
ℹ tests 36  ℹ pass 36  ℹ fail 0

$ node --test test/interceptors/cache.js
ℹ tests 87  ℹ pass 87  ℹ fail 0

$ node --test test/interceptors/response-error.js test/interceptors/redirect.js \
       test/interceptors/retry.js test/interceptors/interceptors-on-client.js
ℹ tests 109  ℹ pass 109  ℹ fail 0

lint (must pass)

$ npm run lint
> undici@8.10.0 lint
> eslint --cache
[exit code 0]

Not verified locally: the remaining test suites (npm test runs 14 of them);
left to CI, as this change only touches the deduplication handler and its
interceptor tests.

Status


Implementation and validation used AI assistance; I reviewed the final diff and results.

A request that joined an in-flight deduplicated request never settled if
its signal aborted it: the synthetic waiting controller's abort() only
flipped flags and marked the handler done, but nothing ever called the
waiting handler's onResponseError, so the caller's promise hung forever.
The same happened when the signal was already aborted at join time.

The waiting controller's abort() now notifies the handler via
onResponseError with the abort reason (RequestAbortedError when no
reason was given), and errorWaitingHandler delegates the notification
to abort() so each waiting handler is errored exactly once.

Signed-off-by: pacocartones <manusanchezhl@gmail.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.36%. Comparing base (dd85997) to head (49efa72).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/handler/deduplication-handler.js 66.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5673      +/-   ##
==========================================
- Coverage   93.43%   93.36%   -0.08%     
==========================================
  Files         110      110              
  Lines       38733    38782      +49     
==========================================
+ Hits        36190    36207      +17     
- Misses       2543     2575      +32     

☔ View full report in Codecov by Harness.
📢 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.

@pacocartones

Copy link
Copy Markdown
Contributor Author

For the reviewer: the 2 red jobs are the known flaky test/http2-request-never-settles.js (fails identically on main, e.g. run 31085361670, and on #5637) — 1504/1505 pass and the single failure is that file, nothing in this diff touches http2. Everything else is green, including the full deduplicate/interceptor area.

@pacocartones

Copy link
Copy Markdown
Contributor Author

Heads-up on the red check: the only failure is test/http2-request-never-settles.js, and its root cause is now identified upstream — nodejs/node#64841, a V8 Maglev SIGSEGV regression in Node ≥ 24.15.0 on Linux, reproduced locally with the issue's recipe (crash log byte-identical to the CI signature; still present on v24.19.0). Not related to this PR's changes; a re-run may pass by luck, but the real fix has to land in Node. (More data in #5674's description.)

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