[Areson]

Lag.Prepare never assigned configured, so its per-level loop had three bugs (all also on main):

• Every level re-ran the full writer setup, and a plan with different writers at different levels wasn't rejected.
• With the blip writer, only the first level (random map order) got report-no-heartbeat. Other levels reported repl.lag.current = -1 instead of dropping it.
• The heartbeat reader was made before repl-check was set, so it never selected @@<repl-check> and report-not-a-replica did nothing.

Now Prepare sets up the writer once, from the first level (by name) that sets pfs or blip, or else by auto-detecting once. Levels with no writer or auto use that writer, because inheritance can leave them unset. Only pfs at one level and blip at another is rejected, whatever the map order. Prepare also sets report-no-heartbeat, report-not-a-replica and default-channel-name at every level. The reader gets repl-check from the level options.

This also documents the cleanup-on-error contract on blip.Collector.Prepare: a collector releases what it started before returning an error, and returns a nil cleanup. The engine call from #189 stays as a documented backstop. Lag already did this.

BlipReader.Stop now waits for the reader to return (separate commit). Before, a heartbeat query running at stop could report status after the loader removed the monitor's status, and the removed monitor reappeared in /status/monitors. Stop can now block up to ReadTimeout (2s). Monitor.Stop's db.Close already waited for that query, so the wait moves earlier rather than adding time. It fits in #190's 3s default stop timeout, and exceeding that timeout only reports error:stop. A reader stopped before its Start goroutine runs doesn't start, so Stop doesn't block on it. TestReader now stops its reader.

The new tests run without MySQL, except TestPrepareAutoAndPFSOnMySQL80. Each fails on the old Prepare.

🤖 Generated with Claude Code

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/196, 298c1b3.

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

This PR fixes repl.lag multi-level setup. The writer is now chosen once and deterministically, report-no-heartbeat, report-not-a-replica and default-channel-name apply at every level, and repl-check reaches the heartbeat reader. BlipReader.Stop now blocks until the reader goroutine exits. The riskiest change is the blocking Stop, and I found its locking and lifecycle sound. The remaining notes are low-impact.

Assumptions & unknowns

• I reviewed 37c09a26 first. During the review the head moved to 298c1b35. The new head is a rebase onto main (ef742f2): git range-diff shows all three commits patch-identical, and none of the PR's 10 files differ between the two heads. Links are pinned to 298c1b35.
• This was a source-only review. Local Go commands (go build, go vet, go test -race) could not run in this environment, so nothing was executed and no pin checks ran.
• PR CI ran only DCO, Semgrep and zizmor (all pass). No Go tests ran in CI for this PR.

Nice-to-have

[NICE-TO-HAVE] Heartbeat reader options set on a non-config level are silently ignored (PLAUSIBLE)

File:

blip/metrics/repl.lag/lag.go

Line 210 in 298c1b3

if writer, err = c.configure(ctx, configLevel, plan, writer, plan.Levels[configLevel].Collect[DOMAIN].Options); err != nil {

Failure scenario: Take two levels that don't inherit from each other, for example freqs 5s and 7s (see plan/sort.go:71), and leave writer unset on both. If only the later level (by name) sets repl-check, table, source-id/source-role or network-latency, those options are dropped with no error or log. Before, verifyReplCheck ran at every level, so a bad repl-check there failed Prepare. Now it only runs at the config level.
Evidence: configure(ctx, configLevel, plan, writer, plan.Levels[configLevel].Collect[DOMAIN].Options) builds the reader from one level's options (ReplCheck: options[OPT_REPL_CHECK], L309), and verifyReplCheck runs only there (L249). This is not a regression: base also built the reader from one level, picked at random by map order, and passed an empty c.replCheck. Head makes the choice deterministic. Assumption: such plans exist in practice; they need non-nested freqs or explicit per-level overrides.
Suggested fix: Either reject or log reader options that differ across levels, or state in the Prepare doc comment and the release note that these options are read from a single level only.

General suggestions

• heartbeat/heartbeat_test.go:207: defer hr.Stop() now blocks until run returns. The mock waiter's unconditional hbChan <- lag (L196, cap 1) can block the reader inside Wait. So if the test fails at the db.Exec fatal mid-test, it hangs until the go test timeout instead of failing promptly. A non-blocking send (select { case hbChan <- lag: default: }) avoids this.
• Plans with writer: pfs at one level and writer: blip at another now fail Prepare. Base prepared them, because its mismatch check was dead code. The release notes and doc comment disclose this and it matches the stated contract. Consider listing it under upgrade notes as a behavior change, since affected monitors retry Prepare indefinitely.

The one thing that could have broken, verified

BlipReader.Stop now waits on doneChan (heartbeat/reader.go:232-246). Verified by inspection:

• Stop closes stopChan and reads started under the lock, then waits without it. run takes the lock only briefly to update lag, so it can't deadlock.
• Start (L119-133) refuses to run after Stop, so go c.lagReader.Start() racing a Stop can't leave Stop blocked or start an orphan reader.
• run closes doneChan on every exit path via defer, and a running query is bounded by ReadTimeout (2s), which is less than the 3s DEFAULT_MONITOR_LOADER_STOP_TIMEOUT (config.go:361).

Verified correct

• Writer selection (lag.go:191-208):
  • Levels are sorted, ""/auto defer to an explicit writer, invalid values are rejected, and only pfs mixed with blip errors.
  • configure never returns auto, so Collect can't hit its panic for a prepared level.
• Per-level options: dropNoHeartbeat, dropNotAReplica and defaultChannelNameOverrides are set for every collecting level and read per level in collectBlip and collectPFS. This fixes the base behavior where only one random level dropped -1.
• Cleanup on error:
  • The deferred stopReader covers a verifyReplCheck failure after the reader started, and Prepare returns a nil cleanup on error, matching the new blip.Collector.Prepare contract text.
  • The engine backstop call is harmless, because a double Stop is safe.
  • Collectors are constructed fresh per Engine.Prepare, so no lagReader state carries across.
• c.replCheck used by collectPFS (pfs.go:84) still comes from the config level and is set after the trial collect, the same as base.
• New lag_test.go tests use the real BlipReader or a newReader hook at the Prepare entry point. By inspection, they would fail against base Prepare; they were not executed.
• Not applicable: security, schema or wire compatibility, migrations.

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/196, 3a20531.

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

This is a re-review. The only new commit is 3a20531, which addresses all three notes from the previous review of 298c1b3. Its doc text matches the Prepare code, and its test change removes the hang from the failure path. I found nothing new in the full PR diff that needs a fix.

Assumptions & unknowns

• I reviewed head 3a205314 against main (ef742f2, which is also the merge-base). The previous review covered 298c1b35. The only change since then is commit 3a20531, which touches release-notes.md, heartbeat_test.go and a doc comment in lag.go.
• This was a source-only review. Local Go commands weren't permitted in this environment, so no tests, builds or pin checks ran.
• CI at review time: DCO, Semgrep and zizmor passed, and build was still pending.

Prior findings

• Reader options set on a non-config level are silently ignored (NICE-TO-HAVE): mitigated by documentation. The Prepare doc comment and the release note now state the rule. It is the first level, by name, that sets writer to pfs or blip, else the first level that collects lag. I checked the rule against sort.Strings(levels) / configLevel := levels[0] and the override loop at L190-L210. The option list (table, source-id, source-role, repl-check, network-latency) matches what prepareBlip and configure read from options. The options are still ignored with no warning at runtime, which is the documented choice.
• TestReader mock waiter can hang the deferred Stop (suggestion): fixed, verified by inspection. The send at heartbeat_test.go:197-200 is now select { case hbChan <- lag: case <-done: }. defer close(done) (L212) is registered after defer hr.Stop(), so it runs first. Defers also run on t.Fatal, so the reader can't stay blocked in the send.
• List the pfs-and-blip rejection as a behavior change (suggestion): fixed. It now has its own Behavior change bullet at release-notes.md:58. "Previously each level used its own writer" is accurate: on main, configured is declared but never assigned, so the mismatch check never ran.

General suggestions

• docs/content/metrics/domains/repl.lag/_index.md is where users look up repl.lag options, and it still doesn't say which options are read from one level. A sentence there would help. The release note groups repl-check under "heartbeat reader options", but it also drives the PFS path (pfs.go:84). Saying "writer options" would make clear that the single-level rule applies to PFS users too.

The one thing that could have broken, verified

The riskiest change since the last round is the TestReader defer ordering, now that BlipReader.Stop waits for run to return. I checked it by inspection:

• Defers run last-in-first-out, so the order is close(done), then hr.Stop(), then the ReadErrorWait reset, then db.Close.
• After done closes, the reader's remaining blocking points are bounded. The heartbeat query has a ReadTimeout (2s) context, and wait returns when stopChan closes.
• Stop waits on doneChan without holding the lock, so it can't deadlock with run.
• The success-path assertions are unchanged.

Verified correct

• The BlipReader Start/Stop lifecycle (unchanged since the last round) still holds:
  • A Stop before Start leaves Start a no-op.
  • A double Stop is safe.
  • run closes doneChan on every exit path.
• On the Prepare error path, the reader is stopped and a nil cleanup is returned, and the engine backstop doesn't double-count the failed collector.
• report-no-heartbeat, report-not-a-replica and default-channel-name are still set at every level (lag.go:216-222).
• Engine cleanup runs under e.Lock(), so a plan change can now block Collect for up to 2s while a heartbeat query is running. That wait is bounded, disclosed at release-notes.md:59, and in line with the existing collectorWG.Wait() under the same lock. It's not a defect.
• Not applicable: security, wire or schema compatibility, migrations.

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/196, 66ed6db.

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

This is a re-review. The only new commit is 66ed6db, which changes docs and a doc comment and addresses the one suggestion from the review of 3a20531. I checked the new text against Prepare, and it is accurate. The PR now has a merge conflict with main and needs a rebase.

Assumptions & unknowns

• I reviewed head 66ed6db8 against its merge-base ef742f2. git range-diff shows that the four earlier commits are identical to the head I last reviewed (3a20531). The only new commit is 66ed6db (parent 3a20531), which touches release-notes.md, repl.lag/_index.md and the Prepare doc comment in lag.go.
• main has since moved to d1cecf1 (#193), which is not in this PR. GitHub reports the PR as CONFLICTING; see General suggestions.
• This was a source-only review. Running Go commands wasn't permitted in this environment, so no tests, builds or pin checks ran.
• CI at review time: DCO passed.

Prior findings

• Document the single-level options on the repl.lag domain page, and call repl-check a writer option (suggestion): fixed, verified by inspection.
  • The new paragraph at _index.md:74-76 matches the code:
    • Levels are sorted by name (sort.Strings(levels), configLevel := levels[0]), and the first pfs/blip writer wins (lag.go:190-210).
    • The per-level loop sets only report-no-heartbeat, report-not-a-replica and default-channel-name (L216-L222).
    • Together the two lists cover all nine options in Help().
  • The release note and the doc comment now say "writer options", which covers the PFS path's use of repl-check (pfs.go:84).
• Earlier findings (reader options on non-config levels; TestReader hang; mixed-writer upgrade note) stay closed as recorded in the previous round. Nothing in the earlier commits changed.

General suggestions

• Rebase onto main. #193 (d1cecf1) appends TestCollectPFSReturnsDeadlineExceeded to metrics/repl.lag/lag_test.go, and so does this PR, so git merge-tree reports a conflict there. The conflict is in the import block and the appended tests; keep both sides.
  • The conflict is textual only. #193's test uses only NewLag, lagWriterIn, replCheck and Collect, which this PR keeps with the same types.
  • #193's %w change in pfs.go doesn't change the trial collectPFS in configure.
  • CI should run on the rebased head.
• Pre-existing doc mismatches on the page this PR edits:
  • network-latency is documented as a duration string such as 250ms (_index.md:118). The code parses it with strconv.Atoi as milliseconds and silently uses 50 ms on failure (lag.go:298-300).
  • report-not-a-replica is listed under "Blip Heartbeat", but collectPFS applies it too (pfs.go:70).
  • These can be fixed in a separate PR.

The one thing that could have broken, verified

Since the last round, the riskiest part is the semantic interaction with #193 at rebase time, not the docs-only commit. I checked it by inspection:

• #193's test sets lagWriterIn["kpi"] directly and skips Prepare. All maps used are initialized by NewLag (lag.go:54-65).
• The expired context fails the repl-check query at pfs.go:86, which #193 wraps with %w.
• No helper or test names collide with the PR's new fakeReader, blipPlan, unreachableDB and stopReader helpers.

Verified correct

• The doc comment at lag.go:159-166 describes Prepare accurately, including the pfs/blip rejection at L207-L208.
• The Prepare flow is unchanged since the last round:
  • The deferred stopReader runs on error.
  • configure runs once.
  • repl-check reaches the reader from the level options.
  • Prepare returns a nil cleanup when no blip reader started.
• Not applicable: security, wire or schema compatibility, migrations.

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/196, 237ecd7.

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

This is a re-review. The new head 237ecd7 is a rebase of the five commits reviewed at 66ed6db onto main (d1cecf1, #193). It resolves the lag_test.go conflict noted last round and drops nothing from either side. A fresh pass over the full diff found no defects. CI at this head ran go build and go test -v ./..., and both passed.

Assumptions & unknowns

• Reviewed head 237ecd72 against origin/main d1cecf1, which is also the merge-base. The PR is MERGEABLE.
• Range-diff against 66ed6db: commit 3 is identical. Commits 1, 2, 4 and 5 differ only in rebase context in release-notes.md and lag_test.go. git diff 66ed6db 237ecd7 touches the same 13 files with the same line counts as git diff ef742f2 d1cecf1 (#193), and the repl.lag hunks are exactly #193's %w change and its TestCollectPFSReturnsDeadlineExceeded.
• I couldn't run Go commands in this environment, so local tests and pin checks didn't run. I used the CI log for test results instead.
• CI skips the MySQL-backed tests (TestReader, TestPrepareAutoAndPFSOnMySQL80, TestPrepareForSingleLevelAndNoSourceOnMySQL80), and doesn't run with -race.

Prior findings

• Rebase onto main (suggestion): fixed, verified by inspection. Both sides of the lag_test.go conflict are kept: the errors and test/mock imports, #193's TestCollectPFSReturnsDeadlineExceeded, and this PR's tests. CI shows TestCollectPFSReturnsDeadlineExceeded passing at this head.
• Pre-existing doc mismatches (network-latency units; report-not-a-replica listed under Blip Heartbeat only): still open, out of scope. They were deferred to a separate PR last round and are unchanged.
• Earlier findings (reader options on non-config levels, the TestReader hang, the mixed-writer note, the domain-page docs) stay closed. The code and docs for them are unchanged by the rebase.

General suggestions

• The release note scopes the repl-check fix to multiple levels, but it also changes single-level plans. On base, prepareBlip built the reader with c.replCheck, which was still "" at that point, so repl-check had no effect on the heartbeat reader in any plan. Now it uses options[OPT_REPL_CHECK] (lag.go:311). As a result, a single-level writer: blip (or auto-detected blip) plan with repl-check set now drops repl.lag.current on non-replicas, unless report-not-a-replica=yes.
  • This matches the documented contract (_index.md:87), so it's a fix, not a regression.
  • release-notes.md:60 files it under "at multiple levels". Consider a sentence saying that with repl-check set, non-replicas now stop reporting heartbeat lag.

The one thing that could have broken, verified

The riskiest change in this round is the rebase over #193, which edits the same test file and pfs.go.

• Verified by inspection: the tree delta between the two heads equals #193's delta, file for file. #193's %w wrapping in collectPFS doesn't change how configure uses the trial collectPFS result (error or nil).
• Verified by execution in CI at 237ecd7:
  • All new repl.lag tests pass: TestPrepareWritersAtDifferentLevels (10 subtests), TestPrepareSetsOptionsAtEveryLevel, TestPrepareBlipPassesReplCheckToReader, and TestPrepareBlipAtMultipleLevelsDropsNoHeartbeat.
  • All new heartbeat tests pass: TestBlipReaderStopWaitsForRunningQuery, TestBlipReaderStopBeforeStart, and TestBlipReaderQuerySelectsReplCheck.
  • #193's test passes.

Verified correct

• BlipReader.Stop waits on doneChan without holding the lock.
  • The query is bounded by context.WithTimeout(..., ReadTimeout) (reader.go:158), and every other wait selects on stopChan.
  • run takes no engine or collector lock, so the up-to-2s hold under e.Lock during a plan swap is bounded, disclosed, and not a deadlock.
• Writer selection is deterministic: levels are sorted, ""/auto levels inherit, and only mixing pfs with blip errors. configure runs once and never returns auto. Per-level options are set for every collecting level (lag.go:216-222).
• On the error path, the deferred stopReader stops a reader started before a verifyReplCheck failure, and Prepare returns a nil cleanup. The engine backstop is harmless.
• Not applicable: security, wire or schema compatibility, migrations.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run; CI go test -v ./... passed at the reviewed head.

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/196, 15d2a49.

Verdict: ship — clean, approve.

This is a re-review. The only new commit is 15d2a49, which moves the heartbeat-reader repl-check change out of the multiple-levels note and into its own Behavior change note. That was the one suggestion from the review of 237ecd7. I checked every claim in the new note against the code at base and head, and they're accurate. A fresh pass over the full diff found no defects.

Assumptions & unknowns

• I reviewed head 15d2a499 against origin/main d1cecf1, which is also the merge-base. The PR is MERGEABLE. The parent of 15d2a49 is the previously reviewed 237ecd7, so the earlier five commits are unchanged. git diff 237ecd7 15d2a49 touches only release-notes.md (+2/−1).
• Running Go commands wasn't permitted in this environment, so no local tests or pin checks ran. CI build passed at this head, but I couldn't open its log, so I can't list which tests ran in it.

Prior findings

• The release note scoped the repl-check fix to multiple levels (suggestion): fixed, verified by inspection. release-notes.md:61 is now a standalone behavior change, and the multiple-levels note at L60 no longer claims it. Each claim in the new note holds:
  • "previously it ignored it at every level": on base, prepareBlip built the reader with c.replCheck before that field was assigned, and later levels returned early once c.lagReader != nil. The query therefore selected the constant 1, never @@<repl-check>.
  • Non-replica drops repl.lag.current: head passes ReplCheck: options[OPT_REPL_CHECK] (lag.go:311). The reader selects @@<repl-check> (reader.go:109-110) and sets isRepl = false on 0 (L182-L184). Lag() then returns Replica: false, Milliseconds: -1 (L251-L252), and collectBlip drops the metric when dropNotAReplica is set (lag.go:330-332).
  • "reports -1 with report-not-a-replica=yes": in that case it falls through and reports float64(lag.Milliseconds), which is -1 (lag.go:337-340).
  • Scoping to the Blip writer: the scope is correct. An auto-detected Blip writer reaches the same lag.go:311. The PFS path already honored repl-check on base (pfs.go:84-92, unchanged).
• Pre-existing doc mismatches (network-latency units; report-not-a-replica listed under Blip Heartbeat only): still open, out of scope. They are unchanged and deferred to a separate PR.
• Earlier findings (reader options on non-config levels, the TestReader hang, the mixed-writer note, the domain-page docs, the rebase onto #193) stay closed. The code and docs for them are unchanged in this round.

The one thing that could have broken, verified

The new note is now the release's upgrade guidance for single-level Blip-writer plans. If it were wrong, operators would misread a metric disappearing. I verified its claims by inspection end to end, from options[OPT_REPL_CHECK] through the reader query and Lag() to collectBlip (links above).

I also checked one edge case and refuted it: a repl-check sysvar whose value isn't an integer. It fails the int scan at reader.go:159, and the failure is not silent: it is reported as error:heartbeat-reader monitor status at L173. It also violates the documented contract (a numeric sysvar, server_id recommended), and the PFS path already rejects it on base.

Verified correct

• The BlipReader Start/Stop lifecycle still holds, unchanged since the last round:
  • A Stop before Start leaves Start a no-op (reader.go:119-133).
  • Stop waits on doneChan without holding the lock.
  • run closes doneChan on every exit path.
  • The wait is bounded by ReadTimeout.
• Writer selection is deterministic (levels sorted by name), and only mixing pfs with blip errors.
• report-no-heartbeat, report-not-a-replica and default-channel-name are set at every level.
• On the Prepare error path, the reader is stopped and a nil cleanup is returned.
• CI at 15d2a49: build, DCO, Semgrep and zizmor all passed.
• Not applicable: security, wire or schema compatibility, migrations.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run; CI build passed at the reviewed head.

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/196, 3a4f01f.

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

This is a re-review after a rebase. The new head is exactly the previously approved 15d2a49 rebased onto main at 91c7483, which added #194 (retry options on built-in sinks). The PR's own changes are unchanged, and #194 doesn't touch any code this PR uses. A fresh finder and verifier pass over the full diff found no code defects. It found one inaccurate release-note phrase and one optional design improvement.

Assumptions & unknowns

• I reviewed head 3a4f01f7 against origin/main 91c7483, which is also the merge-base. The PR is MERGEABLE.
• Running Go commands wasn't permitted in this environment, so no local tests or pin checks ran. I read the CI build job log for this head instead (see Verified correct).
• The MySQL-backed tests (TestPrepareAutoAndPFSOnMySQL80, TestReader) were skipped in CI, so the PFS auto-detect path and the real-reader stop path are verified by inspection only.

Prior findings

• The rebase dropped nothing. git range-diff shows commits 1–5 differ only in release-notes context. Outside release-notes.md, the diff from 15d2a49 to 3a4f01f has the same patch-id as d1cecf1..91c7483 (#194) itself. All four of #194's release-note bullets are kept (L60-L63), and this PR's bullets follow them.
• Every earlier finding stays as it was. The code and docs behind each are unchanged:
  • The reader options on non-config levels, the TestReader hang, the mixed-writer note, the domain-page docs, the #193 rebase and the repl-check behavior-change note are all fixed.
  • The pre-existing doc mismatches (network-latency units, and report-not-a-replica listed under Blip Heartbeat only) are still open and out of scope.

General suggestions

• The release note overstates the report-not-a-replica fix. release-notes.md:64 says report-no-heartbeat and report-not-a-replica "now apply at every level, not only at one random level." That is true only of report-no-heartbeat.
  • On base, configured was never assigned, so every level reached c.dropNotAReplica[levelName] = ....
  • Only dropNoHeartbeat sat after prepareBlip's if c.lagReader != nil early return (L259-L263).
  • With the Blip writer, report-not-a-replica had no effect at all on base, because the reader ignored repl-check. That is already covered by the behavior-change bullet at L65.
  • Suggested fix: drop report-not-a-replica from the L64 bullet, or say it now takes effect through repl-check.
• Optional: BlipReader.Stop could cancel the query instead of waiting for it. The heartbeat query uses context.WithTimeout(context.Background(), ReadTimeout) (reader.go:158), so Stop waits up to 2s (L244) against a hung MySQL.
  • The heartbeat writer derives its context from stopchan.Context and returns right away.
  • The wait is disclosed, bounded, within the 3s stop timeout, and covered by config-file.md's "cleanup function doesn't return promptly" case, so this is not a defect.

The one thing that could have broken, verified

The rebase onto #194 could have silently dropped or merged over code or release-note lines, which is the only thing that changed since the last approval. Verified by execution with git:

• range-diff paired commits 1–5, which differ only in release-notes context.
• Outside the release notes, the head-to-head delta has the same patch-id as #194 itself (fb03cc47… for both).
• The only release-notes change between the two heads is #194's four added bullets.
• git diff --stat origin/main...HEAD lists exactly the PR's original 11 files.

Verified correct

• Writer selection is set up once, from the first level by sorted name that sets pfs or blip. Only mixing those two is rejected. report-no-heartbeat, report-not-a-replica and default-channel-name are set at every level (lag.go:219-220). The reader gets options[OPT_REPL_CHECK] (L311). All verified by inspection.
• The Prepare error path stops a started reader and returns a nil cleanup. The engine's cleanup call stays as a harmless backstop. Verified by inspection.
• The BlipReader Start/Stop lifecycle is sound:
  • A Stop before Start leaves Start a no-op.
  • Stop waits on doneChan without holding the lock.
  • run closes doneChan on every exit path.
• No interaction with #194. It touches only sink/ and docs, with no overlap with heartbeat/, repl.lag, monitor/engine.go or collector.go.
• CI at 3a4f01f: build (go test -v ./...), DCO and zizmor passed. The log shows every package ok, 0 --- FAIL, and all the new tests passing, including TestPrepareWritersAtDifferentLevels (10 subtests), TestPrepareSetsOptionsAtEveryLevel, TestPrepareBlipPassesReplCheckToReader, TestPrepareBlipAtMultipleLevelsDropsNoHeartbeat, TestBlipReaderStopWaitsForRunningQuery, TestBlipReaderStopBeforeStart and TestBlipReaderQuerySelectsReplCheck.
• Not applicable: security, wire or schema compatibility, migrations.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run; CI go test -v ./... passed at the reviewed head.

[zyin-squareup]

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

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

This is a re-review. The only new commit is e4ef501, a one-line release-note edit that drops report-not-a-replica from the "now applies at every level" bullet, as the previous round suggested. The edited line is now accurate against the base and head code. A fresh finder and verifier pass found no code defects.

Assumptions & unknowns

• I reviewed head e4ef5018 against origin/main 91c7483, which is also the merge-base. The PR is MERGEABLE.
• git diff 3a4f01f..e4ef501 changes only release-notes.md:64. Everything else matches the head approved last round.
• Running Go commands wasn't permitted in this environment, so no local tests or pin checks ran. The change is docs-only, so there is nothing to mutation-test. I read the CI build job log for this head instead.

Prior findings

• Release note overstated the report-not-a-replica fix: fixed. The bullet now names only report-no-heartbeat. That matches base:
  • c.dropNotAReplica[levelName] = ... ran at every level, because configured was never assigned.
  • c.dropNoHeartbeat[levelName] = ... ran only for the first level to reach prepareBlip, after the if c.lagReader != nil early return.
  • The "has no effect with the Blip writer" part stays covered by the repl-check behavior-change bullet at L65.
  • Verified by inspection. This is a docs fix, so no test applies.
• Optional BlipReader.Stop cancellation suggestion: unchanged, not a defect. The wait is still disclosed and bounded at 2s.
• All earlier code and docs findings stay fixed. The code is byte-identical to the last approved head. The pre-existing doc mismatches (network-latency units, and report-not-a-replica listed under Blip Heartbeat only) are still out of scope.

General suggestions

• Optional: say what unset or auto levels did before. release-notes.md:64 states the new rule: "Levels that don't set writer, or set it to auto, use the writer set at another level."
  • On base, each such level auto-detected on its own. With one level at writer: blip, an unset level on a PFS-capable MySQL 8 used pfs.
  • On head it uses blip (lag.go:198, L218).
  • The rule and the PR description disclose this. Adding "Previously such levels auto-detected their own writer" would make it as explicit as the pfs/blip bullet at L66.

The one thing that could have broken, verified

The new commit could have touched more than the release note, or made it inaccurate in a new way. Verified by execution with git: the 3a4f01f..e4ef501 diff is one line in one file. Verified by inspection: the new wording matches base lag.go:217/:259-263 and head lag.go:216-222, where every level gets dropNoHeartbeat, dropNotAReplica and defaultChannelNameOverrides.

Verified correct

• Writer selection: it uses sorted level names and the first level that sets pfs/blip. Only mixing pfs and blip is rejected (lag.go:190-210). Verified by inspection.
• Error path, reader lifecycle and engine comments: unchanged since last round. A fresh finder pass re-traced them and found them sound.
• CI at e4ef501:
  • build (go test -v ./...), DCO, Semgrep and zizmor passed. The log has 0 --- FAIL lines.
  • The new tests pass, including TestPrepareWritersAtDifferentLevels (10 subtests), TestPrepareSetsOptionsAtEveryLevel, TestPrepareBlipAtMultipleLevelsDropsNoHeartbeat, TestPrepareBlipPassesReplCheckToReader and the three TestBlipReader* tests.
  • The MySQL-backed TestPrepareAutoAndPFSOnMySQL80 and TestReader were skipped.
• Not applicable: security, wire or schema compatibility, migrations.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run; CI go test -v ./... passed at the reviewed head.

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/196, 5777c64.

Verdict: ship — clean, approve.

This is a re-review. The only new commit is 5777c64, a one-sentence release-note edit. It says what unset or auto repl.lag levels did before, which was the one suggestion from the review of e4ef501. I checked the new sentence against base lag.go and it is accurate. A fresh finder pass over the full PR diff found no defects.

Assumptions & unknowns

• I reviewed head 5777c64d against origin/main 91c7483, which is also the merge-base. The PR is MERGEABLE. The parent of 5777c64 is the previously approved e4ef501, and git diff e4ef501 5777c64 changes only release-notes.md:64 (+1/−1).
• Running Go commands wasn't permitted in this environment, so no local tests or pin checks ran. The change is docs-only, so there is nothing to mutation-test. CI build passed at this head, but I couldn't open its log, so I can't list which tests ran.
• The finder found no candidates, so the verifier stage had nothing to check.

Prior findings

• Say what unset or auto levels did before (suggestion): fixed, verified by inspection. The bullet now adds "Previously each of these levels auto-detected its own writer, so it could use pfs while another level used blip." Both halves hold on base:
  • configured := "" is never assigned, so the "already configured" branch and its mismatch error (L177-L179) never run.
  • Every collecting level reaches case "auto", "":. It tries collectPFS first, and on success sets writer = LAG_WRITER_PFS. So with another level at writer: blip on a PFS-capable server, the unset level used pfs.
  • "could" is the right word. Two unset levels on the same server normally detect the same writer. The mix is deterministic only when another level sets blip explicitly.
• Optional BlipReader.Stop cancellation suggestion: unchanged, not a defect. The wait is still disclosed and bounded by ReadTimeout (2s).
• All earlier code and docs findings stay closed. The code is byte-identical to the last approved head. The pre-existing doc mismatches (network-latency units, and report-not-a-replica listed under Blip Heartbeat only) are still out of scope.

The one thing that could have broken, verified

The new commit could have touched more than the release note, or made the note inaccurate in a new way.

• Verified by execution with git: e4ef501..5777c64 is one line in one file.
• Verified by inspection: the new wording matches base lag.go:155-215 (per-level auto-detection with a dead mismatch check) and head lag.go:195-218. On head, ""/auto levels continue and then get the chosen writer.

Verified correct

• Writer selection: levels are sorted by name, and the first level that sets pfs/blip wins. Only mixing pfs and blip is rejected (lag.go:190-210). report-no-heartbeat, report-not-a-replica and default-channel-name are set at every level (L216-L222). Verified by inspection.
• Prepare error path: the deferred stopReader runs on any error, and Prepare returns a nil cleanup. The engine backstop is harmless. Verified by inspection.
• BlipReader Start/Stop lifecycle:
  • A Stop before Start leaves Start a no-op.
  • Stop waits on doneChan without holding the lock.
  • run closes doneChan on every exit path.
  • A fresh finder pass re-traced all three.
• CI at 5777c64: build, DCO, Semgrep and zizmor passed.
• Not applicable: security, wire or schema compatibility, migrations.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run; CI build passed at the reviewed head.