Skip to content

fix(python): allow blob writer cleanup across threads#7827

Merged
wjones127 merged 2 commits into
lance-format:mainfrom
justinrmiller:codex/fix-packed-blob-writer-thread-drop
Jul 17, 2026
Merged

fix(python): allow blob writer cleanup across threads#7827
wjones127 merged 2 commits into
lance-format:mainfrom
justinrmiller:codex/fix-packed-blob-writer-thread-drop

Conversation

@justinrmiller

Copy link
Copy Markdown
Contributor

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:

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

@github-actions github-actions Bot added the A-python Python bindings label Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Blob 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.

Changes

Blob writer state management

Layer / File(s) Summary
Shared writer-state helpers
python/src/blob.rs
Writer access is centralized through mutex-locking helpers that handle completed writers and poisoned locks.
Packed writer synchronization
python/src/blob.rs
PyPackedBlobWriter uses mutex-backed state for accessors, writes, finishing, and failure cleanup.
Dedicated writer synchronization and regression coverage
python/src/blob.rs, python/python/tests/test_blob.py
PyDedicatedBlobWriter adopts the shared state flow, and a threaded parametrized test validates exception cleanup for both writer constructors.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: xuanwo, bubblecal

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: enabling blob writer cleanup across threads.
Description check ✅ Passed The description is directly related to the changeset and explains the thread-safe writer cleanup fix and regression test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Jul 17, 2026
@justinrmiller
justinrmiller marked this pull request as ready for review July 17, 2026 00:45

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Preserve 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 | 🔵 Trivial

Error messages lack per-instance context.

finished_writer/poisoned_writer only interpolate the static class name (writer_name), never the blob's path/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

📥 Commits

Reviewing files that changed from the base of the PR and between f868f51 and 423c194.

📒 Files selected for processing (2)
  • python/python/tests/test_blob.py
  • python/src/blob.rs

@wjones127 wjones127 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@wjones127
wjones127 merged commit 4f86078 into lance-format:main Jul 17, 2026
15 checks passed
wjones127 pushed a commit that referenced this pull request Jul 21, 2026
## 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-python Python bindings bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants