From 68ecc5787c3864db833d3216d159e6f31408d0e1 Mon Sep 17 00:00:00 2001 From: kloptops Date: Thu, 29 Oct 2026 22:11:32 +0800 Subject: [PATCH] Theme server: stopping it while its first renderer starts leaves nothing behind The managed server starts its first renderer in the background (warm()). Stopped before that finished (B soon after opening the Theme Editor, or a slow CI runner: test_managed_server failed on 3.12 and 3.13 on main), RenderPool.stop() didn't know about the zygote yet. serve.py removed its work folder and exited, and the zygote, still starting, made the folder again with mkdir(parents=True), leaving pm-theme-server-* in /tmp (seen on muOS too). - RenderPool.stop() marks the pool as stopping (nothing starts after it) and waits up to 10 s for a start under way, then stops it with the rest. PortMaster kills the server after 15 s. - worker.py makes its folders one level at a time, never parents=True: one starting after the server's folder is gone fails instead of making it. Reproduced with every core busy: the old code left the folder behind when stopped straight after starting; with this, 18 of 18 runs pass. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01NxABmR1rUUgi4iicVLZchY --- PortMaster/pylibs/theme_server/serve.py | 31 ++++++--- PortMaster/pylibs/theme_server/worker.py | 13 ++-- tests/test_theme_server.py | 28 ++++++++ tests/test_theme_server_http.py | 85 ++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 12 deletions(-) diff --git a/PortMaster/pylibs/theme_server/serve.py b/PortMaster/pylibs/theme_server/serve.py index 3e12c71a..6ac81a70 100644 --- a/PortMaster/pylibs/theme_server/serve.py +++ b/PortMaster/pylibs/theme_server/serve.py @@ -369,6 +369,8 @@ def __init__(self, themes, ports_config, work_dir, max_workers=MAX_WORKERS, use_ self.counter = 0 ## Screen sizes whose renderer is starting, for /api/status. self.starting = set() + ## Set by stop(): no renderer starts after it. + self.stopping = False def status(self): """What the renderers are doing, so the page can say: {'starting': [resolution], @@ -401,6 +403,9 @@ def _worker(self, resolution, theme_dir, scheme, lang, loaded): ## One start at a time: the page asks for every screen at once. with self.start_lock: + if self.stopping: + raise WorkerError("the server is stopping") + with self.lock: worker = self.workers.get(resolution) if worker is not None and worker.alive(): @@ -470,14 +475,24 @@ def warm(self, theme_id="default_theme", scheme=None, resolution="640x480", lang theme_dir, scheme, loaded = self._wanted(theme_id, scheme, lang) self._worker(resolution, theme_dir, scheme, lang, loaded) - def stop(self): - with self.lock: - workers, self.workers = list(self.workers.values()), {} - for worker in workers: - worker.stop() - if self.zygote: - self.zygote.stop() - self.zygote = None + def stop(self, start_timeout=10): + """Stops the renderers and the zygote: also one still starting (the first one, warm(), + can still be starting when PortMaster stops the server), which would otherwise go on + into the work folder after it's removed. Waits up to start_timeout seconds for that + start: PortMaster kills the server after 15.""" + self.stopping = True + started = self.start_lock.acquire(timeout=start_timeout) + try: + with self.lock: + workers, self.workers = list(self.workers.values()), {} + for worker in workers: + worker.stop() + if self.zygote: + self.zygote.stop() + self.zygote = None + finally: + if started: + self.start_lock.release() ################################################################################ diff --git a/PortMaster/pylibs/theme_server/worker.py b/PortMaster/pylibs/theme_server/worker.py index 58b1cb34..bbe0e659 100644 --- a/PortMaster/pylibs/theme_server/worker.py +++ b/PortMaster/pylibs/theme_server/worker.py @@ -102,7 +102,11 @@ def prepare_tools_dir(work, ports_config, theme_dir, scheme): if tools.exists(): shutil.rmtree(str(tools)) - config.mkdir(parents=True) + ## One level at a time, never parents=True: if serve.py has already removed its + ## folder (stopped while this one was starting), this fails instead of making it again. + tools.mkdir() + (tools / "PortMaster").mkdir() + config.mkdir() (tools / "PortMaster" / "themes").mkdir() (tools / "PortMaster" / "libs").mkdir() (work / "ports").mkdir(exist_ok=True) @@ -293,7 +297,8 @@ def zygote(options): control = socket.socket(fileno=options.control_fd) shared = Path(options.work) - shared.mkdir(parents=True, exist_ok=True) + ## In serve.py's folder, which is gone if it stopped while this was starting. + shared.mkdir(exist_ok=True) ## The themes (linked as renderers load them) and the config pugwash reads at start; ## each renderer's HarbourMaster has its own copy of the ports. prepare_tools_dir(shared, Path(options.ports_config), PM_DIR / "pylibs" / "default_theme", None) @@ -341,7 +346,7 @@ def zygote(options): control.close() signal.signal(signal.SIGCHLD, signal.SIG_DFL) work = Path(spawn['work']) - work.mkdir(parents=True, exist_ok=True) + work.mkdir(exist_ok=True) log = open(str(work.with_suffix(".log")), "w") os.dup2(log.fileno(), 2) os.dup2(log.fileno(), 1) @@ -391,7 +396,7 @@ def main(argv=None): width, height = parse_resolution(options.resolution) work = Path(options.work) - work.mkdir(parents=True, exist_ok=True) + work.mkdir(exist_ok=True) prepare_tools_dir(work, Path(options.ports_config), PM_DIR / "pylibs" / "default_theme", None) renderer_env(width, height, options.lang) use_tools_dir(work) diff --git a/tests/test_theme_server.py b/tests/test_theme_server.py index 16097284..538265ac 100644 --- a/tests/test_theme_server.py +++ b/tests/test_theme_server.py @@ -43,7 +43,9 @@ def test_prepare_tools_dir(tmp_path): theme.mkdir() (theme / "theme.json").write_text("{}") + ## (Its callers make the work folder: it never makes the folders above tools.) work = tmp_path / "work" + work.mkdir() assert worker.prepare_tools_dir(work, cache_config, theme, "Dark") == "My Theme" config = work / "tools" / "PortMaster" / "config" ## The screenshot folders are linked, the files copied: on a device this is PortMaster's @@ -553,6 +555,32 @@ def test_managed_server(tmp_path): proc.kill() +@pytest.mark.sdl +@pytest.mark.own_display +@pytest.mark.parametrize("delay", [0, 0.05, 0.2, 0.5, 1.0]) +def test_managed_server_stopped_while_starting(tmp_path, delay): + ## Stopped (B, or PortMaster quitting) while its first renderer is still starting: + ## that start mustn't carry on into the folder the server has already removed. + import time + + proc = start_managed(tmp_path) + try: + ready = json.loads(proc.stdout.readline() or "{}") + if not ready.get('ok'): + pytest.skip(f"the renderer can't start here: {ready.get('error')}") + + time.sleep(delay) + proc.stdin.close() + assert proc.wait(timeout=60) == 0 + ## A renderer left behind would still be writing there. + time.sleep(0.5) + assert [path for path in (tmp_path / "tmp").iterdir() if path.name.startswith("pm-theme-server-")] == [] + + finally: + if proc.poll() is None: + proc.kill() + + @pytest.mark.sdl def test_theme_editor_scene(gui, hm_dirs, monkeypatch): import time diff --git a/tests/test_theme_server_http.py b/tests/test_theme_server_http.py index ad8cc6dd..9d549f58 100644 --- a/tests/test_theme_server_http.py +++ b/tests/test_theme_server_http.py @@ -574,3 +574,88 @@ def test_copy_theme_with_changes(server, themes_dir): status, answer = post(server[0], "/api/theme_copy", {'theme': "default_theme", 'name': "Mine", 'text': '{"#info": {"name": "x"}, "main_menu": {}}'}) assert status == 200 assert json.loads((themes_dir / "Mine" / "theme.json").read_text()) == {"#info": {"name": "Mine"}, "main_menu": {}} + + +################################################################################ +## Stopping while a renderer starts +class SlowZygote: + """A zygote that takes `delay` seconds to start, and says when it was stopped.""" + delay = 0.5 + made = [] + + def __init__(self, ports_config, work): + time.sleep(self.delay) + self.stopped = False + SlowZygote.made.append(self) + + def alive(self): + return not self.stopped + + def stop(self): + self.stopped = True + + +class FakeWorker: + made = [] + + def __init__(self, *args, **kwargs): + self.stopped = False + FakeWorker.made.append(self) + + def alive(self): + return not self.stopped + + def stop(self): + self.stopped = True + + +@pytest.fixture +def slow_pool(tmp_path, themes_dir, monkeypatch): + SlowZygote.made, FakeWorker.made = [], [] + monkeypatch.setattr(serve, "Zygote", SlowZygote) + monkeypatch.setattr(serve, "Worker", FakeWorker) + return serve.RenderPool(serve.Themes(themes_dir), tmp_path / "config", tmp_path / "work") + + +def test_stop_waits_for_a_start(slow_pool): + ## warm() is still starting the first renderer when PortMaster stops the server: stop() + ## waits for it and stops what it started, or it would carry on in the removed folder. + warm = threading.Thread(target=slow_pool.warm) + warm.start() + time.sleep(0.1) + + slow_pool.stop() + warm.join() + + assert len(SlowZygote.made) == 1 and SlowZygote.made[0].stopped + assert len(FakeWorker.made) == 1 and FakeWorker.made[0].stopped + assert slow_pool.workers == {} and slow_pool.zygote is None + + +def test_nothing_starts_after_stop(slow_pool): + slow_pool.stop() + with pytest.raises(serve.WorkerError): + slow_pool.warm() + assert SlowZygote.made == [] and FakeWorker.made == [] + + +def test_stop_gives_up_waiting(slow_pool, monkeypatch): + ## A start that never ends: stop() still returns (PortMaster kills the server at 15 s). + monkeypatch.setattr(SlowZygote, "delay", 3) + warm = threading.Thread(target=slow_pool.warm, daemon=True) + warm.start() + time.sleep(0.1) + + began = time.monotonic() + slow_pool.stop(start_timeout=0.3) + assert time.monotonic() - began < 2 + + +def test_renderer_folders_need_the_servers(tmp_path): + ## A renderer starting after serve.py removed its folder fails, rather than making it again. + from theme_server import worker + + gone = tmp_path / "pm-theme-server-x" / "zygote" + with pytest.raises(FileNotFoundError): + worker.prepare_tools_dir(gone, tmp_path / "config", serve.DEFAULT_THEME_DIR, None) + assert not (tmp_path / "pm-theme-server-x").exists()