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

[fujibee]

Part 3 of 3 splitting the Windows connect fix. All three together are on win-connect-fix-850, which is the branch to install for a real walk.

This is the one that made the other two findable. Landing it separately is the point: without it the next failure in this family is another opaque 000.

What is wrong

_remote_http_post_json sent curl's stderr to /dev/null and reported 000 for every kind of failure alike. The caller sees only that code, so when a request failed there was nothing anywhere saying why.

That cost the Windows run a long time: connect returned a bare 000, and the reason — curl could not open a path embedded in its own config file — was in the stderr this line was discarding.

Captured to a file rather than passed straight through, and shown only when curl actually failed: on the success path curl -sS is already silent, and an unconditional pass-through would put curl's output in the middle of a caller's.

Scope, stated because it is narrower than the reasoning

This fixes the POST helper only, which is what the verified Windows commit changed.

_remote_http_get_json still carries 2>/dev/null, and the same argument applies to it — pull goes through the GET path, so a failure there is still an opaque 000 with the reason discarded. It embeds no paths in its config, so PR-A's defect does not reach it, but the diagnosability one does.

Left out deliberately rather than folded in: this PR carries what was measured on the Windows machine, and extending it here would mix verified work with unverified. Worth a follow-up.

What was verified on Windows 11 / Git Bash

Quoted rather than summarised:

Verified on Windows 11 / Git Bash: connect now returns a real HTTP response, writes remote_binding, remote status reports connected, and the sync engine round-trips (push.ack seq 1 stored -> pull.applied). Existing-store team migration is NOT yet exercised.

What was verified on macOS, and what that is worth

tests/test_remote.bats — 122 ok / 0 not ok, on this branch alone.

Green was not evidence anything was diagnosed. When this was written, no test in the suite asserted on curl's stderr — that is no longer true, and the tests added since are described at the end of this body. What green establishes is that capturing it, and emitting it only on failure, leaves every existing caller's behaviour and every reported HTTP code unchanged.

Not measured

A team with an existing store has not been migrated on Windows. The verified run was a fresh team.

---

Tests, added at adbf2b34d74f

tests/test_remote_curl_stderr.bats — 5 ok / 0 not ok, driving the production _remote_http_post_json. Production is untouched; the diff since the reviewed head is this one file.

The helper's stderr is captured to a file rather than read from bats's $output, which merges the streams. The http code goes to stdout and the diagnosis has to go to stderr — a test that cannot tell them apart cannot check that.

case
failing curl	the diagnosis reaches the caller's stderr
succeeding curl	nothing does — and the stub wrote to stderr anyway
both	the http code is unchanged, 200 and 000
both	no scratch file left in the run's own TMPDIR
control	the leftover check fires on a planted leftover

The second row is the one that needs explaining. Real curl -sS is quiet on success, so a stub that also stayed quiet would pass against a version that dumped stderr unconditionally — and that version drops noise into the middle of a caller's output. Making the stub noisy on success is what separates "shown only when curl failed" from "the stream happened to be empty".

The last row is the control on an absence: a glob that matches nothing looks exactly like a glob aimed at the wrong directory.

mutation	red
M0 no mutation	0
M1 2>/dev/null restored	the diagnosis reaches stderr
M2 stderr shown on success too	success stays quiet
M3 the error file never removed	no scratch left behind
M4 failure reports curl's exit code	the code contract, and the two cases that read it

Not covered

The signal path. The scratch file is removed on both normal paths, and the trap set earlier in the function does not name it — its list is the config and the fifo. That is a reading of the trap's text, not a measurement: I tried to drive a signal through it and the probe hung on this host, because the bounded copier keeps the run alive while curl is being waited on.

Adding curl_err to that trap is one line, but it changes the fix rather than testing it, so it is the author's call rather than mine.

Windows is unverified by me, here as elsewhere in this series.

---

Tests and one further production change, at d59284c11e38

Review asked for one line — put curl_err in the existing trap so an early exit cannot strand it. I wrote that line, added a test for it, and the test failed. What the measurement then said is why this head carries more than tests.

An EXIT trap set inside a function cannot read that function's locals

It runs after the frame is gone, so a single-quoted body expands $cfg in the caller's scope, where no local of that name exists. It removes the empty string, returns 0, and reads as a cleanup that worked.

bash 3.2.57   TRAP SEES: [EMPTY]
bash 5.3.15   TRAP SEES: [EMPTY]

So this was never about curl_err. The pre-existing trap has never swept anything on an early exit. Driven with a stub curl that fails after writing its headers and a cat that fails, so errexit leaves the function between the mktemp and the tail cleanup, a real run left all three behind:

agmsg-curl-cfg.cn24V2        a 0600 config naming the request body
agmsg-curl-err.cJJHwm
agmsg-header-pipe.2u0Xbp

The config is the one that matters — it is the file this helper exists to keep out of curl's argv.

The fix: bake the paths into the trap with printf %q at set time instead of expanding them when it fires, and create curl_err with the other temporaries so it exists before the trap that must remove it. Same run afterwards leaves nothing. This is more than the one line asked for, and the reason is that the one line does not work; the tests bind the behaviour, not the mechanism, so a different shape is open to the author.

tests/test_remote_curl_stderr.bats — 7 ok / 0 not ok

case
failing curl	the diagnosis reaches the caller's stderr
succeeding curl	nothing does — and the stub wrote to stderr anyway
both	the http code is unchanged, 200 and 000
both	no scratch file left in the run's own TMPDIR
early exit	errexit out of the middle sweeps all three files
premise	an EXIT trap cannot read the locals of the function that set it — measured, so the printf %q baking is not mistaken for ceremony and simplified back
control	the leftover check fires on a planted leftover

mutation	red
M0 no mutation	0
M1 2>/dev/null restored	the diagnosis reaches stderr
M2 stderr shown on success too	success stays quiet
M3 the error file never removed	no scratch left behind
M4 failure reports curl's exit code	the code contract, and the two cases that read it

Existing suite unchanged on this head: tests/test_remote.bats — 122 ok / 0 not ok.

The early-exit case took three attempts, and each failure is worth naming

1. A signal. The probe hung: on that path the bounded copier is never reaped, so it outlives the shell holding the inherited stdout and bats's capture waits on it. Review suggested errexit instead, which is bounded.
2. Writing the fake cat through the sandbox symlink. > follows a symlink, so it wrote to the system's own /bin/cat — refused by the OS here. On a machine where it was not refused, the test would have replaced a system binary. The symlink is removed first now.
3. A failure stub that exited without writing the headers, which strands the copier on open() and hangs for the same reason as (1). The stub now writes headers and then fails, which is what a real curl does when it dies on the body.

Windows is unverified by me.

---

Update at a38d6f8e1430 — one directory, armed before anything else exists

Review's remaining point: whatever is created before the trap is armed is unprotected, and the previous shape kept that window — it made three things and then armed a trap over all three. A failure at the second or third strands the first, including a 0600 config naming the request body.

Reordering cannot close it, because there is always a first allocation. So there is now exactly one:

work_dir="$(mktemp -d ...)"
trap "rm -rf <baked work_dir>" EXIT INT TERM
cfg="$work_dir/config"; curl_err="$work_dir/stderr"; header_fifo="$work_dir/header"

Everything else is made inside a directory that is already condemned; the normal path removes the same directory. The printf %q baking stays, for the measured reason above.

New case: make mkfifo fail — after the directory exists and after the config is inside it — and assert nothing survives.

And four of my absence assertions were aimed at names that no longer exist

The leftover checks globbed agmsg-curl-cfg.* and agmsg-curl-err.*. This layout never creates those. They would have passed on any behaviour, and gone on passing while the new directory leaked. Both they and their planted-leftover control now use the name the helper really mints.

An absence assertion pointed at a name nothing uses is indistinguishable from a clean run — and I wrote four of them without noticing the rename I had just made underneath.

tests/test_remote_curl_stderr.bats   8 ok / 0 not ok
tests/test_remote.bats             122 ok / 0 not ok   (unchanged)

Filed separately, not folded in

On the failure path the bounded copier is not reaped when the function leaves early, and it holds the caller's inherited stdout — which is what hung three of my probes. It predates the stderr work, so it is #864 rather than a change here.