fix(build): repair singleton double-checked locking race and harden aarch64 portability

[u70b3]

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:

1. 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-only MEMORY_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 uses std::atomic<T*> with an acquire fast-path load and release publication, while preserving LazyInstantiation::Create(T*&)'s public signature.

2. IOHook::Impl::mode_ has a data race. Reset() writes the plain enum while IO threads read it in Try(). The fix makes it std::atomic<Mode> and stores it before the existing sequentially consistent pos_ and io_count_ stores.

3. SerializationUtils::DeserializeBinaryRow violates strict aliasing. It read an arity from a byte-filled buffer through reinterpret_cast<int32_t*>. The fix uses memcpy, matching the serialize side without changing the wire format.

4. Extreme cache/block sizes can trigger undefined double -> int conversions. CacheManager and SstFileWriter could produce architecture-dependent results for pathological configurations. A common-layer SaturatingDoubleToInteger<T> helper now implements Java-style saturation and is used at both sites.

FieldSumAgg INT8 sum/negation was also audited and intentionally left unchanged: modulo-256 addition and negation produce the same stored bits regardless of plain-char signedness. 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 in CoreOptions::GetCachePageSize.

Tests

The concurrency tests were verified red before the fix and TSan-clean after it on the Kunpeng-920:

Test	Coverage
SingletonTest.TestConcurrentIOHookGetInstance	Contended first publication of Singleton<IOHook>
IOHookTest.TestConcurrentResetAndTry	Concurrent Reset() / Clear() and Try() access
SerializationUtilsTest.TestDeserializeBinaryRowFromStream	Stream round-trip and big-endian arity prefix
CacheManagerTest	Saturation, normal split, and eviction behavior
SaturatingCastTest	int64/int32 normal, boundary, infinity, and NaN cases

The FactoryCreator first-publication storm from the initial revision was removed because static REGISTER_PAIMON_FACTORY constructors initialize it before main(), so it cannot test first publication reliably.

Local x86-64 validation (GCC 13, Debug):

• paimon-common-factories-test: 9/9 passed
• paimon-common-test: 1435/1435 passed
• paimon-common-sst-file-format-test: 32/32 passed
• pre-commit run --files <changed>: all checks passed
• git diff --check: clean

API 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)

[Copilot]

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 to std::atomic<T*> with acquire-load / release-store, while keeping LazyInstantiation::Create(T*&)’s signature.
• Remove UB/architecture divergence: strict-aliasing in SerializationUtils::DeserializeBinaryRow and undefined double -> int{32,64} casts via a new SaturatingDoubleToInteger<T> helper used in cache sizing and SST block sizing.
• Align TINYINT sum/negate behavior with Java signed-byte semantics across ABIs and add regression tests; make IOHook::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.

[u70b3]

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.

[SteNicholas]

The individual accesses are now data-race-free, but Reset() still does not publish a coherent configuration. For example, starting from (SILENT, -1, 0), Reset(INT64_MAX, RETURN_ERROR) can store the new mode, after which a concurrent Try() increments the old counter, reads the old -1 position, and returns IOError. Both stable configurations would return OK (the completed reset should not fire until INT64_MAX), so this is a torn state rather than either valid outcome. Since the new test explicitly supports concurrent Reset()/Try(), please publish an immutable combined state including a fresh counter, or synchronize the complete operations; a regression test can alternate Clear() with Reset(INT64_MAX, RETURN_ERROR) and assert that Try() never fails.

[SteNicholas]

This accepts every signed integral type, but it does not implement the Java/Paimon policy claimed above for narrow targets. For example, SaturatingDoubleToInteger<int8_t>(300.9) returns 127, while Java first converts to int32_t and then narrows the low bits, producing 44; a huge value similarly yields 127 here instead of -1. Please either implement the existing wide-then-narrow behavior for int8_t/int16_t, or restrict TargetType to the currently intended int32_t and int64_t instantiations and document that narrower contract.