Fix ComQueue dropping oldest Fw::Buffer without returning ownership (#5014)

* Fix ComQueue dropping oldest Fw::Buffer without returning ownership

When a buffer queue configured with QUEUE_DROP_OLDEST overflows, the
oldest queued Fw::Buffer was silently discarded by Queue::enqueue via
CircularBuffer::rotate without returning ownership through
bufferReturnOut. This leaked buffer-pool entries, eventually starving
dependent communication or telemetry paths.

Fix:
- Add optional 'discarded' output parameter to Types::Queue::enqueue()
  that captures the about-to-be-dropped message data via peek before
  the rotate.
- In ComQueue::enqueue(), pass an Fw::Buffer-sized output buffer for
  buffer queues and invoke bufferReturnOut_out() when DROP_OLDEST fires.

Closes #5011

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Add discarded_size assert to Queue::enqueue, use proper Fw::Buffer serialization, add UTs

- Queue::enqueue: added discarded_size parameter with FW_ASSERT validating
  the output buffer is large enough when discarded != nullptr
- ComQueue: switched buffer queues from raw reinterpret_cast<U8*>(&buffer)
  to proper serializeTo/deserializeFrom using Fw::Buffer::SERIALIZED_SIZE.
  This affects buffQueueIn_handler (serialize before enqueue), the discard
  path in enqueue(), processQueue() (deserialize after dequeue), and
  drainQueue() (deserialize after dequeue).
- Added 4 Queue UTs: DropOldestDiscardedOutput, DropOldestNullDiscarded,
  DropNewestIgnoresDiscarded, LIFODropOldestDiscardedOutput
- Added 2 ComQueue UTs: ComQueueDropOldestNoBufferReturn (nullptr discarded
  path), BufferQueueFlushAfterDropOldest (drainQueue deserialization)

All 14 Queue UTs and 25 ComQueue UTs pass.

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Run clang-format on changed files

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Revert Queue changes, fix ComQueue drop-oldest with pre-emptive dequeue

Reverted all Utils/Types/Queue changes (discarded parameter, UTs) per
review feedback — the fix now lives entirely in ComQueue.

ComQueue::enqueue() now checks if a buffer queue is full with DROP_OLDEST
before calling Queue::enqueue().  When full, it dequeues the oldest entry
first, returns buffer ownership via bufferReturnOut, then enqueues the new
message into the now-available slot.  This prevents buffer-pool leaks
without modifying the Queue API.

Also reverted the Fw::Buffer serialization changes (to be done in a
separate PR) and removed the added ComQueue/Queue UTs.

Updated existing testBufferQueueDropOldestMode assertion from 2 to 3
buffer returns to reflect the fix.

All 10 Queue UTs and 22 ComQueue UTs pass.

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Add Queue::popFront() to fix LIFO+DROP_OLDEST pre-emptive dequeue

Queue::dequeue() respects queue mode (LIFO removes from back), but
DROP_OLDEST always discards the front entry.  The pre-emptive dequeue
in ComQueue was using dequeue(), which would remove the wrong entry
for LIFO+DROP_OLDEST queues.

Added Queue::popFront() — always peeks offset 0 then rotates, matching
the rotate-based removal that Queue::enqueue() uses for DROP_OLDEST.
ComQueue::enqueue() now calls popFront() instead of dequeue() for the
pre-emptive overflow path.

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Rename deqStatus to dequeueStatus to fix spelling abbreviation

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Author: LeStarch

Svc/ComQueue/ComQueue.cpp

@@ -315,8 +315,34 @@ bool ComQueue::enqueue(const FwIndexType queueNum, QueueType queueType, const U8
         static_cast<FwIndexType>(queueNum - ((queueType == QueueType::COM_QUEUE) ? 0 : COM_PORT_COUNT));
     FW_ASSERT(expectedSize == size, static_cast<FwAssertArgType>(size), static_cast<FwAssertArgType>(expectedSize));
     FW_ASSERT(portNum >= 0, static_cast<FwAssertArgType>(portNum));
+
+    // For buffer queues with DROP_OLDEST, check if the queue is full before enqueuing.
+    // If full, dequeue the oldest entry first so we can return buffer ownership before
+    // Queue::enqueue() silently discards it via rotate.  This prevents buffer-pool leaks.
+    bool preEmptiveOverflow = false;
+    if (queueType == QueueType::BUFFER_QUEUE) {
+        Types::Queue& queue = this->m_queues[queueNum];
+        for (FwIndexType i = 0; i < TOTAL_PORT_COUNT; i++) {
+            if (this->m_prioritizedList[i].index == queueNum &&
+                this->m_prioritizedList[i].overflowMode == Types::QUEUE_DROP_OLDEST &&
+                queue.getQueueSize() >= this->m_prioritizedList[i].depth) {
+                // Queue is full and will drop oldest; remove the front entry to return ownership.
+                // popFront() always removes from the front (oldest) regardless of queue mode,
+                // matching the rotate-based removal that Queue::enqueue() uses for DROP_OLDEST.
+                Fw::Buffer droppedBuffer;
+                Fw::SerializeStatus dequeueStatus =
+                    queue.popFront(reinterpret_cast<U8*>(&droppedBuffer), sizeof(droppedBuffer));
+                FW_ASSERT(dequeueStatus == Fw::FW_SERIALIZE_OK, static_cast<FwAssertArgType>(dequeueStatus));
+                this->bufferReturnOut_out(portNum, droppedBuffer);
+                preEmptiveOverflow = true;
+                break;
+            }
+        }
+    }
+
     Fw::SerializeStatus status = this->m_queues[queueNum].enqueue(data, size);
-    if (status == Fw::FW_SERIALIZE_NO_ROOM_LEFT || status == Fw::FW_SERIALIZE_DISCARDED_EXISTING) {
+    if (preEmptiveOverflow || status == Fw::FW_SERIALIZE_NO_ROOM_LEFT ||
+        status == Fw::FW_SERIALIZE_DISCARDED_EXISTING) {
         if (!this->m_throttle[queueNum]) {
             this->log_WARNING_HI_QueueOverflow(queueType, portNum);
             this->m_throttle[queueNum] = true;

Svc/ComQueue/test/ut/ComQueueTester.cpp

@@ -829,10 +829,8 @@ void ComQueueTester ::testBufferQueueDropOldestMode() {
     // Verify we only have 2 messages
     ASSERT_from_dataOut_SIZE(2);
 
-    // Verify buffers were returned (2 sent + dropped buffer1)
-    // Note: When DROP_OLDEST happens, the dropped buffer should NOT be returned
-    // since it was consumed by the queue
-    ASSERT_from_bufferReturnOut_SIZE(2);
+    // Verify buffers were returned (2 sent + 1 dropped buffer1 returned for ownership)
+    ASSERT_from_bufferReturnOut_SIZE(3);
 
     component.cleanup();
 }

Utils/Types/Queue.cpp

@@ -90,6 +90,18 @@ Fw::SerializeStatus Queue::dequeue(U8* const message, const FwSizeType size) {
     }
 }
 
+Fw::SerializeStatus Queue::popFront(U8* const message, const FwSizeType size) {
+    FW_ASSERT(m_message_size > 0);
+    FW_ASSERT(m_message_size <= size, static_cast<FwAssertArgType>(size), static_cast<FwAssertArgType>(m_message_size));
+    FW_ASSERT(message != nullptr);
+    // Always remove from the front (oldest), regardless of queue mode
+    Fw::SerializeStatus result = m_internal.peek(message, m_message_size, 0);
+    if (result != Fw::FW_SERIALIZE_OK) {
+        return result;
+    }
+    return m_internal.rotate(m_message_size);
+}
+
 FwSizeType Queue::get_high_water_mark() const {
     FW_ASSERT(m_message_size > 0, static_cast<FwAssertArgType>(m_message_size));
     return m_internal.get_high_water_mark() / m_message_size;

Utils/Types/Queue.hpp

@@ -101,6 +101,20 @@ class Queue {
      */
     Fw::SerializeStatus dequeue(U8* const message, const FwSizeType size);
 
+    /**
+     * \brief removes and returns the oldest (front) message regardless of queue mode
+     *
+     * Unlike dequeue(), which respects the queue mode (FIFO vs LIFO), this method always
+     * removes from the front of the queue.  This is needed when callers must capture the
+     * oldest entry before it is discarded (e.g. returning buffer ownership on DROP_OLDEST
+     * overflow).
+     *
+     * \param message: buffer of at least m_message_size bytes to receive the oldest message
+     * \param size: size of the buffer being supplied
+     * \return: Fw::SERIALIZE_OK on success, something else on failure
+     */
+    Fw::SerializeStatus popFront(U8* const message, const FwSizeType size);
+
     /**
      * Return the largest tracked allocated size
      */