[Areson]

Stacked on #196. Tests only.

• Engine cleanup tests run in CI. Both tests in monitor/engine_cleanup_test.go skipped in CI because they needed MySQL, but Engine.Prepare only pings the database. New mock.PingDB (next to mock.ContextDB) is a *sql.DB whose pings succeed and queries fail right away. Both tests use it. TestEnginePrepareErrorCleansUpPreparedCollectors fails if either cleanup path in Engine.Prepare from Stop repl.lag heartbeat reader on monitor stop #189 is removed. mock.ContextDB also connects without MySQL, but its queries block until the context ends, so each Engine.Stop waits the full heartbeat read timeout (2s).
• No dependence on test order. The leak tests required no heartbeat readers anywhere in the process before they started. A plain before/after count can still flake when a reader stopped by an earlier test exits during the test. So a test calls test.Goroutines.Label, which sets a pprof label on the test goroutine that the reader goroutines it starts inherit, and counts only goroutines with that label. Error path tests also check that Prepare called Stop, because a leaked reader may not have started yet when the count is taken. TestPrepareBlipAtMultipleLevelsDropsNoHeartbeat now checks that its reader stops, too.
• Leaks from tests that don't count readers are still caught. A TestMain in monitor and metrics/repl.lag waits after all tests for no heartbeat readers in the process, and fails the run otherwise. It runs once, after all tests, so it doesn't depend on test order.
• Counts can't silently drop to 0. test.Goroutines parses the text goroutine profile. If its format changes, a count of 0 would make every "no readers left" check pass. The count finds its own goroutine in the profile, with the group label, or panics. It also panics on a stack deep enough that the profile might have truncated it, since truncation drops the outer frames, like the reader's run.
• One copy of the constants. test.HeartbeatReaderFunc and test.HeartbeatReaderTimeout replace the copies in both test packages. The new tests from Set up repl.lag once and apply options at every level #196 use them too.

🤖 Generated with Claude Code

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/197, c2ff589.

Verdict: ship — 1 finding: 0 blocking, 0 important, 0 nice-to-have, 1 suggestion.

This PR changes only tests. It makes the heartbeat-reader leak tests independent of test order by counting only goroutines with a pprof label. It also runs the engine cleanup tests without MySQL by using mock.PingDB. The highest risk was a goroutine count that silently reads 0 and makes the "no readers left" checks pass. The label inheritance and the foundSelf guard prevent this, and no defects survived verification.

Assumptions & unknowns

• Reviewed head c2ff58942afd081a4b3bba5466c49d4f8605dca1 against origin/Areson/repl-lag-level-setup, fetched fresh. The PR is stacked on #196.
• Source-only review. Local go test and CI-log retrieval were not available in this session. Execution evidence is limited to CI's pass status.
• The claim that the pprof debug=1 format contains # labels: {"k":"v"} comes from reading the source, with CI passing on Go 1.24 as indirect evidence. If the format did not match, the foundSelf panic would fire.

General suggestions

• monitor/engine_cleanup_test.go#L43: consider adding t.Cleanup(engine.Stop) after NewEngine. When a t.Fatalf at L48 or L52 fires, the engine is currently left running, so the new TestMain waits 5s and prints a second, redundant FAIL. This gap predates the PR, and it only matters on a run that is already failing.

The one thing that could have broken, verified

A labeled count that never sees the reader would turn every Wait(..., 0, ...) into a pass. I verified by inspection that the labeled test goroutine starts the reader:

• lag.go#L317 runs go c.lagReader.Start(), and reader.go#L131 runs go r.run(). Both are synchronous from Prepare, so the child goroutines inherit the label.
• Each zero check has a guard: a preceding Wait(..., 1, ...), or requireReaderStopped on the error paths, where a not-yet-started reader would otherwise count 0.
• goroutine.go#L126 panics if the caller's own labeled stack is missing, instead of returning 0.

Verified correct

• Label matching: fmt.Sprintf("%q:%q", ...) includes the closing quote, so group id 1 can't match 10.
• Truncation guard: it checks only stacks without fn, which is correct because truncation drops the outer run frame. 100 frames is below the 128-frame profile cap.
• mock.PingDB: Engine.Prepare only calls PingContext (engine.go#L165). Queries fail through Prepare with a non-ErrBadConn error, so database/sql does not retry. The reader waits ReadErrorWait (1s) between attempts, so it doesn't spin.
• Double cleanup(): BlipReader.Stop is safe to call more than once, concurrently, and before Start (reader.go#L232).
• Spy reader: it wraps the real reader through the newReader seam. Reverting the stop-on-error defer in Lag.Prepare leaves stopped false and fails the test, verified by inspection.
• TestMain process-wide check: no other monitor or repl.lag test leaves a reader running. The repl.lag tests register stopReader, and no monitor test plan uses repl.lag. I found no false failure, including with local MySQL.
• Removed symbols: the old test.Goroutines(fn) func, requireNoReaders, and the duplicated constants have no remaining callers.
• Categories: security, wire compatibility, migrations, and production performance are N/A because the PR changes only tests. CI passed: build (go test ./...), DCO, Semgrep, and zizmor.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run.

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/197, 6c1d8fa.

Verdict: ship — 2 findings: 0 blocking, 0 important, 0 nice-to-have, 2 suggestions.

This is a re-review after a rebase. The PR's two commits are patch-identical to the earlier approved head c2ff589, and the only change is the newer base. The new base changes only sink/ and docs, which these test-only changes don't touch. A fresh finder and verifier pass over the full diff found no defects.

Assumptions & unknowns

• Reviewed head 6c1d8fac48f3cf012058bf6c8f23667040882573 against origin/Areson/repl-lag-level-setup, fetched fresh. The PR is stacked on #196.
• git range-diff shows both commits = (identical) to the previously reviewed abe6e08/c2ff589. The base moved by sink/factory.go, sink/retry.go, their tests, and docs only.
• Source-only review: I couldn't run go test in this session. Execution evidence is CI only.

Prior findings

• The suggestion from the last round (add t.Cleanup(engine.Stop) in TestEnginePrepareStopDoesNotLeakHeartbeatReader) is still open. It was optional and predates the PR; it's repeated below.

General suggestions

• monitor/engine_cleanup_test.go#L43 and metrics/repl.lag/lag_test.go#L205: if a count check calls t.Fatalf before engine.Stop() or cleanup(), the reader keeps running until the process exits, and TestMain adds a second FAIL after a 5s wait. A t.Cleanup(engine.Stop) here, or stopReader(t, cleanup) as in TestPrepareBlipAtMultipleLevelsDropsNoHeartbeat, would avoid that. This predates the PR and only affects runs that are already failing.
• test/goroutine.go#L110: the comment says it panics "if a counted stack might be truncated". The guard at L130 actually checks stacks that don't contain fn, which is the right design because truncation drops the outer run frame. Consider rewording the comment to match.

The one thing that could have broken, verified

A labeled count that never sees the reader would turn every Wait(..., 0, ...) into a pass. I verified by inspection that the labeled goroutine starts the reader:

• lag.go#L317 runs go c.lagReader.Start(), and reader.go#L131 runs go r.run(). Both run synchronously from Prepare after Label(), so both inherit the label.
• Each zero check is paired with a preceding Wait(..., 1, ...), or with requireReaderStopped on the error paths.
• goroutine.go#L144 panics instead of returning 0 if the caller's labeled stack is missing.

Verified correct

• Rebase: the PR patches are unchanged, and the new sink/ base code starts no heartbeat readers, so the process-wide TestMain checks are unaffected.
• Profile parsing: the Go 1.24 debug=1 format (N @ …, # labels: {"k":"v"}, #\t frames) matches the parser. The %q:%q match includes the closing quote, so id 1 can't match 11.
• Truncation guard: maxFrames = 100 is below the 128-frame default (go.mod is go 1.24).
• mock.PingDB: Engine.Prepare only calls PingContext (engine.go#L165). errPingDB isn't ErrBadConn, so database/sql doesn't retry, and the reader waits ReadErrorWait between attempts instead of spinning.
• Cleanup paths: TestEnginePrepareErrorCleansUpPreparedCollectors still pins both Engine.Prepare cleanup paths (engine.go#L192, #L231). Verified by inspection.
• Spy reader: it wraps the real newBlipReader through the c.newReader seam, so reverting the stop-on-error defer in Lag.Prepare fails requireReaderStopped. Verified by inspection.
• Removed symbols: the old test.Goroutines(fn), requireNoReaders, and the local constants have no remaining callers.
• TestMain false failures: I found none. No monitor test plan uses repl.lag, and the repl.lag tests that start readers register stopReader.
• Categories: security, wire compatibility, migrations, and production performance are N/A because the PR changes only tests. CI at this head passed: build (go test ./...), DCO, Semgrep OSS, and zizmor.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run.

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/197, 8bafcd4.

Verdict: ship — 2 findings: 0 blocking, 0 important, 0 nice-to-have, 2 suggestions.

This is a re-review. Since the last approved head 6c1d8fa, the only change is one commit, 8bafcd4. It changes two lines in TestEnginePrepareStopDoesNotLeakHeartbeatReader: each engine now stops in t.Cleanup, and the database closes in t.Cleanup after the engines. The change is correct: Engine.Stop is idempotent, and cleanups run in the right order. A fresh finder and verifier pass found no defects.

Assumptions & unknowns

• Reviewed head 8bafcd400dbf2fd327ccfff7f4c552ea00c6a332 against origin/Areson/repl-lag-level-setup, fetched fresh. The PR is stacked on #196.
• git range-diff shows the first two commits are identical to the previously reviewed 01bf52c/6c1d8fa, and 8bafcd4 is the only new commit. The base did not move.
• This was a source-only review. I couldn't run go test in this session, so the only execution evidence is CI.

Prior findings

• t.Cleanup(engine.Stop) in TestEnginePrepareStopDoesNotLeakHeartbeatReader: fixed at engine_cleanup_test.go#L44. I verified this by inspection only; the change only affects runs that are already failing, so CI can't exercise it.
• The same backstop in TestPrepareBlipAtMultipleLevelsReturnsReaderCleanup: still open, repeated below. It was optional.
• The countGoroutines doc comment wording: still open, repeated below. It was optional.

General suggestions

• metrics/repl.lag/lag_test.go#L200: if this t.Fatalf fires while cleanup is non-nil, the reader is left running. TestMain then waits 5s and prints a second, redundant FAIL. Registering stopReader(t, cleanup) after the err check, as #L274 already does, would match the fix in the monitor test. This gap predates the PR and only affects runs that are already failing.
• test/goroutine.go#L110: the comment says the function panics "if a counted stack might be truncated". The guard at #L129 checks stacks that do not contain fn. That is the right design, because truncation drops the outer run frame. Consider "if a stack without fn might be truncated".

The one thing that could have broken, verified

The new t.Cleanup(engine.Stop) calls Stop a second time after the loop's own Stop. It would also run on an engine whose Prepare failed. I verified by inspection that both cases are safe:

• engine.go#L544-L545 runs cl.cleanup() and then cl.cleanup = nil, so a second Stop never runs the reader cleanup twice. A repeated cl.cancel() is a no-op on a context cancel func, and collectorWG.Wait() returns immediately.
• If Prepare fails, it cleans up its own collectors in its defer. It assigns e.collectors only after every error return, so Stop on that engine iterates over nothing.
• Cleanups run last-in, first-out. The db.Close cleanup at #L22 is registered first, so it runs after all three engine cleanups, as the comment says.

Verified correct

• Goroutine label counting: the reader goroutines are started by go c.lagReader.Start() and then go r.run() from the labeled test goroutine, so they inherit its label. Each zero check has a guard: either a preceding Wait(..., 1, ...) or requireReaderStopped. A missing self-stack panics instead of returning 0.
• mock.PingDB: Engine.Prepare only calls PingContext. Queries fail through Prepare with a non-ErrBadConn error, so database/sql doesn't retry them, and the reader waits ReadErrorWait between attempts.
• TestEnginePrepareErrorCleansUpPreparedCollectors pins both cleanup paths in Engine.Prepare: the deferred loop and the backstop for a cleanup returned with an error. Verified by inspection.
• Process-wide TestMain checks:
  • No other monitor test leaves a blip reader running. The only default-plan path is TestLoaderReloadKeepsSinkCounters, which needs MySQL. On that path repl.lag resolves to the pfs writer, and the test unloads its monitor.
  • CI has no MySQL service, so those tests skip there.
• Categories: security, compatibility, migrations, and production performance are N/A because the PR changes only tests. CI at this head passed: build (go test ./...), DCO, Semgrep OSS, and zizmor.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run.