feat: add index build progress callbacks to segment builders - #86
Merged
jja725 merged 2 commits intoSep 21, 2026
Merged
Conversation
u70b3
marked this pull request as ready for review
September 20, 2026 01:21
Bridge lance core's IndexBuildProgress trait to a C callback on LanceIndexSegmentBuilder (E3 of the distributed-build track): - lance_index_segment_builder_set_progress_callback with START/PROGRESS/ COMPLETE events; total/unit meaningful on START, completed on PROGRESS. - Thread-safe/reentrant, non-blocking, no-reentrancy contract; callback and context must outlive the builder (conservative: spawned worker tasks and error paths may still deliver events). - Advisory only: the callback cannot abort the build. - C++ fluent IndexSegmentBuilder::progress_callback wrapper. - Direct async-trait dependency for the trait impl (already transitive via lance-index). Refs lance-format#55.
u70b3
force-pushed
the
feat/distributed-index-build-pr3
branch
from
September 20, 2026 01:21
253d55f to
44e3a5d
Compare
Review on lance-format#86 found the callback lifetime contract was unenforceable on error paths: lance core's inverted builder clones the progress handle into spawned tokenize_docs workers whose JoinHandles can be dropped early, so a detached clone could invoke the raw C callback/context after execute_uncommitted returned -- a use-after-free once the caller retired the context. Add a shared ProgressCallbackGate (Arc-shared across every clone core hands to worker tasks): execute_uncommitted retires it through a drop guard on the success, error, and panic exit paths, disabling new callback entries and draining in-flight invocations before returning; late detached calls become no-ops. The "invocations only occur while executing" contract is now enforced rather than advisory, and the callback/context only need to stay valid until execute_uncommitted returns (C and C++ docs updated). Also document that stage names are diagnostic-only and not a stable cross-version contract. Regression tests at the gate level: a detached task-owned clone calling after retirement is a no-op (the review reproducer, inverted), and retire blocks until an in-flight invocation exits.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The callback-lifetime blocker is fixed in 4a5f53b: a gate shared by all progress clones disables late entries and drains in-flight callbacks before execute_uncommitted returns on success, error, or panic. The C and C++ lifetime contract now matches that enforced boundary.
jja725
approved these changes
Sep 21, 2026
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
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.
E3 of the RFC #55 distributed-build track: an FFI bridge over lance core's
IndexBuildProgresstrait for the uncommitted segment builders. Build stages (train_ivf,train_quantizer,shuffle,merge_partitions, ...) and per-stage progress are now observable from C/C++.API
total/unitare meaningful only for STAGE_START (0/"" otherwise);completedonly for STAGE_PROGRESS.stageis always non-NULL, borrowed, valid for the call duration. Stage names are diagnostic-only, not a stable cross-version contract.callback_ctxmay be NULL; setting a callback replaces any previously set callback; it must be set before execute (the builder is single-use), otherwise the setter returns -1.Contract (documented on the typedef and setter)
lance_*function.execute_uncommittedruns, and this is enforced rather than contractual: a shared retirement gate (ProgressCallbackGate) disables new callback entries and drains in-flight invocations beforeexecute_uncommittedreturns — on success, error, and panic paths via a drop guard — so a core worker task detached on an error path (e.g. the inverted index'stokenize_docsworkers, whoseJoinHandles can be dropped early) degrades to a no-op instead of touching freed context.callback/callback_ctxmust remain valid untilexecute_uncommittedreturns.Deviations from the RFC sketch
lance_index_segment_builder_set_progress/user_data; this PR usesset_progress_callback/callback_ctxto match the newer in-treelance_scanner_set_statistics_callbackconvention. No semantic change.lance_scanner_set_statistics_callbackboundary convention (reject invalid values at the API boundary). Consequence: a set callback can be replaced but not unset — install a no-op callback to disable reporting.Implementation notes
tokenize_docsworkers whoseJoinHandles can be dropped early on error paths, which could otherwise invoke the raw C context after the build returned. The gate (SeqCst disable + drain,Arc-shared across every clone core hands to worker tasks) closes that window and makes the documented boundary enforceable.Ok(()); the only error path is a defensive interior-NUL invariant violation in stage/unit strings (never panics).async-traitadded for the#[async_trait]impl oflance_index::progress::IndexBuildProgress(already in the dependency graph transitively via lance-index; Cargo.lock gains only the lance-c edge).Tests
Gate-level unit tests (src/index_segment.rs):
detached_clone_cannot_enter_callback_after_retire— the review reproducer, inverted: a task-owned clone calling after gate retirement is a no-op and never touches the context.retire_drains_in_flight_invocation— retirement disables new entries, then blocks until an in-flight invocation exits.Rust (tests/c_api_test.rs):
test_vector_index_segment_progress_callback— IVF_PQ 256x16 through the segment builder; event codes, per-stage START/COMPLETE pairing, numeric mapping invariants (PROGRESS ⇒ total==0, START ⇒ completed==0), shuffle and merge_partitions observed, shuffle PROGRESS completed <= total with unit == "rows", ctx round-trip.test_scalar_index_segment_progress_callback_sees_load_data— BTree build sees load_data START/COMPLETE.test_vector_index_segment_progress_callback_multi_fragment_subset— 2-fragment dataset, fragment-scoped build with callback active.test_index_segment_builder_progress_callback_edge_cases— NULL builder / NULL callback / NULL callback_ctx (arrives verbatim) / sentinel ctx round-trip / set after execute / set twice (only the replacement receives events).test_index_segment_builder_progress_callback_success_clears_error.C (tests/cpp/test_c_api.c) and C++ (tests/cpp/test_cpp_api.cpp):
test_index_segment_builder_progresseach — real compile-and-run coverage, including the fluentprogress_callbacksetter on the C++ side.Full suite green:
cargo fmt,cargo check --all-targets,cargo clippy --all-targets -- -D warnings,cargo test(358 c_api tests incl. the 5 new),cargo test --test compile_and_run_test -- --ignored(C + C++ + static OSS transport).Refs #55.