[giswqs]

Root cause

The Shapefile index cache (#668) guards entries and its directory with flock. A flock belongs to the open file description and is released only when every descriptor sharing it is closed. When another thread spawns a process (the cache child-process tests here; GDAL, DuckDB or a plugin in the app), the child holds duplicates of all of the parent's descriptors until it execs. Dropping a File during that window left the lease or directory lock held, so:

• a release found the directory "busy" and deferred cleanup to the retry worker, and tests asserting the file was gone failed (cache_concurrent_builder_uses_private_index_without_replacing_reader),
• a sweep counted a released entry as pinned, so maintain(root, 0) left bytes behind (damaged_shapefiles_are_errors_or_skips_not_panics, cache_shared_readers_and_oversized_indexes_are_protected_until_release, cache_lru_touches_reopened_indexes_and_evicts_oldest_idle),
• reserve_index dropped a lease lock and immediately re-took it with try_lock, which met WouldBlock (ready at tests.rs:49 in cache_unix_release_does_not_mutate_a_root_that_became_unsafe and others).

The retry worker and the thread-local test root were not the cause: the worker only maintains roots it is handed and serializes on the directory lock, and failures vanished when the test ran alone.

Fix

Lock files are wrapped in a small Flock guard that calls unlock() on drop. Unlocking acts on the open file description, so it releases the lock even while a spawned child still holds a duplicate, and it is a no-op for a lock never taken. Leases, the directory lock and the sweep's transient locks all use it. No test assertions were loosened.

A regression test, cache_releases_unlock_even_while_a_spawned_process_holds_a_duplicate, holds a try_clone duplicate (what a forked child holds) across a temporary release, a ready-index release followed by re-reservation, and a sweep. It fails on main and passes with the fix.

Failure rates (same machine, before = main, after = this branch)

Run	Before	After
source:: on 2 CPUs, 80 runs	16/80 (all damaged_shapefiles...)	0/80
source::shp::cache:: on 2 CPUs, 32 threads, 300 runs	5/300 (four different cache tests)	0/300
source::shp::cache:: on 1 CPU, 16 threads, 200 runs	1/200 (ready WouldBlock)	0/200

Fixes #687

Summary by CodeRabbit

• Bug Fixes
  • Cache indexes can now be released and reused reliably, even when another process still holds a duplicate lock.
  • Cache cleanup no longer remains blocked by a duplicate directory lock after a sweep ends.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 814fdb43-c6d2-4fb4-be81-79ee225721b3

📥 Commits

Reviewing files that changed from the base of the PR and between 94bfaeb and e7bfa42.

📒 Files selected for processing: 2

• crates/data/src/source/shp/cache.rs
• crates/data/src/source/shp/cache/tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Walkthrough

Cache lock files now use Flock, which explicitly unlocks on drop. A regression test checks that lease and directory locks are released while cloned descriptors remain open.

Changes: Cache lock release

Layer / File(s)	Summary
Lock wrapper and release behavior crates/data/src/source/shp/cache.rs, crates/data/src/source/shp/cache/tests.rs	Lease and directory lock files use Flock instead of plain File. The regression test checks lease cleanup, ready-index reuse, and directory-lock release while cloned descriptors remain open.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related issues

• Issue 687 reports intermittent Shapefile cache test failures involving deferred cleanup and held directory locks. This change adds explicit unlock-on-drop behavior and a regression test for cloned descriptors.

Suggested reviewers: yongyin-leon

Merge Risk: ⚪ Minimal · up to e7bfa

The cache lock change appears ready to merge after normal checks; no unresolved release-path issue was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the Shapefile cache change and its purpose of fixing flaky cache tests.
Description check		The description explains the root cause, fix, regression test, measured results, and related issue. It does not use the template headings exactly, but it provides the required summary, testing details…
Linked Issues check		Issue [#687] requires consistent Shapefile cache behavior under parallel execution. The PR adds Flock, whose Drop calls unlock() for lease and directory lock files. The change covers temporary l…
Out of Scope Changes check		The changes are limited to Shapefile cache lock lifetime handling in crates/data/src/source/shp/cache.rs and a focused regression test in crates/data/src/source/shp/cache/tests.rs. The lock wrappe…

Full details: Description check

Explanation

The description explains the root cause, fix, regression test, measured results, and related issue. It does not use the template headings exactly, but it provides the required summary, testing details, and issue reference.

Full details: Linked Issues check

Explanation

Issue [#687] requires consistent Shapefile cache behavior under parallel execution. The PR adds Flock, whose Drop calls unlock() for lease and directory lock files. The change covers temporary leases, ready leases, and sweep locks. The regression test cache_releases_unlock_even_while_a_spawned_process_holds_a_duplicate verifies immediate temporary cleanup, ready-index re-reservation, and directory-lock release while a cloned descriptor remains open. The PR also reports zero failures in the listed repeated parallel test runs. The issue's suggested wait or retry approach is not a required implementation when the underlying descriptor-lock problem is fixed.

Full details: Out of Scope Changes check

Explanation

The changes are limited to Shapefile cache lock lifetime handling in crates/data/src/source/shp/cache.rs and a focused regression test in crates/data/src/source/shp/cache/tests.rs. The lock wrapper, lock propagation, and test coverage directly support issue [#687]. No unrelated product behavior or unrelated files are changed.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the locks at night
The lease lets go, though clones stay tight
The sweep unlocks its door with care
A ready index waits right there
Then hops away through cache-clear air}

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

[github-actions]

Code review

I found no bugs or security issues in this PR. It adds a Flock newtype in crates/data/src/source/shp/cache.rs. Dropping it calls File::unlock(), so a lease or directory lock is released even when a process spawned on another thread still holds a duplicate descriptor. The lease, directory lock and existing_lock paths now use it, and a new test covers all three.

I read the changed code against the lock call sites in cache.rs: remove_unpinned, create_locked, pin_index, reserve_index and prune_locked. I did not build or run anything, so the test is unrun.

Bugs

• None found.
  • Flock wraps a file that was opened separately each time, so unlocking a lock that was never taken cannot affect another holder. That holds on Unix flock and on Windows UnlockFileEx, and the error is ignored. Confidence: high.
  • The explicit unlock in Drop runs after the remove_file calls in remove_unpinned and reserve_index. The directory lock is held for the whole sweep, so this ordering is safe. Confidence: high.

Security

• None found. The symlink and regular-file checks in lease_file are unchanged.

Performance

• None found. unlock() adds one syscall per drop.

Quality

• Minor, low confidence: Flock implements Deref and DerefMut to File only so that existing .lock() and .try_lock() calls keep compiling. That is fine, but Flock is a generic name, and DirectoryLock or LeaseFile would say more. I'd leave it unless you want to rename it.
• The new test asserts on lease.lock.as_ref().unwrap(). That is allowed in test code.

AGENTS.md

• No violations.
  • There is no unwrap or unsafe in the non-test changes.
  • File::unlock needs Rust 1.89 or later, and the workspace rust-version is 1.90.
  • No assets, dependencies or translatable strings were touched.
  • This is a concurrency fix with a regression test.

Change size

Compared: the whole PR against main (merge base 94bfaeb to PR head e7bfa42)

Metric	Value
Files changed	2 (added 0, modified 2, removed 0, renamed 0)
Size before	39.7 KB
Size after	41.9 KB
Total size change	+2.2 KB

File	Before	After	Change
crates/data/src/source/shp/cache/tests.rs	22.0 KB	23.3 KB	+1.3 KB
crates/data/src/source/shp/cache.rs	17.7 KB	18.6 KB	+973 B

[github-actions]

📖 Docs preview

Item	Value
Preview	https://preview.rustgis.app/pr-704/
Web app	https://preview.rustgis.app/pr-704/app/
Commit	e7bfa42

GitHub Pages takes a minute or two to publish a new commit.