Repository navigation
fix(orm): stop re-reading per row in FK, CLEARING, and relation probes - #420
Merged
Merged
Conversation
Add a per-transaction memo lifetime keyed on the stable inner writer, the only scope the aggregate write barrier can reach: its closure is built at module scope, so a flag there lives as long as the isolate. - insert(): probe each distinct foreign id once per statement, not per row - aggregate barrier: scan the CLEARING range once per transaction, retired by a generation counter bumped in setCountState, the sole CLEARING writer - relation `where`: memoize _getById per query execution, so a residual stream reads each distinct target once instead of once per scanned row Consumers 1 and 3 deliberately do not use the per-transaction memo: insert -> delete parent -> insert would write a dangling key, and a query ctx never has an inner writer.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🦋 Changeset detectedLatest commit: ef07860 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…n-memo-lifetime # Conflicts: # packages/kitcn/src/orm/insert.read-amplification.vitest.ts # packages/kitcn/src/orm/insert.ts
Member
Autoclosure terminal receiptExact head: Head proof: local HEAD = fetched Per-PR task evidence
Proof
Feedback inventory
Final inventory before receipt: helper 0 unresolved threads, 2 included PR comments, 0 review bodies; raw API 3 issue comments and 0 reviews; GraphQL 0 threads. No deferred feedback. |
This was referenced Sep 5, 2026
MikeyZhang75
added a commit
that referenced
this pull request
Sep 6, 2026
A memo entry is a snapshot shared by every read of that key in one execution, but relation loading writes nested `with` results and `extras` straight onto the target document it is handed, and `hydrateDateFieldsForRead` then copies every own key it finds. So a relation asked for as `true` came back carrying a nested relation only the `where` had requested. Return a shallow copy per caller from both memo twins. The writes are all top-level, so that is exactly as much isolation as they need. This also fixes the same leak on the `_id` path, where `_documentByNormalizedId` (#420) has it today: the new `_getById` regression test fails on main. Reported by @chatgpt-codex-connector on #448.
zbeyens
added a commit
that referenced
this pull request
Sep 7, 2026
* fix(orm): memoize non-_id relation target reads per execution A relation `where` runs as a post-fetch membership predicate over a residual stream, one row at a time, so every per-batch dedup map inside the relation loaders is handed a batch of one. #420 fixed this for joins on the primary id with `_documentByNormalizedId`; a join on any other column resolves its target with `.first()` and had no memo, so it re-read once per drain. Add `_firstByFields`, an execution-scoped sibling of `_getById` keyed on (table, index, columns, values), and route all three single-target `.first()` sites through it: `_loadOneRelation`'s non-id branch, through-relation target resolution, and the `_count` through path. 50-row scan with 2 distinct targets: 50 target reads -> 2. Fixes #446 * docs(plans): record PR #448 in the 446 task plan * fix(orm): encode relation memo keys losslessly `JSON.stringify` renders every ArrayBuffer as `{}` and NaN/Infinity as `null`, and throws on int64. The residual relation `where` hands the relation loader one row at a time, so its own per-batch key map is a map of one and never compares two join values — the memo key was the only discriminator, and it aliased them. A `bytes()` or float-special join value therefore matched the wrong parent: `where: { teamByToken: { name: 'Alpha' } }` returned all 50 members instead of Alpha's 25. Key on `convexToJson`, the same wire encoding `internal/query-key.ts` already uses, so every Convex value round-trips distinctly. A value it rejects is not a Convex value and goes unmemoized rather than sharing an entry. Reported by @chatgpt-codex-connector on #448. * docs(plans): sync 446 plan to the lossless memo key * fix(orm): hand each caller its own copy of a memoized relation target A memo entry is a snapshot shared by every read of that key in one execution, but relation loading writes nested `with` results and `extras` straight onto the target document it is handed, and `hydrateDateFieldsForRead` then copies every own key it finds. So a relation asked for as `true` came back carrying a nested relation only the `where` had requested. Return a shallow copy per caller from both memo twins. The writes are all top-level, so that is exactly as much isolation as they need. This also fixes the same leak on the `_id` path, where `_documentByNormalizedId` (#420) has it today: the new `_getById` regression test fails on main. Reported by @chatgpt-codex-connector on #448. * docs(plans): record the memo isolation fix in the 446 plan * chore: verify PR 448 against refreshed fixtures --------- Co-authored-by: zbeyens <zbeyens@udecode.dev>
This branch was successfully deployed
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
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.
🐛 Fixes #406
🧭 Task plan: docs/plans/2026-08-21-406-orm-per-transaction-memo-lifetime.md
🟢 95-100% confidence
bun check, 4 new read-bound test files✅ Outcome
insert()probes each distinct foreign id once per statement instead of once per row (8 → 1).CLEARINGrange once per transaction instead of once per written row (12 → 1).wherereads each distinct target document once per query instead of once per scanned row (50 → 2).orm/transaction-cache.tsowns the per-transaction memo lifetime the ORM was missing.insert()is per-statement becauseinsert -> delete parent -> insertin one transaction would otherwise write a dangling foreign key with no hook and no warning. Relation targets are per-execution because a query ctx never has an inner writer, so the proposed anchor is unreachable there.relationDefinition.whereis a synchronous column-only predicate and issues zero reads. The amplification ismatchesPostFetchMembershiphanding_applyRelationsFilterToRowsa one-row array, which starves every downstream de-duplication map.🏗️ Design
getOrmLifecycleInnerDb(db) ?? db. The fallback is required, not defensive: the lifecycle refuses to wrap readers and no-ops for schemas without triggers or aggregate indexes, so the inner-db symbol is absent for every query and most mutations. That formula converges on the same raw writer from the main scope,skipRules,withoutTriggers, and the scheduled workers.ctx.dbis a fresh literal per UDF invocation, so the key can never leak across transactions; a nestedctx.runMutationjust starts a fresh memo, which costs extra reads rather than staleness.setCountStateis the only writer that can move a state row intoCLEARING, so it bumps an isolate generation counter that retires cached clean answers even across the nested-invocation boundary no transaction-scoped invalidation could reach. The counter only ever invalidates, so it cannot reintroduce the closure trap the issue warns about._idexistence check, and switches off when the table has lifecycle hooks, which are the only path that runs user code between rows.orm/aggregate-index/runtime.tscan use it without dragginglifecycle.tsinto the aggregate entry's bundle;import-graph.test.tsstill passes.🧪 Verified
bun checkpasses: lint, typecheck 5/5, 1312 bun tests, 864 vitest tests, CLI, concave smoke, fixture parity,test:verify,test:runtime.setCountStateflippingCLEARINGmid-transaction, and a target patched between two reads.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.