From ea205f4562c6304b55de9ce82099f33606c25fbc Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:09:43 -0400 Subject: [PATCH 01/16] Flush keyCache on transaction rollback 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) --- Testing/UnitTesting/TestYapDatabase.m | 59 +++++++++++++++++++++++++++ YapDatabase/YapDatabaseConnection.m | 9 ++-- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/Testing/UnitTesting/TestYapDatabase.m b/Testing/UnitTesting/TestYapDatabase.m index 82d735a5a..bfd13ca8c 100644 --- a/Testing/UnitTesting/TestYapDatabase.m +++ b/Testing/UnitTesting/TestYapDatabase.m @@ -1277,4 +1277,63 @@ - (void)testObjectAndMetadataPolicies { XCTAssert(collectionConfig.metadataPolicy == YapDatabasePolicyShare); } +// Regression test: rolling back a write transaction must flush the keyCache. +// +// setObject: populates keyCache (rowid <-> collectionKey) eagerly, before commit/rollback is decided +// (YapDatabaseTransaction.m, insertForRowidStatement). On rollback, YapDatabaseConnection flushes +// objectCache + metadataCache but historically NOT keyCache, so a rolled-back INSERT leaves a phantom +// rowid <-> collectionKey entry for a row that no longer exists. Because the table is `rowid INTEGER +// PRIMARY KEY` (no AUTOINCREMENT), that rowid is reused by the next insert. getRowid:forCollectionKey: +// and collectionKeyForRowid: trust keyCache, so the phantom then resolves the WRONG row -> a write for +// one collection is persisted into a different collection's row (and reads return the wrong object). +- (void)testRollbackFlushesKeyCache_rowidReuseCorruption +{ + NSURL *databaseURL = [self databaseURL:NSStringFromSelector(_cmd)]; + [[NSFileManager defaultManager] removeItemAtURL:databaseURL error:NULL]; + + YapDatabase *database = [[YapDatabase alloc] initWithURL:databaseURL]; + XCTAssertNotNil(database); + + YapDatabaseConnection *connA = [database newConnection]; + YapDatabaseConnection *connB = [database newConnection]; + + NSString *const collA = @"A"; + NSString *const collB = @"B"; + NSString *const key = @"shared-key"; // both siblings share the same key + + // 1. connA inserts (collA,key) -> rowid 1, then ROLLS BACK. keyCache retains a phantom entry + // (rowid 1 <-> (collA,key)) for a row that no longer exists on disk. + [connA readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [transaction setObject:@"objA" forKey:key inCollection:collA]; + [transaction rollback]; + }]; + + // 2. connB inserts (collB,key). The table is empty again, so sqlite REUSES rowid 1. + // This insert is broadcast to connA only by collectionKey (never by rowid), so connA's + // phantom keyCache entry is never purged. + [connB readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [transaction setObject:@"objB" forKey:key inCollection:collB]; + }]; + + // 3. connA writes (collA,key) again. getRowid:forCollectionKey:(collA,key) trusts the stale + // reverse map and returns rowid 1 -> UPDATE ... WHERE rowid = 1 -> clobbers collB's row. + [connA readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [transaction setObject:@"objA-updated" forKey:key inCollection:collA]; + }]; + + // 4. Verify from a FRESH (cold-cache) connection so we read what is actually on disk. + YapDatabaseConnection *connC = [database newConnection]; + __block id valB; + __block id valA; + [connC readWithBlock:^(YapDatabaseReadTransaction *transaction) { + valB = [transaction objectForKey:key inCollection:collB]; + valA = [transaction objectForKey:key inCollection:collA]; + }]; + + XCTAssertEqualObjects(valB, @"objB", + @"collB's row was corrupted by a write meant for collA (rollback did not flush keyCache)"); + XCTAssertEqualObjects(valA, @"objA-updated", + @"the write for collA landed on the wrong row (rollback did not flush keyCache)"); +} + @end diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index 388ff5b18..0bdbf5a2d 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -2924,14 +2924,13 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction // Rollback sqlite database transaction. [transaction rollbackTransaction]; - + // Rollback-Write-Transaction: Step 3 of 3 // // Reset any in-memory variables which may be out-of-sync with the database. - - [objectCache removeAllObjects]; - [metadataCache removeAllObjects]; - + + [self _flushMemoryWithFlags:YapDatabaseConnectionFlushMemoryFlags_Caches]; + } else // if (!transaction->rollback) { From c87a851dcdcb39cfb1e5547094b146a22e08ab66 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:10:09 -0400 Subject: [PATCH 02/16] Treat a failed sqlite COMMIT as a rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- YapDatabase/Internal/YapDatabasePrivate.h | 3 +- YapDatabase/YapDatabase.m | 24 +++++++- YapDatabase/YapDatabaseConnection.m | 64 ++++++++++++++++---- YapDatabase/YapDatabaseTransaction.m | 73 ++++++++++++++++++++--- 4 files changed, 140 insertions(+), 24 deletions(-) diff --git a/YapDatabase/Internal/YapDatabasePrivate.h b/YapDatabase/Internal/YapDatabasePrivate.h index 9481cf3f4..d8b7db067 100644 --- a/YapDatabase/Internal/YapDatabasePrivate.h +++ b/YapDatabase/Internal/YapDatabasePrivate.h @@ -146,6 +146,7 @@ static NSString *const ext_key_class = @"class"; * - snapshot : NSNumber with the changeset's snapshot */ - (void)notePendingChangeset:(NSDictionary *)changeset fromConnection:(YapDatabaseConnection *)connection; +- (void)retractPendingChangeset:(NSDictionary *)changeset fromConnection:(YapDatabaseConnection *)connection; /** * This method is only accessible from within the snapshotQueue. @@ -334,7 +335,7 @@ static NSString *const ext_key_class = @"class"; - (void)beginTransaction; - (void)beginImmediateTransaction; - (void)preCommitReadWriteTransaction; -- (void)commitTransaction; +- (BOOL)commitTransaction; // returns NO if the sqlite COMMIT failed (treat like a rollback) - (void)rollbackTransaction; - (NSDictionary *)extensions; diff --git a/YapDatabase/YapDatabase.m b/YapDatabase/YapDatabase.m index 63b3dadf9..67ebedafc 100644 --- a/YapDatabase/YapDatabase.m +++ b/YapDatabase/YapDatabase.m @@ -3091,11 +3091,33 @@ - (void)notePendingChangeset:(NSDictionary *)pendingChangeset fromConnection:(Ya // We save the changeset in advance to handle possible edge cases. [changesets addObject:pendingChangeset]; - + YDBLogVerbose(@"Adding pending changeset %@ for database: %@", [[changesets lastObject] objectForKey:YapDatabaseSnapshotKey], self); } +/** + * This method is only accessible from within the snapshotQueue. + * + * If the sqlite COMMIT fails after the changeset was registered via notePendingChangeset:, + * nothing was written to disk and the changeset must be retracted so it is never delivered. + * + * This is safe because a pending changeset cannot have been consumed by any other connection yet: + * connections only fetch changesets when they observe a snapshot LARGER than their own, and the + * failed changeset's snapshot never becomes visible anywhere — not on disk (the sqlite transaction + * rolled back, taking the yap2 snapshot write with it), and not in memory (noteCommittedChangeset + * never runs for a failed commit, so neither database->snapshot nor any peer connection advances). +**/ +- (void)retractPendingChangeset:(NSDictionary *)pendingChangeset fromConnection:(YapDatabaseConnection __unused *)sender +{ + NSAssert(dispatch_get_specific(IsOnSnapshotQueueKey), @"Must go through snapshotQueue for atomic access."); + + [changesets removeObjectIdenticalTo:pendingChangeset]; + + YDBLogVerbose(@"Retracted pending changeset %@ for database: %@", + [pendingChangeset objectForKey:YapDatabaseSnapshotKey], self); +} + /** * This method is only accessible from within the snapshotQueue. * diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index 0bdbf5a2d..f07030f80 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -2951,10 +2951,12 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction // The "external" changeset gets plugged into the YapDatabaseModifiedNotification as the userInfo dict. NSNotification *notification = nil; - + NSMutableDictionary *changeset = nil; NSMutableDictionary *userInfo = nil; - + + BOOL snapshotIncremented = NO; + [self getInternalChangeset:&changeset externalChangeset:&userInfo]; if (changeset || userInfo || hasDiskChanges) { @@ -2963,11 +2965,13 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction // // If hasDiskChanges is NO, then the database file was not modified. // However, something was "touched" or an in-memory extension was changed. - + if (hasDiskChanges || enableMultiProcessSupport) snapshot = [self incrementSnapshotInDatabase]; else snapshot++; + + snapshotIncremented = YES; if (changeset == nil) changeset = [NSMutableDictionary dictionaryWithSharedKeySet:sharedKeySetForInternalChangeset]; @@ -3137,21 +3141,55 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction // from the database. If it doesn't match what we expect, then we know we've run into the race condition, // and we make the read-only transaction back out and try again. - [transaction commitTransaction]; - + BOOL committed = [transaction commitTransaction]; + if (!committed) + { + // The sqlite COMMIT failed (e.g. disk full / I/O error while appending to the WAL), + // which means the transaction was rolled back — either automatically by sqlite, or + // explicitly by commitTransaction. Nothing we wrote exists on disk, but all of our + // in-memory state says it does. This is the same situation as an explicit rollback, + // so it needs the same cache flush; keyCache especially, because stale + // rowid <-> collectionKey entries + rowid reuse silently corrupt OTHER rows on the + // next write (see the flush in the rollback branch above). + + NSUInteger flags = YapDatabaseConnectionFlushMemoryFlags_Caches | + (NSUInteger)YapDatabaseConnectionFlushMemoryFlags_Extension_State; + + [self _flushMemoryWithFlags:flags]; + + // Undo the snapshot increment (the yap2 snapshot write rolled back with the + // transaction, so disk still has the old value). Leaving the in-memory snapshot + // ahead of disk would let another process claim the same snapshot number for a + // REAL commit — which we would then never detect (snapshot equality), leaving + // every cache silently stale. The pending changeset is retracted below. + + if (snapshotIncremented) + { + snapshot--; + } + } + __block uint64_t minSnapshot = UINT64_MAX; - + dispatch_sync(database->snapshotQueue, ^{ @autoreleasepool { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wimplicit-retain-self" - + // Post-Write-Transaction: Step 7 of 11 // // Notify database of changes, and drop reference to set of changed keys. - + // + // If the commit FAILED, the pending changeset describes a transaction that never + // happened — retract it instead of delivering it. No other connection can have + // consumed it yet (see retractPendingChangeset:), so peers never see phantom data + // and neither database->snapshot nor any peer connection advances. + if (changeset) { - [database noteCommittedChangeset:changeset fromConnection:self]; + if (committed) + [database noteCommittedChangeset:changeset fromConnection:self]; + else + [database retractPendingChangeset:changeset fromConnection:self]; } // Post-Write-Transaction: Step 8 of 11 @@ -3177,13 +3215,13 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction #pragma clang diagnostic pop }}); - if (changeset) + if (changeset && committed) { // Post-Write-Transaction: Step 9 of 11 // // We added frames to the WAL. // We can invoke a checkpoint if there are no other active connections. - + if (minSnapshot == UINT64_MAX) { [database asyncCheckpoint:snapshot]; @@ -3227,8 +3265,8 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction // Post-Write-Transaction: Step 11 of 11 // // Post YapDatabaseModifiedNotification (if needed) - - if (notification) + + if (notification && committed) { dispatch_async(dispatch_get_main_queue(), ^{ [[NSNotificationCenter defaultCenter] postNotification:notification]; diff --git a/YapDatabase/YapDatabaseTransaction.m b/YapDatabase/YapDatabaseTransaction.m index 167b3c188..ca473a82f 100644 --- a/YapDatabase/YapDatabaseTransaction.m +++ b/YapDatabase/YapDatabaseTransaction.m @@ -153,29 +153,84 @@ - (void)preCommitReadWriteTransaction [yapMemoryTableTransaction commit]; } -- (void)commitTransaction +- (BOOL)commitTransaction { + BOOL committed = NO; + sqlite3_stmt *statement = [connection commitTransactionStatement]; if (statement) { // COMMIT TRANSACTION; - + int status = sqlite3_step(statement); - if (status != SQLITE_DONE) + if (status == SQLITE_DONE) { + committed = YES; + } + else + { + // COMMIT can fail (SQLITE_FULL / SQLITE_IOERR / SQLITE_NOMEM, e.g. disk full while + // appending to the WAL). Per sqlite's documented "response to errors within a + // transaction", such errors may have already rolled the ENTIRE transaction back + // automatically. Either way, nothing was committed. Callers MUST treat this like a + // rollback (flush caches, don't trust the changeset), otherwise in-memory state + // (keyCache in particular) describes rows that don't exist on disk, and rowid reuse + // later corrupts unrelated rows. + YDBLogError(@"Couldn't commit transaction: %d %s", status, sqlite3_errmsg(connection->db)); + + if (isReadWriteTransaction && !sqlite3_get_autocommit(connection->db)) + { + // sqlite did NOT auto-rollback; the transaction is still open. + // Roll it back explicitly so the outcome is deterministic + // (a zombie transaction would swallow every subsequent BEGIN/COMMIT). + + sqlite3_stmt *rbStatement = [connection rollbackTransactionStatement]; + if (rbStatement) + { + int rbStatus = sqlite3_step(rbStatement); + if (rbStatus != SQLITE_DONE) + { + YDBLogError(@"Couldn't rollback failed commit: %d %s", + rbStatus, sqlite3_errmsg(connection->db)); + } + sqlite3_reset(rbStatement); + } + } } - + sqlite3_reset(statement); } - + if (isReadWriteTransaction) { - [extensions enumerateKeysAndObjectsUsingBlock:^(id __unused extNameObj, id extTransactionObj, BOOL __unused *stop) { - - [(YapDatabaseExtensionTransaction *)extTransactionObj didCommitTransaction]; - }]; + if (committed) + { + [extensions enumerateKeysAndObjectsUsingBlock:^(id __unused extNameObj, id extTransactionObj, BOOL __unused *stop) { + + [(YapDatabaseExtensionTransaction *)extTransactionObj didCommitTransaction]; + }]; + } + else + { + // The transaction was rolled back (by sqlite or by us, above), so extensions must be + // told the truth. didCommitTransaction would have them act on a commit that never + // happened — e.g. YapDatabaseRelationship deletes files from disk in didCommit, and + // extension connections would adopt in-memory state describing rolled-back rows. + + [extensions enumerateKeysAndObjectsUsingBlock:^(id __unused extNameObj, id extTransactionObj, BOOL __unused *stop) { + + [(YapDatabaseExtensionTransaction *)extTransactionObj didRollbackTransaction]; + }]; + + // The memory tables were committed in preCommitReadWriteTransaction (before the sqlite + // COMMIT ran), so they hold values for a transaction that never happened. Strip them. + + [yapMemoryTableTransaction rollback]; + } } + + return committed; } - (void)rollbackTransaction From 87ab7104d526c82ebd796abb96c31d04b5862eae Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:10:19 -0400 Subject: [PATCH 03/16] Purge writer's own edgeCache when Relationship edges are deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../UnitTesting/TestYapDatabaseRelationship.m | 103 ++++++++++++++++++ .../YapDatabaseRelationshipConnection.m | 14 ++- .../YapDatabaseRelationshipTransaction.m | 51 ++++++--- 3 files changed, 149 insertions(+), 19 deletions(-) diff --git a/Testing/UnitTesting/TestYapDatabaseRelationship.m b/Testing/UnitTesting/TestYapDatabaseRelationship.m index 8f9fc8407..329025837 100644 --- a/Testing/UnitTesting/TestYapDatabaseRelationship.m +++ b/Testing/UnitTesting/TestYapDatabaseRelationship.m @@ -4405,4 +4405,107 @@ - (void)testDeleteAndNotify XCTAssert([Node_NotifyCount notifyCount] == 1); } +// Regression test: deleting a node's edges must purge the WRITER's own edgeCache. +// +// deleteEdgesWithSourceOrDestination: (run when a node is deleted) records the deleted edge +// rowids in the changeset so PEER connections purge their edgeCache, but historically never +// purged the writer's OWN edgeCache. The edges table is `rowid INTEGER PRIMARY KEY` (no +// AUTOINCREMENT), so sqlite reuses a freed edge rowid on the next insert. Because an edge INSERT +// is not broadcast to peers (only deletedEdges/modifiedEdges are), a peer that deleted the old +// edge keeps a stale edgeCache entry for that rowid. When the reused rowid is later read during a +// cascade (enumerateExistingEdgesWithDestination: patches only src/dst rowids from disk and keeps +// the CACHED edge's nodeDeleteRules), the wrong delete rule is applied — the cascade can delete +// the wrong node, or (as asserted here) fail to delete the node it should have. +// +// Two connections are required: conn1 deletes the old edge (its own cache isn't purged), conn2 +// inserts a new edge reusing the freed rowid (not broadcast into conn1's cache), then conn1 reads +// the stale entry during a cascade. +- (void)testDeletedNodePurgesWriterEdgeCache_rowidReuseWrongDeleteRule +{ + NSURL *databaseURL = [self databaseURL:NSStringFromSelector(_cmd)]; + + [[NSFileManager defaultManager] removeItemAtURL:databaseURL error:NULL]; + YapDatabase *database = [[YapDatabase alloc] initWithURL:databaseURL]; + + XCTAssertNotNil(database); + + YapDatabaseConnection *conn1 = [database newConnection]; + YapDatabaseConnection *conn2 = [database newConnection]; + + YapDatabaseRelationship *relationship = [[YapDatabaseRelationship alloc] init]; + + BOOL registered = [database registerExtension:relationship withName:@"relationship"]; + XCTAssertTrue(registered, @"Error registering extension"); + + // Nodes for the OLD edge (E1) and the NEW edge (E2). Plain string nodes; manual edges. + NSString *const oldSrc = @"oldSrc"; // E1 source + NSString *const oldDst = @"oldDst"; // E1 destination + NSString *const newSrc = @"newSrc"; // E2 source -- the live node that must be cascade-deleted + NSString *const newDst = @"newDst"; // E2 destination -- deleting this must delete newSrc + + // 1. conn1: insert the four nodes and add E1 (oldSrc -> oldDst) with a NON-deleting rule. + // E1 gets edge rowid 1; conn1's edgeCache now holds 1 -> E1 (rules = NotifyIfSourceDeleted). + [conn1 readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + + [transaction setObject:oldSrc forKey:oldSrc inCollection:nil]; + [transaction setObject:oldDst forKey:oldDst inCollection:nil]; + [transaction setObject:newSrc forKey:newSrc inCollection:nil]; + [transaction setObject:newDst forKey:newDst inCollection:nil]; + + YapDatabaseRelationshipEdge *e1 = + [YapDatabaseRelationshipEdge edgeWithName:@"e1" + sourceKey:oldSrc + collection:nil + destinationKey:oldDst + collection:nil + nodeDeleteRules:YDB_NotifyIfSourceDeleted]; + + [[transaction ext:@"relationship"] addEdge:e1]; + }]; + + // 2. conn1: delete oldSrc. Its edge E1 is deleted via deleteEdgesWithSourceOrDestination:, + // freeing edge rowid 1. E1's rule is non-deleting, so oldDst survives. WITHOUT the fix, + // conn1's edgeCache still holds the phantom 1 -> E1. + [conn1 readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + + [transaction removeObjectForKey:oldSrc inCollection:nil]; + }]; + + // 3. conn2: add E2 (newSrc -> newDst) with DeleteSourceIfDestinationDeleted. insertEdge reuses + // the freed edge rowid 1. The insert is NOT broadcast into conn1's edgeCache, so conn1's + // phantom entry (1 -> E1, non-deleting rule) is neither updated nor purged. + [conn2 readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + + YapDatabaseRelationshipEdge *e2 = + [YapDatabaseRelationshipEdge edgeWithName:@"e2" + sourceKey:newSrc + collection:nil + destinationKey:newDst + collection:nil + nodeDeleteRules:YDB_DeleteSourceIfDestinationDeleted]; + + [[transaction ext:@"relationship"] addEdge:e2]; + }]; + + // 4. conn1: delete newDst. The cascade reads edge rowid 1 for the "destination deleted" rule. + // - Fixed: cache was purged in step 2 -> cache miss -> E2 deserialized from disk -> + // DeleteSourceIfDestinationDeleted -> newSrc is deleted. + // - Buggy: stale hit returns E1 (non-deleting rule) -> newSrc is NOT deleted. + [conn1 readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + + [transaction removeObjectForKey:newDst inCollection:nil]; + }]; + + // 5. Verify from a FRESH connection (cold cache) so we read what is actually on disk. + YapDatabaseConnection *conn3 = [database newConnection]; + __block id valNewSrc = nil; + [conn3 readWithBlock:^(YapDatabaseReadTransaction *transaction) { + valNewSrc = [transaction objectForKey:newSrc inCollection:nil]; + }]; + + XCTAssertNil(valNewSrc, + @"newSrc should have been cascade-deleted by E2's DeleteSourceIfDestinationDeleted rule; " + @"a stale edgeCache entry (rowid reuse) applied the OLD edge's non-deleting rule instead"); +} + @end diff --git a/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipConnection.m b/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipConnection.m index cb9ab8822..afe12ad20 100644 --- a/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipConnection.m +++ b/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipConnection.m @@ -247,18 +247,26 @@ - (void)postCommitCleanup - (void)postRollbackCleanup { YDBLogAutoTrace(); - + [protocolChanges removeAllObjects]; [manualChanges removeAllObjects]; [inserted removeAllObjects]; [deletedOrder removeAllObjects]; [deletedInfo removeAllObjects]; - + reset = NO; - + [deletedEdges removeAllObjects]; [modifiedEdges removeAllObjects]; [filesToDelete removeAllObjects]; + + // The transaction's flush may have inserted/updated edges into edgeCache before the rollback + // was decided. Those entries describe edge rows that no longer exist (or hold pre-rollback + // nodeDeleteRules). Edge rowids get reused by sqlite, so a stale entry here later resolves a + // DIFFERENT edge's rowid to the old edge — wrong delete rules can then cascade-delete live + // rows from the main database. Flush it. + + [edgeCache removeAllObjects]; } - (NSArray *)internalChangesetKeys diff --git a/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipTransaction.m b/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipTransaction.m index 2926c389e..24c0e114f 100644 --- a/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipTransaction.m +++ b/YapDatabase/Extensions/Relationships/YapDatabaseRelationshipTransaction.m @@ -2830,64 +2830,76 @@ - (void)deleteEdge:(YapDatabaseRelationshipEdge *)edge **/ - (void)deleteEdgesWithSourceOrDestination:(int64_t)rowid { + NSMutableArray *deletedEdgeRowids = [NSMutableArray array]; + // Step 1: // First record the edges that are getting deleted { sqlite3_stmt *statement = [parentConnection findEdgesWithNodeStatement]; if (statement == NULL) return; - + // SELECT "rowid" FROM "tableName" WHERE "src" = ? OR "dst" = ?; - + int const column_idx_rowid = SQLITE_COLUMN_START; - + int const bind_idx_src = SQLITE_BIND_START + 0; int const bind_idx_dst = SQLITE_BIND_START + 1; - + sqlite3_bind_int64(statement, bind_idx_src, rowid); sqlite3_bind_int64(statement, bind_idx_dst, rowid); - + int status; while ((status = sqlite3_step(statement)) == SQLITE_ROW) { int64_t edgeRowid = sqlite3_column_int64(statement, column_idx_rowid); - + [parentConnection->deletedEdges addObject:@(edgeRowid)]; + [deletedEdgeRowids addObject:@(edgeRowid)]; } - + if (status != SQLITE_DONE) { YDBLogError(@"sqlite_step error: %d %s", status, sqlite3_errmsg(databaseTransaction->connection->db)); } - + sqlite3_clear_bindings(statement); sqlite3_reset(statement); } - + // Step 2: // Then actually go ahead and delete the edges { sqlite3_stmt *statement = [parentConnection deleteEdgesWithNodeStatement]; if (statement == NULL) return; - + // DELETE FROM "tableName" WHERE "src" = ? OR "dst" = ?; - + int const bind_idx_src = SQLITE_BIND_START + 0; int const bind_idx_dst = SQLITE_BIND_START + 1; - + sqlite3_bind_int64(statement, bind_idx_src, rowid); sqlite3_bind_int64(statement, bind_idx_dst, rowid); - + int status = sqlite3_step(statement); if (status != SQLITE_DONE) { YDBLogError(@"Error executing statement: %d %s", status, sqlite3_errmsg(databaseTransaction->connection->db)); } - + sqlite3_clear_bindings(statement); sqlite3_reset(statement); } + + // Step 3: + // Purge the deleted edges from OUR OWN edgeCache. The deletedEdges changeset only purges + // PEER connections' caches; the writer never processes its own changeset. Edge rowids get + // reused by sqlite, so a stale entry here later resolves a different (reused) edge rowid to + // the old edge's nodeDeleteRules — a wrong-direction cascade can then delete live rows from + // the main database. + + [parentConnection->edgeCache removeObjectsForKeys:deletedEdgeRowids]; } /** @@ -2914,10 +2926,17 @@ - (void)removeAllProtocolEdges YDBLogError(@"Error executing statement: %d %s", status, sqlite3_errmsg(databaseTransaction->connection->db)); } - + sqlite3_reset(statement); - + [parentConnection->protocolChanges removeAllObjects]; + + // Every protocol edge row was just deleted (and their rowids are now free for reuse), + // but nothing records them in deletedEdges, so neither our edgeCache nor any peer's + // would be purged. This only runs from populateTable (registration/repopulation); + // dropping the whole cache is the safe move. + + [parentConnection->edgeCache removeAllObjects]; } /** From 8f0023acb05a1d813fa9a3ba0086f839f4a5a13d Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:10:36 -0400 Subject: [PATCH 04/16] Don't update caches/extensions when a bulk DELETE statement fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- YapDatabase/YapDatabaseTransaction.m | 63 ++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 13 deletions(-) diff --git a/YapDatabase/YapDatabaseTransaction.m b/YapDatabase/YapDatabaseTransaction.m index ca473a82f..54be4bed7 100644 --- a/YapDatabase/YapDatabaseTransaction.m +++ b/YapDatabase/YapDatabaseTransaction.m @@ -5738,15 +5738,23 @@ - (void)removeObjectsForKeys:(NSArray *)keys inCollection:(NSString *)collection } status = sqlite3_step(statement); + + sqlite3_finalize(statement); + statement = NULL; + if (status != SQLITE_DONE) { YDBLogError(@"Error executing 'removeKeys:inCollection:' statement (B): %d %s", status, sqlite3_errmsg(connection->db)); + + // The DELETE didn't happen. Do NOT update caches/changeset or tell extensions the + // rows were removed — that would durably desync extension tables (views/indexes) + // and peers from the main table. Abort the remaining batches too. + + FreeYapDatabaseString(&_collection); + return; } - - sqlite3_finalize(statement); - statement = NULL; - + connection->hasDiskChanges = YES; [connection->mutationStack markAsMutated]; // mutation during enumeration protection @@ -6049,20 +6057,42 @@ - (void)removeAllObjectsInCollection:(NSString *)collection } status = sqlite3_step(statement); + + sqlite3_finalize(statement); + statement = NULL; + if (status != SQLITE_DONE) { YDBLogError(@"Error executing 'removeAllObjectsInCollection:' statement: %d %s", status, sqlite3_errmsg(connection->db)); + + // The DELETE didn't happen. Do NOT update caches/changeset or tell extensions the + // rows were removed — that would durably desync extension tables (views/indexes) + // and peers from the main table. Abort the remaining batches too. + + FreeYapDatabaseString(&_collection); + return; } - - sqlite3_finalize(statement); - statement = NULL; - + connection->hasDiskChanges = YES; [connection->mutationStack markAsMutated]; // mutation during enumeration protection - + + // The up-front cache purge (above) can be undone by extension hooks that read rows + // belonging to a later batch (repopulating keyCache/objectCache mid-wipe). + // Purge again per-batch so no stale rowid <-> collectionKey entry survives the wipe. + + [connection->keyCache removeObjectsForKeys:foundRowids]; + + for (NSString *key in foundKeys) + { + YapCollectionKey *cacheKey = [[YapCollectionKey alloc] initWithCollection:collection key:key]; + + [connection->objectCache removeObjectForKey:cacheKey]; + [connection->metadataCache removeObjectForKey:cacheKey]; + } + [connection->removedRowids addObjectsFromArray:foundRowids]; - + for (YapDatabaseExtensionTransaction *extTransaction in [self orderedExtensions]) { [extTransaction didRemoveObjectsForKeys:foundKeys @@ -6092,13 +6122,20 @@ - (void)removeAllObjectsInAllCollections } int status = sqlite3_step(statement); + + sqlite3_reset(statement); + if (status != SQLITE_DONE) { YDBLogError(@"Error executing 'removeAllStatement': %d %s", status, sqlite3_errmsg(connection->db)); + + // The DELETE didn't happen — the main table still holds every row. Broadcasting + // allKeysRemoved / telling extensions to truncate their tables here would durably + // desync extension tables and peers from the main table. Bail out instead. + + return; } - - sqlite3_reset(statement); - + connection->hasDiskChanges = YES; [connection->mutationStack markAsMutated]; // mutation during enumeration protection From af5258692b3edf41f06eb0c4c6512d308c4c0dac Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:10:47 -0400 Subject: [PATCH 05/16] Fix use-after-free in _enumerateKeysInCollections: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- YapDatabase/YapDatabaseTransaction.m | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/YapDatabase/YapDatabaseTransaction.m b/YapDatabase/YapDatabaseTransaction.m index 54be4bed7..2e7da12c2 100644 --- a/YapDatabase/YapDatabaseTransaction.m +++ b/YapDatabase/YapDatabaseTransaction.m @@ -2865,21 +2865,25 @@ - (void)_enumerateKeysInCollections:(NSArray *)collections { YDBLogError(@"sqlite_step error: %d %s", status, sqlite3_errmsg(connection->db)); } - - sqlite_enum_reset(statement, needsFinalize); + + sqlite3_clear_bindings(statement); // ok: within loop + sqlite3_reset(statement); // ok: within loop FreeYapDatabaseString(&_collection); - + if (!stop && mutation.isMutated) { + sqlite_enum_reset(statement, needsFinalize); @throw [self mutationDuringEnumerationException]; } - + if (stop) { break; } - + } // end for (NSString *collection in collections) + + sqlite_enum_reset(statement, needsFinalize); } /** @@ -3171,6 +3175,7 @@ - (void)_enumerateKeysAndObjectsInCollections:(NSArray *)collections if (!stop && mutation.isMutated) { + sqlite_enum_reset(statement, needsFinalize); @throw [self mutationDuringEnumerationException]; } @@ -3521,6 +3526,7 @@ - (void)_enumerateKeysAndMetadataInCollections:(NSArray *)collections if (!stop && mutation.isMutated) { + sqlite_enum_reset(statement, needsFinalize); @throw [self mutationDuringEnumerationException]; } @@ -3950,6 +3956,7 @@ - (void)_enumerateRowsInCollections:(NSArray *)collections if (!stop && mutation.isMutated) { + sqlite_enum_reset(statement, needsFinalize); @throw [self mutationDuringEnumerationException]; } From 4773118104588896a6478de06083601bf0b83b9c Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:11:01 -0400 Subject: [PATCH 06/16] Report readSnapshotFromDatabase errors and guard backwards snapshots 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) --- YapDatabase/YapDatabaseConnection.m | 105 ++++++++++++++++++++++------ 1 file changed, 83 insertions(+), 22 deletions(-) diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index f07030f80..e664efab2 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -2431,7 +2431,8 @@ - (void)preReadTransaction:(YapDatabaseReadTransaction *)transaction // If the two match then our snapshots are in sync. // If they don't then we need to get caught up by processing changesets. - dbSnapshot = [self readSnapshotFromDatabase]; + BOOL dbSnapshotError = NO; + dbSnapshot = [self readSnapshotFromDatabase:&dbSnapshotError]; if (wal_file == NULL) { wal_file = yap_vfs_last_opened_wal(database->yap_vfs_shim); @@ -2439,17 +2440,37 @@ - (void)preReadTransaction:(YapDatabaseReadTransaction *)transaction wal_file->yap_database_connection = (__bridge void *)self; } } - - if (snapshot < dbSnapshot) + + if (dbSnapshotError) + { + // We could not read the authoritative on-disk snapshot, so we cannot verify our + // caches are current. Treat this like an undetected external modification: + // force a full flush (below), and keep our own snapshot value. + + expectsChangesets = YES; + changesets = nil; + dbSnapshot = snapshot; + } + else if (snapshot < dbSnapshot) { // The transaction can see the sqlite commit from another transaction, // and it hasn't processed the changeset(s) yet. // We need to fetch them now. - + expectsChangesets = YES; changesets = [database pendingAndCommittedChangesetsSince:snapshot until:dbSnapshot]; } - + else if (dbSnapshot < snapshot) + { + // The on-disk snapshot went BACKWARDS relative to us. Normal operation cannot + // produce this; it means the database file changed underneath us (restored from + // a backup, replaced by another tool) or the snapshot row was lost. Our caches + // describe a different file. Adopt the on-disk value and flush everything (below). + + expectsChangesets = YES; + changesets = nil; + } + myState->longLivedReadTransaction = (longLivedReadTransaction != nil); myState->sqlLevelSharedReadLock = YES; needsMarkSqlLevelSharedReadLock = NO; @@ -2774,16 +2795,18 @@ - (void)preReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction // In case of multiple processes accessing the database, // we can't know for sure so we must make this assumption. + BOOL dbSnapshotError = NO; + if (enableMultiProcessSupport) { - dbSnapshot = [self readSnapshotFromDatabase]; + dbSnapshot = [self readSnapshotFromDatabase:&dbSnapshotError]; } else { (void)[self readSnapshotFromDatabase]; // create wal_file dbSnapshot = [database snapshot]; } - + if (wal_file == NULL) { wal_file = yap_vfs_last_opened_wal(database->yap_vfs_shim); @@ -2791,23 +2814,44 @@ - (void)preReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction wal_file->yap_database_connection = (__bridge void *)self; } } + + if (dbSnapshotError) + { + // We could not read the authoritative on-disk snapshot, so we cannot verify our + // caches are current. Treat this like an undetected external modification: + // force a full flush (below), and keep our own snapshot value. + + expectsChangesets = YES; + changesets = nil; + dbSnapshot = snapshot; + } + else if (enableMultiProcessSupport && (dbSnapshot < snapshot)) + { + // The on-disk snapshot went BACKWARDS relative to us. Normal operation cannot + // produce this; it means the database file changed underneath us (restored from + // a backup, replaced by another tool) or the snapshot row was lost. Our caches + // describe a different file. Adopt the on-disk value and flush everything (below). + + expectsChangesets = YES; + changesets = nil; + } } else { // We can just grab the snapshot from YapDatabase's in-memory version. - + dbSnapshot = [database snapshot]; } - - if (snapshot < dbSnapshot) + + if (!expectsChangesets && (snapshot < dbSnapshot)) { // The transaction hasn't processed recent changeset(s) yet. // We need to fetch them now. - + expectsChangesets = YES; changesets = [database pendingAndCommittedChangesetsSince:snapshot until:dbSnapshot]; } - + myState->lastTransactionSnapshot = dbSnapshot; myState->lastTransactionTime = mach_absolute_time(); needsMarkSqlLevelSharedReadLock = NO; @@ -3527,37 +3571,54 @@ - (void)postPseudoReadWriteTransaction * This allows us to validate the transaction, and check for a particular race condition. **/ - (uint64_t)readSnapshotFromDatabase +{ + return [self readSnapshotFromDatabase:NULL]; +} + +- (uint64_t)readSnapshotFromDatabase:(BOOL *)errorPtr { sqlite3_stmt *statement = [self yapGetDataForKeyStatement]; - if (statement == NULL) return 0; - + if (statement == NULL) + { + if (errorPtr) *errorPtr = YES; + return 0; + } + uint64_t result = 0; - + BOOL error = NO; + // SELECT data FROM 'yap2' WHERE extension = ? AND key = ? ; - + int const bind_idx_extension = SQLITE_BIND_START + 0; int const bind_idx_key = SQLITE_BIND_START + 1; - + const char *extension = ""; sqlite3_bind_text(statement, bind_idx_extension, extension, (int)strlen(extension), SQLITE_STATIC); - + const char *key = "snapshot"; sqlite3_bind_text(statement, bind_idx_key, key, (int)strlen(key), SQLITE_STATIC); - + int status = sqlite3_step(statement); if (status == SQLITE_ROW) { result = (uint64_t)sqlite3_column_int64(statement, SQLITE_COLUMN_START); } - else if (status == SQLITE_ERROR) + else if (status != SQLITE_DONE) { + // Any error (SQLITE_IOERR, SQLITE_BUSY, SQLITE_CORRUPT, ...), not just SQLITE_ERROR. + // In multiprocess mode this SELECT is the authoritative external-modification check, + // so callers must know it failed — returning a silent 0 would make them conclude + // "nothing changed" and trust stale caches. + YDBLogError(@"Error executing 'yapGetDataForKeyStatement': %d %s", status, sqlite3_errmsg(db)); + error = YES; } - + sqlite3_clear_bindings(statement); sqlite3_reset(statement); - + + if (errorPtr) *errorPtr = error; return result; } From 4e9630c48cbc733688ab07022272e168f686a959 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:11:11 -0400 Subject: [PATCH 07/16] Remove -wal/-shm sidecars during corruption recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- YapDatabase/YapDatabase.m | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/YapDatabase/YapDatabase.m b/YapDatabase/YapDatabase.m index 67ebedafc..264c95277 100644 --- a/YapDatabase/YapDatabase.m +++ b/YapDatabase/YapDatabase.m @@ -528,8 +528,32 @@ - (id)initWithURL:(NSURL *)inURL options:(nullable YapDatabaseOptions *)inOption YDBLogError(@"Error renaming corrupt database file: (%@ -> %@) %@", [databasePath lastPathComponent], [newDatabasePath lastPathComponent], error); } + else + { + // The -wal and -shm files MUST go with the main file. If a stale -wal + // is left behind, sqlite treats it as a hot journal for the NEW (empty) + // database file and replays the OLD database's WAL frames into it — + // resurrecting the very content we just declared corrupt. + + for (NSString *suffix in @[@"-wal", @"-shm"]) + { + NSString *sidecarPath = [databasePath stringByAppendingString:suffix]; + if ([[NSFileManager defaultManager] fileExistsAtPath:sidecarPath]) + { + [[NSFileManager defaultManager] moveItemAtPath: sidecarPath + toPath: [newDatabasePath stringByAppendingString:suffix] + error: NULL]; + // If the move fails, fall back to deleting — anything is better + // than the new database adopting the old WAL. + if ([[NSFileManager defaultManager] fileExistsAtPath:sidecarPath]) + { + [[NSFileManager defaultManager] removeItemAtPath:sidecarPath error:NULL]; + } + } + } + } } - + } while (i < INT_MAX && !renamed && !failed); if (renamed) @@ -552,9 +576,15 @@ - (id)initWithURL:(NSURL *)inURL options:(nullable YapDatabaseOptions *)inOption NSError *error = nil; BOOL deleted = [[NSFileManager defaultManager] removeItemAtPath:databasePath error:&error]; - + if (deleted) { + // The -wal and -shm files MUST go too, or sqlite replays the old WAL + // into the fresh database file (see the Rename branch above). + + [[NSFileManager defaultManager] removeItemAtPath:[databasePath stringByAppendingString:@"-wal"] error:NULL]; + [[NSFileManager defaultManager] removeItemAtPath:[databasePath stringByAppendingString:@"-shm"] error:NULL]; + isNewDatabaseFile = YES; result = openConfigCreate(); if (result) { From b51bdddf1f5a584722dde540f3bad2fc284a6842 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Mon, 6 Jul 2026 11:11:30 -0400 Subject: [PATCH 08/16] Honor default object/metadata policy in processChangeset mixed branch 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) --- YapDatabase/YapDatabaseConnection.m | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index e664efab2..bfa929d94 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -4343,11 +4343,11 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newObject != yapTouch) { YapDatabasePolicy objectPolicy = YapDatabasePolicyContainment; - NSNumber *op = objectPolicies[cacheKey.collection]; + NSNumber *op = objectPolicies[cacheKey.collection] ?: [database getDefaultObjectPolicy]; if (op) { objectPolicy = (YapDatabasePolicy)[op integerValue]; } - + if (objectPolicy == YapDatabasePolicyContainment) { [objectCache removeObjectForKey:cacheKey]; } @@ -4465,11 +4465,11 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newMetadata != yapTouch) { YapDatabasePolicy metadataPolicy = YapDatabasePolicyContainment; - NSNumber *mp = metadataPolicies[cacheKey.collection]; + NSNumber *mp = metadataPolicies[cacheKey.collection] ?: [database getDefaultMetadataPolicy]; if (mp) { metadataPolicy = (YapDatabasePolicy)[mp integerValue]; } - + if (metadataPolicy == YapDatabasePolicyContainment) { [metadataCache removeObjectForKey:cacheKey]; } From 85fc5c94814d58aeee66ee87b537eb1b39617b9b Mon Sep 17 00:00:00 2001 From: constantintin Date: Wed, 8 Jul 2026 10:15:43 +0200 Subject: [PATCH 09/16] Roll back failed commit even when COMMIT statement fails to prepare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The explicit-ROLLBACK recovery was nested inside if (statement), so when commitTransactionStatement returned NULL (sqlite3_prepare failure, e.g. SQLITE_NOMEM — likely right after a memory warning, since the default autoFlushMemoryFlags finalizes the cached statements) no ROLLBACK was ever issued. The caller performed a full in-memory rollback while the sqlite transaction stayed open; the next write's BEGIN would fail and its COMMIT would durably commit both transactions' rows, resurrecting supposedly rolled-back data under a snapshot whose changeset doesn't describe it. Hoist the recovery out of the if (statement) block, guarded by !sqlite3_get_autocommit so it only fires when a transaction is actually still open. Also fall back to sqlite3_exec("ROLLBACK") when the cached ROLLBACK statement itself fails to prepare, since a NOMEM that broke the COMMIT prepare would likely break that one too. Co-Authored-By: Claude Fable 5 --- YapDatabase/YapDatabaseTransaction.m | 48 ++++++++++++++++++---------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/YapDatabase/YapDatabaseTransaction.m b/YapDatabase/YapDatabaseTransaction.m index 2e7da12c2..e54df8778 100644 --- a/YapDatabase/YapDatabaseTransaction.m +++ b/YapDatabase/YapDatabaseTransaction.m @@ -178,28 +178,42 @@ - (BOOL)commitTransaction // later corrupts unrelated rows. YDBLogError(@"Couldn't commit transaction: %d %s", status, sqlite3_errmsg(connection->db)); + } - if (isReadWriteTransaction && !sqlite3_get_autocommit(connection->db)) - { - // sqlite did NOT auto-rollback; the transaction is still open. - // Roll it back explicitly so the outcome is deterministic - // (a zombie transaction would swallow every subsequent BEGIN/COMMIT). + sqlite3_reset(statement); + } - sqlite3_stmt *rbStatement = [connection rollbackTransactionStatement]; - if (rbStatement) - { - int rbStatus = sqlite3_step(rbStatement); - if (rbStatus != SQLITE_DONE) - { - YDBLogError(@"Couldn't rollback failed commit: %d %s", - rbStatus, sqlite3_errmsg(connection->db)); - } - sqlite3_reset(rbStatement); - } + if (!committed && isReadWriteTransaction && !sqlite3_get_autocommit(connection->db)) + { + // sqlite did NOT auto-rollback; the transaction is still open. This covers both a failed + // COMMIT step (above) and a NULL commitTransactionStatement (prepare failed, e.g. + // SQLITE_NOMEM after a memory warning flushed the cached statements). + // Roll it back explicitly so the outcome is deterministic + // (a zombie transaction would swallow every subsequent BEGIN/COMMIT). + + sqlite3_stmt *rbStatement = [connection rollbackTransactionStatement]; + if (rbStatement) + { + int rbStatus = sqlite3_step(rbStatement); + if (rbStatus != SQLITE_DONE) + { + YDBLogError(@"Couldn't rollback failed commit: %d %s", + rbStatus, sqlite3_errmsg(connection->db)); } + sqlite3_reset(rbStatement); } + else + { + // If preparing COMMIT failed with NOMEM, preparing ROLLBACK likely fails too. + // sqlite3_exec is a last-ditch attempt that doesn't depend on the statement cache. - sqlite3_reset(statement); + int rbStatus = sqlite3_exec(connection->db, "ROLLBACK TRANSACTION;", NULL, NULL, NULL); + if (rbStatus != SQLITE_OK) + { + YDBLogError(@"Couldn't rollback failed commit (exec): %d %s", + rbStatus, sqlite3_errmsg(connection->db)); + } + } } if (isReadWriteTransaction) From 954c6f6472306ed79a52b175bee403d1cc544a4b Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Wed, 8 Jul 2026 11:26:43 -0400 Subject: [PATCH 10/16] Use move return value for sidecar relocation during corruption rename Rely on -moveItemAtPath:toPath:error:'s return value instead of a separate -fileExistsAtPath: probe, which races. On any move failure (including a missing sidecar), fall back to deleting so the freshly renamed database can never adopt the old -wal/-shm. Co-Authored-By: Claude Opus 4.8 (1M context) --- YapDatabase/YapDatabase.m | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/YapDatabase/YapDatabase.m b/YapDatabase/YapDatabase.m index 264c95277..1d214c141 100644 --- a/YapDatabase/YapDatabase.m +++ b/YapDatabase/YapDatabase.m @@ -538,17 +538,16 @@ - (id)initWithURL:(NSURL *)inURL options:(nullable YapDatabaseOptions *)inOption for (NSString *suffix in @[@"-wal", @"-shm"]) { NSString *sidecarPath = [databasePath stringByAppendingString:suffix]; - if ([[NSFileManager defaultManager] fileExistsAtPath:sidecarPath]) + + // Rely on the move's own return value rather than a separate + // fileExistsAtPath probe (which races). If the move fails — including + // because the sidecar doesn't exist — fall back to deleting, since + // anything is better than the new database adopting the old WAL. + if (![[NSFileManager defaultManager] moveItemAtPath: sidecarPath + toPath: [newDatabasePath stringByAppendingString:suffix] + error: NULL]) { - [[NSFileManager defaultManager] moveItemAtPath: sidecarPath - toPath: [newDatabasePath stringByAppendingString:suffix] - error: NULL]; - // If the move fails, fall back to deleting — anything is better - // than the new database adopting the old WAL. - if ([[NSFileManager defaultManager] fileExistsAtPath:sidecarPath]) - { - [[NSFileManager defaultManager] removeItemAtPath:sidecarPath error:NULL]; - } + [[NSFileManager defaultManager] removeItemAtPath:sidecarPath error:NULL]; } } } From 7bb4cfb01531e2e62d2c6cd71b5f7f964de87ee1 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Wed, 8 Jul 2026 11:26:51 -0400 Subject: [PATCH 11/16] Hoist default policy lookup out of per-key processChangeset loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getDefaultObjectPolicy/getDefaultMetadataPolicy each take the database's configLock. Calling them inside the per-key keysToUpdate loops (and the no-removal shortcut blocks) took the shared unfair lock once per changed key, on every connection, for every commit. Read the default once per changeset instead — also a consistent policy snapshot across the batch. Co-Authored-By: Claude Opus 4.8 (1M context) --- YapDatabase/YapDatabaseConnection.m | 48 +++++++++++++++++++---------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index bfa929d94..76b838dcc 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -4258,13 +4258,17 @@ - (void)processChangeset:(NSDictionary *)changeset id yapNull = [YapNull null]; // value == yapNull : setPrimitive or containment policy id yapTouch = [YapTouch touch]; // value == yapTouch : touchObjectForKey: was used - + + // Read the default policy once (it takes database's configLock) rather than + // re-fetching it per key inside the loop. + NSNumber *defaultObjectPolicy = [database getDefaultObjectPolicy]; + [changeset_objectChanges enumerateKeysAndObjectsUsingBlock:^(id key, id newObject, BOOL __unused *stop) { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wimplicit-retain-self" - + __unsafe_unretained YapCollectionKey *cacheKey = (YapCollectionKey *)key; - + if ([objectCache containsKey:cacheKey]) { if (newObject == yapNull) @@ -4274,7 +4278,7 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newObject != yapTouch) { YapDatabasePolicy objectPolicy = YapDatabasePolicyContainment; - NSNumber *op = objectPolicies[cacheKey.collection] ?: [database getDefaultObjectPolicy]; + NSNumber *op = objectPolicies[cacheKey.collection] ?: defaultObjectPolicy; if (op) { objectPolicy = (YapDatabasePolicy)[op integerValue]; } @@ -4328,14 +4332,18 @@ - (void)processChangeset:(NSDictionary *)changeset }]; [objectCache removeObjectsForKeys:keysToRemove]; - + id yapNull = [YapNull null]; // value == yapNull : setPrimitive or containment policy id yapTouch = [YapTouch touch]; // value == yapTouch : touchObjectForKey: was used - + + // Read the default policy once (it takes database's configLock) rather than + // re-fetching it per key inside the loop. + NSNumber *defaultObjectPolicy = [database getDefaultObjectPolicy]; + for (YapCollectionKey *cacheKey in keysToUpdate) { id newObject = [changeset_objectChanges objectForKey:cacheKey]; - + if (newObject == yapNull) { [objectCache removeObjectForKey:cacheKey]; @@ -4343,7 +4351,7 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newObject != yapTouch) { YapDatabasePolicy objectPolicy = YapDatabasePolicyContainment; - NSNumber *op = objectPolicies[cacheKey.collection] ?: [database getDefaultObjectPolicy]; + NSNumber *op = objectPolicies[cacheKey.collection] ?: defaultObjectPolicy; if (op) { objectPolicy = (YapDatabasePolicy)[op integerValue]; } @@ -4380,13 +4388,17 @@ - (void)processChangeset:(NSDictionary *)changeset id yapNull = [YapNull null]; // value == yapNull : setPrimitive or containment policy id yapTouch = [YapTouch touch]; // value == yapTouch : touchObjectForKey: was used - + + // Read the default policy once (it takes database's configLock) rather than + // re-fetching it per key inside the loop. + NSNumber *defaultMetadataPolicy = [database getDefaultMetadataPolicy]; + [changeset_metadataChanges enumerateKeysAndObjectsUsingBlock:^(id key, id newMetadata, BOOL __unused *stop) { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wimplicit-retain-self" - + __unsafe_unretained YapCollectionKey *cacheKey = (YapCollectionKey *)key; - + if ([metadataCache containsKey:cacheKey]) { if (newMetadata == yapNull) @@ -4396,7 +4408,7 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newMetadata != yapTouch) { YapDatabasePolicy metadataPolicy = YapDatabasePolicyContainment; - NSNumber *mp = metadataPolicies[cacheKey.collection] ?: [database getDefaultMetadataPolicy]; + NSNumber *mp = metadataPolicies[cacheKey.collection] ?: defaultMetadataPolicy; if (mp) { metadataPolicy = (YapDatabasePolicy)[mp integerValue]; } @@ -4450,14 +4462,18 @@ - (void)processChangeset:(NSDictionary *)changeset }]; [metadataCache removeObjectsForKeys:keysToRemove]; - + id yapNull = [YapNull null]; // value == yapNull : setPrimitive or containment policy id yapTouch = [YapTouch touch]; // value == yapTouch : touchObjectForKey: was used - + + // Read the default policy once (it takes database's configLock) rather than + // re-fetching it per key inside the loop. + NSNumber *defaultMetadataPolicy = [database getDefaultMetadataPolicy]; + for (YapCollectionKey *cacheKey in keysToUpdate) { id newMetadata = [changeset_metadataChanges objectForKey:cacheKey]; - + if (newMetadata == yapNull) { [metadataCache removeObjectForKey:cacheKey]; @@ -4465,7 +4481,7 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newMetadata != yapTouch) { YapDatabasePolicy metadataPolicy = YapDatabasePolicyContainment; - NSNumber *mp = metadataPolicies[cacheKey.collection] ?: [database getDefaultMetadataPolicy]; + NSNumber *mp = metadataPolicies[cacheKey.collection] ?: defaultMetadataPolicy; if (mp) { metadataPolicy = (YapDatabasePolicy)[mp integerValue]; } From dab691a83a3d3bbcec0d7124e1e2b4e82c4dfb4b Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Wed, 8 Jul 2026 12:06:19 -0400 Subject: [PATCH 12/16] Drop redundant import for Xcode 26 module builds already pulls in on Darwin. Importing the private header directly trips Xcode 26's stricter module rules ("Use of private header from outside its module"), breaking the framework build. Removing it restores compilation with no functional change. Co-Authored-By: Claude Opus 4.8 (1M context) --- YapDatabase/Extensions/ActionManager/Utilities/YapReachability.m | 1 - 1 file changed, 1 deletion(-) diff --git a/YapDatabase/Extensions/ActionManager/Utilities/YapReachability.m b/YapDatabase/Extensions/ActionManager/Utilities/YapReachability.m index d9726a627..68d6ae7c4 100644 --- a/YapDatabase/Extensions/ActionManager/Utilities/YapReachability.m +++ b/YapDatabase/Extensions/ActionManager/Utilities/YapReachability.m @@ -46,7 +46,6 @@ CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF #import #import -#import #import #import #import From 782b8616c2b197765062e8ca2ad01d1afa26a923 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Wed, 8 Jul 2026 12:06:33 -0400 Subject: [PATCH 13/16] Roll back transaction when a bulk DELETE fails removeObjectsForKeys:inCollection: and removeAllObjectsInCollection: could abort a failed bulk DELETE by returning early while the surrounding transaction went on to commit its other changes. This left two distinct inconsistencies: - removeAllObjectsInCollection: records the collection in the changeset's removedCollections (and strips its pending per-key changes) BEFORE the DELETE. A failed DELETE then committed a phantom collection-removal for rows still on disk. The no-extensions shortcut wasn't guarded at all and committed anyway after merely logging the error. - removeObjectsForKeys:inCollection: invokes willRemoveObjectsForKeys: on every extension before the DELETE. A failed DELETE left the will-hook unpaired, rows on disk, and no error surfaced. Treat a failed bulk DELETE like a failed COMMIT (see "Treat a failed sqlite COMMIT as a rollback"): mark the transaction for rollback so nothing is committed and extensions are told via didRollbackTransaction. Covers the step-failure, prepare-failure, and shortcut paths in both methods. Adds regression tests that force the DELETE step to fail deterministically (PRAGMA query_only) and verify the whole transaction rolls back: the rows survive, the rollback flag is set, and an unrelated write in the same transaction is discarded rather than silently committed. Co-Authored-By: Claude Opus 4.8 (1M context) --- Testing/UnitTesting/TestYapDatabase.m | 129 ++++++++++++++++++++++++++ YapDatabase/YapDatabaseTransaction.m | 72 ++++++++++---- 2 files changed, 182 insertions(+), 19 deletions(-) diff --git a/Testing/UnitTesting/TestYapDatabase.m b/Testing/UnitTesting/TestYapDatabase.m index bfd13ca8c..15a6eacd2 100644 --- a/Testing/UnitTesting/TestYapDatabase.m +++ b/Testing/UnitTesting/TestYapDatabase.m @@ -1336,4 +1336,133 @@ - (void)testRollbackFlushesKeyCache_rowidReuseCorruption @"the write for collA landed on the wrong row (rollback did not flush keyCache)"); } +// Regression test: a bulk DELETE that fails mid-transaction must roll the whole transaction back, +// not silently commit its other changes. +// +// removeObjectsForKeys:inCollection: invokes willRemoveObjectsForKeys: on every extension BEFORE +// stepping the DELETE. If the DELETE step fails, the old code just returned: the will-hook had +// already fired (with no matching did-hook), the rows were left on disk, no error was surfaced, and +// the transaction went on to COMMIT its unrelated writes. The fix treats a failed bulk DELETE like +// a failed COMMIT -> the entire transaction is rolled back. +// +// The DELETE is forced to fail deterministically by toggling `PRAGMA query_only` ON around the call +// (a write step then returns SQLITE_READONLY), and OFF again before the block ends. +- (void)testFailedBulkDeleteRollsBackTransaction_removeObjectsForKeys +{ + NSURL *databaseURL = [self databaseURL:NSStringFromSelector(_cmd)]; + [[NSFileManager defaultManager] removeItemAtURL:databaseURL error:NULL]; + + YapDatabase *database = [[YapDatabase alloc] initWithURL:databaseURL]; + XCTAssertNotNil(database); + + YapDatabaseConnection *connection = [database newConnection]; + + NSString *const collection = @"docs"; + NSString *const sentinelCollection = @"unrelated"; + NSArray *const keys = @[@"k1", @"k2", @"k3", @"k4"]; // >1 forces the bulk (non single-key) path + + // Seed the collection in a committed transaction. + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + for (NSString *key in keys) { + [transaction setObject:key forKey:key inCollection:collection]; + } + }]; + + __block BOOL rollbackWasSet = NO; + + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + + // An unrelated write in the SAME transaction. If the transaction is (wrongly) allowed to + // commit, this sentinel survives; if it is correctly rolled back, it disappears. + [transaction setObject:@"sentinel" forKey:@"sentinel" inCollection:sentinelCollection]; + + // Make the next write step fail with SQLITE_READONLY. + int rc = sqlite3_exec(connection->db, "PRAGMA query_only=ON;", NULL, NULL, NULL); + XCTAssertEqual(rc, SQLITE_OK, @"failed to enable query_only"); + + [transaction removeObjectsForKeys:keys inCollection:collection]; + + rollbackWasSet = transaction->rollback; + + // Restore normal mode so the transaction machinery (rollback) runs unhindered. + rc = sqlite3_exec(connection->db, "PRAGMA query_only=OFF;", NULL, NULL, NULL); + XCTAssertEqual(rc, SQLITE_OK, @"failed to disable query_only"); + }]; + + XCTAssertTrue(rollbackWasSet, + @"a failed bulk DELETE should mark the transaction for rollback"); + + // Read from a FRESH connection so we observe what is actually on disk (cold caches). + YapDatabaseConnection *verify = [database newConnection]; + [verify readWithBlock:^(YapDatabaseReadTransaction *transaction) { + + XCTAssertEqual([transaction numberOfKeysInCollection:collection], (NSUInteger)[keys count], + @"the failed DELETE must not have removed any rows"); + for (NSString *key in keys) { + XCTAssertEqualObjects([transaction objectForKey:key inCollection:collection], key, + @"row %@ should be untouched", key); + } + + XCTAssertFalse([transaction hasObjectForKey:@"sentinel" inCollection:sentinelCollection], + @"the transaction silently committed its other writes instead of rolling back"); + }]; +} + +// Regression test: removeAllObjectsInCollection: takes a no-extensions "shortcut" that adds the +// collection to the changeset's removedCollections BEFORE stepping the DELETE. If that DELETE +// failed, the old shortcut logged the error but committed anyway -> peers received a phantom +// collection-removal for rows still on disk. The fix rolls the transaction back instead. +// +// (This exercises the shortcut path specifically: no extensions are registered.) +- (void)testFailedRemoveAllInCollectionRollsBack_shortcutPath +{ + NSURL *databaseURL = [self databaseURL:NSStringFromSelector(_cmd)]; + [[NSFileManager defaultManager] removeItemAtURL:databaseURL error:NULL]; + + YapDatabase *database = [[YapDatabase alloc] initWithURL:databaseURL]; + XCTAssertNotNil(database); + + YapDatabaseConnection *connection = [database newConnection]; + + NSString *const collection = @"docs"; + NSString *const sentinelCollection = @"unrelated"; + NSArray *const keys = @[@"k1", @"k2", @"k3"]; + + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + for (NSString *key in keys) { + [transaction setObject:key forKey:key inCollection:collection]; + } + }]; + + __block BOOL rollbackWasSet = NO; + + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + + [transaction setObject:@"sentinel" forKey:@"sentinel" inCollection:sentinelCollection]; + + int rc = sqlite3_exec(connection->db, "PRAGMA query_only=ON;", NULL, NULL, NULL); + XCTAssertEqual(rc, SQLITE_OK, @"failed to enable query_only"); + + [transaction removeAllObjectsInCollection:collection]; + + rollbackWasSet = transaction->rollback; + + rc = sqlite3_exec(connection->db, "PRAGMA query_only=OFF;", NULL, NULL, NULL); + XCTAssertEqual(rc, SQLITE_OK, @"failed to disable query_only"); + }]; + + XCTAssertTrue(rollbackWasSet, + @"a failed removeAllObjectsInCollection: should mark the transaction for rollback"); + + YapDatabaseConnection *verify = [database newConnection]; + [verify readWithBlock:^(YapDatabaseReadTransaction *transaction) { + + XCTAssertEqual([transaction numberOfKeysInCollection:collection], (NSUInteger)[keys count], + @"the failed DELETE must not have removed the collection's rows"); + + XCTAssertFalse([transaction hasObjectForKey:@"sentinel" inCollection:sentinelCollection], + @"the transaction broadcast a phantom removal and committed instead of rolling back"); + }]; +} + @end diff --git a/YapDatabase/YapDatabaseTransaction.m b/YapDatabase/YapDatabaseTransaction.m index e54df8778..3b56ef3db 100644 --- a/YapDatabase/YapDatabaseTransaction.m +++ b/YapDatabase/YapDatabaseTransaction.m @@ -5739,7 +5739,12 @@ - (void)removeObjectsForKeys:(NSArray *)keys inCollection:(NSString *)collection { YDBLogError(@"Error creating 'removeKeys:inCollection:' statement (B): %d %s", status, sqlite3_errmsg(connection->db)); - + + // Earlier batches may already have been removed (caches + didRemove hooks). Bailing + // out now would commit a partial removal. Roll the whole transaction back instead. + + rollback = YES; + FreeYapDatabaseString(&_collection); return; } @@ -5768,9 +5773,14 @@ - (void)removeObjectsForKeys:(NSArray *)keys inCollection:(NSString *)collection YDBLogError(@"Error executing 'removeKeys:inCollection:' statement (B): %d %s", status, sqlite3_errmsg(connection->db)); - // The DELETE didn't happen. Do NOT update caches/changeset or tell extensions the - // rows were removed — that would durably desync extension tables (views/indexes) - // and peers from the main table. Abort the remaining batches too. + // The DELETE didn't happen, but we already invoked willRemoveObjectsForKeys: on + // every extension for this batch (and may have fully removed earlier batches). + // Silently returning would leave will/did hooks unpaired, the rows on disk, and no + // error surfaced — yet the transaction would still commit its OTHER changes. + // Treat a failed bulk DELETE like a failed COMMIT: roll the whole transaction back + // so nothing is committed and extensions are told via didRollbackTransaction. + + rollback = YES; FreeYapDatabaseString(&_collection); return; @@ -5925,29 +5935,44 @@ - (void)removeAllObjectsInCollection:(NSString *)collection if ([[self extensions] count] == 0) { sqlite3_stmt *statement = [connection removeCollectionStatement]; - if (statement == NULL) return; - + if (statement == NULL) + { + // removedCollections was already recorded above; committing a phantom collection-removal + // (for rows still on disk) must not happen. Roll the whole transaction back. + + rollback = YES; + return; + } + // DELETE FROM "database2" WHERE "collection" = ?; - + int const bind_idx_collection = SQLITE_BIND_START; - + YapDatabaseString _collection; MakeYapDatabaseString(&_collection, collection); sqlite3_bind_text(statement, bind_idx_collection, _collection.str, _collection.length, SQLITE_STATIC); - + int status = sqlite3_step(statement); + + sqlite3_clear_bindings(statement); + sqlite3_reset(statement); + FreeYapDatabaseString(&_collection); + if (status != SQLITE_DONE) { YDBLogError(@"Error executing 'removeCollectionStatement': %d %s, collection(%@)", status, sqlite3_errmsg(connection->db), collection); + + // The DELETE failed, but removedCollections was already recorded above. Committing would + // broadcast a phantom collection-removal for rows still on disk. Roll the transaction + // back instead. (Previously this path logged the error but committed anyway.) + + rollback = YES; + return; } - - sqlite3_clear_bindings(statement); - sqlite3_reset(statement); - FreeYapDatabaseString(&_collection); - + connection->hasDiskChanges = YES; [connection->mutationStack markAsMutated]; // mutation during enumeration protection - + return; } // end shortcut @@ -6058,7 +6083,12 @@ - (void)removeAllObjectsInCollection:(NSString *)collection { YDBLogError(@"Error creating 'removeAllObjectsInCollection:' statement: %d %s", status, sqlite3_errmsg(connection->db)); - + + // removedCollections was already recorded before the loop (see above). Bailing out + // now would commit a phantom collection-removal. Roll the transaction back instead. + + rollback = YES; + FreeYapDatabaseString(&_collection); return; } @@ -6087,9 +6117,13 @@ - (void)removeAllObjectsInCollection:(NSString *)collection YDBLogError(@"Error executing 'removeAllObjectsInCollection:' statement: %d %s", status, sqlite3_errmsg(connection->db)); - // The DELETE didn't happen. Do NOT update caches/changeset or tell extensions the - // rows were removed — that would durably desync extension tables (views/indexes) - // and peers from the main table. Abort the remaining batches too. + // The DELETE didn't happen, but this method already added the collection to the + // changeset's removedCollections (and stripped its pending per-key changes) before + // the loop. Committing would broadcast a phantom collection-removal for rows still + // on disk, desyncing extension tables and peers. Treat a failed bulk DELETE like a + // failed COMMIT: roll the whole transaction back so nothing is committed. + + rollback = YES; FreeYapDatabaseString(&_collection); return; From 49424c90abd1eff52aeb2ccbf33eec54840c8102 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Wed, 8 Jul 2026 12:38:30 -0400 Subject: [PATCH 14/16] Handle failed COMMIT in extension registration and memory tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failed-COMMIT hardening fixes surfaced by review: registerExtension:withName: returned the result of createIfNeeded (YES) without consulting whether the sqlite COMMIT actually succeeded. On a failed commit the extension's tables were rolled back, yet the connection kept it in registeredExtensions and reported success — split-brain state that no peer ever learned of (the changeset was retracted). postReadWrite- Transaction: now returns the commit outcome, and registerExtension: undoes its in-memory bookkeeping and returns NO when the commit fails. YapMemoryTableTransaction's commit pushes a (snapshot, changedKeys) entry onto the table's history. preCommitReadWriteTransaction commits the memory tables before the sqlite COMMIT; when that COMMIT fails, commitTransaction rolls the memory tables back — but rollback reverted the stored values only, leaking the history entry. After the caller's snapshot-- and a retry, the same snapshot was pushed again, breaking the array's ordered-unique invariant. rollback now removes the entry the matching commit pushed. Adds regression tests for both: a focused YapMemoryTable commit/rollback test (asserting no leaked/duplicate history), and a registration test that forces the COMMIT to fail via a sqlite commit hook and asserts registerExtension: returns NO and leaves nothing registered. Co-Authored-By: Claude Opus 4.8 (1M context) --- Testing/UnitTesting/TestYapDatabase.m | 95 +++++++++++++++++++++++++++ YapDatabase/Internal/YapMemoryTable.m | 36 +++++++++- YapDatabase/YapDatabaseConnection.m | 41 +++++++++--- 3 files changed, 161 insertions(+), 11 deletions(-) diff --git a/Testing/UnitTesting/TestYapDatabase.m b/Testing/UnitTesting/TestYapDatabase.m index 15a6eacd2..e461bb716 100644 --- a/Testing/UnitTesting/TestYapDatabase.m +++ b/Testing/UnitTesting/TestYapDatabase.m @@ -6,6 +6,8 @@ #import #import #import +#import +#import #if PODFILE_USE_FRAMEWORKS // Works with `use_frameworks`, but not with `use_modular_headers` @@ -15,6 +17,19 @@ #import "YapProxyObjectPrivate.h" #endif +// Returning non-zero from a sqlite commit hook converts the pending COMMIT into a ROLLBACK and makes +// the COMMIT step return an error — a deterministic way to exercise the failed-commit code paths. +static int YDBTestFailCommitHook(void *context) +{ + return 1; +} + +// The registration connection is private; declaring the accessor lets the failed-commit test reach +// its sqlite handle to install the commit hook above. +@interface YapDatabase (YDBTestRegistrationConnection) +- (YapDatabaseConnection *)registrationConnection; +@end + @interface TestYapDatabase : XCTestCase @end @@ -1465,4 +1480,84 @@ - (void)testFailedRemoveAllInCollectionRollsBack_shortcutPath }]; } +// Committing a memory-table transaction and then rolling it back — exactly what +// happens when preCommitReadWriteTransaction commits the memory tables and the sqlite COMMIT then +// fails — must remove the (snapshot, changedKeys) entry that the commit pushed onto the table's +// history. Otherwise the entry leaks, and a retry that reuses the same snapshot number pushes a +// duplicate, breaking the history array's ordered-unique invariant (and asyncCheckpoint never drains +// it once the snapshot regresses). +- (void)testMemoryTableRollbackAfterCommitDoesNotLeakHistory +{ + YapMemoryTable *table = [[YapMemoryTable alloc] initWithKeyClass:[NSString class]]; + + YapMemoryTableTransaction *txn = [table newReadWriteTransactionWithSnapshot:1]; + [txn setObject:@"value" forKey:@"key"]; + [txn commit]; // preCommitReadWriteTransaction commits the memory tables (pushes history) + [txn rollback]; // commitTransaction rolls them back after the sqlite COMMIT failed + + XCTAssertEqual([(NSArray *)[table valueForKey:@"snapshots"] count], (NSUInteger)0, + @"rollback after commit must remove the pushed history entry (snapshots)"); + XCTAssertEqual([(NSArray *)[table valueForKey:@"changes"] count], (NSUInteger)0, + @"rollback after commit must remove the pushed history entry (changes)"); + + // A retry that reuses the same snapshot number (the enclosing connection did snapshot--) must + // leave exactly one history entry, not a duplicate. + YapMemoryTableTransaction *retry = [table newReadWriteTransactionWithSnapshot:1]; + [retry setObject:@"value2" forKey:@"key"]; + [retry commit]; + + XCTAssertEqual([(NSArray *)[table valueForKey:@"snapshots"] count], (NSUInteger)1, + @"retry at the same snapshot must not create a duplicate history entry"); +} + +// registerExtension: must return NO when the sqlite COMMIT that would persist +// the extension fails, and must not leave the extension half-registered. Previously it returned the +// result of createIfNeeded (YES) regardless of the commit outcome, leaving split-brain state: the +// connection believed the extension existed while its tables had been rolled back, and no peer ever +// learned of it. +// +// The registration COMMIT is forced to fail with a sqlite commit hook installed on the (primed) +// registration connection. +- (void)testRegisterExtensionReturnsNOWhenCommitFails +{ + NSURL *databaseURL = [self databaseURL:NSStringFromSelector(_cmd)]; + [[NSFileManager defaultManager] removeItemAtURL:databaseURL error:NULL]; + + YapDatabase *database = [[YapDatabase alloc] initWithURL:databaseURL]; + XCTAssertNotNil(database); + + YapDatabaseViewGrouping *grouping = [YapDatabaseViewGrouping withKeyBlock: + ^NSString *(YapDatabaseReadTransaction *transaction, NSString *collection, NSString *key){ + return @""; + }]; + YapDatabaseViewSorting *sorting = [YapDatabaseViewSorting withObjectBlock: + ^(YapDatabaseReadTransaction *transaction, NSString *group, + NSString *collection1, NSString *key1, id obj1, + NSString *collection2, NSString *key2, id obj2) + { + return NSOrderedSame; + }]; + + // Prime the lazily-created registration connection with a successful registration. + YapDatabaseAutoView *viewA = + [[YapDatabaseAutoView alloc] initWithGrouping:grouping sorting:sorting versionTag:@"1" options:nil]; + XCTAssertTrue([database registerExtension:viewA withName:@"viewA"], + @"priming registration should succeed"); + + // Make the NEXT registration's COMMIT fail (createIfNeeded still succeeds). + YapDatabaseConnection *regConn = [database registrationConnection]; + sqlite3_commit_hook(regConn->db, YDBTestFailCommitHook, NULL); + + YapDatabaseAutoView *viewB = + [[YapDatabaseAutoView alloc] initWithGrouping:grouping sorting:sorting versionTag:@"1" options:nil]; + BOOL didRegisterB = [database registerExtension:viewB withName:@"viewB"]; + + sqlite3_commit_hook(regConn->db, NULL, NULL); // remove the hook + + XCTAssertFalse(didRegisterB, + @"registerExtension: must return NO when the commit fails"); + XCTAssertNil([[database registeredExtensions] objectForKey:@"viewB"], + @"a failed-commit registration must not leave the extension registered"); +} + @end diff --git a/YapDatabase/Internal/YapMemoryTable.m b/YapDatabase/Internal/YapMemoryTable.m index c21f3ab66..0578165d9 100644 --- a/YapDatabase/Internal/YapMemoryTable.m +++ b/YapDatabase/Internal/YapMemoryTable.m @@ -58,10 +58,11 @@ @interface YapMemoryTableTransaction () { @public __unsafe_unretained YapMemoryTable *table; - + uint64_t snapshot; BOOL isReadWriteTransaction; - + BOOL didCommit; + NSMutableSet *changedKeys; } @end @@ -643,6 +644,8 @@ - (void)commit [table->changes addObject:changedKeys]; } [table->lock unlock]; + + didCommit = YES; } } @@ -650,6 +653,35 @@ - (void)rollback { if (isReadWriteTransaction && [changedKeys count] > 0) { + if (didCommit) + { + // This transaction was already committed (which pushed a (snapshot, changedKeys) entry + // onto the table's history) before something forced the enclosing database transaction + // to roll back — e.g. the sqlite COMMIT failed after preCommitReadWriteTransaction had + // committed the memory tables. asyncRollback (below) reverts the stored values, but the + // history entry must ALSO be removed. Otherwise it leaks (asyncCheckpoint drains from + // the front and never reaches it once the snapshot regresses) and, after the caller's + // snapshot-- + retry, the same snapshot number is pushed again — breaking the array's + // ordered-unique invariant. + // + // Only this connection's writer touches these history arrays, and it does so serially, + // so our entry is still the last one; asyncCheckpoint only ever removes from the front. + + [table->lock lock]; + { + NSUInteger count = [table->snapshots count]; + if (count > 0 && + [[table->snapshots objectAtIndex:(count - 1)] unsignedLongLongValue] == snapshot) + { + [table->snapshots removeObjectAtIndex:(count - 1)]; + [table->changes removeObjectAtIndex:(count - 1)]; + } + } + [table->lock unlock]; + + didCommit = NO; + } + [table asyncRollback:snapshot withChanges:changedKeys]; } } diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index 76b838dcc..879e468e8 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -2934,8 +2934,12 @@ - (void)preReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction * This method must be invoked from within the connectionQueue. * This method must be invoked from within the database.writeQueue. **/ -- (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction +- (BOOL)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction { + // Whether the sqlite COMMIT succeeded. Stays NO for a deliberate rollback and for a failed + // commit; callers needing the durable outcome (e.g. registerExtension:) consult the return value. + BOOL committed = NO; + if (transaction->rollback) { YDBLogVerbose(@"YapDatabaseConnection(%p) rollback read-write transaction", self); @@ -3185,7 +3189,7 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction // from the database. If it doesn't match what we expect, then we know we've run into the race condition, // and we make the read-only transaction back out and try again. - BOOL committed = [transaction commitTransaction]; + committed = [transaction commitTransaction]; if (!committed) { // The sqlite COMMIT failed (e.g. disk full / I/O error while appending to the WAL), @@ -3341,10 +3345,12 @@ - (void)postReadWriteTransaction:(YapDatabaseReadWriteTransaction *)transaction removedRowids = nil; [mutationStack clear]; - + // Drop IsOnConnectionQueueKey flag from writeQueue since we're exiting writeQueue. - + dispatch_queue_set_specific(database->writeQueue, IsOnConnectionQueueKey, NULL, NULL); + + return committed; } /** @@ -5266,16 +5272,33 @@ - (BOOL)registerExtension:(YapDatabaseExtension *)extension withName:(NSString * else { // Registration failed. - + [transaction rollback]; } - - [self postReadWriteTransaction:transaction]; + + BOOL committed = [self postReadWriteTransaction:transaction]; + + if (result && !committed) + { + // createIfNeeded succeeded, but the sqlite COMMIT that would have persisted the + // extension's tables failed (and was rolled back). The in-memory registration + // bookkeeping was already populated above, so leaving it would report a phantom + // registration: this connection would believe the extension exists while its tables do + // not, and no peer ever learns of it (the changeset was retracted). Undo the bookkeeping + // and report failure. The database-level registeredExtensions never saw it. + + result = NO; + + [self didUnregisterExtensionWithName:extensionName]; + [self removeRegisteredExtensionConnectionWithName:extensionName]; + [transaction removeRegisteredExtensionTransactionWithName:extensionName]; + } + registeredExtensionsChanged = NO; - + #pragma clang diagnostic pop }}); - + return result; } From e07070807b333ef68259d245b68a04423a92efc4 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Wed, 8 Jul 2026 12:43:59 -0400 Subject: [PATCH 15/16] Guard backwards-snapshot branch in preReadTransaction for single-process mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit preReadTransaction's "database replaced underneath us" branch fired whenever the on-disk snapshot lagged the connection's in-memory snapshot. In single- process mode that lag is legitimate: a touch-only/in-memory commit bumps the memory snapshot without writing the yap2 snapshot row. Any subsequent read that took the disk-read path (a long-lived read, a cold wal_file, or a concurrent writer) then wrongly concluded the file was swapped and flushed all caches + extension/view state, regressed the snapshot, and re-processed the same changeset — cache thrash and double-applied view changesets on every such begin. Guard the branch with enableMultiProcessSupport, mirroring the equivalent branch in preWriteTransaction. Only in multiprocess mode can the on-disk snapshot going backwards genuinely mean external modification. Adds a regression test: after a touch-only commit, beginning a long-lived read must not regress the connection's snapshot (and data stays readable). Co-Authored-By: Claude Opus 4.8 (1M context) --- Testing/UnitTesting/TestYapDatabase.m | 51 +++++++++++++++++++++++++++ YapDatabase/YapDatabaseConnection.m | 8 ++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/Testing/UnitTesting/TestYapDatabase.m b/Testing/UnitTesting/TestYapDatabase.m index e461bb716..2cd499ca3 100644 --- a/Testing/UnitTesting/TestYapDatabase.m +++ b/Testing/UnitTesting/TestYapDatabase.m @@ -1560,4 +1560,55 @@ - (void)testRegisterExtensionReturnsNOWhenCommitFails @"a failed-commit registration must not leave the extension registered"); } +// Regression test (#4): in single-process mode a touch-only/in-memory commit bumps the connection's +// in-memory snapshot without writing the on-disk (yap2) snapshot row, so the on-disk snapshot +// legitimately lags. preReadTransaction's "database replaced underneath us" branch must not fire in +// that case (it's guarded by enableMultiProcessSupport). Previously it did, so any read that took +// the disk-read path — e.g. beginLongLivedReadTransaction — flushed all caches and regressed the +// connection's snapshot on every begin. +- (void)testTouchThenLongLivedReadDoesNotRegressSnapshot +{ + NSURL *databaseURL = [self databaseURL:NSStringFromSelector(_cmd)]; + [[NSFileManager defaultManager] removeItemAtURL:databaseURL error:NULL]; + + YapDatabase *database = [[YapDatabase alloc] initWithURL:databaseURL]; + XCTAssertNotNil(database); // single-process mode (enableMultiProcessSupport defaults to NO) + + YapDatabaseConnection *connection = [database newConnection]; + + NSString *const collection = @"docs"; + NSString *const key = @"k"; + + // A real write (advances both the in-memory and on-disk snapshot). + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [transaction setObject:@"v" forKey:key inCollection:collection]; + }]; + + // A touch-only commit: bumps the in-memory snapshot but writes nothing to disk, so the on-disk + // snapshot now lags the connection's in-memory snapshot. + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [transaction touchObjectForKey:key inCollection:collection]; + }]; + + uint64_t snapshotBefore = connection.snapshot; + + // Begin a long-lived read. This forces preReadTransaction down the disk-read path, where it + // reads the (lagging) on-disk snapshot. It must NOT conclude the file was swapped. + [connection beginLongLivedReadTransaction]; + + uint64_t snapshotAfter = connection.snapshot; + + __block id value = nil; + [connection readWithBlock:^(YapDatabaseReadTransaction *transaction) { + value = [transaction objectForKey:key inCollection:collection]; + }]; + + [connection endLongLivedReadTransaction]; + + XCTAssertEqual(snapshotAfter, snapshotBefore, + @"beginning a read after a touch-only commit must not regress the connection's snapshot"); + XCTAssertEqualObjects(value, @"v", + @"data must remain readable and correct"); +} + @end diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index 879e468e8..0ef7e0a26 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -2460,12 +2460,18 @@ - (void)preReadTransaction:(YapDatabaseReadTransaction *)transaction expectsChangesets = YES; changesets = [database pendingAndCommittedChangesetsSince:snapshot until:dbSnapshot]; } - else if (dbSnapshot < snapshot) + else if (enableMultiProcessSupport && (dbSnapshot < snapshot)) { // The on-disk snapshot went BACKWARDS relative to us. Normal operation cannot // produce this; it means the database file changed underneath us (restored from // a backup, replaced by another tool) or the snapshot row was lost. Our caches // describe a different file. Adopt the on-disk value and flush everything (below). + // + // Guarded by enableMultiProcessSupport (mirroring preWriteTransaction): in single- + // process mode the on-disk snapshot legitimately lags our in-memory one after a + // touch-only/in-memory commit (which bumps the memory snapshot without writing the + // yap2 snapshot row). Treating that as a file swap here would flush every cache and + // regress the snapshot on each long-lived / cold-wal read begin — pure thrash. expectsChangesets = YES; changesets = nil; From 7c2c4cbaa640c3662f46951cb49427e2967c19a9 Mon Sep 17 00:00:00 2001 From: Kent Sutherland Date: Wed, 8 Jul 2026 13:37:34 -0400 Subject: [PATCH 16/16] Recover from an unbridgeable changeset gap instead of asserting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If sqlite auto-rolls-back a read-write transaction mid-way (which it may do on SQLITE_FULL / SQLITE_IOERR / SQLITE_NOMEM per its "errors within a transaction" contract), statements after the error — including the yap2 snapshot write in incrementSnapshotInDatabase — run in autocommit mode and are individually durable. The failed COMMIT path then retracts the changeset and decrements the in-memory snapshot, so the on-disk snapshot can end up ahead of memory with no changeset bridging the two. A later read on the disk-read path would then fetch an incomplete changeset set and trip the snapshot == dbSnapshot assertion in preReadTransaction (a hard crash, since NSAssert is enabled in production). Fixes, in layers: - pendingAndCommittedChangesetsSince: no longer gates its "all snapshots present" check on enableMultiProcessSupport. When the delta can't be reconstructed, it returns nil in single-process mode too, so the caller cold-flushes and adopts the on-disk state as truth — the same safe recovery multiprocess mode already relied on. This is provably a no-op in normal single-process operation (the counter increments by exactly 1 per non-empty commit, each recording a changeset). - incrementSnapshotInDatabase: checks sqlite3_get_autocommit() first. If the transaction was auto-rolled-back, it skips the snapshot write (and logs) rather than durably advancing the on-disk snapshot past a phantom commit. Safe: this method's only caller is postReadWriteTransaction, always inside a real transaction. - commitTransaction: logs when it observes sqlite already auto-rolled-back, distinguishing that from an explicit rollback. Adds a deterministic regression test that manufactures the disk-ahead-of- memory state out-of-band and asserts a disk-read-path read recovers (adopts the on-disk snapshot, data stays readable) instead of crashing. Co-Authored-By: Claude Opus 4.8 (1M context) --- Testing/UnitTesting/TestYapDatabase.m | 56 ++++++++++++++++++++++++ YapDatabase/YapDatabase.m | 14 +++++- YapDatabase/YapDatabaseConnection.m | 16 ++++++- YapDatabase/YapDatabaseTransaction.m | 62 +++++++++++++++++---------- 4 files changed, 122 insertions(+), 26 deletions(-) diff --git a/Testing/UnitTesting/TestYapDatabase.m b/Testing/UnitTesting/TestYapDatabase.m index 2cd499ca3..64c7113b6 100644 --- a/Testing/UnitTesting/TestYapDatabase.m +++ b/Testing/UnitTesting/TestYapDatabase.m @@ -1611,4 +1611,60 @@ - (void)testTouchThenLongLivedReadDoesNotRegressSnapshot @"data must remain readable and correct"); } +// If the on-disk snapshot ends up ahead of the in-memory snapshot with no +// changeset to bridge the gap — which can happen if sqlite auto-rolls-back a transaction mid-way and +// a later autocommit write durably advances the yap2 snapshot row past a changeset that was then +// retracted — a read that takes the disk-read path must recover by flushing and adopting the on-disk +// snapshot, not fast-forward into the snapshot == dbSnapshot assertion. +// +// The bad state is manufactured directly (raw-writing the yap2 snapshot row out-of-band) so the test +// is deterministic and doesn't depend on sqlite's version-specific auto-rollback behavior. +- (void)testDiskSnapshotAheadOfMemoryRecoversGracefully +{ + NSURL *databaseURL = [self databaseURL:NSStringFromSelector(_cmd)]; + [[NSFileManager defaultManager] removeItemAtURL:databaseURL error:NULL]; + + YapDatabase *database = [[YapDatabase alloc] initWithURL:databaseURL]; + XCTAssertNotNil(database); // single-process mode (enableMultiProcessSupport defaults to NO) + + YapDatabaseConnection *connection = [database newConnection]; + + NSString *const collection = @"docs"; + NSString *const key = @"k"; + + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [transaction setObject:@"v" forKey:key inCollection:collection]; + }]; + + uint64_t diskAhead = connection.snapshot + 1; + + // Advance ONLY the on-disk yap2 snapshot row, out-of-band, so the in-memory snapshot stays put + // and no changeset exists for the new value — reproducing the end-state of #6. + [connection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + NSString *sql = [NSString stringWithFormat: + @"INSERT OR REPLACE INTO \"yap2\" (\"extension\", \"key\", \"data\") VALUES ('', 'snapshot', %llu);", + (unsigned long long)diskAhead]; + int rc = sqlite3_exec(connection->db, [sql UTF8String], NULL, NULL, NULL); + XCTAssertEqual(rc, SQLITE_OK, @"raw yap2 snapshot write should succeed"); + }]; + + // A read on the disk-read path (long-lived) now sees the on-disk snapshot ahead of memory with no + // bridging changeset. It must flush and adopt the on-disk snapshot instead of asserting. + [connection beginLongLivedReadTransaction]; + + __block id value = nil; + [connection readWithBlock:^(YapDatabaseReadTransaction *transaction) { + value = [transaction objectForKey:key inCollection:collection]; + }]; + + uint64_t snapshotAfter = connection.snapshot; + + [connection endLongLivedReadTransaction]; + + XCTAssertEqual(snapshotAfter, diskAhead, + @"connection must adopt the on-disk snapshot after an unbridgeable changeset gap"); + XCTAssertEqualObjects(value, @"v", + @"data must remain readable after the recovery flush"); +} + @end diff --git a/YapDatabase/YapDatabase.m b/YapDatabase/YapDatabase.m index 1d214c141..094e2ac92 100644 --- a/YapDatabase/YapDatabase.m +++ b/YapDatabase/YapDatabase.m @@ -3176,13 +3176,23 @@ - (NSArray *)pendingAndCommittedChangesetsSince:(uint64_t)connectionSnapshot unt } } - if (options.enableMultiProcessSupport) + // Every snapshot number between connectionSnapshot and maxSnapshot must have a changeset for us + // to reconstruct the delta. If any are missing, return nil so the caller cold-flushes and adopts + // the on-disk state as truth, rather than fast-forwarding to an inconsistent snapshot (which + // would trip the snapshot == dbSnapshot assertion in preReadTransaction/preWriteTransaction). + // + // In multiprocess mode a gap normally means another process committed. In single-process mode it + // should never happen in normal operation (the counter increments by exactly 1 per non-empty + // commit, each recording a changeset) — but it CAN if sqlite auto-rolled-back a transaction + // mid-way and a later autocommit write durably advanced the on-disk snapshot past a changeset + // that was then retracted. Either way, cold-flushing is the safe recovery, so the check is no + // longer gated on enableMultiProcessSupport. { const uint64_t expectedSnapshotsCount = maxSnapshot - connectionSnapshot; if (expectedSnapshotsCount != relevantChangesets.count) { YDBLogVerbose(@"Expected snapshot count not found: expected(%llu) != found(%llu)." - @" Database seems to have been modified from another process. Discarding changeset.", + @" Discarding changeset; connection will flush and re-read the on-disk state.", expectedSnapshotsCount, (uint64_t)relevantChangesets.count); return nil; } diff --git a/YapDatabase/YapDatabaseConnection.m b/YapDatabase/YapDatabaseConnection.m index 0ef7e0a26..f7656d30f 100644 --- a/YapDatabase/YapDatabaseConnection.m +++ b/YapDatabase/YapDatabaseConnection.m @@ -3640,7 +3640,21 @@ - (uint64_t)readSnapshotFromDatabase:(BOOL *)errorPtr - (uint64_t)incrementSnapshotInDatabase { uint64_t newSnapshot = snapshot + 1; - + + if (sqlite3_get_autocommit(db) != 0) + { + // This runs inside the read-write transaction, so autocommit should be off. If sqlite reports + // autocommit, it auto-rolled-back the transaction mid-way (e.g. SQLITE_FULL / SQLITE_IOERR on + // an earlier statement — see the SQLite docs on errors within a transaction). Writing the + // snapshot row now would run in autocommit mode and DURABLY advance the on-disk snapshot past + // a commit that never happened, leaving disk ahead of memory. Skip it: the COMMIT will fail + // and postReadWriteTransaction will treat this as a rollback. + + YDBLogWarn(@"incrementSnapshotInDatabase: transaction was auto-rolled-back by sqlite;" + @" skipping snapshot write to avoid a durable on-disk/in-memory snapshot mismatch."); + return newSnapshot; + } + sqlite3_stmt *statement = [self yapSetDataForKeyStatement]; if (statement == NULL) return newSnapshot; diff --git a/YapDatabase/YapDatabaseTransaction.m b/YapDatabase/YapDatabaseTransaction.m index 3b56ef3db..b5c2f006e 100644 --- a/YapDatabase/YapDatabaseTransaction.m +++ b/YapDatabase/YapDatabaseTransaction.m @@ -183,35 +183,51 @@ - (BOOL)commitTransaction sqlite3_reset(statement); } - if (!committed && isReadWriteTransaction && !sqlite3_get_autocommit(connection->db)) + if (!committed && isReadWriteTransaction) { - // sqlite did NOT auto-rollback; the transaction is still open. This covers both a failed - // COMMIT step (above) and a NULL commitTransactionStatement (prepare failed, e.g. - // SQLITE_NOMEM after a memory warning flushed the cached statements). - // Roll it back explicitly so the outcome is deterministic - // (a zombie transaction would swallow every subsequent BEGIN/COMMIT). - - sqlite3_stmt *rbStatement = [connection rollbackTransactionStatement]; - if (rbStatement) + if (sqlite3_get_autocommit(connection->db)) { - int rbStatus = sqlite3_step(rbStatement); - if (rbStatus != SQLITE_DONE) - { - YDBLogError(@"Couldn't rollback failed commit: %d %s", - rbStatus, sqlite3_errmsg(connection->db)); - } - sqlite3_reset(rbStatement); + // sqlite already rolled the transaction back itself — either the failed COMMIT above, or + // an auto-rollback triggered by an error on an EARLIER statement (SQLite may do this for + // SQLITE_FULL / SQLITE_IOERR / SQLITE_NOMEM; see the docs on errors within a transaction). + // In the latter case, statements after the error ran in autocommit mode and are durable; + // we can't undo those here, but treating this as a failed commit (postReadWriteTransaction + // flushes + retracts the changeset) plus the changeset-gap recovery in + // pendingAndCommittedChangesetsSince keeps caches consistent with what's actually on disk. + + YDBLogWarn(@"commitTransaction: transaction is no longer active (sqlite auto-rolled-back);" + @" treating as a failed commit."); } else { - // If preparing COMMIT failed with NOMEM, preparing ROLLBACK likely fails too. - // sqlite3_exec is a last-ditch attempt that doesn't depend on the statement cache. - - int rbStatus = sqlite3_exec(connection->db, "ROLLBACK TRANSACTION;", NULL, NULL, NULL); - if (rbStatus != SQLITE_OK) + // sqlite did NOT auto-rollback; the transaction is still open. This covers both a failed + // COMMIT step (above) and a NULL commitTransactionStatement (prepare failed, e.g. + // SQLITE_NOMEM after a memory warning flushed the cached statements). + // Roll it back explicitly so the outcome is deterministic + // (a zombie transaction would swallow every subsequent BEGIN/COMMIT). + + sqlite3_stmt *rbStatement = [connection rollbackTransactionStatement]; + if (rbStatement) { - YDBLogError(@"Couldn't rollback failed commit (exec): %d %s", - rbStatus, sqlite3_errmsg(connection->db)); + int rbStatus = sqlite3_step(rbStatement); + if (rbStatus != SQLITE_DONE) + { + YDBLogError(@"Couldn't rollback failed commit: %d %s", + rbStatus, sqlite3_errmsg(connection->db)); + } + sqlite3_reset(rbStatement); + } + else + { + // If preparing COMMIT failed with NOMEM, preparing ROLLBACK likely fails too. + // sqlite3_exec is a last-ditch attempt that doesn't depend on the statement cache. + + int rbStatus = sqlite3_exec(connection->db, "ROLLBACK TRANSACTION;", NULL, NULL, NULL); + if (rbStatus != SQLITE_OK) + { + YDBLogError(@"Couldn't rollback failed commit (exec): %d %s", + rbStatus, sqlite3_errmsg(connection->db)); + } } } }