Skip to content

Fix scheduled cleanup of superseded media-usage generations - #2324

Merged
khoinguyenpham04 merged 19 commits into
mainfrom
fix/media-usage-cleanup-sequence-0
Aug 11, 2026
Merged

Fix scheduled cleanup of superseded media-usage generations#2324
khoinguyenpham04 merged 19 commits into
mainfrom
fix/media-usage-cleanup-sequence-0

Conversation

@khoinguyenpham04

@khoinguyenpham04 khoinguyenpham04 commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

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

  • 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 (targeted: media-usage repository, migrations, cleanup, and query-plan suites)
  • 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 — N/A; no admin UI strings are changed.
  • I have added a changeset (if this PR changes a published package)
  • New features link to an approved Discussion: N/A — bug fix.

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: OpenAI Codex (GPT-5.6 Terra)

Screenshots / test output

No visual changes.

pnpm lint:json | jq '.diagnostics | length'  # 0
pnpm lint:quick
pnpm typecheck
pnpm format

packages/core focused SQLite suite
  Test Files  5 passed (5)
       Tests  88 passed | 1 skipped (89)

Focused PostgreSQL suite: 156 passed
D1 query-plan/bind proof: matched committed snapshots exactly
SQLite query-count proof: matched committed snapshots exactly

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.

@changeset-bot

changeset-bot Bot commented Aug 2, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 3107e88

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

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Scope check

This 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.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 2, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-playground 3107e88 Aug 11 2026, 04:49 PM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 2, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-do 3107e88 Aug 11 2026, 04:49 PM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 2, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-cache 3107e88 Aug 11 2026, 04:53 PM

@github-actions

github-actions Bot commented Aug 2, 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.

@khoinguyenpham04
khoinguyenpham04 marked this pull request as ready for review August 2, 2026 23:10
@github-actions github-actions Bot added the review/needs-review No maintainer or bot review yet label Aug 2, 2026
@khoinguyenpham04 khoinguyenpham04 added the bot:review Trigger an emdashbot code review on this PR label Aug 2, 2026
emdashbot[bot]

This comment was marked as outdated.

@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 2, 2026
@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-review No maintainer or bot review yet labels Aug 2, 2026
emdashbot[bot]

This comment was marked as outdated.

@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Aug 3, 2026
@khoinguyenpham04 khoinguyenpham04 added the bot:review Trigger an emdashbot code review on this PR label Aug 3, 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.

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.

Comment thread packages/core/tests/integration/database/media-usage-cleanup-plan.test.ts Outdated
@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 3, 2026
@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond review/needs-rereview Author pushed changes since the last review and removed review/needs-rereview Author pushed changes since the last review review/awaiting-author Reviewed; waiting on the author to respond labels Aug 3, 2026
@khoinguyenpham04 khoinguyenpham04 added the bot:review Trigger an emdashbot code review on this PR label Aug 3, 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.

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.

Comment thread packages/core/tests/integration/database/media-usage-cleanup-plan.test.ts Outdated
Comment thread packages/core/tests/integration/database/media-usage-cleanup.test.ts Outdated
@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 3, 2026
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/approved Approved; no new commits since labels Aug 11, 2026
Base automatically changed from feature/media-library to main August 11, 2026 14:55
@khoinguyenpham04
khoinguyenpham04 force-pushed the fix/media-usage-cleanup-sequence-0 branch from bd23774 to e245c98 Compare August 11, 2026 14:55
@khoinguyenpham04
khoinguyenpham04 force-pushed the fix/media-usage-cleanup-sequence-0 branch from e245c98 to 3107e88 Compare August 11, 2026 16:43
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 11, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
docs 3107e88 Aug 11 2026, 04:47 PM

@khoinguyenpham04
khoinguyenpham04 merged commit c9d5ebd into main Aug 11, 2026
72 of 73 checks passed
@khoinguyenpham04
khoinguyenpham04 deleted the fix/media-usage-cleanup-sequence-0 branch August 11, 2026 18:08
@emdashbot emdashbot Bot mentioned this pull request Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants