Fix error counter keying on dynamic error messages#182
Fix error counter keying on dynamic error messages#182andrewwormald wants to merge 3 commits intomainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe ErrorCounter interface methods (Add, Count, Clear) were refactored to replace variadic labels with an explicit first label parameter and optional extras. The implementation now uses a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
| func makeKey(label string, extras []string) string { | ||
| if len(extras) == 0 { | ||
| return label | ||
| } | ||
| return label + "-" + strings.Join(extras, "-") |
There was a problem hiding this comment.
Use an unambiguous key encoding here.
Line 51 can collide across different label tuples, for example ("process-a", "run-1") and ("process", "a", "run-1") both become process-a-run-1. That will merge unrelated counters and let Clear remove the wrong entry, which undermines the safety net this PR is fixing. Please encode segment boundaries explicitly instead of "-"-joining labels, and add a regression test for one of these collision cases.
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 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.
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
Refactor
Tests