mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-06 19:28:06 +00:00
fix(backup): make a restored device match the one that was backed up
Found by wiping a working device and reinstalling from scratch. Every problem here is invisible until you actually do that, which is why a green test suite and eleven hours of uptime had not surfaced any of them. **Four enabled plugins vanished on restore.** Weather, stocks, music and leaderboard have a registry `id` that differs from the `id` in their own manifest: the registry calls them `weather`, everything else calls them `ledmatrix-weather`. Installation already prefers the manifest id for the directory name and warns when the two disagree, so on disk, in config.json and in a backup they are `ledmatrix-weather` -- but nothing resolved that in reverse. Restore asked the store for `ledmatrix-weather` and got "Plugin not found in registry", four times, and the device came back missing four plugins the user had enabled. Registry lookup now falls back to matching `plugin_path`, which already records `plugins/ledmatrix-weather`. Renaming the published ids would have orphaned `plugin_state.json` entries keyed on the old ones. Exact id still wins, so a path that collides with another entry's id cannot shadow it. Against the live registry and a real 28-plugin install this takes unresolvable directories from five to one -- the one being starlark-apps, which is genuinely not in the registry. **Secrets could not be restored at all.** A fresh install left config_secrets.json group-readable but not group-writable, and the web interface -- which is what performs a restore -- does not necessarily run as the owner. Every other file in the backup restored; secrets failed with EACCES. Now group-writable, so the account running the web UI can put them back. **A partial restore reported "Restore had errors" and nothing else.** That is the same message whether the whole thing failed or it quietly dropped your API keys. It now names what was restored, what failed, and which plugins were not reinstalled. **ytm_auth.json was never in the backup.** It sits in config/ beside the three files that are, and is pure device-local auth: losing it silently signs the user out of YouTube Music. Backed up and restored with the wifi config, which it resembles. **Backups were written inside the directory a reinstall deletes.** config/backups/exports is destroyed by the reinstall the user was told to make it before. Exports now go beside the install, falling back to the old path when that is not writable. **The installer reboots without asking in non-interactive mode**, which the README did not mention -- easy to hit when piping the install, and alarming when a device you are installing onto disappears. Documented, with --no-reboot-prompt. Its log also claimed root:ledmatrix while printing a hardcoded group name rather than the one it used. Tests: registry resolution gets its own suite, including the collision case and third-party entries with an empty plugin_path. The existing round-trip test passed throughout this because its fixture plugin has a directory name equal to its id -- the one shape that cannot fail -- so it now carries ytm_auth too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
This commit is contained in:
co-authored by
Claude Opus 5
parent
2af41c561b
commit
ffd706a454
@@ -357,6 +357,12 @@ sudo bash ./first_time_install.sh
|
||||
|
||||
This single script installs services, dependencies, configures permissions and sudoers, and validates the setup.
|
||||
|
||||
It finishes by asking whether to reboot. If you run it non-interactively — piped, over a script, or with `-y` — there is no one to ask, so **it reboots immediately without prompting**. Pass `--no-reboot-prompt` to install without rebooting:
|
||||
|
||||
```bash
|
||||
sudo bash ./first_time_install.sh -y --no-reboot-prompt
|
||||
```
|
||||
|
||||
</details>
|
||||
|
||||
</details>
|
||||
|
||||
+10
-3
@@ -1497,13 +1497,20 @@ if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then
|
||||
if [ "$SERVICE_USER" = "root" ]; then
|
||||
# Service runs as root - set ownership to root so it can read as owner
|
||||
chown "root:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
|
||||
echo "✓ Secrets file permissions set (root:ledmatrix for root service)"
|
||||
echo "✓ Secrets file permissions set (root:$LEDMATRIX_GROUP for root service)"
|
||||
else
|
||||
# Service runs as regular user - use ACTUAL_USER and rely on group membership
|
||||
chown "$ACTUAL_USER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
|
||||
echo "✓ Secrets file permissions set ($ACTUAL_USER:ledmatrix)"
|
||||
echo "✓ Secrets file permissions set ($ACTUAL_USER:$LEDMATRIX_GROUP)"
|
||||
fi
|
||||
chmod 640 "$PROJECT_ROOT_DIR/config/config_secrets.json"
|
||||
# Group-writable, not just group-readable. The web interface performs
|
||||
# backup restores, and it does not necessarily run as the file's owner: on
|
||||
# a fresh install the secrets file ended up owned by root while the web
|
||||
# service ran as the login user, so restoring a backup failed with
|
||||
# "Permission denied: config_secrets.json" while every other file in the
|
||||
# same backup restored fine. Read-only for the group makes secrets the one
|
||||
# thing a restore cannot put back.
|
||||
chmod 660 "$PROJECT_ROOT_DIR/config/config_secrets.json"
|
||||
fi
|
||||
|
||||
# Set proper permissions for YTM auth file (readable by all users including root service)
|
||||
|
||||
@@ -82,6 +82,10 @@ BUNDLED_FONTS: frozenset[str] = frozenset({
|
||||
_CONFIG_REL = Path("config/config.json")
|
||||
_SECRETS_REL = Path("config/config_secrets.json")
|
||||
_WIFI_REL = Path("config/wifi_config.json")
|
||||
# Sits in config/ next to the three above and is pure user state — a
|
||||
# YouTube Music session that has to be re-authenticated by hand if lost.
|
||||
# It was omitted from backups, so a restore silently signed the user out.
|
||||
_YTM_REL = Path("config/ytm_auth.json")
|
||||
_FONTS_REL = Path("assets/fonts")
|
||||
_PLUGIN_UPLOADS_REL = Path("assets/plugins")
|
||||
_STATE_REL = Path("data/plugin_state.json")
|
||||
@@ -303,6 +307,9 @@ def create_backup(
|
||||
if (project_root / _WIFI_REL).exists():
|
||||
zf.write(project_root / _WIFI_REL, _WIFI_REL.as_posix())
|
||||
contents.append("wifi")
|
||||
if (project_root / _YTM_REL).exists():
|
||||
zf.write(project_root / _YTM_REL, _YTM_REL.as_posix())
|
||||
contents.append("ytm_auth")
|
||||
|
||||
# User-uploaded fonts.
|
||||
user_fonts = iter_user_fonts(project_root)
|
||||
@@ -348,6 +355,7 @@ def preview_backup_contents(project_root: Path) -> Dict[str, Any]:
|
||||
"has_config": (project_root / _CONFIG_REL).exists(),
|
||||
"has_secrets": (project_root / _SECRETS_REL).exists(),
|
||||
"has_wifi": (project_root / _WIFI_REL).exists(),
|
||||
"has_ytm_auth": (project_root / _YTM_REL).exists(),
|
||||
"user_fonts": [p.name for p in iter_user_fonts(project_root)],
|
||||
"plugin_uploads": len(iter_plugin_uploads(project_root)),
|
||||
"plugins": list_installed_plugins(project_root),
|
||||
@@ -429,6 +437,8 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]:
|
||||
detected.append("secrets")
|
||||
if _WIFI_REL.as_posix() in names:
|
||||
detected.append("wifi")
|
||||
if _YTM_REL.as_posix() in names:
|
||||
detected.append("ytm_auth")
|
||||
if any(n.startswith(_FONTS_REL.as_posix() + "/") for n in names):
|
||||
detected.append("fonts")
|
||||
if any(
|
||||
@@ -546,6 +556,18 @@ def restore_backup(
|
||||
elif (tmp_dir / _WIFI_REL).exists():
|
||||
result.skipped.append("wifi")
|
||||
|
||||
# YouTube Music session. Follows restore_wifi rather than getting its
|
||||
# own flag: it is device-local auth in the same sense, and a separate
|
||||
# toggle for one file would be noise in the restore dialog.
|
||||
if options.restore_wifi and (tmp_dir / _YTM_REL).exists():
|
||||
try:
|
||||
_copy_file(tmp_dir / _YTM_REL, project_root / _YTM_REL)
|
||||
result.restored.append("ytm_auth")
|
||||
except OSError as e:
|
||||
result.errors.append(f"Failed to restore ytm_auth.json: {e}")
|
||||
elif (tmp_dir / _YTM_REL).exists():
|
||||
result.skipped.append("ytm_auth")
|
||||
|
||||
# User fonts — skip anything that collides with a bundled font.
|
||||
tmp_fonts = tmp_dir / _FONTS_REL
|
||||
if options.restore_fonts and tmp_fonts.exists():
|
||||
|
||||
@@ -1143,7 +1143,7 @@ class PluginStoreManager:
|
||||
"""
|
||||
registry = self.fetch_registry()
|
||||
plugins = registry.get('plugins', []) or []
|
||||
plugin_info = next((p for p in plugins if p['id'] == plugin_id), None)
|
||||
plugin_info = self._match_registry_entry(plugins, plugin_id)
|
||||
|
||||
if not plugin_info:
|
||||
return None
|
||||
@@ -1183,6 +1183,37 @@ class PluginStoreManager:
|
||||
|
||||
return plugin_info
|
||||
|
||||
@staticmethod
|
||||
def _match_registry_entry(plugins: List[Dict], plugin_id: str) -> Optional[Dict]:
|
||||
"""Find a registry entry by its id, or by the directory it installs to.
|
||||
|
||||
Four shipped plugins have a registry ``id`` that differs from the ``id``
|
||||
in their own manifest: ``weather`` installs to ``plugins/ledmatrix-weather``,
|
||||
and likewise stocks, music and leaderboard. Installation already prefers
|
||||
the manifest id for the directory name, so on disk, in ``config.json``
|
||||
and in a backup manifest those plugins are called ``ledmatrix-weather``.
|
||||
|
||||
Only the registry calls them ``weather``, and nothing resolved that in
|
||||
reverse: restoring a backup asked the store for ``ledmatrix-weather``
|
||||
and got "Plugin not found in registry", silently dropping four enabled
|
||||
plugins from a restored device.
|
||||
|
||||
Matching ``plugin_path`` fixes it without renaming any published id,
|
||||
which would orphan ``plugin_state.json`` entries keyed on the old ones.
|
||||
Exact id always wins, so an entry whose *path* happens to collide with
|
||||
another entry's id cannot shadow it.
|
||||
"""
|
||||
if not plugin_id:
|
||||
return None
|
||||
exact = next((p for p in plugins if p.get('id') == plugin_id), None)
|
||||
if exact is not None:
|
||||
return exact
|
||||
for entry in plugins:
|
||||
path = (entry.get('plugin_path') or '').rstrip('/')
|
||||
if path and path.rsplit('/', 1)[-1] == plugin_id:
|
||||
return entry
|
||||
return None
|
||||
|
||||
def get_registry_info(self, plugin_id: str) -> Optional[Dict]:
|
||||
"""
|
||||
Get plugin information from the registry cache only (no GitHub API calls).
|
||||
@@ -1198,7 +1229,7 @@ class PluginStoreManager:
|
||||
"""
|
||||
registry = self.fetch_registry()
|
||||
plugins = registry.get('plugins', []) or []
|
||||
return next((p for p in plugins if p.get('id') == plugin_id), None)
|
||||
return self._match_registry_entry(plugins, plugin_id)
|
||||
|
||||
def install_plugin(self, plugin_id: str, branch: Optional[str] = None) -> bool:
|
||||
"""Install a plugin, keeping any existing install until the new one is
|
||||
|
||||
@@ -41,6 +41,13 @@ def _make_project(root: Path) -> Path:
|
||||
json.dumps({"ap_mode": {"ssid": "LEDMatrix"}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
# Device-local auth that lives in config/ like the three above. It was
|
||||
# omitted from backups, so a restore silently signed the user out of
|
||||
# YouTube Music and they had to re-authenticate by hand.
|
||||
(root / "config" / "ytm_auth.json").write_text(
|
||||
json.dumps({"token": "YTM-TOKEN"}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
fonts = root / "assets" / "fonts"
|
||||
fonts.mkdir(parents=True)
|
||||
@@ -240,6 +247,10 @@ def test_restore_roundtrip(project: Path, empty_project: Path, tmp_path: Path) -
|
||||
restored_secrets = json.loads((empty_project / "config" / "config_secrets.json").read_text())
|
||||
assert restored_secrets["ledmatrix-weather"]["api_key"] == "SECRET"
|
||||
|
||||
assert "ytm_auth" in result.restored
|
||||
restored_ytm = json.loads((empty_project / "config" / "ytm_auth.json").read_text())
|
||||
assert restored_ytm["token"] == "YTM-TOKEN"
|
||||
|
||||
# User font restored, bundled font untouched.
|
||||
assert (empty_project / "assets" / "fonts" / "my-custom-font.ttf").read_bytes() == b"\x00\x01USER"
|
||||
assert (empty_project / "assets" / "fonts" / "5x7.bdf").read_text() == "BUNDLED"
|
||||
|
||||
@@ -0,0 +1,107 @@
|
||||
"""A plugin must be findable in the registry by the id it calls itself.
|
||||
|
||||
Four shipped plugins have a registry ``id`` that differs from the ``id`` in
|
||||
their own ``manifest.json``:
|
||||
|
||||
directory / manifest.json id registry id
|
||||
ledmatrix-weather weather
|
||||
ledmatrix-stocks stocks
|
||||
ledmatrix-music music
|
||||
ledmatrix-leaderboard leaderboard
|
||||
|
||||
The installer already knows about this: it deliberately names the install
|
||||
directory after the *manifest* id (store_manager, "Use manifest ID for
|
||||
directory name"), and warns when the two disagree. So on disk, in
|
||||
``config.json`` and in a backup manifest, these plugins are called
|
||||
``ledmatrix-weather``. Only the registry calls them ``weather``.
|
||||
|
||||
Nothing resolved that in reverse. Asking the store to install
|
||||
``ledmatrix-weather`` -- which is exactly what restoring a backup does --
|
||||
failed with "Plugin not found in registry", and four enabled plugins went
|
||||
missing from a restored device with no error surfaced to the user.
|
||||
|
||||
Renaming the registry ids would orphan existing ``plugin_state.json`` entries
|
||||
keyed on the old ones, so the lookup resolves ``plugin_path`` instead: the
|
||||
registry already records ``plugins/ledmatrix-weather``, which is unambiguous
|
||||
and needs no published identity to change.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
|
||||
|
||||
from src.plugin_system.store_manager import PluginStoreManager # noqa: E402
|
||||
|
||||
|
||||
# Shaped like the real registry: id and plugin_path basename disagree for the
|
||||
# first entry, agree for the second.
|
||||
REGISTRY: Dict[str, List[Dict[str, Any]]] = {
|
||||
"plugins": [
|
||||
{
|
||||
"id": "weather",
|
||||
"name": "Weather",
|
||||
"plugin_path": "plugins/ledmatrix-weather",
|
||||
"repo": "https://github.com/ChuckBuilds/ledmatrix-plugins",
|
||||
},
|
||||
{
|
||||
"id": "ledmatrix-flights",
|
||||
"name": "Flights",
|
||||
"plugin_path": "plugins/ledmatrix-flights",
|
||||
"repo": "https://github.com/ChuckBuilds/ledmatrix-plugins",
|
||||
},
|
||||
{
|
||||
"id": "third-party",
|
||||
"name": "Third Party",
|
||||
"plugin_path": "",
|
||||
"repo": "https://github.com/someone/thing",
|
||||
},
|
||||
]
|
||||
}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def store(monkeypatch: pytest.MonkeyPatch) -> PluginStoreManager:
|
||||
manager = PluginStoreManager.__new__(PluginStoreManager)
|
||||
monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: REGISTRY, raising=False)
|
||||
return manager
|
||||
|
||||
|
||||
def _ids(entry: Optional[Dict[str, Any]]) -> Optional[str]:
|
||||
return entry.get("id") if entry else None
|
||||
|
||||
|
||||
class TestRegistryLookupByManifestId:
|
||||
def test_exact_registry_id_still_resolves(self, store: PluginStoreManager) -> None:
|
||||
assert _ids(store.get_registry_info("weather")) == "weather"
|
||||
|
||||
def test_manifest_id_resolves_via_plugin_path(self, store: PluginStoreManager) -> None:
|
||||
"""The case that broke restore: asked by the name on disk."""
|
||||
assert _ids(store.get_registry_info("ledmatrix-weather")) == "weather", (
|
||||
"a plugin installed as 'ledmatrix-weather' could not be found in a "
|
||||
"registry that lists it under plugin_path plugins/ledmatrix-weather")
|
||||
|
||||
def test_matching_id_and_path_unaffected(self, store: PluginStoreManager) -> None:
|
||||
assert _ids(store.get_registry_info("ledmatrix-flights")) == "ledmatrix-flights"
|
||||
|
||||
def test_unknown_plugin_still_returns_none(self, store: PluginStoreManager) -> None:
|
||||
assert store.get_registry_info("no-such-plugin") is None
|
||||
|
||||
def test_empty_plugin_path_is_not_a_wildcard(self, store: PluginStoreManager) -> None:
|
||||
"""Third-party entries carry plugin_path "" — that must not match ""."""
|
||||
assert store.get_registry_info("") is None
|
||||
|
||||
def test_exact_id_wins_over_a_path_match(self, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""If some other entry's path collides with a real id, id wins."""
|
||||
registry = {
|
||||
"plugins": [
|
||||
{"id": "decoy", "plugin_path": "plugins/weather"},
|
||||
{"id": "weather", "plugin_path": "plugins/ledmatrix-weather"},
|
||||
]
|
||||
}
|
||||
manager = PluginStoreManager.__new__(PluginStoreManager)
|
||||
monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: registry, raising=False)
|
||||
assert _ids(manager.get_registry_info("weather")) == "weather"
|
||||
@@ -7843,7 +7843,31 @@ def clear_old_errors():
|
||||
# Backup / Restore
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_BACKUP_EXPORT_DIR = PROJECT_ROOT / "config" / "backups" / "exports"
|
||||
def _resolve_backup_export_dir() -> Path:
|
||||
"""Where exported backups live: beside the install, not inside it.
|
||||
|
||||
They used to be written to ``<project>/config/backups/exports``. That is
|
||||
inside the directory a reinstall deletes, so the documented recovery path
|
||||
-- export a backup, then reinstall -- destroyed the backup it had just
|
||||
told the user to make. Anyone who downloaded the ZIP was fine; anyone
|
||||
relying on the on-device copy was not.
|
||||
|
||||
Falls back to the old location when the parent directory is not writable,
|
||||
so an unusual layout degrades to previous behaviour instead of failing to
|
||||
export at all.
|
||||
"""
|
||||
preferred = PROJECT_ROOT.parent / "ledmatrix-backups"
|
||||
try:
|
||||
preferred.mkdir(parents=True, exist_ok=True)
|
||||
probe = preferred / ".writetest"
|
||||
probe.write_text("", encoding="utf-8")
|
||||
probe.unlink()
|
||||
return preferred
|
||||
except OSError:
|
||||
return PROJECT_ROOT / "config" / "backups" / "exports"
|
||||
|
||||
|
||||
_BACKUP_EXPORT_DIR = _resolve_backup_export_dir()
|
||||
|
||||
|
||||
def _safe_backup_path(filename: str) -> Path:
|
||||
@@ -7997,7 +8021,22 @@ def backup_restore():
|
||||
|
||||
data = result.to_dict()
|
||||
if not result.success:
|
||||
return jsonify({'status': 'error', 'message': 'Restore had errors', 'data': data}), 500
|
||||
# Name what failed, and what nonetheless landed. A restore is
|
||||
# partial far more often than it is total -- a fresh install can
|
||||
# leave config_secrets.json unwritable by the web service, so
|
||||
# config restores and secrets do not. "Restore had errors" alone
|
||||
# left the user unable to tell a wholly failed restore from one
|
||||
# that quietly dropped their API keys.
|
||||
failed_plugins = [p.get('plugin_id') for p in (result.plugins_failed or [])]
|
||||
parts = []
|
||||
if result.restored:
|
||||
parts.append(f"restored: {', '.join(result.restored)}")
|
||||
if result.errors:
|
||||
parts.append(f"failed: {'; '.join(result.errors)}")
|
||||
if failed_plugins:
|
||||
parts.append(f"plugins not reinstalled: {', '.join(failed_plugins)}")
|
||||
message = 'Restore incomplete — ' + ('. '.join(parts) if parts else 'see logs')
|
||||
return jsonify({'status': 'error', 'message': message, 'data': data}), 500
|
||||
return jsonify({'status': 'success', 'data': data})
|
||||
except Exception as e:
|
||||
logger.error("backup_restore failed: %s", e, exc_info=True)
|
||||
|
||||
Reference in New Issue
Block a user