diff --git a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift index 032e550..563e78f 100644 --- a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift +++ b/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.. 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 } } diff --git a/chess-server/Tests/AppTests/AuthAbuseTests.swift b/chess-server/Tests/AppTests/AuthAbuseTests.swift index 8f41462..6250f72 100644 --- a/chess-server/Tests/AppTests/AuthAbuseTests.swift +++ b/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") + } }