From 71a7af5b3f7c4d203769b43c90a249bb803dd42b Mon Sep 17 00:00:00 2001 From: oskar Date: Wed, 26 Aug 2026 21:05:38 +0200 Subject: [PATCH] fix(control-plane): unwedge incidents that never get service_healthy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _resolve_incident() only ever fires from process_event() on a service_healthy/service_recovered event. A service that is removed, renamed, or was only ever a one-off test never emits that event again, so its incident stays "active" in world/incidents.json forever — this is what left 5 incidents wedged on VPS until a manual on-node edit during the 2026-08-26 recon session (docs/sessions/2026-08-26.md). Two independent unwedging mechanisms, both in observer._prune_stale_world (runs every cycle, so no new event is required to trigger either): (a) Time-based fallback: any active incident with last_occurrence older than INCIDENT_STALE_RESOLVE_SECS (env, default 24h) auto-resolves with resolved_reason="auto_stale_no_events_24h". Unlike the existing orphan case (Case 2, 5-min guard, only unlinked incidents), this also clears a service's lingering incident_id link — that link is exactly what a decommissioned service's incident never gets a chance to clear via the normal event path. (b) Manual path: an operator touches world/resolve-requests/; the observer consumes the flag file each cycle, force-resolves with resolved_reason= "manual_operator", and always removes the flag (even for an unknown/already-resolved id) so a mistyped flag can't sit forever looking unprocessed. Chose a flag file over adding a mutation endpoint to operator_ui.py: /action/mutate only knows actions//.json, there is no incidents equivalent, and world/incidents.json is exclusively observer-owned (rewritten wholesale every cycle by _save_world) — a second writer (the HTTP handler thread) would race the observer's own writes. A flag file needs no new HTTP surface and reuses the same "operator drops a file, the owning process consumes it" pattern the actions pending/approved queue already uses. Smaller diff, no new attack surface on a server with no auth on writes. Tests added to test_incident_lifecycle.py: stale-resolve past the threshold (service still linked), negative case (fresh active incident stays active), configurable threshold, manual-flag resolve + flag removal, flag for an unknown incident, flag for an already-resolved incident. Full control-plane suite: 179 passed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017WDKj5LRY8vdQMx57dfNnu --- scripts/observer/observer.py | 105 ++++++++++++ .../tests/test_incident_lifecycle.py | 161 ++++++++++++++++++ 2 files changed, 266 insertions(+) diff --git a/scripts/observer/observer.py b/scripts/observer/observer.py index d2d8656..4d349d4 100644 --- a/scripts/observer/observer.py +++ b/scripts/observer/observer.py @@ -129,6 +129,24 @@ WORLD_DIR = Path(RUNTIME_PATH) / "world" OBSERVER_STATE_FILE = STATE_DIR / "observer_checkpoint.json" FAILED_EVENTS_DIR = STATE_DIR / "observer_failed_events" +# Manual incident-resolve flag drop: an operator (or a future UI action) touches +# world/resolve-requests/ and the observer picks it up here, once +# per loop iteration — see _process_resolve_requests. Chosen over adding a new +# operator-ui/API mutation endpoint (see COMMIT 1b rationale in the commit +# message): /action/mutate only knows about actions//.json, there is +# no equivalent for incidents.json, and incidents.json is written exclusively by +# the observer (world/*.json is observer-owned, overwritten wholesale every +# cycle by _save_world) — a second writer would race it. A flag file needs no +# new HTTP surface, no new writer of world/incidents.json, and follows the same +# "operator drops a file, the owning process consumes it" pattern the actions +# pending/approved queue already uses. +RESOLVE_REQUESTS_DIR = WORLD_DIR / "resolve-requests" + +# Time-based fallback for incidents that can never receive the service_healthy +# event _resolve_incident() waits for (service removed/renamed/decommissioned +# from services.yaml, or a one-off test service) — see _prune_stale_world Case 3. +INCIDENT_STALE_RESOLVE_SECS = int(os.getenv("INCIDENT_STALE_RESOLVE_SECS", str(24 * 3600))) + REPO_ROOT = Path(__file__).parent.parent.parent INVENTORY_TOPOLOGY = REPO_ROOT / "inventory" / "topology.yaml" @@ -258,6 +276,7 @@ class Observer: EVENTS_DIR.mkdir(parents=True, exist_ok=True) LOGS_DIR.mkdir(parents=True, exist_ok=True) FAILED_EVENTS_DIR.mkdir(parents=True, exist_ok=True) + RESOLVE_REQUESTS_DIR.mkdir(parents=True, exist_ok=True) def _quarantine_event_file(self, file_path: str, node_dir: str, exc: Exception) -> None: """Move an unreadable/unprocessable event out of the hot path.""" @@ -514,6 +533,55 @@ class Observer: node_name, event_liveness, "up" if prom_up else "down", age, ) + def _process_resolve_requests(self, now): + """Manual incident-resolve path (COMMIT 1b): consume flag files dropped + at world/resolve-requests/. + + There is no operator-ui/API mutation endpoint for incidents (only for + actions, via /action/mutate — see RESOLVE_REQUESTS_DIR comment), and + incidents.json is exclusively observer-owned, so a flag file the + observer itself polls is the smallest addition that lets an operator + force-resolve a wedged incident without editing world/incidents.json + by hand. + + The flag is always removed, even when the incident_id is unknown or + already resolved, so a stale/mistyped/duplicate flag cannot sit + forever looking like it hasn't been picked up yet. + """ + if not RESOLVE_REQUESTS_DIR.exists(): + return + for flag_path in RESOLVE_REQUESTS_DIR.iterdir(): + if not flag_path.is_file(): + continue + incident_id = flag_path.name + inc = self.world_state["incidents"].get(incident_id) + if inc and inc.get("status") == "active": + logger.info( + f"Manually resolving incident {incident_id} " + f"(service={inc.get('service')}, node={inc.get('node')}) " + f"via resolve-request flag" + ) + inc["status"] = "resolved" + inc["resolved_at"] = now + inc["resolved_reason"] = "manual_operator" + svc_key = f"{inc.get('node')}/{inc.get('service')}" + svc = self.world_state["services"].get(svc_key) + if svc and svc.get("incident_id") == incident_id: + svc["incident_id"] = None + elif inc: + logger.info( + f"Resolve-request flag for {incident_id} ignored " + f"(status already '{inc.get('status')}') — removing flag" + ) + else: + logger.warning( + f"Resolve-request flag for unknown incident {incident_id} — removing flag" + ) + try: + flag_path.unlink() + except OSError as exc: + logger.error(f"Failed to remove resolve-request flag {flag_path}: {exc}") + def _prune_stale_world(self): """Remove world-state entries for nodes absent from the topology inventory. @@ -599,6 +667,15 @@ class Observer: # the authoritative decision above is already committed. self._shadow_compare_liveness(node_name, liveness, node_info, prom_liveness_map, now) + # Manual resolve-request flags: independent of the linked/orphan + # distinction below (an operator can force-resolve either kind), so + # process before the auto-resolve cases. Own try/except: a bad flag + # file must not block the auto-resolve cases below from running. + try: + self._process_resolve_requests(now) + except Exception as exc: + logger.error(f"Error processing resolve-request flags: {exc}") + try: # Collect incident_ids currently referenced by any service entry. linked_ids: set = { @@ -650,6 +727,34 @@ class Observer: inc["status"] = "resolved" inc["resolved_at"] = now + # Case 3 — long-idle active incident, time-based fallback (COMMIT 1a). + # _resolve_incident() only fires on a service_healthy/service_recovered + # event, which a removed/renamed/decommissioned or one-off test + # service will never emit again — such an incident would otherwise + # stay "active" forever, unlike Case 2 above it is NOT limited to + # orphaned (unlinked) incidents: a service entry can keep pointing at + # incident_id indefinitely too (that lingering link is exactly the + # case _resolve_incident() never got a chance to clear), so this + # checks every active incident regardless of linked_ids. + for inc_id, inc in self.world_state["incidents"].items(): + if inc.get("status") != "active": + continue + age = now - _parse_ts(inc.get("last_occurrence")) + if age > INCIDENT_STALE_RESOLVE_SECS: + logger.info( + f"Auto-resolving stale incident {inc_id} " + f"(service={inc.get('service')}, node={inc.get('node')}): " + f"no events for {int(age)}s " + f"(threshold {INCIDENT_STALE_RESOLVE_SECS}s)" + ) + inc["status"] = "resolved" + inc["resolved_at"] = now + inc["resolved_reason"] = "auto_stale_no_events_24h" + svc_key = f"{inc.get('node')}/{inc.get('service')}" + svc = self.world_state["services"].get(svc_key) + if svc and svc.get("incident_id") == inc_id: + svc["incident_id"] = None + except Exception as exc: logger.error(f"Error during incident auto-resolve in _prune_stale_world: {exc}") diff --git a/services/control-plane/tests/test_incident_lifecycle.py b/services/control-plane/tests/test_incident_lifecycle.py index 0053d50..f740648 100644 --- a/services/control-plane/tests/test_incident_lifecycle.py +++ b/services/control-plane/tests/test_incident_lifecycle.py @@ -58,6 +58,14 @@ def _redirect_observer_paths(tmp_path, monkeypatch): monkeypatch.setattr(obs_mod, "REPO_ROOT", repo) monkeypatch.setattr(obs_mod, "FAILED_EVENTS_DIR", state / "observer_failed_events") monkeypatch.setattr(obs_mod, "OBSERVER_STATE_FILE", state / "observer_checkpoint.json") + # RESOLVE_REQUESTS_DIR is derived from WORLD_DIR *at import time* — same + # footgun as OBSERVER_STATE_FILE above, redirect explicitly. + monkeypatch.setattr(obs_mod, "RESOLVE_REQUESTS_DIR", world / "resolve-requests") + # INCIDENT_STALE_RESOLVE_SECS is read from env at import time; restore the + # default after each test since individual tests monkeypatch it directly + # (module attribute, not covered by monkeypatch.setattr auto-restore across + # the *value* tests assign mid-test via `obs_mod.X = ...`). + monkeypatch.setattr(obs_mod, "INCIDENT_STALE_RESOLVE_SECS", 24 * 3600) def _make_observer_simple(tmp_path: Path) -> Observer: @@ -754,3 +762,156 @@ def test_event_ts_from_path_mtime_fallback(tmp_path): ts = _event_ts_from_path(str(p)) assert isinstance(ts, int) assert ts > 0 + + +# --------------------------------------------------------------------------- +# 10. Wedged-incident fix (2026-08-26): _resolve_incident() only fires on +# service_healthy/service_recovered, which a removed/renamed/decommissioned +# service never sends again. Two independent unwedging mechanisms: +# (a) time-based fallback — Case 3 in _prune_stale_world +# (b) manual flag file — world/resolve-requests/ +# --------------------------------------------------------------------------- + +def test_prune_resolves_stale_active_incident_after_threshold(tmp_path, monkeypatch): + """An active incident with no events for > INCIDENT_STALE_RESOLVE_SECS + auto-resolves, even though the service still links to it (the case + _resolve_incident() never got a chance to clear because no + service_healthy event will ever arrive for a decommissioned service).""" + obs = _make_observer_simple(tmp_path) + import observer.observer as obs_mod + monkeypatch_threshold = 3600 + monkeypatch.setattr(obs_mod, "INCIDENT_STALE_RESOLVE_SECS", monkeypatch_threshold) + + inc_id = "inc-999-piha-decommissioned-svc" + obs.world_state["services"]["piha/decommissioned-svc"] = { + "node": "piha", "service": "decommissioned-svc", + "status": "unhealthy", "last_check": None, + "incident_id": inc_id, + } + obs.world_state["incidents"][inc_id] = { + "id": inc_id, "status": "active", "node": "piha", + "service": "decommissioned-svc", + "last_occurrence": time.time() - monkeypatch_threshold - 60, + } + + obs._prune_stale_world() + + assert obs.world_state["incidents"][inc_id]["status"] == "resolved" + assert obs.world_state["incidents"][inc_id]["resolved_reason"] == "auto_stale_no_events_24h" + assert obs.world_state["services"]["piha/decommissioned-svc"]["incident_id"] is None + + +def test_prune_does_not_resolve_fresh_active_incident(tmp_path): + """Negative case: a fresh active incident (well within the threshold) must + stay active — the fallback must not resolve genuinely ongoing incidents.""" + obs = _make_observer_simple(tmp_path) + + inc_id = "inc-1000-piha-outline" + obs.world_state["services"]["piha/outline"] = { + "node": "piha", "service": "outline", + "status": "unhealthy", "last_check": None, + "incident_id": inc_id, + } + obs.world_state["incidents"][inc_id] = { + "id": inc_id, "status": "active", "node": "piha", "service": "outline", + "last_occurrence": time.time() - 60, # 1 minute ago — nowhere near stale + } + + obs._prune_stale_world() + + assert obs.world_state["incidents"][inc_id]["status"] == "active" + assert "resolved_reason" not in obs.world_state["incidents"][inc_id] + + +def test_prune_stale_resolve_respects_custom_threshold_env(tmp_path, monkeypatch): + """INCIDENT_STALE_RESOLVE_SECS is configurable — a lower threshold resolves + an incident that the default 24h threshold would still consider active. + + Linked to a (non-healthy) service so the orphan path (Case 2, 5-min guard) + cannot also explain the resolution — this isolates Case 3.""" + obs = _make_observer_simple(tmp_path) + import observer.observer as obs_mod + monkeypatch.setattr(obs_mod, "INCIDENT_STALE_RESOLVE_SECS", 600) # 10 minutes + + inc_id = "inc-1001-piha-flaky-test-svc" + obs.world_state["services"]["piha/flaky-test-svc"] = { + "node": "piha", "service": "flaky-test-svc", + "status": "unhealthy", "last_check": None, + "incident_id": inc_id, + } + obs.world_state["incidents"][inc_id] = { + "id": inc_id, "status": "active", "node": "piha", "service": "flaky-test-svc", + "last_occurrence": time.time() - 900, # 15 min ago > 10 min threshold + } + + obs._prune_stale_world() + + assert obs.world_state["incidents"][inc_id]["status"] == "resolved" + assert obs.world_state["incidents"][inc_id]["resolved_reason"] == "auto_stale_no_events_24h" + + +def test_resolve_request_flag_resolves_active_incident_and_is_removed(tmp_path): + """Manual path: touching world/resolve-requests/ force-resolves + an active incident and the flag is consumed (removed) in the same cycle.""" + obs = _make_observer_simple(tmp_path) + import observer.observer as obs_mod + + inc_id = "inc-1002-vps-gokapi" + obs.world_state["services"]["vps/gokapi"] = { + "node": "vps", "service": "gokapi", + "status": "unhealthy", "last_check": None, + "incident_id": inc_id, + } + obs.world_state["incidents"][inc_id] = { + "id": inc_id, "status": "active", "node": "vps", "service": "gokapi", + "last_occurrence": time.time() - 30, # fresh — would NOT auto-resolve + } + + flag = obs_mod.RESOLVE_REQUESTS_DIR / inc_id + flag.parent.mkdir(parents=True, exist_ok=True) + flag.touch() + + obs._prune_stale_world() + + assert obs.world_state["incidents"][inc_id]["status"] == "resolved" + assert obs.world_state["incidents"][inc_id]["resolved_reason"] == "manual_operator" + assert obs.world_state["services"]["vps/gokapi"]["incident_id"] is None + assert not flag.exists() + + +def test_resolve_request_flag_for_unknown_incident_is_removed(tmp_path): + """A flag for a nonexistent incident_id must not crash and must be removed + (otherwise a mistyped/stale flag sits forever looking unprocessed).""" + obs = _make_observer_simple(tmp_path) + import observer.observer as obs_mod + + flag = obs_mod.RESOLVE_REQUESTS_DIR / "inc-does-not-exist" + flag.parent.mkdir(parents=True, exist_ok=True) + flag.touch() + + obs._prune_stale_world() # must not raise + + assert not flag.exists() + + +def test_resolve_request_flag_for_already_resolved_incident_is_removed(tmp_path): + """A flag for an already-resolved incident is a harmless no-op: removed, + original resolved_reason left untouched.""" + obs = _make_observer_simple(tmp_path) + import observer.observer as obs_mod + + inc_id = "inc-1003-vps-outline" + obs.world_state["incidents"][inc_id] = { + "id": inc_id, "status": "resolved", "node": "vps", "service": "outline", + "resolved_at": time.time() - 10, + "resolved_reason": "manual_operator", + } + flag = obs_mod.RESOLVE_REQUESTS_DIR / inc_id + flag.parent.mkdir(parents=True, exist_ok=True) + flag.touch() + + obs._prune_stale_world() + + assert obs.world_state["incidents"][inc_id]["status"] == "resolved" + assert obs.world_state["incidents"][inc_id]["resolved_reason"] == "manual_operator" + assert not flag.exists()