feat: Add EOF and TIMEOUT exception handling in SolConnection._deacti…

[DawidBerk]

…vate_sol_session

[Copilot]

Pull request overview

This PR improves robustness of SOL session teardown in SolConnection by retrying graceful deactivation when pexpect encounters TIMEOUT/EOF, and introducing a fallback cleanup path that finds and kills defunct/stopped ipmiutil processes. It adds unit tests to validate the retry and fallback behaviors.

Changes:

• Add retry loop and TIMEOUT/EOF handling to SolConnection._deactivate_sol_session.
• Add _kill_defunct_ipmiutil_processes fallback that parses ps output and kills stopped (STAT starts with T) ipmiutil processes.
• Add unit tests covering successful deactivation, retry behavior, fallback invocation, and kill-path edge cases.

Reviewed changes

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

File	Description
mfd_connect/sol.py	Adds retryable SOL deactivation and a fallback cleanup routine to kill defunct/stopped ipmiutil processes.
tests/unit/test_mfd_connect/test_sol.py	Adds unit tests for the new deactivation retry logic and defunct-process kill fallback.

---

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

mfd_connect/sol.py:429

• sudo/kill spawn failures (e.g., missing sudo in PATH, OSError, etc.) are not caught, which can leak raw exceptions instead of the expected SolException.

            except (pexpect.TIMEOUT, pexpect.EOF) as e:

mfd_connect/sol.py:404

• This cleanup currently kills any stopped (STAT starts with 'T') ipmiutil process, regardless of whether it’s a SoL session. That can terminate unrelated ipmiutil usage on the controller host. Since the intent is to clean up defunct SoL sessions, scope the match to ipmiutil sol ... lines.

            pid, stat, command = columns[1], columns[7], columns[10]
            if command.rsplit("/", 1)[-1] != self._ipmi_tool_name:
                continue
            if stat.startswith("T"):
                defunct_pids.append(pid)

[coveralls-official]

coverage: 80.8% (+0.2%) from 80.639% — DawidBerk:task_MFD_9170 into intel:main

[tjankows-itp]

This path of execution is not covered in unit-tests.

I don't understand the idea here. Are you sure, that the ps_output can change after interactions with the spawned process ended in line 390? If the execution reached passed lines 392-393, where any possible end-of-line or timeout exceptions are handled, I would assume that the complete output has already been acquired...

[tjankows-itp]

When using ps aux a exceeding timeout for the reply is rather unlikely, but it may be worth to just in case add also pexpect.TIMEOUT as an expected result?