diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 96c761da3..98e5e8320 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -90,8 +90,29 @@ agmsg_sync_autostart() { # reaping rule — seven review rounds of machinery to make a lookup safe # that the thing it protected against no longer needs. Reading the engine # is what settled it, not the review count. + # A START THAT DID NOT HAPPEN IS A FAILED START, AND IS SAID SO (#810). + # + # This used to `return 0` here. Three things went with it: no engine was + # started, NOTHING was printed — the started/slow/failed blocks are all + # built after the loop, so leaving early skips every one of them — and the + # teams after this one were abandoned without being tried. + # + # The cost is specific. #761/#765 exist so that "connected, and not + # syncing" is never silent, and #775 replaced their warning with this + # function on the stated grounds that the warning survives whenever a start + # fails. On this path it did not: the operator was told nothing, which is + # the exact state those issues were opened about, reachable by another door. + # + # `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. The last one + # is uncomfortable here, because a start that outruns its budget + # deliberately leaves its two temp files behind, so this feature can + # contribute to the condition that used to silence it. tmp="$(mktemp 2>/dev/null)" || tmp="" - [ -n "$tmp" ] || return 0 + if [ -z "$tmp" ]; then + failed="$failed$team could not create a temporary file (is the temp filesystem full or unwritable?)"$'\n' + continue + fi # stderr folded in: `cmd_sync_start` says why it refused on stderr, and that # sentence is the useful half of a failure. Swallowing it would leave this # printing "could not start" with the reason on the floor. diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index 1dbc7ef8c..bdf0dd39b 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -397,3 +397,32 @@ write_fake_remote() { # # Recorded rather than silently dropped, because "there used to be a check" # reads as an oversight to whoever finds this next. + +@test "a team whose temp file cannot be made is reported, and the rest are still tried (#810)" { + # `mktemp` failing is a start that did not happen, and #761/#765 exist so + # that a start that did not happen is never silent. Before this, the function + # returned early: no engine, NO OUTPUT — the started/slow/failed blocks are + # built after the loop — and every team after this one abandoned untried. + # + # Driven by putting `mktemp` on PATH as a command that fails, which is what + # a full or unwritable temp filesystem looks like from inside this function. + source "$SCRIPTS/lib/sync-autostart.sh" + local bindir="$TEST_SKILL_DIR/failing-bin" + mkdir -p "$bindir" + printf '%s\n' '#!/usr/bin/env bash' 'exit 1' > "$bindir/mktemp" + chmod +x "$bindir/mktemp" + + local fake; fake="$(write_answering_remote started)" + PATH="$bindir:$PATH" run agmsg_sync_autostart "$fake" teamone teamtwo + [ "$status" -eq 0 ] + + # Said, in the #765 block, with its runnable remedy. + printf '%s' "$output" | grep -q 'connected, but not syncing' + printf '%s' "$output" | grep -q 'temporary file' + # BOTH teams: the second must not be abandoned because the first could not + # allocate. That is what `return` did and `continue` does not. + printf '%s' "$output" | grep -q 'teamone' + printf '%s' "$output" | grep -q 'teamtwo' + # The remedy is per team and runnable, unchanged from #765. + printf '%s' "$output" | grep -q 'sync start' +}