Skip to content

Fix[mqb]: Handle orphan confirms - #1195

Merged
dorjesinpo merged 4 commits into
mainfrom
fix/orphan-confirms
Mar 27, 2026
Merged

Fix[mqb]: Handle orphan confirms#1195
dorjesinpo merged 4 commits into
mainfrom
fix/orphan-confirms

Conversation

@dorjesinpo

Copy link
Copy Markdown
Collaborator
  1. Do not break on orphan (auto) confirm
  2. Do not send auto confirms if message refcount is 0

@dorjesinpo
dorjesinpo requested a review from a team as a code owner March 15, 2026 15:08
@dorjesinpo dorjesinpo added the bug Something isn't working label Mar 15, 2026
@dorjesinpo
dorjesinpo force-pushed the fix/orphan-confirms branch from 8843758 to 60bcf04 Compare March 15, 2026 15:59
@dorjesinpo dorjesinpo changed the title Handle orphan confirms Fix[mqb]: Handle orphan confirms Mar 15, 2026
@dorjesinpo
dorjesinpo force-pushed the fix/orphan-confirms branch from 60bcf04 to ea8e700 Compare March 15, 2026 17:32
Comment thread src/groups/mqb/mqbi/mqbi_queueengine.h Outdated
Comment thread src/groups/mqb/mqbmock/mqbmock_queueengine.cpp Outdated
Comment thread src/groups/mqb/mqbmock/mqbmock_queueengine.h Outdated
Comment thread src/groups/mqb/mqbmock/mqbmock_queueengine.h Outdated
Comment thread src/groups/mqb/mqbi/mqbi_queueengine.h Outdated
d_autoConfirms.emplace_back(appKey, handle);

return mqbi::StorageResult::e_SUCCESS;
d_autoConfirms.emplace_back(appKey, DataStoreRecordHandle());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that d_autoConfirms now keeps an invalid DataStoreRecordHandle.
What if that record handle never becomes valid (we never call FileBackedStorage::put) and we try to remove it in FileBackedStorage?

void FileBackedStorage::clearSelection()
{
for (AutoConfirms::const_iterator it = d_autoConfirms.begin();
it != d_autoConfirms.end();
++it) {
d_store_p->removeRecordRaw(it->d_confirmRecordHandle);
}
d_autoConfirms.clear();
d_currentlyAutoConfirming = bmqt::MessageGUID();
}

This might happen when all app subscriptions are negative (so all are auto confirmed) and we try to move to the next message.
See the integration test, I've posted a code update that causes broker to crash

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectForAutoConfirming should not call clearSelection
d_autoConfirms are used in two mutually exclusive ways - by RootQueueEngine and by replication.
The former never stores valid handles.

Comment thread src/groups/mqb/mqbs/mqbs_filebackedstorage.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_filebackedstorage.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_filebackedstorage.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_filebackedstorage.cpp Outdated
Comment thread src/integration-tests/test_app_subscriptions.py
@dorjesinpo
dorjesinpo force-pushed the fix/orphan-confirms branch from ea8e700 to 2a24779 Compare March 25, 2026 01:13
@dorjesinpo

Copy link
Copy Markdown
Collaborator Author

Fixes for IT failures:
#1175
#1193

@dorjesinpo
dorjesinpo force-pushed the fix/orphan-confirms branch from 2a24779 to b0a70f4 Compare March 25, 2026 19:15
@dorjesinpo dorjesinpo self-assigned this Mar 25, 2026
@dorjesinpo
dorjesinpo force-pushed the fix/orphan-confirms branch 2 times, most recently from 7224cce to 60590c2 Compare March 25, 2026 21:00
@dorjesinpo
dorjesinpo requested a review from kaikulimu March 26, 2026 15:36
Comment thread src/groups/mqb/mqbs/mqbs_filebackedstorage.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_virtualstoragecatalog.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_virtualstoragecatalog.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_virtualstoragecatalog.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_virtualstoragecatalog.cpp Outdated
Comment thread src/groups/mqb/mqbs/mqbs_filebackedstorage.cpp Outdated
@kaikulimu

kaikulimu commented Mar 26, 2026

Copy link
Copy Markdown
Collaborator

I only reviewed mqbs_filestore.cpp, and left one comment (as a reply to Evgenii's comment). The rest looks good.

Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
@dorjesinpo
dorjesinpo force-pushed the fix/orphan-confirms branch from 1a6ae4c to 7c082fd Compare March 26, 2026 20:03
@dorjesinpo dorjesinpo assigned 678098 and unassigned dorjesinpo Mar 27, 2026
@678098 678098 assigned dorjesinpo and unassigned 678098 Mar 27, 2026
@dorjesinpo
dorjesinpo merged commit 4af4746 into main Mar 27, 2026
45 of 48 checks passed
@dorjesinpo
dorjesinpo deleted the fix/orphan-confirms branch March 27, 2026 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants