Add the package-storage audit route and harden entitlement estimate reads - #1030
Conversation
GET /admin/package-storage-audit.json (admin RBAC) reports, platform-wide, every has_app package's legacy raw-id bucket size (probed per-user via StorageRunner, no registration side effects; 4096 bytes = never-written floor), which published app sources still import the banned ambient storage (reusing the repo-check AST scan), and orphaned kind='app' inventory rows. This is step 1 of #1024: decide delete-vs-migrate for the legacy app bucket from evidence. Also gives the entitlement estimator one bounded retry per chunk before failing closed: package delete now clears buckets concurrently with live traffic, and we observed a production write flake coinciding exactly with a bucket deletion.
|
Warning Review limit reached
Next review available in: 11 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 (4)
📝 WalkthroughWalkthroughAdds an admin JSON endpoint that audits package storage usage, ambient imports, and orphan buckets. It also adds retry handling for storage estimate reads, an empty-bucket baseline constant, route wiring, exported scan logic, and comprehensive tests. ChangesPackage storage audit endpoint
Storage estimate reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant AuditHandler
participant Database
participant StorageRunner
participant SourceLoader
Admin->>AuditHandler: Request package storage audit
AuditHandler->>Database: Query app packages and orphan buckets
AuditHandler->>StorageRunner: Estimate legacy bucket bytes
AuditHandler->>SourceLoader: Load package source
AuditHandler-->>Admin: Return JSON audit report
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 |
|
🔎 Preview deployed: https://kody-pr-1030.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/admin-package-storage-audit.node.test.ts (1)
261-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the empty-bucket baseline from the exported constant instead of hardcoding 4096.
nonEmptyLegacyBucketscompares againstemptyStorageRunnerEstimatedBytes; if that floor changes, these assertions silently stop testing the boundary. Import the constant and usebaseline/baseline * 2.Also applies to: 328-347
🤖 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/handlers/admin-package-storage-audit.node.test.ts` around lines 261 - 269, Update the storage audit test mock and related assertions in the mockModule.storageRunnerRpc scenarios to import and use the exported emptyStorageRunnerEstimatedBytes constant, assigning baseline and baseline * 2 for empty and data buckets instead of hardcoding 4096 and 8192. Apply the same replacement to the additional cases around the referenced assertions.
🤖 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/handlers/admin-package-storage-audit.node.test.ts`:
- Around line 112-135: Update the mock query handler’s orphan filtering around
packageIds to validate package ownership by both userId and storageId, then add
a test fixture where one user’s bucket storageId matches another user’s package
and assert that bucket is reported as an orphan. Ensure same-user package
matches remain excluded.
In `@packages/worker/src/app/handlers/admin-package-storage-audit.ts`:
- Around line 156-170: Update the query in listOrphanAppBuckets to join
saved_packages on both package id/storage_id and matching user_id, ensuring
orphan detection is scoped to the bucket owner. Add the same report limit used
by the packages query so the orphan results are bounded consistently.
In `@packages/worker/src/storage-runner.ts`:
- Around line 583-600: Update the readChunk retry flow around storageRunnerRpc
and getEstimatedBytes so the initial Promise.all attempt is fully settled before
starting the delayed retry, preventing overlapping reads from exceeding the
five-request concurrency cap. Preserve the existing retry result behavior, and
add a multi-storage-ID test covering one immediate failure and one pending read
to verify the retry waits for both outcomes.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/admin-package-storage-audit.node.test.ts`:
- Around line 261-269: Update the storage audit test mock and related assertions
in the mockModule.storageRunnerRpc scenarios to import and use the exported
emptyStorageRunnerEstimatedBytes constant, assigning baseline and baseline * 2
for empty and data buckets instead of hardcoding 4096 and 8192. Apply the same
replacement to the additional cases around the referenced assertions.
🪄 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: 3b92526d-c3b4-4f21-bedc-52bb85fa7e2a
📒 Files selected for processing (8)
packages/worker/src/app/handlers/admin-package-storage-audit.node.test.tspackages/worker/src/app/handlers/admin-package-storage-audit.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/repo/checks.tspackages/worker/src/storage-runner.entitlement.node.test.tspackages/worker/src/storage-runner.tspackages/worker/src/storage-runner.workers.test.ts
Scope the orphan-bucket join by user_id so a cross-user package id collision cannot hide an orphan, bound the orphan list (500 rows + orphanTruncated), and settle all in-flight estimate reads before the entitlement retry so the concurrency cap holds.
3 participants
Summary
Pre-cleanup groundwork for #1024 (removing the legacy raw-package-id app bucket), plus one hardening fix informed by production evidence:
GET /admin/package-storage-audit.json(admin RBAC): the read-only audit that gates the Remove the legacy raw-package-id app storage bucket #1024 delete-vs-migrate decision. Platform-wide, it reports everyhas_apppackage's legacy raw-id bucket size (probed per-user throughstorageRunnerRpc; probing is side-effect-free —getEstimatedBytesnever registers buckets — and 4096 bytes is the never-written SQLite floor), which published app sources still import the banned ambientstorage(reusing the repo-check AST scan, now exported), and orphanedkind='app'inventory rows from packages deleted before Clear package-owned durable state on delete and key app values by appId only #1026. Bounded:?limit=(default 200, max 500) with atruncatedflag, DO probes in concurrency-5 chunks, per-package error tolerance so one bad row can't sink the report.assertStorageRunnerWriteWithinEntitlementfans outgetEstimatedBytesover every registered bucket and fails closed on any error. Since Clear package-owned durable state on delete and key app values by appId only #1026, package delete clears buckets concurrently with live traffic, and we observed exactly one production write flake ("Unable to verify the storage byte entitlement…") coinciding to the second with a package deletion. Each chunk now gets one bounded retry (150ms) before the existing fail-closed error.Production verification that motivated this (run via MCP against prod):
packageStorage()round-trips (KV + SQL) in the real invocation runtime across runs;package_deletedeallocates the bucket (post-delete query:no such table); zeroValue scope "app"errors in run records since the fallback removal.Testing
npm run validate(full local gate)getEstimatedBytesbaseline (4096) and probe-does-not-register behaviorRefs #1024
System recap — composes existing primitives (low risk)
Mode: recap · Base:
main@9bf6dd26· Head:2f9e8(audit commit)Classification: composes — a read-only admin surface wiring existing primitives together, plus a bounded retry inside an existing durable-storage code path. No schema changes, no new primitives,
primitives.yamlunchanged.Primitives touched
app-ui/ admin/admin/package-storage-audit.json(RBAC)durable-storagesaved-packagessaved_packages/user_storage_bucketsrbacrequireUserWithRole('admin')gatingSystem map
The admin route reads package rows and bucket inventory from D1, probes legacy buckets through StorageRunner per owning user, and scans published sources with the existing repo-check helper.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
flowchart LR adminRoute["app-ui<br/>Admin audit route"]:::touched rbac["rbac<br/>Role-based access control"]:::touched d1AppDb["d1-app-db<br/>D1 app database"]:::untouched durableStorage["durable-storage<br/>StorageRunner buckets"]:::extended repoChecks["saved-packages<br/>Repo checks / published source"]:::touched adminRoute -->|"requireUserWithRole('admin')"| rbac adminRoute -->|"saved_packages has_app rows + orphan kind='app' inventory"| d1AppDb adminRoute -->|"getEstimatedBytes per raw-id bucket (per-user, read-only)"| durableStorage adminRoute -->|"loadPackageSourceBySourceId + collectAmbientStorageImportFiles"| repoChecks classDef touched fill:#1a7f37,color:#fff classDef extended fill:#9a6700,color:#fff classDef added fill:#cf222e,color:#fff classDef untouched fill:#57606a,color:#fffInvariants
Per-user isolation: the D1 enumeration is deliberately platform-wide (admin surface, RBAC-gated, read-only), but every StorageRunner probe passes the row's own
userId, so DO access stays namespaced. No writes anywhere in the audit path.Summary by CodeRabbit
New Features
limitparameter.Bug Fixes
Tests