[fix] drop a dead host client instead of caching it forever

[malickyeu]

Summary

Fourth finding from the review — an availability bug rather than a security one,
but the kind that quietly trains people to distrust the tool.

A dead connection stayed cached. The Docker client is cached per host, and an
SSH one captures its SSH connection inside the transport's dialer. Once that
connection died — the machine rebooted, the link dropped — every later call failed
against the same object, forever. Nothing evicted it: Disconnect is only reached
when a host is edited, deleted or re-trusted. So the host fired a critical
"offline" alert and then never recovered, no matter that the daemon came back,
until someone re-saved the host or restarted the binary.

A failed health probe now drops the cached client, so the next 30-second sweep
dials afresh. Eviction lives in Ping because that is the one call guaranteed to
run again; a healthy host never reaches the branch, so there is no churn.

One unreachable host froze the others. Client() held the manager-wide mutex
across buildClient, which for SSH hosts performs a synchronous dial — and
ssh.Dial's handshake is not bounded by the request context. A single sleeping
laptop therefore stalled every Docker call in the app, the local host included,
in ten-second bursts (indefinitely against a peer that accepts TCP and then
stalls). The dial now happens outside the lock, with the cache re-checked
afterwards so concurrent first-time callers share one connection and the loser is
closed rather than leaked.

Type of change

• Bug fix
• New feature
• Docs only
• Refactor / chore

Checklist

• go test -short ./... and go vet ./... pass
• gofmt gate is clean (gofmt -l $(git ls-files '*.go') after staging)
• Frontend type-checks — N/A (no web/src change)
• Rebuilt and committed web/dist — N/A (no web/src change)
• Added/updated tests for the change — 4 cases in a new manager_connection_test.go
• Updated docs/ and added a CHANGELOG.md entry for user-facing changes

Notes for reviewers

Manager.newClient is now injectable — the same seam auth.Service uses for
LDAP. Connection bookkeeping (caching, eviction, not holding the lock across a
dial) was otherwise only reachable with a real daemon or a real sshd, i.e. only in
the tiers that don't run in CI. Production still uses buildClient.

The eviction test drives a real httptest server that hijacks and closes the
connection mid-flight to imitate a dead peer, then flips it back to healthy — so it
asserts the recovery that used to be impossible, not just the eviction.

Mutation-tested:

Broken on purpose	Result
removed m.Disconnect(hostID) from the failure path	a failed ping must drop the cached client … have 1
put the lock back around buildClient	a slow dial for one host blocked another (after the 2s timeout)

Concurrency cases also run clean under -race -count=3.

Deliberately not done here: eviction on every failing call. Only the health
probe evicts. Any request can fail for reasons that say nothing about the
connection (a 404, a container that just exited), and tearing down a working SSH
tunnel on those would trade this bug for a worse one.

[Copilot]

Pull request overview

Fixes Docker host availability handling by evicting dead cached clients on failed health probes and by preventing a slow first-time client dial for one host from blocking all other hosts via a manager-wide mutex. Adds targeted unit tests and updates user documentation/changelog to describe the recovery and non-blocking behavior.

Changes:

• Evict cached per-host Docker clients on Ping failure so a subsequent sweep redials and can recover after transient disconnects.
• Restructure Manager.Client() to avoid holding the global lock across client construction/dial, while still ensuring one cached client “wins” and losers are closed.
• Add connection-bookkeeping tests and update docs/changelog to reflect automatic recovery and improved responsiveness.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/docker/ops.go	Evicts cached client on ping failure to allow automatic recovery on next sweep.
internal/docker/manager.go	Makes client creation injectable for tests; removes lock-holding across potentially slow dials and handles concurrent first-time callers.
internal/docker/manager_connection_test.go	Adds tests for eviction, no-churn on healthy pings, non-blocking dials, and concurrent callers sharing one cached client.
docs/hosts.md	Documents automatic recovery behavior after a failed probe.
CHANGELOG.md	Adds user-facing “Fixed” entries describing recovery and non-freezing behavior.

Suppressed comments (1)

internal/docker/manager_connection_test.go:208

• The goroutine closes over the loop variable i, so multiple goroutines can write to the wrong index (and the race detector will flag this). Pass i as a parameter to the goroutine to ensure each caller stores its client in the correct slot.

		go func() {
			defer wg.Done()
			c, err := m.Client(context.Background(), id)
			if err != nil {
				t.Errorf("Client: %v", err)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.