[k-ziomek]

No description provided.

[Copilot]

🟢 Ready to approve

The changes are focused, are covered by updated unit tests, and align with the goal of preventing unbounded output retention.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR addresses potential unbounded memory growth in the rshell_server by ensuring stored command outputs don’t accumulate indefinitely and are removed after retrieval or after a TTL expires.

Changes:

• Added timestamp tracking for stored command outputs and a TTL-based cleanup routine to evict stale entries.
• Updated get_output() to remove outputs (and timestamps) once consumed to prevent indefinite retention.
• Extended unit tests to cover timestamp bookkeeping, cleanup behavior, and queue removal on client disconnect.

File summaries

File	Description
mfd_connect/rshell_server.py	Adds output timestamp tracking + stale-output cleanup, and removes outputs after retrieval/disconnect.
tests/unit/test_mfd_connect/test_rshell_server.py	Updates tests to validate output eviction, timestamp handling, and stale cleanup behavior.

Review details

Comments suppressed due to low confidence (1)

mfd_connect/rshell_server.py:65

• For consistency with monotonic TTL comparisons, store timestamps using time.monotonic() (not time.time()).

def _store_output(command_id: str, output: str, rc: int) -> None:
    """Persist command output together with its insertion timestamp."""
    _cleanup_stale_outputs()
    output_queue[command_id] = output_object(output=output, rc=rc)
    output_queue_timestamps[command_id] = time.time()

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[coveralls-official]

coverage: 80.839% (+0.2%) from 80.639% — k-ziomek:task_MFD_8903 into intel:main

[Copilot]

🟡 Not ready to approve

The TTL cleanup uses a monotonic clock while stored timestamps use wall-clock time, which prevents stale entries from ever expiring in production.

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

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

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

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Pull request overview

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

Suppressed comments (1)

mfd_connect/rshell_server/rshell_server.py:64

• _cleanup_stale_outputs() measures current_time with time.monotonic(), but _store_output() records created_at with time.time(). Mixing monotonic time with wall-clock time means current_time - created_at will typically be a large negative number, so stale entries will never be collected in production (defeating the memory-growth fix). Use the same clock source for both (prefer time.monotonic() for TTL calculations).

def _store_output(command_id: str, output: str, rc: int) -> None:
    """Persist command output together with its insertion timestamp."""
    _cleanup_stale_outputs()
    output_queue[command_id] = output_object(output=output, rc=rc)
    output_queue_timestamps[command_id] = time.time()

[Copilot]

Pull request overview

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

[adrianlasota]

fix the commits, there is merge commit

[abaczek]

Added a follow-up commit to this PR:

• Track active commands and reject unsolicited, duplicate and late results, including after abandoned-ID eviction.
• Release result payloads, queue entries and per-client state after completion or disconnect; wake disconnected callers.
• Bound results to 16 MiB each / 64 MiB total, with 512 active commands and 256 clients. Reject overflow explicitly instead of silently evicting another caller's work.
• Report transport errors separately from command output and retry client polling after HTTP errors.

Validation: 102 RShell tests and mfd-code-standard passed for this change.

The independent unread-output-pipe fix is in #53. Its branch includes both changes for combined testing; its staging base mirrors this PR. No merge is required to test the combined branch.

[Copilot]

🟡 Changes recommended

Unresolved command-lifecycle and diagnostic-header issues can block admission and break timeout reporting.

3 open findings

• Expire undelivered control commands to release client admission slots · New
• Sanitize and cap timeout diagnosis before setting response header · New
• Retain diagnostics for commands abandoned while still running · New

🧠 Review effort: Balanced

[a-gorczew]

Please, add input params to docstring as for it's for other, already existing methods

[abaczek]

Addressed in 76c5d98: added :param connection: to _close() using the existing docstring convention, including the supported None value.

[a-gorczew]

@abaczek Please capitalize the commit title.

[abaczek]

Addressed the commit-title comment: the updated commit is Fix: bound rshell result storage and clean up completed commands (76c5d98).

The four open review threads are addressed in the same commit, with focused regression tests. Validation: 119 RShell tests and mfd-code-standard passed. No changes were pushed to #53 or other PRs.