Skip to content

Sbarvick/ssh replace options race fix - #11399

Open
sbarvick-dd wants to merge 1 commit into
erlang:maintfrom
sbarvick-dd:sbarvick/ssh_replace_options_race_fix
Open

Sbarvick/ssh replace options race fix#11399
sbarvick-dd wants to merge 1 commit into
erlang:maintfrom
sbarvick-dd:sbarvick/ssh_replace_options_race_fix

Conversation

@sbarvick-dd

Copy link
Copy Markdown

Fixes a race in ssh:daemon_replace_options/2 where the daemon supervisor could automatically shut down if its final active connection terminated while the listener was being replaced. A temporary significant guard child now keeps the supervisor alive for the duration of the replacement. A deterministic regression test reproduces the original noproc failure and verifies the corrected behavior.

This issue was observed in older systems in which ssh:daemon_replace_options/2 was called during reconfiguration and typically included having listening ipv4 and ipv6 procs.

This fix was largely written by Codex, with much prompting and refining.

@CLAassistant

CLAassistant commented Jul 28, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@sbarvick-dd
sbarvick-dd changed the base branch from maint to master July 28, 2026 19:59
@sbarvick-dd
sbarvick-dd force-pushed the sbarvick/ssh_replace_options_race_fix branch from 45f550a to 5c737dd Compare July 28, 2026 20:00
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

CT Test Results

    2 files     27 suites   11m 29s ⏱️
  505 tests   482 ✅ 23 💤 0 ❌
1 728 runs  1 689 ✅ 39 💤 0 ❌

Results for commit de74e7e.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@u3s u3s added the team:PS Assigned to OTP team PS label Jul 29, 2026
@u3s

u3s commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

thanks for the contribution.
We were aware of the problem, but didn't have time to fix it.

@Mikaka27 Mikaka27 self-assigned this Jul 29, 2026
@Mikaka27

Copy link
Copy Markdown
Contributor

Would it be possible for you to rebase this branch from starting point? Ideally (if there are no conflicts) it would be best to base this on patch-base-27 tag so we could apply it to otp_27, 28 and 29. If not and you would have some conflicts, you could base this on OTP-29.0.4 tag instead, and I will create older branches manually and test them overnight in the CI.

@sbarvick-dd

Copy link
Copy Markdown
Author

I created a new PR to maint-27 based on the patch-base-27 tag. It is #11408. It needed some adjustments because 27 was a bit different.

daemon_replace_options/2 temporarily removes the daemon's acceptor
supervisor while replacing the listening socket and its options.

If the final active SSH connection terminated during this interval,
ssh_system_sup could have no remaining significant children. Its
all_significant auto-shutdown policy would then stop the daemon before
the replacement acceptor was started, causing the operation to exit
with noproc.

Add a temporary significant guard child during acceptor replacement.
The guard keeps ssh_system_sup alive until the new acceptor is running
and is always removed afterward, preserving the daemon's normal
shutdown behavior outside the replacement window.

Add a deterministic Common Test case that pauses replacement after the
listener is stopped, closes the final connection, and verifies that:

* daemon_replace_options/2 succeeds
* the daemon supervisor remains alive
* daemon information remains available
@Mikaka27
Mikaka27 force-pushed the sbarvick/ssh_replace_options_race_fix branch from 5c737dd to de74e7e Compare August 7, 2026 17:32
@Mikaka27
Mikaka27 changed the base branch from master to maint August 7, 2026 17:32
@Mikaka27 Mikaka27 added the testing currently being tested, tag is used by OTP internal CI label Aug 7, 2026
@Mikaka27

Mikaka27 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

I've rebased this from patch-base-28 tag, that way I can merge this to OTP 28, 29, and master.

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

Labels

team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants