Skip to content

Use recursive mutex to deal with GIL <-> internal lock deadlocks - #462

Merged
evertlammerts merged 1 commit into
duckdb:v1.5-variegatafrom
evertlammerts:fix_435_456
May 19, 2026
Merged

evertlammerts merged 1 commit into
duckdb:v1.5-variegatafrom
evertlammerts:fix_435_456

Conversation

@evertlammerts

Copy link
Copy Markdown
Member

Related to #435 and #456

#435 tries to use the same connection across multiple threads, which is not supported. Specifically, in this case the result is overwritten and destroyed concurrently, causing a segfault. This PR fixes the segfault by synchronising access to the result with a recursive mutex. But note that connections cannot be used across threads, no matter what.

#456: we sometimes need to destruct python-managed objects after releasing the GIL. This PR makes sure that we hold on to the GIL during the destruction of PyRelation and Connection::Close(). Note: I rely on CI to check whether the fix actually works here...

@evertlammerts
evertlammerts merged commit fd8889e into duckdb:v1.5-variegata May 19, 2026
15 checks passed
@evertlammerts
evertlammerts deleted the fix_435_456 branch May 19, 2026 10:17
kaka11chen added a commit to AstroVela/lance-duckdb that referenced this pull request Nov 8, 2026
Windows drive paths such as `C:\...` and `D:/...` were parsed as
non-file URI schemes in mutation workers. They consequently reopened the
dataset through Lance's direct local writer, bypassing the object-store
wrapper that tracks mutation files. UPDATE replacements and
partial-DELETE files could be written and then rejected by transaction
artifact validation.

Use upstream `uri_to_url()` and OS-aware `to_file_path()` before
selecting `file-object-store`. Resolve the C++ error-redaction policy
before the mutation FFI call so path normalization cannot erase its Rust
error. Keep artifact validation and diagnostic redaction in place.

Regression coverage now includes local paths/file URIs with spaces,
Unicode and literal percent escapes, plus the existing remote URI
behavior. The existing Windows/macOS qualification script now exercises
UPDATE, whole-fragment and partial DELETE, and full-table mutations in
both local and Ray modes. The failed-UPDATE test requires the underlying
nullability error rather than accepting only the generic transaction
prefix.

### Validation

- `cargo test --locked --offline --lib --features vane-distributed
ffi::distributed_mutation::tests`: **5 passed**; compiled the changed
Rust source.
- C++ syntax compilation of `src/lance_distributed_write.cpp` with the
released Vane 0.3.0 SDK headers and `LANCE_VANE_DISTRIBUTED`:
**passed**.
- `python -m pytest -q python/tests/test_vane_platform_release.py`: **23
passed**.
- `uv run make format`, `cargo fmt --all --check`, Black, Python syntax
checks and `git diff --check`: **passed**.
- Updated qualification script: **local and Ray passed on Linux using
the existing signed 0.3.0/0.3.0.1 wheels**. This checks the expanded
script, not a newly linked provider. The Ray run used one node/two CPUs;
inspection of 38 Ray log files found no mutation error or Python fatal.

Windows verification of the rebuilt provider and the end-to-end
error-detail regression remain pending. No TestPyPI/PyPI publication is
part of this PR.

### Windows verification

Use the matched Windows base/provider wheels built for this PR (the
`vane-lance-full-windows` CI artifact), in a fresh CPython 3.12 x64
environment. The already published `0.3.0.1` wheel and the old test
kit's pinned `install` step still select the old native code.

From this PR checkout, after installing those CI wheels:

```powershell
$env:VANE_EXPECTED_EXTENSION_TRUST_IDENTITY = "vane-ci-test-key"
$env:VANE_DISTRIBUTED_NODE_COUNT = "1"
$env:VANE_DISTRIBUTED_WORKER_SLOTS = "2"
python scripts/verify_vane_platform.py --runner local --directory C:/vane-pr-test/local
python scripts/verify_vane_platform.py --runner ray --directory C:/vane-pr-test/ray
```

Use fresh output directories. The script verifies signed loading, exact
mutated rows and the empty result after full DELETE. Inspect actor
stderr as well as command exit status.

The separate GIL abort is tracked in
[AstroVela/vane#1146](AstroVela/vane#1146),
with the related upstream
[duckdb-python#456](duckdb/duckdb-python#456)
and [#462](duckdb/duckdb-python#462). That
crash's exact native caller still needs a Windows stack; this mutation
fix does not claim to fix it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant