Repository navigation
fix(orm): resume soft cascade delete instead of replaying its range - #422
Merged
Merged
Conversation
Soft delete only stamps deletionTime, so processed children stayed in the foreign key index range and every scheduled batch re-read them, growing reads with the square of row count until the campaign died on Convex's read limit — stranding aggregate index state at CLEARING. Soft cascade batches now forward the pagination cursor. Safe here because deletionTime is not indexed, so nothing moves relative to the cursor; every other action rewrites the FK columns or removes the row, so those keep re-querying from null. Drops the deletionTime filter too: Convex pays for those rows anyway and filtering them out strands the cursor, so already stamped rows are skipped in JS. A byte-truncated batch re-pages from the same cursor at the size that fit. Convex allows one paginated query per execution, so it cannot ask for a second cursor; the size strictly decreases and settles. Adds opt-in countDocumentReads scanned metric — documents reads 110 with and without the bug, so it cannot see filter-consumed reads. 4000 descendants: 117,177 scanned rows -> 8,152.
🦋 Changeset detectedLatest commit: 7620ba0 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.
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Resolves the countDocumentReads collision flagged in PR #422: #399 landed a scanned-row counter on main that reads QueryImpl.state.query and walks a paginated range cursor by cursor. That is strictly better than this branch's chain-replay version, so convex/setup.testing.ts now matches main exactly and the cascade read-bound tests assert on main's counter. Both counters independently report 1100 scanned rows for the 100-child repro and 4214 for the byte-budget one, so the bounds are cross-validated, not tuned to one implementation. Red calibration re-run against main's pre-fix worker: all three cases still fail.
Member
Autoclosure terminal receipt
Verdict: ready to squash-merge. |
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 #407
🧭 Task plan: docs/plans/407-orm-soft-cascade-linear-reads.md
🟢 95-100% confidence
✅ Outcome
Soft cascade delete only stamps
deletionTime, so processed children stayed inside the foreign key index range and every scheduled batch replayed them. Reads grew with the square of the row count until the campaign died on Convex's per-execution read limit, which could strandaggregateIndex/rankIndexstate atCOUNT_STATUS_CLEARINGand turn the lifecycle write barrier into a total write outage on that table.Batches now resume where the previous one stopped, so a full campaign's reads scale linearly with the number of children. Hard cascade,
set null,set default, and cascade update are untouched.🏗️ Design
The issue recommended a
_creationTimewatermark. That is unsound:_creationTimeis not unique within a Convex table — snapshot import takes a caller-supplied value verbatim (import_facing.rs:107), it is an f64 in ms that quantizes to ~244ns buckets, and the per-transaction base is not allocator-serialized the way commit timestamps are..gt('_creationTime', wm)compiles toBound::Excluded, whose start key isincrement()of the byte prefix, so it drops every tie-twin regardless of_id— silently leaving a child uncascaded.Soft cascade forwards the pagination cursor instead. A cursor is an index-key position that includes
_id, so it is exact under ties and independent of the index shape. It is safe here specifically becausedeletionTimeis not an indexed column, so nothing moves relative to the cursor; every other cascade action rewrites the foreign key columns or removes the row, so those keep re-querying from null — already linear, since processed rows leave the range.root-updateandroot-deletein the same worker already forward cursors across scheduled batches.The
deletionTimefilter is dropped rather than kept: Convex pays to read those rows either way, and filtering the cursor's own row out of the result strands the cursor. Already-stamped rows are skipped in JS.A child inserted behind the cursor mid-campaign is not stamped. That is inherent to not re-reading the range — any design meeting the issue's linear-reads criterion has it — and the race was never settled, since the campaign could have passed that key before the insert.
🧪 Verified
The wasted reads are consumed by
.filter(), sodocumentsreports 110 with and without the bug. These bounds assert onreads.scannedfrom #399, which landed onmainwhile this PR was open;convex/setup.testing.tshere is byte-identical tomain. This branch had briefly carried its own scanned counter — both implementations independently report 1,100 scanned rows for the 100-child repro and 4,214 for the byte-budget one, so the bounds are cross-validated rather than tuned to one mechanism.Review caught a real defect in the first cut: the byte-truncation path called
.paginate()twice in one execution, which Convex rejects withMultiplePaginatedDatabaseQueriesand convex-test does not model. A truncated batch now re-pages from the same cursor at the size that fit;takeRowsWithinByteBudgetalways keeps one row and a truncated page is strictly shorter than requested, so the size strictly decreases and settles. That also cut the byte path from 626 to 82 scanned rows for 60 children. A per-invocation paginate-call guard was added and verified non-vacuous.bun checkgreen on this branch after mergingmain: lint, typecheck,bun test,vitest,test:cli,test:concave,fixtures:check,test:verify,test:runtime. Env-gated stress lane green at 4,000 descendants.