fix(sync): a start that never began is a failed start, not a silent return (#810)

[fujibee]

Declared reviewers: 1

Closes #810.

Landing on integration/remote. Head 5255e6810cd24abbb9da9525fea192edd9a0044d.

Closes does not fire off the default branch, so #810 is closed by hand after landing.

The defect, which I shipped

agmsg_sync_autostart did this per team:

tmp="$(mktemp 2>/dev/null)" || tmp=""
[ -n "$tmp" ] || return 0

Three things went with that return, and none was announced:

1. no engine was started;
2. nothing was printed — the started / slow / failed blocks are all built after the loop, so leaving early skips every one of them;
3. the teams after this one were abandoned untried, because it is return, not continue.

Why it costs more than it looks

#761/#765 exist so that connected, and not syncing is never silent, and #775 — mine — replaced their warning block with this function, on the stated grounds that the warning survives whenever a start fails. tests/test_delivery.bats says so in as many words:

its warning, its wording and its runnable remedy are exactly what remains when the start FAILS

That held for the failure the test drives, a sync start that runs and refuses. It did not hold here. A start that never began is also a start that failed, and on that path the operator was told nothing at all — the exact state those two issues were opened about, reachable through a door I added.

mktemp failing is not a missing binary. It is TMPDIR unset to a path that does not exist, or not writable, or a full filesystem — and the last one is uncomfortable, because a start that outruns its budget deliberately leaves its two temp files behind. The feature can contribute to the condition that used to silence it.

The fix

Report it as what it is, and keep going:

if [ -z "$tmp" ]; then
  failed="$failed$team	could not create a temporary file (…)"$'\n'
  continue
fi

The failed block then prints the per-team bash … sync start <team> remedy that #765 established, unchanged.

Test

One case, driven by putting a failing mktemp on PATH — which is what a full or unwritable temp filesystem looks like from inside this function, rather than a mock of the branch.

It asserts the warning, the reason, the remedy, and both team names: the second must not be lost because the first could not allocate.

mutation	result
the silent return 0 restored	red — connected, but not syncing is absent
continue changed to break	red — the second team is absent

Two mutations, two different assertions. The second matters because a fix that only added the message would still abandon the rest of the teams, and one assertion cannot tell those apart.

Measurements

tests/test_sync_autostart.bats                 11 tests, 0 failures
tests/test_delivery.bats                      179 tests, 0 failures
.github/scripts/check-enforced-assertions.sh  638, at the baseline (638)

Drift

merge-base                 6b29693b61ca3d54c1861d0a5dfe5323b26a2a05
origin/integration/remote  6b29693b61ca3d54c1861d0a5dfe5323b26a2a05
git diff --stat <merge-base> origin/integration/remote -- scripts/lib/sync-autostart.sh tests/test_sync_autostart.bats
  → empty

Re-measured immediately before landing.

Not in this PR

#802 and #804 are the same family — a failure turned into an ordinary value, and an unexpected mktemp dependency — and are left to their own issues. What is specific here is that the thing lost was a user-visible warning another issue was opened to create.

Attribution

Found by review after #775 had landed, and filed as #810 rather than mentioned. I landed the defect; the reading that caught it is not mine.