Skip to content

Fix race conditions when establishing SSH connections#371

Merged
giffels merged 6 commits intoMatterMiners:masterfrom
maxfischer2781:bugfix/sshguard
Apr 28, 2025
Merged

Fix race conditions when establishing SSH connections#371
giffels merged 6 commits intoMatterMiners:masterfrom
maxfischer2781:bugfix/sshguard

Conversation

@maxfischer2781
Copy link
Member

This PR addresses two race conditions around SSH connections. Major changes include:

  • The state of bound and connection is now handled as a single entity.

This fixes using an SSH connection before any bound has been created (#369) as well as using a bound of a previous connection. Closes #370.

@maxfischer2781
Copy link
Member Author

Looks like the coverage upload of the unittests got broken by codecov/codecov-action v5.4.1.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.94%. Comparing base (58b66e0) to head (728f7fe).
Report is 11 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #371   +/-   ##
=======================================
  Coverage   98.94%   98.94%           
=======================================
  Files          55       55           
  Lines        2266     2271    +5     
=======================================
+ Hits         2242     2247    +5     
  Misses         24       24           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@giffels
Copy link
Member

giffels commented Apr 15, 2025

Looks like the coverage upload of the unittests got broken by codecov/codecov-action v5.4.1.

Fixed again!

@maxfischer2781 maxfischer2781 requested review from a team, eileen-kuehn and giffels and removed request for a team April 16, 2025 06:24
@giffels
Copy link
Member

giffels commented Apr 16, 2025

I have added a test case, that explicitly tests the race condition in #369 and checks that no new connection is created by the second attempt.

@giffels giffels added the bug Something isn't working label Apr 16, 2025
@maxfischer2781
Copy link
Member Author

Nice, thanks! I wasn't sure how to test this, your test looks very good.

giffels
giffels previously approved these changes Apr 16, 2025
Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for providing a fix. Just decide whether you like to go for a namedtuple or stay with a tuple.

Co-authored-by: Manuel Giffels <giffels@gmail.com>
Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your work!

giffels added a commit to giffels/tardis that referenced this pull request Apr 16, 2025
Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@giffels giffels added this pull request to the merge queue Apr 28, 2025
Merged via the queue into MatterMiners:master with commit 8a9da65 Apr 28, 2025
17 checks passed
giffels added a commit to giffels/tardis that referenced this pull request Apr 28, 2025
@giffels giffels mentioned this pull request Apr 28, 2025
@giffels giffels mentioned this pull request Jan 28, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants