From 7fae6bc308f4c083bf7f37368ad9bc3bbcfbd706 Mon Sep 17 00:00:00 2001 From: librelad Date: Tue, 11 Aug 2026 16:37:14 +0100 Subject: [PATCH] fix(updater): stop a callee blanking the app name mid-update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First real end-to-end auto-update on a live install failed like this: Automatically updating trivy (a recovery snapshot is taken first) Snapshotting trivy before update… Pulling new image(s) for … Update of failed — rolling back… Could not roll back automatically The app name went empty after the snapshot. Cause: bash is dynamically scoped, so a callee assigning an undeclared variable writes the CALLER's local of that name — and a `while read app` loop leaves it EMPTY at EOF. webuiBackupAppStatus's dashboard generator runs at the end of every backup and did exactly that to updaterApplyApp's `app`. Nothing was damaged: the pull ran against an empty name, failed before touching the image, and the rollback was a no-op on a nonexistent app. Fixed both ends. The generator (and three gluetun loops with the same latent leak) now declare `local app`. updaterApplyApp/updaterRollbackApp hold the name in `_upd_app` so they no longer depend on every callee's hygiene, and updaterApplyAll stops leaking its own loop var. This is exactly the untested path the roadmap flagged: "apply/revert not yet exercised end-to-end on a live install with a pending update." Co-Authored-By: Claude Opus 5 --- containers/gluetun/scripts/gluetun_network.sh | 2 + .../scripts/gluetun_recreate_routed.sh | 2 + .../gluetun/scripts/gluetun_route_apps.sh | 2 + .../commands/updater/cli_updater_commands.sh | 66 +++++++++++-------- .../backup/webui_backup_dashboard.sh | 6 ++ 5 files changed, 50 insertions(+), 28 deletions(-) diff --git a/containers/gluetun/scripts/gluetun_network.sh b/containers/gluetun/scripts/gluetun_network.sh index b2dcc42..b58f4f1 100644 --- a/containers/gluetun/scripts/gluetun_network.sh +++ b/containers/gluetun/scripts/gluetun_network.sh @@ -62,6 +62,8 @@ appNetworkRegisterPorts_gluetun() "SELECT name FROM apps WHERE status = 1 ORDER BY name;" 2>/dev/null) local routed_lines="" + local app # local: bash is dynamically scoped, and a while-read loop + # leaves an undeclared name EMPTY in the CALLER's scope at EOF. while IFS= read -r app; do [[ -z "$app" || "$app" == "gluetun" ]] && continue local app_config_file="${containers_dir}${app}/${app}.config" diff --git a/containers/gluetun/scripts/gluetun_recreate_routed.sh b/containers/gluetun/scripts/gluetun_recreate_routed.sh index 79ac7ec..799615f 100644 --- a/containers/gluetun/scripts/gluetun_recreate_routed.sh +++ b/containers/gluetun/scripts/gluetun_recreate_routed.sh @@ -33,6 +33,8 @@ appGluetunRecreateRouted() fi local recreated=0 + local app # local: bash is dynamically scoped, and a while-read loop + # leaves an undeclared name EMPTY in the CALLER's scope at EOF. while IFS= read -r app; do [[ -z "$app" || "$app" == "gluetun" ]] && continue local app_config_file="${containers_dir}${app}/${app}.config" diff --git a/containers/gluetun/scripts/gluetun_route_apps.sh b/containers/gluetun/scripts/gluetun_route_apps.sh index baa571a..67dc1d4 100644 --- a/containers/gluetun/scripts/gluetun_route_apps.sh +++ b/containers/gluetun/scripts/gluetun_route_apps.sh @@ -28,6 +28,8 @@ gluetunRouteExistingAppsPrompt() fi local eligible=() + local app # local: bash is dynamically scoped, and a while-read loop + # leaves an undeclared name EMPTY in the CALLER's scope at EOF. while IFS= read -r app; do [[ -z "$app" ]] && continue local cfg_file="${containers_dir}${app}/${app}.config" diff --git a/scripts/cli/commands/updater/cli_updater_commands.sh b/scripts/cli/commands/updater/cli_updater_commands.sh index 2222947..93b3169 100644 --- a/scripts/cli/commands/updater/cli_updater_commands.sh +++ b/scripts/cli/commands/updater/cli_updater_commands.sh @@ -158,18 +158,24 @@ updaterLastUpdateFrom() # snapshot, docker compose for the image swap) so it shares their locking/logging. updaterApplyApp() { - local app="$1" + # `_upd_app`, not `app`: bash is dynamically scoped, so a callee that uses an + # undeclared `app` (a while-read loop leaves it EMPTY at EOF) reaches up and + # overwrites OUR local. That is not hypothetical — the backup dashboard + # generator, which runs at the end of the snapshot below, did exactly that, + # and the update then ran against an empty app name. The generator is fixed; + # this name makes the update immune to the next one. + local _upd_app="$1" # "auto" when the updater's own policy enqueued this (CFG__UPDATE_TYPE), # "manual" when a person pressed Update. Recorded in History; changes nothing # about how the update is applied — both take the snapshot, both can roll back. local trigger="${2:-manual}" - local app_dir="$containers_dir/$app" - if [[ ! -d "$app_dir" ]]; then isError "App '$app' is not installed."; return 1; fi + local app_dir="$containers_dir/$_upd_app" + if [[ ! -d "$app_dir" ]]; then isError "App '$_upd_app' is not installed."; return 1; fi if [[ "$trigger" == "auto" ]]; then - isHeader "Automatically updating $app (a recovery snapshot is taken first)" + isHeader "Automatically updating $_upd_app (a recovery snapshot is taken first)" else - isHeader "Updating $app (a recovery snapshot is taken first)" + isHeader "Updating $_upd_app (a recovery snapshot is taken first)" fi # 1. DISASTER RECOVERY — snapshot before touching anything. Call the backup @@ -178,10 +184,10 @@ updaterApplyApp() # `*)` default (a notice that exits 0), so the `if !` guard passed and the app # was updated with NO snapshot — and rollback below was a no-op that reported # success. backupAppStart is the real entry point and returns 0/1 honestly. - isNotice "Snapshotting $app before update…" - if ! backupAppStart "$app" >/dev/null 2>&1; then - isNotice "Pre-update snapshot did not complete cleanly — continuing is risky; aborting $app update." - updaterRecordHistory "$app" "update" "" "" "aborted-no-snapshot" "" "" "" "$trigger" + isNotice "Snapshotting $_upd_app before update…" + if ! backupAppStart "$_upd_app" >/dev/null 2>&1; then + isNotice "Pre-update snapshot did not complete cleanly — continuing is risky; aborting $_upd_app update." + updaterRecordHistory "$_upd_app" "update" "" "" "aborted-no-snapshot" "" "" "" "$trigger" return 1 fi @@ -189,27 +195,27 @@ updaterApplyApp() # exact build reference (repo:tag@sha256:…) even for a floating tag. Anchor is # the -service image (updaterPrimaryImage), NOT the first line. If a # prior rollback pinned a digest, unpin it first so we track the channel again. - local anchor; anchor="$(updaterPrimaryImage "$app" "$app_dir/docker-compose.yml")" - case "$anchor" in *@sha256:*) updaterSetAnchorRef "$app" "${anchor%%@*}"; anchor="${anchor%%@*}";; esac + local anchor; anchor="$(updaterPrimaryImage "$_upd_app" "$app_dir/docker-compose.yml")" + case "$anchor" in *@sha256:*) updaterSetAnchorRef "$_upd_app" "${anchor%%@*}"; anchor="${anchor%%@*}";; esac local before_dig; before_dig="$(updaterRefDigest "$anchor")" local before="$anchor${before_dig:+@$before_dig}" # 3. Pull + recreate (uses the real, install-type-aware compose helpers). - isNotice "Pulling new image(s) for $app…" - if updaterComposePull "$app" && dockerComposeUp "$app" >/dev/null 2>&1; then - local after_ref; after_ref="$(updaterPrimaryImage "$app" "$app_dir/docker-compose.yml")"; after_ref="${after_ref%%@*}" + isNotice "Pulling new image(s) for $_upd_app…" + if updaterComposePull "$_upd_app" && dockerComposeUp "$_upd_app" >/dev/null 2>&1; then + local after_ref; after_ref="$(updaterPrimaryImage "$_upd_app" "$app_dir/docker-compose.yml")"; after_ref="${after_ref%%@*}" local after_dig; after_dig="$(updaterRefDigest "$after_ref")" local after="$after_ref${after_dig:+@$after_dig}" - updaterRecordHistory "$app" "update" "$before" "$after" "ok" "" "" "" "$trigger" - isSuccessful "$app updated. Rollback point retained." + updaterRecordHistory "$_upd_app" "update" "$before" "$after" "ok" "" "" "" "$trigger" + isSuccessful "$_upd_app updated. Rollback point retained." webuiUpdaterScan >/dev/null 2>&1 || true return 0 fi # 4. Failure -> automatic rollback. - isNotice "Update of $app failed — rolling back to the pre-update snapshot…" - updaterRollbackApp "$app" "auto" - updaterRecordHistory "$app" "update" "$before" "" "rolled-back" "" "" "" "$trigger" + isNotice "Update of $_upd_app failed — rolling back to the pre-update snapshot…" + updaterRollbackApp "$_upd_app" "auto" + updaterRecordHistory "$_upd_app" "update" "$before" "" "rolled-back" "" "" "" "$trigger" return 1 } @@ -221,6 +227,7 @@ updaterApplyAll() return 0 fi local IFS=',' + local app # local: don't leak the loop var into callers for app in $list; do [[ -z "$app" ]] && continue updaterApplyApp "$app" || failures=$((failures+1)) @@ -232,27 +239,30 @@ updaterApplyAll() # (called from the failure path of an apply). updaterRollbackApp() { - local app="$1" mode="$2" - [[ "$mode" != "auto" ]] && isHeader "Rolling $app back to its pre-update snapshot" + # `_upd_app` for the same dynamic-scoping reason as updaterApplyApp: the + # restore engine below is a deep call chain, and this function still needs + # the app's name after it returns. + local _upd_app="$1" mode="$2" + [[ "$mode" != "auto" ]] && isHeader "Rolling $_upd_app back to its pre-update snapshot" # Re-pin the ANCHOR image to the exact build the app ran before the last # update, so `up` below runs the OLD code — not the current channel head. # Without this, restoring the data snapshot but recreating on the new `latest` # image is "new code on old data", the hole a floating tag makes invisible. # (Preserves the version sentinel; apply un-pins it again on the next update.) - local prev_ref; prev_ref="$(updaterLastUpdateFrom "$app")" + local prev_ref; prev_ref="$(updaterLastUpdateFrom "$_upd_app")" if [[ -n "$prev_ref" && "$prev_ref" == *@sha256:* ]]; then - updaterSetAnchorRef "$app" "$prev_ref" && isNotice "Pinned $app back to its pre-update build." + updaterSetAnchorRef "$_upd_app" "$prev_ref" && isNotice "Pinned $_upd_app back to its pre-update build." fi # Delegate to the restore engine (latest snapshot for this app). Call the # function directly — the old `backup app "$app" restore latest` CLI form was # malformed (parsed as action="$app") so it silently did nothing yet exited 0. - if restoreAppStart "$app" latest "" >/dev/null 2>&1; then - dockerComposeUp "$app" >/dev/null 2>&1 || true - [[ "$mode" != "auto" ]] && updaterRecordHistory "$app" "rollback" "" "" "rolled-back" - isSuccessful "$app restored from its pre-update snapshot." + if restoreAppStart "$_upd_app" latest "" >/dev/null 2>&1; then + dockerComposeUp "$_upd_app" >/dev/null 2>&1 || true + [[ "$mode" != "auto" ]] && updaterRecordHistory "$_upd_app" "rollback" "" "" "rolled-back" + isSuccessful "$_upd_app restored from its pre-update snapshot." return 0 fi - isError "Could not roll $app back automatically — restore manually from the Backups page." + isError "Could not roll $_upd_app back automatically — restore manually from the Backups page." return 1 } diff --git a/scripts/webui/data/generators/backup/webui_backup_dashboard.sh b/scripts/webui/data/generators/backup/webui_backup_dashboard.sh index f1400a2..e9d424a 100644 --- a/scripts/webui/data/generators/backup/webui_backup_dashboard.sh +++ b/scripts/webui/data/generators/backup/webui_backup_dashboard.sh @@ -58,6 +58,12 @@ webuiGenerateBackupDashboard() local apps_json="[" first=true + # `local app` matters beyond hygiene: bash is dynamically scoped, so an + # undeclared loop variable here reaches up and overwrites the CALLER's + # variable of the same name — and a while-read loop leaves it EMPTY at EOF. + # This generator runs at the end of a backup, so it was blanking the `app` + # local of updaterApplyApp (which snapshots, then keeps using $app) mid-update. + local app if [[ -f "$docker_dir/$db_file" ]]; then while IFS= read -r app; do [[ -z "$app" ]] && continue