Skip to content

fix(core): make media usage collection deletion crash-safe - #2433

Merged
khoinguyenpham04 merged 15 commits into
mainfrom
feature/media-usage-safe-collection-deletion
Aug 12, 2026
Merged

fix(core): make media usage collection deletion crash-safe#2433
khoinguyenpham04 merged 15 commits into
mainfrom
feature/media-usage-safe-collection-deletion

Conversation

@khoinguyenpham04

@khoinguyenpham04 khoinguyenpham04 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Makes collection deletion safe for the durable Media Usage indexing system introduced by #2422.

This stacked PR:

  • creates an exact collection-ID tombstone before detaching an activated collection;
  • fences content writes and physical table deletion across SQLite, PostgreSQL, D1, and Durable Object SQL;
  • removes registry identity before the content table and keeps the slug reserved until cleanup finalizes;
  • processes work, sources, occurrences, and status in leased, retryable batches of at most 50 rows;
  • adds bounded, authorized operator list/retry endpoints, client methods, OpenAPI documentation, and REST documentation;
  • preserves expanded-mode V1 deletion and never deletes media assets.

Incremental capture remains fail-closed and is not activated automatically. This PR is stacked directly on #2422; review its four commits relative to feature/media-usage-projection-admission.

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 (not applicable; no admin UI or user-facing UI strings)
  • I have added a changeset (if this PR changes a published package)
  • New features link to an approved Discussion: not applicable; this fixes deletion safety for the stacked Media Usage implementation in fix(core): bound durable media usage projection work #2422

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: OpenAI Codex (GPT-5) with adversarial reviewer subagents

Screenshots / test output

No visual changes.

Verified locally:

  • package typecheck, type-aware lint, formatting, workspace/core builds, and Changesets validation;
  • focused migration, lifecycle, concurrency, retry, authorization, OpenAPI/schema, scheduler, and cleanup suites;
  • full Cloudflare adapter suite: 310 tests;
  • real workerd/D1 guarded-deletion and maximum-batch cost tests;
  • D1 limits: at most 40 queries per phase tick, 70 rows written, 100 binds, 100 KB SQL, and 2.5 seconds wall time;
  • SQLite query plans use the declared due, lease, operator, source, and occurrence cursor indexes.

PostgreSQL concurrency coverage is included and typechecked; runtime execution requires EMDASH_TEST_PG, which was not available locally. The unrelated macOS /var versus /private/var path assertion remains reproducible outside this diff; affected Media Usage tests pass independently.


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 feature/media-usage-safe-collection-deletion. Updated automatically when the playground redeploys.

@changeset-bot

changeset-bot Bot commented Aug 12, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: e187d59

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

Copy link
Copy Markdown
Contributor

Scope check

This PR changes 3,824 lines across 44 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.

@khoinguyenpham04
khoinguyenpham04 marked this pull request as ready for review August 12, 2026 08:24
@khoinguyenpham04 khoinguyenpham04 added the bot:review Trigger an emdashbot code review on this PR label Aug 12, 2026
@github-actions github-actions Bot added the review/needs-review No maintainer or bot review yet label Aug 12, 2026
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 12, 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 90f01f8 Aug 12 2026, 11:30 AM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 12, 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 e187d59 Aug 12 2026, 04:13 PM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 12, 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 e187d59 Aug 12 2026, 04:33 PM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 12, 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 e187d59 Aug 12 2026, 04:12 PM

@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 large, careful change that makes Media Usage-aware collection deletion crash-safe with tombstones, leases, bounded batches, and operator recovery. The overall approach fits EmDash’s architecture and the diff is mostly well-tested across SQLite, Postgres, D1, and DO SQL.

I reviewed statically. The two issues that should be fixed before merge are:

  1. The D1 fence guard can return has_content on a forceDelete: true deletion if the fence update loses a race, which would make deleteCollection(..., { force: true }) spuriously fail.
  2. exactCleanupRowsRemain runs three queries concurrently inside a Kysely transaction, which is unsafe on Postgres (single-client transaction) and can deadlock or error.

The remaining findings are AGENTS.md / consistency nits: identifier validation, redundant slug checks, a JS-vs-DB timestamp mix, missing OpenAPI .meta({ id }) on the new Zod schemas, and a broad error-message regex in D1.

Comment thread packages/cloudflare/src/db/d1.ts
Comment thread packages/core/src/media/usage/collection-deletion-processor.ts
Comment thread packages/cloudflare/src/db/d1.ts
Comment thread packages/cloudflare/src/db/do-sql-class.ts
Comment thread packages/core/src/schema/registry.ts
Comment thread packages/core/src/media/usage/collection-deletion-processor.ts
Comment thread packages/core/src/api/schemas/media-usage.ts Outdated
Comment thread packages/cloudflare/src/db/d1.ts Outdated
@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 12, 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 12, 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 the right change for the stacked Media Usage work: it makes collection deletion durable and crash-safe by splitting the teardown into leased, retryable phases and adding operator recovery endpoints. The approach fits EmDash’s existing lease/cleanup patterns, and the Cloudflare adapters implement the fenced guard without bypassing the repo abstractions.

I reviewed the core repository, processor, adapter guards, routes, schemas, migration, registry integration, and tests statically (I did not run the test suite). The implementation is generally solid and well-tested for lifecycle, concurrency, and recovery, but I found one correctness bug in the processor that should be fixed before merge: checkpoint updates write updated_at from JavaScript instead of from the dialect-aware timestamp helper, which breaks lexical ordering assumptions used by the operator page cursor and the due queue on SQLite/D1. I also flagged a smaller maintainability issue inside the transaction helpers.

Comment thread packages/core/src/media/usage/collection-deletion-processor.ts Outdated
Comment thread packages/core/src/media/usage/collection-deletion-processor.ts Outdated
Base automatically changed from feature/media-usage-projection-admission to main August 12, 2026 09:39
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review needs-rebase and removed review/awaiting-author Reviewed; waiting on the author to respond labels Aug 12, 2026
@khoinguyenpham04 khoinguyenpham04 added the bot:review Trigger an emdashbot code review on this PR label Aug 12, 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 still the right change for the stacked Media Usage work. It makes collection deletion durable and crash-safe by tombstoning the collection ID, running fenced front phases, and cleaning up usage rows, sources, work, and status in bounded, leased batches. The approach fits EmDash’s existing lease/cleanup patterns.

The previously identified correctness bug is fixed: collection-deletion-processor.ts now updates updated_at through collectionDeletionCurrentTimestamp(db) (which is dialect-aware), and the repository methods consistently use timestampOffset(...). A regression test was added that verifies progress handoffs use database time rather than a faked Date, so the operator cursor and due-queue ordering assumptions on SQLite/D1 hold.

I reviewed the core repository and processor, the D1 and Durable Object SQL adapter guards, the Astro routes and authorization, the OpenAPI/client additions, the migration, the registry integration, and the updated tests. The implementation is solid and well-tested for lifecycle, concurrency, and recovery. I found no logic bugs or security issues. Two minor suggestions:

  1. The D1 and DO collection-deletion guards use the raw binding/primary stub directly from config, bypassing the request-scoped session and its bookmark. For admin requests running with D1/DO read-replica sessions, this creates a window where a fast-following authenticated read may be routed to a replica that has not yet observed the dropped table or deleted collection row. If that causes stale admin UI state, consider routing the guard update through the request-scoped session when one exists, or at least documenting the consistency trade-off.
  2. In MediaUsageCollectionDeletionRepository.deleteRegistryAndCheckpoint, raw timestamp/lease fragments are built from this.db even though they run on the trx argument. Build them from trx instead so the connection and transaction context are unambiguous.

The prior bug is resolved, and the remaining notes are not blockers.

Comment thread packages/cloudflare/src/db/d1.ts
Comment thread packages/core/src/media/usage/collection-deletion.ts Outdated
@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 12, 2026
@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 12, 2026
@khoinguyenpham04 khoinguyenpham04 added the bot:review Trigger an emdashbot code review on this PR label Aug 12, 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 still the right change for the stacked Media Usage work. The approach — tombstone the exact collection ID, fence and detach registry/content in leased front phases, then clean up work/sources/status in bounded batches — fits EmDash’s existing lease/cleanup machinery and preserves the V1 deletion path. I checked the new core repository/processor, the D1 and Durable Object SQL guard implementations, the Astro routes and authorization, the OpenAPI/client additions, the registry slug-locking integration, the migration, and the expanded test suites.

The previously identified correctness bug is fixed: collection-deletion-processor.ts now advances updated_at through collectionDeletionCurrentTimestamp(db), and repository methods consistently use timestampOffset(...). The prior note about building deleteRegistryAndCheckpoint fragments from the trx argument is also addressed.

I found no logic bugs, security issues, or missing authorization checks. The one remaining prior observation — that the D1/DO guards use the raw binding/primary stub directly instead of routing through a request-scoped session — is unchanged, but as noted before it is a consistency trade-off, not a blocker.

One AGENTS.md testing convention issue: the descriptor tests pin a newly-added supportsCollectionDeletionGuard flag back to the literal value set in the implementation. Config-pin tests cannot fail on a real user-visible regression; they should be deleted or replaced with behavior coverage of the guard itself.

Comment thread packages/cloudflare/tests/do-config.test.ts Outdated
Comment thread packages/cloudflare/tests/do-config.test.ts Outdated
@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 12, 2026
@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-rereview Author pushed changes since the last review labels Aug 12, 2026
@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 12, 2026
@khoinguyenpham04 khoinguyenpham04 added the bot:review Trigger an emdashbot code review on this PR label Aug 12, 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 still the right change for the stacked Media Usage work. The tombstone/lease/batch cleanup approach fits EmDash’s existing lease machinery and preserves the V1 deletion path, and the new admin routes are correctly scoped and authorized.

I reviewed the diff statically (no tests/builds run). This re-review focused on what changed since the previous review:

  • Fixed from prior review: the updated_at / collectionDeletionCurrentTimestamp issue in the processor is resolved, and deleteRegistryAndCheckpoint is built from the trx argument.
  • Still open from prior review: the generateDialectModule test still pins the literal supportsCollectionDeletionGuard flag to the export string, an AGENTS.md testing-convention issue.
  • New concern: the “fence content writes while deleting” guarantee is only enforced on PostgreSQL. MediaUsageRepository.lockCanonicalSourceCollection returns true immediately on SQLite/D1/DO SQL, so the FOR KEY SHARE row lock that blocks concurrent replaceSource/markSourceAttempted writes during collection deletion does nothing on those backends. This leaves a window where new _emdash_media_usage_sources/_emdash_media_usage rows can be inserted while a collection is being dropped, which contradicts the PR description’s claim of a cross-platform fence and could leave orphaned index rows.

I found no logic bugs, authorization errors, or unsafe interpolated SQL. The two findings below are the only blockers to a clean approval.


Findings

  • [needs fixing] packages/core/src/database/repositories/media-usage.ts:2192-2201

    The content-write fence is only real on PostgreSQL. On SQLite, D1, and Durable Object SQL the helper returns true without acquiring any lock, so replaceSource, replaceSourceIfCurrent, and markSourceAttemptedIfMatching proceed unprotected while a collection deletion is in flight.

    This contradicts the PR description’s claim that content writes are fenced across SQLite, PostgreSQL, D1, and DO SQL. It also leaves an opening for orphaned source/occurrence rows if a media-usage projection transaction commits after the registry row is deleted but before the cleanup phases finish.

    At minimum, the non-Postgres path should not silently succeed. A cheap first-line defense is to remove the isPostgres short-circuit and verify that the referenced collection still exists (returning false once it has disappeared). That does not serialize transactions the way FOR KEY SHARE does, but it stops post-deletion source writes on every backend. If true serialization on SQLite/D1/DO is intentionally out of scope, the scope should be documented in code and the PR description/tests should reflect the limitation.

    		if (source.collectionId === undefined || source.collectionId === null) return true;
    		if (!source.collectionSlug) return false;
    		const collection = await db
    			.selectFrom("_emdash_collections")
    			.select("id")
    			.where("id", "=", source.collectionId)
    			.where("slug", "=", source.collectionSlug)
    			.$if(isPostgres(this.db), (qb) => qb.forKeyShare())
    			.executeTakeFirst();
    		return collection !== undefined;
    
  • [suggestion] packages/core/tests/unit/astro/integration/virtual-modules.test.ts:69-80

    This test still pins the new supportsCollectionDeletionGuard flag to the literal export string it produces. That was flagged in the previous review and remains unaddressed.

    A config-pin test cannot fail on a real user-visible regression — it only fails when someone intentionally changes either the flag or the generated string. Per AGENTS.md, delete it or replace it with behavior coverage (e.g., a test that loads the generated virtual dialect module for a guard-enabled adapter and confirms executeCollectionDeletionGuard is callable/exists).

@emdashbot emdashbot Bot removed the bot:review Trigger an emdashbot code review on this PR label Aug 12, 2026
@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-rereview Author pushed changes since the last review labels Aug 12, 2026
@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 12, 2026
@github-actions github-actions Bot added review/approved Approved; no new commits since and removed review/needs-rereview Author pushed changes since the last review labels Aug 12, 2026
@khoinguyenpham04
khoinguyenpham04 merged commit 0cd7c73 into main Aug 12, 2026
71 of 72 checks passed
@khoinguyenpham04
khoinguyenpham04 deleted the feature/media-usage-safe-collection-deletion branch August 12, 2026 16:58
@emdashbot emdashbot Bot mentioned this pull request Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants