fix(ip-pool): reconcile stale leases against live clones (§2.8, W1)

A job killed mid-flight never released its IP lease; with only 4 pool addresses a single leak exhausted the pool ("IP pool exhausted after 60s"). IpSlotPool.reconcile() releases leases whose owner clone-dir is gone; vm cleanup runs it only against the real build_vms base. Closes TODO §2.8.
2026-07-23 17:15:23 +02:00
parent b0415f36e4
commit f0d856b644
8 changed files with 263 additions and 3 deletions
@@ -299,7 +299,7 @@ Cooldown: max 3 restart/h via JSON state file `F:\CI\State\runner-restart-log.js
 Oltre il limite: EventId 1004 (Error) + webhook `:sos:` senza restart. Webhook opzionale
 Discord/Gitea identico a `Watch-DiskSpace.ps1`.

-### 2.8 [P2] [ ] IP-pool auto-riconciliazione vs clone reali — APERTO (2026-06-07)
+### 2.8 [P2] [x] IP-pool auto-riconciliazione vs clone reali — CHIUSO (2026-07-23)

 `ip-pool.json` non viene riconciliato contro i clone vivi all'avvio. Un job
 killato mid-flight (SIGKILL/crash) **leaka il lease in modo permanente**; con
@@ -312,6 +312,16 @@ mai rilasciati. Workaround manuale: reset a `{}` con `build-vms/` vuoto.
 cui clone-dir non esiste più in `CloneBaseDir`. Opzionale: allargare il pool
 (es. 201–210) per dare margine oltre parallelism. Aggiungere test.

+**Risolto 2026-07-23 (W1 del web-UI plan)**: `IpSlotPool.reconcile(live_owners)`
+rilascia i lease il cui owner (== nome clone-dir) non è più fra le sotto-dir di
+`CloneBaseDir`; `operations.vms.reconcile_ip_leases(clone_base, pool)` calcola i
+`live_owners` e delega. `vm cleanup` lo invoca **solo** quando opera sul clone
+base reale (`base_path == config.paths.build_vms`) — un `--clone-base-dir`
+custom NON riconcilia (i lease appartengono al build-vms reale). Supporta
+`--what-if` (dry-run, nessuna scrittura). Test: `test_ip_pool.py` (reconcile),
+`test_operations_vms.py` (reconcile_ip_leases), `test_commands_vm.py` (CLI, real
+vs custom base). Allargamento del pool oltre 201–204: non fatto (opzionale).
+
 ### 2.9 [P2] [ ] `retention run` non copre backup/template/ISO — APERTO (2026-07-22)

 `ci-retention-policy` agisce solo su **artifact, log e lease IP**. Su questo host
@@ -18,8 +18,8 @@

 Stato passi (spunta man mano):

- [ ] **W0** — Scaffolding: extra `[web]` in `pyproject.toml`, dir package `operations/` + `web/`, skeleton test. Nessuna logica.
- [ ] **W1** — Layer `operations/vms.py`: estrai la logica da `commands/vm.py` in funzioni tipate che ritornano dataclass. Refactor dei comandi CLI per chiamarle. **Comportamento CLI invariato.**
+- [x] **W0** — Scaffolding: extra `[web]` in `pyproject.toml`, dir package `operations/` + `web/`, skeleton test. Nessuna logica.
+- [x] **W1** — Layer `operations/vms.py`: estrai la logica da `commands/vm.py` in funzioni tipate che ritornano dataclass. Refactor dei comandi CLI per chiamarle. **Comportamento CLI invariato.** (Incluso: seam `VmBackend.list_running()` + fold del fix §2.8 IP-pool leak via `IpSlotPool.reconcile` / `operations.vms.reconcile_ip_leases`.)
 - [ ] **W2** — Layer `operations/cache.py` + `operations/host.py`: stato cache cargo, purge, disco, runner, backup template. Refactor `retention.py` / `monitor.py`.
 - [ ] **W3** — `operations/audit.py`: append-only JSONL + lettura. Config `[web]` in `config.py`.
 - [ ] **W4** — App web core: `web/app.py` factory + `commands/web.py` (`ci web serve`). Dashboard **read-only** (VM, cache, disco, runner). Ancora **senza auth**, bind localhost.
@@ -27,6 +27,7 @@ from ci_orchestrator.backends import load_backend
 from ci_orchestrator.backends.errors import BackendError, BackendNotAvailable
 from ci_orchestrator.backends.protocol import VmHandle
 from ci_orchestrator.config import load_config
+from ci_orchestrator.ip_pool import IpSlotPool
 from ci_orchestrator.operations import vms as ops_vms

 if TYPE_CHECKING:  # pragma: no cover
@@ -307,6 +308,24 @@ def vm_cleanup(
            f"[vm cleanup] removed stale lock file (age: {lock_age_min} min): {lock_path}"
        )

+    # Reconcile the IP pool against surviving clones (TODO §2.8). Only when
+    # cleaning the *real* clone base — the lease owners are clone names created
+    # under config.paths.build_vms, so reconciling a custom --clone-base-dir
+    # would wrongly release every live lease.
+    if config.ip_pool is not None and base_path.resolve() == config.paths.build_vms.resolve():
+        pool = IpSlotPool(
+            addresses=config.ip_pool.addresses,
+            netmask=config.ip_pool.netmask,
+            gateway=config.ip_pool.gateway,
+            lease_file=config.ip_pool.lease_file,
+        )
+        released = ops_vms.reconcile_ip_leases(base_path, pool, dry_run=what_if)
+        if released:
+            verb = "would release" if what_if else "released"
+            click.echo(
+                f"[vm cleanup] {verb} {len(released)} stale IP lease(s): {', '.join(released)}"
+            )
+
    click.echo("[vm cleanup] done.")


@@ -93,6 +93,32 @@ class IpSlotPool:
                leases[ip] = None
                self._write_leases(leases)

+    def reconcile(self, live_owners: set[str], *, dry_run: bool = False) -> list[str]:
+        """Release leases whose owner is not among *live_owners*.
+
+        Owners are clone names — identical to the clone directory names under
+        ``CloneBaseDir`` (``acquire`` is called with the clone name). A lease
+        whose owner clone no longer exists on disk is a permanent leak: a job
+        killed mid-flight (SIGKILL/crash) never calls :meth:`release`, and with
+        only four pool addresses a single leak exhausts the pool (TODO §2.8).
+
+        Returns the IPs released — or, when *dry_run* is set, the IPs that
+        *would* be released (no write performed).
+        """
+        with _file_lock(self._lock_file):
+            leases = self._read_leases()
+            stale = [
+                ip
+                for ip, owner in leases.items()
+                if owner is not None and owner not in live_owners
+            ]
+            if not stale or dry_run:
+                return stale
+            for ip in stale:
+                leases[ip] = None
+            self._write_leases(leases)
+            return stale
+
    # ── internals ──────────────────────────────────────────────────────────

    def _read_leases(self) -> dict[str, str | None]:
@@ -21,6 +21,7 @@ from ci_orchestrator.backends.protocol import VmHandle
 if TYPE_CHECKING:  # pragma: no cover
    from ci_orchestrator.backends.protocol import VmBackend
    from ci_orchestrator.config import Config
+    from ci_orchestrator.ip_pool import IpSlotPool

 # Matches the default ``vm cleanup --max-age-hours`` threshold; used to flag
 # a live VM as an orphan candidate in ``list_live_vms``.
@@ -302,6 +303,27 @@ def cleanup_orphans(
    return results


+def reconcile_ip_leases(
+    clone_base: Path, pool: IpSlotPool, *, dry_run: bool = False
+) -> list[str]:
+    """Release IP-pool leases whose owner clone directory no longer exists (TODO §2.8).
+
+    ``live_owners`` is the set of sub-directory names directly under
+    ``clone_base`` (each clone dir name equals the lease owner recorded at
+    ``acquire`` time). Any lease pointing at a clone that is gone is released,
+    closing the permanent-leak hole left by a job killed mid-flight.
+
+    Returns the IPs released (or, with *dry_run*, those that would be). If
+    ``clone_base`` cannot be enumerated, returns ``[]`` without touching the
+    lease file — we must not release leases we cannot prove are stale.
+    """
+    try:
+        live_owners = {entry.name for entry in clone_base.iterdir() if entry.is_dir()}
+    except OSError:
+        return []
+    return pool.reconcile(live_owners, dry_run=dry_run)
+
+
 __all__ = [
    "LiveVm",
    "Orphan",
@@ -309,5 +331,6 @@ __all__ = [
    "cleanup_orphans",
    "list_live_vms",
    "list_orphans",
+    "reconcile_ip_leases",
    "remove_vm",
 ]
@@ -395,6 +395,85 @@ def test_vm_cleanup_backend_unavailable(monkeypatch: pytest.MonkeyPatch, tmp_pat
    assert "vmrun not available" in result.output


+def _config_with_pool(base: Path, lease_file: Path) -> Any:
+    from ci_orchestrator.config import BackendConfig, Config, IpPoolConfig, Paths
+
+    return Config(
+        paths=Paths(
+            root=base.parent,
+            templates=base.parent / "templates",
+            build_vms=base,
+            artifacts=base.parent / "artifacts",
+            keys=base.parent / "keys",
+        ),
+        backend=BackendConfig(),
+        ip_pool=IpPoolConfig(
+            addresses=["10.0.0.1", "10.0.0.2"],
+            netmask="255.255.255.0",
+            gateway="10.0.0.254",
+            lease_file=lease_file,
+        ),
+    )
+
+
+def test_vm_cleanup_reconciles_ip_pool_on_real_base(
+    monkeypatch: pytest.MonkeyPatch, tmp_path: Path
+) -> None:
+    """§2.8: cleaning the real clone base releases leases of vanished clones."""
+    from ci_orchestrator.ip_pool import IpSlotPool
+
+    base = tmp_path / "build-vms"
+    base.mkdir()
+    (base / "Clone_live").mkdir()  # still on disk
+
+    lease = tmp_path / "ip-pool.json"
+    pool = IpSlotPool(["10.0.0.1", "10.0.0.2"], "255.255.255.0", "10.0.0.254", lease)
+    pool.acquire("Clone_live")   # 10.0.0.1 — kept
+    pool.acquire("Clone_dead")   # 10.0.0.2 — dir gone, must be released
+
+    monkeypatch.setattr(vm_module, "_make_backend", lambda _v: None)
+    monkeypatch.setattr(vm_module, "load_config", lambda: _config_with_pool(base, lease))
+
+    result = CliRunner().invoke(cli, ["vm", "cleanup", "--clone-base-dir", str(base)])
+    assert result.exit_code == 0, result.output
+    assert "released 1 stale IP lease" in result.output
+    assert "10.0.0.2" in result.output
+
+    import json
+
+    leases = json.loads(lease.read_text())
+    assert leases["10.0.0.1"] == "Clone_live"
+    assert leases["10.0.0.2"] is None
+
+
+def test_vm_cleanup_skips_reconcile_on_custom_base(
+    monkeypatch: pytest.MonkeyPatch, tmp_path: Path
+) -> None:
+    """A custom --clone-base-dir must NOT reconcile the real pool (leases stay)."""
+    from ci_orchestrator.ip_pool import IpSlotPool
+
+    real_base = tmp_path / "build-vms"
+    real_base.mkdir()
+    custom = tmp_path / "elsewhere"
+    custom.mkdir()
+
+    lease = tmp_path / "ip-pool.json"
+    pool = IpSlotPool(["10.0.0.1"], "255.255.255.0", "10.0.0.254", lease)
+    pool.acquire("Clone_dead")  # would be released if the guard misfired
+
+    monkeypatch.setattr(vm_module, "_make_backend", lambda _v: None)
+    monkeypatch.setattr(vm_module, "load_config", lambda: _config_with_pool(real_base, lease))
+
+    result = CliRunner().invoke(cli, ["vm", "cleanup", "--clone-base-dir", str(custom)])
+    assert result.exit_code == 0, result.output
+    assert "stale IP lease" not in result.output
+
+    import json
+
+    leases = json.loads(lease.read_text())
+    assert leases["10.0.0.1"] == "Clone_dead"  # untouched
+
+
 # ── vmware ───────────────────────────────────────────────────────────────────


@@ -115,3 +115,45 @@ def test_non_dict_lease_json_treated_as_empty(tmp_path: Path) -> None:
    pool = make_pool(tmp_path)
    ip = pool.acquire("owner-1")
    assert ip == "10.0.0.1"
+
+
+# ── reconcile (TODO §2.8) ─────────────────────────────────────────────────────
+
+
+def test_reconcile_releases_leases_with_dead_owners(tmp_path: Path) -> None:
+    pool = make_pool(tmp_path)
+    pool.acquire("clone-alive")   # 10.0.0.1
+    pool.acquire("clone-dead")    # 10.0.0.2
+
+    released = pool.reconcile({"clone-alive"})
+
+    assert released == ["10.0.0.2"]
+    leases = json.loads((tmp_path / "ip-pool.json").read_text())
+    assert leases["10.0.0.1"] == "clone-alive"  # kept
+    assert leases["10.0.0.2"] is None           # released
+
+
+def test_reconcile_keeps_all_when_all_owners_live(tmp_path: Path) -> None:
+    pool = make_pool(tmp_path)
+    pool.acquire("clone-a")
+    pool.acquire("clone-b")
+    assert pool.reconcile({"clone-a", "clone-b"}) == []
+    leases = json.loads((tmp_path / "ip-pool.json").read_text())
+    assert leases["10.0.0.1"] == "clone-a"
+    assert leases["10.0.0.2"] == "clone-b"
+
+
+def test_reconcile_dry_run_reports_without_writing(tmp_path: Path) -> None:
+    pool = make_pool(tmp_path)
+    pool.acquire("clone-dead")  # 10.0.0.1
+
+    released = pool.reconcile(set(), dry_run=True)
+
+    assert released == ["10.0.0.1"]
+    leases = json.loads((tmp_path / "ip-pool.json").read_text())
+    assert leases["10.0.0.1"] == "clone-dead"  # untouched
+
+
+def test_reconcile_empty_pool_returns_empty(tmp_path: Path) -> None:
+    pool = make_pool(tmp_path)  # no acquires → all leases None
+    assert pool.reconcile(set()) == []
@@ -445,3 +445,64 @@ def test_remove_vm_dir_removal_failure(monkeypatch: pytest.MonkeyPatch, tmp_path
    monkeypatch.setattr(vms_module, "_try_remove_dir", lambda _p: False)
    result = remove_vm(backend, vmx)  # type: ignore[arg-type]
    assert result.dir_removed is False
+
+
+# ── reconcile_ip_leases (TODO §2.8) ───────────────────────────────────────────
+
+
+def _make_pool(tmp_path: Path) -> Any:
+    from ci_orchestrator.ip_pool import IpSlotPool
+
+    return IpSlotPool(
+        addresses=["10.0.0.1", "10.0.0.2"],
+        netmask="255.255.255.0",
+        gateway="10.0.0.254",
+        lease_file=tmp_path / "ip-pool.json",
+    )
+
+
+def test_reconcile_ip_leases_releases_dead_clone(tmp_path: Path) -> None:
+    from ci_orchestrator.operations.vms import reconcile_ip_leases
+
+    base = tmp_path / "build-vms"
+    base.mkdir()
+    (base / "clone-alive").mkdir()          # survives on disk
+    pool = _make_pool(tmp_path)
+    pool.acquire("clone-alive")             # 10.0.0.1
+    pool.acquire("clone-dead")              # 10.0.0.2 — no dir on disk
+
+    released = reconcile_ip_leases(base, pool)
+
+    assert released == ["10.0.0.2"]
+
+
+def test_reconcile_ip_leases_dry_run_no_write(tmp_path: Path) -> None:
+    from ci_orchestrator.operations.vms import reconcile_ip_leases
+
+    base = tmp_path / "build-vms"
+    base.mkdir()
+    pool = _make_pool(tmp_path)
+    pool.acquire("clone-dead")              # 10.0.0.1, dir absent
+
+    released = reconcile_ip_leases(base, pool, dry_run=True)
+
+    assert released == ["10.0.0.1"]
+    import json
+
+    leases = json.loads((tmp_path / "ip-pool.json").read_text())
+    assert leases["10.0.0.1"] == "clone-dead"  # not written
+
+
+def test_reconcile_ip_leases_unreadable_base_is_noop(tmp_path: Path) -> None:
+    from ci_orchestrator.operations.vms import reconcile_ip_leases
+
+    missing = tmp_path / "does-not-exist"
+    pool = _make_pool(tmp_path)
+    pool.acquire("clone-dead")
+
+    # clone_base cannot be enumerated → must NOT release anything.
+    assert reconcile_ip_leases(missing, pool) == []
+    import json
+
+    leases = json.loads((tmp_path / "ip-pool.json").read_text())
+    assert leases["10.0.0.1"] == "clone-dead"