diff --git a/module/scripts/clawde-service/sidecar_process_reconcile.py b/module/scripts/clawde-service/sidecar_process_reconcile.py index 82d0fae..27fad0b 100644 --- a/module/scripts/clawde-service/sidecar_process_reconcile.py +++ b/module/scripts/clawde-service/sidecar_process_reconcile.py @@ -2,6 +2,11 @@ import pathlib import signal import subprocess +import time + +SPAWNED_COMMAND_RECORD_SUFFIX = ".spawned-command" +TERMINATION_DEADLINE_SECONDS = 5.0 +TERMINATION_POLL_SECONDS = 0.1 def find_sidecar_process_ids(process_match_pattern: str) -> list[int]: @@ -20,6 +25,66 @@ def terminate_sidecar_process(process_id: int) -> None: pass +def process_is_alive(process_id: int) -> bool: + try: + os.kill(process_id, 0) + except ProcessLookupError: + return False + return True + + +def termination_poll_attempts() -> int: + return int(TERMINATION_DEADLINE_SECONDS / TERMINATION_POLL_SECONDS) + + +def wait_for_process_to_exit(process_id: int) -> None: + for _attempt in range(termination_poll_attempts()): + if not process_is_alive(process_id): + return + time.sleep(TERMINATION_POLL_SECONDS) + + +def spawned_command_record_for(sidecar_specification: dict) -> pathlib.Path: + return pathlib.Path( + sidecar_specification["log_file"] + SPAWNED_COMMAND_RECORD_SUFFIX + ) + + +def recorded_spawned_command(sidecar_specification: dict) -> str | None: + record_file = spawned_command_record_for(sidecar_specification) + try: + return record_file.read_text() + except OSError: + return None + + +def record_spawned_command(sidecar_specification: dict) -> None: + record_file = spawned_command_record_for(sidecar_specification) + record_file.parent.mkdir(parents=True, exist_ok=True) + record_file.write_text(sidecar_specification["command"]) + + +def live_processes_run_the_current_command(sidecar_specification: dict) -> bool: + return ( + recorded_spawned_command(sidecar_specification) + == (sidecar_specification["command"]) + ) + + +def replace_processes_running_superseded_code( + sidecar_specification: dict, live_process_ids: list[int] +) -> list[int]: + if not live_process_ids or live_processes_run_the_current_command( + sidecar_specification + ): + return live_process_ids + for process_id in live_process_ids: + terminate_sidecar_process(process_id) + for process_id in live_process_ids: + wait_for_process_to_exit(process_id) + return [] + + def open_sidecar_log_file(log_file_path: str): log_file = pathlib.Path(log_file_path) log_file.parent.mkdir(parents=True, exist_ok=True) @@ -31,6 +96,7 @@ def detach_from_supervisor(command: str) -> str: def spawn_sidecar_process(sidecar_specification: dict) -> None: + record_spawned_command(sidecar_specification) with open_sidecar_log_file(sidecar_specification["log_file"]) as log_file: subprocess.run( ["sh", "-c", detach_from_supervisor(sidecar_specification["command"])], @@ -51,6 +117,9 @@ def reconcile_one_sidecar_process( for process_id in live_process_ids: terminate_sidecar_process(process_id) return False + live_process_ids = replace_processes_running_superseded_code( + sidecar_specification, live_process_ids + ) for duplicate_process_id in live_process_ids[1:]: terminate_sidecar_process(duplicate_process_id) if live_process_ids: diff --git a/module/scripts/tests/unit/sidecar_process_test_support.py b/module/scripts/tests/unit/sidecar_process_test_support.py index dbfefaf..41ab577 100644 --- a/module/scripts/tests/unit/sidecar_process_test_support.py +++ b/module/scripts/tests/unit/sidecar_process_test_support.py @@ -23,6 +23,11 @@ def make_sidecar_specification_with_lifetime(tmp_path, lifetime, enabled=True): } +def record_the_sidecar_as_launched_from_its_current_command(specification): + sidecar_process_reconcile.record_spawned_command(specification) + return specification + + def make_session_specification(tmp_path): return { "name": "clawde", @@ -54,4 +59,9 @@ def record_process_lookups(monkeypatch, live_process_ids): "terminate_sidecar_process", terminated_process_ids.append, ) + monkeypatch.setattr( + sidecar_process_reconcile, + "wait_for_process_to_exit", + lambda _process_id: None, + ) return spawned_specifications, terminated_process_ids diff --git a/module/scripts/tests/unit/test_clawde_service_sidecar_process_reconcile.py b/module/scripts/tests/unit/test_clawde_service_sidecar_process_reconcile.py index 878edf6..81444f3 100644 --- a/module/scripts/tests/unit/test_clawde_service_sidecar_process_reconcile.py +++ b/module/scripts/tests/unit/test_clawde_service_sidecar_process_reconcile.py @@ -14,6 +14,7 @@ make_sidecar_specification_with_lifetime, make_session_specification, record_process_lookups, + record_the_sidecar_as_launched_from_its_current_command, ) @@ -35,7 +36,10 @@ def test_a_live_sidecar_is_never_relaunched(tmp_path, monkeypatch): ) sidecar_process_reconcile.reconcile_one_sidecar_process( - make_sidecar_specification(tmp_path), True + record_the_sidecar_as_launched_from_its_current_command( + make_sidecar_specification(tmp_path) + ), + True, ) assert spawned_specifications == [] @@ -50,7 +54,10 @@ def test_duplicate_sidecar_processes_are_culled_down_to_the_oldest( ) sidecar_process_reconcile.reconcile_one_sidecar_process( - make_sidecar_specification(tmp_path), True + record_the_sidecar_as_launched_from_its_current_command( + make_sidecar_specification(tmp_path) + ), + True, ) assert terminated_process_ids == [450, 700] @@ -175,7 +182,9 @@ def test_a_service_lifetime_sidecar_survives_its_agents_dormancy(tmp_path, monke spawned_specifications, terminated_process_ids = record_process_lookups( monkeypatch, [4321] ) - specification = make_sidecar_specification_with_lifetime(tmp_path, "service") + specification = record_the_sidecar_as_launched_from_its_current_command( + make_sidecar_specification_with_lifetime(tmp_path, "service") + ) sidecar_process_reconcile.reconcile_one_sidecar_process( specification, diff --git a/module/scripts/tests/unit/test_clawde_service_sidecar_process_supersession.py b/module/scripts/tests/unit/test_clawde_service_sidecar_process_supersession.py new file mode 100644 index 0000000..4f48d8e --- /dev/null +++ b/module/scripts/tests/unit/test_clawde_service_sidecar_process_supersession.py @@ -0,0 +1,147 @@ +import pathlib +import sys + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent)) +sys.path.insert( + 0, str(pathlib.Path(__file__).resolve().parent.parent.parent / "clawde-service") +) + +import sidecar_process_reconcile +from sidecar_process_test_support import ( + SIDECAR_NAME, + make_sidecar_specification, + record_process_lookups, + record_the_sidecar_as_launched_from_its_current_command, +) + + +def reconcile_a_running_sidecar(specification, monkeypatch, live_process_ids=(4321,)): + spawned_specifications, terminated_process_ids = record_process_lookups( + monkeypatch, list(live_process_ids) + ) + sidecar_process_reconcile.reconcile_one_sidecar_process(specification, True) + return spawned_specifications, terminated_process_ids + + +def test_a_sidecar_still_running_superseded_code_is_replaced(tmp_path, monkeypatch): + specification = record_the_sidecar_as_launched_from_its_current_command( + make_sidecar_specification(tmp_path, command="python3 /nix/store/old-bridge.py") + ) + specification["command"] = "python3 /nix/store/new-bridge.py" + + spawned_specifications, terminated_process_ids = reconcile_a_running_sidecar( + specification, monkeypatch + ) + + assert terminated_process_ids == [4321] + assert [specification["name"] for specification in spawned_specifications] == [ + SIDECAR_NAME + ] + + +def test_every_process_of_a_superseded_sidecar_is_terminated_before_the_replacement( + tmp_path, monkeypatch +): + waited_for_process_ids = [] + specification = record_the_sidecar_as_launched_from_its_current_command( + make_sidecar_specification(tmp_path, command="python3 /nix/store/old-bridge.py") + ) + specification["command"] = "python3 /nix/store/new-bridge.py" + spawned_specifications, terminated_process_ids = record_process_lookups( + monkeypatch, [700, 120, 450] + ) + monkeypatch.setattr( + sidecar_process_reconcile, + "wait_for_process_to_exit", + waited_for_process_ids.append, + ) + + sidecar_process_reconcile.reconcile_one_sidecar_process(specification, True) + + assert terminated_process_ids == [120, 450, 700] + assert waited_for_process_ids == [120, 450, 700] + assert len(spawned_specifications) == 1 + + +def test_a_sidecar_running_the_current_code_is_left_alone(tmp_path, monkeypatch): + specification = record_the_sidecar_as_launched_from_its_current_command( + make_sidecar_specification(tmp_path, command="python3 /nix/store/bridge.py") + ) + + spawned_specifications, terminated_process_ids = reconcile_a_running_sidecar( + specification, monkeypatch + ) + + assert spawned_specifications == [] + assert terminated_process_ids == [] + + +def test_a_sidecar_launched_before_this_record_existed_is_replaced_exactly_once( + tmp_path, monkeypatch +): + specification = make_sidecar_specification(tmp_path, command="true") + + _, first_terminated_process_ids = reconcile_a_running_sidecar( + specification, monkeypatch + ) + sidecar_process_reconcile.record_spawned_command(specification) + _, second_terminated_process_ids = reconcile_a_running_sidecar( + specification, monkeypatch + ) + + assert first_terminated_process_ids == [4321] + assert second_terminated_process_ids == [] + + +def test_spawning_records_the_command_it_launched(tmp_path): + specification = make_sidecar_specification(tmp_path, command="true") + + sidecar_process_reconcile.spawn_sidecar_process(specification) + + assert sidecar_process_reconcile.recorded_spawned_command(specification) == "true" + + +def test_a_superseded_sidecar_that_should_stop_is_terminated_without_replacement( + tmp_path, monkeypatch +): + specification = record_the_sidecar_as_launched_from_its_current_command( + make_sidecar_specification(tmp_path, command="python3 /nix/store/old-bridge.py") + ) + specification["command"] = "python3 /nix/store/new-bridge.py" + spawned_specifications, terminated_process_ids = record_process_lookups( + monkeypatch, [4321] + ) + + sidecar_process_reconcile.reconcile_one_sidecar_process(specification, False) + + assert terminated_process_ids == [4321] + assert spawned_specifications == [] + + +def test_waiting_returns_as_soon_as_the_process_is_gone(monkeypatch): + monkeypatch.setattr( + sidecar_process_reconcile, "process_is_alive", lambda _process_id: False + ) + monkeypatch.setattr( + sidecar_process_reconcile, + "time", + type("NeverSleeps", (), {"sleep": staticmethod(lambda _seconds: None)}), + ) + + sidecar_process_reconcile.wait_for_process_to_exit(4321) + + +def test_waiting_gives_up_on_a_process_that_refuses_to_exit(monkeypatch): + slept_seconds = [] + monkeypatch.setattr( + sidecar_process_reconcile, "process_is_alive", lambda _process_id: True + ) + monkeypatch.setattr( + sidecar_process_reconcile, + "time", + type("CountsSleeps", (), {"sleep": staticmethod(slept_seconds.append)}), + ) + + sidecar_process_reconcile.wait_for_process_to_exit(4321) + + assert len(slept_seconds) == sidecar_process_reconcile.termination_poll_attempts()