fix(ansible): overlapping runs: one per host, one per schedule, one action run per alert rule (#5158)

- One run per host: start_run claims every host it touches (addresses read
  from the inventory Steward wrote; a source's own inventory file is claimed
  as that file) before it takes a run slot. The claim is all-or-nothing. A run
  waiting on a host shows queued and names the run it waits for, honours a
  cancel, and fails after HOST_WAIT_SECONDS (1 h) with which host and run.
- A due schedule whose previous run is still queued or running skips the
  firing: last_run_at moves on, last_run_id still names the run going, and
  last_error says "Skipped: the previous run was still going". The Schedules
  page shows that beside the run (muted), and a launch error in red.
- An alert rule that fires while the action run from an earlier firing is
  still going starts no new run; the event says so and names the run. An
  in-process set closes the gap between the check and the run being linked.
- docs/core/ansible.md: queued/cancelled statuses and the overlap rules.

Tests: tests/core/test_ansible_overlap.py (claim keys, waiting, all-or-nothing,
cancel, deadline) and tests/integration/test_ansible_overlap.py (two real
runs on one host take turns; schedule skip then fire; alert action skip then
start).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

steward/ansible/executor.py

@@ -38,6 +38,111 @@ def artifact_path(run_id: str) -> str:
     return os.path.join(ARTIFACT_DIR, f"{run_id}.log")
 
 
+# ── One run per host (#5158) ──────────────────────────────────────────────────
+# Two runs against the same host would race each other's changes, so a run
+# claims every host it touches before it takes a run slot, and waits (queued)
+# while another run holds one. The claim is all-or-nothing, so two runs over
+# overlapping hosts can't each hold half and wait on the other.
+
+HOST_WAIT_SECONDS = 3600.0  # a run waiting longer than this for a host fails
+
+
+class HostBusy(Exception):
+    """A run gave up waiting for a host another run held."""
+
+
+def run_hosts(inventory_content: str | None, inventory_path: str | None) -> set[str]:
+    """What a run touches, as claim keys: the address of each host in the YAML
+    inventory Steward wrote for it, or, for a source's own inventory file
+    (whose hosts Steward doesn't read), that file."""
+    if not inventory_content:
+        return {f"inventory:{inventory_path}"} if inventory_path else set()
+    import yaml
+    try:
+        data = yaml.safe_load(inventory_content)
+    except yaml.YAMLError:
+        return set()  # ansible-playbook will refuse it and say why
+    # A host listed again under a group carries no vars there, so its address
+    # is whichever listing gives one.
+    addresses: dict[str, str | None] = {}
+
+    def walk(group) -> None:
+        if not isinstance(group, dict):
+            return
+        members = group.get("hosts")
+        if isinstance(members, dict):
+            for name, hostvars in members.items():
+                addr = hostvars.get("ansible_host") if isinstance(hostvars, dict) else None
+                addresses[str(name)] = addr or addresses.get(str(name))
+        children = group.get("children")
+        if isinstance(children, dict):
+            for child in children.values():
+                walk(child)
+
+    if isinstance(data, dict):
+        for group in data.values():
+            walk(group)
+    return {str(addr or name).strip().lower() for name, addr in addresses.items()}
+
+
+def _host_gate(app: "Quart") -> dict:
+    """The claims and the wake-up event, bound to the running loop like the
+    run-slot semaphore (one loop in prod, one per test)."""
+    loop = asyncio.get_running_loop()
+    gate = getattr(app, "_ansible_host_gate", None)
+    if gate is None or gate["loop"] is not loop:
+        gate = {"loop": loop, "held": {}, "changed": asyncio.Event()}
+        app._ansible_host_gate = gate
+    return gate
+
+
+async def _claim_hosts(app: "Quart", run_id: str, hosts: set[str]) -> bool:
+    """Hold every host in hosts for run_id, waiting while another run holds
+    any of them. Returns whether it had to wait. Raises _Aborted when the run
+    is cancelled while waiting, HostBusy after HOST_WAIT_SECONDS."""
+    from steward.models.ansible import AnsibleRunStatus
+
+    gate = _host_gate(app)
+    held: dict[str, str] = gate["held"]
+    give_up = time.monotonic() + HOST_WAIT_SECONDS
+    waited = False
+    while True:
+        busy = sorted({held[h] for h in hosts if h in held and held[h] != run_id})
+        if not busy:
+            for h in hosts:
+                held[h] = run_id
+            return waited
+        if run_id in _cancelled:
+            raise _Aborted
+        remaining = give_up - time.monotonic()
+        if remaining <= 0:
+            names = ", ".join(sorted(h for h in hosts if h in held))
+            raise HostBusy(f"gave up after {HOST_WAIT_SECONDS:.0f} s waiting for "
+                           f"{names}, held by run {', '.join(busy)}")
+        if not waited:
+            waited = True
+            await _set_status(app, run_id, AnsibleRunStatus.queued)
+            _broadcast(run_id, f"[queued — waiting for run {', '.join(busy)} "
+                               "to finish on the same host]")
+        changed = gate["changed"]
+        try:
+            # Woken by a release; the short timeout also notices a cancel.
+            await asyncio.wait_for(changed.wait(), timeout=min(remaining, 2.0))
+        except asyncio.TimeoutError:
+            pass
+
+
+def _release_hosts(app: "Quart", run_id: str, hosts: set[str]) -> None:
+    gate = getattr(app, "_ansible_host_gate", None)
+    if gate is None:
+        return
+    for h in hosts:
+        if gate["held"].get(h) == run_id:
+            del gate["held"][h]
+    changed, gate["changed"] = gate["changed"], asyncio.Event()
+    changed.set()
+
+
 @dataclass
 class _Done:
     status: str
@@ -392,8 +497,9 @@ async def start_run(
 ) -> None:
     """Execute ansible-playbook as a subprocess and update the DB run row.
 
-    Respects a global concurrency cap (app._ansible_semaphore): if no slot is
-    free the run shows as 'queued' until one frees. Supports cancellation, a
+    Waits (as 'queued') while another run holds one of its hosts (#5158), then
+    for a slot under the global concurrency cap (app._ansible_semaphore).
+    Supports cancellation, a
     persistent full-log artifact, and a parsed per-host result summary.
 
     connection is an optional per-run SSH override (user/password) for
@@ -442,17 +548,23 @@ async def start_run(
         sem = asyncio.Semaphore(max(1, cap))
         app._ansible_semaphore = sem
         app._ansible_sem_loop = loop
-    queued = sem.locked()
-    if queued:
-        await _set_status(app, run_id, AnsibleRunStatus.queued)
-        _broadcast(run_id, "[queued — waiting for an Ansible run slot]")
-    await sem.acquire()
+    hosts = run_hosts(inventory_content, inventory_path)
 
     final_status = "failed"
     logf = None
     proc = None
     interrupted = False
+    slot_held = False
     try:
+        # The hosts first, so a run waiting on a host doesn't sit on a run slot.
+        queued = await _claim_hosts(app, run_id, hosts)
+        if sem.locked():
+            if not queued:
+                await _set_status(app, run_id, AnsibleRunStatus.queued)
+            queued = True
+            _broadcast(run_id, "[queued — waiting for an Ansible run slot]")
+        await sem.acquire()
+        slot_held = True
         if run_id in _cancelled:  # cancelled while queued — abort before launching
             raise _Aborted
         if queued:
@@ -592,7 +704,9 @@ async def start_run(
             logf.close()
         _run_procs.pop(run_id, None)
         _cancelled.discard(run_id)
-        sem.release()
+        if slot_held:
+            sem.release()
+        _release_hosts(app, run_id, hosts)
 
     results = {"hosts": parse_recap(_run_lines.get(run_id, [])), "failures": failures}
     try:

steward/ansible/runner.py

@@ -16,6 +16,19 @@ from steward.ansible.inventory_gen import (
 )
 from steward.models.ansible import AnsibleRun, AnsibleRunStatus
 
+# A run in one of these hasn't finished: a schedule or an alert action whose
+# previous run is still in one skips its next firing (#5158).
+IN_FLIGHT = (AnsibleRunStatus.queued, AnsibleRunStatus.running)
+
+
+async def run_in_flight(db, run_id: str | None) -> bool:
+    """Whether run_id names a run that hasn't finished. Runs left running by a
+    restart are marked interrupted at startup, so this can't stick."""
+    if not run_id:
+        return False
+    run = await db.get(AnsibleRun, run_id)
+    return run is not None and run.status in IN_FLIGHT
+
 
 async def trigger_run(
     app,

steward/ansible/scheduler.py

@@ -7,7 +7,7 @@ from datetime import datetime, timezone
 
 from sqlalchemy import select
 
-from steward.ansible.runner import trigger_run
+from steward.ansible.runner import run_in_flight, trigger_run
 from steward.models.ansible_schedule import AnsibleSchedule
 
 logger = logging.getLogger(__name__)
@@ -27,7 +27,8 @@ def is_due(schedule: AnsibleSchedule, now: datetime) -> bool:
 
 
 async def run_due_schedules(app) -> int:
-    """Trigger every due schedule once. Returns the number launched."""
+    """Trigger every due schedule once. Returns the number launched. A due
+    schedule whose previous run hasn't finished skips this firing."""
     now = datetime.now(timezone.utc)
     async with app.db_sessionmaker() as db:
         schedules = (await db.execute(
@@ -38,6 +39,21 @@ async def run_due_schedules(app) -> int:
     for s in schedules:
         if not is_due(s, now):
             continue
+        async with app.db_sessionmaker() as db:
+            busy = await run_in_flight(db, s.last_run_id)
+        if busy:
+            # One run at a time per schedule (#5158). This firing is skipped,
+            # not deferred, so the cadence holds; last_run_id still names the
+            # run that's going.
+            async with app.db_sessionmaker() as db:
+                async with db.begin():
+                    fresh = await db.get(AnsibleSchedule, s.id)
+                    if fresh:
+                        fresh.last_run_at = now
+                        fresh.last_error = "Skipped: the previous run was still going"
+            logger.info("Schedule %s (%s) skipped: run %s still going",
+                        s.id, s.name, s.last_run_id)
+            continue
         run = None
         err: str | None = None
         try:

steward/core/alerts.py

@@ -497,8 +497,47 @@ async def _run_ansible_action(
     inventory must be ones the source lists (ansible/guards.py). The firing
     event links to the run, or says why the action did not start.
     """
-    from steward.ansible.runner import trigger_run
+    from steward.ansible.runner import IN_FLIGHT, trigger_run
+    from steward.models.ansible import AnsibleRun
 
+    if rule.id in _actions_starting:
+        await _note_event(app, event_id, "Ansible action skipped: an earlier "
+                          "firing's action was still starting")
+        return
+    async with app.db_sessionmaker() as db:
+        busy = (await db.execute(
+            select(AnsibleRun.id)
+            .join(AlertEvent, AlertEvent.ansible_run_id == AnsibleRun.id)
+            .where(AlertEvent.rule_id == rule.id, AnsibleRun.status.in_(IN_FLIGHT))
+            .limit(1))).scalar_one_or_none()
+    if busy:
+        # One action run at a time per rule (#5158): a re-firing while the
+        # last one is still going would run the same fix over it.
+        await _note_event(app, event_id, f"Ansible action skipped: run {busy} "
+                          "from an earlier firing was still going")
+        return
+    _actions_starting.add(rule.id)
+    try:
+        await _start_ansible_action(app, rule, value, event_id, trigger_run)
+    finally:
+        _actions_starting.discard(rule.id)
+
+
+# Rules whose action is between the in-flight check and its run being linked
+# to the event, so two firings that close together can't both start one.
+_actions_starting: set[str] = set()
+
+
+async def _note_event(app: "Quart", event_id: str, why: str) -> None:
+    async with app.db_sessionmaker() as db:
+        async with db.begin():
+            ev = (await db.execute(
+                select(AlertEvent).where(AlertEvent.id == event_id))).scalar_one_or_none()
+            if ev is not None:
+                ev.note = f"{ev.note}; {why}" if ev.note else why
+
+
+async def _start_ansible_action(app, rule, value, event_id, trigger_run) -> None:
     action = rule.ansible_action or {}
     playbook = action.get("playbook") or ""
     inventory = action.get("inventory") or ""
@@ -529,17 +568,15 @@ async def _run_ansible_action(
     if err:
         logger.error("Ansible action for rule %r not started: %s", rule.name, err)
 
+    if run is None:
+        await _note_event(app, event_id, f"Ansible action not started: {err}")
+        return
     async with app.db_sessionmaker() as db:
         async with db.begin():
             ev = (await db.execute(
                 select(AlertEvent).where(AlertEvent.id == event_id))).scalar_one_or_none()
-            if ev is None:
-                return
-            if run is not None:
+            if ev is not None:
                 ev.ansible_run_id = run.id
-            else:
-                why = f"Ansible action not started: {err}"
-                ev.note = f"{ev.note}; {why}" if ev.note else why
 
 
 async def check_silence(app: "Quart") -> None:

steward/mcp/tools/automation.py

@@ -527,8 +527,10 @@ async def list_schedules() -> dict:
 
     `check: true` schedules are dry runs that never change anything.
     `last_error` is set when a firing could not start at all (a source or
-    playbook that went away); a run that started and failed shows in
-    `last_run_status`, and get_ansible_run(last_run_id) has its log.
+    playbook that went away), or was skipped because the schedule's previous
+    run was still going (then `last_run_id` still names that run). A run that
+    started and failed shows in `last_run_status`, and
+    get_ansible_run(last_run_id) has its log.
     """
     async with db_session() as db:
         scheds = await _schedules(db)

steward/templates/ansible/schedules.html

@@ -38,13 +38,18 @@
     <td style="font-size:0.85rem;" title="{{ s.inventory_scope }}">{{ row.scope_label }}</td>
     <td style="font-size:0.85rem;">{{ fmt_interval(s.interval_seconds) }}</td>
     <td style="font-size:0.82rem;">
-      {% if s.last_error %}
-        <span style="color:var(--red);" title="{{ s.last_error }}">launch error</span>
-      {% elif row.last_run %}
+      {% if row.last_run %}
         {% set sc = {"queued":"var(--text-dim)","running":"var(--yellow)","success":"var(--green)","failed":"var(--red)","interrupted":"var(--orange)","cancelled":"var(--orange)"}.get(row.last_run.status.value, "var(--text-muted)") %}
         <a href="/ansible/runs/{{ row.last_run.id }}" style="color:{{ sc }};font-weight:500;">{{ row.last_run.status.value }}</a>
-        <span style="color:var(--text-dim);">· {{ s.last_run_at.strftime("%m-%d %H:%M") }}</span>
-      {% else %}
+        <span style="color:var(--text-dim);">· {{ row.last_run.started_at.strftime("%m-%d %H:%M") }}</span>
+      {% endif %}
+      {% if s.last_error %}
+        {# With a run beside it, the last firing was skipped because that run was
+           still going (#5158); without one, the firing couldn't start a run. #}
+        <div style="color:{{ 'var(--text-muted)' if row.last_run else 'var(--red)' }};">
+          {{ s.last_error }} · {{ s.last_run_at.strftime("%m-%d %H:%M") }}
+        </div>
+      {% elif not row.last_run %}
         <span style="color:var(--text-dim);">never</span>
       {% endif %}
     </td>