From 1dca438015f95f82c1050e1811f1a9fa043e2949 Mon Sep 17 00:00:00 2001 From: oskar Date: Thu, 27 Aug 2026 14:48:16 +0200 Subject: [PATCH] fix(supervisor): apply started_at suffix to redeploy action_id too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Domknięcie COMMIT 2 z task/incident-resolve-fix (2026-08-26): redeploy-- carried the same latent collision risk as container-restart-- before that fix — two different incidents for the same node+service can still overwrite each other's cancelled/completed/failed history. Verified before applying the same fix: no code anywhere reconstructs `redeploy-{node}-{service}` for an exact-match lookup. _cancel_resolved_pending_actions matches on the node/service *fields* inside each pending file, not the id string; executor.py, node_agent.py and deploy-runner.sh all treat action_id as an opaque string read back from the action's own JSON/marker file. Safe to extend the suffix. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01VMn76yx2CNuHKFVMrYcKWA --- services/control-plane/src/supervisor.py | 40 ++++++++------- .../test_supervisor_action_id_uniqueness.py | 50 +++++++++++++++++-- 2 files changed, 68 insertions(+), 22 deletions(-) diff --git a/services/control-plane/src/supervisor.py b/services/control-plane/src/supervisor.py index b97aa77..8eb0a69 100644 --- a/services/control-plane/src/supervisor.py +++ b/services/control-plane/src/supervisor.py @@ -452,13 +452,15 @@ class 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 + # Both container_restart and redeploy 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). + # docs/sessions/2026-08-26.md). redeploy carries the same latent risk + # via the same code path and got the same fix on 2026-08-27. # # The suffix is the triggering incident's started_at, NOT time.time() # at generation time: reconcile() calls _generate_recommendation on @@ -482,20 +484,22 @@ class Supervisor: # 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: - 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}" + # + # Safe to add the suffix to redeploy ids: verified no code anywhere + # else reconstructs `redeploy-{node}-{service}` for an exact-match + # lookup. _cancel_resolved_pending_actions matches on the node/service + # *fields* inside each pending file, not on the id string; executor.py, + # node_agent.py and deploy-runner.sh all treat action_id as an opaque + # string read back from the action's own JSON/marker file, never + # reconstructed from node+service. + action_prefix = "container-restart" if trigger_type in CONTAINER_RESTART_TRIGGERS else "redeploy" + 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"{action_prefix}-{node}-{service}-{started_ts}" 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}" + action_id = f"{action_prefix}-{node}-{service}" # Skip if an action for this ID is already live in any active state # (pending → approved → running). This prevents re-creation after diff --git a/services/control-plane/tests/test_supervisor_action_id_uniqueness.py b/services/control-plane/tests/test_supervisor_action_id_uniqueness.py index 703b62e..a63371b 100644 --- a/services/control-plane/tests/test_supervisor_action_id_uniqueness.py +++ b/services/control-plane/tests/test_supervisor_action_id_uniqueness.py @@ -1,4 +1,5 @@ -"""action_id uniqueness for container_restart (2026-08-26 fix). +"""action_id uniqueness for container_restart (2026-08-26) and redeploy +(2026-08-27, same latent risk, same fix). Before this fix, _generate_recommendation() built container_restart action ids as the bare `container-restart--` — no timestamp, no @@ -172,10 +173,51 @@ def test_fallback_bare_id_stable_across_repeated_calls(sup, tmp_path): 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.""" +def test_redeploy_action_id_falls_back_to_bare_without_incident(sup, tmp_path): + """Non-container_restart drift (redeploy path) with no linked incident + record keeps the pre-fix bare node-service id, same fallback as + container_restart.""" drift = _drift("piha", "outline", trigger_type="service_unhealthy") sup._generate_recommendation(drift) assert (tmp_path / "actions" / "pending" / "redeploy-piha-outline.json").exists() + + +def test_redeploy_action_id_carries_incident_started_at_suffix(sup, tmp_path): + """Domknięcie COMMIT 2 z task/incident-resolve-fix (2026-08-27): redeploy + ids carry the same started_at suffix as container_restart, closing the + same latent node+service collision risk.""" + _seed_incident(sup, "piha", "outline", "inc-3000-piha-outline", started_at=3000) + drift = _drift("piha", "outline", trigger_type="service_unhealthy") + + sup._generate_recommendation(drift) + + pending = _pending(tmp_path) + assert len(pending) == 1 + assert pending[0].name == "redeploy-piha-outline-3000.json" + + +def test_redeploy_new_incident_after_old_completed_gets_different_action_id(sup, tmp_path): + """Same collision scenario as the container_restart case, for redeploy: + 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", "outline", "inc-3000-piha-outline", started_at=3000) + drift = _drift("piha", "outline", trigger_type="service_unhealthy") + sup._generate_recommendation(drift) + + first_action_path = tmp_path / "actions" / "pending" / "redeploy-piha-outline-3000.json" + assert first_action_path.exists() + + 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() + + _seed_incident(sup, "piha", "outline", "inc-4000-piha-outline", started_at=4000) + sup._generate_recommendation(drift) + + second_action_path = tmp_path / "actions" / "pending" / "redeploy-piha-outline-4000.json" + assert second_action_path.exists() + assert json.loads((completed_dir / "redeploy-piha-outline-3000.json").read_text())["status"] == "completed"