Skip to content

fix(sqlite): keep verified foreign-file pins until reader close - #4732

Closed
ScriptedAlchemy wants to merge 1 commit into
masterfrom
fix/4513-verified-reader-pin-lifetime
Closed

ScriptedAlchemy wants to merge 1 commit into
masterfrom
fix/4513-verified-reader-pin-lifetime

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

Root cause

Refs #4513; this is a separate lock-lifetime defect from #4720.

open_verified_immutable_reader binds a SQLite connection to a pinned database inode, but Cursor's composer reader immediately called VerifiedReader::into_parts(). That discarded the raw file pin while the extracted SQLite connection stayed live inside SqliteReadConn. On Unix, closing any file descriptor for a SQLite database inode may release the process's advisory record locks, including locks owned by another connection. The verified reader's ordinary field drop order also released the pin before closing its own SQLite connection.

Change

  • Keep the complete verified foreign reader inside the existing shared host reader instead of decomposing it into a bare connection. Plain Hermes SQLite connections continue using the same host reader without a second registry or runtime.
  • Expose the already-verified canonical path through a read-only getter and delete into_parts so the original early-drop route cannot recur.
  • Order the verified reader's fields so the SQLite connection closes before its raw inode pin.
  • Add a Linux behavioral regression that counts real descriptors for the target inode through verified open, conversion into the host reader, handle cloning, querying, and final close. Both the SQLite descriptor and the retained pin must survive until the last handle closes.

No change to SQLite configuration, database contents, admission checks, authorization, receipts, or filesystem cleanup.

Validation

  • Source-reviewed the full three-file change and its only VerifiedReader::into_parts caller; the published commit changes those three files only.
  • A separate native Linux Python/SQLite subprocess mechanism probe reproduced advisory lock loss: a second process was blocked by an active BEGIN IMMEDIATE before an unrelated raw database descriptor was opened/closed, then was admitted after the close. This establishes the OS mechanism, not a pass for the Rust regression.
  • Not run: Bazel, Rust compile, rustfmt, or the new Rust unit regression. The task environment has neither Bazel nor a Rust toolchain nor a supplied checkout. Hosted CI must execute //crates/tracedecay-sessions:unit_test (including the new Linux descriptor-lifetime regression), //crates/tracedecay-rusqlite-runtime:unit_test, and the Cursor composer integration journeys on the exact PR head.

Remaining scope

This fixes the verified foreign reader's premature pin drop. #4720 independently addresses opening/closing a temporary raw descriptor during pathname identity verification. Other shared writer/reader pin clone and explicit lost-file release lifetimes require separate auditing; this PR does not demonstrate that the reported live sessions.db + external sqlite3 read workflow is corruption-safe. Keep #4513 open pending actual SQLite-backed stress/integrity coverage, especially on macOS. The existing PR repair/merge automation owns CI follow-up and authorized merges.


Devin Review

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.
@changeset-bot

changeset-bot Bot commented Nov 3, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 5596273

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@devin-ai-integration devin-ai-integration 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.

🔍 Devin Review: 1 flag

Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

Consolidated into #4742 (integrate/open-pull-requests) at the captured head shown in its description, as requested. Continue review and further fixes on #4742. This closure does not assert CI readiness; the replacement remains draft. Source branch retained.

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