Fix scheduled cleanup of superseded media-usage generations - #2324
Fix scheduled cleanup of superseded media-usage generations#2324khoinguyenpham04 wants to merge 17 commits into
Conversation
🦋 Changeset detectedLatest commit: 3a9a9e8 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 |
Scope checkThis PR changes 2,629 lines across 13 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 959e69e | Aug 03 2026, 09:43 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 959e69e | Aug 03 2026, 09:43 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 959e69e | Aug 03 2026, 09:43 AM |
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. |
![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 stated problem: media-usage superseded generations were already deletable by the repository, but the scheduled path never called the cleanup, so they accumulated. Adding a scheduler-only, lease-bounded tick with a persisted cursor and a promotion fence is consistent with EmDash’s cron-driven maintenance model, and the PR includes good integration/query-plan coverage.
The implementation is mostly careful — generation write leases protect in-flight promotions, the cleanup lease check gates deletes, and triggers prevent rolling-deployment code from re-promoting a generation that cleanup has already reclaimed. I did not run the test suite (no shell), so my notes are from static review.
One issue rises to blocker-level if true: the PostgreSQL branch of activeCleanupLeaseExpression puts FOR UPDATE inside an EXISTS subquery. PostgreSQL does not allow FOR UPDATE in subqueries, so every cleanup query that passes a cleanupLease (candidate scan, orphan/stale/abandoned deletes, and expired writer-lease cleanup) should fail on Postgres. The existing statement-level trigger path already serializes source-side mutations against the cleanup singleton, so the cleanup queries only need the lease-token/expiry check here. I’ve left a line-anchored suggestion to remove the misplaced lock.
Aside from that, the diff is clean: no new logged-out hot-path queries, no UI/i18n changes, changeset is present and correctly scoped to the emdash package, and the tests verify behavior rather than implementation details.
github-actions
Bot
added
review/awaiting-author
There was a problem hiding this comment.
This PR addresses a real problem—unbounded growth of superseded media-usage projection generations—and the approach is consistent with the media-library umbrella it belongs to. The scheduler-only lease, bounded cursor sweep, generation write leases, and cleanup fence are elaborate, but they map to the stated requirements (single-flight, age window, rolling-deployment safety) and the new tests exercise the cross-dialect behavior, concurrency serialization, and query budgets.
I inspected the diff, read the changed source files and migrations, and traced the call sites up through runSystemCleanup/runScheduledTasks. I did not run the test suite, so I rely on the author’s reported results for passing behavior. No data-loss or security issues jumped out. The new cleanup path does not touch logged-out request code, and the query budgets are enforced in tests.
The remaining issues are convention/test-quality items, not runtime bugs:
- AGENTS.md comment discipline: the added JSDoc in
cleanup.tscontains justification language ("intentionally only called" / "request paths never ..."), andcleanup.tsadds a numbered step comment (// 6. ...). AGENTS.md explicitly forbids both numbered comments and justification/narrative comments. - Narrative comment in
media-usage.ts: the new// Guarded source deletes remove only...comment explains the design rationale rather than a non-obvious runtime invariant. - Brittle query-count assertion:
media-usage-cleanup-plan.test.tsasserts an exact statement count (toHaveLength(8)) in addition to the budget ceiling. The exact count will break on any intentional refactor that stays within the documented 14-statement cap.
Overall the implementation looks sound; fix the comment style before merge.
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review of the media-usage cleanup Sequence 0 child. The implementation still looks sound: the scheduler-only lease, bounded cursor sweep, generation write leases, cleanup fence, and cross-dialect tests address the stated problem without adding logged-out request queries or touching authorization/CSRF paths.
The prior review’s comment-discipline findings have been addressed: the justification/numbered-step comments in cleanup.ts and the narrative comment in media-usage.ts are gone, and the new cleanup code stays clear of numbered/justification comments.
The only remaining issue from the prior review is the brittle query-count assertion, now pinned to the new 14-statement cap instead of fixed. That is an AGENTS.md test-quality violation (a config-literal/test-count pinned back at the implementation). I did not run the test suite, so I rely on the author’s reported results for passing behavior; statically, no new runtime bugs, security issues, or data-integrity gaps jumped out.
github-actions
Bot
added
review/awaiting-author
…lan.test.ts Co-authored-by: emdashbot[bot] <273199577+emdashbot[bot]@users.noreply.github.com>
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a re-review of the Sequence 0 media-usage cleanup child. The approach is sound: a scheduler-only tick, persisted single-flight lease, age safety window, keyset cursor, bounded failure backoff, and cleanup-owned promotion fence address the stated unbounded-growth problem without adding logged-out request queries or touching auth/CSRF paths.
I read the full diff, the new cleanup/repository code, the two migrations, the scheduler wiring, and the new tests. The implementation looks correct: the lease check and fence prevent deleting in-flight generations, the cleanup_lease_token markings let normal source deletes remove rows without advancing the fence, and the cleanup row is integrated into runSystemCleanup as a non-fatal subsystem. No new runtime bugs, security issues, or data-integrity gaps stood out statically.
The prior review’s comment-discipline findings are resolved: the new cleanup code and supporting repository changes contain no numbered or justification comments.
The one still-open concern from the prior review is the brittle query-count assertion. media-usage-cleanup-plan.test.ts hard-codes the exact statement and bind budgets (14 and 52) as magic numbers. That is an AGENTS.md test-quality violation: asserting a config literal back at the implementation cannot fail on a real regression and will fail on any legitimate budget change. I also noticed media-usage-cleanup.test.ts asserts the literal 5000 instead of using the exported MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS constant.
I did not run the test suite; I rely on the author-reported results for passing behavior.
| expect(captured.length).toBeLessThanOrEqual(14); | ||
| for (const query of captured) { | ||
| expect(query.parameters.length).toBeLessThanOrEqual(52); |
There was a problem hiding this comment.
[needs fixing] These two expectations pin the exact statement and bind budgets (14 and 52) as magic literals. That is the config-pin/tautology AGENTS.md warns about: the test will fail the moment someone intentionally adjusts the budget and cannot fail on any real regression that still stays under the budget. The same pattern repeats in the failure-path test at lines 140–142.
If the caps are a public contract, add exported constants such as MEDIA_USAGE_CLEANUP_STATEMENT_BUDGET and MEDIA_USAGE_CLEANUP_BIND_BUDGET in cleanup.ts and assert against them. Otherwise, drop the numeric caps and keep the existing EXPLAIN QUERY PLAN assertions, which actually verify observable behavior (index coverage and no temp sort).
| expect(captured.length).toBeLessThanOrEqual(14); | |
| for (const query of captured) { | |
| expect(query.parameters.length).toBeLessThanOrEqual(52); | |
| expect(captured.length).toBeLessThanOrEqual(MEDIA_USAGE_CLEANUP_STATEMENT_BUDGET); | |
| for (const query of captured) { | |
| expect(query.parameters.length).toBeLessThanOrEqual(MEDIA_USAGE_CLEANUP_BIND_BUDGET); | |
| } |
|
|
||
| const result = await cleanupMediaUsage(db); | ||
| expect(result).toEqual( | ||
| expect.objectContaining({ candidateRows: 1, deletedRows: 0, durationMs: 5000 }), |
There was a problem hiding this comment.
[suggestion] This assertion hard-codes 5000, the same value as MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS. Use the exported constant (or an asymmetric range matcher) so the test does not need a manual update when the budget constant changes.
| expect.objectContaining({ candidateRows: 1, deletedRows: 0, durationMs: 5000 }), | |
| expect(result).toEqual( | |
| expect.objectContaining({ candidateRows: 1, deletedRows: 0, durationMs: MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS }), | |
| ); |
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a re-review of the Sequence 0 media-usage cleanup child. The approach remains sound: a scheduler-only tick, persisted single-flight lease, age safety window, keyset cursor, bounded failure backoff, and cleanup-owned fence address the stated unbounded-growth problem without adding logged-out request queries or touching auth/CSRF paths.
I re-read the cleanup implementation, both migrations, the repository changes, and the new tests. The comment-discipline findings from the prior review are resolved, and no new runtime bugs, security issues, or data-integrity gaps stood out statically.
The one still-open concern from the prior review has not been addressed. media-usage-cleanup-plan.test.ts still hard-codes the exact SQL-statement and bind-parameter budgets (14 and 52) as magic numbers. That is a config-pin test under AGENTS.md — it restates implementation choices and cannot fail on a real regression; it will only fail when someone intentionally changes the budget. I also still see the five-second admission budget duplicated across both new test files (MAX_CLEANUP_ADMISSION_TIME_MS = 5_000) instead of being imported from the source that defines MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS.
I did not run the test suite; I rely on the author-reported results for passing behavior.
| const MAX_CLEANUP_STATEMENTS_PER_TICK = 14; | ||
| const MAX_BIND_PARAMETERS_PER_CLEANUP_STATEMENT = 52; |
There was a problem hiding this comment.
[needs fixing] These two constants hard-code the cleanup tick's exact statement and bind budgets. That is a config-pin test: the assertions re-state the implementation's current budget and cannot fail on a real regression. They will only fail when someone intentionally changes the budget. AGENTS.md explicitly forbids this kind of tautological assertion.
Keep the index-plan checks (EXPLAIN QUERY PLAN / no temp B-tree), and remove the budget-pin assertions:
| const MAX_CLEANUP_STATEMENTS_PER_TICK = 14; | |
| const MAX_BIND_PARAMETERS_PER_CLEANUP_STATEMENT = 52; | |
| -const MAX_CLEANUP_STATEMENTS_PER_TICK = 14; | |
| -const MAX_BIND_PARAMETERS_PER_CLEANUP_STATEMENT = 52; | |
| const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; |
| export const MEDIA_USAGE_CLEANUP_INTERVAL_MS = 60 * 1000; | ||
| export const MEDIA_USAGE_CLEANUP_LEASE_MS = 5 * 60 * 1000; | ||
| export const MEDIA_USAGE_CLEANUP_SAFETY_WINDOW_MS = 60 * 60 * 1000; | ||
| const MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS = 5 * 1000; |
There was a problem hiding this comment.
[needs fixing] This time budget is asserted in both new cleanup tests via a locally-defined MAX_CLEANUP_ADMISSION_TIME_MS = 5_000. Export it so the tests can import it instead of duplicating the value:
| const MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS = 5 * 1000; | |
| -export const MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS = 5 * 1000; | |
| +export const MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS = 5 * 1000; |
Then replace the local constants in media-usage-cleanup.test.ts and media-usage-cleanup-plan.test.ts with the imported value.
| type DialectTestContext, | ||
| } from "../../utils/test-db.js"; | ||
|
|
||
| const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; |
There was a problem hiding this comment.
[needs fixing] This local constant duplicates the source's MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS in packages/core/src/media/usage/cleanup.ts. Once that constant is exported, import and reuse it:
| const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; | |
| -import { | |
| - cleanupMediaUsage, | |
| - MEDIA_USAGE_CLEANUP_CANDIDATE_LIMIT, | |
| - MEDIA_USAGE_CLEANUP_DELETE_LIMIT, | |
| - MEDIA_USAGE_CLEANUP_INTERVAL_MS, | |
| -} from "../../../src/media/usage/cleanup.js"; | |
| +import { | |
| + cleanupMediaUsage, | |
| + MEDIA_USAGE_CLEANUP_CANDIDATE_LIMIT, | |
| + MEDIA_USAGE_CLEANUP_DELETE_LIMIT, | |
| + MEDIA_USAGE_CLEANUP_INTERVAL_MS, | |
| + MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS, | |
| +} from "../../../src/media/usage/cleanup.js"; |
| const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; | |
| -const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; | |
| +const MAX_CLEANUP_ADMISSION_TIME_MS = MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS; |
|
|
||
| const MAX_CLEANUP_STATEMENTS_PER_TICK = 14; | ||
| const MAX_BIND_PARAMETERS_PER_CLEANUP_STATEMENT = 52; | ||
| const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; |
There was a problem hiding this comment.
[needs fixing] This local constant duplicates the source's MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS in packages/core/src/media/usage/cleanup.ts. Once that constant is exported, import and reuse it:
| const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; | |
| -import { | |
| - cleanupMediaUsage, | |
| - MEDIA_USAGE_CLEANUP_CANDIDATE_LIMIT, | |
| - MEDIA_USAGE_CLEANUP_DELETE_LIMIT, | |
| -} from "../../../src/media/usage/cleanup.js"; | |
| +import { | |
| + cleanupMediaUsage, | |
| + MEDIA_USAGE_CLEANUP_CANDIDATE_LIMIT, | |
| + MEDIA_USAGE_CLEANUP_DELETE_LIMIT, | |
| + MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS, | |
| +} from "../../../src/media/usage/cleanup.js"; |
| const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; | |
| -const MAX_CLEANUP_ADMISSION_TIME_MS = 5_000; | |
| +const MAX_CLEANUP_ADMISSION_TIME_MS = MEDIA_USAGE_CLEANUP_TIME_BUDGET_MS; |
github-actions
Bot
added
review/awaiting-author
1 participant
What does this PR do?
Fixes unbounded growth of superseded media-usage projection generations. The existing cleanup operations were never invoked by production scheduled maintenance, allowing obsolete occurrences to accumulate indefinitely.
This adds a scheduler-only, database-clock cleanup tick with a persisted single-flight lease, age safety window, finite keyset sweep cursor, bounded retry backoff, and a cleanup-owned fence for rolling deployments. The tick is capped at 250 candidates, 50 projection deletions, 49 expired writer-lease deletions, 14 SQL statements, 52 binds, and five seconds. An additive index supports the age-ordered scan on SQLite/D1 and PostgreSQL.
It is a completed Sequence 0 child of the media-library umbrella and should merge only into draft PR #2218.
Closes: N/A — child PR for #2218
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (targeted: media-usage repository, migrations, cleanup, and query-plan suites)pnpm formathas been runmessages.pochanges except in translation PRs — N/A; no admin UI strings are changed.AI-generated code disclosure
Screenshots / test output
No visual changes.
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/media-usage-cleanup-sequence-0. Updated automatically when the playground redeploys.