From 4150f80740b22dc637b9d14f7b2d6a03bed6d69c Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:23:25 -0700 Subject: [PATCH 1/2] Svc::ComAggregator: bound timeout-signal enqueue in FILL state timeout_handler forwards a periodic (10 Hz in our configuration) timeout signal into the component's internal state-machine queue. PR #4402 added an m_allow_timeout guard that suppresses this signal while the state machine is in WAIT_STATUS, preventing queue overflow when a com transmission is slow. That guard does not cover the FILL state (m_allow_timeout == true): if the component's own dispatch thread stalls (e.g. downstream backpressure on a serial/radio link, or thread starvation under system load) for longer than queue_depth / timeout_rate, ticks keep arriving and filling the depth-bounded queue while nothing drains it. Once full, the autocoded signal-send in the state machine (aggregationMachine_sendSignal timeout) hits its FW_ASSERT on QUEUE_FULL, which is fatal in flight configurations. fill and status are each flow-controlled to at most one in-flight message by the com protocol, so timeout is the only unbounded producer feeding this queue. Since timeout ticks are periodic and idempotent (a missed tick is retried on the next cycle), skip forwarding the signal whenever the queue does not have headroom for it plus the (at most one each) in-flight fill/status signals, rather than only checking m_allow_timeout. Observed on hardware (PROVES CubeSat, RP2350/Zephyr, F' v3.1.1) during HWIL soak testing: a stalled dispatch thread (CDC-ACM host-side USB stall) let 10 Hz ticks fill a depth-15 queue in ~1.5 s, triggering the queue-full assert and a hard reboot. Generative AI (Claude, Anthropic) was used to help root-cause this defect during HWIL debugging and to draft this change; disclosed per AI_POLICY.md in the upstream pull request description. --- Svc/ComAggregator/ComAggregator.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/Svc/ComAggregator/ComAggregator.cpp b/Svc/ComAggregator/ComAggregator.cpp index c4edf6d8be7..2e313f99600 100644 --- a/Svc/ComAggregator/ComAggregator.cpp +++ b/Svc/ComAggregator/ComAggregator.cpp @@ -8,6 +8,12 @@ namespace Svc { +namespace { +//! Queue slots that must remain free for a timeout signal to be enqueued: the timeout itself plus one +//! in-flight 'fill' and one in-flight 'status' signal (each bounded to one message by the com protocol). +constexpr FwSizeType TIMEOUT_QUEUE_HEADROOM = 3; +} // namespace + // ---------------------------------------------------------------------- // Component construction and destruction // ---------------------------------------------------------------------- @@ -55,7 +61,16 @@ void ComAggregator ::timeout_handler(FwIndexType portNum, U32 context) { // // Behaviorally, this solution will work exactly like the naive implementation with an infinite queue depth, but // prevents queue overflow when using finite queues. - if (this->m_allow_timeout) { + // + // Even so, timeout remains the only signal source without flow control: 'fill' and 'status' are each bounded + // to one in-flight message by the com protocol, but the rate group keeps delivering ticks while this + // component's dispatch thread is stalled (downstream backpressure, thread starvation). In the FILL state + // (m_allow_timeout true) such a stall would still fill the queue with timeout signals and trip the queue-full + // assertion in the autocoded signal send. Ticks are periodic and idempotent, so additionally skip the signal + // unless the queue has headroom for it plus the in-flight flow-controlled signals; a skipped tick is simply + // retried on the next cycle. + if (this->m_allow_timeout && + (this->m_queue.getMessagesAvailable() + TIMEOUT_QUEUE_HEADROOM <= this->m_queue.getDepth())) { this->aggregationMachine_sendSignal_timeout(); } } From 97e1501ae65cf1c498ee1cc9188ce3eb63e42f22 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:23:40 -0700 Subject: [PATCH 2/2] Svc::ComAggregator: add UT for FILL-state timeout-flood headroom check The existing test_timeout_overflow_prevention only exercises the WAIT_STATUS-adjacent path: it drives a comStatusIn failure first, so m_allow_timeout is already false during the flood. It does not put the state machine in FILL and stall dispatch, so it does not cover the FILL-state gap closed by the previous commit. Add test_timeout_overflow_prevention_fill_state(), the complement of the existing test: run test_initial() to land in FILL (m_allow_timeout == true) with an empty aggregation buffer and empty queue, then invoke timeout 2 * TEST_INSTANCE_QUEUE_DEPTH times back-to-back with no intervening drain -- simulating a stalled dispatch thread receiving periodic sched ticks. After every invocation, assert the queue never reaches full depth. Then confirm the flood had no side effects (dataOut never fired), confirm recovery via dispatchCurrentMessages once the simulated stall clears, and confirm normal fill/timeout operation resumes cleanly afterward. Note: dispatchOne's underlying doDispatch() issues a blocking Os::Queue::receive with no "queue empty" return path of its own (MSG_DISPATCH_EMPTY is only produced by dispatchCurrentMessages, which snapshots the message count up front). Draining must use dispatchCurrentMessages rather than looping dispatchOne until an empty status, which would hang. Verified: with the bounded-timeout fix applied, all 11 ComAggregator UTs pass (including the new test). With the fix reverted, the new test fails deterministically at the 20th flood iteration (getMessagesAvailable() == TEST_INSTANCE_QUEUE_DEPTH == 20), confirming it exercises the regression this fix addresses. Generative AI (Claude, Anthropic) was used to help draft this test during HWIL follow-up work; disclosed per AI_POLICY.md in the upstream pull request description. --- .../test/ut/ComAggregatorTestMain.cpp | 6 +++ .../test/ut/ComAggregatorTester.cpp | 42 +++++++++++++++++++ .../test/ut/ComAggregatorTester.hpp | 4 ++ 3 files changed, 52 insertions(+) diff --git a/Svc/ComAggregator/test/ut/ComAggregatorTestMain.cpp b/Svc/ComAggregator/test/ut/ComAggregatorTestMain.cpp index 5c118ae0087..44bd5914aca 100644 --- a/Svc/ComAggregator/test/ut/ComAggregatorTestMain.cpp +++ b/Svc/ComAggregator/test/ut/ComAggregatorTestMain.cpp @@ -63,6 +63,12 @@ TEST(OffNominal, TimeoutOverflowPrevention) { tester.test_timeout(); } +TEST(OffNominal, TimeoutOverflowPreventionFillState) { + Svc::ComAggregatorTester tester; + tester.test_initial(); + tester.test_timeout_overflow_prevention_fill_state(); +} + TEST(Nominal, HoldWhileWaiting) { Svc::ComAggregatorTester tester; tester.test_initial(); diff --git a/Svc/ComAggregator/test/ut/ComAggregatorTester.cpp b/Svc/ComAggregator/test/ut/ComAggregatorTester.cpp index 98c2055fab5..b41ee15dd82 100644 --- a/Svc/ComAggregator/test/ut/ComAggregatorTester.cpp +++ b/Svc/ComAggregator/test/ut/ComAggregatorTester.cpp @@ -266,6 +266,48 @@ void ComAggregatorTester ::test_timeout_overflow_prevention() { this->clearHistory(); } +//! Tests that a stalled dispatch thread in the FILL state cannot flood the queue with timeout +//! signals past its depth, and that the component recovers cleanly once draining resumes +void ComAggregatorTester ::test_timeout_overflow_prevention_fill_state() { + // Precondition: initial has run. The state machine is now in the FILL state + // (m_allow_timeout == true) with an empty aggregation buffer and an empty queue. Unlike + // test_timeout_overflow_prevention (which drives m_allow_timeout to false via a comStatusIn + // failure before flooding, exercising the WAIT_STATUS-adjacent gate from the pre-existing + // fix), this test floods while m_allow_timeout stays true, exercising the FILL-state gap. + ASSERT_EQ(this->component.m_queue.getMessagesAvailable(), 0); + + // Simulate a stalled dispatch thread: the rate group keeps delivering 'timeout' ticks (this + // port is invoked directly here, exactly as a rate-group member call would) while nothing + // drains the component's message queue via dispatchOne. Loop well past the queue depth -- + // without the FILL-state headroom check, this would keep enqueueing right up to depth and + // trip the autocoded queue-full assertion inside aggregationMachine_sendSignal_timeout. + for (FwSizeType i = 0; i < static_cast(TEST_INSTANCE_QUEUE_DEPTH) * 2; i++) { + this->invoke_to_timeout(0, 0); + // The queue must never be allowed to reach its full depth: headroom is reserved for the + // (at most one each) in-flight 'fill'/'status' signals, so FILL-state flooding cannot trip + // the queue-full assert no matter how long the simulated stall runs. + ASSERT_LT(this->component.m_queue.getMessagesAvailable(), static_cast(TEST_INSTANCE_QUEUE_DEPTH)); + } + // With the aggregation buffer empty, isNotEmpty is false, so every dispatched timeout is a + // no-op transition (see test_timeout_zero); flooding has no side effects of its own. + ASSERT_from_dataOut_SIZE(0); + + // Confirm recovery once the (simulated) dispatch stall clears and draining resumes: every + // queued signal is still processable without error, and the queue returns to empty. + // (dispatchOne's underlying doDispatch() issues a BLOCKING queue receive, so drain exactly the + // snapshotted number of queued messages via dispatchCurrentMessages rather than looping until + // an "empty" status that a blocking dispatch will never produce.) + ASSERT_EQ(this->dispatchCurrentMessages(this->component), + Svc::ComAggregatorComponentBase::MsgDispatchStatus::MSG_DISPATCH_OK); + ASSERT_EQ(this->component.m_queue.getMessagesAvailable(), 0); + ASSERT_from_dataOut_SIZE(0); + this->clearHistory(); + + // Normal fill/timeout operation resumes cleanly after the flood and drain. + this->test_fill_multi(); + this->test_timeout(); +} + void ComAggregatorTester ::test_timeout_zero() { // Precondition: initialize has run this->invoke_to_timeout(0, 0); diff --git a/Svc/ComAggregator/test/ut/ComAggregatorTester.hpp b/Svc/ComAggregator/test/ut/ComAggregatorTester.hpp index 1ea0a1dcc23..856a323f455 100644 --- a/Svc/ComAggregator/test/ut/ComAggregatorTester.hpp +++ b/Svc/ComAggregator/test/ut/ComAggregatorTester.hpp @@ -65,6 +65,10 @@ class ComAggregatorTester final : public ComAggregatorGTestBase { //! Tests timeout operation void test_timeout_overflow_prevention(); + //! Tests that a stalled dispatch thread in the FILL state cannot flood the queue with timeout + //! signals past its depth, and that the component recovers cleanly once draining resumes + void test_timeout_overflow_prevention_fill_state(); + //! Tests timeout operation sends no empty buffer void test_timeout_zero();