Skip to content

fix: reuse SSH transports for remote collectors - #77

Open
zovirollc wants to merge 3 commits into
MiaAI-Lab:mainfrom
zovirollc:fix/reuse-ssh-transports
Open

fix: reuse SSH transports for remote collectors#77
zovirollc wants to merge 3 commits into
MiaAI-Lab:mainfrom
zovirollc:fix/reuse-ssh-transports

Conversation

@zovirollc

@zovirollc zovirollc commented Sep 4, 2026

Copy link
Copy Markdown

Summary

Remote collectors currently create a full SSH/PAM login for every metric poll. Reuse one authenticated OpenSSH transport per remote unit, while keeping each collector command on its own exec channel with the existing timeout and output limit.

  • Single-flight initial and recovery probes using ControlMaster=auto and ControlPersist.
  • Isolate sockets by unit, target, user, authentication mode, key identity and credential using a process-salted digest in a private 0700 directory. Passwords stay out of argv and socket paths.
  • Invalidate the matching readiness generation when a post-probe command reports a transport failure, timeout, signal or local execution error. Subsequent calls share a fresh probe; late failures cannot evict a newer recovery generation.
  • Keep readiness for ordinary remote nonzero exit statuses. Failed commands are never automatically replayed.
  • Preserve main's sshCommandSpec API for independent tunnel callers.

SSH_CONTROL_PERSIST_SECONDS defaults to 60 seconds, is capped at one hour, and accepts 0 to restore one connection per command. Key and sshpass authentication are supported.

Evidence

On a two-DGX-Spark deployment, the old collectors generated about 3.6 complete SSH/PAM login lifecycles per second on the worker. An on/off/on monitor test measured polkitd anonymous RSS growth of 0.954 MiB/min, flat while monitoring was stopped, and 0.966 MiB/min after restart. A same-command-rate comparison measured 0.526 MiB/min with independent SSH connections and 0 with a multiplexed connection.

Production validation of the initial transport-reuse change showed a 0.000 MiB/min anonymous-RSS slope in two observation windows; a cold start dropped from 56 SSH authentications to one. These observations support reducing session churn, not a claim to fix an internal polkit allocation bug. The post-probe failure recovery added during review is covered by regression tests; it has not been separately deployed in that production test.

OpenSSH reference: https://man.openbsd.org/ssh_config.5#ControlPersist

Validation

Updated against main at e03b9d6, including its SSH tunnel helper and test fixes.

  • SSH multiplex regression suite: 7/7 pass. Includes post-probe transport failure, one recovery probe for concurrent callers before TTL expiry, stale failures during recovery, ordinary remote exit status, and timeout recovery.
  • npm test: 197/197 pass, including the local TCP tunnel tests (requires permission to bind loopback sockets).
  • npm run typecheck: pass.
  • npm run build: pass.
  • git diff --check: pass.

@smazurov

smazurov commented Sep 6, 2026

Copy link
Copy Markdown

Independent confirmation of both the problem and this fix, from a two-node DGX Spark (GB10) cluster running Ubuntu, monitored by sparkDash on the head with one remote worker over key auth.

I found the session churn while investigating something else, derived the same ~3.6/s figure from the call sites before finding #73, then measured it. Numbers below are from the worker.

Before, on cc44d352:

Accepted publickey from head, 120s window:  432   (3.6/s)
fork rate (/proc/stat), worker:             630/s
fork rate, head (local collectors):          15/s
concurrent ssh clients on head:                5

The daemon that grew for us was dbus-daemon --system, not polkitd:

worker  dbus-daemon RSS:  1642 MB   (uptime ~20d)
head    dbus-daemon RSS:      5 MB   (uptime  21d)

The head reads /proc locally (isLocal: true) and only the worker is polled over SSH, so that 300x gap tracks the SSH path exactly. The system bus had issued 9,552,611 connection IDs since boot, and Debug.Stats reported PeakMatchRulesPerConnection at 2048, which is the default per-connection ceiling.

After pinning our submodule to a561c22 and redeploying, same measurement:

Accepted publickey from head, 120s window:    0   (0.00/s)
fork rate, worker:                           44/s
concurrent ssh clients on head:                1
control socket:  srw------- in a 0700 mkdtemp dir

The four tests in ssh.multiplex.test.js pass on Node v24.18.0.

One negative result worth recording. I sampled polkitd and dbus-daemon RSS over a 120 second window and got 0.000 MiB/min for both, so I could not reproduce a per-minute slope on our hardware. The growth appears to arrive in allocator-sized steps rather than smoothly, which means short windows can miss it entirely. The 1.6 GB accumulation over 20 days is the evidence on our side, not a measured rate.

Context for why this mattered to us beyond log noise: these are unified-memory boxes where the inference runtime reserves most of 121 GB, so host headroom is single-digit GB. 1.6 GB held by dbus was a meaningful share of what was left. I am not claiming this PR fixes that; the reservation was our own misconfiguration. But on a memory-constrained host the churn is not free.

@vincenzopalazzo

Copy link
Copy Markdown
Collaborator

Concept ACK — SSH ControlMaster reuse

Production Readiness: NO-GO

Real bug: remote collectors were burning a full SSH/PAM login per metric poll. Design is mostly right — salted digest isolation, password out of argv/ControlPath, ControlMaster=auto, opt-out via SSH_CONTROL_PERSIST_SECONDS=0, single-flight probe.

Critical: mux state is not invalidated when a post-probe sshExec fails. A dead master before expiresAt can recreate the auth storm this PR exists to stop. Clear _multiplexStates on multiplexed transport failure, then this is mergeable.

Happy to ACK after that.

— Vincent

@zovirollc

Copy link
Copy Markdown
Author

Fixed the post-probe failure invalidation in sshExec. Transport failures (exit 255), timeouts/signals and local execution errors now invalidate the readiness generation used by that command. Subsequent concurrent calls share one recovery probe before the old TTL expires. A late failure from the old generation cannot clear a newer recovery probe. Ordinary remote nonzero exits retain readiness, and failed commands are not replayed.

Added regression tests covering those cases, including 12 concurrent recovery callers plus a late old failure. Also merged current main (e03b9d6) and resolved the conflict around sshCommandSpec, preserving the new tunnel API.

Validation: SSH suite 7/7, full suite 197/197, typecheck, build and diff checks all pass. The earlier unrelated test failures are fixed in current main.

The failure-recovery change is ready for another review; the production measurements in the description remain explicitly attributed to the initial transport-reuse version.

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.

4 participants