From e2776094f2eb1c0e376ea1d93e868e4972b4e9d9 Mon Sep 17 00:00:00 2001 From: Mengye Ren Date: Wed, 30 Sep 2026 23:30:41 -0400 Subject: [PATCH] A bad author override holds only its own slots; the tick keeps running The tick no longer validates every override entry at startup and exits on the first bad one. Each claim checks its own slot: a bad entry holds fresh claims for its slots with one log line per tick, without falling back to the fleet author; an unparseable setting holds claims for the targets it names (or all), while sweeps, wakes and delivery continue. outerloop start and init still refuse a bad setting. --- CHANGELOG.md | 7 +- docs/install.md | 7 +- src/outerloop/tick.py | 124 +++++++++++++++++++++++-------- tests/test_author_overrides.py | 129 ++++++++++++++++++++++++++++++--- tests/test_tick.py | 25 +++++++ 5 files changed, 247 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41c80897..86917803 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ Versions follow [SemVer](https://semver.org). ## [Unreleased] +- Bad author overrides hold only affected fresh claims during ticks, with one log + per entry per tick; malformed settings hold named targets, or all fresh claims + when unreadable. Other tick services and bound runs continue; `outerloop start` + and `outerloop init` remain strict. No persisted state changes. + - Add read-only `outerloop status` (text/`--json`) for local runs and endpoint outages. Endpoint waits stay out of the published board/status strip and never trigger research-log commits; log one shared outage start and recovery with @@ -16,7 +21,7 @@ Versions follow [SemVer](https://semver.org). keys/journals are tolerated; ended runs and in-flight PRs are unchanged. Rollback to the preceding kernel safely ignores the additive state. -- Startup validation of `OUTERLOOP_AUTHOR_OVERRIDES` (the tick and `outerloop start`) uses the +- Startup validation of `OUTERLOOP_AUTHOR_OVERRIDES` (`outerloop start`) uses the image sessions actually run with, the default image when `OUTERLOOP_IMAGE` is unset. Before, a codex override on a deployment without `OUTERLOOP_IMAGE` failed validation and stopped the tick. diff --git a/docs/install.md b/docs/install.md index 997b3b5b..9b95fdf3 100644 --- a/docs/install.md +++ b/docs/install.md @@ -627,8 +627,11 @@ OUTERLOOP_AUTHOR_OVERRIDES='{"owner/repo":[{"backend":"claude","model":"served-m ``` This is deployment configuration, not a contract setting. The setting is -parsed and validated at startup. Endpoint overrides select their own profile in -`model`; they do not inherit `OUTERLOOP_AUTHOR_ENDPOINT`. Native overrides use +strictly validated by `outerloop start` and `outerloop init`; during ticks, an unusable +entry holds fresh claims only for its slots, without falling back to the fleet author. +A malformed setting holds fresh claims for every readable target key (or all targets +if unreadable), while existing runs and other tick services continue. +Endpoint overrides select their own profile in `model`; they do not inherit `OUTERLOOP_AUTHOR_ENDPOINT`. Native overrides use the selected backend's author credential. Normal author/judge credential separation still applies to the effective override credential. diff --git a/src/outerloop/tick.py b/src/outerloop/tick.py index 44c8debf..7bb5d9ba 100644 --- a/src/outerloop/tick.py +++ b/src/outerloop/tick.py @@ -22,12 +22,13 @@ import socket import subprocess import sys -from collections.abc import Sequence +from collections.abc import Mapping, Sequence from dataclasses import asdict, dataclass, field, replace from pathlib import Path from typing import Any, Protocol from uuid import uuid4 +from outerloop.author_overrides import AuthorOverride from outerloop.compute import ( GONE, Compute, @@ -1910,6 +1911,9 @@ def tick( min_tick_s, ) return TickReport(coalesced=True, launch_blocked=_launches_held(root)) + author_errors: set[str] = set() + if service_spec is not None: + _preflight_claim_overrides(service_spec, author_errors) report = sweep( root, compute, @@ -2051,6 +2055,7 @@ def tick( limits, dry_run=service_dry_run, records=tick_records, + author_errors=author_errors, ) if launch_ok and contract is not None else None @@ -2082,6 +2087,7 @@ def tick( limits=limits, dry_run=service_dry_run, records=tick_records, + author_errors=author_errors, ) except Exception as exc: log.warning("self-initiated selection failed: %s", exc) @@ -2438,11 +2444,77 @@ def _climb_panel_argv(spec: ServiceSpec) -> list[str]: return argv +def _malformed_override_targets() -> tuple[str, ...] | None: + """Recover target scope only from a readable JSON object; otherwise hold all.""" + from outerloop.author_overrides import SETTING + + try: + data = json.loads(os.environ.get(SETTING, "")) + except ValueError: + return None + return tuple(data) if isinstance(data, dict) and data else None + + +def _claim_overrides(target: str) -> Mapping[str, tuple[AuthorOverride, ...]]: + from outerloop.author_overrides import overrides + + try: + return overrides() + except ValueError: + targets = _malformed_override_targets() + if targets is not None and target not in targets: + return {} + raise + + +def _claim_author_error(spec: ServiceSpec, agent_id: str, reported: set[str] | None = None) -> str: + """Use the author preflight for claims, logging each affected entry once per tick.""" + error = _author_config_error(spec, agent_id) + if not error or isinstance(error, EndpointWaitReason): + return error + from outerloop.author_overrides import overrides + + try: + selected = next((o for o in overrides().get(spec.target, ()) if o.matches(agent_id)), None) + slots = ", ".join(selected.slots) if selected and selected.slots else "all" + scope = f"target {spec.target}, slots {slots}" + except ValueError: + targets = _malformed_override_targets() + scope = f"targets {', '.join(targets)}, slots all" if targets else "all targets, slots all" + message = f"fresh claims held: {scope}: author misconfigured — {error}" + if reported is None or message not in reported: + log.error("%s", message) + if reported is not None: + reported.add(message) + return error + + +def _preflight_claim_overrides(spec: ServiceSpec, reported: set[str]) -> None: + from outerloop.author_overrides import overrides + + try: + entries = overrides() + except ValueError: + targets = _malformed_override_targets() + _claim_author_error( + replace(spec, target=targets[0] if targets else spec.target), "agent-01", reported + ) + return + for target, group in entries.items(): + for entry in group: + _claim_author_error( + replace(spec, target=target), + entry.slots[0] if entry.slots else "agent-01", + reported, + ) + + def _selected_author(spec: ServiceSpec, agent_id: str = "agent-01") -> tuple[str, str]: from outerloop.attempt import fleet_author_model - from outerloop.author_overrides import select_override - selected = select_override(spec.target, agent_id) + selected = next( + (o for o in _claim_overrides(spec.target).get(spec.target, ()) if o.matches(agent_id)), None + ) if selected: return selected.backend, selected.resolved_model() backend = os.environ.get("OUTERLOOP_AUTHOR_BACKEND") or "claude" @@ -2451,10 +2523,13 @@ def _selected_author(spec: ServiceSpec, agent_id: str = "agent-01") -> tuple[str def _climb_author_argv(spec: ServiceSpec, agent_id: str = "agent-01") -> list[str]: from outerloop.attempt import effective_author_credential - from outerloop.author_overrides import overrides, select_override + from outerloop.author_overrides import overrides - if not overrides(): - return [] + try: + if not overrides(): + return [] + except ValueError: + _claim_overrides(spec.target) # Only unaffected targets may bind the fleet author. backend, model = _selected_author(spec, agent_id) credential = effective_author_credential(backend, model) return [ @@ -2465,7 +2540,11 @@ def _climb_author_argv(spec: ServiceSpec, agent_id: str = "agent-01") -> list[st model, "--key-file", str(credential.key_file), - *(["--author-overridden"] if select_override(spec.target, agent_id) else []), + *( + ["--author-overridden"] + if any(o.matches(agent_id) for o in _claim_overrides(spec.target).get(spec.target, ())) + else [] + ), ] @@ -2755,6 +2834,7 @@ def service_self_initiated( limits: EffectiveLimits | None = None, dry_run: bool = False, records: list[RunRecord] | None = None, + author_errors: set[str] | None = None, ) -> tuple[str, str] | None: """The default background mode: when nothing else needs doing, climb the least-recently-attempted benchmark. @@ -2763,6 +2843,7 @@ def service_self_initiated( `compute.submit` and the climb job writing its run record — without it, every tick during Slurm queue latency would launch a duplicate climb. """ + author_errors = author_errors if author_errors is not None else set() limits = limits if limits is not None else effective_limits(getattr(contract, "budgets", None)) paused = outage_active(root, now, role="solver") if paused: @@ -2823,6 +2904,9 @@ def service_self_initiated( if len(occupied) >= width: return None slot_agent = _free_agent_slot(occupied, width) + while slot_agent is not None and _claim_author_error(spec, slot_agent, author_errors): + occupied.add(slot_agent) + slot_agent = _free_agent_slot(occupied, width) if slot_agent is None: return None dead_attempts = read_tombstones(root, spec.target, contract, now) @@ -2845,17 +2929,6 @@ def service_self_initiated( if lane_error := _gpu_lane_error(contract, benchmark, spec): log.error("attempt on %s not launched: %s", benchmark, lane_error) return None - author_error = _author_config_error(spec, slot_agent) - if author_error: - if isinstance(author_error, EndpointWaitReason): - return None - log.error( - "climb on %s not launched: author misconfigured — %s " - "(fix OUTERLOOP_AUTHOR_BACKEND/_MODEL)", - benchmark, - author_error, - ) - return None panel_error = _panel_preflight_error(spec, slot_agent) if panel_error: log.error( @@ -3071,6 +3144,7 @@ def service_intake( limits: EffectiveLimits | None = None, dry_run: bool = False, records: list[RunRecord] | None = None, + author_errors: set[str] | None = None, ) -> tuple[str, str] | None: """The requested lane: claim at most ONE qualifying issue per tick and submit a climb job for it. The claim comment (posted by the climb job @@ -3125,16 +3199,7 @@ def service_intake( if lane_error := _gpu_lane_error(contract, task.benchmark, spec): log.error("attempt on %s not launched: %s", task.benchmark, lane_error) return None - author_error = _author_config_error(spec) - if author_error: - if isinstance(author_error, EndpointWaitReason): - return None - log.error( - "issue #%d not claimed: author misconfigured — %s " - "(fix OUTERLOOP_AUTHOR_BACKEND/_MODEL)", - task.number, - author_error, - ) + if _claim_author_error(spec, "agent-01", author_errors): return None panel_error = _panel_preflight_error(spec) if panel_error: @@ -3602,11 +3667,8 @@ def main() -> int: "OUTERLOOP_CADENCE_MIN via the chain's own parser (default 30)", ) args = parser.parse_args() - from outerloop.author_overrides import validate_overrides - try: gpu_lanes = gpu_lanes_from_env() - validate_overrides(os.environ, startup_image()) except ValueError as exc: parser.error(str(exc)) logging.basicConfig(level=logging.INFO, format="%(asctime)s %(message)s") diff --git a/tests/test_author_overrides.py b/tests/test_author_overrides.py index 08921383..477cd0ba 100644 --- a/tests/test_author_overrides.py +++ b/tests/test_author_overrides.py @@ -240,7 +240,7 @@ def launch(**kw): backend, "served-model[endpoint=onprem]", ) - monkeypatch.setenv("OUTERLOOP_AUTHOR_OVERRIDES", "{}") + monkeypatch.setenv("OUTERLOOP_AUTHOR_OVERRIDES", "broken json") # Exercise the actual dispatched wake entrypoint, including its harness. monkeypatch.setattr(attempt, "_lease_held_by_another_job", lambda *a: "") monkeypatch.setattr("outerloop.tick.dispatch_wake_armed", lambda *a: True) @@ -460,16 +460,6 @@ def test_startup_validation_uses_the_image_the_tick_runs(monkeypatch): assert "requires --image" not in str(exc) -def test_tick_startup_validates_with_startup_image(): - # The tick's entry point must validate overrides with the image sessions run with. - import inspect - - from outerloop import tick - - src = inspect.getsource(tick.main) - assert "validate_overrides(os.environ, startup_image())" in src - - @pytest.mark.parametrize( ("mode", "env_value", "file_value", "expected"), [ @@ -521,3 +511,120 @@ def test_start_validates_with_the_launched_tick_image( with contextlib.suppress(SystemExit): cli.main(["start", "--dry-run", "--root", str(tmp_path / "state")]) assert seen and seen[0] == expected + + +@pytest.mark.parametrize( + "raw", + [ + "broken json", + '{"owner/repo":{"backend":"claude","model":"claude-trial","unknown":true}}', + '{"owner/repo":[{"backend":"claude","model":"claude-trial","slots":["agent-01"]},{"backend":"codex","model":"trial","slots":["agent-01"]}]}', + ], +) +def test_malformed_claim_scope(deployment, monkeypatch, caplog, raw): + from outerloop import tick + + monkeypatch.setenv("OUTERLOOP_AUTHOR_OVERRIDES", raw) + reported: set[str] = set() + tick._preflight_claim_overrides(deployment, reported) + for slot in ("agent-01", "agent-02"): + assert tick._claim_author_error(deployment, slot, reported) + assert caplog.text.count("fresh claims held:") == 1 + other = replace(deployment, target="owner/other") + if raw.startswith("{"): + assert tick._author_config_error(other) == "" + argv = tick._climb_author_argv(other) + assert "--author-bound" in argv + assert "--author-overridden" not in argv + else: + assert tick._author_config_error(other) + + +def test_bad_entry_skips_its_slots_without_fallback(deployment, monkeypatch, caplog): + from outerloop import tick + from outerloop.contract import load_contract + + monkeypatch.setenv( + "OUTERLOOP_AUTHOR_OVERRIDES", + json.dumps( + { + "owner/repo": [ + { + "backend": "codex", + "model": "claude-wrong", + "slots": ["agent-01", "agent-02"], + }, + {"backend": "claude", "model": "claude-trial", "slots": ["agent-03"]}, + ] + } + ), + ) + spec = replace(deployment, panel="") + contract = load_contract( + """ +benchmarks: + - {name: bench, command: c, metric: m, direction: min} +budgets: {max_active_attempts: 3, gpu_hours_per_run: 1, runs_per_week: 500} +scope: {allowed: [src/]} +roadmap: docs/roadmap.md +""", + spec.target, + ) + submitted = [] + + def submit(*args): + submitted.append(args[-1]) + return "123" + + monkeypatch.setattr(tick, "submit", submit) + monkeypatch.setattr(tick, "_flight_command", lambda home, name, now, argv: " ".join(argv)) + monkeypatch.setattr( + "outerloop.intake.pick_issue", lambda *args: SimpleNamespace(benchmark="bench", number=1) + ) + for now in (1000000, 1000100): + caplog.clear() + reported: set[str] = set() + tick._preflight_claim_overrides(spec, reported) + assert ( + tick.service_intake( + spec.run_root, + cast(Any, object()), + cast(Any, object()), + spec, + now, + contract=contract, + author_errors=reported, + ) + is None + ) + assert tick.service_self_initiated( + spec.run_root, + cast(Any, object()), + spec, + contract, + now, + records=[], + author_errors=reported, + ) == ("bench", "123") + tick.clear_pending(spec.run_root, spec.target, "agent-03") + assert caplog.text.count("fresh claims held:") == 1 + assert "owner/repo, slots agent-01, agent-02" in caplog.text + assert all("--agent-id agent-03" in job.command for job in submitted) + assert all("--model claude-trial" in job.command for job in submitted) + assert tick._selected_author(spec, "agent-01") == ("codex", "claude-wrong") + + +@pytest.mark.parametrize( + "raw", ["broken json", '{"owner/repo":{"backend":"codex","model":"claude-wrong"}}'] +) +def test_start_rejects_bad_overrides(deployment, monkeypatch, capsys, raw): + from outerloop import cli + + settings = deployment.home / "settings.env" + settings.write_text("") + settings.chmod(0o600) + monkeypatch.setattr(cli, "ENV_FILE", settings) + monkeypatch.setenv("OUTERLOOP_ENV_FILE", str(settings)) + monkeypatch.setenv("OUTERLOOP_AUTHOR_OVERRIDES", raw) + assert cli.main(["start", "--local", "--dry-run", "--root", str(deployment.run_root)]) == 2 + assert "OUTERLOOP_AUTHOR_OVERRIDES" in capsys.readouterr().err diff --git a/tests/test_tick.py b/tests/test_tick.py index 33233896..dcdaaecb 100644 --- a/tests/test_tick.py +++ b/tests/test_tick.py @@ -4686,3 +4686,28 @@ def test_hermes_panel_preflight_runtime(tmp_path, monkeypatch, state): assert problem == "" else: assert "bash scripts/install_hermes.sh" in problem + + +@pytest.mark.parametrize("raw", ["broken json", '{"o/r":{"unexpected":true}}']) +def test_malformed_overrides_do_not_stop_tick_main(tmp_path, monkeypatch, caplog, raw): + import sys + + from outerloop import tick as mod + + waiting_run(tmp_path) + compute = FakeSlurm(states={"100": "FAILED"}).compute() + dispatcher = RecordingDispatcher() + spec = mod.ServiceSpec( + account="", partition="", run_root=tmp_path, target="o/r", home=tmp_path, image="" + ) + monkeypatch.setenv("OUTERLOOP_AUTHOR_OVERRIDES", raw) + monkeypatch.setattr(sys, "argv", ["tick", "--root", str(tmp_path), "--min-free-gb", "0"]) + monkeypatch.setattr(mod, "compute_from_env", lambda: compute) + monkeypatch.setattr(mod, "_service_spec_from_env", lambda *a, **kw: (None, spec)) + monkeypatch.setattr(mod, "_wake_dispatcher_from_env", lambda *a: (dispatcher, True)) + monkeypatch.setattr("time.time", lambda: NOW) + assert mod.main() == 0 + assert dispatcher.dispatched == [("r1", "experiment FAILED")] + assert load_record(tmp_path, "r1").wake_attempts == 1 + assert caplog.text.count("fresh claims held:") == 1 + assert mod._author_config_error(spec)