Repository navigation
fix(storage): preserve SQLite locks during pathname identity checks - #4720
Closed
ScriptedAlchemy wants to merge 5 commits into
Closed
ScriptedAlchemy wants to merge 5 commits into
ScriptedAlchemy wants to merge 5 commits into
Conversation
Use pathname metadata on Unix instead of opening and closing a raw file descriptor, which releases process-owned SQLite record locks. Keep the pinned identity and both HAS_MOVED-adjacent checks intact. Add cross-process rollback and WAL lock regressions, normal-release and integrity checks, and missing/replaced/alias identity coverage. Refs #4513
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ScriptedAlchemy
added a commit
that referenced
this pull request
Nov 3, 2026
Keep verified foreign SQLite readers together with their inode pins when moving into the shared host reader. Drop the SQLite connection before its raw pin, and verify that cloned host handles retain both descriptors until the final handle is released on Linux. Refs #4513; complements #4720 without addressing the writer pin lifecycle.
Replace ephemeral raw file opens in both SQLite reader and writer startup with a metadata-only identity check on Unix. Closing those descriptors while SQLite is live releases process-wide POSIX locks without VFS bookkeeping. Keep existing non-Unix identity handling and typed refusal paths. Add a subprocess regression proving an independent SQLite writer stays excluded across repeated worker identity checks, then commits intact. Refs #4513.
Apply the exact Bazel rustfmt_check_2024 diff reported in repository gates run 41701101003. Preserve the cross-process SQLite exclusion and pathname assertions unchanged.
This was referenced Nov 3, 2026
Owner
Author
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.
Root cause
On Unix, closing any raw descriptor for a database inode can release process-wide SQLite advisory locks, including those held by separate SQLite connections that cannot observe the close. The original pathname identity check opened and closed a temporary
Fileafter a SQLite connection was live. Further review found the same pattern in both reader and writer worker startup: each calledOpenedDatabaseFile::pin(...).identity()and immediately dropped the temporary pin after SQLite opened the database.This is the SQLite documented close()/POSIX lock corruption hazard. Refs #4513; this is a concrete partial fix, not resolution of the reported incident.
Fix
verify_current_pathidentity checks metadata-only, including both pathname checks aroundSQLITE_FCNTL_HAS_MOVED. The existing retained identity fence, attachment validation, cancellation and admission controls remain.live_path_identity, which usesstd::fs::metadataand the existing dev/inode identity on Unix. Preserve typed refusals for missing/nonfile paths and the existing non-Unix handle-based identity behavior.Regression and verification
Four Rust regression tests plus a subprocess helper now cover rollback and WAL writer/journal exclusion across identity checks; the newly added worker-startup regression checks cross-process writer exclusion through repeated
live_path_identitycalls, safe normal lock release, committed rows,integrity_check, and absent/nonfile paths. Other existing cases cover aliases and replacement refusal. The test child requires an execution marker, so zero-selected tests cannot pass silently.Executed independent native Linux Python 3.13.5 / SQLite 3.46.1 subprocess probes. A second-process writer remained blocked across metadata-only
stat; closing a separate raw descriptor while the original SQLite transaction was live incorrectly let the second process acquire the write lock. This validates the Unix mechanism, not the Rust implementation.Source-level verification: independently inspected the four-file follow-up diff, relevant worker/locator lifetime paths, existing test harness, and publication ancestry. The follow-up was committed as
d18cbd5on this PR's existing branch via a non-force expected-head update.Not executed: Bazel builds/tests, Rust compilation, rustfmt/Clippy, macOS/Windows validation or the live daemon's external
sqlite3reproduction. The execution environment has no Bazel or Rust toolchain. Hosted CI needs to run the isolated PR head, particularly:bazel test //crates/tracedecay-rusqlite-runtime:unit_test --test_output=errorsRemaining acceptance
Do not close #4513 from this PR. Other retained raw-descriptor clone/release/drop lifetimes and multi-connection teardown require further audit; the reported read-only external
sqlite3workflow requires realistic end-to-end concurrency/integrity testing with the repo's SQLite build and macOS coverage. The separate PR #4732 addresses a distinct verified foreign-reader pin lifetime. The PR-repair automation handles CI, conflict repair, and authorized merges.