From 9e89e2d3c8bd8022f679f898d54f2b45f92d6c06 Mon Sep 17 00:00:00 2001 From: MickLesk <47820557+MickLesk@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:37:31 +0100 Subject: [PATCH 1/3] Shield pct start and the template check from a closed console A closed console sends SIGHUP to every process in the foreground group. The run survives it since #219, but pct start and the pveam list | grep template check did not: in three days 10 runs reported "would not start" for a container that came up, and 5 "not available after download" for a template on disk. --- .github/workflows/verify-split-files.yml | 5 ++ pve/backend.func | 16 +++-- tests/hangup-host-commands.sh | 88 ++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 tests/hangup-host-commands.sh diff --git a/.github/workflows/verify-split-files.yml b/.github/workflows/verify-split-files.yml index 85a3153..544c61b 100644 --- a/.github/workflows/verify-split-files.yml +++ b/.github/workflows/verify-split-files.yml @@ -31,6 +31,7 @@ on: - "tests/docker-lxcfs.sh" - "tests/java-memory.sh" - "tests/sudo-env.sh" + - "tests/hangup-host-commands.sh" - "tests/deb822-local-keyring.sh" - "tests/gpu-pci-class.sh" - "tests/nvidia-version-pin.sh" @@ -92,6 +93,7 @@ on: - "tests/docker-lxcfs.sh" - "tests/java-memory.sh" - "tests/sudo-env.sh" + - "tests/hangup-host-commands.sh" - "tests/deb822-local-keyring.sh" - "tests/gpu-pci-class.sh" - "tests/nvidia-version-pin.sh" @@ -179,6 +181,9 @@ jobs: - name: sudo environment tests run: bash tests/sudo-env.sh + - name: Hangup during host commands tests + run: bash tests/hangup-host-commands.sh + - name: deb822 local keyring tests run: bash tests/deb822-local-keyring.sh diff --git a/pve/backend.func b/pve/backend.func index da4bd73..ffbee09 100644 --- a/pve/backend.func +++ b/pve/backend.func @@ -177,8 +177,8 @@ _pct_start_failed() { # also return 0 and leave the container stopped, so ask for the real state. _pct_restart() { local ctid="$1" what="$2" err - err="$(pct start "$ctid" 2>&1)" || true - [[ "$(pct status "$ctid" 2>/dev/null)" == "status: running" ]] && return 0 + err="$(_cs_shield_hangup pct start "$ctid" 2>&1)" || true + [[ "$(_cs_shield_hangup pct status "$ctid" 2>/dev/null)" == "status: running" ]] && return 0 msg_error "Container ${ctid} did not start after ${what}" _pct_start_failed "$ctid" "$err" return 1 @@ -268,6 +268,11 @@ _pve_wait_for_detached_install() { done } +# A closed console killed this pipeline, and a downloaded template read as missing. +_template_in_storage() { + pveam list "$TEMPLATE_STORAGE" 2>/dev/null | grep -qF "$1" +} + # Where a template lands in $TEMPLATE_STORAGE, or the default dir layout when # pvesm cannot say. _template_path() { @@ -1080,7 +1085,8 @@ EOF # the container simply would not start. # pct can also fail and leave it running ("CT 104 already running"). local _start_out - if ! _start_out="$(pct start "$CTID" 2>&1)" && [[ "$(pct status "$CTID" 2>/dev/null)" != "status: running" ]]; then + # A closed console killed pct start halfway, and a container that came up was reported as one that would not. + if ! _start_out="$(_cs_shield_hangup pct start "$CTID" 2>&1)" && [[ "$(_cs_shield_hangup pct status "$CTID" 2>/dev/null)" != "status: running" ]]; then msg_error "Container ${CTID} was created but would not start" _pct_start_failed "$CTID" "$_start_out" exit 227 @@ -3586,7 +3592,7 @@ create_lxc_container() { printf '%s\n' "$_dl_out" >>"${BUILD_LOG:-/dev/null}" # pveam can report a failed download and still exit 0, so only the # template in storage counts as success. - if pveam list "$TEMPLATE_STORAGE" 2>/dev/null | grep -qF "$TEMPLATE"; then + if _cs_shield_hangup _template_in_storage "$TEMPLATE"; then msg_ok "Template download successful." break fi @@ -3608,7 +3614,7 @@ create_lxc_container() { done fi - if ! pveam list "$TEMPLATE_STORAGE" 2>/dev/null | grep -q "$TEMPLATE"; then + if ! _cs_shield_hangup _template_in_storage "$TEMPLATE"; then msg_error "Template $TEMPLATE not available in storage $TEMPLATE_STORAGE after download." exit 223 fi diff --git a/tests/hangup-host-commands.sh b/tests/hangup-host-commands.sh new file mode 100644 index 0000000..2342e60 --- /dev/null +++ b/tests/hangup-host-commands.sh @@ -0,0 +1,88 @@ +#!/usr/bin/env bash +# A closed console sends SIGHUP to the whole foreground group. Since the run +# survives that, an unshielded pct start or template check failed instead: +# 10 runs in three days reported "would not start" for a container that came +# up, 5 "not available after download" for a template on disk. +set -uo pipefail + +CORE_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." && pwd)" +BACKEND="$CORE_DIR/pve/backend.func" +TEST_DIR="$(mktemp -d)" +trap 'rm -rf -- "$TEST_DIR"' EXIT +passed=0 failed=0 + +expect() { + if [[ "$2" == "$3" ]]; then + echo "PASS $1" + passed=$((passed + 1)) + else + echo "FAIL $1: expected <$2>, got <$3>" >&2 + failed=$((failed + 1)) + fi +} + +if ! command -v setsid >/dev/null || [[ ! -r /proc/self/stat ]]; then + echo "SKIP needs setsid and /proc" + exit 0 +fi + +# Each stub hangs up its whole process group, as a closing console does, then does its work. +mkdir -p "$TEST_DIR/bin" +cat >"$TEST_DIR/bin/pct" <<'EOF' +#!/usr/bin/env bash +case "$1" in +start) + kill -HUP -- "-$(awk '{print $5}' /proc/$$/stat)" + sleep 0.3 + : >"$MARK/started" + echo "Started pve-container@$2.service" + ;; +status) [[ -f "$MARK/started" ]] && echo "status: running" || echo "status: stopped" ;; +esac +EOF +cat >"$TEST_DIR/bin/pveam" <<'EOF' +#!/usr/bin/env bash +kill -HUP -- "-$(awk '{print $5}' /proc/$$/stat)" +sleep 0.3 +echo "local:vztmpl/debian-13-standard_13.6-1_amd64.tar.zst 129.66MB" +EOF +chmod +x "$TEST_DIR/bin/pct" "$TEST_DIR/bin/pveam" + +{ + awk '/^_cs_shield_hangup[(][)] [{]/,/^}/' "$CORE_DIR/core/core.func" + awk '/^(_template_in_storage|_pct_restart)[(][)] [{]/,/^}/' "$BACKEND" +} >"$TEST_DIR/funcs.sh" + +# In its own session, with the HUP trap the engine has, the way a console runs it. +run() { + rm -f "$TEST_DIR/started" + MARK="$TEST_DIR" PATH="$TEST_DIR/bin:$PATH" setsid -w bash -c ' + source "$1" + trap ":" HUP + msg_error() { :; } + _pct_start_failed() { :; } + TEMPLATE_STORAGE=local + eval "$2"' _ "$TEST_DIR/funcs.sh" "$1" 2>/dev/null +} +started() { [[ -f "$TEST_DIR/started" ]] && echo started || echo aborted; } + +run 'pct start 101 >/dev/null 2>&1' +expect "the stub's hangup kills an unshielded pct start (the old code)" aborted "$(started)" + +out="$(run 'out="$(_cs_shield_hangup pct start 101 2>&1)"; echo "$out"')" +expect "a shielded pct start finishes" started "$(started)" +expect "and its output arrives in full" "Started pve-container@101.service" "$out" + +expect "_pct_restart sees the container running" 0 "$(run '_pct_restart 101 "the config change"; echo $?')" + +expect "the old template check loses a template on disk" 1 \ + "$(run 'pveam list local 2>/dev/null | grep -q debian-13-standard_13.6-1; echo $(($? != 0))')" +expect "the shielded check finds it" 0 \ + "$(run '_cs_shield_hangup _template_in_storage debian-13-standard_13.6-1_amd64.tar.zst; echo $?')" + +expect "the start step uses the shield" 1 "$(grep -c '_start_out="$(_cs_shield_hangup pct start "$CTID" 2>&1)"' "$BACKEND")" +expect "both template checks use it" 2 "$(grep -c '_cs_shield_hangup _template_in_storage "$TEMPLATE"' "$BACKEND")" +expect "the pveam list | grep pipeline lives only in the helper" 1 "$(grep -c 'pveam list "$TEMPLATE_STORAGE" 2>/dev/null | grep -q' "$BACKEND" | tr -d ' ')" + +printf '%s passed, %s failed\n' "$passed" "$failed" +((failed == 0)) From 28c903b8cf816d3de0e83a84db91fa784eb48885 Mon Sep 17 00:00:00 2001 From: MickLesk <47820557+MickLesk@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:38:39 +0100 Subject: [PATCH 2/3] Move the test entry away from the other open branches --- .github/workflows/verify-split-files.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/verify-split-files.yml b/.github/workflows/verify-split-files.yml index 544c61b..bd55c94 100644 --- a/.github/workflows/verify-split-files.yml +++ b/.github/workflows/verify-split-files.yml @@ -31,9 +31,9 @@ on: - "tests/docker-lxcfs.sh" - "tests/java-memory.sh" - "tests/sudo-env.sh" - - "tests/hangup-host-commands.sh" - "tests/deb822-local-keyring.sh" - "tests/gpu-pci-class.sh" + - "tests/hangup-host-commands.sh" - "tests/nvidia-version-pin.sh" - "tests/external-installer.sh" - "tests/forge-version-key.sh" @@ -93,9 +93,9 @@ on: - "tests/docker-lxcfs.sh" - "tests/java-memory.sh" - "tests/sudo-env.sh" - - "tests/hangup-host-commands.sh" - "tests/deb822-local-keyring.sh" - "tests/gpu-pci-class.sh" + - "tests/hangup-host-commands.sh" - "tests/nvidia-version-pin.sh" - "tests/external-installer.sh" - "tests/forge-version-key.sh" @@ -181,15 +181,15 @@ jobs: - name: sudo environment tests run: bash tests/sudo-env.sh - - name: Hangup during host commands tests - run: bash tests/hangup-host-commands.sh - - name: deb822 local keyring tests run: bash tests/deb822-local-keyring.sh - name: GPU detection by PCI class tests run: bash tests/gpu-pci-class.sh + - name: Hangup during host commands tests + run: bash tests/hangup-host-commands.sh + - name: NVIDIA userspace version pin tests run: bash tests/nvidia-version-pin.sh From abb6d5881b1cddacff138956a3da044b9a61d573 Mon Sep 17 00:00:00 2001 From: MickLesk <47820557+MickLesk@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:49:52 +0100 Subject: [PATCH 3/3] Keep a shielded command's exit status inside $(...) Inside a command substitution the engine's HUP trap is already reset, so that subshell died of the hangup and returned 129 for a command that had finished. update-apps runs vzdump this way: ten backups since the shield landed ran to their notification and were still reported as "Backup failed", and the update stopped. In a subshell that does not catch HUP itself, ignore it there too. --- core/core.func | 7 +++++++ tests/hangup-host-commands.sh | 12 +++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/core/core.func b/core/core.func index 05312ce..2e07fe8 100644 --- a/core/core.func +++ b/core/core.func @@ -1013,6 +1013,13 @@ _cs_reattach_output() { # subshell sets the signal to ignore, which exec carries into the program, so # it finishes even though the terminal is gone. _cs_shield_hangup() { + # Inside $(...) the engine's trap is already reset, so that subshell died of the + # hangup and returned 129 for a command that finished (update-apps: "Backup failed"). + local _sig="" + if [[ "$BASHPID" != "$$" && -r "/proc/$BASHPID/status" ]]; then + while read -r _sig; do [[ "$_sig" == SigCgt:* ]] && break; done <"/proc/$BASHPID/status" + ((16#${_sig##*[[:space:]]} & 1)) || trap '' HUP + fi ( trap '' HUP # apt restores the terminal it was given and exits 100 when it is gone. diff --git a/tests/hangup-host-commands.sh b/tests/hangup-host-commands.sh index 2342e60..b8f53d1 100644 --- a/tests/hangup-host-commands.sh +++ b/tests/hangup-host-commands.sh @@ -46,7 +46,13 @@ kill -HUP -- "-$(awk '{print $5}' /proc/$$/stat)" sleep 0.3 echo "local:vztmpl/debian-13-standard_13.6-1_amd64.tar.zst 129.66MB" EOF -chmod +x "$TEST_DIR/bin/pct" "$TEST_DIR/bin/pveam" +cat >"$TEST_DIR/bin/vzdump" <<'EOF' +#!/usr/bin/env bash +kill -HUP -- "-$(awk '{print $5}' /proc/$$/stat)" +sleep 0.3 +echo "INFO: notified via target mail-to-root" +EOF +chmod +x "$TEST_DIR/bin/pct" "$TEST_DIR/bin/pveam" "$TEST_DIR/bin/vzdump" { awk '/^_cs_shield_hangup[(][)] [{]/,/^}/' "$CORE_DIR/core/core.func" @@ -73,6 +79,10 @@ out="$(run 'out="$(_cs_shield_hangup pct start 101 2>&1)"; echo "$out"')" expect "a shielded pct start finishes" started "$(started)" expect "and its output arrives in full" "Started pve-container@101.service" "$out" +# update-apps runs vzdump this way: the backup finished, yet the subshell returned 129. +expect "a shielded command inside \$(...) keeps its own exit status" "0 INFO: notified via target mail-to-root" "$(run 'out="$(_cs_shield_hangup vzdump 101 2>&1)"; echo "$? $out"')" +expect "called directly, it leaves the engine's own HUP trap alone" 1 "$(run '_cs_shield_hangup true; trap -p HUP | grep -c "trap -- '"':'"'"')" + expect "_pct_restart sees the container running" 0 "$(run '_pct_restart 101 "the config change"; echo $?')" expect "the old template check loses a template on disk" 1 \