test: stop the sim-ferroamp broker deadlocking on close

[frahlg]

What hangs

go/cmd/sim-ferroamp hung for the full 10-minute go test timeout in run 30378575736 (attempt 1, job "core (Go)"), failing #704, which had only touched Lua drivers. The rerun passed in 1m19s.

The goroutine dump from the failing run points at mochi-mqtt v2.7.9, not at our test logic. Two goroutines waiting on each other:

• the test, in the deferred s.Close(): Server.Close → closeListenerClients → Clients.GetByListener → Clients.Len → blocked on RWMutex.RLock
• the broker's connection loop: EstablishConnection → attachClient → Clients.Delete → blocked on RWMutex.Lock

clients.go is the reason:

func (cl *Clients) GetByListener(id string) []*Client {
	cl.RLock()
	defer cl.RUnlock()
	clients := make([]*Client, 0, cl.Len())   // takes RLock again

sync.RWMutex is not reentrant. A writer queued between the two read locks blocks the second one, and the writer waits for the first. Neither moves again.

Server.Close walks GetByListener, and the broker runs Clients.Delete the moment a client drops, so defer cli.Disconnect(100) followed by defer s.Close() is the race. A narrow window, which is why it is rare and clears on rerun. v2.7.9 is the newest release, so there is nothing to upgrade to.

The fix

Every shutdown now goes through closeBroker, which waits for the client count to fall back to the inline client before closing. No disconnect is then in flight and no writer can queue between the two read locks. A 5-second deadline on Close is the backstop: should the broker still wedge, the test says so in seconds rather than burning ten minutes on an unrelated pull request.

The core Go job is capped at 120s per package for the same reason. -timeout is per package and the slowest takes about 25s, so the margin is wide and any future hang costs two minutes instead of ten.

One more, spotted alongside: tok.WaitTimeout(...) && tok.Error() != nil let a timed-out connect through, because WaitTimeout reports whether the token finished in time. The test then ran against a client that never attached and failed later for a reason that read like a broker bug. connectClient checks both.

Verification

• Full Go suite green under -timeout 120s, no package near the cap.
• 25 runs of the sim-ferroamp tests under -race: all green, no races, and the barrier converges at once — the "still holds N clients" line never appears.
• The backstop was tested with a throwaway test that forces the deadlock: closeBroker reported after 5.001s with a clear message and the binary exited normally despite the wedged goroutine. That file is not part of this change.

Left out

go/test/e2e/stack_test.go:410 closes its broker the same way, right after ShutdownAll() disconnects the driver's client. It is already bounded by make e2e's -timeout 180s, so it costs three minutes rather than ten. Worth a follow-up rather than widening this change.

🤖 Generated with Claude Code