diff --git a/CLAUDE.md b/CLAUDE.md index f151e0e..3d03c0d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,7 +54,7 @@ loop→Indigo writes go straight through `device_sync.apply_states` (thread-safe | `server_process.py` | `ServerProcess` = the matter-server (controller) specialisation of `LaunchAgent`: its prefs, its argv, its pinned version. Gated by the `serverLocation` pref — the config asks "is matter-server on this Mac?"; `local` (turnkey default) manages it here on loopback, `remote` connects to a server elsewhere. `manageLaunchAgent`/host/port are derived from that in `startup` (see `plugin.py:server_location`) | | `commission_jobs.py` | Commissioning job state machine (API.md §3.2/§3.3) | | `device_sync.py` | Node↔Indigo reconciliation + state/command seams | -| `fabric_backup.py` | Fabric backup/restore for the matter-server storage dir (issue #26) — zip snapshots into a sibling `backups/`, move-aside restore, retention prune. **Since E5 it also covers the export bridge node's storage dir** (`identity.json` + `endpoint-map.json`) under a reserved `bridge-node/` archive prefix, so an old controller-only archive still restores; a missing/empty bridge dir is skipped **with a WARNING naming the path**, because that path is derived from the controller's rather than configured. **Restore deliberately does not extract the bridge members** — that needs the node stopped and there is no stop seam until E7 — so it reports and skips. Pure + injectable (paths, clock, `stop()/start()` control) so it unit-tests against `tmp_path` | +| `fabric_backup.py` | Fabric backup/restore for the matter-server storage dir (issue #26) — zip snapshots into a sibling `backups/`, move-aside restore, retention prune. **Since E5 it also covers the export bridge node's storage dir** (`identity.json` + `endpoint-map.json`) under a reserved `bridge-node/` archive prefix, so an old controller-only archive still restores; a missing/empty bridge dir is skipped **with a WARNING naming the path**, because that path is derived from the controller's rather than configured. **Since #136, restore extracts the bridge members too** — it stops the bridge node (via the same `stop()`/`start()` seam, now given a second control), extracts its half alongside the controller fabric, and restarts it afterwards if it was running; a bridge-restart failure never rolls back an otherwise-good controller restore (XG5). It still falls back to reporting-and-skipping the bridge side (loudly, with the manual recipe) when there is no bridge control, no usable bridge storage path, or the path overlaps the controller's. Pure + injectable (paths, clock, two `stop()/start()` controls) so it unit-tests against `tmp_path` | | `http_handlers.py` | Domio API routing (served over IWS, not aiohttp) | | `matter_model.py` | Parse matter-server node dict → node/endpoint objects | | `matter_handlers/` | One `ClusterHandler` per cluster + registry (OnOff in v1) | diff --git a/docs/HANDOVER.md b/docs/HANDOVER.md index a3976fa..10fd195 100644 --- a/docs/HANDOVER.md +++ b/docs/HANDOVER.md @@ -1500,7 +1500,7 @@ New, and deferred on purpose: 3. **Restoring the bridge-node storage from a backup** needs a bridge `stop()`/ `start()` control — E7's launchd agent. Backup is done; restore reports and - skips (above). + skips (above). [wired by #136, v2026.9.0] 4. **Pinning endpoint numbers from the map** (the `Endpoint.Configuration.number` finding above) — a protocol-level decision about what `preserveEndpointNumbers` promises, not a patch. @@ -2136,7 +2136,7 @@ restore it). Don't re-derive them. | #133 | `get_pairing` races the last-fabric self-reset → spurious error | | #134 | Group the two Install/update menu items (11 apart, confusable) | | #135 | Reset reconnect backoff after a successful install (29s dead wait) | -| #136 | Wire bridge-storage restore (E7 gave it the stop seam it needed) | +| ~~#136~~ | ~~Wire bridge-storage restore (E7 gave it the stop seam it needed)~~ — done, `fabric_backup.restore_backup` takes a second `stop()/start()` control | | #137 | Docs site has no route to the export half (matter.html/index.html) | | #138 | Three screenshots wanted for the walkthrough | | #139 | Live validation tracker + the solved room remedy | diff --git a/docs/INSTALL.md b/docs/INSTALL.md index efe96c2..04bd4e1 100644 --- a/docs/INSTALL.md +++ b/docs/INSTALL.md @@ -634,10 +634,18 @@ Losing *that* is the number-one real-world cause of duplicated accessories: ever ecosystem re-creates every accessory, and their names, rooms and automations go with the old ones. -**Back up the Matter fabric…** includes this directory in the zip. **Restore does not -yet put it back** — it reports the bridge files it found and skips them, because -restoring them safely needs the bridge node stopped and that wiring is a -follow-up. So for now, if you need to restore the bridge side, copy +**Back up the Matter fabric…** includes this directory in the zip, and **Restore a +fabric backup…** puts it back too: it stops the bridge node (if it is running) +before restoring, extracts its half of the archive alongside the controller +fabric, and starts it again afterwards. If the bridge node was not running +before the restore, it is left stopped — it starts again the next time +something is exported. Its previous storage is moved aside, never deleted, the +same as the controller's. The restored storage holds the accessory identities +and endpoint numbers as of that backup; if a paired ecosystem has changed +since, the bridge REPORTS the drift in the log rather than renumbering +anything. If restore has no way to stop the bridge node (for example, no +LaunchAgent control could be built), it falls back to the old behaviour: it +reports the bridge files it found and skips them, and you can copy `bridge-node/` out of the zip by hand with the bridge stopped (*Stop the Matter bridge…*). Your export **list** is not in here at all — it lives in the plugin's preferences and rides along with Indigo's own database backup. diff --git a/indigo-matter.indigoPlugin/Contents/Info.plist b/indigo-matter.indigoPlugin/Contents/Info.plist index 7f3e9e5..3e87b0a 100644 --- a/indigo-matter.indigoPlugin/Contents/Info.plist +++ b/indigo-matter.indigoPlugin/Contents/Info.plist @@ -20,7 +20,7 @@ IwsApiVersion 1.0.0 PluginVersion - 2026.8.24 + 2026.9.0 ServerApiVersion 3.6 diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml b/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml index fb3e38a..5ee1709 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml @@ -325,14 +325,14 @@ You normally never need this: the bridge starts and stops with your export list. menuRestoreFabricBackup - + - + diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.py index 61ac9ca..f54c5d3 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.py @@ -9,10 +9,12 @@ first so a bad restore is itself reversible. Everything here is pure and injectable (paths, a ``now`` ``datetime`` clock, and -an abstract ``server_control`` with ``stop()/start()``) so the whole module unit- -tests against ``tmp_path`` with a fake control — no real launchctl, no real -matter-server. The plugin passes a :class:`server_process.ServerProcess` as the -control; tests pass a fake that records the call order. +an abstract ``server_control`` with ``stop()/start()``, optionally ``is_alive()``) +so the whole module unit-tests against ``tmp_path`` with fake controls — no real +launchctl, no real matter-server/bridge node. The plugin passes a +:class:`server_process.ServerProcess` as ``server_control`` and a +:class:`bridge_agent.BridgeProcess` as ``bridge_control``; tests pass fakes that +record the call order. Backups live in a sibling ``backups/`` directory NEXT TO the storage dir (never inside it), so a backup never self-includes and a restore never touches them. @@ -21,7 +23,9 @@ import logging import os +import shutil import zipfile +from dataclasses import dataclass from datetime import datetime from typing import Any, Optional @@ -238,136 +242,587 @@ def bridge_members_in(archive_path: str) -> list[str]: return [name for name in zf.namelist() if name.startswith(BRIDGE_MEMBER_PREFIX)] -def _safe_extract(archive_path: str, dest: str) -> None: - """Extract the **controller** members of ``archive_path`` into ``dest``. +@dataclass +class _BridgePlan: + """Everything a restore needs to know about the bridge half of the archive. + + Bundled rather than several separate parameters: :func:`_rollback` needs the + same set, pylint's ``max-args`` is 8, and "is there bridge work to do at + all" collapses to one ``is None`` check throughout :func:`restore_backup`. + """ + dest: str # bridge storage dir; members extract here, prefix stripped + control: Any # stop()/start() seam — never None in a live plan + members: int + was_alive: bool = False # captured BEFORE the stop; gates the start (XG5) + moved_aside_to: Optional[str] = None + started: Optional[bool] = None # None = we never stopped it, so we never started it + + +def _members_for(names: list[str], prefix: str) -> list[str]: + """Select one side's members from an archive's full name list. + + ``prefix=""`` means the CONTROLLER's members — everything NOT under + :data:`BRIDGE_MEMBER_PREFIX`. A truthy ``prefix`` means the bridge's own + members, under it. The two selections are complementary by construction: no + member is ever extracted twice or silently dropped. + """ + if prefix: + return [name for name in names if name.startswith(prefix) and name != prefix] + return [name for name in names if not name.startswith(BRIDGE_MEMBER_PREFIX)] - Any member whose resolved path escapes ``dest`` (via ``..`` or an absolute - path) is rejected before a single byte is written. - :data:`BRIDGE_MEMBER_PREFIX` members are skipped: they belong to a different - directory owned by a different process, and writing them under the - controller's storage would put a second Matter node's credentials somewhere - nothing will ever read them. :func:`restore_backup` reports them instead — - loudly, because a half-restore nobody mentioned is how a user discovers next - week that their exported accessories all came back as new ones. +def _assert_no_zip_slip(names: list[str], dest: str, prefix: str = "") -> list[str]: + """Select this side's members and prove every one lands inside ``dest``. + + The path checked is the one that will actually be WRITTEN — prefix + stripped, exactly as :func:`_safe_extract` writes it — not the raw member + name, or a bridge member crafted as ``bridge-node/../escape.txt`` would + look safe under the archive-relative name (it resolves inside the archive + root) while still escaping ``dest`` once the prefix is removed. Returns + the selected members so a caller who already has this list does not have + to filter twice. """ + members = _members_for(names, prefix) dest_root = os.path.realpath(dest) + for member in members: + target = os.path.realpath(os.path.join(dest, member[len(prefix):])) + if target != dest_root and not target.startswith(dest_root + os.sep): + raise ValueError(f"Unsafe path in archive (zip-slip): {member!r}") + return members + + +def _safe_extract(archive_path: str, dest: str, *, prefix: str = "") -> None: + """Extract this side's members of ``archive_path`` into ``dest``. + + ``prefix=""`` (the default, and the only mode before #136) selects the + **controller's** members — everything not under :data:`BRIDGE_MEMBER_PREFIX` + — and extracts them at ``dest``'s root, byte-identical to before. A truthy + ``prefix`` selects the **bridge's** own members and strips it off the + written path, so ``bridge-node/identity.json`` in the archive lands at + ``dest/identity.json``. :meth:`zipfile.ZipFile.extractall` cannot do that + renaming without mutating shared ``ZipInfo`` objects, so the prefix path + copies each member by hand instead. + + Any member whose resolved (prefix-stripped) path would escape ``dest`` is + rejected via :func:`_assert_no_zip_slip` before a single byte is written. + :func:`restore_backup` additionally pre-flights both sides before either + control is stopped — the check here is defence in depth, not the only line. + + Which side gets restored at all is decided by :func:`restore_backup` + (via :func:`_plan_bridge_restore`): a missing bridge control or storage + path means the bridge side is reported and skipped, never extracted with a + truthy ``prefix``. + """ with zipfile.ZipFile(archive_path, "r") as zf: - members = [name for name in zf.namelist() if not name.startswith(BRIDGE_MEMBER_PREFIX)] + members = _assert_no_zip_slip(zf.namelist(), dest, prefix) + if not prefix: + zf.extractall(dest, members=members) + return for member in members: - target = os.path.realpath(os.path.join(dest, member)) - if target != dest_root and not target.startswith(dest_root + os.sep): - raise ValueError(f"Unsafe path in archive (zip-slip): {member!r}") - zf.extractall(dest, members=members) - - -def restore_backup( + rel = member[len(prefix):] + if rel.endswith("/"): + os.makedirs(os.path.join(dest, rel), exist_ok=True) + continue + target = os.path.join(dest, rel) + os.makedirs(os.path.dirname(target), exist_ok=True) + with zf.open(member) as src, open(target, "wb") as out: + shutil.copyfileobj(src, out) + + +#: The bridge-skip warning, one shared template for every reason it fires: +#: no control to stop it with, no usable storage path, or a storage path +#: that overlaps the controller's. Loud on purpose — a half-restore nobody +#: mentioned is how a user discovers next week that every exported +#: accessory came back as a new one. +_SKIP_MSG = ("This backup also contains %d file(s) from the Matter bridge node and they are NOT being " + "restored: %s. To restore them by hand, stop the bridge node (Plugins ▸ Matter ▸ Stop the " + "Matter bridge…), then extract the '%s' entries of %s over %s. Restoring the controller fabric only.") + + +def _plan_bridge_restore( archive_path: str, storage_path: str, - server_control: Any, + bridge_storage_path: Optional[str], + bridge_control: Optional[Any], *, - now: datetime, - logger: Optional[Any] = None, -) -> dict: - """Restore a fabric backup over the live storage dir — safely. + log: Any, +) -> Optional[_BridgePlan]: + """What :func:`restore_backup` needs to restore the bridge side too, or ``None``. + + Returns ``None`` SILENTLY when the archive has no bridge members at all — + an old, controller-only archive must restore exactly as it always did, + with nothing extra logged. Every OTHER reason to fall back to + controller-only (no control given, no usable path, an overlapping path) is + a real member left behind, so it is a WARNING naming the manual recipe — + a fallback, never a refusal: the controller fabric is the single point + of total loss, the bridge storage is recoverable by re-pairing. + """ + members = bridge_members_in(archive_path) + if not members: + return None + + recipe_dest = os.path.join(os.path.dirname(storage_path), "bridge-node") + if bridge_control is None or not bridge_storage_path: + reason = ("this restore was given no way to stop the bridge node" if bridge_control is None + else "there is no usable bridge storage path") + log.warning(_SKIP_MSG, len(members), reason, BRIDGE_MEMBER_PREFIX, archive_path, recipe_dest) + return None + + dest = os.path.normpath(bridge_storage_path) + if dest == storage_path or dest.startswith(storage_path + os.sep) or storage_path.startswith(dest + os.sep): + reason = f"the bridge storage path {dest} overlaps the controller's" + log.warning(_SKIP_MSG, len(members), reason, BRIDGE_MEMBER_PREFIX, archive_path, recipe_dest) + return None + + return _BridgePlan(dest, bridge_control, len(members)) + + +def _control_is_alive(control: Any, log: Any) -> bool: + """Whether ``control``'s process is (or may be) up. Defaults to True when unsure. + + ``is_alive()`` is an OPTIONAL extension of the ``stop()``/``start()`` seam + (:meth:`launch_agent.LaunchAgent.is_alive`) — a control without it is + treated as alive. Fail-SAFE, and deliberately asymmetric with the rest of + this module's failure handling: the cost of stopping an already-stopped + node is one log line, while the cost of extracting new storage under a + still-live node is a corrupted store. + """ + probe = getattr(control, "is_alive", None) + if not callable(probe): + return True + try: + return bool(probe()) + except Exception as exc: # pylint: disable=broad-except + # A raising is_alive() is not evidence of death — but it must not be + # silent, or a persistently-raising probe looks indistinguishable + # from one that has never been called. + log.debug("bridge is_alive() raised, treating as alive: %s", exc) + return True + + +def _restart_bridge(plan: _BridgePlan, log: Any) -> bool: + """Start the bridge node back up. Never raises; the caller decides what a False means. + + Four different callers use this result, and every one of them checks it: + the abort path in :func:`_stop_for_restore` (a best-effort restart when + the controller failed to stop), the move-aside failure path in + :func:`restore_backup`, the post-restore finish step in + :func:`_finish_bridge_restore` (which logs a specific ERROR naming where + the restored storage sits), and the rollback's bridge-restart step in + :func:`_rollback` (already escalating loudly and appending to that + message instead). Each logs its own consequence, in its own words — none + of them wants this function logging on their behalf and either + duplicating or contradicting it — ``log`` is only touched here if + ``start()`` itself raises. + """ + try: + return bool(plan.control.start()) + except Exception as exc: # pylint: disable=broad-except + log.debug("Matter bridge node restart raised: %s", exc) + return False - Steps: - 1. Validate the archive exists, is a valid zip with at least one member, - and passes ``testzip``; zip-slip guard. (All before touching anything.) - 2. ``server_control.stop()`` — the server MUST go down before we touch the - live fabric. A False return (launchctl failed) ABORTS here, before any - move-aside or extract, so we never write over a still-running server. - 3. Move the existing storage dir aside to - ``.pre-restore-`` (NEVER delete in place; skipped - if the storage dir does not exist). - 4. Extract the archive into a fresh ``storage_path`` and assert the result - is non-empty (a validly-zipped but empty backup would silently wipe the - fabric — so that is treated as a failure and rolled back). - 5. ``server_control.start()`` — a False return is a failure (server down on - the new fabric) and triggers rollback, same as an exception. - 6. On ANY failure during 3–5: roll back without ever stranding the original. - If a partial new ``storage_path`` exists it is moved aside to - ``.failed-`` (not rmtree'd, so a half-removable dir - cannot block the rename-back), THEN the original is renamed back into - place, THEN ``start()`` is attempted on the restored fabric. A failing - rollback ``start()`` is escalated at ERROR with explicit manual-recovery - guidance — that is the loudest case: server down AND fabric possibly not - back. The function always ends by raising a wrapped ``RuntimeError`` so - the original cause is never masked. - - Returns ``{restored_from, moved_aside_to}``. ``moved_aside_to`` is ``None`` - if there was no existing storage dir to preserve. - - ``server_control`` is an abstract seam (an object with ``stop()`` and - ``start()`` returning bools); this function never imports indigo or calls - launchctl. - - **The bridge node's members are backed up but not restored here** (E5). - Restoring them safely needs the bridge node stopped for the same reason step - 2 stops the controller — it holds the directory open and would write over a - restore the moment it next persisted — and there is no bridge ``stop()`` - seam until E7 wires its launchd agent. Silently extracting them anyway would - be the worst of the three options, so they are reported with the manual - recipe instead, and the controller's fabric is restored as it always was. + +def _load_and_validate_archive(archive_path: str) -> list[str]: + """Restore step 1: exists, is a valid zip, ``testzip`` clean, non-empty. + + All before touching anything — a corrupt or content-empty zip is surfaced + up front, before either control is stopped or a byte is moved. A + zero-member archive is a wipe waiting to happen. """ - log = _resolve_logger(logger) if not os.path.isfile(archive_path): raise FileNotFoundError(f"Backup archive does not exist: {archive_path}") if not zipfile.is_zipfile(archive_path): raise ValueError(f"Not a valid zip archive: {archive_path}") - # Surface a corrupt or content-empty zip up front (before stopping the server - # or moving anything). A zero-member archive is a wipe waiting to happen. with zipfile.ZipFile(archive_path, "r") as zf: bad = zf.testzip() if bad is not None: raise ValueError(f"Corrupt member in archive {archive_path}: {bad}") - if not zf.namelist(): + names = zf.namelist() + if not names: raise ValueError(f"Backup archive has no members, refusing to restore (would wipe fabric): {archive_path}") + return names - storage_path = os.path.normpath(storage_path) - skipped = bridge_members_in(archive_path) - if skipped: - log.warning( - "This backup also contains %d file(s) from the Matter bridge node. They are " - "NOT being restored: the bridge node has to be stopped first and this plugin cannot " - "stop it yet. If you need them, stop the bridge node and extract the '%s' entries of " - "%s over %s by hand. Restoring the controller fabric only.", - len(skipped), BRIDGE_MEMBER_PREFIX, archive_path, - os.path.join(os.path.dirname(storage_path), "bridge-node")) - - # C1: the server MUST be down before we touch the live fabric. If stop() - # reports failure we abort here — storage is untouched, nothing moved aside. - if not server_control.stop(): + +def _stop_for_restore(plan: Optional[_BridgePlan], server_control: Any, log: Any) -> None: + """Stop the bridge (if alive) then the controller; raise on either failure. + + Bridge first — cheapest abort on the failure-prone control, and the + controller keeps its existing (pre-#136) abort semantics either way. A + controller stop failure gets a best-effort bridge restart before raising, + so an abort here never leaves a bridge we just stopped down for nothing. + """ + if plan is not None: + plan.was_alive = _control_is_alive(plan.control, log) + if plan.was_alive and not plan.control.stop(): + raise RuntimeError( + "the Matter bridge node failed to stop; aborting restore before touching anything" + ) + # C1 (stop half): a False stop() is honoured as an abort. A RAISING + # stop() must be treated the same way — otherwise a bridge we just + # stopped above is left down with no restart attempted and no log line, + # because the exception would propagate straight past the bridge-restart + # logic below it. + try: + controller_stopped = server_control.stop() + except Exception as stop_exc: # pylint: disable=broad-except + if plan is not None and plan.was_alive and not _restart_bridge(plan, log): + log.error( + "The Matter bridge node did not restart after the controller's stop() " + "raised (%s). Its pairings are intact; exported accessories are " + "unavailable until it starts — check the bridge node's error log, then " + "reload the plugin.", stop_exc, + ) + raise RuntimeError( + "matter-server failed to stop; aborting restore before touching the live fabric" + ) from stop_exc + if not controller_stopped: + if plan is not None and plan.was_alive and not _restart_bridge(plan, log): + # Best-effort: put the bridge back the way we found it before + # raising — nothing else in this function has touched anything yet. + # A failure here is not fatal to the abort, but it must not be + # silent — the same consequence the rollback's bridge-restart + # step and the move-aside failure path already spell out. + log.error( + "The Matter bridge node did not restart after the controller failed to " + "stop. Its pairings are intact; exported accessories are unavailable " + "until it starts — check the bridge node's error log, then reload the " + "plugin." + ) raise RuntimeError( "matter-server failed to stop; aborting restore before touching the live fabric" ) - moved_aside_to: str | None = None + +class _MoveAsideDoubleFault(OSError): + """The bridge rename failed AND undoing the controller's rename also failed. + + Distinct from the single-fault case (a bridge rename failure whose undo + of the controller rename succeeded, or never had to happen) so + :func:`restore_backup`'s except handler can tell them apart: a double + fault leaves the controller fabric stranded at ``moved_aside_to``, not at + ``storage_path`` — starting matter-server would recreate ``storage_path`` + as a fresh, empty fabric (the launch agent's ``ensure_installed`` makes + the dir), which then blocks the manual ``mv`` recovery this exception + describes. Carries both paths so the handler can name them without + re-deriving anything. + """ + + def __init__(self, moved_aside_to: str, storage_path: str): + super().__init__( + f"double fault moving storage aside: controller fabric stranded at " + f"{moved_aside_to}, not in place at {storage_path}" + ) + self.moved_aside_to = moved_aside_to + self.storage_path = storage_path + + +def _move_aside_for_restore( + storage_path: str, plan: Optional[_BridgePlan], now: datetime, log: Any +) -> Optional[str]: + """Move existing dir(s) aside as one unit, before either extract runs. + + NEVER deletes in place. Controller then bridge, so :func:`_rollback` has + one uniform shape to undo regardless of which side later fails. + + All-or-nothing: a controller rename that succeeds followed by a bridge + rename that raises would otherwise strand the controller fabric aside + with nothing to show for it, so a failing bridge rename undoes the + controller rename before re-raising — after this function returns + normally OR raises, either both sides are aside or neither is, never one. + If the undo itself also raises (a double fault, vanishingly rare), it is + raised as :class:`_MoveAsideDoubleFault` instead of the bare + ``OSError`` — the single-fault case keeps raising the original — so the + caller can tell the two apart and respond differently. + """ + moved_aside_to: Optional[str] = None if os.path.isdir(storage_path): moved_aside_to = f"{storage_path}.pre-restore-{_stamp(now)}" os.rename(storage_path, moved_aside_to) + if plan is not None and os.path.isdir(plan.dest): + bridge_moved_aside_to = f"{plan.dest}.pre-restore-{_stamp(now)}" + try: + os.rename(plan.dest, bridge_moved_aside_to) + except OSError: + if moved_aside_to is not None: + try: + os.rename(moved_aside_to, storage_path) + except OSError as undo_exc: + log.error( + "Moving the Matter bridge node's storage aside failed, and undoing " + "the controller's move-aside ALSO failed (%s): the controller fabric " + "is at %s, NOT in place at %s — move it back by hand, with " + "matter-server stopped.", + undo_exc, moved_aside_to, storage_path, + ) + raise _MoveAsideDoubleFault(moved_aside_to, storage_path) from undo_exc + raise + plan.moved_aside_to = bridge_moved_aside_to + return moved_aside_to + + +def _extract_for_restore(archive_path: str, storage_path: str, plan: Optional[_BridgePlan]) -> None: + """Extract into fresh dir(s); raise if either result is empty (H4). + + A validly-zipped but empty backup would silently wipe the fabric (or the + bridge storage) — refusing it here is what lets the caller's ``except`` + treat it exactly like any other extraction failure and roll back. + """ + os.makedirs(storage_path, exist_ok=True) + _safe_extract(archive_path, storage_path) + if not _is_nonempty_dir(storage_path): + raise RuntimeError( + f"Restored storage dir is empty after extracting {archive_path}; " + "refusing to leave the fabric wiped" + ) + if plan is not None: + os.makedirs(plan.dest, exist_ok=True) + _safe_extract(archive_path, plan.dest, prefix=BRIDGE_MEMBER_PREFIX) + if not _is_nonempty_dir(plan.dest): + raise RuntimeError( + f"Restored Matter bridge storage dir is empty after extracting " + f"{archive_path}; refusing to leave it wiped" + ) + + +def _finish_bridge_restore(plan: Optional[_BridgePlan], log: Any) -> None: + """After a successful restore: restart a bridge that was running, report honestly. + + Called OUTSIDE :func:`restore_backup`'s try/rollback — see the XG5 + paragraph on :func:`restore_backup` itself for why a restart failure here + never undoes a good controller restore. + """ + if plan is None: + return + if plan.was_alive: + plan.started = _restart_bridge(plan, log) + if not plan.started: + log.error( + "The fabric restore SUCCEEDED, but the Matter bridge node did not start " + "again afterwards. Its restored storage is in place at %s and its pairings " + "are intact; exported accessories are unavailable until it starts. Check " + "the bridge node's error log, then reload the plugin or re-export " + "something.", plan.dest) + else: + log.info( + "Matter bridge node storage restored to %s. The node was not running, so it " + "has been left stopped — it starts again the next time something is " + "exported.", plan.dest) + + +def restore_backup( + archive_path: str, + storage_path: str, + server_control: Any, + *, + now: datetime, + logger: Optional[Any] = None, + bridge_storage_path: Optional[str] = None, + bridge_control: Optional[Any] = None, +) -> dict: + """Restore a fabric backup over the live storage dir — safely. + + Steps (controller-only when ``bridge_control``/``bridge_storage_path`` are + absent, or the archive has no bridge members — see + :func:`_plan_bridge_restore`): + + 1. Validate the archive exists, is a valid zip with at least one member, + and passes ``testzip``; pre-flight the zip-slip guard for BOTH sides. + All of this happens before either control is touched. + 2. If there is bridge work to do and the bridge is alive, stop it FIRST + (:func:`_control_is_alive`/``bridge_control.stop()``) — cheapest abort + on the failure-prone control. A False ``stop()`` ABORTS here; nothing + has moved. + 3. ``server_control.stop()`` — the controller MUST go down before we + touch the live fabric. A False return ABORTS here too, after a + best-effort restart of any bridge we just stopped. + 4. Move the existing storage dir(s) aside to ``.pre-restore- + `` (NEVER delete in place; skipped if a dir does not exist) — + controller then bridge, as one unit, before either extract. + All-or-nothing: a failing bridge rename undoes the controller + rename first (see :func:`_move_aside_for_restore`). This step's own + failure handler then raises a wrapped ``RuntimeError`` — in the + (near-universal) single-fault case, the disk is unchanged and a + restart of both daemons is attempted; in the rare double fault + (the undo also failed), the controller fabric is stranded aside and + its restart is deliberately skipped instead (starting it would + recreate an empty fabric at the original path). Either way this + failure never reaches step 7's :func:`_rollback`. + 5. Extract the archive into fresh dir(s) and assert each result is + non-empty (a validly-zipped but empty backup would silently wipe the + fabric — treated as a failure and rolled back). + 6. ``server_control.start()`` — a False return is a failure and triggers + rollback, same as an exception. + 7. On ANY failure during 5–6: roll back without ever stranding either + original — bridge undone first, then the controller (verbatim of the + controller-only behaviour), then the controller is restarted, then + (if we stopped it) the bridge. A failing rollback ``start()`` is + escalated at ERROR with explicit manual-recovery guidance and a + ``CRITICAL:`` message prefix. The function always ends by raising a + wrapped ``RuntimeError`` so the original cause is never masked. + 8. OUTSIDE the try, and NEVER rolled back: if we stopped a live bridge, + start it again. A False here does not undo a good controller + restore — rolling back a restored fabric to protect a secondary + export daemon would destroy what the user came for — it is reported + as an ERROR naming where the restored storage already sits. If the + bridge was not running before the restore, XG5 says it stays that + way; it starts again on the next export. + + Returns a dict: ``restored_from``, ``moved_aside_to`` (``None`` if there + was no existing controller storage dir to preserve), ``bridge_restored`` + (bool), ``bridge_members`` (count), ``bridge_moved_aside_to`` (``None`` if + no existing bridge storage dir, or the bridge side was not restored), and + ``bridge_started`` (``None`` when the bridge was never stopped for this + restore — either it was not part of the plan, or it was already stopped; + a bool once it was). + + ``server_control``/``bridge_control`` are abstract seams (objects with + ``stop()``/``start()`` returning bools, and an OPTIONAL ``is_alive()`` — + see :func:`_control_is_alive`); this function never imports indigo or + calls launchctl. + + **Why bridge-start failure never rolls the controller back (XG5).** The + bridge's ``start()`` returning False for "never installed" is by design + (:meth:`launch_agent.LaunchAgent.start`) — treating it as a restore + failure would abort every restore run against a Mac that has never + exported anything. The aliveness gate at step 2 is what keeps that honest + in the other direction: only a bridge that was actually running before the + restore is expected to be running after it, so a real regression there + IS reported loudly, just not as a reason to undo the controller. + """ + log = _resolve_logger(logger) + names = _load_and_validate_archive(archive_path) + + storage_path = os.path.normpath(storage_path) + plan = _plan_bridge_restore(archive_path, storage_path, bridge_storage_path, bridge_control, log=log) + + # Pre-flight the zip-slip guard for BOTH sides before either control is + # touched. This used to fire only inside _safe_extract, after both + # processes were already stopped — a hostile archive must never get to + # take two daemons down before being rejected. + _assert_no_zip_slip(names, storage_path) + if plan is not None: + _assert_no_zip_slip(names, plan.dest, BRIDGE_MEMBER_PREFIX) + log.info( + "Restoring %d file(s) of Matter bridge node storage into %s alongside the " + "controller fabric.", plan.members, plan.dest) + + _stop_for_restore(plan, server_control, log) try: - os.makedirs(storage_path, exist_ok=True) - _safe_extract(archive_path, storage_path) - # H4: a validly-zipped but empty restore is a silent wipe. Refuse it. - if not _is_nonempty_dir(storage_path): - raise RuntimeError( - f"Restored storage dir is empty after extracting {archive_path}; " - "refusing to leave the fabric wiped" + moved_aside_to = _move_aside_for_restore(storage_path, plan, now, log) + except _MoveAsideDoubleFault as exc: + # Double fault: the controller fabric is stranded aside, not at + # storage_path, and nothing there predates it. Starting matter-server + # would recreate storage_path as a FRESH EMPTY fabric (the launch + # agent's ensure_installed makedirs), which then blocks the manual + # `mv` this message prescribes — so, unlike the single-fault case, + # the controller start is deliberately SKIPPED. The bridge's own + # directory was never touched by this fault, so it is still + # restarted best-effort if we stopped it. + if plan is not None and plan.was_alive: + if not _restart_bridge(plan, log): + log.error( + "The Matter bridge node did not restart after a failed move-aside during " + "restore. Its pairings are intact; exported accessories are unavailable " + "until it starts — check the bridge node's error log, then reload the " + "plugin." + ) + raise RuntimeError( + f"Fabric restore from {archive_path} failed while moving the existing storage " + f"aside, and undoing that also failed: the controller fabric is at " + f"{exc.moved_aside_to}, NOT in place at {exc.storage_path}. matter-server was " + "NOT restarted — starting it now would recreate an empty fabric at the original " + f"path. To recover manually: stop matter-server if it is running, move " + f"{exc.moved_aside_to} back to {exc.storage_path}, then start it." + ) from exc + except OSError as exc: + # Single fault: best-effort, put both daemons back the way we found + # them. This is NOT routed through _rollback — there is nothing for + # it to undo, the move-aside itself already did (or explicitly + # failed to, and logged why) — this only needs to bring the daemons + # back up. A RAISING start() must be caught here too, or the CRITICAL + # log line below and the bridge restart never run and the wrapped + # RuntimeError never raises — the caller would see the bare + # exception with none of this context. + try: + started = server_control.start() + except Exception as start_exc: # pylint: disable=broad-except + started = False + log.error("CRITICAL: matter-server restart raised after a failed move-aside (%s)", start_exc) + if not started: + log.error( + "CRITICAL: matter-server is DOWN after a failed move-aside during restore. " + "To recover manually: check ~/Library/Logs/indigo-matter/matter-server.err.log " + "and start matter-server (it should pick up the original fabric at %s).", + storage_path, ) - # C1: start() returns a bool; False (launchctl failed) is a failure that - # must trigger rollback exactly like an exception would. + if plan is not None and plan.was_alive: + if not _restart_bridge(plan, log): + log.error( + "The Matter bridge node did not restart after a failed move-aside during " + "restore. Its pairings are intact; exported accessories are unavailable " + "until it starts — check the bridge node's error log, then reload the " + "plugin." + ) + raise RuntimeError( + f"Fabric restore from {archive_path} failed while moving the existing storage " + "aside; the disk is unchanged unless the error above says otherwise — a restart " + "of both daemons was attempted — check the log above for restart failures" + ) from exc + + try: + _extract_for_restore(archive_path, storage_path, plan) + # C1 (start half): start() returns a bool; False (launchctl failed) is + # a failure that must trigger rollback exactly like an exception would. if not server_control.start(): raise RuntimeError("matter-server failed to start after restore") except BaseException as exc: # noqa: BLE001 — re-raised after rollback - _rollback(storage_path, moved_aside_to, server_control, now=now, log=log) + _rollback(storage_path, moved_aside_to, server_control, now=now, log=log, bridge=plan) raise RuntimeError( f"Fabric restore from {archive_path} failed and was rolled back " f"(original fabric preserved at {moved_aside_to or storage_path})" ) from exc - return {"restored_from": archive_path, "moved_aside_to": moved_aside_to} + # OUTSIDE the try: a bridge that fails to restart never rolls a good + # controller restore back — see the docstring's XG5 paragraph. + _finish_bridge_restore(plan, log) + + return { + "restored_from": archive_path, + "moved_aside_to": moved_aside_to, + "bridge_restored": plan is not None, + # The count of bridge members the ARCHIVE carries, not how many were + # actually restored — a skipped/refused bridge side still tells the + # caller how much was left behind, which is what makes the log + # warning's count and this field agree. + "bridge_members": plan.members if plan is not None else len(bridge_members_in(archive_path)), + "bridge_moved_aside_to": plan.moved_aside_to if plan is not None else None, + "bridge_started": plan.started if plan is not None else None, + } + + +def _bridge_rollback_failure_message(bridge: _BridgePlan, exc: OSError) -> tuple[str, tuple]: + """The bridge-rollback-mechanics failure text: two shapes, one cause. + + A previous bridge copy exists and could not be put back (the wording + that shape has always had), or there was never one to put back — + claiming a "previous copy" in that case would send a user hunting for + something that never existed. Returns ``(message, args)`` so the caller + logs it in one place. + """ + if bridge.moved_aside_to is not None: + return ( + "Rolling the Matter bridge node's storage back FAILED (%s). Its previous " + "storage is PRESERVED at %s but is NOT in place; move it back to %s by " + "hand with the bridge node stopped.", + (exc, bridge.moved_aside_to, bridge.dest), + ) + return ( + "The partially-restored Matter bridge storage at %s could not be moved " + "aside (%s). No previous bridge storage predated this restore; clean up " + "%s by hand with the bridge node stopped.", + (bridge.dest, exc, bridge.dest), + ) def _rollback( @@ -377,17 +832,47 @@ def _rollback( *, now: datetime, log: Any, + bridge: Optional[_BridgePlan] = None, ) -> None: - """Undo a failed restore without ever stranding or wiping the original fabric. - - Move any partial new ``storage_path`` aside (never rely on a possibly-failing - ``rmtree`` to clear the way for the rename-back), then rename the original - ``moved_aside_to`` back into place, then bring the server up on it. A failure - of the rollback mechanics is escalated and re-raised; a failure of the - rollback ``start()`` is the loudest case (user is now fabric-less / server - down) and is logged at ERROR with manual-recovery guidance naming the - original fabric location. + """Undo a failed restore without ever stranding or wiping either original. + + Bridge undone FIRST, controller second — the reverse of the extract + order (and the same bridge-first order the stops used), so the + controller (the single point of total loss) is always the last, + most-supervised step. Each side moves any partial new dir aside + (never relies on a possibly-failing ``rmtree`` to clear the way for the + rename-back), then renames the original ``*_aside_to`` back into place. + + The bridge's own undo is wrapped in its own ``try``/``except OSError`` and + NEVER raises — a failure to roll the bridge back must not abort the + controller's rollback, which is the one that protects the fabric. A + failure of the controller's rollback mechanics IS re-raised (today's + behaviour, unchanged) and is the loudest case: escalated at ERROR with a + ``CRITICAL:`` message prefix and manual-recovery guidance, extended with + a note that the bridge is still stopped when this restore was the one + that stopped it. + + Bringing the controller back up is attempted regardless (verbatim of the + controller-only behaviour); a failure there is the second-loudest case — + server down AND fabric possibly not back — logged the same way, at ERROR + with a ``CRITICAL:`` prefix. Only once all of that is settled is the + bridge restarted, if we are the one who stopped it — the rollback's + bridge-restart step, least critical of the four, because pairings are + intact either way and this only delays exported accessories coming back. """ + if bridge is not None: + try: + if os.path.exists(bridge.dest): + bridge_failed_aside = f"{bridge.dest}.failed-{_stamp(now)}" + os.rename(bridge.dest, bridge_failed_aside) + log.warning("Moved partial failed Matter bridge restore aside to %s", bridge_failed_aside) + if bridge.moved_aside_to is not None and os.path.isdir(bridge.moved_aside_to): + os.rename(bridge.moved_aside_to, bridge.dest) + except OSError as bridge_rollback_exc: + message, args = _bridge_rollback_failure_message(bridge, bridge_rollback_exc) + log.error(message, *args) + # Never raise: must not abort the controller's rollback below. + try: # Clear the partial new dir out of the way WITHOUT rmtree(ignore_errors): # a half-removable dir would otherwise leave storage_path occupied and the @@ -399,13 +884,15 @@ def _rollback( if moved_aside_to is not None and os.path.isdir(moved_aside_to): os.rename(moved_aside_to, storage_path) except OSError as rollback_exc: + bridge_note = (" The Matter bridge node was stopped for this restore and is still stopped." + if bridge is not None and bridge.was_alive else "") # The rollback mechanics themselves failed: the original fabric is still # safe at moved_aside_to, but it is NOT back in place. Escalate loudly. log.error( "CRITICAL: fabric restore rollback FAILED (%s). Your original fabric is " "PRESERVED but NOT in place. To recover manually: stop matter-server, then " - "move %s back to %s, then start matter-server.", - rollback_exc, moved_aside_to, storage_path, + "move %s back to %s, then start matter-server.%s", + rollback_exc, moved_aside_to, storage_path, bridge_note, ) raise @@ -429,3 +916,14 @@ def _rollback( "(it should pick up the original fabric at %s).", storage_path, storage_path, ) + + # The rollback's bridge-restart step: the bridge, if we are the one who + # stopped it. Loud but not fatal — ERROR, not CRITICAL, because pairings + # are intact either way. + if bridge is not None and bridge.was_alive: + if not _restart_bridge(bridge, log): + log.error( + "The Matter bridge node did not restart after the rollback. Its pairings are " + "intact; exported accessories are unavailable until it starts — check the " + "bridge node's error log, then reload the plugin." + ) diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py index 724d901..3805e2b 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py @@ -3010,6 +3010,33 @@ def _bridge_storage_path(self) -> str: """ return bridge_agent.bridge_storage_path(self._resolve_storage_path()) + def _bridge_restore_control(self) -> Optional["bridge_agent.BridgeProcess"]: + """The bridge's ``stop()``/``start()`` seam for :func:`fabric_backup.restore_backup`. + + ``stop()``/``start()``, NOT ``uninstall()``: restore wants the node + back in exactly its prior lifecycle state, plist included, so that a + reboot afterwards behaves exactly as it would have before the + restore. ``uninstall()`` is ``menuStopBridgeNode``'s primitive + (:meth:`_stop_bridge_agent`) and answers "make sure a reboot cannot + bring this back" — the wrong question here, and why THIS path leaves + the XAC1 latch alone (see the call site in ``menuRestoreFabricBackup``). + + Built from CURRENT prefs when no agent object exists yet, exactly as + ``menuStopBridgeNode`` does — this must work in a session that never + exported anything. Construction writes nothing and runs no launchctl. + """ + if self.bridge_process is not None: + return self.bridge_process + try: + self.bridge_process = bridge_agent.BridgeProcess(dict(self.pluginPrefs), self.logger) + except Exception as exc: # pylint: disable=broad-except + self.logger.warning( + "Matter bridge: could not build a control for the bridge node (%s), so any " + "bridge files in this backup will be reported and skipped. The controller " + "fabric restores normally.", exc) + return None + return self.bridge_process + @staticmethod def _human_size(num_bytes: int) -> str: size = float(num_bytes) @@ -3085,9 +3112,23 @@ def menuRestoreFabricBackup(self, valuesDict, menuId=""): # noqa: N802, ARG002 try: storage_path = self._resolve_storage_path() + # The bridge control/path are passed in, but the XAC1 latch (which + # session started the bridge agent) is NEVER touched here in either + # direction: alive+latched -> stop/start -> unchanged, correct; + # alive+unlatched -> unchanged, because setting it would arm a + # future bootout of an agent whose lifecycle this session does not + # own; stopped-by-us + restart-failed -> the latch is left EXACTLY + # as it was: if this session had started the agent it stays set + # (so the next empty-export transition still uninstalls the + # RunAtLoad plist); if a prior session's agent, it stays unset — + # no worse than before the restore (XAC1/XG5). + # restore_backup uses stop()/start(), never uninstall(), so the + # plist survives and the latch's claim stays true throughout. result = fabric_backup.restore_backup( selected, storage_path, self.server_process, now=datetime.now(timezone.utc), logger=self.logger, + bridge_storage_path=self._bridge_storage_path(), + bridge_control=self._bridge_restore_control(), ) # restore_backup only returns on success: the server was stopped, the # fabric was swapped, the restored dir is non-empty, and start() @@ -3100,6 +3141,24 @@ def menuRestoreFabricBackup(self, valuesDict, menuId=""): # noqa: N802, ARG002 "came back.", result["restored_from"], result["moved_aside_to"], ) + if result["bridge_restored"]: + if result["bridge_started"] is False: + self.logger.error( + "The controller fabric restored, but the Matter bridge node did not " + "come back up. %s", self._bridge_agent_diagnosis() or + "Check the bridge node's error log.") + else: + # There may have been no pre-existing bridge dir to preserve — + # say nothing rather than "preserved at None". + preserved = ( + f" (previous copy preserved at {result['bridge_moved_aside_to']})" + if result["bridge_moved_aside_to"] else "") + self.logger.info( + "The Matter bridge node's storage was restored too%s%s. It now holds " + "the accessory identities and endpoint numbers as of that backup — if " + "a paired ecosystem has changed since, the bridge REPORTS endpoint-map " + "drift in the log and renumbers nothing.", preserved, + " and the node has been restarted" if result["bridge_started"] else "") return (True, valuesDict) except Exception as exc: # noqa: BLE001 # restore_backup rolled back and preserved the original fabric (or diff --git a/tests/test_fabric_backup.py b/tests/test_fabric_backup.py index d2eff2d..154e7c9 100644 --- a/tests/test_fabric_backup.py +++ b/tests/test_fabric_backup.py @@ -15,6 +15,9 @@ import fabric_backup +_IDENTITY_JSON = '{"installId": "abc", "passcode": 20202021, "discriminator": 3840}' +_ENDPOINT_MAP_JSON = '{"version": 1, "endpoints": {"indigo-101": 2}}' + def _make_storage(tmp_path, *, files: dict[str, str] | None = None): """Create a fake matter-server storage dir with some fabric files.""" @@ -41,6 +44,10 @@ class FakeControl: makes only the first start() RETURN False (the rollback start succeeds) — exercising the bool-False path. ``rollback_start_fails`` makes the SECOND (rollback) start() return False — exercising the loud manual-recovery path. + ``start_always_false`` makes EVERY start() return False — a never-installed + bridge, whose ``start()`` fails by design and must not abort a good restore. + ``alive`` is what ``is_alive()`` reports; ``name``/``log`` let a bridge and a + controller share one ordered call log across #136's two-control tests. """ def __init__( @@ -50,22 +57,38 @@ def __init__( stop_returns: bool = True, start_returns_false: bool = False, rollback_start_fails: bool = False, + start_always_false: bool = False, + alive: bool = True, + name: str = "", + log: list[str] | None = None, ): self.calls: list[str] = [] self._fail_start = fail_start self._stop_returns = stop_returns self._start_returns_false = start_returns_false self._rollback_start_fails = rollback_start_fails + self._start_always_false = start_always_false + self._alive = alive + self._name = name + self._log = log self._start_count = 0 + def _note(self, method: str) -> None: + if self._log is not None: + self._log.append(f"{self._name}.{method}") + def stop(self) -> bool: self.calls.append("stop") + self._note("stop") return self._stop_returns def start(self) -> bool: self.calls.append("start") + self._note("start") self._start_count += 1 is_first = self._start_count == 1 + if self._start_always_false: + return False if is_first and self._fail_start: raise RuntimeError("boom on start") if is_first and self._start_returns_false: @@ -74,12 +97,27 @@ def start(self) -> bool: return False return True + def is_alive(self) -> bool: + # Deliberately does NOT append to .calls (it is not stop()/start()) but + # DOES append to the shared log, so ordering tests can filter it out. + if self._log is not None: + self._log.append(f"{self._name}.is_alive") + return self._alive + + +def _two_controls(**bridge_kwargs): + """A shared ordered-call log plus a named ("ctl", "bridge") FakeControl pair.""" + log: list[str] = [] + ctl = FakeControl(name="ctl", log=log) + bridge = FakeControl(name="bridge", log=log, **bridge_kwargs) + return log, ctl, bridge + class FakeLogger: """Captures log records by level so tests can assert on escalations.""" def __init__(self): - self.records: dict[str, list[str]] = {"info": [], "warning": [], "error": []} + self.records: dict[str, list[str]] = {"info": [], "warning": [], "error": [], "debug": []} def _record(self, level, msg, *args): try: @@ -96,6 +134,9 @@ def warning(self, msg, *args): def error(self, msg, *args): self._record("error", msg, *args) + def debug(self, msg, *args): + self._record("debug", msg, *args) + def text(self, level: str) -> str: return "\n".join(self.records[level]) @@ -299,6 +340,8 @@ def test_restore_rejects_zip_slip_member(tmp_path): fabric_backup.restore_backup(str(evil), fresh, control, now=_NOW) # the escape file must NOT have been written outside the target assert not (tmp_path / "escape.txt").exists() + # the zip-slip pre-flight fires before either control is touched + assert control.calls == [] # ---------------------------------------------------------------------- @@ -478,6 +521,28 @@ def boom_remove(path): assert "Could not prune" in logger.text("warning") +# ---------------------------------------------------------------------- +# C5 — _control_is_alive edge cases (fail-safe: default to alive) +# ---------------------------------------------------------------------- +def test_control_is_alive_defaults_true_without_an_is_alive_method(): + class _NoIsAlive: # a control without the OPTIONAL is_alive() extension + def stop(self): + return True + + def start(self): + return True + + assert fabric_backup._control_is_alive(_NoIsAlive(), FakeLogger()) is True + + +def test_control_is_alive_treats_a_raising_probe_as_alive_and_logs_debug(): + control = Mock() + control.is_alive.side_effect = RuntimeError("boom") + logger = FakeLogger() + assert fabric_backup._control_is_alive(control, logger) is True + assert "is_alive" in logger.text("debug") + + def _make_bridge_storage(tmp_path, *, files: dict[str, str] | None = None): """The export bridge node's storage dir — a SIBLING of the controller's.""" storage = tmp_path / "appsupport" / "bridge-node" @@ -548,7 +613,8 @@ def test_restore_leaves_the_bridge_members_alone_and_says_so(self, tmp_path): Extracting them under the CONTROLLER's storage dir would put a second Matter node's credentials where nothing will ever read them; doing it in - place needs the bridge node stopped, and there is no stop seam until E7. + place needs the bridge node stopped, and this restore, when the caller + supplies no bridge control, has no way to do that. So: restore the fabric, and name the files, the prefix and the manual recipe. """ @@ -651,3 +717,721 @@ def test_the_success_line_names_the_directory_it_covered(self, tmp_path): else str(call.args[0]) for call in logger.info.call_args_list) assert str(bridge) in said assert not logger.warning.called + + +def _bridge_restore_archive(tmp_path, *, bridge=True): + """A backup archive over the standard fixtures — optionally with bridge members.""" + storage = _make_storage(tmp_path) + bridge_path = _make_bridge_storage(tmp_path) if bridge else None + archive = fabric_backup.create_backup(storage, now=_NOW, bridge_storage_path=bridge_path) + return storage, archive + + +class TestBridgeRestore: + """#136 — restore now stops/starts the bridge node and extracts its half too. + + ``BridgeProcess`` gives ``restore_backup`` the ``stop()``/``start()`` seam it + was missing (E7); this pins the two-control sequence, the extraction, and + every way it falls back to the controller-only behaviour these tests' + siblings above already pin. + """ + + def _archive(self, tmp_path, *, bridge=True): + return _bridge_restore_archive(tmp_path, bridge=bridge) + + def test_bridge_members_extract_into_the_bridge_dir_with_the_prefix_stripped(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + _log, ctl, bridge = _two_controls() + + result = fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert result["bridge_restored"] is True + assert result["bridge_members"] == 3 + with open(os.path.join(bridge_dest, "identity.json")) as fh: + assert fh.read() == _IDENTITY_JSON + with open(os.path.join(bridge_dest, "endpoint-map.json")) as fh: + assert fh.read() == _ENDPOINT_MAP_JSON + with open(os.path.join(bridge_dest, "node-indigo-matter-bridge", "state.json")) as fh: + assert fh.read() == "{}" + + def test_nothing_named_bridge_node_appears_under_the_controller_storage(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + _log, ctl, bridge = _two_controls() + + fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + # Not nested under the controller's storage, and not double-nested under + # its own destination either (the prefix-strip pin). + assert not os.path.exists(os.path.join(fresh_storage, "bridge-node")) + assert not os.path.exists(os.path.join(bridge_dest, "bridge-node")) + + def test_stop_order_is_bridge_then_controller_and_start_order_is_the_reverse(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + log, ctl, bridge = _two_controls() + + fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered == ["bridge.stop", "ctl.stop", "ctl.start", "bridge.start"] + + def test_bridge_stop_failure_aborts_before_the_controller_is_touched(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + log, ctl, bridge = _two_controls(stop_returns=False) + + with pytest.raises(RuntimeError, match="bridge node failed to stop"): + fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert not any(entry.startswith("ctl.") for entry in log) + assert not os.path.exists(fresh_storage) + assert not os.path.exists(bridge_dest) + + def test_controller_stop_failure_restarts_the_bridge_it_stopped_then_aborts(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + log: list[str] = [] + ctl = FakeControl(name="ctl", log=log, stop_returns=False) + bridge = FakeControl(name="bridge", log=log) + + with pytest.raises(RuntimeError, match="failed to stop"): + fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered == ["bridge.stop", "ctl.stop", "bridge.start"] + assert not os.path.exists(fresh_storage) + assert not os.path.exists(bridge_dest) + + def test_controller_stop_failure_logs_when_the_abort_path_bridge_restart_also_fails(self, tmp_path): + """A2/F2: the abort-path bridge restart result must not be discarded.""" + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + log: list[str] = [] + ctl = FakeControl(name="ctl", log=log, stop_returns=False) + bridge = FakeControl(name="bridge", log=log, start_always_false=True) + logger = FakeLogger() + + with pytest.raises(RuntimeError, match="failed to stop"): + fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered == ["bridge.stop", "ctl.stop", "bridge.start"] + err = logger.text("error") + assert "bridge" in err.lower() + assert "did not restart" in err + + def test_a_bridge_that_is_not_running_is_neither_stopped_nor_started(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + log, ctl, bridge = _two_controls(alive=False) + logger = FakeLogger() + + result = fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert "bridge.stop" not in log + assert "bridge.start" not in log + assert os.path.isfile(os.path.join(bridge_dest, "identity.json")) + assert result["bridge_started"] is None + assert "left stopped" in logger.text("info") + + def test_a_bridge_that_cannot_start_does_not_fail_an_otherwise_good_restore(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + _log, ctl, bridge = _two_controls(start_always_false=True) + logger = FakeLogger() + + result = fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert result["bridge_restored"] is True + assert result["bridge_started"] is False + assert os.path.isfile(os.path.join(fresh_storage, "config")) + err = logger.text("error") + assert bridge_dest in err + parent = os.path.dirname(bridge_dest) + assert not any(".failed-" in name for name in os.listdir(parent)) + + def test_a_failing_bridge_extract_rolls_both_dirs_back(self, tmp_path, monkeypatch): + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + log, ctl, bridge = _two_controls() + + real_extract = fabric_backup._safe_extract + + def flaky_extract(archive_path, dest, *, prefix=""): + if prefix: + raise RuntimeError("boom on bridge extract") + return real_extract(archive_path, dest, prefix=prefix) + + monkeypatch.setattr(fabric_backup, "_safe_extract", flaky_extract) + + with pytest.raises(RuntimeError, match="rolled back"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + with open(os.path.join(storage, "config")) as fh: + assert fh.read() == "fabric-config" + with open(os.path.join(bridge_dest, "identity.json")) as fh: + assert fh.read() == _IDENTITY_JSON + parent = os.path.dirname(bridge_dest) + assert any(name.startswith("bridge-node.failed-") for name in os.listdir(parent)) + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered[-2:] == ["ctl.start", "bridge.start"] + + def test_a_failing_controller_extract_rolls_the_bridge_back_too(self, tmp_path, monkeypatch): + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + _log, ctl, bridge = _two_controls() + + real_extract = fabric_backup._safe_extract + + def flaky_extract(archive_path, dest, *, prefix=""): + if not prefix: + raise RuntimeError("boom on controller extract") + return real_extract(archive_path, dest, prefix=prefix) + + monkeypatch.setattr(fabric_backup, "_safe_extract", flaky_extract) + + with pytest.raises(RuntimeError, match="rolled back"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + with open(os.path.join(bridge_dest, "identity.json")) as fh: + assert fh.read() == _IDENTITY_JSON + parent = os.path.dirname(bridge_dest) + assert not any(name.startswith("bridge-node.pre-restore-") for name in os.listdir(parent)) + + def test_the_bridge_dir_is_moved_aside_never_deleted(self, tmp_path): + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + # mutate the live bridge storage so we can prove the backup (not the live + # dir) won, same discipline as the controller's move-aside test above. + with open(os.path.join(bridge_dest, "identity.json"), "w") as fh: + fh.write("MUTATED") + _log, ctl, bridge = _two_controls() + + result = fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + moved = result["bridge_moved_aside_to"] + assert moved is not None + assert os.path.isdir(moved) + with open(os.path.join(moved, "identity.json")) as fh: + assert fh.read() == "MUTATED" + with open(os.path.join(bridge_dest, "identity.json")) as fh: + assert fh.read() == _IDENTITY_JSON + + def test_zip_slip_in_a_bridge_member_is_rejected_before_anything_is_stopped(self, tmp_path): + evil = tmp_path / "evil.zip" + with zipfile.ZipFile(evil, "w") as zf: + zf.writestr("config", "x") + zf.writestr(f"{fabric_backup.BRIDGE_MEMBER_PREFIX}../../escape.txt", "pwned") + # The one-dot variant: escapes only once the prefix is stripped — + # the raw archive-relative name still resolves inside the + # archive root, which is exactly why the check must be done on + # the WRITTEN (prefix-stripped) path, not the raw member name. + zf.writestr(f"{fabric_backup.BRIDGE_MEMBER_PREFIX}../escape2.txt", "pwned too") + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + log, ctl, bridge = _two_controls() + + with pytest.raises(ValueError, match="zip-slip"): + fabric_backup.restore_backup( + str(evil), fresh_storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert log == [] + assert not (tmp_path / "escape.txt").exists() + assert not (tmp_path / "escape2.txt").exists() + assert not (tmp_path / "restored" / "escape2.txt").exists() + + def test_bridge_members_are_skipped_when_no_bridge_control_is_given(self, tmp_path): + storage, archive = self._archive(tmp_path) + logger = FakeLogger() + control = FakeControl() + + result = fabric_backup.restore_backup(archive, storage, control, now=_NOW, logger=logger) + + assert result["bridge_restored"] is False + assert result["bridge_members"] == 3 + warning = logger.text("warning") + assert fabric_backup.BRIDGE_MEMBER_PREFIX in warning + assert "Stop the Matter bridge" in warning + # C12: the reason clause names the actual cause, not just the recipe. + assert "this restore was given no way to stop the bridge node" in warning + assert os.path.isfile(os.path.join(storage, "config")) + + def test_a_controller_only_archive_never_touches_the_bridge_control(self, tmp_path): + _storage, archive = self._archive(tmp_path, bridge=False) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + result = fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert result["bridge_restored"] is False + assert not any(entry.startswith("bridge.") for entry in log) # not even is_alive + assert "bridge" not in logger.text("warning").lower() + + def test_a_bridge_dest_inside_the_controller_storage_is_refused_not_extracted(self, tmp_path): + storage, archive = self._archive(tmp_path) + bridge_dest = os.path.join(storage, "bridge-node") # INSIDE the controller's storage + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + result = fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert result["bridge_restored"] is False + assert "overlaps" in logger.text("warning") + assert not os.path.exists(os.path.join(storage, "bridge-node")) + assert not any(entry.startswith("bridge.") for entry in log) + assert os.path.isfile(os.path.join(storage, "config")) + + def test_a_failing_bridge_restart_during_rollback_is_loud_but_not_fatal(self, tmp_path): + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + with open(os.path.join(storage, "config"), "w") as fh: + fh.write("ORIGINAL-LIVE") + log: list[str] = [] + ctl = FakeControl(name="ctl", log=log, fail_start=True) + bridge = FakeControl(name="bridge", log=log, start_always_false=True) + logger = FakeLogger() + + with pytest.raises(RuntimeError, match="rolled back"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + with open(os.path.join(storage, "config")) as fh: + assert fh.read() == "ORIGINAL-LIVE" + err = logger.text("error") + assert "bridge" in err.lower() + assert "pairings are intact" in err + + def test_a_failing_bridge_move_aside_undoes_the_controller_move_and_restarts_both(self, tmp_path): + """F1 (HIGH): move-aside must be all-or-nothing and recoverable. + + A controller rename that succeeds followed by a bridge rename that + raises must not strand the controller fabric aside with both daemons + stopped — the controller move is undone and both daemons come back. + """ + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + with open(os.path.join(storage, "config"), "w") as fh: + fh.write("ORIGINAL-LIVE") + # Pre-create the bridge's collision dir WITH CONTENT (the fixed clock + # makes the stamp deterministic) — an empty dir renames over fine on + # some platforms, so this must be non-empty to guarantee the OSError. + collision = f"{bridge_dest}.pre-restore-{fabric_backup._stamp(_NOW)}" + os.makedirs(collision) + with open(os.path.join(collision, "stray.txt"), "w") as fh: + fh.write("leftover") + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + with pytest.raises(RuntimeError, match="moving the existing storage aside"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + # controller storage back IN PLACE with its original content — the + # rename was undone, not left aside. + with open(os.path.join(storage, "config")) as fh: + assert fh.read() == "ORIGINAL-LIVE" + parent = os.path.dirname(storage) + stem = os.path.basename(storage) + assert not any(name.startswith(f"{stem}.pre-restore-") for name in os.listdir(parent)) + # both daemons were restarted, best-effort, in that order. + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered[-2:] == ["ctl.start", "bridge.start"] + + def test_a_failing_bridge_move_aside_restart_failures_are_logged(self, tmp_path): + """Mutation guard: removing the restarts must be caught by a test.""" + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + collision = f"{bridge_dest}.pre-restore-{fabric_backup._stamp(_NOW)}" + os.makedirs(collision) + with open(os.path.join(collision, "stray.txt"), "w") as fh: + fh.write("leftover") + log: list[str] = [] + ctl = FakeControl(name="ctl", log=log, start_always_false=True) + bridge = FakeControl(name="bridge", log=log, start_always_false=True) + logger = FakeLogger() + + with pytest.raises(RuntimeError, match="moving the existing storage aside"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + # both starts were attempted even though both returned False. + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered[-2:] == ["ctl.start", "bridge.start"] + err = logger.text("error") + assert "matter-server" in err.lower() + assert "bridge" in err.lower() + + def test_move_aside_single_fault_raising_start_is_still_reported_and_bridge_restarted(self, tmp_path): + """F1: server_control.start() in the move-aside except handler was the + ONE unguarded start() in the module — a raise there used to propagate + straight past the CRITICAL log line, the bridge restart, and the + wrapped RuntimeError. It must be caught like _rollback's start() is. + """ + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + with open(os.path.join(storage, "config"), "w") as fh: + fh.write("ORIGINAL-LIVE") + collision = f"{bridge_dest}.pre-restore-{fabric_backup._stamp(_NOW)}" + os.makedirs(collision) + with open(os.path.join(collision, "stray.txt"), "w") as fh: + fh.write("leftover") + log: list[str] = [] + ctl = FakeControl(name="ctl", log=log, fail_start=True) # start() raises + bridge = FakeControl(name="bridge", log=log) + logger = FakeLogger() + + with pytest.raises(RuntimeError, match="moving the existing storage aside"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + err = logger.text("error") + assert "CRITICAL" in err + assert "restart raised" in err + # the bridge was still restarted despite the controller's start() raising. + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered[-2:] == ["ctl.start", "bridge.start"] + + def test_move_aside_double_fault_skips_controller_start_and_names_the_aside_path(self, tmp_path, monkeypatch): + """F2: the double fault (bridge rename AND its undo both fail) must not + be treated like the single fault. Starting matter-server here would + recreate storage_path as a FRESH EMPTY fabric, blocking the manual + `mv` the error message prescribes — so the controller start must be + SKIPPED. The bridge's own dir was never touched, so it still restarts. + """ + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + with open(os.path.join(storage, "config"), "w") as fh: + fh.write("ORIGINAL-LIVE") + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + moved_aside_to = f"{storage}.pre-restore-{fabric_backup._stamp(_NOW)}" + real_rename = fabric_backup.os.rename + + def flaky_rename(src, dst): + if src == bridge_dest: + raise OSError("simulated bridge rename failure") + if src == moved_aside_to and dst == storage: + raise OSError("simulated undo failure") + return real_rename(src, dst) + + monkeypatch.setattr(fabric_backup.os, "rename", flaky_rename) + + with pytest.raises(RuntimeError) as excinfo: + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + msg = str(excinfo.value) + assert moved_aside_to in msg + assert "stop matter-server" in msg + # the controller start must NOT have been attempted. + assert "ctl.start" not in log + # the bridge's own dir was never touched by this fault, so it still + # restarts best-effort. + assert "bridge.start" in log + err = logger.text("error") + assert "with matter-server stopped" in err + + # ------------------------------------------------------------------ + # C1 — R1 bridge-rollback-mechanics failure must not abort the + # controller's rollback, and the two message shapes fire correctly. + # ------------------------------------------------------------------ + def test_bridge_rollback_mechanics_failure_does_not_abort_controller_rollback(self, tmp_path, monkeypatch): + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + with open(os.path.join(storage, "config"), "w") as fh: + fh.write("ORIGINAL-LIVE") + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + real_extract = fabric_backup._safe_extract + + def flaky_extract(archive_path, dest, *, prefix=""): + if prefix: + raise RuntimeError("boom on bridge extract") + return real_extract(archive_path, dest, prefix=prefix) + + monkeypatch.setattr(fabric_backup, "_safe_extract", flaky_extract) + + real_rename = fabric_backup.os.rename + + def flaky_rename(src, dst): + # Fail ONLY the bridge rollback's rename-back (moved_aside_to -> + # bridge.dest) — scoped by both src and dst so nothing else + # (the controller's own renames, the bridge's failed-aside + # rename) is affected, keeping the test order-independent. + if os.path.basename(src).startswith("bridge-node.pre-restore-") and dst == bridge_dest: + raise OSError("simulated rename-back failure") + return real_rename(src, dst) + + monkeypatch.setattr(fabric_backup.os, "rename", flaky_rename) + + with pytest.raises(RuntimeError, match="rolled back"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + # the controller rollback STILL completed despite the bridge failure. + with open(os.path.join(storage, "config")) as fh: + assert fh.read() == "ORIGINAL-LIVE" + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert "ctl.start" in ordered + err = logger.text("error") + assert "PRESERVED at" in err + assert "move it back" in err + + # ------------------------------------------------------------------ + # C2 — a raising bridge start() after an otherwise-successful restore + # is reported, never swallowed. + # ------------------------------------------------------------------ + def test_bridge_restart_raise_after_success_is_reported_not_swallowed(self, tmp_path): + _storage, archive = self._archive(tmp_path) + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + _log, ctl, bridge = _two_controls(fail_start=True) + logger = FakeLogger() + + result = fabric_backup.restore_backup( + archive, fresh_storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert result["bridge_restored"] is True + assert result["bridge_started"] is False + assert "did not start" in logger.text("error") + assert "restart raised" in logger.text("debug") + + # ------------------------------------------------------------------ + # C3 — fallback (b): a bridge control WAS given, but there is no usable + # bridge storage path. + # ------------------------------------------------------------------ + def test_bridge_control_given_but_no_storage_path_warns_and_skips(self, tmp_path): + storage, archive = self._archive(tmp_path) + logger = FakeLogger() + control = FakeControl() + bridge = FakeControl() + + result = fabric_backup.restore_backup( + archive, storage, control, now=_NOW, logger=logger, + bridge_storage_path=None, bridge_control=bridge) + + assert result["bridge_restored"] is False + warning = logger.text("warning") + assert "no usable bridge storage path" in warning + assert os.path.isfile(os.path.join(storage, "config")) + + # ------------------------------------------------------------------ + # C4 — bridge-side H4: an emptied bridge extraction must refuse and + # roll BOTH sides back, not just skip the bridge. + # ------------------------------------------------------------------ + def test_bridge_side_empty_extraction_refuses_and_rolls_both_dirs_back(self, tmp_path, monkeypatch): + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + with open(os.path.join(storage, "config"), "w") as fh: + fh.write("ORIGINAL-LIVE") + with open(os.path.join(bridge_dest, "identity.json"), "w") as fh: + fh.write("ORIGINAL-BRIDGE") + log, ctl, bridge = _two_controls() + + real_extract = fabric_backup._safe_extract + + def noop_bridge_extract(archive_path, dest, *, prefix=""): + if prefix: + return None # extracts nothing — the H4 empty-result case + return real_extract(archive_path, dest, prefix=prefix) + + monkeypatch.setattr(fabric_backup, "_safe_extract", noop_bridge_extract) + + with pytest.raises(RuntimeError, match="rolled back"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + with open(os.path.join(storage, "config")) as fh: + assert fh.read() == "ORIGINAL-LIVE" + with open(os.path.join(bridge_dest, "identity.json")) as fh: + assert fh.read() == "ORIGINAL-BRIDGE" + ordered = [entry for entry in log if not entry.endswith(".is_alive")] + assert ordered[-2:] == ["ctl.start", "bridge.start"] + + # ------------------------------------------------------------------ + # C7 — the other two overlap clauses (the third is already pinned by + # test_a_bridge_dest_inside_the_controller_storage_is_refused_not_extracted). + # ------------------------------------------------------------------ + def test_bridge_dest_identical_to_controller_storage_is_refused(self, tmp_path): + storage, archive = self._archive(tmp_path) + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + result = fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=storage, bridge_control=bridge) + + assert result["bridge_restored"] is False + # C12: the reason clause names both paths, not just "overlaps". + assert f"the bridge storage path {storage} overlaps the controller's" in logger.text("warning") + assert not any(entry.startswith("bridge.") for entry in log) + assert os.path.isfile(os.path.join(storage, "config")) + + def test_controller_storage_inside_the_bridge_dest_is_refused(self, tmp_path): + storage, archive = self._archive(tmp_path) + bridge_dest = os.path.dirname(storage) # storage_path is INSIDE this + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + result = fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert result["bridge_restored"] is False + assert "overlaps" in logger.text("warning") + assert not any(entry.startswith("bridge.") for entry in log) + assert os.path.isfile(os.path.join(storage, "config")) + + # ------------------------------------------------------------------ + # C8 — directory entries in the archive (not just files) extract cleanly + # on the prefix-stripping (bridge) path. + # ------------------------------------------------------------------ + def test_bridge_side_directory_entries_extract_cleanly(self, tmp_path): + archive = tmp_path / "manual.zip" + prefix = fabric_backup.BRIDGE_MEMBER_PREFIX + with zipfile.ZipFile(archive, "w") as zf: + zf.writestr("config", "controller-config") + zf.writestr(prefix, "") # the bare prefix dir entry itself + zf.writestr(f"{prefix}sub/", "") # a nested dir entry + zf.writestr(f"{prefix}sub/file.json", '{"a": 1}') + fresh_storage = str(tmp_path / "restored" / "matter-server") + bridge_dest = str(tmp_path / "restored" / "bridge-node") + _log, ctl, bridge = _two_controls() + + result = fabric_backup.restore_backup( + str(archive), fresh_storage, ctl, now=_NOW, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + assert result["bridge_restored"] is True + # the bare prefix entry did not become a stray file/dir named "". + assert sorted(os.listdir(bridge_dest)) == ["sub"] + with open(os.path.join(bridge_dest, "sub", "file.json")) as fh: + assert fh.read() == '{"a": 1}' + + # ------------------------------------------------------------------ + # C10 — the pre-existing controller rollback-mechanics CRITICAL branch, + # first tested here, WITH the bridge_note appended. + # ------------------------------------------------------------------ + def test_controller_rollback_mechanics_failure_appends_the_bridge_note(self, tmp_path, monkeypatch): + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "appsupport" / "bridge-node") + with open(os.path.join(storage, "config"), "w") as fh: + fh.write("ORIGINAL-LIVE") + log: list[str] = [] + ctl = FakeControl(name="ctl", log=log, fail_start=True) # forces rollback + bridge = FakeControl(name="bridge", log=log) + logger = FakeLogger() + + real_rename = fabric_backup.os.rename + pre_restore_storage = f"{storage}.pre-restore-{fabric_backup._stamp(_NOW)}" + + def flaky_rename(src, dst): + # Fail ONLY the controller rollback's rename-back + # (pre_restore_storage -> storage), leaving every other rename + # (the bridge's own, the controller's move-aside-in) untouched + # so the test is not order-fragile. + if src == pre_restore_storage and dst == storage: + raise OSError("simulated controller rename-back failure") + return real_rename(src, dst) + + monkeypatch.setattr(fabric_backup.os, "rename", flaky_rename) + + with pytest.raises(OSError): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + err = logger.text("error") + assert "CRITICAL" in err + assert "The Matter bridge node was stopped for this restore and is still stopped." in err + + # ------------------------------------------------------------------ + # F4 — the no-previous-copy shape of _bridge_rollback_failure_message + # had no coverage: deleting that branch left the suite green. + # ------------------------------------------------------------------ + def test_bridge_rollback_message_when_there_was_no_previous_bridge_copy(self, tmp_path, monkeypatch): + """A bridge dest that did NOT exist before this restore (so + ``moved_aside_to`` is None) whose failed-aside rename during rollback + itself fails must report "No previous bridge storage" rather than the + other shape's false claim that a previous copy is PRESERVED. + """ + storage, archive = self._archive(tmp_path) + bridge_dest = str(tmp_path / "restored" / "bridge-node") # does NOT pre-exist + log, ctl, bridge = _two_controls() + logger = FakeLogger() + + real_extract = fabric_backup._safe_extract + + def flaky_extract(archive_path, dest, *, prefix=""): + if prefix: + raise RuntimeError("boom on bridge extract") + return real_extract(archive_path, dest, prefix=prefix) + + monkeypatch.setattr(fabric_backup, "_safe_extract", flaky_extract) + + real_rename = fabric_backup.os.rename + + def flaky_rename(src, dst): + if src == bridge_dest and str(dst).startswith(f"{bridge_dest}.failed-"): + raise OSError("simulated failed-aside rename failure") + return real_rename(src, dst) + + monkeypatch.setattr(fabric_backup.os, "rename", flaky_rename) + + with pytest.raises(RuntimeError, match="rolled back"): + fabric_backup.restore_backup( + archive, storage, ctl, now=_NOW, logger=logger, + bridge_storage_path=bridge_dest, bridge_control=bridge) + + err = logger.text("error") + assert "No previous bridge storage predated this restore" in err + assert "PRESERVED" not in err diff --git a/tests/test_plugin_behaviour.py b/tests/test_plugin_behaviour.py index 9dc3090..1ce330e 100644 --- a/tests/test_plugin_behaviour.py +++ b/tests/test_plugin_behaviour.py @@ -10,6 +10,7 @@ import asyncio import importlib import json +import os from pathlib import Path from concurrent.futures import TimeoutError as FuturesTimeoutError from types import SimpleNamespace @@ -38,6 +39,7 @@ def plug(plugin_mod): p.jobs = None p.pluginPrefs = {} p.export_bridge = None + p.bridge_process = None p._exported_ids = frozenset() p._subscribed_to_devices = False return p @@ -762,6 +764,196 @@ def fake_start(): assert (Path(storage) / "config").read_text() == "ORIGINAL-LIVE" +# --------------------------------------------------------------------------- +# Restore now wires the bridge node's stop()/start() seam too (#136) +# --------------------------------------------------------------------------- +def _fake_restore_result(archive_path, *, bridge_restored=False, bridge_members=0, + bridge_moved_aside_to=None, bridge_started=None): + return { + "restored_from": archive_path, + "moved_aside_to": None, + "bridge_restored": bridge_restored, + "bridge_members": bridge_members, + "bridge_moved_aside_to": bridge_moved_aside_to, + "bridge_started": bridge_started, + } + + +def test_menu_restore_passes_the_bridge_control_and_path(plug, tmp_path, monkeypatch): + import fabric_backup + storage = _storage_with_fabric(tmp_path) + seen = {} + + def fake_restore_backup(archive_path, storage_path, server_control, *, now, logger=None, + bridge_storage_path=None, bridge_control=None): + seen["bridge_storage_path"] = bridge_storage_path + seen["bridge_control"] = bridge_control + return _fake_restore_result(archive_path) + + monkeypatch.setattr(fabric_backup, "restore_backup", fake_restore_backup) + plug.server_process = SimpleNamespace(storage_path=storage) + plug.pluginPrefs = {} + + ok, _vd = plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + + assert ok is True + bridge_path = seen["bridge_storage_path"] + assert bridge_path.endswith("bridge-node") + assert os.path.dirname(bridge_path) == os.path.dirname(storage) # a sibling of storage_path + control = seen["bridge_control"] + assert hasattr(control, "stop") and hasattr(control, "start") and hasattr(control, "is_alive") + + +def test_menu_restore_builds_a_bridge_control_when_the_session_never_exported(plug, tmp_path, monkeypatch): + import bridge_agent + import fabric_backup + storage = _storage_with_fabric(tmp_path) + seen = {} + + def fake_restore_backup(archive_path, storage_path, server_control, *, now, logger=None, + bridge_storage_path=None, bridge_control=None): + seen["bridge_control"] = bridge_control + return _fake_restore_result(archive_path) + + monkeypatch.setattr(fabric_backup, "restore_backup", fake_restore_backup) + plug.server_process = SimpleNamespace(storage_path=storage) + plug.pluginPrefs = {} + assert plug.bridge_process is None + + plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + + assert isinstance(seen["bridge_control"], bridge_agent.BridgeProcess) + assert plug.bridge_process is seen["bridge_control"] # kept, not thrown away + + +def test_menu_restore_reuses_an_existing_bridge_process(plug, tmp_path, monkeypatch): + import fabric_backup + storage = _storage_with_fabric(tmp_path) + sentinel = Mock() + plug.bridge_process = sentinel + seen = {} + + def fake_restore_backup(archive_path, storage_path, server_control, *, now, logger=None, + bridge_storage_path=None, bridge_control=None): + seen["bridge_control"] = bridge_control + return _fake_restore_result(archive_path) + + monkeypatch.setattr(fabric_backup, "restore_backup", fake_restore_backup) + plug.server_process = SimpleNamespace(storage_path=storage) + plug.pluginPrefs = {} + + plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + + assert seen["bridge_control"] is sentinel + + +def test_menu_restore_falls_back_to_no_bridge_control_when_one_cannot_be_built(plug, tmp_path, monkeypatch): + import bridge_agent + import fabric_backup + storage = _storage_with_fabric(tmp_path) + seen = {} + + def boom(*_a, **_k): + raise RuntimeError("no bridge_protocol on this build") + + monkeypatch.setattr(bridge_agent, "BridgeProcess", boom) + + def fake_restore_backup(archive_path, storage_path, server_control, *, now, logger=None, + bridge_storage_path=None, bridge_control=None): + seen["bridge_control"] = bridge_control + return _fake_restore_result(archive_path) + + monkeypatch.setattr(fabric_backup, "restore_backup", fake_restore_backup) + plug.server_process = SimpleNamespace(storage_path=storage) + plug.pluginPrefs = {} + + ok, _vd = plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + + assert ok is True # a bridge control we can't build never blocks the controller restore + assert seen["bridge_control"] is None + plug.logger.warning.assert_called() + + +def test_menu_restore_never_clears_the_XAC1_latch(plug, tmp_path, monkeypatch): + """menuRestoreFabricBackup must not call note_agent_stopped() — see plugin.py's + call-site comment: restore uses stop()/start(), never uninstall(), so the + XAC1 latch's claim ("this session started the bridge agent") stays true + whether or not this restore touched a live bridge.""" + import fabric_backup + storage = _storage_with_fabric(tmp_path) + plug.export_bridge = Mock() + plug.server_process = SimpleNamespace(storage_path=storage) + plug.pluginPrefs = {} + + monkeypatch.setattr(fabric_backup, "restore_backup", + lambda archive_path, *_a, **_k: _fake_restore_result(archive_path)) + plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + plug.export_bridge.note_agent_stopped.assert_not_called() + + def fake_restore_fail(*_a, **_k): + raise RuntimeError("boom") + + monkeypatch.setattr(fabric_backup, "restore_backup", fake_restore_fail) + plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + plug.export_bridge.note_agent_stopped.assert_not_called() + + +def test_menu_restore_reports_the_bridge_outcome_honestly(plug, tmp_path, monkeypatch): + import fabric_backup + storage = _storage_with_fabric(tmp_path) + plug.server_process = SimpleNamespace(storage_path=storage) + plug.pluginPrefs = {} + + monkeypatch.setattr(fabric_backup, "restore_backup", + lambda archive_path, *_a, **_k: _fake_restore_result( + archive_path, bridge_restored=True, bridge_members=3, + bridge_moved_aside_to="/x/bridge-node.pre-restore-1", bridge_started=True)) + ok, _vd = plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + assert ok is True + msg = " ".join(str(a) for c in plug.logger.info.call_args_list for a in c.args) + assert "bridge" in msg.lower() + assert "endpoint" in msg.lower() + assert "drift" in msg.lower() + # C6: a pre-existing bridge dir must be named, not silently dropped. + assert "/x/bridge-node.pre-restore-1" in msg + + plug.logger.reset_mock() + # Force the read-only diagnosis to come back empty so this leg pins the + # deterministic FALLBACK text rather than a real (environment-dependent) + # LaunchAgent diagnosis. + monkeypatch.setattr(plug, "_bridge_agent_diagnosis", lambda: None) + monkeypatch.setattr(fabric_backup, "restore_backup", + lambda archive_path, *_a, **_k: _fake_restore_result( + archive_path, bridge_restored=True, bridge_members=3, + bridge_moved_aside_to="/x/bridge-node.pre-restore-1", bridge_started=False)) + ok, _vd = plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + assert ok is True # the controller fabric still restored — only the bridge didn't come back + plug.logger.error.assert_called() + err_msg = " ".join(str(a) for c in plug.logger.error.call_args_list for a in c.args) + assert "did not come back up" in err_msg + assert "Check the bridge node's error log." in err_msg + + # C6 (cb395f8 pin): no pre-existing bridge dir AND the bridge was never + # stopped for this restore — the BRIDGE line must say nothing rather + # than "preserved at None" and must not mention "restarted" either. + # (The FIRST info() call is the pre-existing, out-of-scope controller + # line — scope this assertion to the bridge-specific call so it is not + # confused by that unrelated "None".) + plug.logger.reset_mock() + monkeypatch.setattr(fabric_backup, "restore_backup", + lambda archive_path, *_a, **_k: _fake_restore_result( + archive_path, bridge_restored=True, bridge_members=3, + bridge_moved_aside_to=None, bridge_started=None)) + ok, _vd = plug.menuRestoreFabricBackup({"backup": "/x.zip", "confirm": True}, "restoreFabricBackup") + assert ok is True + bridge_msg = " ".join(str(a) for a in plug.logger.info.call_args_list[-1].args) + assert "None" not in bridge_msg + assert "preserved" not in bridge_msg.lower() + # C11: bridge_started=None (never stopped, so never restarted) reads + # honestly — no claim that it "has been restarted". + assert "restarted" not in bridge_msg.lower() + + # --------------------------------------------------------------------------- # Decommission menu (getMatterNodes picker + menuDecommissionDevice) # ---------------------------------------------------------------------------