stream: fix flaky stream destroy, reachable from HTTP/2 teardown

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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

[pimterry]

Some of the HTTP/2 tests have become flaky, e.g. nodejs/reliability#1623 shows yesterday:

• parallel/test-worker-terminate-http2-respond-with-file failed 11 times
• parallel/test-stream-pipeline-http2 failed 10 times

Best guess is these are both due to the window update PR #64623 (cc @mcollina) since the timing lines up exactly. I think this is really just the window size highlighting existing issues though.

This PR fixes the first issue, which triggers flakes in parallel/test-worker-terminate-http2-respond-with-file. I'll kick off a stress test to confirm, but I can reproduce this locally, and reproduced as resolved with this fix. I'm still working on the 2nd, which only reproduces on Mac and seems a bit more complex.

Actual failure in the 1st test is a crash with pure virtual method called. That fires due to ReadStop within this trace:

Worker::Run → FreeEnvironment
→ Environment::RunCleanup
→ BaseObjectList::Cleanup
→ fs::FileHandle::~FileHandle
→ StreamResource::~StreamResource
→ StreamPipe::ReadableListener::OnStreamDestroy
→ StreamPipe::ReadableListener::OnStreamRead

I.e. during destroy within the destructor chain, we call OnStreamRead, which tries to call stream()->ReadStop inside the sources destructor.

This is only called because OnStreamDestroy manually calls OnStreamRead with an error code to reuse its error/eof teardown logic. We don't need most of that in the destruction scenario (which is only ever called from ~StreamResource, where the stream is already dead).

I've refactored out the relevant bit to split them up (just guarding just fails in the next line, where previous_listener_ is also null). That then exposed two other bugs for the same state, where two other methods that get reached later in this teardown flow also fail to check if the stream is already destroyed - those just need simple guards.

Seems like this is a flaky race because it depends on whether the sink (Http2Stream) or the file handle gets destroyed first by Cleanup().

[pimterry]

cc @nodejs/http2

[codecov]

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.

Files with missing lines	Patch %	Lines
src/stream_pipe.cc	0.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@ 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 

Files with missing lines	Coverage Δ
src/stream_pipe.cc	58.71% <0.00%> (-1.47%)	⬇️

... and 87 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.

[pimterry]

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.

[mcollina]

lgtm

I concur with the analysis

[nodejs-github-bot]

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

[panva]

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();
});

[pimterry]

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.

[panva]

@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?

[pimterry]

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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[panva]

Landed in 780229b...404b0cf

[aduh95]

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

[pimterry]

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