Clear package-owned durable state on delete and key app values by appId only - #1026
Conversation
deleteSavedPackageProjection removed D1 rows but orphaned every package-owned StorageRunner bucket (package bucket, legacy app root, facet/internal-DO buckets, job scratch, service scratch) and left app-scoped values behind while package secrets were deleted. Collect the package-owned storage id set from deterministic ids plus the user_storage_buckets inventory (user-scoped, bound-param prefix matches), clear each bucket with per-bucket error tolerance (inventory rows survive failed clears so account deletion can still enumerate them), and delete app-scoped values alongside package secrets. The values-service import gets the same package-registry boundary exception (with the same extraction TODO) as the secrets service.
The app value scope fell back to the run's storageId when appId was absent, letting non-package contexts (ad hoc execute or job runs with a bound storageId) mint app-scoped value buckets keyed by arbitrary storage ids. Package config is keyed by the saved package id, and every package surface sets appId, so the fallback only served the contradiction. App scope now resolves from appId or is unavailable.
Review found the inventory LIKE matching over-deleted for non-UUID
package ids: package_save accepts arbitrary ids, so a package id of
'job' matched every job:* bucket and LIKE metacharacters ('%', '_')
survived into the bound prefixes. Replace the SQL LIKE query with exact
JS-side matching over listUserStorageBucketIds; prefix arms (facets,
service scratch, job scratch) apply only to UUID-shaped package ids,
where they are provably unambiguous. Non-UUID packages still clear the
deterministic id set.
Value buckets minted under the removed app-scope storageId fallback (binding keys shaped job:/exec:) are unreachable by any read path now that app scope resolves from appId only. Migration 0109 deletes them, guarded so binding keys matching a real saved package id survive; value_entries cascade. Adds the read-path regression test showing fallback-keyed buckets are invisible with only a storageId bound.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
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-1026.kody-a99.workers.dev Worker: Mocks:
|
3 participants
Summary
Brings the two items deferred from #1022 into scope (follow-up requested on #231's conductor run; overlaps item 4 of #1024):
deleteSavedPackageProjectionpreviously removed only D1 rows, orphaning every package-owned StorageRunner bucket (package bucket, legacy raw-id app root, facet/internal-DO buckets, job scratch, service scratch) and leaving app-scoped values behind while package secrets were deleted. It now collects the package-owned storage id set (deterministic ids plus theuser_storage_bucketsinventory), clears each bucket with per-bucket error tolerance — inventory rows survive failed clears so account deletion can still enumerate them — and deletes app-scoped values alongside secrets.appIdonly. Theappvalue scope no longer falls back to the run'sstorageId, so non-package contexts (ad hoc execute orjob_scheduleruns with a bound storage id) can no longer mint "app"-scoped value buckets keyed by arbitrary storage ids. Every package surface setsappIdto the saved package id, so package code is unaffected. Migration0109deletes the now-unreachable fallback-keyed buckets (job:/exec:binding keys), guarded so any binding key matching a real saved package id survives.An independent review of the initial implementation found — and empirically verified — an over-deletion hole: saved package ids are caller-suppliable (not guaranteed UUIDs), so raw SQL
LIKEprefix matching let a package id of'job'match everyjob:*bucket, and%/_survived into bound prefixes. Fixed before merge: inventory matching is now exact JS-side comparison, with prefix arms (facets, service/job scratch) applied only to UUID-shaped package ids where they are provably unambiguous; non-UUID packages clear the deterministic id set only.The
#mcp/valuesimport from package-registry gets the same lint boundary exception (with the same extraction TODO) as the existing#mcp/secretsone.Testing
npm run validate(full local gate)'job'/'%'), pure filter tests for the UUID gate, migration 0109 guard behavior, app-scope save rejection withoutappId, and the read-path regression showing fallback-keyed buckets are unreachable3aca213eRefs #1024 (delivers the package-delete cleanup portion; the legacy app-bucket removal remains tracked there)
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@356426e6· Head:d8544633Classification: extends —
saved-packagesdelete behavior now destroys package-owned durable state;mcp-servervalue binding contract narrows;d1-app-dbgains cleanup migration 0109. No new primitives;primitives.yamlunchanged.Primitives touched
saved-packagesmcp-serverappIdonly (storageId fallback removed)d1-app-dbdurable-storageclearStorage()/ inventory helpers reused as-isSystem map
Package delete flows from the delete capability through the registry service, which enumerates buckets from the D1 inventory and clears each StorageRunner DO; the values service loses its storage-id fallback and the migration removes rows that fallback created.
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
Per-user isolation unchanged: bucket enumeration is
WHERE user_id = ?, everyclearStorage()goes throughstorageRunnerRpc({ userId }), and the review confirmed cross-user deletion is structurally impossible. Deletion is destructive by design but reachable only through the intent-checkedpackage_deletecapability; the UUID gate prevents same-user cross-namespace over-deletion for adversarial package ids.