Repository navigation
Back off on-demand Metadata refreshes while a leader stays excluded (release-0.1) - #97
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 19c806b. Configure here.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The behavior is comprehensively tested and documented; the remaining documentation-placement issue is minor.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds bounded exponential backoff for on-demand metadata refreshes while preserving non-blocking Produce behavior.
Changes:
- Adds overflow-safe refresh backoff and periodic reset logic.
- Covers leader drops, routing misses, failures, and shutdown behavior.
- Updates configuration and architecture documentation.
| File | Description |
|---|---|
README.md |
Documents refresh pacing behavior. |
pkg/wgo/config.go |
Clarifies interval configuration. |
pkg/wgo/client.go |
Implements metadata refresh backoff. |
pkg/wgo/client_test.go |
Tests timing, saturation, recovery, and shutdown. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| backoff.reset() | ||
| continue | ||
| } | ||
| ticker.Reset(c.cfg.MetadataRefreshInterval) |
There was a problem hiding this comment.
Question, not a blocker: the backoff resets only on a periodic tick, and this line pushes the tick back on every on-demand fetch. A workload that keeps producing routing misses, such as one that produces to an unknown topic, never gets a periodic tick. It stays at the 10s ceiling indefinitely. Before this change, those misses refreshed every 1s. Is that the trade-off that you want? If so, a short note in the README "How it works" section would help, because AGENTS.md asks for it when refresh behavior changes.
— via Claude Code
There was a problem hiding this comment.
Yes, this is intentional and follows the #95 discussion about sustained 1s all-topic Metadata refreshes creating excessive load. The first fetch can run immediately; subsequent gaps are 1s → 2s → 4s → 8s → 10s, then remain at 10s while nudges continue. Metadata still refreshes at the normal cadence.
The trade-off also applies to unknown topics: the backoff is shared across the client, so persistent misses for topic A can leave a newly encountered topic B waiting for the next 10s fetch, without its own fast retry sequence.
For now, I think that is acceptable. We can capture a future design for bounded resets when relevant metadata changes, I'll see if #87 has all the metrics to measure this and if not will add it there.
There was a problem hiding this comment.
The README is updated
stephclay
left a comment
There was a problem hiding this comment.
Thanks for the fixes! LGTM.
— via Claude Code
| close(release) | ||
| synctest.Wait() | ||
|
|
||
| assert.Equal(t, before, onDemandRefreshes(c)) |
There was a problem hiding this comment.
Nit, not blocking: this test catches the regression only about half the time. If the check is removed, select still picks refreshCtx.Done() over the queued nudge in about half of the runs, and the test passes. I removed the check locally and ran the test 40 times. It failed 20 of them. CI can therefore go green if someone drops the check later. A deterministic version needs more control over the select than the test has now, so I'm fine with keeping it as it is.
— via Claude Code
There was a problem hiding this comment.
I'll take a look to this in the next PR


Summary
OnDemandMetadataRefreshIntervaland double up toMetadataRefreshInterval. A periodic refresh puts the gap back. Callers oftriggerRefreshare unchanged.on-demand, so a cooldown that has reached the ceiling does not reset the delay. Doubling saturates at the ceiling so a large interval cannot wraptime.Duration.