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/k8s_template_context.py

@@ -31,6 +31,7 @@
 from madengine.core.additional_context_defaults import DEFAULT_GUEST_OS
 from madengine.core.dataprovider import Data
 from madengine.core.errors import ConfigurationError
+from madengine.core.timeout import resolve_run_timeout
 from madengine.utils.gpu_config import resolve_runtime_gpus
 from madengine.utils.path_utils import get_madengine_root
 
@@ -545,7 +546,11 @@ def _prepare_template_context(
             "nnodes": nnodes,
             "nproc_per_node": nproc_per_node,
             "master_port": master_port,
-            "timeout": self.config.timeout,
+            # Resolved here, not taken from config.timeout: that value caps the
+            # submitting process's wait on the Job and never saw the model card.
+            # K8s has no inner madengine to re-resolve against the card (unlike
+            # SLURM), so the card's timeout has to be applied at render time.
+            "timeout": resolve_run_timeout(model_info, self.config.cli_timeout),
             # Environment - Merge base env vars with data/tools env vars
             "env_vars": self._prepare_env_vars(model_info),
             # Volumes

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/kubernetes/job.yaml.j2

@@ -300,14 +300,34 @@ spec:
             {%   endfor %}
             {% endif %}
 
-            # Execute launcher with tool chain
-            MODEL_START_TIME=$(date +%s.%N)
+            # Execute launcher with tool chain.
+            # The command goes into a file so `timeout` can wrap it: a tool chain
+            # may begin with an environment assignment, which `timeout` would
+            # otherwise try to exec as the program name.
+            cat > /tmp/run_model.sh << 'MODEL_EOF'
             {% if launcher_tool_chain and launcher_tool_chain != "bash /tmp/run_launcher.sh" %}
             {{ launcher_tool_chain }}
             {% else %}
             bash /tmp/run_launcher.sh
             {% endif %}
+            MODEL_EOF
+            MODEL_START_TIME=$(date +%s.%N)
+            {% if timeout > 0 %}
+            echo "⏰ Setting timeout to {{ timeout }} seconds."
+            if timeout {{ timeout }} bash /tmp/run_model.sh; then
+                MODEL_EXIT_CODE=0
+            else
+                MODEL_EXIT_CODE=$?
+                if [ "$MODEL_EXIT_CODE" -eq 124 ]; then
+                    echo "ERROR: model script timed out after {{ timeout }}s"
+                fi
+                exit $MODEL_EXIT_CODE
+            fi
+            {% else %}
+            echo "⏰ No timeout set; the model script runs unbounded."
+            bash /tmp/run_model.sh
             MODEL_EXIT_CODE=$?
+            {% endif %}
             MODEL_END_TIME=$(date +%s.%N)
             MODEL_DURATION=$(awk "BEGIN {printf \"%.6f\", $MODEL_END_TIME - $MODEL_START_TIME}")
             echo "test_duration: ${MODEL_DURATION}s"
@@ -480,14 +500,36 @@ spec:
                 {%   endfor %}
                 {% endif %}
 
-                # Execute script with tool chain
-                MODEL_START_TIME=$(date +%s.%N)
+                # Execute script with tool chain.
+                # The command goes into a file so `timeout` can wrap it: a tool
+                # chain may begin with an environment assignment, which `timeout`
+                # would otherwise try to exec as the program name. The heredoc
+                # terminator sits at the block scalar's own indent so that it
+                # lands in column 0 of the rendered shell script.
+                cat > /tmp/run_model.sh << 'MODEL_EOF'
                 {% if direct_script_tool_chain and direct_script_tool_chain != "bash " ~ model_script %}
                 {{ direct_script_tool_chain }}
                 {% else %}
                 bash {{ model_script }}
                 {% endif %}
+            MODEL_EOF
+                MODEL_START_TIME=$(date +%s.%N)
+                {% if timeout > 0 %}
+                echo "⏰ Setting timeout to {{ timeout }} seconds."
+                if timeout {{ timeout }} bash /tmp/run_model.sh; then
+                    MODEL_EXIT_CODE=0
+                else
+                    MODEL_EXIT_CODE=$?
+                    if [ "$MODEL_EXIT_CODE" -eq 124 ]; then
+                        echo "ERROR: model script timed out after {{ timeout }}s"
+                    fi
+                    exit $MODEL_EXIT_CODE
+                fi
+                {% else %}
+                echo "⏰ No timeout set; the model script runs unbounded."
+                bash /tmp/run_model.sh
                 MODEL_EXIT_CODE=$?
+                {% endif %}
                 MODEL_END_TIME=$(date +%s.%N)
                 MODEL_DURATION=$(awk "BEGIN {printf \"%.6f\", $MODEL_END_TIME - $MODEL_START_TIME}")
                 echo "test_duration: ${MODEL_DURATION}s"

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 %}