Skip to content

fix(ext/node): preserve TLS upgrade state on reconnect - #32838

Merged
bartlomieju merged 1 commit into
denoland:mainfrom
r3wretrhy:codex/preserve-tls-upgrade-state-public
Mar 23, 2026
Merged

bartlomieju merged 1 commit into
denoland:mainfrom
r3wretrhy:codex/preserve-tls-upgrade-state-public

Conversation

@r3wretrhy

Copy link
Copy Markdown
Contributor

Summary

  • run the TLS post-connect upgrade hook against the active handle instead of a closure-captured original handle
  • rebuild TLS upgrade promise/resolver metadata when net.Socket reinitializes its handle during reconnect fallback
  • add a regression test covering kReinitializeHandle preserving TLS upgrade state

Testing

  • deno fmt ext/node/polyfills/_tls_wrap.js ext/node/polyfills/net.ts tests/unit_node/tls_test.ts
  • cargo test -p unit_node_tests --test unit_node tls_test -- --nocapture

@CLAassistant

CLAassistant commented Mar 19, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@r3wretrhy
r3wretrhy force-pushed the codex/preserve-tls-upgrade-state-public branch 2 times, most recently from ae03ecc to 703abbe Compare March 19, 2026 13:38
@r3wretrhy

Copy link
Copy Markdown
Contributor Author

I’ve already signed the CLA. It looks like the remaining checks are still waiting for workflow approval before they can report back.

Could a maintainer approve the workflow run when convenient?

@r3wretrhy
r3wretrhy force-pushed the codex/preserve-tls-upgrade-state-public branch 4 times, most recently from 68e9934 to a3d05ed Compare March 20, 2026 01:54
@r3wretrhy

Copy link
Copy Markdown
Contributor Author

Hey @bartlomieju ,I just pushed the fixes for the previous CI errors. The workflow needs re-approval due to the force-push. Mind approving it again when convenient? Thanks!

@r3wretrhy
r3wretrhy force-pushed the codex/preserve-tls-upgrade-state-public branch from a3d05ed to 8054f65 Compare March 21, 2026 12:55
@r3wretrhy
r3wretrhy force-pushed the codex/preserve-tls-upgrade-state-public branch from 8054f65 to beca16e Compare March 21, 2026 13:11
@r3wretrhy

r3wretrhy commented Mar 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Hey @bartlomieju, I pushed a follow-up fix for the Windows HTTP/2 callback type mismatch that was failing CI.

This update switches the affected nghttp2 callback/data provider usage to the *2 APIs (set_select_padding_callback2, nghttp2_data_provider2, submit_request2, submit_response2, and submit_data2) so the callback signatures line up with the generated binding types on Windows.

Locally I re-ran:

  • cargo build --locked --bin deno --bin denort --bin test_server --features=panic-trace
  • deno run --allow-write --allow-read --allow-run --allow-net --allow-env ./tools/lint.js

Could someone re-approve/re-run CI on the latest commit when convenient? Thanks!

@bartlomieju bartlomieju changed the title fix(node): preserve TLS upgrade state on reconnect fix(ext/node): preserve TLS upgrade state on reconnect Mar 23, 2026

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@bartlomieju
bartlomieju merged commit af67058 into denoland:main Mar 23, 2026
113 checks passed
@r3wretrhy

Copy link
Copy Markdown
Contributor Author

LGTM, thanks!

Thanks for the review and for merging this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants