fix: close the remaining Cloudflare platform-limit gaps - #398
Merged
Conversation
A `.global()` insert returns before `onWrite`, so the global branch charges the transaction meter itself — but it charged after `global.insert` had already resolved. That write lands in D1, and a mutation's rollback is `state.storage.transaction`, which rewinds the Durable Object's SQLite and nothing else. So a ceiling breach threw with the D1 row committed and no way to undo it: a rollback that silently kept half the write, and with `insertMany`, every row before the offending one. Charging first makes the breach refuse the write instead of following it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
SQLite parses `a AND b AND c` left-deep — one expression-tree node per clause — and Workerd caps SQLITE_LIMIT_EXPR_DEPTH at 100 where stock SQLite allows 1,000. A `where` assembled programmatically reaches that in a way no hand-written predicate would: a filter builder over a wide form, an RLS policy merged into a caller's predicate, an OR across a long id list. Past it the statement fails to parse; it does not run slowly. Confirmed against a real SQLite with the limit lowered: 200 flat terms raise "Expression tree is too large", the same 200 balanced parse fine. `joinClauses` now splits the chain in half recursively, so 200 clauses nest 8 deep instead of 200. AND and OR are associative under SQL's three-valued logic — `(a AND b) AND c` and `a AND (b AND c)` agree on true, false and NULL alike — so the regrouping cannot change what matches, and the left-to-right walk keeps bound parameters in the same order. The rendered SQL changes shape, so three emitted-SQL assertions move with it (including the pagination seek predicate, whose parameter order is unchanged). Each now says why the shape is what it is, so it does not get "tidied" back to flat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
Every builder in this package sizes its own statement — compounds nest, long `IN` lists bind one JSON parameter, batch INSERTs chunk, clause chains balance. Nothing checked the result, so a builder regression or a hand-written statement reached SQLite as a bare `SQLITE_ERROR` from prepare, naming neither the limit nor the statement. `runSql` is the one place every statement passes through, and both ceilings are readable there from values already in hand: the rendered text's length and the parameter count. O(1) each, so the hot path pays nothing. Deliberately a backstop and not the defence: it fires only when something upstream already went wrong, which is exactly when a clear message is worth most. Existing suites pass unchanged, which is its own evidence the builders stay under both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
…ings A cross-shard read issues one Durable Object RPC per shard. Those are in-house subrequests, so they do not consume the six external connection slots — but they do count against the per-invocation subrequest ceiling (1,000 on Free; on Paid, the configured limit, default 10,000), and that ceiling is reached by breadth alone. Bounded concurrency does not help: it paces the fan-out, it does not shrink it. `fan_out_breadth` warns at 500 shards. Not a hard cap, deliberately — the real ceiling depends on the account's plan and configured limit, so refusing at a fixed number would break deployments that work today. It reads the shard count from the `shardTraffic` feeder `hot_shard` already consumes, so it costs no extra cross-shard work, and it counts per function group because the ceiling is per invocation rather than per deployment. The Limits page claimed the runtime warns when a DO crosses 1 GB, sustains >700 req/s, or fans out over 100 shards. None of the three exists anywhere in the codebase — not in the runtime, the observability layer, the Vite overlay, or the Studio. That section now describes the advisor lints that do exist and the runway each leaves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
In the default mode `defineRag` stores each chunk's text as the vector's metadata, and Vectorize caps that at 10 KiB. `chunkSize` is caller-configurable and was unchecked, so a value above the cap meant every upsert failed at the far side with nothing naming `chunkSize` as the cause. The ceiling was already written down in the package's own docblock — documented, not enforced. Checked once when the RAG is defined rather than on every write, and skipped entirely when a `textStore` moves chunk text out of metadata, which is what that option is for. Compared in characters against a byte ceiling deliberately: one character is at least one byte, so this rejects only sizes that could not fit even in pure ASCII, and never a config that works. Also documents Pipelines (5 MB per ingestion call, which `ctx.pipelines.send` forwards as one call) and the Vectorize ceilings on the Limits page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
The `defineRag` test did not type-check — `RagTextStore` is `{ getMany, put }`
and the stub supplied `get`. Vitest does not type-check, so the suite was green
while `lint:types` was red. That is the second time this branch's tests passed
without the gate; the test was written after the last typecheck ran.
`joinClauses` had three branches that earned nothing and one that was dangerous.
The `clauses.length === 1 && first` guard made an unreachable state recurse
forever rather than return — falsy `first` fell through to a split that recursed
on the same array. The `length === 2` case rendered exactly what the general one
does, and `if (!left || !right)` was provably dead, since both halves of a split
are non-empty. Two branches now, and the depth argument moves into the docblock.
The expression-depth test asserted paren nesting, which the flat form it
replaced never exceeded — it passed on the implementation it was meant to guard.
It now measures terms per level, mirroring `widestCompound` in the same file,
and fails on a flat `sql.join`. A behavioural case runs 200 AND'd conditions
through real SQLite.
`fan_out_breadth` claimed to count one invocation's fan-out from a signal that
carries neither. The Studio feeder sets no group at all, so every shard collapsed
into one bucket, and idle shards counted — so a healthy 600-tenant app whose
every read is shard-pinned got a permanent WARN telling it to narrow a read it
never made. It now measures shard-set *capacity*, says so, and filters idle
shards the way `hot_shard` does.
`defineRag` rejected `chunkSize` even when a custom `chunk` splitter made it
inert, and tested `=== undefined` where the rest of the function uses truthiness.
The `insertMany` global branch charged per row inside the write loop, so a breach
still left every earlier row committed in D1 — the comment claimed otherwise. It
now charges the whole batch up front, like the shard-local branch beside it, and
the single-insert comment states the trade the reorder makes.
Also: `MAX_BOUND_PARAMS` duplicated `WORKERD_SQLITE_LIMITS.boundParams` in the
same package, whose docblock exists to stop exactly that; the backstop's three
throw paths had no tests; the Limits page contradicted itself on subrequests
(50 external vs 1,000 internal); and both canonical lint tables were missing two
runtime lints.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
✅ Deploy Preview for lunorash ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
|
Thank you for confirming the Contributor License Agreement! 🙏 |
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
WalkthroughThe pull request adds a fan-out advisor lint, documents platform limits, validates Vectorize metadata sizing, enforces SQLite limits, centralizes write metering, balances generated predicates, and simplifies reprojection backfill queries. ChangesRuntime advisor lints
Vectorize metadata validation
Shard-engine limit enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/shard-engine/src/where-sql.ts (1)
211-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the empty-array case explicit.
compileNodecallsjoinClausesat Line 280 without an empty-array check. The currentclauses.length <= 1guard returnsundefined, so runtime behavior is safe. Split the zero- and one-clause cases explicitly to document this contract and prevent a future midpoint change from recursing forever on[].Proposed fix
- // Both halves of a split are non-empty and strictly smaller, so the - // recursion always reaches this case. - if (clauses.length <= 1) { + if (clauses.length === 0) { + return undefined; + } + if (clauses.length === 1) { return clauses[0]; }🤖 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/shard-engine/src/where-sql.ts` around lines 211 - 215, Update joinClauses to handle clauses.length === 0 explicitly before the single-clause case, returning the established empty-result value for empty arrays, then retain clauses.length === 1 returning clauses[0]. Keep the existing recursive split behavior unchanged for multiple clauses and preserve compileNode’s contract when it calls joinClauses without an empty-array check.
🤖 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 `@apps/docs/src/content/docs/limits.mdx`:
- Around line 80-81: Update the external “Subrequests” row in the limits table
to describe 10,000 as the Paid default and state that the configured Paid limit
applies, supporting configuration up to 10 million while preserving the Free
limit and existing fetch() description.
- Around line 120-122: Update the limits documentation to scope the
metadata-ceiling `chunkSize` validation to configurations where Lunora stores
chunk text in Vectorize metadata; clarify that configuring `textStore` removes
this validation because chunk text is stored externally.
- Around line 117-118: Update the limits table entry for “topK with full
metadata” to reflect a maximum of 50 when returnMetadata is "all"; retain the
100 limit only for queries that omit full metadata and vector values.
In `@packages/ai/src/rag/define-rag.ts`:
- Around line 33-40: Update the Vectorize metadata-size validation and its
configuration comment to measure the complete per-vector metadata payload,
including chunk text, metadata(), input.metadata, sourceId, chunkIndex, and
chunk-zero bookkeeping, rather than text alone. Account for UTF-8 byte length
and ensure chunkSize=10,240 cannot overflow the 10 KiB limit; preserve the
unrestricted textStore path. Add boundary coverage for ASCII, multibyte text,
caller metadata, and textStore behavior.
In `@packages/shard-engine/src/ctx-db.ts`:
- Around line 3364-3368: Update both global write paths in
packages/shard-engine/src/ctx-db.ts: at lines 3364-3368, charge the global
writer’s normalized single-row estimate before global.insert; at lines
3480-3483, charge normalized estimates for the full batch before the D1 loop.
Use the existing normalization and accounting mechanisms so writtenBytes
reflects defaults and metadata and enforces the transaction ceiling.
- Around line 3474-3485: The insertMany global-table path must precharge every
document before invoking the first writer.insert call. Update the insertMany
flow to record headroom for the entire documents batch ahead of sequential
writes, while preserving meterExempt behavior and avoiding duplicate precharging
that could overcount usage.
In `@packages/shard-engine/src/do-exec.ts`:
- Around line 30-34: Update runSql’s SQL length validation to measure query text
with TextEncoder().encode(query).byteLength instead of query.length, and report
the byte count in the LunoraError while preserving the existing limit check.
Update the sqlTextLength documentation in drizzle.ts to state that the limit is
measured in bytes.
---
Nitpick comments:
In `@packages/shard-engine/src/where-sql.ts`:
- Around line 211-215: Update joinClauses to handle clauses.length === 0
explicitly before the single-clause case, returning the established empty-result
value for empty arrays, then retain clauses.length === 1 returning clauses[0].
Keep the existing recursive split behavior unchanged for multiple clauses and
preserve compileNode’s contract when it calls joinClauses without an empty-array
check.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8b6567b-3dec-4a9e-97f8-5cfaf5d6e057
⛔ Files ignored due to path filters (6)
api-snapshots/advisor.api.mdis excluded by none and included by nonepackages/advisor/__tests__/runtime-lints.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/ai/__tests__/rag.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/shard-engine/__tests__/query-args.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/shard-engine/__tests__/where-sql.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/shard-engine/__tests__/workerd-sql-limits.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**
📒 Files selected for processing (9)
apps/docs/src/content/docs/concepts/advisors.mdxapps/docs/src/content/docs/limits.mdxpackages/advisor/src/index.tspackages/advisor/src/lints/runtime/fan-out-breadth.tspackages/ai/src/rag/define-rag.tspackages/shard-engine/src/ctx-db.tspackages/shard-engine/src/do-exec.tspackages/shard-engine/src/drizzle.tspackages/shard-engine/src/where-sql.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #398 +/- ##
==========================================
+ Coverage 87.09% 87.17% +0.08%
==========================================
Files 1172 1183 +11
Lines 63383 64154 +771
Branches 15447 15734 +287
==========================================
+ Hits 55202 55927 +725
- Misses 7654 7700 +46
Partials 527 527
🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 18.02%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 1 shard × 1000 rows (single round-trip) |
3.2 ms | 2.7 ms | +18.02% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/remaining-platform-limits (b2000c9) with alpha (424b60d)
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
The lint task runs `eslint . --max-warnings=0`, so `vitest/prefer-expect-assertions` fails the build at warning severity. Both tests added in this branch were missing `expect.assertions(...)`. They passed locally because I filtered eslint output for `error`, which is exactly the class that does not fail this gate — the check to run is the package's own `lint:eslint`, and to read its exit code rather than its text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
* fix(shard-engine): close the two deferred platform-limit holes **The re-projection scan could not run on a wide table.** `legacyRowPredicate` bound five parameters per reprojectable column and OR'd one clause per column, so a table with 21 `v.bigint()`/`v.bytes()` columns exceeded Workerd's 100-parameter cap — and its 100-term OR chain would have hit the expression-depth ceiling on the way. Both `countLegacyRows` (the `--dry-run` figure) and `isLegacyRow` were unusable there. The field list now rides in as one JSON parameter walked by `json_each`, the same shape `sqliteInList` uses: four parameters and one `EXISTS`, whatever the column count. `json_each` yields paths already quoted by `jsonPathSegment`, so appending `[0]`/`[1]` keeps a field literally named `a.b` resolving to itself. **A `.global()` patch or replace consumed no ceiling at all.** Both fall back to the D1 writer and return before `onWrite`, where the meter normally charges — so a mutation could rewrite a global table without ever touching its budget, the exact hole the `insert` branch's comment describes. Both now charge before crossing the boundary, since a D1 write commits where the DO's transaction cannot roll it back. `patch` charges the delta rather than the merged row: the row lives in D1 and reading it back to size it would double the round-trips on every global patch. That meters a global patch lighter than the shard-local path, which charges the whole merged document — under-counting by the untouched fields is the right side of that trade, since the delta is what the call actually sends. `replace` has the whole document in hand, so its charge is exact. Both regressions are pinned by tests confirmed to fail without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP * fix(shard-engine): meter the global delete, and name the metering gate Review found the third sibling still open. `delete`'s `.global()` fallback has the same shape as the `patch` and `replace` branches this branch fixed — no located row, no pinned table, delegate to D1 and return before `onWrite` — and it charged nothing. `deleteWhere` over a `.global()` table routes through `deleteMany` to it, so a whole batch was free of the meter however many rows it removed. It now charges a row and no bytes, which is what `onWrite` charges for the local delete paths. The reason it was missed is the reason it is now hard to miss again: `if (!meterExempt) { headroom?.recordWrite(x) }` was copy-pasted at six sites across 700 lines, so "did this branch remember?" was invisible. One named `meterWrite` makes an absence an obviously-missing line. Also from review: the metering test moves to `ctx-db.headroom.test.ts`, whose header describes exactly this ("a runaway read or write through the real `ctx.db` must be stopped with an attributable error"), and asserts the error CODE via the file's `codeOf` helper. Its old assertion — `toThrow(/TRANSACTION_LIMIT_EXCEEDED|limit/)` — could never match on the first alternative, since `toThrow` reads the message, so it was really `/limit/` and would have passed on any error carrying that word. It now covers all three branches. The workerd suite pins the re-projection predicate's shape directly. That file's docblock claimed `json_each` had one use in the repo and "nothing else would notice if that changed"; there are two shapes now, and authorization is per function rather than per query, so the correlated `EXISTS` with a `||`-computed path is pinned separately — it is the stricter of the two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP * refactor(shard-engine): walk the document instead of building a path per field `legacyRowPredicate` kept the frame it inherited — build a JSON path per field, extract at that path — and only changed how the paths were delivered. The frame was what cost. `json_each` over the document walks the row's own top-level members, where the key IS the field name, so there is no path to build. That deletes the `jsonPathSegment` dependency from this module, the path mapping, the `|| '[0]'` concatenation, and the docblock paragraphs that existed only to explain the JSON-path grammar. A field literally named `a.b` — or one carrying a quote, a bracket, or an emoji — is now compared as a plain string and cannot re-parse as a nested key. The statement stays four bound parameters wide whatever the column count, which was the point of the previous change. `type = 'array'` is load-bearing, not an optimisation: `json_each` hands back a scalar member's raw SQL text, and `json_extract('abc', '$[0]')` is a malformed-JSON error rather than NULL. It also preserves the exclusion the whole predicate turns on — a current projection is a JSON string, so it never reaches the element tests, and a tagged `Date` under `v.any()` still fails on the kind. Equivalence was measured, not argued: old and new agree on every cell of 21 document shapes against 9 field-name sets, including quotes, brackets, backslashes, unicode and the empty name. The workerd test moves with it — the shape it pins is the one the engine now emits, and `json_each`'s `type` and `key` columns over a column (rather than a bound literal) are the parts an allowlist could plausibly treat differently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
**`insertMany` could half-commit a global batch.** It loops the single-row `insert`, which charges its own row immediately before pushing to D1 — so a ceiling breach part-way down left every earlier row committed in a backend the DO's transaction cannot roll back. The whole batch is charged up front now, the way `insertManyUnsafe`'s global branch already was, and the loop runs through `runUnmetered` so no row is counted twice. Shard-local batches are untouched: the DO transaction rewinds them, so there is nothing to pre-empt. **The statement-length backstop measured the wrong unit.** `SQLITE_LIMIT_SQL_LENGTH` counts bytes; `String.length` counts UTF-16 units, so a multi-byte statement could pass and still be refused by SQLite. One UTF-16 unit is at most three UTF-8 bytes, so anything under a third of the limit cannot breach it — that comparison stays the whole cost for every statement this package emits, and only the remainder pays for an encode. **`defineRag`'s ceiling covers the whole metadata object**, not the chunk text alone: the source id, chunk index, chunk #0's bookkeeping, and any `metadata` the caller attaches all live in the same 10 KiB. Spending the entire ceiling on text was provably wrong, so the check now reserves room for the rest. The reserve is a generous guess by necessity — the caller's `metadata` is unknown when the RAG is defined — and the guard remains a floor, not a guarantee. Docs, all three factual: Paid external subrequests are configurable to 10M rather than fixed at 10,000; Vectorize's full-metadata `topK` is 50, where 20 is the legacy-V1 number `defineRag` still encodes (flagged rather than changed, a cap being a behaviour change); and the `chunkSize` check is scoped to the case where Lunora actually stores chunk text in metadata. Not taken: charging the global writer's normalized row rather than the caller's input. The meter bounds THIS isolate's memory, and normalization happens in D1 — the caller's document is what this isolate holds, so it is the honest figure. Same reasoning the `patch` delta already documents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ai/src/rag/define-rag.ts (1)
34-52:⚠️ Potential issue | 🟠 MajorValidate the complete metadata payload at index time.
The 2 KiB reserve is only a heuristic. Line [279] accepts
chunkSize === 8_192, but a built-in chunk containing8_192écharacters is16_384UTF-8 bytes before metadata overhead. The check also skipstextStoreconfigurations, althoughinput.metadataand bookkeeping fields still enter the object at Lines [468-491]. A customchunkis similarly unchecked.An oversized payload can then fail at
context.vectors.upsertaftertextStore.putorlexicalStore.indexhas already run at Lines [446-464]. Measure every final metadata object with the exact UTF-8 representation sent to Vectorize before any side-store write. Include custom chunks andinput.metadata. Keep the configuration check only as early feedback. Also change “lifts the constraint entirely”:textStoreremoves onlyTEXT_KEY, not the complete per-vector metadata limit.Cloudflare’s limits page, updated August 5, 2026, lists metadata per vector as 10 KiB. (developers.cloudflare.com)
As per path instructions,
packages/**/src/**/*.tsmust verify error handling and follow project patterns.Verification
#!/usr/bin/env bash set -euo pipefail rg -n -C 8 \ 'VECTORIZE_METADATA_BYTES|METADATA_OVERHEAD_RESERVE|input\.metadata|TEXT_KEY|textStore\.put|lexicalStore\.index|vectors\.upsert' \ packages/ai/src/rag/define-rag.ts python - <<'PY' text = "é" * 8192 assert len(text.encode("utf-8")) == 16384 assert len(text.encode("utf-8")) > 10 * 1024 print("Multibyte boundary exceeds the Vectorize limit before metadata overhead.") PYAlso applies to: 267-282
🤖 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/ai/src/rag/define-rag.ts` around lines 34 - 52, Replace the heuristic metadata reserve with exact validation of every final Vectorize metadata object in the indexing flow, including built-in and custom chunks, input.metadata, and textStore configurations; measure the UTF-8 byte length before textStore.put or lexicalStore.index and fail through the project’s established error-handling pattern before any side-store writes or vectors.upsert. Retain the existing configuration check only as early feedback, and update its documentation to state that textStore removes TEXT_KEY but does not remove the complete per-vector metadata limit. Anchor the changes around VECTORIZE_METADATA_BYTES, the chunk/index path near textStore.put and lexicalStore.index, and the final metadata construction using TEXT_KEY.Source: Path instructions
🤖 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/shard-engine/src/ctx-db.ts`:
- Around line 3627-3628: Replace the runUnmetered wrapper in insertOne with a
local, per-operation metering bypass so concurrent operations sharing the writer
remain metered. Extract the global forwarding and broadcast logic into an
internal helper that accepts whether its own charge should be skipped, and
invoke it from insertOne with the precharged case while preserving normal
metering for other writer operations.
---
Duplicate comments:
In `@packages/ai/src/rag/define-rag.ts`:
- Around line 34-52: Replace the heuristic metadata reserve with exact
validation of every final Vectorize metadata object in the indexing flow,
including built-in and custom chunks, input.metadata, and textStore
configurations; measure the UTF-8 byte length before textStore.put or
lexicalStore.index and fail through the project’s established error-handling
pattern before any side-store writes or vectors.upsert. Retain the existing
configuration check only as early feedback, and update its documentation to
state that textStore removes TEXT_KEY but does not remove the complete
per-vector metadata limit. Anchor the changes around VECTORIZE_METADATA_BYTES,
the chunk/index path near textStore.put and lexicalStore.index, and the final
metadata construction using TEXT_KEY.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c3c8146-0b45-4b0d-81df-72660727a075
⛔ Files ignored due to path filters (2)
packages/ai/__tests__/rag.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/shard-engine/__tests__/workerd-sql-limits.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**
📒 Files selected for processing (5)
apps/docs/src/content/docs/limits.mdxpackages/ai/src/rag/define-rag.tspackages/shard-engine/src/ctx-db.tspackages/shard-engine/src/do-exec.tspackages/shard-engine/src/drizzle.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shard-engine/src/drizzle.ts
- apps/docs/src/content/docs/limits.mdx
- packages/shard-engine/src/do-exec.ts
`insertMany` suppressed the per-row charge with `runUnmetered`, which sets a
writer-wide flag for the duration of its await. Anything interleaved into
that await on the same writer — a routine
`Promise.all([ctx.db.insertMany("<global>", rows), ctx.db.insert(…)])` —
skipped `meterWrite` too, local writes included. `deleteAll` accepts that
trade for an unusual pattern; a global `insertMany` is ordinary.
The global half of `insert` moves into an `insertGlobal` helper that takes
the charge as a parameter, so the batch suppresses only its own rows and a
concurrent write stays metered. `runUnmetered` is back to `deleteAll` as its
sole caller, which is what its own docblock already claimed.
Also record what the global charge does not cover: the caller's document,
not the row D1 stores, so `writtenBytes` runs light by whatever the far-side
defaults add. `writtenRows` — the ceiling that protects the isolate — is
exact.
Vectorize metadata is now measured where it is assembled. The config-time
`chunkSize` check compares CHARACTERS against a byte ceiling, so ~3.4k
characters of CJK exceed 10 KiB at a `chunkSize` it waves through, and the
caller's own `metadata` is not known until index time — the overhead reserve
can only guess at it. Both reached Vectorize and failed there with nothing
naming the cause, which is the failure the check exists to replace.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
12 tasks
Contributor
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 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.
Closes the platform-limit gaps left open by #396 and #397, and finishes the audit of every package that calls a Cloudflare API.
Correctness
The meter charged after the cross-backend write. A
.global()insert returns beforeonWrite, so the global branch charges the transaction meter itself — but it charged afterglobal.insertresolved. That row lands in D1, and a mutation's rollback isstate.storage.transaction, which rewinds the Durable Object's SQLite and nothing else. A ceiling breach therefore threw with the D1 row committed and no way to undo it.insertManynow charges the whole batch up front, the way its shard-local sibling already did, so a breach is found while none of the rows exist.Expression depth. SQLite parses
a AND b AND cleft-deep — one expression-tree node per clause — against Workerd'sSQLITE_LIMIT_EXPR_DEPTHof 100, where stock SQLite allows 1,000. Awhereassembled programmatically reaches that in a way no hand-written predicate would: a filter builder over a wide form, an RLS policy merged into a caller's predicate, anORacross a long id list. Verified against a real SQLite with the limit lowered — 200 flat terms raiseExpression tree is too large, the same 200 balanced parse fine.joinClausesnow splits in half, so 200 clauses nest 8 deep instead of 200.AND/ORare associative under three-valued logic, so regrouping cannot change what matches, and the left half always comes first so bound parameters number exactly as before. SQLite's ownwhereSplitrecurses into both children of anAND, so the planner decomposes a balanced tree into the same term set — index selection is unaffected.Guards
Statement backstop.
runSqlis the one place every statement passes through; it now rejects text over 100,000 characters or 100 bound parameters, both O(1) reads on values already in hand. Deliberately a backstop rather than the defence — every builder sizes its own statement, so this fires only when one regressed or a caller hand-wrote SQL, which is exactly when a message naming the limit beats a bareSQLITE_ERRORfrom prepare.Vectorize metadata.
defineRagstores each chunk's text as vector metadata unless atextStoreis supplied, and Vectorize caps that at 10 KiB.chunkSizewas unchecked, so a value above the cap meant every upsert failed at the far side with nothing naming the cause. The ceiling was already in the package's own docblock — documented, not enforced. Skipped when a customchunksplitter makeschunkSizeinert, and when atextStoremoves text out.Visibility
fan_out_breadthwarns when a shard group has enough active shards that a cross-shard read over it would approach the per-invocation internal-subrequest ceiling. It measures capacity, not observed fan-out, and says so — theshardTrafficfeeder reports live shards, not what any one invocation touched. Not a hard cap either: the real ceiling is 1,000 on Free but the configured limit (default 10,000) on Paid, so refusing at a fixed number would break deployments that work today.The Limits page gains Pipelines and Workers AI/Vectorize sections, and its subrequest row — which said
50 (Free) / 1000 (Paid)— now separates external from internal, since the internal ceiling is the one a cross-shard read spends.Audit result
Complete against the canonical binding list in
@lunora/platform: D1/DO SQL, KV, R2, Analytics Engine, Vectorize, Queues all have an owner for every documented ceiling. Verified-correct-as-is:browser,notify(its Web Push 4 KB is a browser-vendor limit, not Cloudflare),mail,images,container,observability, andagent— whose 8 MB utterance cap bounds DO memory on a buffer that is never stored, so it does not meet the 2 MB row ceiling.Left to the platform, now documented: Queues byte caps and Pipelines' 5 MB ingestion call. Measuring either means serializing every payload a second time on a send path, and both are rejected clearly by Cloudflare.
Known, not addressed
legacyRowPredicateinreprojection-backfill.tsbinds 5 parameters per field unchunked, so a table with 21+v.bigint()/v.bytes()columns exceeds the parameter cap. Pre-existing and unreachable on Cloudflare before this branch too (it already failed); the new backstop now names it instead of letting SQLite do so. It wants its own change — chunking anOR-across-fields scan changes what the query means.The global
patch/patchManybranches still call norecordWriteat all, so the "unbounded rows to a.global()table" hole the insert comment describes remains open for patches. Pre-existing, untouched here.Testing
Both thermo review passes ran against this branch and their findings are folded in — including one that would have shipped CI red (a test that passed under Vitest while failing
lint:types, since Vitest does not type-check) and one lint that would have fired a permanent false WARN on any app with 500+ tenants.The depth test now measures terms-per-level rather than paren nesting, and was confirmed to fail against a flat
sql.join— the previous version passed on the implementation it was meant to guard.Green: shard-engine 1101, advisor 475, ai 124, sql-store 108, do 530, runtime 898;
api:checkmatches all 47 snapshots; eslint, prettier andlint:typesclean on every touched package.🤖 Generated with Claude Code
https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
Summary by CodeRabbit
New Features
Bug Fixes
Documentation