GeezerVison

cc @nodejs/http2

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.32%. Comparing base (b33cc0e) to head (da4b684).
⚠️ Report is 81 commits behind head on main.

@@            Coverage Diff             @@
##             main   #65079      +/-   ##
==========================================
+ Coverage   90.29%   90.32%   +0.03%     
==========================================
  Files         759      759              
  Lines      247598   248458     +860     
  Branches    46680    46869     +189     
==========================================
+ Hits       223566   224420     +854     
+ Misses      15503    15448      -55     
- Partials     8529     8590      +61     

I'll kick off a stress test to confirm

Stress testing inconclusive - the tests passed on both main and this branch, even with multiple runs. They're definitely failing in PRs though. I can reliably reproduce the reported failure locally (that first failure above, it hits for 2-3% of runs) and validate that it's fixed with this change. The trace is where is crashes on my machine with the matching error, and it's clearly a broken flow, so I think this is the right fix regardless.

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

Even with this PR's change I can still fail test-stream-pipeline-http2.js with a timeout 31/1000 times. Without it about 56/1000 times.

This diff that claude spit out to deflake test-stream-pipeline-http2.js makes that 0/1000 with or without this PR, but I don't know enough about this subsystem to say whether that retains the point of the test or not.

diff --git a/test/parallel/test-stream-pipeline-http2.js b/test/parallel/test-stream-pipeline-http2.js
--- a/test/parallel/test-stream-pipeline-http2.js
+++ b/test/parallel/test-stream-pipeline-http2.js
@@ -27,10 +27,11 @@
-    let cnt = 10;
+    let received = 0;
     req.on('data', (data) => {
-      cnt--;
-      if (cnt === 0) rs.destroy();
+      received += data.length;
+      // HTTP/2 data event boundaries are non-deterministic.
+      if (received >= 32 * 1024) rs.destroy();
     });

Even with this PR's change I can still fail test-stream-pipeline-http2.js

Yes, sorry if the description isn't clear - both failing tests are related to that PR, but this PR only fixes the first of the two. I.e. test-worker-terminate-http2-respond-with-file.

I haven't opened a fix for the second yet, because I think it's actually exposed a real bug that's a bit complicated, and mac-only. Looks like that fix would resolve the test, but from what I can tell so far there's a real underlying deadlock that's reproducible independently, so I want to get a proper fix for that instead. I'll update when I have more info there.

@pimterry can you land this with the fix/workaround i posted to the remaining flake knowing you'll revisit the underlying deadlock problem and possibly change it again?

Good plan @panva, now done 👍.

I'll open the other fix separately. I've got it working now but the deadlock comes from code we added to resolve past CVEs, so needs some thought and it'd be nice not to rush it.

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

Landed in 780229b...404b0cf

IIUC 64a762b should not be backported as it is based on a semver-major change. If I'm wrong, please open a manual backport PR

@aduh95 this can actually be safely backported I think. It fixes a bug (broken stream teardown) that exists independently of the semver-major change (increasing default HTTP/2 window sizes) and shouldn't conflict with anything else I'm aware of.

The bug was effectively invisible with small HTTP/2 windows, but it's still technically present, and users could always manually configure window sizes in the past which would likely trigger this as well, so it's probably reachable in all versions.