Skip to content

perf: index media-usage created_at for the cleanup sweep - #2391

Closed
edrpls wants to merge 11 commits into
emdash-cms:mainfrom
edrpls:fix/media-usage-created-at-index
Closed

perf: index media-usage created_at for the cleanup sweep#2391
edrpls wants to merge 11 commits into
emdash-cms:mainfrom
edrpls:fix/media-usage-created-at-index

Conversation

@edrpls

@edrpls edrpls commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds idx__emdash_media_usage_created_at on _emdash_media_usage(created_at, id), as migration 062_media_usage_created_at_index.

The maintenance sweep from #2306 runs three age-gated SELECT/DELETE pairs against _emdash_media_usage filtered and ordered on created_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's 061_fts_plain_text, and merge order is enforced by the branch chain. Review the last four commits; the earlier ones are #2313/#2314. The trailing id column covers the GC batches' (created_at, id) ordering with no residual sort.

Type of change

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • User-visible strings in the admin UI are wrapped for translation (if applicable). Do not include messages.po changes except in translation PRs — a workflow extracts catalogs on merge to main.
  • I have added a changeset
  • New features link to an approved Discussion: https://github.com/emdash-cms/emdash/discussions/...

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

  • This PR includes AI-generated code — model/tool: Claude Fable 5 (Claude Code)

Screenshots / test output

Test Files  1 passed (1)
     Tests  19 passed (19)   tests/integration/database/migrations.test.ts

@changeset-bot

changeset-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 60cf2ab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages
Name Type
emdash Patch
@emdash-cms/cloudflare Patch
@emdash-cms/sandbox-workerd Patch
@emdash-cms/plugin-mcp-smoke Patch
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/do-demo-site Patch
@emdash-cms/do-solo-demo-site Patch
@emdash-cms/admin Patch
@emdash-cms/auth Patch
@emdash-cms/blocks Patch
@emdash-cms/gutenberg-to-portable-text Patch
@emdash-cms/x402 Patch
create-emdash Patch
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

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

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts and included in the trailing-migrations re-run test.
  • Confirmed the index name follows the existing idx__emdash_media_usage_* convention and that up/down use ifNotExists/ifExists.
  • Traced the GC calls in packages/core/src/media/usage/gc.ts down to the three cleanup methods in media-usage.ts; all three SELECT/delete age-gated batches order by u.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.

Comment thread packages/core/src/database/migrations/059_media_usage_created_at_index.ts Outdated
Comment thread packages/core/src/database/migrations/runner.ts Outdated
@pkg-pr-new

pkg-pr-new Bot commented Aug 9, 2026

Copy link
Copy Markdown

Open in StackBlitz

@emdash-cms/admin

npm i https://pkg.pr.new/@emdash-cms/admin@2391

@emdash-cms/auth

npm i https://pkg.pr.new/@emdash-cms/auth@2391

@emdash-cms/auth-atproto

npm i https://pkg.pr.new/@emdash-cms/auth-atproto@2391

@emdash-cms/blocks

npm i https://pkg.pr.new/@emdash-cms/blocks@2391

@emdash-cms/cloudflare

npm i https://pkg.pr.new/@emdash-cms/cloudflare@2391

@emdash-cms/contentful-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/contentful-to-portable-text@2391

emdash

npm i https://pkg.pr.new/emdash@2391

create-emdash

npm i https://pkg.pr.new/create-emdash@2391

@emdash-cms/gutenberg-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/gutenberg-to-portable-text@2391

@emdash-cms/plugin-cli

npm i https://pkg.pr.new/@emdash-cms/plugin-cli@2391

@emdash-cms/plugin-types

npm i https://pkg.pr.new/@emdash-cms/plugin-types@2391

@emdash-cms/registry-client

npm i https://pkg.pr.new/@emdash-cms/registry-client@2391

@emdash-cms/registry-lexicons

npm i https://pkg.pr.new/@emdash-cms/registry-lexicons@2391

@emdash-cms/registry-verification

npm i https://pkg.pr.new/@emdash-cms/registry-verification@2391

@emdash-cms/sandbox-workerd

npm i https://pkg.pr.new/@emdash-cms/sandbox-workerd@2391

@emdash-cms/x402

npm i https://pkg.pr.new/@emdash-cms/x402@2391

@emdash-cms/plugin-ai-moderation

npm i https://pkg.pr.new/@emdash-cms/plugin-ai-moderation@2391

@emdash-cms/plugin-atproto

npm i https://pkg.pr.new/@emdash-cms/plugin-atproto@2391

@emdash-cms/plugin-audit-log

npm i https://pkg.pr.new/@emdash-cms/plugin-audit-log@2391

@emdash-cms/plugin-color

npm i https://pkg.pr.new/@emdash-cms/plugin-color@2391

@emdash-cms/plugin-embeds

npm i https://pkg.pr.new/@emdash-cms/plugin-embeds@2391

@emdash-cms/plugin-field-kit

npm i https://pkg.pr.new/@emdash-cms/plugin-field-kit@2391

@emdash-cms/plugin-forms

npm i https://pkg.pr.new/@emdash-cms/plugin-forms@2391

@emdash-cms/plugin-webhook-notifier

npm i https://pkg.pr.new/@emdash-cms/plugin-webhook-notifier@2391

commit: 60cf2ab

@github-actions github-actions Bot added area/core size/S review/awaiting-author Reviewed; waiting on the author to respond review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Aug 9, 2026
@edrpls
edrpls force-pushed the fix/media-usage-created-at-index branch 2 times, most recently from 322a2d2 to f51b704 Compare August 9, 2026 11:16
@ascorbic ascorbic added the bot:review Trigger an emdashbot code review on this PR label Aug 9, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts and the unified diff.
  • Verified the migration is statically imported in runner.ts and 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.ts through to the three repository cleanup methods; all three filter/order on u.created_at.
  • Checked index naming against the existing idx__emdash_media_usage_* convention and verified createIndex(...).ifNotExists() / dropIndex(...).ifExists() idempotency.
  • Reviewed the changeset -- it targets the emdash package 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.

Comment thread packages/core/src/database/migrations/061_media_usage_created_at_index.ts Outdated
@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 9, 2026
@github-actions github-actions Bot added review/approved Approved; no new commits since review/needs-rereview Author pushed changes since the last review and removed review/needs-rereview Author pushed changes since the last review review/approved Approved; no new commits since labels Aug 9, 2026
@edrpls
edrpls force-pushed the fix/media-usage-created-at-index branch from e5611cb to d43482c Compare August 9, 2026 14:34
@edrpls
edrpls force-pushed the fix/media-usage-created-at-index branch from d43482c to 1e920f9 Compare August 9, 2026 15:22
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Overlapping PRs

This PR modifies files that are also changed by other open PRs:

This may cause merge conflicts or duplicated work. A maintainer will coordinate.

@edrpls
edrpls force-pushed the fix/media-usage-created-at-index branch from 1e920f9 to 000909b Compare August 10, 2026 15:18
edrpls and others added 4 commits August 11, 2026 10:17
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
edrpls and others added 7 commits August 11, 2026 10:17
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
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
@edrpls
edrpls force-pushed the fix/media-usage-created-at-index branch from 000909b to 60cf2ab Compare August 11, 2026 16:19
@edrpls

edrpls commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #2324: its 061_media_usage_cleanup migration created idx__emdash_media_usage_cleanup_scan on (created_at, id, source_key, generation), whose leading prefix covers every filter and (created_at, id) batch ordering this PR's narrower index targeted — adding this one now would be pure write-amplification redundancy. That also completes the follow-up agreed in the #2306 review, just via the upstream rework instead of this PR. Thanks for the review passes, @ascorbic — the landing chain is now just #2313#2314.

@edrpls edrpls closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core overlap review/needs-rereview Author pushed changes since the last review size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants