diff --git a/Testing/UnitTesting/TestYapDatabase.m b/Testing/UnitTesting/TestYapDatabase.m index 82d735a5a..64c7113b6 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 @@ -1277,4 +1292,379 @@ - (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)"); +} + +// 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"); + }]; +} + +// 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"); +} + +// 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"); +} + +// 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/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/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 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]; } /** 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/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/YapDatabase.m b/YapDatabase/YapDatabase.m index 63b3dadf9..094e2ac92 100644 --- a/YapDatabase/YapDatabase.m +++ b/YapDatabase/YapDatabase.m @@ -528,8 +528,31 @@ - (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]; + + // 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] removeItemAtPath:sidecarPath error:NULL]; + } + } + } } - + } while (i < INT_MAX && !renamed && !failed); if (renamed) @@ -552,9 +575,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) { @@ -3091,11 +3120,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. * @@ -3125,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 388ff5b18..f7656d30f 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,43 @@ - (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 (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; + } + myState->longLivedReadTransaction = (longLivedReadTransaction != nil); myState->sqlLevelSharedReadLock = YES; needsMarkSqlLevelSharedReadLock = NO; @@ -2774,16 +2801,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 +2820,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; @@ -2890,8 +2940,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); @@ -2924,14 +2978,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) { @@ -2952,10 +3005,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) { @@ -2964,11 +3019,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]; @@ -3138,21 +3195,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]; - + 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 @@ -3178,13 +3269,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]; @@ -3228,8 +3319,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]; @@ -3260,10 +3351,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; } /** @@ -3490,37 +3583,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; } @@ -3530,7 +3640,21 @@ - (uint64_t)readSnapshotFromDatabase - (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; @@ -4160,13 +4284,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) @@ -4176,7 +4304,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]; } @@ -4230,14 +4358,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]; @@ -4245,11 +4377,11 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newObject != yapTouch) { YapDatabasePolicy objectPolicy = YapDatabasePolicyContainment; - NSNumber *op = objectPolicies[cacheKey.collection]; + NSNumber *op = objectPolicies[cacheKey.collection] ?: defaultObjectPolicy; if (op) { objectPolicy = (YapDatabasePolicy)[op integerValue]; } - + if (objectPolicy == YapDatabasePolicyContainment) { [objectCache removeObjectForKey:cacheKey]; } @@ -4282,13 +4414,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) @@ -4298,7 +4434,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]; } @@ -4352,14 +4488,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]; @@ -4367,11 +4507,11 @@ - (void)processChangeset:(NSDictionary *)changeset else if (newMetadata != yapTouch) { YapDatabasePolicy metadataPolicy = YapDatabasePolicyContainment; - NSNumber *mp = metadataPolicies[cacheKey.collection]; + NSNumber *mp = metadataPolicies[cacheKey.collection] ?: defaultMetadataPolicy; if (mp) { metadataPolicy = (YapDatabasePolicy)[mp integerValue]; } - + if (metadataPolicy == YapDatabasePolicyContainment) { [metadataCache removeObjectForKey:cacheKey]; } @@ -5152,16 +5292,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; } diff --git a/YapDatabase/YapDatabaseTransaction.m b/YapDatabase/YapDatabaseTransaction.m index 167b3c188..b5c2f006e 100644 --- a/YapDatabase/YapDatabaseTransaction.m +++ b/YapDatabase/YapDatabaseTransaction.m @@ -153,29 +153,114 @@ - (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)); } - + sqlite3_reset(statement); } - + + if (!committed && isReadWriteTransaction) + { + if (sqlite3_get_autocommit(connection->db)) + { + // 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 + { + // 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. + + 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) { - [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 @@ -2810,21 +2895,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); } /** @@ -3116,6 +3205,7 @@ - (void)_enumerateKeysAndObjectsInCollections:(NSArray *)collections if (!stop && mutation.isMutated) { + sqlite_enum_reset(statement, needsFinalize); @throw [self mutationDuringEnumerationException]; } @@ -3466,6 +3556,7 @@ - (void)_enumerateKeysAndMetadataInCollections:(NSArray *)collections if (!stop && mutation.isMutated) { + sqlite_enum_reset(statement, needsFinalize); @throw [self mutationDuringEnumerationException]; } @@ -3895,6 +3986,7 @@ - (void)_enumerateRowsInCollections:(NSArray *)collections if (!stop && mutation.isMutated) { + sqlite_enum_reset(statement, needsFinalize); @throw [self mutationDuringEnumerationException]; } @@ -5663,7 +5755,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; } @@ -5683,15 +5780,28 @@ - (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, 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; } - - sqlite3_finalize(statement); - statement = NULL; - + connection->hasDiskChanges = YES; [connection->mutationStack markAsMutated]; // mutation during enumeration protection @@ -5841,29 +5951,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 @@ -5974,7 +6099,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; } @@ -5994,20 +6124,46 @@ - (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, 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; } - - 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 @@ -6037,13 +6193,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