Skip to content

MLE-32051 Fix multipart body termination for Node.js v26 compatibility - #1116

Open
jonmille wants to merge 4 commits into
developfrom
MLE-32051
Open

MLE-32051 Fix multipart body termination for Node.js v26 compatibility#1116
jonmille wants to merge 4 commits into
developfrom
MLE-32051

Conversation

@jonmille

@jonmille jonmille commented Aug 24, 2026

Copy link
Copy Markdown

Summary

Fixes client.documents.write() failing with ECONNRESET / socket hang-up when writing an array of documents on Node.js v26.x.

Closes GH #1104 | Jira: MLE-32051


Problem

When calling client.documents.write([doc1, doc2, ...]) on Node.js v26, the HTTP request body is never properly terminated. MarkLogic receives the part content but not the closing multipart boundary (--boundary--), and responds with:

HTTP 500: XDMP-MULTIPART-BOUNDARY: xdmp:get-request-part-body("json")
         -- Ending of the boundary is incorrect

The client then receives an ECONNRESET after ~30 seconds.

Root cause: multipart-stream 2.0.1 uses sandwich-stream (a Readable) to assemble the multipart body. For each part it creates a PassThrough stream, writes headers + CRNL + body content as separate .write() calls, and adds it to sandwich-stream. sandwich-stream's _currentStreamOnReadable calls read() to pull data from each sub-stream. On Node.js v26, read() returns only the first write-chunk from a PassThrough; the 'readable' event does not re-fire for subsequent chunks, leaving them stranded. Because _currentStreamOnEnd never triggers, sandwich-stream's _pushTail() / push(null) is never reached and the closing boundary is never emitted.

The bug affects Node.js v26.6.0 (the reporter's version) and v26.7.0. Node.js v24.x is not affected.


Fix

requester.js — bypasses sandwich-stream in both affected code paths:

  • multipartRequester()requestPartList path (client.documents.write(array)): detects whether any part carries stream content (typeof content.pipe === 'function').
    • All-synchronous content (the v26 bug scenario — array of document objects): the multipart body is assembled directly as incremental Buffer writes to the request, avoiding sandwich-stream entirely.
    • Stream content present (binary writes, large documents, Data Services ValueStream): falls back to multipart-stream with pipe(). Stream-driven PassThroughs receive writes asynchronously after listeners are attached, so the Node.js v26 'readable' regression does not affect them.
  • chunkedMultipartRequester(): non-stream (metadata) parts are written to the request as Buffer chunks; the final streaming content part is forwarded via requestWriter.pipe(request, { end: false }) followed by the closing boundary and request.end().

The bindingParam and requestPartsProvider paths in multipartRequester() retain multipart-stream with pipe() and an explicit err => request.destroy(err) error handler to properly tear down the socket on stream errors.


Testing

Tested manually with repro.mjs (see MLE-32051):

Node.js Before fix After fix
v24.19.0 PASS PASS
v26.6.0 FAIL (ECONNRESET after 30s) PASS (0.39s)
v26.7.0 FAIL (ECONNRESET after 30s) PASS (0.70s)

MarkLogic server: marklogic-server-ubi:11.3.6-ubi-2.2.6

Existing tests continue to pass with: `npx mocha test-basic/documents-core.js --timeout 0

Copilot AI lite review requested due to automatic review settings August 24, 2026 19:01

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.

Pull request overview

This PR fixes client.documents.write([...]) failures on Node.js v26 by avoiding the sandwich-stream-based multipart body assembly that can strand chunks and prevent the final closing boundary from being emitted.

Changes:

  • Reworks multipartRequester() to directly assemble the multipart body for the requestPartList path (array write) and end the request with a closing boundary.
  • Reworks chunkedMultipartRequester() to write non-stream parts directly and then stream the final content part, followed by the closing boundary.
Suppressed comments (2)

lib/requester.js:487

  • This manual 'data'/'end' forwarding bypasses stream backpressure and can lead to large in-memory buffering when the provider emits large bodies. Prefer piping the multipart stream into the request (and handle stream errors).
    multipartStream.on('data', chunk => request.write(chunk));
    multipartStream.on('end', () => request.end());

lib/requester.js:579

  • Forwarding requestWriter 'data' events directly into request.write() bypasses stream backpressure and can buffer large streamed content in memory. Prefer piping requestWriter into the request with { end: false } and then append the closing boundary in the 'end' handler.
          requestWriter.on('data', chunk => request.write(chunk));
          requestWriter.on('end', () => {
            request.write(Buffer.from(CRNL + '--' + boundary + '--'));
            request.end();
          });

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

Comment thread lib/requester.js Outdated
Comment thread lib/requester.js Outdated
Comment thread lib/requester.js

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you add repro tests to the test suite in the repo?

@RitaChen609

Copy link
Copy Markdown

@jonmille When I ran "npx mocha test-basic/bindingFromParam.js" on nodejs 26, 22 tests were failing. Apparently, the PR's fix for nodejs 26 is incomplete.

FYI Copilot analysis:

test-basic/bindingFromParam.js reproduces the same underlying multipart-boundary-termination bug from MLE-32051:

3 passing / 22 failing — all failures are 20000ms timeouts (hung write, never completed)

Root cause match: db.rows.query(plan, options, bindingParam) sets operation.bindingParam in rows.js:256, which routes through the operation.bindingParam branch of multipartRequester() in requester.js:427. Looking at the current PR code, this branch was left unchanged — it still relies entirely on multipart-stream's addPart() (wrapping the form-data object as a single part), which the PR's own root-cause analysis identifies as the buggy code path on Node v26.

Why even the "no attachment" tests hang: addPart() always writes the part's headers (Content-Type, Accept) as separate synchronous .write() calls to a fresh PassThrough, then pipes the form stream into it — the header writes alone are enough to trigger the same "only first buffered chunk survives, no further readable" regression, so every bindingParam-based query hangs, not just ones with binary/stream attachments.

Why exactly 2 tests pass: 'test without column' and 'test with wrong column type;' never reach the network layer at all — they throw synchronously during op.fromParam(...) argument validation (caught in a try/catch), so no multipart request is ever sent.

This is a more severe gap than the "mixed array" issue — it means the PR's fix is incomplete not just for an edge case, but for an entire, commonly-used feature (Optic fromParam bindings) on Node v26. Every real bindingParam write is broken.

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.

3 participants