fix(remote): keep curl's stderr when the request fails (#850)

scripts/remote.sh

@@ -291,13 +291,39 @@ cmd_doctor() {
 # config file is 0600 and removed immediately after the call.
 _remote_http_post_json() {
   local url="$1" body_file="$2" out_file="$3" header_file="$4" cfg http_code \
-    fifo_dir header_fifo copier_pid curl_output curl_status=0
-  cfg="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-cfg.XXXXXX")"
-  fifo_dir="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-header-pipe.XXXXXX")"
-  header_fifo="$fifo_dir/header"
-  mkfifo "$header_fifo"
+    work_dir header_fifo copier_pid curl_output curl_status=0 curl_err
+  # ONE ALLOCATION BEFORE THE TRAP, AND EVERYTHING ELSE INSIDE IT.
+  #
+  # Two separate things go wrong with the obvious ordering, and this shape is
+  # the smallest one that closes both.
+  #
+  # First, a trap cannot expand what it cannot see. An EXIT trap set inside a
+  # function runs after that function's frame is gone, so a single-quoted body
+  # expands `$cfg` in the CALLER's scope, where no local of that name exists.
+  # It removes "" and returns 0, so the cleanup reads as working. Measured on
+  # bash 3.2.57 and 5.3.15: a local is EMPTY inside an EXIT trap fired by
+  # errexit from within the function. `printf %q` fixes the value at set time
+  # and survives a TMPDIR containing spaces.
+  #
+  # Second, anything created BEFORE the trap is armed is unprotected. Creating
+  # the config, then a directory, then an error file, and only then arming the
+  # trap leaves a window where the second or third allocation fails and the
+  # first is stranded -- including a 0600 config naming the request body. The
+  # window cannot be closed by ordering alone, because there is always a first
+  # allocation.
+  #
+  # So: acquire ONE directory, arm the trap on it immediately, and create
+  # everything else inside. A failure at any later step leaves nothing outside
+  # a directory that is already condemned. `rm -rf` on a path this function
+  # minted is the whole cleanup.
+  work_dir="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-curl.XXXXXX")"
+  trap "rm -rf $(printf '%q' "$work_dir")" EXIT INT TERM
+  cfg="$work_dir/config"
+  curl_err="$work_dir/stderr"
+  header_fifo="$work_dir/header"
+  : > "$cfg"
   chmod 600 "$cfg"
-  trap 'rm -f "$cfg" "$header_fifo"; rmdir "$fifo_dir" 2>/dev/null || true' EXIT INT TERM
+  mkfifo "$header_fifo"
   # Reaped on both normal paths below (waited on success, killed and waited on
   # failure), so this is short-lived by construction -- but the EXIT trap only
   # removes files, it does not kill the copier. A signal arriving before curl
@@ -317,11 +343,21 @@ _remote_http_post_json() {
     printf 'max-filesize = "2097152"\n'
     printf 'data = "@%s"\n' "$body_file"
   } > "$cfg"
-  if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>/dev/null); then
+  # Do not discard curl's stderr. On failure it is the only record of WHY, and
+  # the caller only ever sees the HTTP code — which this function reports as
+  # "000" for every kind of failure alike. A Windows run spent a long time on a
+  # bare 000 whose cause (curl could not open a path embedded in the config)
+  # was sitting in the stderr this line was throwing away.
+  #
+  # Captured rather than passed through, and shown only when curl actually
+  # failed: on the success path curl -sS is already silent, and a stray write
+  # to stderr here would land in the middle of a caller's output.
+  if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>"$curl_err"); then
     :
   else
     curl_status=$?
   fi
+  [ "$curl_status" -ne 0 ] && [ -s "$curl_err" ] && cat "$curl_err" >&2
   if [ "$curl_status" -ne 0 ]; then
     kill "$copier_pid" 2>/dev/null || true
     wait "$copier_pid" 2>/dev/null || true
@@ -331,8 +367,9 @@ _remote_http_post_json() {
   else
     http_code="000"
   fi
-  rm -f "$cfg" "$header_fifo"
-  rmdir "$fifo_dir" 2>/dev/null || true
+  # One directory holds the config, the error file and the fifo, so the normal
+  # path removes exactly what the trap would have.
+  rm -rf "$work_dir"
   trap - EXIT INT TERM
   printf '%s' "$http_code"
 }

tests/test_remote_curl_stderr.bats

@@ -0,0 +1,255 @@
+#!/usr/bin/env bats
+# WHEN THE ONLY THING A CALLER SEES IS "000", THROWING AWAY curl's STDERR IS
+# THROWING AWAY THE DIAGNOSIS (#850).
+#
+# `_remote_http_post_json` reports `000` for every kind of failure alike: a
+# refused connection, a timeout, a path curl could not open. The reason existed
+# each time -- curl wrote it to stderr -- and `2>/dev/null` discarded it. A
+# Windows run spent an afternoon on a bare `000` whose cause was in that stream.
+#
+# WHAT HAS TO HOLD, and each is its own case here:
+#
+#   on failure   the diagnosis reaches the caller's stderr
+#   on success   nothing does, even if curl wrote something -- the show is
+#                gated on curl having FAILED, not on the stream being empty
+#   either way   the http code is exactly what it was before
+#   either way   no scratch file is left behind
+#
+# The stderr of the helper is captured to a FILE rather than read from bats's
+# `$output`, which merges the two streams: a test that cannot tell stdout from
+# stderr cannot check that a message went to the right one, and "the message
+# appears somewhere" is what this fix is not about.
+
+load test_helper
+
+SANDBOX_TOOLS=(bash dirname mktemp mkfifo chmod rm rmdir sed cp cat grep python3 uname)
+
+setup() {
+  setup_test_env
+
+  STUB_SRC="$BATS_TEST_TMPDIR/stubs"
+  mkdir -p "$STUB_SRC"
+
+  # A curl whose behaviour the test dictates: STUB_CURL_MODE says whether it
+  # succeeds, and STUB_CURL_STDERR is written to stderr either way. Writing on
+  # the success path too is the point of one of the cases below -- it is how
+  # "shown only when curl failed" is told apart from "the stream was empty".
+  cat > "$STUB_SRC/curl" <<'STUB'
+#!/usr/bin/env bash
+set -u
+cfg=""; out=""; prev=""
+for arg in "$@"; do
+  case "$prev" in
+    -K) cfg="$arg" ;;
+    -o) out="$arg" ;;
+  esac
+  prev="$arg"
+done
+[ -z "${STUB_CURL_STDERR:-}" ] || printf '%s\n' "$STUB_CURL_STDERR" >&2
+hdr="$(sed -n 's/^dump-header = "\(.*\)"$/\1/p' "$cfg")"
+if [ "${STUB_CURL_MODE:-ok}" = "fail" ]; then
+  # A failure AFTER the headers were written -- curl exceeding max-filesize on
+  # the body, say. The headers matter here: leaving the fifo without a writer
+  # strands the bounded copier on open(), and everything downstream of that
+  # waits on a process that will never finish. That is a real property of the
+  # failure path, and driving it is a different experiment from this one.
+  [ -z "$hdr" ] || printf 'HTTP/1.1 200 OK\r\n\r\n' > "$hdr"
+  exit 63
+fi
+[ -z "$hdr" ] || printf 'HTTP/1.1 200 OK\r\n\r\n' > "$hdr"
+[ -z "$out" ] || printf '{"ok":true}' > "$out"
+printf '200'
+STUB
+  chmod +x "$STUB_SRC/curl"
+}
+
+teardown() { teardown_test_env; }
+
+sandbox_path() {
+  local dir tool src
+  dir="$(mktemp -d "$BATS_TEST_TMPDIR/sandbox.XXXXXX")"
+  for tool in "${SANDBOX_TOOLS[@]}"; do
+    src="$(command -v "$tool")" || { echo "host lacks $tool" >&2; return 1; }
+    ln -s "$src" "$dir/$tool"
+  done
+  ln -s "$STUB_SRC/curl" "$dir/curl"
+  printf '%s' "$dir"
+}
+
+# Runs the helper with its own TMPDIR, so "what scratch files remain" is a
+# question about this call and not about everything else on the machine.
+# stdout (the http code) lands in $output; stderr lands in $ERR_FILE.
+post_with_curl() {
+  local mode="$1" stderr_text="$2"
+  RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")"
+  ERR_FILE="$BATS_TEST_TMPDIR/helper-stderr"
+  local bin; bin="$(sandbox_path)"
+  local body="$RUN_TMPDIR/body.json"
+  printf '{"t":"secret"}' > "$body"
+
+  run env PATH="$bin" TMPDIR="$RUN_TMPDIR" STUB_CURL_MODE="$mode" \
+    STUB_CURL_STDERR="$stderr_text" bash -c '
+    set -uo pipefail
+    . '"$SCRIPTS"'/remote.sh 2>/dev/null
+    _remote_http_post_json "https://example.invalid/v1/x" "'"$body"'" \
+      "'"$RUN_TMPDIR"'/out-body" "'"$RUN_TMPDIR"'/out-header" 2>"'"$ERR_FILE"'"
+  '
+}
+
+@test "a failing curl's diagnosis reaches the caller's stderr (#850)" {
+  # The whole point. Without this the operator has "000" and nothing else, and
+  # the reason they need is written down and then deleted.
+  post_with_curl fail "curl: (26) Failed to open/read local data from file"
+  [ "$status" -eq 0 ]
+  [ "$output" = "000" ]
+
+  grep -q 'Failed to open/read local data' "$ERR_FILE"
+}
+
+@test "a successful curl's stderr is NOT shown, even when it wrote something (#850)" {
+  # Distinguishes "shown only when curl failed" from "the stream happened to be
+  # empty". curl -sS is quiet on success, so a test that let it stay quiet here
+  # would pass against a version that dumped stderr unconditionally -- and that
+  # version would drop noise into the middle of a caller's output.
+  post_with_curl ok "a progress line nobody asked for"
+  [ "$status" -eq 0 ]
+  [ "$output" = "200" ]
+
+  [ ! -s "$ERR_FILE" ]
+}
+
+@test "the http code is unchanged on both paths (#850)" {
+  # The contract this must not have altered while adding the diagnosis.
+  post_with_curl ok ""
+  [ "$output" = "200" ]
+
+  post_with_curl fail "curl: (7) Failed to connect"
+  [ "$output" = "000" ]
+}
+
+@test "no scratch file is left behind, on either path (#850)" {
+  # The config, the error file and the fifo now live in one directory the helper
+  # mints, so this is one glob rather than three.
+  #
+  # THE NAME MATTERS AND ALMOST GOT THIS WRONG. An earlier version of this test
+  # globbed agmsg-curl-cfg.* and agmsg-curl-err.*, which the new layout never
+  # creates -- the check would have passed on any behaviour whatsoever, and gone
+  # on passing if the directory leaked. An absence assertion aimed at a name
+  # nothing uses is indistinguishable from a clean run.
+  post_with_curl ok ""
+  [ "$output" = "200" ]
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+
+  post_with_curl fail "curl: (7) Failed to connect"
+  [ "$output" = "000" ]
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+}
+
+@test "an early exit between the mktemp and the cleanup still sweeps the file (#850)" {
+  # The hole the explicit cleanup cannot cover: it only runs if the function
+  # GETS there. A signal is the obvious way out early and it is also the one I
+  # could not drive -- the probe hung, because the bounded copier keeps the run
+  # alive while curl is being waited on. A review pointed out that the signal is
+  # not the only exit, and errexit is a bounded one.
+  #
+  # `cat` is the last command of the `&& &&` chain that shows the diagnosis, so
+  # under `set -e` a failing cat leaves the function immediately -- after the
+  # mktemp, before the rm. Everything that survives that has to come from the
+  # trap, which is exactly the property under test.
+  RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")"
+  local bin; bin="$(sandbox_path)"
+  local body="$RUN_TMPDIR/body.json"
+  printf '{"t":"secret"}' > "$body"
+
+  # A `cat` that always fails, shadowing the real one for this run only. The
+  # symlink is REMOVED first: `>` through a symlink writes to its target, which
+  # here is the system's own /bin/cat. The first attempt did exactly that and
+  # was refused by the OS -- on a machine where it was not refused, this test
+  # would have replaced a system binary.
+  rm -f "$bin/cat"
+  printf '#!/usr/bin/env bash\nexit 1\n' > "$bin/cat"
+  chmod +x "$bin/cat"
+
+  # Output goes to FILES, not to bats's capture pipe. On this path the bounded
+  # copier is never reaped -- the trap removes files and does not kill it -- so
+  # it outlives the shell still holding the inherited stdout and stderr. Those
+  # being a pipe is what makes `run` wait forever; those being files is what
+  # makes this test finish. The orphan is a real property of the early-exit
+  # path and is reported alongside this test rather than papered over.
+  run env PATH="$bin" TMPDIR="$RUN_TMPDIR" STUB_CURL_MODE=fail \
+    STUB_CURL_STDERR="curl: (26) Failed to open/read local data" bash -c '
+    set -euo pipefail
+    . '"$SCRIPTS"'/remote.sh 2>/dev/null
+    _remote_http_post_json "https://example.invalid/v1/x" "'"$body"'" \
+      "'"$RUN_TMPDIR"'/out-body" "'"$RUN_TMPDIR"'/out-header"
+  ' >"$RUN_TMPDIR/driver-stdout" 2>"$RUN_TMPDIR/driver-stderr"
+  # It leaves early: the function never reaches its `printf` of the code.
+  [ "$status" -ne 0 ]
+  [ ! -s "$RUN_TMPDIR/driver-stdout" ]
+
+  # And nothing is stranded. This is the trap's work, not the tail's -- and all
+  # three are asserted, because measuring this path is what showed the trap had
+  # never swept ANY of them. It expanded function locals after the frame was
+  # gone, removed empty strings, and returned 0.
+  refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null
+  refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+}
+
+@test "a failure while setting up leaves nothing behind either (#850)" {
+  # The window the previous shape could not close: anything created BEFORE the
+  # trap is armed is unprotected, and there is always a first allocation. The
+  # answer is that there is now only ONE allocation before the trap, and
+  # everything else is made inside it.
+  #
+  # Driven by making `mkfifo` fail, which happens after the directory exists and
+  # after the config has been written into it. Under `set -e` that leaves the
+  # function immediately -- before curl, before any cleanup the tail would do.
+  RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")"
+  local bin; bin="$(sandbox_path)"
+  local body="$RUN_TMPDIR/body.json"
+  printf '{"t":"secret"}' > "$body"
+
+  rm -f "$bin/mkfifo"
+  printf '#!/usr/bin/env bash\nexit 1\n' > "$bin/mkfifo"
+  chmod +x "$bin/mkfifo"
+
+  run env PATH="$bin" TMPDIR="$RUN_TMPDIR" bash -c '
+    set -euo pipefail
+    . '"$SCRIPTS"'/remote.sh 2>/dev/null
+    _remote_http_post_json "https://example.invalid/v1/x" "'"$body"'" \
+      "'"$RUN_TMPDIR"'/out-body" "'"$RUN_TMPDIR"'/out-header"
+  '
+  [ "$status" -ne 0 ]
+
+  # Nothing survives: not the directory, and so not the config inside it. The
+  # config is the file that matters -- it is what this helper exists to keep
+  # out of curl's argv, and a stranded copy names the request body.
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+}
+
+@test "an EXIT trap cannot read the locals of the function that set it (#850)" {
+  # The premise the trap's shape rests on, measured here rather than asserted
+  # in a comment. If a future bash made locals visible to an EXIT trap, the
+  # `printf %q` baking would look like pointless ceremony and someone would
+  # simplify it back into a single-quoted body -- reopening the leak. This test
+  # is what tells them the ceremony is load-bearing.
+  run bash -c '
+    f() { local v="hello"; trap '"'"'printf "TRAP_SEES=[%s]\n" "${v:-EMPTY}"'"'"' EXIT; false; }
+    set -e
+    f
+  '
+  [ "$output" = "TRAP_SEES=[EMPTY]" ]
+}
+
+@test "the leftover check can see a leftover when there is one (#850)" {
+  # Control on the assertion above, which is an absence: a glob that matches
+  # nothing looks exactly like a glob pointed at the wrong directory. Plant one
+  # and confirm the same check fires.
+  # Planted under the name the helper really uses, so this controls the glob
+  # that the absence assertions actually run.
+  post_with_curl ok ""
+  mkdir -p "$RUN_TMPDIR/agmsg-curl.planted"
+  run ls -d "$RUN_TMPDIR"/agmsg-curl.*
+  [ "$status" -eq 0 ]
+}