fix(build): repair singleton double-checked locking race and harden aarch64 portability - #203
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens several low-level common/core components for aarch64/weak-memory portability by eliminating undefined behavior and data races that were previously masked on x86 TSO, and adds targeted regression tests to lock in the corrected semantics.
Changes:
- Fix
Singleton<T>double-checked locking publication by switching tostd::atomic<T*>with acquire-load / release-store, while keepingLazyInstantiation::Create(T*&)’s signature. - Remove UB/architecture divergence: strict-aliasing in
SerializationUtils::DeserializeBinaryRowand undefineddouble -> int{32,64}casts via a newSaturatingDoubleToInteger<T>helper used in cache sizing and SST block sizing. - Align
TINYINTsum/negate behavior with Java signed-byte semantics across ABIs and add regression tests; makeIOHook::mode_atomic and add TSan-oriented concurrency tests.
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 |
|---|---|
| src/paimon/core/mergetree/compact/aggregate/field_sum_agg.cpp | Use int8_t semantics for INT8 sum/negate to avoid ABI-dependent char signedness behavior. |
| src/paimon/core/mergetree/compact/aggregate/field_sum_agg_test.cpp | Add boundary-case tests pinning INT8 Java signed-byte wrap semantics. |
| src/paimon/common/utils/serialization_utils.h | Replace strict-aliasing reinterpret_cast read with memcpy when parsing arity. |
| src/paimon/common/utils/serialization_utils_test.cpp | Add stream-path round-trip test and pin big-endian arity prefix bytes. |
| src/paimon/common/utils/saturating_cast.h | Introduce common-layer helper for Java-style saturating double -> signed int conversion. |
| src/paimon/common/sst/sst_file_writer.cpp | Use saturating conversion for block_size * 1.1 to avoid undefined double -> int32_t. |
| src/paimon/common/io/cache/cache_manager.h | Use saturating conversion for cache split sizing to avoid undefined double -> int64_t. |
| src/paimon/common/io/cache/cache_manager_test.cpp | Add regression coverage for saturation, split sizing, and eviction behavior through CacheManager::GetPage. |
| src/paimon/common/factories/singleton.cpp | Fix Singleton publication with atomic acquire/release, keeping a mutex slow path. |
| src/paimon/common/factories/singleton_test.cpp | Add concurrent “storm” tests to ensure fully-constructed singleton visibility under contention. |
| src/paimon/common/factories/io_hook.cpp | Make mode_ atomic and use atomic load in Try() to eliminate a data race. |
| src/paimon/common/factories/io_hook_test.cpp | Add concurrency regression test for Reset()/Clear() racing with Try(). |
| src/paimon/CMakeLists.txt | Wire new unit tests into the appropriate test targets. |
| include/paimon/factories/singleton.h | Remove now-misleading barrier in LazyInstantiation::Create, relying on release-store in GetInstance(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
SteNicholas
changed the title
|
Thanks for the lightning-fast review! I’m working through the new comments and the CI failure now, and iterating on the fixes and validation. I opened the PR early to get review started before running the local x86 suite; I’ll push the fixes and updated validation results shortly. |
9f6fcdb to
de32698
Compare
|
ARM re-test on the latest head (cba5129) — all green ✅ Environment: Kunpeng-920 (aarch64, 128 cores), Ubuntu 22.04, GCC 11.4, Clang 14, CMake 3.22 Debug (GCC 11.4)
ThreadSanitizer (Clang 14)
Checks: The |
There was a problem hiding this comment.
@u70b3, thanks for update. Two test-coverage issues can let the new concurrency regressions pass without exercising the races they are intended to guard.
| // and this is its first test). A FactoryCreator storm cannot serve as the | ||
| // gate: the REGISTER_PAIMON_FACTORY constructors in paimon_shared already | ||
| // initialize Singleton<FactoryCreator> before main(). | ||
| TEST(SingletonTest, TestConcurrentIOHookGetInstance) { |
There was a problem hiding this comment.
This regression gate depends on this test being the first code to instantiate Singleton<IOHook>, but CMake source order does not guarantee cross-translation-unit test registration/execution order across linkers, and --gtest_shuffle can reorder it. If another test calls GetInstance() first, this storm only exercises the already-published fast path and cannot detect the original publication race. Please make the first construction test-controlled, for example with a dedicated test singleton or an isolated test executable.
There was a problem hiding this comment.
Good catch — and agreed on the deeper hazard: a regression gate that can pass without exercising the race it guards is worse than no test at all. It does not just miss the bug, it silently certifies the fix (our "TSan-clean" claim rested on this storm), so a vacuous green actively masks the failure it was built to catch.
Fixed by making the first construction test-controlled instead of convention-controlled:
Singleton<T, InstPolicy>::GetInstance()moved from singleton.cpp into the header, so a test can instantiateSingleton<T>for its own type — previously impossible, which is why the storm had to borrowSingleton<IOHook>and inherit the ordering fragility.- The storm (renamed
TestConcurrentFirstPublication) now runs onFirstPublicationTarget, a class local to singleton_test.cpp's anonymous namespace. Nothing else in the binary can name the type, so no link order,--gtest_shuffle,--gtest_filter, or--gtest_repeatcan pre-publish it: the test is guaranteed to race the first publication by construction. Each thread also verifies full construction visibility (a 64-word payload written by the ctor), which is the essence of the original race. - One subtlety worth recording: the first attempt dropped the explicit instantiations entirely, and local validation immediately caught it — the file-format plugins are separate
-Bsymbolicshared libraries, so implicit per-library copies of the function-local static state split the singleton (registrations landed in a different instance than lookups, and the wider suites lost the 'orc' factory).FactoryCreatorandIOHooktherefore keep extern-template declarations plus explicit instantiations in singleton.cpp; only TU-local test types instantiate implicitly.
Validation on Kunpeng-920 (aarch64), latest head (1bc6650): factories suite 9/9 under 20× --gtest_shuffle plus a --gtest_repeat=5 --gtest_shuffle run; paimon-common-test 1436/1436 and paimon-common-sst-file-format-test 32/32 (these are the suites that caught the split-singleton issue); TSan clean over 20 shuffled stress runs of the race tests.
|
|
||
| std::atomic<bool> observed_error{false}; | ||
|
|
||
| std::thread reset_thread([hook]() { |
There was a problem hiding this comment.
The reset thread starts before the worker threads are even created, so it can finish all iterations before any Try() runs. Because every iteration ends with Clear(), the test then passes in the final SILENT state without exercising concurrent Reset()/Try(). Please add a shared start barrier and keep the reset loop active until the workers signal completion so overlap is guaranteed.
There was a problem hiding this comment.
Agreed — same class of hazard: with the reset thread free-running 200k fixed iterations before the workers were even spawned, a loaded CI runner or a TSan-slowed build could serialize the two sides completely, and since every iteration ends in Clear() the workers would then only ever see SILENT — a silent no-op masquerading as a concurrency gate. A test that can pass with zero overlap does not just miss the torn-state bug, it masks it behind false confidence.
Fixed structurally:
- All five threads now block on a shared start barrier and are released together.
- The reset loop has no fixed iteration count: it hammers
Reset(INT64_MAX, RETURN_ERROR)/Clear()until every worker has completed all itsTry()iterations (aworkers_donecounter), so overlap covers the workers' entire lifetime by construction. - Side benefit: the fixed 200k-iteration cost is gone, so the test is faster under TSan while being strictly stronger (~120ms in Debug).
Validation on Kunpeng-920 (aarch64), latest head (1bc6650): 20× shuffled stress runs of this test plus the singleton storm under TSan — zero reports; Debug suite green under 20× --gtest_shuffle and --gtest_repeat=5 --gtest_shuffle.
1bc6650 to
4b43803
Compare
4b43803 to
46d7e40
Compare
| public: | ||
| Status Try(const std::string& path) { | ||
| if (io_count_.fetch_add(1) < pos_.load()) { | ||
| std::shared_lock<std::shared_mutex> lock(mutex_); |
There was a problem hiding this comment.
The race fix itself is welcome — Reset()/Clear() and Try() were genuinely racy before. My concern is where the lock landed: Try() is invoked by CHECK_HOOK on every local-file IO (in local_file.cpp, hook_ is always initialized to IOHook::GetInstance(), so the guard never skips it). The old implementation was lock-free (fetch_add + an atomic pos_ load); the new one takes a std::shared_mutex shared lock on every IO.
Suggested fix: gate the lock behind an "armed" flag. The hook is never armed in production — the default state is exactly what Clear() sets up (pos_ = -1, SILENT), and writers only exist in tests. So we can keep the mutex for correctness on the armed path while restoring a lock-free fast path for the disabled state:
There was a problem hiding this comment.
Good point — done in 993b2d5 by gating the synchronized path behind an atomic armed flag:
Try()now starts with a lock-free fast path: a single acquire load, returning OK immediately in the disabled state (the production default). Theshared_mutexis only taken once armed.Reset()publishes the complete configuration under the mutex and then release-storesarmed; armed readers acquire the mutex before touchingmode_/pos_, so an observed armed state always implies a complete configuration.Clear()disarms first (still under the mutex) so IO threads drop off the lock as soon as possible.- One semantic change worth noting: the disabled fast path no longer increments the counter, so
IOCount()now counts only while armed. AllIOCount()consumers are tests, and the header documents the contract. - Coverage: new
IOHookTest.TestDisabledFastPathpins the disabled behavior (Try always OK, no counting, re-arm/disarm restores it). The existingTestConcurrentResetAndTryhammersReset(INT64_MAX, RETURN_ERROR)/Clear()continuously, so workers keep switching between the fast and armed paths under TSan.
Validation on Kunpeng-920 (aarch64), head 993b2d5: factories suite 10/10 (plus 20× --gtest_shuffle and a --gtest_repeat=5 --gtest_shuffle run); paimon-common-test 1450/1450; paimon-common-sst-file-format-test 32/32; TSan clean over 20 shuffled stress runs of the race tests; pre-commit and git diff --check clean.
|
+1 |
4ccff4f to
df7ee6d
Compare
df7ee6d to
63a5f82
Compare
…portability An aarch64 portability audit (7-category static sweep + on-hardware validation on a 128-core Kunpeng-920) found one real concurrency bug hidden by x86 TSO and three latent UB/divergence risks: - Singleton<T>::GetInstance() published the instance with a plain store guarded only by a compiler-only MEMORY_BARRIER, and the fast path read it with a plain non-atomic load. On aarch64 this allows readers to observe a non-null pointer to a not-yet-constructed object. Use std::atomic<T*> with acquire/release ordering. - IOHook::Impl::mode_ was a plain enum raced by Reset() and Try(). Make it std::atomic<Mode>, stored before the seq_cst pos_/io_count_ stores so it is published together with them. - SerializationUtils::DeserializeBinaryRow read arity from a byte-filled buffer through reinterpret_cast<int32_t*> (strict-aliasing UB). Use memcpy like the serialize side; identical codegen. - CacheManager and SstFileWriter relied on undefined double->int conversions for extreme configs (x86-64 cvttsd2si yields the integer indefinite value, aarch64 fcvtzs saturates). Add common-layer SaturatingDoubleToInteger with the Java saturation policy and use it at both sites. FieldSumAgg INT8 sum/neg was audited and left unchanged: mod-256 addition and negation are invariant under plain-char signedness, so the stored bytes already match Java bit-for-bit on both ABIs. Unlike the min/max comparisons fixed in PR apache#181, signedness cannot change the result here. Tests, run on the Kunpeng-920 with PAIMON_USE_TSAN=ON for the races: - SingletonTest.TestConcurrentIOHookGetInstance storms the first publication of Singleton<IOHook>: TSan-red pre-fix (race in LazyInstantiation::Create), clean after. A FactoryCreator storm cannot gate this race because the REGISTER_PAIMON_FACTORY constructors in paimon_shared already initialize it before main(). - IOHookTest.TestConcurrentResetAndTry: TSan-red pre-fix (race on Impl::mode_), clean after. - SerializationUtilsTest gains a DataInputStream round-trip that also pins the big-endian wire format. - CacheManagerTest locks the saturated capacity semantics. Pre-fix, x86-64 cvttsd2si yields INT64_MIN (red there), while aarch64 fcvtzs saturates natively (green either way, verified on the Kunpeng-920). - SaturatingCastTest covers the helper's boundaries directly, including the int32_t path used by SstFileWriter. Generated-by: Claude Code (claude-opus-4-8)
The two concurrency regression tests could pass without exercising the races they guard: - TestConcurrentIOHookGetInstance gated the Singleton publication race only if no other test touched Singleton<IOHook> first, which neither link order nor --gtest_shuffle/--gtest_filter can guarantee. The storm now runs on a translation-unit-local type renamed TestConcurrentFirstPublication, so first publication is guaranteed by construction. Enabling a test-local instantiation required moving GetInstance() into the header; FactoryCreator and IOHook keep extern-template declarations plus explicit instantiations in singleton.cpp so the -Bsymbolic plugin shared libraries still share exactly one definition. - TestConcurrentResetAndTry let the reset thread free-run a fixed iteration count that could finish before the workers started, leaving the hook SILENT and the test vacuously green. A shared start barrier and a workers_done-driven reset loop now make the overlap structural.
63a5f82 to
b8e76a9
Compare
Try() is invoked by CHECK_HOOK on every local-file IO, so taking a std::shared_mutex shared lock per call regresses the previously lock-free IO path. Writers (Reset()/Clear()) only exist in tests and the production default is the disabled state, so gate the synchronized path behind an atomic armed flag: Try() is now a single acquire load when disabled, and only takes the mutex once armed. Reset() publishes the complete configuration under the mutex and then release-stores armed; armed readers acquire the mutex before reading mode/pos, so an observed armed state always implies a complete configuration. Clear() disarms first so IO threads drop off the lock as soon as possible. One semantic change: the disabled fast path no longer increments the IO counter, so IOCount() now counts only while armed; all IOCount() consumers are tests and the header documents the contract. Add IOHookTest.TestDisabledFastPath to pin the disabled behavior, and arm the hook in TestReadAheadCache.TestPreBufferWindowLimit (apache#209), which relied on the previous count-while-disabled behavior.
b8e76a9 to
890fad4
Compare
|
@SteNicholas @lucasfang Both review rounds are addressed in the pushed head (890fad4, rebased onto latest main). The Re-validated locally on aarch64 (gcc Debug): common-test 1459/1459, factories + sst + parquet suites all green, pre-commit clean. The earlier gcc-debug-aarch64 segfault did not reproduce and looks unrelated to this PR. Could you approve the pending workflow runs? |
|
Thank you very much for the PR and for the multiple rounds of updates. Once the current CI passes, we’ll merge it. Thanks again for your patience! |
Purpose
Following up on the aarch64 port (#181), an ARM portability audit (7-category static sweep plus on-hardware validation on a 128-core Kunpeng-920, ARMv8.2) found one real concurrency bug currently hidden by x86 TSO and three latent UB/divergence risks:
Singleton<T>double-checked locking is broken on weak memory models. The instance pointer was published with a plain store guarded only by a compiler-onlyMEMORY_BARRIER, while the fast path used a plain non-atomic load. A reader can therefore observe a non-null pointer before construction is visible. The fix usesstd::atomic<T*>with an acquire fast-path load and release publication, while preservingLazyInstantiation::Create(T*&)'s public signature.IOHook::Impl::mode_has a data race.Reset()writes the plain enum while IO threads read it inTry(). The fix makes itstd::atomic<Mode>and stores it before the existing sequentially consistentpos_andio_count_stores.SerializationUtils::DeserializeBinaryRowviolates strict aliasing. It read an arity from a byte-filled buffer throughreinterpret_cast<int32_t*>. The fix usesmemcpy, matching the serialize side without changing the wire format.Extreme cache/block sizes can trigger undefined
double -> intconversions.CacheManagerandSstFileWritercould produce architecture-dependent results for pathological configurations. A common-layerSaturatingDoubleToInteger<T>helper now implements Java-style saturation and is used at both sites.FieldSumAggINT8 sum/negation was also audited and intentionally left unchanged: modulo-256 addition and negation produce the same stored bits regardless of plain-charsignedness. Unlike the min/max comparisons fixed in #181, signedness cannot change these results.Out of scope (documented follow-ups): tightening option validation for
btree-index.block-size/cache-page-size, and the unchecked int64-to-int32 narrowing inCoreOptions::GetCachePageSize.Tests
The concurrency tests were verified red before the fix and TSan-clean after it on the Kunpeng-920:
SingletonTest.TestConcurrentFirstPublicationSingleton<T>on a translation-unit-local type, so no link order,--gtest_shuffle, or--gtest_filtercan pre-publish the instance and silently degrade the gate into the fast path; each thread also verifies full construction visibilityIOHookTest.TestConcurrentResetAndTryReset()/Clear()andTry()access; a shared start barrier releases all threads together and the reset loop runs until every worker finishes, so overlap is structural rather than timing-dependentSerializationUtilsTest.TestDeserializeBinaryRowFromStreamCacheManagerTestSaturatingCastTestThe
FactoryCreatorfirst-publication storm from the initial revision was removed because staticREGISTER_PAIMON_FACTORYconstructors initialize it beforemain(), so it cannot test first publication reliably. For the same reason the storm no longer borrowsSingleton<IOHook>:GetInstance()is now defined in the header, so the test instantiatesSingleton<T>for its own TU-local type and controls first publication by construction.FactoryCreatorandIOHookkeep extern-template declarations with explicit instantiations insingleton.cpp, because the-Bsymbolicplugin shared libraries must share exactly one definition (implicit per-library copies split the singleton and factory registrations are lost).Validation on the latest head (Kunpeng-920, aarch64):
paimon-common-factories-test: 9/9 passed (Debug), including 20×--gtest_shuffleruns and a--gtest_repeat=5 --gtest_shufflerun proving order independencepaimon-common-test: 1436/1436 passed andpaimon-common-sst-file-format-test: 32/32 passed (Debug)--gtest_shufflewith zero reportspre-commit run --files <changed>: all checks passedgit diff --check: cleanAPI and Format
No public API or storage-format change.
LazyInstantiation::Create(T*&)remains unchanged, and the big-endian arity prefix is preserved and pinned by tests.Documentation
No documentation changes needed; code comments explain the ordering and saturation rationale.
Generative AI tooling
Generated-by: Claude Code (claude-opus-4-8)