fix(cloudflare): reclaim stranded coalescing flush after request cancel (#1927)#2135
Conversation
…el (emdash-cms#1927) On workerd a request cancelled after queueing a coalesced query drops its pending setTimeout(0), so the flush it scheduled never fires and #flushScheduled stays true — every later query on the connection awaits a flush that never runs (the same hazard class single-flight-cache.ts already guards). Make the flush flag reclaimable: each schedule stamps a deadline, and if it lapses with the flag still set the next caller clears the stale flag and reschedules rather than queueing behind a dead timer. Applied to both the D1 and Durable Object SQL coalescing connections.
🦋 Changeset detectedLatest commit: 6538d33 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
area/cloudflare
size/M
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}
There was a problem hiding this comment.
This is the right fix applied in the right place. The bug it addresses—a cancelled workerd request dropping its pending setTimeout(0) and leaving #flushScheduled stuck true—is the same cancellation hazard already documented and mitigated by single-flight-cache.ts / init-lock.ts. A 1 s reclaim deadline is cheap, generous defense-in-depth for a setTimeout(0) flush, and the logic is safe: reclaiming clears only a stale flag, queued queries stay in the buffer, and a late-arriving original timer just drains an empty buffer.
What I checked:
- Read the diff and the full
coalescing-d1.ts,coalescing-do-sql.ts,d1.ts,do-sql.ts, and both test files. - Traced the reclaim flow against race conditions (reclaim while a live flush is in flight, double-reclaim, queries arriving between
flushScheduled = falseandsplice). No correctness issues. - Verified the changeset is scoped to
@emdash-cms/cloudflareas a patch. - Confirmed AGENTS.md conventions are not violated (no UI/i18n/SQL/API/authorization/locale-filter/index concerns in this change).
- Confirmed the PR includes regression tests for D1; DO SQL uses the same pattern but was not covered.
Headline conclusion: the fix is sound and well-tested for D1. Two minor follow-ups are worth doing before merge: keep the D1/DO SQL implementations structurally consistent, and add the same DO SQL regression tests so both fixed paths are verified.
Findings
-
[suggestion]
packages/cloudflare/src/db/coalescing-do-sql.ts:145-147The DO SQL connection inlines the stale-flush reclaim check that
CoalescingD1Connectionextracts into#reclaimStrandedFlush. Since the two dialects intentionally mirror each other, using the same structure reduces the chance a future edit updates one but not the other.if (this.#flushScheduled) { this.#reclaimStrandedFlush(); if (this.#flushScheduled) return; } -
[suggestion]
packages/cloudflare/tests/db/coalescing-do-sql.test.ts:155The cancellation-safe flush fix is also applied to
CoalescingDOSqlConnection, but the new regression tests only coverCoalescingD1Connection. This file already has a convenientsetup()helper, so adding the same two stranded/live-flush cases here would verify both paths mentioned in the bug report (#1927).describe("cancellation-safe flush (#1927)", () => { afterEach(() => { vi.useRealTimers(); }); it("reclaims a stranded flush so a later query is not left hanging", async () => { vi.useFakeTimers(); const { batchQuery, dialect } = setup({ batchQuery: vi.fn().mockResolvedValue([{ rows: [{ id: 1 }] }] as DOQueryResult[]), }); const conn = await dialect.createDriver().acquireConnection(); const first = conn.executeQuery(CompiledQuery.raw("select 1")); vi.clearAllTimers(); await vi.advanceTimersByTimeAsync(2_000); const second = conn.executeQuery(CompiledQuery.raw("select 1")); await vi.runAllTimersAsync(); await expect(first).resolves.toMatchObject({ rows: [{ id: 1 }] }); await expect(second).resolves.toMatchObject({ rows: [{ id: 1 }] }); }); it("does not reclaim a live flush within its deadline", async () => { vi.useFakeTimers(); const { batchQuery, dialect } = setup(); const conn = await dialect.createDriver().acquireConnection(); const p1 = conn.executeQuery(CompiledQuery.raw("select 1")); const p2 = conn.executeQuery(CompiledQuery.raw("select 2")); await vi.runAllTimersAsync(); await Promise.all([p1, p2]); expect(batchQuery).toHaveBeenCalledTimes(1); expect(batchQuery.mock.calls[0]![0]).toEqual([ { sql: "select 1", params: [] }, { sql: "select 2", params: [] }, ]); }); });
Addressing emdashbot review on emdash-cms#2135: extract the stranded-flush reclaim into a #reclaimStrandedFlush method on the DO SQL connection so it mirrors CoalescingD1Connection structurally (future edits can't drift one but not the other), and add the same two cancellation-safe flush regression tests for CoalescingDOSqlConnection so both fixed paths are verified (emdash-cms#1927).
github-actions
Bot
added
size/L
review/needs-rereview
|
@emdashbot both suggestions addressed in 6538d33: Structural parity (coalescing-do-sql.ts:145-147): the DO SQL connection now extracts the stale-flush check into a DO SQL regression coverage (coalescing-do-sql.test.ts): added the same Full |
There was a problem hiding this comment.
This is a re-review of the Cloudflare coalescing-flush cancellation fix. Both of the prior review's suggestions have been addressed, and the change remains the right fix in the right place.
What changed since the last review:
packages/cloudflare/src/db/coalescing-do-sql.tsnow extracts the stale-flush reclaim check into its own#reclaimStrandedFlush()method, matching the structure incoalescing-d1.ts. The two dialect implementations are now structurally consistent.packages/cloudflare/tests/db/coalescing-do-sql.test.tsnow has the same two regression tests — one that reclaims a stranded flush so a later query is not left hanging, and one that confirms a live flush within its deadline is not prematurely reclaimed.
What I checked:
- Re-read the diff and the full
coalescing-d1.ts,coalescing-do-sql.ts, and both test files. - Re-traced the reclaim race: a genuinely dropped timer leaves
#flushScheduledstuck until the next caller passes the deadline, clears the flag, and reschedules. A late-arriving original timer clears the flag and drains an empty buffer; this can create a harmless extra empty flush but does not lose queries or break ordering. - Verified the approach is consistent with the existing
single-flight-cache.ts/init-lock.tsreclaimable-lock pattern inpackages/core. - Verified the changeset is present, scoped to
@emdash-cms/cloudflareas a patch, and written as user-facing release notes. - Confirmed no AGENTS.md conventions are violated: no UI/i18n strings, no SQL interpolation, no API/auth/locale/index/migration changes.
Headline conclusion: the fix is sound, both code paths are now consistent, and both are covered by regression tests. LGTM.
github-actions
Bot
added
review/approved
2 participants
What does this PR do?
Fixes a hang where a request cancelled mid-query on Cloudflare D1 could wedge the query-coalescing buffer. On workerd, a request that is cancelled after queueing a coalesced query drops its pending
setTimeout(0), so the flush it scheduled never fires and#flushScheduledstaystrue— every later query on the connection then awaits a flush that never runs. This is the same hazard class thatsingle-flight-cache.tsalready documents and defends against; the two coalescing connections had escaped that hardening.The flush flag is now reclaimable: each schedule stamps a short deadline (1s — a live
setTimeout(0)fires within a turn, so this only ever trips on a genuinely dropped timer), and if that deadline lapses with the flag still set, the next caller clears the stale flag and reschedules instead of queueing behind a dead timer. Applied to both the coalescing D1 connection and the Durable Object SQL connection (the latter is long-lived and its buffer can be shared across requests, so it is the more exposed of the two).Context: this came up alongside #2040 (the default-config
ConnectionMutexdeadlock, fixed in #2125). As analysed in #1927, the coalescing connections are not the default path and are mostly short-lived/per-request, but the hazard is real and the fix is cheap defence-in-depth.Closes #1927
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 in
packages/cloudflare/tests/db/coalescing-d1.test.ts(cancellation-safe flush (#1927)): one reclaims a stranded flush so a later query is not left hanging, one confirms a live flush within its deadline is not prematurely reclaimed. Fulltests/dbsuite passes (165 tests).