fix(control-plane): unwedge incidents that never get service_healthy
_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/<incident-id>; 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/<status>/<id>.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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WDKj5LRY8vdQMx57dfNnu
This commit is contained in:
oskar
parent
4fa10f0a72
commit
71a7af5b3f
|
|
@ -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/<incident-id> 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/<status>/<id>.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/<incident-id>.
|
||||
|
||||
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}")
|
||||
|
||||
|
|
|
|||
|
|
@ -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/<incident-id>
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
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/<incident-id> 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()
|
||||
|
|
|
|||
Loading…
Reference in a new issue