| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Signed-off-by: Tim Perry <pimterry@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #64407 +/- ##
==========================================
- Coverage 90.25% 90.23% -0.02%
==========================================
Files 741 741
Lines 241207 241216 +9
Branches 45424 45445 +21
==========================================
- Hits 217696 217671 -25
- Misses 15087 15123 +36
+ Partials 8424 8422 -2 see 35 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
|
CC @nodejs/http2 |
Sorry, something went wrong.
|
Stress test on main & PR both pass 🤦. Can't verify that way then, but I'm still fairly confident this is the right fix. |
Sorry, something went wrong.
|
Fast-track has been requested by @anonrig. Please 👍 to approve. |
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Sorry, something went wrong.
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64407 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64407 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64407 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64407 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
| Back | FazBrowse Home | New Git URL |
A fix for a flake from today's list: nodejs/reliability#1597.
Can't reproduce the flake locally directly, but I can with a small delay on the server stream response to simulate the very plausible IO delay on the response. Flake 100% reproduces with a setTimeout into the server stream handler at the top:
This fails every time, as in the flake example in https://github.com/nodejs/reliability/blob/main/reports/2026-07-10.md:
Error [ERR_HTTP2_TOO_MANY_ORIGINS]: The server sent more ORIGIN frames than the allowed number of -0 at Http2Session.onOrigin (node:internal/http2/core:783:23) Emitted 'error' event on ClientHttp2Stream instance at: at emitErrorNT (node:internal/streams/destroy:170:8) at emitErrorCloseNT (node:internal/streams/destroy:129:3) at process.processTicksAndRejections (node:internal/process/task_queues:90:21) { code: 'ERR_HTTP2_TOO_MANY_ORIGINS' }I'll do a stress test as well and see if I can confirm the fix directly.
Flake explanation:
This PR fixes that by dropping the request (and server handler) completely from all tests here (the immediate flake, as well as the others). They aren't necessary - I think they exist because this was copied from test-http2-origin, which does actually use the request flow.
These tests though operate purely on just the session directly (server session handler triggers the interval, client session emits the error). The requests were ignored and the server handler did nothing. Dropping the requests completely simplifies the test and kills the race.
Would be good to get confirmation of that from @mcollina, since this was added as a security fix (https://github.com/nodejs-private/node-private/pull/855) which I can't see but I'm fairly confident.