Commit graph

3 commits

Author SHA1 Message Date
oskar 91db6829f4 fix(control-plane): unique container_restart action_id, no more history overwrite
_generate_recommendation() built container_restart ids as the bare
container-restart-<node>-<service>. 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-<node>-<service>-<unixts> — 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-<node>-<service>, 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-<node>-<service> ids — no observed collision, out of
    scope for this fix (flagged as a latent follow-up below).
  - The HA-specific container-restart-<node>-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/<id>.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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012pjmfPfrF5UYHqki2YwvdG
2026-08-26 22:23:07 +02:00
oskar fbf165fbea fix(supervisor): route healthcheck_failed to container_restart
healthcheck_failed incidents fell through to redeploy, which is broken as
wired (executor calls deploy-node.sh with arguments it ignores, at a path
that does not exist in the container) — so 3376 healthcheck_failed events
dead-ended with no working remediation (recon D14/D15). A container restart
plausibly heals a failing healthcheck and rides the executor path that
actually works; redeploy returns to the map once etap 2 fixes the executor.

service_unhealthy / deployment_failed / missing_service stay on redeploy —
theoretical until etap 2, kept so drift remains visible in pending actions
(noted in comments). CLAUDE.md routing table updated to match; stale
mqtt_unreachable example in the observer's trigger_type comment refreshed.

Tests: trigger-type recognition and the end-to-end observer→supervisor
reconcile test parametrized over both container_restart triggers, with an
assertion that no redeploy action is also generated. Full control-plane
suite: 147 passed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-29 19:24:01 +02:00
oskar 4bfd6c4429 fix(observer): map containers_not_running to unhealthy status + incident — dead/crash-looping containers created no incident, so supervisor never alerted
node-agent (and stability-agent) emit containers_not_running for exited/dead
and crash-looping containers, but the observer's event->status/incident map
only handled service_recovered/service_healthy/service_unhealthy/
healthcheck_failed. containers_not_running fell through: status stayed at its
last "healthy" value and no incident was opened, so the supervisor saw no drift
and generated no action — a dead container produced ZERO operator alerts
(matches the "action queue empty despite failures" symptom).

- containers_not_running now sets status=unhealthy and opens an incident whose
  trigger_type ("containers_not_running") is already in the supervisor's
  CONTAINER_RESTART_TRIGGERS, so remediation (container_restart) fires with no
  supervisor change. Recovery is unchanged: service_healthy resolves the
  incident via the existing svc_key->incident_id link.
- container_restarting / container_state_unexpected (added in 4746ebe) are kept
  intentionally observational — no incident, status not flipped to unhealthy
  (would cause a false redeploy for a transient blip) — but leave a
  last_observation trace so they don't vanish. A real crash-loop still escalates
  via node-agent re-emitting containers_not_running.

Tests: services/control-plane/tests/test_observer_container_events.py — status
+ incident + trigger_type, end-to-end reconcile -> container_restart, recovery
auto-resolve, observational no-incident/trace, idempotent (no incident
multiplication). Full control-plane suite: 117 passed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-22 16:28:31 +02:00