From 71a7af5b3f7c4d203769b43c90a249bb803dd42b Mon Sep 17 00:00:00 2001 From: oskar Date: Wed, 26 Aug 2026 21:05:38 +0200 Subject: [PATCH 1/3] 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() From 91db6829f4c41d88d0fc1cc8a71bd265670aa0ed Mon Sep 17 00:00:00 2001 From: oskar Date: Wed, 26 Aug 2026 21:08:04 +0200 Subject: [PATCH 2/3] fix(control-plane): unique container_restart action_id, no more history overwrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _generate_recommendation() built container_restart ids as the bare container-restart--. Two DIFFERENT incidents for the same node+service (e.g. a generic containers_not_running restart, later followed — after recovery and recurrence — by an unrelated restart for the same service) produced the identical id. Once the first action reached cancelled/completed/failed, the second action's own transition into that same directory silently overwrote the first one's history file. This is exactly what happened 2026-08-26 to a shadow-mode HA-websocket restart colliding with an unrelated 08-06 entry (docs/sessions/2026-08-26.md) — worked around by hand-renaming the file that session. Fix: suffix the id with the triggering incident's started_at — container-restart--- — NOT time.time() at generation call time. reconcile() calls _generate_recommendation() on every loop iteration while the drift persists, and the pending/ approved/running existence check immediately below is what makes that idempotent; it only works if repeated calls for the SAME ongoing incident produce the SAME id. started_at is fixed for an incident's whole life (observer._handle_incident only bumps last_occurrence/occurrence_count on repeat occurrences — see COMMIT-1-adjacent code) and changes only when a genuinely new incident opens for that service, which is exactly "same id while ongoing, different id on recurrence". When the incident record is missing/unlinked, fall back to the bare pre-fix id (container-restart--, no suffix) — NOT time.time(). This is not just a malformed-data corner case: observer._prune_stale_world Case 3 (commit 71a7af5) clears a service's incident_id after 24h of event silence even while the drift is still ongoing, so a live restarting service can naturally hit this path. time.time() would mint a new action_id — and a new pending file — on every single reconcile() tick, which is the exact non-idempotency this commit exists to fix, just via a different trigger. The bare id can't distinguish same-incident from different-incident recurrences the way the suffixed id can, but it is stable across calls, which is what the dedup check actually needs. Scope: only the generic CONTAINER_RESTART_TRIGGERS path (_generate_recommendation). Left unchanged, deliberately: - redeploy-- ids — no observed collision, out of scope for this fix (flagged as a latent follow-up below). - The HA-specific container-restart--homeassistant id used by _generate_ha_container_restart / _generate_ha_shadow_alert / _cancel_ha_container_restart: these three functions rely on an exact-match lookup of that fixed id (cooldown check via _ha_action_recently_completed, and the cancel path finding the specific pending file to move) — adding a suffix there would break both without a broader refactor to prefix-glob lookups. - alert-ha-*/alert-node-* ids: _ha_action_recently_completed also exact-matches these for cooldown dedup; a suffix would defeat cooldown entirely (every occurrence would look "new"). node-agent idempotency gate confirmed unaffected: _already_processed() in node_agent.py does a full-string action_id match against processed-actions/.done, guarding against RE-processing the exact same dispatched action file (e.g. a duplicate rsync delivery) — not against a new action_id for a new occurrence of the same service. A suffixed id is legitimately a new action to node-agent, which is the correct behavior (a genuine new incident should actually restart the container again). Tests: test_supervisor_action_id_uniqueness.py covers (1) repeated _generate_recommendation() calls for the same ongoing incident produce the same id and do not duplicate the pending file, (2) a new incident after the old one completed gets a different id and does not overwrite the old completed record, (3) fallback to the bare pre-fix id when the incident record is missing, (4) that bare fallback id is stable across repeated calls for the same missing-record drift — no duplicate pending file, same as case (1) but for the no-incident path, (5) redeploy ids stay bare. Updated test_observer_container_events.py's end-to-end assertion to match by prefix instead of exact filename. Full control-plane suite: 184 passed; node-agent suite: 70 passed (unchanged, confirming the idempotency gate needed no code change). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012pjmfPfrF5UYHqki2YwvdG --- services/control-plane/src/supervisor.py | 65 ++++++- .../tests/test_observer_container_events.py | 11 +- .../test_supervisor_action_id_uniqueness.py | 181 ++++++++++++++++++ 3 files changed, 249 insertions(+), 8 deletions(-) create mode 100644 services/control-plane/tests/test_supervisor_action_id_uniqueness.py diff --git a/services/control-plane/src/supervisor.py b/services/control-plane/src/supervisor.py index 6de3dc9..b97aa77 100644 --- a/services/control-plane/src/supervisor.py +++ b/services/control-plane/src/supervisor.py @@ -16,6 +16,23 @@ def _atomic_write_json(path: Path, data) -> None: os.fsync(f.fileno()) os.replace(tmp, path) + +def _parse_ts(ts) -> float: + """Return a Unix timestamp float from ts (int/float, or an ISO-8601 string + as stability-agent / events.py write it). Mirrors observer.observer._parse_ts + — duplicated rather than imported to keep supervisor.py's dependency on the + observer module (an unrelated component with its own import-time sys.path + manipulation) at zero. Returns 0.0 on None/unparseable input.""" + if ts is None: + return 0.0 + if isinstance(ts, (int, float)): + return float(ts) + try: + from datetime import datetime + return datetime.fromisoformat(str(ts).replace("Z", "+00:00")).timestamp() + except Exception: + return 0.0 + # Constants and Paths RUNTIME_PATH = os.getenv("RUNTIME_PATH", "/opt/homelab") WORLD_DIR = Path(RUNTIME_PATH) / "world" @@ -433,13 +450,51 @@ class Supervisor: service = drift["service"] trigger_type = drift.get("trigger_type") - # Choose action type first so we can build the stable, deterministic ID. - # Stable IDs mean reconcile is truly idempotent: the same drift always - # produces the same filename, so we never create duplicates even across - # restarts of the supervisor. + # Choose action type first so we can build the ID. + # + # container_restart IDs carry a suffix so two DIFFERENT incidents for + # the same node+service never collide in cancelled/completed/failed + # (2026-08-26: a generic containers_not_running restart and a later + # ha-diag-agent shadow-mode restart both used the bare + # container-restart-piha-homeassistant id and overwrote each other's + # history — worked around manually that session, see + # docs/sessions/2026-08-26.md). + # + # The suffix is the triggering incident's started_at, NOT time.time() + # at generation time: reconcile() calls _generate_recommendation on + # every loop iteration while the drift persists, and the pending/ + # approved/running existence check below is what makes that + # idempotent — it only works if repeated calls for the SAME ongoing + # incident produce the SAME action_id. started_at stays fixed for the + # life of one incident (observer._handle_incident only bumps + # last_occurrence/occurrence_count on repeat occurrences) and changes + # only when a new incident is opened for that service — exactly the + # cases we want "same id" and "different id" for, respectively. + # + # No incident record → bare id (the pre-fix format, no suffix at + # all), NOT time.time(). A missing/unlinked incident_id is not just + # a malformed-data edge case: observer._prune_stale_world Case 3 + # (commit 71a7af5) clears service.incident_id after 24h of event + # silence even while the underlying drift is still ongoing, so this + # path is hit by a live, still-restarting service. time.time() would + # mint a new action_id — and a new pending file — on every single + # reconcile() tick, defeating the dedup check below entirely. The + # bare id has no incident to distinguish "same" from "different" + # occurrences by, but it is at least stable across calls, which is + # what idempotency here actually requires. if trigger_type in CONTAINER_RESTART_TRIGGERS: - action_id = f"container-restart-{node}-{service}" + incident_id = self.actual_state["services"].get(drift["svc_key"], {}).get("incident_id") + incident = self.actual_state["incidents"].get(incident_id, {}) if incident_id else {} + started_ts = int(_parse_ts(incident.get("started_at"))) + if started_ts: + action_id = f"container-restart-{node}-{service}-{started_ts}" + else: + action_id = f"container-restart-{node}-{service}" else: + # redeploy IDs stay bare (node-service) — out of scope for this + # fix (see commit message: no observed collision here yet), and + # _cancel_resolved_pending_actions/_ha_action_recently_completed + # do not key off redeploy ids so nothing here depends on it. action_id = f"redeploy-{node}-{service}" # Skip if an action for this ID is already live in any active state diff --git a/services/control-plane/tests/test_observer_container_events.py b/services/control-plane/tests/test_observer_container_events.py index c2e80a0..9814997 100644 --- a/services/control-plane/tests/test_observer_container_events.py +++ b/services/control-plane/tests/test_observer_container_events.py @@ -178,10 +178,15 @@ def test_supervisor_generates_container_restart_from_observer_output( sup = Supervisor() sup.reconcile() - action_id = "container-restart-piha-paperless" - action_path = actions / "pending" / f"{action_id}.json" - assert action_path.exists(), "supervisor did not generate a container_restart" + # action_id carries a per-incident timestamp suffix (2026-08-26 fix: two + # different incidents for the same node/service must never collide in + # cancelled/completed/failed — see supervisor._generate_recommendation), + # so match by prefix rather than an exact id. + matches = list((actions / "pending").glob("container-restart-piha-paperless-*.json")) + assert len(matches) == 1, "supervisor did not generate exactly one container_restart" + action_path = matches[0] action = json.loads(action_path.read_text()) + assert action["action_id"] == action_path.stem assert action["type"] == "container_restart" assert action["node"] == "piha" assert action["service"] == "paperless" diff --git a/services/control-plane/tests/test_supervisor_action_id_uniqueness.py b/services/control-plane/tests/test_supervisor_action_id_uniqueness.py new file mode 100644 index 0000000..703b62e --- /dev/null +++ b/services/control-plane/tests/test_supervisor_action_id_uniqueness.py @@ -0,0 +1,181 @@ +"""action_id uniqueness for container_restart (2026-08-26 fix). + +Before this fix, _generate_recommendation() built container_restart action +ids as the bare `container-restart--` — no timestamp, no +incident reference. Two DIFFERENT incidents for the same node+service (e.g. a +generic containers_not_running restart followed, after recovery and a later +recurrence, by another restart) produced the identical id. Once the first +action reached cancelled/completed/failed, the second action's move into that +same directory overwrote the first one's history file outright — this is +exactly what happened on 2026-08-26 to a shadow-mode HA-websocket restart +colliding with an unrelated 08-06 entry (docs/sessions/2026-08-26.md), worked +around manually that session by hand-renaming the file. + +The fix anchors the id's suffix to the triggering incident's `started_at` +instead of wall-clock time-of-generation, so: + - repeated reconcile() calls for the SAME ongoing incident keep producing + the SAME action_id (required for the pending/approved/running dedup + check in _generate_recommendation to still work — see its comment), and + - a NEW incident (after the old one resolved) gets a NEW id, so its + eventual cancelled/completed/failed file cannot collide with the old + incident's. +""" +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent / "src")) +import supervisor as supervisor_module # noqa: E402 +from supervisor import Supervisor, CONTAINER_RESTART_TRIGGERS # noqa: E402 + + +@pytest.fixture +def sup(tmp_path, monkeypatch): + actions = tmp_path / "actions" + events = tmp_path / "events" + world = tmp_path / "world" + repo = tmp_path / "repo" + for d in (actions, events, world, repo / "inventory", repo / "hosts"): + d.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(supervisor_module, "ACTIONS_DIR", actions) + monkeypatch.setattr(supervisor_module, "EVENTS_DIR", events) + monkeypatch.setattr(supervisor_module, "WORLD_DIR", world) + monkeypatch.setattr(supervisor_module, "REPO_ROOT", repo) + s = Supervisor() + # _generate_recommendation only reads actual_state — bypass disk-backed + # _load_desired_state()/_load_actual_state() and set it directly. + s.actual_state = {"services": {}, "nodes": {}, "incidents": {}} + return s + + +def _drift(node, service, trigger_type="containers_not_running"): + return { + "type": "unhealthy_service", + "svc_key": f"{node}/{service}", + "node": node, + "service": service, + "status": "unhealthy", + "trigger_type": trigger_type, + } + + +def _seed_incident(sup, node, service, incident_id, started_at, status="active"): + svc_key = f"{node}/{service}" + sup.actual_state["services"][svc_key] = { + "node": node, "service": service, "status": "unhealthy", + "incident_id": incident_id, + } + sup.actual_state["incidents"][incident_id] = { + "id": incident_id, "status": status, + "trigger_type": "containers_not_running", + "started_at": started_at, + "last_occurrence": started_at, + } + + +def _pending(tmp_path): + return list((tmp_path / "actions" / "pending").glob("*.json")) + + +def test_repeated_calls_for_same_incident_produce_same_action_id_no_duplicate(sup, tmp_path): + """Simulates reconcile() calling _generate_recommendation() on every loop + iteration while the drift persists: must not spam a new pending action + each time.""" + _seed_incident(sup, "piha", "paperless", "inc-1000-piha-paperless", started_at=1000) + drift = _drift("piha", "paperless") + + sup._generate_recommendation(drift) + sup._generate_recommendation(drift) + sup._generate_recommendation(drift) + + pending = _pending(tmp_path) + assert len(pending) == 1, f"expected exactly one pending action, got {[p.name for p in pending]}" + assert pending[0].name == "container-restart-piha-paperless-1000.json" + + +def test_new_incident_after_old_completed_gets_different_action_id(sup, tmp_path): + """A second, later incident for the same node/service must not collide + with the first incident's already-completed action file.""" + _seed_incident(sup, "piha", "paperless", "inc-1000-piha-paperless", started_at=1000) + drift = _drift("piha", "paperless") + sup._generate_recommendation(drift) + + first_action_path = tmp_path / "actions" / "pending" / "container-restart-piha-paperless-1000.json" + assert first_action_path.exists() + + # First incident's action ran to completion (moved out of pending/ by the + # executor in real life; simulate that here). + completed_dir = tmp_path / "actions" / "completed" + completed_dir.mkdir(parents=True, exist_ok=True) + first_action = json.loads(first_action_path.read_text()) + first_action["status"] = "completed" + (completed_dir / first_action_path.name).write_text(json.dumps(first_action)) + first_action_path.unlink() + + # A NEW incident recurs later for the same node+service (old one resolved + # in between — new incident_id, new started_at). + _seed_incident(sup, "piha", "paperless", "inc-2000-piha-paperless", started_at=2000) + sup._generate_recommendation(drift) + + second_action_path = tmp_path / "actions" / "pending" / "container-restart-piha-paperless-2000.json" + assert second_action_path.exists() + # The first incident's completed record must be untouched — not overwritten. + assert json.loads((completed_dir / "container-restart-piha-paperless-1000.json").read_text())["status"] == "completed" + + +def test_fallback_to_bare_id_when_incident_record_missing(sup, tmp_path): + """Malformed/missing incident data (incident_id set on the service but no + matching record in incidents.json) must still produce a usable + (non-crashing) action_id, not block remediation. The fallback must be the + bare pre-fix id — NOT a time.time() suffix — since this path is hit + naturally (not just on malformed data): observer._prune_stale_world + Case 3 (commit 71a7af5) clears service.incident_id after 24h of event + silence while the drift is still ongoing, so a time.time() suffix would + mint a new id on every reconcile() tick forever.""" + svc_key = "piha/paperless" + sup.actual_state["services"][svc_key] = { + "node": "piha", "service": "paperless", "status": "unhealthy", + "incident_id": "inc-missing", + } + # Deliberately no matching entry in sup.actual_state["incidents"]. + drift = _drift("piha", "paperless") + + sup._generate_recommendation(drift) + + pending = _pending(tmp_path) + assert len(pending) == 1 + assert pending[0].name == "container-restart-piha-paperless.json" + + +def test_fallback_bare_id_stable_across_repeated_calls(sup, tmp_path): + """Same missing-incident-record scenario, but simulating reconcile() + calling _generate_recommendation() on every loop iteration while the + drift persists: must not spam a new pending action each time, exactly + like the has-an-incident-record case above.""" + svc_key = "piha/paperless" + sup.actual_state["services"][svc_key] = { + "node": "piha", "service": "paperless", "status": "unhealthy", + "incident_id": "inc-missing", + } + drift = _drift("piha", "paperless") + + sup._generate_recommendation(drift) + sup._generate_recommendation(drift) + sup._generate_recommendation(drift) + + pending = _pending(tmp_path) + assert len(pending) == 1, f"expected exactly one pending action, got {[p.name for p in pending]}" + assert pending[0].name == "container-restart-piha-paperless.json" + + +def test_redeploy_action_id_stays_bare(sup, tmp_path): + """Non-container_restart drift (redeploy path) is out of scope for this + fix and keeps its existing bare node-service id.""" + drift = _drift("piha", "outline", trigger_type="service_unhealthy") + sup._generate_recommendation(drift) + + assert (tmp_path / "actions" / "pending" / "redeploy-piha-outline.json").exists() From 74ff3ee1e2fa89872205b24d707e72961c2c9e24 Mon Sep 17 00:00:00 2001 From: oskar Date: Wed, 26 Aug 2026 21:09:46 +0200 Subject: [PATCH 3/3] chore(vps): remove gokapi (operator decision 2026-08-26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gokapi was VPS desired state (hosts/vps/services.yaml) with no matching runtime on the node — a real deployment gap left open at the end of the 2026-08-26 recon session ("redeploy-vps-gokapi pozostawiony — realna luka wdrożeniowa", docs/sessions/2026-08-26.md). Operator decision this session: drop it instead of deploying it. Verified zero footprint on VPS: no data, no container, no image, no /opt/homelab/config/gokapi. Removed the desired-state entry from hosts/vps/services.yaml and the services/gokapi/ compose stack. No hosts/vps/runtime/gokapi override existed to remove. Grepped the repo for dangling references: jobs/deploy-runner/tests and services/control-plane/tests use "gokapi" only as an arbitrary example service name in synthetic tmp_path fixtures (not reading the real services/gokapi/ directory) — unaffected, left as-is. Fixed one stale mention in services/control-plane/env.example's example-services comment. kb/ and docs/sessions/ mentions (service doc, cutover runbook, an open backlog item, prior session logs) are historical/ narrative record, not code or active config — left untouched, out of this task's scope; flagged as a follow-up below. Full control-plane (183), node-agent (70), and deploy-runner (44) test suites pass unchanged. Follow-up (not done here — kb/ editing is out of scope for this worktree task): kb/decisions/backlog-aktywne.md still has an open "gokapi: deploy-node VPS rzuca błąd — brakujący .env" entry that is now moot and should be closed/removed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017WDKj5LRY8vdQMx57dfNnu --- hosts/vps/services.yaml | 17 ----------- services/control-plane/env.example | 2 +- services/gokapi/README.md | 5 ---- services/gokapi/docker-compose.yml | 43 --------------------------- services/gokapi/env.example | 47 ------------------------------ services/gokapi/healthcheck.sh | 27 ----------------- services/gokapi/service.yaml | 34 --------------------- 7 files changed, 1 insertion(+), 174 deletions(-) delete mode 100644 services/gokapi/README.md delete mode 100644 services/gokapi/docker-compose.yml delete mode 100644 services/gokapi/env.example delete mode 100755 services/gokapi/healthcheck.sh delete mode 100644 services/gokapi/service.yaml diff --git a/hosts/vps/services.yaml b/hosts/vps/services.yaml index 3dd3f09..5fef7f1 100644 --- a/hosts/vps/services.yaml +++ b/hosts/vps/services.yaml @@ -60,23 +60,6 @@ services: data_path: /opt/homelab/data/fleet-prometheus logs_path: /opt/homelab/logs/fleet-prometheus - gokapi: - role: public-file-share - deployment_model: docker-compose - exposure: public - offline_required: false - depends_on: - local: [] - external: [] - ports: - - name: http - container_port: 53842 - protocol: tcp - runtime: - config_path: /opt/homelab/config/gokapi - data_path: /opt/homelab/data/gokapi - logs_path: /opt/homelab/logs/gokapi - stability-agent: role: node-watchdog # read-only docker.sock watchdog, emits filesystem events # Deploys via its own deploy-local.sh, outside the declarative pipeline diff --git a/services/control-plane/env.example b/services/control-plane/env.example index 53fe9c3..2437ed9 100644 --- a/services/control-plane/env.example +++ b/services/control-plane/env.example @@ -1,6 +1,6 @@ # Copy to .env next to docker-compose.yml (gitignored); docker compose picks # it up automatically. Same convention as services/fleet-prometheus, -# services/llm-gateway, services/gokapi, services/ollama. +# services/llm-gateway, services/ollama. # Tailscale IP of the VPS node. The operator-ui port (18180) is published as # ${TAILSCALE_BIND_IP}:18180:8080 so the mesh-facing bind is never 0.0.0.0. diff --git a/services/gokapi/README.md b/services/gokapi/README.md deleted file mode 100644 index 6b69b3e..0000000 --- a/services/gokapi/README.md +++ /dev/null @@ -1,5 +0,0 @@ -# gokapi - -Lekki self-hosted "Firefox Send" alternative — link do jednego pliku, na zewnątrz, z limitem pobrań/czasu. **Osobny serwis od Nextclouda, celowo.**. - -Dokumentacja: [kb/services/gokapi.md](../../kb/services/gokapi.md) diff --git a/services/gokapi/docker-compose.yml b/services/gokapi/docker-compose.yml deleted file mode 100644 index 1d63b5e..0000000 --- a/services/gokapi/docker-compose.yml +++ /dev/null @@ -1,43 +0,0 @@ -# Gokapi — public file-share (Firefox Send-style), separate from Nextcloud. -# -# Nextcloud stays the private "twierdza" (mesh/kapala.org only). Gokapi is -# the OPPOSITE end of the spectrum on purpose: a small, single-container, -# genuinely public link-share for sending someone outside the mesh a file. -# Runs on VPS (Hetzner, public host) — home nodes stay untouched. -services: - gokapi: - # Pinned to the current stable release (v2.2.4, verified against - # github.com/Forceu/Gokapi releases + Docker Hub f0rc3/gokapi tags on - # 2026-07-09) — never :latest, so upgrades are a deliberate git diff, - # same convention as nextcloud. - image: f0rc3/gokapi:v2.2.4 - container_name: gokapi - restart: unless-stopped - env_file: - - .env - volumes: - # /opt/homelab/data convention. `data` holds uploaded files (churns - # constantly — links expire and get deleted); `config` holds - # config.json + the E2E encryption master key. See README backup note: - # config is the part that actually needs backing up. - - /opt/homelab/data/gokapi/data:/app/data - - /opt/homelab/data/gokapi/config:/app/config - ports: - # PUBLIC reachability is via npm@VPS + share.okit.pl, NOT this bind. - # Bound ONLY to the VPS Tailscale interface (TAILSCALE_BIND_IP), never - # 0.0.0.0 — same defense-in-depth convention as fleet-prometheus: the - # raw port does not exist on the public Hetzner IP (135.181.153.108) - # at all. npm, running as its own container on the same host, reaches - # gokapi via Docker hairpin NAT through this real interface IP - # (loopback would NOT work for that trick — see fleet-prometheus / - # nextcloud for the same pattern). Requires .env (from env.example) - # next to this file at deploy. - - "${TAILSCALE_BIND_IP}:53842:53842" - # Image ships curl (alpine-based), so an in-container check works here — - # unlike vikunja's image, which has neither curl nor wget. - healthcheck: - test: ["CMD", "curl", "-fs", "--max-time", "5", "http://127.0.0.1:53842/"] - interval: 30s - timeout: 10s - retries: 5 - start_period: 15s diff --git a/services/gokapi/env.example b/services/gokapi/env.example deleted file mode 100644 index bd13fe7..0000000 --- a/services/gokapi/env.example +++ /dev/null @@ -1,47 +0,0 @@ -# Gokapi host-local binds + disk-protection tuning — copy to .env (gitignored) -# next to docker-compose.yml and fill in real values. Never commit .env. -# No auth secrets here: Gokapi has no headless admin env vars — the admin -# account is created in the one-time /setup web wizard (see README). - -# Tailscale IP of the VPS node (ubuntu-4gb-hel1-1). Bind the gokapi port -# ONLY to this interface — never 0.0.0.0. The public internet never reaches -# port 53842 directly; share.okit.pl reaches gokapi only through npm@VPS -# (which runs on the same host and connects to this IP via Docker hairpin -# NAT — see docker-compose.yml). Verify when rebuilding the host: -# tailscale ip -4. -TAILSCALE_BIND_IP=100.95.58.48 - -TZ=Europe/Warsaw - -# --- Disk protection ------------------------------------------------------- -# VPS has an 80 GB disk shared with npm, outline, joplin, ai-cluster, -# fleet-prometheus's TSDB, etc. — gokapi is NOT the only tenant, and it has -# no S3 backend (local disk only, by design). Keep uploads small and leave -# headroom so expiring links actually reclaim space before the disk fills. - -# Per-file upload cap in MB. 5120 = 5 GB. Upstream default is 102400 -# (100 GB), which would let a single upload fill the whole disk. -GOKAPI_MAX_FILESIZE=5120 - -# Refuse new uploads once free disk space drops below this many MB. Upstream -# default is 400; raised here for the same reason as above. -GOKAPI_MIN_FREE_SPACE=2048 - -# npm@VPS and gokapi run on the same host; npm reaches gokapi's Tailscale- -# bound port via Docker hairpin NAT, so the connection arrives from the -# docker bridge gateway, not the real client IP. Trust that range so -# X-Forwarded-For / IP logging work correctly — same lesson as nextcloud's -# TRUSTED_PROXIES. Narrow to the exact /24 once the stack is up on the live -# host: docker network inspect gokapi_default. -GOKAPI_TRUSTED_PROXIES=172.16.0.0/12 - -# --- NOT env vars — Gokapi has no headless config for these. Set them in -# the one-time /setup wizard on first boot (see README Cutover checklist): -# - Admin username/password (Authentication step) -# - Storage backend: Local (Storage step — NOT S3, -# per Oskar's decision) -# - Encryption level: End-to-End (Level 3) (Encryption step — -# Oskar wants E2E ON) -# - Default expiry / max downloads: there is NO global default in Gokapi — -# it's chosen per-upload in the web form. Use conservative values (e.g. -# 7 days / 10 downloads) each time to keep the disk from filling. diff --git a/services/gokapi/healthcheck.sh b/services/gokapi/healthcheck.sh deleted file mode 100755 index f4bffd4..0000000 --- a/services/gokapi/healthcheck.sh +++ /dev/null @@ -1,27 +0,0 @@ -#!/bin/bash -# Healthcheck for gokapi (public file-share, https://share.okit.pl via npm@VPS) - -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" - -# The port is bound to the Tailscale interface only, so localhost won't answer. -# Read the bind IP from .env (same file compose uses for the port mapping). -if [ -f "$SCRIPT_DIR/.env" ]; then - # shellcheck disable=SC1091 - source "$SCRIPT_DIR/.env" -fi -BIND_IP="${TAILSCALE_BIND_IP:-127.0.0.1}" - -# Container must be running -if ! docker ps --filter "name=gokapi" --filter "status=running" | grep -qw "gokapi"; then - echo "[FAIL] gokapi container is not running" - exit 1 -fi - -# Web server must answer (setup wizard or login page, either is fine) -if ! curl -sf --max-time 5 "http://${BIND_IP}:53842/" > /dev/null; then - echo "[FAIL] gokapi is not responding on ${BIND_IP}:53842" - exit 1 -fi - -echo "[OK] gokapi is healthy" -exit 0 diff --git a/services/gokapi/service.yaml b/services/gokapi/service.yaml deleted file mode 100644 index 1116228..0000000 --- a/services/gokapi/service.yaml +++ /dev/null @@ -1,34 +0,0 @@ -service: - name: gokapi - owner_node: vps - role: public-file-share # Firefox Send-style link sharing, deliberately separate from Nextcloud - exposure: public # public via npm@VPS (share.okit.pl). The container's own port binds - # to TAILSCALE_BIND_IP only, never 0.0.0.0 — npm is the sole public - # entry point (see docker-compose.yml). Same pattern as vikunja. - dependencies: [] # standalone; npm@VPS proxies to it but gokapi has no upstream deps - ports: - - container: 53842 - host: 53842 - protocol: tcp - healthcheck: - type: http - endpoint: http://localhost:53842/ # setup wizard until first admin login, then the login page - interval: 30s - timeout: 10s - retries: 5 - restart_policy: unless-stopped - persistence: - paths: - - /opt/homelab/data/gokapi/data # uploaded files — ephemeral by design, links expire - - /opt/homelab/data/gokapi/config # config.json + E2E encryption master key — BACK THIS UP - runtime: - config_files: - - .env # host-local binds + disk-protection tuning (gitignored, from env.example) - env_vars: - - TAILSCALE_BIND_IP # required — mesh-only port bind; npm@VPS reaches it via hairpin NAT - - GOKAPI_MAX_FILESIZE # per-file cap in MB — keeps the shared VPS disk from filling - - GOKAPI_MIN_FREE_SPACE # MB headroom before uploads are refused - - GOKAPI_TRUSTED_PROXIES # npm@VPS as seen through Docker hairpin NAT (docker bridge subnet) - # No admin/auth env vars: Gokapi has no headless setup — admin account, - # storage backend, encryption level are all set in the /setup web - # wizard on first boot (deploy-time step, see README).