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

CHANGELOG.md

@@ -7,6 +7,22 @@ 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. 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 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.
+
+- **A model card's `timeout` was never enforced on Kubernetes**: `job.yaml.j2` never referenced the timeout at all, so K8s jobs ran the model script unbounded regardless of what the model card or `--timeout` said. Unlike SLURM, K8s has no inner `madengine run` inside the pod to re-resolve against the card, so `k8s_template_context.py` now calls `resolve_run_timeout()` at render time and the template wraps the model script in `timeout {{ timeout }}`, treating a non-positive resolved value as "run unbounded" per v1 precedence. A script killed by the timeout surfaces as exit code 124 with an explicit `model script timed out after Ns` line in the pod log. Both the launcher and direct-script paths are covered.
+
 ## [2.1.3] - 2026-07-15
 
 ### Added

docs/cli-reference.md

@@ -223,7 +223,7 @@ madengine run [OPTIONS]
 | `--tags` | `-t` | TEXT | `[]` | Model tags to run (can specify multiple) |
 | `--manifest-file` | `-m` | TEXT | `""` | Build manifest file path (for pre-built images) |
 | `--registry` | `-r` | TEXT | `None` | Docker registry URL |
-| `--timeout` | | INT | `-1` | Timeout in seconds (-1=default 7200s, 0=no timeout) |
+| `--timeout` | | INT | `-1` | Timeout in seconds. `-1` means "not passed" and falls through to the model card's `timeout`, or 7200s if it has none; `0` disables the timeout; a positive value overrides the model card. See [Usage — Custom Timeouts](usage.md#custom-timeouts). |
 | `--additional-context` | `-c` | TEXT | `"{}"` | Additional context as JSON string |
 | `--additional-context-file` | `-f` | TEXT | `None` | File containing additional context JSON |
 | `--keep-alive` | | FLAG | `False` | Keep Docker containers alive after run (local Docker only; ignored with a warning on SLURM/K8s) |

docs/configuration.md

@@ -335,7 +335,9 @@ 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`. Omit the field to get the
+7200s (2 hour) default. Use `0` (or any non-positive value) to run the model
+without a timeout:
 
 ```json
 {
@@ -349,6 +351,10 @@ Or use the command-line option, which overrides the model's timeout:
 madengine run --tags model --timeout 7200
 ```
 
+Full precedence rules, including the `-1` sentinel and how the timeout is
+applied on SLURM and Kubernetes, are in
+[Usage — Custom Timeouts](usage.md#custom-timeouts).
+
 ### Local Data Mirroring
 
 Force local data caching:

docs/usage.md

@@ -446,6 +446,21 @@ 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 distributed runs. On SLURM 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.
+On Kubernetes the timeout is resolved when the Job manifest is rendered — the
+pod has no inner `madengine` to resolve it — and the model script is wrapped in
+`timeout`, so a model that overruns is killed with exit code 124 and logs
+`model script timed out after Ns`.
+
 ### Debugging
 
 ```bash

docs/wiki/index.html

@@ -655,15 +655,20 @@ <h2>CLI — <code>run</code></h2>
   --verbose / --no-verbose</code></pre>
 
 <h4>Timeout resolution</h4>
+<p>Precedence, lowest to highest: the 7200&nbsp;s default, then the model card's
+<code>timeout</code> field, then an explicit <code>--timeout</code>.</p>
 <table>
-<thead><tr><th>Value</th><th>Resolved timeout</th></tr></thead>
+<thead><tr><th><code>--timeout</code></th><th>Resolved timeout</th></tr></thead>
 <tbody>
-<tr><td><code>-1</code> (default)</td><td>7200 s (2 hours)</td></tr>
-<tr><td><code>0</code></td><td>Disabled (no timeout)</td></tr>
-<tr><td>model card <code>timeout</code> field</td><td>Used when CLI is default (-1)</td></tr>
-<tr><td>Explicit positive int</td><td>That many seconds, overrides model card</td></tr>
+<tr><td><code>-1</code> (default)</td><td>Not passed; falls through to the model card's <code>timeout</code>, or 7200&nbsp;s (2 hours) if the card has none</td></tr>
+<tr><td><code>0</code></td><td>Disabled (no timeout), overriding the model card</td></tr>
+<tr><td>Explicit positive int</td><td>That many seconds, overrides the model card — including <code>--timeout 7200</code>, which is distinguishable from not passing the flag</td></tr>
+<tr><td><code>&lt; -1</code></td><td>Rejected with exit code 4 (<code>INVALID_ARGS</code>)</td></tr>
 </tbody>
 </table>
+<p>A model card may also set <code>"timeout": 0</code> (or any non-positive value)
+to declare "no timeout". Resolution lives in
+<code>core/timeout.py</code> and is shared by local, SLURM, and K8s runs.</p>
 </section>
 
 <!-- CLI: REPORT / DATABASE -->
@@ -842,7 +847,7 @@ <h4>Key field notes</h4>
 <thead><tr><th>Field</th><th>Notes</th></tr></thead>
 <tbody>
 <tr><td><code>n_gpus</code></td><td><code>"-1"</code> = use all GPUs on the host (<code>MAD_SYSTEM_NGPUS</code>). Positive int = that many GPUs. Used for perf CSV metadata.</td></tr>
-<tr><td><code>timeout</code></td><td>Used when CLI <code>--timeout=-1</code> (default). Explicit CLI value always wins.</td></tr>
+<tr><td><code>timeout</code></td><td>Seconds. Overrides the 7200&nbsp;s default; overridden in turn by an explicit <code>--timeout</code>. A non-positive value (<code>0</code>, <code>-1</code>) means "no timeout". Omit the field to take the default.</td></tr>
 <tr><td><code>skip_gpu_arch</code></td><td>Comma-separated GPU arch names (e.g. <code>"gfx908,A100"</code>). Model is skipped if detected arch matches. Disable with <code>--disable-skip-gpu-arch</code>.</td></tr>
 <tr><td><code>multiple_results</code></td><td>Path to CSV file (relative to model dir) with per-result rows that are appended to <code>perf.csv</code> individually.</td></tr>
 <tr><td><code>DOCKER_IMAGE_NAME</code> in <code>env_vars</code></td><td>Required for <code>slurm_multi</code>: specifies the registry image for parallel <code>srun docker pull</code> on compute nodes. Also set automatically by <code>DockerBuilder</code> after a successful push.</td></tr>

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: