fix(db): deterministic total order for every offset-paged list - #1704
Draft
ohdearquant wants to merge 4 commits into
Draft
fix(db): deterministic total order for every offset-paged list#1704ohdearquant wants to merge 4 commits into
ohdearquant wants to merge 4 commits into
Conversation
Offset pagination ordered several list surfaces by a timestamp alone (created_at or updated_at). Rows sharing a timestamp had no defined relative order, so consecutive pages could both include and both omit rows near page boundaries: paged sweeps duplicated some rows and missed others. Append the row id as a final tiebreak to every offset-paged ORDER BY, in the same direction as the primary key, making each page order a deterministic total order. The note-store default order, which already carried an id tiebreak, now uses the same direction as its primary key so all default list orders are uniform. Closes #1671
- revert the filtered-note default tiebreak to id ASC: the pre-existing clause was already a deterministic total order and the direction flip was an unneeded user-visible ordering change, inconsistent with the bounded variant in the same store - pack-level sweep tests now force one shared timestamp across all rows via direct SQL so the sweeps genuinely exercise the tiebreak instead of relying on wall-clock microsecond collisions that never occur - add offset sweeps for the entity name-prefix CASE branch and the multi-field custom-sort path, pinning that the appended id tiebreak follows the last sort key's direction
Follow-up to the review of the #1671 tiebreak series: - Add the offset sweeps missing for the two changed queries that had no regression test: the knowledge.list(kind="domain") paging (created_at DESC, id DESC) and the knowledge.index batch re-embed paging sweep (created_at ASC, id ASC). Both force one shared created_at via direct SQL so the tiebreak is load-bearing, and both assert exact expected order in addition to no duplicates / no misses. - Extend the existing proposal and knowledge atom sweeps to assert the concatenated pages follow the documented order (updated_at DESC, proposal_id DESC and created_at DESC, id DESC); a uniqueness-only sweep still passes with a wrong tiebreak direction. - The entity name-prefix sweep now seeds one row named exactly the prefix ("Alpha") with the same created_at as the rest and asserts it sorts into the exact-match-first group ahead of prefix matches. - Fix the graph.rs comment: the appended id tiebreak follows the LAST sort field's direction (the behavior the multi-field sweeps codify), not "the primary key's direction". - Scope the "sound" comments in entity.rs / graph.rs / note.rs: the deterministic total order removes tie-order instability only; offset paging can still duplicate or skip rows under concurrent inserts/deletes or sort-key updates (that would need snapshot isolation or keyset pagination). - Document that the note filtered-default clause (created_at DESC, id ASC) is already a deterministic total order and intentionally left unchanged. No SQL change; behavior is identical. Ref: PR #1704
The tiebreak removes tie-order instability; it does not protect offset pages against concurrent inserts, deletes, or updated_at changes. The sibling entity/graph/note comments already state this.
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
1 participant
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.
Problem
Offset pagination ordered several list surfaces by a timestamp alone (
created_atorupdated_at). Rows sharing a timestamp have no defined relative order in SQLite, so consecutive pages could both include and both omit rows near page boundaries: paged sweeps duplicated some rows and missed others.Closes #1671.
Change
Append the row id as a final tiebreak to every offset-paged
ORDER BY, in the same direction as that query's primary sort key, making each page order a deterministic total order. Covered surfaces: note listing (default and JSON-path sorts), entity and edge listing, proposal listing, knowledge domain/atom listing, and the index handler's pages. The note-store default order already carried an id tiebreak in the opposite direction; it now matches its primary key's direction so all default list orders are uniform.Tests
Per-surface regression tests seed rows sharing one timestamp and sweep with a page size that lands boundaries inside the tied group, asserting every row appears exactly once (no duplicates, no misses). An end-to-end knowledge sweep covers 119 atoms in 7 same-second batches at page size 13.
cargo test -p khive-db -p khive-pack-kg -p khive-pack-knowledge: 1,450+ passing across all binaries. Clippy clean with-D warnings.