CQ: Fix shared store scanner missing messages #12392
Merged
Conversation
This file contains 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
lhoguin
force-pushed
the
loic-fix-cq-scan
branch
from
40d57b3 to
4c5d171
Compare
gomoripeti
reviewed
Sep 26, 2024
lhoguin
force-pushed
the
loic-fix-cq-scan
branch
from
4c5d171 to
acb04e9
Compare
|
This should be better. If not, I will fix tomorrow. |
|
All good. Just need more testing and if possible some real world testing. |
|
This PR can be tested with the following (x86-64) Docker image tag https://hub.docker.com/layers/pivotalrabbitmq/rabbitmq/sha-acb04e929f9b1030dd077c62991147876b08fff5/images/sha256-353eb18c3b4d98fd393eacaf98234a90bfc582f0c6d1f38232f8a4317b0cf4be?context=explore |
lhoguin
force-pushed
the
loic-fix-cq-scan
branch
6 times, most recently
from
dd8870c to
d863c71
Compare
|
This should be the most up to date image: https://hub.docker.com/layers/pivotalrabbitmq/rabbitmq/loic-fix-cq-scan/images/sha256-ab439bb28985d6699f8b5a7c6ca91ca0d183202521f808b888c874b41146c02e?context=explore |
It was still possible, although rare, to have message store files lose message data, when the following conditions were met: * the message data contains byte values 255 (255 is used as an OK marker after a message) * the message is located after a 0-filled hole in the file * the length of the data is at least 4096 bytes and if we misread it (as detailed below) we encounter a 255 byte where we expect the OK marker The trick for the code to previously misread the length can be explained as follow: A message is stored in the following format: <<Len:64, MsgIdAndMsg:Len/unit:8, 255>> With MsgId always being 16 bytes in length. So Len is always at least 16, if the message data Msg is empty. But technically it never is. Now if we have a zero filled hole just before this message, we may end up with this: <<0, Len:64, MsgIdAndMsg:Len/unit:8, 255>> When we are scanning we are testing bytes to see if there is a message there or not. We look for a Len that gives us byte 255 after MsgIdAndMsg. Len of value 4096 looks like this in binary: <<0:48, 16, 0>> Problem is if we have leading zeroes, Len may look like this: <<0, 0:48, 16, 0>> If we take the first 64 bits we get a potential length of 16. We look at the byte after the next 16 bytes. If it is 255, we think this is a message and skip by this amount of bytes, and mistakenly miss the real message. Solving this by changing the file format would be simple enough, but we don't have the luxury to afford that. A different solution was found, which is to combine file scanning with checking that the message exists in the message store index (populated from queues at startup, and kept up to date over the life time of the store). Then we know for sure that the message above doesn't exist, because the MsgId won't be found in the index. If it is, then the file number and offset will not match, and the check will fail. There remains a small chance that we get it wrong during dirty recovery. Only a better file format would improve that.
lhoguin
force-pushed
the
loic-fix-cq-scan
branch
from
d863c71 to
639e905
Compare
lhoguin
changed the title
|
Rebased and marked ready for review. But I recommend merging this after #12425 just to be sure that everything is green. |
michaelklishin
added
backport-v3.13.x
backport-v4.0.x
and removed
backport-v3.13.x
backport-v4.0.x
labels
|
Two positive reports from two users in #12367:
|
|
#12425 was merged. |
michaelklishin
approved these changes
Oct 8, 2024
|
@Mergifyio backport v4.0.x |
✅ Backports have been created
|
michaelklishin
added a commit
that referenced
this pull request
Oct 8, 2024
CQ: Fix shared store scanner missing messages (backport #12392)
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.
Fix for #12367