RDKEMW-25167: Forward console logs to rialto logger

[skywojciechowskim]

RDKEMW-25167: Forward console logs to rialto logger

Reason for change: Forward console logs to rialto logger instead of /dev/null
Test Procedure: Rialto CI

[github-actions]

Pull request title must follow the pattern:
< JIRA TICKET >: < one line summary of change less than 65 characters >

Pull request description must follow the Commit message format for RDK-E:
https://etwiki.sys.comcast.net/spaces/RDKAR/pages/1407997180/Commit+Message+Format+For+RDKE

1. Associated JIRA ticket in format : < JIRA TICKET >: < one line summary of change less than 65 characters >
2. Detailed reason for change information
3. Test procedure. Only references links to Jira ticket or sub tickets where test steps and references are captured.

< JIRA TICKET >: < one line summary of change less than 65 characters >
< empty line >
Reason for change:
Test Procedure: < https://ccp.sys.comcast.net/browse/JIRA TICKET/url/to/test_step_section>

[Copilot]

🟡 Changes recommended

Unresolved descriptor cleanup, shutdown, buffering, and failure-handling issues can lose logs or block the service.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR forwards Rialto session-server stdout/stderr to Rialto logging instead of discarding it.

Changes:

• Adds threaded pipe-based console-log forwarding.
• Integrates forwarding into the server lifecycle.
• Removes child-process output suppression and updates the build.

File summaries

File	Change	Review findings
serverManager/common/source/SessionServerApp.cpp	Updates child stdio handling.	Moderate (1 vote): Preserve stdin redirection while forwarding stdout/stderr.
media/server/service/source/main.cpp	Manages forwarder startup and shutdown.	Moderate (1 vote): Handle startup failures and fully unwind partial descriptor setup. Nit (2 votes): Destroy appSessionServer before stopping the forwarder.
media/server/service/source/ConsoleLogForwarding.h	Declares the forwarding component.	None.
media/server/service/source/ConsoleLogForwarding.cpp	Implements pipe readers and Rialto logging.	Critical (2 votes): Clean up and restore descriptors on all setup failures. Moderate (1 vote each): Avoid low-numbered descriptor collisions; drain queued output on shutdown; flush streams before restoration; handle failed start() explicitly; make thread startup exception-safe. Nit (1 vote): Add focused forwarding and rollback tests.
media/server/service/CMakeLists.txt	Adds the implementation to the build.	Nit (1 vote): Add component tests for forwarding, partial lines, shutdown draining, and setup failures.

Review details

Suppressed comments (10)

media/server/service/CMakeLists.txt:42

• This adds process-wide stdout/stderr redirection and two reader threads, but no unit or component test covers the new class. The service test target already exercises the other service implementations (tests/unittests/media/server/service/CMakeLists.txt:20-44); add coverage for disabled-console forwarding, partial lines and shutdown draining, and descriptor/setup failures before relying on CI integration tests alone.

        source/ConsoleLogForwarding.cpp

media/server/service/source/ConsoleLogForwarding.cpp:42

• pipe() may return descriptors 0, 1, or 2 when a standard stream is closed. For example, m_stdoutPipe[0] can be 1; the subsequent dup(STDOUT_FILENO) then duplicates the pipe reader, and dup2(..., STDOUT_FILENO) closes that reader, leaving the worker reading a write end while stdout eventually blocks when the pipe fills. Reserve the pipe descriptors above STDERR_FILENO and preserve the original closed/open state before redirecting.

    if (pipe(m_stdoutPipe) == -1)

media/server/service/source/ConsoleLogForwarding.cpp:132

• stop() sets m_running to false before closing the pipe writers, so a reader that has not yet drained data already queued in the kernel exits at this condition and those console lines are lost during shutdown. Continue each reader until EOF after the writers are closed so pending output is drained.

    while (m_running)

media/server/service/source/ConsoleLogForwarding.cpp:82

• The descriptor swap does not flush stdout/stderr or the C++ output streams while they still refer to the pipe. Because stdout is typically fully buffered when connected to a pipe, newline-terminated console output can remain buffered and later be written to the restored descriptor after forwarding stops. Flush the relevant streams before restoring the descriptors.

    if (m_stdoutFd != -1)

media/server/service/source/ConsoleLogForwarding.cpp:32

• The result of start() is discarded, so a pipe/descriptor setup failure silently lets the server continue without forwarding any console output. In the deployment path that previously discarded these descriptors, this can make the feature fail while logs remain unavailable; handle the failure explicitly (for example, report it and apply a defined fallback or terminate startup).

    start();

media/server/service/source/ConsoleLogForwarding.cpp:151

• Because the pending data is emitted only after a newline, a writer that produces a long or unterminated stdout/stderr record makes buffer grow without bound. Console streams can contain arbitrary writes, so this can exhaust the server's memory and eventually block the producer; cap the pending record and forward bounded chunks or partial records.

        buffer.append(temp, n);

        size_t pos;
        while ((pos = buffer.find('\n')) != std::string::npos)

media/server/service/source/ConsoleLogForwarding.cpp:70

• Thread startup is not exception-safe: if construction of the second std::thread throws after the first succeeds, the partially constructed object's joinable m_stdoutThread is destroyed during constructor unwinding and calls std::terminate. Under thread/resource exhaustion this turns forwarding setup failure into a server crash; roll back and join already-created threads and restore the descriptors before propagating the failure.

    m_stdoutThread = std::thread(&ConsoleLogForwarding::readLoop, this, m_stdoutPipe[0]);

    m_stderrThread = std::thread(&ConsoleLogForwarding::readLoop, this, m_stderrPipe[0]);

media/server/service/source/ConsoleLogForwarding.cpp:40

• This new class adds file-descriptor replacement, asynchronous readers, line buffering, and shutdown handling, but the service unit-test target lists no tests for it (tests/unittests/media/server/service/CMakeLists.txt:20-44). Add focused tests for forwarding, partial-line flushing, descriptor restoration, and setup/rollback failures so these paths are exercised in CI.

bool ConsoleLogForwarding::start()

media/server/service/source/main.cpp:32

• start() is fallible, but its result is discarded here. On a failed dup2 (especially the second one), m_running is still false, so the destructor's stop() returns without rollback; stdout/stderr can remain attached to a pipe with no reader and the process can block once it fills. Handle the failure and make setup unwind all partially acquired descriptors before continuing or exiting.

    auto consoleLogForwarding{std::make_unique<firebolt::rialto::server::service::ConsoleLogForwarding>()};

serverManager/common/source/SessionServerApp.cpp:367

• The removed block also redirected STDIN_FILENO to /dev/null, but the new forwarder only replaces stdout and stderr. With console logging disabled, the exec'd session server now inherits the server manager's stdin, so a terminal or input pipe can remain attached and be consumed or block a library read. Preserve the old stdin-only redirection while forwarding stdout/stderr.

                const std::string kAppMgmtSocketStr{std::to_string(newSocket)};

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

---

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

[github-actions]

Coverage statistics of your commit:
WARNING: Lines coverage decreased from: 85.0% to 84.8%
WARNING: Functions coverage decreased from: 93.4% to 93.2%

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address descriptor cleanup, stdin isolation, buffering, and shutdown-order issues before approval.

Review effort: Lite
Findings: 1 · 1

Open (2)

• When either dup fails, m_running is still false, so stop() immediately returns and leaves the…
• The forwarder is destroyed here while appSessionServer is still alive; since appSessionServer…

[github-actions]

Coverage statistics of your commit:
WARNING: Lines coverage decreased from: 85.0% to 84.8%
WARNING: Functions coverage decreased from: 93.4% to 93.2%