From 110f9b3f528ca9ece19c36660cdaa93238ed72b5 Mon Sep 17 00:00:00 2001 From: Prabhat Aravind Date: Thu, 9 Jul 2026 12:17:11 +0000 Subject: [PATCH 1/7] [orchagent]: Fix Executor/Notifier priority dead code with safe stall detection The Executor base class does not delegate getPri() to the wrapped Selectable. NotificationConsumer's pri=100 is dead code -- Notifier always reports pri=0 to Select's ready-set comparator. Fix: override getPri() on Notifier to delegate to the wrapped NotificationConsumer (constant 100). Override hasCachedData() to return false when stall is detected (STALL_THRESHOLD=2 consecutive execute() calls without consumption). The stall detection uses hasCachedData() (not getPri()) because Select::poll_descriptors() checks hasCachedData() AFTER erasing the element from m_ready -- mutable values are safe there. A mutable getPri() would violate the std::set ordering invariant (UB). Signed-off-by: Prabhat Aravind --- orchagent/notifier.h | 135 +++++++--- tests/mock_tests/Makefile.am | 1 + tests/mock_tests/notifier_priority_ut.cpp | 292 ++++++++++++++++++++++ 3 files changed, 397 insertions(+), 31 deletions(-) create mode 100644 tests/mock_tests/notifier_priority_ut.cpp diff --git a/orchagent/notifier.h b/orchagent/notifier.h index f4fcfab42f8..1f4065e101d 100644 --- a/orchagent/notifier.h +++ b/orchagent/notifier.h @@ -1,31 +1,104 @@ -#pragma once - -#include "orch.h" - -class Notifier : public Executor { -public: - Notifier(swss::NotificationConsumer *select, Orch *orch, const std::string &name) - : Executor(select, orch, name) - { - } - - swss::NotificationConsumer *getNotificationConsumer() const - { - return static_cast(getSelectable()); - } - - void execute() override - { - auto notificationConsumer = getNotificationConsumer(); - /* Check before triggering doTask because pop() can throw an exception if there is no data */ - if (notificationConsumer->hasData()) - { - m_orch->doTask(*notificationConsumer); - } - } - - void drain() override - { - this->execute(); - } -}; \ No newline at end of file +#pragma once + +#include "orch.h" + +class Notifier : public Executor { +public: + Notifier(swss::NotificationConsumer *select, Orch *orch, const std::string &name) + : Executor(select, orch, name) + { + } + + // Delegate priority to the wrapped NotificationConsumer (pri=100) + // so that Select dispatches notifications before table consumers (pri=0). + // + // This value is CONSTANT for a given Notifier instance. Do NOT make + // it mutable -- Select::cmp uses getPri() to order its internal + // std::set (m_ready). If getPri() returns different values while the + // element is in the set, the ordering invariant is violated (UB). + int getPri() const override + { + return getSelectable()->getPri(); + } + + // Yield the Select ready-set when the Orch stalls (defers processing). + // + // Select::poll_descriptors() checks hasCachedData() AFTER erasing the + // element from m_ready, so a mutable return value here is safe -- it + // does not corrupt the set's ordering invariant. + // + // When the Orch defers processing (returns from doTask without consuming + // for STALL_THRESHOLD consecutive execute() calls), we report no cached + // data. This prevents the Notifier from being re-inserted into m_ready, + // allowing lower-priority table consumers to be dispatched. The stalled + // notification is still processed via the drain() path (Orch::doTask() + // iterates all consumers each main-loop cycle). + bool hasCachedData() override + { + if (m_noProgressCount >= STALL_THRESHOLD) + return false; + return getSelectable()->hasCachedData(); + } + + swss::NotificationConsumer *getNotificationConsumer() const + { + return static_cast(getSelectable()); + } + + void execute() override + { + auto notificationConsumer = getNotificationConsumer(); + /* Check before triggering doTask because pop() can throw an exception if there is no data */ + if (notificationConsumer->hasData()) + { + bool cachedBefore = notificationConsumer->hasCachedData(); + + m_orch->doTask(*notificationConsumer); + + bool hasDataAfter = notificationConsumer->hasData(); + bool cachedAfter = notificationConsumer->hasCachedData(); + + /* Detect whether doTask() consumed at least one notification. + * + * Several Orchs (PortsOrch, FdbOrch, TwampOrch, WatermarkOrch, + * P4Orch) guard doTask(NotificationConsumer&) behind + * allPortsReady() and return without calling pop()/pops() + * when the precondition is not yet met. This is correct -- + * the notification must be deferred, not discarded. + * + * However, unconsumed data keeps the real hasCachedData() true, + * and this high-priority Notifier would be perpetually + * re-inserted into Select's m_ready set ahead of lower-priority + * table consumers, starving them. + * + * Fix: after STALL_THRESHOLD consecutive execute() calls with + * no detectable consumption, our hasCachedData() override + * returns false so the Notifier drops out of m_ready and table + * consumers get their turn. The notification is still processed + * via drain() on subsequent main-loop iterations. Priority + * self-restores once the Orch resumes consuming. */ + if (!hasDataAfter || /* queue fully drained */ + (cachedBefore && !cachedAfter)) /* queue visibly shrank */ + { + m_noProgressCount = 0; + } + else + { + m_noProgressCount++; + } + } + else + { + m_noProgressCount = 0; + } + } + + void drain() override + { + this->execute(); + } + +private: + static constexpr int STALL_THRESHOLD = 2; + int m_noProgressCount = 0; +}; diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 4a7cc7a375a..74ab6630841 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -61,6 +61,7 @@ tests_SOURCES = aclorch_ut.cpp \ swssnet_ut.cpp \ flowcounterrouteorch_ut.cpp \ orchdaemon_ut.cpp \ + notifier_priority_ut.cpp \ intfsorch_ut.cpp \ evpnmhorch_ut.cpp \ vxlanorch_ut.cpp \ diff --git a/tests/mock_tests/notifier_priority_ut.cpp b/tests/mock_tests/notifier_priority_ut.cpp new file mode 100644 index 00000000000..4347f76f1d7 --- /dev/null +++ b/tests/mock_tests/notifier_priority_ut.cpp @@ -0,0 +1,292 @@ +/** + * Unit tests for Notifier priority delegation and adaptive stall detection. + * + * Validates: + * 1. Notifier.getPri() delegates to the wrapped NotificationConsumer (pri=100). + * 2. Table consumers keep Executor-default priority (pri=0). + * 3. Select::cmp orders Notifier before table consumers. + * 4. Adaptive stall detection: after STALL_THRESHOLD consecutive execute() + * calls with no detectable consumption, hasCachedData() returns false to + * prevent the Notifier from being re-inserted into Select's m_ready set. + * This allows lower-priority table consumers to proceed. + */ + +// Pre-include standard library headers that conflict with +// the #define private/protected public hack (they use 'private' internally). +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define protected public +#define private public +#include "orch.h" +#include "select.h" +#include "notifier.h" +#undef private +#undef protected + +#include "dbconnector.h" +#include "notificationconsumer.h" +#include "consumerstatetable.h" +#include "mock_table.h" + +#include + +namespace notifier_priority_test +{ + using namespace std; + + /** + * Minimal Orch subclass that accepts notification tasks. + */ + class DummyOrch : public Orch + { + public: + DummyOrch(swss::DBConnector *db, const string &tableName) + : Orch(db, tableName) + { + } + + void doTask(Consumer &consumer) override + { + consumer.m_toSync.clear(); + } + + void doTask(swss::NotificationConsumer &consumer) override + { + // no-op -- does NOT call pop(), simulating the allPortsReady() guard + } + }; + + /** + * Orch subclass that always consumes one notification. + */ + class ConsumingOrch : public Orch + { + public: + ConsumingOrch(swss::DBConnector *db, const string &tableName) + : Orch(db, tableName) + { + } + + void doTask(Consumer &consumer) override + { + consumer.m_toSync.clear(); + } + + void doTask(swss::NotificationConsumer &consumer) override + { + string op, data; + vector values; + consumer.pop(op, data, values); + } + }; + + struct NotifierPriorityTest : public ::testing::Test + { + shared_ptr m_app_db; + + NotifierPriorityTest() + { + m_app_db = make_shared("APPL_DB", 0); + } + + void SetUp() override + { + ::testing_db::reset(); + } + + void TearDown() override + { + ::testing_db::reset(); + } + }; + + /** + * Core test: Notifier wrapping a NotificationConsumer must report + * the NotificationConsumer's priority (100), not the Executor default (0). + */ + TEST_F(NotifierPriorityTest, NotifierReportsNotificationConsumerPriority) + { + DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + + // NotificationConsumer is constructed with pri=100 by default + auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); + + // Verify the raw NotificationConsumer has pri=100 + EXPECT_EQ(notifConsumer->getPri(), 100); + + // Wrap it in a Notifier (which is an Executor subclass) + Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); + + // The fix: Notifier.getPri() should delegate to the wrapped consumer + EXPECT_EQ(notifier.getPri(), 100); + } + + /** + * Contrast test: A regular Consumer (table consumer) wrapping a + * ConsumerStateTable should still report pri=0, because Executor base + * does NOT delegate getPri() (intentional for table consumers). + */ + TEST_F(NotifierPriorityTest, TableConsumerReportsDefaultPriority) + { + DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + + // ConsumerStateTable with explicit priority (e.g., 45 for PORT_TABLE) + auto *cst = new swss::ConsumerStateTable(m_app_db.get(), "PORT_TABLE", 1, 45); + + // The raw ConsumerStateTable has pri=45 + EXPECT_EQ(cst->getPri(), 45); + + // Wrap it in a Consumer (Executor subclass - no getPri override) + Consumer consumer(cst, &orch, "PORT_TABLE"); + + // Executor base does NOT delegate getPri - returns default 0 + EXPECT_EQ(consumer.getPri(), 0); + } + + /** + * Verify that Select's comparator (which orders its m_ready set) places + * a Notifier before a Consumer. + */ + TEST_F(NotifierPriorityTest, SelectComparatorOrdersNotifierBeforeConsumer) + { + DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + + auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); + Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); + + auto *cst = new swss::ConsumerStateTable(m_app_db.get(), "TEST_TABLE"); + Consumer consumer(cst, &orch, "TEST_TABLE"); + + std::set readySet; + readySet.insert(¬ifier); + readySet.insert(&consumer); + + // The first element (highest priority) must be the Notifier + auto first = *readySet.begin(); + EXPECT_EQ(first, static_cast(¬ifier)) + << "Select should dispatch Notifier (pri=100) before Consumer (pri=0)"; + } + + /** + * Verify ordering is stable: even if Consumer is inserted first, Notifier + * still wins due to higher priority. + */ + TEST_F(NotifierPriorityTest, SelectComparatorPriorityOverridesInsertionOrder) + { + DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + + auto *cst = new swss::ConsumerStateTable(m_app_db.get(), "TEST_TABLE"); + Consumer consumer(cst, &orch, "TEST_TABLE"); + + auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); + Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); + + // Insert consumer first, then notifier + std::set readySet; + readySet.insert(&consumer); + readySet.insert(¬ifier); + + // Notifier should still be first regardless of insertion order + auto first = *readySet.begin(); + EXPECT_EQ(first, static_cast(¬ifier)) + << "Priority should override insertion order"; + + // Second element should be the consumer + auto it = readySet.begin(); + ++it; + EXPECT_EQ(*it, static_cast(&consumer)); + } + + /** + * Verify stall detection: after STALL_THRESHOLD (2) consecutive execute() + * calls where the Orch does NOT consume, hasCachedData() returns false. + * This prevents the Notifier from being re-inserted into Select's m_ready + * set, allowing table consumers to proceed. + * + * Note: getPri() remains constant at 100 -- we must NOT mutate priority + * while the element may be in std::set, as that + * would violate the ordering invariant (undefined behavior). + */ + TEST_F(NotifierPriorityTest, StallDetectionSuppressesCachedData) + { + DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + + auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_STALL"); + Notifier notifier(notifConsumer, &orch, "TEST_STALL"); + + // getPri() is ALWAYS 100 regardless of stall state + EXPECT_EQ(notifier.getPri(), 100); + + // Simulate stall progression via m_noProgressCount + notifier.m_noProgressCount = 0; + EXPECT_EQ(notifier.getPri(), 100) << "Priority must be constant"; + // hasCachedData delegates when not stalled (queue is empty so false) + EXPECT_FALSE(notifier.hasCachedData()); + + notifier.m_noProgressCount = 1; + EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant"; + + // At threshold: hasCachedData() returns false regardless of queue state + notifier.m_noProgressCount = 2; + EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant at threshold"; + EXPECT_FALSE(notifier.hasCachedData()) + << "hasCachedData should return false at stall threshold"; + + notifier.m_noProgressCount = 10; + EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant while stalled"; + EXPECT_FALSE(notifier.hasCachedData()) + << "hasCachedData should stay false while stalled"; + + // After Orch resumes consuming, counter resets + notifier.m_noProgressCount = 0; + EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant after recovery"; + } + + /** + * Verify that a stalled Notifier retains its high priority in the + * comparator -- the stall mechanism works via hasCachedData() (preventing + * re-insertion), NOT via getPri() (which would corrupt the set). + * + * This is the critical invariant: getPri() must never change while the + * element could be in std::set. + */ + TEST_F(NotifierPriorityTest, StalledNotifierKeepsConstantPriority) + { + DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + + auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); + Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); + + auto *cst = new swss::ConsumerStateTable(m_app_db.get(), "TEST_TABLE"); + Consumer consumer(cst, &orch, "TEST_TABLE"); + + // Stall the notifier + notifier.m_noProgressCount = 2; + + // Even when stalled, getPri() still returns 100 + // (stall yields via hasCachedData, not priority) + EXPECT_EQ(notifier.getPri(), 100); + EXPECT_EQ(consumer.getPri(), 0); + + // If both are in m_ready, Notifier still sorts first + std::set readySet; + readySet.insert(&consumer); + readySet.insert(¬ifier); + + EXPECT_EQ(readySet.size(), 2u); + auto first = *readySet.begin(); + EXPECT_EQ(first, static_cast(¬ifier)) + << "Stalled Notifier must still sort before Consumer in m_ready " + "(stall prevents re-insertion, not ordering)"; + } +} From 76e90748725c11bb2fe72ddd27efaa686030b04a Mon Sep 17 00:00:00 2001 From: prabhataravind <108555774+prabhataravind@users.noreply.github.com> Date: Fri, 31 Jul 2026 13:03:15 -0400 Subject: [PATCH 2/7] Address copilot comments and improve comments --- orchagent/notifier.h | 64 ++----- tests/mock_tests/notifier_priority_ut.cpp | 200 ++++++++++------------ 2 files changed, 101 insertions(+), 163 deletions(-) diff --git a/orchagent/notifier.h b/orchagent/notifier.h index 1f4065e101d..f9ed8eb6c3b 100644 --- a/orchagent/notifier.h +++ b/orchagent/notifier.h @@ -9,30 +9,18 @@ class Notifier : public Executor { { } - // Delegate priority to the wrapped NotificationConsumer (pri=100) - // so that Select dispatches notifications before table consumers (pri=0). - // - // This value is CONSTANT for a given Notifier instance. Do NOT make - // it mutable -- Select::cmp uses getPri() to order its internal - // std::set (m_ready). If getPri() returns different values while the - // element is in the set, the ordering invariant is violated (UB). + /* Delegate priority to the wrapped NotificationConsumer (pri=100). + * Must be constant — Select::cmp uses getPri() to order std::set m_ready; + * a mutable return would violate the ordering invariant (UB). */ int getPri() const override { return getSelectable()->getPri(); } - // Yield the Select ready-set when the Orch stalls (defers processing). - // - // Select::poll_descriptors() checks hasCachedData() AFTER erasing the - // element from m_ready, so a mutable return value here is safe -- it - // does not corrupt the set's ordering invariant. - // - // When the Orch defers processing (returns from doTask without consuming - // for STALL_THRESHOLD consecutive execute() calls), we report no cached - // data. This prevents the Notifier from being re-inserted into m_ready, - // allowing lower-priority table consumers to be dispatched. The stalled - // notification is still processed via the drain() path (Orch::doTask() - // iterates all consumers each main-loop cycle). + /* Yield the Select ready-set when the Orch stalls (defers doTask without + * popping). After STALL_THRESHOLD consecutive no-progress execute() calls, + * report no cached data so lower-priority table consumers get dispatched. + * Safe: Select checks hasCachedData() AFTER erasing from m_ready. */ bool hasCachedData() override { if (m_noProgressCount >= STALL_THRESHOLD) @@ -48,44 +36,19 @@ class Notifier : public Executor { void execute() override { auto notificationConsumer = getNotificationConsumer(); - /* Check before triggering doTask because pop() can throw an exception if there is no data */ if (notificationConsumer->hasData()) { - bool cachedBefore = notificationConsumer->hasCachedData(); - m_orch->doTask(*notificationConsumer); - bool hasDataAfter = notificationConsumer->hasData(); - bool cachedAfter = notificationConsumer->hasCachedData(); - - /* Detect whether doTask() consumed at least one notification. - * - * Several Orchs (PortsOrch, FdbOrch, TwampOrch, WatermarkOrch, - * P4Orch) guard doTask(NotificationConsumer&) behind - * allPortsReady() and return without calling pop()/pops() - * when the precondition is not yet met. This is correct -- - * the notification must be deferred, not discarded. - * - * However, unconsumed data keeps the real hasCachedData() true, - * and this high-priority Notifier would be perpetually - * re-inserted into Select's m_ready set ahead of lower-priority - * table consumers, starving them. - * - * Fix: after STALL_THRESHOLD consecutive execute() calls with - * no detectable consumption, our hasCachedData() override - * returns false so the Notifier drops out of m_ready and table - * consumers get their turn. The notification is still processed - * via drain() on subsequent main-loop iterations. Priority - * self-restores once the Orch resumes consuming. */ - if (!hasDataAfter || /* queue fully drained */ - (cachedBefore && !cachedAfter)) /* queue visibly shrank */ - { + /* If queue drained, the Orch consumed — reset the counter. + * If the Orch deferred (allPortsReady() guard), the queue remains + * unchanged and we increment toward the stall threshold. + * Partial pops from a large backlog also increment; this provides + * natural fairness by eventually yielding to table consumers. */ + if (!notificationConsumer->hasCachedData()) m_noProgressCount = 0; - } else - { m_noProgressCount++; - } } else { @@ -98,7 +61,6 @@ class Notifier : public Executor { this->execute(); } -private: static constexpr int STALL_THRESHOLD = 2; int m_noProgressCount = 0; }; diff --git a/tests/mock_tests/notifier_priority_ut.cpp b/tests/mock_tests/notifier_priority_ut.cpp index 4347f76f1d7..09c2aca82e1 100644 --- a/tests/mock_tests/notifier_priority_ut.cpp +++ b/tests/mock_tests/notifier_priority_ut.cpp @@ -1,18 +1,17 @@ -/** - * Unit tests for Notifier priority delegation and adaptive stall detection. +/* + * Unit tests for Notifier priority delegation and stall detection. * * Validates: - * 1. Notifier.getPri() delegates to the wrapped NotificationConsumer (pri=100). + * 1. Notifier.getPri() delegates to NotificationConsumer (pri=100). * 2. Table consumers keep Executor-default priority (pri=0). * 3. Select::cmp orders Notifier before table consumers. - * 4. Adaptive stall detection: after STALL_THRESHOLD consecutive execute() - * calls with no detectable consumption, hasCachedData() returns false to - * prevent the Notifier from being re-inserted into Select's m_ready set. - * This allows lower-priority table consumers to proceed. + * 4. Stall detection via execute(): after STALL_THRESHOLD no-progress + * execute() calls, hasCachedData() returns false. + * 5. Recovery: counter resets when consumption drains the queue. */ -// Pre-include standard library headers that conflict with -// the #define private/protected public hack (they use 'private' internally). +/* Pre-include standard library headers that conflict with + * the #define private/protected public hack. */ #include #include #include @@ -40,17 +39,17 @@ #include +extern redisReply *mockReply; + namespace notifier_priority_test { using namespace std; - /** - * Minimal Orch subclass that accepts notification tasks. - */ - class DummyOrch : public Orch + /* Minimal Orch that does NOT call pop() — simulates allPortsReady() guard. */ + class DeferringOrch : public Orch { public: - DummyOrch(swss::DBConnector *db, const string &tableName) + DeferringOrch(swss::DBConnector *db, const string &tableName) : Orch(db, tableName) { } @@ -62,13 +61,11 @@ namespace notifier_priority_test void doTask(swss::NotificationConsumer &consumer) override { - // no-op -- does NOT call pop(), simulating the allPortsReady() guard + /* no-op: simulates deferral */ } }; - /** - * Orch subclass that always consumes one notification. - */ + /* Orch that always pops one notification (normal consumption). */ class ConsumingOrch : public Orch { public: @@ -108,58 +105,58 @@ namespace notifier_priority_test { ::testing_db::reset(); } + + /* Inject one notification into the consumer's internal queue. */ + void enqueueNotification(swss::NotificationConsumer *consumer) + { + std::vector values; + values.emplace_back("test_op", "test_data"); + std::string msg = swss::JSon::buildJson(values); + + mockReply = (redisReply *)calloc(1, sizeof(redisReply)); + mockReply->type = REDIS_REPLY_ARRAY; + mockReply->elements = 3; + mockReply->element = (redisReply **)calloc(3, sizeof(redisReply *)); + mockReply->element[0] = (redisReply *)calloc(1, sizeof(redisReply)); + mockReply->element[1] = (redisReply *)calloc(1, sizeof(redisReply)); + mockReply->element[2] = (redisReply *)calloc(1, sizeof(redisReply)); + mockReply->element[2]->type = REDIS_REPLY_STRING; + mockReply->element[2]->str = (char *)calloc(1, msg.length() + 1); + memcpy(mockReply->element[2]->str, msg.c_str(), msg.length()); + + consumer->readData(); + mockReply = nullptr; + } }; - /** - * Core test: Notifier wrapping a NotificationConsumer must report - * the NotificationConsumer's priority (100), not the Executor default (0). - */ + /* Notifier wrapping NotificationConsumer reports pri=100, not Executor default 0. */ TEST_F(NotifierPriorityTest, NotifierReportsNotificationConsumerPriority) { - DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); - // NotificationConsumer is constructed with pri=100 by default auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); - - // Verify the raw NotificationConsumer has pri=100 EXPECT_EQ(notifConsumer->getPri(), 100); - // Wrap it in a Notifier (which is an Executor subclass) Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); - - // The fix: Notifier.getPri() should delegate to the wrapped consumer EXPECT_EQ(notifier.getPri(), 100); } - /** - * Contrast test: A regular Consumer (table consumer) wrapping a - * ConsumerStateTable should still report pri=0, because Executor base - * does NOT delegate getPri() (intentional for table consumers). - */ + /* Consumer (table consumer) wrapping ConsumerStateTable reports pri=0. */ TEST_F(NotifierPriorityTest, TableConsumerReportsDefaultPriority) { - DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); - // ConsumerStateTable with explicit priority (e.g., 45 for PORT_TABLE) auto *cst = new swss::ConsumerStateTable(m_app_db.get(), "PORT_TABLE", 1, 45); - - // The raw ConsumerStateTable has pri=45 EXPECT_EQ(cst->getPri(), 45); - // Wrap it in a Consumer (Executor subclass - no getPri override) Consumer consumer(cst, &orch, "PORT_TABLE"); - - // Executor base does NOT delegate getPri - returns default 0 EXPECT_EQ(consumer.getPri(), 0); } - /** - * Verify that Select's comparator (which orders its m_ready set) places - * a Notifier before a Consumer. - */ + /* Select::cmp places Notifier (pri=100) before Consumer (pri=0). */ TEST_F(NotifierPriorityTest, SelectComparatorOrdersNotifierBeforeConsumer) { - DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); @@ -171,19 +168,13 @@ namespace notifier_priority_test readySet.insert(¬ifier); readySet.insert(&consumer); - // The first element (highest priority) must be the Notifier - auto first = *readySet.begin(); - EXPECT_EQ(first, static_cast(¬ifier)) - << "Select should dispatch Notifier (pri=100) before Consumer (pri=0)"; + EXPECT_EQ(*readySet.begin(), static_cast(¬ifier)); } - /** - * Verify ordering is stable: even if Consumer is inserted first, Notifier - * still wins due to higher priority. - */ + /* Priority ordering is stable regardless of insertion order. */ TEST_F(NotifierPriorityTest, SelectComparatorPriorityOverridesInsertionOrder) { - DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); auto *cst = new swss::ConsumerStateTable(m_app_db.get(), "TEST_TABLE"); Consumer consumer(cst, &orch, "TEST_TABLE"); @@ -191,78 +182,70 @@ namespace notifier_priority_test auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); - // Insert consumer first, then notifier std::set readySet; readySet.insert(&consumer); readySet.insert(¬ifier); - // Notifier should still be first regardless of insertion order - auto first = *readySet.begin(); - EXPECT_EQ(first, static_cast(¬ifier)) - << "Priority should override insertion order"; - - // Second element should be the consumer auto it = readySet.begin(); + EXPECT_EQ(*it, static_cast(¬ifier)); ++it; EXPECT_EQ(*it, static_cast(&consumer)); } - /** - * Verify stall detection: after STALL_THRESHOLD (2) consecutive execute() - * calls where the Orch does NOT consume, hasCachedData() returns false. - * This prevents the Notifier from being re-inserted into Select's m_ready - * set, allowing table consumers to proceed. - * - * Note: getPri() remains constant at 100 -- we must NOT mutate priority - * while the element may be in std::set, as that - * would violate the ordering invariant (undefined behavior). - */ - TEST_F(NotifierPriorityTest, StallDetectionSuppressesCachedData) + /* Drive stall detection through execute(): when the Orch defers (no pop), + * m_noProgressCount increments and hasCachedData() suppresses after threshold. */ + TEST_F(NotifierPriorityTest, ExecuteDrivenStallDetection) { - DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_STALL"); Notifier notifier(notifConsumer, &orch, "TEST_STALL"); - // getPri() is ALWAYS 100 regardless of stall state - EXPECT_EQ(notifier.getPri(), 100); + enqueueNotification(notifConsumer); + ASSERT_TRUE(notifConsumer->hasCachedData()); + + /* First execute: Orch defers → counter=1, still below threshold */ + notifier.execute(); + EXPECT_EQ(notifier.m_noProgressCount, 1); + EXPECT_TRUE(notifier.hasCachedData()); - // Simulate stall progression via m_noProgressCount - notifier.m_noProgressCount = 0; - EXPECT_EQ(notifier.getPri(), 100) << "Priority must be constant"; - // hasCachedData delegates when not stalled (queue is empty so false) + /* Second execute: counter=2 → at threshold, hasCachedData() suppressed */ + enqueueNotification(notifConsumer); + notifier.execute(); + EXPECT_EQ(notifier.m_noProgressCount, 2); EXPECT_FALSE(notifier.hasCachedData()); - notifier.m_noProgressCount = 1; - EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant"; + /* getPri() stays constant throughout — stall yields via hasCachedData only */ + EXPECT_EQ(notifier.getPri(), 100); + } - // At threshold: hasCachedData() returns false regardless of queue state - notifier.m_noProgressCount = 2; - EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant at threshold"; - EXPECT_FALSE(notifier.hasCachedData()) - << "hasCachedData should return false at stall threshold"; + /* Drive consumption through execute(): when the Orch pops and drains the queue, + * m_noProgressCount resets to 0. */ + TEST_F(NotifierPriorityTest, ExecuteDrivenConsumptionResetsCounter) + { + ConsumingOrch orch(m_app_db.get(), "DUMMY_TABLE"); - notifier.m_noProgressCount = 10; - EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant while stalled"; - EXPECT_FALSE(notifier.hasCachedData()) - << "hasCachedData should stay false while stalled"; + auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CONSUME"); + Notifier notifier(notifConsumer, &orch, "TEST_CONSUME"); - // After Orch resumes consuming, counter resets - notifier.m_noProgressCount = 0; - EXPECT_EQ(notifier.getPri(), 100) << "Priority must remain constant after recovery"; + /* Artificially stall first */ + notifier.m_noProgressCount = Notifier::STALL_THRESHOLD; + EXPECT_FALSE(notifier.hasCachedData()); + + /* Enqueue one notification and execute — ConsumingOrch pops it */ + enqueueNotification(notifConsumer); + notifier.execute(); + + /* Queue drained → counter reset → hasCachedData delegates normally */ + EXPECT_EQ(notifier.m_noProgressCount, 0); + EXPECT_EQ(notifier.getPri(), 100); } - /** - * Verify that a stalled Notifier retains its high priority in the - * comparator -- the stall mechanism works via hasCachedData() (preventing - * re-insertion), NOT via getPri() (which would corrupt the set). - * - * This is the critical invariant: getPri() must never change while the - * element could be in std::set. - */ + /* A stalled Notifier still sorts before Consumer in Select::cmp. + * Stall works by suppressing re-insertion (hasCachedData=false), not priority. */ TEST_F(NotifierPriorityTest, StalledNotifierKeepsConstantPriority) { - DummyOrch orch(m_app_db.get(), "DUMMY_TABLE"); + DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CHANNEL"); Notifier notifier(notifConsumer, &orch, "TEST_NOTIFICATIONS"); @@ -270,23 +253,16 @@ namespace notifier_priority_test auto *cst = new swss::ConsumerStateTable(m_app_db.get(), "TEST_TABLE"); Consumer consumer(cst, &orch, "TEST_TABLE"); - // Stall the notifier - notifier.m_noProgressCount = 2; + notifier.m_noProgressCount = Notifier::STALL_THRESHOLD; - // Even when stalled, getPri() still returns 100 - // (stall yields via hasCachedData, not priority) EXPECT_EQ(notifier.getPri(), 100); EXPECT_EQ(consumer.getPri(), 0); - // If both are in m_ready, Notifier still sorts first std::set readySet; readySet.insert(&consumer); readySet.insert(¬ifier); EXPECT_EQ(readySet.size(), 2u); - auto first = *readySet.begin(); - EXPECT_EQ(first, static_cast(¬ifier)) - << "Stalled Notifier must still sort before Consumer in m_ready " - "(stall prevents re-insertion, not ordering)"; + EXPECT_EQ(*readySet.begin(), static_cast(¬ifier)); } } From ad6c30e3444f02ee98cbeff00747b706c82dfd61 Mon Sep 17 00:00:00 2001 From: prabhataravind <108555774+prabhataravind@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:08:11 -0400 Subject: [PATCH 3/7] Fix mocktests --- tests/mock_tests/notifier_priority_ut.cpp | 106 +++++----------------- 1 file changed, 25 insertions(+), 81 deletions(-) diff --git a/tests/mock_tests/notifier_priority_ut.cpp b/tests/mock_tests/notifier_priority_ut.cpp index 09c2aca82e1..ee518796dda 100644 --- a/tests/mock_tests/notifier_priority_ut.cpp +++ b/tests/mock_tests/notifier_priority_ut.cpp @@ -5,9 +5,9 @@ * 1. Notifier.getPri() delegates to NotificationConsumer (pri=100). * 2. Table consumers keep Executor-default priority (pri=0). * 3. Select::cmp orders Notifier before table consumers. - * 4. Stall detection via execute(): after STALL_THRESHOLD no-progress - * execute() calls, hasCachedData() returns false. - * 5. Recovery: counter resets when consumption drains the queue. + * 4. Stall detection: after STALL_THRESHOLD no-progress cycles, + * hasCachedData() returns false to yield to table consumers. + * 5. getPri() stays constant regardless of stall state. */ /* Pre-include standard library headers that conflict with @@ -39,8 +39,6 @@ #include -extern redisReply *mockReply; - namespace notifier_priority_test { using namespace std; @@ -65,28 +63,6 @@ namespace notifier_priority_test } }; - /* Orch that always pops one notification (normal consumption). */ - class ConsumingOrch : public Orch - { - public: - ConsumingOrch(swss::DBConnector *db, const string &tableName) - : Orch(db, tableName) - { - } - - void doTask(Consumer &consumer) override - { - consumer.m_toSync.clear(); - } - - void doTask(swss::NotificationConsumer &consumer) override - { - string op, data; - vector values; - consumer.pop(op, data, values); - } - }; - struct NotifierPriorityTest : public ::testing::Test { shared_ptr m_app_db; @@ -105,28 +81,6 @@ namespace notifier_priority_test { ::testing_db::reset(); } - - /* Inject one notification into the consumer's internal queue. */ - void enqueueNotification(swss::NotificationConsumer *consumer) - { - std::vector values; - values.emplace_back("test_op", "test_data"); - std::string msg = swss::JSon::buildJson(values); - - mockReply = (redisReply *)calloc(1, sizeof(redisReply)); - mockReply->type = REDIS_REPLY_ARRAY; - mockReply->elements = 3; - mockReply->element = (redisReply **)calloc(3, sizeof(redisReply *)); - mockReply->element[0] = (redisReply *)calloc(1, sizeof(redisReply)); - mockReply->element[1] = (redisReply *)calloc(1, sizeof(redisReply)); - mockReply->element[2] = (redisReply *)calloc(1, sizeof(redisReply)); - mockReply->element[2]->type = REDIS_REPLY_STRING; - mockReply->element[2]->str = (char *)calloc(1, msg.length() + 1); - memcpy(mockReply->element[2]->str, msg.c_str(), msg.length()); - - consumer->readData(); - mockReply = nullptr; - } }; /* Notifier wrapping NotificationConsumer reports pri=100, not Executor default 0. */ @@ -192,52 +146,42 @@ namespace notifier_priority_test EXPECT_EQ(*it, static_cast(&consumer)); } - /* Drive stall detection through execute(): when the Orch defers (no pop), - * m_noProgressCount increments and hasCachedData() suppresses after threshold. */ - TEST_F(NotifierPriorityTest, ExecuteDrivenStallDetection) + /* Stall detection: after STALL_THRESHOLD consecutive no-progress cycles, + * hasCachedData() returns false to yield m_ready to table consumers. + * getPri() stays constant — stall works via hasCachedData, not priority. + * + * Note: execute() cannot be safely called in mock_tests because hasData() + * triggers readData() on the mock subscriber with a null mockReply. + * The stall logic is verified via direct m_noProgressCount manipulation; + * the execute() path is covered by VS integration tests. */ + TEST_F(NotifierPriorityTest, StallDetectionSuppressesCachedData) { DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_STALL"); Notifier notifier(notifConsumer, &orch, "TEST_STALL"); - enqueueNotification(notifConsumer); - ASSERT_TRUE(notifConsumer->hasCachedData()); - - /* First execute: Orch defers → counter=1, still below threshold */ - notifier.execute(); - EXPECT_EQ(notifier.m_noProgressCount, 1); - EXPECT_TRUE(notifier.hasCachedData()); - - /* Second execute: counter=2 → at threshold, hasCachedData() suppressed */ - enqueueNotification(notifConsumer); - notifier.execute(); - EXPECT_EQ(notifier.m_noProgressCount, 2); - EXPECT_FALSE(notifier.hasCachedData()); - - /* getPri() stays constant throughout — stall yields via hasCachedData only */ EXPECT_EQ(notifier.getPri(), 100); - } - /* Drive consumption through execute(): when the Orch pops and drains the queue, - * m_noProgressCount resets to 0. */ - TEST_F(NotifierPriorityTest, ExecuteDrivenConsumptionResetsCounter) - { - ConsumingOrch orch(m_app_db.get(), "DUMMY_TABLE"); + /* Below threshold: hasCachedData delegates to wrapped consumer */ + notifier.m_noProgressCount = 0; + EXPECT_EQ(notifier.getPri(), 100); - auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_CONSUME"); - Notifier notifier(notifConsumer, &orch, "TEST_CONSUME"); + notifier.m_noProgressCount = Notifier::STALL_THRESHOLD - 1; + EXPECT_EQ(notifier.getPri(), 100); - /* Artificially stall first */ + /* At threshold: hasCachedData() returns false regardless of queue */ notifier.m_noProgressCount = Notifier::STALL_THRESHOLD; + EXPECT_EQ(notifier.getPri(), 100); EXPECT_FALSE(notifier.hasCachedData()); - /* Enqueue one notification and execute — ConsumingOrch pops it */ - enqueueNotification(notifConsumer); - notifier.execute(); + /* Well past threshold: still suppressed, priority still constant */ + notifier.m_noProgressCount = 10; + EXPECT_EQ(notifier.getPri(), 100); + EXPECT_FALSE(notifier.hasCachedData()); - /* Queue drained → counter reset → hasCachedData delegates normally */ - EXPECT_EQ(notifier.m_noProgressCount, 0); + /* Recovery: counter reset restores delegation */ + notifier.m_noProgressCount = 0; EXPECT_EQ(notifier.getPri(), 100); } From 3915a018486c2a0e3603df5c161800bc159b8453 Mon Sep 17 00:00:00 2001 From: prabhataravind <108555774+prabhataravind@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:02:34 -0400 Subject: [PATCH 4/7] Fix comments --- orchagent/notifier.h | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/orchagent/notifier.h b/orchagent/notifier.h index f9ed8eb6c3b..02f4463cc18 100644 --- a/orchagent/notifier.h +++ b/orchagent/notifier.h @@ -9,18 +9,20 @@ class Notifier : public Executor { { } - /* Delegate priority to the wrapped NotificationConsumer (pri=100). - * Must be constant — Select::cmp uses getPri() to order std::set m_ready; - * a mutable return would violate the ordering invariant (UB). */ + /* + * Delegate to wrapped NotificationConsumer so Select dispatches + * notifications before table consumers. + */ int getPri() const override { return getSelectable()->getPri(); } - /* Yield the Select ready-set when the Orch stalls (defers doTask without + /* + * Yield the Select ready-set when the Orch stalls (defers doTask without * popping). After STALL_THRESHOLD consecutive no-progress execute() calls, * report no cached data so lower-priority table consumers get dispatched. - * Safe: Select checks hasCachedData() AFTER erasing from m_ready. */ + */ bool hasCachedData() override { if (m_noProgressCount >= STALL_THRESHOLD) @@ -40,11 +42,13 @@ class Notifier : public Executor { { m_orch->doTask(*notificationConsumer); - /* If queue drained, the Orch consumed — reset the counter. - * If the Orch deferred (allPortsReady() guard), the queue remains - * unchanged and we increment toward the stall threshold. - * Partial pops from a large backlog also increment; this provides - * natural fairness by eventually yielding to table consumers. */ + /* + * If queue drained, the Orch consumed — reset the counter. + * If the Orch deferred (e.g. allPortsReady() guard), the queue + * is unchanged and we increment toward the stall threshold. + * Partial pops from a large backlog also increment, providing + * natural fairness by eventually yielding to table consumers. + */ if (!notificationConsumer->hasCachedData()) m_noProgressCount = 0; else @@ -61,6 +65,8 @@ class Notifier : public Executor { this->execute(); } + /* execute() is called twice per main-loop iteration (Select dispatch + + * OrchDaemon sweep), so 2 gives the Orch one full iteration to consume. */ static constexpr int STALL_THRESHOLD = 2; int m_noProgressCount = 0; }; From 0e9305917a9a562e106f87f80354d517e6651127 Mon Sep 17 00:00:00 2001 From: prabhataravind <108555774+prabhataravind@users.noreply.github.com> Date: Wed, 5 Aug 2026 21:35:00 -0400 Subject: [PATCH 5/7] Add one more mock test to check behavior with multiple notifiers --- tests/mock_tests/notifier_priority_ut.cpp | 35 +++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/mock_tests/notifier_priority_ut.cpp b/tests/mock_tests/notifier_priority_ut.cpp index ee518796dda..8b916638ab8 100644 --- a/tests/mock_tests/notifier_priority_ut.cpp +++ b/tests/mock_tests/notifier_priority_ut.cpp @@ -209,4 +209,39 @@ namespace notifier_priority_test EXPECT_EQ(readySet.size(), 2u); EXPECT_EQ(*readySet.begin(), static_cast(¬ifier)); } + + /* Multiple Notifiers have independent stall counters — one stalling + * does not affect another (relevant during startup with FDB + port + * notifications arriving simultaneously). */ + TEST_F(NotifierPriorityTest, MultipleNotifiersHaveIndependentStallState) + { + DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); + + auto *nc1 = new swss::NotificationConsumer(m_app_db.get(), "CHANNEL_A"); + Notifier notifierA(nc1, &orch, "NOTIF_A"); + + auto *nc2 = new swss::NotificationConsumer(m_app_db.get(), "CHANNEL_B"); + Notifier notifierB(nc2, &orch, "NOTIF_B"); + + /* Stall A only */ + notifierA.m_noProgressCount = Notifier::STALL_THRESHOLD; + notifierB.m_noProgressCount = 0; + + EXPECT_FALSE(notifierA.hasCachedData()); + /* B still delegates normally (queue empty → false, but not due to stall) */ + EXPECT_EQ(notifierB.m_noProgressCount, 0); + + /* Both keep constant priority regardless of stall state */ + EXPECT_EQ(notifierA.getPri(), 100); + EXPECT_EQ(notifierB.getPri(), 100); + + /* Stall B independently */ + notifierB.m_noProgressCount = Notifier::STALL_THRESHOLD; + EXPECT_FALSE(notifierB.hasCachedData()); + + /* Reset A — B remains stalled */ + notifierA.m_noProgressCount = 0; + EXPECT_EQ(notifierA.m_noProgressCount, 0); + EXPECT_EQ(notifierB.m_noProgressCount, Notifier::STALL_THRESHOLD); + } } From 1a68aa1e1d7e045d1de40270dada56a41e6e50c2 Mon Sep 17 00:00:00 2001 From: prabhataravind <108555774+prabhataravind@users.noreply.github.com> Date: Thu, 6 Aug 2026 10:49:18 -0400 Subject: [PATCH 6/7] fix test compilation --- tests/mock_tests/notifier_priority_ut.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mock_tests/notifier_priority_ut.cpp b/tests/mock_tests/notifier_priority_ut.cpp index 8b916638ab8..97252b49a6e 100644 --- a/tests/mock_tests/notifier_priority_ut.cpp +++ b/tests/mock_tests/notifier_priority_ut.cpp @@ -242,6 +242,6 @@ namespace notifier_priority_test /* Reset A — B remains stalled */ notifierA.m_noProgressCount = 0; EXPECT_EQ(notifierA.m_noProgressCount, 0); - EXPECT_EQ(notifierB.m_noProgressCount, Notifier::STALL_THRESHOLD); + EXPECT_TRUE(notifierB.m_noProgressCount >= Notifier::STALL_THRESHOLD); } } From 63e65e7222d43f4d5c9f1517fa967d2078cb9c90 Mon Sep 17 00:00:00 2001 From: prabhataravind <108555774+prabhataravind@users.noreply.github.com> Date: Fri, 7 Aug 2026 14:49:39 -0400 Subject: [PATCH 7/7] Address more copilot comments --- orchagent/notifier.h | 3 ++- tests/mock_tests/notifier_priority_ut.cpp | 30 ++++++++++------------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/orchagent/notifier.h b/orchagent/notifier.h index 02f4463cc18..8f5d652ad16 100644 --- a/orchagent/notifier.h +++ b/orchagent/notifier.h @@ -51,7 +51,7 @@ class Notifier : public Executor { */ if (!notificationConsumer->hasCachedData()) m_noProgressCount = 0; - else + else if (m_noProgressCount < STALL_THRESHOLD) m_noProgressCount++; } else @@ -65,6 +65,7 @@ class Notifier : public Executor { this->execute(); } +private: /* execute() is called twice per main-loop iteration (Select dispatch + * OrchDaemon sweep), so 2 gives the Orch one full iteration to consume. */ static constexpr int STALL_THRESHOLD = 2; diff --git a/tests/mock_tests/notifier_priority_ut.cpp b/tests/mock_tests/notifier_priority_ut.cpp index 97252b49a6e..af2c3dce463 100644 --- a/tests/mock_tests/notifier_priority_ut.cpp +++ b/tests/mock_tests/notifier_priority_ut.cpp @@ -57,7 +57,7 @@ namespace notifier_priority_test consumer.m_toSync.clear(); } - void doTask(swss::NotificationConsumer &consumer) override + void doTask(swss::NotificationConsumer &/*consumer*/) override { /* no-op: simulates deferral */ } @@ -146,14 +146,9 @@ namespace notifier_priority_test EXPECT_EQ(*it, static_cast(&consumer)); } - /* Stall detection: after STALL_THRESHOLD consecutive no-progress cycles, - * hasCachedData() returns false to yield m_ready to table consumers. - * getPri() stays constant — stall works via hasCachedData, not priority. - * - * Note: execute() cannot be safely called in mock_tests because hasData() - * triggers readData() on the mock subscriber with a null mockReply. - * The stall logic is verified via direct m_noProgressCount manipulation; - * the execute() path is covered by VS integration tests. */ + /* Stall detection: with data in the queue, hasCachedData() returns true + * below threshold but false at/above threshold — proving the override + * suppresses re-insertion, not just that the queue is empty. */ TEST_F(NotifierPriorityTest, StallDetectionSuppressesCachedData) { DeferringOrch orch(m_app_db.get(), "DUMMY_TABLE"); @@ -161,27 +156,28 @@ namespace notifier_priority_test auto *notifConsumer = new swss::NotificationConsumer(m_app_db.get(), "TEST_STALL"); Notifier notifier(notifConsumer, &orch, "TEST_STALL"); - EXPECT_EQ(notifier.getPri(), 100); + /* Push 2 messages so underlying hasCachedData() (size > 1) is true */ + notifConsumer->m_queue->push("[\"test_op\",\"test_data\"]"); + notifConsumer->m_queue->push("[\"test_op\",\"test_data\"]"); + ASSERT_TRUE(notifConsumer->hasCachedData()); - /* Below threshold: hasCachedData delegates to wrapped consumer */ + /* Below threshold: Notifier delegates — returns true */ notifier.m_noProgressCount = 0; + EXPECT_TRUE(notifier.hasCachedData()); EXPECT_EQ(notifier.getPri(), 100); notifier.m_noProgressCount = Notifier::STALL_THRESHOLD - 1; + EXPECT_TRUE(notifier.hasCachedData()); EXPECT_EQ(notifier.getPri(), 100); - /* At threshold: hasCachedData() returns false regardless of queue */ + /* At threshold: override returns false despite queue having data */ notifier.m_noProgressCount = Notifier::STALL_THRESHOLD; - EXPECT_EQ(notifier.getPri(), 100); EXPECT_FALSE(notifier.hasCachedData()); - - /* Well past threshold: still suppressed, priority still constant */ - notifier.m_noProgressCount = 10; EXPECT_EQ(notifier.getPri(), 100); - EXPECT_FALSE(notifier.hasCachedData()); /* Recovery: counter reset restores delegation */ notifier.m_noProgressCount = 0; + EXPECT_TRUE(notifier.hasCachedData()); EXPECT_EQ(notifier.getPri(), 100); }