Repository navigation
UT[mqbs]: proxy VS removal must not release refs of unpushed Apps - #1810
Draft
chrisbeard wants to merge 5 commits into
Draft
chrisbeard wants to merge 5 commits into
chrisbeard wants to merge 5 commits into
Conversation
On a proxy, a message's refCount counts only the Apps it was pushed to, but every App slot of a new DataStreamMessage starts as e_PUT, which counts as pending. Removing the virtual storage of an App that never received the message still releases a reference, so a later CONFIRM from one App reaches zero references while another App holds the message. This test reproduces that bug in the storage layer with minimal step. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
Contributor
Author
|
Test failure, as expected: TEST /home/runner/work/blazingmq/blazingmq/src/groups/mqb/mqbs/mqbs_inmemorystorage.t.cpp CASE 22 |
On a proxy, a message's refCount includes only the Apps named in its PUSH. However, every App slot starts in e_PUT, which appears pending. When an App loses its last reader, the proxy removes its virtual storage. This wrongly releases references for messages that were never pushed to that App. The refCount then falls below the number of Apps that still hold the message. A subsequent CONFIRM can consequently delete the message while another App still needs it. If that App's copy is in the PushStream, the proxy aborts when it tries to load the deleted message. If the copy is in a redelivery list, the message is silently dropped. Add integration coverage for three variants: - An App closes while another App has a PushStream backlog. - An App crashes while another App has both PushStream and redelivery backlogs. - An App crashes while another App has only a redelivery backlog. The tests verify that the proxy remains alive and that every remaining App receives all its messages. Note that three apps are required to trigger the bug, the proxy does not assert for cases with only two Apps Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
On a proxy, a message's refCount equals the number of Apps that received that message in a PUSH. The proxy marks exactly those Apps as 'e_PUSH'. Every other App slot keeps the initial 'e_PUT', which counts as pending. When an App lost its last reader, 'purgeImpl' released a reference for every pending slot, so messages never pushed to that App lost a reference. A later CONFIRM from one App then removed the message while another App still had it queued, leading to silent message loss or an assert in 'PushStreamIterator'. Add 'VirtualStorageCatalog::holdsReference' and release references only for App slots that hold one. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
dorjesinpo
previously approved these changes
Oct 14, 2026
| const bool holdsRef = holdsReference( | ||
| appMessageView(*dataStreamMessage, vs->ordinal())); | ||
|
|
||
| if (vs->remove(dataStreamMessage, replacingOrdinal) && holdsRef) { |
Contributor
There was a problem hiding this comment.
It is important that remove gets called always.
Contributor
|
|
'remove' must run for every message, even when this App holds no reference, so explicitly call it on its own instead of inside of an if statement. After looking more closely though, the check can be simplified to just use 'holdsRef'; checking the return value of 'remove' is redundant. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
… other Apps' messages After a proxy restarts, a consumer can confirm messages it was pushed before the restart. If the restarted proxy was pushed those messages for other Apps only, the CONFIRM from the unpushed App still releases a reference, so a later CONFIRM from one of those Apps deletes a message another App still has queued. This can cause a consumer to incorrectly never receive the the message. test_confirm_after_proxy_restart reproduces this bug using three Apps. This test fails until the fix in a following commit. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
This branch has not been deployed
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
2 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.
On a proxy, a message's refCount counts only the Apps it was pushed to, but every App slot of a new DataStreamMessage starts as e_PUT, which counts as pending. Removing the virtual storage of an App that never received the message still releases a reference, so a later CONFIRM from one App reaches zero references while another App holds the message.
This test reproduces that bug in the storage layer with minimal step.