Fix error counter keying on dynamic error messages#182
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ErrorCounter public API was changed: Add, Count and Clear now take a required label string and optional extras ...string instead of an error plus variadic labels. Internal Counter logic now builds keys via makeKey(label, extras) (no error message included). Add increments using the new key, Count returns the keyed count, and Clear deletes the key entry. Tests and call sites (pause logic and several internal tests) were updated to use the new label-based arguments. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous approach silently dropped the error message from the key but kept labels optional. This meant callers without labels would share a single global counter — a worse behaviour change. Now the interface requires at least one label (label string, extras ...string), making the contract explicit. All existing callers already pass two labels (processName, runID) so no call sites change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/errorcounter/errorcounter.go`:
- Around line 47-51: The makeKey function currently joins label and extras with
"-" which can produce ambiguous keys (e.g., ("process-a","run-1") vs
("process","a","run-1")); update makeKey to encode segment boundaries
unambiguously (for example by prefixing lengths or using a separator-and-escape
scheme or null-byte delimiter) so each segment is recoverable and collisions are
impossible, and ensure any code that relies on makeKey (e.g., Clear or lookup
paths) continues to use the new encoding; also add a regression test that
constructs two colliding tuples like ("process-a","run-1") and
("process","a","run-1") and verifies they map to distinct keys and that Clear
only removes the intended entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b794c086-32b3-40b3-92b1-cc3fc50473bf
📒 Files selected for processing (3)
errors.gointernal/errorcounter/errorcounter.gointernal/errorcounter/errorcounter_test.go
The error value was accepted but ignored for keying — drop it entirely to make the label-only keying explicit in the API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/errorcounter/errorcounter.go (1)
44-48:⚠️ Potential issue | 🟠 MajorUse unambiguous key encoding; current join can collide.
At Line 48,
label + "-" + strings.Join(extras, "-")can map different label tuples to the same key (for example,("a-b","c")vs("a","b-c")). That can merge counters and clear the wrong entry.Proposed fix
import ( + "strconv" "strings" "sync" ) @@ func makeKey(label string, extras []string) string { - if len(extras) == 0 { - return label - } - return label + "-" + strings.Join(extras, "-") + parts := make([]string, 0, len(extras)+1) + parts = append(parts, label) + parts = append(parts, extras...) + + var b strings.Builder + for _, part := range parts { + b.WriteString(strconv.Itoa(len(part))) + b.WriteByte(':') + b.WriteString(part) + b.WriteByte('|') + } + return b.String() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/errorcounter/errorcounter.go` around lines 44 - 48, The makeKey function currently concatenates label and extras with "-" which allows collisions (e.g. ("a-b","c") vs ("a","b-c")); change makeKey to produce an unambiguous encoding by encoding the tuple instead of naive joining — for example, serialize a small struct {Label string; Extras []string} (or otherwise encode lengths/escape separators) using a stable encoder (e.g., json.Marshal) and return that string; update makeKey to use that encoder so keys are unique and reversible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/errorcounter/errorcounter.go`:
- Around line 44-48: The makeKey function currently concatenates label and
extras with "-" which allows collisions (e.g. ("a-b","c") vs ("a","b-c"));
change makeKey to produce an unambiguous encoding by encoding the tuple instead
of naive joining — for example, serialize a small struct {Label string; Extras
[]string} (or otherwise encode lengths/escape separators) using a stable encoder
(e.g., json.Marshal) and return that string; update makeKey to use that encoder
so keys are unique and reversible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a9a1332-4e22-428e-80b7-da5047503d50
📒 Files selected for processing (7)
errors.gointernal/errorcounter/errorcounter.gointernal/errorcounter/errorcounter_test.gopause.gopause_internal_test.gostep_internal_test.gotimeout_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- errors.go
Labels containing hyphens (e.g. "a-b","c" vs "a","b-c") could map to the same key. Use \x00 as the separator since it cannot appear in human-readable label strings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
| } | ||
|
|
||
| func (c *Counter) Add(err error, labels ...string) int { | ||
| func (c *Counter) Add(label string, extras ...string) int { |
There was a problem hiding this comment.
This is technically a breaking change, but because we're pre v1.0.0, it's fine to release in a minor 👍 just worth noting in the release notes
2 participants
Problem
Error counter uses
err.Error()as part of the map key. Errors with dynamic content (timestamps, IDs) create unique keys, soPauseAfterErrCountthreshold is never reached. Also, zeroed keys are never pruned, leaking memory.Why
Records that should be paused after N errors retry forever — the safety net silently doesn't work.
Fix
Key only on labels (processName + runID) which are stable. Use
delete()instead of zeroing onClear().🤖 Generated with Claude Code
Summary by CodeRabbit