fix(index): fail fast when FTS posting pipeline would deadlock#7350
fix(index): fail fast when FTS posting pipeline would deadlock#7350hfutatzhanghb wants to merge 4 commits into
Conversation
When only one lance-cpu blocking thread is available, the pipelined FTS posting-list writer deadlocks silently after logging "writing N posting lists". Reject the build early with a descriptive error and add regression tests that reproduce the deadlock in a child process. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Xuanwo @BubbleCal @yanghua Hi, please help review this PR when have free time, Thanks very much! |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@claude review this pr. |
BubbleCal
left a comment
There was a problem hiding this comment.
Requesting changes for two correctness issues in the CPU-thread deadlock guard.
| /// also submits column page encoding via `spawn_cpu`. With only one `lance-cpu` blocking | ||
| /// thread the producer blocks on the bounded channel while the writer waits for encoding, | ||
| /// which deadlocks with no further log output. | ||
| const MIN_CPU_THREADS_FOR_FTS_POSTING_PIPELINE: usize = 2; |
There was a problem hiding this comment.
A fixed threshold of 2 threads does not cover concurrent worker flushes. resolve_num_workers can still choose all available CPU threads, so with LANCE_CPU_THREADS=2 two workers can each occupy a spawn_cpu producer and then both writers wait for nested page-encoding spawn_cpu work with no free thread. This should reserve capacity for nested encoding or limit the number of concurrent posting writers/workers.
There was a problem hiding this comment.
@BubbleCal Thanks for the review. Addressed the threshold concern using a tokio::sync::Semaphore:
Fix: Added fts_posting_write_semaphore() with available_cpu_threads - 1 permits (at least 1). write_posting_lists now acquires a permit before calling write_posting_lists_pipelined, so at most available - 1 producers hold spawn_cpu threads concurrently, always leaving one free for the consumer's nested page encoding.
This handles the concurrent-worker case correctly:
LANCE_CPU_THREADS=2,num_workers=2: semaphore has 1 permit → only 1 writer runs at a time → no deadlock.LANCE_CPU_THREADS=4,num_workers=4: semaphore has 3 permits → up to 3 writers concurrently, 1 thread remains free for consumers.
Two new tests verify the semaphore behavior in child processes with LANCE_CPU_THREADS=2 and LANCE_CPU_THREADS=4.
…dlock Add a tokio::sync::Semaphore that limits concurrent write_posting_lists_pipelined calls to available_cpu_threads - 1, ensuring at least one spawn_cpu thread is always free for the consumer's nested page encoding inside FileWriter::write_batch. Previously only an early guard rejected configurations with fewer than 2 lance-cpu threads, but with LANCE_CPU_THREADS=2 and num_workers=2, both workers could occupy both spawn_cpu threads as producers, deadlocking when consumers needed a thread for page encoding. The semaphore fixes the concurrent-worker deadlock case while preserving the early guard for hopeless single-thread configs.
Xuanwo
left a comment
There was a problem hiding this comment.
The new semaphore addresses the multi-worker flush case, but the one-CPU guard is still too broad. write_posting_lists calls check_fts_posting_pipeline_cpu_threads() before it knows whether the write can actually deadlock, so even a single-batch posting-list write is rejected under LANCE_CPU_THREADS=1.
A single-batch producer can send the only batch and release the sole spawn_cpu thread before the consumer encodes pages, so this rejects small valid FTS builds. In the review run, LANCE_CPU_THREADS=1 cargo test -p lance-index scalar::inverted::builder::tests::test_write_posting_lists_batches_multiple_rows -- --exact --nocapture failed from this guard.
Please either use a serial/non-pipelined fallback when only one CPU thread is available, or restructure the producer so it does not block inside the sole spawn_cpu thread before rejecting all small FTS builds in that configuration.
Summary
lance-cpublocking threads are available, instead of hanging silently after loggingwriting N posting lists.invalid_inputerror that explains the deadlock, mentionsLANCE_CPU_THREADS, and suggests setting it to at least 2.tokio::sync::Semaphorethat limits concurrentwrite_posting_lists_pipelinedcalls toavailable_cpu_threads - 1, ensuring at least onespawn_cputhread is always free for the consumer's nested page encoding insideFileWriter::write_batch. This prevents the concurrent-worker deadlock whennum_workersequals all available CPU threads.InvertedIndexBuilder::update_indexintoInnerBuilder::write_posting_listsso that updates that never write posting lists (e.g., emptynew_datawith only deleted-fragment metadata) are not rejected.Root cause
write_posting_listsruns batch encoding onspawn_cpuwhileFileWriter::write_batchalso submits column page encoding viaspawn_cpu. With only one blocking thread, the producer blocks on the bounded channel while the consumer waits for nested encoding, causing a deadlock with no further log output.With ≥2 threads but
num_workersequal to all available threads, all producers occupy allspawn_cputhreads and consumers deadlock identically. The semaphore prevents this by serializing flush-phase concurrency.Test plan
cargo test -p lance-index test_fts_posting_pipeline_cpu_threads_error_messagecargo test -p lance-index test_fts_posting_pipeline_write_posting_lists_deadlocks_with_one_cpu_threadcargo test -p lance-index test_empty_update_with_one_cpu_thread_records_deleted_fragmentsLANCE_CPU_THREADS=2 cargo test -p lance-index test_fts_posting_semaphore_serializes_with_two_cpu_threadsLANCE_CPU_THREADS=4 cargo test -p lance-index test_fts_posting_semaphore_permits_scale_with_threadscargo fmt --allcargo clippy -p lance-index --lib --tests -- -D warnings