[Areson]

Stack: PR 3 of 5. Stacked on #188. Review only this PR's commits, ee8d0c8 and e663e58.

Why

Each prepared plan leaks one heartbeat reader goroutine (heartbeat.BlipReader) when repl.lag is collected at more than one level. That's common, because levels inherit domains. Lag.Prepare calls prepareBlip for every level. After the first call, prepareBlip returns a nil cleanup func, which overwrites the one that stops the reader. So Engine.Stop never stops it. The leaked reader keeps querying a closed database handle and writes errors into the status of the next monitor with the same ID.

What

• prepareBlip returns the same cleanup func for every level.
• Lag.Prepare stops its reader if it fails after it starts the reader.
• Engine.Prepare calls the cleanup funcs of collectors it discards on error.
• BlipReader waits return on Stop, so the reader exits promptly.

How

test/goroutine.go adds a small helper that counts goroutines, so tests can assert that a stop leaves none behind.

Risk

Low. The change only affects cleanup on stop and on prepare errors.

Testing

A local benchmark ran 8 monitors in one process and restarted all of them in 10 waves.

• After the 10 restarts there were 8 heartbeat readers, against 88 before.
• The goroutine count stayed flat.

Bigger picture

Part of a stack of 5 fixes found by the same benchmark. Monitor restarts are frequent when a host loses and regains its source; that's the case PRs 4 and 5 speed up.

🤖 Generated with Claude Code

[zyin-squareup]

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Summary: Reviewed only ee8d0c8 and e663e58. The fix is correct. prepareBlip now returns c.stopReader for every level. Before, each later level returned nil, which overwrote the cleanup in Lag.Prepare. That overwrite always happened, because configured is never assigned, so every level reaches prepareBlip. Now Engine.stopCollectors gets a real cleanup func, and the leak is closed. The other error-path cleanups are consistent with the Collector.Prepare contract in collector.go:22-27 ("called once when the collector is destroyed"):

• the deferred stop on named err in Lag.Prepare
• the deferred cleanup of discarded collectors in Engine.Prepare
• the cleanup of the failing collector

Calling BlipReader.Stop twice is safe, because it checks stopChan and doneChan before closing. repl.lag is the only in-tree collector that returns a cleanup func, so the new engine error-path cleanup cannot affect other domains. The early return from wait() only changes behaviour on Stop; otherwise the reader keeps its old time.Sleep timing.

Blocking: none.

Non-blocking: one inline note on heartbeat/reader.go: a single stale status write can still happen after Stop when a query is in flight.

Validation:

• go build ./... and go vet on the heartbeat, metrics/repl.lag, monitor and test packages: pass.
• go test -run TestPrepareBlip ./metrics/repl.lag/: pass.
• The two new monitor engine tests need the MySQL fixture, which was not available locally. CI build is green.
• I could not run revert checks in my sandbox. By reading the assertions, reverting prepareBlip to return nil, nil would always fail TestPrepareBlipAtMultipleLevelsReturnsReaderCleanup at its nil-cleanup t.Fatal. Removing the deferred stop would fail TestPrepareBlipErrorStopsReader with 1 reader running.