From 647b19cf4a11f16821042a7c24fab6876b6cea5b Mon Sep 17 00:00:00 2001 From: librelad Date: Thu, 27 Aug 2026 12:15:15 +0100 Subject: [PATCH] restore: ask the repository about a snapshot by id, not by app tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit storageSnapshotSourcePath resolved a snapshot's source path with engineSnapshotsJson "$idx" "$snapshot_id" but that function's second parameter is an app TAG filter. So it ran `restic snapshots --tag app=`, matched nothing, and returned 1 — every time, for every snapshot, since the file was written. Nothing broke loudly, because both callers have a fallback: * storageRestoreAppTo fell through to "restoring in place", reinstating the exact cross-root bug the file exists to fix — restoring onto a host whose containers root differs from the source's matched no include path and restored nothing, silently * the first-run preflight never read a manifest, so every app reported size "?" and its fit and location checks passed unconditionally. Thirteen green ticks that had checked nothing. Add engineSnapshotPaths: restic answers it with a positional snapshot id, kopia by filtering its list. borg has no adapter on purpose — it rebuilds its listing from archive metadata that carries no paths — so a missing adapter is a quiet "no" and those callers keep their in-place fallback. Add scripts/dev/lp-preflight-test, which pins the cases that must say NO: an app too big for the disk, one this version no longer ships, one whose storage location is gone, and a resolver that reaches for the app-tag filter again. Verified against both historical bugs — reintroducing either fails the test. Co-Authored-By: Claude Opus 5 --- scripts/backup/engine/engine_dispatch.sh | 12 ++ scripts/backup/engine/kopia_snapshots.sh | 20 ++++ scripts/backup/engine/restic_snapshots.sh | 25 +++++ scripts/dev/lp-preflight-test | 104 ++++++++++++++++++ .../source/files/arrays/function_manifest.sh | 9 ++ scripts/storage/storage_restore_path.sh | 21 ++-- 6 files changed, 181 insertions(+), 10 deletions(-) create mode 100755 scripts/dev/lp-preflight-test diff --git a/scripts/backup/engine/engine_dispatch.sh b/scripts/backup/engine/engine_dispatch.sh index 0b54449..f834c43 100644 --- a/scripts/backup/engine/engine_dispatch.sh +++ b/scripts/backup/engine/engine_dispatch.sh @@ -92,6 +92,18 @@ engineForgetSystem() { local i="$1"; shift; engineDispatch "$(engineForL engineCheckLocation() { local i="$1"; shift; engineDispatch "$(engineForLocation "$i")CheckLocation" "$i" "$@"; } engineDumpFile() { local i="$1"; shift; engineDispatch "$(engineForLocation "$i")DumpFile" "$i" "$@"; } +# The paths a single snapshot was taken from. Not every engine can answer: borg +# reconstructs its listing from archive metadata that carries no paths, so it +# has no adapter on purpose. A missing adapter is therefore a quiet "no" rather +# than engineDispatch's error — callers fall back to restoring in place, which +# is correct for borg and merely conservative elsewhere. +engineSnapshotPaths() { + local i="$1"; shift + local fn; fn="$(engineForLocation "$i")SnapshotPaths" + declare -f "$fn" >/dev/null 2>&1 || return 1 + "$fn" "$i" "$@" +} + # ---- Aggregate helpers (iterate enabled locations) --------------------------- engineInstallAll() diff --git a/scripts/backup/engine/kopia_snapshots.sh b/scripts/backup/engine/kopia_snapshots.sh index ea248b5..bc38611 100644 --- a/scripts/backup/engine/kopia_snapshots.sh +++ b/scripts/backup/engine/kopia_snapshots.sh @@ -122,3 +122,23 @@ kopiaSnapshotListFiles() kopiaEnvUnset return $rc } + +# The path recorded on one snapshot. kopia has no "describe this id" listing, so +# filter the full list; ids are matched whole or by the 8-char short form that +# kopiaSnapshotsJson publishes as short_id. +kopiaSnapshotPaths() +{ + local idx="$1" + local snapshot_id="$2" + + command -v jq >/dev/null 2>&1 || return 1 + kopiaEnvExport "$idx" || return 1 + local raw + raw=$(runBackupOp kopia snapshot list --all --json 2>/dev/null) + local rc=$? + kopiaEnvUnset + [[ $rc -eq 0 && -n "$raw" ]] || return 1 + + printf '%s' "$raw" | jq -r --arg id "$snapshot_id" \ + '.[] | select(.id == $id or .id[0:8] == $id) | .source.path' 2>/dev/null +} diff --git a/scripts/backup/engine/restic_snapshots.sh b/scripts/backup/engine/restic_snapshots.sh index 503d919..9a1c4b2 100644 --- a/scripts/backup/engine/restic_snapshots.sh +++ b/scripts/backup/engine/restic_snapshots.sh @@ -61,3 +61,28 @@ resticSnapshotListFiles() resticEnvUnset return $rc } + +# The paths recorded ON one snapshot, one per line. +# +# Deliberately its own call rather than a filter on resticSnapshotsJson: that +# function's second parameter is an app TAG filter, so asking it for a snapshot +# id runs `--tag app=` and matches nothing. storageSnapshotSourcePath did +# exactly that, so it always returned 1 and every restore silently took the +# "restore in place" fallback — the very bug storage_restore_path.sh exists to +# fix. Restore accepts a snapshot id positionally; use that. +resticSnapshotPaths() +{ + local idx="$1" + local snapshot_id="$2" + + resticEnvExport "$idx" || return 1 + local json + json=$(runBackupOp restic snapshots "$snapshot_id" --json --no-lock 2>/dev/null) + local rc=$? + resticEnvUnset + [[ $rc -eq 0 && -n "$json" ]] || return 1 + + printf '%s' "$json" \ + | grep -o '"paths":\[[^]]*\]' | head -1 \ + | grep -o '"/[^"]*"' | tr -d '"' +} diff --git a/scripts/dev/lp-preflight-test b/scripts/dev/lp-preflight-test new file mode 100755 index 0000000..ef346eb --- /dev/null +++ b/scripts/dev/lp-preflight-test @@ -0,0 +1,104 @@ +#!/bin/bash +# Self-contained unit test for the first-run restore preflight +# (scripts/restore/restore_preflight.sh) and the snapshot path resolver it +# leans on (scripts/storage/storage_restore_path.sh). Runs against a throwaway +# tree in $TMPDIR — never touches a real install. Exits non-zero on failure. +# +# scripts/dev/lp-preflight-test +# +# It exists because this preflight has twice shipped checks that could not fail: +# +# * the manifest is pretty-printed, so `"size_bytes": 123` carries a space +# that a `"key":[0-9]*` pattern misses — size came back empty, and the fit +# check was skipped for every app +# * storageSnapshotSourcePath asked engineSnapshotsJson for a snapshot id, +# but that argument is an app TAG filter — so it matched nothing, returned +# 1 every time, and the manifest was never read at all +# +# Both failed silently and *looked* like a clean report: thirteen green ticks. +# So the cases worth keeping are the ones that must say NO. + +REPO="$(cd "$(dirname "$0")/../.." && pwd)" +BASE="$(mktemp -d "${TMPDIR:-/tmp}/lp-preflight-test-XXXXXX")" +trap 'rm -rf "$BASE"' EXIT + +fail=0 +chk(){ if [[ "$2" == "$3" ]]; then echo " ok $1"; else echo " FAIL $1: got '$2' want '$3'"; fail=1; fi; } +has(){ if [[ "$2" == *"$3"* ]]; then echo " ok $1"; else echo " FAIL $1: '$2' lacks '$3'"; fail=1; fi; } + +# --- a machine to check against ---------------------------------------------- +install_containers_dir="$BASE/templates/" +mkdir -p "$BASE/templates/bookstack" "$BASE/templates/nextcloud" "$BASE/templates/jellyfin" "$BASE/data" +: > "$BASE/templates/bookstack/bookstack.config" +: > "$BASE/templates/nextcloud/nextcloud.config" +: > "$BASE/templates/jellyfin/jellyfin.config" +# note: no template for 'obsolete' — that is case 2 + +isNotice(){ :; }; isError(){ :; }; isSuccessful(){ :; }; isQuestion(){ :; } +appDir(){ printf '%s/data/%s' "$BASE" "$1"; } +storageLocationName(){ printf 'primary'; } +storageLocationPath(){ [[ "$1" == "here" ]] && { printf '%s/data' "$BASE"; return 0; }; return 1; } +migrateDiscoverApps(){ :; } +engineSnapshotLatestId(){ printf 'abc123'; } + +source "$REPO/scripts/restore/restore_preflight.sh" + +# Stubbed AFTER the source, or the real definition wins. Manifests are +# pretty-printed exactly as the real ones are — size_bytes at the top level, +# location under "storage" — because the spaces after those colons are the +# point of this fixture, not an accident. +_manifest(){ + printf '{\n "app": "%s",\n "size_bytes": %s,\n "storage": {\n "location": "%s"\n }\n}\n' \ + "$1" "$2" "$3" +} +restorePreflightManifest(){ _manifest "$2" "$MANIFEST_SIZE" "$MANIFEST_LOC"; } + +echo "--- an app that restores as-is ---" +MANIFEST_LOC="default"; MANIFEST_SIZE=147483648 +rec=$(restorePreflightApp 1 bookstack somehost) +chk "verdict" "$(cut -f1 <<< "$rec")" "ok" +chk "size parsed" "$(cut -f3 <<< "$rec")" "140M" + +echo "--- an app this version no longer ships ---" +rec=$(restorePreflightApp 1 obsolete somehost) +chk "verdict" "$(cut -f1 <<< "$rec")" "skip" +has "reason" "$rec" "no longer ships" + +echo "--- an app too big for the disk (the check that could not fail) ---" +MANIFEST_LOC="default"; MANIFEST_SIZE=9007199254740992 # 8 PiB +rec=$(restorePreflightApp 1 jellyfin somehost) +chk "verdict" "$(cut -f1 <<< "$rec")" "skip" +has "reason" "$rec" "needs 8192T" + +echo "--- an app whose old storage location is gone ---" +MANIFEST_LOC="ssd"; MANIFEST_SIZE=1048576 +rec=$(restorePreflightApp 1 nextcloud somehost) +chk "verdict" "$(cut -f1 <<< "$rec")" "move" +has "reason" "$rec" "not on this machine" + +echo "--- a location this machine still has is NOT a move ---" +MANIFEST_LOC="here"; MANIFEST_SIZE=1048576 +rec=$(restorePreflightApp 1 nextcloud somehost) +chk "verdict" "$(cut -f1 <<< "$rec")" "ok" + +echo "--- storageSnapshotSourcePath picks the app's path from the snapshot ---" +# If this ever calls engineSnapshotsJson again it is passing a snapshot id into +# an app-tag filter, which is the bug. Make that loud rather than silent. +engineSnapshotsJson(){ echo " FAIL resolver used the app-tag filter again" >&2; fail=1; return 1; } +engineSnapshotPaths(){ printf '/mnt/ssd/apps/nextcloud\n/libreportal-containers/bookstack\n'; } +source "$REPO/scripts/storage/storage_restore_path.sh" +chk "matching app" "$(storageSnapshotSourcePath 1 abc123 nextcloud)" "/mnt/ssd/apps/nextcloud" +chk "other app" "$(storageSnapshotSourcePath 1 abc123 bookstack)" "/libreportal-containers/bookstack" +storageSnapshotSourcePath 1 abc123 grafana >/dev/null 2>&1 \ + && { echo " FAIL absent app should not resolve"; fail=1; } \ + || echo " ok absent app does not resolve" + +# An engine with no adapter (borg) must fall back quietly, not error. +engineSnapshotPaths(){ return 1; } +storageSnapshotSourcePath 1 abc123 bookstack >/dev/null 2>&1 \ + && { echo " FAIL engine without paths should not resolve"; fail=1; } \ + || echo " ok engine without paths falls back" + +echo "" +if (( fail )); then echo "FAILED"; exit 1; fi +echo "All preflight checks passed." diff --git a/scripts/source/files/arrays/function_manifest.sh b/scripts/source/files/arrays/function_manifest.sh index 03ab01c..13a7f84 100644 --- a/scripts/source/files/arrays/function_manifest.sh +++ b/scripts/source/files/arrays/function_manifest.sh @@ -504,6 +504,7 @@ declare -gA LP_FN_MAP=( [engineRestoreSystemLatest]="backup/engine/engine_dispatch.sh" [engineSnapshotLatestId]="backup/engine/engine_dispatch.sh" [engineSnapshotListFiles]="backup/engine/engine_dispatch.sh" + [engineSnapshotPaths]="backup/engine/engine_dispatch.sh" [engineSnapshotsJson]="backup/engine/engine_dispatch.sh" [engineSystemSnapshotsJson]="backup/engine/engine_dispatch.sh" [exitScript]="start/start_exit.sh" @@ -651,6 +652,7 @@ declare -gA LP_FN_MAP=( [kopiaRestoreSystemLatest]="backup/engine/kopia_snapshots.sh" [kopiaSnapshotLatestId]="backup/engine/kopia_snapshots.sh" [kopiaSnapshotListFiles]="backup/engine/kopia_snapshots.sh" + [kopiaSnapshotPaths]="backup/engine/kopia_snapshots.sh" [kopiaSnapshotsJson]="backup/engine/kopia_snapshots.sh" [kopiaSystemSnapshotsJson]="backup/engine/kopia_snapshots.sh" [libreportal_catalog_install_post_setup]="libreportal_catalog/scripts/libreportal_catalog_install_hooks.sh" @@ -898,6 +900,7 @@ declare -gA LP_FN_MAP=( [resticRetentionFor]="backup/engine/restic_forget.sh" [resticSnapshotLatestId]="backup/engine/restic_snapshots.sh" [resticSnapshotListFiles]="backup/engine/restic_snapshots.sh" + [resticSnapshotPaths]="backup/engine/restic_snapshots.sh" [resticSnapshotsJson]="backup/engine/restic_snapshots.sh" [resticSystemSnapshotsJson]="backup/engine/restic_snapshots.sh" [_resticUsernsPrefix]="backup/engine/restic_restore.sh" @@ -1739,6 +1742,7 @@ declare -gA LP_FN_ROOT=( [engineRestoreSystemLatest]="scripts" [engineSnapshotLatestId]="scripts" [engineSnapshotListFiles]="scripts" + [engineSnapshotPaths]="scripts" [engineSnapshotsJson]="scripts" [engineSystemSnapshotsJson]="scripts" [exitScript]="scripts" @@ -1886,6 +1890,7 @@ declare -gA LP_FN_ROOT=( [kopiaRestoreSystemLatest]="scripts" [kopiaSnapshotLatestId]="scripts" [kopiaSnapshotListFiles]="scripts" + [kopiaSnapshotPaths]="scripts" [kopiaSnapshotsJson]="scripts" [kopiaSystemSnapshotsJson]="scripts" [libreportal_catalog_install_post_setup]="containers" @@ -2133,6 +2138,7 @@ declare -gA LP_FN_ROOT=( [resticRetentionFor]="scripts" [resticSnapshotLatestId]="scripts" [resticSnapshotListFiles]="scripts" + [resticSnapshotPaths]="scripts" [resticSnapshotsJson]="scripts" [resticSystemSnapshotsJson]="scripts" [_resticUsernsPrefix]="scripts" @@ -3012,6 +3018,7 @@ engineRestoreSnapshot() { unset -f engineRestoreSnapshot; __lpAutoload "${instal engineRestoreSystemLatest() { unset -f engineRestoreSystemLatest; __lpAutoload "${install_scripts_dir}backup/engine/engine_dispatch.sh"; engineRestoreSystemLatest "$@"; } engineSnapshotLatestId() { unset -f engineSnapshotLatestId; __lpAutoload "${install_scripts_dir}backup/engine/engine_dispatch.sh"; engineSnapshotLatestId "$@"; } engineSnapshotListFiles() { unset -f engineSnapshotListFiles; __lpAutoload "${install_scripts_dir}backup/engine/engine_dispatch.sh"; engineSnapshotListFiles "$@"; } +engineSnapshotPaths() { unset -f engineSnapshotPaths; __lpAutoload "${install_scripts_dir}backup/engine/engine_dispatch.sh"; engineSnapshotPaths "$@"; } engineSnapshotsJson() { unset -f engineSnapshotsJson; __lpAutoload "${install_scripts_dir}backup/engine/engine_dispatch.sh"; engineSnapshotsJson "$@"; } engineSystemSnapshotsJson() { unset -f engineSystemSnapshotsJson; __lpAutoload "${install_scripts_dir}backup/engine/engine_dispatch.sh"; engineSystemSnapshotsJson "$@"; } exitScript() { unset -f exitScript; __lpAutoload "${install_scripts_dir}start/start_exit.sh"; exitScript "$@"; } @@ -3159,6 +3166,7 @@ kopiaRestoreSnapshot() { unset -f kopiaRestoreSnapshot; __lpAutoload "${install_ kopiaRestoreSystemLatest() { unset -f kopiaRestoreSystemLatest; __lpAutoload "${install_scripts_dir}backup/engine/kopia_snapshots.sh"; kopiaRestoreSystemLatest "$@"; } kopiaSnapshotLatestId() { unset -f kopiaSnapshotLatestId; __lpAutoload "${install_scripts_dir}backup/engine/kopia_snapshots.sh"; kopiaSnapshotLatestId "$@"; } kopiaSnapshotListFiles() { unset -f kopiaSnapshotListFiles; __lpAutoload "${install_scripts_dir}backup/engine/kopia_snapshots.sh"; kopiaSnapshotListFiles "$@"; } +kopiaSnapshotPaths() { unset -f kopiaSnapshotPaths; __lpAutoload "${install_scripts_dir}backup/engine/kopia_snapshots.sh"; kopiaSnapshotPaths "$@"; } kopiaSnapshotsJson() { unset -f kopiaSnapshotsJson; __lpAutoload "${install_scripts_dir}backup/engine/kopia_snapshots.sh"; kopiaSnapshotsJson "$@"; } kopiaSystemSnapshotsJson() { unset -f kopiaSystemSnapshotsJson; __lpAutoload "${install_scripts_dir}backup/engine/kopia_snapshots.sh"; kopiaSystemSnapshotsJson "$@"; } libreportal_catalog_install_post_setup() { unset -f libreportal_catalog_install_post_setup; __lpAutoload "${install_containers_dir}libreportal_catalog/scripts/libreportal_catalog_install_hooks.sh"; libreportal_catalog_install_post_setup "$@"; } @@ -3406,6 +3414,7 @@ resticRestoreSystemLatest() { unset -f resticRestoreSystemLatest; __lpAutoload " resticRetentionFor() { unset -f resticRetentionFor; __lpAutoload "${install_scripts_dir}backup/engine/restic_forget.sh"; resticRetentionFor "$@"; } resticSnapshotLatestId() { unset -f resticSnapshotLatestId; __lpAutoload "${install_scripts_dir}backup/engine/restic_snapshots.sh"; resticSnapshotLatestId "$@"; } resticSnapshotListFiles() { unset -f resticSnapshotListFiles; __lpAutoload "${install_scripts_dir}backup/engine/restic_snapshots.sh"; resticSnapshotListFiles "$@"; } +resticSnapshotPaths() { unset -f resticSnapshotPaths; __lpAutoload "${install_scripts_dir}backup/engine/restic_snapshots.sh"; resticSnapshotPaths "$@"; } resticSnapshotsJson() { unset -f resticSnapshotsJson; __lpAutoload "${install_scripts_dir}backup/engine/restic_snapshots.sh"; resticSnapshotsJson "$@"; } resticSystemSnapshotsJson() { unset -f resticSystemSnapshotsJson; __lpAutoload "${install_scripts_dir}backup/engine/restic_snapshots.sh"; resticSystemSnapshotsJson "$@"; } _resticUsernsPrefix() { unset -f _resticUsernsPrefix; __lpAutoload "${install_scripts_dir}backup/engine/restic_restore.sh"; _resticUsernsPrefix "$@"; } diff --git a/scripts/storage/storage_restore_path.sh b/scripts/storage/storage_restore_path.sh index a902570..35e24c8 100644 --- a/scripts/storage/storage_restore_path.sh +++ b/scripts/storage/storage_restore_path.sh @@ -23,17 +23,18 @@ storageSnapshotSourcePath() { local idx="$1" snapshot_id="$2" app="$3" - local json path + local path - json=$(engineSnapshotsJson "$idx" "$snapshot_id" 2>/dev/null) || return 1 - # "paths":["/libreportal-containers/bookstack"] - path=$(printf '%s' "$json" \ - | grep -o '"paths":\[[^]]*\]' \ - | head -1 \ - | grep -o '"/[^"]*"' \ - | tr -d '"' \ - | grep -E "/${app}(/|$)" \ - | head -1) + # Ask about this ONE snapshot. The first version passed the id to + # engineSnapshotsJson, whose second parameter is an app TAG filter — so it + # searched for `--tag app=`, matched nothing, and returned 1 + # every single time. Nothing broke loudly, because both callers have a + # fallback: storageRestoreAppTo restored in place (reinstating the + # cross-root bug this file was written to fix) and the restore preflight + # reported every app's size as "?" while its fit and location checks passed + # unconditionally. + path=$(engineSnapshotPaths "$idx" "$snapshot_id" 2>/dev/null \ + | grep -E "/${app}(/|$)" | head -1) [[ -n "$path" ]] || return 1 printf '%s' "${path%/}" }