fix(session): delete reverted messages boundary-last and tie-break ids by raw order - #42819
Open
NamedIdentity wants to merge 2 commits into
Open
fix(session): delete reverted messages boundary-last and tie-break ids by raw order#42819NamedIdentity wants to merge 2 commits into
NamedIdentity wants to merge 2 commits into
Conversation
added 2 commits
August 15, 2026 14:41
Revert cleanup removed the boundary first. Each removal is a separate durable transaction, so an interruption after the boundary left the remaining reverted rows unreachable: the next cleanup's findIndex returned -1, remove was empty, and clearRevert still discarded the only marker that could have located them. Those rows then rejoined the transcript as ordinary history and were sent to the model. Iterating newest-first makes the boundary its own progress marker, so an interrupted cleanup leaves a state the next one completes. The part loop had the same shape and the same fix.
…ation Message ordering tie-broke equal time.created with localeCompare, while storage pages with ORDER BY time_created, id under SQLite's BINARY collation. The two disagree: for ids differing only in suffix case, en-US collation and raw byte order sort them oppositely, so a paginated fetch and the client could order the same pair differently. localeCompare also returns 0 for canonically-equivalent distinct strings. MessageID only requires a msg prefix and is not restricted to ASCII, so two distinct primary keys can compare equal and the sort becomes input-order dependent. Applies to the TUI message store and the shared transcript. The session list sort is left alone -- it has no time component and feeds a binary search whose relation would have to change with it.
github-actions
Bot
added
needs:compliance
Contributor
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
NamedIdentity
changed the title
fix(session): delete reverted messages boundary-last and tie-break ids by raw order
github-actions
Bot
removed
needs:title
needs:compliance
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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.
Issue for this PR
Closes #42816
Type of change
What does this PR do?
Two fixes to the ID rollover ordering work.
1. Revert cleanup deletes the boundary message first.
SessionRevert.cleanupremoves messages from the revert boundary onward. The boundary isremove[0]when nopartIDis set, and eachremoveMessagecommits separately. If that loop is interrupted, the boundary is already gone but the rest are not. The next cleanup callsfindIndexfor the boundary, gets-1, soremoveis empty and it deletes nothing.clearRevert()still runs at the end, discarding the marker that could have located those rows. They stay in the session permanently and get sent to the model on the next turn.Using
.toReversed()deletes the boundary last, so it works as a progress marker. An interrupted cleanup then leaves a state the next one can finish. The part loop below it has the same problem and the same fix.2. The
localeComparetie-break does not match storage order.Storage pages with
ORDER BY time_created, id, and SQLite's default collation is BINARY.sync.tsxandShare.tsxtie-break the same data withlocaleCompare, which is locale collation. For two messages sharing atime.createdthese can return opposite results, so the client and the database can disagree about which comes first. Ids minted in the same millisecond by different processes share the timestamp prefix, so the random mixed case suffix decides, and that is exactly where the two collations differ.localeComparecan also return 0 for two distinct strings that are canonically equivalent.MessageIDonly requires amsgprefix, so two different primary keys can compare equal and the sort becomes dependent on input order.Raw
<and>match what storage already does.How did you verify your code works?
Read the affected paths and traced the interruption sequence above. Confirmed
toReversed()is already used elsewhere in the repo, so there are no target or lib concerns.Both fixes have been running in our fork for a day. We hit the same rollover in production on 2026-08-14, wrote the same core fix independently, then found yours had landed. These two turned up while auditing ours.
I have not run the full test suite against this branch.
Screenshots / recordings
Not a UI change.
Checklist