diff --git a/docs/TESTING.md b/docs/TESTING.md index 3f57914..ef45351 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -60,7 +60,8 @@ pio device list The runner flashes the selected board, captures normal-boot serial output with the repository Bash helper, requires Unity's `Tests 0 Failures` and `OK` summary, and prints lifecycle metrics. Hardware work shares a lock with the -portal contract, so two invocations cannot flash or use the same board at once. +portal contract and DeviceFramework's hardware runners on the same host, so +two first-party invocations cannot flash or use the same board at once. ## Docker portal contract @@ -71,6 +72,11 @@ Wi-Fi adapter, then runs its HTTP and browser contract in a pinned Playwright Docker image. Docker uses host networking only to reach the already-routed portal; it never runs NetworkManager or changes host adapters. +The runner resolves PlatformIO from `WIFIMANAGER_PIO_EXECUTABLE`, then `PATH`, +then PlatformIO's standard `~/.platformio/penv/bin/pio` installation. That +makes the same command work from a non-interactive SSH shell without modifying +the user's `PATH`. + ```bash ./tools/portal-hardware doctor --client-interface wlx74da385d4165 ./tools/portal-hardware run \ @@ -131,9 +137,13 @@ the fixture to a real LAN. Copy the ignored template below, add local credentials, and pass it explicitly; it is mounted read-only into the test container and is never logged by the runner. -A retained session is deliberately never overwritten. If a previous `up` or an -interrupted `run` left one behind, run `./tools/portal-hardware down` first; -that removes only the named temporary connection recorded by the tool. +A retained session is deliberately never overwritten. Before touching +NetworkManager, the runner atomically records its uniquely generated connection +name; after creation it atomically replaces that pending record with the exact +UUID. Ordinary failures and interrupts remove that connection immediately, and +`down` accepts either record after an uncatchable host termination or an +intentional `up`/`--keep` session. It never removes another NetworkManager +connection. ```bash cp test/portal-station.env.example test/portal-station.env diff --git a/tools/lib/portal-hardware-session.sh b/tools/lib/portal-hardware-session.sh index 030d0e7..940756d 100755 --- a/tools/lib/portal-hardware-session.sh +++ b/tools/lib/portal-hardware-session.sh @@ -18,7 +18,11 @@ wm_default_route_interface() { } wm_acquire_hardware_lock() { - local lock_file="${WM_HARDWARE_LOCK_FILE:-/tmp/wifimanager-hardware.lock}" + # First-party runners deliberately share this lock: a WiFiManager portal + # test and a DeviceFramework hardware test can otherwise serial-flash the + # same selected board concurrently. Keep the WiFiManager override for + # isolated tests and let callers redirect the common lock with TMPDIR. + local lock_file="${WM_HARDWARE_LOCK_FILE:-${TMPDIR:-/tmp}/deviceframework-hardware-test.lock}" # `ota` deliberately acquires this before it builds firmware, then calls # the shared portal-start helper which also acquires it. Keep that nested # path idempotent so the lock covers the whole A/B contract rather than @@ -81,31 +85,59 @@ wm_remove_connection_by_name() { } wm_create_portal_connection() { - local interface="$1" ssid="$2" password="$3" name uuid + local interface="$1" ssid="$2" password="$3" platform="${4:-}" name uuid name="wifimanager-portal-${RANDOM}-$(date +%s)" + # Publish the owned name before the first NetworkManager mutation. The + # caller's signal trap can then remove it throughout the pending-to-active + # state transition. + WM_PORTAL_CONNECTION_NAME="$name" + WM_PORTAL_CONNECTION_UUID="" + WM_PORTAL_CONNECTION_OWNED=yes + export WM_PORTAL_CONNECTION_UUID WM_PORTAL_CONNECTION_NAME WM_PORTAL_CONNECTION_OWNED + # A pending record is atomically committed before the first NetworkManager + # mutation. If the host dies after that point, `down` can remove this + # exact name even before NetworkManager's UUID has been obtained. + if [[ -n "$platform" ]] && ! wm_write_state "$interface" "$platform" "" "$name"; then + WM_PORTAL_CONNECTION_OWNED=no + unset WM_PORTAL_CONNECTION_UUID WM_PORTAL_CONNECTION_NAME + return 1 + fi nmcli device disconnect "$interface" >/dev/null 2>&1 || true - wm_wait_for_portal_ssid "$interface" "$ssid" + if ! wm_wait_for_portal_ssid "$interface" "$ssid"; then + wm_cleanup_created_connection + return 1 + fi if ! nmcli connection add type wifi ifname "$interface" con-name "$name" ssid "$ssid" \ ipv4.method auto ipv4.never-default yes ipv6.method ignore connection.autoconnect no >/dev/null; then + wm_cleanup_created_connection return 1 fi - if ! nmcli connection modify "$name" wifi-sec.key-mgmt wpa-psk wifi-sec.psk "$password"; then - wm_remove_connection_by_name "$name" - return 1 - fi - if ! nmcli connection up "$name" ifname "$interface"; then - wm_remove_connection_by_name "$name" - return 1 - fi + # NetworkManager creates the UUID at `connection add`, before any later + # configuration or association step. Capture it immediately so cleanup + # has an exact identifier throughout the remaining critical section. uuid="$(nmcli -g connection.uuid connection show "$name")" if [[ -z "$uuid" || "$uuid" == "--" ]]; then - wm_remove_connection_by_name "$name" + wm_cleanup_created_connection echo "NetworkManager did not return a UUID for the portal connection." >&2 return 1 fi WM_PORTAL_CONNECTION_UUID="$uuid" - WM_PORTAL_CONNECTION_NAME="$name" - export WM_PORTAL_CONNECTION_UUID WM_PORTAL_CONNECTION_NAME + export WM_PORTAL_CONNECTION_UUID + # Atomically replace the pending record with the exact UUID before + # association. A normal signal trap removes it immediately; the durable + # record also makes `down` useful after an uncatchable host termination. + if [[ -n "$platform" ]] && ! wm_write_state "$interface" "$platform" "$uuid" "$name"; then + wm_cleanup_created_connection + return 1 + fi + if ! nmcli connection modify "$name" wifi-sec.key-mgmt wpa-psk wifi-sec.psk "$password"; then + wm_cleanup_created_connection + return 1 + fi + if ! nmcli connection up "$name" ifname "$interface"; then + wm_cleanup_created_connection + return 1 + fi } wm_verify_portal_route() { @@ -118,10 +150,24 @@ wm_verify_portal_route() { } wm_remove_connection() { - local uuid="$1" - [[ -n "$uuid" ]] || return 0 - nmcli connection down uuid "$uuid" >/dev/null 2>&1 || true - nmcli connection delete uuid "$uuid" >/dev/null 2>&1 || true + local uuid="${1:-}" name="${2:-}" + if [[ -n "$uuid" ]]; then + nmcli connection down uuid "$uuid" >/dev/null 2>&1 || true + nmcli connection delete uuid "$uuid" >/dev/null 2>&1 || true + elif [[ -n "$name" ]]; then + wm_remove_connection_by_name "$name" + fi +} + +wm_cleanup_created_connection() { + # Only remove the connection this process named. This is intentionally + # separate from `finish_portal_session`, which reads a retained state file + # for an explicit later `down` command. + [[ "${WM_PORTAL_CONNECTION_OWNED:-no}" == "yes" ]] || return 0 + wm_remove_connection "${WM_PORTAL_CONNECTION_UUID:-}" "${WM_PORTAL_CONNECTION_NAME:-}" + wm_clear_state + WM_PORTAL_CONNECTION_OWNED=no + unset WM_PORTAL_CONNECTION_UUID WM_PORTAL_CONNECTION_NAME } wm_state_file() { @@ -138,16 +184,25 @@ wm_require_no_active_session() { } wm_write_state() { - local interface="$1" platform="$2" uuid="$3" name="$4" root file + local interface="$1" platform="$2" uuid="$3" name="$4" root file temporary_file state root="$(wm_portal_state_root)" file="$(wm_state_file)" install -d -m 700 "$root" - ( - umask 077 - printf 'WM_PORTAL_INTERFACE=%s\nWM_PORTAL_PLATFORM=%s\nWM_PORTAL_CONNECTION_UUID=%s\nWM_PORTAL_CONNECTION_NAME=%s\n' \ - "$interface" "$platform" "$uuid" "$name" >"$file" - ) - chmod 600 "$file" + if [[ -n "$uuid" ]]; then + state="active" + else + state="pending" + fi + temporary_file="$(mktemp "$root/.session.env.XXXXXX")" || return 1 + if ! { + printf 'WM_PORTAL_STATE=%s\nWM_PORTAL_INTERFACE=%s\nWM_PORTAL_PLATFORM=%s\nWM_PORTAL_CONNECTION_UUID=%s\nWM_PORTAL_CONNECTION_NAME=%s\n' \ + "$state" "$interface" "$platform" "$uuid" "$name" + } >"$temporary_file"; then + rm -f "$temporary_file" + return 1 + fi + chmod 600 "$temporary_file" + mv -f "$temporary_file" "$file" } wm_load_state() { @@ -161,9 +216,10 @@ wm_load_state() { WM_PORTAL_PLATFORM="" WM_PORTAL_CONNECTION_UUID="" WM_PORTAL_CONNECTION_NAME="" + WM_PORTAL_STATE="" while IFS='=' read -r key value; do case "$key" in - WM_PORTAL_INTERFACE|WM_PORTAL_PLATFORM|WM_PORTAL_CONNECTION_UUID|WM_PORTAL_CONNECTION_NAME) + WM_PORTAL_STATE|WM_PORTAL_INTERFACE|WM_PORTAL_PLATFORM|WM_PORTAL_CONNECTION_UUID|WM_PORTAL_CONNECTION_NAME) printf -v "$key" '%s' "$value" ;; '') ;; @@ -173,11 +229,34 @@ wm_load_state() { ;; esac done <"$file" - [[ -n "$WM_PORTAL_INTERFACE" && -n "$WM_PORTAL_PLATFORM" && -n "$WM_PORTAL_CONNECTION_UUID" && -n "$WM_PORTAL_CONNECTION_NAME" ]] || { + [[ -n "$WM_PORTAL_INTERFACE" && -n "$WM_PORTAL_PLATFORM" && -n "$WM_PORTAL_CONNECTION_NAME" ]] || { echo "Incomplete WiFiManager portal session state." >&2 return 1 } - export WM_PORTAL_INTERFACE WM_PORTAL_PLATFORM WM_PORTAL_CONNECTION_UUID WM_PORTAL_CONNECTION_NAME + # State files written before this recovery format carried an exact UUID + # but no phase. Continue to accept those retained sessions as active. + if [[ -z "$WM_PORTAL_STATE" && -n "$WM_PORTAL_CONNECTION_UUID" ]]; then + WM_PORTAL_STATE="active" + fi + case "$WM_PORTAL_STATE" in + pending) + [[ -z "$WM_PORTAL_CONNECTION_UUID" ]] || { + echo "Invalid WiFiManager pending portal session state." >&2 + return 1 + } + ;; + active) + [[ -n "$WM_PORTAL_CONNECTION_UUID" ]] || { + echo "Incomplete WiFiManager active portal session state." >&2 + return 1 + } + ;; + *) + echo "Invalid WiFiManager portal session state." >&2 + return 1 + ;; + esac + export WM_PORTAL_STATE WM_PORTAL_INTERFACE WM_PORTAL_PLATFORM WM_PORTAL_CONNECTION_UUID WM_PORTAL_CONNECTION_NAME } wm_clear_state() { diff --git a/tools/portal-hardware b/tools/portal-hardware index 62b45d9..c4573f6 100755 --- a/tools/portal-hardware +++ b/tools/portal-hardware @@ -52,6 +52,12 @@ ota_environment_b="" ota_firmware_a="" ota_firmware_b="" ota_browser_prebuilt="no" +# `wm_create_portal_connection` sets these before NetworkManager mutates the +# secondary adapter. Keeping the ownership record in-process means the +# signal trap can clean up the exact pending or active connection immediately. +WM_PORTAL_CONNECTION_UUID="" +WM_PORTAL_CONNECTION_NAME="" +WM_PORTAL_CONNECTION_OWNED="no" while [[ $# -gt 0 ]]; do case "$1" in --platform) [[ $# -ge 2 ]] || usage; platform="$2"; shift 2 ;; @@ -71,12 +77,43 @@ done require_common() { wm_require ip wm_require nmcli - wm_require pio + wm_pio_available wm_require docker wm_require curl docker compose version >/dev/null } +wm_pio_executable() { + # Hardware runners are commonly launched over non-interactive SSH, where + # a normal PlatformIO installation is not necessarily on PATH. Honor an + # explicit override first, then PATH, then PlatformIO's standard venv. + # This mirrors DeviceFramework's resolver without changing a user's PATH. + local executable + if [[ -n "${WIFIMANAGER_PIO_EXECUTABLE:-}" ]]; then + executable="$WIFIMANAGER_PIO_EXECUTABLE" + else + executable="$(command -v pio 2>/dev/null || true)" + if [[ -z "$executable" && -x "$HOME/.platformio/penv/bin/pio" ]]; then + executable="$HOME/.platformio/penv/bin/pio" + fi + fi + [[ -n "$executable" && -x "$executable" ]] || { + echo "PlatformIO is required; install it or set WIFIMANAGER_PIO_EXECUTABLE." >&2 + return 1 + } + printf '%s\n' "$executable" +} + +wm_pio_available() { + wm_pio_executable >/dev/null +} + +wm_pio() { + local executable + executable="$(wm_pio_executable)" || return + "$executable" "$@" +} + pio_for_portal_environment() { local environment="$1" shift @@ -89,7 +126,7 @@ pio_for_portal_environment() { esp32_core_3_3_11|esp32_ota_*) ;; *) - pio "$@" + wm_pio "$@" return ;; esac @@ -100,7 +137,7 @@ pio_for_portal_environment() { cache_dir="${WIFIMANAGER_PLATFORMIO_CACHE_DIR:-$core_dir/cache}" install -d -m 700 "$core_dir" "$packages_dir" "$cache_dir" PLATFORMIO_CORE_DIR="$core_dir" PLATFORMIO_PACKAGES_DIR="$packages_dir" \ - PLATFORMIO_CACHE_DIR="$cache_dir" pio "$@" + PLATFORMIO_CACHE_DIR="$cache_dir" wm_pio "$@" } prepare_output_dir() { @@ -220,19 +257,15 @@ start_portal_session() { if [[ -n "$expected_a_artifact" ]]; then ota_assert_a_artifact_matches_build "$expected_a_artifact" fi - if ! wm_create_portal_connection "$client_interface" "$ssid" "default1"; then + if ! wm_create_portal_connection "$client_interface" "$ssid" "default1" "$platform"; then return 1 fi if ! wm_verify_portal_route "$client_interface"; then - wm_remove_connection "$WM_PORTAL_CONNECTION_UUID" + wm_cleanup_created_connection return 1 fi if ! wait_for_portal_ready; then - wm_remove_connection "$WM_PORTAL_CONNECTION_UUID" - return 1 - fi - if ! wm_write_state "$client_interface" "$platform" "$WM_PORTAL_CONNECTION_UUID" "$WM_PORTAL_CONNECTION_NAME"; then - wm_remove_connection "$WM_PORTAL_CONNECTION_UUID" + wm_cleanup_created_connection return 1 fi printf 'Portal connected on %s. Artifacts: %s\n' "$client_interface" "$output_dir" @@ -457,10 +490,27 @@ run_ota_contract() { finish_portal_session() { wm_load_state - wm_remove_connection "$WM_PORTAL_CONNECTION_UUID" + wm_remove_connection "$WM_PORTAL_CONNECTION_UUID" "$WM_PORTAL_CONNECTION_NAME" wm_clear_state } +cleanup_portal_session() { + local status="${1:-$?}" + # A signal must never fall through into the rest of the hardware run, and + # a recursive EXIT trap must not obscure the original status. + trap - EXIT HUP INT TERM + if [[ "${WM_PORTAL_CONNECTION_OWNED:-no}" == "yes" ]] && \ + { (( status != 0 )) || { [[ "$command_name" != "up" && "$keep" != "yes" ]]; }; }; then + wm_cleanup_created_connection || true + fi + exit "$status" +} + +trap 'cleanup_portal_session "$?"' EXIT +trap 'cleanup_portal_session 129' HUP +trap 'cleanup_portal_session 130' INT +trap 'cleanup_portal_session 143' TERM + case "$command_name" in doctor) [[ -n "$client_interface" ]] || usage @@ -480,12 +530,6 @@ case "$command_name" in ;; run) start_portal_session - cleanup() { - if [[ "$keep" != "yes" ]]; then - finish_portal_session || true - fi - } - trap cleanup EXIT INT TERM export PORTAL_ARTIFACT_DIR="$output_dir" export LOCAL_UID="$(id -u)" export LOCAL_GID="$(id -g)" @@ -535,12 +579,6 @@ case "$command_name" in prepare_ota_firmware prepare_ota_contract_image start_portal_session "$ota_environment_a" yes "$ota_firmware_a" - cleanup() { - if [[ "$keep" != "yes" ]]; then - finish_portal_session || true - fi - } - trap cleanup EXIT INT TERM # A must be visible through the real portal before a B upload can be # meaningful. On ESP8266 the marker also reports the actual active diff --git a/tools/tests/test-portal-hardware-cli.sh b/tools/tests/test-portal-hardware-cli.sh index 903da61..3732e61 100755 --- a/tools/tests/test-portal-hardware-cli.sh +++ b/tools/tests/test-portal-hardware-cli.sh @@ -18,6 +18,8 @@ printf '%s\n' '#!/usr/bin/env bash' \ 'echo "192.168.4.1 dev wlan-client src 192.168.4.2"' >"$stub_bin/ip" printf '%s\n' '#!/usr/bin/env bash' \ 'printf "%s\\n" "$*" >>"$CALL_LOG"' \ +'if [[ "${NMCLI_FAIL_ADD:-}" == "yes" && "$1" == "connection" && "$2" == "add" ]]; then exit 7; fi' \ +'if [[ "${NMCLI_SIGNAL_PARENT:-}" == "yes" && "$1" == "connection" && "$2" == "add" ]]; then kill -TERM "$PPID"; exit 0; fi' \ 'if [[ "${NMCLI_FAIL_UP:-}" == "yes" && "$1" == "connection" && "$2" == "up" ]]; then exit 7; fi' \ 'if [[ "$1" == "-t" && "$2" == "-f" && "$3" == "SSID" ]]; then echo "WM Contract ESP8266"; exit 0; fi' \ 'if [[ "$1" == "-g" && "$2" == "connection.uuid" ]]; then echo "stub-uuid"; exit 0; fi' \ @@ -80,12 +82,70 @@ fi source "$root/tools/lib/portal-hardware-session.sh" wm_wait_for_portal_ssid() { return 0; } export NMCLI_FAIL_UP=yes -if wm_create_portal_connection wlan-client 'fixture portal' placeholder; then +if wm_create_portal_connection wlan-client 'fixture portal' placeholder esp8266; then echo 'failed association was reported as success' >&2 exit 1 fi unset NMCLI_FAIL_UP +grep -Eq 'connection delete (uuid stub-uuid|wifimanager-portal-)' "$CALL_LOG" +[[ ! -e "$(wm_state_file)" ]] || { + echo 'failed association left portal recovery state behind' >&2 + exit 1 +} + +# The pending record must be removed even when NetworkManager rejects the +# connection before it has a UUID. +export NMCLI_FAIL_ADD=yes +if wm_create_portal_connection wlan-client 'fixture portal' placeholder esp8266; then + echo 'failed connection creation was reported as success' >&2 + exit 1 +fi +unset NMCLI_FAIL_ADD grep -Eq 'connection delete wifimanager-portal-' "$CALL_LOG" +[[ ! -e "$(wm_state_file)" ]] || { + echo 'failed connection creation left pending recovery state behind' >&2 + exit 1 +} + +# A portal scan failure must stop before `connection add` and clear the +# pre-add pending record, even though this helper is invoked in an `if`. +wm_wait_for_portal_ssid() { return 1; } +if wm_create_portal_connection wlan-client 'fixture portal' placeholder esp8266; then + echo 'failed portal scan was reported as success' >&2 + exit 1 +fi +wm_wait_for_portal_ssid() { return 0; } +[[ ! -e "$(wm_state_file)" ]] || { + echo 'failed portal scan left pending recovery state behind' >&2 + exit 1 +} + +# Exercise the real runner trap: a TERM immediately after `connection add` +# must remove its exact pending name and clear the atomically written state. +: >"$CALL_LOG" +export NMCLI_SIGNAL_PARENT=yes +if "$root/tools/portal-hardware" up --platform esp8266 --port /dev/null \ + --client-interface wlan-client >/dev/null 2>&1; then + echo 'runner survived a connection-creation interrupt' >&2 + exit 1 +fi +unset NMCLI_SIGNAL_PARENT +grep -Eq 'connection delete wifimanager-portal-' "$CALL_LOG" +[[ ! -e "$(wm_state_file)" ]] || { + echo 'runner interrupt left a portal state record behind' >&2 + exit 1 +} + +# Model an uncatchable host death after the pre-add atomic write. `down` must +# accept its name-only pending record and remove that one connection. +wm_write_state wlan-client esp8266 '' wifimanager-pending-recovery +grep -Fxq 'WM_PORTAL_STATE=pending' "$(wm_state_file)" +"$root/tools/portal-hardware" down >/dev/null +grep -Fq 'connection delete wifimanager-pending-recovery' "$CALL_LOG" +[[ ! -e "$(wm_state_file)" ]] || { + echo 'pending portal recovery state was not cleared' >&2 + exit 1 +} # A retained session must be removed explicitly, never silently overwritten. wm_write_state wlan-client esp8266 stale-uuid stale-name @@ -120,6 +180,9 @@ grep -Fq 'compose.ota.yaml' "$root/tools/portal-hardware" grep -Fq 'wait_for_ota_marker B' "$root/tools/portal-hardware" grep -Fq 'pio_for_portal_environment "$ota_environment_a"' "$root/tools/portal-hardware" grep -Fq 'WIFIMANAGER_PLATFORMIO_CORE_DIR' "$root/tools/portal-hardware" +grep -Fq 'WIFIMANAGER_PIO_EXECUTABLE' "$root/tools/portal-hardware" +grep -Fq 'deviceframework-hardware-test.lock' "$root/tools/lib/portal-hardware-session.sh" +grep -Fq 'WM_PORTAL_CONNECTION_OWNED' "$root/tools/lib/portal-hardware-session.sh" grep -Fq 'assert_ota_fixture_pair' "$root/scripts/test.sh" grep -Fq 'WiFiManager Unity compile check passed' "$root/scripts/test.sh" grep -Fq 'eagle.flash.4m1m.ld' "$root/test/portal-harness/platformio.ini"