From c4e87675297c6a7bcd0c0915d888384c53e0a2c6 Mon Sep 17 00:00:00 2001 From: florian-simvia Date: Fri, 11 Sep 2026 15:55:14 +0200 Subject: [PATCH] fix: keep the FAILED status of a launch that never started _launch_local and _launch_slurm record status=FAILED and then raise to report the failure, but registry_transaction only saved when the body completed normally, so that write was discarded and the case stayed PENDING forever with no way to tell it never ran. The save now runs in a finally. Audited every call site: the other nine either raise before writing anything, or write once with nothing that can raise, or (cleanup, kill) record discoveries that are correct to keep. generate_cases gains from it: the case directories it already created on disk were never rolled back either, so discarding their registry entries only left the two out of step. --- CHANGELOG.md | 1 + csauto/registry.py | 15 ++++++++++++--- tests/runner/test_runner.py | 29 +++++++++++++++++++++++++++++ tests/unit/test_registry.py | 25 +++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d891c38..cf49917 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Make the dashboard follow what each solver can actually do: panels and action bu - `csauto doctor` reports the panels and capabilities derived for the configured solver ### Fixed +- A case whose launch failed outright (no docker or `sbatch` on PATH, an unreadable binary, a rejected `sbatch` submission) stayed `PENDING` forever. `_launch_local` and `_launch_slurm` record `status=FAILED` and then raise to report the failure, but `registry_transaction` skipped its save whenever the body raised, so that write was discarded. The save now runs in a `finally`, which also stops `csauto prepare` from discarding the registry entries of cases it already created on disk when it aborts part way - A case launched from the web UI stayed `RUNNING` forever when the run crashed before the solver started (a missing mesh, say). Two causes, both fixed: `is_process_alive` reported a zombie as alive, because `os.kill(pid, 0)` succeeds on a process that exited but was never reaped, which is what every run launched by the long-lived server becomes; and `detect_run_outcome` only read `run_solver.log` and `listing`, so a failure that produces neither went undetected even though code_saturne writes an explicit `run_status.failed` marker next to them. Zombies are now reported as dead (and reaped when we are the parent), and the status markers are read when the logs give no verdict - Opening the solver GUI on a case whose shared dirs are symlinks (the default since `mesh_mode = "symlink"` became the default in 0.4.1) left those symlinks dangling inside the container: `build_gui_command` (docker) and the singularity branch of `build_runtime_gui_command` mounted only the runs dir, unlike their `run` counterparts which also bind the symlink targets. Both now bind them the same way, `MESH` read-only and `POST` writable - Requesting a restart on a solver without restart support returned HTTP 500 "Launch error", a client error reported as a server fault; it now returns HTTP 400 naming the solver diff --git a/csauto/registry.py b/csauto/registry.py index e66e39f..85e7442 100644 --- a/csauto/registry.py +++ b/csauto/registry.py @@ -116,11 +116,20 @@ def mutate_registry( @contextmanager def registry_transaction(runs_dir: Path) -> Iterable[dict[str, dict[str, Any]]]: - """Load/update/save registry.json under a single lock.""" + """Load/update/save registry.json under a single lock. + + The save also runs when the body raises. Callers that record an outcome and + then raise (a launch that writes status=FAILED before reporting the failure, + for one) would otherwise lose that write and leave the case in its previous + state forever. Discarding it buys no atomicity anyway, since the filesystem + side effects of those same bodies are not rolled back either. + """ with registry_lock(runs_dir): registry = _load_registry_unlocked(runs_dir) - yield registry - _save_registry_unlocked(runs_dir, registry) + try: + yield registry + finally: + _save_registry_unlocked(runs_dir, registry) def update_case(registry: dict[str, dict[str, Any]], case_id: str, **updates: Any) -> None: diff --git a/tests/runner/test_runner.py b/tests/runner/test_runner.py index f579268..686e0db 100644 --- a/tests/runner/test_runner.py +++ b/tests/runner/test_runner.py @@ -1057,3 +1057,32 @@ def test_is_process_alive_reports_a_live_process_as_alive() -> None: from csauto.runner import is_process_alive assert is_process_alive(os.getpid()) is True + + +def test_failed_launch_leaves_the_case_failed_not_pending(monkeypatch, runs_dir: Path, case_factory) -> None: + """A case whose launch never started must not stay PENDING forever. + + _launch_local writes status=FAILED and then raises; that write used to be + discarded, leaving the case PENDING with no way to tell it never ran. + """ + case_factory(runs_dir, "case0001") + + def popen_stub(*_args, **_kwargs): + raise OSError("docker not found") + + monkeypatch.setattr("subprocess.Popen", popen_stub) + monkeypatch.setattr("shutil.which", lambda _name: "/bin/true") + + with pytest.raises(RuntimeError, match="Failed to launch case0001"): + run_cases( + runs_dir, + nprocs=1, + nt=1, + max_parallel=1, + case_filter=["case0001"], + docker_image="image", + resume_only_failed=False, + source="test", + ) + + assert load_registry(runs_dir)["case0001"]["status"] == STATUS_FAILED diff --git a/tests/unit/test_registry.py b/tests/unit/test_registry.py index da33fc9..42b12c0 100644 --- a/tests/unit/test_registry.py +++ b/tests/unit/test_registry.py @@ -2,6 +2,8 @@ from pathlib import Path +import pytest + from csauto.registry import append_history, load_registry, read_history, registry_transaction, update_case @@ -82,3 +84,26 @@ def test_registry_transaction_keeps_valid_json(tmp_path: Path) -> None: pass registry = load_registry(runs_dir) assert "case0001" in registry + + +def test_registry_transaction_persists_writes_made_before_an_exception(runs_dir: Path) -> None: + """A launch that fails writes status=FAILED and then raises; that write must survive. + + Filesystem side effects are not rolled back either, so discarding the registry + write buys no atomicity, only an inconsistent state. + """ + with registry_transaction(runs_dir) as registry: + update_case(registry, "case0001", status="PENDING") + + with pytest.raises(RuntimeError, match="launch failed"), registry_transaction(runs_dir) as registry: + update_case(registry, "case0001", status="FAILED") + raise RuntimeError("launch failed") + + assert load_registry(runs_dir)["case0001"]["status"] == "FAILED" + + +def test_registry_transaction_saves_normally_without_an_exception(runs_dir: Path) -> None: + with registry_transaction(runs_dir) as registry: + update_case(registry, "case0001", status="DONE") + + assert load_registry(runs_dir)["case0001"]["status"] == "DONE"