From e72dfde470911a0a1fb31558e65125ec613cb9db Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 04:49:25 +0000 Subject: [PATCH 1/4] Guest cleanup: batch candidate IDs and re-assert the delete predicate (closes #155) run() passed the full candidate list to IN(...) queries; the two-column game lookup emits 2xN binds, so past ~16k candidates it exceeded the driver bind-parameter ceiling, threw, and (swallowed by the scheduler) stopped guest cleanup permanently. Now processes candidates in batches of batchSize so every IN(...) stays under the ceiling regardless of candidate count. Also re-asserts appleUserID == null inside the user delete, so an account that links Sign in with Apple between the candidate read and the delete is spared (L1 TOCTOU). The existing AuthAbuseTests cleanup cases still guard correctness for small candidate sets. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- .../App/Auth/GuestAccountCleanup.swift | 85 ++++++++++++------- 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift index 032e550..7796469 100644 --- a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift +++ b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift @@ -16,10 +16,17 @@ 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. @discardableResult static func run(on db: Database, now: Date = Date()) 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) @@ -29,42 +36,54 @@ enum GuestAccountCleanup { let candidateIDs = try candidates.map { try $0.requireID() } guard !candidateIDs.isEmpty else { return 0 } - // 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() + removed += deletable.count + } + return removed } } From 08f6000d1ca5cbfed0a49189c7e5e585e2faffc6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 04:50:43 +0000 Subject: [PATCH 2/4] Lint: no space around the range operator (repo .swiftformat --ranges no-space) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- chess-server/Sources/App/Auth/GuestAccountCleanup.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift index 7796469..da7fbb6 100644 --- a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift +++ b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift @@ -43,7 +43,7 @@ enum GuestAccountCleanup { // 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)]) + let batch = Array(candidateIDs[start.. Date: Sun, 12 Jul 2026 11:36:50 +0000 Subject: [PATCH 3/4] Guest cleanup: make batchSize injectable + test reaping across batches (#155) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security review (rule 5) asked for a regression that exercises the load-bearing M3 fix. batchSize is now an injectable parameter (default unchanged), and a new test seeds 5 deletable guests, runs with batchSize 2, and asserts all 5 are reaped across the 2+2+1 batch boundaries — the behavior the pre-batch single-IN(...) query couldn't do past the bind ceiling. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- .../Sources/App/Auth/GuestAccountCleanup.swift | 2 +- chess-server/Tests/AppTests/AuthAbuseTests.swift | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift index da7fbb6..355b401 100644 --- a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift +++ b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift @@ -22,7 +22,7 @@ enum GuestAccountCleanup { /// Runs one cleanup pass and returns how many accounts were deleted. @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) async throws -> Int { let cutoff = now.addingTimeInterval(-retention) // Tokens live `RefreshToken.lifetime` from issue, so "issued after the // cutoff" is "expires after cutoff + lifetime". diff --git a/chess-server/Tests/AppTests/AuthAbuseTests.swift b/chess-server/Tests/AppTests/AuthAbuseTests.swift index 8f41462..1bdcb7f 100644 --- a/chess-server/Tests/AppTests/AuthAbuseTests.swift +++ b/chess-server/Tests/AppTests/AuthAbuseTests.swift @@ -221,4 +221,20 @@ 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") + } } From 2510253223599e06f5736c025557251bca5977b4 Mon Sep 17 00:00:00 2001 From: Yakiv Kovalskyi Date: Sun, 12 Jul 2026 14:04:31 +0200 Subject: [PATCH 4/4] Guest cleanup: add the L1 TOCTOU regression + honest reap count (#155) Security review (rule 5) accepted the batch-boundary regression at e7d51ee but left the L1 mid-pass-link re-assertion untested; the prior pass judged it unreachable without instrumenting run(). It's reachable with a small seam. - run() gains `afterCandidateSnapshot`, a no-op-by-default async closure that fires once inside the TOCTOU window between the candidate read and the reaps. testCleanupSparesGuestLinkedMidPass uses it to commit an Apple link on a candidate mid-pass and asserts the DELETE's `appleUserID == null` re-assertion spares it while two controls are still reaped. Removing the re-assert makes the test fail (verified). - Fix a contract violation the test surfaced: `removed` counted `deletable.count`, so a guest spared by the re-assert inflated the "accounts deleted" total the scheduler logs. Now count rows actually reaped (deletable minus survivors), so the returned/logged count is truthful. The batch test's removed==5 is unchanged. Full server suite green (87 tests). Still draft; no self-approval. Co-Authored-By: Claude Opus 4.8 --- .../App/Auth/GuestAccountCleanup.swift | 21 ++++++++++++-- .../Tests/AppTests/AuthAbuseTests.swift | 29 +++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift index 355b401..563e78f 100644 --- a/chess-server/Sources/App/Auth/GuestAccountCleanup.swift +++ b/chess-server/Sources/App/Auth/GuestAccountCleanup.swift @@ -21,8 +21,19 @@ enum GuestAccountCleanup { 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(), batchSize: Int = GuestAccountCleanup.batchSize) 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". @@ -35,6 +46,7 @@ enum GuestAccountCleanup { .all() let candidateIDs = try candidates.map { try $0.requireID() } guard !candidateIDs.isEmpty else { return 0 } + try await afterCandidateSnapshot() // Process candidates in batches so no single IN (…) query exceeds the // driver's bind-parameter ceiling (SQLite ~32k, Postgres ~65k). Past @@ -81,7 +93,12 @@ enum GuestAccountCleanup { .filter(\.$id ~~ deletable) .filter(\.$appleUserID == .null) .delete() - removed += deletable.count + // 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 1bdcb7f..6250f72 100644 --- a/chess-server/Tests/AppTests/AuthAbuseTests.swift +++ b/chess-server/Tests/AppTests/AuthAbuseTests.swift @@ -237,4 +237,33 @@ final class AuthAbuseTests: XCTestCase { 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") + } }