Skip to content

Fix data race in OptimisticAllocator during parallel COPY - #949

Open
adsharma wants to merge 1 commit into
mainfrom
fix/optimistic-allocator-race
Open

Fix data race in OptimisticAllocator during parallel COPY#949
adsharma wants to merge 1 commit into
mainfrom
fix/optimistic-allocator-race

Conversation

@adsharma

@adsharma adsharma commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Symptom

Parallel COPY (16 workers) intermittently aborts with a glibc sysmalloc assertion. The crashing thread is typically inside an unrelated later malloc — e.g. InMemChunkedNodeGroupCollection::merge growing chunkedGroups during the rel partitioner merge — while another worker waits on the merge mutex. Classic use-after-corruption: the heap was already trashed by an earlier bad write.

Root cause

Blame: ba66140

LocalStorage::addOptimisticAllocator() caches one OptimisticAllocator per StorageManager and hands the same instance to every COPY worker thread (node and rel paths). But OptimisticAllocator::allocatePageRange() did an unsynchronized push_back onto optimisticallyAllocatedPages, despite the class being documented as thread-local-only. Concurrent vector mutation corrupts the heap; the crash detonates at the next malloc that touches it.

ThreadSanitizer report (TSan + BM_MALLOC build, LSQB SF=1 load):

WARNING: ThreadSanitizer: data race
  Write of size 8 ... by thread T1:
    #0 OptimisticAllocator::allocatePageRange
    #1 Column::flushData ... NodeBatchInsert::executeInternal
    #2 ProcessorTask::run / TaskScheduler::runWorkerThread
  Previous write ... by thread T16 (same stacks)

Fix

Synchronize the internally-tracked page ranges with a mutex in allocatePageRange(), rollback() and commit(). The underlying PageManager::allocatePageRange is already internally synchronized, so this is the only unprotected layer. Allocation happens per flushed chunk group, so lock contention is negligible. Also updates the stale thread-local docstring.

Verification

  • Pre-fix TSan build: race reported during parallel node COPY (Message table).
  • Post-fix TSan build: full LSQB SF=1 load (10 node + 21 edge COPYs) completes, exit 0, zero TSan reports, row counts correct.
  • Post-fix release shell (previously crashed every run): full load completes, counts correct.
  • clang-format clean.

LocalStorage caches one OptimisticAllocator per StorageManager and shares
it across all COPY worker threads, but allocatePageRange() mutated the
optimisticallyAllocatedPages vector without synchronization. Concurrent
push_backs corrupted the heap, crashing later mallocs (e.g. partitioner
merge during parallel rel COPY).

Guard the tracked page ranges with a mutex; the underlying PageManager
is already internally synchronized.
@adsharma

adsharma commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Design note: why shared + locked, not thread-local

Constraint — one worker writes to many files. OptimisticAllocator is bound to a single PageManager (one data file) at construction. Since per-partition data files (ba6614049), a single COPY worker fans out to many files: the partitioned flush path (node_batch_insert.cpp:writeAndResetNodeGroup via targets[partitionIdx].optimisticAllocator) flushes different partitions/files through different allocators within one worker loop. Kuzu's thread-local design (one allocator in worker-local state) only worked when there was exactly one file. So the allocator mapping must be keyed by file, not by thread — that is what forces the sharing, and why reverting to pure thread-local would require re-plumbing every flush site to a per-(worker x file) lookup.

Why a mutex is proportionate here. The hot per-row path (appends into chunked groups) is already thread-local per worker — TSan confirmed the allocator vector was the only shared state. The lock is taken once per flushed column chunk (once per 2048 rows per column; ~18k acquisitions over a multi-minute load), each critical section a vector push (tens of ns, one cache line). No measurable contention or scalability impact at this granularity.

A per-(worker x file) refactor would restore zero-sharing at the cost of touching all three flush sites (node plain, node partitioned, rel) plus lifecycle management for N x M allocator objects. Left as a possible follow-up; deliberately out of scope for this crash fix.

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.

1 participant