fix(timeout): restore v1 precedence, unify default, fix distributed timeout resolution

CHANGELOG.md

@@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **`--timeout` precedence restored to the v1 rule; default unified at 7200s** (**behavior change**): The v2 rewrite spread timeout handling across five layers, and the sentinel was materialized into a concrete value at the CLI before the resolver ever saw it. Because the resolver then detected "the user did not pass `--timeout`" by comparing the value against the default, an explicit `--timeout 7200` was indistinguishable from no flag at all and silently lost to a model card `timeout` — in v1 it won. The CLI now forwards the user's value verbatim and precedence is applied in one place: default < model card < explicit `--timeout`, where `-1` means "not specified" and falls through, `0` means "no timeout" and beats the levels below it. Separately, the distributed path defaulted to 3600s while local execution used 7200s and `docs/cli-reference.md` documented 7200 as *the* default; SLURM runs now get the same 2h default as local runs. (Kubernetes threads the value onto `DeploymentConfig` but no manifest template reads it, so nothing is enforced there either before or after.) Resolution moved from `execution/container_runner_helpers.py` to `core/timeout.py` (re-exported from its old home) so the deployment layer shares one rule instead of reimplementing it, and `build_orchestrator`/`run_orchestrator` now agree that a manifest `timeout` of `null` means "the model card specified none".
+
+  **Upgrade note:** a `build_manifest.json` written by 2.1.x or earlier stored `-1` as filler for every model *without* a `timeout` key (`model.get("timeout", -1)`). Since a model card's `-1` is now a real value meaning "no timeout", replaying such a manifest runs those models unbounded rather than at the 7200s default. Rebuild the manifest, or pass an explicit `--timeout`, when reusing one written before this change.
+
+### Fixed
+
+- **`--timeout 0` crashed instead of disabling the timeout**: The CLI mapped `0` to `None`, and three consumers were unprepared for it. `execution/container_runner.py` and `deployment/slurm.py` guarded with a bare `timeout > 0`, raising `TypeError: '>' not supported between instances of 'NoneType' and 'int'` on the self-managed-launcher and in-allocation paths. The SLURM job template used `{{ timeout | default(3600) }}`, but Jinja's `default` filter only substitutes for *undefined* values, so `None` rendered the literal string `--timeout None` into the generated script, which Typer then rejected. Only an `int` crosses layer boundaries now, and the sentinel is mapped to `subprocess`/`communicate` semantics by a single named guard, `core.timeout.subprocess_timeout()` — needed because `subprocess` reads `timeout=0` as "expire immediately", not "no timeout". A fourth site with the same latent bug (the model script invocation in `container_runner.py`, which reaches `communicate()`) was fixed at the same time. Regression tests added for each.
+
+- **SLURM/K8s runs lost their wall-clock timeout cap by default**: `_execute_distributed` forwarded the CLI's `-1` sentinel straight into `DeploymentConfig.timeout` instead of resolving it first, unlike the local path (which resolves in `container_runner.py`). `subprocess_timeout(-1)` maps to `None`, so the SLURM in-allocation path (`slurm.py`'s `_run_inside_existing_allocation`) ran with no timeout at all on any run that didn't pass an explicit `--timeout`. `_execute_distributed` now calls `resolve_run_timeout()` before building `DeploymentConfig`, restoring the 7200s default cap. The generated SLURM job script's `--timeout` argument (`job.sh.j2`) no longer uses `{{ timeout | default(3600) }}`, which only ever caught `undefined`, not `None`.
+
+- **A model card's `timeout` was ignored on SLURM**: SLURM resolves the timeout twice — once in `_execute_distributed`, and again inside the job, where the generated script re-invokes `madengine run`. Rendering the *resolved* value into that script made the inner run see an explicit `--timeout`, which correctly outranks the model card, so a model declaring `"timeout": 3600` silently ran under the 7200s default instead. (Before the precedence fix above this was masked: the rendered `7200` happened to equal the constant the old resolver compared against, so it read as "no flag passed".) `DeploymentConfig` now carries the two values separately — `timeout`, the resolved cap on madengine's own wait, and `cli_timeout`, the sentinel forwarded verbatim for the in-job run to resolve against the model card itself. Local runs were never affected.
+
 ## [2.1.3] - 2026-07-15
 
 ### Added

docs/configuration.md

@@ -335,7 +335,8 @@ Format: Comma-separated list with hyphen ranges.
 
 ### Timeout Settings
 
-Set a per-model timeout (seconds) in `models.json`:
+Set a per-model timeout (seconds) in `models.json`. Use `0` (or any
+non-positive value) to run the model without a timeout:
 
 ```json
 {

docs/usage.md

@@ -446,6 +446,15 @@ madengine run --tags model --timeout 7200
 madengine run --tags model --timeout 0
 ```
 
+Precedence, lowest to highest: the 7200s default, then a model card's `timeout`
+field, then `--timeout`. `--timeout -1` (the default) means "not passed" and
+falls through to the level below, so an explicit `--timeout 7200` still
+overrides a model card timeout even though it equals the default. A resolved
+timeout of `0` or less means no timeout — including a model card that sets
+`"timeout": 0` or `-1`. The same default and precedence apply to SLURM runs:
+the submitting process caps its own wait at the resolved timeout, and forwards
+`--timeout` unresolved to the job, so a model card's value still wins there.
+
 ### Debugging
 
 ```bash

src/madengine/cli/commands/run.py

@@ -24,6 +24,7 @@
     ConfigurationError,
     ExecutionError,
 )
+from madengine.core.timeout import DEFAULT_RUN_TIMEOUT
 
 from ..constants import (
     ExitCode,
@@ -188,14 +189,14 @@ def run(
         effective_additional_context = repr(merged)
         effective_additional_context_file = None
 
-    # Convert -1 (default) to actual default timeout value (7200 seconds = 2 hours)
-    if timeout == -1:
-        timeout = 7200
-    # 0 means "no timeout" per the help text — map to None so subprocess never expires
-    elif timeout == 0:
-        timeout = None
-
-    timeout_display = "disabled" if timeout is None else f"{timeout}s"
+    # The sentinel is passed through untouched (-1 unspecified, 0 no timeout);
+    # resolve_run_timeout applies precedence against the model card downstream.
+    if timeout == 0:
+        timeout_display = "disabled"
+    elif timeout == -1:
+        timeout_display = f"{DEFAULT_RUN_TIMEOUT}s (default)"
+    else:
+        timeout_display = f"{timeout}s"
 
     try:
         # Check if we're doing execution-only or full workflow

src/madengine/core/timeout.py

@@ -1,14 +1,79 @@
 #!/usr/bin/env python3
-"""Module to define the Timeout class.
+"""Module to define the Timeout class and run-timeout resolution.
 
-This module provides the Timeout class to handle timeouts.
+This module provides the Timeout class to handle timeouts, plus the single
+definition of how a run timeout is resolved and how it maps onto subprocess
+semantics.
+
+Resolution follows madengine v1: the default is overridden by the model card,
+which is overridden by an explicit ``--timeout``. Only the CLI has a sentinel,
+``-1``, meaning "not passed". Any non-positive resolved timeout runs unbounded,
+which both consumers below already implement.
 
 Copyright (c) Advanced Micro Devices, Inc. All rights reserved.
 """
 # built-in modules
 import signal
+import typing
 from typing import Optional
 
+# Default run timeout (2 hours). Single source of truth for local and
+# distributed execution alike.
+DEFAULT_RUN_TIMEOUT = 7200
+
+
+def resolve_run_timeout(
+    model_info: typing.Dict,
+    cli_timeout: typing.Optional[int],
+    default_timeout: int = DEFAULT_RUN_TIMEOUT,
+) -> int:
+    """Resolve the effective run timeout.
+
+    Precedence, lowest to highest: default < model card < CLI. A model card's
+    ``timeout`` is taken as-is, including a non-positive one, which means the
+    author asked for no timeout. A CLI timeout of -1 means ``--timeout`` was
+    not passed and falls through to the level below.
+
+    ``None`` in the model card is ignored so that manifests written by older
+    builds (which store ``null`` for an absent timeout) still load.
+
+    Args:
+        model_info: Model info dict; may have a "timeout" key.
+        cli_timeout: Timeout from the CLI; -1 if not passed.
+        default_timeout: Value used when neither level specifies one.
+
+    Returns:
+        int: Effective timeout in seconds; non-positive means no timeout.
+    """
+    timeout = default_timeout
+
+    model_timeout = model_info.get("timeout")
+    if model_timeout is not None:
+        timeout = model_timeout
+
+    if cli_timeout is not None and cli_timeout >= 0:
+        timeout = cli_timeout
+
+    return timeout
+
+
+def subprocess_timeout(timeout: typing.Optional[int]) -> Optional[int]:
+    """Map a resolved timeout onto ``subprocess``/``communicate`` semantics.
+
+    ``subprocess`` treats ``timeout=0`` as "expire immediately", not as "no
+    timeout", so a non-positive timeout cannot be passed through directly and
+    becomes ``None`` instead.
+
+    Args:
+        timeout: Resolved timeout in seconds; non-positive means no timeout.
+
+    Returns:
+        Optional[int]: Seconds to pass to subprocess, or None for no timeout.
+    """
+    if timeout is None or timeout <= 0:
+        return None
+    return timeout
+
 
 class Timeout:
     """Class to handle timeouts.

src/madengine/deployment/base.py

@@ -19,6 +19,8 @@
 from jinja2 import Environment, FileSystemLoader
 from rich.console import Console
 
+from madengine.core.timeout import DEFAULT_RUN_TIMEOUT
+
 
 # Regex for parsing "performance: <value> <metric>" log lines.
 # Value: optional sign, integer/decimal, scientific notation (e or E).
@@ -67,7 +69,15 @@ class DeploymentConfig:
     target: str  # "slurm", "k8s" (NOT "local" - that uses container_runner)
     manifest_file: str
     additional_context: Dict[str, Any] = field(default_factory=dict)
-    timeout: int = 3600
+    timeout: int = DEFAULT_RUN_TIMEOUT
+    # The CLI's --timeout verbatim (-1 unspecified, 0 no timeout), for the
+    # generated job script to forward to the madengine it re-invokes. Distinct
+    # from `timeout` above, which is this process's own wall-clock cap and must
+    # already be resolved. Resolving both alike would flatten the sentinel into
+    # a concrete value, which the inner CLI cannot distinguish from an explicit
+    # --timeout and would therefore rank above the model card. Defaults to the
+    # sentinel so a config built without it forwards "unspecified".
+    cli_timeout: int = -1
     monitor: bool = True
     cleanup_on_failure: bool = True
 

src/madengine/deployment/slurm.py

@@ -29,6 +29,7 @@
 )
 from .config_loader import ConfigLoader, apply_deployment_config
 from .slurm_node_selector import SlurmNodeSelector
+from madengine.core.timeout import subprocess_timeout
 from madengine.utils.gpu_config import resolve_runtime_gpus
 from madengine.utils.run_details import get_build_number, get_pipeline
 from madengine.utils.path_utils import scripts_base_dir_from
@@ -712,7 +713,9 @@ def debug(self, msg):
             "shared_workspace": self.slurm_config.get("shared_workspace"),
             "shared_data": self.config.additional_context.get("shared_data"),
             "results_dir": self.slurm_config.get("results_dir"),
-            "timeout": self.config.timeout,
+            # The sentinel, not config.timeout: the job script re-invokes
+            # madengine, and that run applies model-card precedence itself.
+            "timeout": self.config.cli_timeout,
             "live_output": self.config.additional_context.get("live_output", False),
             "tags": " ".join(model_info.get("tags", [])),
             "multiple_results": model_info.get("multiple_results"),
@@ -1288,7 +1291,7 @@ def _run_inside_existing_allocation(self) -> DeploymentResult:
             # Don't capture output - let it stream directly to console
             result = subprocess.run(
                 ["bash", str(self.script_path)],
-                timeout=self.config.timeout if self.config.timeout > 0 else None,
+                timeout=subprocess_timeout(self.config.timeout),
             )
 
             if result.returncode == 0:

src/madengine/deployment/templates/slurm/job.sh.j2

@@ -677,7 +677,7 @@ echo "[DEBUG] $(date -Iseconds) Node ${SLURM_PROCID} ($(hostname)): about to run
 # Environment variables (MASTER_ADDR, MAD_MULTI_NODE_RUNNER, etc.) are inherited
 $MAD_CLI_COMMAND run \
     --manifest-file "$EXEC_MANIFEST" \
-    --timeout {{ timeout | default(3600) }} \
+    --timeout {{ timeout }} \
     {% if shared_data %}--force-mirror-local {{ shared_data }}{% endif %} \
     {% if live_output %}--live-output{% endif %} \
     > "${NODE_LOG_OUT}" 2>> "${NODE_LOG_ERR}"
@@ -766,7 +766,7 @@ echo ""
 # Environment variables (MASTER_ADDR, MAD_MULTI_NODE_RUNNER, etc.) are inherited
 $MAD_CLI_COMMAND run \
     {% if manifest_file %}--manifest-file "$EXEC_MANIFEST"{% else %}--tags {{ tags }}{% endif %} \
-    --timeout {{ timeout | default(3600) }} \
+    --timeout {{ timeout }} \
     {% if shared_data %}--force-mirror-local {{ shared_data }}{% endif %} \
     {% if live_output %}--live-output{% endif %}
 

src/madengine/execution/container_runner.py

@@ -23,7 +23,7 @@
 from madengine.core.console import Console, redact_secrets
 from madengine.core.context import Context
 from madengine.core.docker import Docker
-from madengine.core.timeout import Timeout
+from madengine.core.timeout import DEFAULT_RUN_TIMEOUT, Timeout, subprocess_timeout
 from madengine.core.dataprovider import Data
 from madengine.utils.ops import PythonicTee, file_print
 from madengine.reporting.update_perf_csv import (
@@ -985,7 +985,7 @@ def _run_self_managed(
                         shell=True,
                         cwd=script_dir,
                         env=env,
-                        timeout=timeout if timeout > 0 else None,
+                        timeout=subprocess_timeout(timeout),
                     )
 
                     run_results["test_duration"] = time.time() - test_start_time
@@ -1100,7 +1100,7 @@ def run_container(
         keep_alive: bool = False,
         keep_model_dir: bool = False,
         skip_model_run: bool = False,
-        timeout: int = 7200,
+        timeout: int = DEFAULT_RUN_TIMEOUT,
         tools_json_file: str = "scripts/common/tools.json",
         phase_suffix: str = "",
         generate_sys_env_details: bool = True,
@@ -1600,11 +1600,12 @@ def run_container(
                         else:
                             self.rich_console.print("[bold blue]Running model...[/bold blue]")
                             # Use the container timeout (default 7200s) for script execution
-                            # to prevent indefinite hangs
+                            # to prevent indefinite hangs. A resolved timeout of 0 means
+                            # "no timeout", which communicate() spells as None.
                             try:
                                 model_output = model_docker.sh(
                                     f"cd {model_dir} && {script_name} {model_args}",
-                                    timeout=timeout,
+                                    timeout=subprocess_timeout(timeout),
                                 )
                             except RuntimeError as run_err:
                                 # On script failure, collect lightweight diagnostics from the
@@ -2764,7 +2765,7 @@ def run_models_from_manifest(
         self,
         manifest_file: str,
         registry: str = None,
-        timeout: int = 7200,
+        timeout: int = DEFAULT_RUN_TIMEOUT,
         keep_alive: bool = False,
         keep_model_dir: bool = False,
         skip_model_run: bool = False,

src/madengine/execution/container_runner_helpers.py

@@ -8,6 +8,10 @@
 import re
 import typing
 
+# Timeout resolution lives in core.timeout so the deployment layer can share it;
+# re-exported here for existing importers.
+from madengine.core.timeout import resolve_run_timeout  # noqa: F401
+
 # Default substrings matched in container run logs post-hoc (see ContainerRunner).
 DEFAULT_LOG_ERROR_PATTERNS: typing.Tuple[str, ...] = (
     "OutOfMemoryError",
@@ -200,35 +204,6 @@ def resolve_run_status(
     return "FAILURE", "no performance metrics"
 
 
-def resolve_run_timeout(
-    model_info: typing.Dict,
-    cli_timeout: int,
-    default_cli_timeout: int = 7200,
-) -> int:
-    """
-    Resolve effective run timeout from model config and CLI.
-
-    - If model has a timeout and CLI is using default (7200), use model's timeout.
-    - If CLI timeout is explicitly set (not default), it overrides model timeout.
-
-    Args:
-        model_info: Model info dict; may have "timeout" key.
-        cli_timeout: Timeout from CLI.
-        default_cli_timeout: Value considered "default" for CLI (typically 7200).
-
-    Returns:
-        Effective timeout in seconds.
-    """
-    if (
-        "timeout" in model_info
-        and model_info["timeout"] is not None
-        and model_info["timeout"] > 0
-        and cli_timeout == default_cli_timeout
-    ):
-        return model_info["timeout"]
-    return cli_timeout
-
-
 def _docker_image_ref_for_log_naming(docker_image: str) -> str:
     """
     Reduce a Docker image reference to a stable filename-safe log naming component.

src/madengine/orchestration/build_orchestrator.py

@@ -567,7 +567,9 @@ def _execute_with_prebuilt_image(
                     "training_precision": model.get("training_precision", ""),
                     "multiple_results": model.get("multiple_results", ""),
                     "tags": model.get("tags", []),
-                    "timeout": model.get("timeout", -1),
+                    # None (JSON null) = the card specified none; a card's -1 is
+                    # a real value meaning "no timeout", not filler.
+                    "timeout": model.get("timeout"),
                     "args": model.get("args", ""),
                     "slurm": model.get("slurm", {}),
                     "distributed": model_distributed,