Fix auto-update leaving service down after installer failure

Two bugs from #1630 that combined to take installs down silently: 1. perform_update()'s install-failed rollback branch restored the backup but never restarted pulse.service. Since the generated pulse-update.service gates on ExecCondition=systemctl is-active, every later timer run was then skipped and the install stayed down until manual intervention. Restart is now guaranteed by a service_was_active-guarded restart in that branch plus an ensure_service_restarted RETURN trap so no exit path can miss it (re-fix of #1323, originally c0b3a0e66, lost in 778a2577b and only partially restored in 672e81985). 2. install.sh aborted under errexit when writing the /bin/update helper on a read-only filesystem - after the new binary was installed and the service stopped, landing in bug 1's no-restart branch. The stock pulse-update.service uses ProtectSystem=strict, so /bin and /usr/local/bin are read-only on stock unattended updates; transient read-only remounts hit the same path. The helper write, PATH appends, and the /usr/local/bin/pulse symlink are now idempotent and non-fatal with a warning (install_binary_symlink). Contract: deployment-installability now pins fail-closed service availability for unattended updates and non-fatal writes outside the hardened unit's writable set, with proofs in pulse_auto_update_test.go and root_install_sh_test.go plus shell regression coverage in scripts/tests/test-pulse-auto-update.sh (installer-exits-nonzero path) and scripts/tests/test-install-update-resilience.sh (read-only helper and symlink paths, verified under set -e, root-safe via ENOTDIR). Fixes #1630 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-04 13:23:28 +00:00 · 2026-07-27 09:28:45 +01:00 · 2026-07-27 09:28:45 +01:00 · 806cbe83dc
commit 806cbe83dc
parent bf24a9a9cf
7 changed files with 569 additions and 15 deletions
--- a/docs/release-control/v6/internal/subsystems/deployment-installability.md
+++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md
@ -458,6 +458,32 @@ TLS floor in the dynamic config.
   (`TestRootInstallServiceGrantsIcmpProbeCapability`) pins the pairing, and
   `docs/CONFIGURATION.md` documents the `systemctl edit` override for units
   written before the grant existed.
+   Unattended update execution must fail closed on service availability
+   (#1630): `scripts/pulse-auto-update.sh` `perform_update` must leave the
+   service running on every exit path when it was active before the attempt,
+   including the installer-exits-nonzero rollback branch. The generated
+   `pulse-update.service` gates on `ExecCondition=systemctl is-active`, so a
+   service left stopped also silently disables every future unattended run.
+   This is enforced by a `service_was_active`-guarded restart in each rollback
+   branch plus the `ensure_service_restarted` RETURN-trap backstop, and pinned
+   by `scripts/installtests/pulse_auto_update_test.go`
+   (`TestPerformUpdateRestartsServiceWhenInstallerFails`,
+   `TestEnsureServiceRestartedHonorsPriorServiceState`) and
+   `scripts/tests/test-pulse-auto-update.sh`. For the same reason, root
+   `install.sh` writes outside the hardened update unit's writable set
+   (`ProtectSystem=strict` leaves only the install dir, config dir and `/tmp`
+   writable) must be idempotent and non-fatal warnings rather than errexit
+   aborts: the `/bin/update` helper heredoc, the PATH appends to
+   `/etc/profile` and `/etc/bash.bashrc`, and the `/usr/local/bin/pulse`
+   convenience symlink (`install_binary_symlink`, which must also keep an
+   already-correct link without rewriting it). An abort in any of these kills
+   the installer after the new binary is installed and the service is
+   stopped, landing in the rollback branch above; transient read-only
+   remounts hit the same paths on unhardened installs. Pinned by
+   `scripts/installtests/root_install_sh_test.go`
+   (`TestRootInstallScriptUpdateHelperWriteIsNonFatalOnReadOnlyPath`,
+   `TestRootInstallScriptBinarySymlinkIsIdempotentAndNonFatal`) and
+   `scripts/tests/test-install-update-resilience.sh`.
   The top-level `install.sh` asset published on GitHub Releases must be the
   root Pulse SERVER installer (the LXC / systemd / Proxmox VE installer that
   accepts `--version vX.Y.Z`, `--rc`, `--stable`, and friends). The rendered
--- a/install.sh
+++ b/install.sh
@ -3173,6 +3173,30 @@ download_release_archive() {
    return 0
 }

+# Create (or refresh) the convenience symlink to the installed binary.
+# Never fatal and idempotent: the link path can be unwritable — the generated
+# pulse-update.service runs the unattended updater with ProtectSystem=strict
+# (only the install dir, config dir and /tmp are writable), and transient
+# read-only filesystems hit the same error (#1630). The symlink is a
+# nice-to-have; an existing correct link is kept and a failure only warns.
+install_binary_symlink() {
+    local target="$1"
+    local link_path="$2"
+
+    if [[ "$(readlink "$link_path" 2>/dev/null)" == "$target" ]]; then
+        print_success "Symlink already in place at $link_path"
+        return 0
+    fi
+
+    mkdir -p "$(dirname "$link_path")" 2>/dev/null || true
+    if ln -sf "$target" "$link_path" 2>/dev/null; then
+        print_success "Symlink created at $link_path"
+    else
+        print_warn "Could not create symlink at $link_path (read-only filesystem?); pulse remains available at $target"
+    fi
+    return 0
+}
+
 install_pulse_archive() {
    local archive_path="$1"
    local expected_release="${2:-}"
@ -3263,10 +3287,8 @@ install_pulse_archive() {
    chown -R pulse:pulse "$INSTALL_DIR"

    rm -f "$INSTALL_DIR/bin/pulse.old"
-    mkdir -p "$(dirname "$BINARY_LINK_PATH")"
-    ln -sf "$INSTALL_DIR/bin/pulse" "$BINARY_LINK_PATH"
    print_success "Pulse binary installed to $INSTALL_DIR/bin/pulse"
-    print_success "Symlink created at $BINARY_LINK_PATH"
+    install_binary_symlink "$INSTALL_DIR/bin/pulse" "$BINARY_LINK_PATH"

    if [[ -f "$temp_extract/VERSION" ]]; then
        cp "$temp_extract/VERSION" "$INSTALL_DIR/VERSION"
@ -3730,8 +3752,7 @@ build_from_source() {
        fi
    done

-    mkdir -p "$(dirname "$BINARY_LINK_PATH")"
-    ln -sf "$INSTALL_DIR/bin/pulse" "$BINARY_LINK_PATH"
+    install_binary_symlink "$INSTALL_DIR/bin/pulse" "$BINARY_LINK_PATH"

    echo "$branch-$(git rev-parse --short HEAD)" > "$INSTALL_DIR/VERSION"
    echo "$branch" > "$BUILD_FROM_SOURCE_MARKER"
@ -3816,7 +3837,14 @@ setup_update_command() {
    local update_timer_path="${UPDATE_TIMER_PATH:-${PULSE_UPDATE_TIMER_PATH:-/etc/systemd/system/${service_name}-update.timer}}"
    local profile_path="${PULSE_PROFILE_PATH:-/etc/profile}"
    local bashrc_path="${PULSE_BASHRC_PATH:-/etc/bash.bashrc}"
-    cat > "$update_helper_path" <<EOF
+
+    # Writing the helper must never abort the installer: with errexit active a
+    # failed redirect here used to kill the run after the new binary was
+    # already installed and the service stopped (#1630). The path can
+    # legitimately be unwritable — the unattended updater runs under
+    # ProtectSystem=strict, and transient read-only filesystems hit this too.
+    mkdir -p "$(dirname "$update_helper_path")" 2>/dev/null || true
+    if ! cat 2>/dev/null > "$update_helper_path" <<EOF
 #!/usr/bin/env bash
 # Pulse update command
 # This script re-runs the Pulse installer using the configured manual channel
@ -3910,17 +3938,24 @@ fi
 echo ""
 echo "Update complete! Pulse will restart automatically."
 EOF
+    then
+        print_warn "Could not write the 'update' helper to $update_helper_path (read-only filesystem?); skipping optional update command setup"
+        return 0
+    fi

-    chmod +x "$update_helper_path"
+    chmod +x "$update_helper_path" 2>/dev/null || \
+        print_warn "Could not mark $update_helper_path executable"

    # Ensure /usr/local/bin is in PATH for all users
    if ! grep -q '/usr/local/bin' "$profile_path" 2>/dev/null; then
-        echo 'export PATH="/usr/local/bin:$PATH"' >> "$profile_path"
+        echo 'export PATH="/usr/local/bin:$PATH"' 2>/dev/null >> "$profile_path" || \
+            print_warn "Could not update PATH in $profile_path"
    fi

    # Also add to bash profile if it exists
    if [[ -f "$bashrc_path" ]] && ! grep -q '/usr/local/bin' "$bashrc_path" 2>/dev/null; then
-        echo 'export PATH="/usr/local/bin:$PATH"' >> "$bashrc_path"
+        echo 'export PATH="/usr/local/bin:$PATH"' 2>/dev/null >> "$bashrc_path" || \
+            print_warn "Could not update PATH in $bashrc_path"
    fi
 }

--- a/scripts/installtests/pulse_auto_update_test.go
+++ b/scripts/installtests/pulse_auto_update_test.go
@ -387,3 +387,141 @@ func TestInstalledBinaryIsPulseProGuard(t *testing.T) {
 		}
 	})
 }
+
+// TestPerformUpdateRestartsServiceWhenInstallerFails asserts the #1630
+// guarantee: when the downloaded installer exits non-zero (for example a
+// write to a read-only path aborting it under errexit) after it has already
+// stopped the service, perform_update must restore the backup AND leave the
+// service running. The generated pulse-update.service gates on
+// ExecCondition=systemctl is-active, so a service left stopped would also
+// disable every future unattended run.
+func TestPerformUpdateRestartsServiceWhenInstallerFails(t *testing.T) {
+	script := `
+set -uo pipefail
+TMP=$(mktemp -d)
+trap 'rm -rf "$TMP"' EXIT
+GITHUB_REPO="rcourtman/Pulse"
+INSTALL_DIR="$TMP/opt/pulse"
+CONFIG_DIR="$TMP/etc/pulse"
+mkdir -p "$INSTALL_DIR/bin" "$CONFIG_DIR"
+printf 'v5.1.24\n' > "$INSTALL_DIR/VERSION"
+printf '#!/usr/bin/env bash\necho v5.1.24\n' > "$INSTALL_DIR/bin/pulse"
+chmod +x "$INSTALL_DIR/bin/pulse"
+export INSTALL_DIR
+
+log() { echo "[$1] ${*:2}"; }
+detect_service_name() { echo pulse; }
+get_current_version() { tr -d '\r\n' < "$INSTALL_DIR/VERSION"; }
+verify_release_signature() { return 0; }
+sleep() { :; }
+
+# curl writes the installer / signature to the -o target; the fake installer
+# fails outright, like the real one aborting on a read-only filesystem after
+# it has stopped the service.
+curl() {
+  local out="" prev="" arg
+  for arg in "$@"; do
+    if [[ "$prev" == "-o" ]]; then out="$arg"; fi
+    prev="$arg"
+  done
+  if [[ -n "$out" ]]; then
+    case "$out" in
+      *.sig.*) printf 'dummy-signature\n' > "$out" ;;
+      *) printf '#!/usr/bin/env bash\nexit 1\n' > "$out" ;;
+    esac
+  fi
+  return 0
+}
+
+# Service is running at the was-active capture, then down (the installer
+# stopped it before failing) until an explicit start/restart.
+IS_ACTIVE_CALLS=0
+SERVICE_UP="no"
+STARTS=0
+systemctl() {
+  case "$1" in
+    is-active)
+      ((IS_ACTIVE_CALLS += 1))
+      if (( IS_ACTIVE_CALLS == 1 )); then return 0; fi
+      [[ "$SERVICE_UP" == "yes" ]] && return 0 || return 1
+      ;;
+    start|restart)
+      ((STARTS += 1))
+      SERVICE_UP="yes"
+      return 0
+      ;;
+  esac
+  return 1
+}
+` + extractAutoUpdateFunction(t, "is_prerelease_tag") + `
+` + extractAutoUpdateFunction(t, "resolve_install_script_url") + `
+` + extractAutoUpdateFunction(t, "wait_for_service_active") + `
+` + extractAutoUpdateFunction(t, "ensure_service_restarted") + `
+` + extractAutoUpdateFunction(t, "perform_update") + `
+if perform_update v5.1.25; then
+  echo "RESULT:succeeded"
+else
+  echo "RESULT:failed"
+fi
+echo "STARTS:$STARTS"
+echo "SERVICE:$SERVICE_UP"
+echo "VERSION:$(tr -d '\r\n' < "$INSTALL_DIR/VERSION")"
+`
+
+	out, err := exec.Command("bash", "-c", script).CombinedOutput()
+	if err != nil {
+		t.Fatalf("bash: %v\n%s", err, out)
+	}
+	got := string(out)
+	for _, want := range []string{"RESULT:failed", "SERVICE:yes", "VERSION:v5.1.24"} {
+		if !strings.Contains(got, want) {
+			t.Fatalf("missing %q in perform_update installer-failure output:\n%s", want, got)
+		}
+	}
+	if strings.Contains(got, "STARTS:0") {
+		t.Fatalf("service was never restarted after installer failure:\n%s", got)
+	}
+}
+
+// TestEnsureServiceRestartedHonorsPriorServiceState asserts the RETURN-trap
+// backstop behind the #1630 fix: a service that was inactive before the
+// update is left alone, and a service that was active is started again.
+func TestEnsureServiceRestartedHonorsPriorServiceState(t *testing.T) {
+	script := `
+set -uo pipefail
+SERVICE_UP="no"
+STARTS=0
+systemctl() {
+  case "$1" in
+    is-active) [[ "$SERVICE_UP" == "yes" ]] && return 0 || return 1 ;;
+    start|restart) ((STARTS += 1)); SERVICE_UP="yes"; return 0 ;;
+  esac
+  return 1
+}
+sleep() { :; }
+log() { echo "[$1] ${*:2}"; }
+` + extractAutoUpdateFunction(t, "wait_for_service_active") + `
+` + extractAutoUpdateFunction(t, "ensure_service_restarted") + `
+ensure_service_restarted pulse false || echo "INACTIVE_PATH_FAILED"
+echo "STARTS_AFTER_INACTIVE:$STARTS"
+ensure_service_restarted pulse true || echo "ACTIVE_PATH_FAILED"
+echo "STARTS_AFTER_ACTIVE:$STARTS"
+echo "SERVICE:$SERVICE_UP"
+`
+
+	out, err := exec.Command("bash", "-c", script).CombinedOutput()
+	if err != nil {
+		t.Fatalf("bash: %v\n%s", err, out)
+	}
+	got := string(out)
+	for _, want := range []string{"STARTS_AFTER_INACTIVE:0", "STARTS_AFTER_ACTIVE:1", "SERVICE:yes"} {
+		if !strings.Contains(got, want) {
+			t.Fatalf("missing %q in ensure_service_restarted output:\n%s", want, got)
+		}
+	}
+	for _, reject := range []string{"INACTIVE_PATH_FAILED", "ACTIVE_PATH_FAILED"} {
+		if strings.Contains(got, reject) {
+			t.Fatalf("ensure_service_restarted must always return 0 (it runs in a RETURN trap under set -e):\n%s", got)
+		}
+	}
+}
--- a/scripts/installtests/root_install_sh_test.go
+++ b/scripts/installtests/root_install_sh_test.go
@ -604,8 +604,8 @@ func TestRootInstallScriptSupportsInstanceScopedServerInstalls(t *testing.T) {
 		`UPDATE_SERVICE_PATH="${PULSE_UPDATE_SERVICE_PATH:-$(default_update_service_path_for_service "$SERVICE_NAME")}"`,
 		`UPDATE_TIMER_PATH="${PULSE_UPDATE_TIMER_PATH:-$(default_update_timer_path_for_service "$SERVICE_NAME")}"`,
 		`if [[ "$SERVICE_NAME_EXPLICIT" == "true" ]]; then`,
-		`mkdir -p "$(dirname "$BINARY_LINK_PATH")"`,
-		`ln -sf "$INSTALL_DIR/bin/pulse" "$BINARY_LINK_PATH"`,
+		`install_binary_symlink "$INSTALL_DIR/bin/pulse" "$BINARY_LINK_PATH"`,
+		`ln -sf "$target" "$link_path"`,
 		`safe_systemctl enable "$update_timer_unit" || true`,
 		`safe_systemctl start "$update_timer_unit" || true`,
 		`Environment="PULSE_SERVICE_NAME=$service_name"`,
@ -1350,3 +1350,94 @@ func TestRootInstallServiceGrantsIcmpProbeCapability(t *testing.T) {
 		t.Fatal("AmbientCapabilities=CAP_NET_RAW is not in the unit's security hardening block")
 	}
 }
+
+// TestRootInstallScriptUpdateHelperWriteIsNonFatalOnReadOnlyPath asserts the
+// #1630 guarantee: setup_update_command's write of the /bin/update helper
+// (and its PATH appends) must not abort the installer under errexit when the
+// destination is unwritable. The stock pulse-update.service runs the
+// unattended updater with ProtectSystem=strict, leaving /bin and
+// /usr/local/bin read-only; the old behavior killed the installer after the
+// new binary was installed and the service stopped, and the auto-update
+// rollback then left Pulse down. A regular file as the "parent directory"
+// makes writes beneath it fail with ENOTDIR, which also fails when the test
+// runs as root (unlike chmod 555).
+func TestRootInstallScriptUpdateHelperWriteIsNonFatalOnReadOnlyPath(t *testing.T) {
+	script := `
+set -euo pipefail
+TMP=$(mktemp -d)
+trap 'rm -rf "$TMP"' EXIT
+touch "$TMP/blocker"
+GITHUB_REPO="rcourtman/Pulse"
+INSTALL_SIGNATURE_IDENTITY="pulse-installer"
+INSTALL_SIGNATURE_NAMESPACE="pulse-install"
+PINNED_RELEASE_SSH_PUBLIC_KEY="test-key"
+print_warn() { echo "WARN: $*"; }
+print_success() { echo "OK: $*"; }
+release_signature_key_available() { :; }
+require_release_signature_verifier() { :; }
+verify_release_signature() { :; }
+` + extractRootInstallShellFunction(t, "setup_update_command") + `
+UPDATE_HELPER_PATH="$TMP/blocker/update" \
+PULSE_PROFILE_PATH="$TMP/profile" \
+PULSE_BASHRC_PATH="$TMP/bashrc" \
+setup_update_command
+echo "SURVIVED_UNWRITABLE"
+UPDATE_HELPER_PATH="$TMP/bin/update" \
+PULSE_PROFILE_PATH="$TMP/profile" \
+PULSE_BASHRC_PATH="$TMP/bashrc" \
+setup_update_command
+[[ -x "$TMP/bin/update" ]] && echo "HELPER_WRITTEN"
+grep -q "Pulse update command" "$TMP/bin/update" && echo "HELPER_BODY_OK"
+`
+
+	out, err := exec.Command("bash", "-c", script).CombinedOutput()
+	if err != nil {
+		t.Fatalf("setup_update_command aborted the installer on an unwritable helper path: %v\n%s", err, out)
+	}
+	got := string(out)
+	for _, want := range []string{"SURVIVED_UNWRITABLE", "WARN:", "HELPER_WRITTEN", "HELPER_BODY_OK"} {
+		if !strings.Contains(got, want) {
+			t.Fatalf("missing %q in setup_update_command output:\n%s", want, got)
+		}
+	}
+}
+
+// TestRootInstallScriptBinarySymlinkIsIdempotentAndNonFatal asserts the
+// companion #1630 guarantee for the /usr/local/bin/pulse convenience
+// symlink: an unwritable link path only warns, a writable one creates the
+// link, and an already-correct link is kept without needing ln at all
+// (the update-run case where the link survives but the fs is read-only).
+func TestRootInstallScriptBinarySymlinkIsIdempotentAndNonFatal(t *testing.T) {
+	script := `
+set -euo pipefail
+TMP=$(mktemp -d)
+trap 'rm -rf "$TMP"' EXIT
+touch "$TMP/blocker"
+mkdir -p "$TMP/bin"
+touch "$TMP/bin/pulse-binary"
+print_warn() { echo "WARN: $*"; }
+print_success() { echo "OK: $*"; }
+` + extractRootInstallShellFunction(t, "install_binary_symlink") + `
+install_binary_symlink "$TMP/bin/pulse-binary" "$TMP/blocker/pulse"
+echo "SURVIVED_UNWRITABLE"
+install_binary_symlink "$TMP/bin/pulse-binary" "$TMP/bin/pulse"
+[[ "$(readlink "$TMP/bin/pulse")" == "$TMP/bin/pulse-binary" ]] && echo "LINK_CREATED"
+ln() { echo "LN_CALLED_AGAIN"; return 1; }
+install_binary_symlink "$TMP/bin/pulse-binary" "$TMP/bin/pulse"
+echo "SURVIVED_EXISTING_LINK"
+`
+
+	out, err := exec.Command("bash", "-c", script).CombinedOutput()
+	if err != nil {
+		t.Fatalf("install_binary_symlink aborted under errexit: %v\n%s", err, out)
+	}
+	got := string(out)
+	for _, want := range []string{"SURVIVED_UNWRITABLE", "WARN:", "LINK_CREATED", "already in place", "SURVIVED_EXISTING_LINK"} {
+		if !strings.Contains(got, want) {
+			t.Fatalf("missing %q in install_binary_symlink output:\n%s", want, got)
+		}
+	}
+	if strings.Contains(got, "LN_CALLED_AGAIN") {
+		t.Fatalf("install_binary_symlink should not invoke ln when the correct link already exists:\n%s", got)
+	}
+}
--- a/scripts/pulse-auto-update.sh
+++ b/scripts/pulse-auto-update.sh
@ -346,6 +346,35 @@ wait_for_service_active() {
    return 1
 }

+# Guarantee the Pulse service is left running after an update attempt if (and
+# only if) it was running beforehand. Invoked from perform_update's RETURN trap
+# so that no exit path — present or future — can leave Pulse stopped (#1630:
+# the install-failed rollback branch restored the backup but never restarted
+# the service). This matters doubly because the generated pulse-update.service
+# uses ExecCondition=systemctl is-active pulse.service: once Pulse is down,
+# every subsequent timer run is skipped and the install stays down until
+# someone intervenes. Always returns 0 (it runs under set -e in a trap).
+ensure_service_restarted() {
+    local service_name=$1
+    local service_was_active=$2
+
+    if [[ "$service_was_active" != "true" ]]; then
+        return 0
+    fi
+    if systemctl is-active --quiet "$service_name" 2>/dev/null; then
+        return 0
+    fi
+
+    log warn "Pulse service is not active after update attempt; starting it"
+    systemctl start "$service_name" 2>/dev/null || true
+    if wait_for_service_active "$service_name" 20; then
+        log info "Pulse service is running again"
+    else
+        log error "Pulse service could not be started after update attempt; manual intervention required (systemctl start $service_name)"
+    fi
+    return 0
+}
+
 perform_update() {
    local new_version=$1
    local service_name=$(detect_service_name)
@ -360,6 +389,11 @@ perform_update() {
        service_was_active="true"
    fi

+    # Whatever way this function exits, never leave Pulse stopped when it was
+    # running before the update (#1630). Extended below once the installer
+    # tempfiles exist.
+    trap 'ensure_service_restarted "$service_name" "$service_was_active"' RETURN
+
    # Refuse to install a prerelease via the unattended updater. The stable
    # channel must never cross onto a tag like v6.0.0-rc.2, even if every
    # caller above this point thought it was safe.
@ -406,7 +440,7 @@ perform_update() {

    installer_tmp=$(mktemp /tmp/pulse-update-installer.XXXXXX)
    signature_tmp=$(mktemp /tmp/pulse-update-installer.sig.XXXXXX)
-    trap 'rm -f "$installer_tmp" "$signature_tmp"' RETURN
+    trap 'rm -f "$installer_tmp" "$signature_tmp"; ensure_service_restarted "$service_name" "$service_was_active"' RETURN

    if ! curl -fsSL "$install_script_url" -o "$installer_tmp"; then
        log error "Failed to download installer from $install_script_url"
@ -510,10 +544,18 @@ perform_update() {
        if [[ -f "$backup_dir/VERSION" ]]; then
            cp -f "$backup_dir/VERSION" "$INSTALL_DIR/VERSION"
        fi
-        
+
+        # Restart the restored binary if Pulse was running before the update.
+        # The installer stops the service before it can fail, so skipping this
+        # left Pulse down indefinitely (#1630); the RETURN trap above is the
+        # backstop if this path ever changes.
+        if [[ "$service_was_active" == "true" ]]; then
+            systemctl restart "$service_name" || true
+        fi
+
        # Clean up backup
        rm -rf "$backup_dir"
-        
+
        return 1
    fi
 }
--- a/scripts/tests/test-install-update-resilience.sh
+++ b/scripts/tests/test-install-update-resilience.sh
@ -68,4 +68,68 @@ fi
 [[ "$START_ATTEMPTS" -ge 1 ]] || fail "should attempt an explicit restart, got $START_ATTEMPTS"
 [[ "$PRINT_BUF" == *"did not come back up"* ]] || fail "should surface a clear error, got: $PRINT_BUF"

-echo "PASS: install.sh update-resilience helpers (#1323)"
+# --- Cases 5+: read-only-filesystem resilience (#1630) -----------------------
+# The unattended updater runs the installer under pulse-update.service, whose
+# ProtectSystem=strict leaves /bin and /usr/local/bin read-only; transient
+# read-only remounts hit the same paths. Writes outside the install dir must
+# be non-fatal: aborting used to kill the installer with errexit active after
+# the new binary was installed and the service stopped.
+TMPDIR_RO="$(mktemp -d)"
+# A regular file as the "parent directory" makes any write beneath it fail
+# with ENOTDIR — unlike chmod 555, this also fails when running as root.
+touch "$TMPDIR_RO/blocker"
+
+# --- Case 5: setup_update_command must not abort the installer (errexit) ----
+out=$(
+    set -e
+    PRINT_BUF=""
+    UPDATE_HELPER_PATH="$TMPDIR_RO/blocker/update"
+    PULSE_PROFILE_PATH="$TMPDIR_RO/profile"
+    PULSE_BASHRC_PATH="$TMPDIR_RO/bashrc"
+    setup_update_command
+    printf '%s' "$PRINT_BUF"
+)
+rc=$?
+[[ "$rc" -eq 0 ]] || fail "setup_update_command must not abort the installer when the helper path is unwritable (rc=$rc)"
+[[ "$out" == *"WARN:"*"$TMPDIR_RO/blocker/update"* ]] || fail "should warn about the unwritable helper path, got: $out"
+
+# --- Case 6: setup_update_command still writes the helper when it can -------
+out=$(
+    set -e
+    PRINT_BUF=""
+    UPDATE_HELPER_PATH="$TMPDIR_RO/bin/update"
+    PULSE_PROFILE_PATH="$TMPDIR_RO/profile"
+    PULSE_BASHRC_PATH="$TMPDIR_RO/bashrc"
+    setup_update_command
+    printf '%s' "$PRINT_BUF"
+)
+rc=$?
+[[ "$rc" -eq 0 ]] || fail "setup_update_command should succeed on a writable path (rc=$rc)"
+[[ -x "$TMPDIR_RO/bin/update" ]] || fail "should create an executable update helper at a writable path"
+grep -q "Pulse update command" "$TMPDIR_RO/bin/update" || fail "helper should contain the expected script body"
+
+# --- Case 7: install_binary_symlink is non-fatal on an unwritable path ------
+PRINT_BUF=""
+install_binary_symlink "$TMPDIR_RO/bin/update" "$TMPDIR_RO/blocker/pulse" || \
+    fail "install_binary_symlink must not fail on an unwritable link path"
+[[ "$PRINT_BUF" == *"WARN:"* ]] || fail "should warn when the symlink cannot be created, got: $PRINT_BUF"
+
+# --- Case 8: install_binary_symlink creates and keeps the link ---------------
+PRINT_BUF=""
+install_binary_symlink "$TMPDIR_RO/bin/update" "$TMPDIR_RO/bin/pulse" || \
+    fail "install_binary_symlink should succeed on a writable path"
+[[ "$(readlink "$TMPDIR_RO/bin/pulse")" == "$TMPDIR_RO/bin/update" ]] || \
+    fail "symlink should point at the installed binary"
+# Idempotence: with the correct link already present it must not need ln at
+# all (matches an update run where the link survives but the fs is read-only).
+ln() { return 1; }
+PRINT_BUF=""
+install_binary_symlink "$TMPDIR_RO/bin/update" "$TMPDIR_RO/bin/pulse" || \
+    fail "install_binary_symlink should succeed when the correct link already exists"
+[[ "$PRINT_BUF" == *"already in place"* ]] || \
+    fail "should recognise an existing correct link, got: $PRINT_BUF"
+unset -f ln
+
+rm -rf "$TMPDIR_RO"
+
+echo "PASS: install.sh update-resilience helpers (#1323, #1630)"
--- a/scripts/tests/test-pulse-auto-update.sh
+++ b/scripts/tests/test-pulse-auto-update.sh
@ -199,12 +199,170 @@ INSTALLER
  return "${status}"
 }

+test_ensure_service_restarted_noops_when_service_was_inactive() {
+  ENSURE_TEST_STARTS=0
+  systemctl() {
+    if [[ "$1" == "start" ]] || [[ "$1" == "restart" ]]; then
+      ((ENSURE_TEST_STARTS += 1))
+    fi
+    return 1
+  }
+  sleep() { :; }
+
+  ensure_service_restarted pulse "false" || return 1
+  if (( ENSURE_TEST_STARTS != 0 )); then
+    echo "expected no start attempts when the service was inactive before the update, got ${ENSURE_TEST_STARTS}" >&2
+    return 1
+  fi
+  return 0
+}
+
+test_ensure_service_restarted_starts_stopped_service() {
+  ENSURE_TEST_STARTS=0
+  ENSURE_TEST_UP="no"
+  systemctl() {
+    case "$1" in
+      is-active)
+        [[ "$ENSURE_TEST_UP" == "yes" ]] && return 0 || return 1
+        ;;
+      start|restart)
+        ((ENSURE_TEST_STARTS += 1))
+        ENSURE_TEST_UP="yes"
+        return 0
+        ;;
+    esac
+    return 1
+  }
+  sleep() { :; }
+
+  ensure_service_restarted pulse "true" || return 1
+  if (( ENSURE_TEST_STARTS != 1 )); then
+    echo "expected exactly one start attempt, got ${ENSURE_TEST_STARTS}" >&2
+    return 1
+  fi
+  if [[ "$ENSURE_TEST_UP" != "yes" ]]; then
+    echo "expected the service to be running afterwards" >&2
+    return 1
+  fi
+  return 0
+}
+
+test_perform_update_restarts_service_when_installer_fails() {
+  # Regression for #1630: the installer stops the service and can then exit
+  # non-zero (e.g. a write to a read-only path aborts it). perform_update's
+  # rollback branch must restart Pulse — previously it restored the backup
+  # and returned 1 with the service left stopped, which also disabled every
+  # future timer run via pulse-update.service's ExecCondition.
+  local tmpdir
+  tmpdir="$(mktemp -d)"
+  local status=0
+  # perform_update installs a RETURN trap referencing these; declare them here
+  # so the trap is safe under set -u if it surfaces in this calling scope.
+  local installer_tmp="" signature_tmp=""
+  local service_name="pulse" service_was_active="false"
+
+  INSTALL_DIR="${tmpdir}/opt/pulse"
+  CONFIG_DIR="${tmpdir}/etc/pulse"
+  mkdir -p "${INSTALL_DIR}/bin" "${CONFIG_DIR}"
+
+  printf 'v5.1.24\n' > "${INSTALL_DIR}/VERSION"
+  cat > "${INSTALL_DIR}/bin/pulse" <<'EOF'
+#!/usr/bin/env bash
+echo "v5.1.24"
+EOF
+  chmod +x "${INSTALL_DIR}/bin/pulse"
+
+  export INSTALL_DIR
+
+  is_prerelease_tag() { return 1; }
+  detect_service_name() { echo "pulse"; }
+  resolve_install_script_url() { echo "http://localhost/install.sh"; }
+  verify_release_signature() { return 0; }
+  get_current_version() { tr -d '\r\n' < "${INSTALL_DIR}/VERSION"; }
+
+  # curl writes the installer / signature to the -o target. The fake installer
+  # fails outright, like the /bin/update heredoc aborting on a read-only
+  # filesystem after the real installer has already stopped the service.
+  curl() {
+    local out="" prev=""
+    local arg
+    for arg in "$@"; do
+      if [[ "$prev" == "-o" ]]; then out="$arg"; fi
+      prev="$arg"
+    done
+    if [[ -n "$out" ]]; then
+      case "$out" in
+        *.sig.*) printf 'dummy-signature\n' > "$out" ;;
+        *)
+          cat > "$out" <<'INSTALLER'
+#!/usr/bin/env bash
+exit 1
+INSTALLER
+          ;;
+      esac
+    fi
+    return 0
+  }
+
+  # Service is running when perform_update checks (was-active capture), then
+  # down (the real installer stops it before failing) until start/restart.
+  AUTOUPDATE_TEST_IS_ACTIVE_CALLS=0
+  AUTOUPDATE_TEST_UP="no"
+  AUTOUPDATE_TEST_STARTS=0
+  systemctl() {
+    case "$1" in
+      is-active)
+        ((AUTOUPDATE_TEST_IS_ACTIVE_CALLS += 1))
+        if (( AUTOUPDATE_TEST_IS_ACTIVE_CALLS == 1 )); then
+          return 0
+        fi
+        [[ "$AUTOUPDATE_TEST_UP" == "yes" ]] && return 0 || return 1
+        ;;
+      start|restart)
+        ((AUTOUPDATE_TEST_STARTS += 1))
+        AUTOUPDATE_TEST_UP="yes"
+        return 0
+        ;;
+    esac
+    return 1
+  }
+
+  sleep() { :; }
+
+  if perform_update "v5.1.25"; then
+    echo "perform_update unexpectedly succeeded when the installer failed" >&2
+    status=1
+  fi
+  # perform_update installs a RETURN trap; clear it so it does not leak into
+  # subsequent function returns in this sourced test harness.
+  trap - RETURN 2>/dev/null || true
+
+  if (( AUTOUPDATE_TEST_STARTS < 1 )); then
+    echo "expected the service to be restarted after installer failure, got ${AUTOUPDATE_TEST_STARTS} start attempts" >&2
+    status=1
+  fi
+  if [[ "$AUTOUPDATE_TEST_UP" != "yes" ]]; then
+    echo "expected the service to be running after the failed update" >&2
+    status=1
+  fi
+  if [[ "$(tr -d '\r\n' < "${INSTALL_DIR}/VERSION")" != "v5.1.24" ]]; then
+    echo "expected VERSION to remain v5.1.24 after failed install" >&2
+    status=1
+  fi
+
+  rm -rf "${tmpdir}"
+  return "${status}"
+}
+
 main() {
  assert_success "wait_for_service_active retries until active" test_wait_for_service_active_succeeds_after_retry
  assert_success "wait_for_service_active times out when never active" test_wait_for_service_active_times_out_when_never_active
  assert_success "pick_highest_stable_tag ignores list order and prereleases" test_pick_highest_stable_tag_ignores_list_order_and_prereleases
  assert_success "get_latest_stable_version prefers highest version over created order" test_get_latest_stable_version_prefers_highest_over_created_order
  assert_success "perform_update restores backup when service stays down" test_perform_update_restores_backup_when_service_stays_down
+  assert_success "ensure_service_restarted no-ops when service was inactive" test_ensure_service_restarted_noops_when_service_was_inactive
+  assert_success "ensure_service_restarted starts a stopped service" test_ensure_service_restarted_starts_stopped_service
+  assert_success "perform_update restarts service when installer fails" test_perform_update_restarts_service_when_installer_fails

  if (( failures > 0 )); then
    echo "Total failures: ${failures}" >&2