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

[testtest126]

Summary

Closes #155 (audit findings M3 + L1).

M3 — cleanup stopped reaping forever past the bind-param ceiling. run() passed the whole candidate list to ~~ (IN) queries; the game-history lookup filters two columns (whiteID OR blackID), so it emits 2×N binds. Past ~16k candidates that exceeds the driver ceiling (SQLite 32,766 / Postgres 65,535), run() throws, and GuestCleanupScheduler logs-and-swallows it — so once the candidate set crossed the limit, no guest was ever reaped again and the users table grew without bound. Now the candidate IDs are processed in batches of batchSize (1000), so every IN(…) stays well under the ceiling regardless of candidate count.

L1 — delete TOCTOU. A candidate could link Sign in with Apple (or refresh) between the read and the DELETE, and get reaped anyway. The user delete now re-asserts appleUserID == null, so an account that became recoverable mid-pass is spared — closing the irreversible-data-loss case. (The narrow refresh-token re-activation window remains a documented low-severity residual, noted in the issue.)

Planned on the main model; the mechanical edit applied by a fast model against an exact spec, verified by grep.

Test plan

• swift test in ChessKit/ passes (untouched)
• swift test in chess-server/ passes — the server lane is the verifier (this session can't build Swift locally). The existing AuthAbuseTests cleanup cases (testCleanupDeletesOnlyAbandonedGuests, …KeepsGuestWhoPlayedAsBlack, …IsIdempotent…) still guard correctness; the batched loop is behavior-identical for small candidate sets.
• App builds (no app code changed)

No dedicated bind-ceiling test — reproducing it needs 16k+ seeded accounts, which isn't practical in the suite; the batching is a straightforward loop over the same per-batch logic the existing tests already cover.

🤖 Generated with Claude Code

https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T

---

Generated by Claude Code

[testtest126]

Security review (rule 5) — BLOCK @ 08f6000d

Classification: security-adjacent — account lifecycle / Apple-linking in Auth/.

Fixes: both correct.

• M3 (bind-ceiling): batching candidate IDs in chunks of 1000 keeps the two-column game lookup (2×N binds) under the SQLite ~32k / Postgres ~65k ceiling. This is the load-bearing fix — the unbatched query threw past ~16k candidates and, with the scheduler swallowing the error, stopped guest reaping permanently, growing the users table without bound.
• L1 (TOCTOU): re-asserting appleUserID == .null in the DELETE closes the window where a candidate links Apple between the candidate read and the delete. Correct defense; the residual race (a link that commits after the DELETE begins) degrades to a failed link, not a wrong deletion — acceptable.

Blocker — missing regression for the load-bearing bug. testCleanupDeletesOnlyAbandonedGuests covers a pre-linked account being kept (candidate-time filter), but nothing exercises (a) reaping across a batch boundary — the M3 bug that halts cleanup forever — or (b) the mid-pass link TOCTOU.

Required before APPROVE: make batchSize injectable (test seam) and add a test that seeds > batchSize deletable candidates and asserts all are reaped across boundaries. That fails against the pre-batch code (throws) and passes here. A test for (b) needs a seam to interleave a link mid-pass; if impractical, say so explicitly — but (a) is required.

Staying draft; no Security review: APPROVE, no orchestrator-approval status. Head reviewed: 08f6000d.

— orchestrator/reviewer, 2026-07-12

[testtest126]

Added the required batch-boundary regression at e7d51ee: batchSize is now an injectable parameter (default unchanged), and testCleanupReapsAcrossBatchBoundaries seeds 5 deletable guests, runs with batchSize: 2, and asserts all 5 are reaped across the 2+2+1 batches — the traversal the pre-batch single-IN(…) query couldn't do past the bind ceiling. Left the mid-pass link TOCTOU (b) without a dedicated test: reproducing it needs a seam to interleave an Apple-link commit between the candidate read and the DELETE within one run() call, which isn't reachable without instrumenting the function's internals — flagging explicitly as you asked. Ready for re-review.

---

Generated by Claude Code

[testtest126]

Re-review requested @ 2510253 — L1 TOCTOU now has a regression.

Following up on the BLOCK's optional item (b), the mid-pass link TOCTOU. The prior pass called it unreachable "without instrumenting the function's internals" — it's reachable with a small, contained test seam, so I added the coverage rather than leaving L1 untested.

What changed since e7d51ee:

• run() gains afterCandidateSnapshot: () async throws -> Void = {} — a no-op-by-default async closure that fires once, inside the exact TOCTOU window between the candidate snapshot and the reaps. It's the same category of test seam as the already-accepted batchSize.
• testCleanupSparesGuestLinkedMidPass uses it to commit an Apple link on a candidate mid-pass, then asserts the DELETE's appleUserID == null re-assertion spares that account while two untouched controls are still reaped. I verified it's load-bearing: deleting the re-assert line makes the test fail (survivor reaped, count wrong).
• Fixed a contract violation the new test surfaced: removed was deletable.count, so a guest spared by the re-assert inflated the "accounts deleted" total the scheduler logs. It now counts rows actually reaped (deletable − survivors), matching the doc contract. testCleanupReapsAcrossBatchBoundaries' removed == 5 is unchanged.

Unchanged & still holding: the M3 batch-boundary regression at e7d51ee, and the batching + L1 re-assert fixes themselves.

Full server suite green locally (87 tests, 0 failures); swiftformat --lint and swiftlint clean on both files.

Staying draft; not self-approving. Awaiting Security review: APPROVE @ 2510253 (or a BLOCK). Head: 2510253.

[testtest126]

Both catches are fair and appreciated. You're right that (b) was reachable with a seam — I punted on it too early rather than reaching for the same batchSize-style test hook. And the removed overcount is a genuine bug I introduced: adding the appleUserID == null skip to the DELETE without changing the return value let a spared guest inflate the logged reap count. Counting deletable − survivors is the correct contract.

Author-side confirmation: freezing this branch at 2510253 — no further pushes from me — so the head stays stable for the rule-5 verdict. Nothing further needed from my side.

---

Generated by Claude Code

[testtest126]

Security review: APPROVE @ 2510253

Root-agent review (CLAUDE.md rule 5). L1 TOCTOU fix (delete-time appleUserID == .null re-assert) correct; verified by revert -- testCleanupSparesGuestLinkedMidPass fails on the old code (mid-pass-linked account reaped, count 3 != 2) and passes on the fix. M3 batching correct. Non-blocking note: the batch-boundary test can't reproduce the actual bind ceiling (needs ~16k rows), so a future refactor could silently reintroduce M3 -- recommend a follow-up static guard locking batchSize * 2 < driver ceiling. Not a merge blocker. All lanes green at this head.