From df085d9004850d94f797500ff7253f6d97971886 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 15 Apr 2026 12:22:20 +0000 Subject: [PATCH 1/3] fix(renode): address review follow-ups from PR #533 Closes #556 MEDIUM fixes: - Fix close() SocketStream leak: add RenodeMonitor.close_sync() using SocketAttribute.raw_socket for best-effort synchronous cleanup - Eliminate private attribute traversal across sibling drivers: add RenodePower.is_running property and send_monitor_command() public method so RenodeFlasher and Renode no longer access _process/_monitor - Make shared firmware state explicit: add Renode.set_firmware() method instead of directly writing parent._firmware_path/_load_command - Fix off() inconsistent state on terminate() failure: wrap process cleanup in try/finally so _process is always reset to None - Add error/cleanup path tests: on() mid-startup failure cleanup, connect() persistent OSError timeout, _read_until_prompt empty receive - Fix idempotency tests: add assertions that Popen/RenodeMonitor are NOT called on duplicate on()/off() calls LOW fixes: - Replace dead hasattr guard with power.is_running check - Extract _configure_simulation() from on() to reduce method length - Add TOCTOU race comment to _find_free_port() - Remove decorative separator comments from tests - Add test coverage: _find_renode not on PATH, read() NotImplementedError, monitor_cmd() success path, close() timeout-kill, close_sync(), is_running property, set_firmware() Co-Authored-By: Claude Opus 4.6 --- .../jumpstarter_driver_renode/driver.py | 139 +++++---- .../jumpstarter_driver_renode/driver_test.py | 294 ++++++++++++------ .../jumpstarter_driver_renode/monitor.py | 18 +- 3 files changed, 299 insertions(+), 152 deletions(-) diff --git a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py index ef8475278..728dbb5f1 100644 --- a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py +++ b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py @@ -22,11 +22,13 @@ _ELF_MAGIC = b"\x7fELF" -_ALLOWED_LOAD_COMMANDS = frozenset({ - "sysbus LoadELF", - "sysbus LoadBinary", - "sysbus LoadSymbolsFrom", -}) +_ALLOWED_LOAD_COMMANDS = frozenset( + { + "sysbus LoadELF", + "sysbus LoadBinary", + "sysbus LoadSymbolsFrom", + } +) def _detect_load_command(firmware_path: str) -> str: @@ -42,6 +44,9 @@ def _detect_load_command(firmware_path: str) -> str: def _find_free_port() -> int: + # NOTE: TOCTOU race — the port is released before Renode binds it, + # so another process could grab it first. Switching to Unix domain + # sockets would eliminate this, but Renode does not yet support them. with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.bind(("127.0.0.1", 0)) return s.getsockname()[1] @@ -50,10 +55,7 @@ def _find_free_port() -> int: def _find_renode() -> str: path = shutil.which("renode") if path is None: - raise FileNotFoundError( - "renode executable not found in PATH. " - "Install Renode from https://renode.io/" - ) + raise FileNotFoundError("renode executable not found in PATH. Install Renode from https://renode.io/") return path @@ -70,10 +72,7 @@ async def flash(self, source, load_command: str | None = None): and resets the machine. """ if load_command is not None and load_command not in _ALLOWED_LOAD_COMMANDS: - raise ValueError( - f"unsupported load_command {load_command!r}, " - f"allowed: {sorted(_ALLOWED_LOAD_COMMANDS)}" - ) + raise ValueError(f"unsupported load_command {load_command!r}, allowed: {sorted(_ALLOWED_LOAD_COMMANDS)}") firmware_path = self.parent._tmp_dir.name + "/firmware" async with await FileWriteStream.from_path(firmware_path) as stream: @@ -85,15 +84,13 @@ async def flash(self, source, load_command: str | None = None): cmd = load_command else: cmd = _detect_load_command(firmware_path) - self.parent._firmware_path = firmware_path - self.parent._load_command = cmd + self.parent.set_firmware(firmware_path, cmd) - if hasattr(self.parent.children["power"], "_process"): - monitor = self.parent.children["power"]._monitor - if monitor is not None: - await monitor.execute(f'{cmd} @"{firmware_path}"') - await monitor.execute("machine Reset") - self.logger.info("firmware hot-loaded and machine reset") + power: RenodePower = self.parent.children["power"] + if power.is_running: + await power.send_monitor_command(f'{cmd} @"{firmware_path}"') + await power.send_monitor_command("machine Reset") + self.logger.info("firmware hot-loaded and machine reset") @export async def dump(self, target, partition: str | None = None): @@ -110,6 +107,21 @@ class RenodePower(PowerInterface, Driver): _process: Popen | None = field(init=False, default=None, repr=False) _monitor: RenodeMonitor | None = field(init=False, default=None, repr=False) + @property + def is_running(self) -> bool: + """Whether the Renode process is running with an active monitor.""" + return self._process is not None and self._monitor is not None + + async def send_monitor_command(self, command: str) -> str: + """Send a command to the Renode monitor. + + Provides a public interface for sibling drivers to interact with + the monitor without accessing private attributes directly. + """ + if self._monitor is None: + raise RuntimeError("Renode is not running") + return await self._monitor.execute(command) + @export async def on(self) -> None: """Start Renode, connect monitor, configure platform, and begin simulation.""" @@ -135,37 +147,31 @@ async def on(self) -> None: self._monitor = RenodeMonitor() try: await self._monitor.connect("127.0.0.1", port) - - machine = self.parent.machine_name - self._monitor.add_expected_prompt(machine) - await self._monitor.execute(f'mach create "{machine}"') - await self._monitor.execute( - f"machine LoadPlatformDescription @{self.parent.platform}" - ) - - pty_path = self.parent._pty - await self._monitor.execute( - f'emulation CreateUartPtyTerminal "term" "{pty_path}"' - ) - await self._monitor.execute( - f"connector Connect {self.parent.uart} term" - ) - - for cmd in self.parent.extra_commands: - await self._monitor.execute(cmd) - - if self.parent._firmware_path: - load_cmd = self.parent._load_command or "sysbus LoadELF" - await self._monitor.execute( - f'{load_cmd} @"{self.parent._firmware_path}"' - ) - + await self._configure_simulation() await self._monitor.execute("start") self.logger.info("Renode simulation started") except Exception: await self.off() raise + async def _configure_simulation(self) -> None: + """Set up the machine, platform, UART, and firmware in the monitor.""" + machine = self.parent.machine_name + self._monitor.add_expected_prompt(machine) + await self._monitor.execute(f'mach create "{machine}"') + await self._monitor.execute(f"machine LoadPlatformDescription @{self.parent.platform}") + + pty_path = self.parent._pty + await self._monitor.execute(f'emulation CreateUartPtyTerminal "term" "{pty_path}"') + await self._monitor.execute(f"connector Connect {self.parent.uart} term") + + for cmd in self.parent.extra_commands: + await self._monitor.execute(cmd) + + if self.parent._firmware_path: + load_cmd = self.parent._load_command or "sysbus LoadELF" + await self._monitor.execute(f'{load_cmd} @"{self.parent._firmware_path}"') + @export async def off(self) -> None: """Stop simulation, disconnect monitor, and terminate the Renode process.""" @@ -181,12 +187,14 @@ async def off(self) -> None: await self._monitor.disconnect() self._monitor = None - self._process.terminate() try: - await to_thread.run_sync(self._process.wait, 5) - except TimeoutExpired: - self._process.kill() - self._process = None + self._process.terminate() + try: + await to_thread.run_sync(self._process.wait, 5) + except TimeoutExpired: + self._process.kill() + finally: + self._process = None @export async def read(self) -> AsyncGenerator[PowerReading, None]: @@ -197,13 +205,16 @@ def close(self): """Synchronous cleanup for use during driver teardown.""" if self._process is not None: if self._monitor is not None: + self._monitor.close_sync() self._monitor = None - self._process.terminate() try: - self._process.wait(timeout=5) - except TimeoutExpired: - self._process.kill() - self._process = None + self._process.terminate() + try: + self._process.wait(timeout=5) + except TimeoutExpired: + self._process.kill() + finally: + self._process = None @dataclass(kw_only=True) @@ -228,9 +239,7 @@ class Renode(Driver): extra_commands: list[str] = field(default_factory=list) allow_raw_monitor: bool = False - _tmp_dir: TemporaryDirectory = field( - init=False, default_factory=TemporaryDirectory - ) + _tmp_dir: TemporaryDirectory = field(init=False, default_factory=TemporaryDirectory) _firmware_path: str | None = field(init=False, default=None) _load_command: str | None = field(init=False, default=None) _active_monitor_port: int = field(init=False, default=0) @@ -248,6 +257,11 @@ def __post_init__(self): self.children["flasher"] = RenodeFlasher(parent=self) self.children["console"] = PySerial(url=self._pty, check_present=False) + def set_firmware(self, path: str, load_command: str) -> None: + """Set the firmware path and load command for the next power-on.""" + self._firmware_path = path + self._load_command = load_command + @property def _pty(self) -> str: return str(Path(self._tmp_dir.name) / "pty") @@ -275,10 +289,7 @@ async def monitor_cmd(self, command: str) -> str: """ if not self.allow_raw_monitor: raise RuntimeError( - "raw monitor access is disabled; " - "set allow_raw_monitor: true in exporter config to enable" + "raw monitor access is disabled; set allow_raw_monitor: true in exporter config to enable" ) power: RenodePower = self.children["power"] - if power._monitor is None: - raise RuntimeError("Renode is not running") - return await power._monitor.execute(command) + return await power.send_monitor_command(command) diff --git a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py index 5051a2cfa..fa5d25f10 100644 --- a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py +++ b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py @@ -12,6 +12,7 @@ RenodeFlasher, RenodePower, _detect_load_command, + _find_renode, ) from jumpstarter_driver_renode.monitor import RenodeMonitor, RenodeMonitorError @@ -23,11 +24,6 @@ def anyio_backend(): return "asyncio" -# --------------------------------------------------------------------------- -# 5.1 RenodeMonitor unit tests -# --------------------------------------------------------------------------- - - class TestRenodeMonitor: @pytest.mark.anyio async def test_monitor_connect_retry(self): @@ -41,18 +37,14 @@ async def mock_connect_tcp(host, port): if call_count < 3: raise OSError("Connection refused") stream = AsyncMock() - stream.receive = AsyncMock( - return_value=b"Renode v1.15\n(monitor) \n" - ) + stream.receive = AsyncMock(return_value=b"Renode v1.15\n(monitor) \n") return stream with patch( "jumpstarter_driver_renode.monitor.connect_tcp", side_effect=mock_connect_tcp, ): - with patch( - "jumpstarter_driver_renode.monitor.sleep", new_callable=AsyncMock - ): + with patch("jumpstarter_driver_renode.monitor.sleep", new_callable=AsyncMock): await monitor.connect("127.0.0.1", 12345) assert call_count == 3 @@ -63,9 +55,7 @@ async def test_monitor_execute_command(self): """Execute sends command and returns response text.""" monitor = RenodeMonitor() stream = AsyncMock() - responses = iter( - [b"some output\n(monitor) \n", b""] - ) + responses = iter([b"some output\n(monitor) \n", b""]) stream.receive = AsyncMock(side_effect=lambda size: next(responses)) monitor._stream = stream monitor._buffer = b"" @@ -79,9 +69,7 @@ async def test_monitor_execute_error_response(self): """Monitor raises RenodeMonitorError on error responses.""" monitor = RenodeMonitor() stream = AsyncMock() - stream.receive = AsyncMock( - return_value=b"Could not find peripheral\n(monitor) \n" - ) + stream.receive = AsyncMock(return_value=b"Could not find peripheral\n(monitor) \n") monitor._stream = stream monitor._buffer = b"" @@ -146,18 +134,14 @@ async def mock_connect_tcp(host, port): if call_count < 2: stream.receive = AsyncMock(side_effect=OSError("not ready")) else: - stream.receive = AsyncMock( - return_value=b"Renode v1.15\n(monitor) \n" - ) + stream.receive = AsyncMock(return_value=b"Renode v1.15\n(monitor) \n") return stream with patch( "jumpstarter_driver_renode.monitor.connect_tcp", side_effect=mock_connect_tcp, ): - with patch( - "jumpstarter_driver_renode.monitor.sleep", new_callable=AsyncMock - ): + with patch("jumpstarter_driver_renode.monitor.sleep", new_callable=AsyncMock): await monitor.connect("127.0.0.1", 12345) streams[0].aclose.assert_called_once() @@ -167,9 +151,7 @@ async def test_monitor_error_detection_per_line(self): """Error markers are detected even when not on the first line.""" monitor = RenodeMonitor() stream = AsyncMock() - stream.receive = AsyncMock( - return_value=b"info text\nError executing command\n(monitor) \n" - ) + stream.receive = AsyncMock(return_value=b"info text\nError executing command\n(monitor) \n") monitor._stream = stream monitor._buffer = b"" @@ -187,10 +169,53 @@ def test_monitor_prompt_matches_expected_only(self): assert monitor._is_prompt(b"(my-machine)") is True assert monitor._is_prompt(b"(other)") is False + @pytest.mark.anyio + async def test_connect_timeout_on_persistent_error(self): + """connect() raises TimeoutError when OSError persists.""" + monitor = RenodeMonitor() + + async def always_fail(host, port): + raise OSError("Connection refused") + + with patch( + "jumpstarter_driver_renode.monitor.connect_tcp", + side_effect=always_fail, + ): + with pytest.raises(TimeoutError): + await monitor.connect("127.0.0.1", 12345, timeout=0.5) + + @pytest.mark.anyio + async def test_read_until_prompt_connection_closed(self): + """_read_until_prompt raises ConnectionError on empty receive.""" + monitor = RenodeMonitor() + stream = AsyncMock() + stream.receive = AsyncMock(return_value=b"") + monitor._stream = stream + monitor._buffer = b"" + + with pytest.raises(ConnectionError, match="connection closed"): + await monitor._read_until_prompt() -# --------------------------------------------------------------------------- -# 5.2 RenodePower unit tests -# --------------------------------------------------------------------------- + def test_close_sync_closes_raw_socket(self): + """close_sync() closes the underlying socket and clears state.""" + monitor = RenodeMonitor() + mock_socket = MagicMock() + stream = MagicMock() + stream.extra = MagicMock(return_value=mock_socket) + monitor._stream = stream + monitor._buffer = b"leftover" + + monitor.close_sync() + + mock_socket.close.assert_called_once() + assert monitor._stream is None + assert monitor._buffer == b"" + + def test_close_sync_no_stream(self): + """close_sync() is safe to call when not connected.""" + monitor = RenodeMonitor() + monitor.close_sync() + assert monitor._stream is None def _make_driver(**kwargs) -> Renode: @@ -218,9 +243,7 @@ async def test_power_on_command_sequence(self): "jumpstarter_driver_renode.driver._find_free_port", return_value=54321, ): - with patch( - "jumpstarter_driver_renode.driver.Popen" - ) as mock_popen: + with patch("jumpstarter_driver_renode.driver.Popen") as mock_popen: mock_popen.return_value = MagicMock() with patch( "jumpstarter_driver_renode.driver.RenodeMonitor", @@ -240,9 +263,7 @@ async def test_power_on_command_sequence(self): @pytest.mark.anyio async def test_power_on_with_extra_commands(self): """Extra commands are sent between connector Connect and LoadELF.""" - driver = _make_driver( - extra_commands=["sysbus WriteDoubleWord 0x40090030 0x0301"] - ) + driver = _make_driver(extra_commands=["sysbus WriteDoubleWord 0x40090030 0x0301"]) driver._firmware_path = "/tmp/test.elf" power: RenodePower = driver.children["power"] mock_monitor = AsyncMock(spec=RenodeMonitor) @@ -255,9 +276,7 @@ async def test_power_on_with_extra_commands(self): "jumpstarter_driver_renode.driver._find_free_port", return_value=54321, ): - with patch( - "jumpstarter_driver_renode.driver.Popen" - ) as mock_popen: + with patch("jumpstarter_driver_renode.driver.Popen") as mock_popen: mock_popen.return_value = MagicMock() with patch( "jumpstarter_driver_renode.driver.RenodeMonitor", @@ -266,17 +285,9 @@ async def test_power_on_with_extra_commands(self): await power.on() calls = [c.args[0] for c in mock_monitor.execute.call_args_list] - connect_idx = next( - i for i, c in enumerate(calls) if "connector Connect" in c - ) - load_idx = next( - i for i, c in enumerate(calls) if "LoadELF" in c - ) - extra_idx = next( - i - for i, c in enumerate(calls) - if "WriteDoubleWord" in c - ) + connect_idx = next(i for i, c in enumerate(calls) if "connector Connect" in c) + load_idx = next(i for i, c in enumerate(calls) if "LoadELF" in c) + extra_idx = next(i for i, c in enumerate(calls) if "WriteDoubleWord" in c) assert connect_idx < extra_idx < load_idx @pytest.mark.anyio @@ -294,9 +305,7 @@ async def test_power_on_without_firmware(self): "jumpstarter_driver_renode.driver._find_free_port", return_value=54321, ): - with patch( - "jumpstarter_driver_renode.driver.Popen" - ) as mock_popen: + with patch("jumpstarter_driver_renode.driver.Popen") as mock_popen: mock_popen.return_value = MagicMock() with patch( "jumpstarter_driver_renode.driver.RenodeMonitor", @@ -315,7 +324,12 @@ async def test_power_on_idempotent(self): power: RenodePower = driver.children["power"] power._process = MagicMock() - await power.on() + with patch("jumpstarter_driver_renode.driver.Popen") as mock_popen: + with patch("jumpstarter_driver_renode.driver.RenodeMonitor") as mock_monitor_cls: + await power.on() + + mock_popen.assert_not_called() + mock_monitor_cls.assert_not_called() @pytest.mark.anyio async def test_power_off_terminates_process(self): @@ -366,6 +380,9 @@ async def test_power_off_idempotent(self): await power.off() + assert power._process is None + assert power._monitor is None + @pytest.mark.anyio async def test_power_close_calls_off(self): """close() terminates the process.""" @@ -380,10 +397,110 @@ async def test_power_close_calls_off(self): mock_process.terminate.assert_called_once() assert power._process is None + def test_close_kills_on_timeout(self): + """close() kills process when wait() times out.""" + driver = _make_driver() + power: RenodePower = driver.children["power"] + mock_process = MagicMock() + mock_process.wait = MagicMock(side_effect=TimeoutExpired("renode", 5)) + power._process = mock_process + + power.close() + + mock_process.terminate.assert_called_once() + mock_process.kill.assert_called_once() + assert power._process is None + + def test_close_cleans_up_monitor_socket(self): + """close() calls close_sync() on the monitor before terminating.""" + driver = _make_driver() + power: RenodePower = driver.children["power"] + mock_process = MagicMock() + mock_process.wait = MagicMock() + power._process = mock_process + mock_monitor = MagicMock(spec=RenodeMonitor) + power._monitor = mock_monitor + + power.close() + + mock_monitor.close_sync.assert_called_once() + assert power._monitor is None + assert power._process is None + + @pytest.mark.anyio + async def test_power_on_cleanup_on_failure(self): + """on() cleans up process when monitor setup fails.""" + driver = _make_driver() + power: RenodePower = driver.children["power"] + + mock_monitor = AsyncMock(spec=RenodeMonitor) + mock_monitor.execute.side_effect = RenodeMonitorError("setup failed") + mock_process = MagicMock() + mock_process.wait = MagicMock() + + with patch( + "jumpstarter_driver_renode.driver._find_renode", + return_value="/usr/bin/renode", + ): + with patch( + "jumpstarter_driver_renode.driver._find_free_port", + return_value=54321, + ): + with patch( + "jumpstarter_driver_renode.driver.Popen", + return_value=mock_process, + ): + with patch( + "jumpstarter_driver_renode.driver.RenodeMonitor", + return_value=mock_monitor, + ): + with pytest.raises(RenodeMonitorError): + await power.on() + + assert power._process is None + assert power._monitor is None + mock_process.terminate.assert_called_once() + + @pytest.mark.anyio + async def test_off_cleans_up_on_terminate_failure(self): + """off() resets _process to None even if terminate() raises.""" + driver = _make_driver() + power: RenodePower = driver.children["power"] + + mock_process = MagicMock() + mock_process.terminate = MagicMock(side_effect=ProcessLookupError) + power._process = mock_process + power._monitor = AsyncMock(spec=RenodeMonitor) + + await power.off() + + assert power._process is None + assert power._monitor is None + + @pytest.mark.anyio + async def test_power_read_not_implemented(self): + """read() raises NotImplementedError.""" + driver = _make_driver() + power: RenodePower = driver.children["power"] -# --------------------------------------------------------------------------- -# 5.3 RenodeFlasher unit tests -# --------------------------------------------------------------------------- + with pytest.raises(NotImplementedError): + await power.read() + + def test_is_running_property(self): + """is_running reflects process and monitor state.""" + driver = _make_driver() + power: RenodePower = driver.children["power"] + + assert power.is_running is False + + power._process = MagicMock() + assert power.is_running is False + + power._monitor = MagicMock() + assert power.is_running is True + + power._process = None + assert power.is_running is False class TestRenodeFlasher: @@ -401,12 +518,8 @@ async def test_flash_stores_firmware_path(self, tmp_path): with patch.object(flasher, "resource") as mock_resource: mock_res = AsyncMock() mock_res.__aiter__ = lambda self: self - mock_res.__anext__ = AsyncMock( - side_effect=[firmware_data, StopAsyncIteration()] - ) - mock_resource.return_value.__aenter__ = AsyncMock( - return_value=mock_res - ) + mock_res.__anext__ = AsyncMock(side_effect=[firmware_data, StopAsyncIteration()]) + mock_resource.return_value.__aenter__ = AsyncMock(return_value=mock_res) mock_resource.return_value.__aexit__ = AsyncMock() await flasher.flash(str(firmware_file)) @@ -429,12 +542,8 @@ async def test_flash_while_running_sends_load_and_reset(self): with patch.object(flasher, "resource") as mock_resource: mock_res = AsyncMock() mock_res.__aiter__ = lambda self: self - mock_res.__anext__ = AsyncMock( - side_effect=[elf_data, StopAsyncIteration()] - ) - mock_resource.return_value.__aenter__ = AsyncMock( - return_value=mock_res - ) + mock_res.__anext__ = AsyncMock(side_effect=[elf_data, StopAsyncIteration()]) + mock_resource.return_value.__aenter__ = AsyncMock(return_value=mock_res) mock_resource.return_value.__aexit__ = AsyncMock() await flasher.flash("/some/firmware.elf") @@ -452,12 +561,8 @@ async def test_flash_custom_load_command(self): with patch.object(flasher, "resource") as mock_resource: mock_res = AsyncMock() mock_res.__aiter__ = lambda self: self - mock_res.__anext__ = AsyncMock( - side_effect=[b"\x00", StopAsyncIteration()] - ) - mock_resource.return_value.__aenter__ = AsyncMock( - return_value=mock_res - ) + mock_res.__anext__ = AsyncMock(side_effect=[b"\x00", StopAsyncIteration()]) + mock_resource.return_value.__aenter__ = AsyncMock(return_value=mock_res) mock_resource.return_value.__aexit__ = AsyncMock() await flasher.flash( @@ -498,11 +603,6 @@ def test_detect_load_command_binary(self, tmp_path): assert _detect_load_command(str(raw)) == "sysbus LoadBinary" -# --------------------------------------------------------------------------- -# 5.4 Configuration validation tests -# --------------------------------------------------------------------------- - - class TestRenodeConfig: def test_renode_defaults(self): """Default values are set correctly.""" @@ -565,10 +665,35 @@ def test_renode_get_machine_name(self): driver = _make_driver(machine_name="test-mcu") assert driver.get_machine_name() == "test-mcu" + def test_find_renode_not_on_path(self): + """_find_renode raises FileNotFoundError when binary is not on PATH.""" + with patch( + "jumpstarter_driver_renode.driver.shutil.which", + return_value=None, + ): + with pytest.raises(FileNotFoundError, match="renode executable not found"): + _find_renode() -# --------------------------------------------------------------------------- -# 5.5 E2E test with serve() -# --------------------------------------------------------------------------- + def test_set_firmware(self): + """set_firmware stores path and command on the driver.""" + driver = _make_driver() + driver.set_firmware("/tmp/fw.elf", "sysbus LoadELF") + assert driver._firmware_path == "/tmp/fw.elf" + assert driver._load_command == "sysbus LoadELF" + + @pytest.mark.anyio + async def test_monitor_cmd_success(self): + """monitor_cmd succeeds when allow_raw_monitor is True and running.""" + driver = _make_driver(allow_raw_monitor=True) + power: RenodePower = driver.children["power"] + mock_monitor = AsyncMock(spec=RenodeMonitor) + mock_monitor.execute = AsyncMock(return_value="OK\n") + power._process = MagicMock() + power._monitor = mock_monitor + + result = await driver.monitor_cmd("version") + assert result == "OK\n" + mock_monitor.execute.assert_called_once_with("version") @pytest.mark.skipif( @@ -591,11 +716,6 @@ def test_driver_renode_e2e(tmp_path): renode.power.off() -# --------------------------------------------------------------------------- -# 5.6 Client and CLI tests -# --------------------------------------------------------------------------- - - class TestRenodeClient: def test_client_serve_properties(self): """Client properties round-trip through serve().""" diff --git a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py index 1f110782c..3ba49cf86 100644 --- a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py +++ b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py @@ -3,7 +3,7 @@ import logging from anyio import connect_tcp, fail_after, sleep -from anyio.abc import SocketStream +from anyio.abc import SocketAttribute, SocketStream logger = logging.getLogger(__name__) @@ -90,6 +90,22 @@ async def disconnect(self) -> None: self._stream = None self._buffer = b"" + def close_sync(self) -> None: + """Best-effort synchronous close of the monitor connection. + + Used during synchronous driver teardown when an event loop may + not be available for ``await disconnect()``. + """ + stream = self._stream + self._stream = None + self._buffer = b"" + if stream is not None: + try: + raw_sock = stream.extra(SocketAttribute.raw_socket) + raw_sock.close() + except Exception: + pass + async def _read_until_prompt(self) -> str: """Read from the stream until a monitor prompt line is detected. From cbc0316760ed646d3dce1a2356fc9b31a1e6cf45 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 15 Apr 2026 12:45:31 +0000 Subject: [PATCH 2/3] fix(renode): catch ProcessLookupError in off() and close() ProcessLookupError is expected when the process has already exited. The try/finally ensures _process is reset to None, but the exception must also be caught to prevent it from propagating to callers. Co-Authored-By: Claude Opus 4.6 --- .../jumpstarter_driver_renode/driver.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py index 728dbb5f1..37ca63c54 100644 --- a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py +++ b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py @@ -193,6 +193,8 @@ async def off(self) -> None: await to_thread.run_sync(self._process.wait, 5) except TimeoutExpired: self._process.kill() + except ProcessLookupError: + pass finally: self._process = None @@ -213,6 +215,8 @@ def close(self): self._process.wait(timeout=5) except TimeoutExpired: self._process.kill() + except ProcessLookupError: + pass finally: self._process = None From f4fd0a65279be8aca4dd252f55d86e0749e6268e Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 15 Apr 2026 13:35:23 +0000 Subject: [PATCH 3/3] fix(renode): fix LoadELF parameter format and improve error detection - Remove spurious '@' prefix from LoadELF file path argument that caused a parameter mismatch error in Renode - Add "There was an error" and "Parameters did not match" to monitor error markers so these failures are caught immediately - Strip leading whitespace from response lines before checking error markers so indented error messages are not missed - Add tests for parameter mismatch detection and leading whitespace Co-Authored-By: Claude Opus 4.6 --- .../jumpstarter_driver_renode/driver.py | 2 +- .../jumpstarter_driver_renode/driver_test.py | 33 +++++++++++++++++++ .../jumpstarter_driver_renode/monitor.py | 13 ++++++-- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py index 37ca63c54..2ce08813b 100644 --- a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py +++ b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py @@ -170,7 +170,7 @@ async def _configure_simulation(self) -> None: if self.parent._firmware_path: load_cmd = self.parent._load_command or "sysbus LoadELF" - await self._monitor.execute(f'{load_cmd} @"{self.parent._firmware_path}"') + await self._monitor.execute(f'{load_cmd} "{self.parent._firmware_path}"') @export async def off(self) -> None: diff --git a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py index fa5d25f10..c9f18ab4e 100644 --- a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py +++ b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py @@ -158,6 +158,39 @@ async def test_monitor_error_detection_per_line(self): with pytest.raises(RenodeMonitorError, match="Error executing"): await monitor.execute("bad command") + @pytest.mark.anyio + async def test_monitor_detects_parameter_mismatch_error(self): + """Monitor raises RenodeMonitorError on parameter mismatch responses.""" + monitor = RenodeMonitor() + stream = AsyncMock() + stream.receive = AsyncMock( + return_value=( + b"The following methods are available:\n" + b" - Void LoadELF (ReadFilePath fileName)\n" + b"\n" + b"There was an error executing command 'sysbus LoadELF'\n" + b"Parameters did not match the signature\n" + b"(monitor) \n" + ) + ) + monitor._stream = stream + monitor._buffer = b"" + + with pytest.raises(RenodeMonitorError, match="There was an error"): + await monitor.execute('sysbus LoadELF @"/tmp/firmware"') + + @pytest.mark.anyio + async def test_monitor_detects_error_with_leading_whitespace(self): + """Error markers are detected even with leading whitespace.""" + monitor = RenodeMonitor() + stream = AsyncMock() + stream.receive = AsyncMock(return_value=b" Could not find peripheral 'foo'\n(monitor) \n") + monitor._stream = stream + monitor._buffer = b"" + + with pytest.raises(RenodeMonitorError, match="Could not find"): + await monitor.execute("bad command") + def test_monitor_prompt_matches_expected_only(self): """_is_prompt only matches prompts in the expected set.""" monitor = RenodeMonitor() diff --git a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py index 3ba49cf86..83211f8e5 100644 --- a/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py +++ b/python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py @@ -52,7 +52,15 @@ async def connect(self, host: str, port: int, timeout: float = 10) -> None: self._stream = None await sleep(0.5) - _ERROR_MARKERS = ("Could not find", "Error", "Invalid", "Failed", "Unknown") + _ERROR_MARKERS = ( + "Could not find", + "Error", + "Invalid", + "Failed", + "Unknown", + "There was an error", + "Parameters did not match", + ) async def execute(self, command: str, timeout: float = 30) -> str: """Send a command and return the response text (excluding the prompt). @@ -75,7 +83,8 @@ async def execute(self, command: str, timeout: float = 30) -> str: stripped = response.strip() if stripped: for line in stripped.splitlines(): - if any(line.startswith(m) for m in self._ERROR_MARKERS): + clean = line.strip() + if any(clean.startswith(m) for m in self._ERROR_MARKERS): raise RenodeMonitorError(stripped) return response