fix(core): bound revision cleanup work - #2407
Conversation
🦋 Changeset detectedLatest commit: a32d1c0 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
area/core
size/L
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
emdash-demo-do | a32d1c0 | Aug 10 2026, 02:46 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | a32d1c0 | Aug 10 2026, 02:44 PM |
@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: |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | a32d1c0 | Aug 10 2026, 02:43 PM |
![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 fix for #2211. Replacing the cron-time full-table GROUP BY on revisions with a durable _emdash_revision_prune_queue table is a sound, idiomatic approach for EmDash: writes coalesce pruning work per entry via an upsert, request-lifetime pruning is preserved by deferring through after(), and scheduled cleanup bounds its work to a small batch ordered by the queue's revision id. The migration backfills existing excessive histories once and is idempotent.
I read the diff, the changed handlers/runtime/cleanup/migration files, and the new tests. I also traced the remaining RevisionRepository call sites to confirm all production revision writes go through RevisionRepository.create, which now queues the entry automatically. The change sticks to the background-maintenance scope, adds no logged-out hot-path queries, and includes a changeset.
Two minor items worth tidying:
-
The module docstring for
runSystemCleanupstill claims every subsystem cleanup is “a single DELETE with a WHERE clause,” but revision cleanup is now a SELECT + per-entry prune. That should be updated so future readers don't get the wrong expectation. -
The new migration test asserts a specific SQLite
EXPLAIN QUERY PLANstring. Query-plan choice depends on the SQLite version, statistics, and table size; on a tiny table the planner may scan or use a temp sort even when the index exists. This assertion is brittle and tests the planner rather than observable behavior. The same behavior is already implicitly tested by the cleanup tests that rely on the queue; if you want to verify the index, checksqlite_master/Kysely introspection instead, or just drop the plan assertion.
Neither is a blocker, but I'd remove the stale docstring claim and the brittle EXPLAIN assertion before merging.
Findings
-
[suggestion]
packages/core/src/cleanup.ts:45The docstring is now stale. Revision cleanup is no longer a single
DELETE; it selects from_emdash_revision_prune_queueand runspruneOldRevisionsper queued entry. Update the claim so it doesn't mislead future readers.* Safe to call frequently -- each subsystem cleanup is idempotent and bounded, * so repeated calls with nothing to clean are cheap (no-op queries). -
[suggestion]
packages/core/tests/unit/database/migrations/059_revision_prune_queue.test.ts:41-50Asserting a specific
EXPLAIN QUERY PLANstring is brittle: SQLite's planner may scan a tiny table or sort in memory even when the index exists. This tests the query planner, not observable migration behavior. The cleanup integration tests already exercise the queue ordering, so you can drop the plan assertion.expect(queued.rows).toEqual([{ collection: "post", entry_id: "entry-1", revision_id: "50" }]); await down(db);
There was a problem hiding this comment.
Pull request overview
This PR bounds revision-retention cleanup work by replacing the scheduled full-table revisions scan with a durable _emdash_revision_prune_queue, coalescing pruning per (collection, entry) on revision writes and processing a small batch during scheduled cleanup to avoid excessive D1 row reads (fixing #2211).
Changes:
- Add
_emdash_revision_prune_queue(migration + DB types) and enqueue pruning work on revision writes via an upsert. - Update request-lifetime pruning paths to prune a queued entry via
after(), and update scheduled cleanup to process a bounded batch (10) ordered by oldest queued revision id. - Add unit/integration tests covering migration behavior, request-lifetime retention without cron, and ensuring scheduled cleanup doesn’t query
revisionswhen the queue is empty.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/tests/unit/database/migrations/059_revision_prune_queue.test.ts | Tests new migration creates/uses the prune queue and is idempotent. |
| packages/core/tests/unit/cleanup.test.ts | Adds scheduled-cleanup coverage for queue-based pruning and “no full scan” behavior. |
| packages/core/tests/integration/database/migrations.test.ts | Registers the new migration in the integration migration list. |
| packages/core/tests/integration/content/revision-retention.test.ts | Ensures request-lifetime after() pruning bounds history without scheduled cleanup. |
| packages/core/src/emdash-runtime.ts | Switches revision pruning from direct pruning to queued pruning via after(). |
| packages/core/src/database/types.ts | Adds _emdash_revision_prune_queue to the typed DB schema. |
| packages/core/src/database/repositories/revision.ts | Enqueues pruning work on revision creation and adds pruneQueuedEntry(). |
| packages/core/src/database/migrations/runner.ts | Registers migration 059_revision_prune_queue. |
| packages/core/src/database/migrations/059_revision_prune_queue.ts | Creates the prune-queue table/index and seeds it from existing excessive histories. |
| packages/core/src/cleanup.ts | Updates scheduled cleanup to prune a bounded batch from the queue instead of scanning revisions. |
| packages/core/src/api/handlers/revision.ts | Uses queued pruning via after() after restore writes. |
| .changeset/fix-cron-revision-row-reads.md | Adds user-visible release note for the cron row-read fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const pruned = await this.pruneOldRevisions(collection, entryId, keepCount); | ||
| await this.db | ||
| .deleteFrom("_emdash_revision_prune_queue") | ||
| .where("collection", "=", collection) | ||
| .where("entry_id", "=", entryId) | ||
| .where("revision_id", "=", queuedRevisionId) | ||
| .execute(); | ||
| return pruned; |
| } catch (error) { | ||
| console.error( | ||
| `[revisions] Failed to queue revision pruning for ${input.collection}/${input.entryId}:`, | ||
| error, | ||
| ); | ||
| } |
| async function pruneQueuedRevisions(db: Kysely<Database>): Promise<number> { | ||
| const queued = await db | ||
| .selectFrom("_emdash_revision_prune_queue") | ||
| .selectAll() | ||
| .orderBy("revision_id") | ||
| .limit(REVISION_PRUNE_BATCH_SIZE) | ||
| .execute(); | ||
| const revisionRepo = new RevisionRepository(db); | ||
| let totalPruned = 0; | ||
|
|
github-actions
Bot
added
review/awaiting-author
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. |
There was a problem hiding this comment.
This is a sound fix for #2211: replacing the cron-time full-table GROUP BY on revisions with a durable _emdash_revision_prune_queue table is idiomatic for EmDash's background-maintenance architecture. Writes coalesce pruning work per entry via a primary-key upsert, request-lifetime pruning is preserved by deferring through after(), and scheduled cleanup bounds its work to a small batch ordered by the queue's revision id. The migration backfills existing excessive histories once and is idempotent.
I re-read the diff, the changed production files (cleanup.ts, RevisionRepository, emdash-runtime.ts, api/handlers/revision.ts, the migration, and database/types.ts), and traced the remaining RevisionRepository.create call sites to confirm all production revision writes queue the entry automatically. The new integration and unit tests cover the request-lifetime path, the scheduled batch, the oldest-first ordering, and the absence of a full-table scan query.
Both items from the prior review have been addressed in this revision:
- The stale
cleanup.tsdocstring claim that every subsystem cleanup is "a single DELETE with a WHERE clause" has been replaced with language accurately describing the new bounded queue-based revision cleanup. - The brittle SQLite
EXPLAIN QUERY PLANassertion in the migration test has been removed; the migration test now checks observable behavior only.
I don't see any remaining logic, regression, security, or AGENTS.md issues. No blocking findings.
github-actions
Bot
added
review/approved
There was a problem hiding this comment.
This is a sound fix for #2211: replacing the cron-time full-table GROUP BY on revisions with a durable _emdash_revision_prune_queue table fits EmDash's background-maintenance pattern, and the bounded batch with oldest-first ordering addresses the core performance problem. The migration is idempotent and the previous round's documentation/test issues are resolved.
However, the implementation diverges from the PR description and from AGENTS.md: the description says request-lifetime pruning is retained "through after()", and AGENTS.md says maintenance/bookkeeping writes should be deferred with after() so they don't block TTFB. In practice all three request-lifetime pruning sites now await the prune DELETE inline, which adds latency to every admin save on a revision-enabled collection and regresses from the prior fire-and-forget behavior. These should be wrapped in after() (with internal catch logging, since after() itself swallows rejections). The deferred task can race safely with scheduled cleanup because pruneQueuedEntry is idempotent and deletes the queue row by its captured revision id.
The other changed code looks correct: RevisionRepository.create queues every write, deleteByEntry clears the queue, all production revision inserts go through the repository, SQL identifiers are validated or parameterized, and the migration creates the expected primary key and index. I didn't spot any remaining security, data-integrity, or AGENTS.md convention issues.
| } else { | ||
| void revisionRepo.pruneOldRevisions(collection, resolvedId, 50).catch(() => {}); | ||
| try { | ||
| await revisionRepo.pruneQueuedEntry(collection, resolvedId, revision.id, 50); | ||
| } catch (error) { | ||
| console.error( | ||
| `[revisions] Failed to prune revisions for ${collection}/${resolvedId}:`, | ||
| error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[needs fixing] After staging a draft revision, this awaits pruneQueuedEntry before returning the content response. The PR description says request-lifetime pruning is retained "through after()", and AGENTS.md says maintenance/bookkeeping writes should be deferred with after() so they don't block TTFB. Awaiting this DELETE inline adds latency to every save on a revision-enabled collection.
Wrap the prune in after(); revisionRepo.pruneQueuedEntry(...) is already internally safe to lose to the cleanup retry path, and after() will log unhandled rejections.
| } else { | |
| void revisionRepo.pruneOldRevisions(collection, resolvedId, 50).catch(() => {}); | |
| try { | |
| await revisionRepo.pruneQueuedEntry(collection, resolvedId, revision.id, 50); | |
| } catch (error) { | |
| console.error( | |
| `[revisions] Failed to prune revisions for ${collection}/${resolvedId}:`, | |
| error, | |
| ); | |
| } | |
| } else { | |
| after(() => { | |
| revisionRepo | |
| .pruneQueuedEntry(collection, resolvedId, revision.id, 50) | |
| .catch((error) => { | |
| console.error( | |
| `[revisions] Failed to prune revisions for ${collection}/${resolvedId}:`, | |
| error, | |
| ); | |
| }); | |
| }); | |
| } |
| try { | ||
| await revisionRepo.pruneQueuedEntry(revision.collection, revision.entryId, newDraft.id, 50); | ||
| } catch (error) { | ||
| console.error( | ||
| `[revisions] Failed to prune revisions for ${revision.collection}/${revision.entryId}:`, | ||
| error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[needs fixing] Same issue as above: request-lifetime pruning is awaited inline during draft restore. This should be deferred with after() so it doesn't block the response.
| try { | |
| await revisionRepo.pruneQueuedEntry(revision.collection, revision.entryId, newDraft.id, 50); | |
| } catch (error) { | |
| console.error( | |
| `[revisions] Failed to prune revisions for ${revision.collection}/${revision.entryId}:`, | |
| error, | |
| ); | |
| } | |
| after(() => { | |
| revisionRepo | |
| .pruneQueuedEntry(revision.collection, revision.entryId, newDraft.id, 50) | |
| .catch((error) => { | |
| console.error( | |
| `[revisions] Failed to prune revisions for ${revision.collection}/${revision.entryId}:`, | |
| error, | |
| ); | |
| }); | |
| }); |
| const pruneRepo = new RevisionRepository(db); | ||
| void pruneRepo.pruneOldRevisions(revision.collection, revision.entryId, 50).catch(() => {}); | ||
| try { | ||
| await pruneRepo.pruneQueuedEntry(revision.collection, revision.entryId, queuedRevisionId, 50); | ||
| } catch (error) { | ||
| console.error( | ||
| `[revisions] Failed to prune revisions for ${revision.collection}/${revision.entryId}:`, | ||
| error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[needs fixing] The legacy revision-restore handler also awaits pruneQueuedEntry inline after committing the transaction. It should be deferred with after() instead, and you'll need to add import { after } from "../../after.js"; at the top of the file.
| const pruneRepo = new RevisionRepository(db); | |
| void pruneRepo.pruneOldRevisions(revision.collection, revision.entryId, 50).catch(() => {}); | |
| try { | |
| await pruneRepo.pruneQueuedEntry(revision.collection, revision.entryId, queuedRevisionId, 50); | |
| } catch (error) { | |
| console.error( | |
| `[revisions] Failed to prune revisions for ${revision.collection}/${revision.entryId}:`, | |
| error, | |
| ); | |
| } | |
| const pruneRepo = new RevisionRepository(db); | |
| after(() => { | |
| pruneRepo | |
| .pruneQueuedEntry(revision.collection, revision.entryId, queuedRevisionId, 50) | |
| .catch((error) => { | |
| console.error( | |
| `[revisions] Failed to prune revisions for ${revision.collection}/${revision.entryId}:`, | |
| error, | |
| ); | |
| }); | |
| }); |
There was a problem hiding this comment.
This is the right change for #2211: it replaces the cron full-table revision scan with a small bounded queue, coalesces per-entry pruning into request-lifetime after() work, and uses a deferred-task tracker to keep connection-backed adapters open until that work finishes. The prior review's lifecycle concern is addressed — runOutsideRequest now awaits closed in its finally block, so the returned promise does not resolve before the real adapter close runs.
I read the queue writes, the idempotent migration, the cleanup batch ordering, the lifecycle wiring, and the tests. The logic is sound and the added test coverage (bodyless/stream/cancellation/render-error paths, oldest-first cleanup, no full-table scan, migration retry, test-db teardown draining) is strong.
One AGENTS.md violation remains: a comment in runOutsideRequest justifies the await closed line by comparing it to the HTTP path. That kind of "unlike X" justification comment is exactly what AGENTS.md asks us to delete.
| // An event handler's returned promise owns its lifetime; unlike an HTTP | ||
| // response, there is no later stream completion to anchor adapter teardown. |
There was a problem hiding this comment.
[needs fixing] This comment justifies a single line by comparing it to another code path (unlike an HTTP response...), which is the kind of "unlike X" / decision-justification comment AGENTS.md says should be deleted. The line await closed; already says what happens; the comparison adds only reviewer-facing narrative.
| // An event handler's returned promise owns its lifetime; unlike an HTTP | |
| // response, there is no later stream completion to anchor adapter teardown. | |
| await closed; |
github-actions
Bot
added
review/approved
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review complete. The prior AGENTS.md finding has been addressed: the justificatory “unlike the HTTP path” comment that explained await closed in runOutsideRequest has been replaced with a concise description of why the separate teardown guards exist, and I found no remaining forbidden patterns (“intentionally”, “for safety”, “unlike”, numbered comments, or reviewer-directed notes) across the changed source.
I re-read the queue write paths, the idempotent migration, the bounded cleanup ordering, the lifecycle wiring in scoped-db.ts/after.ts, and the added tests. The approach remains sound: it replaces the cron full-table revision scan with a small _emdash_revision_prune_queue table, coalesces per-entry pruning into request-lifetime after() work, and correctly keeps connection-backed adapters open until both the response stream and request-owned deferred work settle. The migration is retry-safe (IF NOT EXISTS, ON CONFLICT DO UPDATE), the batch cleanup is oldest-first and bounded, and the test coverage now exercises bodyless responses, stream completion, cancellation, render failure, nested deferred tasks, and test-database teardown draining.
No new logic issues or AGENTS.md violations. LGTM.
github-actions
Bot
added
review/approved
bf8f571 to
a32d1c0
Compare
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is the right change for #2211. It replaces the scheduled full-table revision scan with a durable, bounded prune queue, coalesces per-entry pruning into request-lifetime after() work, and correctly coordinates connection-backed adapter teardown with both response completion and deferred work. The approach fits EmDash's architecture: the cron path becomes bounded and idempotent, revision-mutating handlers stay non-blocking, and the lifecycle helpers are generic enough to cover HTTP responses, event handlers, and test teardown.
I re-read the diff and the changed source files, traced the queue write/read paths, checked the migration for retry-safety and ordering, verified the lifecycle wiring in after.ts/deferred-tasks.ts/scoped-db.ts/middleware.ts, and reviewed the added tests. I also re-checked for AGENTS.md comment-discipline violations in the changed source; the prior finding has been addressed and I found no remaining forbidden patterns ("intentionally", "for safety", "unlike", numbered or reviewer-directed comments).
Everything looks solid: the migration is forward-only and idempotent (IF NOT EXISTS, ON CONFLICT DO UPDATE), pruneQueuedRevisions is oldest-first and bounded, error handling in cleanup is non-fatal and leaves failed queue rows for retry, and the deferred-task tracker correctly gates adapter close on both response settlement and task completion. No logged-out hot-path queries are added.
No blocking issues found. LGTM.
github-actions
Bot
added
review/approved
What does this PR do?
Replaces the scheduled full-table revision-history scan with a durable, bounded pruning queue. Revision writes coalesce pruning work per content entry, while scheduled cleanup processes at most ten oldest entries per invocation as a retry path. A migration queues existing histories over the 50-revision limit once.
Revision-mutating handlers continue to defer their bounded per-entry prune through
after(). Request-scoped deferred tasks now participate in the database lifecycle, so connection-backed adapters close only after both the response and all request-owned deferred work settle. HTTP responses remain non-blocking throughwaitUntil, while request-free event handlers await their deferred teardown before returning. This covers bodyless and streamed responses, cancellation, render errors, and nested deferred tasks without adding database queries to the logged-out hot path. Direct-runtime tests also drain deferred work before destroying their test database.This generalized lifecycle coordination replaces the prefetch-specific close deferral merged in #2409. Layout prefetch now uses the common
after()tracker, and the rebased lifecycle tests retain streamed-response teardown coverage.Closes #2211
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.Admin localization and a feature Discussion are not applicable: this changes background database maintenance and request-resource ownership without adding UI or a feature.
AI-generated code disclosure
Screenshots / test output
mainafter fix: prefer uncached Hyperdrive after content writes #2280, fix(core): refresh redirect cache across isolates #2408, and fix(core): keep Hyperdrive pool alive through layout prefetch #2409 merged/varvs/private/vartemp-path normalization and taxonomy cursor pagination omitting duplicate-label terms)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/issue-2211-cron-row-reads. Updated automatically when the playground redeploys.