fix(api): resolve data race in Controller.Debug and enable race detector#2933
fix(api): resolve data race in Controller.Debug and enable race detector#2933anyasabo wants to merge 2 commits intotphakala:mainfrom
Conversation
The test and test-coverage tasks run `go test` without CGO_ENABLED or CGO_CFLAGS, so the go-tflite dependency fails to compile when those vars are not already exported in the shell. The lint task already handles this via the CGO_FLAGS variable, but the test tasks were missing it. Add an env block to both tasks so `task test` works out of the box after `task setup-dev`. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables the Go race detector and CGO in the Taskfile for testing and addresses a potential race condition in the API controller's Debug method by removing an unsafe fallback to c.Settings. A review comment suggests enabling the race detector in the test-coverage task as well to maintain consistency across test commands.
| cmds: | ||
| - mkdir -p coverage | ||
| # Use noembed,skipfrontend tags to skip model and frontend embedding for faster tests | ||
| - go test -tags noembed,skipfrontend ./... -coverprofile=coverage/coverage.out {{.TEST_FLAGS}} |
There was a problem hiding this comment.
The -race flag is missing from the test-coverage task. To ensure consistency with the pull request's objective of catching data races automatically in CI and local testing, the race detector should be enabled here as well, especially since CGO_ENABLED: "1" has already been added to the environment to support it.
- go test -tags noembed,skipfrontend -race ./... -coverprofile=coverage/coverage.out {{.TEST_FLAGS}}Fix a data race between Controller.UpdateSectionSettings (which writes c.Settings) and a goroutine spawned by handleSettingsChanges that calls Controller.Debug (which reads c.Settings as a fallback when the global settings snapshot is nil). The fix removes the c.Settings fallback from Debug(). When conf.GetSettings() returns nil (standalone test Controllers that don't set the global), Debug now returns silently instead of racing on the unprotected field. This is safe because Debug is purely for logging. Also enables the race detector across test invocations: - Add -race to the CI unit test run in golangci-test.yml (with timeout bumped from 120s/5min to 300s/10min for race overhead) - Add -race and CGO env vars to `task test` in Taskfile.yml so local runs also catch races - Add CGO env vars to `task test-coverage` (no -race since coverage and race detection don't combine well) Co-authored-by: Cursor <cursoragent@cursor.com>
anyasabo
force-pushed
the
fix/data-race-and-enable-race-detector
branch
from
f6df242 to
66b2907
Compare
1 participant
Summary
Based on #2931 (CGO env vars for test tasks).
Controller.UpdateSectionSettings(writesc.Settings) and a goroutine spawned byhandleSettingsChangesthat callsController.Debug(readsc.Settingsas fallback). TheDebugmethod's fallback toc.Settingswhenconf.GetSettings()returns nil is removed -- it now returns silently, which is safe since it's purely for logging.-race) in CI unit tests and intask testso future races are caught automatically.Test plan
go test -race -short ./...passes locally with zero race warningsTestDashboardLayoutWidthPersistencespecifically verified with-race(was the only failing test)