Skip to content

ci: run the race detector for the modules that are already race-clean - #1461

Open
dwin-gharibi wants to merge 1 commit into
TencentCloud:masterfrom
dwin-gharibi:ci-enable-race-detector
Open

ci: run the race detector for the modules that are already race-clean#1461
dwin-gharibi wants to merge 1 commit into
TencentCloud:masterfrom
dwin-gharibi:ci-enable-race-detector

Conversation

@dwin-gharibi

Copy link
Copy Markdown
Contributor

Closes #1460.

Motivation

Nothing in the repo passes -race to go test, so concurrency bugs ship undetected. Enabling it surfaces
12 races and 12 failing tests today — five of the races in production code, including a torn interface{}
read in CubeMaster/pkg/base/localcache.

What this changes

Adds -race to the four modules that are already clean, so this can land immediately without waiting on
any fix:

target file
cubelog-test Makefile:356
cubedb-test Makefile:360
cube-lifecycle-manager-test Makefile:364
Go SDK tests/unittest/run_sdk_test.sh:72

Nothing else changes — same packages, same flags otherwise.

Deliberately staged

cubeops-test, cubemaster-test and cubelet-pkg-test are not switched on here, because they are red
today and this PR would fail CI on arrival. They should be enabled as a follow-up once the corresponding
fixes land:

target blocked on
cubeops-test the cachingFakeFetchOnly map-read race in internal/handler/store_test.go
cubemaster-test the five localcache production races, plus the queueworker / recov / bufferqueue test races
cubelet-pkg-test the queueworker / recov test races

Each of those has its own branch. Landing this PR first means the gate exists and every new module or
package added to these four targets is race-checked from day one.

Testing

All four gated modules verified clean under the detector:

cubelog                  0  <- races + failures
CubeDB                   0
cube-lifecycle-manager   0
sdk/go                   0

(go test -race -count=1 ./... per module, counting WARNING: DATA RACE and FAIL lines.)

Cost

-race typically costs 2-5x runtime and ~5-10x memory. These four are the small, fast modules — the ones
whose runtime is dominated by process startup rather than the tests themselves — so the CI impact is
minimal. The expensive modules (CubeMaster, Cubelet) are the ones deliberately left for the follow-up, which
is a good moment to measure the real cost before committing to it.

cubelog-test keeps -short, so the long-running cases stay skipped.

Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
Copilot AI lite review requested due to automatic review settings August 21, 2026 11:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread Makefile
.PHONY: cubedb-test
cubedb-test:
cd CubeDB && go mod download && go test ./...
cd CubeDB && go mod download && go test -race ./...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Low — -race adds a C-toolchain requirement at test time, even though CubeDB is pure Go.

The race detector needs cgo enabled plus a working C compiler (gcc/clang) when the test binary links — being "pure Go with no CGO" (the Makefile comment at lines 400-402) doesn't help here. On any host where CGO_ENABLED=0 is exported — a pattern this repo itself uses for its production builds (Makefile:313, Makefile:330, Makefile:374, CubeDB/…-adjacent Dockerfiles) — this target will now hard-fail with go: -race requires cgo; enable cgo during builds.

CI is fine: unit-test-check.yml runs this on GitHub-hosted runners that ship gcc, and cube-lifecycle-manager-test runs inside the builder image which installs build-essential/clang (docker/Dockerfile.builder:81-137). But local developers following the repo's CGO_ENABLED=0 convention will break. Consider documenting the new requirement, or failing with a clearer message when go env CGO_ENABLED is 0.

@cubesandboxbot

Copy link
Copy Markdown

AI-generated review summary

Reviewed PR #1461 (ci: run the race detector for the modules that are already race-clean) against the base branch. The diff is small and CI-only: it adds -race to four already-clean test targets and updates one comment. I verified the changed lines against the base tree and the CI workflows that consume them.

Verdict: no blocking issues. The change is consistent, well-scoped, and the deliberate staging of the currently-red targets (cubeops-test, cubemaster-test, cubelet-pkg-test) for a follow-up is reasonable.

What the change does (verified against base tree)

Target Command Where it runs
cubelog-test (Makefile:356) go test -short -race ./... host runner (Makefile:400-402: "pure Go with no CGO")
cubedb-test (Makefile:360) go test -race ./... host runner
cube-lifecycle-manager-test (Makefile:364) go test -race ./... builder container via builder-run
Go SDK (tests/unittest/run_sdk_test.sh:72) go test -race ./... host runner (sdk-test-check.yml, amd64 + arm64)
  • All four run on linux/amd64 or linux/arm64, both supported by Go's race detector.
  • None of the consuming workflows (unit-test-check.yml, sdk-test-check.yml) set CGO_ENABLED=0, so CI will link the race runtime.
  • The cube-lifecycle-manager-test builder image installs build-essential/clang (docker/Dockerfile.builder:81-137), so the detector links inside the container.
  • The comment update in tests/unittest/run_sdk_test.sh:63 is accurate: sdk/go/integration_test.go:4 carries //go:build integration, so go test -race ./... still skips it.
  • The PR's claim that nothing in the repo previously passed -race to go test checks out (the other -race matches in the tree are test names, not flags).

Findings

  1. Low — -race adds a C-toolchain requirement at test time for all four targets (inline comment on Makefile:360). The race detector requires cgo + a C compiler when the test binary links; "pure Go with no CGO" only applies to building, not to race-instrumented testing. Any environment with CGO_ENABLED=0 (which this repo itself uses for production builds at Makefile:313, Makefile:330, Makefile:374) will now hard-fail these targets with go: -race requires cgo. CI is unaffected. Suggest documenting the requirement, or failing with a clearer diagnostic when go env CGO_ENABLED is 0.

  2. Low / informational — cubedb CI will race-check the dockertest DB suite. CubeDB/migrate/dockertest_fixture_test.go:38-45 makes the DB-container tests required when CI=true, so under -race those tests run with the detector enabled. The 2-5× time / 5-10× memory overhead applies to the test binary while it also drives MySQL/Postgres containers. Probably fine on standard runners, but worth watching for timeout or OOM flakes on the smaller arm64 runner — it's the one target here where -race cost is not negligible.

  3. Informational — slightly stale comment. Makefile:400-402 says cubelog/cubedb run on the host because they are "pure Go with no CGO and no builder-only deps". That remains true for building, but it now reads as if no C toolchain is involved at all. A one-line clarification ("...and the host has a C toolchain for -race") would keep the rationale accurate.

Conclusion

Approve. This is a clean, well-motivated CI change that establishes a race-detector gate for four modules immediately without blocking on the red ones. No correctness issues in the diff itself; the only actionable item is documenting the new cgo requirement for local/CGO_ENABLED=0 environments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug Report] The race detector is not enabled in any test target, so concurrency bugs ship undetected

2 participants