CloseAll() iterates connections map without connectionsMu — fatal map race on SIGHUP/shutdown during active scrapes

[krishna3554]

Summary

SSHConnectionManager.CloseAll() iterates the connections map without acquiring connectionsMu, while every write path (connect() inserts under Lock()) takes it. Sending SIGHUP (config reload) or triggering shutdown while any scrape is in flight can therefore run an unsynchronized map iteration concurrently with a locked map write - which in Go is a fatal, unrecoverable runtime error (fatal error: concurrent map iteration and map write) that kills the whole exporter.

Static-analysis finding against current master; not executed here.

Location

• File: pkg/connector/connection_manager.go
• Function: CloseAll() (~lines 147-151):

func (m *SSHConnectionManager) CloseAll() {
	for _, c := range m.connections {
		c.Stop(fmt.Errorf("end of world"))
	}
}

• Contrast: writes are guarded in connect() (~lines 108-113: m.connectionsMu.Lock(); ... m.connections[device.Host] = c), reads in getExistingConnection() use RLock.
• Call sites: main.go ~line 170 (after <-ctx.Done()) and ~line 238 inside reinitialize() (SIGHUP path).

Problem

Nothing serializes CloseAll() against in-flight scrapes:

1. SIGHUP reload: reinitialize() holds configMu, but scrape handlers call GetSSHConnection() → connect() without taking configMu. A reload arriving while any target's collection is between getExistingConnection() and the final map insert runs CloseAll()'s iteration on a map that another goroutine may be writing.
2. Shutdown: <-ctx.Done() fires on server error/cancel; HTTP handlers already dispatched may still be executing collector code that inserts connections when CloseAll() starts iterating.

Go's runtime aborts the entire process with concurrent map iteration and map write (and similarly for iteration racing a delete) - this is not recoverable via recover() and produces no Prometheus-style graceful shutdown.

Note the map-write race window is widened by design: connect() performs the full SSH handshake before taking the lock, so the insert lands long after the initial unlocked check, exactly during the seconds-long window a SIGHUP is likely to hit.

Trigger / Reproduction

Based on static analysis; no runtime run performed:

1. Run junos_exporter scraping several devices continuously.
2. While a scrape is being established (new connection handshake), send SIGHUP (or cause the HTTP server to exit and trigger the shutdown path).
3. If the map insert in connect() interleaves with CloseAll()'s iteration, the process dies with the fatal map error instead of reloading gracefully.

Expected Behavior

CloseAll() should take m.connectionsMu.Lock() (write lock; it semantically empties the manager's state) for the duration of the iteration - matching the discipline every other method applies.

Actual Behavior

Unsynchronized iteration; fatal crash under concurrency rather than clean connection teardown.

Impact

A routine operational action - reloading the config with SIGHUP while metrics are being scraped - can abort the exporter entirely, dropping all in-flight monitoring until an external supervisor restarts it. Because Go's map-race abort leaves no stack-trace log through the normal logger, diagnosis is difficult.

Suggested Direction

func (m *SSHConnectionManager) CloseAll() {
	m.connectionsMu.Lock()
	defer m.connectionsMu.Unlock()
	for _, c := range m.connections {
		c.Stop(fmt.Errorf("end of world"))
	}
}

Optionally also clear the map afterwards so post-reload state cannot resurrect stale entries.

Evidence

• Missing lock quoted above vs locked insert/read paths in the same file.
• reinitialize() guards only configMu; scrapers do not acquire it (collection path: junOSCollector.Collect → GetSSHConnection).
• Go memory model/runtime: unsynchronized concurrent map access is detected and aborts the program.