fix(remote): keep curl's stderr, and hold the temporaries in one condemned directory (#850)

[fujibee]

Declared reviewers: 1

Describes head c7f9010c996378e0b5092bbf8ee01d5f2ff91886, based on main at dd61c04.

This replaces #853, which GitHub closed and will not reopen. When #886 merged, its branch was deleted; GitHub closes any PR stacked on a deleted base, and once the head has been force-pushed it refuses to reopen ("the branch was force-pushed or recreated"). Same branch, same work, same review thread contents — copied below so nothing is lost — but the number changes.

The earlier CLEARED verdict is void, and not only because of the renumbering: rebasing onto main changed every commit SHA. The four commits are identical in content to the ones reviewed at 597d345; the tree is the same work applied to a main that now contains #886, #895 and #899.

Where it stands after the rebase

head        c7f9010c996378e0b5092bbf8ee01d5f2ff91886
base        main (dd61c04)
drift       STRUCTURAL, NOT MEASURED — merge-base == origin/main
tests       24 ok / 0 not ok across the three files covering this helper

The rebase was structurally required, not a preference: the old merge-base was #886's pre-merge head, which is not an ancestor of main.

---

(Everything below is the body as it stood on #853, kept verbatim. Its head references are historical — the current head is the one named above.)

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.

---

Everything below this line up to the next heading is history. It describes edcebc5, whose contract for the failing-diagnosis case was the opposite of the current one.

Rebuilt on current main at edcebc597e39bd7959ddfe77f516050517810477

The earlier verdicts on this PR are void. Everything above describes heads that predate #868, which landed the header-sink work and gave this helper two arms. Drift against today's main was NON-EMPTY — 213 insertions in scripts/remote.sh, the one file this changes, so the branch was rebuilt rather than rebased.

Base is now fix/curl-tests-read-merged-stream (#886), and that is a real dependency, not tidiness — measured in both directions below.

What this adds to main

Curl's stderr is kept and shown when curl actually failed. The caller only ever sees the HTTP code, and this helper reports 000 for every kind of failure alike — refused connection, timeout, a path curl could not open. Shown only on failure: curl -sS is silent on success, so an unconditional dump would land in the middle of a caller's output.

What it simplifies

All temporaries move into one directory, minted before the trap is armed:

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"

Two reasons, both measured earlier in this series:

• An EXIT trap cannot expand the locals of the function that set it. It runs after the frame is gone, so a single-quoted $cfg expands in the caller's scope and removes "". bash 3.2.57 and 5.3.15 both report EMPTY. printf %q fixes the value at set time.
• Anything created before the trap is armed is unprotected, and reordering cannot fix that because there is always a first allocation.

It also removes a hazard instead of guarding it. main carries a comment warning that on the marker path header_fifo IS header_file, so an unconditional rm -f "$header_fifo" would delete the headers the caller asked for. The caller's file is outside work_dir, so rm -rf cannot reach it — the guard becomes a property of the layout. Two traps become one and fifo_dir disappears.

The dependency on #886, measured both ways

main's harness + this change   ->  not ok 21  an untranslated POSIX path ... is the reported 000
#886's harness + this change   ->  ok

tests/test_remote_curl_config_paths.bats compares bats's $output — stdout and stderr merged — against "000". Showing curl's diagnosis puts a line in front of the code. That test is already intermittently red on main for the same reason under CI load; this change turns intermittent into certain. #886 is a prerequisite.

Tests

tests/test_remote_curl_stderr.bats — 8 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 left in the run's own TMPDIR
early exit	errexit out of the middle sweeps everything
setup failure	mkfifo fails after the directory exists — nothing survives
premise	an EXIT trap cannot read the locals of the function that set it, measured
control	the leftover check fires on a planted leftover

22 ok / 0 not ok across this file and the two already covering this helper (test_remote_header_sink.bats, test_remote_curl_config_paths.bats).

Windows is unverified by me. Related: the header copier is not reaped on an early exit (#864).

---

History from here to the next heading. f94c333 concluded that the survivor question could not be measured; the head below measures it.

Current head: f94c333f28c7731c2b35d1e8d56b224c37c9685c

Two review rounds since edcebc5. Both changed what this PR claims, so the section above is history and this one is the state.

The diagnosis line was itself an exit

[ "$curl_status" -ne 0 ] && [ -s "$curl_err" ] && cat "$curl_err" >&2

Under set -e a failing cat — closed stderr, a reader that went away, a full disk — ends the function there. The copier is never reaped, the work_dir is never removed, and the caller gets nothing where this helper promises 000 for every failure. Being unable to explain a failure turned it into a different failure.

And the test I shipped asserted that. It drove a failing cat, asserted nonzero status and empty stdout, and its own comment noted the copier was left orphaned. It measured the defect and called it the property; the reviewer read the test as the specification, which is what a test is.

Now: reap and decide first, then write the diagnosis best-effort. Same case, contract the other way round.

mutation	red
M0 no mutation	0
M1 diagnosis back before the branch, fatal	the new case
M2 same position, || true removed	the new case
M3 diagnosis removed entirely	the diagnosis reaches stderr
M4 diagnosis shown on success too	success stays quiet

M1 and M2 redden separately, so position and best-effort are bound apart.

What this case does NOT assert, and why

The second round questioned the copier check I added: it passed vacuously on an empty record, and its teardown killed by remembered PID — a number the kernel may reuse, so the fixture could have signalled an unrelated process on the runner.

Fixing it properly produced a finding. Four instruments, each failing its own positive control:

instrument	what it did
a pid the copier records itself	the failure path kills it before the forked shell runs its first line — log empty
pgrep -f <full script path>	no match while the process is alive
pgrep -f bounded-copy.py	matched an unrelated shell whose argv merely contained the string — would have killed the wrong process
lsof +D <work dir>	nothing: a copier blocked in open() holds no fd on it yet

One fact explains all four: a copier waiting on the fifo is still a forked bash wearing its parent's command line — measured, comm reads bash — because python3 is not exec'd until curl opens the pipe. It is identifiable as a child of that shell and by nothing else, and after the shell exits, not even that.

A fifth attempt — running the driven shell in its own process group, so the group could be the identity — hung the harness, the same way three earlier probes in this series died: the orphan holds the capture pipe.

So the assertion is removed rather than left as a green that measures nothing, and the case carries all of the above where the next person will be tempted to add it back. The reaping is in production (kill, then wait, before the code is returned); what is unproven is the absence of a survivor, and that behaviour is already filed as #864.

Still asserted

status 0 and "000" on stdout   the contract, unchanged by a failed write
no work_dir left               cleanup still runs

tests/test_remote_curl_stderr.bats — 8 ok / 0 not ok. Base remains #886, for the reason measured above.

---

Current head: 597d345525efb1b5401b167ac38fdffd973b90fe

The section above says the absence of a survivor cannot be measured. That was wrong, and the reviewer named the identity I had not used: the helper's own child table.

Every instrument I tried looked for the copier in the process table after the fact. All four failed for one reason — a copier waiting on the fifo is still a forked bash wearing its parent's command line, because python3 is not exec'd until curl opens the pipe. Nothing outside can name it.

But wait is a builtin over the shell's own waitable children. A successful wait is the observation, and it needs no identity of its own.

So the driven shell shadows kill and wait, records the pid each was given, and delegates to the builtins. Production runs unchanged; only the seam is recorded.

the pid killed is the pid waited on
the wait collected that child -- rc is not 127, bash's "not a child of this shell"

It earns its place

mutation	red
M0 no mutation	0
M1 diagnosis before the branch, fatal	the failing-diagnosis case
M2 same position, || true removed	the failing-diagnosis case
M3 diagnosis removed entirely	the diagnosis reaches stderr
M4 diagnosis shown on success too	success stays quiet
M5 failure arm no longer reaps	the failing-diagnosis case

M5 is the one that matters. Status stays 0, stdout stays "000", the work_dir is still gone — every assertion this case had before would be green on a build that never reaps. The seam is what tells them apart.

M1 and M2 redden through the same case for the opposite reason: a fatal cat exits before the seam is reached, so nothing is recorded.

The boundary, narrowed

#864 still holds the other early-exit routes — signals in particular, where the trap removes files and does not kill a process. That this PR's failing-diagnosis route reaches kill and wait is measured here rather than deferred.

tests/test_remote_curl_stderr.bats — 8 ok / 0 not ok; 22 ok / 0 not ok across the three files covering this helper. Base remains #886.