[Bug] Fix stale queue-dispatch pending flag after synchronous dispatch - #502
Merged
Conversation
QueueMessagesDispatcher marked the dispatch as pending AFTER the message bus dispatch. A synchronously dispatched DispatchQueueMessagesMessage is handled inline (sync:// transport), and the handler clears the pending state in its finally block - so the flag was re-created afterwards and, with no message left to clear it, suppressed every subsequent save-triggered queue dispatch for the whole TmpStore lifetime (5 min). The same race exists for asynchronous dispatches when a worker handles the message before markAsPending() runs. Mark the dispatch as pending before dispatching and clear it again if the dispatch itself fails. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes stale queue-dispatch state caused by synchronous or fast asynchronous message handling.
Changes:
- Marks dispatches pending before sending.
- Clears pending state if dispatch throws.
- Adds functional regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Service/SearchIndex/IndexQueue/QueueMessagesDispatcher.php |
Corrects pending-state lifecycle. |
tests/Functional/SearchIndex/QueueMessagesDispatcherTest.php |
Adds regression coverage for synchronous dispatch. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ed-dispatch rollback Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Problem
QueueMessagesDispatcher::dispatchQueueMessages()marks the dispatch as pending after dispatching the message. A synchronous dispatch (update:index,deployment:reindex,UpdateLanguageSettingsHandler, and the installer via the post-install command) is handled inline on thesync://transport, andDispatchQueueMessagesHandlerclears the pending state in itsfinallyblock — i.e. beforemarkAsPending()runs. The stale flag then suppresses every save-triggered queue dispatch (messageShouldBeTriggered()returnsfalse) for the whole TmpStore lifetime of 5 minutes. The same race exists for asynchronous dispatches when a worker consumes the message beforemarkAsPending()executes.Practical impact: for up to 5 minutes after
generic-data-index:update:index(or a freshpimcore-install, which runs it as post-install command), queue entries created by element saves are not dispatched — reproducible on a clean skeleton install (found while verifying Elasticsearch 8.12.2 for pimcore/internal-improvements#25: seeded elements' follow-up queue entries stayed undispatched withdispatched = 0while the stalegeneric_data_index_queue_messages_dispatch_pendingTmpStore entry existed).Fix
Mark the dispatch as pending before dispatching, and clear the flag again if the dispatch itself throws. For synchronous dispatches the inline handler then clears the flag as the last step (correct end state); for asynchronous dispatches the flag can no longer race with a fast worker.
Regression test
tests/Functional/SearchIndex/QueueMessagesDispatcherTest— red on the previous code (pendingMessageExists()stayedtrueafter a synchronous dispatch), green with the fix.🤖 Generated with Claude Code