fix(h2): validate maxConcurrentStreams and read settings.initialWindowSize

[marko1olo]

This relates to...

A regression from #5498, which landed after the v8.9.0 bump, so both halves below are on main and have not shipped yet.

Rationale

The h2 option namespacing left two wires crossed.

1. h2Options.maxConcurrentStreams is validated against the wrong property. The guard tests h2Options.connectionWindowSize:

if (h2Options.maxConcurrentStreams != null && (!Number.isInteger(h2Options.connectionWindowSize) || h2Options.maxConcurrentStreams < 1)) {

It is the block below it, copied. With maxConcurrentStreams set and connectionWindowSize absent, Number.isInteger(undefined) is false and the negation makes the condition true, so a valid integer throws. It also fails open in the other direction.

new Client(origin, { h2Options: { maxConcurrentStreams: 10 } })
// InvalidArgumentError: h2Options.maxConcurrentStreams must be a positive integer, greater than 0

new Client(origin, { h2Options: { maxConcurrentStreams: 10, connectionWindowSize: 65535 } })
// fine — an unrelated option makes the same call pass

new Client(origin, { h2Options: { maxConcurrentStreams: 'not-a-number', connectionWindowSize: 65535 } })
// accepted

Through Agent or Pool it is worse, because clients are created lazily: construction succeeds and then every request rejects with that message, including against a plain HTTP/1.1 origin where no h2 session is ever negotiated.

const agent = new Agent({ h2Options: { maxConcurrentStreams: 10 } })
await request('http://127.0.0.1:PORT/', { dispatcher: agent })
// InvalidArgumentError: h2Options.maxConcurrentStreams must be a positive integer, greater than 0

2. h2Options.settings.initialWindowSize is validated and then dropped. The validation checks h2Options.settings?.initialWindowSize, which matches docs/docs/api/Client.md, but the session options read h2Options?.initialWindowSize, so the documented option never reaches the session and an undocumented flat one is honoured instead. { h2Options: { settings: { initialWindowSize: 131072 } } } leaves sessionOptions.initialWindowSize undefined.

Changes

Two lines in lib/dispatcher/client.js: validate maxConcurrentStreams against itself, and read settings.initialWindowSize where the session options are built. I kept Number.isInteger rather than the looser typeof !== 'number' used by the legacy branch, because it is what the error message promises.

test/http2-options.js covers both, reading client[kHTTP2Options] directly. It fails 0/2 on current main and passes 2/2 here. npm run test:h2 is green.

Features

• Implemented a new feature

Bug Fixes

• Fixed a bug

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.49%. Comparing base (1089808) to head (56f62ca).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5608      +/-   ##
==========================================
- Coverage   93.50%   93.49%   -0.02%     
==========================================
  Files         110      110              
  Lines       38303    38303              
==========================================
- Hits        35816    35812       -4     
- Misses       2487     2491       +4     

☔ 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.

[mcollina]

This seems like a behavior change

[marko1olo]

It is, yes — and intentionally so. The existing code validated maxConcurrentStreams against connectionWindowSize (a copy-paste error from the block above), so the check was effectively dead for any caller that didn't also set connectionWindowSize. The same bug let invalid values through undetected.

This replaces the cross-wired check with one that tests maxConcurrentStreams against itself, which is what the error message has always said it does. Happy to add a note in the PR description if the intent isn't obvious from the commit message alone.

[metcoder95]

Yeah, the change is valid; this is an oversight on my end, it is included within the new h2 namespacing.

It should be all right to do the move as we are supporting two ways of setting this up (legacy top-level, namepaced)