Wyciek plikow dispatch potwierdzony 2026-08-06 (session log, follow-up #1): LUSTRO re-pullowalo te same dwie akcje co 60 s przez wiele dni, odbijajac sie od bramki idempotencji, i nie zostawilo po sobie ani jednej linii w logach. Przyczyna zlozona z dwoch niezaleznych defektow: 1. Executor tworzyl actions/dispatch/<node>/ z 0o755 (aerbot:aerbot). Rsync-pull z noda uwierzytelnia sie jako inny uzytkownik, bedacy tylko *czlonkiem* tej grupy. --remove-source-files musi zrobic unlink pliku, a unlink wymaga prawa zapisu w katalogu nadrzednym, nie na samym pliku. Zrodlo przezywalo pobranie. (dispatch/piha mialo historycznie 775 i dlatego dzialalo.) 2. node-agent traktowal rc=23 jako benign obok 0 i 24, wiec rsync zglaszal porazke, a agent ja polykal. Executor: _ensure_inbox_dir() = mkdir + bezwarunkowy os.chmod(0o775). chmod jest bezwarunkowy z dwoch powodow: mkdir(mode=) jest maskowany przez umask procesu (przy 0o022 daje dokladnie feralne 0o755), a inboxy zalozone przez wczesniejszy build juz istnieja na flocie z 0o755. Naprawa w miejscu zapisu, a nie skanem przy starcie: jedno idempotentne wywolanie na tej samej sciezce kodu, ktora pisze plik dispatch, wiec nie da sie rozjechac z pisarzami. Blad chmod nie jest fatalny — akcja i tak sie wykonuje, a nieskasowane zrodlo widac teraz po stronie noda. Objete tez actions/deploy/<node>/ (deploy-runner): ten sam wzorzec drenowania tym samym rsync-pullem, ten sam defekt, jedno wywolanie obok. node-agent: klasyfikacja kodow wyjscia zamiast wspolnej listy benign. Weryfikacja empiryczna rsync 3.4.1 pokazala, ze rc=23 pokrywa dwa rozne przypadki, a rozroznia je dopiero stderr: * `change_dir ... No such file or directory` — executor zaklada inbox dopiero przy pierwszym dispatchu, wiec kazdy nod, do ktorego nic nie poszlo, dostaje rc=23 co cykl. DEBUG — inaczej byloby po linii na minute z wiekszosci floty i realny sygnal utonalby w szumie. * `sender failed to remove <plik>: Permission denied` — wlasnie ten wyciek. WARNING z pelnym stderr. Pusty (ale istniejacy) inbox to rc=0, nie 23 — dotychczasowy komentarz w kodzie mowil inaczej. rc=24 zostaje benign (wyscig z executorem piszacym inbox), pozostale kody to teraz ERROR, nie WARNING. Zachowanie funkcjonalne bez zmian: retry i idempotencja dzialaja jak dotad, zmienia sie wylacznie widocznosc. Testy: 4 nowe w test_executor_dispatch.py (oba inboxy 0o775 pod umask 0o022, naprawa istniejacego 0o755 in place, dispatch przezywa nieudany chmod), 5 w test_action_dispatch.py na klasyfikacje rc. Zastapiony test_pull_treats_empty_source_returncodes_as_non_error — kodyfikowal wlasnie to zalozenie, ktore okazalo sie bugiem. Oba zestawy sprawdzone mutacja: bez chmod padaja 3 testy executora, przy starej liscie benign pada test rc=23. node-agent 70 passed, control-plane 173 passed. Refs docs/sessions/2026-08-06.md (follow-up #1) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
315 lines
11 KiB
Python
315 lines
11 KiB
Python
"""Tests for NodeAgent remediation dispatch: pull_dispatched_actions /
|
|
process_dispatched_actions / _execute_dispatched_action.
|
|
|
|
Covers the security gates required by kb/phases/backlog.md "PROJEKT: remediacja
|
|
bez SSH": the agent executes only actions addressed to its own node, refuses
|
|
anything outside the container_restart whitelist, never restarts its own
|
|
container, and treats a repeated dispatch of the same action_id as a no-op.
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
import shutil
|
|
import time
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
import node_agent
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _isolated_runtime_state():
|
|
"""ACTIONS_DIR/STATE_DIR/EVENTS_DIR are a single temp dir shared for the
|
|
whole test session (conftest.py sets RUNTIME_PATH once at import time).
|
|
Clear the dispatch/processed-actions/events state before and after each
|
|
test so tests don't leak into each other or other test modules."""
|
|
def _clear():
|
|
for d in (node_agent.ACTIONS_DIR, node_agent.EVENTS_DIR, node_agent.STATE_DIR):
|
|
if d.exists():
|
|
shutil.rmtree(d)
|
|
d.mkdir(parents=True, exist_ok=True)
|
|
|
|
_clear()
|
|
yield
|
|
_clear()
|
|
|
|
|
|
def _write_dispatch(agent, action_id, node, action_type="container_restart",
|
|
container_name="zigbee2mqtt"):
|
|
inbox = agent._dispatch_inbox_dir()
|
|
inbox.mkdir(parents=True, exist_ok=True)
|
|
path = inbox / f"{action_id}.json"
|
|
path.write_text(json.dumps({
|
|
"action_id": action_id,
|
|
"type": action_type,
|
|
"node": node,
|
|
"service": container_name,
|
|
"container_name": container_name,
|
|
"dispatched_at": time.time(),
|
|
}))
|
|
return path
|
|
|
|
|
|
def _fake_docker_client():
|
|
client = MagicMock()
|
|
container = MagicMock()
|
|
client.containers.get.return_value = container
|
|
return client, container
|
|
|
|
|
|
def _last_action_result_payload(agent):
|
|
events_dir = agent._node_events_dir()
|
|
files = sorted(events_dir.glob("evt-*-action_result-*.json"))
|
|
assert files, "expected an action_result event to have been emitted"
|
|
return json.loads(files[-1].read_text())["payload"]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Node scoping
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_executes_action_addressed_to_own_node(agent):
|
|
agent.docker_client, container = _fake_docker_client()
|
|
_write_dispatch(agent, "act-1", node=agent.node_name)
|
|
|
|
agent.process_dispatched_actions()
|
|
|
|
container.restart.assert_called_once()
|
|
payload = _last_action_result_payload(agent)
|
|
assert payload["action_id"] == "act-1"
|
|
assert payload["success"] is True
|
|
|
|
|
|
def test_refuses_action_addressed_to_another_node(agent):
|
|
agent.docker_client, container = _fake_docker_client()
|
|
_write_dispatch(agent, "act-2", node="some-other-node")
|
|
# Force it into THIS node's inbox directly (simulates a mis-delivery —
|
|
# the dispatch dir is normally already scoped by node name).
|
|
inbox = agent._dispatch_inbox_dir()
|
|
action_file = inbox / "act-2.json"
|
|
data = json.loads(action_file.read_text())
|
|
assert data["node"] == "some-other-node"
|
|
|
|
agent.process_dispatched_actions()
|
|
|
|
container.restart.assert_not_called()
|
|
payload = _last_action_result_payload(agent)
|
|
assert payload["success"] is False
|
|
assert "not '" + agent.node_name + "'" in payload["error"]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Type whitelist
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@pytest.mark.parametrize("bad_type", ["redeploy", "disk_cleanup", "shell_exec", "alert_only"])
|
|
def test_rejects_non_whitelisted_action_types(agent, bad_type):
|
|
agent.docker_client, container = _fake_docker_client()
|
|
_write_dispatch(agent, f"act-{bad_type}", node=agent.node_name, action_type=bad_type)
|
|
|
|
agent.process_dispatched_actions()
|
|
|
|
container.restart.assert_not_called()
|
|
payload = _last_action_result_payload(agent)
|
|
assert payload["success"] is False
|
|
assert "not whitelisted" in payload["error"]
|
|
|
|
|
|
def test_accepts_container_restart(agent):
|
|
assert "container_restart" in node_agent.ALLOWED_DISPATCH_ACTION_TYPES
|
|
assert node_agent.ALLOWED_DISPATCH_ACTION_TYPES == {"container_restart"}
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Self-restart guard
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_refuses_to_restart_itself(agent):
|
|
agent.docker_client, container = _fake_docker_client()
|
|
_write_dispatch(agent, "act-self", node=agent.node_name, container_name="node-agent")
|
|
|
|
agent.process_dispatched_actions()
|
|
|
|
container.restart.assert_not_called()
|
|
agent.docker_client.containers.get.assert_not_called()
|
|
payload = _last_action_result_payload(agent)
|
|
assert payload["success"] is False
|
|
assert "restart itself" in payload["error"]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Idempotency
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_same_action_id_executes_only_once(agent):
|
|
agent.docker_client, container = _fake_docker_client()
|
|
_write_dispatch(agent, "act-dup", node=agent.node_name)
|
|
|
|
agent.process_dispatched_actions()
|
|
assert container.restart.call_count == 1
|
|
|
|
# Dispatch file is consumed after processing (real pipeline: rsync
|
|
# --remove-source-files means it can't be re-pulled either), but even if
|
|
# the SAME action_id is re-delivered, the processed marker must block it.
|
|
_write_dispatch(agent, "act-dup", node=agent.node_name)
|
|
agent.process_dispatched_actions()
|
|
|
|
assert container.restart.call_count == 1, "action_id re-delivery must be a no-op"
|
|
|
|
|
|
def test_dispatch_file_removed_after_processing(agent):
|
|
agent.docker_client, _ = _fake_docker_client()
|
|
action_file = _write_dispatch(agent, "act-cleanup", node=agent.node_name)
|
|
|
|
agent.process_dispatched_actions()
|
|
|
|
assert not action_file.exists()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Docker restart failure surfaces as a failed action_result
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_docker_restart_exception_reports_failure(agent):
|
|
client = MagicMock()
|
|
client.containers.get.side_effect = Exception("no such container")
|
|
agent.docker_client = client
|
|
_write_dispatch(agent, "act-fail", node=agent.node_name)
|
|
|
|
agent.process_dispatched_actions()
|
|
|
|
payload = _last_action_result_payload(agent)
|
|
assert payload["success"] is False
|
|
assert "no such container" in payload["error"]
|
|
|
|
|
|
def test_no_docker_client_reports_failure(agent):
|
|
agent.docker_client = None
|
|
_write_dispatch(agent, "act-nodocker", node=agent.node_name)
|
|
|
|
agent.process_dispatched_actions()
|
|
|
|
payload = _last_action_result_payload(agent)
|
|
assert payload["success"] is False
|
|
assert "Docker SDK unavailable" in payload["error"]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# pull_dispatched_actions: rsync gating (mirrors test_ship_events_to_vps.py)
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_pull_skips_when_vps_host_not_set(agent, monkeypatch):
|
|
monkeypatch.setattr(node_agent, "VPS_EVENTS_HOST", "")
|
|
fake_run = MagicMock()
|
|
monkeypatch.setattr(node_agent.subprocess, "run", fake_run)
|
|
|
|
agent.pull_dispatched_actions()
|
|
|
|
fake_run.assert_not_called()
|
|
|
|
|
|
def test_pull_skips_on_vps_node(agent, monkeypatch):
|
|
agent.node_name = node_agent.VPS_NODE_NAME
|
|
fake_run = MagicMock()
|
|
monkeypatch.setattr(node_agent.subprocess, "run", fake_run)
|
|
|
|
agent.pull_dispatched_actions()
|
|
|
|
fake_run.assert_not_called()
|
|
|
|
|
|
def test_pull_invokes_rsync_pull_direction(agent, monkeypatch):
|
|
captured = {}
|
|
|
|
def fake_run(cmd, **kwargs):
|
|
captured["cmd"] = cmd
|
|
return MagicMock(returncode=0, stderr="")
|
|
|
|
monkeypatch.setattr(node_agent.subprocess, "run", fake_run)
|
|
agent.pull_dispatched_actions()
|
|
|
|
cmd = captured["cmd"]
|
|
assert cmd[0] == "rsync"
|
|
assert "--remove-source-files" in cmd
|
|
# Source (remote VPS) comes before destination (local inbox) — pull, not push.
|
|
remote_arg = f"{node_agent.VPS_EVENTS_USER}@{node_agent.VPS_EVENTS_HOST}:"
|
|
assert any(a.startswith(remote_arg) for a in cmd[:-1])
|
|
assert cmd[-1] == str(agent._dispatch_inbox_dir()) + "/"
|
|
|
|
|
|
# ----------------------------------------------------------------------
|
|
# rsync exit-code classification (dispatch leak, 2026-08-06)
|
|
#
|
|
# rc=23 used to be benign-listed together with 0 and 24, which is why the
|
|
# leak — files fetched but never removed from VPS, so re-pulled every 60 s —
|
|
# produced no log line at all for days. These pin the four outcomes.
|
|
# ----------------------------------------------------------------------
|
|
|
|
# Verbatim rsync 3.4.1 stderr for the two distinct rc=23 causes.
|
|
_STDERR_UNDELETABLE_SOURCE = (
|
|
"rsync: [sender] sender failed to remove act-123.json: Permission denied (13)\n"
|
|
"rsync error: some files/attrs were not transferred "
|
|
"(see previous errors) (code 23) at main.c(1356) [sender=3.4.1]"
|
|
)
|
|
_STDERR_MISSING_INBOX = (
|
|
'rsync: [sender] change_dir "/opt/homelab/actions/dispatch/test-node" '
|
|
"failed: No such file or directory (2)\n"
|
|
"rsync error: some files/attrs were not transferred "
|
|
"(see previous errors) (code 23) at main.c(1356) [sender=3.4.1]"
|
|
)
|
|
|
|
|
|
def _pull_with(agent, monkeypatch, returncode, stderr=""):
|
|
monkeypatch.setattr(
|
|
node_agent.subprocess, "run",
|
|
lambda cmd, **kwargs: MagicMock(returncode=returncode, stderr=stderr),
|
|
)
|
|
agent.pull_dispatched_actions()
|
|
|
|
|
|
def test_pull_rc0_logs_nothing(agent, monkeypatch, caplog):
|
|
with caplog.at_level("DEBUG"):
|
|
_pull_with(agent, monkeypatch, 0)
|
|
|
|
assert "Dispatch pull" not in caplog.text
|
|
|
|
|
|
def test_pull_rc24_vanished_source_stays_benign(agent, monkeypatch, caplog):
|
|
"""A file removed between file-list and transfer is a race with the
|
|
executor writing the inbox, not a fault."""
|
|
with caplog.at_level("WARNING"):
|
|
_pull_with(agent, monkeypatch, 24, "rsync warning: some files vanished")
|
|
|
|
assert caplog.text == ""
|
|
|
|
|
|
def test_pull_rc23_undeletable_source_warns_with_stderr(agent, monkeypatch, caplog):
|
|
"""The leak itself: loud, and carrying the rsync stderr that names it."""
|
|
with caplog.at_level("WARNING"):
|
|
_pull_with(agent, monkeypatch, 23, _STDERR_UNDELETABLE_SOURCE)
|
|
|
|
assert "WARNING" in caplog.text
|
|
assert "rc=23" in caplog.text
|
|
# Full stderr forwarded, so the operator sees which file and why.
|
|
assert "sender failed to remove act-123.json: Permission denied" in caplog.text
|
|
|
|
|
|
def test_pull_rc23_missing_remote_inbox_is_quiet(agent, monkeypatch, caplog):
|
|
"""A node that has never been dispatched to gets rc=23 on every cycle
|
|
because the executor has not created its inbox yet. Warning here would be
|
|
a line a minute on most of the fleet — and would bury the case above."""
|
|
with caplog.at_level("WARNING"):
|
|
_pull_with(agent, monkeypatch, 23, _STDERR_MISSING_INBOX)
|
|
|
|
assert caplog.text == ""
|
|
|
|
|
|
def test_pull_other_returncode_logs_error(agent, monkeypatch, caplog):
|
|
with caplog.at_level("WARNING"):
|
|
_pull_with(agent, monkeypatch, 255, "ssh: connect to host vps port 22: No route to host")
|
|
|
|
assert "ERROR" in caplog.text
|
|
assert "rc=255" in caplog.text
|
|
assert "No route to host" in caplog.text
|