Skip to content

Fix test_https_over_http_error on Windows with OpenSSL 3.1+#828

Open
julianz- wants to merge 1 commit into
cherrypy:mainfrom
julianz-:fix/https-over-http-windows-openssl
Open

Fix test_https_over_http_error on Windows with OpenSSL 3.1+#828
julianz- wants to merge 1 commit into
cherrypy:mainfrom
julianz-:fix/https-over-http-windows-openssl

Conversation

@julianz-

Copy link
Copy Markdown
Member

test_https_over_http_error started failing in the CI on Windows with Python 3.14 after recent GitHub Actions runner image updates bumped Python 3.14.5 → 3.14.6 and OpenSSL 3.6.2 → 3.6.3 (win25/20260614.167 deployed ~June 19, win22/20260616.203 deployed shortly after).

The test expects record layer failure when IS_ABOVE_OPENSSL31 is True (OpenSSL > 3.1), but with Python 3.14.6 bundling OpenSSL 3.6.3 on Windows both windows-2025 and windows-2022 now return wrong version number instead. This is a known Windows behavior: the TCP reset is surfaced to userspace before OpenSSL fully processes the server's response, causing a different error code to be reported (documented in CPython's own test_ssl.py).

Linux is currently unaffected — Ubuntu 24.04 uses system OpenSSL 3.0.x, so IS_ABOVE_OPENSSL31 is False there and the test falls through to the elif IS_ABOVE_OPENSSL10 branch, which already expects wrong version number.

What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • 📋 docs update
  • 📋 tests/coverage improvement
  • 📋 refactoring
  • 💥 other

📋 What is the related issue number (starting with #)

Resolves #

What is the current behavior? (You can also link to an open issue here)

CI errors:

>       assert expected_substring in ssl_err.value.args[-1]
E       AssertionError: assert 'record layer failure' in '[SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:1082)'

e.g.

https://github.com/cherrypy/cheroot/actions/runs/27889445941/job/82595847403?pr=827

What is the new behavior (if this is a feature change)?

📋 Other information:

📋 Contribution checklist:

(If you're a first-timer, check out
[this guide on making great pull requests][making a lovely PR])

  • I wrote descriptive pull request text above
  • I think the code is well written
  • I wrote [good commit messages]
  • I have [squashed related commits together][related squash] after
    the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided A mark meaning that a new change log entry is present within the patch. label Jun 21, 2026
@julianz- julianz- force-pushed the fix/https-over-http-windows-openssl branch from cc2efd3 to deeb517 Compare June 21, 2026 22:14
@read-the-docs-community

read-the-docs-community Bot commented Jun 21, 2026

Copy link
Copy Markdown

Documentation build overview

📚 cheroot | 🛠️ Build #33239042 | 📁 Comparing f5bb643 against latest (18f4ba8)

  🔍 Preview build  

2 files changed
± history/index.html
± pkg/cheroot.server/index.html

@julianz- julianz- requested a review from webknjaz June 21, 2026 22:15
@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.20%. Comparing base (18f4ba8) to head (f5bb643).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #828      +/-   ##
==========================================
+ Coverage   78.19%   78.20%   +0.01%     
==========================================
  Files          41       41              
  Lines        4788     4791       +3     
  Branches      547      548       +1     
==========================================
+ Hits         3744     3747       +3     
  Misses        905      905              
  Partials      139      139              

On Windows, the TCP reset (RST) takes a different code path than Linux,
causing OpenSSL 3.1+ to report 'wrong version number' rather than
'record layer failure'. Accept either string when IS_ABOVE_OPENSSL31
is True — the same pattern used in CPython's own test_ssl.py.
@julianz- julianz- force-pushed the fix/https-over-http-windows-openssl branch from deeb517 to f5bb643 Compare June 21, 2026 22:26

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

Thanks! I've been meaning to ask you to look into what's changed in the CI env.

See a few notes on the patch inline.

P.S. I wonder how difficult it'd be to have predictable OpenSSL code paths in CI. Is CPython built statically, or can we somehow substitute OpenSSL it uses with something we pre-build? Since we have a few TLS-dependent code paths in runtime, it'd be good to run tests against each of them in CI at some point.

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.

Since this patch doesn't fix a bug in Cheroot's runtime, it shouldn't be listed among bugfixes in the change log. Fixing a test is infra work. So it could be a contrib note. Perhaps, a packaging note when we declare that we support new things (like a new Python version, a new OS or a new dep in this case).

Though, feel free to opt out of attaching a change note if it doesn't feel important to surface to the change log audience. You can add the bot:chronographer:skip label and the bot will stop flagging a missing note in the PR.

Comment thread cheroot/test/test_ssl.py
assert (
'record layer failure' in actual_error
or 'wrong version number' in actual_error
), actual_error

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.

This shouldn't be needed since pytest is invoked w/ CLI options that should reveal the vars in scope:

Suggested change
), actual_error
)

@webknjaz webknjaz mentioned this pull request Jun 23, 2026
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided A mark meaning that a new change log entry is present within the patch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants