Nightly tests are failing

[github-actions[bot]]

[github-actions[bot]]

Tests against nightly failed, see: https://github.com/nodejs/undici/actions/runs/10418179441

[KhafraDev]

@metcoder95

 test at test/http2.js:370:1
✖ [v20] Request should fail if allowH2 is false and server advertises h1 only (8.785301ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected
  
  + '80E807C2167F0000:error:0A000460:SSL routines:ssl3_read_bytes:tlsv1 alert no application protocol:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1605:SSL alert number 120\n'
  - 'Client network socket disconnected before secure TLS connection was established'
      at res.<computed> [as strictEqual] (/home/runner/work/undici/undici/node_modules/@matteo.collina/tspl/tspl.js:52:35)
      at TestContext.<anonymous> (/home/runner/work/undici/undici/test/http2.js:408:9)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:879:9)
      at async Test.processPendingSubtests (node:internal/test_runner/test:590:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: '80E807C2167F0000:error:0A000460:SSL routines:ssl3_read_bytes:tlsv1 alert no application protocol:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1605:SSL alert number 120\n',
    expected: 'Client network socket disconnected before secure TLS connection was established',
    operator: 'strictEqual'
  }

[github-actions[bot]]

Tests against nightly failed, see: https://github.com/nodejs/undici/actions/runs/10431397022

[github-actions[bot]]

Tests against nightly failed, see: https://github.com/nodejs/undici/actions/runs/10439694412

[metcoder95]

I'll take a look later this week, maybe we can skip it for now; did something changed within the OpenSSL that is causing that failure on nightly?

[RafaelGSS]

@KhafraDev by the test name (v20) shouldn't it execute only on v20.x?

[KhafraDev]

It executes on versions >= 20 https://github.com/nodejs/undici/blob/69cfd97591c0a1eec563590df862caeed304f1c3/test/http2.js#L372

[RafaelGSS]

Is this failing on the nightly build only (nodejs#main) or has it failed since Node.js 22.6.0?

[metcoder95]

Only on the nightly. It seems related to the way OpenSSL handles the missing of ALPN response. Tried to do bisect Today but didn’t have the time.

[metcoder95]

Did something changed on that regard for v23?

[RafaelGSS]

Can someone test it using the v22.7.0-proposal but removing https://github.com/nodejs/node/pull/54452/commits/a8c78c6f5014e68946be56caaf50fb3c6e7ce25a and https://github.com/nodejs/node/pull/54452/commits/df3e6bbfed2897389dc3fbf83e76907ab9fbb430?

[KhafraDev]

I'm sorry I don't have any time to test.

[RafaelGSS]

I can confirm dropping OpenSSL update passes the test. I'll remove it for the next release and for main branch. Let's discuss the reason in the PR.

[RafaelGSS]

https://github.com/nodejs/node/pull/54491

[RafaelGSS]

It seems undici needs to update its test suite for Node.js 22.8.0 (https://github.com/nodejs/node/pull/54491#issuecomment-2303008950)

[Uzlopak]

Why though?

Doesnt it need to be wrapped again in nodejs, as it is done before?

https://github.com/nodejs/node/blob/8b0c699f2aafdf81bc4834b9bd2a4923741d3a6f/lib/_tls_wrap.js#L1732

Now we get the openssl warning directly.

[Uzlopak]

@RafaelGSS

Please see https://github.com/nodejs/node/pull/54492

With that PR the undici tests pass again.

[metcoder95]

It seems undici needs to update its test suite for Node.js 22.8.0 (nodejs/node#54491 (comment))

This might depend on the outcome of this new behaviour from OpenSSL, isn't it? If moving forward with the update, we will need to at least document this gotcha when attempting to use ALPN

[RafaelGSS]

I might not be the best person to discuss it, but looks like not wrapping it on Node.js would make more sense, it gives the library the ability to decide what to do on OpenSSL error codes. As mentioned by Richard, this is a bug fix from OpenSSL, but I can see why this is problematic as it can be considered semver-major on error code changes.

[Uzlopak]

Just one argument for the wrapping is that ECONNRESET is a posix error and ERR_SSL_WRONG_VERSION_NUMBER not.

[RafaelGSS]

@richardlau what's your thought on this? I don't think we can hold the OpenSSL update for too long. Our choice is either accept https://github.com/nodejs/node/pull/54492 as an expected approach or clarify that users should rely on OpenSSL error codes.

[Uzlopak]

Also keep in mind the error has the very descriptive error message 0E807C2167F0000:error:0A000460:SSL routines:ssl3_read_bytes:tlsv1 alert no application protocol:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1605:SSL alert number 120\n

[metcoder95]

From the perspective of the issue, I'd like to rely on OpenSSL error codes if they were more descriptive compared to what currently throws.

The wrap made by @Uzlopak seems like a good step forward but it might require a wider consensus (I guess?)

[Uzlopak]

ugh...