fix: garbage-collect superseded media-usage generations - #2306
Conversation
Every content save writes a fresh generation of _emdash_media_usage rows and leaves the superseded generation behind. Reads join on current_generation, so non-current rows are pure dead weight — and the three repository GC methods that reclaim them had no callers, growing the table one generation per save without bound (90.5% of rows stale on an audited production database). Compose the GC methods into a sweep behind a one-hour safety window (guarded writers insert occurrence rows before winning the source CAS) with a bounded per-tick batch, and run it from runSystemCleanup so both scheduler drivers pick it up. A large backlog drains across ticks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 343d532 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 |
github-actions
Bot
added
review/needs-review
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right change for the right problem. _emdash_media_usage was growing without bound because the repository already had GC helpers but nothing invoked them; wiring them into the existing cron-only runSystemCleanup path is additive, low-risk, and avoids adding a DELETE round-trip to the authenticated save path.
I checked the changed files, traced runSystemCleanup call sites in emdash-runtime.ts, and confirmed the sweep is only reached from scheduler ticks (not logged-out HTTP routes), so the three extra age-gated queries per tick fit the project's query-count conventions. The sweep correctly preserves in-flight work via the one-hour cutoff, and the three repository GC partitions (stale created_at < indexed_at, abandoned created_at >= indexed_at, orphaned no-source) are disjoint. The integration test reproduces the superseded-generation case under describeEachDialect and will fail on main because the new module is absent.
No logic bugs or regressions. I did find one AGENTS.md comment-convention violation (a numbered subsystem comment) and one performance/discipline note: the GC queries order _emdash_media_usage by created_at, but the table has no index on that column, so backlogs will scan. Both are fixable in small follow-up edits.
| console.error("[cleanup] Failed to prune revisions:", error); | ||
| } | ||
|
|
||
| // 6. Media-usage occurrence rows superseded by newer generations |
There was a problem hiding this comment.
[needs fixing] Line 146 adds a numbered subsystem comment (// 6. ...). AGENTS.md's Comments section prohibits numbered comments, and the next line (await cleanupMediaUsageGenerations(db)) already says what the step does. Drop the numbered comment.
| // 6. Media-usage occurrence rows superseded by newer generations | |
| try { | |
| const usage = await cleanupMediaUsageGenerations(db); | |
| result.mediaUsageStaleGenerations = usage.staleGenerations; | |
| result.mediaUsageAbandonedGenerations = usage.abandonedGenerations; | |
| result.mediaUsageOrphanOccurrences = usage.orphanOccurrences; | |
| } catch (error) { | |
| console.error("[cleanup] Failed to clean media-usage generations:", error); | |
| } |
| const cutoff = new Date(Date.now() - GC_MAX_AGE_MS).toISOString(); | ||
| const repo = new MediaUsageRepository(db); | ||
| return { | ||
| staleGenerations: await repo.deleteStaleGenerationsOlderThan(cutoff, GC_BATCH_LIMIT), |
There was a problem hiding this comment.
[suggestion] The sweep now actively runs three age-gated DELETE/SELECT pairs against _emdash_media_usage ordered by created_at (see MediaUsageRepository.deleteStaleGenerationsOlderThan, deleteAbandonedGenerationsOlderThan, and deleteOrphanOccurrencesOlderThan). Migration 046 created indexes on media_id, source_key+generation, and provider+provider_asset_id, but not on created_at. Without it, the maintenance sweep will scan or sort the whole table for any non-trivial backlog. Consider adding a forward-only migration such as:
// packages/core/src/database/migrations/055_media_usage_created_at_index.ts
import type { Kysely } from "kysely";
export async function up(db: Kysely<unknown>): Promise<void> {
await db.schema
.createIndex("idx__emdash_media_usage_created_at")
.ifNotExists()
.on("_emdash_media_usage")
.column("created_at")
.execute();
}
export async function down(db: Kysely<unknown>): Promise<void> {
await db.schema.dropIndex("idx__emdash_media_usage_created_at").ifExists().execute();
}(If production already has this index out-of-band, a guarded ifNotExists migration is still the safest place to document/make it portable.)
There was a problem hiding this comment.
Agreed the created_at index is worth having for backlogs. I'm deliberately not adding the migration in this PR because of migration-name interlock with two other in-flight PRs from this audit batch: Kysely's runner rejects any new migration that sorts before an already-executed one, and 055/056 are claimed by the FTS fixes about to open from the same fork. Grabbing 057 here would brick upgrades for any site that runs this PR's migration before those land. I'll send the index as a tiny follow-up PR once the in-flight numbering settles — or fold it into whichever of the batch merges last, if maintainers prefer.
| * the superseded generation behind; reads join on current_generation, so | ||
| * stale rows are dead weight that grows one generation per save. The sweep | ||
| * composes the repository GC methods behind a safety window and runs from | ||
| * runSystemCleanup (untestable here directly — see tests/unit/cleanup.test.ts). |
There was a problem hiding this comment.
[suggestion] Line 8 points readers to tests/unit/cleanup.test.ts for runSystemCleanup coverage, but that file explicitly says it does not test runSystemCleanup directly. The parenthetical is also reviewer-facing justification/narrative, which AGENTS.md discourages. Remove the cross-reference and keep only the test's actual purpose.
| * runSystemCleanup (untestable here directly — see tests/unit/cleanup.test.ts). | |
| /** | |
| * Maintenance sweep for media-usage generations. | |
| * | |
| * Every content save writes a fresh generation of occurrence rows and leaves | |
| * the superseded generation behind; reads join on current_generation, so | |
| * stale rows are dead weight that grows one generation per save. The sweep | |
| * composes the repository GC methods behind a safety window and is exercised | |
| * here without the scheduler. | |
| */ |
There was a problem hiding this comment.
Adopted your suggested docstring — pushed in 8ccea09.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
The sweep's own behavior was covered, but nothing asserted that runSystemCleanup actually invokes it — the step could have been dropped without a test noticing. runSystemCleanup is directly callable from integration tests, so assert the new result fields end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Follow-up in 89d8219: added an end-to-end test asserting |
89d8219 to
343d532
Compare
1 participant
What does this PR do?
Fixes
_emdash_media_usagegrowing without bound: the table's generation model writes a fresh set of occurrence rows on every content save and leaves the superseded generation behind, and the three repository GC methods that exist to reclaim them (deleteStaleGenerationsOlderThan,deleteAbandonedGenerationsOlderThan,deleteOrphanOccurrencesOlderThan) had zero call sites. On the audited production deployment (Macabro festival site, emdash 0.31.1), 90.5% of rows (3,136 / 3,466) were stale generations, growing ~191 rows/day. Reads join oncurrent_generation, so the stale rows are pure dead weight.The fix wires the existing GC into the periodic maintenance path:
cleanupMediaUsageGenerations(db)inmedia/usage/gc.tscomposes the three GC methods behind a shared one-hour cutoff and a bounded per-tick batch (500). The age gate matters: guarded writers insert occurrence rows before winning the source CAS, so the window must exceed any plausible in-flight write (it mirrors the pending-upload abandonment window). The batch cap amortizes a large backlog across ticks instead of one oversized D1 batch.runSystemCleanupruns it as a new independent, non-fatal step (same try/catch-per-subsystem pattern as the other five), which covers both scheduler drivers — the Cloudflare Cron Trigger path and the Node scheduler tick.CleanupResultis extended additively with three new count fields.Deliberately not done: deleting the superseded generation inline in the save path. That would add a DELETE round-trip to every authenticated save on D1, would not reclaim abandoned (losing-CAS) generations or orphans anyway, and the repository's existing dialect-parity tests intentionally construct stale generations via
replaceSource— the sweep-only fix is purely additive. The cron path is not the logged-out hot path, so the three idle SELECTs per tick are within the project's query rules.Found during a measured database audit of a production deployment.
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: no admin UI strings changedAI-generated code disclosure
Screenshots / test output
The new test fails on
main(the sweep module does not exist — nothing calls the GC). After the fix, the sweep suite runs underdescribeEachDialect(SQLite + Postgres parity) and asserts: two saves + aged superseded rows → only the current generation survives and current usage still resolves; superseded rows younger than the safety window are untouched.🤖 Generated with Claude Code