Make mutating storage-byte entitlement checks O(1) via stored bucket estimates - #1084
Conversation
…estimates Persist each StorageRunner bucket's byte estimate on its user_storage_buckets inventory row (migration 0112) so mutating packageStorage().sql / storage.set entitlement checks read most bucket sizes from D1 instead of fanning getEstimatedBytes RPCs across every inventoried Durable Object. Only the bucket being written (plus any never-measured bucket, a one-time backfill) is probed live; probe results and post-write refreshes persist with UPDATE-only statements so they can never recreate rows removed by account/package/job deletion.
…ill lane Production incidents (2026-07-30 ~15:30 UTC) showed transient per-bucket DO estimate-read failures blocking tiny packageStorage() writes with 'could not be read after 2 attempts'. Live probes now retry with backoff (150ms/600ms/2400ms, 4 attempts) before failing closed — affordable now that mutating checks probe only the target bucket plus never-measured rows. A new storage_bucket_estimate_backfill cron lane seeds stored estimates for unmeasured inventory rows in bounded batches so freshly migrated inventories converge without each user's first write paying the whole-inventory probe. Regression tests cover the observed mode: an unreachable peer bucket with a stored estimate can never block another bucket's write.
📝 WalkthroughWalkthroughStorage bucket byte estimates are added to the D1 inventory, reused during entitlement checks, refreshed after successful mutations, and backfilled through a scheduled worker lane. Retry handling now uses a multi-delay backoff schedule with per-bucket failure isolation. ChangesStorage estimate lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduledWorker
participant EstimateBackfill
participant StorageBucketsService
participant StorageRunner
participant D1
ScheduledWorker->>EstimateBackfill: backfillStorageBucketEstimates
EstimateBackfill->>StorageBucketsService: listStorageBucketsMissingEstimates
StorageBucketsService->>D1: query NULL estimates
EstimateBackfill->>StorageRunner: getEstimatedBytes per bucket
EstimateBackfill->>StorageBucketsService: updateStorageBucketEstimate
StorageBucketsService->>D1: persist estimated bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7b8bce6. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/worker/src/storage-buckets/estimate-backfill.ts (1)
26-78: 🩺 Stability & Availability | 🔵 TrivialConsider surfacing aggregate backfill failures for alerting.
backfillStorageBucketEstimatesnever rejects — every row failure is caught internally viaPromise.allSettledand onlyconsole.warn'd. That means the scheduled lane inpackages/worker/src/index.tswill always resolve successfully for this lane, even iffailedis consistently non-zero across many ticks (e.g., a systemic DO issue). The lane's Sentry alerting path (scheduled_lane_failed) can never catch this class of problem. Consider throwing (or logging atconsole.error/reporting to Sentry) whenfailed > 0after a sweep so persistent backfill failures become visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/storage-buckets/estimate-backfill.ts` around lines 26 - 78, Update backfillStorageBucketEstimates to surface aggregate row failures after processing all chunks: when failed > 0, report the sweep through the existing scheduled-lane failure mechanism or throw an error so scheduled_lane_failed can detect it, while preserving the scanned/updated/failed result and per-row diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/architecture/entitlements.md`:
- Around line 272-274: Update the architecture documentation sentence describing
live probing to state that any inventoried bucket without a stored estimate is
probed, and that failed probes leave the estimate missing and are retried by the
backfill lane until successful; remove the inaccurate “one-time backfill per
bucket” wording.
In `@packages/worker/src/storage-buckets/service.ts`:
- Around line 131-160: Clarify the outcome contract of
updateStorageBucketEstimate so rejected non-finite values are distinguishable
from a missing row, rather than both returning false. Update
normalizeEstimatedBytes and its callers as needed, then make
backfillStorageBucketEstimates count rejected estimates as failed and only count
successful database updates as updated.
---
Nitpick comments:
In `@packages/worker/src/storage-buckets/estimate-backfill.ts`:
- Around line 26-78: Update backfillStorageBucketEstimates to surface aggregate
row failures after processing all chunks: when failed > 0, report the sweep
through the existing scheduled-lane failure mechanism or throw an error so
scheduled_lane_failed can detect it, while preserving the scanned/updated/failed
result and per-row diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d7f4c7b-b763-491c-91d2-484c83a0fe20
📒 Files selected for processing (15)
docs/contributing/architecture/entitlements.mdpackages/worker/migrations/0117-user-storage-bucket-estimated-bytes.sqlpackages/worker/src/index.tspackages/worker/src/index.workers.test.tspackages/worker/src/storage-buckets/estimate-backfill.node.test.tspackages/worker/src/storage-buckets/estimate-backfill.tspackages/worker/src/storage-buckets/estimate-backfill.workers.test.tspackages/worker/src/storage-buckets/service.tspackages/worker/src/storage-buckets/service.workers.test.tspackages/worker/src/storage-buckets/test-schema.tspackages/worker/src/storage-runner.entitlement.node.test.tspackages/worker/src/storage-runner.read-sql-entitlement.node.test.tspackages/worker/src/storage-runner.tspackages/worker/src/storage-runner.workers.test.tstools/migration-ledger.json
| function normalizeEstimatedBytes(estimatedBytes: number) { | ||
| if (!Number.isFinite(estimatedBytes)) return null | ||
| return Math.max(0, Math.round(estimatedBytes)) | ||
| } | ||
|
|
||
| /** | ||
| * Awaited UPDATE-only estimate persist. Returns whether an inventory row was | ||
| * updated (false when the row does not exist — registration owns creation). | ||
| */ | ||
| export async function updateStorageBucketEstimate(input: { | ||
| db: D1Database | ||
| userId: string | ||
| storageId: string | ||
| estimatedBytes: number | ||
| updatedAt?: Date | ||
| }): Promise<boolean> { | ||
| const estimatedBytes = normalizeEstimatedBytes(input.estimatedBytes) | ||
| if (estimatedBytes === null) return false | ||
| const result = await input.db | ||
| .prepare(estimateUpdateStatement) | ||
| .bind( | ||
| input.userId, | ||
| input.storageId, | ||
| estimatedBytes, | ||
| (input.updatedAt ?? new Date()).toISOString(), | ||
| ) | ||
| .run() | ||
| return (result.meta?.changes ?? 0) > 0 | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ambiguous updateStorageBucketEstimate return contract masks silent no-ops (including in the backfill lane).
normalizeEstimatedBytes returning null for non-finite input causes updateStorageBucketEstimate to return false — the exact same signal used for "row does not exist." The JSDoc only documents the row-absence case. Downstream, backfillStorageBucketEstimates (packages/worker/src/storage-buckets/estimate-backfill.ts, lines 55-67) awaits this call but discards the boolean, so a row that never actually got its NULL estimate cleared is still counted as updated. If a bucket's live probe ever yields a non-finite/garbage value, the row stays NULL forever, is re-selected by every future backfill sweep, and is reported as a success each time — no signal ever surfaces that convergence isn't happening.
Consider having callers check the boolean (or having updateStorageBucketEstimate distinguish "not found" from "rejected value" so estimate-backfill.ts can count the latter as failed).
🛠️ Proposed fix (root cause + backfill counting)
export async function updateStorageBucketEstimate(input: {
db: D1Database
userId: string
storageId: string
estimatedBytes: number
updatedAt?: Date
}): Promise<boolean> {
const estimatedBytes = normalizeEstimatedBytes(input.estimatedBytes)
- if (estimatedBytes === null) return false
+ if (estimatedBytes === null) {
+ throw new Error(
+ `invalid estimatedBytes for ${input.userId}/${input.storageId}`,
+ )
+ }- await updateStorageBucketEstimate({
+ const wroteRow = await updateStorageBucketEstimate({
db: input.env.APP_DB,
userId: row.userId,
storageId: row.storageId,
estimatedBytes,
updatedAt: input.now,
})
+ if (!wroteRow) throw new Error('estimate update did not affect a row')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/storage-buckets/service.ts` around lines 131 - 160,
Clarify the outcome contract of updateStorageBucketEstimate so rejected
non-finite values are distinguishable from a missing row, rather than both
returning false. Update normalizeEstimatedBytes and its callers as needed, then
make backfillStorageBucketEstimates count rejected estimates as failed and only
count successful database updates as updated.
|
🔎 Preview deployed: https://kody-pr-1084.kody-a99.workers.dev Worker: Mocks:
|
…ill retry semantics
2 participants
Context
Platform follow-up to #1055, now also covering the 2026-07-30 production incidents (~15:30 UTC): tiny
packageStorage().setrun-ledger writes from@kentcdodds/codemod-runnerwere blocked twice withUnable to verify the storage byte entitlement because the bucket estimate for storageId "package:…" could not be read after 2 attempts— a different peer bucket each time. Root cause: every cold mutatingpackageStorage().sql/storage.setentitlement check fannedgetEstimatedBytes()RPCs across every inventoried StorageRunner Durable Object for the user, so one transient per-bucket read failure (or hung/cold DO) failed or stalled unrelated writes.What changed
1. O(1) mutating entitlement baseline via stored per-bucket estimates. Migration
0117addsestimated_bytes+estimated_bytes_updated_attouser_storage_buckets. The baseline read is now two D1 queries plus live probes of only the bucket being written (always measured fresh) and any never-measured bucket. Peer buckets contribute their stored D1 estimates — their DOs are not contacted, so an unreachable peer can no longer block a write.2. Estimate persistence is UPDATE-only (
recordStorageBucketEstimate/updateStorageBucketEstimate): it can never recreate an inventory row removed by account/package/job deletion. Mutating StorageRunner RPCs also refresh their own bucket's estimate post-write, fire-and-forget, throttled per bucket per isolate (15s).clearStorageintentionally does not refresh (clears almost always precede row deletion; a rare user-facing clear leaves a stale-high, fail-safe estimate).3. Strengthened live-probe retry policy. Probes retry with backoff (
storageEstimateReadRetryDelaysMs = [150, 600, 2400], 4 attempts, each read still bounded ~2s and fail-closed at exhaustion) — affordable now that the probe set is one or two buckets instead of the whole inventory. Production showed a single 150ms retry losing to transient DO read failures.4. First-deploy backfill. A new
storage_bucket_estimate_backfillcron lane (every 5-min tick) seeds estimates for never-measured inventory rows in bounded batches (24/tick, concurrency 8, short per-row retries, per-row failure tolerance), so inventories converge to steady state within a few ticks instead of each user's first mutating write paying (and possibly failing on) the whole-inventory probe. Converged inventories make the lane a single cheap SELECT.Unchanged: read-only SQL still skips the baseline entirely (#1055); the per-run cache still dedupes baselines within a sandbox; every query and DO name stays scoped by
userId. Docs:docs/contributing/architecture/entitlements.mdstorage-bytes section rewritten.Note: the migration was renumbered
0112→0113→0117as main took those prefixes while this branch waited;origin/mainis merged in.Tests
storage-runner.entitlement.node.test.ts): an unreachable peer bucket with a stored estimate never blocks another bucket's write across a 40-bucket inventory (only the target is probed); the fail-closed error surfaces only after the full 4-attempt policy is exhausted (asserts attempt count and message).storage-runner.read-sql-entitlement.node.test.ts: stored estimates → only target probed; stored estimates count toward denial (current = stored + live probe); probed values persisted; mutating SQL schedules the post-write refresh, read-only SQL does not; run-cache retry-after-rejected-baseline updated for the new policy.estimate-backfill.workers.test.ts(real D1 + DOs): backfill seeds NULL rows in bounded batches, converges, then no-ops.estimate-backfill.node.test.ts: per-row probe failures are logged, left NULL for later sweeps, and never block peers.storage-buckets/service.workers.test.ts(real D1): UPDATE-only persistence cannot create rows; refresh throttle; failed refresh clears the throttle for retry.storage-runner.workers.test.ts: mutating writes persist estimates end-to-end; existing aggregate/deletion-race tests still pass.index.workers.test.ts: the scheduled handler runs the new backfill lane.npm run validategreen locally on the merged tree (unit suites 479 files / 1635 tests; Playwright E2E 19 passed and MCP E2E passed when run per-suite — two admin E2E flakes under fully-parallel local validate were VM resource contention; CI runs these as separate jobs).Agent run: https://cursor.com/agents/bc-2791778f-3d00-47ed-a3e7-62c7c2fe332d
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@5be54d3b· Head:7b8bce6dClassification: extends — the storage-byte entitlement enforcement path, the bucket-inventory schema, and the cron lane list change shape; no new primitive.
Primitives touched
durable-storaged1-app-dbuser_storage_buckets.estimated_bytes(_updated_at)entitlementsstorage_bytescounting reads stored estimates; probe retry policy strengthened (docs updated)scheduled-cronstorage_bucket_estimate_backfilllane seeds unmeasured rows in bounded batchesSystem map
Mutating
packageStorage().sql/storage.setentitlement checks now read peer bucket sizes from D1 and probe only the written bucket live; a cron lane converges unmeasured rows.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
Invariants
user_id; DO names stay[userId, storageId]-keyed.Note
Medium Risk
Touches the storage-byte entitlement enforcement path and quota accounting semantics (stale stored estimates are acceptable but change when limits bite); mitigations include UPDATE-only persistence, fail-closed live probes, and regression tests for the production incident.
Overview
Fixes production failures where tiny mutating writes were blocked because storage-byte entitlement checks fanned
getEstimatedBytesRPCs across every inventoried StorageRunner bucket; one transient peer read failure could fail the whole write.D1-backed estimates (migration 0118):
user_storage_bucketsgainsestimated_bytes/estimated_bytes_updated_at. Mutating baselines sum D1 payload bytes plus stored per-bucket estimates; live DO probes are limited to the write target (always refreshed) and buckets with NULL estimates. Probes persist via UPDATE-only writes so deleted inventory rows cannot be recreated; post-mutation paths refresh the written bucket’s estimate (throttled per isolate).Write path: Estimate reads use multi-step backoff (
[150, 600, 2400], four attempts, ~2s cap per read) before failing closed. A scheduledstorage_bucket_estimate_backfilllane seeds NULL rows in bounded batches so deploy/migration does not force whole-inventory probes on first writes.Docs (
entitlements.md) and unit/workers tests cover peer non-blocking, stored-estimate accounting, backfill, and cron wiring.Reviewed by Cursor Bugbot for commit 2c42a53. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes