fix(plugin): cap origin response headers at 64 KiB, not undici's 16 KiB default; v0.30.0

[harper-joseph]

The bug

util/upstream.js built its dispatcher as new Agent({}), leaving maxHeaderSize unset. undici then falls back to Node's http.maxHeaderSize:

client.js:140-145  // "If maxHeaderSize is not provided, use the default value from the http module"
$ node -e 'console.log(require("http").maxHeaderSize)'
16384

16 KiB — a header-flood mitigation for servers accepting untrusted requests, which is the wrong default for a reverse proxy reading its own origin's response. Browsers and CDNs allow far more, which is why the page loads fine for a real user and only breaks when Harper proxies it.

The cap is cumulative across the whole response head, not per header:

trackHeader (len) {
  this.headersSize += len
  if (this.headersSize >= this.headersMaxSize) {
    util.destroy(this.socket, new HeadersOverflowError())   // <- kills the CONNECTION
  }
}

So no single header needs to be large — a Set-Cookie pile-up plus CSP / Link: rel=preload / NEL / Report-To clears 16 KiB on one page. And undici's response is to destroy the socket, not reject a single request, so the crawler gets a 500 and the retry pays a fresh TLS handshake.

Observed in production

GET /desktop/catalog/womens-clothing.jsp?CN=Gender:Womens+Department:Clothing&cc=... -> 500 in 703ms
HeadersOverflowError  code: 'UND_ERR_HEADERS_OVERFLOW'
  at Parser.trackHeader (undici/lib/dispatcher/client-h1.js:515)

This is deterministic per URL, not a flake — it's a property of that origin response, so those facet URLs 500 every time they're hit uncached. Once in 24h at current volume only because very little traffic has been proxied so far; the rate scales with cache misses and passthrough as bot traffic ramps.

The fix

• New origin.maxResponseHeaderBytes (default 64 KiB, min 16 KiB), documented with the reasoning and the memory trade-off (a bigger cap means more worst-case memory held per connection while a head is parsed — hence bounded, not unlimited).
• Applied to both dispatcher constructions. The plain and staging-pinned Agents are built on separate branches, so the pinned one would otherwise have kept the 16 KiB default — and a staging deploy 500ing on every large-header page reads as a staging-edge fault, not a config gap.

Restart-scoped, deliberately

undici fixes maxHeaderSize at construction and offers no way to change it on a live Agent. Rather than work around that, the option is declared scope: 'restart' — config.js reports a live edit as pending-restart and the running dispatchers keep the value they were built with.

That keeps the hot path (every cache-miss and passthrough fetch) at the single !ip branch it had before:

if (!ip) return (agent ??= new Agent(agentOptions()));

An earlier revision keyed the dispatcher cache on (ip, cap) so a config change took effect live. That bought a reload nobody asked for at the cost of building a cache key and probing a Map on every origin fetch, so it's gone. ??= short-circuits, so there's no per-request allocation; the singleton is lazy only because the cap isn't known at import time.

Tests

Behavioral, against a real http server — not undici's internal kMaxHeadersSize symbol, so they survive an undici refactor and prove the thing that actually broke:

• a 32 KiB response head succeeds under the default cap (and the default is asserted to exceed http.maxHeaderSize)
• a fresh module instance with a 16 KiB cap still overflows — proving the option is wired through to undici, not merely stored
• the unpinned dispatcher is built once and ignores a live cap change — both halves of restart scope: no per-request rebuild, and no silent live effect
• restartPaths() includes the option, guarding the scope declaration itself
• the staging-pinned dispatcher carries the cap too
• an under-minimum value falls back to the default rather than silently restoring the 16 KiB failure

config.test.js's exact restart-path assertion is updated for the new scoped option.

383/383 pass; npm run lint and format:check clean.

Notes

Version 0.30.0 — 0.29.0 is reserved by #66.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a new configuration option maxResponseHeaderBytes (defaulting to 64 KiB) to control the maximum response header size accepted from the origin, preventing UND_ERR_HEADERS_OVERFLOW errors from Undici. It updates the dispatcher caching mechanism in upstream.js to handle dynamic configuration changes by re-keying and closing superseded agents, and adds comprehensive behavioral tests. I have no feedback to provide as there are no review comments.

[harper-joseph]

The approach changed materially after the review above, so flagging it for a re-read rather than letting a stale summary stand.

What the previous review saw: a dispatcher cache keyed on (ip, cap) that closed superseded Agents so a config change took effect live.

What is there now: that is removed. origin.maxResponseHeaderBytes is declared scope: 'restart' instead, and the hot path is back to the single branch it had before the PR:

if (!ip) return (agent ??= new Agent(agentOptions()));

Rationale: undici fixes maxHeaderSize at construction, and the keyed cache was paying a template-string key plus a Map probe on every origin fetch — the cache-miss and passthrough path — to buy a live reload nobody asked for. Declaring the restart scope is both cheaper and more honest: config.js now reports a live edit as pending-restart rather than it silently having no effect.

Tests were reworked accordingly — the "rebuilds on cap change" test is gone, replaced by one asserting the dispatcher is built once and ignores a live cap change, plus a restartPaths() guard so the scope declaration can't be dropped silently. Enforcement is still proven end-to-end via a fresh module instance with a 16 KiB cap.

383/383 pass, lint and format clean.

[harper-joseph]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new configuration option, maxResponseHeaderBytes, which defaults to 64 KiB. This option configures the undici Agent's maxHeaderSize to prevent connection destruction with UND_ERR_HEADERS_OVERFLOW when origin response headers exceed Node's default 16 KiB limit. The change includes lazy initialization of the undici agents to apply this configuration, along with comprehensive integration tests. The review feedback suggests improving the readability of the test helper by extracting a magic number used in header padding into a named constant with an explanatory comment.

[gemini-code-assist]

The magic number 12 in the repeat count makes it difficult to understand how the header value length is derived. To improve readability and maintainability of this test helper, consider extracting this into a named constant with a comment explaining the calculation.

Suggested change

	const per = 1024;
	for (let i = 0; i < Math.ceil(bytes / per); i++) headers[`x-pad-${i}`] = 'a'.repeat(per - 12);
	const per = 1024;
	// A header name like x-pad-99 is 8 chars. The value length is set to leave some slack.
	const valueLength = per - 12;
	for (let i = 0; i < Math.ceil(bytes / per); i++) headers['x-pad-' + i] = 'a'.repeat(valueLength);

[harper-joseph]

Taken, and made exact rather than approximate. The - 12 was a fudge, and the suggested comment ("leave some slack") would have documented the fudge rather than removed it.

undici counts header name and value bytes — Parser.onHeaderField and onHeaderValue each call trackHeader with their own buffer length — and does not count the : or CRLF delimiters. So budgeting name.length + value.length per header is precisely what the cap sees:

const HEADER_BYTES = 1024;
const name = `x-pad-${i}`;
headers[name] = "a".repeat(HEADER_BYTES - name.length);

32 headers now sum to exactly 32768 bytes, so the thresholds the tests assert against (16 KiB, 64 KiB) are exact rather than approximate.

[gemini-code-assist]

Code Review

This pull request introduces a new configuration option, maxResponseHeaderBytes, to allow configuring the maximum response header size accepted from the origin, resolving issues where large headers caused connection termination with UND_ERR_HEADERS_OVERFLOW. The undici Agent is updated to apply this limit lazily, and comprehensive tests are added to verify its behavior. The review feedback suggests adding a maximum constraint of 8 MiB to the schema to prevent excessive memory usage, handling potential errors during server listening in tests to avoid hangs, and properly awaiting server closure to prevent resource leaks.

[gemini-code-assist]

The description for maxResponseHeaderBytes states that the option is bounded to prevent excessive worst-case memory usage per connection. However, the schema definition only specifies a min constraint and lacks a max constraint. To truly bound the memory usage and prevent accidental misconfiguration (e.g., setting an excessively large value that could lead to Out-Of-Memory issues), consider adding a reasonable max constraint, such as 8 * 1024 * 1024 (8 MiB).

Suggested change

	{ unit: 'bytes', min: 16 * 1024, scope: 'restart' }
	{ unit: 'bytes', min: 16 * 1024, max: 8 * 1024 * 1024, scope: 'restart' }

References

1. Validate and reject unusable or structural configuration overrides at the entry point (where configuration is parsed or applied) by restoring them to safe defaults, rather than adding defensive guards in downstream utility functions.

[harper-joseph]

Good catch — the description claimed the value was "bounded rather than unlimited" while the schema only declared a min, so the doc was writing a cheque the code did not honour. Added a max.

I went with 1 MiB rather than 8 MiB. No legitimate response head comes close to either, so the ceiling exists purely to catch a typo — a stray factor of a thousand — and a tighter one bounds the worst case eight times better while still leaving 16x headroom above the 64 KiB default. Since the cap is what undici may hold per connection while parsing a head, the multiplier that matters is concurrent connections, which argues for the smaller number.

Also confirmed this lands in the right layer, per your reference: enforceSchemaConstraints already handles max (config.js:215) by warning and restoring the default, so an out-of-range value is rejected at config-apply time rather than guarded downstream. Test now covers both directions plus the inclusive edge.

[gemini-code-assist]

If server.listen fails or encounters an error, the returned Promise will hang indefinitely because there is no error handler. To make the test more robust and prevent potential hangs, listen to the 'error' event and reject the Promise if an error occurs.

Suggested change

	await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve));
	await new Promise((resolve, reject) => { server.listen(0, '127.0.0.1', resolve); server.once('error', reject); });

[harper-joseph]

Taken. A hung test is much harder to diagnose than a failed one, and EADDRINUSE or a sandbox that forbids binding would both have hung here forever.

Registered the handler before listen so an error emitted synchronously cannot be missed:

await new Promise((resolve, reject) => {
  server.once("error", reject);
  server.listen(0, "127.0.0.1", resolve);
});

[gemini-code-assist]

In Node.js, server.close() is an asynchronous operation. Calling it without awaiting its completion can leave the port active or keep pending handles on the event loop, potentially causing resource leaks or side effects in subsequent tests. Consider wrapping it in a Promise and awaiting it to ensure the server is fully closed before proceeding.

Suggested change

	server.close();
	await new Promise((resolve) => server.close(resolve));

[harper-joseph]

Taken — close() is async and this helper is used by four tests, so a lingering handle could leak into the next one. Now await new Promise((resolve) => server.close(resolve)) inside the finally, so teardown completes even when the assertion throws.

[harper-joseph]

Pushed 16c260a. All four review threads addressed and replied to individually.

From review: exact byte accounting in the test helper (the - 12 fudge is gone), max: 1 MiB on the schema so the "bounded" claim in the description is actually true, listen error handling, and awaited close().

From my own re-read, one substantive bug the reviews did not catch — worth calling out because it made the PR's central claim only half true:

origin.staging.ip is live-scoped. So enabling staging mints a pinned dispatcher long after boot, and because agentOptions() re-read config at each construction, that dispatcher would pick up a cap edited in the meantime while the unpinned singleton kept the boot value. Two dispatchers silently disagreeing, and a pending-restart notice that only applied to one of them.

Fixed by capturing the cap once on first use:

let capturedMaxHeaderSize;
const agentOptions = () => ({
  maxHeaderSize: (capturedMaxHeaderSize ??= config.origin.maxResponseHeaderBytes),
});

The regression test was verified to actually guard it — with the capture reverted, a pinned dispatcher built after a live drop to 16 KiB overflows on a 32 KiB head and the test fails; with it, it passes. I checked that rather than assuming, since a test that passes both ways is worse than none.

384/384 pass, lint and format clean.