Fix PR #5205 CI failures: format, spelling, macOS portability, and PriorityMemQueue sanitizer/UT issues#173
Conversation
…re conflicts Adopt the reviewed CountingSemaphore API from nasa#5204 (now merged into devel) and adapt PriorityMemQueue/AtomicQueue to the single-argument constructor. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…Queue UTs
- priority_buffer_analyzer.py: replace Python 3.10+ match/case with
if/elif/isinstance so black 21.6b0 (pinned in CI) can parse and format it.
- expect.txt: add domain terms used by the new PriorityMemQueue/AtomicQueue
code and remove forbidden camelCase entries (SafeRTOS, ThreadX).
- PriorityMemQueue UTs: fix pre-existing shadow-model bugs that made the
random/edge tests flaky (intermittent FAIL/HANG):
* isFull() means all enabled priorities full, so FillQueue/Send/SendFull
must target an enabled priority that still has room; a (blocking) send to
an already-full priority would block forever.
* Receive only fires when a message is held by an enabled priority
(messages in disabled priorities are unreachable by the real queue), and
the shadow receive now picks the highest enabled priority FIFO and
returns EMPTY when none are reachable.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Original prompt from thomas.boyer.chammard
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
check-spelling ignores the needed plural entries 'bitmasks'/'spinlocks' (used by the new PriorityMemQueue code) as redundant variants because the dictionary-covered singular forms 'bitmask'/'spinlock' were also listed, leaving the plural code tokens unrecognized. Remove the singular entries so the plural ones become active. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| virtual ~PriorityMemQueue(); | ||
|
|
||
| //! \brief copy constructor is forbidden | ||
| PriorityMemQueue(const QueueInterface& other) = delete; |
| PriorityMemQueue(const QueueInterface& other) = delete; | ||
|
|
||
| //! \brief copy constructor is forbidden | ||
| PriorityMemQueue(const QueueInterface* other) = delete; |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| // Bounded loop with compile-time limit for JPL Power of Ten compliance | ||
| for (U32 reps = 0; reps < LOOP_GUARD_LIMIT; ++reps) { | ||
| // Get enabled priorities and non-empty hint mask | ||
| U32 enabledPriorities = this->m_handle.m_priorityMask.load(std::memory_order_acquire); | ||
| U32 nonEmptyHint = this->m_handle.m_nonEmptyMask.load(std::memory_order_acquire); | ||
|
|
||
| // Combine masks: only scan priorities that are both enabled and likely non-empty | ||
| U32 scanMask = enabledPriorities & nonEmptyHint; | ||
|
|
||
| // Scan priorities and attempt dequeue | ||
| if (scanAndDequeue(this->m_handle, destination, capacity, actualSize, priority, scanMask)) { | ||
| return QueueInterface::Status::OP_OK; | ||
| } | ||
|
|
||
| // No message available | ||
| if (blockType == QueueInterface::BlockingType::BLOCKING) { | ||
| // Wait for a message | ||
| FW_ASSERT(this->m_handle.m_notEmptySem != nullptr, this->m_handle.m_id); | ||
| Os::CountingSemaphoreInterface::Status semStatus = this->m_handle.m_notEmptySem->wait(); | ||
| FW_ASSERT(semStatus == Os::CountingSemaphoreInterface::Status::OP_OK, | ||
| static_cast<FwAssertArgType>(semStatus)); | ||
| } else { | ||
| // Non-blocking, return empty | ||
| return QueueInterface::Status::EMPTY; | ||
| } | ||
| } | ||
|
|
||
| // Should never reach here - loop guard prevents infinite loop | ||
| FW_ASSERT(false, this->m_handle.m_id, LOOP_GUARD_LIMIT); | ||
| return QueueInterface::Status::UNKNOWN_ERROR; |
There was a problem hiding this comment.
🔴 Unbounded counting semaphore accumulation in receive() causes FW_ASSERT after sustained throughput
Every send() posts to m_notEmptySem (line 634), but receive() only consumes a semaphore credit via wait() when scanAndDequeue fails (line 734). When scanAndDequeue succeeds on the first iteration (message was already available), the function returns OP_OK at line 727 without consuming the corresponding semaphore credit.
This causes the semaphore count to monotonically increase during sustained throughput: each message that is dequeued without the receiver needing to block leaves one unconsumed credit. When traffic eventually stops and a blocking receive encounters an empty queue, it enters a spin loop — each iteration consumes one stale credit via wait(), immediately returns, scans, finds nothing, and loops again. If the accumulated credits exceed LOOP_GUARD_LIMIT (2000), the receiver hits the FW_ASSERT(false, ...) at line 744.
Triggering scenario
In a typical F' active component with a dispatch loop calling receive(BLOCKING), if >2000 messages are processed without the receiver ever needing to block (e.g., messages arrive faster than processing time, or multiple senders keep the queue non-empty), the semaphore accumulates >2000 credits. The next quiet period triggers the assert.
At 100 messages/second this takes ~20 seconds of sustained throughput without a single gap.
Prompt for agents
The counting semaphore m_notEmptySem in PriorityMemQueue accumulates unbounded credits because send() always posts but receive() only consumes via wait() when the queue appears empty. When messages are dequeued on the fast path (scanAndDequeue succeeds on first try), no credit is consumed.
The fix needs to ensure credits don't accumulate unboundedly. Two possible approaches:
1. Consume a semaphore credit on the success path: After scanAndDequeue succeeds (line 726-728), call m_notEmptySem->tryWait() to consume the credit that was posted by the corresponding send(). This mirrors how AtomicQueue handles its m_notFullSem (tryWait after non-blocking enqueue).
2. Replace the bounded loop (LOOP_GUARD_LIMIT) with a design where spurious credits are harmless. For example, use a binary semaphore or condition-variable pattern instead of a counting semaphore, where the receiver always checks the actual queue state after waking.
Approach 1 is the simpler fix: add a tryWait() call right before `return QueueInterface::Status::OP_OK` at line 727. This keeps the semaphore count approximately tracking actual messages available. The tryWait may fail under concurrency (another receiver consumed it), which is acceptable since the semaphore is just a notification mechanism.
Relevant files: Os/Generic/PriorityMemQueue.cpp (receive function around line 716-745, send function around line 630-635)
Was this helpful? React with 👍 or 👎 to provide feedback.
| void PriorityMemQueueHandle::enablePriority(FwQueuePriorityType priority) { | ||
| FW_ASSERT(priority < Os::Generic::Queue::MAX_PRIORITIES, priority, this->m_id); | ||
| // Enabling a priority is only allowed if it's in use | ||
| FW_ASSERT(this->m_atomicQueues != nullptr, this->m_id, priority); | ||
|
|
||
| // MEMORY ORDERING: seq_cst for control path operations ensures total ordering | ||
| // Atomic update of priority mask using fetch_or with seq_cst (control path) | ||
| this->m_priorityMask.fetch_or(priorityBitMask(priority), std::memory_order_seq_cst); | ||
| } |
There was a problem hiding this comment.
🚩 enablePriority lacks bounds check against m_maxPriority
The enablePriority() function at line 151 asserts priority < MAX_PRIORITIES (16) but does NOT check priority <= m_maxPriority. This means a user can enable a priority bit that has no corresponding allocated AtomicQueue in the array. This is not a crash bug because: (1) scanAndDequeue at Os/Generic/PriorityMemQueue.cpp:655 only iterates up to handle.m_maxPriority, so bits beyond that are ignored during receive, and (2) send() uses resolvePriorityQueue which checks against m_maxPriority and falls back to DEFAULT. However, it's a design inconsistency — enabling an out-of-bounds priority silently does nothing, which could confuse users debugging priority issues.
Was this helpful? React with 👍 or 👎 to provide feedback.
Cast FwSizeType arguments to size_t in %zu printf statements so the PriorityMemQueue unit test compiles cleanly under -Werror,-Wformat on platforms where FwSizeType is unsigned long long (e.g. macOS arm64). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The unit test hard-coded Os_CountingSemaphore_Posix (and Os_Mutex_Posix) in CHOOSES_IMPLEMENTATIONS, which fails to link on Darwin where the counting semaphore implementation is Os_CountingSemaphore_Darwin. Drop the platform-specific picks and rely on the platform config defaults (matching the cross-platform Types_Atomic_Queue_test), keeping only the Os_Generic_PriorityMemQueue queue implementation under test. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
teardownInternal() rescanned the caller-owned s_configs array to find and release its slot. Since configure() only borrows that pointer, the array can be out of scope by the time the queue destructor runs (caught by ASan as stack-use-after-scope on macOS/Coverage builds). Capture the claimed slot index at create() time and release it via the internally-allocated s_configsUsed array, never dereferencing s_configs during teardown. This also makes teardown idempotent across an explicit teardown() plus the destructor. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Coverage report — base
|
| Module | Line | Function | Branch |
|---|---|---|---|
CFDP/Checksum |
71.15 | 57.14 | 53.85 |
Drv/AsyncByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/ByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/Ip |
44.32 | 57.38 | 25.36 |
Drv/TcpClient |
75.00 | 100.00 | 47.37 |
Drv/TcpServer |
84.72 | 100.00 | 60.00 |
Drv/Udp |
68.09 | 90.91 | 42.86 |
Fw/Buffer |
81.25 | 89.47 | 58.62 |
Fw/DataStructures |
98.48 | 96.76 | 83.21 |
Fw/Dp |
94.83 | 96.67 | 95.95 |
Fw/FilePacket |
75.24 | 89.06 | 54.30 |
Fw/Log |
75.71 | 84.21 | 60.71 |
Fw/Logger |
100.00 | 100.00 | 100.00 |
Fw/SerializableFile |
90.00 | 100.00 | 79.41 |
Fw/Time |
88.62 | 85.48 | 86.84 |
Fw/Tlm |
53.76 | 60.00 | 37.50 |
Fw/Types |
53.84 | 56.01 | 34.32 |
Os |
14.93 | 17.70 | 11.87 |
Os/Generic |
87.95 | 88.16 | 69.69 |
Os/Generic/Types |
92.39 | 92.86 | 71.29 |
Os/Posix |
62.00 | 84.21 | 44.10 |
Svc/ActiveRateGroup |
100.00 | 100.00 | 92.31 |
Svc/ActiveTextLogger |
79.05 | 90.00 | 71.23 |
Svc/AssertFatalAdapter |
94.74 | 100.00 | 86.67 |
Svc/BufferAccumulator |
88.00 | 94.12 | 74.49 |
Svc/BufferLogger |
92.62 | 86.96 | 80.46 |
Svc/BufferManager |
99.05 | 100.00 | 83.64 |
Svc/BufferRepeater |
91.67 | 100.00 | 75.00 |
Svc/ChronoTime |
100.00 | 100.00 | 100.00 |
Svc/CmdDispatcher |
96.97 | 91.67 | 91.75 |
Svc/CmdSequencer |
93.89 | 97.12 | 84.63 |
Svc/CmdSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComLogger |
97.37 | 91.67 | 83.91 |
Svc/ComSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComStub |
98.51 | 100.00 | 85.19 |
Svc/DpCatalog |
78.00 | 100.00 | 66.27 |
Svc/DpManager |
97.44 | 100.00 | 100.00 |
Svc/DpWriter |
97.58 | 90.00 | 97.22 |
Svc/FileDownlink |
84.15 | 90.91 | 71.90 |
Svc/FileManager |
88.41 | 93.33 | 82.44 |
Svc/FileUplink |
92.35 | 96.55 | 80.95 |
Svc/FileWorker |
90.29 | 100.00 | 84.87 |
Svc/FprimeDeframer |
100.00 | 100.00 | 97.92 |
Svc/FprimeFramer |
100.00 | 100.00 | 95.45 |
Svc/FprimeRouter |
88.89 | 100.00 | 78.26 |
Svc/FpySequencer |
86.76 | 99.04 | 76.89 |
Svc/GenericHub |
100.00 | 100.00 | 85.64 |
Svc/Health |
100.00 | 100.00 | 88.66 |
Svc/LinuxTimer |
97.06 | 100.00 | 82.35 |
Svc/OsTime |
70.00 | 83.33 | 60.98 |
Svc/PassiveRateGroup |
100.00 | 100.00 | 89.47 |
Svc/PolyDb |
100.00 | 100.00 | 53.57 |
Svc/PosixTime |
100.00 | 100.00 | 75.00 |
Svc/PrmDb |
92.75 | 89.47 | 88.24 |
Svc/RateGroupDriver |
100.00 | 100.00 | 68.75 |
Svc/SeqDispatcher |
73.08 | 80.00 | 71.05 |
Svc/StaticMemory |
100.00 | 100.00 | 100.00 |
Svc/SystemResources |
98.63 | 100.00 | 76.19 |
Svc/TlmChan |
77.59 | 85.71 | 63.03 |
Svc/TlmPacketizer |
92.18 | 100.00 | 77.45 |
Svc/Version |
96.10 | 100.00 | 86.96 |
Utils |
20.04 | 26.44 | 24.07 |
Utils/Types |
92.11 | 95.83 | 77.08 |
Modules without UTs
CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
Change Description
This branch carries the work from nasa#5205 (the
Os/GenericPriorityMemQueue/AtomicQueuefeature) and adds focused commits that fix the four CI checks that were failing on that PR, plus macOS/sanitizer issues that were latent because the new unit tests were not being compiled in the upstream macOS/coverage jobs.Fixes layered on top of nasa#5205:
match/casewithif/elifincmake/autocoder/scripts/priority_buffer_analyzer.pyso it formats cleanly under the pinned black version..github/actions/spelling/expect.txt, removed forbidden/misspelled entries, and dropped redundant singular variants.PriorityMemQueueUTs: tightened the rule-based test's shadow model so queue-full / ordering expectations are deterministic.-Werror,-Wformat):FwSizeTypeisunsigned long longon macOS arm64 while%zuexpectssize_t. Cast the affected printf arguments tosize_tacross the UT files so the format strings are portable.Os_Mutex_Posix/Os_CountingSemaphore_PosixinCHOOSES_IMPLEMENTATIONS, which do not exist on Darwin. Dropped the platform-specific picks so the platform config selects the implementation (matching theTypes_Atomic_Queue_testpattern).Stack-use-after-scope fix (the last failing check)
PriorityMemQueue::configure()only borrows the caller'sQueueConfigarray (s_configs = queueConfigs).teardownInternal()then re-scanned that borrowed array to find and release its slot:When the config array is a local (as in
MultipleReconfigurations), the compiler can end its lifetime before the queue destructor runs at end of scope, so this read is a stack-use-after-scope (ASan caught it atPriorityMemQueue.cpp:522).The slot index is now captured at
create()time (while the array is alive) and released via the internally-allocateds_configsUsedarray, never touching the borroweds_configsduring teardown:This also makes teardown idempotent across an explicit
teardown()followed by the destructor.Rationale
Gets nasa#5205 to a green CI so it can be integrated. The macOS/sanitizer defects were real but hidden upstream because the new
PriorityMemQueueunit tests were not compiled in the upstream macOS/coverage jobs.Testing/Review Recommendations
UTs [macos-latest]), and Coverage [PR].fprime-util format --check), and thePriorityMemQueue_UnitTest(33/33) all pass.create()/teardownInternal()slot-tracking change inOs/Generic/PriorityMemQueue.{hpp,cpp}and confirm the borrowed-s_configslifetime coupling is fully removed from teardown.Future Work
None for the CI fix. Devin Review raised separate observations about semaphore accounting in
PriorityMemQueue.cpp; those are pre-existing to nasa#5205 and out of scope for this CI fix.AI Usage (see policy)
AI (Devin) was used to diagnose the CI failures, implement the fixes, and verify them locally and against fork CI.
IAMAI
Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/4ca5ec2ae7ab4361b87c3c5a096c9607