From 101261bd9b453f488ab14abfe8b0f8cfc09dd093 Mon Sep 17 00:00:00 2001
From: David Young <david@federated.computer>
Date: Thu, 5 Nov 2026 08:39:54 -0700
Subject: [PATCH] update: a failing hook without run_update fails the update

Since a65f07c ran each hook in a subshell, a hook that does its work as
it's sourced (12-migrations, 15-services, 20-aur, 30-flatpak and others)
had its failure lost: the run_update check after source left the
subshell's status at 0, so update said "Done." and exited 0. The smoketest's
40-failing-migration caught it. update now exits the subshell with the
sourced hook's status.

Also, the smoketest scenarios that check whether a migration ran look for
"Running migration " so install.sh's "Running migrations..." heading
doesn't match (11-refresh failed on it).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 smoketest/scenarios/11-refresh.sh       |  3 ++-
 smoketest/scenarios/20-noop-update.sh   |  3 ++-
 smoketest/scenarios/32-new-migration.sh |  4 ++--
 tests/update.bats                       | 25 +++++++++++++++++++++++++
 update                                  |  5 +++++
 5 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/smoketest/scenarios/11-refresh.sh b/smoketest/scenarios/11-refresh.sh
index 8d6f0f2..1f2aee7 100644
--- a/smoketest/scenarios/11-refresh.sh
+++ b/smoketest/scenarios/11-refresh.sh
@@ -9,7 +9,8 @@ scenario() {
 
   run_in_vm refresh 'TERM=dumb ~/.local/bin/refresh'
   assert_status 0
-  refute_output "Running migration"
+  # One migration's line, not install.sh's "Running migrations..." heading
+  refute_output "Running migration "
 
   fingerprint "$SMOKETEST_RESULTS/after.fingerprint"
   assert_same_fingerprint "$SMOKETEST_RESULTS/before.fingerprint" "$SMOKETEST_RESULTS/after.fingerprint"
diff --git a/smoketest/scenarios/20-noop-update.sh b/smoketest/scenarios/20-noop-update.sh
index a0f150c..92900f6 100644
--- a/smoketest/scenarios/20-noop-update.sh
+++ b/smoketest/scenarios/20-noop-update.sh
@@ -11,7 +11,8 @@ scenario() {
   cd_update
   assert_status 0
   assert_output "Done."
-  refute_output "Running migration"
+  # One migration's line, not install.sh's "Running migrations..." heading
+  refute_output "Running migration "
   # install_packages prints this only when it installs something
   refute_output "Installing:"
 
diff --git a/smoketest/scenarios/32-new-migration.sh b/smoketest/scenarios/32-new-migration.sh
index 3e90088..bf36bf5 100644
--- a/smoketest/scenarios/32-new-migration.sh
+++ b/smoketest/scenarios/32-new-migration.sh
@@ -10,10 +10,10 @@ scenario() {
 
   cd_update
   assert_status 0
-  assert_output "Running migration"
+  assert_output "Running migration "
   cd_update
   assert_status 0
-  refute_output "Running migration"
+  refute_output "Running migration "
 
   assert_vm "the migration ran exactly once" '[ "$(cat ~/smoketest-migration)" = ran ]'
   assert_vm "the migration is recorded" '
diff --git a/tests/update.bats b/tests/update.bats
index 8c024d6..4338cba 100644
--- a/tests/update.bats
+++ b/tests/update.bats
@@ -32,6 +32,12 @@ hook() {
   printf '#!/bin/bash\nrun_update() {\n%s\n}\n' "$2" > "$CD/updates.d/$1.sh"
 }
 
+# plain_hook NAME BODY: a hook that does its work as it's sourced, with no
+# run_update, like updates.d/12-migrations.sh
+plain_hook() {
+  printf '#!/bin/bash\n%s\n' "$2" > "$CD/updates.d/$1.sh"
+}
+
 ran() {
   [ "$(cat "$RAN" 2> /dev/null)" = "$(printf '%s\n' "$@")" ]
 }
@@ -70,6 +76,25 @@ ran() {
   ran a b c
 }
 
+@test "a failing hook without run_update fails the update too" {
+  plain_hook 10-a 'echo a >> "$RAN"; false'
+  plain_hook 20-b 'echo b >> "$RAN"'
+
+  run "$CD/update"
+  [ "$status" -eq 1 ]
+  [[ "$output" == *"Done, with errors in: 10-a.sh"* ]]
+  ran a b
+}
+
+@test "a hook without run_update that succeeds doesn't fail the update" {
+  plain_hook 10-a 'echo a >> "$RAN"'
+
+  run "$CD/update"
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"Done."* ]]
+  ran a
+}
+
 @test "a hook that exits doesn't end the update" {
   hook 10-a 'echo a >> "$RAN"; exit 0'
   hook 20-b 'echo b >> "$RAN"'
diff --git a/update b/update
index d59f971..402d87f 100755
--- a/update
+++ b/update
@@ -78,10 +78,15 @@ for plugin in "$COREDESKTOP/updates.d/"*.sh; do
     (
         unset -f is_installed run_update
         source "$plugin"
+        # A hook without run_update does its work as it's sourced, so its
+        # last command's status is the hook's
+        status=$?
         if declare -f run_update > /dev/null; then
             if ! declare -f is_installed > /dev/null || is_installed; then
                 run_update
             fi
+        else
+            exit "$status"
         fi
     )
     # Checked separately: under `( ... ) ||`, bash would ignore set -e inside the hook