[orchagent]: Fix notification priority dead code in Executor/Notifier

[prabhataravind]

Why I did it

The Executor base class does not delegate getPri() to the wrapped Selectable. As a result, NotificationConsumer's pri=100 is dead code — Notifier always reports pri=0 to Select's ready-set comparator, defeating the intended priority scheduling. Additionally, several Orchs (PortsOrch, FdbOrch, TwampOrch, etc.) guard doTask(NotificationConsumer&) behind allPortsReady() and return without calling pop(). The unconsumed notification keeps hasCachedData() true, so once priority is activated, the high-priority Notifier is perpetually re-inserted into m_ready, starving table consumers.

Work item tracking

• Microsoft ADO 38597119

How I did it

• Override Notifier::getPri() to delegate to the wrapped NotificationConsumer, so Select dispatches notifications before table consumers.
• Override Notifier::hasCachedData() with stall detection: after STALL_THRESHOLD (2) consecutive no-progress execute() calls, report no cached data so the Notifier drops out of m_ready and lower-priority table consumers proceed. Stalled notifications are still processed via drain() each loop; the counter resets once the Orch resumes consuming.
• STALL_THRESHOLD=2 matches the two execute() calls per OrchDaemon main-loop iteration (Select dispatch + sweep), giving the Orch one full iteration to consume before yielding.

How to verify it

• 6 unit tests in tests/mock_tests/notifier_priority_ut.cpp: priority delegation, Select::cmp ordering, stall-threshold suppression of hasCachedData(), constant priority invariant under stall.
• All existing VS integration tests pass in CI.

Details if related

Why getPri() must be constant:
Select::select() re-inserts the Notifier into m_ready (std::set<Selectable*, Select::cmp>) before returning it to the caller. The caller then calls execute(). If getPri() were mutable (e.g. returning 100 normally, 0 when stalled), execute() would change its return value while the element is still in the set — the red-black tree's position becomes stale, violating the strict-weak-ordering invariant. This caused orchagent to hang during VS test startup in an earlier iteration.

The fix keeps getPri() constant and uses hasCachedData() for stall yielding instead. hasCachedData() is only evaluated after erasing from the set, so varying results are safe.

Consumption detection:
After doTask(), if hasCachedData() is false the queue was fully drained and the counter resets. If the queue is unchanged (Orch deferred) or only partially drained (Orch popped one from a backlog), the counter increments. Partial-pop incrementing provides natural fairness — a large notification backlog periodically yields to table consumers rather than monopolizing dispatch.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[prabhataravind]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[prabhataravind]

/azp run Azure.sonic-swss

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

orchagent/notifier.h:65

• STALL_THRESHOLD and m_noProgressCount are currently public, which unnecessarily exposes internal scheduling state as part of Notifier’s public API and allows external code to mutate it (potentially breaking invariants). These can be private; the unit test already uses the private/protected→public macro hack, so it can still access them without widening the production API surface.

    static constexpr int STALL_THRESHOLD = 2;
    int m_noProgressCount = 0;

orchagent/notifier.h:47

• The PR description says stall detection is based on "no-consume iterations" (i.e., the Orch defers without popping). However, this implementation increments m_noProgressCount whenever hasCachedData() remains true after doTask(), and the comment explicitly states partial pops from a backlog also increment. That means suppression can engage under sustained notification load even when notifications are being consumed. If that behavior is intentional, the PR description/mechanism should be updated to match; if not, the progress signal needs to distinguish "deferred" vs "consumed but backlog remains".

This issue also appears on line 64 of the same file.

            /* 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. */

tests/mock_tests/notifier_priority_ut.cpp:156

• The stall-related unit test is still not exercising the real state transitions: it directly mutates Notifier::m_noProgressCount instead of driving Notifier::execute() with a consuming Orch vs a deferring Orch. This won’t catch regressions in the execute()-based progress/stall detection logic (the core behavior change in this PR). Consider introducing a test double for swss::NotificationConsumer (or a controllable fixture) so execute() can be called safely and the counter changes can be asserted via behavior rather than internal mutation.

    /* 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. */

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

orchagent/notifier.h:51

• m_noProgressCount++ can grow without bound while notifications keep arriving. Because it’s a signed int, this risks signed overflow (undefined behavior) in a long-running daemon. Since the counter is only used for a threshold check, it should be saturated/clamped at STALL_THRESHOLD.

            if (!notificationConsumer->hasCachedData())
                m_noProgressCount = 0;
            else
                m_noProgressCount++;

orchagent/notifier.h:65

• STALL_THRESHOLD and m_noProgressCount are currently public, which makes Notifier’s internal stall-tracking state part of the public API and allows accidental modification from other code. Consider making these private (tests can still access via the existing #define private public hack).

    static constexpr int STALL_THRESHOLD = 2;
    int m_noProgressCount = 0;

tests/mock_tests/notifier_priority_ut.cpp:156

• The stall-detection test verifies behavior by directly mutating Notifier::m_noProgressCount instead of driving state transitions through Notifier::execute() with an Orch that consumes vs defers. This won’t catch regressions in the real execute()-based progress detection logic that determines when hasCachedData() starts suppressing re-insertion into Select’s ready-set.

    /* 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. */

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

orchagent/notifier.h:71

• Notifier exposes STALL_THRESHOLD and m_noProgressCount as public API, but these are internal scheduling details. Keeping them public makes it easy for non-test code to accidentally depend on or modify the stall logic, and it also weakens encapsulation of the Notifier executor.

    /* 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;

tests/mock_tests/notifier_priority_ut.cpp:157

• The stall test verifies behavior by directly mutating Notifier::m_noProgressCount, so it does not exercise the new execute()-based stall detection/progress tracking logic (the code that updates m_noProgressCount based on hasData()/hasCachedData()). Other mock_tests in this repo drive NotificationConsumer paths by preparing mockReply and calling readData()/doTask(), so it should be possible to add at least one unit test that calls Notifier::execute() repeatedly and asserts the counter transitions and hasCachedData suppression end-to-end.

     * 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)

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

orchagent/notifier.h:71

• STALL_THRESHOLD and m_noProgressCount are currently part of Notifier's public API even though they are internal implementation details for stall detection. Exposing mutable internal state makes it easy for production code to accidentally couple to (or mutate) the stall logic, and it increases the surface area of this header unnecessarily.

    /* 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;

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

tests/mock_tests/notifier_priority_ut.cpp:176

• StallDetectionSuppressesCachedData does not actually exercise the stall-detection transitions in Notifier::execute(), and it never arranges for the wrapped NotificationConsumer to have cached data. As written, hasCachedData() is likely false even without the stall logic, so EXPECT_FALSE(notifier.hasCachedData()) at the threshold can become a false positive and miss regressions. Consider mocking a notification (via the existing mockReply pattern used in portsorch_ut.cpp / twamporch_ut.cpp), calling notifier.execute() repeatedly with a deferring Orch, and asserting (a) underlying consumer hasCachedData()==true below threshold and (b) notifier.hasCachedData()==false at/after threshold while getPri remains constant.

    /* 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");

        EXPECT_EQ(notifier.getPri(), 100);

        /* Below threshold: hasCachedData delegates to wrapped consumer */
        notifier.m_noProgressCount = 0;
        EXPECT_EQ(notifier.getPri(), 100);

        notifier.m_noProgressCount = Notifier::STALL_THRESHOLD - 1;
        EXPECT_EQ(notifier.getPri(), 100);

        /* At threshold: hasCachedData() returns false regardless of queue */
        notifier.m_noProgressCount = Notifier::STALL_THRESHOLD;
        EXPECT_EQ(notifier.getPri(), 100);
        EXPECT_FALSE(notifier.hasCachedData());

orchagent/notifier.h:71

• m_noProgressCount is currently a public data member, which makes the stall mechanism externally mutable and effectively part of Notifier's public API. Since tests already use the private/protected-to-public include hack, this can be made private without impacting the unit test while improving encapsulation.

    /* 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;

orchagent/notifier.h:55

• m_noProgressCount is incremented without bounds. In a long-running orchagent, this can eventually overflow int and flip negative, which would disable the stall suppression check (m_noProgressCount >= STALL_THRESHOLD) and re-enable cached-data reporting unexpectedly. Consider saturating the counter at STALL_THRESHOLD (or using an unsigned/saturating type) to keep the logic stable over time.

            if (!notificationConsumer->hasCachedData())
                m_noProgressCount = 0;
            else
                m_noProgressCount++;

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).