perf: index media-usage created_at for the cleanup sweep - #2391
Conversation
🦋 Changeset detectedLatest commit: 60cf2ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Approach
This is the right follow-up to the media-usage GC work: the cleanup queries in MediaUsageRepository are all age-gated on _emdash_media_usage.created_at, and the existing indexes on that table lead with source/media identity columns. Once stale generations back up, the planner has to scan the whole table. A single-column index on created_at is the smallest, safest fix and matches the maintenance sweep exactly.
What I checked
- Read the diff and the new migration (
059_media_usage_created_at_index.ts). - Verified the migration is imported and registered in
runner.tsand included in the trailing-migrations re-run test. - Confirmed the index name follows the existing
idx__emdash_media_usage_*convention and thatup/downuseifNotExists/ifExists. - Traced the GC calls in
packages/core/src/media/usage/gc.tsdown to the three cleanup methods inmedia-usage.ts; all threeSELECT/delete age-gated batches order byu.created_at. - Confirmed the changeset targets the correct package (
emdash) and describes an observable effect. - There are no admin UI strings, no new logged-out queries, no SQL interpolation, and no authorization changes here.
Headline conclusion: the change is clean and safe, with two minor notes below.
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
322a2d2 to
f51b704
Compare
There was a problem hiding this comment.
Approach
This is the right follow-up to the media-usage GC work. The three cleanup methods in MediaUsageRepository (deleteStaleGenerationsOlderThan, deleteAbandonedGenerationsOlderThan, deleteOrphanOccurrencesOlderThan) all gate and order batches on _emdash_media_usage.created_at, while the table's existing indexes lead with source/media identity columns. A single-column index on created_at is the smallest, safest fix and matches those queries exactly.
What changed since the last review
The migration has been renumbered from 059 to 061 to leave slots 059/060 for the in-flight FTS migrations, which resolves the merge-order concern noted previously.
What I checked
- Read the new migration
061_media_usage_created_at_index.tsand the unified diff. - Verified the migration is statically imported in
runner.tsand registered under the matching name. - Confirmed it is included in the trailing-migrations re-run test case.
- Traced the GC calls in
media/usage/gc.tsthrough to the three repository cleanup methods; all three filter/order onu.created_at. - Checked index naming against the existing
idx__emdash_media_usage_*convention and verifiedcreateIndex(...).ifNotExists()/dropIndex(...).ifExists()idempotency. - Reviewed the changeset -- it targets the
emdashpackage and describes an observable performance effect for operators. - No new queries on logged-out routes, no admin UI strings, no SQL interpolation, and no authorization changes.
Headline conclusion: the change is correct and safe. One minor comment-trim suggestion below.
e5611cb to
d43482c
Compare
d43482c to
1e920f9
Compare
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
1e920f9 to
000909b
Compare
FTS5 tables were external-content (content='ec_<slug>'), which forces the index to mirror raw column values — and Portable Text fields store JSON, so structural tokens polluted the index (27-29% of it on an audited production database). Searching "normal" (a PT style value) matched 870/906 posts, "_type" matched every document, and snippets showed JSON fragments. Rebuild the FTS tables as self-contained FTS5 whose Portable Text columns hold extracted prose: every JSON string under a text, alt, caption, or code key (span text, image alt/caption, code blocks — the same semantics as extractPlainText). Extraction lives in SQL (json_tree) because the sync triggers cannot call into JS, with json_valid guarding legacy bare-string rows. Self-contained tables also retire the external-content 'delete' choreography and its corruption modes (migration 039's subject): removal is a plain DELETE, a harmless no-op for never-indexed rows, and INSERT OR REPLACE makes concurrent D1 populates converge. Migration 055 rebuilds every search-enabled collection's index and triggers on upgrade; the trigger SQL is lock-step with FTSManager per 039's precedent. The search query layer is unchanged — it joins ec_* by id for metadata, and snippet() now reads the stored prose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
json_tree exposes output columns named key/value/type/path and friends; a bare column reference inside the extraction subquery binds to those instead of the outer ec_* column, so populating a Portable Text field slugged with one of these names silently indexed NULL. Triggers were unaffected (NEW.-qualified). Qualify the populate and migration references with the content table name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A portableText column holding a JSON scalar ('2024', '"Some title"')
passed json_valid, extracted to NULL, and silently dropped out of the
index. Gate extraction on json_type IN ('array','object') so scalars
stay raw-indexed, in both the manager and the rebuild migration.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
Shipping the guard inside the rebuild spares existing sites a second back-to-back full re-tokenization from a separate follow-up migration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
The FTS update trigger fired on ANY row UPDATE, deleting and re-inserting the document's full index entry even when no searchable column changed. Metadata-only saves — status flips, scheduling, autosave version bumps — and the publish path's rewrite-identical-values UPDATEs each paid full re-tokenization: measured 49x CPU on metadata-only saves and 78-89% of a save's WAL bytes on an audited production deployment. Add a WHEN guard comparing raw column values with null-safe IS NOT: the trigger fires only when an indexed value, the row's locale, or its trash state actually changed. deleted_at stays in the guard so trash/restore keep syncing the index. Raw-column comparison remains valid change detection for Portable Text fields whose indexed values are extracted text. Existing deployments pick up the guarded triggers through the FTS rebuild migration on the base branch, so upgrades re-tokenize the corpus once, not twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
The GC sweep added in emdash-cms#2306 selects and deletes _emdash_media_usage rows by age, but every index on the table leads with identity columns, so the age-gated queries scan the table once a backlog builds. Follow-up agreed with ascorbic in the emdash-cms#2306 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
The GC batches order by (created_at, id); the trailing id column removes the residual sort. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175x2Nu2m7LZUznaEQUAVQd
000909b to
60cf2ab
Compare
|
Closing as superseded by #2324: its |
What does this PR do?
Adds
idx__emdash_media_usage_created_aton_emdash_media_usage(created_at, id), as migration062_media_usage_created_at_index.The maintenance sweep from #2306 runs three age-gated
SELECT/DELETEpairs against_emdash_media_usagefiltered and ordered oncreated_at. The table's existing indexes (046, 052) all lead with identity columns, so those queries scan the whole table once a backlog of stale generations builds up. This is the follow-up agreed with @ascorbic in the #2306 review (thread).Stacked on #2314 (which sits on #2313), mirroring how those two are stacked — the migration is
062, immediately after #2313's061_fts_plain_text, and merge order is enforced by the branch chain. Review the last four commits; the earlier ones are #2313/#2314. The trailingidcolumn covers the GC batches'(created_at, id)ordering with no residual sort.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.N/A notes: no admin UI strings (i18n item n/a); not a feature — maintainer-requested follow-up from the #2306 review, so no Discussion. Test coverage is the migration's entry in the trailing-migrations re-run case of
tests/integration/database/migrations.test.ts(an index-existence assertion would only restate the migration).AI-generated code disclosure
Screenshots / test output