fix(fts): use async send in FTS index builder to prevent thread-pool …#7423
Merged
Conversation
github-actions
Bot
added
bug
Contributor
Author
|
Hi @westonpace - would be happy for your review. |
wjones127
requested changes
Jul 2, 2026
wjones127
left a comment
wjones127
left a comment
Contributor
There was a problem hiding this comment.
Thanks for doing this. It took me a bit to reason about this. I think we need to add better docs to the spawn_cpu() function and maybe check other usage. Basically it seems like we should never use spawn_cpu() for any work that will cause the thread to sleep. No channels, no IO, and no locks. It should just consume CPU and finish. If you need a blocking thread that does any of those things, we should instead just use a spawn_blocking against a unbounded threadpool. Does that sound right to you @westonpace ?
a-agmon
force-pushed
the
fix/fts-async-send
branch
from
710a9fa to
db619f2
Compare
a-agmon
force-pushed
the
fix/fts-async-send
branch
from
db619f2 to
d8dcf5d
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
|
Created follow up: #7637 |
wjones127
approved these changes
Jul 6, 2026
This was referenced Jul 6, 2026
wjones127
added a commit
that referenced
this pull request
Jul 7, 2026
`spawn_cpu` runs work on a dedicated pool sized to `get_num_compute_intensive_cpus()`, which collapses to a **single blocking thread** on hosts with `<= 3` CPUs. A closure that ever *waits* — blocking channel send/recv, I/O, a contended lock, or `block_on` — parks that thread and can starve the very work that would unblock it, deadlocking the pool with a silent 0% hang. This is the failure fixed in #7423. This PR writes the rule down and audits the call sites: - Expand the `spawn_cpu` doc comment with the "must never wait on anything" rule (no channels / no I/O / no locks / no `block_on`), the rationale, and a pointer to the recommended pattern (keep the waiting in async code, hand only pure CPU work to `spawn_cpu`). - Add a concise Concurrency rule to `rust/AGENTS.md`. ### Audit All ~20 `spawn_cpu` call sites were reviewed, following transitive calls. Every production site is clean (I/O/loading is awaited before the closure; the closures do pure in-memory CPU work) except one: - **IVF streaming partition search** (`ivf/v2.rs`): a single `spawn_cpu` closure does `blocking_recv` + `blocking_send` on capacity-1 channels. This is a deliberate optimization from #6475 (run a query's whole sequential search on one CPU worker to avoid per-partition fan-out, a measured 14–30% latency win), so fixing it trades a benchmarked perf win against small-host correctness and needs the #6475 author's input. Tracked in #7642 rather than fixed here. The FTS builder site is already correct after #7423. Closes #7637 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a-agmon
pushed a commit
to a-agmon/lance
that referenced
this pull request
Jul 8, 2026
The streaming/sequential partition search ran its whole recv/search/send loop inside a single `spawn_cpu` closure, using `blocking_recv` and `blocking_send` on capacity-1 channels. Both park the CPU-pool thread, and the producer feeding the input channel does object-store I/O whose partition decode can itself need the CPU pool. On hosts whose CPU pool collapses to a single thread (`<= 3` CPUs), or under enough concurrent queries, the parked search thread and the work that would unblock it can starve each other and deadlock the pool with a silent 0% hang — the same class as lance-format#7423. Move the channel `recv`/`send` back into the async task and hand only the pure-CPU search to `spawn_cpu`, over a batch of `STREAMING_SEARCH_BATCH_SIZE` partitions at a time. No CPU-pool thread ever parks on a channel, so the deadlock is structurally gone, while batching amortizes the per-dispatch overhead the single-worker design in lance-format#6475 was avoiding (fan-out paid N/K times instead of N). `should_stop` is still checked per partition, so early-stop granularity is unchanged. The `PreparedThreadCapturingIndex` test mock is updated to mirror the new non-blocking structure. Adds a regression test that a search spanning multiple batches still prepares and searches every partition in order. Closes lance-format#7642 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes lancedb/lancedb#3568 (the issue arises in lancedb indexing)
Building a full-text-search index hangs permanently at 0% CPU on hosts
whose Lance CPU pool has a single thread.
The CPU compute pool is sized
max(1, num_cpus - LANCE_IO_CORE_RESERVATION)(default reservation 2), so any machine with <= 3 visible CPUs (1-vCPU VMs, CI runners, CPU-limited Kubernetes pods) collapses to a 1-thread pool and deadlocks.Root cause is in
write_posting_lists. The posting-list producer runs on the CPU pool viaspawn_cpuand pushes batches into a capacity-1async_channelusing the synchronoustx.send_blocking(). When the channel is full,send_blockingparks the OS thread it is running on. On a single-thread pool that is the only thread, and the async consumer's column encoder (write_record_batch->spawn_cpu) needs that same pool to drain the channel. The parked producer and the starved consumer wait on each other forever: no timeout, no error, just a silent hang at 0% CPU.The hang only triggers once the posting lists span a second output batch (so the producer reaches a second, blocking send), which is why it appears as a data-size "cliff".
The PR restructures the producer as an async task that builds each batch on the CPU pool via
spawn_cpuand dispatches it withtx.send(batch).await. When the channel is full,send().awaityields the task back to the runtime instead of parking a pool thread, so the consumer can always be scheduled to drain it. Between batches the producer holds no pool thread while waiting, making the pool size irrelevant. The builder and the remaining posting-list iterator are handed back out of eachspawn_cpucall so the cross-batch cache-group accumulator is preserved.In addition, it adds a regression test that writes a partition whose posting lists span many output batches (exercising channel back-pressure) under a timeout and verifies every batch is searchable.
(verbose comments added in the code intentionally for review purposes - can be removed if inappropriate. I just thought it might be helpful as the issue is somewhat confusing)