Stop the update agent dying on its own tag arithmetic

Since 1.2.0 the agent has ended with a non-zero status on every tick, before
writing anything. The console showed a check that never finished and a
last-checked time frozen at the moment of the deployment, and the update button
could not be reached at all — the one mechanism that would have delivered the
fix.

Two instances of the same trap, both mine, both under `set -Eeuo pipefail`:

The tag count was `release_version_gt … && echo`. The LAST iteration is almost
always a tag that is not newer, so the loop ended non-zero, pipefail carried it
out of the pipeline, the command substitution inherited it, and set -e ended
the agent. `if` rather than `&&` is the whole fix: an if whose condition is
false still returns 0.

The newest tag came from `git tag … | head -1`. head exits after one line, git
takes SIGPIPE, pipefail does the rest. install-agent.sh has carried a written
warning about exactly this since it was written; I read it and wrote it anyway.

Both now live in deploy/lib/release.sh, because the reason neither was caught
is that no PHP test can reach a bash pipeline.
tests/Feature/ReleaseComparisonTest.php runs the real functions in a real
throwaway repository under the agent's own set -Eeuo pipefail, and asserts the
EXIT CODE as well as the answer — an answer nobody lives to read is not an
answer. The SIGPIPE case needs four hundred tags to reproduce, because a short
list finishes writing before head leaves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

VERSION

@@ -1 +1 @@
-1.2.1
+1.2.2

deploy/lib/release.sh

@@ -42,6 +42,58 @@ release_manifest_version() {
     sed -n 's/.*"version"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' "$MANIFEST_FILE" 2>/dev/null | head -1
 }
 
+# release_version_gt A B — true when A is strictly a higher version than B.
+#
+# By version, not by ancestry: a tag can be cut from anywhere, and what an
+# operator is being offered is a version number. `sort -V` does the comparing,
+# so v1.10.0 correctly beats v1.9.0 where a string comparison would not.
+release_version_gt() {
+    [[ "$1" != "$2" ]] && [[ "$(printf '%s\n%s\n' "$1" "$2" | sort -V | tail -1)" == "$1" ]]
+}
+
+# release_newest_tag — the highest v* tag by version order, or nothing.
+#
+# Deliberately not `git tag … | head -1`. head exits after the first line, git
+# gets SIGPIPE, and under `set -o pipefail` that non-zero status leaves the
+# command substitution and ends the caller — the same way the tag count did,
+# and the same trap install-agent.sh already carries a warning about. A read
+# loop over a process substitution is not a pipeline and cannot do it.
+release_newest_tag() {
+    local tag
+    while read -r tag; do
+        [[ -n "$tag" ]] || continue
+        printf '%s' "$tag"
+        return 0
+    done < <(git tag --list 'v*' --sort=-v:refname 2>/dev/null)
+
+    return 0
+}
+
+# release_tags_ahead VERSION — how many v* tags are higher than VERSION.
+#
+# Lives here rather than inline in the agent because it has to be testable. The
+# inline version killed the update agent on every tick: the loop body was
+# `release_version_gt … && echo`, so the LAST iteration — almost always a tag
+# that is not newer — left the loop with a non-zero status, `pipefail` carried
+# it out of the pipeline, the command substitution inherited it, and `set -e`
+# ended the agent before it could write a status. The console then showed a
+# check that never finished and a last-checked time frozen at the deployment.
+#
+# `if` rather than `&&` is the whole fix: an if whose condition is false still
+# returns 0. Anything added here must keep that property.
+release_tags_ahead() {
+    local current="$1" tag count=0
+
+    while read -r tag; do
+        [[ -n "$tag" ]] || continue
+        if release_version_gt "${tag#v}" "$current"; then
+            count=$((count + 1))
+        fi
+    done < <(git tag --list 'v*' --sort=-v:refname 2>/dev/null)
+
+    printf '%s' "$count"
+}
+
 release_remember() { # release_remember MODE SOURCE
     mkdir -p "$(dirname "$MODE_FILE")"
     printf '%s' "$1" > "$MODE_FILE"

deploy/update-agent.sh

@@ -216,22 +216,20 @@ FETCH_ERROR=''
 DEPLOYED_VERSION="$(release_manifest_version)"
 [[ -n "$DEPLOYED_VERSION" ]] || DEPLOYED_VERSION="$(release_version)"
 
-# version_gt A B — true when A is strictly a higher version than B.
-version_gt() {
-    [[ "$1" != "$2" ]] && [[ "$(printf '%s\n%s\n' "$1" "$2" | sort -V | tail -1)" == "$1" ]]
-}
-
 if git fetch --quiet --tags --force origin 2>/dev/null; then
-    # Newest by version order, not by tag date.
+    # Newest by version order, not by tag date. Through the helper rather than
-    NEWEST_TAG="$(git tag --list 'v*' --sort=-v:refname 2>/dev/null | head -1)"
+    # `| head -1`: head exits after one line, git takes SIGPIPE, and pipefail
+    # ends the agent — see release_newest_tag.
+    NEWEST_TAG="$(release_newest_tag)"
     NEWEST_VERSION="${NEWEST_TAG#v}"
 
-    if [[ -n "$NEWEST_TAG" ]] && version_gt "$NEWEST_VERSION" "$DEPLOYED_VERSION"; then
+    if [[ -n "$NEWEST_TAG" ]] && release_version_gt "$NEWEST_VERSION" "$DEPLOYED_VERSION"; then
         # How many releases ahead, so the console can say "2 Aktualisierungen"
-        # rather than only that something exists.
+        # rather than only that something exists. Both helpers live in
-        BEHIND="$(git tag --list 'v*' --sort=-v:refname 2>/dev/null | while read -r t; do
+        # deploy/lib/release.sh so a test can run them — the version that lived
-            version_gt "${t#v}" "$DEPLOYED_VERSION" && echo "$t"
+        # here ended the agent on every tick under `set -e`, and no test in a
-        done | wc -l | tr -d ' ')"
+        # PHP suite can reach a bash pipeline.
+        BEHIND="$(release_tags_ahead "$DEPLOYED_VERSION")"
         TARGET_RELEASE="$NEWEST_TAG"
         REMOTE_COMMIT="$(git rev-parse "refs/tags/${NEWEST_TAG}^{commit}" 2>/dev/null || echo '')"
     else

tests/Feature/ReleaseComparisonTest.php

@@ -0,0 +1,145 @@
+<?php
+
+use Symfony\Component\Process\Process;
+
+/**
+ * The update agent's version arithmetic, run as the shell actually runs it.
+ *
+ * These exist because a PHP suite cannot reach a bash pipeline, and the bug
+ * that made them necessary was invisible to every other kind of test: the tag
+ * count was written as `release_version_gt … && echo`, so the LAST iteration —
+ * almost always a tag that is not newer — left the loop with a non-zero status.
+ * `pipefail` carried it out of the pipeline, the command substitution inherited
+ * it, and `set -e` ended the agent before it could write a status file. On the
+ * server the console showed a check that never finished and a last-checked time
+ * frozen at the moment of the deployment.
+ *
+ * Every case therefore runs under the agent's own `set -Eeuo pipefail`, and
+ * asserts the EXIT CODE as well as the answer. An answer nobody lives to read
+ * is not an answer.
+ */
+function runRelease(string $snippet, array $tags = []): Process
+{
+    $lib = base_path('deploy/lib/release.sh');
+
+    // A throwaway repository, because release_tags_ahead reads real tags. A
+    // fixed identity and no signing, so this cannot pick up whatever the
+    // machine running the suite happens to have configured.
+    $repo = sys_get_temp_dir().'/clupilot-release-test-'.bin2hex(random_bytes(6));
+    $tagCommands = '';
+
+    foreach ($tags as $tag) {
+        $tagCommands .= "git tag {$tag}\n";
+    }
+
+    $script = <<<BASH
+    set -Eeuo pipefail
+    mkdir -p {$repo} && cd {$repo}
+    git init -q .
+    git -c user.email=t@example.com -c user.name=t commit -q --allow-empty -m init
+    {$tagCommands}
+    . {$lib}
+    {$snippet}
+    BASH;
+
+    $process = Process::fromShellCommandline('bash -c '.escapeshellarg($script));
+    $process->run();
+
+    (new Process(['rm', '-rf', $repo]))->run();
+
+    return $process;
+}
+
+it('counts the releases ahead without taking the agent down with it', function () {
+    // The exact shape that failed: the newest tag is ahead, and the tags after
+    // it in the list are not — so the loop's final iteration is a false one.
+    $process = runRelease(
+        'release_tags_ahead 1.2.0',
+        ['v1.1.0', 'v1.1.1', 'v1.2.0', 'v1.2.1'],
+    );
+
+    expect($process->getExitCode())->toBe(0)
+        ->and(trim($process->getOutput()))->toBe('1');
+});
+
+it('survives a run where no tag at all is ahead', function () {
+    // Every iteration false, including the last. This is the ordinary state of
+    // an up-to-date server, which is to say: most ticks, on most machines.
+    $process = runRelease(
+        'release_tags_ahead 1.2.1',
+        ['v1.1.0', 'v1.2.0', 'v1.2.1'],
+    );
+
+    expect($process->getExitCode())->toBe(0)
+        ->and(trim($process->getOutput()))->toBe('0');
+});
+
+it('survives a repository with no tags whatsoever', function () {
+    $process = runRelease('release_tags_ahead 1.0.0');
+
+    expect($process->getExitCode())->toBe(0)
+        ->and(trim($process->getOutput()))->toBe('0');
+});
+
+it('counts several releases ahead', function () {
+    $process = runRelease(
+        'release_tags_ahead 1.1.0',
+        ['v1.1.0', 'v1.1.1', 'v1.2.0', 'v1.2.1'],
+    );
+
+    expect($process->getExitCode())->toBe(0)
+        ->and(trim($process->getOutput()))->toBe('3');
+});
+
+it('picks the newest tag out of a long list without dying on a broken pipe', function () {
+    // The second instance of the same trap. `git tag … | head -1` makes head
+    // exit after one line, git takes SIGPIPE, and pipefail ends the caller.
+    // Few tags hide it — git finishes writing before head leaves — so it needs
+    // enough of them to actually fill a pipe buffer.
+    $tags = [];
+
+    for ($i = 1; $i <= 400; $i++) {
+        $tags[] = 'v1.'.$i.'.0';
+    }
+
+    $process = runRelease('release_newest_tag', $tags);
+
+    expect($process->getExitCode())->toBe(0)
+        ->and(trim($process->getOutput()))->toBe('v1.400.0');
+});
+
+it('answers with nothing, and cleanly, when there are no releases yet', function () {
+    $process = runRelease('release_newest_tag; echo "[$?]"');
+
+    expect($process->getExitCode())->toBe(0)
+        ->and(trim($process->getOutput()))->toBe('[0]');
+});
+
+it('orders by version, not alphabetically', function () {
+    // v1.10.0 is newer than v1.9.0 and sorts earlier as a string. Getting this
+    // wrong means a server sitting on 1.9.0 is told it is current.
+    $process = runRelease(
+        'release_tags_ahead 1.9.0',
+        ['v1.9.0', 'v1.10.0'],
+    );
+
+    expect($process->getExitCode())->toBe(0)
+        ->and(trim($process->getOutput()))->toBe('1');
+});
+
+it('answers the pairwise comparison the same way, and exits cleanly either way', function () {
+    $cases = [
+        ['1.2.1', '1.2.0', 'ja'],
+        ['1.2.0', '1.2.1', 'nein'],
+        ['1.2.0', '1.2.0', 'nein'],
+        ['1.10.0', '1.9.0', 'ja'],
+        ['2.0.0', '1.99.99', 'ja'],
+    ];
+
+    foreach ($cases as [$a, $b, $expected]) {
+        $process = runRelease("if release_version_gt {$a} {$b}; then echo ja; else echo nein; fi");
+
+        expect($process->getExitCode())->toBe(0)
+            ->and(trim($process->getOutput()))->toBe($expected, "{$a} > {$b}");
+    }
+});