[GrinZero]

Summary

This PR supersedes #62915 with the same feature work rebased onto current main.

It adds builtin http/https request-body support to network inspection, so Network.getRequestPostData can return text request bodies while preserving the existing rejection behavior for binary request bodies.

It also moves builtin http response-body tracking to a raw-byte hook before IncomingMessage decoding, so response inspection remains correct even when user code calls response.setEncoding(...).

In addition, this PR lets Network.requestWillBeSent carry JS-provided initiator data captured at request construction time. The C++ inspector side validates structured initiator.stack objects against the inspector protocol schema before forwarding them to DevTools.

Problem

Builtin http/https network inspection currently emits request and response lifecycle events, but the builtin client path does not expose request-body bytes to the inspector. As a result, Network.getRequestPostData cannot return POST data for builtin http/https requests.

There are two related gaps:

• IncomingMessage 'data' events are not a stable raw-byte source. If user code calls response.setEncoding('utf8'), chunks observed through userland become strings, while the inspector protocol expects byte-oriented payloads.
• Delaying requestWillBeSent until request-body state is known means a C++ stack captured at emission time would point into Node.js internals rather than the user call site. The initiator needs to be captured when the request is created and validated before being emitted.

Approach

1. Reuse the existing Network.dataSent(...) -> NetworkAgent::getRequestPostData(...) buffering pipeline instead of changing the CDP schema.
2. Add builtin HTTP diagnostics events for request body chunks and request body completion:
   • http.client.request.bodyChunkSent
   • http.client.request.bodySent
3. Capture builtin HTTP response body bytes through a parser-side raw-byte diagnostics event before stream decoding:
   • http.client.response.bodyChunkReceived
4. Defer requestWillBeSent until body state is known, so hasPostData is accurate for both body and body-less requests.
5. Capture the initiator stack in JS at request creation time, pass it through the inspector binding, and validate initiator.stack with generated inspector protocol schema support in C++.
6. Document the new diagnostics channels and message shapes.

Behavior

After this change:

• builtin http and https POST requests with UTF-8 text bodies are available through Network.getRequestPostData
• binary request bodies still reject with the existing inspector error behavior
• builtin http response inspection continues to work even if user code calls response.setEncoding('utf8')
• Network.requestWillBeSent reports accurate hasPostData
• Network.requestWillBeSent can carry JS-captured initiator stack data from the request construction site
• malformed initiator stack input is rejected by C++ schema validation

Tests

This PR adds and extends coverage in:

• test/parallel/test-diagnostics-channel-http.js
• test/parallel/test-inspector-network-http.js
• test/parallel/test-inspector-emit-protocol-event.js
• test/parallel/test-inspector-emit-protocol-event-errors.js

The updated tests cover:

• request body chunk and request body finished diagnostics events
• response body raw-byte diagnostics events
• text request bodies split across write() and end()
• binary request bodies
• http and https Network.getRequestPostData
• binary request-body rejection semantics
• response inspection when the client calls response.setEncoding('utf8')
• accurate hasPostData for body-less requests
• JS-provided initiator stack parsing and forwarding
• malformed initiator stack error handling

Verification

Validated with the targeted test suite:

python3 tools/test.py \
  parallel/test-diagnostics-channel-http \
  parallel/test-inspector-network-http \
  parallel/test-inspector-emit-protocol-event \
  parallel/test-inspector-emit-protocol-event-errors

Also validated manually with an end-to-end inspector run:

./out/Release/node --inspect --experimental-network-inspection ./test.js

Refs

• Replaces inspector,http: support builtin http request bodies #62915
• Refs Tracking issue: Stabilizing network inspection in Node.js #53946

[GrinZero]

e2e same like: #62915 (comment)

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/inspector
• @nodejs/net

[codecov]

Codecov Report

❌ Patch coverage is 97.21362% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.50%. Comparing base (1c90519) to head (c140813).
⚠️ Report is 339 commits behind head on main.

Files with missing lines	Patch %	Lines
src/inspector/network_agent.cc	92.62%	4 Missing and 5 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #64369    +/-   ##
========================================
  Coverage   90.49%   90.50%            
========================================
  Files         797      797            
  Lines      277598   277899   +301     
  Branches    53336    53405    +69     
========================================
+ Hits       251220   251515   +295     
+ Misses      16790    16789     -1     
- Partials     9588     9595     +7     

Files with missing lines	Coverage Δ
lib/internal/http.js	92.54% <100.00%> (+0.05%)	⬆️
lib/internal/http/client.js	97.43% <100.00%> (+0.04%)	⬆️
lib/internal/http/common.js	98.61% <100.00%> (+0.06%)	⬆️
lib/internal/http/outgoing.js	98.00% <100.00%> (+0.02%)	⬆️
lib/internal/inspector/network_http.js	97.56% <100.00%> (+1.86%)	⬆️
src/inspector/network_agent.cc	89.28% <92.62%> (+1.24%)	⬆️

... and 31 files with indirect coverage changes

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

[GrinZero]

I think it's time to conduct a re-review. Thank you. @metcoder95 @ShogunPanda @mcollina

[GrinZero]

push

[GrinZero]

push

[GrinZero]

Friendly ping on the next step.

The existing review feedback has been addressed, @metcoder95 has approved, and the PR is currently mergeable. It is still labeled "needs-ci" — could a collaborator please trigger the full CI?

If CI is green, could @nodejs/http and @nodejs/inspector give it a final look or let me know if anything else is needed before landing?

Happy to address any follow-up feedback promptly. Thanks!

[GrinZero]

push

[GrinZero]

push... @jasnell Can you help me add the label to run the CI

[GrinZero]

Hi @mcollina , I’ve rebased #64369 onto the latest main and resolved the HTTP outgoing conflict. The PR is mergeable again, and CI is running. The targeted HTTP/Inspector tests pass locally. When you have a chance, could you review the HTTP integration, particularly the request-body tracking and raw response-body hook? I’d appreciate your feedback and will address any comments promptly. Thanks!
And, can you help me add the label to run the CI...

[mcollina]

lgtm

[mcollina]

Can you do a run of the benchmarks?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78032/

[GrinZero]

Can you do a run of the benchmarks?

I tried comparing the main branch and found that the newly added features resulted in a performance drop of around 18%. Due to the collection of body data, this decrease is actually within expectations. But did CI just fail? I think while investigating, I can also see if there are any ways to optimize.

GrinZero is missing the Overall/Read permission

I can’t access the Jenkins results due to the CI read permissions. Both node-test-commit/92841 and node-test-pull-request/78032 failed, although the reported platform checks appear green. Could someone with CI access share the failing job/platform, test name, and relevant console output? If the failure is unrelated to this change, could you please rerun the affected job? Thanks! @mcollina

[GrinZero]

I reduced the inspection overhead by encoding initiator stacks directly into CBOR, removing a redundant request-body copy, and simplifying the body-completion notification. Async caller frames and validation checks are preserved. With Network inspection enabled on both sides, local benchmarks showed an 8.1%–9.7% throughput improvement over the previous implementation; throughput remains approximately 10%–11% below the main baseline. Regression tests and the remote Inspector E2E passed locally.

[GrinZero]

@mcollina plz cc again.

[GrinZero]

@leah-1ee I noticed you're online. Could you help me kick off CI? I'm basically ready and need to move forward at the moment.

[leah-1ee]

I'd be happy to help, but I don't have permission to trigger CI. I think you'll need to ask a collaborator.

[GrinZero]

I'd be happy to help, but I don't have permission to trigger CI. I think you'll need to ask a collaborator.

Thanks for getti. This is really tough to move forward with 😭 I really need a way to stay in touch with them reliably

[GrinZero]

Hi @mcollina , I’ve rebased #64369 onto the latest main and resolved the conflict. The PR is mergeable again, and CI is running. The targeted HTTP/Inspector tests pass locally. Can you help me add the label to run the CI...

[GrinZero]

@trivikr I noticed you're online. Could you help me kick off CI? I'm basically ready and need to move forward at the mome

[GrinZero]

@joyeecheung I noticed you're online. Could you help me kick off CI?...

[GrinZero]

Hi @mcollina @metcoder95,

I'd like to ask for some help getting #64369 through the final review and landing process.

This work originally started in #62915 back in April and was resubmitted as #64369 in July. I appreciate the reviews and feedback so far.

Current status:

• The PR is mergeable, with no outstanding review threads.
• GitHub Actions checks are green.
• The previously raised review comments have been addressed.
• The implementation has been optimized following the benchmark request.
• With Network Inspection enabled, the remaining throughput overhead is approximately 10–11% compared to main.

Could a collaborator help clarify the remaining steps?

1. Is the current performance overhead acceptable, or is further optimization required before landing?
2. Could someone re-review the latest changes and trigger the full Jenkins CI?
3. Once the requirements are satisfied, could the PR be marked author ready and added to the commit queue?

If there are still technical concerns, I'm happy to address them. I'd also appreciate guidance on whether the scope should be split into smaller PRs.

After several months of work and multiple review rounds, I'd really like to understand what is still blocking this contribution and establish a clear path toward landing.

Thanks again for your time and help!

[GrinZero]

Hi @legendecas @jasnell @islandryu,

I'd also really appreciate your help with #64369, given your experience with Node.js's Network Inspector implementation and the underlying request/response buffering mechanisms.

I've summarized the current status and remaining blockers in my comment above.

At this point, I'm hoping to get a final technical assessment, particularly regarding the implementation approach and performance trade-offs.

Would one of you be willing to take another look and potentially help shepherd this PR through the remaining review, CI, and landing steps?

This contribution has been in progress since April, and I'd genuinely appreciate any guidance on what is still needed to get it across the finish line.

I'm fully committed to addressing any remaining concerns.

Thanks so much for your time and for all the work you do on Node.js!

[GrinZero]

Thank you very much for replying to me and letting me know how to proceed next! Thank you!

@GrinZero

To request a Jenkins CI run (without bypassing protections) it needs an approval based on the latest commit here. See below ⬇️ where it currently says "At least 1 approving review is required by reviewers with write access."

Good luck!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78817/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78831/

[MikeMcC399]

I re-ran Jenkins and it was successful. It seems that the linting failure was indeed a flake.

Note that GitHub is currently suffering from an incident https://www.githubstatus.com/incidents/smtdswd1h2w4 (and caused the first Jenkins re-run to fail with "fatal: Could not read from remote repository.")

The next step is for a collaborator familiar with this subject matter to review the latest commit, approve it, then land it.

[GrinZero]

I re-ran Jenkins and it was successful. It seems that the linting failure was indeed a flake.

Note that GitHub is currently suffering from an incident https://www.githubstatus.com/incidents/smtdswd1h2w4 (and caused the first Jenkins re-run to fail with "fatal: Could not read from remote repository.")

The next step is for a collaborator familiar with this subject matter to review the latest commit, and land it, if it's approved.

Thank you, sir. I have been troubled by this for a long time, and I never expected to make such rapid progress after meeting you. Thank you

[MikeMcC399]

Correction: The linting error was due to a bug in #66837 which has been corrected. Jenkins CI rebases on main before it runs, so the repeat run was rebased on the fix, and that is why it worked.

[GrinZero]

Correction: The linting error was due to a bug in #66837 which has been corrected. Jenkins CI rebases on main before it runs, so the repeat run was rebased on the fix, and that is why it worked.

That sounds like good news - it means the issue isn't due to this PR, so we can move forward more smoothly

[MikeMcC399]

The gating factor is the availability of a collaborator to review, approve and land your PR.

[GrinZero]

@mcollina @legendecas @jasnell The CI (including Jenkins) for this PR has been completed. Please help review the code. Thank you

[legendecas]

I tried comparing the main branch and found that the newly added features resulted in a performance drop of around 18%. Due to the collection of body data, this decrease is actually within expectations.

This seems non-trivial. Which benchmark did you run?

[GrinZero]

I tried comparing the main branch and found that the newly added features resulted in a performance drop of around 18%. Due to the collection of body data, this decrease is actually within expectations.

This seems non-trivial. Which benchmark did you run?

@legendecas

Hello~

Thank you for taking the time to review this PR!

Let me explain why I consider some additional overhead expected, and how I ran the benchmarks.

First, the approximately 18% figure in my earlier comment predates the optimizations. The latest recorded local benchmark results show a 10.0%–11.0% throughput reduction compared with the main baseline, with Network inspection enabled on both sides.

The additional overhead is expected because the PR extends the work performed during inspection: it retains request-body data for Network.getRequestPostData and captures and serializes initiator stacks at request construction time. These operations add CPU and memory work to each inspected request. The benchmark repeatedly sends small requests, making that per-request work visible in throughput.

I used Node’s existing benchmark/http/client-request-body.js through benchmark/compare.js:

• Environment: Apple M2 Max, macOS arm64, Release builds.
• Baseline: main at cff7b12df1f9.
• Workload: local HTTP POST requests with 1 KiB bodies, using UTF-8 strings and Buffers, through both write() and end().
• Sampling: 12 runs per binary and configuration, lasting 3 seconds each.
• Inspection: both binaries used --experimental-network-inspection, with a preload script connecting an Inspector Session and calling Network.enable before measurement. Logs confirmed Network events on both sides.

The results after optimization were:

Body type	Method	Main (requests/s)	Optimized PR (requests/s)	Change
UTF-8 string	write()	9,777	8,703	−10.99%
UTF-8 string	end()	9,812	8,734	−10.99%
Buffer	write()	9,848	8,860	−10.03%
Buffer	end()	9,887	8,843	−10.55%

The optimizations improved throughput by 8.1%–9.7% over the previous implementation, through direct CBOR encoding of initiator stacks, removal of a redundant request-body copy, and simplification of body-completion notification.

Separately, the inspection-disabled HTTP benchmarks in the original run showed no statistically significant regression.

These measurements were taken on September 30, before the later rebases. I have retained the benchmark commands, preload script, and raw CSV results for reproducibility.

[GrinZero]

@marcastben-hash Thanks for reviewing the HTTP integration. These are useful points. My preference would be to address them in focused follow-up PRs rather than make all four prerequisites for landing, if the HTTP and Inspector maintainers are comfortable with that.

1. Request timing: I agree that taking the timestamps when the deferred event is emitted changes the timing shown by the inspector. A body-less request can lose time spent waiting in the Agent queue, whereas a request with an early non-empty write is reported from that write. Capturing the timestamps at request creation would preserve the previous timing origin. That alone would not make queued requests appear earlier in DevTools, so earlier visibility needs separate consideration. I would propose addressing these in a focused inspector timing follow-up.
2. Channel semantics: These events describe the application writing body data and ending the body, not the bytes reaching the peer or necessarily being flushed to the socket. I agree the documentation should make this explicit. If we decide to rename the channels instead, we should settle the names before they ship to avoid a compatibility issue later.
3. Buffer ownership: Diagnostics subscribers receive the existing buffers and should treat them as read-only. For example, mutating a response chunk before it reaches the stream can affect what the application reads. The inspector’s own request-body cache copies the bytes, but that does not make the buffers exposed through diagnostics channels immutable. I would document this contract in a follow-up.
4. Server performance: The default server path does not perform the inspector’s body copying or initiator-stack collection. The shared HTTP paths do gain a small number of field initializations, symbol reads/writes, and conditional checks, but the existing sending and parsing flow remains in place.

My expectation is that these additions are unlikely to cause a substantial regression, though that needs measurement. A server benchmark with inspection disabled, covering response writes and incoming request bodies, would be the right way to verify this on the current revision. I would prefer to handle that measurement and any resulting optimization in a focused follow-up.


[legendecas]

Sorry if I was not clear. I was curious about the performance cost on regular mode, that is when inspector not enabled, and how much cost do these new changes incur?