Bound package-storage audit work and add keyset pagination - #1077
Conversation
The audit never completed on production: loadPackageSourceBySourceId can hang on Artifacts fetches, a hanging promise never trips the per-package try/catch, and the chunk's Promise.all wedged until the stale-run TTL (even with limit 8). Probes and source scans now race a deadline (10s / 20s) and record a timed-out error row instead of blocking the report, and callers can walk large installs in reliable slices via an opaque startAfter keyset cursor (route query param and MCP start_after input, nextStartAfter in the report).
|
Warning Review limit reached
Next review available in: 16 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 (3)
📝 WalkthroughWalkthroughThe package storage audit now supports keyset pagination through HTTP and MCP interfaces, returning continuation cursors for truncated results. Per-package legacy bucket and source scans also enforce configurable deadlines, with tests covering paging and timeout errors. ChangesPackage storage audit flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminInterface
participant buildPackageStorageAuditReport
participant saved_packages
Client->>AdminInterface: Send limit and start_after
AdminInterface->>buildPackageStorageAuditReport: Forward pagination cursor
buildPackageStorageAuditReport->>saved_packages: Query rows after cursor with limit plus one
saved_packages-->>buildPackageStorageAuditReport: Ordered package rows
buildPackageStorageAuditReport-->>AdminInterface: Return report and nextStartAfter
AdminInterface-->>Client: Return paginated audit response
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-1077.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: 1
🧹 Nitpick comments (3)
packages/worker/src/package-storage-audit/service.ts (2)
204-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeyset predicate and ordering are consistent; consider de-duplicating the SQL.
The
(user_id > ? OR (user_id = ? AND id > ?))predicate matchesORDER BY user_id ASC, id ASCunder SQLite's default binary text collation, andlimit + 1truncation is preserved. The two near-identical query strings could collapse into one built from a conditional WHERE fragment and params array.♻️ Optional consolidation
- const result = input.startAfter - ? await input.db - .prepare( - `SELECT user_id AS userId, id AS packageId, kody_id AS kodyId, - source_id AS sourceId - FROM saved_packages - WHERE has_app = 1 - AND (user_id > ? OR (user_id = ? AND id > ?)) - ORDER BY user_id ASC, id ASC - LIMIT ?`, - ) - .bind( - input.startAfter.userId, - input.startAfter.userId, - input.startAfter.packageId, - input.limit + 1, - ) - .all<AppPackageRow>() - : await input.db - .prepare( - `SELECT user_id AS userId, id AS packageId, kody_id AS kodyId, - source_id AS sourceId - FROM saved_packages - WHERE has_app = 1 - ORDER BY user_id ASC, id ASC - LIMIT ?`, - ) - .bind(input.limit + 1) - .all<AppPackageRow>() + const keysetClause = input.startAfter + ? 'AND (user_id > ? OR (user_id = ? AND id > ?))' + : '' + const keysetParams = input.startAfter + ? [ + input.startAfter.userId, + input.startAfter.userId, + input.startAfter.packageId, + ] + : [] + const result = await input.db + .prepare( + `SELECT user_id AS userId, id AS packageId, kody_id AS kodyId, + source_id AS sourceId + FROM saved_packages + WHERE has_app = 1 + ${keysetClause} + ORDER BY user_id ASC, id ASC + LIMIT ?`, + ) + .bind(...keysetParams, input.limit + 1) + .all<AppPackageRow>()🤖 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/package-storage-audit/service.ts` around lines 204 - 237, Optionally consolidate the duplicated SQL in listAppPackagesForAudit by defining one shared SELECT/ORDER BY query, adding the keyset WHERE fragment only when input.startAfter is present, and building the bind parameters accordingly. Preserve the existing ordering, cursor predicate, and input.limit + 1 truncation behavior.
183-202: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDeadline bounds waiting, not the underlying work.
withDeadlineonly stops awaiting; thegetEstimatedBytes()RPC and source load keep running (and keep consuming subrequest/CPU budget) after rejection. Acceptable if the intent is purely "don't block the report", but if the production hang was resource exhaustion, the probes need real cancellation (e.g.AbortSignalplumbed through). Worth a comment stating the chosen semantics.🤖 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/package-storage-audit/service.ts` around lines 183 - 202, The withDeadline helper only bounds waiting while the underlying operation continues running; choose and document the intended semantics, and if resource cleanup is required, propagate an AbortSignal through the callers and cancellable getEstimatedBytes/source-load operations so timeout aborts them. Otherwise add a clear comment that the helper intentionally stops awaiting without cancelling the work.packages/worker/src/package-storage-audit/service.node.test.ts (1)
42-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStub comparator uses
localeCompare, production ordering is SQLite binary collation.
localeCompareorders e.g.'_'/'A'/'a'differently than SQLite's defaultBINARYcollation, so keyset paging could pass here and skip/repeat rows in production for mixed-case or punctuated ids. A plain</>comparison mirrors the real ordering.♻️ Proposed change
- const byUser = left.userId.localeCompare(right.userId) - if (byUser !== 0) return byUser - return left.packageId.localeCompare(right.packageId) + if (left.userId !== right.userId) return left.userId < right.userId ? -1 : 1 + if (left.packageId === right.packageId) return 0 + return left.packageId < right.packageId ? -1 : 1🤖 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/package-storage-audit/service.node.test.ts` around lines 42 - 49, Update comparePackageKeys to use plain lexicographic < and > comparisons for userId, then packageId when userId matches, matching SQLite BINARY collation; preserve the comparator’s negative, zero, and positive return 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/package-storage-audit/service.ts`:
- Around line 152-181: Update parseStartAfterCursor to throw an exported
InvalidStartAfterCursorError for all caller-supplied cursor validation failures
instead of a plain Error. In
packages/worker/src/package-storage-audit/service.ts lines 152-181, define and
use the distinguishable error type; in
packages/worker/src/app/handlers/admin-package-storage-audit.ts lines 26-32,
catch that type around buildPackageStorageAuditReport and return a 400 JSON
response while rethrowing all other errors.
---
Nitpick comments:
In `@packages/worker/src/package-storage-audit/service.node.test.ts`:
- Around line 42-49: Update comparePackageKeys to use plain lexicographic < and
> comparisons for userId, then packageId when userId matches, matching SQLite
BINARY collation; preserve the comparator’s negative, zero, and positive return
contract.
In `@packages/worker/src/package-storage-audit/service.ts`:
- Around line 204-237: Optionally consolidate the duplicated SQL in
listAppPackagesForAudit by defining one shared SELECT/ORDER BY query, adding the
keyset WHERE fragment only when input.startAfter is present, and building the
bind parameters accordingly. Preserve the existing ordering, cursor predicate,
and input.limit + 1 truncation behavior.
- Around line 183-202: The withDeadline helper only bounds waiting while the
underlying operation continues running; choose and document the intended
semantics, and if resource cleanup is required, propagate an AbortSignal through
the callers and cancellable getEstimatedBytes/source-load operations so timeout
aborts them. Otherwise add a clear comment that the helper intentionally stops
awaiting without cancelling the work.
🪄 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: bf1556e1-dc8a-4b25-820c-7c52fbebbe9b
📒 Files selected for processing (6)
packages/worker/src/app/handlers/admin-package-storage-audit.node.test.tspackages/worker/src/app/handlers/admin-package-storage-audit.tspackages/worker/src/mcp/capabilities/admin/admin-package-storage-audit.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-package-storage-audit.tspackages/worker/src/package-storage-audit/service.node.test.tspackages/worker/src/package-storage-audit/service.ts
Typed InvalidStartAfterCursorError so the admin route answers 400 and the MCP capability raises McpCallerError instead of both surfacing internal 500s for caller-supplied bad cursors.
3 participants
Summary
Production hardening for the audit shipped in #1032: on prod the audit never completed — even
limit: 8hit the 180s stale-run TTL. Root cause: per-package work had no deadline;loadPackageSourceBySourceIdcan hang on Artifacts fetches, a hanging promise never trips the per-package try/catch, and the chunk'sPromise.allwedged forever.timed out after Nmserror row instead of blocking; the report always completes. The losing promise keeps running in the background (uncancellable fetch) — the report just stops waiting for it.startAftercursor ({userId, packageId}JSON) over the(user_id, id)ordering,nextStartAfterreturned when truncated — threaded through the admin route (?startAfter=) and the MCP capability (start_after), so large installs are walkable in small reliable slices.Testing
npm run validateintent: full loop runs in CI (this repo's CI mirrors it); targeted suites green locally: service (hanging probe/scan completes with timeout error rows; two-page cursor walk), route param passthrough, capability schemanpm run typecheckgreenRefs #1024
System recap — composes existing primitives (low risk)
Mode: recap · Base:
main@bb9491ed· Head:726b849dClassification: composes — deadline/pagination hardening inside the existing audit service and its two thin surfaces. No schema changes, no new primitives.
Primitives touched
capability-registrystart_after/nextStartAfteron the audit capabilityapp-ui/ admin?startAfter=passthrough on the audit routesaved-packagessaved_packagesqueryInvariants
Read-only audit stays admin-gated on both surfaces; per-user DO probing unchanged. Deadlines only shorten how long the report waits — they add no new access.
Summary by CodeRabbit
New Features
Bug Fixes