Track storage bucket ownership in D1 instead of deriving it from run history - #965
Conversation
Add user_storage_buckets as authoritative state (with a one-time package_runtime_runs backfill), register buckets on StorageRunner mutators with isolate-local dedupe, and cover the table in account export/deletion targets. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Repoint DR inventory, account deletion, and account export off package_runtime_runs onto user_storage_buckets plus manifest/state service enumeration so post-#955 buckets and idle services stay discoverable. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
durable_object_summaries pages again over user_storage_buckets and entity/state tables without per-page manifest fetches. Manifest-inclusive enumeration stays on deletion and one-shot full export inventory. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe PR adds ChangesStorage inventory lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StorageRunner
participant StorageBucketService
participant D1Database
participant AccountWorkflow
StorageRunner->>StorageBucketService: Register writable storage bucket
StorageBucketService->>D1Database: Upsert user_storage_buckets
AccountWorkflow->>D1Database: Read registered buckets and service states
AccountWorkflow->>StorageRunner: Export or clear discovered storage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
🔎 Preview deployed: https://kody-pr-965.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/worker/src/app/account-deletion.node.test.ts (2)
181-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew mock branches look correct; the legacy
package_runtime_runsbranches below are now unreachable.Given the new contract test asserts owned sources no longer reference
package_runtime_runs, the routing branches at Lines 222-307 are dead fixture code and can be dropped.🤖 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/app/account-deletion.node.test.ts` around lines 181 - 221, Remove the now-unreachable legacy package_runtime_runs mock-routing branches immediately following the package_service_states handling, through the section before the next active query branch. Keep the new package_service_states and user_storage_buckets branches unchanged, and remove only fixture logic that handles package_runtime_runs.
2181-2184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShared
consoleWarnspy is mutated without restoration. Both tests install a silencing implementation on the module-level spy from#worker/test-support/console-spies.tsbut never restore it, so warnings can stay suppressed for later tests in the same file unless the support module re-installs the spy per test.
packages/worker/src/app/account-deletion.node.test.ts#L2181-L2184: restore/resetconsoleWarnin the existingfinallyblock.packages/worker/src/app/account-export.node.test.ts#L1456-L1456: resetconsoleWarnat the end of the test (or rely on a globalrestoreAllMocks).🤖 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/app/account-deletion.node.test.ts` around lines 2181 - 2184, Restore or reset the shared consoleWarn spy after the account-deletion test, using its existing finally block, so the mock implementation cannot leak to later tests. Also reset consoleWarn at the end of the account-export test, or ensure the global restoreAllMocks mechanism reliably handles it: packages/worker/src/app/account-deletion.node.test.ts#L2181-L2184 requires the finally-block cleanup; packages/worker/src/app/account-export.node.test.ts#L1456-L1456 requires end-of-test cleanup.packages/worker/src/app/account-export.ts (2)
458-468: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShared SQL fragment couples callers to a placeholder count.
exportStorageIdBaseSqlneeds exactly fouruserIdbinds, repeated at Lines 1848-1852 and 1902-1906. Exporting a small helper that returns{ sql, binds }(or building the bind array from a constant) would keep them in sync if a UNION branch is ever added.🤖 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/app/account-export.ts` around lines 458 - 468, The shared exportStorageIdBaseSql fragment currently requires callers to manually maintain four userId binds. Add a helper near exportStorageIdBaseSql that returns the SQL and its corresponding bind array, then update both call sites around the discovery queries to consume that helper instead of hardcoding repeated userId values; keep the SQL and bind order synchronized if UNION branches change.
1897-1909: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-row ownership probe inside the page loop is an N+1.
Each discovered service issues its own
SELECT 1against the base set. Sinceselectedis already bounded bypageSize, one query withid IN (...)(or aNOT INanti-join over the base subquery) would collapse this to a single round trip.🤖 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/app/account-export.ts` around lines 1897 - 1909, Replace the per-row ownership query in the loop over selected with one batched ownership lookup for all selected storage IDs, using a parameterized IN predicate or equivalent anti-join against exportStorageIdBaseSql. Build an ID-to-ownership result set from that single query, then reuse it while processing each row; preserve the existing ownership semantics and bindings.packages/worker/src/app/account-user-inventory.ts (2)
79-112: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winManifest loads run strictly sequentially per saved package.
Each iteration awaits a manifest load, so enumeration cost is O(packages) round trips on the deletion/full-export path. Bounded-concurrency batching (e.g. chunks of 5-10 via
Promise.all) keeps the same failure isolation with much lower latency for users with many packages.🤖 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/app/account-user-inventory.ts` around lines 79 - 112, The package manifest enumeration loop should use bounded concurrency instead of awaiting each load sequentially. Update the flow around loadPackageManifestBySourceId to process savedPackage items in small batches (for example, 5–10 at a time) with Promise.all, while preserving per-package error isolation, service aggregation through byKey, and the existing warning for failed loads.
107-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winManifest-load failures are only
console.warned, so deletion silently under-reports.
listAccountUserPackageServicesis used byaccount-deletion.ts(listUserPackageServices→collectUserDeletionInventory), which builds a caller-visiblewarningsarray. A manifest failure here means a manifest-only service DO is never purged, yet the deletion result reports clean success. Consider accepting an optionalwarnings: Array<string>and pushing these messages so the deletion/export result reflects incomplete enumeration.♻️ Sketch
export async function listAccountUserPackageServices(input: { env: Env userId: string baseUrl: string + warnings?: Array<string> }): Promise<Array<AccountUserPackageService>> { @@ } catch (error) { - console.warn( - `Failed to load package manifest for service enumeration (package ${savedPackage.id}): ${getErrorMessage(error)}`, - ) + const message = `Failed to load package manifest for service enumeration (package ${savedPackage.id}): ${getErrorMessage(error)}` + input.warnings?.push(message) + console.warn(message) }🤖 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/app/account-user-inventory.ts` around lines 107 - 117, Update listAccountUserPackageServices to accept an optional warnings: Array<string> parameter and append both manifest-enumeration failure messages to it instead of only calling console.warn. Update the account-deletion call chain, including listUserPackageServices and collectUserDeletionInventory, to pass through the existing caller-visible warnings array while preserving console warnings where appropriate.packages/worker/src/app/account-export.node.test.ts (1)
1582-1588: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate contract assertion.
account-deletion.node.test.tsalready asserts./account-export.tscontains nopackage_runtime_runsreference in its owned-sources loop; this test repeats it.🤖 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/app/account-export.node.test.ts` around lines 1582 - 1588, Remove the duplicate test `account export source no longer reads package_runtime_runs` from `account-export.node.test.ts`, relying on the existing owned-sources assertion in `account-deletion.node.test.ts` to enforce this contract.
🤖 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 `@packages/worker/src/app/account-export.ts`:
- Around line 715-718: Update countUserStorageIds to use the same D1-only source
as durable_object_summaries paging—exportStorageIdBaseSql plus
package_service_states—instead of listUserStorageIds, so storage_runners.count
matches the ids clients can enumerate.
- Around line 1544-1545: The section readers must avoid one-shot inventory
helpers on per-request paths. In packages/worker/src/app/account-export.ts lines
1544-1545, replace the listUserStorageIds(...).includes(...) check with a
targeted exportStorageIdBaseSql probe and package_service_states fallback. Also
update lines 1668-1674 to resolve {packageId, serviceName} from
package_service_states first, falling back to manifest enumeration only when no
row exists.
---
Nitpick comments:
In `@packages/worker/src/app/account-deletion.node.test.ts`:
- Around line 181-221: Remove the now-unreachable legacy package_runtime_runs
mock-routing branches immediately following the package_service_states handling,
through the section before the next active query branch. Keep the new
package_service_states and user_storage_buckets branches unchanged, and remove
only fixture logic that handles package_runtime_runs.
- Around line 2181-2184: Restore or reset the shared consoleWarn spy after the
account-deletion test, using its existing finally block, so the mock
implementation cannot leak to later tests. Also reset consoleWarn at the end of
the account-export test, or ensure the global restoreAllMocks mechanism reliably
handles it: packages/worker/src/app/account-deletion.node.test.ts#L2181-L2184
requires the finally-block cleanup;
packages/worker/src/app/account-export.node.test.ts#L1456-L1456 requires
end-of-test cleanup.
In `@packages/worker/src/app/account-export.node.test.ts`:
- Around line 1582-1588: Remove the duplicate test `account export source no
longer reads package_runtime_runs` from `account-export.node.test.ts`, relying
on the existing owned-sources assertion in `account-deletion.node.test.ts` to
enforce this contract.
In `@packages/worker/src/app/account-export.ts`:
- Around line 458-468: The shared exportStorageIdBaseSql fragment currently
requires callers to manually maintain four userId binds. Add a helper near
exportStorageIdBaseSql that returns the SQL and its corresponding bind array,
then update both call sites around the discovery queries to consume that helper
instead of hardcoding repeated userId values; keep the SQL and bind order
synchronized if UNION branches change.
- Around line 1897-1909: Replace the per-row ownership query in the loop over
selected with one batched ownership lookup for all selected storage IDs, using a
parameterized IN predicate or equivalent anti-join against
exportStorageIdBaseSql. Build an ID-to-ownership result set from that single
query, then reuse it while processing each row; preserve the existing ownership
semantics and bindings.
In `@packages/worker/src/app/account-user-inventory.ts`:
- Around line 79-112: The package manifest enumeration loop should use bounded
concurrency instead of awaiting each load sequentially. Update the flow around
loadPackageManifestBySourceId to process savedPackage items in small batches
(for example, 5–10 at a time) with Promise.all, while preserving per-package
error isolation, service aggregation through byKey, and the existing warning for
failed loads.
- Around line 107-117: Update listAccountUserPackageServices to accept an
optional warnings: Array<string> parameter and append both manifest-enumeration
failure messages to it instead of only calling console.warn. Update the
account-deletion call chain, including listUserPackageServices and
collectUserDeletionInventory, to pass through the existing caller-visible
warnings array while preserving console warnings where appropriate.
🪄 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: 6dc01f71-9ec9-44e3-935a-475fbc945336
📒 Files selected for processing (21)
docs/contributing/disaster-recovery.mdpackages/worker/migrations/0097-user-storage-buckets.sqlpackages/worker/src/app/account-data-targets.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-export.node.test.tspackages/worker/src/app/account-export.tspackages/worker/src/app/account-retention-dispositions.node.test.tspackages/worker/src/app/account-retention-dispositions.tspackages/worker/src/app/account-user-inventory.tspackages/worker/src/dr/exporter.node.test.tspackages/worker/src/dr/exporter.tspackages/worker/src/entitlements/test-schema.tspackages/worker/src/storage-buckets/migration.node.test.tspackages/worker/src/storage-buckets/service.node.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.tspackages/worker/src/storage-runner.workers.test.tstools/migration-ledger.json
Make storage_runners counts match D1 discovery paging, probe ownership and package-service lookups without full inventory rebuilds, batch the service-stage base check, and surface manifest degradation in deletion warnings so incomplete purges cannot look clean. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/worker/src/app/account-export.ts`:
- Around line 481-507: Update isExportDiscoverableStorageId to safely handle
malformed percent-encoding in packagePart or servicePart: wrap both
decodeURIComponent calls in error handling and return false when decoding
throws. Preserve the existing database lookup for valid decoded components and
the current false returns for invalid service storage IDs.
🪄 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: 34895cfc-e075-459e-9dbd-abc44dad1f7e
📒 Files selected for processing (5)
packages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-export.node.test.tspackages/worker/src/app/account-export.tspackages/worker/src/app/account-user-inventory.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/worker/src/app/account-user-inventory.ts
- packages/worker/src/app/account-export.node.test.ts
- packages/worker/src/app/account-deletion.node.test.ts
Guard decodeURIComponent on caller-supplied service: storage ids so a bare % returns the normal not-found path instead of throwing URIError. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
There are 5 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d009f75. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/worker/src/app/account-export.node.test.ts`:
- Around line 1740-1762: Update the test around readAccountExportSection to
create the required user_storage_buckets or ownership fixture for the supplied
storageId, ensuring lookup reaches URI decoding and validation. Keep the
malformed service identifier `service:pkg%:worker%` and assert it is converted
to the “Storage runner was not found for account export.” error, rather than
passing through missing-ownership handling.
🪄 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: 84d9d7bd-ec88-468c-83e9-7da6559671f0
📒 Files selected for processing (2)
packages/worker/src/app/account-export.node.test.tspackages/worker/src/app/account-export.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/app/account-export.ts
Account deletion clears StorageRunner DOs then deletes user_storage_buckets. Registering on clearStorage could fire-and-forget an upsert that recreates rows for an already-deleted user. Registration stays on genuine write paths. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Union listRunRecordStorageIds into export count, ownership probes, and discovery paging so RunLog-only buckets stay exportable and count≡paging. Restore package_runtime_runs on the deletion path only (issue #956), and dedupe package-manifest loads within a single inventory request. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>

Unblocks the legacy removal tracked in #956, and closes a backup gap that is live right now.
The live gap
StorageRunnerDurable Objects are named[userId, storageId], and Cloudflare cannot enumerate DOs by name — so anything asking "which buckets does this user own?" has to derive it from D1. Three consumers derived it partly frompackage_runtime_runs, a run-history table that #955 stopped writing.dr/exporter.tsbuilds the platform disaster-recovery inventory from D1 only, so a bucket created since the #955 deploy and referenced only by a run record is already missing from backups. Account deletion and export were on the same footing, just with a longer fuse: their legacy arms keep working until the rows drain around 2026-08-25, after which a bucket known only there becomes unenumerable and its Durable Object storage would survive account deletion.Why this had to happen now rather than after the drain
The legacy
package_runtime_runsrows are the only surviving record of pre-#955 ad-hoc buckets. Migration0097backfills from them while they still exist. Wait for the drain and that history is gone permanently, with no way to reconstruct which buckets a user owns.What changed
Bucket ownership is now state, in a
user_storage_bucketstable keyed(user_id, storage_id). Deriving it from run history was the original mistake — the samestate-vs-historyinvariant #955 added, violated one layer down.Registration happens at the single accessor,
storageRunnerRpc, on mutating operations only (setValue,deleteValue,importStorage, andsqlQuerywhenwritable). Reads never register, and neither doesclearStorage— see the deletion race below. An in-isolate dedupe set bounded at 4096 keeps a hot isolate to one write per bucket, so this does not become a per-event D1 write.Service enumeration is now authoritative too. It previously required a
PackageServiceInstanceDO to have projected intopackage_service_states, so a stopped service whose DO never woke was only findable through the legacy arm. Services are declared inpackage.json#kody.services, so deletion now unions manifest-declared services with the state table, degrading to the state table alone if a repo fetch fails. A manifest error can never abort a deletion, and it now surfaces a caller-visible warning rather than reporting clean success.What this changes for #956
The DR gap is fixed rather than documented, and the "verify
package_service_stateshas converged before 2026-08-25" deadline item is gone, because bucket enumeration no longer depends on projection having happened.Correction to an earlier version of this description: it claimed every
package_runtime_runsread was removed. That was wrong, and review caught it. One legacy arm is deliberately restored —listAccountUserPackageServices({ includeLegacyRuntimeRuns: true })on the account-deletion path only. Migration0097backfills storage ids, not(packageId, serviceName)tuples, so a pre-#955 service whose DO never projected and whose manifest declaration has since been removed would otherwise be missed, leaving its Durable Object alive after deletion. Correctness beats the cleanliness claim; #956 tracks removing it after the drain.Reviewing this
packages/worker/src/storage-buckets/service.tsis the new writer and readers.packages/worker/src/app/account-user-inventory.tsis the shared enumeration extracted from the two copies that had drifted betweenaccount-deletion.tsandaccount-export.ts.One deliberate split worth knowing: account export paging stays on bounded per-request sources, while account deletion pays the manifest fetches. Paging wants cheap bounded pages; deletion wants completeness because a missed bucket leaks storage. Count and paging share one source set (
jobs,archived_job_artifacts,user_storage_buckets, app packages,package_service_statesstorage ids, and oneRunLogRPC) specifically so they cannot report different totals — there is a test asserting they agree, including for a RunLog-only id.Known follow-up, called out rather than hidden: a manifest-declared service that has never run still won't appear in paginated export discovery. Deletion covers it. Making it cheap would mean registering declared service buckets at publish time.
Review round
Three automated passes found seven issues, all fixed in-branch. The ones worth knowing about:
clearStorageregistered ownership, and account deletion callsclearStorage— so a fire-and-forget upsert could recreate rows for an already-deleted user. My own change introduced a data-residue path of exactly the kind this PR exists to prevent. Fixed by removingclearStoragefrom the registering set, after tracing every purge path that runs after D1 rows are deleted.decodeURIComponenton a caller-supplied storage id threwURIErrorinstead of returning not-found.Bugbot's final pass is clean; CodeRabbit marked its findings addressed.
Verification
npm run validategreen: 1387 tests across 423 files, Playwright E2E, MCP E2E,primitives:check,migrations:check.Tests cover the cases that motivated this: deletion purging a bucket known only via
user_storage_buckets, a service declared only in a manifest, a service known only via legacy rows, a manifest load failure warning without aborting, DR inventory including bucket and service state, and the backfill picking up legacy-only buckets.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@ca183641· Head:54878b37Classification: extends — no new primitive; bucket ownership moves from derived history to stored state, and three consumers are repointed.
Primitives touched
durable-storagestorageRunnerRpcregisters ownership on mutating opsd1-app-db0097addsuser_storage_buckets+ backfillbackup-control-planeaccount-exportapp-uientitlementsSystem map
Bucket ownership stops being inferred from run history and becomes a D1 table that DR, deletion, and export all read.
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
package_runtime_runshistoryuser_storage_bucketsstate tableInvariants
Applies
state-vs-historyone layer below where #955 introduced it: ownership is state and must not be inferred from run history.no-per-event-shared-writesis respected — registration is write-path only, deduped in isolate, and bounded.per-user-isolationunchanged;dr/exporter.tsremains the documented operator-level exception.Summary by CodeRabbit
user_storage_buckets) with automatic updates on storage writes.package_service_states.