fix(shard-engine): close the two deferred platform-limit holes - #399
Merged
prisis merged 3 commits intoAug 10, 2026
Conversation
✅ 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! 🙏 |
WalkthroughWrite metering is centralized and applied to normal, global-table, and batch write paths before database operations. Reprojection backfill now uses a fixed-parameter SQLite predicate to detect legacy wire-tagged values. ChangesShard engine updates
Estimated code review effort: 3 (Moderate) | ~20 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #399 +/- ##
==========================================
- Coverage 87.09% 85.81% -1.29%
==========================================
Files 1172 852 -320
Lines 63383 51418 -11965
Branches 15447 12655 -2792
==========================================
- Hits 55202 44123 -11079
+ Misses 7654 6874 -780
+ Partials 527 421 -106
🚀 New features to boost your workflow:
|
prisis
force-pushed
the
fix/deferred-limit-findings
branch
from
August 10, 2026 13:56
a0ba7f7 to
a060a04
Compare
Merging this PR will degrade performance by 11.06%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
**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
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
…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
prisis
force-pushed
the
fix/deferred-limit-findings
branch
from
August 10, 2026 14:52
25d9e35 to
02650f8
Compare
![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
🤖 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`:
- Line 3389: Update global insert, insertMany, and by-id replace metering to
pass the normalized persisted-row object to meterWrite rather than the raw
caller document. Reuse the object after D1 applies _id/_creationTime defaults
for inserts and the post-OCC replacement object for replace, matching
shard-local metering behavior.
🪄 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: 728f1702-760e-4f54-9855-8f8bd030b5fb
⛔ Files ignored due to path filters (3)
packages/do/__tests__/workerd/sql-limits.workerd.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/shard-engine/__tests__/ctx-db.headroom.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/shard-engine/__tests__/reprojection-backfill.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**
📒 Files selected for processing (2)
packages/shard-engine/src/ctx-db.tspackages/shard-engine/src/reprojection-backfill.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shard-engine/src/reprojection-backfill.ts
| if (!meterExempt) { | ||
| headroom?.recordWrite(document); | ||
| } | ||
| meterWrite(document); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/shard-engine/src/transaction-headroom.ts --items all
rg -n -C 10 '\brecordWrite\s*\(|writtenBytes|applyInsertDefaults|global\.(insert|replace)' packages/shard-engine/srcRepository: anolilab/lunora
Length of output: 23269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read and inspect the transaction headroom API and bytes estimator semantics.
sed -n '64,112p' packages/shard-engine/src/transaction-headroom.ts
sed -n '1,140p' packages/shard-engine/src/estimate-bytes.ts
# Map/inspect the surrounding implementation for the three global metering contexts.
awk '/function applyInsertDefaults|const applyInsertDefaults|const makeCtxDb|= makeGlobal|replace\(|\.global\(|estimateBytes/' packages/shard-engine/src/ctx-db.ts | head -80
sed -n '3360,3392p' packages/shard-engine/src/ctx-db.ts
sed -n '3490,3514p' packages/shard-engine/src/ctx-db.ts
sed -n '4048,4076p' packages/shard-engine/src/ctx-db.ts
# Search precise definitions/usages for globalDb and defaulting/creationTime in global writers.
rg -n -C 8 'globalDb|allowExplicitId|_creationTime|applyInsertDefaults|replace\(' packages/shard-engine/src/ctx-db.ts packages/shard-engine/srcRepository: anolilab/lunora
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect TransactionHeadroomTracker API documentation.
sed -n '77,99p' packages/shard-engine/src/transaction-headroom.ts
# Inspect all default-producing mutation paths in ctx-db.ts without excessive output.
sed -n '1688,1722p' packages/shard-engine/src/ctx-db.ts
sed -n '3290,3436p' packages/shard-engine/src/ctx-db.ts
sed -n '3480,3518p' packages/shard-engine/src/ctx-db.ts
sed -n '3538,3554p' packages/shard-engine/src/ctx-db.ts
sed -n '4058,4106p' packages/shard-engine/src/ctx-db.ts
# Find tests that cover global default/ID metering/limits.
rg -n -C 6 'global|D1|insertDefaults|_creationTime|maxWrittenBytes|recordWrite|meterWrite' packages/shard-engine/__tests__ packages/shard-engine/test* 2>/dev/null || trueRepository: anolilab/lunora
Length of output: 50372
Charge the same normalized write object across global insert/replace paths.
Global insert, insertMany, and by-id replace pass raw caller documents to meterWrite, but D1 can add _id/_creationTime defaults. For consistency with shard-local metering, meter the normalized object that represents the persisted row for single and batch inserts, and meter the post-OCC replacement object for by-id replace.
🤖 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/ctx-db.ts` at line 3389, Update global insert,
insertMany, and by-id replace metering to pass the normalized persisted-row
object to meterWrite rather than the raw caller document. Reuse the object after
D1 applies _id/_creationTime defaults for inserts and the post-OCC replacement
object for replace, matching shard-local metering behavior.
prisis
added a commit
that referenced
this pull request
Aug 10, 2026
* fix(shard-engine): charge the meter before the cross-backend write 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 * fix(shard-engine): balance clause chains under the expression-depth cap 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 * fix(shard-engine): backstop the statement size and parameter count 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 * feat(advisor): warn on a wide fan-out, and fix the docs' phantom warnings 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 * fix(ai): reject a chunkSize that cannot fit Vectorize metadata 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 * fix: address the review findings on the platform-limit guards 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 * test(shard-engine): declare the assertion count on two new tests 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 (#399) * 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> * fix: address the review comments on the platform-limit guards **`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 * fix: keep the metering exemption to the row that was precharged `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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
The two findings #398 named as known-but-unaddressed. Both are pre-existing, and both are the same failure shape as the bug that started this work: fine on a small schema, broken once it grows.
The re-projection scan could not run on a wide table
legacyRowPredicatebound five parameters per reprojectable column and OR'd one clause per column. A table with 21v.bigint()/v.bytes()columns therefore exceeded Workerd's 100-parameter cap, and its 100-termORchain would have hit the expression-depth ceiling on the way. That took out both callers:isLegacyRow, which the per-row migration check runs, andcountLegacyRows.Correction to an earlier draft of this description:
countLegacyRowsis exported but has no caller anywhere in the repo — no CLI command, Studio page, or admin RPC consumes it. Calling it "the--dry-runfigure an operator reads" described a path that is not wired up. Today the fix is reachable only throughbuildReprojectionMigration's per-rowisLegacyRow. That gap is pre-existing and out of scope here.The field list now rides in as one JSON parameter walked by
json_each, the same shapesqliteInListalready uses in this package:Four parameters and one
EXISTS, whatever the column count, and noORchain to nest.json_eachyields paths already quoted byjsonPathSegment, so appending[0]/[1]keeps a field literally nameda.bresolving to itself rather than to a nestedb— verified directly against SQLite, not just reasoned about.A
.global()patch or replace consumed no ceiling at allBoth fall back to the D1 writer and return before
onWrite, which is where the meter normally charges. So a mutation could rewrite a.global()table without ever touching its transaction budget — the exact hole theinsertbranch's own comment describes, left open for the two write paths beside it.patchManyinherits it, since it delegates topatch.Both now charge before crossing the boundary, for the reason #398 established for
insert: a D1 write commits where the DO'sstorage.transactioncannot roll it back, so a breach has to be found while the row is still unwritten.One asymmetry worth stating plainly:
patchcharges the delta, not 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 throughonWrite. Under-counting by the untouched fields is the right side of that trade — the delta is what the call actually sends — but it is a real difference between the two paths, not an oversight.replacehas the whole document in hand, so its charge is exact.Review
Both thermo passes ran against this branch.
The bug/security pass returned no High and no Medium, having verified the rewrite empirically rather than by argument: an old-vs-new equivalence matrix over 17 document shapes × 9 field-name shapes agreeing in every cell, the predicate executed inside a real Durable Object, and a benchmark showing 1.41–1.52x on a full scan with no query-plan change (neither form could ever use an index, since the DO's expression indexes are on
$.field, not$.field[0]).The quality pass found the fix incomplete against its own title. Folded in:
deletewas still unmetered — same shape as the two branches fixed here, anddeleteWhereover a.global()table routes through it, so a whole batch escaped the meter. Now charged.!meterExemptgate was copy-pasted at six sites across 700 lines, which is whydeletewas missed. One namedmeterWritemakes an absence a visibly-missing line.toThrow(/TRANSACTION_LIMIT_EXCEEDED|limit/)reads the message, which never contains the code — so it was really/limit/and would have passed on any error carrying that word. Moved toctx-db.headroom.test.tsand asserted on the code via that file'scodeOfhelper.This branch is also now stacked on #398 rather than on
alpha. Its comment cites the globalinsertbranch's charge ordering, and that ordering only exists in #398 — onalphathe citation was simply false.The follow-up, now taken
Review suggested reframing the predicate to walk the document's own members rather than extract at a list of paths. That is now the third commit, and it is the version that should have been written first.
json_eachover__doc__walks the row's top-level members, where the key is the field name — so there is no path to build at all. That deletes this module'sjsonPathSegmentdependency, the path mapping, the|| '[0]'concatenation, and the two docblock paragraphs that existed only to explain the JSON-path grammar. A field literally nameda.b, or one carrying a quote, a bracket or an emoji, is compared as a plain string and cannot re-parse as a nested key.type = 'array'is load-bearing rather than an optimisation:json_eachhands back a scalar member's raw SQL text, andjson_extract('abc', '$[0]')is a malformed-JSON error, notNULL. It also preserves the exclusion the whole predicate turns on — a current projection is a JSON string, so it never reaches the element tests.Equivalence was measured rather than argued: old and new agree on every cell of 21 document shapes × 9 field-name sets, including quotes, brackets, backslashes, unicode and the empty name. New tests cover the shapes the reframing newly depends on — a scalar member, a too-short array, a JSON null, a tagged
Dateunderv.any(), a tagged value under a non-reprojectable field, and thea.bname.Testing
Both regressions are pinned by tests confirmed to fail without the fix:
The first of those caught a bug in its own setup on the way — seeding with
encodeDocJsonre-encodes into the current projection, which is precisely the shape the predicate must not match, so the row it created was not legacy at all.The re-projection predicate is now pinned on real workerd as well: authorization is per function rather than per query, and the correlated
EXISTSwith a||-computed path is the stricter of the twojson_eachshapes the engine emits. That file's docblock previously claimed there was only one use in the repo and "nothing else would notice if that changed".Green: shard-engine 1105, sql-store 108, do 592 (including the gated workerd suite), runtime 898;
api:checkmatches all 47 snapshots; eslint, prettier andlint:typesclean.🤖 Generated with Claude Code
https://claude.ai/code/session_011sk9BLaUZDkPVAZ1sKhuDP
Summary by CodeRabbit