diff --git a/README.md b/README.md index 529e36c..8fa8361 100644 --- a/README.md +++ b/README.md @@ -102,6 +102,7 @@ Each task gets its own directory, its own worker, its own log, and its own verdi | `model` | Which model a harness engine runs for this task — fills the engine's `{model}` placeholder (e.g. `"openrouter/moonshotai/kimi-k2.7"`); empty uses the engine's `model_default` | | `task_type` | Optional free-form string naming the kind of work this task is, so the model-performance log can slice pass rates by task shape rather than only by model. Suggested vocabulary: `code-feature`, `code-fix`, `code-review`, `test-hardening`, `docs`, `research`, `persona-review`, `copywriting`, `site-build`, `motion-design`, `image-gen`, `data-pipeline`, `format-conversion`, `probe`, `bakeoff`. Empty is allowed; the log just reports it under `(none)`. | | `timeout_s` | Per-task kill timer (default 900) | +| `check_timeout_s` | Optional per-task override of the default check timeout for checks that boot infrastructure. Match the timeout to the task applies to checks too. | | `engine_args` | Extra CLI flags for this task's worker, spliced in at the engine's `{engine_args}` placeholder — e.g. `["-c", "model_reasoning_effort=low"]` so the orchestrator picks reasoning depth per task | | `verified` | One plain-English sentence saying what the check proves — shown on the results page next to "finished & checked" | | `full_access` | Worker runs unsandboxed — required for workers that spawn their own sub-workers; must also be enabled in config | @@ -357,6 +358,7 @@ Every community PR that lands in main is credited here — that's a project rule - [@davekopecek](https://github.com/davekopecek) (Dave Kopecek) — committed the design-reference fixture so the design-token guard runs on every machine (#30) - [@snapsynapse](https://github.com/snapsynapse) (Sam Rogers) — graceful shutdown on SIGINT/SIGTERM with worker-tree cleanup and finished state, plus the 14-test end-to-end CLI regression suite (#4) - [@mlava](https://github.com/mlava) (Mark Lavercombe) — named setup failures across every diagnostic surface (#37) and `run --baseline`, the no-workers check preflight (#38) +- [@davidfoy](https://github.com/davidfoy) (David Foy) — per-task `check_timeout_s` so infrastructure-booting checks don't time out under the fixed default (OA-140) Contributions are welcome — see [CONTRIBUTING.md](CONTRIBUTING.md) for the philosophy and what gets a PR merged fast. The short version: small and scoped, rebased on current main, every claim backed by an executed test. Authorship is always preserved — where a maintainer pushes a mechanical fix to your branch, you remain the commit author. diff --git a/ringer.py b/ringer.py index 0dca41b..c7b520c 100755 --- a/ringer.py +++ b/ringer.py @@ -1010,6 +1010,7 @@ class TaskSpec: # engine's {model} placeholder); empty means the engine's model_default. model: str = "" task_type: str = "" + check_timeout_s: int | None = None @classmethod def from_obj(cls, obj: dict[str, Any]) -> "TaskSpec": @@ -1050,6 +1051,11 @@ def from_obj(cls, obj: dict[str, Any]) -> "TaskSpec": task_type = obj.get("task_type", "") if not isinstance(task_type, str): raise ValueError(f"task {key}: task_type must be a string") + check_timeout_s = None + if "check_timeout_s" in obj: + check_timeout_s = int(obj.get("check_timeout_s")) + if check_timeout_s <= 0: + raise ValueError(f"task {key}: check_timeout_s must be positive") return cls( key=key, spec=spec, @@ -1062,6 +1068,7 @@ def from_obj(cls, obj: dict[str, Any]) -> "TaskSpec": verified=verified.strip(), model=model.strip(), task_type=task_type.strip(), + check_timeout_s=check_timeout_s, ) @@ -1202,6 +1209,10 @@ def lint_manifest( f"{task.key}: no 'verified' description; a reader of the results page sees " "'checked' but not what the check proves — add one plain-English sentence." ) + if task.check_timeout_s is None and check_boots_infrastructure(task.check): + findings.append( + f"{task.key}: check appears to boot infrastructure; set check_timeout_s to match the task." + ) if include_model_log_nudges and not task.task_type: findings.append( f"{task.key}: no task_type; the model log buckets this as (untyped) — " @@ -1267,6 +1278,13 @@ def check_cannot_fail(check: str) -> bool: return consists_only_of_echo_commands(stripped) +INFRA_BOOT_CHECK_RE = re.compile(r"(?:\bdocker\b|\bcompose\s+up\b)", re.IGNORECASE) + + +def check_boots_infrastructure(check: str) -> bool: + return bool(INFRA_BOOT_CHECK_RE.search(strip_shell_comments(check))) + + def strip_shell_comments(command: str) -> str: result: list[str] = [] in_single = False @@ -7951,7 +7969,11 @@ def run_models_command(config: AppConfig, args: argparse.Namespace) -> int: class Verifier: async def verify(self, task: TaskSpec, taskdir: Path) -> VerifyResult: - check_returncode, check_timed_out, output = await self._run_check(task.check, taskdir) + check_returncode, check_timed_out, output = await self._run_check( + task.check, + taskdir, + timeout_s=task.check_timeout_s, + ) missing_files = tuple( rel for rel in task.expect_files if not self._is_nonempty_file(self._expect_file_path(taskdir, rel)) ) @@ -7989,7 +8011,13 @@ def _expect_file_path(taskdir: Path, path: str) -> Path: return candidate if candidate.is_absolute() else taskdir / candidate @staticmethod - async def _run_check(command: str, cwd: Path) -> tuple[int | None, bool, str]: + async def _run_check( + command: str, + cwd: Path, + timeout_s: int | None = None, + ) -> tuple[int | None, bool, str]: + effective_timeout_s = timeout_s if timeout_s is not None else CHECK_TIMEOUT_S + timeout_source = "task check_timeout_s" if timeout_s is not None else "default" proc = await asyncio.create_subprocess_shell( command, cwd=str(cwd), @@ -8000,7 +8028,10 @@ async def _run_check(command: str, cwd: Path) -> tuple[int | None, bool, str]: ) timed_out = False try: - stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=CHECK_TIMEOUT_S) + stdout, _ = await asyncio.wait_for( + proc.communicate(), + timeout=effective_timeout_s, + ) except asyncio.TimeoutError: timed_out = True terminate_process_group(proc) @@ -8011,7 +8042,7 @@ async def _run_check(command: str, cwd: Path) -> tuple[int | None, bool, str]: stdout, _ = await proc.communicate() output = stdout.decode("utf-8", errors="replace") if stdout else "" if timed_out: - output += f"\n[ringer.py] check timed out after {CHECK_TIMEOUT_S}s\n" + output += f"\n[ringer.py] check timed out after {effective_timeout_s}s ({timeout_source})\n" return proc.returncode, timed_out, output diff --git a/tests/test_check_timeout.py b/tests/test_check_timeout.py new file mode 100644 index 0000000..c51dc30 --- /dev/null +++ b/tests/test_check_timeout.py @@ -0,0 +1,204 @@ +#!/usr/bin/env python3 +"""Oracle for per-task check timeouts (OA-140). + +A fixed CHECK_TIMEOUT_S = 60 fails any check that boots real infrastructure +(a docker-backed test stack takes ~90s), marking good workers failed. These +tests pin the contract for the optional per-task `check_timeout_s` field: + +- absent -> behavior identical to today (module constant CHECK_TIMEOUT_S, + consulted at call time so tests can shrink it) +- present -> positive int, threaded into the check's asyncio.wait_for +- the timeout message names the limit AND where it came from, so retry + prompts and post-mortems show whether the cap was chosen or inherited +- lint warns when a check looks like it boots infrastructure (generic + patterns only: docker / compose up) while check_timeout_s is unset + +Set RINGER_SLOW_ORACLE=1 to also run the ticket-verbatim slow cases +(a real 90s check against the real 60s default). +""" +from __future__ import annotations + +import asyncio +import os +import sys +import tempfile +import time +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT)) + +import ringer as ringer_module # noqa: E402 +from ringer import Manifest, TaskSpec, Verifier, lint_manifest # noqa: E402 + +LONG_SPEC = ( + "Create the requested artifact in the current working directory, keep the change scoped, " + "and make the check command able to explain any failure clearly." +) + +PLAIN_CHECK = ( + "test -s output.txt && grep -q 'ready' output.txt || " + "{ echo 'FAIL: output.txt missing or does not contain ready'; exit 1; }" +) + +STACK_BOOT_CHECK = ( + "docker compose up -d && ./scripts/wait-healthy.sh && pytest tests/ || " + "{ echo 'FAIL: stack tests failed'; exit 1; }" +) + + +def task_obj(key: str = "one", **overrides: object) -> dict[str, object]: + obj: dict[str, object] = { + "key": key, + "spec": LONG_SPEC, + "check": PLAIN_CHECK, + "expect_files": ["output.txt"], + "verified": "output.txt exists and contains ready", + "task_type": "probe", + } + obj.update(overrides) + return obj + + +class TaskSpecCheckTimeoutTests(unittest.TestCase): + def test_absent_field_parses_as_none(self) -> None: + task = TaskSpec.from_obj(task_obj()) + self.assertIsNone(task.check_timeout_s) + + def test_present_field_parses_as_int(self) -> None: + task = TaskSpec.from_obj(task_obj(check_timeout_s=120)) + self.assertEqual(task.check_timeout_s, 120) + + def test_nonpositive_field_rejected(self) -> None: + for bad in (0, -5): + with self.assertRaises(ValueError): + TaskSpec.from_obj(task_obj(check_timeout_s=bad)) + + +class VerifierCheckTimeoutTests(unittest.TestCase): + def verify(self, task: TaskSpec) -> "ringer_module.VerifyResult": + with tempfile.TemporaryDirectory() as tmp: + return asyncio.run(Verifier().verify(task, Path(tmp))) + + def test_task_check_timeout_is_honored(self) -> None: + # sleep 3 passes under the 60s default; with a 1s per-task cap the + # check must time out, and quickly (proof the cap was threaded, not + # the constant). + task = TaskSpec.from_obj( + task_obj(check="sleep 3 && echo ok", check_timeout_s=1, expect_files=[]) + ) + start = time.monotonic() + result = self.verify(task) + elapsed = time.monotonic() - start + self.assertTrue(result.check_timed_out) + self.assertFalse(result.ok) + self.assertLess(elapsed, 3.0) + + def test_timeout_message_names_task_source(self) -> None: + task = TaskSpec.from_obj( + task_obj(check="sleep 3 && echo ok", check_timeout_s=1, expect_files=[]) + ) + result = self.verify(task) + self.assertIn("check timed out after 1s", result.raw_output_excerpt) + self.assertIn("task check_timeout_s", result.raw_output_excerpt) + + def test_timeout_message_names_default_source(self) -> None: + # The default path must consult the module constant at call time; + # shrink it so the default-timeout branch is provable in seconds. + original = ringer_module.CHECK_TIMEOUT_S + ringer_module.CHECK_TIMEOUT_S = 1 + try: + task = TaskSpec.from_obj( + task_obj(check="sleep 3 && echo ok", expect_files=[]) + ) + result = self.verify(task) + finally: + ringer_module.CHECK_TIMEOUT_S = original + self.assertTrue(result.check_timed_out) + self.assertIn("check timed out after 1s", result.raw_output_excerpt) + self.assertIn("default", result.raw_output_excerpt) + self.assertNotIn("task check_timeout_s", result.raw_output_excerpt) + + def test_generous_task_timeout_lets_slow_check_pass(self) -> None: + original = ringer_module.CHECK_TIMEOUT_S + ringer_module.CHECK_TIMEOUT_S = 1 + try: + task = TaskSpec.from_obj( + task_obj(check="sleep 3 && echo ok", check_timeout_s=30, expect_files=[]) + ) + result = self.verify(task) + finally: + ringer_module.CHECK_TIMEOUT_S = original + self.assertFalse(result.check_timed_out) + self.assertEqual(result.check_returncode, 0) + self.assertTrue(result.ok) + + +class LintStackBootTests(unittest.TestCase): + def manifest(self, tasks: list[dict[str, object]]) -> Manifest: + temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(temp_dir.cleanup) + return Manifest.from_obj( + { + "run_name": "check-timeout-lint-test", + "workdir": str(Path(temp_dir.name) / "work"), + "max_parallel": 1, + "worktrees": False, + "tasks": tasks, + } + ) + + def findings_mentioning_check_timeout(self, tasks: list[dict[str, object]]) -> list[str]: + findings = lint_manifest(self.manifest(tasks), allow_noncanonical_route=True) + return [f for f in findings if "check_timeout_s" in f] + + def test_warns_on_stack_boot_check_without_field(self) -> None: + findings = self.findings_mentioning_check_timeout( + [task_obj(check=STACK_BOOT_CHECK)] + ) + self.assertEqual(len(findings), 1) + self.assertIn("one:", findings[0]) + + def test_no_warning_when_field_is_set(self) -> None: + findings = self.findings_mentioning_check_timeout( + [task_obj(check=STACK_BOOT_CHECK, check_timeout_s=300)] + ) + self.assertEqual(findings, []) + + def test_no_warning_on_plain_check(self) -> None: + findings = self.findings_mentioning_check_timeout([task_obj()]) + self.assertEqual(findings, []) + + +@unittest.skipUnless( + os.environ.get("RINGER_SLOW_ORACLE") == "1", + "ticket-verbatim slow oracle; set RINGER_SLOW_ORACLE=1 (runs ~2.5 min)", +) +class SlowTicketOracleTests(unittest.TestCase): + """OA-140 oracle, verbatim: sleep 90 && echo ok against the REAL default.""" + + def verify(self, task: TaskSpec) -> "ringer_module.VerifyResult": + with tempfile.TemporaryDirectory() as tmp: + return asyncio.run(Verifier().verify(task, Path(tmp))) + + def test_90s_check_fails_with_default(self) -> None: + task = TaskSpec.from_obj( + task_obj(check="sleep 90 && echo ok", expect_files=[]) + ) + result = self.verify(task) + self.assertTrue(result.check_timed_out) + self.assertFalse(result.ok) + + def test_90s_check_passes_with_check_timeout_120(self) -> None: + task = TaskSpec.from_obj( + task_obj(check="sleep 90 && echo ok", check_timeout_s=120, expect_files=[]) + ) + result = self.verify(task) + self.assertFalse(result.check_timed_out) + self.assertEqual(result.check_returncode, 0) + self.assertTrue(result.ok) + + +if __name__ == "__main__": + unittest.main()