Run C API work on the runtime's worker threads, not the caller's stack by aaltshuler · Pull Request #69 · lancedb/lancedb-c

aaltshuler · 2026-09-02T08:30:41Z

Problem

Every database-executing entry point runs runtime.block_on(async { … })
on the calling thread, so lance/datafusion planning executes on whatever
stack the caller has. Hosts calling from small fixed-size stacks overflow —
Ceph RGW's beast coroutines use 512 KiB, and examples/asio_coroutine.cpp
measures ~552 KB for a merge_insert. lancedb_run_on_stack() (#48) cannot
reliably protect such callers: stacker decides whether to grow from
remaining_stack(), computed against the OS thread's stack bounds
(pthread_getattr_np), which say nothing about a Boost coroutine's
separately mapped stack. On one host we observed it grow (by luck of
mapping order); the same heuristic can decide "enough space" and let the
operation overflow. The unwrapped entry points have no protection at all.

Change

src/runtime.rs: build the runtime explicitly
(Builder::new_multi_thread, lancedb-worker threads, 8 MiB worker
stacks; LANCEDB_C_WORKER_STACK_SIZE / LANCEDB_C_WORKER_THREADS
override) and add run_blocking: spawn the future onto the runtime and
park the caller on the JoinHandle — the pattern Lance's Python bindings
use (BackgroundExecutor::spawn). Panics inside the work become
Error::Runtime (→ LANCEDB_ERROR_RUNTIME) instead of unwinding across
extern "C"; the "called from within a runtime" misuse panic is caught
and reported the same way.
All 37 block_on call sites converted. Raw C pointers are read before the
hop (table_create, merge_insert); the table-metadata entry points copy
entries out while holding the dataset read guard on the worker; futures
without a Result output use run_blocking_infallible.
include/lancedb.h: "Threading model" section (workers, env knobs, the
runtime-context restriction, and that Arrow objects may be released on
worker threads — producers must tolerate cross-thread release).
lancedb_run_on_stack() is retained for compatibility.
No C ABI change.

Tests

tests/test_stack.cpp (lancedb_stack_tests): merge_insert, a 20-level
nested SQL delete, vector search, scalar index creation and the metadata
round trip, each on a pthread with a 256 KiB stack. Against the
previous design merge_insert, the nested delete and scalar index creation
segfault; with this change all five pass.
Rust unit tests for the helper: work runs on a worker thread; a panicking
future returns Error::Runtime; a 64 KiB-stack caller completes a deep
recursive future.
Existing suites unchanged (run_on_stack test still passes).

Notes for reviewers

Blocking work inside object-store implementations now runs on runtime
workers for the top-level operation as well as for lance-io's spawned
reads. Implementations that block (Ceph's SAL-backed store does) should
use block_in_place/spawn_blocking; the Ceph side does that in its
matching change.
Compatible with allow for explicit tokio runtime definitions #37 (LanceDBRuntime): run_blocking can resolve the
runtime the same way.
Measured on Ceph RGW with stock 512 KiB coroutine stacks: planning ran on
lancedb-worker threads in every case; the workload passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01E63tgb7eXHwzmaQBSErFWu

Every database-executing entry point used Runtime::block_on on the calling thread, so lance/datafusion planning ran on whatever stack the caller had. Hosts that call the C API from small fixed-size stacks (Ceph RGW's beast coroutines use 512 KiB) overflow; lancedb_run_on_stack() cannot reliably help there because stacker measures remaining stack against the OS thread's bounds, which do not describe a coroutine's separately mapped stack. Introduce runtime::run_blocking: spawn the future onto the runtime and park the caller on the JoinHandle (the pattern Lance's Python bindings use in BackgroundExecutor::spawn). Build the runtime explicitly with an 8 MiB worker stack (LANCEDB_C_WORKER_STACK_SIZE / LANCEDB_C_WORKER_THREADS override it). Panics inside the work, and the block_on misuse panic, are reported as LANCEDB_ERROR_RUNTIME instead of aborting the process. Convert all 37 call sites. Raw C pointers are read before the hop (table_create, merge_insert); the table-metadata entry points copy the entries out while holding the dataset read guard on the worker; futures without a Result output use run_blocking_infallible. Add tests/test_stack.cpp, which runs merge_insert, a nested SQL delete, vector search, scalar index creation and the metadata round trip from a 256 KiB pthread. Against the previous design the merge_insert, nested delete and scalar-index sections segfault (planning exceeds the caller's stack); with this change all five pass. Rust unit tests cover the helper itself. Document the threading model in lancedb.h; lancedb_run_on_stack() is kept for compatibility. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E63tgb7eXHwzmaQBSErFWu Signed-off-by: Andrew Altshuler <andrew@modernrelay.com>

yuvalif · 2026-09-07T11:03:33Z

  add_test(NAME lancedb_vector_index_tests COMMAND ${TEST_ENV_PREFIX} $<TARGET_FILE:lancedb_vector_index_tests>)
  # Run vector query tests WITHOUT valgrind (too slow under valgrind)
  add_test(NAME lancedb_vector_query_tests COMMAND ${TEST_ENV_PREFIX} $<TARGET_FILE:lancedb_vector_query_tests>)
+  # Small-stack regression tests run on dedicated 256 KiB pthreads; not under valgrind


why not under valgrind?

yuvalif · 2026-09-07T11:09:12Z

+/// Upstream measured ~552 KB of stack for a merge_insert in
+/// `examples/asio_coroutine.cpp`; tokio's default of 2 MiB leaves little margin
+/// for debug builds, so we allocate 8 MiB.  Stacks are mmap'd and committed
+/// lazily, so the cost is virtual address space, not RSS.


can you measure how much more memory consumption this adds?
note that we may use high concurrency client count (e.g. 1K) - this is 8GB vs. 2GB.

yuvalif · 2026-09-07T12:12:57Z

+ * lancedb_run_on_stack() below is no longer required for the above and is
+ * retained for compatibility with callers that already use it.
+ */
 /**


given the issues you mentione, it is probably better to remove the lancedb_run_on_stack() API.
this would be a breaking change, but probably ok at this early stage of the library

Copilot

🔵 Needs a closer look

It changes core runtime/threading behavior across many FFI entry points and warrants careful human validation despite the added tests.

Pull request overview

This PR changes the C bindings’ execution model so synchronous C entry points no longer block_on futures on the caller’s stack, but instead spawn the work onto dedicated Tokio worker threads (with configurable worker count and stack size) and block the caller only on completion—preventing stack overflows in small-stack hosts (e.g., coroutines/fibers).

Changes:

Introduces a dedicated Tokio runtime + run_blocking/run_blocking_infallible bridge that catches panics and converts them to Error::Runtime.
Converts C API entry points from direct runtime.block_on(...) to run_blocking(...), copying/owning raw-pointer inputs before hopping threads where needed.
Adds documentation of the threading model to the C header and adds a new small-stack regression test suite wired into CMake/CTest.

File summaries

File	Description
`src/runtime.rs`	Adds explicit multi-thread Tokio runtime construction and the `run_blocking` bridge with panic-to-error handling plus unit tests.
`src/connection.rs`	Switches connection/session entry points to `run_blocking` and removes the old runtime singleton from this module.
`src/table.rs`	Converts table entry points to `run_blocking`, cloning/owning inputs to avoid crossing worker threads with raw pointers.
`src/index.rs`	Converts index-related entry points to `run_blocking`, owning config-derived values before thread hop.
`src/query.rs`	Converts query execution/result conversion to `run_blocking`.
`src/lib.rs`	Exposes the new internal `runtime` module.
`include/lancedb.h`	Documents the threading model, runtime-context restriction, and cross-thread Arrow release implications.
`tests/test_stack.cpp`	Adds regression tests that run operations on 256 KiB pthread stacks to validate no caller-stack polling.
`CMakeLists.txt`	Builds and registers the new `lancedb_stack_tests` executable in CTest.

Review details

Files reviewed: 9/9 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+struct StackJob {
+  void (*fn)(StackJob&);
+  LanceDBTable* table;
+  LanceDBError result = LANCEDB_UNKNOWN;
+  char* error_message = nullptr;
+  uint64_t count = 0;
+};


yuvalif · 2026-09-07T12:58:36Z

Panics inside the work become Error::Runtime (→ LANCEDB_ERROR_RUNTIME) instead of unwinding across
extern "C"; the "called from within a runtime" misuse panic is caught and reported the same way.

could you please add a test that covers that?
seems like overall coverage decreased, and this is probably something worthwhile testing.
(btw, settign the default stack size too small is not caught via panic, but crashes with SIGSEGV).

Compatible with allow for explicit tokio runtime definitions #37 (LanceDBRuntime): run_blocking can resolve the runtime the same way.

we should probably create a combined PR, that allows passing stack size, thread count (and maybe other parameters) from C into the runtime creation. using a env variable used when the process tarts gived very little flexibility. the thread count paramerter may be even more critical as we run reindexign and compaction in out background process, and we want to avoid it taking too much CPU.

yuvalif · 2026-09-07T13:03:33Z

could you please prepare a ceph/s3vectors PR that point to this change as its lancedb-c submodule (temporarily)?
and remove the "call_on_stack()" invocations?
this will help us validate this work using a real client before merging this PR.

This was referenced Sep 2, 2026

rgw/s3vectors: run SAL calls off tokio workers in lancedb-rgw-store ceph/ceph#71480

Open

rgw/s3vector: adding s3vectors API support to the RGW ceph/ceph#66066

Open

aaltshuler force-pushed the spawn-hop branch from 082b4ff to 7fb3b1b Compare September 2, 2026 08:55

yuvalif reviewed Sep 7, 2026

View reviewed changes

yuvalif requested a lite review from Copilot September 7, 2026 12:17

Copilot started reviewing on behalf of yuvalif September 7, 2026 12:18 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Comment thread tests/test_stack.cpp

Comment on lines +24 to +30

struct StackJob {

void (*fn)(StackJob&);

LanceDBTable* table;

LanceDBError result = LANCEDB_UNKNOWN;

char* error_message = nullptr;

uint64_t count = 0;

};

Navigation Menu

Uh oh!

Run C API work on the runtime's worker threads, not the caller's stack - #69

Run C API work on the runtime's worker threads, not the caller's stack#69
aaltshuler wants to merge 1 commit into
lancedb:mainfrom
aaltshuler:spawn-hop

aaltshuler commented Sep 2, 2026

Uh oh!

yuvalif Sep 7, 2026

Uh oh!

yuvalif Sep 7, 2026

Uh oh!

yuvalif Sep 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

yuvalif commented Sep 7, 2026

Uh oh!

yuvalif commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aaltshuler commented Sep 2, 2026

Problem

Change

Tests

Notes for reviewers

Uh oh!

yuvalif Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

yuvalif Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

yuvalif Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

yuvalif commented Sep 7, 2026

Uh oh!

yuvalif commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants