perf(blocklist): seek instead of scan, serve from a swapped copy, decay every 6h - #26
Open
jbrahy wants to merge 4 commits into
Open
perf(blocklist): seek instead of scan, serve from a swapped copy, decay every 6h#26jbrahy wants to merge 4 commits into
jbrahy wants to merge 4 commits into
Conversation
A full blocklist sync against the 732k seeded numbers took long enough
to look broken. Three independent problems compounded, none of which
was visible when the table held three rows.
1. Every page was a full table scan plus a filesort. keysetPredicate
wrapped the indexed column -- UNIX_TIMESTAMP(updated_at) > ? -- and a
function on a column cannot drive an index seek:
type: ALL key: NULL rows: 715574 Using where; Using filesort
0.771s per 500-row page, ~1465 pages, so roughly a billion row reads
and ~19 minutes of pure DB time for one sync.
Fixed by comparing the bare columns as a row value, with
FROM_UNIXTIME applied to the PARAMETER instead, plus migration 0007
indexing (updated_at, phone_number_id). Measured after:
type: index key: idx_phone_numbers_updated_at_id rows: 1999
0.040s per 1000-row page. Filesort gone, ~358x fewer rows examined.
2. The client asked for 500 entries per page while the server's
maxBlocklistLimit is 1000, paying double the round trips for nothing.
pageLimit now defaults to 1000.
3. Saves were quadratic in disk writes. BlocklistStore.save serializes
the WHOLE state atomically, and sync() called it after every page. At
732k entries the file is ~8.8MB, so ~1465 per-page saves wrote ~6.4GB
to flash for a single sync. State is now written every pagesPerSave
(10) pages.
Durability is preserved rather than traded away: a page failure
persists whatever was folded in before rethrowing, so progress
survives a mid-sync error exactly as it did when every page was
saved. Two tests cover it -- the tail save after the last batch
boundary, and a failure mid-batch.
Net: ~19 minutes of DB time becomes ~29 seconds, and ~6.4GB of flash
writes becomes ~325MB.
The index leads with the ordering columns rather than extending
(status, updated_at), because the base query's status IN (...) would
otherwise produce three ranges whose union is not in global keyset
order -- MySQL would still sort, which is the cost being removed. The
migration documents when to revisit that: if most rows ever become
non-blockable, the residual status filter would have to skip many rows
to fill a page.
Deployed and verified on production before this commit: migration 7
applied, index in use, 17/17 smoke checks passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBmEXErCWYt3WZCZKaS7m9
The decay pass rewrites derived state for every number. At 732k numbers a pass measured 58m52s against a 15-minute timer, so it fired, found the unit still active, skipped, and effectively ran back-to-back forever -- saturating MySQL at ~618 selects/sec and competing with the blocklist reads devices sync from. For that whole hour readers were also querying a table mid-rewrite. Split populate from serve: - Migration 0008 adds blocklist_serving and blocklist_serving_next, the derived read model the delta serves. - RecomputeAll publishes both passes at once via SwapBlocklistServing: rebuild the standby copy, then one atomic RENAME TABLE. A reader sees the whole old snapshot or the whole new one, never a partial rewrite. - BlocklistDelta reads blocklist_serving instead of phone_numbers. - The timer drops to 6 hours. phone_numbers stays the single source of truth and is never swapped. Only the derived projection is, so a community report, attestation or admin override landing mid-rebuild cannot be lost -- which a swap of the whole schema would have done. RecomputeNumberServing keeps the request path immediate: a reported number patches its own row in the live serving copy inside the report transaction, so it reaches devices on the next sync rather than waiting up to 6 hours for a swap. The bulk pass deliberately does not do this -- patching 732k rows one at a time would both double its work and destroy the isolation the swap exists to provide. 6 hours is not a compromise. This pass only applies time decay; reports and overrides recompute their own number synchronously. With a 30-day half-life a score moves 0.58% in 6 hours. The serving table's updated_at carries NO "ON UPDATE CURRENT_TIMESTAMP" and is copied verbatim, because it IS the sync cursor. A restamp would put every device's stored cursor behind the entire table at once and make every client re-download all 732k entries after every swap. There is a test that fails if that regresses. Verified on production: migration 8 applied in 23s, all three tables at 732,382 rows, blocklist_serving serving 732,379 blocked, EXPLAIN shows idx_blocklist_serving_updated_at_id with no filesort, 17/17 smoke checks passed, and DB load fell from ~12,362 selects/20s to 1. Note for whoever deploys next: migration 0008 takes ~23s, which exceeds the ~14s health-check window in scripts/deploy.sh. Deploy with SKIP_RESTART=1 and restart by hand, or widen that window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PBmEXErCWYt3WZCZKaS7m9
jbrahy
changed the title
0008 published a rebuilt snapshot with an atomic RENAME TABLE. That is correct but heavier than the job needs: RENAME takes an EXCLUSIVE metadata lock, so it waits on in-flight queries and every new reader queues behind it. Nothing is physically moved -- both copies just sit there -- so the switch should be a pointer change, not DDL. - Migration 0009 renames the two copies to blocklist_serving_a and blocklist_serving_b and adds blocklist_serving_slot, a singleton row naming the live slot. The CHECK pins the primary key to one value so a second row cannot exist and "which slot is live" can never be ambiguous. active_slot is an ENUM, so an unexpected value cannot even be stored. - SwapBlocklistServing fills whichever slot is NOT live, then updates the pointer. One row, no DDL lock. A request either resolves the pointer before the update and serves the whole previous snapshot, or after and serves the whole new one. - BlocklistDelta resolves the pointer once per call and substitutes the table name into its query templates. The table name comes only from servingTable, which accepts nothing but the two known slots, so a pointer value can never reach a query as arbitrary text. There is a test asserting an unknown slot is rejected, because this is the one place a database value becomes part of a query string rather than a bound parameter. Index names are now per-slot. They did not have to be -- index names are per-table -- but with the tables no longer swapping identities, an EXPLAIN naming idx_blocklist_serving_a_* tells you which slot you read. Tests cover what is distinctive here: the active slot alternates a->b->a so a rebuild never overwrites the slot being served, bulk changes stay invisible until the pointer moves, a reported number is servable without waiting for a switch, and a switch does not restamp updated_at (which would put every device's cursor behind the whole table and force a full 732k re-download). Verified on production: migration 9 applied in 3s, pointer at slot a, both slots holding 732,382 rows, EXPLAIN using idx_blocklist_serving_a_updated_at_id with no filesort, 17/17 smoke checks passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PBmEXErCWYt3WZCZKaS7m9
A first sync pages through the entire blocklist. With only a spinner on
the button, a screen that is working looks like a screen that is hung.
Server publishes the denominator:
- Migration 0010 adds servable_count to blocklist_serving_slot, and
SwapBlocklistServing counts while rebuilding and publishes it in the
same UPDATE that moves the pointer -- so the count a reader sees always
describes the slot it was pointed at.
- The blocklist response gains "total". Additive, so an older client
ignores it.
Not a COUNT(*) per request: the delta's status filter is not the leading
column of any index, so a count scans, and a full sync asks ~733 times.
The rebuild already walks every row, making the count free there.
Client reports and renders it:
- SyncProgress{applied, total} with a clamped fraction, emitted after
every page through an optional @sendable handler on SyncService.sync.
Optional so the extensions and tests pay nothing.
- BlocklistData.total is OPTIONAL, because a server predating the field
omits it and decoding must not fail against one. No total means no
fraction, an indeterminate bar, and a bare count instead of a
percentage.
- StatusScreen shows a bar, a percentage and "302,400 of 732,379
numbers" while syncing.
Two deliberate imprecisions, both documented where they live:
The fraction is CLAMPED. total counts servable rows, but a delta also
carries "unblock" tombstones that are not in that count, so a
removal-heavy sync legitimately applies more entries than the total. A
bar that runs past its end reads as a bug; clamped reads as done.
The denominator DRIFTS. Per-row patches from UpsertServingRow do not
adjust servable_count, so between switches it is off by however many
numbers were reported in that window -- a handful out of 732k, which is
not worth a write on every report.
Progress is cleared on the way INTO a sync, not on the way out. The
handler hands updates over through a MainActor task, so the last page's
update can land after syncNow() returns; clearing on exit would race it
and sometimes lose, leaving a value set with no sync running. Clearing
on entry is race-free, and the screen only renders progress while
isSyncing.
Verified: 118 SpamFilterKit tests and the SyncStatusViewModel suite pass
with 0 failures; go test ./... green across 15 packages; migration 10
applied to production with servable_count backfilled to 732,380, which
is exactly the servable set (732,379 blocked + 1 overridden_block).
Pre-existing and untouched: SpamFilterUITests testLookupFlow and
testReportHappyPath fail on a pristine main checkout too -- confirmed by
running them against a clean tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBmEXErCWYt3WZCZKaS7m9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A full blocklist sync against the 732k seeded numbers took long enough to look broken. Three independent problems compounded, none of which was visible when the table held three rows.
1. Every page was a full table scan plus a filesort
keysetPredicatewrapped the indexed column --UNIX_TIMESTAMP(updated_at) > ?-- and a function on a column cannot drive an index seek:0.771s per 500-row page, ~1465 pages -- roughly a billion row reads and ~19 minutes of pure DB time for one sync.
Fixed by comparing the bare columns as a row value, with
FROM_UNIXTIMEapplied to the parameter instead, plus migration 0007 indexing(updated_at, phone_number_id):0.040s per 1000-row page. Filesort gone, ~358x fewer rows examined.
FROM_UNIXTIME(0)is1970-01-01 00:00:00, not NULL, so the(0, 0)full-snapshot cursor still compares correctly instead of yielding NULL and returning nothing -- verified on the production MySQL 8.4 withtime_zone=SYSTEM.Why the index leads with the ordering columns
Rather than extending the existing
(status, updated_at). The base query filtersstatus IN ('blocked','overridden_block','suspected'), so a status-leading index produces three ranges whose union is not in global(updated_at, phone_number_id)order -- MySQL would still sort, which is the cost being removed. The migration documents when to revisit: if most rows ever become non-blockable (seeded scores decay), the residual status filter would have to skip many rows to fill a page.2. The client asked for half the rows it was allowed
SyncService.pageLimitdefaulted to 500 while the server'smaxBlocklistLimitis 1000 -- double the round trips for nothing. Now defaults to 1000.3. Saves were quadratic in disk writes
BlocklistStore.saveserializes the whole state atomically, andsync()called it after every page. At 732k entries the file is ~8.8MB, so ~1465 per-page saves wrote ~6.4GB to flash for a single sync. State is now written everypagesPerSave(10) pages.Durability is preserved rather than traded away: a page failure persists whatever was folded in before rethrowing, so progress survives a mid-sync error exactly as it did when every page was saved. The pre-existing
test_sync_pageFailure_rethrows_doesNotReload_priorPageStillPersistedpasses unchanged, and two new tests cover the cases batching introduces -- the tail save after the last batch boundary, and a failure mid-batch.Net
Verification
make deploy-server, which ran).EXPLAINconfirms the index is in use, timings above are measured on the live 732k-row table, and 17/17 production smoke checks passed.