fix(cloudflare): remove D1 ConnectionMutex on the raw-binding singleton (#2040)#2125
Open
swissky wants to merge 1 commit into
Open
fix(cloudflare): remove D1 ConnectionMutex on the raw-binding singleton (#2040)#2125swissky wants to merge 1 commit into
swissky wants to merge 1 commit into
Conversation
With the default D1 config (sessions disabled) all runtime queries go through one module-scope Kysely singleton whose dialect reports supportsMultipleConnections: false, so Kysely serializes every query behind a ConnectionMutex. On Workers a request canceled mid-query never settles its promise, so releaseLock() never runs and every later obtainLock() waits forever — one canceled request deadlocks the isolate (emdash-cms#2040). The mutex protects nothing on the raw binding: kysely-d1 rejects transactions and concurrent prepare().all() calls are independent subrequests. Add RawBindingD1Dialect, identical to EmDashD1Dialect except it reports supportsMultipleConnections: true, and use it for the raw-binding singleton in createDialect. The session-backed dialects keep their serialization (the non-coalesce path relies on the mutex to keep a D1DatabaseSession's bookmark single-in-flight). Fixes emdash-cms#2040
🦋 Changeset detectedLatest commit: 384fb66 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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
review/needs-review
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This is the right fix for #2040, scoped correctly and implemented cleanly.
Approach judgment: The deadlock is real: Kysely's RuntimeDriver serializes queries when the adapter reports supportsMultipleConnections: false. On Workers, a cancelled request can strand the ConnectionMutex, deadlocking the whole isolate. The raw D1 binding has no transaction support, no meaningful connection lifecycle, and no session bookmark to protect, so the mutex there provides no benefit while creating a catastrophic failure mode. Keeping the mutex on the session-backed path (which advances a D1 bookmark per query and must not interleave physical calls) is the correct boundary. Adding a separate RawBindingD1Dialect rather than mutating EmDashD1Dialect preserves that invariant and mirrors the override already used by CoalescingD1Adapter.
What I checked:
- All changed hunks in
d1-dialect.ts,d1.ts, the new test file, and the changeset. - Cross-referenced against
coalescing-d1.tsto confirm thesupportsMultipleConnections: trueoverride pattern is already established and safe there. - Verified
createRequestScopedDbstill usesEmDashD1Dialectfor the plain session path andCoalescingD1Dialectfor the coalescing session path, so the mutex is preserved where load-bearing. - Checked AGENTS.md conventions: no new user-facing strings, no SQL interpolation, no content-table queries, no migrations, no API routes, no authorization changes, no new dependencies, changeset present and user-facing.
Headline conclusion: The change is minimal, consistent with existing patterns, and well-defended by the new regression tests (raw binding overlaps, session dialect still serializes). I found no blockers or correctness issues.
github-actions
Bot
added
review/approved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
With the default D1 config (
d1({ binding: "DB" }), sessions disabled) all runtime queries go through one module-scope Kysely singleton. Its dialect (EmDashD1Dialect→ kysely-d1) uses Kysely'sSqliteAdapter, which reportssupportsMultipleConnections: false, so Kysely'sRuntimeDriverserializes every query behind aConnectionMutex(acquire → execute → release). On Workers, when a request is canceled while its query is in flight, the pending I/O promise never settles, soreleaseLock()never runs — and every laterobtainLock()waits forever. One canceled request (a visitor clicking away on a slow cold page is enough) permanently deadlocks the whole isolate.The mutex protects nothing on the raw binding: kysely-d1's
D1Connectionrejects transactions outright,releaseConnectionis a no-op, and concurrentprepare().all()calls are independent subrequests. This PR addsRawBindingD1Dialect— identical toEmDashD1Dialectexcept its adapter reportssupportsMultipleConnections: true— and uses it for the raw-binding singleton increateDialect. This is the same override the existingCoalescingD1Adapteralready applies, scoped to the path where it's safe.The fix is deliberately scoped to the raw-binding path. The session-backed non-coalesce path in
createRequestScopedDbalso builds anEmDashD1Dialect, and there the mutex is load-bearing: aD1DatabaseSessionadvances its bookmark per executed query, and concurrent physical calls on one session could interleave the bookmark and persist a stale one atcommit(), breaking read-your-writes. That path keeps the stock adapter (mutex) — verified by a dedicated test.Closes #2040
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.AI-generated code disclosure
Screenshots / test output
New regression tests prove (via an in-flight overlap counter) that concurrent queries on the raw-binding dialect overlap (
maxInFlight === 2) instead of serializing, while the session-backedEmDashD1Dialectstill reportssupportsMultipleConnections: falseand still serializes (maxInFlight === 1), preserving the session bookmark invariant.