Repository navigation
fix(storage): preserve whole recovery generations with resumable moves - #199
Conversation
johnny4young
left a comment
There was a problem hiding this comment.
Summary: This adds a durable cross-process ownership ledger (.store-ownership.sqlite) with shared and exclusive generation claims. Each actual SQLite connection pins the claim through an sqlite3_create_function_v2 destructor. Unreadable-store recovery now moves the whole generation (db, wal, shm, .encrypting, blobs) into a .unreadable-* directory under a journal that can be resumed after interruption. Migrations now run before blob encryption and the downgrade.
Verdict: the design is sound. I found no correctness bug in the moves, the journal or the claim protocol, and no lock-order deadlock: ownership mutex → lifecycle mutex is used consistently, and nothing re-enters the non-recursive NSLock. Below are risks and small issues.
🟠 Stale claims from a reused or unverifiable PID never clear
StoreProcessOwnership.processState reclaims only on ESRCH. A claim left by a process that was jetsam-killed or crashed, whose PID is later reused (very likely after a reboot, where PIDs restart), returns EPERM or success. Such a claim stays forever, or until that unrelated process exits. In this PR it blocks only exclusive work: recovery, plaintext→encrypted conversion, and first creation of an encrypted store, which also needs exclusivity because needsPlaintextConversion returns true when the file is absent. #200 builds on this and makes it far more visible. Suggestion: store a reuse-proof identity alongside the PID and treat a mismatch as exited:
- the process start time (
proc_pidinfoPROC_PIDTBSDINFOpbi_start_tvsec/usec) where it is permitted, and/or - the boot session (
sysctlbyname("kern.bootsessionuuid")). A claim from a previous boot can never be live.
This changes the schema (user_version 2), so I left it to you.
🟡 Nits
StoreGenerationRecovery.archivecreates the.unreadable-*directory before thepresent.contains("gancho.sqlite")guard, so the no-database path leaves an empty hidden directory behind. Move thecreateDirectoryafter the guard.- The journal is fsynced (
synchronize()), but the parent directory and the moves are not. On APFS this is usually fine; considerfcntl(F_FULLFSYNC)on the directory if you want the crash-ordering claim indocs/STORE_RECOVERY.mdto hold strictly. - Opens of an existing encrypted store take a shared claim, so they are not blocked. While another process holds an exclusive claim (first creation or plaintext conversion), however, iOS extensions opening the store get
busyimmediately and fall back to the ephemeral store. That is expected per the doc; just make sure the keyboard and widgets show that state gracefully. - Older recovery artifacts (
gancho.sqlite.unreadable-*files created by earlier builds) are not migrated or recognized. That is harmless, but worth one line inSTORE_RECOVERY.md.
Validation: there is no Swift toolchain in my environment, so this is code reading only. I could not exercise the close_v2 zombie-destructor path. CI is green.
Generated by Claude Code
Create the .unreadable-* directory only after confirming a database is present, so a directory without one is left untouched. Flush the store directory once the journal and archive directory exist, and flush both directories after the moves and again after dropping the journal, so a crash can never leave renames on disk without the journal describing them.
A claim was reclaimed only when kill(pid, 0) reported ESRCH. PIDs restart at every boot, so a claimant killed while holding a claim could leave it attached to an unrelated live process and block recovery, conversion and blob work indefinitely. Claims now record kern.bootsessionuuid and the claimant's start time from sysctl(KERN_PROC_PID) in a new owner_identity table (ledger version 2; version-1 ledgers are migrated in place and their claims keep the PID-only rule). A claim from another boot is stale, and so is a PID this process can signal whose start time differs from the recorded one. Unknown or unreadable stamps stay fail-closed; malformed ones fail the ledger.
Flatten the ledger claim type to one nesting level and use the failable String(bytes:encoding:) initializer for the boot session UUID.
…red downgrades - Write the recovery journal under a staged name, fsync it, then rename it into place, so a crash can never publish an empty journal that fails every later open closed - Remember whether an ownership claim is exclusive and make downgrade a no-op for a shared claim, so an ordinary open no longer writes to the ownership ledger and cannot fail on a busy or suspended ledger
Summary
Fresh-key recovery preserves the whole database/WAL/SHM/
.encrypting/blob generation in one journaled, resumable.unreadable-*directory, and a durable cross-process ownership ledger (.store-ownership.sqlite) keeps recovery and conversion away from live pools. Details below.Review follow-ups
dff833f— fix(storage): sync recovery moves and skip empty archivesarchivecreates.unreadable-*only after confirminggancho.sqliteexists (no empty hidden directory when there is no database).F_FULLFSYNC, falling back tofsync) after the journal and archive directory are created, both directories after the moves, and again after the journal is removed.db7ec0c— fix(storage): stamp ownership claims with boot session and start timekern.bootsessionuuidand the claimant's start time (sysctl(KERN_PROC_PID)) in a newowner_identitytable. The ledger moves touser_version2. A version-1 ledger is migrated in place; its claims keep the PID-only rule.kill(pid,0)givesESRCH; or (c) the PID can be signalled and its start time differs from the recorded one. Unknown stamps, unreadable start times and processes that can't be signalled stay fail-closed. Malformed stamps fail the ledger closed.docs/STORE_RECOVERY.mdupdated.Decisions taken
killsucceeds. On iOS other apps' and extensions' PIDs usually returnEPERM, so there only the boot-session rule applies. That covers reboots, the common reuse case.owner_identityis a separate table rather thanALTER TABLE owner, so the exact-schema check stays strict for both tables.How it was tested (this round)
swiftc -parse+swift-format lint --strict(Swift 6.0.3 container).sysctl/kinfo_proc/renameatx_npand GRDB code are not compiled locally; relies on macOS CI (see below).Known limitations & follow-ups
Merge order
Base of the storage stack: merge this first, then #200 (which already merges this branch), then the v0.10.0 release PR #193.
CI: macOS jobs for the new head were queued when this description was written; check the PR checks tab.
Original description (state at
681ec09; superseded where it conflicts with the sections above, in particular the PID-reuse paragraph):Human-review readiness
Scoped implementation and regression coverage are complete at unchanged head 681ec09. Fresh verification confirms both required native jobs and GitGuardian successful, the expected non-blocking beta-canary skip, and no pending discussion comments, submitted reviews or unresolved inline review threads. Review readiness does not authorize merge, release or deployment.
Release qualification remains open: physical iOS/App Group suspension/expiration, foreground resume, restart/reboot and cross-process stress; a quiescent, fully upgraded/restarted app/extension/CLI/MCP cohort; and native execution of the combined repair/MCP tree. Individual CI and a textually clean combination do not qualify this rollout. Mixed-version participants are unsupported. These are documented release/device/integration gates, not incomplete scoped source or a waived required CI check.
Problem
Fresh-key recovery moved database files while old-key blobs and thumbnails stayed in the live content-addressed namespace. Identical recapture could reuse unreadable ciphertext, and maintenance could destroy the archived database's binary recovery material.
Repair
Preserve database/WAL/SHM, interrupted encryption staging and the entire blob/thumbnail directory in one private, journaled recovery generation. Resume interrupted moves before opening any new namespace; ambiguous members fail closed. Durable process-owned generation registrations pin pools and retained GRDB writers. Exclusive recovery conflicts atomically with every generation/blob owner. Plaintext or absent-file initialization retains exclusive ownership through conversion and pool/blob initialization. Authenticate/migrate before legacy blob changes. Replace the earlier lifetime flock with a metadata-only WAL coordinator: transactions and connection-close/checkpoints are fenced against iOS suspension, and no new application-owned kernel lock is retained during pool lifetime or async work.
Regression coverage added
Interrupt and resume each of five generation member moves; verify preserved bytes; reject recovery while an independent shared lease is active; recover binary content under key B, recapture the same bytes successfully, run maintenance, then reopen the preserved generation under key A and read the original payload. Added deterministic WAL idle-writer, scope conflicts, live/exited/unverifiable/reused PID policy, exact old-token release, deferred/SQLite-busy release retry, late-observer suspension rejection, versioned-ledger fail-closed and exclusive conversion tests.
Compatibility and review
The existing fresh-key plus SQLITE_NOTADB trigger is not expanded. Recovery generations are never garbage-collected. Historical per-file unreadable archives remain untouched and may require explicit recovery review. Review this preservation boundary before dependent PR200.
Exact-head validation
Published head: 681ec09
Uploaded/local staged tree: 54d9f6b70ffde30a1d2939672e5c4b90ee77699a (verified identical before publication)
Actual-connection generation lifetime
GRDB can retain closed Database/configuration objects in dispatch watchdogs, so a strong prepare callback conservatively outlived the actual connection and blocked valid recovery. Preparation now captures the generation weakly and throws if it is gone. A private random per-connection SQLite scalar function retains one ARC reference through create_function_v2 user data. Its destructor drops only that connection’s reference after the pager closes, including close_v2 zombie completion; sibling writer/readers/snapshots remain pinned. Registration failure also invokes the destructor, so there is no double release. SQLCipher builds use SQLCipher C symbols. No persisted schema or application query uses the function.
Added native tests keep a closed writer/configuration explicitly alive without over-pinning, reject reopening copied configuration after the generation is gone, and prove an independent snapshot blocks recovery beyond its originating pool’s close. The seven previous busy failures and both writer/configuration/snapshot lifetime regressions passed unchanged on this exact published head. Three installed Linux SQLite C-model cases passed for sibling release, delayed zombie destruction, and exactly-once failed registration; they do not qualify the packaged Swift/SQLCipher bridge.
Lifecycle and rollout qualification
Only a positive, bounded PID returning documented ESRCH from signal-zero validity checking is reclaimed. PID reuse/reboot collisions, permissions/sandbox failures, malformed identity and suspended owners stay fail-closed; no timeout or missing heartbeat proves death. Exact UUID release retries only known-finished operations. A reused or unverifiable stale PID can withhold availability; callers report busy rather than silently reset. Older binaries do not participate: ownership safety requires quiescing/upgrading/restarting all app/extension/CLI/MCP participants. Main schema and sealed format remain compatible, but mixed-version ownership is not qualified. The optional raw-key migration retains its separate existing opt-in/device rollout boundary. See store recovery protocol.
Independent source review cleared the revised protocol. Physical iOS/App Group app, keyboard, share/widget suspension, expiration, restart/reboot and cross-process stress remain required. Global suspension seam tests require an isolated iOS invocation. Build success alone does not establish device lifecycle safety.
Combined source review
All five repair trees plus unmerged MCP PR195 at c90e07d combine without textual conflicts in a disposable local tree. Native combined execution remains unverified. MCP optional binary content may become nil after legitimate deletion/GC; the captured row hash is never replaced by a newer body lookup. No stronger availability promise for deleted content or new public-stack dependency is inferred.
Ready for human review only. No merge/release, live Keychain, clipboard, real user library or backup access.
🤖 Generated with Claude Code
https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn
Generated by Claude Code