Skip to content

docs: record the 204/304 Content-Length divergence from upstream - #138

Merged
ronag merged 3 commits into
nxtedition:masterfrom
ronag:claude/no-body-divergence-docs
Oct 13, 2026
Merged

ronag merged 3 commits into
nxtedition:masterfrom
ronag:claude/no-body-divergence-docs

Conversation

@ronag

@ronag ronag commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Closes #111. Documents the fork's deliberate 204/304 Content-Length behavior and links the parser comments and existing regressions to that decision.

For responses to non-HEAD requests without Transfer-Encoding, when reuse is otherwise allowed:

  • A 304 with nonzero Content-Length succeeds and permits connection reuse; bytes after its headers are parsed as the next pipelined response.
  • A 204 with nonzero Content-Length fails with ResponseContentLengthMismatchError.
  • Upstream nodejs/undici#5864 instead accepts both responses and closes their connections.

The README separately states that non-HEAD 204/304 responses carrying Transfer-Encoding close the connection. The table's qualification excludes HEAD's default reset and the Transfer-Encoding close guard. Runtime behavior is preserved.

Validation on exact head 5809d5bf561fbe13254671a78fd8ca53003299a8:

  • macOS Node.js 26.7.0 and Linux/amd64 Node.js 26.11.1: 889 unit tests passed, two skipped; all 163 node-test tests passed.
  • TypeScript declaration tests and ESLint passed on both platforms.
  • Package dry-run and git diff --check passed locally.
  • Linux ran in a disposable container on root@tv2k-srv4.bmux, limited to two CPUs, 2 GiB and 512 processes, using nxtedition/node@sha256:a56a3f12c2bd3572cef35f9dc6c4637cff970f6a8d70e049098cf44841dd7a75.

This client deliberately keeps a 304's connection when the response
carries Content-Length, parses bytes after a 304 as the next pipelined
response, and rejects a 204 with a non-zero Content-Length (RFC 9110
§8.6, RFC 9112 §6.3). Upstream undici's nodejs#5864 closes the
connection in the first two cases and accepts the third.

Document that in README.md, and point the code and the tests that pin
it at that section, so a future upstream audit doesn't reopen it.

nodejs#5864 also resets the parsed length for HEAD/204/304. That isn't needed
here: nxtedition#136's hasCompleteBody() covers the paused/reset case, the HEAD
and 304 checks in onMessageComplete() already ignore the length, and a
stale length could only change the error type of a request already
aborted from onHeaders().

Closes nxtedition#111.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are documentation-only; the remaining HEAD qualification is a minor documentation correction.

1 open finding
What changed in this PR

This PR records the fork’s deliberate 204/304 Content-Length behavior so future upstream audits can distinguish it from undici’s policy.

Changes:

  • Documents the response and connection-handling differences.
  • Links the existing parser logic and regression tests to that decision.
File Description
test/​client-304-content-length.js Points the tests to the documented decision.
README.md Adds the behavior comparison.
lib/​dispatcher/​client-h1.js Explains the existing check and its upstream divergence.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread README.md Outdated
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new table misstates behavior for HEAD requests and for 304 responses with Transfer-Encoding.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Low severity Table overgeneralizes outcomes across request and response conditions

README.md:17

The table states these outcomes for all requests, but they depend on the request and response headers. onMessageComplete() skips the length-mismatch check for HEAD, so a HEAD 204 with a non-zero Content-Length does not fail; HEAD also normally resets its connection, so the 304 reuse and pipelining claims do not hold by default. For non-HEAD 304s, onHeadersComplete() closes the connection when Transfer-Encoding is present, as the paragraph below notes. Limit the 304 rows to non-HEAD responses without Transfer-Encoding, and the 204 row to non-HEAD requests.

🧠 Review effort: Lite

@ronag
ronag marked this pull request as ready for review October 13, 2026 11:57

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@ronag
ronag marked this pull request as draft October 13, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The comment-only changes align with the existing behavior and tests, with no unresolved findings.

0 open findings

🧠 Review effort: Lite

@ronag
ronag marked this pull request as ready for review October 13, 2026 12:36
@ronag
ronag merged commit 7913f83 into nxtedition:master Oct 13, 2026
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.

Confirm divergence from upstream #5864 (204/304 Content-Length handling)

2 participants