From 3e1a8df44b39972dc6c79ea2502664fdc0572be3 Mon Sep 17 00:00:00 2001 From: dmccoystephenson Date: Sun, 9 Aug 2026 20:13:16 +0000 Subject: [PATCH 1/2] fix: clean the front-end up wherever the game ends cleanup() had exactly one production caller, on the Pyodide path, so a run that finished under any other front-end released nothing: the pygame window closed without pygame.quit(), and the server-backed web front-end exited without publishing its ended screen - the tab kept polling a dead process and told a player who had just retired that the connection was lost. play() now calls it once, in a finally, so every ending (retiring, quitting, an unhandled error) goes through the same path. "Quit" in the save-file menu clears running instead of calling exit(0), which used to kill the interpreter from inside __init__ before any front-end could be released; the Pyodide entry point's SystemExit handling and its own cleanup() call go with it. The ended screen also has to survive being published: the browser only discovers it on its next poll, and cleanup() closed the socket immediately after. The server is now held open until the page has actually collected that screen, or a short timeout expires when nothing is listening, and the client stops polling once it renders it. Closes #153 Co-Authored-By: Claude Opus 5 (1M context) --- src/fishE.py | 39 +++++++++++++- src/ui/webUserInterface.py | 58 +++++++++++++++++++- tests/test_fishE.py | 82 +++++++++++++++++++++++++--- tests/ui/test_webUserInterface.py | 88 ++++++++++++++++++++++++++++++- web/pyodide_main.py | 25 +++------ 5 files changed, 261 insertions(+), 31 deletions(-) diff --git a/src/fishE.py b/src/fishE.py index e67fd4c..e987f3c 100644 --- a/src/fishE.py +++ b/src/fishE.py @@ -59,6 +59,14 @@ def __init__(self, interfaceType=INTERFACE_TYPE): # Show save file selection menu (uses the UI above) self._selectSaveFile() + # "Quit" from that menu clears running rather than ending the process, + # so the front-end still gets its cleanup() (see _selectSaveFile and + # play()). Nothing below can run without a chosen slot - get_save_path() + # raises when none was selected - and there is no run to build anyway, + # so construction stops here and play() returns immediately. + if not self.running: + return + # Load the chosen slot over the defaults if it has data. # # Existence is the only condition: a file that is present but empty is a @@ -153,7 +161,10 @@ def _selectSaveFile(self): """Display the save-file menu through the UI and let the player choose. Slots and actions are presented as numbered options (so the menu renders - and reads input through the active front-end — console or pygame).""" + and reads input through the active front-end — console or pygame). + + Returns once a slot is selected, or with self.running cleared if the + player chose "Quit" without picking one.""" while True: # loop instead of recursion to avoid stack overflow save_files = self.saveFileManager.list_save_files() @@ -217,7 +228,16 @@ def _selectSaveFile(self): self._deleteSaveFile(save_files) # loop to show the refreshed menu either way elif kind == "quit": - exit(0) + # Ending the run rather than the process. exit(0) killed the + # interpreter from inside __init__, so cleanup() was never + # reached: the pygame window vanished without pygame.quit(), + # and both browser front-ends left the tab on the save-file + # menu with no ended screen (the Pyodide entry point had to + # catch SystemExit and post one itself). __init__ returns + # early on a cleared running flag, and play() then does + # nothing but clean up - one exit path for every front-end. + self.running = False + return elif kind == "damaged": # A conforming front-end refuses to return an unavailable # option's number, so this should be unreachable. It is handled @@ -266,6 +286,21 @@ def _deleteSaveFile(self, save_files): return False def play(self): + """Run the game loop, releasing the front-end however it ends. + + The cleanup() call is here, once, rather than at each of the places a + run can finish (retiring, quitting, an unhandled error): every + front-end needs it and only this method sees all of those endings. + Without it the pygame window closed without pygame.quit(), and the + browser front-ends never published their ended screen - the tab kept + polling a server that had exited and told a player who had just retired + that the connection was lost. It is a no-op for the console.""" + try: + self._runGameLoop() + finally: + self.userInterface.cleanup() + + def _runGameLoop(self): while self.running: # show the current location and goal progress in the UI header self.userInterface.currentLocationName = self.currentLocation.capitalize() diff --git a/src/ui/webUserInterface.py b/src/ui/webUserInterface.py index ca2d63e..a362d10 100644 --- a/src/ui/webUserInterface.py +++ b/src/ui/webUserInterface.py @@ -21,6 +21,19 @@ os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "web") ) +# How long cleanup() gives the browser to collect the ended screen before it +# closes the server, and how often it checks whether that has happened. +# +# The client only discovers a new screen on its next poll, so shutting the +# socket the instant the ended screen is published closes it before the screen +# can be fetched: a player who had just retired was shown "Lost connection" +# instead of the end of their run. The wait ends as soon as the screen has +# actually gone out (typically within one poll interval), so the timeout is +# only ever paid when nobody is listening - a closed tab, or a game driven by +# something other than a browser. +ENDED_SCREEN_DELIVERY_TIMEOUT_SECONDS = 2.0 +ENDED_SCREEN_DELIVERY_POLL_SECONDS = 0.02 + def _readWebAsset(name): """Read a shared browser-client file from web/, or explain why it could not.""" @@ -99,6 +112,10 @@ def htmlPage(): failures = 0; if (recovered) version = -1; // force a re-render to clear the disconnect banner if (state.version !== version) { version = state.version; FisheClient.render(state.screen); } + // The game shuts its server down once this screen has gone out, so there + // is nothing left to poll for: stop, rather than spend the rest of the + // tab's life failing to reach a process that has finished. + if (state.screen && state.screen.type === "ended") { return; } } catch (e) { failures++; // Don't clobber the intentional "game ended" screen with a scary banner. @@ -135,8 +152,12 @@ def do_GET(self): if self.path in ("/", "/index.html"): self._send(200, "text/html; charset=utf-8", htmlPage().encode("utf-8")) elif self.path.startswith("/state"): - body = json.dumps(ui.get_state()).encode("utf-8") - self._send(200, "application/json", body) + state = ui.get_state() + self._send(200, "application/json", json.dumps(state).encode("utf-8")) + # Recorded only once the bytes are on the wire, so cleanup() + # cannot close the server out from under a response it is + # still writing (see record_state_delivered). + ui.record_state_delivered(state["version"]) else: self._send(404, "text/plain", b"Not found") @@ -184,11 +205,17 @@ def __init__( host="127.0.0.1", port=8000, start_server=True, + endedScreenTimeoutSeconds=ENDED_SCREEN_DELIVERY_TIMEOUT_SECONDS, ): super().__init__(currentPrompt, timeService, player) self._lock = threading.Lock() self._screen = {"type": "loading"} self._version = 0 + # The newest screen version the browser has been handed. Starts below + # the first version so "nothing has been collected yet" is a state + # cleanup() can tell apart from "the current screen has been seen". + self._deliveredVersion = -1 + self._endedScreenTimeoutSeconds = endedScreenTimeoutSeconds self._inputQueue = queue.Queue() self._server = None if start_server: @@ -211,6 +238,29 @@ def submit_input(self, value): """Deliver the player's browser response to the waiting game thread.""" self._inputQueue.put(value) + def record_state_delivered(self, version): + """Note that the browser has been sent the screen at this version. + + Only cleanup() reads this, to know the ended screen reached the page + before the server is closed. Kept as the highest version seen so an + overlapping poll that finishes late cannot walk it backwards.""" + with self._lock: + self._deliveredVersion = max(self._deliveredVersion, version) + + def awaitScreenDelivery(self, timeout): + """Block until the current screen has been sent to the browser. + + Returns True if it went out, False if the timeout ran out first - + which is the ordinary outcome when no page is polling.""" + deadline = time.monotonic() + timeout + while True: + with self._lock: + if self._deliveredVersion >= self._version: + return True + if time.monotonic() >= deadline: + return False + time.sleep(ENDED_SCREEN_DELIVERY_POLL_SECONDS) + # --- transport seams --------------------------------------------------- # Every screen below is built once and shared by both web front-ends; only # how a screen reaches the browser (_present) and how the response comes @@ -303,6 +353,10 @@ def timedKeyPress(self, message): def cleanup(self): self._present({"type": "ended"}) if self._server is not None: + # Hold the server open until the page has the ended screen; + # otherwise the run's last screen is never fetched and the browser + # reports a lost connection instead. + self.awaitScreenDelivery(self._endedScreenTimeoutSeconds) self._server.shutdown() self._server.server_close() self._server = None diff --git a/tests/test_fishE.py b/tests/test_fishE.py index baac6c5..cbd8fe4 100644 --- a/tests/test_fishE.py +++ b/tests/test_fishE.py @@ -900,13 +900,14 @@ def test_selectSaveFile_delete_then_quit(): # Delete submenu: Delete Slot 1 / Cancel -> "2" (Cancel) # Menu again: same options -> "4" (Quit) game.userInterface.showOptions.side_effect = ["3", "2", "4"] + game.running = True - # call/check - Quit calls exit(0), which raises SystemExit - try: - game._selectSaveFile() - assert False, "expected SystemExit" - except SystemExit as e: - assert e.code == 0 + # call - Quit returns rather than raising SystemExit; ending the process + # here skipped the front-end's cleanup() (see FishE.play) + game._selectSaveFile() + + # check - the run is over and no slot was claimed + assert game.running is False game.saveFileManager.select_save_slot.assert_not_called() @@ -1160,3 +1161,72 @@ def test_play_appends_the_fleet_report_and_the_eviction_together(): # check assert "The Marauder landed 12 fish." in game.prompt.text assert housing.EVICTION_MESSAGE in game.prompt.text + + +def test_play_cleans_up_the_front_end_when_the_run_ends(): + # Retiring or quitting used to return out of play() with nothing released: + # the pygame window closed without pygame.quit(), and neither browser + # front-end ever published its ended screen. + game = createGameForPlay() + game.locations[LocationType.HOME].run.return_value = LocationType.NONE + + # call + game.play() + + # check + game.userInterface.cleanup.assert_called_once_with() + + +def test_play_cleans_up_the_front_end_when_the_loop_raises(): + # An error mid-run is exactly when a leftover window or a still-bound + # server is hardest to explain, so cleanup happens on the way out either way. + game = createGameForPlay() + game.locations[LocationType.HOME].run.side_effect = RuntimeError("kraken") + + # call/check - the error still reaches the caller + try: + game.play() + assert False, "expected RuntimeError" + except RuntimeError as error: + assert str(error) == "kraken" + game.userInterface.cleanup.assert_called_once_with() + + +def test_quit_from_the_save_file_menu_ends_the_run_without_exiting(): + # "Quit" called exit(0) from inside __init__, ending the process before + # any front-end could clean up - which is why the Pyodide entry point had + # to catch SystemExit and publish an ended screen of its own. It now + # clears running, __init__ stops early, and play() does nothing but hand + # the front-end the same cleanup() every other ending gets. + with tempfile.TemporaryDirectory() as data_directory: + fishE.Player = Player + fishE.Stats = Stats + fishE.TimeService = TimeService + fishE.Prompt = Prompt + fishE.PlayerJsonReaderWriter = PlayerJsonReaderWriter + fishE.StatsJsonReaderWriter = StatsJsonReaderWriter + fishE.TimeServiceJsonReaderWriter = TimeServiceJsonReaderWriter + fishE.SaveFileManager = SaveFileManager + + config = Config() + config.dataDirectory = data_directory + + userInterface = MagicMock() + # An empty save directory offers "Create New Save (Slot 1)" then "Quit". + userInterface.showOptions.return_value = "2" + factory = MagicMock() + factory.create_user_interface.return_value = userInterface + + # call + with patch.object(fishE, "Config", return_value=config), patch.object( + fishE, "UserInterfaceFactory", factory + ): + game = fishE.FishE() + game.play() + + # check - no slot was claimed, nothing was written, and the front-end + # was released + assert game.running is False + assert game.saveFileManager.selected_save_slot is None + assert os.listdir(data_directory) == [] + userInterface.cleanup.assert_called_once_with() diff --git a/tests/ui/test_webUserInterface.py b/tests/ui/test_webUserInterface.py index 0a370ba..1e2b177 100644 --- a/tests/ui/test_webUserInterface.py +++ b/tests/ui/test_webUserInterface.py @@ -20,13 +20,21 @@ from world.timeService import TimeService -def makeWebUI(start_server=False, port=0): +def makeWebUI(start_server=False, port=0, endedScreenTimeoutSeconds=0.1): prompt = Prompt("What would you like to do?") player = Player() stats = Stats() timeService = TimeService(player, stats) + # No browser polls these servers, so cleanup()'s wait for the ended screen + # to be collected always runs to its timeout; the production default (two + # seconds) would be paid by every test that starts one. return WebUserInterface( - prompt, timeService, player, port=port, start_server=start_server + prompt, + timeService, + player, + port=port, + start_server=start_server, + endedScreenTimeoutSeconds=endedScreenTimeoutSeconds, ) @@ -308,3 +316,79 @@ def test_showOptions_refuses_an_unavailable_choice(): ui.submit_input("2") # available thread.join(timeout=2) assert box["result"] == "2" + + +def test_cleanup_publishes_the_ended_screen(): + # check - the run's last screen says the game is over, whether or not a + # server is involved (the Pyodide front-end inherits this path) + ui = makeWebUI() + ui.cleanup() + + assert ui.get_state()["screen"] == {"type": "ended"} + + +def test_cleanup_holds_the_server_open_until_the_ended_screen_is_fetched(): + # The browser only learns the game finished on its next poll, so closing + # the socket the instant the ended screen is published loses it: the page + # would show "Lost connection" to a player who had just retired. + ui = makeWebUI(start_server=True, port=0, endedScreenTimeoutSeconds=2.0) + host, port = ui.address + base = "http://127.0.0.1:%d" % port + + thread, box = runInThread(ui.cleanup) + try: + waitForScreen(ui, "ended") + # Still serving: the ended screen has not been collected yet. + assert thread.is_alive() + state = json.loads(urllib.request.urlopen(base + "/state", timeout=2).read()) + assert state["screen"] == {"type": "ended"} + finally: + thread.join(timeout=3) + + # check - once the page has it, the server is closed rather than left running + assert not thread.is_alive() + assert ui.address is None + + +def test_cleanup_stops_waiting_when_nothing_is_listening(): + # A closed tab (or a game driven by anything other than a browser) never + # fetches the ended screen, so the wait has to end on its own. + ui = makeWebUI(start_server=True, port=0, endedScreenTimeoutSeconds=0.2) + + startTime = time.time() + ui.cleanup() + elapsed = time.time() - startTime + + assert 0.2 <= elapsed < 2.0 + assert ui.address is None + + +def test_record_state_delivered_keeps_the_highest_version_seen(): + # Two polls can overlap, and the older one can finish last; the newer + # screen must not be reported as uncollected because of it. + ui = makeWebUI() + ui._present({"type": "dialogue", "text": "Caught a fish!"}) + version = ui.get_state()["version"] + + ui.record_state_delivered(version) + ui.record_state_delivered(version - 1) + + assert ui.awaitScreenDelivery(timeout=0) is True + + +def test_awaitScreenDelivery_reports_an_uncollected_screen(): + # check - a screen published after the last delivery is not treated as seen + ui = makeWebUI() + ui.record_state_delivered(ui.get_state()["version"]) + ui._present({"type": "dialogue", "text": "Caught a fish!"}) + + assert ui.awaitScreenDelivery(timeout=0) is False + + +def test_client_stops_polling_once_the_game_has_ended(): + # check - the server is gone by the time the page renders the ended + # screen, so the poll loop stops rather than failing every 300ms for the + # rest of the tab's life + page = webUserInterface.htmlPage() + + assert 'state.screen.type === "ended"' in page diff --git a/web/pyodide_main.py b/web/pyodide_main.py index d4c50f3..972ce3a 100644 --- a/web/pyodide_main.py +++ b/web/pyodide_main.py @@ -14,29 +14,16 @@ """ from ui.enum.uiType import UIType -from ui.pyodideUserInterface import SharedArrayBufferBridge from fishE import FishE def main(): - game = None - try: - game = FishE(interfaceType=UIType.PYODIDE) - game.play() - except SystemExit: - # Choosing "Quit" in the save-file manager calls exit(). In a browser - # tab there is no process to end, and letting SystemExit escape would - # surface as a Worker error, so it is treated as a normal finish. - pass - - # Nothing else calls cleanup() when the game loop ends, and without it the - # tab would sit on the last screen forever rather than saying it's over. - if game is not None: - game.userInterface.cleanup() - else: - # Quit before the game finished being built: there is no interface to - # clean up, so tell the page directly. - SharedArrayBufferBridge().postScreen({"type": "ended"}) + # play() publishes the ended screen through the front-end's cleanup() on + # every way out of the game - including "Quit" from the save-file manager, + # which ends the run rather than the process so a tab with no process to + # end is not a special case here (see FishE.play). + game = FishE(interfaceType=UIType.PYODIDE) + game.play() main() From 27829c9c98f71e132089a28ced19d349d20059e6 Mon Sep 17 00:00:00 2001 From: dmccoystephenson Date: Sun, 9 Aug 2026 20:37:49 +0000 Subject: [PATCH 2/2] refactor: mark the ended-screen wait private, matching its neighbours Every other internal helper on WebUserInterface (_present, _awaitInput, _header) carries the underscore; only get_state/submit_input/ record_state_delivered are public, because the request handler calls them. Co-Authored-By: Claude Opus 5 (1M context) --- src/ui/webUserInterface.py | 4 ++-- tests/ui/test_webUserInterface.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ui/webUserInterface.py b/src/ui/webUserInterface.py index a362d10..072cb61 100644 --- a/src/ui/webUserInterface.py +++ b/src/ui/webUserInterface.py @@ -247,7 +247,7 @@ def record_state_delivered(self, version): with self._lock: self._deliveredVersion = max(self._deliveredVersion, version) - def awaitScreenDelivery(self, timeout): + def _awaitScreenDelivery(self, timeout): """Block until the current screen has been sent to the browser. Returns True if it went out, False if the timeout ran out first - @@ -356,7 +356,7 @@ def cleanup(self): # Hold the server open until the page has the ended screen; # otherwise the run's last screen is never fetched and the browser # reports a lost connection instead. - self.awaitScreenDelivery(self._endedScreenTimeoutSeconds) + self._awaitScreenDelivery(self._endedScreenTimeoutSeconds) self._server.shutdown() self._server.server_close() self._server = None diff --git a/tests/ui/test_webUserInterface.py b/tests/ui/test_webUserInterface.py index 1e2b177..c9afb57 100644 --- a/tests/ui/test_webUserInterface.py +++ b/tests/ui/test_webUserInterface.py @@ -373,16 +373,16 @@ def test_record_state_delivered_keeps_the_highest_version_seen(): ui.record_state_delivered(version) ui.record_state_delivered(version - 1) - assert ui.awaitScreenDelivery(timeout=0) is True + assert ui._awaitScreenDelivery(timeout=0) is True -def test_awaitScreenDelivery_reports_an_uncollected_screen(): +def test_waiting_for_delivery_reports_an_uncollected_screen(): # check - a screen published after the last delivery is not treated as seen ui = makeWebUI() ui.record_state_delivered(ui.get_state()["version"]) ui._present({"type": "dialogue", "text": "Caught a fish!"}) - assert ui.awaitScreenDelivery(timeout=0) is False + assert ui._awaitScreenDelivery(timeout=0) is False def test_client_stops_polling_once_the_game_has_ended():