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

Unresolved concurrency issues can cause deadlock or lose SpeedTest triggers, and timeout behavior lacks coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 2

Open (6)

• Thread creation race can deadlock unload · New
• World-writable state files allow spoofing feature activation
• Mock or link telemetry symbols in unit tests
• Add telemetry_msgsender dependency or mock to unit tests
• SpeedTest trigger lost during QoSD cleanup
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…

Resolved since last review (1)

• Tests lack telemetry mock setup for t2_event_d

[Copilot]

Copilot review overview

🔵 Needs a closer look

Resolve the SpeedTest restart race, add timeout-expiry coverage, and ensure deterministic worker cleanup.

Review effort: Lite
Findings: 2 · 2

Open (4)

• World-writable state files allow spoofing feature activation
• Add telemetry_msgsender dependency or mock to unit tests
• SpeedTest trigger lost during QoSD cleanup
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…

Resolved since last review (2)

• Thread creation race can deadlock unload
• Mock or link telemetry symbols in unit tests

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical lifecycle and test-compilation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 2

Open (6)

• Wait for timer worker to finish before module unload · New
• Add missing test declaration before namespace-scope block · New
• World-writable state files allow spoofing feature activation
• Add telemetry_msgsender dependency or mock to unit tests
• SpeedTest trigger lost during QoSD cleanup
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues remain in executable path handling, event sequencing, state restoration, and RBUS cleanup.

Review effort: Lite
Findings: 1 · 2

Open (3)

• Add telemetry_msgsender dependency or mock to unit tests
• SpeedTest trigger lost during QoSD cleanup
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…

Resolved since last review (3)

• Add missing test declaration before namespace-scope block
• Wait for timer worker to finish before module unload
• World-writable state files allow spoofing feature activation

Previously missed (1)

In code that hasn't changed since last review

Resume NI only when this worker paused it

source/​AdvSecurityDml/​cosa_adv_security_internal.c:549

ni_qosd_pause() can return TRUE without issuing a pause when either marker is absent, but this worker always calls ni_qosd_resume() after the wait. If NI is enabled/activated during a speed test that started while it was inactive, this cycle enables QoSD even though it never paused it; conversely, deactivation during a real pause can skip the restore. Capture whether this worker actually paused NI and only resume that state.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address the RBUS value leak and inactive-NI timeout worker issues.

Review effort: Lite
Findings: None

Resolved since last review (3)

• Add telemetry_msgsender dependency or mock to unit tests
• SpeedTest trigger lost during QoSD cleanup
• This test arms the production timer for 86,400 seconds but never exercises the timeout branch or…

Previously missed (1)

In code that hasn't changed since last review

Avoid arming speed-test timeout worker when NI is inactive

source/​AdvSecurityDml/​cosa_adv_security_internal.c:491

When NI is disabled or not activated, this returns TRUE and ni_speedtest_trigger() still leaves a detached worker waiting for the configured timeout (typically 86,400 seconds), even though no pause occurred. If the COMPLETE event is missed and NI is enabled during that interval, later STARTING events are rejected by the running-state check, so NI will not be paused for the new speed test. Skip arming the timer when the feature is inactive, or distinguish a skipped pause from a successful pause so the worker exits immediately.

[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