perf(cache): reduce response store Durable Object load - #3213
perf(cache): reduce response store Durable Object load#3213james-elicx wants to merge 17 commits into
Conversation
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@vinext/workers-response-store
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
fd3a4a1 to
77dc071
Compare
77dc071 to
8cbf30a
Compare
|
/bigbonk review for issues Please prioritize correctness issues in the latest Response Store DO changes and return findings or an explicit no-findings verdict before the 10-minute review limit expires. |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Hard deadline: 8 minutes. Stop investigation early enough to post either prioritized actionable findings or an explicit no-findings verdict before 8 minutes elapse. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Hard deadline: 8 minutes. Stop investigation early enough to post either prioritized actionable findings or an explicit no-findings verdict before 8 minutes elapse. |
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Re-review exact head c09351b after addressing your schema migration finding. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| reservation ??= await this.reserveWrite(metadata, keyHash, cacheKey, cacheTags); | ||
| try { | ||
| const result = await pending; | ||
| if (result.backingStoreUpdated) { |
There was a problem hiding this comment.
[P1] Honor the follower's purgeExisting request. This returns the leader's result unchanged, so if the leader used purgeExisting: false and this coalesced follower uses true, no edge purge runs even though the follower reports edgePurgeAccepted: true. The backing write can therefore succeed while an older edge response remains served. Before returning, apply this invocation's purge requirement (or only coalesce writes with compatible purge options), and cover the mixed-options case.
There was a problem hiding this comment.
Fixed in d8c6587. Coalescing is now partitioned by purgeExisting semantics, so a purging follower cannot inherit a non-purging leader result. The mixed-options regression passes, and the package suite is 39/39.
|
/bigbonk review for issues Re-review exact head 3958d51 after addressing the follower cleanup failure finding. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| await metadata | ||
| .finishPendingObjects([objectKey]) | ||
| .catch((error) => this.logCleanupFailure(objectKey, error)); | ||
| await response.body?.cancel().catch(() => {}); |
There was a problem hiding this comment.
[P1] Do not await cancellation of a coalesced follower body. The adapter passes cacheBody, one branch of rendered.body.tee(), into put() and does not return the sibling foreground branch until put() resolves. A tee branch’s cancel() promise waits for the sibling/source to finish, so an overlapping warmup that follows another write can deadlock here: put() waits for cancellation while the caller cannot consume the foreground branch. Start cancellation without awaiting it (while handling rejection), and cover coalescing with a tee-backed response whose sibling is consumed only after put() returns.
There was a problem hiding this comment.
Fixed in e528112. Coalesced follower body cancellation is now fire-and-forget with rejection handled, and the tee-backed regression consumes the sibling only after put() returns.
| await response.body?.cancel().catch(() => {}); | ||
| return result; | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
[P2] Distinguish edge-purge rejection from backing-write failure. pending includes the purge at line 669, so it can reject after the leader has already published successfully. This catch then falls through to storeResponse() and writes the follower as another revision, reproducing a duplicate write on a different post-commit error path. Separate the backing publication result from edge purging, or propagate the leader’s purge failure after disposing of the follower body without rewriting; add a coalesced purgeExisting test where ctx.cache.purge() rejects.
There was a problem hiding this comment.
Fixed in e528112. pendingPuts now shares only the backing publication promise; edge purge runs after that boundary, so a purge rejection propagates without entering the fallback write path.
Posted 2 additional actionable inline findings. |
|
/bigbonk review for issues Re-review exact head e528112 after addressing all three latest findings: tee cancellation, post-publication purge failures, and stale claim publication. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| return { | ||
| backingStoreUpdated: true, | ||
| edgePurgeAccepted: options.purgeExisting | ||
| ? await this.purgeEdgeCacheByTags([purgeTagForEntry(result.entry)]) |
There was a problem hiding this comment.
[P2] Keep this purge rejection out of the fallback-write catch. The try still encloses the follower purgeEdgeCacheByTags() call, so if that purge rejects after pending has confirmed publication, line 651 catches it as a leader failure and falls through to storeResponse(). At that point cancellation of this response body has also already started, so the fallback can either publish another revision or fail while reading a cancelled stream. Dispose of the follower body and perform/return the purge outside the catch that handles pending rejection; add the requested coalesced purging-follower test with ctx.cache.purge() rejecting.
There was a problem hiding this comment.
Fixed in 9a5fbea. The catch now encloses only await pending; all follower cleanup, cancellation, and edge-purge work runs after it, so purge rejection cannot enter the fallback-write path. I also replaced a fixed sleep in the purge/write race regression with an observed reservation barrier. Package tests pass 41/41.
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Re-review exact head 9a5fbea after moving follower purge fully outside the fallback-write catch. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| publication = await metadata.publish(keyHash, revision, candidate, write.claimId); | ||
| } catch (error) { | ||
| await this.deletePendingObjects(metadata, [objectKey]); | ||
| await this.releaseFailedWrite(metadata, write); |
There was a problem hiding this comment.
[P2] Do not classify post-publication alarm failures as write failures. metadata.publish() commits its SQL transaction before awaiting ensureCleanupAlarm() (metadata-do.ts:670). After a DO reactivation, that getAlarm()/setAlarm() call can reject even though this revision is already active; this catch then releases/reports the committed write as failed, and a coalesced follower can fall back to publishing another revision. Keep post-commit alarm maintenance non-throwing (the reservation already established recovery), or otherwise reconcile the publication before propagating the error, and cover a post-commit alarm-storage rejection.
There was a problem hiding this comment.
Fixed in d8f4afa. Post-commit cleanup-alarm maintenance is now caught and logged before immediate R2 cleanup continues, so an alarm API failure cannot make a committed publication appear failed or trigger a follower rewrite. Package tests pass 41/41.
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Re-review exact head d8f4afa after isolating post-commit cleanup-alarm failures. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| ); | ||
| }); | ||
| if (matches.length) { | ||
| await this.ensureCleanupAlarm(invalidatedAt); |
There was a problem hiding this comment.
[P1] Keep post-commit alarm failures from aborting selective purge. The transaction above has already tombstoned these entries and queued their old objects, but if ensureCleanupAlarm() rejects here, this method skips deleteTrackedObjects() and throws back to binding.purge() before the edge purge runs. Retrying cannot recover that edge invalidation because findMatchingEntryRows(..., true) no longer selects the tombstone (active_revision === latest_revision), so the old Workers Cache response can continue to be served. Catch/log this alarm error as publish() now does and continue the immediate R2/edge cleanup; please cover an alarm-storage rejection after the purge transaction commits.
There was a problem hiding this comment.
Fixed in ff28872. Both publication and purge now use the same non-throwing post-commit cleanup-alarm maintenance helper, so selective purge continues through immediate R2 deletion and edge invalidation after an alarm API failure. Package tests pass 41/41.
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Re-review exact head ff28872 after making post-commit alarm maintenance non-throwing for selective purge. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| revision > current.latest_revision || | ||
| (current.active_revision !== null && revision <= current.active_revision) || | ||
| (claimId !== undefined && | ||
| (current.claim_id !== claimId || current.claim_revision !== revision)) || |
There was a problem hiding this comment.
[P1] Reject claims for a superseded active revision. This checks the claim ID and reserved revision, but not revalidation_claims.active_revision. A normal write can reserve revision 2, a stale read can then claim revision 3 against active revision 1, and revision 2 can publish before the revalidation completes. The revision-3 claim still passes these checks and overwrites the newer response even though it regenerated revision 1. Select the claim active revision and require it to equal the entry current active revision; add this interleaving as a regression test.
There was a problem hiding this comment.
Fixed in 58975dd. Publication now requires the claim source active revision to equal the entry current active revision; a regression covers a normal write publishing between claim and regeneration.
| .map(({ object_key }) => object_key); | ||
| this.finishPendingObjects(activeObjectKeys); | ||
| if (expiredObjectKeys.length) { | ||
| await this.env.CACHE_BODIES.delete(expiredObjectKeys); |
There was a problem hiding this comment.
[P1] Invalidate an expired reservation before awaiting its R2 deletion. The expired marker remains publishable while this await yields. If a long-running write reaches R2 around the retention deadline, the sweep can select its marker, then the write can publish that same object while deletion is in flight; the sweep subsequently deletes the now-active body and removes its marker, leaving metadata that serves a permanently missing object. Atomically claim/remove the expired reservation before the R2 await so publish() rejects it (and restore/retain cleanup state if deletion fails), with a race test covering publication during the delete.
There was a problem hiding this comment.
Fixed in 58975dd. Expired reservations are removed before the R2 await so publication rejects during deletion; a failed deletion atomically restores the original markers and invalidation sequences for retry.
| ) | ||
| .toArray(); | ||
| private async maintainCleanupAlarm(createdAt: number): Promise<void> { | ||
| await this.ensureCleanupAlarm(createdAt).catch((error) => { |
There was a problem hiding this comment.
[P2] Preserve a retry when alarm maintenance and immediate deletion both fail. Swallowing this error lets purge/publication continue, but deleteTrackedObjects() also catches R2 deletion failures. If this was a setAlarm() failure with no existing alarm and R2 deletion then fails, the pending marker is left with nothing scheduled to retry it; with no later write, the old object leaks indefinitely. Record the alarm failure and ensure the R2 failure path retries alarm scheduling (or otherwise retains a guaranteed cleanup trigger).
There was a problem hiding this comment.
Fixed in 58975dd. Any immediate R2 cleanup failure now schedules an alarm retry; if scheduling itself fails, the pending marker remains durable and the failure is logged for a later activation/write to recover.
|
Posted 3 actionable inline findings. |
|
/bigbonk review for issues Re-review exact head 58975dd after addressing claim source revision, cleanup deletion race, and cleanup retry scheduling. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| .map(({ object_key }) => object_key); | ||
| const expired = batch.filter(({ active, created_at }) => !active && created_at <= cutoff); | ||
| const expiredObjectKeys = expired.map(({ object_key }) => object_key); | ||
| this.finishPendingObjects([...activeObjectKeys, ...expiredObjectKeys]); |
There was a problem hiding this comment.
[P1] Keep a durable cleanup record while the R2 deletion is in flight. This commits removal of the only pending_objects row before the external await. If the DO is restarted or the event is terminated while CACHE_BODIES.delete() is pending, the catch never runs to restore the row, and the next alarm has no record from which to retry, so the object leaks permanently. The same gap leaves no marker if a fenced writer uploads just after the delete and its subsequent best-effort cleanup fails. Fence publication with durable cleanup state, but retain a discoverable retry record until R2 deletion has completed; cover interruption between this transaction and completion of the delete.
There was a problem hiding this comment.
Fixed in b9b4ca6. pending_objects now has durable publishable vs cleanup state. Expired reservations are atomically made non-publishable but retained through the R2 await; success removes them, interruption or failure leaves a discoverable retry record. Failed best-effort cleanup also upserts that durable record.
| }), | ||
| ); | ||
| try { | ||
| await this.ctx.storage.setAlarm(Date.now() + ORPHAN_CLEANUP_RETRY_MS); |
There was a problem hiding this comment.
[P2] Make the retry alarm actually select these failed deletions. Replacement/purge cleanup markers retain their current created_at, so this alarm fires in one minute but sweepExpiredPendingObjects() uses the one-hour retention cutoff and skips them; it then schedules the next alarm for created_at + ORPHAN_RETENTION_MS. A transient immediate R2 failure therefore is not retried after ORPHAN_CLEANUP_RETRY_MS and leaves the old body for roughly an hour. Mark failed cleanup rows as immediately retryable (or distinguish cleanup retries from live reservations) before scheduling this alarm, and test the alarm one retry interval after a failed immediate deletion.
There was a problem hiding this comment.
Fixed in b9b4ca6. Failed immediate deletions now upsert non-publishable cleanup rows with created_at=0 before scheduling the one-minute retry, so the next sweep selects them immediately instead of waiting for retention expiry.
|
Posted 2 actionable inline findings. |
|
/bigbonk review for issues Re-review exact head b9b4ca6 after adding durable non-publishable cleanup state and immediate retry eligibility. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
| } | ||
| if (expiredObjectKeys.length) { | ||
| await this.env.CACHE_BODIES.delete(expiredObjectKeys); | ||
| this.finishPendingObjects(expiredObjectKeys); |
There was a problem hiding this comment.
[P1] Retain the cleanup tombstone past this first delete. A writer can already be inside CACHE_BODIES.put() when the sweep flips its reservation to non-publishable. If this delete finishes first, this line removes the only durable record; the older put can then complete and recreate the object. Its later publish() is correctly rejected, but that path does not establish a cleanup row before awaiting its best-effort delete, so termination before/during that delete (or termination before publish() altogether) leaks the recreated object permanently. Keep a discoverable non-publishable marker through a follow-up cleanup phase that is safely after the fenced writer, rather than deleting it immediately after the first R2 delete, and cover a put that completes between this delete and marker removal/retry.
There was a problem hiding this comment.
Fixed in 70dae5c. A newly expired live reservation is now fenced and retained for a full retention window after the first delete. Only the follow-up cleanup pass removes that tombstone; a regression recreates the object after the first delete and verifies publication is rejected and the recreated body is removed.
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Re-review exact head 70dae5c after retaining cleanup tombstones through a delayed second pass. Hard deadline: 8 minutes. Stop early enough to post actionable findings or an explicit no-findings verdict. |
2 participants
Summary
Durable Object load
Counts compare this PR with its parent. They count metadata Durable Object RPCs, not the service-binding or Workers Cache request. R2 operations are unchanged unless stated.
put()put()put()followerThe soft-tag lookup is deliberately unchanged from the authoritative design: misses, entries without soft tags, and repeated uses of the same tag set in one request perform zero expiration RPCs. A candidate hit with a new tag set performs one RPC; it is not replaced with synthetic HTTP or Workers Cache marker entries.
SQLite load
Tis the number of tags on a newly published entry or tag selectors in a purge,Nis the number of entries matched by a bulk mutation,Cis the number of live claims removed, andSis the number of old per-entry tag-index rows. Statement counts exclude schema initialization and alarm storage API calls. Row counts are logical table rows and exclude index maintenance.Successful operations leave zero
pending_objectsand zero completedrevalidation_claims; E2E tests inspect SQLite directly to enforce that invariant. The steady durable footprint is oneentriesrow per cache key, onetag_invalidationsrow per tag that has actually been invalidated, and one metadata sequence row and two schema migration marker rows. There are no persistent per-entry tag-index rows.A publication normally performs no tag SQL read. If a tag purge completes between reservation and publication, the existing publication RPC performs up to
ceil(T/100)additional read-only SQL statements to determine whether the candidate's own tags were invalidated. This adds no metadata RPC and keeps tag checks off cache reads and ordinary publications.The first reservation after a Durable Object activation checks the cleanup alarm and creates it if absent; later reservations while that actor remains active do not repeat that storage call. The alarm uses one SQL query per sweep and only handles interrupted or superseded candidates, replacing the old background cleanup RPC on every put.
For 10,000 warmups that seed one cache entry, metadata load falls from about 70,000 DO requests to 30,000. With four tags per entry, logical SQLite writes fall from about 80,000 to 40,000. An App Router warmup that seeds both HTML and RSC from one request falls from about 130,000 to 50,000 DO requests and from about 160,000 to 80,000 logical SQLite writes for 10,000 routes.
Validation
pnpm --filter @vinext/workers-response-store test(43 tests)pnpm exec vitest run tests/response-store-adapter.e2e.test.ts tests/response-store-data.test.tsfrompackages/cloudflare(29 tests)