Repository navigation
Scale hardening: R2 email blobs, derived usage rollups, bounded cron/DO work, cheap search paths - #669
Merged
Conversation
Store inbound raw MIME in R2 (email-raw:v1:{userId}/{messageId}) with
raw_mime_key on email_messages, falling back inline when the binding is
missing. Reads go through loadRawMime, deletes remove the blob, account
deletion enumerates and purges user blobs, and CI resource tooling
provisions the bucket.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
recordUsage writes only the AE data point when USAGE_EVENTS is bound; usage_rollups become an hourly derived aggregate (aggregateUsageRollups over the AE SQL API) with a cachified KV read cache on the admin usage pages. The stable-user-id scan fallback now writes back healed ids and a maintenance endpoint backfills legacy rows. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Hourly retention now drains batches round-robin under a 20s budget, adds policies for email_messages (365d, R2-blob aware), entitlement daily counters (400d) and usage_rollups (24mo), replaces the whole-table ROW_NUMBER prune with a bounded cap-pair pass, prunes stale source snapshot KV keys, batches system-email deletes, and stops writing the legacy retriever scope index blob. Adds supporting indexes. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Cap due jobs at 25 per JobManager alarm with a 1s backlog re-arm, cap runRepoChecks source walks at 2000 files / 15MiB with an actionable check failure, and give package service auto-start retries exponential backoff with jitter (5s doubling to 15min, persisted across eviction). Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Package search ranks D1 projections fused with the existing package vectors and hydrates full source only for returned matches; memory search queries Vectorize first and hydrates top hits by id; community search pre-filters in SQL and bounds candidates at 500. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Account export reads keyset-paged rowid windows (500 rows per query) instead of whole tables, and the memory/job/package reindex drivers process keyset-paged batches of 200 with merged result reporting. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Scheduled lanes log and report to Sentry independently instead of rethrowing, usage aggregation joins the cron, artifact reconcile loops batches under a 60s budget with an unchanged-HEAD fast path, the static capability registry builds lazily, and OG image rendering (satori + resvg wasm) loads dynamically. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Warning Review limit reached
Next review available in: 15 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 (11)
📝 WalkthroughWalkthroughThis PR adds R2-backed raw email MIME storage, expands retention pruning and usage rollups, converts several listings and reindex flows to keyset pagination, isolates scheduled cron lanes, adds stable-user-id healing/backfill, and updates supporting CI/config/docs. ChangesEmail raw MIME R2 storage
Retention scheduler and pruning
Usage metering and admin caching
Pagination, search, and lazy hydration
Stable user id healing and backfill
Scheduled cron, job alarms, and reconcile batching
Package runtime, repo checks, and manifest cache
Estimated code review effort: 5 (Critical) | ~150 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
July 8, 2026 07:28
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
If R2 listing or creation fails, deploys drop the EMAIL_BLOBS binding from the generated Wrangler config and the worker falls back to inline raw MIME storage, instead of failing the resource-ensure run. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
packages/worker/src/repo/checks.ts (1)
908-919: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueByte-cap error message hardcodes "15 MiB".
The message at line 918 hardcodes
(15 MiB)while the actual limit is derived fromrepoChecksSourceMaxTotalBytes. If the constant is ever changed, the message will be misleading. Consider computing the human-readable size from the constant, or remove the hardcoded unit.♻️ Suggested refactor
- message: `Repo checks aborted: source root "${sourceRoot || '/'}" exceeds the ${repoChecksSourceMaxTotalBytes}-byte (15 MiB) publish check limit. Remove or shrink large files that should not be published (for example build output, vendored dependencies, or data files) and run the checks again.`, + message: `Repo checks aborted: source root "${sourceRoot || '/'}" exceeds the ${repoChecksSourceMaxTotalBytes}-byte (${Math.round(repoChecksSourceMaxTotalBytes / (1024 * 1024))} MiB) publish check limit. Remove or shrink large files that should not be published (for example build output, vendored dependencies, or data files) and run the checks again.`,🤖 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/repo/checks.ts` around lines 908 - 919, The repo checks byte-cap error message in checks.ts hardcodes “15 MiB” even though the limit comes from repoChecksSourceMaxTotalBytes. Update the message in the totalBytes overflow branch to derive the human-readable size from that constant (or omit the fixed unit) so the text stays correct if the limit changes; use the existing check logic around repoChecksSourceMaxTotalBytes and the totalBytes comparison as the target location.packages/worker/src/package-retrievers/manifest-cache.node.test.ts (1)
266-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for refresh under a no-
list()binding.This test only reads a pre-seeded legacy index; it never calls
refreshPackageRetrieverManifestCachein the no-list()mode. That refresh path is exactly where the legacy index gets deleted while per-entry keys remain unreadable (see manifest-cache.ts Lines 429-434), so the current suite would not catch that regression. Consider a case that refreshes and then assertslistPackageRetrieversForScopestill resolves entries whenlistis unavailable.🤖 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-retrievers/manifest-cache.node.test.ts` around lines 266 - 339, The current no-list coverage in manifest-cache.node.test.ts only verifies reading a preseeded legacy index and misses the refresh path where the legacy index may be removed. Update the test around listPackageRetrieversForScope and refreshPackageRetrieverManifestCache to simulate a no-kv.list binding, run a refresh, and then assert the scope listing still returns the expected retriever entry. Use the existing helpers like createKv, legacyKey, and refreshPackageRetrieverManifestCache so the regression where refresh deletes the legacy index but entries are still unreadable is caught.packages/worker/src/usage/aggregate-rollups.ts (2)
115-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an explicit timeout to the Analytics Engine fetch call.
The
fetchto the Cloudflare SQL API has no timeout. A slow or hung response will consume the cron lane's entire wall-clock budget. Since lanes run viaPromise.allSettled, other lanes survive, but the usage aggregation lane would silently fail for that hour. AddingAbortSignal.timeoutbounds the damage and allows the next hourly run to catch up.⏱️ Proposed fix
const response = await fetch(url, { method: 'POST', headers: { authorization: `Bearer ${input.apiToken}`, }, body: input.query, + signal: AbortSignal.timeout(15_000), })🤖 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/usage/aggregate-rollups.ts` around lines 115 - 121, The Analytics Engine request in aggregate-rollups’s fetch call has no timeout, so a hung Cloudflare SQL API response can block the usage aggregation lane for the whole cron window. Update the fetch path in aggregate-rollups to use an explicit AbortSignal.timeout (or equivalent abort handling) for the request, and make sure the timeout value is applied alongside the existing authorization header and body so the lane fails fast and the next hourly run can recover.
172-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract
validRowsto avoid duplicating the filter logic.The
(row) => row.user_id && row.metricfilter is applied twice — once forstatementsand once forusers. Extracting avalidRowsvariable improves readability and ensures both paths stay in sync if the filter condition changes.♻️ Proposed refactor
- const statements = rows - .filter((row) => row.user_id && row.metric) - .map((row) => - env.APP_DB.prepare(usageRollupAbsoluteUpsertStatement).bind( - row.user_id, - row.metric, - month, - toCount(row.event_count), - toCount(row.error_count), - toCount(row.total_duration_ms), - toCount(row.total_cpu_ms), - toCount(row.total_bytes), - updatedAt, - ), - ) - const users = new Set( - rows.filter((row) => row.user_id && row.metric).map((row) => row.user_id), - ).size + const validRows = rows.filter((row) => row.user_id && row.metric) + const statements = validRows.map((row) => + env.APP_DB.prepare(usageRollupAbsoluteUpsertStatement).bind( + row.user_id, + row.metric, + month, + toCount(row.event_count), + toCount(row.error_count), + toCount(row.total_duration_ms), + toCount(row.total_cpu_ms), + toCount(row.total_bytes), + updatedAt, + ), + ) + const users = new Set(validRows.map((row) => row.user_id)).size🤖 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/usage/aggregate-rollups.ts` around lines 172 - 189, The filter logic for valid rollup rows is duplicated in aggregate-rollups handling, so extract a shared validRows variable before building statements and users. Use that single filtered collection in the statement generation and in the unique user count so the condition stays consistent and the code is easier to read; the relevant logic is in the rows processing inside the rollup aggregation flow.packages/worker/src/email/system-email.ts (1)
244-264: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider using
db.batch()for atomic chunked deletes.The three DELETE statements (attachments, delivery_events, messages) run as independent D1 operations. If the worker is interrupted between them, child records could be deleted while parent rows remain, leaving temporary inconsistency until the next retention run. Wrapping them in
db.batch()executes them as a single transaction with no additional code complexity.♻️ Proposed refactor
- await input.db - .prepare( - `DELETE FROM email_attachments - WHERE message_id IN (${placeholders})`, - ) - .bind(...messageIds) - .run() - await input.db - .prepare( - `DELETE FROM email_delivery_events - WHERE message_id IN (${placeholders})`, - ) - .bind(...messageIds) - .run() - await input.db - .prepare( - `DELETE FROM email_messages - WHERE id IN (${placeholders})`, - ) - .bind(...messageIds) - .run() + await input.db.batch([ + input.db + .prepare( + `DELETE FROM email_attachments + WHERE message_id IN (${placeholders})`, + ) + .bind(...messageIds), + input.db + .prepare( + `DELETE FROM email_delivery_events + WHERE message_id IN (${placeholders})`, + ) + .bind(...messageIds), + input.db + .prepare( + `DELETE FROM email_messages + WHERE id IN (${placeholders})`, + ) + .bind(...messageIds), + ])🤖 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/email/system-email.ts` around lines 244 - 264, The three DELETE operations for email attachments, delivery events, and email messages are currently executed as separate D1 calls, which can leave the data temporarily inconsistent if the worker stops midway. Update the deletion flow in the email retention logic that uses input.db.prepare(...).bind(...).run() to run those statements through db.batch() instead, so the attachment, delivery_events, and messages deletes commit atomically for each chunk. Keep the same messageIds placeholders and chunking behavior, just group the existing statements into one batched transaction.packages/worker/src/mcp/tools/search.ts (2)
929-989: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider wrapping
queryPackageVectorScoresin a fallback to deterministic scoring on failure.If
embedTextForVectorizeorindex.querythrows (e.g., AI binding rate limit, transient Vectorize outage), the entiresearchUnifiedcall fails. A try/catch that returnsnullon error would let the caller gracefully fall back tocosineSimilarity-based offline scoring, improving resilience without masking the issue (the error can still be logged).🛡️ Proposed fallback wrapper
async function queryPackageVectorScores(input: { env: Env query: string rows: Array<PackageSearchRow> limit: number }): Promise<Map<string, number> | null> { const index = getCapabilityVectorIndex(input.env) if (!index) return null + try { const userIds = Array.from( new Set(input.rows.map((row) => row.record.userId).filter(Boolean)), ) const recordIdByVectorId = new Map( input.rows.map( (row) => [savedPackageVectorId(row.record.id), row.record.id] as const, ), ) const queryVector = await embedTextForVectorize(input.env, input.query) const topK = Math.min(Math.max(input.rows.length, input.limit * 5), 100) const vectorMatches = await index.query(queryVector, { topK, returnMetadata: 'none', filter: { kind: { $eq: 'package' }, ...(userIds.length === 1 ? { userId: { $eq: userIds[0] } } : {}), }, }) const scores = new Map<string, number>() for (const match of vectorMatches.matches) { if (typeof match.id !== 'string') continue const recordId = recordIdByVectorId.get(match.id) if (!recordId || scores.has(recordId)) continue scores.set(recordId, match.score) } return scores + } catch (error) { + console.warn( + JSON.stringify({ + message: 'package vector score query failed, falling back to deterministic scoring', + error: error instanceof Error ? error.message : String(error), + }), + ) + return null + } }🤖 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/mcp/tools/search.ts` around lines 929 - 989, Wrap `queryPackageVectorScores` with a try/catch so failures from `embedTextForVectorize` or `index.query` return null and let `buildPackageCandidates` fall back to deterministic scoring. Keep the fallback behavior aligned with the existing offline path in `buildPackageCandidates`, and log the caught error with enough context before returning null.
1436-1443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
rerankingMsnow includes hydration I/O time.
hydrateTopPackageMatches(which loads package sources vialoadPackageSourceBySourceId) is called betweenrerankingStartand thererankingMscalculation. This metric no longer reflects pure reranking time, which could mislead performance analysis. Consider tracking hydration separately.⏱️ Proposed separate hydration timing
const rerankingStart = performance.now() const reranked = rerankCandidates({ candidates, intent, limit, }) const matches = reranked.map((candidate) => candidate.match) + const rerankingMs = Math.max(0, Math.round(performance.now() - rerankingStart)) + const hydrationStart = performance.now() await hydrateTopPackageMatches({ query: intent.normalizedQuery, matches, rows: input.optionalRows.packageRows, }) - const rerankingMs = Math.max( - 0, - Math.round(performance.now() - rerankingStart), - ) + const hydrationMs = Math.max(0, Math.round(performance.now() - hydrationStart))Then include
hydrationMsinphaseTimings(or fold it intocandidateGenerationMsif you prefer not to change the response shape).Also applies to: 1473-1477
🤖 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/mcp/tools/search.ts` around lines 1436 - 1443, Separate the hydration I/O from reranking timing in search.ts: `rerankingMs` is currently measured across `hydrateTopPackageMatches`/`loadPackageSourceBySourceId`, so update the timing in the `buildPackageCandidates` flow to measure pure reranking only and record hydration separately. Add a `hydrationMs` entry to `phaseTimings` (or move that cost into `candidateGenerationMs` if you want to avoid changing the response shape) and make sure the same adjustment is applied wherever `rerankingMs` is computed.
🤖 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.
Nitpick comments:
In `@packages/worker/src/email/system-email.ts`:
- Around line 244-264: The three DELETE operations for email attachments,
delivery events, and email messages are currently executed as separate D1 calls,
which can leave the data temporarily inconsistent if the worker stops midway.
Update the deletion flow in the email retention logic that uses
input.db.prepare(...).bind(...).run() to run those statements through db.batch()
instead, so the attachment, delivery_events, and messages deletes commit
atomically for each chunk. Keep the same messageIds placeholders and chunking
behavior, just group the existing statements into one batched transaction.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 929-989: Wrap `queryPackageVectorScores` with a try/catch so
failures from `embedTextForVectorize` or `index.query` return null and let
`buildPackageCandidates` fall back to deterministic scoring. Keep the fallback
behavior aligned with the existing offline path in `buildPackageCandidates`, and
log the caught error with enough context before returning null.
- Around line 1436-1443: Separate the hydration I/O from reranking timing in
search.ts: `rerankingMs` is currently measured across
`hydrateTopPackageMatches`/`loadPackageSourceBySourceId`, so update the timing
in the `buildPackageCandidates` flow to measure pure reranking only and record
hydration separately. Add a `hydrationMs` entry to `phaseTimings` (or move that
cost into `candidateGenerationMs` if you want to avoid changing the response
shape) and make sure the same adjustment is applied wherever `rerankingMs` is
computed.
In `@packages/worker/src/package-retrievers/manifest-cache.node.test.ts`:
- Around line 266-339: The current no-list coverage in
manifest-cache.node.test.ts only verifies reading a preseeded legacy index and
misses the refresh path where the legacy index may be removed. Update the test
around listPackageRetrieversForScope and refreshPackageRetrieverManifestCache to
simulate a no-kv.list binding, run a refresh, and then assert the scope listing
still returns the expected retriever entry. Use the existing helpers like
createKv, legacyKey, and refreshPackageRetrieverManifestCache so the regression
where refresh deletes the legacy index but entries are still unreadable is
caught.
In `@packages/worker/src/repo/checks.ts`:
- Around line 908-919: The repo checks byte-cap error message in checks.ts
hardcodes “15 MiB” even though the limit comes from
repoChecksSourceMaxTotalBytes. Update the message in the totalBytes overflow
branch to derive the human-readable size from that constant (or omit the fixed
unit) so the text stays correct if the limit changes; use the existing check
logic around repoChecksSourceMaxTotalBytes and the totalBytes comparison as the
target location.
In `@packages/worker/src/usage/aggregate-rollups.ts`:
- Around line 115-121: The Analytics Engine request in aggregate-rollups’s fetch
call has no timeout, so a hung Cloudflare SQL API response can block the usage
aggregation lane for the whole cron window. Update the fetch path in
aggregate-rollups to use an explicit AbortSignal.timeout (or equivalent abort
handling) for the request, and make sure the timeout value is applied alongside
the existing authorization header and body so the lane fails fast and the next
hourly run can recover.
- Around line 172-189: The filter logic for valid rollup rows is duplicated in
aggregate-rollups handling, so extract a shared validRows variable before
building statements and users. Use that single filtered collection in the
statement generation and in the unique user count so the condition stays
consistent and the code is easier to read; the relevant logic is in the rows
processing inside the rollup aggregation flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc7cafe8-c614-437e-b339-2d133505f6db
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (80)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/entitlements.mddocs/contributing/architecture/primitives.yamldocs/contributing/architecture/usage-metering.mdpackages/worker/migrations/0054-email-blob-storage.sqlpackages/worker/migrations/0055-retention-indexes.sqlpackages/worker/package.jsonpackages/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/admin-system-email-data.tspackages/worker/src/app/admin-usage-data.node.test.tspackages/worker/src/app/admin-usage-data.tspackages/worker/src/app/handlers/community-detail.tsxpackages/worker/src/app/retention.node.test.tspackages/worker/src/app/retention.tspackages/worker/src/community/community-scoring.node.test.tspackages/worker/src/community/community-service.node.test.tspackages/worker/src/community/repo.tspackages/worker/src/community/service.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/repo.tspackages/worker/src/email/system-email.tspackages/worker/src/email/system-email.workers.test.tspackages/worker/src/email/test-schema.tspackages/worker/src/email/types.tspackages/worker/src/entitlements/service.tspackages/worker/src/index.tspackages/worker/src/index.workers.test.tspackages/worker/src/jobs/job-reindex.node.test.tspackages/worker/src/jobs/job-reindex.tspackages/worker/src/jobs/manager-do.node.test.tspackages/worker/src/jobs/manager-do.tspackages/worker/src/jobs/reconcile-artifacts-pushes.node.test.tspackages/worker/src/jobs/reconcile-artifacts-pushes.tspackages/worker/src/jobs/repo.tspackages/worker/src/jobs/repo.workers.test.tspackages/worker/src/kv-cachified.tspackages/worker/src/maintenance-handler.node.test.tspackages/worker/src/maintenance-handler.tspackages/worker/src/mcp/capabilities/admin/admin-system-email-get.tspackages/worker/src/mcp/capabilities/email/email-attachment-get.tspackages/worker/src/mcp/capabilities/registry.node.test.tspackages/worker/src/mcp/capabilities/registry.tspackages/worker/src/mcp/capabilities/reindex-batches.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/memory/memory-reindex.node.test.tspackages/worker/src/mcp/memory/memory-reindex.tspackages/worker/src/mcp/memory/memory-search.tspackages/worker/src/mcp/memory/repo.tspackages/worker/src/mcp/memory/service.node.test.tspackages/worker/src/mcp/memory/service.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-invocations/service.tspackages/worker/src/package-registry/package-reindex.node.test.tspackages/worker/src/package-registry/package-reindex.tspackages/worker/src/package-registry/repo.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.tspackages/worker/src/package-runtime/package-service.node.test.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/repo/checks.node.test.tspackages/worker/src/repo/checks.tspackages/worker/src/usage/aggregate-rollups.node.test.tspackages/worker/src/usage/aggregate-rollups.tspackages/worker/src/usage/record-usage.tspackages/worker/src/usage/record-usage.workers.test.tspackages/worker/src/user-id.tspackages/worker/src/user-id.workers.test.tspackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonctools/ci/preview-resources.tstools/ci/production-resources.tstools/ci/resource-utils.node.test.tstools/ci/resource-utils.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
|
🔎 Preview deployed: https://kody-pr-669.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/ci/resource-utils.ts (1)
185-201: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffAny wrangler failure (not just missing permissions) silently drops blob offload.
Both the
createfailure (Line 188) and the post-create re-list failure (Line 195) returnavailable: false, which causeswriteGeneratedWranglerConfigto drop theEMAIL_BLOBSbinding and quietly fall back to inline MIME in D1. A transient network/API hiccup during a production deploy would therefore silently change the raw-email storage path without any hard failure — easy to miss and potentially costly (D1 row-size/cost implications).Consider scoping the fail-soft to genuine authz failures (e.g. inspect
createResult.stderrfor permission/authentication markers) and treating other failures as fatal, so unexpected errors surface instead of degrading silently.Separately, the re-list guard on Line 195 flips
available: falseeven after a successfulcreate, which can leave an orphaned bucket while the binding is dropped (it self-heals on the next run when the list succeeds). Worth a brief comment noting that intent.🤖 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 `@tools/ci/resource-utils.ts` around lines 185 - 201, The R2 bucket provisioning path in createR2BucketName is too broadly fail-soft, causing non-auth wrangler errors to silently disable blob offload. Update the createResult and post-create list checks to distinguish genuine permission/authentication failures from transient or unexpected errors by inspecting createResult.stderr (or equivalent error details), and only return available: false for the authz case; let other failures surface as fatal. Also add a short comment near the namesAfterCreate availability check to explain that it intentionally marks the bucket unavailable if the re-list cannot verify creation.
🤖 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.
Nitpick comments:
In `@tools/ci/resource-utils.ts`:
- Around line 185-201: The R2 bucket provisioning path in createR2BucketName is
too broadly fail-soft, causing non-auth wrangler errors to silently disable blob
offload. Update the createResult and post-create list checks to distinguish
genuine permission/authentication failures from transient or unexpected errors
by inspecting createResult.stderr (or equivalent error details), and only return
available: false for the authz case; let other failures surface as fatal. Also
add a short comment near the namesAfterCreate availability check to explain that
it intentionally marks the bucket unavailable if the re-list cannot verify
creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f15c3b9-b3b3-4bd7-9826-4a0a4eaeeaba
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
docs/contributing/architecture/primitives.yamlpackages/worker/package.jsonpackages/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/admin-system-email-data.tspackages/worker/src/app/admin-usage-data.tspackages/worker/src/app/handlers/community-detail.tsxpackages/worker/src/app/retention.node.test.tspackages/worker/src/community/service.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/repo.tspackages/worker/src/email/system-email.tspackages/worker/src/entitlements/service.tspackages/worker/src/index.tspackages/worker/src/jobs/reconcile-artifacts-pushes.tspackages/worker/src/maintenance-handler.tspackages/worker/src/mcp/capabilities/reindex-batches.tspackages/worker/src/mcp/memory/service.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-invocations/service.tspackages/worker/src/package-registry/package-reindex.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/repo/checks.tspackages/worker/src/user-id.tspackages/worker/wrangler.jsonctools/ci/preview-resources.tstools/ci/production-resources.tstools/ci/resource-utils.node.test.tstools/ci/resource-utils.ts
💤 Files with no reviewable changes (3)
- packages/worker/package.json
- packages/worker/wrangler.jsonc
- packages/worker/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/worker/src/email/outbound.workers.test.ts
- packages/worker/src/entitlements/service.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/worker/src/email/outbound.ts
- packages/worker/src/package-invocations/service.ts
- packages/worker/src/package-registry/package-reindex.ts
- packages/worker/src/repo/checks.ts
- tools/ci/production-resources.ts
- packages/worker/src/app/handlers/community-detail.tsx
- packages/worker/src/package-runtime/package-service.ts
- packages/worker/src/jobs/reconcile-artifacts-pushes.ts
- packages/worker/src/user-id.ts
- packages/worker/src/app/admin-system-email-data.ts
- tools/ci/preview-resources.ts
- packages/worker/src/app/account-export.node.test.ts
- docs/contributing/architecture/primitives.yaml
- packages/worker/src/mcp/memory/service.ts
- packages/worker/src/email/system-email.ts
- packages/worker/src/app/account-deletion.ts
- packages/worker/src/maintenance-handler.ts
- packages/worker/src/community/service.ts
- packages/worker/src/app/account-export.ts
- packages/worker/src/email/repo.ts
- packages/worker/src/app/admin-usage-data.ts
- tools/ci/resource-utils.node.test.ts
- packages/worker/src/app/account-deletion.node.test.ts
- packages/worker/src/mcp/tools/search.ts
- packages/worker/src/app/retention.node.test.ts
- packages/worker/src/mcp/capabilities/reindex-batches.ts
…allbacks Bound the usage aggregation query to the current UTC month on both sides and time out the AE fetch; delete system-email R2 blobs before their rows so failures can retry; run the community semantic fallback when the LIKE prefilter matched nothing; degrade package search to lexical scoring when the vector query fails; derive the repo-checks byte-cap message from its constant. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…dvancing prunes The hourly rollup aggregation now deletes stale current-month rows absent from the AE result; email and system-email retention advance a keyset cursor past blob-skipped rows so a blocked head cannot wedge the queue; retention hasMore is based on selected rather than deleted rows; artifact reconcile pages strictly forward within a tick even when cursor writes fail; trade-off comments document the lean package search projection and the community browse candidate bound. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
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 1df1f6c. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Jul 8, 2026
cursor Bot
pushed a commit
that referenced
this pull request
Jul 8, 2026
* Fix D1 SQLITE_BUSY lock contention from parallel cron lanes Scale hardening (#669) runs multiple D1 write-heavy scheduled lanes in parallel (retention, reconcile, usage aggregation, system-email retention). Those crons can contend for the same SQLite lock and surface as D1_ERROR: NOSENTRY database is locked: SQLITE_BUSY in Sentry. - Run scheduled cron lanes sequentially so they no longer hammer D1 at once - Add runD1WithRetry for transient SQLITE_BUSY with exponential backoff - Wrap retention D1 reads/writes with the retry helper Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * Format d1-retry.ts Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
This was referenced Jul 10, 2026
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the approved Tier 1 and Tier 2 fixes from the scaling audit.
What changed
D1 size & write amplification (Tier 1)
EMAIL_BLOBSR2 bucket (email-raw:v1:{userId}/{messageId}, migration0054); D1 keeps metadata plusraw_mime_key, with inline fallback when the binding is missing. Reads go throughloadRawMime, deletes remove the blob, account deletion purges user blobs, and CI resource tooling provisions the bucket.recordUsagewrites only the Analytics Engine data point in production;usage_rollupsis now a derived aggregate recomputed hourly from the AE SQL API (aggregateUsageRollups), with a@epic-web/cachifiedKV-backed read cache on the admin usage pages. Local dev without AE keeps the direct D1 upsert.Retention & reconcile throughput (Tier 1)
email_messages(365d, R2-blob aware),entitlement_daily_counters(400d),usage_rollups(24mo). The whole-tableROW_NUMBER()prune is replaced with a bounded cap-pair pass. Supporting indexes land in migration0055.Latent bombs & DO hot spots (Tier 2)
stable_user_id: the scan fallback now write-backs healed ids, andPOST /__maintenance/backfill-stable-user-idsbackfills legacy rows (keyset paged). Docs drift about inbound-email scanning corrected.runRepoCheckscaps source walks at 2,000 files / 15MiB with an actionable check failure; package service auto-start retries use exponential backoff with jitter (5s → 15min cap, persisted across eviction).satori+ resvg wasm) loads dynamically. Legacy retriever scope-index KV blobs are no longer written; stale retriever manifest revisions and source-snapshot KV keys are pruned.Notes for reviewers
email_messages.raw_mimestays readable for legacy rows; new inbound mail stores the blob key instead.USAGE_EVENTS+CLOUDFLARE_ACCOUNT_ID+CLOUDFLARE_API_TOKEN.POST /__maintenance/backfill-stable-user-idswith the maintenance bearer secret.System recap — adds a new primitive (high risk)
Mode: recap · Base:
main@27b28a4· Head:0a0a859Classification: adds — introduces the
email-blobs-r2storage primitive (new R2 bucket + frozen key contract); several existing primitives are extended.primitives.yamlis updated in this PR.Primitives touched
email-blobs-r2email-raw:v1:{userId}/{messageId}key contractemailusage-meteringscheduled-crond1-app-dbraw_mime_key) and 0055 (indexes)jobsrepo-sessionspackage-servicesmcp-servercommunity-listingsaccount-exportvectorize-searchbundle-artifacts-kvcapability-registryentitlementsSystem map
Before / after
email_messages.raw_mime(D1)raw_mime_key(inline fallback)recordUsage(prod)SELECT *whole tablesInvariants
per-user-isolation: preserved — R2 keys are user-prefixed (email-raw:v1:{userId}/...), package/memory vector queries filter byuserId, and account deletion covers the new blob surface.canonical-repo-source: untouched; snapshot KV pruning only removes non-current-commit derived artifacts.Summary by CodeRabbit
New Features
Improvements
Bug Fixes