Skip to content

fix: replace t.Logf with slog in newMockCheck to fix data race - #464

Open
lvlcn-t wants to merge 1 commit into
mainfrom
fix/race-condition-mock-check
Open

fix: replace t.Logf with slog in newMockCheck to fix data race#464
lvlcn-t wants to merge 1 commit into
mainfrom
fix/race-condition-mock-check

Conversation

@lvlcn-t

@lvlcn-t lvlcn-t commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Motivation

The mock check's RunFunc and ShutdownFunc captured t *testing.T and called t.Logf from within a goroutine spawned by RegisterCheck. Since the parent subtest could finish before that goroutine executed, the Go race detector flagged a concurrent read/write on t's internal state.

Changes

  • Removed the t *testing.T parameter from newMockCheck
  • Removed t.Helper() call (no longer valid without t)
  • Replaced t.Logf("Run called for check %s", name) with slog.InfoContext(ctx, "Run called", "check", name)
  • Replaced t.Logf("Shutdown called for check %s", name) with slog.Info("Shutdown called", "check", name)
  • Updated all 5 call sites to drop the t argument

For additional information look at the commits.

Tests done

  • Unit tests succeeded (go test -race ./pkg/sparrow/... passes cleanly)
  • E2E tests succeeded

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

@lvlcn-t lvlcn-t self-assigned this Jul 16, 2026
@lvlcn-t lvlcn-t added the bug Something isn't working label Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant