Skip to content

fix(health-twin): readJson must settle its promise on oversized payload - #1132

Open
mdheller wants to merge 2 commits into
mainfrom
fix/health-twin-readjson-hang
Open

fix(health-twin): readJson must settle its promise on oversized payload#1132
mdheller wants to merge 2 commits into
mainfrom
fix/health-twin-readjson-hang

Conversation

@mdheller

Copy link
Copy Markdown
Member

Closes out a finding from Copilot's review on #932 (merged 2026-07-21).

req.destroy() with no error argument does not reliably emit 'error', so on a >2MB body the readJson promise never resolved or rejected — the request handler awaited it forever, a hung connection / resource leak on every oversized POST.

Fix: reject explicitly at the point of destroy() instead of waiting on an event that isn't guaranteed to fire. All call sites already wrap readJson in try/catch, so this turns a silent hang into the 400 those handlers already know how to send.

No automated test added: server.ts calls server.listen() at import time with no test-mode guard, so importing it in a node:test file would leave an open server handle and hang the test runner — a preexisting testability gap Copilot separately flagged on the same PR (no e2e coverage).

req.destroy() with no error argument does not reliably emit 'error' (Node:
destroy(err) only emits 'error' when err is given), so on a >2MB body the
promise never resolved or rejected -- the request handler awaited it forever,
a hung connection / resource leak on every oversized POST.

Reject explicitly at the point of destroy() instead of waiting on an event
that isn't guaranteed to fire. All call sites already wrap readJson in
try/catch, so this turns a silent hang into the 400 those handlers already
know how to send.

Copilot flagged this on PR #932 (merged 2026-07-21); unaddressed since. No
automated test added: server.ts calls server.listen() at import time with no
test-mode guard, so importing it in a node:test file would leave an open
server handle and hang the test runner -- a preexisting testability gap
(Copilot separately flagged the lack of e2e coverage on this same PR).
Copilot AI review requested due to automatic review settings July 30, 2026 20:48
@mdheller
mdheller enabled auto-merge (squash) July 30, 2026 20:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes a hang/resource leak in readJson when an oversized request body is received by ensuring the returned promise always settles (rejects) instead of waiting for a non-guaranteed 'error' event after req.destroy().

Changes:

  • Refactors readJson to explicitly reject when the payload exceeds 2MB.
  • Adds a tooLarge guard to prevent later 'end'/'error' handlers from settling the promise after rejection.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +176 to +180
let raw = '';
let tooLarge = false;
req.on('data', (c) => {
if (tooLarge) return;
raw += c;
Comment thread apps/health-twin/src/server.ts Outdated
// go quiet, and this promise would never settle. Reject explicitly instead of
// waiting on an event that isn't guaranteed to fire, so an oversized body fails
// the request instead of hanging the connection open forever.
if (raw.length > 2_000_000) { tooLarge = true; req.destroy(); reject(new Error('payload too large')); }
Comment thread apps/health-twin/src/server.ts Outdated
// go quiet, and this promise would never settle. Reject explicitly instead of
// waiting on an event that isn't guaranteed to fire, so an oversized body fails
// the request instead of hanging the connection open forever.
if (raw.length > 2_000_000) { tooLarge = true; req.destroy(); reject(new Error('payload too large')); }
Comment thread apps/health-twin/src/server.ts Outdated
// go quiet, and this promise would never settle. Reject explicitly instead of
// waiting on an event that isn't guaranteed to fire, so an oversized body fails
// the request instead of hanging the connection open forever.
if (raw.length > 2_000_000) { tooLarge = true; req.destroy(); reject(new Error('payload too large')); }
@mdheller
mdheller disabled auto-merge July 30, 2026 21:34
…message

Copilot review on #1132: the 2_000_000 limit was a magic number and the error
message didn't say what the limit was. Both trivial; fixed.

Deferred, noted not fixed: Copilot also flagged raw += c as costly and
raw.length as UTF-16 code units rather than bytes (inaccurate for non-ASCII
payloads near the boundary). That's the pre-existing accumulation pattern in
this function, not something this PR introduced -- its scope is the hang on
destroy(), not a rewrite of the buffering strategy. Real observation, out of
scope here.
Copilot AI review requested due to automatic review settings July 30, 2026 21:47
@mdheller
mdheller enabled auto-merge (squash) July 30, 2026 21:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

apps/health-twin/src/server.ts:174

  • MAX_BODY_BYTES is expressed in bytes, but raw.length measures UTF-16 code units (and raw += c will stringify Buffer chunks), so the effective limit can be inaccurate for multi-byte payloads. Track size using bytes (e.g., increment a byteCount by c.length when c is a Buffer, or use Buffer.byteLength(raw)), and only decode/stringify once after the full body is accepted.
const MAX_BODY_BYTES = 2_000_000;

apps/health-twin/src/server.ts:182

  • MAX_BODY_BYTES is expressed in bytes, but raw.length measures UTF-16 code units (and raw += c will stringify Buffer chunks), so the effective limit can be inaccurate for multi-byte payloads. Track size using bytes (e.g., increment a byteCount by c.length when c is a Buffer, or use Buffer.byteLength(raw)), and only decode/stringify once after the full body is accepted.
      raw += c;

apps/health-twin/src/server.ts:187

  • MAX_BODY_BYTES is expressed in bytes, but raw.length measures UTF-16 code units (and raw += c will stringify Buffer chunks), so the effective limit can be inaccurate for multi-byte payloads. Track size using bytes (e.g., increment a byteCount by c.length when c is a Buffer, or use Buffer.byteLength(raw)), and only decode/stringify once after the full body is accepted.
      if (raw.length > MAX_BODY_BYTES) { tooLarge = true; req.destroy(); reject(new Error(`payload too large (max ${MAX_BODY_BYTES} bytes)`)); }

apps/health-twin/src/server.ts:190

  • When rejecting due to an oversized payload, it’s better to provide the same Error to req.destroy(err) rather than calling destroy() without an error. This helps ensure the connection is torn down in an error-aware way across Node versions and makes diagnostics more consistent, while still explicitly rejecting the promise immediately.
      if (raw.length > MAX_BODY_BYTES) { tooLarge = true; req.destroy(); reject(new Error(`payload too large (max ${MAX_BODY_BYTES} bytes)`)); }
    });
    req.on('end', () => { if (!tooLarge) { try { resolve(raw ? JSON.parse(raw) : {}); } catch (e) { reject(e); } } });
    req.on('error', (e) => { if (!tooLarge) reject(e); });

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.

2 participants