From 15e1d807aa0876eafa7fa46ce708d4dfd3376586 Mon Sep 17 00:00:00 2001 From: Jared Miller Date: Tue, 10 Feb 2026 11:51:45 -0500 Subject: [PATCH] Move z-machine restore before interpreter thread start Replaces the async _do_restore() (called after thread launch) with a synchronous _try_restore() called before the thread starts. This eliminates the race condition where restore mutates z-machine state while the interpreter thread is running. The restore prefix message is now part of start()'s return value instead of being sent separately in play.py. --- src/mudlib/commands/play.py | 13 +---- src/mudlib/embedded_if_session.py | 55 ++++++++---------- tests/test_embedded_if.py | 97 +++++++++++++++++++++++++++++-- tests/test_play_command.py | 28 +++------ 4 files changed, 126 insertions(+), 67 deletions(-) diff --git a/src/mudlib/commands/play.py b/src/mudlib/commands/play.py index 96d018a..100e345 100644 --- a/src/mudlib/commands/play.py +++ b/src/mudlib/commands/play.py @@ -91,18 +91,9 @@ async def cmd_play(player: Player, args: str) -> None: await player.send("(type ::help for escape commands)\r\n") - # Check for saved game (both session types now support _do_restore) - if hasattr(session, "_do_restore") and session.save_path.exists(): - await player.send("restoring saved game...\r\n") - restored_text = await session._do_restore() - if restored_text: - await player.send(restored_text + "\r\n") - # Broadcast restored text to spectators - spectator_msg = f"[{player.name}'s terminal]\r\n{restored_text}\r\n" - await broadcast_to_spectators(player, spectator_msg) - elif intro: + if intro: await player.send(intro + "\r\n") - # Broadcast intro to spectators + # Broadcast to spectators spectator_msg = f"[{player.name}'s terminal]\r\n{intro}\r\n" await broadcast_to_spectators(player, spectator_msg) diff --git a/src/mudlib/embedded_if_session.py b/src/mudlib/embedded_if_session.py index 575ac49..2e93f90 100644 --- a/src/mudlib/embedded_if_session.py +++ b/src/mudlib/embedded_if_session.py @@ -40,15 +40,38 @@ class EmbeddedIFSession: safe_name = re.sub(r"[^a-zA-Z0-9_-]", "_", self.player.name) return self._data_dir / "if_saves" / safe_name / f"{self.game_name}.qzl" + def _try_restore(self) -> bool: + """Try to restore from save file before interpreter starts. + + Must be called before the interpreter thread is launched. + Returns True if state was restored successfully. + """ + if not self.save_path.exists(): + return False + try: + save_data = self.save_path.read_bytes() + parser = QuetzalParser(self._zmachine) + parser.load_from_bytes(save_data) + return True + except Exception as e: + logger.debug(f"Restore failed: {e}") + return False + async def start(self) -> str: + """Start the z-machine interpreter, restoring from save if available.""" + restored = self._try_restore() + self._thread = threading.Thread(target=self._run_interpreter, daemon=True) self._thread.start() loop = asyncio.get_running_loop() await loop.run_in_executor(None, self._keyboard._waiting.wait) - intro = self._screen.flush() - return intro + output = self._screen.flush() + if restored: + prefix = "restoring saved game...\r\nrestored." + return f"{prefix}\r\n\r\n{output}" if output else prefix + return output async def handle_input(self, text: str) -> IFResponse: if text.lower() == "::quit": @@ -112,34 +135,6 @@ class EmbeddedIFSession: except Exception as e: return f"error: save failed ({e})" - async def _do_restore(self) -> str: - """Restore game state from disk. Returns status message.""" - if not self.save_path.exists(): - return "" - try: - save_data = self.save_path.read_bytes() - parser = QuetzalParser(self._zmachine) - parser.load_from_bytes(save_data) - # Flush stale intro text from screen buffer - self._screen.flush() - # Feed a blank line to continue execution from restored state - self._keyboard._waiting.clear() - self._keyboard.feed("") - # Wait for interpreter to process and generate output - loop = asyncio.get_running_loop() - - def wait_for_output(): - while not self._done and not self._keyboard._waiting.is_set(): - self._keyboard._waiting.wait(timeout=0.1) - - await loop.run_in_executor(None, wait_for_output) - # Get the output from restored state - output = self._screen.flush() - return f"restored.\r\n{output}" if output else "restored." - except Exception as e: - logger.debug(f"Restore failed: {e}") - return "" - def get_location_name(self) -> str | None: try: location_obj = self._zmachine._mem.read_global(0) diff --git a/tests/test_embedded_if.py b/tests/test_embedded_if.py index a045404..e6d1e13 100644 --- a/tests/test_embedded_if.py +++ b/tests/test_embedded_if.py @@ -92,6 +92,10 @@ async def test_embedded_session_start(): player = Player(name="tester", x=5, y=5, writer=mock_writer) session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1") + # Clean up any existing save to get a fresh start + if session.save_path.exists(): + session.save_path.unlink() + intro = await session.start() assert intro is not None @@ -111,6 +115,10 @@ async def test_embedded_session_handle_input(): player = Player(name="tester", x=5, y=5, writer=mock_writer) session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1") + # Clean up any existing save to get a fresh start + if session.save_path.exists(): + session.save_path.unlink() + await session.start() response = await session.handle_input("look") @@ -204,10 +212,83 @@ async def test_embedded_session_room_objects(): assert len(objects) >= 0 # May or may not have visible objects initially +@requires_zork +@pytest.mark.asyncio +async def test_embedded_session_try_restore_before_thread(): + """_try_restore() is called synchronously before interpreter thread starts.""" + from unittest.mock import patch + + from mudlib.embedded_if_session import EmbeddedIFSession + from mudlib.player import Player + + mock_writer = MockWriter() + player = Player(name="tester", x=5, y=5, writer=mock_writer) + session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1") + + # Create a save file + if session.save_path.exists(): + session.save_path.unlink() + session.save_path.parent.mkdir(parents=True, exist_ok=True) + # Write a minimal valid save (header only, won't actually restore correctly) + session.save_path.write_bytes(b"FORM\x00\x00\x00\x08IFZSQUTZ\x00\x00\x00\x00") + + call_order = [] + + original_try_restore = session._try_restore + original_run_interpreter = session._run_interpreter + + def track_try_restore(): + call_order.append("try_restore") + return original_try_restore() + + def track_run_interpreter(): + call_order.append("run_interpreter") + original_run_interpreter() + + with ( + patch.object(session, "_try_restore", side_effect=track_try_restore), + patch.object(session, "_run_interpreter", side_effect=track_run_interpreter), + ): + await session.start() + + # Verify _try_restore was called before _run_interpreter + assert call_order[0] == "try_restore" + assert call_order[1] == "run_interpreter" + + await session.stop() + if session.save_path.exists(): + session.save_path.unlink() + + +@requires_zork +@pytest.mark.asyncio +async def test_embedded_session_no_restore_without_save(): + """start() does not restore when no save file exists.""" + from mudlib.embedded_if_session import EmbeddedIFSession + from mudlib.player import Player + + mock_writer = MockWriter() + player = Player(name="nosaveplayer", writer=mock_writer, x=0, y=0) + session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1") + + # Ensure no save file exists + if session.save_path.exists(): + session.save_path.unlink() + + intro = await session.start() + + # Should NOT contain restore message + assert "restoring" not in intro.lower() + # Should contain normal game intro + assert "ZORK" in intro or "West of House" in intro + + await session.stop() + + @requires_zork @pytest.mark.asyncio async def test_embedded_session_save_and_restore(): - """Save a game, create new session, restore it.""" + """Save a game, create new session, restore it via start().""" from mudlib.embedded_if_session import EmbeddedIFSession from mudlib.player import Player @@ -215,6 +296,10 @@ async def test_embedded_session_save_and_restore(): # Start first session player = Player(name="testplayer", writer=mock_writer, x=0, y=0) session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1") + # Clean up any existing save to get a fresh start + if session.save_path.exists(): + session.save_path.unlink() + await session.start() # Do something to change state @@ -225,11 +310,13 @@ async def test_embedded_session_save_and_restore(): assert "saved" in save_result.output.lower() await session.stop() - # Start new session - should auto-restore via play.py pattern + # Start new session - should auto-restore via start() + # start() calls _try_restore() BEFORE launching the interpreter thread session2 = EmbeddedIFSession(player, str(ZORK_PATH), "zork1") - await session2.start() - result = await session2._do_restore() - assert result # Should have restored + intro = await session2.start() + # Should contain restore message prefixed to output + assert "restoring saved game" in intro.lower() + assert "restored" in intro.lower() # The game state should reflect the restored state # (location may differ after restore, just verify it works) diff --git a/tests/test_play_command.py b/tests/test_play_command.py index bbd693a..f44946b 100644 --- a/tests/test_play_command.py +++ b/tests/test_play_command.py @@ -131,19 +131,16 @@ async def test_play_handles_dfrotz_missing(player): @pytest.mark.asyncio async def test_play_restores_save_if_exists(player): - """Playing restores saved game if save file exists.""" - from pathlib import Path - + """Playing restores saved game if save file exists (via start()).""" from mudlib.commands.play import cmd_play - # Mock IFSession + # Mock IFSession - restore now happens in start() before thread launches mock_session = Mock() - mock_session.start = AsyncMock(return_value="Welcome to Zork!") - mock_session._do_restore = AsyncMock( - return_value="West of House\nYou are standing in an open field." + restored_output = ( + "restoring saved game...\r\nrestored.\r\n\r\n" + "West of House\nYou are standing in an open field." ) - mock_session.save_path = Mock(spec=Path) - mock_session.save_path.exists = Mock(return_value=True) + mock_session.start = AsyncMock(return_value=restored_output) with patch("mudlib.commands.play.IFSession") as MockIFSession: MockIFSession.return_value = mock_session @@ -154,19 +151,15 @@ async def test_play_restores_save_if_exists(player): await cmd_play(player, "zork1") - # Verify restore was called - mock_session._do_restore.assert_called_once() - # Verify session was created and started mock_session.start.assert_called_once() # Verify mode was pushed assert "if" in player.mode_stack - # Verify restored text was sent + # Verify restored text was sent (start() returns full output with restore) calls = [call[0][0] for call in player.writer.write.call_args_list] full_output = "".join(calls) - assert "restoring" in full_output.lower() assert "West of House" in full_output assert "open field" in full_output @@ -174,16 +167,12 @@ async def test_play_restores_save_if_exists(player): @pytest.mark.asyncio async def test_play_no_restore_if_no_save(player): """Playing does not restore if no save file exists.""" - from pathlib import Path from mudlib.commands.play import cmd_play # Mock IFSession mock_session = Mock() mock_session.start = AsyncMock(return_value="Welcome to Zork!") - mock_session._do_restore = AsyncMock(return_value="") - mock_session.save_path = Mock(spec=Path) - mock_session.save_path.exists = Mock(return_value=False) with patch("mudlib.commands.play.IFSession") as MockIFSession: MockIFSession.return_value = mock_session @@ -194,9 +183,6 @@ async def test_play_no_restore_if_no_save(player): await cmd_play(player, "zork1") - # Verify restore was NOT called - mock_session._do_restore.assert_not_called() - # Verify session was created and started mock_session.start.assert_called_once()