Fix SSH connection sharing

[edolstra]

Motivation

Updated version of NixOS#15351 and #311 to fix SSH connection sharing using a master connection.

Context

Summary by CodeRabbit

• Refactor
  • Improved SSH master connection handling by enhancing control socket support.
  • When a socket path is configured, the system reuses the SSH master via the specified control socket; when not configured, it behaves as a non-multiplexed flow.
  • Connection-establishment checks were updated to align with whether a control socket is in use.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5345ec2b-fdc6-4de1-9da2-fd2d5dd56768

📥 Commits

Reviewing files that changed from the base of the PR and between 83aab6e and 178c13a.

📒 Files selected for processing (2)

• src/libstore/include/nix/store/ssh.hh
• src/libstore/ssh.cc

🚧 Files skipped from review as they are similar to previous changes (2)

• src/libstore/include/nix/store/ssh.hh
• src/libstore/ssh.cc

---

📝 Walkthrough

Walkthrough

SSHMaster private methods are refactored to thread the SSH control socket path explicitly. addCommonSSHOpts gains an std::optional<std::filesystem::path> parameter that injects -S <path> or -S none. isMasterRunning accepts the socket path directly. startMaster returns std::optional<std::filesystem::path>, and startCommand uses that optional to conditionally gate the connection-ready check.

Changes

SSHMaster optional socket path threading

Layer / File(s)	Summary
Updated private method declarations src/libstore/include/nix/store/ssh.hh, src/libstore/ssh.cc	Private declarations for addCommonSSHOpts, isMasterRunning, and startMaster are updated to accept or return optional socket-path types.
addCommonSSHOpts and isMasterRunning implementation src/libstore/ssh.cc	addCommonSSHOpts injects -S <path> or -S none depending on the optional. isMasterRunning now accepts a socket path and delegates -S injection to addCommonSSHOpts.
startMaster and startCommand integration src/libstore/ssh.cc	startMaster returns std::nullopt when master mode is off. startCommand captures the optional path, passes it into addCommonSSHOpts, and gates the connection-established check on socket presence. Master startup uses explicit socket path for isMasterRunning and addCommonSSHOpts.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

🐇 A socket once hidden now travels with care,
passed through each method with optional flair.
-S none or a path, the SSH won't guess—
the rabbit refactored to tidy the mess.
Explicit is better, says the warren decree! 🕳️

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix SSH connection sharing' directly aligns with the PR's main objective to address SSH connection sharing using master connections.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-ssh-master

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/libstore/ssh.cc`:
- Line 117: The args.insert call at line 117 is using bare string literals and
the .string() method for OS path handling, which is inconsistent with the rest
of the function that uses OS_STR() for string literals and .native() for path
conversions. This inconsistency can cause type mismatches on Windows where
OsString differs from std::string. Replace the bare string literals "-S" and
"none" with OS_STR("-S") and OS_STR("none"), and replace the .string() method
call on socketPath with .native() to ensure consistent OS string handling
throughout the function.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9f130085-796f-497b-98a6-8dfd3553b17e

📥 Commits

Reviewing files that changed from the base of the PR and between 21264af and 83aab6e.

📒 Files selected for processing (2)

• src/libstore/include/nix/store/ssh.hh
• src/libstore/ssh.cc

[github-actions]

🚀 Deployed on https://6a2feca9c63a2a1f4b9d6922--determinate-nix.netlify.app