feat(main): keyviz wiring follow-up (--keyvizHistoryColumns + Phase-2 read TODO)

[bootjp]

Summary

Follow-up to PR #647, which was merged at the round-1 commit before round-2 review fixes propagated. This PR carries the orphaned round-2 changes:

• New --keyvizHistoryColumns flag (defaults to keyviz.DefaultHistoryColumns = 1440, i.e. 24h at 60s) so operators can shorten the ring buffer for high-cardinality clusters without rebuilding.
• startKeyVizFlusher early-returns when the sampler is nil instead of spawning a goroutine that just parks on ctx.Done — the goroutine was harmless but had no signal.
• TODO on observeMutation documenting the Phase-2 read-sampling milestone (design §5.1, §10) so future readers don't think the missing read path is a regression. Until that wiring lands the matrix's Reads/ReadBytes series stay zero.

These items came out of Claude bot's round-2 review of #647 but landed after the merge button was pressed.

Test plan

• go build ., go vet ., golangci-lint run ./... clean.
• go test -race -count=1 -run 'TestBuildKeyVizSampler|TestSeedKeyVizRoutes|TestStartKeyVizFlusher' . clean.

[coderabbitai]

Warning

Rate limit exceeded

@bootjp has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 5 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 5 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0cc4e5b3-a2f7-4547-9caa-f1e9135a3beb

📥 Commits

Reviewing files that changed from the base of the PR and between 41c7f0f and 8f5d0e0.

📒 Files selected for processing (5)

• keyviz/sampler.go
• keyviz/sampler_test.go
• kv/sharded_coordinator.go
• main.go
• main_keyviz_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/keyviz-main-wiring-followup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 1m 59s —— View job

---

Code Review — PR #651

Todo list:

• Gather context and read PR description
• Diff changed files against origin/main
• Read main.go changes
• Read kv/sharded_coordinator.go changes
• Verify flag wiring, defaults, and usage consistency
• Cross-check design doc references (§5.1, §10)
• Post final review

---

Summary

Clean, minimal follow-up. All three stated changes are correct. One minor test isolation gap worth fixing.

---

main.go — --keyvizHistoryColumns flag (+8/-2)

Flag registration and wiring (main.go:138, main.go:1380) — correct. The flag defaults to keyviz.DefaultHistoryColumns (1440), the value flows directly into MemSamplerOptions.HistoryColumns inside buildKeyVizSampler, and the flag description accurately describes the unit. No issues here.

startKeyVizFlusher nil guard — the early return on s == nil (before eg.Go) is a clean improvement. The prior code would spawn a goroutine that blocked on ctx.Done with no productive work; the updated comment text is accurate.

Minor: withFlags helper in main_keyviz_test.go doesn't cover the new flag (main_keyviz_test.go:94). The helper saves/restores keyvizEnabled, keyvizStep, keyvizMaxTrackedRoutes, and keyvizMaxMemberRoutesPerSlot — but keyvizHistoryColumns is now a fifth package-level flag var and is not covered. This won't cause any of the current tests to fail, but if a future test modifies *keyvizHistoryColumns and calls withFlags, the value won't be restored, leaking state across tests. Adding it now while the change is fresh keeps the helper consistent with the flag surface: Fix this →

Minor: no test pins the HistoryColumns forwarding path. The three tests cited in the PR test plan exercise buildKeyVizSampler (nil vs non-nil), seedKeyVizRoutes, and startKeyVizFlusher lifecycle — but none asserts that the value of *keyvizHistoryColumns actually reaches MemSamplerOptions.HistoryColumns. A one-line assertion inside TestBuildKeyVizSamplerHonorsEnabledFlag (e.g., set the flag to a non-default value and call s.opts.HistoryColumns == expected, or check through a method if the field is unexported) would lock down the wiring. Not a blocker, but it is the only new wiring in this PR.

---

kv/sharded_coordinator.go — Phase-2 TODO comment (+7)

The comment accurately describes what is deferred and why — the design (§5.1, §10 of docs/admin_ui_key_visualizer_design.md) calls for read attribution from the node that serves the read; that wiring is intentionally out of scope for this writes-first slice. The referenced sections match the actual design doc content. The comment will prevent future readers from filing a "reads are always zero, is this a bug?" question. Good documentation.

One minor style note: the TODO token is TODO(keyviz Phase 2) which is non-standard for Go (TODO(username) or TODO(issue#) being the common idioms). This is a project-level convention question and not a linting failure — just worth knowing if you plan to grep for open TODOs later.

---

Overall

Approve. The flag wiring and nil-guard are correct, the TODO documents a real design gap without over-promising, and the tests cited in the PR description pass. The two "minor" items above are non-blocking but worth a follow-up commit or issue if you want the helper to stay tidy.

[gemini-code-assist]

Code Review

This pull request introduces the keyvizHistoryColumns configuration flag to manage the KeyViz ring buffer size and optimizes the flusher by avoiding goroutine creation when the sampler is disabled. It also adds documentation regarding future read sampling support. A review comment suggests implementing an upper bound for the history columns flag to mitigate potential out-of-memory risks from large pre-allocations.

[gemini-code-assist]

While this flag allows operators to adjust the ring buffer size, setting an excessively large value could lead to significant memory allocation and potential OOM issues at startup, as the buffer is pre-allocated. Consider adding a reasonable upper bound (e.g., 100,000 columns) in buildKeyVizSampler or NewMemSampler to prevent accidental misconfiguration from crashing the node.

References

1. To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests or configuration changes.

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 1m 31s —— View job

---

Code Review — PR #651

Todo list:

• Gather context and read PR description
• Read changed files (keyviz/sampler.go, keyviz/sampler_test.go, kv/sharded_coordinator.go, main.go)
• Check if OOM concern (gemini review) was addressed by MaxHistoryColumns clamp commit
• Verify test isolation gap (withFlags helper coverage)
• Post final review

---

Overall verdict: Approve ✓

All three stated goals are correctly implemented. The gemini-raised OOM concern is fully addressed by the second commit. One pre-existing test isolation gap remains.

---

keyviz/sampler.go — MaxHistoryColumns upper bound

New constant + clamp (sampler.go:85-92, sampler.go:315-317):

const MaxHistoryColumns = 100_000

if opts.HistoryColumns > MaxHistoryColumns {
    opts.HistoryColumns = MaxHistoryColumns
}

This directly addresses the gemini review concern. The clamp sits between the <= 0 default and the first allocation call, so both under- and over-range inputs are handled in the same place. The constant's doc comment explains the design rationale (Phase 3 persistence as the intended path for long retention). Clean.

TestHistoryColumnsClampedAboveMax (sampler_test.go:878-895) — covers 10× MaxHistoryColumns (clamped) and MaxHistoryColumns exactly (preserved). Both branches exercised, behaviour locked down.

---

main.go — --keyvizHistoryColumns flag

Flag registration (main.go:138):

keyvizHistoryColumns = flag.Int("keyvizHistoryColumns", keyviz.DefaultHistoryColumns, "Maximum matrix columns retained in the keyviz ring buffer (each column = one Step)")

Default matches keyviz.DefaultHistoryColumns (1440). Description is accurate. Flag name follows the existing keyviz* naming convention.

Wiring into buildKeyVizSampler (main.go:1380): HistoryColumns: *keyvizHistoryColumns is the only new line here. The MaxHistoryColumns guard in NewMemSampler means the coordinator path doesn't need a defensive clamp.

startKeyVizFlusher nil guard (main.go:1420-1422): Early return on s == nil is correct. The removed goroutine was allocating a stack and a goroutine for the sole purpose of waiting on ctx.Done — harmless, but worthless. The updated comment accurately describes why it was removed.

---

kv/sharded_coordinator.go — Phase-2 TODO

The comment is accurate and self-contained — it names the design document, the specific sections (§5.1, §10), and explains the zero Reads/ReadBytes series without leaving it ambiguous whether that is a bug or an intent. One minor style note (carry-over from the previous round):

TODO(keyviz Phase 2) is non-standard Go TODO style — the common idiom is TODO(username) or TODO(issue#). This won't cause a linting failure, but if you grep for open TODOs later a standardised tag is easier to filter. Low priority, not a blocker.

---

Remaining gap: withFlags helper doesn't cover keyvizHistoryColumns

main_keyviz_test.go:94–117 saves and restores keyvizEnabled, keyvizStep, keyvizMaxTrackedRoutes, and keyvizMaxMemberRoutesPerSlot — but not the new keyvizHistoryColumns. No current test in main_keyviz_test.go sets *keyvizHistoryColumns, so no existing test is broken by this. The risk is that a future test modifying the flag through withFlags will leak state across parallel tests. Fixing it now while the context is fresh is preferable to debugging a subtle test flake later: Fix this →

The end-to-end forwarding path (*keyvizHistoryColumns → buildKeyVizSampler → MemSamplerOptions.HistoryColumns) is also not directly tested via main_keyviz_test.go — a one-liner assertion inside TestBuildKeyVizSamplerHonorsEnabledFlag (set the flag to a non-default value, verify s.opts.HistoryColumns matches) would lock down this wiring. Not blocking, but consistent with the project convention that new flag wiring has a forwarding test.

---

| Branch: feat/keyviz-main-wiring-followup

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 2m 4s —— View job

---

Code Review — PR #651

Todo list:

• Gather context and read PR description
• Read all changed files
• Verify withFlags helper coverage for keyvizHistoryColumns
• Check MaxHistoryColumns clamp implementation and tests
• Check forwarding path test coverage
• Post final review

---

Overall verdict: Approve ✓

All three issues raised in the previous two review rounds have been closed out. The PR is clean.

---

Round-2 gaps — now closed

withFlags helper coverage (main_keyviz_test.go:98–124) — fixed. The helper now accepts historyColumns int as a 5th parameter, saves prevHistoryColumns := *keyvizHistoryColumns, sets the flag, and restores it in the defer. Both call sites pass an explicit value (16 in the enabled branch, default in others). No state will leak across parallel tests.

Forwarding path assertion (main_keyviz_test.go:30) — fixed. The test now calls:

require.Equal(t, 16, s.HistoryColumns())

This pins the wiring *keyvizHistoryColumns → buildKeyVizSampler → MemSamplerOptions.HistoryColumns end-to-end. The HistoryColumns() accessor (sampler.go:781) is nil-safe and exposes the post-default, post-clamp value, so the assertion correctly reflects what the ring buffer actually uses.

---

keyviz/sampler.go — MaxHistoryColumns clamp (commit 5bddeba)

const MaxHistoryColumns = 100_000

if opts.HistoryColumns > MaxHistoryColumns {
    opts.HistoryColumns = MaxHistoryColumns
}

Correct placement: sits after the <= 0 defaulting block and before the first allocation. The doc comment explains the 70-day upper bound and why longer retention belongs to the Phase 3 persistence path. The HistoryColumns() accessor mirrors the nil-safe pattern of the existing Step() accessor.

TestHistoryColumnsClampedAboveMax (sampler_test.go:878–895) exercises both branches: 10× MaxHistoryColumns (clamped to cap) and MaxHistoryColumns exactly (preserved). Both critical boundary conditions covered.

---

kv/sharded_coordinator.go — Phase-2 TODO

The comment accurately describes what is deferred, cites the design sections (§5.1, §10), and makes explicit that zero Reads/ReadBytes is intentional, not a regression. One carry-over style note:

TODO(keyviz Phase 2) is non-standard Go idiom (conventional form is TODO(username) or TODO(issue#)). Not a linter failure, but worth standardising if you ever grep open TODOs by tag. Low priority.

---

main.go — nil guard and flag wiring

startKeyVizFlusher early-return on s == nil is correct and the comment accurately explains why the prior goroutine was dropped. Flag default matches keyviz.DefaultHistoryColumns (1440) and the description text is accurate.

---

All items from the previous two rounds are resolved. No blocking issues remain.
| Branch: feat/keyviz-main-wiring-followup