fix(core): make media usage collection deletion crash-safe - #2433
Conversation
🦋 Changeset detectedLatest commit: e187d59 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 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. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 90f01f8 | Aug 12 2026, 11:30 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | e187d59 | Aug 12 2026, 04:13 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | e187d59 | Aug 12 2026, 04:33 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | e187d59 | Aug 12 2026, 04:12 PM |
There was a problem hiding this comment.
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:
- The D1 fence guard can return
has_contenton aforceDelete: truedeletion if the fence update loses a race, which would makedeleteCollection(..., { force: true })spuriously fail. exactCleanupRowsRemainruns 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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. - In
MediaUsageCollectionDeletionRepository.deleteRegistryAndCheckpoint, raw timestamp/lease fragments are built fromthis.dbeven though they run on thetrxargument. Build them fromtrxinstead so the connection and transaction context are unambiguous.
The prior bug is resolved, and the remaining notes are not blockers.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/collectionDeletionCurrentTimestampissue in the processor is resolved, anddeleteRegistryAndCheckpointis built from thetrxargument. - Still open from prior review: the
generateDialectModuletest still pins the literalsupportsCollectionDeletionGuardflag 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.lockCanonicalSourceCollectionreturnstrueimmediately on SQLite/D1/DO SQL, so theFOR KEY SHARErow lock that blocks concurrentreplaceSource/markSourceAttemptedwrites during collection deletion does nothing on those backends. This leaves a window where new_emdash_media_usage_sources/_emdash_media_usagerows 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-2201The content-write fence is only real on PostgreSQL. On SQLite, D1, and Durable Object SQL the helper returns
truewithout acquiring any lock, soreplaceSource,replaceSourceIfCurrent, andmarkSourceAttemptedIfMatchingproceed 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
isPostgresshort-circuit and verify that the referenced collection still exists (returningfalseonce it has disappeared). That does not serialize transactions the wayFOR KEY SHAREdoes, 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-80This test still pins the new
supportsCollectionDeletionGuardflag 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
executeCollectionDeletionGuardis callable/exists).
What does this PR do?
Makes collection deletion safe for the durable Media Usage indexing system introduced by #2422.
This stacked PR:
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
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
No visual changes.
Verified locally:
PostgreSQL concurrency coverage is included and typechecked; runtime execution requires
EMDASH_TEST_PG, which was not available locally. The unrelated macOS/varversus/private/varpath 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.