Skip to content

core/storage: keep a cursor's gap position across a peer cursor's balance - #10685

Open
turso-github-handyman[bot] wants to merge 4 commits into
mainfrom
auto/9324-an-open-join-returns-earlier-rows-again--e8a98c
Open

turso-github-handyman[bot] wants to merge 4 commits into
mainfrom
auto/9324-an-open-join-returns-earlier-rows-again--e8a98c

Conversation

@turso-github-handyman

@turso-github-handyman turso-github-handyman Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Problem

A scan cursor can sit between rows (skip_advance set) after a delete. If another cursor on the same connection then splits the leaf, try_save_position_for_external_balance loses that position:

leaf: [10 20 30 40 50 60]   scan cursor on 60

DELETE 60    -> cursor in gap after 50, no record
INSERT 58,59 -> leaf splits; cursor wasn't saved (no record), stale page stack
Next         -> 50, 58, 59, 70      // 50 returned twice (#9324)
leaf: [10 20 30 40 50]   scan cursor on 30

DELETE 30    -> cursor on 40, skip_advance
INSERT ...   -> split; saved as SeekTo(40, GE eq_only=true), skip_advance dropped on restore
Next         -> 50                  // 40 skipped

Fix

The invariant: within one transaction, after the same connection writes the table, a forward scan must not return a row it already returned, and must not skip one.

Save skip_advance cursors as a gap, not as an exact row:

before cell K       -> save SeekTo(K, GE eq_only=false)
after last cell K   -> save SeekTo(K, GT)

restore_context(range op) -> no record, skip_advance = true
  Next -> first row after gap
  Prev -> last row before gap

Every other save_context caller uses GE eq_only=true, so this restore path only runs for these cursors. Index cursors with no record read the key cell first, then step back into the gap. Reading a column from a cursor in a gap returns NULL, which matches SQLite for a deleted row.

This only changes the in-memory cursor state of the writing connection. Pages, WAL, locks, and MVCC are untouched.

Correctness argument
  • Other transactions: no pages are written and no shared state is added, so they see nothing different before or after commit.
  • Abort and rollback: nothing extra to undo; the saved position lives only in the cursor and is dropped with it.
  • Crash recovery: no WAL, checkpoint or on-disk change, so recovery finds the same pages as without this change.
  • Lock order: no lock is taken or reordered, so no new deadlock is possible.
  • MVCC: no change to visibility or commit steps.

Tests

All of them fail before the fix and pass after it. The whopper and limbo_sim seeds also pass, but they don't reach this code: the simulators never write a table while the same connection has an open scan on it.

Fixes #9324

Description of AI Usage

Autofixer wrote this change with Claude Code in an isolated sandbox. A separate verifier agent tested and reviewed it in its own container before this PR was opened. A maintainer must review it before merge.

To have Autofixer address review feedback, leave your comments and then mention @turso-github-handyman; it handles all unanswered comments from maintainers in one round.

@turso-github-handyman

turso-github-handyman Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Author

Autofixer merge readiness: ✅ Recommended

  • The independent audit recommends merging this exact patch, and CI is green.

TL;DR: In a transaction, a join's outer scan returned 50|1,50|2 again after DELETE of a leaf's last row then an INSERT that split the leaf. Head saves gap cursors as GE{eq_only:false}/GT. Review: try_save_position_for_external_balance gap branch.

Least sure about: core/storage/btree.rs:7829: GT on the leaf's last cell assumes cell_idx==cell_count only means 'after the last cell'. Can a DESC index scan reach this state? My sweep never hit a bad index scan, even on base

How the audit checked it

Change: Invariant violation · Risk if wrong: medium · Behaviour: checked · Audited: 46420e9e9f27 (approved)

  • Root cause: try_save_position_for_external_balance saved nothing for a skip_advance cursor with has_record=false, and saved skip_advance cursors as an exact row, so a split left a stale stack
  • Why the audit recommends it: Narrow root-cause fix: only skip_advance cursors take the new save path. All 7 new tests fail without it and pass with it. A scan sweep shows 123 bad scans on base and 0 on head. Since the last approved audit only comments changed, and the merge with main is clean.
  • Summary: Approve: since the two approved audits only comments changed, the merge with main is clean, and the fix still removes every repeated or skipped row in a 480-scan sweep.
  • Planned check (from triage): Run /output/repro.sh: it runs the issue's Rust program through the turso crate and compares with SQLite 3.50.4's output (…60|1, DELETE, NULL|2, INSERT, 58|1, 58|2, 59|1, 59|2, 70|1, 70|2). On main it prints 50|1 and 50|2 again after the INSERT and exits 1; after the fix a forward scan must never go back to an earlier row, and the output must match SQLite exactly. The invariant: a cursor whose position was moved by a delete (has_record=false, skip_advance=true) must keep a valid position across another cursor's balance on the same B-tree, within the same transaction.
  • What the check found: Issue join as integration test: base repeats 50|1,50|2 (and skips 40 when a middle row is deleted); head 58,59,70 in order. Sweep of 480 scans: base 123 with repeated or missing rows, head 0
  • Reproduction script (written by triage before the fix): on main (6ef5e7b81dff) bug present; on this PR (a76a9b7f76be) bug gone
  • What it could affect: Only peer cursors with skip_advance set when the same connection balances the same B-tree mid-scan; other save_context callers still use GE eq_only. WAL and MVCC, no on-disk change
  • Existing code checked for reuse: stay_before_entry_if_saved_in_gap none found (searched set_has_record(false)+skip_advance, restore_context); copy_current_index_record is the existing save code moved out, not a copy; test helpers delete_last_row_of_first_le
Commands the audit ran (8)
  • Merge with main is clean; since the last audit only comments changed: git merge --no-edit origin/main; git diff 5494530e0038 46420e9e9f27 → merge ok; diff is only comment lines in btree.rs
  • New unit tests pass on head: timeout 1800 cargo test -p turso_core --lib peer_ → 11 passed; 0 failed
  • Issue reproduction (integration tests) pass on head: timeout 2400 cargo test -p core_tester --test integration_tests test_scan_continues_after → 2 passed
  • With non-test code reverted to main, the new tests fail and the issue's output shows up: python3 /tmp/revert.py; cargo test -p turso_core --lib peer_; cargo test -p core_tester --test integration_tests test_scan_continues_after → 5 unit tests FAILED; both integration tests FAILED: left has (Null,2),(50,1),(50,2),(58,1)...; middle case is missing 40
  • Broader sweep (join outer scan ASC/DESC, rowid and index order, 40 delete targets, 3 insert patterns): no repeats or losses on head: timeout 2400 cargo test -p core_tester --test integration_tests audit_tmp_scan_sweep -- --nocapture (temp test, head vs reverted) → head BAD 0; base BAD 123 (118 ASC table, 5 DESC table)
  • Neighbouring tests pass: cargo test -p turso_core --lib storage::btree; cargo test -p core_tester --test integration_tests query_processing::test_transactions → 85 passed; 68 passed
  • Lint and format clean: cargo fmt --check; cargo clippy -p turso_core --all-features --all-targets -- --deny=warnings → FMT_OK; Finished, no warnings
  • Simulators pass but do not reach the new gap-save branch: SEED=4242 target/release/turso_whopper --mode btree-rebalance --allocation-fault-probability 0; target/debug/limbo_sim --maximum-tests 1000 --min-tick 10 --max-tick 50 --io-backend=memory --seed 4242 → both exit 0; 0 AUDIT_GAP_SAVE hits

Autofixer's recommendation; a maintainer decides. Updates when the patch, CI, or main changes.

@turso-github-handyman

turso-github-handyman Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Author

Autofixer verifier history

Verifiers raised 4 blocker or major findings while Autofixer worked on this pull request. Later rounds may have fixed them; the merge audit re-checks each one.

Findings, oldest first
  • major PR:0 The commit message is only the issue title plus 'Fixes An open join returns earlier rows again after a DELETE and an INSERT on its outer table #9324': it has no scope, no imperative summary and no body. No PR description was available to check. This tier requires a correctness argument: why encoding the gap state as a GE{eq_only:false}/GT saved seek op is safe; that only the new save… (on 3ba724249cee, 2026-10-24T16:46:43.858Z)
  • major core/storage/btree.rs:7801 The fix only covers a peer in a gap with has_record=false. If the deleted row is in the middle of a leaf, the peer's restore leaves it with has_record=true and skip_advance=true; a temporary log line showed this state in the save. The save then takes the has_record branch with GE{eq_only:true}, and… (on ae24c3c90cdc, 2026-10-24T18:28:41.359Z)
  • major core/storage/btree.rs:6604 restore_skip_advance sets has_record to the saved ctx.has_record even when the restore left the cursor with no record. If the saved key is deleted before the cursor restores, a GE{eq_only:false} index seek returns TryAdvance, get_next_record reaches the end of the tree (cell 7 of 6, has_record=fals… (on 72a9dac36ce9, 2026-10-24T19:23:39.316Z)
  • major breaker A backward (ORDER BY ... DESC) index scan, left after the last cell of an index leaf because another statement deleted the rows at the end of that leaf, then another statement inserts enough entries into the same index to split pages. Head saves the position as GT(last cell of the leaf), and prev()… (on 9a10cfbb3766, 2026-10-24T20:14:20.196Z)

@turso-github-handyman

turso-github-handyman Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Author

Autofixer breaker follow-ups

The breaker found 3 defects that main has too, outside this pull request's scope. They do not block it; each is worth an issue of its own.

  • When another statement deletes the row a cursor is on in the middle of a leaf (table or index B-tree), restore_context's NotFound branch leaves has_record=true on the next entry. Column reads before the next Next then return the following row's values where SQLite returns NULL, and that row is later: Turso gives 20|1, 30|2, 30|1, 30|2, 40|1, 40|2, sqlite gives 20|1, NULL|2, 30|1, 30|2, 40|1, 40|2 (2026-10-24T20:14:18.789Z)
CREATE TABLE t1(id INTEGER PRIMARY KEY, k INT); CREATE INDEX i1 ON t1(k); CREATE TABLE t2(x INTEGER PRIMARY KEY); INSERT INTO t2 VALUES (1),(2); INSERT INTO t1 VALUES (10,10),(20,20),(30,30),(40,40); BEGIN; stmt A = SELECT t1.k, t2.x FROM t1 INDEXED BY i1, t2 WHERE t1.k > 0 ORDER BY t1.k; step A 3 times (..20|1); DELETE FROM t1 WHERE id = 20; step A to end
  • When another statement deletes the row an open forward scan cursor is on, and that row is not the last cell of its leaf, reading that cursor again (for example the next inner-loop row of a join) gives the next row's values instead of NULL. The same row is then returned a second time when the scan mo: Turso gives ... 30|1, 40|2, 40|1, 40|2, DONE (40|2 returned twice), sqlite gives ... 30|1, NULL|2, 40|1, 40|2, DONE (2026-10-25T07:01:28.706Z)
CREATE TABLE t1(id INTEGER PRIMARY KEY, v BLOB); CREATE TABLE t2(x INTEGER PRIMARY KEY); INSERT INTO t2 VALUES (1),(2); INSERT INTO t1 VALUES (10,zeroblob(600)),(20,zeroblob(600)),(30,zeroblob(600)),(40,zeroblob(600)); BEGIN; open S = SELECT t1.id, t2.x FROM t1, t2; step S 5 times (last row 30|1); DELETE FROM t1 WHERE id = 30 on the same connection; step S to the end
  • When another statement on the same connection deletes the row an open scan cursor is on, and the row is not the last cell of its leaf, reading that cursor's columns again returns the next row in scan order (for a DESC scan, a row already returned) instead of NULL. The restore that finds the row gone: Turso gives 30|1 then 40|2 (then 40|1 40|2 ...), sqlite gives 30|1 then NULL|2 (2026-10-25T09:57:51.160Z)
CREATE TABLE t1(id INTEGER PRIMARY KEY, v BLOB); CREATE TABLE t2(x INTEGER PRIMARY KEY); INSERT INTO t2 VALUES (1),(2); INSERT INTO t1 VALUES (10,zeroblob(600)),(20,zeroblob(600)),(30,zeroblob(600)),(40,zeroblob(600)),(50,zeroblob(600)),(60,zeroblob(600)),(70,zeroblob(600)); BEGIN; step SELECT t1.id, t2.x FROM t1, t2; after row 30|1 run DELETE FROM t1 WHERE id = 30; step again

@turso-github-handyman

turso-github-handyman Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Author

Autofixer stopped following up on this pull request: it tried to address the merge audit's findings and did not: the breaker found inputs it gets wrong in the last round.

Resumed by a maintainer's mention of Autofixer on 2026-10-25T03:49:41.273Z; Autofixer follows up on this pull request again.

@turso-github-handyman turso-github-handyman Bot added autofixer: follow-ups-recommended Autofixer's audit found follow-ups; they are filed as issues when this merges autofixer: stopped labels Oct 24, 2026
@jussisaurio

Copy link
Copy Markdown
Collaborator

@turso-github-handyman continue this

@turso-github-handyman turso-github-handyman Bot changed the title An open join returns earlier rows again after a DELETE and an INSERT on its outer table core/storage: save a peer cursor that sits in a gap before a balance Oct 25, 2026
A restore that does not find its row leaves the cursor with no record and
skip_advance set. The save before a peer's balance skipped such cursors, so
after a leaf split the next Next walked a stale page stack and returned
earlier rows again. Save the entry after the gap (GE) or the last cell of the
leaf (GT), and put the cursor back in the gap when it is restored.
@turso-github-handyman

Copy link
Copy Markdown
Author

Finished the remaining work. No code change was needed: the btree unit tests and the integration test from the issue fail without the fix (the join returns 50|1 and 50|2 a second time) and pass with it. All 85 btree tests and the 12 whopper regression tests pass. Of the 486 query_processing integration tests, 485 pass; the one failure needs io_uring, which the sandbox blocks. fmt and clippy on turso_core are clean. Whopper (btree-rebalance seeds 1 and 2, fast seed 3) and limbo_sim (seed 7) pass, but a temporary log line showed none of these seeds runs the new branch, because the simulators never write a table while the same connection has a scan open on it. I amended the commit message to the repo style (core/storage: save a peer cursor that sits in a gap before a balance) and wrote a PR description with the correctness argument this tier requires: only the in-memory saved positions of the writing connection's own cursors change, with no page writes, no locks, nothing for a rollback to undo and nothing on disk for recovery. The branch merges cleanly with origin/main, so I did not rebase it.

— Autofixer (34d67f5)

@turso-github-handyman
turso-github-handyman Bot force-pushed the auto/9324-an-open-join-returns-earlier-rows-again--e8a98c branch from a76a9b7 to 34d67f5 Compare October 25, 2026 04:14
@turso-github-handyman

Copy link
Copy Markdown
Author

Autofixer corrected the pull request's text to address the merge audit's findings on 34d67f5. The code is unchanged, so no verifier or breaker ran; a fresh audit checks the pull request.

…lance

A peer cursor left on the row after a deleted mid-leaf row has a record and
skip_advance set; the save before a balance stored it as an exact GE seek, so
the restore dropped skip_advance and the next Next skipped that row. Save any
cursor with skip_advance as sitting in the gap before its cell.

Autofixer-Repair: audit
@turso-github-handyman turso-github-handyman Bot changed the title core/storage: save a peer cursor that sits in a gap before a balance core/storage: save a peer cursor with skip_advance as a gap before a balance Oct 25, 2026
@turso-github-handyman

Copy link
Copy Markdown
Author

Autofixer pushed 5494530 to address the merge audit's findings.

@codspeed

codspeed Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 23.51%

3 warnings, 1 note

⚠️ Unable to measure the execution of 1 benchmark

This benchmark measured no execution time, so its result is not comparable and counts as unchanged. This usually happens when the compiler removes the code under test.

Prevent compiler optimizations

See affected benchmarks

⚠️ 104 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

ℹ️ 2 benchmarks have no flamegraph

  • The flamegraph was generated but could not be saved. Try a shorter benchmark name, or contact us if it keeps failing. (1 benchmark)
  • The flamegraph wasn't generated: no execution data was recorded while the benchmark ran. Make sure the compiler didn't optimize the benchmark out. (1 benchmark)

See affected benchmarks

⚡ 1 improved benchmark
✅ 1164 untouched benchmarks
⏩ 1479 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ CPU Simulation sparse_top_10_query[1024_commits] 557.6 µs 451.5 µs +23.51%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing auto/9324-an-open-join-returns-earlier-rows-again--e8a98c (46420e9) with main (aa93d66)

Open in CodSpeed

Footnotes

  1. 1479 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. ↩

@turso-github-handyman
turso-github-handyman Bot marked this pull request as ready for review October 25, 2026 08:43
@turso-github-handyman turso-github-handyman Bot added the autofixer: merge-recommended Autofixer's independent audit recommends merging this PR as is label Oct 25, 2026
@jussisaurio

Copy link
Copy Markdown
Collaborator

@turso-github-handyman is this change truly minimal? if it is, defend why. if it isn't, make it so.

@turso-github-handyman turso-github-handyman Bot removed the autofixer: merge-recommended Autofixer's independent audit recommends merging this PR as is label Oct 25, 2026
…ance

The doc comment on stay_before_entry_if_saved_in_gap still said only cursors
with no record are saved as a gap; since the previous commit any cursor with
skip_advance is.

Autofixer-Repair: review
@turso-github-handyman

Copy link
Copy Markdown
Author

Kept the fix as is; the code part is minimal. The bug is one function, try_save_position_for_external_balance, dropping skip_advance before another cursor of the same connection splits a leaf. It did this in two ways: it skipped cursors with no record (the issue's case, 50|1 and 50|2 returned again), and it saved cursors with a record by an exact seek, so the next Next skipped a row (deleting row 30 loses 40|1). Same flag, same lines; fixing one case would knowingly leave the other. Code change apart from tests: (1) the save rule: a cursor with skip_advance is saved as the gap before its cell, using that cell's key with GE{eq_only:false}, or the leaf's last cell with GT when it sits after it; (2) a three-line helper called where restore_context finishes, which puts the cursor back in the gap. Every other caller that saves a position uses GE{eq_only:true} (btree.rs:3293, 3340, 7082, 7090), so no other path changes; (3) for index cursors, moving onto the key cell to copy the record and back (the copy code moved into a helper unchanged), without which an index cursor in a gap panics on record.expect. Simpler versions I checked: re-seeking the last cell exactly would read row 50 instead of NULL for the deleted row, unlike SQLite; MustInvalidate loses the position; carrying the flag in CursorContext as SQLite does needs a second flag for no record, so it is more code. I removed parts one at a time: without the helper call in the RequireAdvance path, index_peer_after_last_cell_of_leaf_continues_forward_after_two_inserts fails; without cell_count > 0, cell_count - 1 underflows on an emptied tree. No test reaches the is_leaf check; it is there because a position past the last cell of an interior page is a child pointer, not a row, so there is no key to save, and like the cell_i

— Autofixer (5e1860c)

@turso-github-handyman turso-github-handyman Bot added the autofixer: merge-recommended Autofixer's independent audit recommends merging this PR as is label Oct 25, 2026
@jussisaurio jussisaurio changed the title core/storage: save a peer cursor with skip_advance as a gap before a balance core/storage: keep a cursor's gap position across a peer cursor's balance Oct 25, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@turso-github-handyman

turso-github-handyman Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Author

Autofixer stopped following up on this pull request. Someone else pushed to the branch, so Autofixer will not change it.

What you can do: The pull request is yours to finish: Autofixer will not push to it again.

Resumed by Autofixer once only maintainers had pushed to the branch besides it on Oct 25, 14:05 UTC; Autofixer follows up on this pull request again.

@turso-github-handyman turso-github-handyman Bot added autofixer: stopped and removed autofixer: merge-recommended Autofixer's independent audit recommends merging this PR as is autofixer: stopped labels Oct 25, 2026
@turso-github-handyman

Copy link
Copy Markdown
Author

Autofixer no longer pushes to this branch. A maintainer (jussisaurio) pushed to it, so the pull request is the maintainers' to finish.

Autofixer still audits each new head and keeps the merge readiness and its labels current. To ask it something, mention @turso-github-handyman: it answers in a reply, without changing the code. Failing CI, a conflict with main and the audit's findings are listed in the readiness comment for you to fix.

To hand it back: remove the maintainers' commits from the branch (force-push it back to Autofixer's last commit); Autofixer then repairs and pushes as before.

@turso-github-handyman turso-github-handyman Bot added autofixer: merge-recommended Autofixer's independent audit recommends merging this PR as is and removed autofixer: follow-ups-recommended Autofixer's audit found follow-ups; they are filed as issues when this merges labels Oct 25, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofixer: merge-recommended Autofixer's independent audit recommends merging this PR as is autofixer core Storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An open join returns earlier rows again after a DELETE and an INSERT on its outer table

1 participant