Repository navigation
fix(sqlite): publish complete stores and recover interrupted initialization - #4805
ScriptedAlchemy wants to merge 24 commits into
Conversation
|
|
Review/validation hold: This PR is marked ready solely so the existing PR CI can execute on its exact head. Please do not merge until nonzero hosted Bazel tests, native macOS/Windows SQLite sidecar and locking behavior, and the preserved-failure/retry tradeoff are reviewed. The default PR workflow skips native macOS; a skipped check is not platform acceptance. The cross-process replacement and initial sidecar refusal tests are intended to be exercised by |
|
Refs #4513 — follow-up fix published on this same issue-specific PR at New data-loss path on retry: Patch: both Independent native SQLite external read-only subprocess / concurrent writer probes also passed 32 nonzero Linux assertions, 8 each in rollback and WAL modes (reader result and quick_check). Neither those probes nor this review count as repository Bazel, hosted CI, native macOS or Windows qualification. New-head hosted CI must run nonzero Not complete acceptance: an initialization that got far enough to write a valid database header can still leave an incomplete family, and post-admission POSIX pathname replacement lacks a stronger private ownership/VFS fence. Daemon-level external |
Refs #4513 Remove the stale discard_created call left after unsafe pathname cleanup was deleted. Abort now preserves the database and all sidecars, releases shared descriptors only after normal worker teardown, and transfers a lost-file initialization pin to the existing deferred worker cleanup. Reject initialization publication once drain or close has begun. Add real runtime regressions for abort/read-back/stale handles, separate- process replacement of the database and WAL/SHM/journal after worker close, idempotent abort, and a lost writer held across initialization abort. These Rust tests still require nonzero Bazel and native macOS execution; no Cargo or direct Rust invocation was used.
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Refs #4513 — code review of exact head 1738c8e1de983e062053fb2544c62f3a9c222300.
Three actionable safety findings are attached inline: (1) invalid-header rejection closes an independent raw database descriptor and can release another local SQLite connection's POSIX locks; (2) the SQLite magic does not establish completed initialization or authorize reusing a preserved sidecar family; (3) sidecar reservations do not fence a replacement between reservation and SQLite's first access.
I reproduced the mechanisms in disposable Linux fixtures with native SQLite 3.46.1: the rejected-header/raw-close schedule changed an independent subprocess writer from SQLITE_BUSY to admitted before the original transaction ended; a separate-process post-reservation replacement lost sentinel bytes for each of WAL/SHM/journal; and a header-bearing but otherwise uninitialized WAL database accepted a retry that overwrote/deleted each sentinel. These are standalone mechanism reproductions, NOT execution of this repository's Rust/Bazel tests, and not macOS/Windows qualification.
Keep the removal of unsafe pathname-based cleanup. The next change needs production-path regressions through attachment startup/failure/retry, rather than only calling repository_start_failure directly. Require nonzero Bazel results on the resulting exact head, plus native macOS locking/sidecar coverage and relevant Windows coverage. This review is not an approval or a claim of master completion. Keep #4513 open; ordinary merging remains with the PR-maintenance owner. Do not merge or modify held draft #4776 or release #4763.
| } | ||
| })??; | ||
| if !initialized { | ||
| return Err(OpenedDatabaseFileError::InvalidHeader); |
There was a problem hiding this comment.
[P1] The invalid-header refusal itself can discard another connection's SQLite locks.
Self::pin(path) above opens a new raw File; returning here drops it. The ?? read-error exits do the same. Sharing that newly opened descriptor with this attachment's future workers does not protect a different, already-live SQLite connection in the same process.
Concrete standalone Linux/SQLite 3.46.1 reproduction: open an empty database, hold BEGIN IMMEDIATE (the main file is still zero bytes), and confirm a separate-process writer gets SQLITE_BUSY. Read the header through a second raw descriptor and close it as this refusal does. A fresh subprocess can now enter BEGIN IMMEDIATE before the original transaction rolls back. This reintroduces the lock-loss mechanism #4513 is intended to remove.
Make rejected admission use a lock-safe ownership/lifetime path as well; reading through a newly acquired pin is not sufficient. Add a Rust regression that actually calls pin_initialized while an independent connection holds that transaction and checks cross-process exclusion both before and after refusal. Cover error teardown without leaking descriptors indefinitely, and run it natively on macOS as well as Linux.
| let mut header = [0_u8; 16]; | ||
| match io::Read::read_exact(&mut reader, &mut header) { | ||
| Ok(()) => Ok(header == *b"SQLite format 3\0"), | ||
| Err(error) if error.kind() == io::ErrorKind::UnexpectedEof => Ok(false), |
There was a problem hiding this comment.
[P1] SQLite magic is not proof that the preserved initialization completed.
This admits a database after PRAGMA journal_mode=WAL has written its header but before application initialization completes. On a failed start/retry, there is still no proof that the current WAL/SHM/journal entries belong to this attachment. The new retry regression only exercises an empty main file, so it cannot catch this path.
Standalone Linux/SQLite 3.46.1 reproduction: create a database using only PRAGMA journal_mode=WAL, close it before creating application schema, place a foreign sentinel at one sidecar name, then run the existing-mode WAL/open/write sequence. The 16-byte check passes and the sentinel is overwritten/deleted for each of -wal, -shm, and -journal.
Gate retry on authoritative initialization/namespace ownership or return a typed incomplete-initialization refusal before workers touch ambiguous sidecars. Merely checking more SQLite header bytes would not establish that ownership. Add a real factory failure after header creation followed by both writable and read-only retry, asserting foreign bytes survive and a normally initialized store still reopens.
| .map_err(|_| OpenedDatabaseFileError::Reserve)?; | ||
| let journal = reserve_new_sidecar(&sidecar_path(path, "-journal")) | ||
| .map_err(|_| OpenedDatabaseFileError::Reserve)?; | ||
| opened.created_sidecars = Some(Arc::new(Mutex::new(Some([wal, shm, journal])))); |
There was a problem hiding this comment.
[P1] Retained reservation descriptors do not protect the sidecar directory entries used by SQLite.
After these O_EXCL creations succeed, another process can atomically replace just sessions.db-wal (or -shm/-journal) while leaving the main database unchanged. The retained handle still pins the old sidecar inode, but the worker opens the replacement by pathname; main-file identity/HAS_MOVED checks do not detect that sidecar-only substitution.
A disposable Linux/SQLite 3.46.1 probe reproduced foreign sentinel loss for all three suffixes after a separate subprocess performed exactly that replacement and before the first WAL initialization. Holding the reservation fds throughout did not prevent it.
The new cross-process test swaps files and calls repository_start_failure directly, so it proves the removal of explicit unlink cleanup, not that SQLite startup preserves replacements. Add the sidecar-only replacement schedule through actual attachment startup, including failure/retry. Establish an ownership boundary that remains valid during SQLite's sidecar operations; another pre-open stat/recheck or an advisory lock ignored by the replacing process is not a race-safe fix.
Apply the exact rustfmt changes reported by hosted Bazel Repository gates without touching SQLite behavior or other PR files. Refs #4513
|
Refs #4513 — repaired the exact four formatting differences reported by the failed Repository gates job at the peer's head |
Avoid opening a raw database descriptor when an existing attachment cannot contain the SQLite header. Closing that descriptor could unlock another live SQLite writer; cover the refusal with a cross-process test. This does not fix header-bearing incomplete stores or sidecar swaps. Refs #4513
SQLite's minimum initialized database occupies one 512-byte page. Refuse header-bearing shorter files without opening a raw Unix handle; otherwise they bypass magic-only admission and can release live locks. Refs #4513
|
Refs #4513 — published narrow POSIX-lock preservation follow-ups on this PR:
Independent Linux system SQLite 3.46.1 probe confirmed the pre-fix raw-close lock loss and 5 nonzero metadata-only mitigation assertions; these are not repository Rust/Bazel passes. Hosted exact-head Bazel Scope is deliberately partial: this does not make invalid-header refusal safe for files >=512 bytes; a raw pin still closes on those errors. It also does not prove a header-bearing store completed initialization or protect the sidecar names against noncooperating POSIX replacement after reservation. Keep the earlier three P1 review threads open and PR on review hold. Require an ownership-safe lifecycle and daemon+external |
Existing-mode admission must not open sidecars for databases whose file size cannot be a whole number of SQLite pages. Refuse those paths using metadata before opening a descriptor, preserving foreign journal entries and avoiding POSIX lock-loss on the refusal path. Add production attachment regressions for truncated header-bearing files and WAL, SHM, and journal sentinel preservation. Refs #4513
|
Refs #4513 — published Additional defensive admission fix: an existing SQLite DB must consist of complete pages; every legal SQLite page size is a multiple of 512 bytes. The prior >=512-byte check admitted header-bearing files of size 513 or 1025. Those could still reach the SQLite worker and allow it to delete or change unrelated journal/SHM sidecars despite the database being incomplete. Admission now rejects non-512-aligned lengths using filesystem metadata before opening a raw descriptor (avoiding an additional POSIX close on this refusal path), on all supported platforms. Rust regressions added: direct existing-header tests for 513/1023/1025 bytes, plus a test through real Independent Linux SQLite 3.46.1 mechanism probe (NOT Bazel/Rust tests): of 6 unprotected invalid-page-length cases (2 lengths x 3 suffixes), 2 journal sentinels were deleted, and 6 metadata-only refusal cases preserved all original sentinels. The probe did not demonstrate every suffix is destroyed in this specific invalid-header schedule; it did confirm a real journal-loss path. Still outstanding: this is only a necessary structural guard, not proof of completed initialization or sidecar ownership. Page-aligned but invalid or partially initialized DBs, a header-read failure after a fresh raw pin, and post-reservation pathname replacement remain unresolved. Native macOS/Windows, full external |
Match the pinned Bazel rustfmt_check_2024 output for the refused empty-attachment writer-lock regression without changing its behavior. Refs #4513
|
Refs #4513 — fixed the exact-head hosted Bazel Repository gates rustfmt failure on #4805 without changing behavior. Commit Independent disposable Linux SQLite 3.46.1 checks this run reproduced the successful-init overwrite of pre-existing The existing remaining holds are unchanged: full-page malformed/retry lock safety, complete initialization/sidecar ownership, post-reservation substitution, ordinary external |
|
Refs #4513 — published |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Refs #4513 — issue-specific follow-up published on #4805 (non-force, expected-old-head fast-forwards retaining prior and peer commits): Specific fix: previously a page-aligned malformed database with a bad SQLite header could be opened via Independent disposable Linux SQLite 3.46.1 mechanism checks passed: 6 malformed aligned-header/sidecar preservations and 10 external writer-lock/immutable-probe assertions, plus a separate direct POSIX-fcntl schedule demonstrating exclusion survives SQLite immutable refusal whereas an independent raw open/close releases it. These checks are NOT the Rust regressions, repository Bazel, or native macOS results. Validation hold: exact-head CI run 42465671210 was queued when checked. Require nonzero |
|
Refs #4513 — published issue-specific formatting repair The preceding exact-head CI run 42465671210 failed Repository gates at Keep #4513 open; ordinary PR merging is owned by the existing maintenance lane. Held draft #4776 and release #4763 are untouched. |
|
Refs #4513 — published focused cross-process lock-lifetime regression The new test covers the previously untested successful A disposable Linux system-SQLite 3.46.1 cross-process analog independently returned This is a regression guard, not full repair of sidecar post-reservation replacement or partial initialization recovery. Keep #4513 open and preserve the separate #4803 and held draft #4776 boundaries. Ordinary merge remains with the existing PR-maintenance lane. |
|
Refs #4513 — published |
Existing fixture files created by a bare SQLite open remain zero bytes; they are not completed SQLite stores and must be refused by admission. Use an empty-schema SQLite header instead, preserving fixture intent and the incomplete-initialization safety guard. Refs #4513
|
Refs #4513 — published issue-scoped fixture compatibility repair New hosted Bazel findings (older head This commit materializes a real, page-aligned SQLite header with Open acceptance holds: live daemon + external |
|
Refs #4513 — new reproducible existing-store admission data-loss case, distinct from the new-basename O_EXCL path, plus a deterministic post-reservation replacement schedule. This is a blocker, not a claimed fix. On the current #4805 source head Disposable real-filesystem/native SQLite 3.46.1 reproduction, 3/3 sidecar suffixes: (1) create Separate post-reservation schedule, 3/3 suffixes: with O_EXCL-open descriptors retained for the main file and all three sidecars (the intended #4805 design), rename one reserved sidecar aside, substitute a foreign nonempty sentinel at its original name, then start the SQLite WAL writer. All three writes/commits succeeded; each substituted sentinel disappeared, while the original reserved inode remained held under its renamed path. Hence retained O_EXCL descriptors do not by themselves fence noncooperating POSIX pathname replacement after admission. These are independent Python/native SQLite mechanism proofs, NOT execution of the TraceDecay Rust factory, Bazel, CI, or native macOS/Windows. Required source repair is an actual basename/sidecar provenance and open-time ownership boundary for existing as well as newly initialized stores, with an ownership-preserving VFS or private namespace policy for post-reservation replacements, and real factory/process regressions. Magic/length/schema probing alone, additional path stats, or unlink-by-path do not establish that authority. Keep recovery fail-closed rather than overwrite foreign bytes. The currently queued exact-head CI cannot establish this missing property; native macOS and isolated-daemon+external- Cross-process confirmation (same SQLite 3.46.1): repeated the post-reservation schedule with the replacer in a separate OS process while the parent held all four O_EXCL descriptors. For each suffix, the child renamed the original reserved inode and created foreign sentinel bytes at the original name; the parent proved the held fd still identified the renamed original, then initialized/committed through native SQLite. All 3/3 substituted sentinels disappeared, all 3/3 original inodes remained pinned under their new names, and all 3/3 committed rows survived. This is a deterministic cross-process standalone Python/SQLite reproduction of the uncooperative rename window, not execution of Rust or Bazel and not a macOS result. |
Require the new typed invalid-header refusal for malformed graph stores. The isolated missing-database retry fixture now removes its own closed SQLite sidecars, so it models an absent basename without relaxing the production refusal of unknown WAL, SHM or journal files. Refs #4513
|
Refs #4513 — two issue-scoped, test-only commits published on this PR by exact-old-head nonforce fast-forwards; verified each remote update is 1 commit ahead/0 behind its prior head, preserving concurrent changes.
Exact-old-head hosted Bazel findings (run 42496503283, SHA Separate Linux native Python/SQLite 3.46.1 probe on disposable files passed 15 nonzero checks, including initialized empty-schema pages, cross-process external read-only committed visibility and writer lock exclusion, plus the known dangerous existing initialized database + preexisting foreign WAL/SHM/journal overwrite on all 3 suffixes. Those are standalone mechanism probes, not Rust/Bazel or macOS tests. Remaining high-priority safety blockers (unchanged): |
Refs #4513. Add a non-vacuous cross-process rollback/WAL regression that admits a page-initialized live store, checks a separate process cannot steal BEGIN IMMEDIATE, confirms read-only committed visibility, then verifies final integrity after proper pin release.
|
Refs #4513 — published This adds a non-vacuous Rust cross-process regression for the actual An independent disposable Linux SQLite 3.46.1/Python cross-process analogue passed 16 nonzero behavioral assertions (writer exclusion, immutable schema read, committed visibility and integrity across DELETE/WAL); this is not this repository Rust/Bazel test execution. Exact-head hosted Bazel CI is pending. Native macOS remains unqualified and ordinary PR CI skips its opt-in job. Partial safety coverage only, not issue acceptance: existing-store foreign sidecar provenance, post-reservation POSIX pathname replacement, safe incomplete-store recovery, exact daemon/external |
Refs #4513. Reject a nonregular store before opening its FIFO, without letting the test's fallback writer block after that early refusal. Preserve the original concurrent-open assertion if admission reaches its blocked FIFO read path.
|
Refs #4513 — published an issue-scoped CI regression repair on the existing separate #4805 branch: Actionable hosted Bazel failure on prior head Other failures in the same old-head CI are still not qualified as caused by this patch: Independent disposable Linux SQLite 3.46.1 checks this run passed 14/14 assertions for separate-process read-only committed snapshots and write exclusion, and independently reproduced preexisting foreign WAL/SHM/journal overwrite on an initialized main DB (all 3 suffixes). These are native SQLite/Python probes, not the daemon + external |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Refs #4513. Closing an O_RDONLY identity pin can release a different SQLite writer's POSIX byte locks. Pin existing stores with O_PATH on Linux/Android so the /proc/self/fd worker route retains inode identity without that lock-loss side effect. Exercise pin release against a live separate-process write contender in rollback and WAL modes.
|
Refs #4513 — published Additional real POSIX lock-loss found and narrowly repaired on Linux/Android: retaining an Independent Linux SQLite 3.46.1 proof, not repository Bazel: 22 nonzero assertions across DELETE/WAL and Exact-head hosted CI is pending. No Rust/Bazel pass on this new head is claimed. Previous-head run 42514311881 had 182/183 Linux Bazel test targets passing, with unrelated Still open: foreign sidecar provenance for existing initialized databases, adversarial post-reservation namespace replacement, incomplete-store reconciliation, exact daemon + external |
Repair three hosted automation_runner_test failures by supplying the configured executable identity required by the production prefilter. Use the same identity for the prefilter and retained runner, preserving manual-schedule, exact-ledger, live-lock and lock-race assertions. Add a production-prefilter regression for unconfigured refusal, deduplication and same-occurrence recovery after binding, with zero backend calls and no effectful terminal. Do not relax backend admission, change timeouts, or alter the graph publication changes. Refs #4532
| if !initialized { | ||
| for suffix in ["-wal", "-shm", "-journal"] { | ||
| match std::fs::symlink_metadata(sidecar_path(path, suffix)) { | ||
| Ok(_) => return Err(OpenedDatabaseFileError::IncompleteInitialization), | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => {} | ||
| Err(_) => return Err(OpenedDatabaseFileError::Inspect), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Uncheckpointed WAL stores cannot reopen
When a store's first schema is committed only to WAL, pin_initialized rejects its valid main file as incomplete. The immutable inspection cannot see committed WAL tables, so existing read-only and writable attachments both fail.
Learn more
A SQLite WAL database can contain a valid committed schema in -wal while its main file still contains no application tables. pin_initialized inspects only the main file through open_immutable_reader, then treats the presence of any sidecar as proof of an incomplete initialization. Both attach and attach_read_only call this check before opening SQLite normally. This rejects a valid database with an uncheckpointed first schema, including the fixture exercised by initialized_wal_with_uncheckpointed_schema_remains_attachable.
Example: Create sessions.db in WAL mode, create probe and insert 42, then keep the writer open without checkpointing. The main file lacks probe, while sessions.db-wal contains it; an existing attachment returns IncompleteInitialization instead of reading 42.
Recommended fix: Distinguish an interrupted new-basename reservation from a legitimate existing WAL family using an ownership marker or publication witness, without treating an uncheckpointed main-file schema as an incomplete database or opening unowned sidecars for a genuinely incomplete store.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for suffix in SIDECARS { | ||
| remove_if_present(&self.witness(suffix))?; | ||
| } | ||
| remove_if_present(&self.transferred())?; | ||
| sync_directory(&self.directory, SYNC) |
There was a problem hiding this comment.
🟡 Completed stores retain staging directories
After a successful publication, finish_publication leaves its lock file and .tracedecay-init directory beside every store. Temporary staging remains after the operation finishes, contrary to the repository's staging-cleanup rule.
Learn more
Initialization uses a private directory for schema creation and crash-recovery witnesses. After a successful handoff, finish_publication removes the witnesses and transferred link, but retains schema-installation.lock and its parent directory indefinitely. The repository's AGENTS.md requires temporary staging to be removed when the operation finishes, not left beside each initialized store.
Example: Initialize sessions.db successfully. Its database is published and the witnesses are gone, but sessions.db.tracedecay-init/schema-installation.lock persists across restarts.
Recommended fix: Retire the empty staging directory and its lock after successful publication while respecting live leases and avoiding pathname-based deletion of a replaced directory. Keep crash recovery available only until the handoff finishes.
Was this helpful? React with 👍 or 👎 to provide feedback.
ScriptedAlchemy
changed the title
Refs #4513. Keep the issue open for the live-daemon endurance work and verification on master.
SQLite initialization and recovery
The registered store publisher installs the final schema in an owner-private sibling directory on the same filesystem. It closes the staged workers, folds WAL into the main file, stamps SQLite's application ID and syncs the completed database. An exclusive initialization lease serializes competing creators.
Before publishing the main file, initialization claims the canonical WAL, SHM and rollback-journal names using exclusive hard links to empty private reservation files. A no-replace rename publishes the completed main at its ordinary SQLite path. The publisher verifies the main inode and all three empty reservations before atomically recording the handoff to SQLite. The final attachment checks that it opened the published generation.
A retry after process death can discard unpublished schema and resume its own reservations. An existing open can finish an interrupted handoff from the retained inode witnesses. Occupied, missing or mismatched members cause refusal while ownership is unresolved. Successful cleanup leaves only the private directory and its coordination lock. Ordinary existing stores continue at their canonical paths. For unmarked legacy databases, a main containing only a header or runtime-ledger tables and an occupied sidecar receives a typed refusal before attachment workers open the family. A legacy database whose application schema exists only in an uncheckpointed WAL can also receive that refusal.
Verification
Remaining scope and reviewer holds
Prospective initialization requires hard links and same-filesystem no-replace rename. The private staging directory isolates unpublished schema. Once ownership transfers, SQLite's VFS controls the canonical sidecars. A noncooperating process with permission to replace those directory entries can still interfere. Endurance of a production daemon with an independent sqlite3 reader across restart, platform CI and verification on master remain part of #4513.
The related verified-reader POSIX lock work remains in #4803. The owner's separate hold on the Kev/decision-model draft remains in force. Do not merge #4776.