From 09d8e28b2b0491a72470a5360fb0fbe55914bd57 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 21:13:00 +0000 Subject: [PATCH 1/4] Stop child processes of server: commands on cleanup A string server: command runs via "sh -c". On systems where /bin/sh is dash the shell forks the command rather than exec-ing it, so killing the shell left the actual server running, orphaned - holding its port and keeping any pipe on its stdout open. Servers now run in their own process group, which is killed as a whole when shot-scraper multi or video finishes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VL4haavg5i8JXVFFrPU8p3 --- shot_scraper/cli.py | 25 +++++++++++++++++++++++-- tests/test_shot_scraper.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/shot_scraper/cli.py b/shot_scraper/cli.py index 9e5a15c..9b70695 100644 --- a/shot_scraper/cli.py +++ b/shot_scraper/cli.py @@ -1,5 +1,6 @@ import base64 import secrets +import signal import socket import subprocess import sys @@ -1742,11 +1743,31 @@ def _run_python_code(code): ) from ex +class _ServerProcess(subprocess.Popen): + """ + Runs a server: command in its own process group, so kill() also stops + any child processes - such as the command started by "sh -c" + """ + + def __init__(self, args, **kwargs): + if os.name == "posix": + kwargs["start_new_session"] = True + super().__init__(args, **kwargs) + + def kill(self): + if os.name != "posix": + return super().kill() + try: + os.killpg(self.pid, signal.SIGKILL) + except ProcessLookupError: + pass + + def _start_server(server): if isinstance(server, str): - proc = subprocess.Popen(server, shell=True) + proc = _ServerProcess(server, shell=True) elif isinstance(server, list): - proc = subprocess.Popen(map(str, server)) + proc = _ServerProcess(map(str, server)) else: raise click.ClickException("server: must be a string or list") return proc, server diff --git a/tests/test_shot_scraper.py b/tests/test_shot_scraper.py index 7d80404..c10e663 100644 --- a/tests/test_shot_scraper.py +++ b/tests/test_shot_scraper.py @@ -61,6 +61,35 @@ def test_multi_server(yaml): assert pathlib.Path("output.png").exists() +def _port_is_open(port): + try: + with socket.create_connection(("127.0.0.1", port), timeout=1): + return True + except OSError: + return False + + +@pytest.mark.skipif(sys.platform == "win32", reason="Uses POSIX shell syntax") +def test_multi_server_string_child_process_is_stopped(): + # The trailing command stops the shell from exec-ing python, so python + # runs as a child of "sh -c" - that child should still be stopped + port = find_free_port() + runner = CliRunner() + with runner.isolated_filesystem(): + open("server.yaml", "w").write( + f'- server: "python -m http.server {port}; echo stopped"\n' + f"- url: http://127.0.0.1:{port}/\n" + f" output: output.png\n" + ) + result = runner.invoke(cli, ["multi", "server.yaml"]) + assert result.exit_code == 0, result.output + assert pathlib.Path("output.png").exists() + deadline = time.monotonic() + 5 + while _port_is_open(port) and time.monotonic() < deadline: + time.sleep(0.1) + assert not _port_is_open(port) + + def test_multi_server_slow_start(): # https://github.com/simonw/shot-scraper/issues/197 # A server that takes longer than one second to start listening From dcd16816c1ffc96b5f833eef6c8bb65e5cfa2c23 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 21:23:38 +0000 Subject: [PATCH 2/4] Stop servers with plain functions instead of a Popen subclass _start_server() passes start_new_session to Popen and the new _stop_server() kills the process group. Tests that fake server processes now mock _stop_server(). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VL4haavg5i8JXVFFrPU8p3 --- shot_scraper/cli.py | 39 ++++++++++++++++---------------------- tests/test_shot_scraper.py | 19 +++++++++++-------- 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/shot_scraper/cli.py b/shot_scraper/cli.py index 9b70695..359fd39 100644 --- a/shot_scraper/cli.py +++ b/shot_scraper/cli.py @@ -1743,31 +1743,14 @@ def _run_python_code(code): ) from ex -class _ServerProcess(subprocess.Popen): - """ - Runs a server: command in its own process group, so kill() also stops - any child processes - such as the command started by "sh -c" - """ - - def __init__(self, args, **kwargs): - if os.name == "posix": - kwargs["start_new_session"] = True - super().__init__(args, **kwargs) - - def kill(self): - if os.name != "posix": - return super().kill() - try: - os.killpg(self.pid, signal.SIGKILL) - except ProcessLookupError: - pass - - def _start_server(server): + # Run the server in its own process group so _stop_server() can stop + # any child processes too, such as the command started by "sh -c" + popen_kwargs = {"start_new_session": True} if os.name == "posix" else {} if isinstance(server, str): - proc = _ServerProcess(server, shell=True) + proc = subprocess.Popen(server, shell=True, **popen_kwargs) elif isinstance(server, list): - proc = _ServerProcess(map(str, server)) + proc = subprocess.Popen(map(str, server), **popen_kwargs) else: raise click.ClickException("server: must be a string or list") return proc, server @@ -1816,7 +1799,17 @@ def _cleanup_servers(server_processes, leave_server): ) else: for process, _ in server_processes: - process.kill() + _stop_server(process) + + +def _stop_server(process): + if os.name == "posix": + try: + os.killpg(process.pid, signal.SIGKILL) + except ProcessLookupError: + pass + else: + process.kill() def _record_storyboard( diff --git a/tests/test_shot_scraper.py b/tests/test_shot_scraper.py index c10e663..57b6c57 100644 --- a/tests/test_shot_scraper.py +++ b/tests/test_shot_scraper.py @@ -908,10 +908,6 @@ def __enter__(self): def __exit__(self, exc_type, exc, tb): pass - class FakeServerProcess: - def kill(self): - events.append("server.kill") - fake_page = FakePage() fake_context = FakeContext(fake_page) fake_browser = FakeBrowser() @@ -935,9 +931,14 @@ def kill(self): cli_module, "_start_server", side_effect=lambda server: ( - events.append(("server", server)) or (FakeServerProcess(), server) + events.append(("server", server)) or (object(), server) ), ) + mocker.patch.object( + cli_module, + "_stop_server", + side_effect=lambda process: events.append("server.kill"), + ) mocker.patch.object( cli_module.time, "sleep", @@ -1024,9 +1025,6 @@ def __exit__(self, exc_type, exc, tb): pass class FakeServerProcess: - def kill(self): - events.append("server.kill") - def poll(self): return None @@ -1044,6 +1042,11 @@ def poll(self): events.append(("server", server)) or (FakeServerProcess(), server) ), ) + mocker.patch.object( + cli_module, + "_stop_server", + side_effect=lambda process: events.append("server.kill"), + ) mocker.patch.object( cli_module.time, "sleep", From 3ef3250ce4e51bccf16586b63b2debc923cb0631 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 21:23:38 +0000 Subject: [PATCH 3/4] multi: don't start servers or browser when there is nothing to do If --no-clobber or --output rule out every shot, shot-scraper multi now exits before starting any server: processes or launching the browser. This makes "multi shots.yml -n" a cheap way to create just the missing screenshots, even when the file defines a server. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VL4haavg5i8JXVFFrPU8p3 --- docs/multi.md | 2 +- shot_scraper/cli.py | 25 ++++++++++++++++++------- tests/test_shot_scraper.py | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/docs/multi.md b/docs/multi.md index 091663d..e4be595 100644 --- a/docs/multi.md +++ b/docs/multi.md @@ -20,7 +20,7 @@ Use `-` to pass in YAML from standard input: ```bash echo "- url: http://www.example.com" | shot-scraper multi - ``` -If you run the tool with the `-n` or `--no-clobber` option any shots where the output file aleady exists will be skipped. +If you run the tool with the `-n` or `--no-clobber` option any shots where the output file already exists will be skipped. If every shot is skipped, `shot-scraper` exits without starting any `server:` processes or launching the browser - so this is a cheap way to create just the screenshots that are missing. You can specify a subset of screenshots to take by specifying output files that you would like to create. For example, to take just the shots of `one.png` and `three.png` that are defined in `shots.yml` run this: ```bash diff --git a/shot_scraper/cli.py b/shot_scraper/cli.py index 359fd39..88f3869 100644 --- a/shot_scraper/cli.py +++ b/shot_scraper/cli.py @@ -879,6 +879,14 @@ def multi( shots = [] if not isinstance(shots, list): raise click.ClickException("YAML file must contain a list") + # Nothing to do, for example because --no-clobber skipped every shot, + # so there is no need to start any servers or launch the browser + if not any( + shot.get("sh") or shot.get("python") or "url" in shot + for shot in shots + if not _skipped_by_options(shot, noclobber, outputs) + ): + return with sync_playwright() as p: context, browser_obj = _browser_context( p, @@ -895,13 +903,7 @@ def multi( ) try: for shot in shots: - if ( - noclobber - and shot.get("output") - and pathlib.Path(shot["output"]).exists() - ): - continue - if outputs and shot.get("output") and shot.get("output") not in outputs: + if _skipped_by_options(shot, noclobber, outputs): continue # Run "sh" key if shot.get("sh"): @@ -1743,6 +1745,15 @@ def _run_python_code(code): ) from ex +def _skipped_by_options(shot, noclobber, outputs): + output = shot.get("output") + if not output: + return False + if noclobber and pathlib.Path(output).exists(): + return True + return bool(outputs) and output not in outputs + + def _start_server(server): # Run the server in its own process group so _stop_server() can stop # any child processes too, such as the command started by "sh -c" diff --git a/tests/test_shot_scraper.py b/tests/test_shot_scraper.py index 57b6c57..6c639b0 100644 --- a/tests/test_shot_scraper.py +++ b/tests/test_shot_scraper.py @@ -90,6 +90,42 @@ def test_multi_server_string_child_process_is_stopped(): assert not _port_is_open(port) +SKIPPED_SERVER_YAML = """ +- server: python -m http.server 9024 +- url: http://localhost:9024/ + output: output.png +""".strip() + + +@pytest.mark.parametrize("options", (["-n"], ["-o", "other.png"])) +def test_multi_nothing_to_do_skips_server_and_browser(mocker, options): + start_server = mocker.patch.object(cli_module, "_start_server") + sync_playwright = mocker.patch.object(cli_module, "sync_playwright") + runner = CliRunner() + with runner.isolated_filesystem(): + open("output.png", "w").write("existing") + open("shots.yml", "w").write(SKIPPED_SERVER_YAML) + result = runner.invoke(cli, ["multi", "shots.yml", *options]) + assert result.exit_code == 0, result.output + assert open("output.png").read() == "existing" + start_server.assert_not_called() + sync_playwright.assert_not_called() + + +def test_multi_no_clobber_starts_server_for_missing_shot(): + runner = CliRunner() + with runner.isolated_filesystem(): + open("existing.png", "w").write("existing") + open("shots.yml", "w").write( + SKIPPED_SERVER_YAML + + "\n- url: http://localhost:9024/\n output: existing.png\n" + ) + result = runner.invoke(cli, ["multi", "shots.yml", "-n"]) + assert result.exit_code == 0, result.output + assert pathlib.Path("output.png").exists() + assert open("existing.png").read() == "existing" + + def test_multi_server_slow_start(): # https://github.com/simonw/shot-scraper/issues/197 # A server that takes longer than one second to start listening From 5c135922027b80db290cb5cd0933a76065ec21c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 21:26:13 +0000 Subject: [PATCH 4/4] Trim --no-clobber docs sentence Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VL4haavg5i8JXVFFrPU8p3 --- docs/multi.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/multi.md b/docs/multi.md index e4be595..bac7e7d 100644 --- a/docs/multi.md +++ b/docs/multi.md @@ -20,7 +20,7 @@ Use `-` to pass in YAML from standard input: ```bash echo "- url: http://www.example.com" | shot-scraper multi - ``` -If you run the tool with the `-n` or `--no-clobber` option any shots where the output file already exists will be skipped. If every shot is skipped, `shot-scraper` exits without starting any `server:` processes or launching the browser - so this is a cheap way to create just the screenshots that are missing. +If you run the tool with the `-n` or `--no-clobber` option any shots where the output file already exists will be skipped. If every shot is skipped, `shot-scraper` exits without starting any `server:` processes. You can specify a subset of screenshots to take by specifying output files that you would like to create. For example, to take just the shots of `one.png` and `three.png` that are defined in `shots.yml` run this: ```bash