Fix error counter keying on dynamic error messages#182
Fix error counter keying on dynamic error messages#182andrewwormald wants to merge 4 commits intomainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe ErrorCounter public API was changed: Add, Count and Clear now take a required Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 docstrings
🧪 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.
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
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