From c0321373e629d1de91769fadbcf9d3db5dd9d5a9 Mon Sep 17 00:00:00 2001 From: Chandran Date: Sat, 25 Jul 2026 23:51:26 +0200 Subject: [PATCH] fix(inference): close the log file if inference fails to start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handle_start_inference opens the rollout log file before spawning the subprocess, and the stdout pump thread only takes ownership of that handle once it starts. If anything in between raised — most likely Popen itself — the except block released the inference slot and returned, but never closed the log file, leaking the handle. On Windows that also keeps the log file locked. Track the handle from before the try, hand ownership to the pump thread by clearing it once the thread starts, and close it on the failure path when it is still ours. --- lelab/rollout.py | 12 +++++++++++- tests/test_rollout.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/lelab/rollout.py b/lelab/rollout.py index cfff79a8..0b34f31e 100644 --- a/lelab/rollout.py +++ b/lelab/rollout.py @@ -266,6 +266,11 @@ def handle_start_inference(request: InferenceRequest) -> dict[str, Any]: # Claim the slot now so a concurrent caller losing the race sees us. inference_active = True + # Opened partway through setup; the stdout pump thread takes ownership once + # it starts. Tracked here so a failure before that hand-off can close it + # instead of leaking the file handle (which on Windows also keeps the log + # locked). + log_handle = None try: # `setup_follower_calibration_file` returns the basename without the # .json extension. We need that stripped form for `--robot.id`, @@ -323,9 +328,14 @@ def handle_start_inference(request: InferenceRequest) -> dict[str, Any]: name="inference-stdout-pump", daemon=True, ).start() + log_handle = None # pump thread owns and closes it from here on except Exception as exc: logger.exception("Failed to start inference") - # Subprocess never started — release the slot. + # Startup failed before the pump thread took over (most often Popen + # itself). Release the slot and close the log file if we opened it before + # failing, so the handle isn't leaked. + if log_handle is not None: + log_handle.close() with _state_lock: inference_active = False return {"success": False, "status_code": 500, "message": f"Failed to start inference: {exc}"} diff --git a/tests/test_rollout.py b/tests/test_rollout.py index c0bb1f3d..5e445b70 100644 --- a/tests/test_rollout.py +++ b/tests/test_rollout.py @@ -263,6 +263,38 @@ def test_handle_start_inference_blocked_when_already_active(monkeypatch) -> None assert "already active" in result["message"] +def test_handle_start_inference_closes_log_when_popen_fails(monkeypatch, tmp_path) -> None: + """If Popen fails after the log file is opened, the handle must be closed, + not leaked — on Windows a leaked handle also keeps the log file locked.""" + from lelab import rollout + + monkeypatch.setattr(rollout, "setup_follower_calibration_file", lambda cfg: "robot_a") + monkeypatch.setattr(rollout, "_resolve_policy_path", lambda ref: str(tmp_path / "model")) + + opened: list = [] + real_open = rollout.Path.open + + def tracking_open(self, *args, **kwargs): + handle = real_open(self, *args, **kwargs) + opened.append(handle) + return handle + + monkeypatch.setattr(rollout.Path, "open", tracking_open) + + def boom(*args, **kwargs): + raise OSError("simulated Popen failure") + + monkeypatch.setattr(rollout.subprocess, "Popen", boom) + + result = rollout.handle_start_inference(_stub_request()) + + assert result["success"] is False + assert result["status_code"] == 500 + assert opened, "expected the inference log file to have been opened" + assert all(handle.closed for handle in opened), "log handle leaked on Popen failure" + assert rollout.inference_active is False + + def test_classify_outcome_ok_warns_and_fails() -> None: from lelab.rollout import _classify_outcome