Skip to content

fix(sqlite): retain attachment identity pins until workers close - #4736

Closed
ScriptedAlchemy wants to merge 2 commits into
masterfrom
fix/4513-attachment-pin-lifetime
Closed

ScriptedAlchemy wants to merge 2 commits into
masterfrom
fix/4513-attachment-pin-lifetime

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

Root cause

Refs #4513. Even after the other pathname-identity and verified-foreign-reader fixes, RepositoryPhysicalAttachmentFactory still dropped the original raw SQLite file pin after starting workers for existing read-write and read-only attachments. For newly created stores, commit_initialization dropped its pin while those SQLite connections were still alive. On Unix, closing any raw descriptor for the same inode can discard unrelated process-wide SQLite POSIX locks, risking corruption under concurrent access.

Fix

Keep the attachment's original verified descriptor in its physical owner until the SQLite readers and writer have shut down. For an initialized store, transfer the pending-initialization pin to retained ownership on commit; preserve abort_initialization's exact created-file cleanup authority. Release retained pins only after normal drain/close or after both lost-file worker groups have exited. Keep the retained pin last in state drop order so it cannot close ahead of worker owner destruction. No storage format, WAL policy, admission, authorization, or integrity control changes.

Regression

A Linux attachment regression exercises real SQLite-backed writable, read-only and newly initialized stores, requires a live retained descriptor throughout reads/initialization commit, and verifies it releases after worker shutdown. Existing lost-file, WAL, replacement and shutdown tests remain unchanged and cover their existing lifecycle boundaries.

Verification and scope

Reviewed the attach_opened, attach_read_only, commit_initialization, lost-file and shutdown paths on the current master, and confirmed PR #4720 and PR #4732 change different files. The patch is one Rust source file with a focused regression and no whitespace errors. Rust/Bazel compilation and test execution have not run in this automation environment; CI must execute bazel test //crates/tracedecay-rusqlite-runtime:unit_test --test_output=errors on this exact head, plus the broader macOS concurrency matrix before merge. This is source-level evidence of a further lock-lifetime hazard, not proof that the reported live external-sqlite3 corruption scenario is fully resolved. Keep #4513 open until integrated macOS/Unix stress and integrity verification. The separate PR-repair automation owns CI follow-up and merges.


Devin Review

@changeset-bot

changeset-bot Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6355f7b

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: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

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