From 9356cb871fda8a7bc57389c41be8198cd2165861 Mon Sep 17 00:00:00 2001 From: MickLesk <47820557+MickLesk@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:23:57 +0100 Subject: [PATCH] Stop reporting a closed console as a user abort, and shield the host's own work The telemetry service decides what a user abort is by searching the log text for sigint, ctrl+c, sighup and a few phrases. Our hangup message said SIGHUP, so every run that lost its console was filed as "the operator pressed Ctrl-C": of 300 aborted records in the thirteen hours after the fix shipped, 81 were this message. Twenty of those were real failures -- apt errors, containers that would not start -- which left the failure rate understated and the bugs invisible. The message now says the terminal closed, and a test keeps every hangup message clear of the words the classifier reads. The shield that kept apt and npm alive only covered commands run through silent(). pct create and the four pveam download calls run bare on the host, so a console closed during the template download still killed the run with 129. They go through the same helper now, which is named for what it does rather than for silent(). --- core/core.func | 8 ++++++-- core/error_handler.func | 2 +- docs/backends.md | 5 +++-- incus/build.func | 2 +- pve/backend.func | 10 +++++----- tests/console-hangup.sh | 32 ++++++++++++++++++++++++++++++-- tests/failed-command-output.sh | 2 +- ui/API.txt | 2 +- 8 files changed, 48 insertions(+), 15 deletions(-) diff --git a/core/core.func b/core/core.func index ce46d35..14c9ae0 100644 --- a/core/core.func +++ b/core/core.func @@ -958,7 +958,11 @@ _cs_reattach_output() { exec >"$log" 2>&1 } -_silent_exec() { +# A closed console sends SIGHUP to every process in the foreground group, and a +# program that does not handle it dies. Run the long ones through here: the +# subshell sets the signal to ignore, which exec carries into the program, so +# it finishes even though the terminal is gone. +_cs_shield_hangup() { ( trap '' HUP exec "$@" @@ -999,7 +1003,7 @@ silent() { local rc run=("$@") case "$(type -t "$1" 2>/dev/null)" in function | builtin | "") ;; - *) run=(_silent_exec "$@") ;; + *) run=(_cs_shield_hangup "$@") ;; esac if [[ "${VERBOSE:-no}" == "yes" ]]; then # Not a pipe: the command must stay in this shell so functions keep their side effects. diff --git a/core/error_handler.func b/core/error_handler.func index dcba566..dc13f16 100644 --- a/core/error_handler.func +++ b/core/error_handler.func @@ -889,7 +889,7 @@ on_hangup() { [[ -n "$log" ]] || log="${INSTALL_LOG:-${BUILD_LOG:-/dev/null}}" exec >"$log" 2>&1 export _CS_HANGUP=1 CS_ANIMATION_OK=0 _CS_HANGUP_LOG="$log" - echo "[WARN] $(date '+%Y-%m-%d %H:%M:%S') The terminal hung up (SIGHUP); continuing, output follows here. (pid $$, subshell ${BASH_SUBSHELL}, during: ${BASH_COMMAND:-?})" + echo "[WARN] $(date '+%Y-%m-%d %H:%M:%S') The terminal closed; continuing, output follows here. (pid $$, subshell ${BASH_SUBSHELL}, during: ${BASH_COMMAND:-?})" } # ============================================================================== diff --git a/docs/backends.md b/docs/backends.md index 2d09cb5..0380c5c 100644 --- a/docs/backends.md +++ b/docs/backends.md @@ -16,9 +16,10 @@ wizard calls. | [`vm-core.func`](../pve/vm-core.func) | VM creation via `qm`, including disk import and template handling | | [`vm-app.func`](../pve/vm-app.func) | Deploys an application that normally goes into an LXC container into a full VM instead | -A closed console is not an abort. SIGHUP reaches the host script and the +A closed console is not an abort. The hangup reaches the host script and the install inside the container; both carry on with their output in their logs -(`on_hangup`), external commands under `$STD` ignore the signal, and when +(`on_hangup`), the long-running commands on either side run through +`_cs_shield_hangup` so the signal cannot reach them, and when lxc-attach dies with the terminal the host waits for the container's `.done`/`.failed` markers (`_pve_wait_for_detached_install`) before it reports. Ctrl-C still aborts. diff --git a/incus/build.func b/incus/build.func index c6ba587..123622b 100644 --- a/incus/build.func +++ b/incus/build.func @@ -642,7 +642,7 @@ _incus_on_hangup() { [[ -n "$log" ]] || log="/tmp/incus-hangup-${SESSION_ID:-$$}.log" exec >"$log" 2>&1 export _CS_HANGUP=1 _CS_HANGUP_LOG="$log" CS_STDIN_OK=0 - echo "[WARN] $(date '+%Y-%m-%d %H:%M:%S') The terminal hung up (SIGHUP); continuing, output follows here." + echo "[WARN] $(date '+%Y-%m-%d %H:%M:%S') The terminal closed; continuing, output follows here." } trap 'api_exit_script' EXIT diff --git a/pve/backend.func b/pve/backend.func index a6e19f4..cc0f3ff 100644 --- a/pve/backend.func +++ b/pve/backend.func @@ -138,7 +138,7 @@ _pct_create() { local prev rc prev="$(umask)" umask 022 - pct create "$@" + _cs_shield_hangup pct create "$@" rc=$? umask "$prev" return $rc @@ -2726,7 +2726,7 @@ create_lxc_container() { if [[ ! -f "$fallback_path" ]]; then msg_info "Downloading $os_type $fallback_ver template" - if ! pveam download "$TEMPLATE_STORAGE" "$fallback_template" >>"${BUILD_LOG:-/dev/null}" 2>&1; then + if ! _cs_shield_hangup pveam download "$TEMPLATE_STORAGE" "$fallback_template" >>"${BUILD_LOG:-/dev/null}" 2>&1; then msg_error "Failed to download $os_type $fallback_ver template." return 1 fi @@ -3024,7 +3024,7 @@ create_lxc_container() { if [[ -n "$CUSTOM_TEMPLATE_PATH" ]]; then download_catalog_template "$dest" else - pveam download "$TEMPLATE_STORAGE" "$TEMPLATE" >>"${BUILD_LOG:-/dev/null}" 2>&1 || { + _cs_shield_hangup pveam download "$TEMPLATE_STORAGE" "$TEMPLATE" >>"${BUILD_LOG:-/dev/null}" 2>&1 || { msg_error "Failed to download template '$TEMPLATE' to storage '$TEMPLATE_STORAGE'" exit 222 } @@ -3446,7 +3446,7 @@ create_lxc_container() { local _dl_out="" _fresh="" for attempt in {1..3}; do msg_info "Attempt $attempt: Downloading template $TEMPLATE to $TEMPLATE_STORAGE" - _dl_out="$(pveam download "$TEMPLATE_STORAGE" "$TEMPLATE" 2>&1)" || true + _dl_out="$(_cs_shield_hangup pveam download "$TEMPLATE_STORAGE" "$TEMPLATE" 2>&1)" || true 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. @@ -3655,7 +3655,7 @@ create_lxc_container() { if [[ -n "$CUSTOM_TEMPLATE_PATH" ]]; then download_catalog_template "$LOCAL_TEMPLATE_PATH" else - pveam download local "$TEMPLATE" >>"${BUILD_LOG:-/dev/null}" 2>&1 + _cs_shield_hangup pveam download local "$TEMPLATE" >>"${BUILD_LOG:-/dev/null}" 2>&1 fi msg_ok "Template downloaded to local" else diff --git a/tests/console-hangup.sh b/tests/console-hangup.sh index ea447f1..39cb90e 100644 --- a/tests/console-hangup.sh +++ b/tests/console-hangup.sh @@ -6,7 +6,7 @@ set -uo pipefail CORE_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." && pwd)" -eval "$(awk '/^(silent|_silent_exec|_cs_reattach_output)[(][)] [{]/,/^}/' "$CORE_DIR/core/core.func")" +eval "$(awk '/^(silent|_cs_shield_hangup|_cs_reattach_output)[(][)] [{]/,/^}/' "$CORE_DIR/core/core.func")" eval "$(awk '/^(on_hangup|on_exit|_eh_quiet_spinner)[(][)] [{]/,/^}/' "$CORE_DIR/core/error_handler.func")" eval "$(awk '/^_pve_wait_for_detached_install[(][)] [{]/,/^}/' "$CORE_DIR/pve/backend.func")" @@ -53,7 +53,7 @@ rc=$? expect "the shell survives the hangup" 0 "$rc" expect "nothing more reaches the terminal" "" "$out" expect "later output lands in the log" 1 "$(grep -c 'after hangup' "$LOG")" -expect "the hangup is noted in the log" 1 "$(grep -c SIGHUP "$LOG")" +expect "the hangup is noted in the log" 1 "$(grep -c 'The terminal closed' "$LOG")" expect "the run knows it is detached" 1 "$(grep -c 'flag=1' "$LOG")" # on_exit(): a clean install leaves .done for the host; a failed one does not. @@ -137,5 +137,33 @@ expect "and not back to the original target" 0 "$(grep -c 'after the redirected expect "without a hangup nothing is redirected" 0 "$(grep -c 'no hangup' "$AFTER" 2>/dev/null)" expect "it stays where it was" 1 "$(grep -c 'no hangup' "$TEST_DIR/orig2.log" 2>/dev/null)" +# The telemetry service decides what counts as a user abort by searching the +# log text, so a word in one of our own messages is enough to file a failure +# as "the operator pressed Ctrl-C". A closed console is not that, and 81 of +# 300 aborted records were this message rather than a person. +CLASSIFIER_TOKENS="sigint ctrl+c ctrl-c sighup aborted by user user abort cancelled by user no changes have been made" +hangup_text="$(awk '/^on_hangup[(][)] [{]/,/^}/' "$CORE_DIR/core/error_handler.func") +$(awk '/^_incus_on_hangup[(][)] [{]/,/^}/' "$CORE_DIR/incus/build.func")" +hits=0 +while IFS= read -r token; do + [[ -n "$token" ]] || continue + if printf '%s' "${hangup_text,,}" | grep -qF "$token"; then + echo " token in a hangup message: $token" >&2 + hits=$((hits + 1)) + fi +done <<<"$(printf '%s +' $CLASSIFIER_TOKENS | sed 's/^ *//')" +expect "no hangup message reads as a user abort" 0 "$hits" +expect "both hangup handlers were found" 2 "$(grep -c 'continuing, output follows here' <<<"$hangup_text")" + +# The helper the long host commands go through has to survive the signal. +: >"$LOG" +_cs_shield_hangup sh -c 'kill -HUP $$; echo shielded >>"'"$LOG"'"' +expect "a shielded command survives SIGHUP" 0 "$?" +expect "and ran to the end" 1 "$(grep -c shielded "$LOG")" +unshielded_rc=0 +{ sh -c 'kill -HUP $$; echo unshielded' >/dev/null 2>&1 || unshielded_rc=$?; } 2>/dev/null +expect "without the shield the same command dies" 129 "$unshielded_rc" + echo "$passed passed, $failed failed" ((failed == 0)) diff --git a/tests/failed-command-output.sh b/tests/failed-command-output.sh index e454509..3e6c486 100644 --- a/tests/failed-command-output.sh +++ b/tests/failed-command-output.sh @@ -2,7 +2,7 @@ set -uo pipefail CORE_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." && pwd)" -eval "$(awk '/^(silent|set_std_mode|_silent_exec|_cs_reattach_output)[(][)] [{]/,/^}/' "$CORE_DIR/core/core.func")" +eval "$(awk '/^(silent|set_std_mode|_cs_shield_hangup|_cs_reattach_output)[(][)] [{]/,/^}/' "$CORE_DIR/core/core.func")" eval "$(awk '/^_eh_print_failed_output[(][)] [{]/,/^}/' "$CORE_DIR/core/error_handler.func")" TEST_DIR="$(mktemp -d)" diff --git a/ui/API.txt b/ui/API.txt index 32fea30..82899f4 100644 --- a/ui/API.txt +++ b/ui/API.txt @@ -15,6 +15,7 @@ _cs_run_os_section _cs_run_update _cs_runtime_cache_dir _cs_scripts_ref_line +_cs_shield_hangup _cs_stdin_source _dev_step_end _dev_step_key @@ -51,7 +52,6 @@ _pve_version _pveam_available _pveam_update _sanitize_value -_silent_exec _stop_container_if_installing _telemetry_report_exit _tm_clean_log