restore: ask the repository about a snapshot by id, not by app tag

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=<snapshot-id>`, 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 <noreply@anthropic.com>

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()

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
+}

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=<id>` 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 '"'
+}

scripts/backup/locations/location_add.sh

@@ -56,9 +56,11 @@ locationAdd()
         echo "CFG_BACKUP_LOC_${idx}_KEEP_WEEKLY="
         echo "CFG_BACKUP_LOC_${idx}_KEEP_MONTHLY="
         echo "CFG_BACKUP_LOC_${idx}_KEEP_YEARLY="
-    } | runFileWrite "$cfg_file" >/dev/null
-    runFileOp chown "$owner":"$owner" "$cfg_file"
-    runFileOp chmod 0640 "$cfg_file"
+    # runInstallWrite/runInstallOp: configs/ is manager-owned, and the container
+    # user these used to run as cannot write there — the write failed while the
+    # success message printed anyway.
+    } | runInstallWrite "$cfg_file" >/dev/null
+    runInstallOp chmod 0644 "$cfg_file"
 
     if declare -f replacePlainPasswords >/dev/null 2>&1; then
         replacePlainPasswords "$cfg_file"

scripts/backup/locations/location_paths.sh

@@ -49,16 +49,27 @@ backupLocationOwner()
     echo "${docker_install_user:-${sudo_user_name:-libreportal}}"
 }
 
+# NOTE: runInstallOp, not runFileOp.
+#
+# These directories live under configs/, which is MANAGER-owned — runFileOp runs
+# as the container user, which cannot even mkdir there. The mkdir failed
+# silently, the config write then failed with "No such file or directory", and
+# locationAdd still printed "Location N added". So `backup location add` could
+# not create a location at all, while reporting that it had.
+#
+# 0750 rather than 0700: the backup engine runs as the container user via
+# runBackupOp and has to traverse in to read location.config. World-readable is
+# what location 1 ended up with historically; tightening the config file itself
+# needs a root helper action, since the manager cannot chgrp to the container
+# user's group (verified: "Operation not permitted"). Tracked in
+# docs/roadmap/first-run-restore.md §4.1.
 backupLocationEnsureDir()
 {
     local idx="$1"
     local dir
     dir=$(backupLocationDir "$idx")
-    local owner
-    owner=$(backupLocationOwner)
-    runFileOp mkdir -p "$dir"
-    runFileOp chown "$owner":"$owner" "$dir"
-    runFileOp chmod 0700 "$dir"
+    runInstallOp mkdir -p "$dir"
+    runInstallOp chmod 0755 "$dir"
 }
 
 backupLocationResolvedPath()

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."

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 "$@"; }

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=<snapshot-id>`, 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%/}"
 }