RDKB-66812 RDKB-66844 : Pause Network Intelligence while speedtest is running

[akumar0702]

Reason for change: To pause Network Intelligence while speed test is running
Test Procedure: Trigger speed test and confirm if NI pauses on ST_TR181_STATUS_STARTING event
and resumes on ST_TR181_STATUS_COMPLETE event
Risks: Low
Priority: P0

Signed-off-by: santhosh_gujulvajagadeesh@comcast.com

[Copilot]

🟡 Changes recommended

It introduces a real exit-status handling bug in the new C handler (unsafe WEXITSTATUS usage), plus security/logging/test coverage issues that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds SpeedTest status handling to pause/resume Network Intelligence (cujo-qosd) during SpeedTest runs, wiring an RBUS event into existing AdvSecurity control flows and extending the startup script accordingly.

Changes:

• Subscribe to Device.IP.Diagnostics.X_RDKCENTRAL-COM_SpeedTest.Status and handle status transitions to trigger NI pause/resume.
• Add -speedtestNIStart / -speedtestNIComplete actions to start_adv_security.sh, including an auto-unpause timer.
• Add unit tests validating the new SpeedTest status handler behavior.

File summaries

File	Description
source/test/CcspAdvSecurityDmlTest/CcspAdvSecurityInternalTest.cpp	Adds unit tests for the SpeedTest status RBUS event handler.
source/AdvSecurityDml/cosa_adv_security_internal.c	Subscribes to SpeedTest status RBUS event and triggers NI pause/resume via script calls.
scripts/start_adv_security.sh	Implements NI pause/resume actions and a timer mechanism for automatic re-enable.

Review details

Suppressed comments (1)

source/AdvSecurityDml/cosa_adv_security_internal.c:478

• Same issue as above: WEXITSTATUS(rc) is evaluated even when !WIFEXITED(rc) is true. Guard WEXITSTATUS behind a successful WIFEXITED check.

        rc = v_secure_system(TEMP_DOWNLOAD_LOCATION"/usr/ccsp/advsec/start_adv_security.sh -speedtestNIComplete &");
        if (!WIFEXITED(rc) || WEXITSTATUS(rc) != 0)
        {
            CcspTraceError(("%s: failed to enable cujo-qosd for SpeedTest, rc=%d\n", __FUNCTION__, WEXITSTATUS(rc)));
        }

• Files reviewed: 3/3 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues affect event ordering, test behavior, and timer/status handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/start_adv_security.sh:623

• The timer PID file is never removed when this background timer expires normally. On a later speed-test event, a stale PID can have been reused by an unrelated process, so speedtest_cancel_networkintelligence_timer may send SIGTERM to that process. Clean up the file when the timer exits, but only if it still contains this timer's PID.

scripts/start_adv_security.sh:595

• The subscribed status object is Device.IP.Diagnostics.X_RDKCENTRAL-COM_SpeedTest (cosa_adv_security_internal.c:69), but this lookup uses X_RDK_SpeedTest. On devices exposing only the subscribed object, dmcli will return an invalid/non-numeric value and the pause path will exit without disabling cujo-qosd.

    unpause_timeout=$(dmcli eRT retv "Device.IP.Diagnostics.X_RDK_SpeedTest.SubscriberUnPauseTimeOut" 2>/dev/null)

source/AdvSecurityDml/cosa_adv_security_internal.c:462

• This log evaluates WEXITSTATUS(rc) even when the preceding condition is true because WIFEXITED(rc) is false. WEXITSTATUS is only meaningful for a normally exited child, so signal/-1 failures will be logged with a fabricated status; guard the extraction or log the raw return value in that case.

            CcspTraceError(("%s: failed to disable cujo-qosd for SpeedTest, rc=%d\n", __FUNCTION__, WEXITSTATUS(rc)));

source/AdvSecurityDml/cosa_adv_security_internal.c:471

• This log evaluates WEXITSTATUS(rc) even when the preceding condition is true because WIFEXITED(rc) is false. WEXITSTATUS is only meaningful for a normally exited child, so signal/-1 failures will be logged with a fabricated status; guard the extraction or log the raw return value in that case.

            CcspTraceError(("%s: failed to enable cujo-qosd for SpeedTest, rc=%d\n", __FUNCTION__, WEXITSTATUS(rc)));

source/test/CcspAdvSecurityDmlTest/CcspAdvSecurityInternalTest.cpp:1477

• This expectation has the same accessor mismatch: the handler invokes rbusValue_GetUInt32, not rbusValue_GetString. The status-5 test therefore fails on an unexpected mock call before verifying the completion command.

    EXPECT_CALL(*g_rbusMock, rbusValue_GetString(value, _))
        .Times(1)
        .WillOnce(Return("5"));

source/test/CcspAdvSecurityDmlTest/CcspAdvSecurityInternalTest.cpp:1509

• This no-action test also stubs rbusValue_GetString, while the implementation calls rbusValue_GetUInt32; Google Mock will fail on the unexpected accessor instead of exercising the status-2 path.

    EXPECT_CALL(*g_rbusMock, rbusValue_GetString(value, _))
        .Times(1)
        .WillOnce(Return("2"));

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved moderate findings affect timeout ordering, recovery, clock handling, command failure handling, and test coverage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

source/AdvSecurityDml/cosa_adv_security_internal.c:496

• This deadline is based on CLOCK_REALTIME, which is also the default clock for this condition variable. An NTP or manual wall-clock correction can therefore make the timeout fire early or be postponed indefinitely, enabling qosd while the speed test is still running or leaving it disabled. Use a condition variable configured for CLOCK_MONOTONIC and compute the deadline from that clock.

    if (clock_gettime(CLOCK_REALTIME, &deadline) != 0)
    {
        CcspTraceError(("%s: clock_gettime failed, errno=%d\n", __FUNCTION__, errno));
        return FALSE;
    }
    deadline.tv_sec += timeout;

source/AdvSecurityDml/cosa_adv_security_internal.c:587

• Because this command is backgrounded, v_secure_system only observes the shell that queued start_adv_security.sh, not the cujo-ni-cli operation performed by the script. A failure to disable qosd therefore usually looks like success here, so the timer is left armed and the speed test can run with qosd still enabled. Run this operation synchronously or add an asynchronous result/retry path before treating the start as successful.

        rc = v_secure_system(TEMP_DOWNLOAD_LOCATION"/usr/ccsp/advsec/start_adv_security.sh -speedtestNIStart &");
        if (!WIFEXITED(rc) || WEXITSTATUS(rc) != 0)

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved moderate issues affect QoS ordering, state handling, cleanup races, and test-worker lifetime.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4

Open (4)

• Asynchronous QoSD updates can reorder state changes · New
• SpeedTest trigger lost during QoSD cleanup · New
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…
• The timer is canceled before the asynchronous resume command is started. If cujo-ni-cli fails…

Resolved since last review (2)

• Both status transitions launch their scripts with &. A status 5 callback can therefore run…
• The timer is armed before this explicitly backgrounded -speedtestNIStart operation has actually…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved moderate findings affect QoS synchronization, timeout handling, error handling, RBUS initialization, and test coverage.

Review effort: Lite
Findings: 4

Open (4)

• SpeedTest trigger lost during QoSD cleanup
• Asynchronous QoSD updates can reorder state changes
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…
• The timer is canceled before the asynchronous resume command is started. If cujo-ni-cli fails…

[Copilot]

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 4

Open (8)

• Link telemetry_msgsender only for Network Intelligence builds · New
• Include the telemetry sender API header · New
• Add telemetry_msgsender dependency or mock to unit tests · New
• Mock or link telemetry symbols in unit tests · New
• SpeedTest trigger lost during QoSD cleanup
• Asynchronous QoSD updates can reorder state changes
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…
• The timer is canceled before the asynchronous resume command is started. If cujo-ni-cli fails…

[Copilot]

Copilot review overview

🟡 Changes recommended

Address the RBUS value leak and prevent late QoS re-enablement after NI deactivation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Release RBUS value on error return · New

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical completion-event issue can leave qosd paused until timeout when NI is deactivated during a speed test.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Always forward COMPLETE to active workers after NI deactivation · New

Resolved since last review (1)

• Release RBUS value on error return

[Copilot]

Copilot review overview

🔵 Needs a closer look

Resolve the race that can lose a new SpeedTest pause cycle during worker cleanup.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Always forward COMPLETE to active workers after NI deactivation

[Copilot]

Copilot review overview

🔵 Needs a closer look

A moderate race can cause back-to-back SpeedTests to lose the new pause operation.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

The changes affect runtime NI state and event-driven timer behavior, warranting final human review.

Review effort: Lite
Findings: None

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 2 · 5 · 1

Open (8)

• WEXITSTATUS(rc) is used even when !WIFEXITED(rc) is true, which is undefined behavior. Only… · New
• speedtestEventReceiveHandler dereferences event->data without validating that event and… · New
• The command is backgrounded via trailing &, so v_secure_system() can return success before… · New
• Using CLOCK_REALTIME for a timeout can behave incorrectly if the system clock changes (NTP/time… · New
• usleep() is used but there is no explicit #include <unistd.h> in the newly added include block.… · New
• These tests rely on fixed /tmp/... paths, which can make the suite flaky when tests run… · New
• usleep() is used but there is no explicit #include <unistd.h> in the newly added include block.… · New
• The added macros have inconsistent spacing after #define, which can hinder readability and diff… · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address the STARTING/COMPLETE state race and guard WEXITSTATUS usage for abnormal process termination.

Review effort: Lite
Findings: 2 · 5 · 1

Open (8)

• speedtestEventReceiveHandler dereferences event->data without validating that event and…
• WEXITSTATUS(rc) is used even when !WIFEXITED(rc) is true, which is undefined behavior. Only…
• usleep() is used but there is no explicit #include <unistd.h> in the newly added include block.…
• These tests rely on fixed /tmp/... paths, which can make the suite flaky when tests run…
• usleep() is used but there is no explicit #include <unistd.h> in the newly added include block.…
• Using CLOCK_REALTIME for a timeout can behave incorrectly if the system clock changes (NTP/time…
• The command is backgrounded via trailing &, so v_secure_system() can return success before…
• The added macros have inconsistent spacing after #define, which can hinder readability and diff…