[FIXED] Start time binary search is delete-aware#7751
Merged
neilalexander merged 1 commit intomainfrom Jan 22, 2026
Merged
Conversation
Contributor
|
LGTM for me, the issue is not reproducible in my application. |
wallyqs
pushed a commit
to wallyqs/nats-server
that referenced
this pull request
Jan 21, 2026
These tests demonstrate that: 1. memstore has the same binary search bug as filestore (PR nats-io#7751 fixes filestore) - TestMemStoreGetSeqFromTimeBinarySearchBug shows memstore returns wrong sequence when interior deletes exist (returns 7, expected 3) 2. Extended test coverage for GetSeqFromTime with various delete patterns: - Large gaps (more deleted than remaining) - Single message remaining - Consecutive blocks with deletions - All but first deleted - All but last deleted - Alternating deletes (maximum fragmentation) Test results against current codebase: - Both filestore and memstore fail on multiple patterns - These tests can be used to verify the fix in PR nats-io#7751 and guide a similar fix for memstore
wallyqs
added a commit
to wallyqs/nats-server
that referenced
this pull request
Jan 21, 2026
These tests demonstrate that: 1. memstore has the same binary search bug as filestore (PR nats-io#7751 fixes filestore) - TestMemStoreGetSeqFromTimeBinarySearchBug shows memstore returns wrong sequence when interior deletes exist (returns 7, expected 3) 2. Extended test coverage for GetSeqFromTime with various delete patterns: - Large gaps (more deleted than remaining) - Single message remaining - Consecutive blocks with deletions - All but first deleted - All but last deleted - Alternating deletes (maximum fragmentation) Test results against current codebase: - Both filestore and memstore fail on multiple patterns - These tests can be used to verify the fix in PR nats-io#7751 and guide a similar fix for memstore Signed-off-by: Waldemar Quevedo <wally@nats.io>
wallyqs
pushed a commit
to wallyqs/nats-server
that referenced
this pull request
Jan 21, 2026
Apply the same fix from PR nats-io#7751 (filestore) to memstore. The issue: sort.Search was used with a function that returns false for deleted messages, violating the monotonicity requirement of binary search. This caused incorrect results when searching for timestamps in streams with interior deletes. The fix: Use a custom binary search that skips over deleted messages (gaps) and properly adjusts the search bounds when encountering consecutive deletions. Signed-off-by: Claude <claude@anthropic.com>
wallyqs
approved these changes
Jan 21, 2026
Member
wallyqs
left a comment
wallyqs
left a comment
There was a problem hiding this comment.
LGTM, think we may need similar fix in memstore.
Member
Author
The added test already confirms this is no issue in the memstore. It can use a simple binary search as-is, since its messages are only stored in |
Member
Author
|
Although.. there might be different issues that are not captured by the new (or pre-existing) tests. Will check again tomorrow. |
wallyqs
reviewed
Jan 21, 2026
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
MauriceVanVeen
force-pushed
the
maurice/fs-start-search
branch
from
af1f69e to
e46dc71
Compare
MauriceVanVeen
changed the title
This was referenced Jan 26, 2026
neilalexander
added a commit
that referenced
this pull request
Jan 26, 2026
neilalexander
added a commit
that referenced
this pull request
Jan 26, 2026
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
4 participants
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.
The filestore's
GetSeqFromTimewhich is used to determine the starting sequence for consumers and msg/direct/batch get requests with a start time used a binary search that wasn't aware of deletes. This lead to various incorrect results if the binary search came across such a delete. This PR fixes that by making the search aware of deletes and essentially skipping over these gaps and moving the ranges accordingly.Signed-off-by: Maurice van Veen github@mauricevanveen.com