Issues found during a review of the standard modes protocol support (StandardModes / RequestMessageCoordinator / MavCommandQueue), follow-on to #15007.
1. Received mode_index is not validated against the requested index (correctness bug)
StandardModes::gotMessage() trusts the mode_index in the received AVAILABLE_MODES message without checking it against the index actually requested.
Concrete failure: a _wantReset restart (triggered by availableModesMonitorReceived) at index 22 of 22 sends requestMode(1); the stale index-22 response then arrives and RequestMessageCoordinator matches it by msgId only and delivers it as the response to the new request. mode_index >= number_modes then looks like completion, so requestCompleted fires with a 1-entry (garbage) mode list.
Fix: track the requested index and discard/re-request on mismatch.
2. Unbounded restarts while the vehicle's mode set is changing
availableModesMonitorReceived() restarts the whole download every time the AVAILABLE_MODES_MONITOR seq changes. If external/ROS2 modes register one-by-one during vehicle boot, the download restarts repeatedly; with the initial connect state machine now waiting indefinitely on requestCompleted, the connect sequence can stay in RequestStandardModes for an extended time. Terminates once the mode set stabilizes, but the restart-from-scratch behavior amplifies the cost on slow links (each restart re-downloads all modes one round trip at a time).
3. Teardown paths drop pending result handlers silently
MavCommandQueue::stop() and RequestMessageCoordinator::stop() clear pending entries without invoking their result handlers. Currently only reachable during vehicle teardown (all links removed), so nothing user-visible today — but any future runtime caller of stop() would strand waiters (e.g. the initial connect state machine) with no completion callback. Consider failing pending handlers on stop.
Refs: src/Vehicle/StandardModes.cc, src/Vehicle/RequestMessageCoordinator.cc, src/Vehicle/MavCommandQueue.cc
Issues found during a review of the standard modes protocol support (
StandardModes/RequestMessageCoordinator/MavCommandQueue), follow-on to #15007.1. Received mode_index is not validated against the requested index (correctness bug)
StandardModes::gotMessage()trusts themode_indexin the received AVAILABLE_MODES message without checking it against the index actually requested.Concrete failure: a
_wantResetrestart (triggered byavailableModesMonitorReceived) at index 22 of 22 sendsrequestMode(1); the stale index-22 response then arrives andRequestMessageCoordinatormatches it by msgId only and delivers it as the response to the new request.mode_index >= number_modesthen looks like completion, sorequestCompletedfires with a 1-entry (garbage) mode list.Fix: track the requested index and discard/re-request on mismatch.
2. Unbounded restarts while the vehicle's mode set is changing
availableModesMonitorReceived()restarts the whole download every time the AVAILABLE_MODES_MONITOR seq changes. If external/ROS2 modes register one-by-one during vehicle boot, the download restarts repeatedly; with the initial connect state machine now waiting indefinitely onrequestCompleted, the connect sequence can stay in RequestStandardModes for an extended time. Terminates once the mode set stabilizes, but the restart-from-scratch behavior amplifies the cost on slow links (each restart re-downloads all modes one round trip at a time).3. Teardown paths drop pending result handlers silently
MavCommandQueue::stop()andRequestMessageCoordinator::stop()clear pending entries without invoking their result handlers. Currently only reachable during vehicle teardown (all links removed), so nothing user-visible today — but any future runtime caller ofstop()would strand waiters (e.g. the initial connect state machine) with no completion callback. Consider failing pending handlers on stop.Refs:
src/Vehicle/StandardModes.cc,src/Vehicle/RequestMessageCoordinator.cc,src/Vehicle/MavCommandQueue.cc