Repository navigation
fix(c8y): avoid duplicate software_list requests - #4361
sankalpsthakur wants to merge 3 commits into
Conversation
This is the crux. If the capability message is delivered before one of the pending Even though the order of topic filters can be used with mosquitto to influence the order of delivery of retained messages (didn't know that, but I was able to verify it with claude, very cool), it's not something the MQTT spec requires, but mosquitto implementation detail, which could change or not apply to other brokers. Even though we use only mosquitto, I'm not sure if depending on this is the best approach. Also, even without this change, after the mapper is able to start without fail, it clears the all stale
EDIT: Nevermind, after retrying a few more times, approximately once per 3 runs I get the capability message first which results in the new command being published: This is too flaky to be considered a complete fix, either other approaches have to be considered, or we could reconsider if avoiding duplicate commands in a scenario where the mapper is already degraded is necessary as originally proposed. Failing test is unrelated, seems to be flaky, I'll run tests again and investigate the failing one to potentially fix. |
|
thanks for running it repeatedly. that's the gap the caveat in the description pointed at, and i agree the fix shouldn't depend on subscribe or delivery order. an order-independent version i'd propose: when the capability message arrives, don't mint right away. set a short timeout instead (the availability actor in this crate already uses tedge_timer_ext), and when it fires, mint only if no if you'd rather drop it, since a healthy mapper already clears the stale commands, i'm fine closing it. happy to wait for jarhodes314 on monday. |
|
I have now pushed a revised fix to this PR that waits for the retained messages to arrive before sending the software list, along with a robot test to test for the bug in practice. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
|
merged main for the nfpm rpm fix, ptal |
The c8y mapper requests a software list each time it receives the `te/<entity>/cmd/software_update` capability message. As this message is retained, every start of the mapper mints a new `software_list` command with a fresh `c8y-mapper-<timestamp>` id. A mapper stuck in a crash-restart loop mints one command per restart and never clears any of them, so the agent ends up executing an unbounded number of them. Keep track of the software list commands issued by the mapper, from the moment their retained messages are received back from the broker until they are cleared, and skip the request when one is still pending for the target entity, whatever its status: non-terminal, or terminal but not yet cleared. This relies on the same source of information as `active_commands`: the command instances created by the mapper are delivered back as retained messages on the same connection. Refs thin-edge#4255 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
…ceived The previous fix checks for a pending software list request as soon as the retained capability message arrives, which can be before the retained `software_list` commands from a previous run. The request is then still duplicated on every restart. Only request software lists once no retained message has been received for one second, so the pending requests are known. Refs thin-edge#4255 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Rhodes <jarhodes314@gmail.com>
7047464 to
b997aec
Compare
Robot Results
Failed Tests
|
Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Proposed changes
The c8y mapper requests a software list every time it receives the retained
te/<entity>/cmd/software_updatecapability message, minting a newsoftware_listcommand with a freshc8y-mapper-<timestamp>id.A mapper in a crash-restart loop therefore mints one command per restart and
never clears any of them (#4255).
This change makes the mapper keep track of the
software_listcommands it hasissued, from the moment their retained messages are received back from the
broker until they are cleared, and skip the request when one is still pending
for the target entity, whatever its status (non-terminal, or terminal but not
yet cleared). Only commands carrying the mapper's own id prefix are considered,
and the check is per target entity.
Implementation notes:
CumulocityConvertergets apending_software_list_requests: HashMap<CmdId, EntityTopicId>maintained next to
active_commands: populated in the arm handling themapper's own command instances (retained or echoed), emptied by the clearing
message.
register_software_update_operationconsults it before callingrequest_software_listand logs the pending command id when it skips.software_updateoperation(
operations/handlers/software_update.rs) is intentionally left untouched:a refresh after a change of the installed software is wanted.
re-trigger a request at all) is not implemented here: the system test
mapper-publishing-agent-supported-ops.robot::Agent gets the software list request once it comes upexpects a new request after a restart of the agent, i.e. on unchanged
metadata, and
software.robot::Software list should be populated during startuprelies on the refresh after a mapper restart. Happy to follow up if the
expected behaviour should change.
received before the retained capability message depends on the order of the
topic filters in the SUBSCRIBE packet. The mapper lists
AnyCommandbeforeAnyCommandMetadata, buttedge_mqtt_ext::MqttActorBuilder::build_actorbuilds the initial filter list from a
HashSet, so the order is notguaranteed. When the capability message is delivered first the behaviour is
unchanged (one request is made); when the command instances are delivered
first, no duplicate is minted.
Types of changes
Paste Link to the issue
Refs #4255: #4255
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINES (cargo fmt -p c8y_mapper_ext -- --checkis clean)just checkas mentioned in CODING_GUIDELINES (onlycargo clippy -p c8y_mapper_ext --testswas run locally, see below)Further comments
Seven unit tests added in
converter.rs: the existing behaviour (request on capability), no request while a previous one is pending (init,executing,successfulbut uncleared), request again once cleared, and two guards (a request not issued by the mapper, or pending for another device, does not block a new one).Run locally (crate scope):
cargo fmt --check,cargo check --testsandcargo clippy --testsare clean;cargo test -p c8y_mapper_ext software_list_gives 7 passed. With the new check disabled, the 3 "not requested while pending" cases fail with a secondsoftware_list/c8y-mapper-<ts>request. The fulljust checkand the Robot system tests were not run locally.AI/LLM disclosure