fix(python): allow blob writer cleanup across threads#7827
Conversation
📝 WalkthroughWalkthroughBlob writer state is now mutex-protected for cross-thread access, with shared handling for completed writers and poisoned locks. Packed and dedicated writers use these helpers, and a regression test covers traceback cleanup after threaded write failures. ChangesBlob writer state management
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/blob.rs (1)
388-398: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve the write error on cleanup failure
self.take_inner()?can replace the original object-store write error if cleanup fails; ignore the cleanup failure here so the write error stays visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/src/blob.rs` around lines 388 - 398, Update the error branch in the batch write result handling to attempt self.take_inner() cleanup without propagating its failure, then return Err(error) so the original object-store write error is preserved.
🧹 Nitpick comments (1)
python/src/blob.rs (1)
52-58: 📐 Maintainability & Code Quality | 🔵 TrivialError messages lack per-instance context.
finished_writer/poisoned_writeronly interpolate the static class name (writer_name), never the blob'spath/blob_id. When debugging a poisoned-lock or already-finished error, knowing which writer instance failed is valuable, especially since multiple writers can exist per session/dataset.As per coding guidelines, "Include full error context, including variable names, values, sizes, and types."
♻️ Proposed context enrichment
-fn finished_writer(writer_name: &str) -> PyErr { - PyValueError::new_err(format!("{writer_name} is already finished")) +fn finished_writer(writer_name: &str, path: &str) -> PyErr { + PyValueError::new_err(format!("{writer_name} for '{path}' is already finished")) } -fn poisoned_writer(writer_name: &str) -> PyErr { - PyRuntimeError::new_err(format!("{writer_name} lock is poisoned")) +fn poisoned_writer(writer_name: &str, path: &str) -> PyErr { + PyRuntimeError::new_err(format!("{writer_name} lock for '{path}' is poisoned")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/src/blob.rs` around lines 52 - 58, Update finished_writer and poisoned_writer to accept the affected blob’s path or blob_id in addition to writer_name, and include that per-instance value in both error messages. Update every call site to pass the relevant instance identifier while preserving the existing exception types and messages for the writer class context.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/src/blob.rs`:
- Around line 388-398: Update the error branch in the batch write result
handling to attempt self.take_inner() cleanup without propagating its failure,
then return Err(error) so the original object-store write error is preserved.
---
Nitpick comments:
In `@python/src/blob.rs`:
- Around line 52-58: Update finished_writer and poisoned_writer to accept the
affected blob’s path or blob_id in addition to writer_name, and include that
per-instance value in both error messages. Update every call site to pass the
relevant instance identifier while preserving the existing exception types and
messages for the writer class context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 97ec5176-8970-4864-9e53-67270bf299e6
📒 Files selected for processing (2)
python/python/tests/test_blob.pypython/src/blob.rs
## Summary - make the Python `PackedBlobWriter` and `DedicatedBlobWriter` wrappers safe to release on a thread other than the one that created them - replace PyO3's `unsendable` restriction with mutex-protected writer ownership - add a production-shaped regression test covering both writer types ## Motivation Geneva's Blob v2 checkpoint path prepares packed blob sidecars on a dedicated checkpoint-writer thread. During a large Azure benchmark, Azure returned `503 ServerBusy` from a multipart block `PUT`. The checkpoint thread propagated that object-store exception to the actor's owner thread. The original exception retained its Python traceback. That traceback retained the checkpoint frame and its local `PackedBlobWriter`, so the last reference to the writer was eventually released by the owner thread rather than the checkpoint thread. Because the binding declared the writer as `#[pyclass(unsendable)]`, PyO3 emitted a second unraisable exception: ```text RuntimeError: lance::blob::PyPackedBlobWriter is unsendable, but is being dropped on another thread ``` This secondary error appeared at unrelated Python stack locations, obscured the original Azure failure, and prevented normal RAII cleanup of the writer on that path. The same ownership problem applies to `DedicatedBlobWriter`, which used the same binding policy. ## Root cause The core Rust blob writers are `Send`, so transferring ownership for destruction is safe. They are not `Sync`, however, because the underlying `dyn Writer` is not shareable for concurrent access. PyO3 0.28 requires ordinary Python classes to be both `Send` and `Sync`; simply removing `unsendable` therefore does not compile. ## Fix Store each core writer as `Mutex<Option<Writer>>` and remove the `unsendable` annotation. The mutex makes the Python wrapper `Send + Sync` while retaining exclusive access to the non-`Sync` writer. Existing mutation and finish semantics remain unchanged: - completed writers still raise the existing `ValueError` - writer ownership is still consumed by `finish` - failed bulk writes still drop their active writer through RAII - a poisoned lock surfaces as a descriptive Python `RuntimeError` There is no public Python API change. ## Regression coverage The new parameterized test models the production lifetime directly: 1. create a packed or dedicated writer on a worker thread 2. raise and hand an exception containing that thread's traceback to the owner thread 3. release the exception and force garbage collection on the owner thread 4. assert that `sys.unraisablehook` receives no cross-thread destruction error The test fails on `main` with the same `PyPackedBlobWriter is unsendable` / `PyDedicatedBlobWriter is unsendable` messages and passes with this change. ## Validation - `uv run make build` - `uv run pytest -q python/tests/test_blob.py -k 'packed_blob_writer or failed_blob_writer'` — 32 passed - `uv run pytest -q python/tests/test_blob.py` — 139 passed - `uv run make lint` — Ruff, Pyright (0 errors), Rust formatting, and Clippy passed - commit hooks — Ruff, Ruff format, Rust formatting, and typos passed (cherry picked from commit 4f86078)
Summary
PackedBlobWriterandDedicatedBlobWriterwrappers safe to release on a thread other than the one that created themunsendablerestriction with mutex-protected writer ownershipMotivation
Geneva's Blob v2 checkpoint path prepares packed blob sidecars on a dedicated checkpoint-writer thread. During a large Azure benchmark, Azure returned
503 ServerBusyfrom a multipart blockPUT. The checkpoint thread propagated that object-store exception to the actor's owner thread.The original exception retained its Python traceback. That traceback retained the checkpoint frame and its local
PackedBlobWriter, so the last reference to the writer was eventually released by the owner thread rather than the checkpoint thread. Because the binding declared the writer as#[pyclass(unsendable)], PyO3 emitted a second unraisable exception:This secondary error appeared at unrelated Python stack locations, obscured the original Azure failure, and prevented normal RAII cleanup of the writer on that path. The same ownership problem applies to
DedicatedBlobWriter, which used the same binding policy.Root cause
The core Rust blob writers are
Send, so transferring ownership for destruction is safe. They are notSync, however, because the underlyingdyn Writeris not shareable for concurrent access. PyO3 0.28 requires ordinary Python classes to be bothSendandSync; simply removingunsendabletherefore does not compile.Fix
Store each core writer as
Mutex<Option<Writer>>and remove theunsendableannotation. The mutex makes the Python wrapperSend + Syncwhile retaining exclusive access to the non-Syncwriter. Existing mutation and finish semantics remain unchanged:ValueErrorfinishRuntimeErrorThere is no public Python API change.
Regression coverage
The new parameterized test models the production lifetime directly:
sys.unraisablehookreceives no cross-thread destruction errorThe test fails on
mainwith the samePyPackedBlobWriter is unsendable/PyDedicatedBlobWriter is unsendablemessages and passes with this change.Validation
uv run make builduv run pytest -q python/tests/test_blob.py -k 'packed_blob_writer or failed_blob_writer'— 32 passeduv run pytest -q python/tests/test_blob.py— 139 passeduv run make lint— Ruff, Pyright (0 errors), Rust formatting, and Clippy passed