[CI] Enable race detection for unit tests in CI#442
Merged
cendhu merged 9 commits intohyperledger:mainfrom Mar 24, 2026
Merged
Conversation
dean-amar
changed the title
This was referenced Mar 17, 2026
Contributor
|
There would be a significant number of failing test due to race. As we are using t.Parallel(), many race occurs across packages. I decided not to pollute the production code to accomodate test run methods. |
cendhu
pushed a commit
that referenced
this pull request
Mar 18, 2026
#### Type of change - Bug fix - Test update #### Description - The metrics unit test had apparent race condition when accessing the URL - This PR fix this by using atomic pointer - This is an intermediate solution until we address #441 #### Related issues - discovered by #442 Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
liran-funaro
force-pushed
the
race-detection
branch
from
d107470 to
8b37282
Compare
liran-funaro
force-pushed
the
race-detection
branch
from
8b37282 to
a0a68e9
Compare
This was referenced Mar 18, 2026
Closed
cendhu
pushed a commit
that referenced
this pull request
Mar 19, 2026
#### Type of change - Bug fix - Test update #### Description Root cause: The test was experiencing a race condition when accessing command output buffers. In `UnitTestRunner`, the command execution runs in a goroutine that continuously writes output to `bytes.Buffer` instances (cmdStdOut and cmdStdErr), while simultaneously another goroutine in `assert.Eventually` reads from these buffers via `cmdStdOut.String()` to check for expected output. Since `bytes.Buffer` is not thread-safe for concurrent read/write operations, this caused data races detected by the race detector when multiple goroutines accessed the buffer simultaneously. #### Related issues - discovered by #442 Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
liran-funaro
added a commit
that referenced
this pull request
Mar 19, 2026
#### Type of change - Bug fix #### Description **Root cause:** The `Ready.Reset()` method was experiencing a race condition due to non-atomic struct field replacement. The original implementation used `*r = *NewReady()` to reset the struct, which performs a multi-step memory copy operation that replaces all fields (channels `ready`, `closed`, and `once`). This struct copy is not atomic. Concurrently, other goroutines calling `WaitForReady()` or `WaitForAllReady()` would read from the same channel fields (`r.ready` and `r.closed`) via select statements. This created a classic data race where one goroutine was writing to struct fields while others were simultaneously reading from them. **Race scenario:** 1. Goroutine A calls `Reset()` and begins copying new struct fields (non-atomic multi-step operation) 2. Goroutine B calls `WaitForReady()` and attempts to read `r.ready` or `r.closed` channels 3. Data race occurs: Goroutine A writes to memory while Goroutine B reads from the same memory locations **The fix:** Uses atomic pointer indirection by moving channels into an inner `ready` struct and storing only an `atomic.Pointer[ready]` in the outer `Ready` struct. This enables atomic swapping of the entire internal state via `CompareAndSwap()`, eliminating the race condition while maintaining the same external API. #### Related issues - discovered by #442 Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Enable Go race detection for unit tests only (excluding integration/container tests) to catch data races while keeping CI times reasonable. Race detection is enabled via ENABLE_RACE environment variable in CI workflows but disabled by default for local development. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Add test_flags Makefile variable to pass flags to test commands. Enable race detection in CI for unit tests by passing test_flags="-race". Race detection is disabled by default locally but enabled in CI to catch data races while maintaining fast local development workflow. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
liran-funaro
force-pushed
the
race-detection
branch
from
a0a68e9 to
90fa78d
Compare
cendhu
pushed a commit
that referenced
this pull request
Mar 23, 2026
#### Type of change - Improvement (improvement to code, performance, etc) #### Description - Add support for test_flags in Makefile #### Related issues - helps #442 Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Contributor
|
@dean-amar Please rebase |
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
liran-funaro
force-pushed
the
race-detection
branch
from
96a5649 to
4b0123b
Compare
Contributor
|
@cendhu I've updated this PR and fixed all the currently known races. Please review. |
Contributor
|
@liran-funaro Great effort. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of change
Description
This PR enables Go's race detector for unit tests to catch data race conditions before code is merged.
test-no-dbandtest-all-dbRelated issues