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"