fix: protect all r.closed accesses, add race test, enable -race in CI

The initial `if r.closed` guard in RunDaemon() was still reading
r.closed without holding closedM, causing a data race with Close().
Wrap it with closedM to match the other access sites.

Add TestReporter_RunDaemonClose_Race which exercises RunDaemon() and
Close() concurrently — the exact scenario from #793. Without the fix,
`go test -race` reliably detects the data race.

Enable the -race flag in `make test` so CI catches data races.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
silverwind
2026-02-19 02:54:45 +01:00
parent 5591f0a546
commit a4a6e291d5
3 changed files with 48 additions and 1 deletions

View File

@@ -112,7 +112,7 @@ security-check: deps-tools
GOEXPERIMENT= $(GO) run $(GOVULNCHECK_PACKAGE) -show color ./...
test: fmt-check security-check
@$(GO) test -v -cover -coverprofile coverage.txt ./... && echo "\n==>\033[32m Ok\033[m\n" || exit 1
@$(GO) test -race -v -cover -coverprofile coverage.txt ./... && echo "\n==>\033[32m Ok\033[m\n" || exit 1
.PHONY: vet
vet:

View File

@@ -178,9 +178,13 @@ func (r *Reporter) Fire(entry *log.Entry) error {
}
func (r *Reporter) RunDaemon() {
r.closedM.Lock()
if r.closed {
r.closedM.Unlock()
return
}
r.closedM.Unlock()
if r.ctx.Err() != nil {
return
}

View File

@@ -6,7 +6,9 @@ package report
import (
"context"
"strings"
"sync"
"testing"
"time"
runnerv1 "code.gitea.io/actions-proto-go/runner/v1"
connect_go "connectrpc.com/connect"
@@ -195,3 +197,44 @@ func TestReporter_Fire(t *testing.T) {
assert.Equal(t, int64(3), reporter.state.Steps[0].LogLength)
})
}
func TestReporter_RunDaemonClose_Race(t *testing.T) {
client := mocks.NewClient(t)
client.On("UpdateLog", mock.Anything, mock.Anything).Return(
func(_ context.Context, req *connect_go.Request[runnerv1.UpdateLogRequest]) (*connect_go.Response[runnerv1.UpdateLogResponse], error) {
return connect_go.NewResponse(&runnerv1.UpdateLogResponse{
AckIndex: req.Msg.Index + int64(len(req.Msg.Rows)),
}), nil
},
)
client.On("UpdateTask", mock.Anything, mock.Anything).Return(
func(_ context.Context, req *connect_go.Request[runnerv1.UpdateTaskRequest]) (*connect_go.Response[runnerv1.UpdateTaskResponse], error) {
return connect_go.NewResponse(&runnerv1.UpdateTaskResponse{}), nil
},
)
ctx, cancel := context.WithCancel(context.Background())
taskCtx, err := structpb.NewStruct(map[string]interface{}{})
require.NoError(t, err)
reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{
Context: taskCtx,
})
reporter.ResetSteps(1)
// Start the daemon loop in a separate goroutine.
// RunDaemon reads r.closed and reschedules itself via time.AfterFunc.
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
reporter.RunDaemon()
}()
// Close concurrently — this races with RunDaemon on r.closed.
assert.NoError(t, reporter.Close(""))
// Cancel context so pending AfterFunc callbacks exit quickly.
cancel()
wg.Wait()
time.Sleep(2 * time.Second)
}