Guest cleanup: batch candidate IDs and re-assert the delete predicate (closes #155)

chess-server/Sources/App/Auth/GuestAccountCleanup.swift

@@ -16,10 +16,28 @@ enum GuestAccountCleanup {
     /// How long an abandoned guest account is kept.
     static let retention: TimeInterval = 30 * 24 * 3600
 
+    /// Max IDs per IN (…) query, so bind counts stay under the driver ceiling
+    /// (SQLite ~32k, Postgres ~65k) even for the two-column game lookup.
+    static let batchSize = 1000
+
     /// Runs one cleanup pass and returns how many accounts were deleted.
+    ///
+    /// `afterCandidateSnapshot` is a test seam: it runs once, inside the
+    /// TOCTOU window between reading the candidate set and reaping it, so a
+    /// test can commit an Apple link mid-pass and prove the DELETE's
+    /// `appleUserID == null` re-assertion spares it (#155, L1). No-op by
+    /// default.
     @discardableResult
-    static func run(on db: Database, now: Date = Date()) async throws -> Int {
+    static func run(
+        on db: Database,
+        now: Date = Date(),
+        batchSize: Int = GuestAccountCleanup.batchSize,
+        afterCandidateSnapshot: () async throws -> Void = {}
+    ) async throws -> Int {
         let cutoff = now.addingTimeInterval(-retention)
+        // Tokens live `RefreshToken.lifetime` from issue, so "issued after the
+        // cutoff" is "expires after cutoff + lifetime".
+        let activeThreshold = cutoff.addingTimeInterval(RefreshToken.lifetime)
 
         // Guests old enough to be candidates at all.
         let candidates = try await User.query(on: db)
@@ -28,43 +46,61 @@ enum GuestAccountCleanup {
             .all()
         let candidateIDs = try candidates.map { try $0.requireID() }
         guard !candidateIDs.isEmpty else { return 0 }
+        try await afterCandidateSnapshot()
 
-        // Still active: a refresh token issued within the retention period.
-        // Tokens live `RefreshToken.lifetime` from issue, so "issued after
-        // the cutoff" is "expires after cutoff + lifetime".
-        let activeThreshold = cutoff.addingTimeInterval(RefreshToken.lifetime)
-        let activeIDs = Set(
-            try await RefreshToken.query(on: db)
-                .filter(\.$user.$id ~~ candidateIDs)
-                .filter(\.$expiresAt > activeThreshold)
-                .all()
-                .map { $0.$user.id }
-        )
+        // Process candidates in batches so no single IN (…) query exceeds the
+        // driver's bind-parameter ceiling (SQLite ~32k, Postgres ~65k). Past
+        // ~16k candidates the two-column game query blew the limit, threw, and
+        // — the error being swallowed by the scheduler — stopped guest cleanup
+        // permanently, growing the users table without bound (#155, M3).
+        var removed = 0
+        for start in stride(from: 0, to: candidateIDs.count, by: batchSize) {
+            let batch = Array(candidateIDs[start..<min(start + batchSize, candidateIDs.count)])
 
-        // Has a game history worth keeping (either color; game_records has no
-        // FK to users, so this is the join).
-        let playerIDs = Set(
-            try await GameRecord.query(on: db)
-                .group(.or) { or in
-                    or.filter(\.$whiteID ~~ candidateIDs)
-                    or.filter(\.$blackID ~~ candidateIDs)
-                }
-                .all()
-                .flatMap { [$0.whiteID, $0.blackID] }
-        )
+            // Still active: a refresh token issued within the retention period.
+            let activeIDs = Set(
+                try await RefreshToken.query(on: db)
+                    .filter(\.$user.$id ~~ batch)
+                    .filter(\.$expiresAt > activeThreshold)
+                    .all()
+                    .map { $0.$user.id }
+            )
 
-        let deletable = candidateIDs.filter { !activeIDs.contains($0) && !playerIDs.contains($0) }
-        guard !deletable.isEmpty else { return 0 }
+            // Has a game history worth keeping (either color; game_records has
+            // no FK to users, so this is the join).
+            let playerIDs = Set(
+                try await GameRecord.query(on: db)
+                    .group(.or) { or in
+                        or.filter(\.$whiteID ~~ batch)
+                        or.filter(\.$blackID ~~ batch)
+                    }
+                    .all()
+                    .flatMap { [$0.whiteID, $0.blackID] }
+            )
+
+            let deletable = batch.filter { !activeIDs.contains($0) && !playerIDs.contains($0) }
+            guard !deletable.isEmpty else { continue }
 
-        // The FK cascade covers Postgres; delete tokens explicitly anyway so
-        // behavior doesn't depend on SQLite's foreign_keys pragma in dev/test.
-        try await RefreshToken.query(on: db)
-            .filter(\.$user.$id ~~ deletable)
-            .delete()
-        try await User.query(on: db)
-            .filter(\.$id ~~ deletable)
-            .delete()
-        return deletable.count
+            // The FK cascade covers Postgres; delete tokens explicitly anyway so
+            // behavior doesn't depend on SQLite's foreign_keys pragma in dev/test.
+            try await RefreshToken.query(on: db)
+                .filter(\.$user.$id ~~ deletable)
+                .delete()
+            // Re-assert appleUserID == null in the delete: if an account linked
+            // Apple between the candidate read above and here it is now
+            // recoverable and must not be reaped (#155, L1 TOCTOU).
+            try await User.query(on: db)
+                .filter(\.$id ~~ deletable)
+                .filter(\.$appleUserID == .null)
+                .delete()
+            // Count rows actually reaped, not rows we intended to reap: a guest
+            // spared by the re-assert above must not inflate the total (the doc
+            // contract is "how many accounts were deleted", and the scheduler
+            // logs it).
+            let spared = try await User.query(on: db).filter(\.$id ~~ deletable).count()
+            removed += deletable.count - spared
+        }
+        return removed
     }
 }
 

chess-server/Tests/AppTests/AuthAbuseTests.swift

@@ -221,4 +221,49 @@ final class AuthAbuseTests: XCTestCase {
         XCTAssertEqual(first, 0)
         XCTAssertEqual(second, 0)
     }
+
+    func testCleanupReapsAcrossBatchBoundaries() async throws {
+        // Regression for #155 (M3): the pre-batch code sent every candidate ID
+        // in one IN(…) query, which threw past the driver's bind ceiling and
+        // — swallowed by the scheduler — halted reaping forever. Seed more
+        // deletable guests than one batch holds and confirm the batched loop
+        // reaps every one across boundaries. batchSize is injected small so the
+        // test doesn't need thousands of rows.
+        for _ in 0..<5 {
+            _ = try await seedUser(daysAgo: 60) // old, no token, no game → deletable
+        }
+        let removed = try await GuestAccountCleanup.run(on: app.db, batchSize: 2)
+        XCTAssertEqual(removed, 5, "every deletable guest is reaped across the 2+2+1 batches")
+        let remaining = try await User.query(on: app.db).count()
+        XCTAssertEqual(remaining, 0, "no abandoned guest left behind")
+    }
+
+    func testCleanupSparesGuestLinkedMidPass() async throws {
+        // Regression for #155 (L1 TOCTOU): a guest can link an Apple ID *after*
+        // it is read as a candidate but *before* the DELETE runs. The DELETE
+        // re-asserts `appleUserID == null`, so the now-recoverable account must
+        // survive. The seam commits the link inside that exact window; two
+        // controls confirm the pass still reaps ordinary abandoned guests, and
+        // the returned count reflects only what was actually deleted.
+        let linker = try await seedUser(daysAgo: 60)
+        let controlA = try await seedUser(daysAgo: 60)
+        let controlB = try await seedUser(daysAgo: 60)
+
+        let removed = try await GuestAccountCleanup.run(on: app.db) {
+            // Runs in the candidate-read → delete window: link `linker` to Apple.
+            linker.appleUserID = "apple-subject-midpass"
+            try await linker.save(on: app.db)
+        }
+
+        // The mid-pass link spares `linker`…
+        let survivor = try await User.find(linker.requireID(), on: app.db)
+        XCTAssertNotNil(survivor, "an account linked mid-pass must not be reaped")
+        XCTAssertEqual(survivor?.appleUserID, "apple-subject-midpass")
+        // …while the untouched controls are reaped.
+        let survivingControlA = try await User.find(controlA.requireID(), on: app.db)
+        let survivingControlB = try await User.find(controlB.requireID(), on: app.db)
+        XCTAssertNil(survivingControlA, "an ordinary abandoned guest is still reaped")
+        XCTAssertNil(survivingControlB, "an ordinary abandoned guest is still reaped")
+        XCTAssertEqual(removed, 2, "the spared account must not be counted as deleted")
+    }
 }