Uh oh!
There was an error while loading. Please reload this page.
fix(h1): stop unref'ing the idle socket validation immediate - #5609
Closed
marko1olo wants to merge 1 commit into
Closed
fix(h1): stop unref'ing the idle socket validation immediate#5609marko1olo wants to merge 1 commit into
marko1olo wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #5609 +/- ##
==========================================
- Coverage 93.46% 93.45% -0.02%
==========================================
Files 110 110 Lines 38777 38776 -1 ==========================================
- Hits 36244 36238 -6 - Misses 2533 2538 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kriszyp added a commit
to HarperFast/harper
that referenced
this pull request
Aug 1, 2026
…) stall Point the file-header comments at nodejs/undici#5600 (the unref'd setImmediate idle-socket-validation stall, bundled into Node via undici 8.9.0/26.5.1) and its fixnodejs/undici#5609 — already merged upstream but not yet in a released undici/Node build. Replaces the PR's earlier "recommend filing it" note, which predated finding the existing issue. Co-Authored-By: Claude Opus <noreply@anthropic.com>
marko1olo
commented
Aug 1, 2026
ContributorAuthor
Rebased onto current |
Ethan-Arrowood
commented
Aug 4, 2026
Collaborator
Is this fix still needed? The associated issue has already been closed as "fixed". |
Reusing an idle keep-alive socket can stall for up to a fast-timers tick (~500ms) before the request reaches the wire. With a pool of one connection and a 10ms server, six sequential requests take over a second instead of the ~60ms the server accounts for. `scheduleIdleSocketValidation` moved from an unref'd `setTimeout(..., 0)` to `setImmediate` in nodejs#5499, and the `unref?.()` came along with it. An unref'd immediate does not hold the loop open, so on an otherwise idle loop the check phase can be reached only once the next fast-timers tick wakes things up, and the queued request waits for it. The request the validation exists for is already queued at that point, so there is nothing to keep alive artificially: the immediate is a one-shot that runs on the very next check phase and `clearIdleSocketValidation` cancels it when the socket goes away. test/issue-5600.js fails on the current code with a slowest request around 425ms and passes with the immediate left ref'd; on this machine the six requests go from 1095/1565/2045ms across runs to 122/137/219ms. Signed-off-by: marko1olo <marko1olo@users.noreply.github.com>
marko1oloforce-pushed
the
fix/idle-socket-validation-unref
branch
from
August 14, 2026 04:11
7fd4337 to
0f5b5e9Comparemarko1olo
commented
Aug 14, 2026
ContributorAuthor
Hey @Ethan-Arrowood, I just re-ran the reproduction benchmark against current |
7 tasks
kriszyp added a commit
to HarperFast/harper
that referenced
this pull request
Aug 25, 2026
…rs (Node 26.5.1 flakiness) (#2029) * fix(test): stop using fetch() in the two tests hit by the Node 26.5.1 CI flakiness (#2025) Root cause isolated with a Harper-independent minimal repro (two bare node:http servers, no Harper code at all): Node 26.5.1's bundled undici 8.9.0 has a client-side fetch() regression that produces intermittent latency spikes (a few hundred ms up to 20+ seconds) when a process fetches from more than one origin/port, or mixes GET/POST. Confirmed absent on 26.5.0, v22, and v24 in the same CI run; confirmed absent when the identical Harper server is hit via node:http instead of fetch(). This is not an llhttp/server-side parsing issue and not a Harper logic defect — Harper's own TTL-reset and live-config-reload behavior is correct on every Node version tested. ttlResetOnWrite.test.ts and static-cache-headers.test.ts both use fetch() as their test client and both interleave calls across origins/methods, so they're squarely in the blast radius: a stalled poll or update request reads as "TTL never reset" or "config reload never landed" when it's really the fetch call itself that never landed in time. Switching both to node:http (which supertest, used elsewhere in these suites, already relies on) removes the false negatives entirely — 5/5 clean runs each on 26.5.1 after the change, matching 26.5.0/v22/v24 timing. ttlResetOnWrite.test.ts also gets a real, independent robustness fix: the reset-check was a single point-in-time sample, vulnerable to that same sample landing late and crossing the reset-expiry boundary. It now polls across the whole valid window (RESET_POLL_INTERVAL_MS/RESET_CHECK_SAFETY_MS) so a single slow request can't produce a false NO-RESET. Issue #2025 stays open: the underlying undici regression is upstream (not fixable in Harper source) and needs to be filed against nodejs/undici with the minimal repro; harper-pro's analytics.test.mjs likely shares this root cause (also fetch()-based, cross-origin) and should get the same fix separately. addNodeLeaderNoMeshLeak.test.mjs does not use fetch() at all and is very likely an unrelated cluster-mesh issue — not touched here. Refs #2025 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): close pre-push review findings on the fetch()->node:http swap Fixes from the independent pre-push review of a59d762: - ttlResetOnWrite.test.ts: pollForPresent's retry loop inherited httpRequest's full 5s default timeout per sample against a ~600ms measurement window, so a single slow sample still consumed the whole window and produced exactly the false NO-RESET the poll was added to prevent (review-major). getRecord now takes a timeoutMs override; polling uses a 300ms per-sample bound (RESET_POLL_TIMEOUT_MS) so a slow/stalled sample is abandoned in time for a retry within the window. - ttlResetOnWrite.test.ts: a transport failure (timeout/connection reset) during the reset-check window was indistinguishable from a genuine 404 and got reported to a human as "NO-RESET — defect" (review-major, misleading- logging class). pollForPresent now tracks whether any sample completed cleanly (200 or 404); if none did, the surface is reported INCONCLUSIVE instead of NO-RESET. - ttlResetOnWrite.test.ts: resetDeadline anchored on the update's post-response timestamp, eroding the 200ms safety margin by the update's own round-trip (review-nit); now anchors on when the update was issued. - static-cache-headers.test.ts: the replacement getPath() had no timeout and no `res` error handling, so a dropped/stalled response left the promise pending forever with no upper bound — the same unbounded-stall failure class this PR exists to eliminate, reintroduced in the file whose header claims immunity to it (review-major). Added a timeout + req.destroy on timeout + res.on('error', reject), matching the sibling file's httpRequest shape. - static-cache-headers.test.ts: the 200-status assertion ran inside the `res.on('end')` callback, outside the promise's reject path, so a non-200 threw an orphaned/confusingly-framed failure instead of a clean rejection (review-minor); wrapped in try/catch -> reject. - static-cache-headers.test.ts: getPath dropped the URL's query string (review-nit, latent — no current caller passes one); now includes u.pathname + u.search, matching ttlResetOnWrite.test.ts's httpRequest. - static-cache-headers.test.ts: applyAndWaitForCacheControl still declared the stale fetch Response return type after the SimpleResponse swap (review-nit); corrected. Independent review (codex + gemini + grok + harper-domain) also raised three findings that were adjudicated and dropped as not real: response-side abort leaving httpRequest pending in the TTL suite (measured: the existing timeout + req.destroy already bounds it), a redundant post-deadline poll sample risking false negatives (measured: it can only convert false->true, never the reverse), and multi-value header arrays via the `as string` cast (only single-valued headers are ever read). Reverified after these changes: 3 consecutive full-suite runs of both files on Node 26.5.1 (rocksdb), all green; 26.5.0 control run also green. Refs #2025 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): close review findings on ttlResetOnWrite's httpRequest/pollForPresent - httpRequest(): add the missing res.on('error', reject) guard (flagged by both the claude-review and gemini-code-assist bots) so a response-stream error rejects the promise instead of crashing the test process, matching the sibling static-cache-headers.test.ts helper. - pollForPresent(): stop running an unconditional final sample after deadlineAt with the full RESET_POLL_TIMEOUT_MS budget — it could complete after the real reset-expiry and read a late 404 as a false NO-RESET. Every attempt and sleep is now capped to the time actually remaining before deadlineAt, and a 404 that completes after deadlineAt is no longer counted as a clean sample. Co-Authored-By: Claude Opus <noreply@anthropic.com> * docs(test): reference the upstream nodejs/undici issue for the fetch() stall Point the file-header comments at nodejs/undici#5600 (the unref'd setImmediate idle-socket-validation stall, bundled into Node via undici 8.9.0/26.5.1) and its fixnodejs/undici#5609 — already merged upstream but not yet in a released undici/Node build. Replaces the PR's earlier "recommend filing it" note, which predated finding the existing issue. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): stop pollForPresent from masking a real NO-RESET as inconclusive Pre-push independent review (domain lens, medium) on the round-2 commit: gating sawCleanSample on Date.now() <= deadlineAt (the conservative polling deadline) discarded trustworthy 404s that land between deadlineAt and the real reset-expiry (deadlineAt + RESET_CHECK_SAFETY_MS). In a genuine NO-RESET regression with a short window, that biases the check toward reporting INCONCLUSIVE instead of the regression it exists to catch — the assertion still fails, but a human triaging it reads "transport issue, not a TTL defect" and may write off a real bug as flaky infra. pollForPresent now takes the real expiry (expiresAt) separately from the polling deadline and only discards a 404 that lands after expiresAt. Also closes a related minor finding: skip a sample once less than RESET_POLL_MIN_REMAINING_MS is left in the window, instead of attempting one with a timeout clamped to a few ms — a guaranteed failure that shouldn't count toward "no clean read in the window." Re-verified: 6/6 passing on both rocksdb and lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): apply the same transport/defect separation to pollUntilGone Pre-push independent review (domain lens, major) on the round-3 commit: the transport-vs-defect separation was applied to the presence poll (pollForPresent) but not the absence poll (pollUntilGone) — and the absence poll is the one that turns a stall into the suite's loudest alarm. pollUntilGone called getRecord(id) with no timeoutMs (httpRequest's 5s default), unbounded relative to its own budget (5s in probeSurface, ~4.8s in the race probe) and with no concept of "couldn't measure." One slow/stalled response could consume the whole window, and callers read a false 'false' as either "RESET but DID-NOT-EXPIRE (defect)" in probeSurface or F-002 stale resurrection in the race probe — the exact false-positive class this PR set out to eliminate, just on the other side of the check. pollUntilGone now mirrors pollForPresent: every attempt is capped to EXPIRY_POLL_SAMPLE_TIMEOUT_MS (and further to the time actually remaining), samples with less than EXPIRY_POLL_MIN_REMAINING_MS left are skipped instead of attempted, and an AbsenceResult distinguishes a genuine "still present" read from "never got a clean read." Both call sites (probeSurface's goneObserved and the race probe's resurrection check) now route an inconclusive result to 'not-checked'/transportErr instead of a false defect signal. Re-verified: 6/6 passing on both rocksdb and lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): un-stick pollUntilGone's clean-sample flag Pre-push independent review (round 4, full — codex major, confirmed independently by domain adjudication) on the round-3 commit: sawCleanSample was set true on ANY 200 seen anywhere in the window and never reset, so one early 200 (the common case — pollUntilGone in the race probe is typically entered right after an immediate present-check, i.e. near the start of a ~2.8s window) made every later stall for the rest of the window read as "conclusive: still present." In the race probe that increments outcomes.resurrection — the suite's loudest data-integrity alarm — for a stalled GET, making the transportErr bucket added in the prior commit effectively unreachable on the common path. Conclusiveness now comes from the LAST completed sample, not "any sample anywhere in the window": pollUntilGone tracks lastSampleClean (true on 200/404, false on timeout/error) and returns inconclusive: !lastSampleClean. Also fixes a nit from the same review round: the EXPIRY_POLL_SAMPLE_TIMEOUT_MS and RESET_POLL_TIMEOUT_MS comments claimed each sample is capped "well under the interval," but both timeouts are larger than their poll intervals (intentionally — the interval is the retry cadence, not the cap). Reworded to say "well under the overall window," which is what's actually true and intended. Re-verified: 6/6 passing on both rocksdb and lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): stop letting a clamped terminal sample decide pollUntilGone Pre-push independent review (round 5, domain lens, major): the fixed-size polling window meant the loop's FINAL admitted sample was structurally guaranteed to run with a shortened, clamped timeout budget (the loop only stopped admitting once remaining time dropped below the floor, so the last sample's budget was always somewhere in [floor, floor+cycle) — a fraction of the intended EXPIRY_POLL_SAMPLE_TIMEOUT_MS). Since the prior commit made conclusiveness depend on the LAST completed sample specifically, that structurally-doomed sample was also the one deciding "still present" vs "couldn't measure" — on a loaded CI runner where request latency approaches the clamped budget, a genuine resurrection regression (record present the entire window) could still get reported as transportErr/INCONCLUSIVE, because the one sample that mattered for the verdict never got a fair shot. pollUntilGone now only admits a sample once it can run with the FULL EXPIRY_POLL_SAMPLE_TIMEOUT_MS budget (gates the loop on that instead of a separate, smaller MIN_REMAINING_MS floor), so the deciding sample is never shortchanged. pollForPresent is unaffected — its `sawCleanSample` accumulates OR-style across the whole window rather than keying off the last sample, so a clamped final sample there can't erase evidence gathered earlier. Also updates AbsenceResult's docblock, which still described the pre-prior- commit "any sample, ever" semantics rather than the current "last sample" rule (round 5 nit). Re-verified: 6/6 passing on both rocksdb and lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): give pollForPresent's deciding sample the full window budget Pre-push independent review (round 6, domain lens, major — the sibling of round 5's pollUntilGone fix, explicitly asked for on this function this round): RESET_POLL_TIMEOUT_MS (300ms) clamped every sample, including ones near the true reset-expiry, over a window that's only ~600-800ms wide. A 200 landing at any point up to expiresAt proves the clock reset — the request can't have started before the original expiry — but the code discarded a slow-but-valid response past 300ms even when there was still time left before expiresAt. On a loaded CI runner with ~400ms GET latency, every sample could time out even though the record genuinely reset, misreporting a healthy surface as INCONCLUSIVE — a net-new flake mode versus the single 5s-budget GET this poll replaced (which tolerated ~800ms of latency). pollForPresent now runs in two phases: intermediate samples stay bounded to RESET_POLL_TIMEOUT_MS (and, matching pollUntilGone, are only admitted with that full budget — no more clamping down near deadlineAt) so a stall still leaves room for a retry; the FINAL sample instead gets whatever time remains up to expiresAt, so a valid late 200 isn't discarded just because it took longer than an intermediate sample's bound allows. RESET_POLL_MIN_REMAINING_MS is gone — the intermediate loop's admission gate is now RESET_POLL_TIMEOUT_MS itself, same pattern as pollUntilGone's EXPIRY_POLL_SAMPLE_TIMEOUT_MS gate. Also from the same review round: - static-cache-headers.test.ts's applyAndWaitForCacheControl() wraps its sample in try/catch so one timeout or transient non-200 mid-reload retries within the 20s poll instead of aborting it (recurring minor across rounds 3-6; the non-200 rejection was pre-existing on fetch()'s version too, only the new 5s timeout rejection was net-new). - Fixed a stale caller comment describing pollUntilGone's pre-round-5 "any sample, ever" semantics. - Clarified the file-header comment: INCONCLUSIVE is a diagnosis improvement for surfaces 1-5, not a deflaking one — the assertion still fails, just with an accurate message instead of a false RESET/NO-RESET/resurrection verdict. Only the race probe's transportErr bucket actually absorbs it. Re-verified: 12/12 passing (both suites, rocksdb) and 6/6 on lmdb. Deliberately still deferred (recorded in the dispatch # Review → Risks & open questions, not fixing this pass): http.request's socket-idle vs absolute timeout semantics (recurring nit across all 5 review rounds, low likelihood against Harper's small JSON responses); getRecord() discarding status/error text for INCONCLUSIVE diagnostics (message-quality only); hardcoded http:// ignoring u.protocol (no functional impact, fixtures are plaintext-only); the two suites' hand-rolled HTTP clients not being consolidated into a shared integrationTests/utils/ helper (repo-wide scope, not introduced by this diff). Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): close round 7's remaining minors on the race probe and cache-header poll Pre-push independent review (round 7, full — no new major, LGTM from the graded lens, 4 minors + nits from domain adjudication): - Race probe (test 6): outcomes.transportErr was unbounded, and the round-5/6 fixes widened what routes there. If most rounds land in transportErr, the resurrection===0 assertion can pass having barely measured anything — the probe reports green while catching almost nothing. Added a floor: transportErr <= ROUNDS / 2. - static-cache-headers.test.ts's applyAndWaitForCacheControl(): the round-6 try/catch fix swallowed both transport failures and the strictEqual(200) assertion while `last` kept the previous successful sample's value — so a persistent regression (e.g. the static plugin starts 404ing) reported a stale "last seen" that pointed at the reload path instead of the real failure. Now captures the last error and includes it in the timeout message. Also wrapped the periodic config resend in the same try, for symmetry (lower-likelihood path — it goes over node:http via the ops client, not fetch — but free to close). - Fixed a stale comment: RESET_CHECK_SAFETY_MS no longer describes "the last poll," since pollForPresent's final sample deliberately runs past it to the true expiry — it now only bounds the intermediate samples. Deliberately deferred (documented in the dispatch # Review → Risks & open questions): a scenario-level retry on INCONCLUSIVE (domain's suggested mitigation for the recurring "sample budget vs. loaded-runner latency" theme — a genuine new capability, not a tweak, so flagging as a follow-up rather than adding here) and INCONCLUSIVE not distinguishing "window closed before any sample ran" from "samples ran and never completed cleanly" (narrow edge case — requires an ≥800ms scheduling overshoot — message-wording only, doesn't affect correctness). Re-verified: 12/12 passing (both suites, rocksdb) and 6/6 on lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): scenario-level retry on INCONCLUSIVE, and measure the race probe's actual coverage Pre-push independent review (round 8, 2 majors): 1. probeSurface's reset-check window is inherently tight (bounded by TTL_S, not by per-sample budget tuning) — a slow-but-correct run on a loaded runner (this suite starts Harper with threads.count: 4 plus a background expiry sweep) could still time out every sample and report INCONCLUSIVE even though TTL reset worked, a new flake mode the last several rounds' budget fixes couldn't close by construction. This is the same theme flagged (at lower severity) in rounds 2-7; the reviewer's concrete latency trace this round is what elevated it to major. Root fix per the reviewer: retry the whole seed→update→check scenario (fresh id, fresh timing) on INCONCLUSIVE, up to MAX_PROBE_ATTEMPTS=3, rather than any further budget tuning. probeSurface is now a thin retry wrapper around probeSurfaceOnce; a conclusive RESET/NO-RESET/DID-NOT-EXPIRE result is still never retried or discarded — only INCONCLUSIVE triggers a retry. 2. My own round-7 fix (transportErr <= ROUNDS/2) bounded the wrong quantity: silentLoss and updateError rounds also `continue` before the resurrection check ever runs, so a probe where every round lands in silentLoss (a legitimate outcome of the exact race this probe induces) could pass resurrection===0 having measured the F-002 property in zero rounds. Replaced with an assertion on the measured set directly: cleanReset + resurrection >= ROUNDS / 2. Re-verified: 6/6 passing on both rocksdb and lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): stop the retry from burying a conclusive defect on one axis Pre-push independent review (round 9, major — codex and grok both found it independently from opposite directions): probeSurfaceOnce set `inconclusive` whenever EITHER the reset-check OR the gone-check came back inconclusive, without first checking whether the OTHER axis had already conclusively measured a defect. probeSurface's retry wrapper (added last commit) then discarded that whole result and started over on a fresh id — so a real, measured NO-RESET or DID-NOT-EXPIRE/F-002 resurrection could be thrown away and silently replaced by a lucky retry. The wrapper's own docblock claimed the opposite invariant ("genuinely RESET/NO-RESET/DID-NOT-EXPIRE is never retried or discarded") — this is the code contradicting its documented contract, not a judgment call. probeSurfaceOnce now only sets `inconclusive` when there's no conclusive defect signal on either axis. A mixed case (one axis unmeasured, the other conclusively a defect) gets its own composed finding string (RESET-UNMEASURED + DID-NOT-EXPIRE, or NO-RESET + GONE-UNMEASURED) instead of being folded into a generic INCONCLUSIVE message, and is never retried. Also from the same round: - Setup-phase transport failures (seed/update PUT returning 'error') now mark the attempt inconclusive and retry, instead of throwing a hard `ERROR:` result with zero retries — MAX_PROBE_ATTEMPTS exists precisely for this class of bad luck, and a failed seed carries strictly less information than a half-measured window. - Race probe (test 6): the "immediate" post-update read now uses a tight budget instead of the 5s default, and a 404 landing after the record's natural expiry is no longer credited as silentLoss (that GET being slow is indistinguishable from a legitimate late-natural-expiry, so it's transportErr instead). Also tracks windowMissed (the update itself landed at/after natural expiry, so the round never entered the intended race) and excludes it from the coverage floor's denominator, so a loaded runner that keeps missing the window can't manufacture a coverage failure out of a race that never happened. - httpRequest/getPath: `timeout` on http.request is socket-inactivity, not wall-clock (flagged every round since round 3) — added an explicit setTimeout-based abort alongside it as the real per-sample deadline every poller's "one sample can't run past its budget" invariant depends on. Re-verified: 12/12 passing (both suites, rocksdb) and 6/6 on lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(test): anchor the race probe's timing on issue time, not response time Pre-push independent review (round 10, 2 majors — same anchor-precision bug class the reset-check fixed early in this dispatch, just not carried into the race probe I touched this round): 1. windowMissed (added last commit) gated on updateAt (the raced PUT's response time) instead of when it was issued. The server can apply a write well before its response returns, so a round whose update landed inside the intended race window but whose ACK arrived late got discarded as windowMissed along with whatever F-002 evidence it produced — the same bug the reset-check's `updateIssuedAt` anchor already fixed 250 lines up, just not applied here. Now captures updateIssuedAt before firing and gates on that instead; the comparison bound (seedAt + TTL_MS, the record's LATEST possible natural expiry) is unchanged, so this only marks a round windowMissed when it's certain to have missed. 2. fireAt (the "wait until just before natural expiry" anchor) used the seed's response time, but the server's TTL clock starts at apply time — at-or-before that. Any seed round-trip >= WINDOW_BEFORE_EXPIRY_MS (100ms) could put fireAt after the record had already expired, silently turning the "race" into a plain post-expiry write against a dead key. Worse: that non-race round then lands in cleanReset, which the coverage floor added last commit counts as having measured the F-002 property — so on a loaded runner the floor could be satisfied entirely by rounds that never raced, the exact vacuous-pass hole it was added to close. Now anchors fireAt on the seed's ISSUE time (the EARLIEST possible expiry), guaranteeing the update always fires before any possible true expiry. Also reordered two assertions per a related minor: the racedRounds > 0 check now runs before the generic "no round completed cleanly" smoke guard, so an all-windowMissed run reports its actual cause instead of a generic "environment likely broken." Re-verified: 6/6 passing on both rocksdb and lmdb. Co-Authored-By: Claude Opus <noreply@anthropic.com> * Honor HTTPS URLs in direct-request test helpers Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Tighten TTL race probe coverage boundary Co-Authored-By: GPT-5 Codex <noreply@openai.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: GPT-5 Codex <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This relates to...
Fixes#5600.
Rationale
Reusing an idle keep-alive socket stalls for up to a fast-timers tick before the request reaches the wire. With a pool of one connection against a 10 ms server, six sequential requests take over a second instead of the ~60 ms the server accounts for.
scheduleIdleSocketValidationmoved from an unref'dsetTimeout(..., 0)tosetImmediatein #5499, and theunref?.()came along with it. An unref'd immediate does not hold the loop open, so on an otherwise idle loop the check phase is reached only once the next fast-timers tick wakes things up, and the queued request waits for that.By the time the validation is scheduled, the request it exists for is already queued, so there is nothing to keep alive artificially. The immediate is a one-shot that runs on the very next check phase, and
clearIdleSocketValidationcancels it when the socket goes away, so leaving it ref'd cannot delay process exit by more than that one turn — unlike thesetTimeoutthis was inherited from.Measured on Windows with Node 24, six sequential requests through a single-connection pool:
Changes
lib/dispatcher/client-h1.js: drop theunref?.()on the idle socket validation immediate.test/issue-5600.js: the reproduction from the issue, asserting the slowest of six sequential requests on a single-connection pool stays well under a fast-timers tick. It fails on currentmainwith a slowest request around 425 ms.npx borp --timeout 180000 --expose-gc -p "test/*.js"is 1439 passing here. The one failure in that run istest/http2-dispatcher.js"Should send http2 PING frames", which fails about two runs in three on unpatchedmainon this machine as well — unrelated to HTTP/1, and I have a separate branch for it if that is wanted.Features
Bug Fixes
Status