Skip to content

Fix error counter keying on dynamic error messages #182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andrewwormald wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix error counter keying on dynamic error messages #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
846e7fd
Fix error counter keying on dynamic error messages
andrewwormald Mar 10, 2026
08af657
Add tests for label-based error counter keying
andrewwormald Mar 10, 2026
33333bc
Require at least one label in ErrorCounter API
andrewwormald Mar 10, 2026
01c6cd0
Remove unused err parameter from ErrorCounter interface
andrewwormald Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ var (
ErrInvalidTransition = errors.New("invalid transition")
)

// ErrorCounter defines an interface for counting occurrences of errors with optional labels.
// ErrorCounter defines an interface for counting errors keyed by stable labels.
// At least one label is required — labels should identify the process and run (e.g. processName, runID).
type ErrorCounter interface {
Add(err error, labels ...string) int
Count(err error, labels ...string) int
Clear(err error, labels ...string)
Add(label string, extras ...string) int
Count(label string, extras ...string) int
Clear(label string, extras ...string)
}
31 changes: 17 additions & 14 deletions internal/errorcounter/errorcounter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,34 @@ type Counter struct {
store map[string]int
}

func (c *Counter) Add(err error, labels ...string) int {
func (c *Counter) Add(label string, extras ...string) int {
c.mu.Lock()
defer c.mu.Unlock()

errMsg := err.Error()
errMsg += strings.Join(labels, "-")
c.store[errMsg] += 1
return c.store[errMsg]
key := makeKey(label, extras)
c.store[key] += 1
return c.store[key]
}

func (c *Counter) Count(err error, labels ...string) int {
func (c *Counter) Count(label string, extras ...string) int {
c.mu.Lock()
defer c.mu.Unlock()

errMsg := err.Error()
errMsg += strings.Join(labels, "-")
return c.store[errMsg]
key := makeKey(label, extras)
return c.store[key]
}

func (c *Counter) Clear(err error, labels ...string) {
func (c *Counter) Clear(label string, extras ...string) {
c.mu.Lock()
defer c.mu.Unlock()

errMsg := err.Error()
errMsg += strings.Join(labels, "-")
c.store[errMsg] = 0
return
key := makeKey(label, extras)
delete(c.store, key)
}

func makeKey(label string, extras []string) string {
if len(extras) == 0 {
return label
}
return label + "-" + strings.Join(extras, "-")
Comment on lines +44 to +48
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}
105 changes: 56 additions & 49 deletions internal/errorcounter/errorcounter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package errorcounter_test

import (
"errors"
"testing"

"github.com/stretchr/testify/require"
Expand All @@ -10,52 +9,60 @@ import (
)

func TestErrorCounter(t *testing.T) {
testCases := []struct {
name string
inputErr error
labels []string
iterationCount int
expectedCount int
}{
{
name: "Add 3 and get 3",
inputErr: errors.New("test error"),
labels: []string{"label 1", "label 2"},
iterationCount: 3,
expectedCount: 3,
},
{
name: "Add 1 and get 1 - no labels",
inputErr: errors.New("test error"),
labels: []string{},
iterationCount: 3,
expectedCount: 3,
},
{
name: "Add 0 and get 0",
inputErr: errors.New("test error"),
labels: []string{"label 1"},
iterationCount: 0,
expectedCount: 0,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
c := errorcounter.New()

var currentCount int
for i := 0; i < tc.iterationCount; i++ {
currentCount = c.Add(tc.inputErr, tc.labels...)
}
require.Equal(t, tc.expectedCount, currentCount)

count := c.Count(tc.inputErr, tc.labels...)
require.Equal(t, tc.expectedCount, count)

c.Clear(tc.inputErr, tc.labels...)
count = c.Count(tc.inputErr, tc.labels...)
require.Equal(t, 0, count)
})
}
t.Run("Add 3 and get 3", func(t *testing.T) {
c := errorcounter.New()

c.Add("label 1", "label 2")
c.Add("label 1", "label 2")
count := c.Add("label 1", "label 2")
require.Equal(t, 3, count)

require.Equal(t, 3, c.Count("label 1", "label 2"))

c.Clear("label 1", "label 2")
require.Equal(t, 0, c.Count("label 1", "label 2"))
})

t.Run("Single label", func(t *testing.T) {
c := errorcounter.New()

c.Add("only-label")
count := c.Add("only-label")
require.Equal(t, 2, count)

require.Equal(t, 2, c.Count("only-label"))

c.Clear("only-label")
require.Equal(t, 0, c.Count("only-label"))
})

t.Run("Add 0 and get 0", func(t *testing.T) {
c := errorcounter.New()
require.Equal(t, 0, c.Count("label 1"))
})
}

func TestErrorCounter_ClearRemovesKey(t *testing.T) {
c := errorcounter.New()

c.Add("process", "run-1")
c.Add("process", "run-1")
require.Equal(t, 2, c.Count("process", "run-1"))

c.Clear("process", "run-1")

// After clear, count should be 0 and next Add should return 1.
require.Equal(t, 0, c.Count("process", "run-1"))
require.Equal(t, 1, c.Add("process", "run-1"))
}

func TestErrorCounter_DifferentLabelsSeparateCounters(t *testing.T) {
c := errorcounter.New()

c.Add("process-a", "run-1")
c.Add("process-a", "run-1")
c.Add("process-b", "run-2")

require.Equal(t, 2, c.Count("process-a", "run-1"))
require.Equal(t, 1, c.Count("process-b", "run-2"))
}
4 changes: 2 additions & 2 deletions pause.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func maybePause[Type any, Status StatusType](
return false, nil
}

count := counter.Add(originalErr, processName, run.RunID)
count := counter.Add(processName, run.RunID)
if count < pauseAfterErrCount {
return false, nil
}
Expand All @@ -41,7 +41,7 @@ func maybePause[Type any, Status StatusType](
})

// Run paused - now clear the error counter.
counter.Clear(originalErr, processName, run.RunID)
counter.Clear(processName, run.RunID)
return true, nil
}

Expand Down
4 changes: 2 additions & 2 deletions pause_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ func Test_maybeAutoPause(t *testing.T) {
RunID: "run-id",
}, "test", WithPauseFn(tc.pauseFn))

counter.Clear(testErr, processName, r.RunID)
counter.Clear(processName, r.RunID)
for range tc.errCount {
counter.Add(testErr, processName, r.RunID)
counter.Add(processName, r.RunID)
}

paused, err := maybePause(
Expand Down
2 changes: 1 addition & 1 deletion step_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ func Test_stepConsumer(t *testing.T) {
})

t.Run("Pause record after exceeding allowed error count", func(t *testing.T) {
counter.Clear(testErr, processName, current.RunID)
counter.Clear(processName, current.RunID)

calls := map[string]int{
"consumerFunc": 0,
Expand Down
4 changes: 2 additions & 2 deletions timeout_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,9 @@ func TestProcessTimeout(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
counter.Clear(testErr, processName, tc.record.RunID)
counter.Clear(processName, tc.record.RunID)
for range tc.currentErrCount {
counter.Add(testErr, processName, tc.record.RunID)
counter.Add(processName, tc.record.RunID)
}

calls := map[string]int{}
Expand Down
Loading