From 1562a7983af5d28508c61ac52ac340381944a579 Mon Sep 17 00:00:00 2001 From: Michael Sauer Date: Wed, 23 Sep 2026 21:59:09 +0200 Subject: [PATCH] fix(station): Panel liest den Modus aus einer dauerhaften Datei, nicht aus RUN_DIR Fixrunde 1, Befund 3: RUN_DIR/mode ueberlebt nur einen "systemctl restart" (RuntimeDirectoryPreserve=restart) - "ninlight-station off" fuehrt aber einen echten "systemctl stop" aus, der das RuntimeDirectory leert. build_state() fiel danach unbemerkt auf "recovery" zurueck, auch wenn tatsaechlich eine Migration anstand - die Fusszeile haette dann "in Ordnung" gezeigt, obwohl der Migrationssatz leer ist. Ein Docstring behauptete ausdruecklich das Gegenteil. _read_mode_file() liest jetzt eine neue, dauerhafte LAST_MODE_FILE (unter /var/lib/ninlight-station, wie der Bildsatz-Umschalter selbst) statt RUN_DIR/mode; write_mode() im CLI schreibt beide. Ausserdem: _read_imageset() faengt jetzt auch UnicodeDecodeError ab (Fixrunde 1, Kleinigkeit). Ein neuer Test bildet den echten Ablauf nach (Modus schreiben, RUN_DIR wie bei einem echten Stop leeren, pruefen dass der Modus trotzdem stimmt) - der alte Test fuer den Rettungs-Bildsatz liess die Modusdatei ganz weg und pruefte damit nur den zufaellig gleichen Rueckfallwert, nicht die Wirklichkeit. --- tools/station/display/display.py | 73 +++++++++++++------ tools/station/tests/test_render.py | 111 ++++++++++++++++++++--------- 2 files changed, 132 insertions(+), 52 deletions(-) diff --git a/tools/station/display/display.py b/tools/station/display/display.py index 1c9f5fa..7c7351e 100644 --- a/tools/station/display/display.py +++ b/tools/station/display/display.py @@ -67,6 +67,30 @@ UI_FILE = Path(os.environ.get("NINLIGHT_UI_FILE", "/run/ninlight-button/ui.json" # schreibt server.py nichts mehr. IMAGESET_FILE = Path(os.environ.get("NINLIGHT_IMAGESET_FILE", "/var/lib/ninlight-station/imageset")) +# Der Modus, den "recovery on"/"migrate on" zuletzt angefordert haben - fuer die +# Fusszeile GENAUSO dauerhaft gebraucht wie der Bildsatz-Umschalter oben, aus demselben +# Grund und nach demselben Muster (eigene Datei unter /var/lib, NICHT unter RUN_DIR). +# +# Fixrunde 1, Befund 3: RUN_DIR/mode taugt dafuer NICHT. Die Unit setzt +# RuntimeDirectoryPreserve=restart - das erhaelt /run/ninlight-station nur ueber einen +# "systemctl restart" hinweg (das, was "recovery on"/"migrate on" fuer einen bereits +# laufenden Dienst ausloesen). "ninlight-station off" aber STOPPT den Dienst +# (systemctl stop), und GENAU DANN raeumt systemd das RuntimeDirectory leer, "restart" +# deckt einen echten Stop nicht ab. Ein frueherer Docstring hier behauptete das +# Gegenteil ("die Datei ueberlebt das Abschalten") - das war schlicht falsch, und der +# Fehler war unauffaellig, weil eine fehlende Datei auf "recovery" faellt: genau der +# Modus, den viele Tests ohnehin erwarteten, ohne dass die Faelligkeit der Datei +# je geprueft wurde. Die Folge am Geraet waere die falsche Beruhigung, gegen die die +# Fusszeile ueberhaupt gebaut ist - "in Ordnung" vor einer geplanten Migration, obwohl +# der Migrationssatz leer ist, weil das Panel mangels Datei den Rettungssatz geprueft +# haette. +# +# write_mode() im CLI schreibt deshalb NEBEN der ephemeren RUN_DIR/mode (die +# ausschliesslich server.py's naechsten Start betrifft) auch hierher - dieselbe +# Zeichenkette, zwei Ablagen fuer zwei verschiedene Leser mit verschiedenen +# Haltbarkeitsanforderungen. +LAST_MODE_FILE = Path(os.environ.get("NINLIGHT_LAST_MODE_FILE", + "/var/lib/ninlight-station/mode")) # Dieselben Namen und dieselbe Vorgabe wie in server.py und im CLI - im Zweifel der # Satz OHNE Zugangsdaten (dort steht die Begruendung). IMAGESET_DIRS = {"ohne-wlan": "migrate-nocreds", "mit-wlan": "migrate-wlan"} @@ -215,13 +239,19 @@ def _recent_request(status: dict, now: float | None = None) -> bool: return 0 <= age_s <= ACTIVITY_WINDOW_S -def _read_mode_file(run_dir: Path) -> str: - """Modus, in dem der Dienst laut RUN_DIR/mode tatsaechlich gestartet wurde. Fehlt die - Datei oder ist ihr Inhalt kein bekannter Modus, gilt "recovery" - derselbe Vorgabewert - wie in server.py:resolve_mode().""" +def _read_mode_file(path: Path) -> str: + """Zuletzt angeforderter Modus laut `path`. Fehlt die Datei, ist sie unlesbar + (inklusive ungueltigem UTF-8 - eine halb geschriebene/beschaedigte Datei ist nicht + nur per fehlendem Inhalt kaputt), oder ist ihr Inhalt kein bekannter Modus, gilt + "recovery" - derselbe Vorgabewert wie in server.py:resolve_mode(). Wirft nie: eine + kaputte Datei darf weder das Panel noch "off" aufhalten. + + `path` ist bewusst eine Datei, kein Verzeichnis (anders als frueher, wo hier + `run_dir / "mode"` gelesen wurde) - siehe LAST_MODE_FILE oben, warum die Fusszeile + NICHT mehr aus RUN_DIR liest.""" try: - value = (run_dir / "mode").read_text().strip() - except OSError: + value = path.read_text().strip() + except (OSError, UnicodeDecodeError): return "recovery" return value if value in ("recovery", "migration") else "recovery" @@ -275,7 +305,7 @@ def _read_imageset(imageset_path: Path, conf: dict[str, str], mode: str) -> tupl dann gilt "ok", denn eine unbelegte Warnung ist schlechter als keine.""" try: value = imageset_path.read_text().strip() - except OSError: + except (OSError, UnicodeDecodeError): value = "" name = value if value in IMAGESET_DIRS else IMAGESET_DEFAULT image_dir = conf.get("IMAGE_DIR") @@ -320,16 +350,18 @@ def local_ip() -> str: def build_state(run_dir: Path = RUN_DIR, conf_path: Path = CONF_PATH, service_name: str = SERVICE_NAME, - imageset_path: Path = IMAGESET_FILE) -> dict: + imageset_path: Path = IMAGESET_FILE, + last_mode_path: Path = LAST_MODE_FILE) -> dict: """Baut den zusammengefuehrten Zustand, den render.lines()/compose() erwarten. - Modus kommt vorrangig aus RUN_DIR/mode (Vorgabe "recovery", wie + Modus kommt vorrangig aus `last_mode_path` (Vorgabe "recovery", wie server.py:resolve_mode()) - das ist das, was "ninlight-station on"/"migrate on" - zuletzt geschrieben haben und unabhaengig davon verfuegbar, ob/wann der Serverprozess - seit dem letzten Start schon einmal status.json geschrieben hat. status.json selbst - liefert nur served/recent/last_seen; sein "mode"-Feld wird bewusst NICHT als - Modusquelle verwendet (Fixrunde 1: die urspruengliche Fassung hatte den Vorrang - umgekehrt). + zuletzt geschrieben haben, dauerhaft (siehe LAST_MODE_FILE oben - NICHT die ephemere + RUN_DIR/mode, die server.py fuer den naechsten Start liest) und unabhaengig davon + verfuegbar, ob/wann der Serverprozess seit dem letzten Start schon einmal + status.json geschrieben hat. status.json selbst liefert nur served/recent/ + last_seen; sein "mode"-Feld wird bewusst NICHT als Modusquelle verwendet + (Fixrunde 1: die urspruengliche Fassung hatte den Vorrang umgekehrt). Phase/armed_target kommen aus control.json, nur im Migrationsmodus massgeblich - der Rettungsmodus ignoriert control.json ebenso wie server.py das tut. "off" ueberschreibt @@ -342,17 +374,18 @@ def build_state(run_dir: Path = RUN_DIR, conf_path: Path = CONF_PATH, Ob "imageset_ok" den Rettungs- oder den Migrations-Bildsatz prueft, richtet sich nach dem MODUS, in dem die Station beim naechsten Einschalten anlaufen wuerde - also - RUN_DIR/mode, nicht danach, ob sie gerade laeuft. Deshalb wird der Modus HIER, vor der - "ist der Dienst aktiv"-Abfrage, gelesen: er ist auch im Zustand "off" verfuegbar (die - Datei ueberlebt das Abschalten) und genau dort soll die Vorschau ja gelten - "off" - zeigt ja=noch keinen Modus im engeren Sinn, aber der naechste Start faehrt in genau - diesen Modus, und die Fusszeile soll VORHER zeigen, ob dessen Bildsatz bereitliegt. + `last_mode_path`, nicht danach, ob sie gerade laeuft. Deshalb wird der Modus HIER, + vor der "ist der Dienst aktiv"-Abfrage, gelesen: er ist auch im Zustand "off" + verfuegbar (die Datei ist dauerhaft, siehe LAST_MODE_FILE) und genau dort soll die + Vorschau ja gelten - "off" zeigt ja noch keinen Modus im engeren Sinn, aber der + naechste Start faehrt in genau diesen Modus, und die Fusszeile soll VORHER zeigen, + ob dessen Bildsatz bereitliegt. "activity" (nur in Recovery/Migration, siehe _recent_request()) sagt, ob die letzte Anfrage jeder Art hoechstens ACTIVITY_WINDOW_S Sekunden her ist - das Lebenszeichen, das render.blocks() an die Servierzeile anhaengt.""" conf = _read_conf(conf_path) - mode = _read_mode_file(run_dir) + mode = _read_mode_file(last_mode_path) imageset, imageset_ok = _read_imageset(imageset_path, conf, mode) if not _service_active(service_name): diff --git a/tools/station/tests/test_render.py b/tools/station/tests/test_render.py index bada6fb..c14c09e 100644 --- a/tools/station/tests/test_render.py +++ b/tools/station/tests/test_render.py @@ -840,14 +840,15 @@ def test_autooff_line_says_so_when_the_deadline_has_arrived(): def test_build_state_reads_the_autooff_file(tmp_path, monkeypatch): monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("recovery") + (tmp_path / "last-mode").write_text("recovery") conf_path = tmp_path / "station.conf" conf_path.write_text("SSID='ninlight-recovery'\n") # Noch 6 min 1 s - aufgerundet 7, wie es "ninlight-station status" auch rechnet. (tmp_path / "autooff.json").write_text(json.dumps( {"minutes": 30, "deadline": time.time() + 361, "deferrals": 0})) - state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x") + state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x", + last_mode_path=tmp_path / "last-mode") assert state["autooff_minutes"] == 7 assert "Auto-off in 7 min" in render.lines(state) @@ -855,11 +856,12 @@ def test_build_state_reads_the_autooff_file(tmp_path, monkeypatch): def test_build_state_without_autooff_file_has_no_timer_field(tmp_path, monkeypatch): monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("recovery") + (tmp_path / "last-mode").write_text("recovery") conf_path = tmp_path / "station.conf" conf_path.write_text("SSID='ninlight-recovery'\n") - state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x") + state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x", + last_mode_path=tmp_path / "last-mode") assert "autooff_minutes" not in state assert not any("Auto-off" in line for line in render.lines(state)) @@ -913,12 +915,13 @@ def test_a_switched_off_station_shows_no_timer(tmp_path, monkeypatch): def test_control_json_is_ignored_outside_migration_mode(tmp_path, monkeypatch): monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("recovery") + (tmp_path / "last-mode").write_text("recovery") (tmp_path / "control.json").write_text(json.dumps({"phase": "armed", "armed_target": "g95_cw"})) conf_path = tmp_path / "station.conf" conf_path.write_text("SSID='ninlight-recovery'\n") - state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x") + state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x", + last_mode_path=tmp_path / "last-mode") assert state["mode"] == "recovery" assert "phase" not in state @@ -930,12 +933,13 @@ def test_build_state_off_when_service_not_active(tmp_path, monkeypatch): monkeypatch.setattr(display, "_service_active", lambda service_name: False) # Selbst ein noch so frisches/gueltiges status.json darf "off" nicht verhindern - # "ninlight-station off" laesst status.json bewusst unangetastet liegen. - (tmp_path / "mode").write_text("migration") + (tmp_path / "last-mode").write_text("migration") (tmp_path / "status.json").write_text(json.dumps({"mode": "migration", "served": 5})) state = display.build_state(run_dir=tmp_path, conf_path=tmp_path / "station.conf", service_name="x", - imageset_path=tmp_path / "imageset") + imageset_path=tmp_path / "imageset", + last_mode_path=tmp_path / "last-mode") # "off" und sonst NICHTS aus status.json - nur der Bildsatz, der auch im # ausgeschalteten Zustand gilt (und gerade dann gesehen werden soll). @@ -955,9 +959,10 @@ def test_build_state_off_carries_the_local_address(tmp_path, monkeypatch): def test_build_state_running_carries_no_address(tmp_path, monkeypatch): monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("migration") + (tmp_path / "last-mode").write_text("migration") state = display.build_state(run_dir=tmp_path, conf_path=tmp_path / "station.conf", - service_name="x", imageset_path=tmp_path / "imageset") + service_name="x", imageset_path=tmp_path / "imageset", + last_mode_path=tmp_path / "last-mode") assert "ip" not in state @@ -1022,26 +1027,28 @@ def test_served_line_without_an_activity_field_claims_nothing(): def test_build_state_carries_activity_in_recovery_and_migration(tmp_path, monkeypatch): monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("recovery") + (tmp_path / "last-mode").write_text("recovery") (tmp_path / "status.json").write_text(json.dumps({"last_request_ts": None})) conf = tmp_path / "station.conf" conf.write_text("") state = display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=tmp_path / "imageset") + imageset_path=tmp_path / "imageset", + last_mode_path=tmp_path / "last-mode") assert state["activity"] is False def test_build_state_mode_comes_from_mode_file_not_status_json(tmp_path, monkeypatch): - """Der Modus-Vorrang liegt bei RUN_DIR/mode, nicht bei status.json's "mode"-Feld - - status.json kann veraltet sein (Fixrunde 1, Kleinigkeit: der Vorrang war zuerst - umgekehrt).""" + """Der Modus-Vorrang liegt bei der dauerhaften Modusdatei (last_mode_path), nicht + bei status.json's "mode"-Feld - status.json kann veraltet sein (Fixrunde 1, + Kleinigkeit: der Vorrang war zuerst umgekehrt).""" monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("recovery") + (tmp_path / "last-mode").write_text("recovery") (tmp_path / "status.json").write_text(json.dumps({"mode": "migration", "served": 1})) conf_path = tmp_path / "station.conf" conf_path.write_text("SSID='ninlight-recovery'\n") - state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x") + state = display.build_state(run_dir=tmp_path, conf_path=conf_path, service_name="x", + last_mode_path=tmp_path / "last-mode") assert state["mode"] == "recovery" assert state["served"] == 1 @@ -1050,10 +1057,36 @@ def test_build_state_mode_comes_from_mode_file_not_status_json(tmp_path, monkeyp def test_build_state_falls_back_to_recovery_without_mode_file(tmp_path, monkeypatch): monkeypatch.setattr(display, "_service_active", lambda service_name: True) state = display.build_state(run_dir=tmp_path, conf_path=tmp_path / "station.conf", - service_name="x") + service_name="x", last_mode_path=tmp_path / "last-mode") assert state["mode"] == "recovery" +def test_build_state_reads_the_durable_mode_file_even_after_a_real_stop(tmp_path, monkeypatch): + """Fixrunde 1, Befund 3: RUN_DIR/mode ueberlebt nur einen "systemctl restart", NICHT + das "systemctl stop", das "ninlight-station off" ausloest (die Unit setzt + RuntimeDirectoryPreserve=restart, nicht "yes") - ein frueherer Docstring behauptete + das Gegenteil. Dieser Test bildet GENAU diesen Ablauf nach: "migrate on" schreibt + den Modus, RUN_DIR wird dann (wie bei einem echten Stop) GELEERT, und build_state() + muss den Modus trotzdem noch aus der dauerhaften Datei kennen - nicht auf "recovery" + zurueckfallen, waehrend tatsaechlich eine Migration ansteht.""" + last_mode = tmp_path / "var" / "last-mode" + run_dir = tmp_path / "run" + run_dir.mkdir() + (run_dir / "mode").write_text("migration") + last_mode.parent.mkdir(parents=True) + last_mode.write_text("migration") + + # "systemctl stop" leert das RuntimeDirectory - hier nachgestellt, ohne RUN_DIR + # selbst zu entfernen (das legt server.py/das CLI bei Bedarf neu an). + (run_dir / "mode").unlink() + + monkeypatch.setattr(display, "_service_active", lambda service_name: True) + state = display.build_state(run_dir=run_dir, conf_path=tmp_path / "station.conf", + service_name="x", imageset_path=tmp_path / "imageset", + last_mode_path=last_mode) + assert state["mode"] == "migration" + + def test_read_conf_accepts_export_prefix(tmp_path): """setup_pi.sh prueft beim Nachtragen fehlender Schluessel ausdruecklich auf ein optionales "export "-Praefix (^(export[[:space:]]+)?KEY=) - eine bereits vorhandene @@ -1990,7 +2023,8 @@ def test_the_image_set_line_survives_the_fullest_state(): def test_build_state_reads_the_image_set_switch(tmp_path, monkeypatch): """Dieselbe Datei, die das CLI schreibt und server.py liest - kein zweiter Weg.""" monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("migration") + last_mode = tmp_path / "last-mode" + last_mode.write_text("migration") images = tmp_path / "images" (images / "migrate-nocreds").mkdir(parents=True) (images / "migrate-nocreds" / "g95_cw_dev.img").write_bytes(b"IMG") @@ -2002,7 +2036,7 @@ def test_build_state_reads_the_image_set_switch(tmp_path, monkeypatch): def state(): return display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=switch) + imageset_path=switch, last_mode_path=last_mode) # Ohne Datei: die sichere Vorgabe. assert state()["imageset"] == "ohne-wlan" @@ -2017,11 +2051,14 @@ def test_build_state_reports_a_missing_image_set(tmp_path, monkeypatch): """"Vorhanden" heisst: Verzeichnis da UND mindestens ein Image darin - dieselbe Regel wie in server.py, sonst sagt das Panel "in Ordnung" und die Lampe bekommt nichts. - Ausdruecklich im Migrationsmodus (RUN_DIR/mode) - das ist es, was die migrate-*- - Verzeichnisse betrifft. Der Rettungsmodus prueft ein eigenes Paar (rescue-*), siehe - test_build_state_reports_a_missing_rescue_image_set unten.""" + Ausdruecklich im Migrationsmodus (last_mode_path explizit auf "migration" gesetzt, + nicht dem Vorgabewert ueberlassen - Fixrunde 1, Befund 3: eine fehlende Datei faellt + auf "recovery", und dieser Test soll die MIGRATION pruefen) - das ist es, was die + migrate-*-Verzeichnisse betrifft. Der Rettungsmodus prueft ein eigenes Paar + (rescue-*), siehe test_build_state_reports_a_missing_rescue_image_set unten.""" monkeypatch.setattr(display, "_service_active", lambda service_name: True) - (tmp_path / "mode").write_text("migration") + last_mode = tmp_path / "last-mode" + last_mode.write_text("migration") images = tmp_path / "images" (images / "migrate-nocreds").mkdir(parents=True) (images / "migrate-nocreds" / "g95_cw_dev.img").write_bytes(b"IMG") @@ -2031,25 +2068,33 @@ def test_build_state_reports_a_missing_image_set(tmp_path, monkeypatch): switch.write_text("ohne-wlan") assert display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=switch)["imageset_ok"] is True + imageset_path=switch, + last_mode_path=last_mode)["imageset_ok"] is True switch.write_text("mit-wlan") # migrate-wlan/ gibt es nicht state = display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=switch) + imageset_path=switch, last_mode_path=last_mode) assert state["imageset_ok"] is False assert "MISSING" in _imageset_line(state) # Und ein LEERES Verzeichnis zaehlt genauso als fehlend. (images / "migrate-wlan").mkdir() assert display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=switch)["imageset_ok"] is False + imageset_path=switch, + last_mode_path=last_mode)["imageset_ok"] is False def test_build_state_reports_a_missing_rescue_image_set(tmp_path, monkeypatch): """Derselbe Umschalter gilt jetzt auch fuer die Rettung, mit einem EIGENEN Verzeichnispaar (rescue-*, nicht migrate-*) - server.py begruendet die Trennung. - Ohne Modusdatei gilt "recovery" (dieselbe Vorgabe wie resolve_mode() in server.py), - das Panel muss also hier die rescue-*-Verzeichnisse pruefen, nicht die migrate-*.""" + + last_mode_path wird ausdruecklich auf "recovery" gesetzt, statt den Vorgabewert zu + nutzen (Fixrunde 1, Befund 3 - ein vorheriger Zustand dieses Tests liess die Datei + ganz weg und pruefte damit nur den ZUFAELLIG gleich lautenden Rueckfallwert, nicht + die Wirklichkeit: mit einer fehlenden Datei haette dieser Test nicht bemerkt, wenn + "recovery" faelschlich auch fuer eine echte Migration zurueckkaeme).""" monkeypatch.setattr(display, "_service_active", lambda service_name: True) + last_mode = tmp_path / "last-mode" + last_mode.write_text("recovery") images = tmp_path / "images" (images / "rescue-nocreds").mkdir(parents=True) (images / "rescue-nocreds" / "ninlight_g95_cw_ota.img").write_bytes(b"IMG") @@ -2062,17 +2107,19 @@ def test_build_state_reports_a_missing_rescue_image_set(tmp_path, monkeypatch): switch.write_text("ohne-wlan") assert display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=switch)["imageset_ok"] is True + imageset_path=switch, + last_mode_path=last_mode)["imageset_ok"] is True switch.write_text("mit-wlan") # rescue-wlan/ gibt es nicht state = display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=switch) + imageset_path=switch, last_mode_path=last_mode) assert state["imageset_ok"] is False assert "MISSING" in _imageset_line(state) # Und ein LEERES Verzeichnis zaehlt genauso als fehlend. (images / "rescue-wlan").mkdir() assert display.build_state(run_dir=tmp_path, conf_path=conf, service_name="x", - imageset_path=switch)["imageset_ok"] is False + imageset_path=switch, + last_mode_path=last_mode)["imageset_ok"] is False def test_the_display_and_the_server_agree_on_the_set_names():