Fix corruption from rowid reuse when database transactions aren't committed#10
Open
ksuther wants to merge 8 commits into
Open
Fix corruption from rowid reuse when database transactions aren't committed#10ksuther wants to merge 8 commits into
ksuther wants to merge 8 commits into
Conversation
setObject: populates keyCache (rowid <-> collectionKey) eagerly via insertForRowidStatement, before commit/rollback is decided. On rollback the connection flushed objectCache and metadataCache but NOT keyCache, so a rolled-back INSERT left a phantom rowid <-> collectionKey entry for a row that no longer exists on disk. Because the database2 table is `rowid INTEGER PRIMARY KEY` (no AUTOINCREMENT), sqlite reuses that freed rowid on the next insert. getRowid:forCollectionKey: (writes) and collectionKeyForRowid: (reads) trust keyCache, so the phantom then resolves the WRONG row: a write meant for one collection is persisted into a different collection's row, and reads return the wrong object. In Fantastical this surfaced as an __NSDictionaryI (a calendar item's internalMetadata sibling, same key) coming out of the database where an FBCalendarItem was expected. Flush keyCache (and extension caches, e.g. Relationship's edgeCache which is populated eagerly and whose edge rowids are likewise reused) in the rollback branch via _flushMemoryWithFlags:. Adds a deterministic regression test (testRollbackFlushesKeyCache_rowidReuseCorruption) reproducing the cross-collection clobber with two connections and a reused rowid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
commitTransaction executed COMMIT, logged any error, and proceeded as if the transaction had committed. But sqlite's documented "response to errors within a transaction" says a COMMIT failing with SQLITE_FULL / SQLITE_IOERR / SQLITE_NOMEM (e.g. disk full while appending to the WAL — where the commit actually writes the transaction's pages) may roll the entire transaction back automatically. YapDatabase never checked, so the in-memory state (keyCache in particular) kept describing rows that don't exist on disk, and the pending changeset was broadcast to peers. On the next write, rowid reuse detonates the phantom keyCache entry and persists the wrong object into an unrelated row — the same corruption as a rollback-after-insert, but with no rollback ever called. Fixes: - commitTransaction now returns BOOL. On COMMIT failure it checks sqlite3_get_autocommit(): if the txn is somehow still open (e.g. BUSY) it rolls back explicitly so the outcome is deterministic (a zombie transaction would swallow every later BEGIN/COMMIT). It routes extensions to didRollbackTransaction (didCommit would, e.g., have Relationship delete files from disk for a rolled-back txn) and rolls back the memory tables (they commit in preCommit, before the sqlite COMMIT, so nothing else reverts them). - postReadWriteTransaction checks the return value. On failure it flushes all caches + extension state, undoes the snapshot increment, and retracts the pending changeset instead of broadcasting it (skipping checkpoint and the YapDatabaseModifiedNotification). Leaving the in-memory snapshot ahead of disk would let another process claim the same snapshot number for a real commit that we'd then never detect. - Adds YapDatabase retractPendingChangeset:fromConnection:. Safe because a pending changeset cannot have been consumed by a peer yet: peers only fetch when they observe a LARGER snapshot, and the failed snapshot never becomes visible on disk or in memory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
deleteEdgesWithSourceOrDestination: (run for EVERY deleted node) recorded deleted edges in the changeset so PEER connections purge their edgeCache, but never purged the WRITER's own edgeCache. Edge rowids are reused by sqlite just like main-table rowids, so a stale edgeCache entry later resolves a different (reused) edge rowid to the OLD edge's nodeDeleteRules. With opposing delete rules (DeleteDestinationIfSourceDeleted together with DeleteSourceIfDestinationDeleted), a rule swap flips the cascade direction and can silently DELETE LIVE ROWS from the main database — no error required, this happens in normal operation. Fix: purge the just-deleted edge rowids from parentConnection->edgeCache per delete; removeAllProtocolEdges (registration/repopulation) drops the whole edgeCache since it deletes rows without recording them in deletedEdges; and postRollbackCleanup clears edgeCache too (flush populated it eagerly before the rollback was decided). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
removeObjectsForKeys: (batch B), removeAllObjectsInCollection: (normal path), and removeAllObjectsInAllCollections all updated caches and the changeset and told every extension the rows were gone even when the DELETE statement failed — the same log-and-pray pattern as the commit-failure bug. This durably desyncs extension tables (views/indexes) and peer connections from the main table, and Relationship would then cascade-delete OTHER live rows for a delete that never happened. All three now bail out without any bookkeeping when the DELETE fails, matching the single-row remove path. removeAllObjectsInCollection: additionally re-purges keyCache/objectCache/metadataCache per batch: an extension hook reading a row from a later batch can repopulate keyCache mid-wipe, leaving a stale rowid <-> collectionKey entry that survives the commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sqlite_enum_reset (which finalizes a busy/nested statement) ran INSIDE the per-collection loop, so a second collection would step a freed statement. The three sibling enumeration methods reset+clear the statement per iteration and only finalize once after the loop. Match them: clear/reset inside the loop and call sqlite_enum_reset once after it. Also align the throw path across all four multi-collection enumerators. On the mutation-during-enumeration exception, the loop body has already reset the statement, but the three siblings @throw without finalizing — leaking the freshly-prepared private statement (needsFinalize == YES) that a nested enumeration allocates, if the exception is ever caught. Call sqlite_enum_reset before @throw in _enumerateKeysAndObjectsInCollections:, _enumerateKeysAndMetadataInCollections:, and _enumerateRowsInCollections: too, so the statement handling is identical in all four. Minor (the exception is a fatal programmer error), purely for consistency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
readSnapshotFromDatabase returned 0 on any error and only logged the exact SQLITE_ERROR code. In multiprocess mode this SELECT is the authoritative external-modification check, so a silent 0 made preRead/preReadWrite conclude "nothing changed" and trust stale caches after a transient SQLITE_IOERR / SQLITE_BUSY / SQLITE_CORRUPT. readSnapshotFromDatabase: now reports failure via an out param (any status other than SQLITE_ROW/SQLITE_DONE), and both preReadTransaction and preReadWriteTransaction treat "cannot verify the snapshot" as an undetected external modification and force a full cache flush. Also adds a guard for the one error-free corruption path found: if the on-disk snapshot goes BACKWARDS relative to memory (file restored from a backup or replaced by another tool), adopt the disk value and flush everything rather than trusting caches that describe a different file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a database file is detected as corrupt and renamed away (or deleted), its -wal and -shm sidecar files were left behind. sqlite then treats the stale -wal as a hot journal for the NEW (empty) database file and REPLAYS THE OLD DATABASE'S FRAMES into it, resurrecting the very content just declared corrupt. Both the Rename and Delete corruptAction branches now move/remove the -wal and -shm files alongside the main file (the Rename branch falls back to deleting a sidecar if the move fails — anything is better than the new file adopting the old WAL). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The mixed add/remove branch of processChangeset ignored getDefaultObjectPolicy / getDefaultMetadataPolicy and fell back to Containment, so under a Share policy a cache update from a peer changeset silently became an eviction (stale-but-safe, but a needless cache miss and inconsistent with the pure-update branch). Fall back to the database's default policy, matching the pure-update branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
I tried throwing Opus 4.8 and Fable 5 at our ever-persistent database corruption bug.
The first two commits are the primary fixes where we weren't handling rollbacks or failed commits properly and the key cache wasn't being invalidated.
The short version is the
keyCachewouldn't get invalidated, so the mapping of collection/key to rowid wouldn't get reset. This could then lead to the wrong object getting saved at the wrong rowid because rowids get reused if the transaction isn't committed.The other commits are other fixes that Fable found while analyzing the rollback bug.
Tests
Giant Claude-generated description