Add a ThreadSanitizer build configuration and CI job - #121
Merged
Conversation
The host test suite has only ever run under ASan/UBSan. Those sanitizers cannot be combined with TSan, so a data race in the threaded code paths (the sync task, the connection threads, the inbox handoffs) has no way to show up in CI at all. The library's whole contract with its consumer is about which thread may touch what, and nothing was checking it. `ENABLE_TSAN` is a separate option because of that incompatibility, and configuring both at once now fails rather than producing a build where one sanitizer silently wins. The flags are applied globally, to `CMAKE_C_FLAGS`/`CMAKE_CXX_FLAGS` and the linker flags before the FetchContent dependencies are declared, rather than to the sendspin and sendspin_tests targets alone. TSan intercepts pthread calls in uninstrumented code but cannot see its atomics, and IXWebSocket signals its worker threads with `std::atomic` stop flags; a per-target build reports those as races. Instrumenting everything is what TSan asks for and is the configuration measured clean. The CI job mirrors the existing test job. It drops `vm.mmap_rnd_bits` to 28 first: recent kernels default 32-bit mmap randomization high enough that TSan aborts at startup on GitHub runners with "unexpected memory mapping". The suite is clean today, 123/123 with no TSan report, at roughly twice the ASan wall time.
The test-standards skill told reviewers that CI runs under ASan/UBSan only and that a missing ThreadSanitizer run is not a finding. That rule existed to excuse reviewers from a configuration the project did not have. With the TSan job in CI it inverts: a concurrency finding, or a certification that a threaded test is adequate, is backed by a TSan run, and a mutant that changes thread interaction is built under TSan because ASan cannot see it. CONTRIBUTING.md and tests/README.md gain the matching commands so the "matching CI" claim next to the ASan block stays true.
Every artwork test declared its RecordingListener after the Impl, so the listener was destroyed first while ~Impl was still stopping the drain thread. The drain thread could then call notify_all() on a condition variable the main thread was destroying. On the Linux TSan runtime this reported as a data race in ~RecordingListener across 13 of the 19 tests; the macOS runtime never caught the window. Declaring the listener first makes it outlive the Impl, whose destructor joins the drain thread, so no callback can reach a destroyed listener.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A couple of small but concrete issues remain (CMake flag appending is non-idempotent on reconfigure, and the new TSan docs command should match CI’s BUILD_EXAMPLES=OFF usage).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a ThreadSanitizer (TSan) build configuration and CI job to catch data races in the host test suite, and adjusts artwork role tests to avoid a real TSan-reported teardown race in test scaffolding.
Changes:
- Introduces
ENABLE_TSAN(mutually exclusive withENABLE_SANITIZERS) and applies TSan flags globally so FetchContent dependencies are instrumented. - Adds a
test-tsanGitHub Actions job (withTSAN_OPTIONS=halt_on_error=1and reducedvm.mmap_rnd_bits) and wires it into the CI “gate” job. - Fixes
tests/test_artwork_role.cppfixture lifetime soRecordingListeneroutlives the artwork drain thread during teardown.
File summaries
| File | Description |
|---|---|
CMakeLists.txt |
Adds ENABLE_TSAN option and global TSan flag wiring. |
.github/workflows/ci.yml |
Adds test-tsan job and includes it in the CI gate. |
tests/test_artwork_role.cpp |
Reorders fixture declarations to eliminate a teardown race under TSan. |
tests/README.md |
Documents how to run tests under TSan. |
CONTRIBUTING.md |
Updates contributor instructions to include the TSan workflow. |
CLAUDE.md |
Updates project build/test guidance to mention TSan configuration. |
.claude/skills/test-standards/SKILL.md |
Updates test review guidance now that CI includes a TSan job. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
The host suite has only ever run under ASan/UBSan, which cannot be combined with ThreadSanitizer, so no data race in the sync task, connection threads, or inbox handoffs could show up in CI. This adds an
ENABLE_TSANoption and atest-tsanjob.ENABLE_TSANCMake option, default OFF; configuring it together withENABLE_SANITIZERSis aFATAL_ERROR.CMAKE_C_FLAGS,CMAKE_CXX_FLAGS, linker flags) before the FetchContent dependencies are declared, not per target. TSan intercepts pthread calls in uninstrumented code but cannot see its atomics, and IXWebSocket signals its worker threads withstd::atomicstop flags; a per-target build reports those as false races. This is the one deliberate departure from the shape on theencryption-supportbranch (bf0da76).test-tsanjob mirrorstestand runs withTSAN_OPTIONS=halt_on_error=1. It setsvm.mmap_rnd_bits=28first: recent runner kernels default 32-bit mmap randomization high enough that TSan aborts at startup with "unexpected memory mapping".No production code changes.
tests/CMakeLists.txtis untouched.Test changes
The first CI run of
test-tsanfailed 13 of 19 artwork tests with the same report: a data race in~RecordingListener(tests/test_artwork_role.cpp),pthread_cond_destroyon the main thread againstpthread_cond_broadcastfrom the still-running artwork drain thread.Every artwork test declared its
RecordingListenerafter theImpl, so the listener was destroyed first while~Implwas still stopping the drain thread; the drain thread could callnotify_all()on a condition variable being destroyed. Commit 85f6bc0 declares the listener before theImplin all 19 tests so it outlives the thread~Impljoins. This is a test-scaffolding defect, not a library one;ArtworkRole::Implitself already stops and joins in its destructor.