Skip to content

fix: Windows breakage after #16 — sys.path shadow and Microsoft OpenSSH multiplex#18

Closed
battlesnake wants to merge 5 commits into
RedFox20:masterfrom
battlesnake:feature/supermama
Closed

fix: Windows breakage after #16 — sys.path shadow and Microsoft OpenSSH multiplex#18
battlesnake wants to merge 5 commits into
RedFox20:masterfrom
battlesnake:feature/supermama

Conversation

@battlesnake
Copy link
Copy Markdown
Collaborator

@battlesnake battlesnake commented May 4, 2026

Two issues that hit Windows users after #16:

1. mama_ssh.py shadows stdlib types

When git invokes the wrapper as python mama_ssh.py …, __package__ is empty and the script took a fallback path:

sys.path.insert(0, os.path.dirname(_THIS_DIR))   # = .../mama
from utils import ssh_multiplex

That puts <...>/mama/ at the front of sys.path. Inside mama/ lives a types/ subpackage (mama.types — git/asset/dep_source helpers), which then shadows Python's stdlib types for any subsequent first-time import. The next from types import MethodType, GenericAlias (e.g. inside contextlib, transitively imported by ssh_multiplex.py) explodes:

ImportError: cannot import name 'MethodType' from 'types'
(...site-packages\mama\types\__init__.py)

It mostly bit uv-tool-installed Pythons, where contextlib hadn't been pre-cached before our path manipulation ran. Fix: try the qualified from mama.utils import ssh_multiplex first (works for any pip-installed mama with no path manipulation needed); fall back to inserting the package's parent — never mama/ itself.

2. Microsoft OpenSSH for Windows ControlMaster is flaky

Masters drop with "Connection reset by peer" mid-session and the stale socket then blocks reattach:

mux_client_request_session: read from master failed: Connection reset by peer
ControlSocket C:\Users\jorma/.ssh/cm\<hash> already exists, disabling multiplexing

Detect the buggy client narrowly by ssh -V banner — only the Microsoft port prints OpenSSH_for_Windows_<ver>. When that's the active ssh, skip multiplex; keepalives, parallel fetches, and the concurrency cap stay enabled. Cygwin, Git-Bash/MSYS2, and WSL ssh on Windows report the standard OpenSSH_<ver>p1 banner and keep full multiplex. Result is cached per process.

User config (~/.ssh/config) is never overridden in either case.

The Microsoft OpenSSH port that ships with Windows 10/11 has flaky
ControlMaster support: the master commonly drops mid-session with
"Connection reset by peer" and the stale socket file then blocks
subsequent multiplex attempts. Reported user output:

    mux_client_request_session: read from master failed:
        Connection reset by peer
    ControlSocket C:\Users\jorma/.ssh/cm\<hash> already exists,
        disabling multiplexing

Skip the ControlMaster/ControlPath/ControlPersist `-o` flags entirely
when running on Windows. Keepalives are still added (those work fine),
parallel_load + the fetch semaphore continue to bound concurrency, and
each fetch falls back to its own simple connection — auth happens N
times instead of once, but correctness wins over the perf optimisation.

Windows users who still want multiplexing can configure it in
~/.ssh/config explicitly (e.g. via WSL or a Cygwin ssh); we'll detect
their config via `ssh -G` and respect it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@battlesnake battlesnake self-assigned this May 4, 2026
@battlesnake battlesnake requested a review from RedFox20 May 4, 2026 15:30
Comment thread mama/utils/ssh_multiplex.py Outdated
Mark Kuckian and others added 2 commits May 5, 2026 10:22
Per review feedback on #18 — mama.utils.system.System already
provides static platform checks set from sys.platform at import
time, so use System.windows instead of a local _is_windows().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Microsoft's OpenSSH port for Windows is the actually-buggy client —
Cygwin, Git-Bash/MSYS2, and WSL ssh on Windows all have working
ControlMaster. Probe `ssh -V` once at startup: only the Microsoft port
prints "OpenSSH_for_Windows_<ver>", so we can skip multiplex narrowly
for that one case and let everyone else use it.

Also addresses review nits:
* Trim the inline comment block (it now points at the helper instead
  of inlining the explanation).
* Fix the options_to_add docstring — multiplex is known-broken on the
  Microsoft client, not "unsupported on this platform".
* Add a test for "Windows + user has multiplex configured in ssh config"
  → we still respect the user's config and don't override it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@battlesnake
Copy link
Copy Markdown
Collaborator Author

Pushed two follow-ups:

  • f098b2d — switched to System.windows per your inline comment.
  • 4bb6c77 — refined further: instead of disabling multiplex on all of Windows, probe ssh -V at startup and only skip when the banner contains for_Windows (Microsoft's port). Cygwin, Git-Bash/MSYS2, WSL ssh on Windows all report the standard banner and have working ControlMaster, so they get full multiplex. Result is cached.

Also tightened the inline comment, fixed an inaccurate docstring ("unsupported" → "known-broken"), and added a test for the "Windows + user has multiplex configured in ssh config" path. 41/41 tests pass.

PR description updated to match.

* Drop double-checked locking — racing initialisers both write the same
  bool, the lock was paranoia.
* Drop `or ''` defensive null checks — subprocess.run(text=True) returns
  strings, not None.
* Compress the docstring to 4 lines (matching this file's terse-helper
  style for sibling helpers like is_multiplex_configured).
* Drop the inline call-site comment — the function name says it.

13 lines lighter, same behaviour, all 41 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@battlesnake battlesnake changed the title fix: skip ssh multiplex on Windows where ControlMaster is unreliable fix: skip ssh multiplex on Microsoft OpenSSH for Windows May 5, 2026
When git invokes the wrapper as `python /path/to/mama_ssh.py`,
__package__ is empty and the script took a fallback path that did:

    sys.path.insert(0, os.path.dirname(_THIS_DIR))  # = .../mama
    from utils import ssh_multiplex

That puts the `mama/` package dir at the front of sys.path. Inside
`mama/` lives a `types/` subpackage (mama.types — git/asset/dep_source
etc.), which then shadows Python's stdlib `types`. The next thing
that does `from types import MethodType, GenericAlias` (e.g.
`contextlib`, transitively imported by ssh_multiplex.py) explodes:

    ImportError: cannot import name 'MethodType' from 'types'
    (...site-packages/mama/types/__init__.py)

This bit Windows users on uv-installed Pythons where `contextlib`
hadn't been pre-cached before our path manipulation ran.

Fix: try the qualified `from mama.utils import ssh_multiplex` first
(works for any pip-installed mama, no path manipulation needed). Only
fall back to sys.path manipulation if that fails, and add the
GRANDPARENT of mama_ssh.py — i.e. the dir that contains `mama/` — so
`import mama.<x>` resolves but `mama/types/` never appears as a
top-level import root.

Adds a regression test that runs the wrapper in a fresh subprocess and
asserts the `mama/` dir does not appear on sys.path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@battlesnake battlesnake changed the title fix: skip ssh multiplex on Microsoft OpenSSH for Windows fix: Windows breakage after #16 — sys.path shadow and Microsoft OpenSSH multiplex May 5, 2026
@battlesnake
Copy link
Copy Markdown
Collaborator Author

Pushed 18dabd0 — fixes a second Windows-only bug from #16 that surfaced in Slack today (Mykhailo):

ImportError: cannot import name 'MethodType' from 'types'
(...site-packages\mama\types\__init__.py)

Reproduced and verified locally. The wrapper's fallback path inserted <...>/mama onto sys.path, so mama/types/ shadowed stdlib types the moment something (here: contextlib, transitively via ssh_multiplex.py) did from types import MethodType. uv-tool-installed Pythons don't pre-cache types, so the shadow triggered.

Fixed by importing as the qualified from mama.utils import ssh_multiplex; only fall back to sys.path manipulation when even that fails, and add the grandparent of mama_ssh.py (so mama/ is reachable as a package without leaking mama/types/ onto the path).

Added a regression test that runs the wrapper in a fresh subprocess and asserts the mama/ dir does not appear on sys.path. 42/42 tests pass. PR title + description updated to cover both fixes.

Note: mama.types/ shadowing stdlib types is a pre-existing footgun in mama itself — long-term it might be worth renaming to mama.dep_types or similar, but that's out of scope here.

@battlesnake
Copy link
Copy Markdown
Collaborator Author

Continued in #19 (auto-closed here when the source fork was deleted; same 5 commits at 18dabd0 now sourced directly from RedFox20:feature/supermama).

RedFox20 pushed a commit that referenced this pull request May 5, 2026
…SH multiplex (#19)

* fix: skip ssh multiplex on windows where ControlMaster is unreliable

The Microsoft OpenSSH port that ships with Windows 10/11 has flaky
ControlMaster support: the master commonly drops mid-session with
"Connection reset by peer" and the stale socket file then blocks
subsequent multiplex attempts. Reported user output:

    mux_client_request_session: read from master failed:
        Connection reset by peer
    ControlSocket C:\Users\jorma/.ssh/cm\<hash> already exists,
        disabling multiplexing

Skip the ControlMaster/ControlPath/ControlPersist `-o` flags entirely
when running on Windows. Keepalives are still added (those work fine),
parallel_load + the fetch semaphore continue to bound concurrency, and
each fetch falls back to its own simple connection — auth happens N
times instead of once, but correctness wins over the perf optimisation.

Windows users who still want multiplexing can configure it in
~/.ssh/config explicitly (e.g. via WSL or a Cygwin ssh); we'll detect
their config via `ssh -G` and respect it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ssh_multiplex: use existing System.windows static helper

Per review feedback on #18 — mama.utils.system.System already
provides static platform checks set from sys.platform at import
time, so use System.windows instead of a local _is_windows().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ssh_multiplex: detect buggy ssh by banner instead of platform

Microsoft's OpenSSH port for Windows is the actually-buggy client —
Cygwin, Git-Bash/MSYS2, and WSL ssh on Windows all have working
ControlMaster. Probe `ssh -V` once at startup: only the Microsoft port
prints "OpenSSH_for_Windows_<ver>", so we can skip multiplex narrowly
for that one case and let everyone else use it.

Also addresses review nits:
* Trim the inline comment block (it now points at the helper instead
  of inlining the explanation).
* Fix the options_to_add docstring — multiplex is known-broken on the
  Microsoft client, not "unsupported on this platform".
* Add a test for "Windows + user has multiplex configured in ssh config"
  → we still respect the user's config and don't override it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ssh_multiplex: tighten multiplex_known_broken

* Drop double-checked locking — racing initialisers both write the same
  bool, the lock was paranoia.
* Drop `or ''` defensive null checks — subprocess.run(text=True) returns
  strings, not None.
* Compress the docstring to 4 lines (matching this file's terse-helper
  style for sibling helpers like is_multiplex_configured).
* Drop the inline call-site comment — the function name says it.

13 lines lighter, same behaviour, all 41 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix: stop mama_ssh.py shadowing stdlib `types` via sys.path

When git invokes the wrapper as `python /path/to/mama_ssh.py`,
__package__ is empty and the script took a fallback path that did:

    sys.path.insert(0, os.path.dirname(_THIS_DIR))  # = .../mama
    from utils import ssh_multiplex

That puts the `mama/` package dir at the front of sys.path. Inside
`mama/` lives a `types/` subpackage (mama.types — git/asset/dep_source
etc.), which then shadows Python's stdlib `types`. The next thing
that does `from types import MethodType, GenericAlias` (e.g.
`contextlib`, transitively imported by ssh_multiplex.py) explodes:

    ImportError: cannot import name 'MethodType' from 'types'
    (...site-packages/mama/types/__init__.py)

This bit Windows users on uv-installed Pythons where `contextlib`
hadn't been pre-cached before our path manipulation ran.

Fix: try the qualified `from mama.utils import ssh_multiplex` first
(works for any pip-installed mama, no path manipulation needed). Only
fall back to sys.path manipulation if that fails, and add the
GRANDPARENT of mama_ssh.py — i.e. the dir that contains `mama/` — so
`import mama.<x>` resolves but `mama/types/` never appears as a
top-level import root.

Adds a regression test that runs the wrapper in a fresh subprocess and
asserts the `mama/` dir does not appear on sys.path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ssh_multiplex: memoize multiplex_known_broken with functools.cache

Drops the `_buggy_ssh_cached` global, the `global` declaration, and the
inner `is None` branch. Same behaviour, idiomatic stdlib, less code.

Test fixture switched from setattr-ing the (now removed) global to
calling cache_clear().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* sub_process: add merge_stderr to execute_piped; use it in ssh_multiplex

execute_piped only captured stdout, but `ssh -V` writes its banner to
stderr on most builds. Add an additive merge_stderr=False parameter
that redirects stderr into the captured stdout when set.

Switch multiplex_known_broken() over to execute_piped(merge_stderr=True,
throw=False) instead of building its own subprocess.run + try/except
around (TimeoutExpired, FileNotFoundError, OSError) — execute_piped
returns None on failure, which is what we want for the safe-default
("can't tell, skip mux") branch anyway.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Mark Kuckian <mark.cowan@krattworks.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

2 participants