test: fix flaky http-chunk-extensions-limit test

[Ethan-Arrowood]

Replace the setInterval with a queueMicrotask to make test less flaky.

Fixes: #51883

[nodejs-github-bot]

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

[lpinca]

This significantly changes how the chunks are sent. What is the reason behind the flakiness of the original version?

[Ethan-Arrowood]

My analysis is that the flakiness comes from certain systems closing the socket connection between interval calls.

Is there a purpose for testing how this behaves with an interval? Could we use something more deterministic like adding asynchronous events to the task queue?

[lpinca]

My analysis is that the flakiness comes from certain systems closing the socket connection between interval calls.

Makes sense as that is when ECONNRESET is raised but why?

Is there a purpose for testing how this behaves with an interval? Could we use something more deterministic like adding asynchronous events to the task queue?

Maybe we can use the peer on the other side. When data is received, make the other peer send more data.

[Ethan-Arrowood]

Why? Because I think there are outstanding issues with long running connections on certain systems. @joyeecheung mentioned that to me.

I'm not sure I follow your recommendation. Is using a microtask not sufficient? This at least demonstrates that the separated chunk writes over async operations behave as expected. Is this really any different from the interval test?

[joyeecheung]

Reading the original HackerOne report, it seems making the chunks sent in 2 parts should not affect the validity of the test. Would be better if it's in 20 parts as the original test does though.

Does the test fail reliably if you revert 03a5c34?

Alternatively I think we can also just install an error handler on the client and consider it passes the test if there is a reset. For the exploit, even getting a reset is a failure. It doesn't really have to be a proper 413 (although technically getting a reset instead of a friendly 413 can also be considered a bug, but a known bug is better than a flake I guess)

[joyeecheung]

Actually looking closer at the test I think the interval seems to be less of the problem. The problem is that in the interval callback, there is no mechanism to stop the client from writing even after it already receives the 413 and closes the server. It just keeps trying until it finishes that 20 writes, and if before it finishes it already got the 413 and closes the server, it will get a reset. It doesn't matter we use queueMicrotask or not, what matters is that this only send it in two parts, thus reducing the likelihood of getting a 413 before all the write finishes. But if before the microtask runs, the client gets the 413 and closes the server, it can flake again. I think what we should do is to make sure that after the client gets the 413, no more writes will be attempted.

[joyeecheung]

Hmm actually, the key here is to not send more bytes after the limit is reached, so I think this should not flake, not because of queueMicrotask but because we split the data in just two parts, and only the second write would reach the limit and encounter the 413. The original test writes it in 20 parts and it would get 413 on any of the 17-20th parts, then any subsequent writes can get the reset if the server is already closed when the write happens. Perhaps simply reducing the remaining value to 17000 is already enough to deflake it...

[joyeecheung]

LGTM if reverting the original commit can make the new test fail reliably.

[Ethan-Arrowood]

node on  fix-flaky-test-http-chunk-extensions-limit [$⇡] via 🤖 v18.19.0 via 🐍 v3.9.6 on ☁️  production 
❯ git log
commit 70b992069457183c02b9936aad7d4339ba6f68ed (HEAD -> fix-flaky-test-http-chunk-extensions-limit)
Author: Ethan Arrowood <ethan@arrowood.dev>
Date:   Fri Mar 1 23:22:39 2024 -0700

    Revert "http: add maximum chunk extension size"
    
    This reverts commit 03a5c34a829742f1c47b68f831b2940af44addf6.

commit fcc6a67ff22ee5f11d2d64a1addb5cfcb9b53d59 (origin/fix-flaky-test-http-chunk-extensions-limit)
Author: Ethan Arrowood <ethan@arrowood.dev>
Date:   Fri Mar 1 12:04:57 2024 -0700

    test: fix flaky http-chunk-extensions-limit test
    
    Replace the setInterval with a queueMicrotask to make test less flaky.
    
    Fixes: https://github.com/nodejs/node/issues/51883
    PR-URL: https://github.com/nodejs/node/pull/51943

commit 8e7da604685a64da4224f87b071020a168e0c003 (main)

node on  fix-flaky-test-http-chunk-extensions-limit [$⇡] via 🤖 v18.19.0 via 🐍 v3.9.6 on ☁️  production took 3s 
❯ tools/test.py test/parallel/test-http-chunk-extensions-limit.js 
=== release test-http-chunk-extensions-limit ===                   
Path: parallel/test-http-chunk-extensions-limit
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'HTTP/1.1 200 OK\r\n' +
+   'Content-Type: text/plain\r\n' +
+   'Date: Sat, 02 Mar 2024 06:23:19 GMT\r\n' +
+   'Connection: keep-alive\r\n' +
+   'Keep-Alive: timeout=5\r\n' +
+   'Transfer-Encoding: chunked\r\n' +
+   '\r\n' +
+   '3\r\n' +
+   'bye\r\n' +
+   '0\r\n' +
+   '\r\n'
- 'HTTP/1.1 413 Payload Too Large\r\nConnection: close\r\n\r\n'
    at Socket.<anonymous> (/Users/ethanarrowood/Documents/github/nodejs/node/test/parallel/test-http-chunk-extensions-limit.js:32:14)
    at Socket.<anonymous> (/Users/ethanarrowood/Documents/github/nodejs/node/test/common/index.js:473:15)
    at Socket.emit (node:events:531:35)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'HTTP/1.1 200 OK\r\n' +
    'Content-Type: text/plain\r\n' +
    'Date: Sat, 02 Mar 2024 06:23:19 GMT\r\n' +
    'Connection: keep-alive\r\n' +
    'Keep-Alive: timeout=5\r\n' +
    'Transfer-Encoding: chunked\r\n' +
    '\r\n' +
    '3\r\n' +
    'bye\r\n' +
    '0\r\n' +
    '\r\n',
  expected: 'HTTP/1.1 413 Payload Too Large\r\nConnection: close\r\n\r\n',
  operator: 'strictEqual'
}

Node.js v22.0.0-pre
Command: out/Release/node /Users/ethanarrowood/Documents/github/nodejs/node/test/parallel/test-http-chunk-extensions-limit.js


[00:00|% 100|+   0|-   1]: Done                                    

Failed tests:
out/Release/node /Users/ethanarrowood/Documents/github/nodejs/node/test/parallel/test-http-chunk-extensions-limit.js

Fails after revert 😄

[ShogunPanda]

LGTM!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in a8f281f