Fix corruption from rowid reuse when database transactions aren't committed

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

YapDatabase/Extensions/ActionManager/Utilities/YapReachability.m

@@ -46,7 +46,6 @@ CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
 
 #import <sys/socket.h>
 #import <netinet/in.h>
-#import <netinet6/in6.h>
 #import <arpa/inet.h>
 #import <ifaddrs.h>
 #import <netdb.h>

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

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];
 }
 
 /**

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;

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,13 +644,44 @@ - (void)commit
 			[table->changes addObject:changedKeys];
 		}
 		[table->lock unlock];
+
+		didCommit = YES;
 	}
 }
 
 - (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];
 	}
 }