Repository navigation
MQTTManager: don't let new messages overtake ones buffered while disconnected - #403
Open
jamesmulcahy wants to merge 2 commits into
Open
jamesmulcahy wants to merge 2 commits into
jamesmulcahy wants to merge 2 commits into
Conversation
…onnected Messages published while disconnected from MQTT are buffered and sent after reconnecting. But publish() went back to sending live as soon as the client reported connected, while the reconnect still had to replay the retained values and subscribe to every topic (5 ms each) before it sent the buffer. Anything published in that window went out first and was then overwritten by an older buffered message. On a real install, a media player's state published before the first connection (only its ID and name, as Home Assistant had not answered yet) was buffered, the real state was published live half a second later, and the buffered placeholder then replaced it on the broker. The manager saw no change to send after that, so panels showed an empty media player for hours. The retained replay had the same problem: it published from a copy taken earlier, through publish(), so it could send an older value after a newer one and write the older value back into the retain buffer. is_connected() now also requires that the buffered messages have been sent. Until then publish() and clear_retain() buffer, behind what is already waiting. After connecting, the buffer is sent in order, then the latest value of every retained topic, all under the client lock, and only then are topics subscribed to again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ScopedErrorLog pushed its sink onto, and erased it from, the default logger's sink vector. spdlog does not lock that vector, and every Room runs a thread that logs for the rest of the run, so a capture starting or ending while one of those threads logged could reallocate the vector under it. That crashed a CI run in spdlog::logger::sink_it_, called from Room::_update_room_state(), between tests. main() now adds a single dist_sink to the default logger before any test runs, and ScopedErrorLog adds and removes its sink through that, which locks its own list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jamesmulcahy
marked this pull request as ready for review
October 17, 2026 20:22
This was referenced Oct 17, 2026
tpanajott
reviewed
Oct 18, 2026
|
|
||
| // While set, behave as if connected to the MQTT server, but record what would have been sent to it | ||
| // rather than sending it. Clearing it behaves as losing the connection. | ||
| static void test_set_broker_connected(bool connected) { |
Collaborator
There was a problem hiding this comment.
Should the test_* functions not be in a if defnied(TEST_MODE) && TEST_MODE == 1 clause?
Contributor
Author
There was a problem hiding this comment.
There's an existing #if starting on line 119 which these are also inside -- since the code just above is TEST specific too.
| ::testing::InitGoogleTest(&argc, argv); | ||
| spdlog::set_level(spdlog::level::warn); // Keep test output readable; raise when debugging. | ||
| // Before anything starts a thread that logs, see nspm_test::log_capture_sink(). | ||
| spdlog::default_logger()->sinks().push_back(nspm_test::log_capture_sink()); |
Collaborator
There was a problem hiding this comment.
Again, should this not be only defined for TEST_MODE?
Contributor
Author
There was a problem hiding this comment.
tests/ is only added under if(NSPM_BUILD_TESTS), and that's the same option that sets TEST_MODE=1, so main.cpp is never compiled without it.
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.
Problem
Messages published while MQTTManager is disconnected from MQTT are buffered and sent after it reconnects. But
publish()started sending live again as soon as the client reported connected. At that point the reconnect still had to replay the retained values and resubscribe to every topic, waiting 5 ms per topic, before it sent the buffer. Anything published in that window went out first, and was then overwritten by an older buffered message.Seen on a real install:
The retained replay had a similar problem. It published from a copy taken earlier, through
publish(), so it could send an older value after a newer one, and write the older value back into the retain buffer.Fix
is_connected()now also requires that the buffered messages have been sent. Until then,publish()andclear_retain()keep buffering, behind whatever is already waiting._send(). InTEST_MODEit can record messages instead of sending them.Tests
tests/mqtt_reconnect_test.cppcovers four cases:Notes
publish()and the end of_reconnect_mqtt_client(). Whichever lands second will need a small rebase._reconnect_mqtt_client()doesn't run. Messages published during the drop then stay buffered until the next full reconnect.🤖 Generated with Claude Code