Skip to content

fix: enable TSAN and resolve detected data races#431

Merged
lxy-9602 merged 10 commits into
alibaba:mainfrom
zjw1111:codex/fix-tsan
Jul 20, 2026
Merged

fix: enable TSAN and resolve detected data races#431
lxy-9602 merged 10 commits into
alibaba:mainfrom
zjw1111:codex/fix-tsan

Conversation

@zjw1111

@zjw1111 zjw1111 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Linked issue: N/A

Enable ThreadSanitizer coverage, fix the data races exposed by the full test suite, and streamline the compiler and sanitizer CI workflows.

This change:

  • separates the ASAN, UBSAN, and TSAN build switches while preserving the existing ASAN+UBSAN job;
  • instruments bundled dependencies with TSAN, including building Boost with the configured C++ compiler and external-project compile/link flags;
  • disables Tantivy in the TSAN job because its Rust static library is not TSAN-instrumented, and suppresses reports originating from prebuilt Lance, Lumina, and Jindo shared libraries;
  • makes compaction metric snapshots and test cache counters safe under concurrent access;
  • consolidates GCC 14 and Clang Release/Debug builds into a single matrix workflow while keeping GCC 8 independent;
  • uses concise sanitizer job names and named build-script arguments;
  • compares clang-tidy changes against the pull request target branch instead of always using origin/main;
  • normalizes external-project flag propagation and preserves the warning-flag ordering required by Snappy; and
  • prints captured third-party build logs when an ExternalProject step fails.

Tests

  • Clang 17 TSAN full build with Jindo, Lance, Lumina, and Lucene enabled; Tantivy disabled as configured by the TSAN CI job.
  • ctest --test-dir build-pr-final --output-on-failure -j 64 (27/27 passed).
  • Clang 17 Release build of the bundled snappy_ep target.
  • pre-commit run --files <all changed files>.
  • bash -n ci/scripts/build_paimon.sh.
  • git diff upstream/main...HEAD --check.

API and Format

No API, storage format, or protocol changes.

Documentation

No user-facing documentation changes.

Generative AI tooling

Generated-by: Codex (GPT-5)

Copilot AI review requested due to automatic review settings July 17, 2026 03:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR enables ThreadSanitizer (TSAN) coverage in CI and addresses the data races surfaced when running the full test suite under TSAN, while keeping the existing ASAN+UBSAN coverage intact.

Changes:

  • Added a dedicated TSAN sanitizer job in CI and updated CI build invocations to use named arguments.
  • Updated CMake sanitizer/toolchain configuration so bundled third-party dependencies can be built with TSAN instrumentation where applicable, and added TSAN suppressions for non-instrumented prebuilt shared libraries.
  • Fixed concurrency issues in metrics snapshotting and test cache counters, and adjusted an integration test IO counter to be atomic.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/inte/read_inte_test.cpp Makes the IO counter atomic to avoid TSAN-reported races in the integration test.
src/paimon/testing/utils/counting_cache_test_utils.h Protects cache counter tracking with a mutex for thread-safe access in tests.
src/paimon/core/operation/metrics/compaction_metrics.h Uses atomics for reporter fields to make concurrent metric updates/snapshots TSAN-safe.
src/paimon/core/operation/metrics/compaction_metrics_test.cpp Adds a regression test covering concurrent reporter updates and metric snapshotting.
cmake_modules/ThirdpartyToolchain.cmake Adjusts external-project flags to better support sanitizer builds and TSAN-instrumented dependencies (including Boost).
cmake_modules/san-config.cmake Adds TSAN configuration and prevents enabling ASAN and TSAN simultaneously.
cmake_modules/BuildUtils.cmake Skips -z defs when any sanitizer build is enabled (now including TSAN).
ci/scripts/build_paimon.sh Switches to named CLI arguments; adds separate ASAN/UBSAN/TSAN switches and TSAN-specific Tantivy disablement.
build_support/tsan-suppressions.txt Introduces TSAN suppressions for known non-instrumented prebuilt shared libraries.
.github/workflows/test_with_sanitizer.yaml Adds a matrix to run ASAN+UBSAN and TSAN jobs (with Rust setup only where needed).
.github/workflows/gcc8_test.yaml Updates build script invocation to new named-argument interface.
.github/workflows/gcc_test.yaml Updates build script invocation to new named-argument interface.
.github/workflows/clang_test.yaml Updates build script invocation to new named-argument interface (and preserves clang-tidy behavior).
.github/workflows/build_release.yaml Updates build script invocation to new named-argument interface for Release builds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmake_modules/ThirdpartyToolchain.cmake Outdated
@zjw1111 zjw1111 changed the title fix: enable thread sanitizer testing fix: enable TSAN and resolve detected data races Jul 17, 2026
lucasfang
lucasfang previously approved these changes Jul 17, 2026

@lucasfang lucasfang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1

@lxy-9602
lxy-9602 merged commit 4faf0e9 into alibaba:main Jul 20, 2026
10 checks passed
lxy-9602 pushed a commit to lxy-9602/paimon-cpp that referenced this pull request Jul 23, 2026
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.

4 participants