ci(release): gate releases on a two-sided auto-update canary (#5222)

.claude/rules/bug-prevention-patterns.md

@@ -1,6 +1,7 @@
 ---
 paths:
   - "crates/core/src/bin/**"
+  - "scripts/**"
 ---
 
 # Bug Prevention Patterns (freenet-core)
@@ -59,6 +60,103 @@ A future revert of any of these null-stdio calls fails CI with a
 specific, issue-numbered error message rather than shipping the
 regression silently.
 
+## Log markers that a CI gate greps for
+
+A gate that decides whether a release ships by grepping the node's log is only
+as real as the marker it greps for. Two independent mechanisms silently turn
+such a gate into one that cannot fail, and #5236 hit both at once — in the
+canary whose entire purpose was to stop a vacuous release signal.
+
+**1. Level.** `crates/core/Cargo.toml:124` enables tracing's
+`release_max_level_info`, which compiles out everything below INFO *in release
+builds*. A `debug!` marker therefore does not exist in the binary the gate
+inspects. It is present in every debug build, so it looks fine locally and in
+any test that runs a debug binary; the gate observes nothing and can only pass
+vacuously. The `Startup update check complete` marker was a `debug!`, which made
+the most common healthy outcome ("finished, staying on this version") produce no
+log ending at all — byte-for-byte indistinguishable from a node killed
+mid-request.
+
+**2. Anchor.** A whole-file `grep -F` for the marker is satisfied by ANY
+occurrence in the file, including a `//` comment — very often one inside the
+file's own `#[cfg(test)] mod tests` block, where log excerpts get pasted as
+documentation. The source pin then tracks the comment, not the code.
+
+| Marker | How it broke |
+|--------|--------------|
+| `Startup update check complete` | Emitted at `debug!`, so absent from every release binary. The canary's "did the check finish?" assertion could never observe it. |
+| `failed to parse latest version` | Occurs twice in `auto_update.rs`: the production `tracing::warn!` (the format literal `Startup update check: failed to parse latest version '{}'`) and a prose comment inside that file's own `#[cfg(test)] mod tests` (`// WARN failed to parse latest version 'v0.2.121':`). Both are quoted rather than cited by line number on purpose — the first version of this row cited `:1546`/`:1757`, which this very commit's +23 lines had already shifted to `:1569`/`:1780`. A line number in a rule about stale pins rots faster than the thing it describes. Rewording the production line left all 22 assertions green — including `ok - source pin: parse-failure marker` — while a node carrying the #5221 bug then logged check-ran + reworded-warn + check-complete and the canary reported `OK: parsed GitHub's response`. An ordinary log reword deletes the gate, with CI green throughout. |
+| (no marker at all) | The gate had nothing to say WHICH release the node compared against, so its healthy verdict was byte-identical to a silently-wrong comparator's — see the positive-fact rule below. Closed by `MARKER_LATEST_SEEN`. |
+| `triggering auto-update` | A fixed string, so it never matched `freenet.rs:609`'s "triggering IMMEDIATE auto-update". A node that took the urgent path read as one that never decided to update, for as long as that site had existed. Fail-closed, hence unnoticed. Closed by `MARKER_TRIGGERED_RE` plus a count pin. |
+
+### The rule
+
+- Emit any gate-observed marker at **`info!` or above**. Never `debug!`/`trace!`.
+- **Pin the emitting call, not the file.** Match the macro together with its
+  literal (`tracing::warn!("<marker>`), whitespace-stripped on both sides so a
+  rustfmt reflow cannot disarm it. A bare file grep is satisfied by prose.
+- **Pin every arm that shares the marker.** `compare_versions_for_startup` has
+  two parse-failure arms; a pin on one lets the other drift.
+- Prefer a marker string that is **specific enough not to appear in prose** —
+  keeping the `Startup update check: ` prefix is what stops the test-module
+  comment quoted above from matching at all.
+- **Assert a POSITIVE fact, not the absence of an error.** "No error appeared"
+  is satisfied by a component that is silently WRONG as well as by one that
+  works: a `version_from_tag` regressed to a constant, or a normaliser
+  truncating `0.2.121` to `0.2.12`, parses, compares, declines to update and
+  logs a clean completion — a log byte-identical to a healthy node's. Make the
+  code log the value it acted on, and have the gate compare it against an
+  independently-obtained expected value. Resolve that expected value from the
+  **same source the code uses** (here, the `releases/latest` redirect, not
+  `api.github.com`): two sources that are allowed to disagree produce failures
+  that are not bugs.
+- **Pin the COUNT when a marker is supposed to match a SET of call sites.** A
+  fixed-string `MARKER_TRIGGERED` missed `freenet.rs:609` ("triggering
+  IMMEDIATE auto-update") for as long as that site existed, so a node taking
+  the urgent path read as one that never decided to update. It failed CLOSED,
+  which is exactly why nobody noticed — **fail-closed is not the same as
+  correct, and it is the condition under which a wrong enumeration survives
+  longest.** But **the count must not be derived from the marker it audits.**
+  The first version of that pin computed the actual site count as
+  `grep -cE "$MARKER_TRIGGERED_RE"` — the very regex under audit — so a site
+  the regex failed to match was missing from the count too, and the two errors
+  cancelled. Demonstrated: adding a sixth trigger site worded "triggering a
+  fresh auto-update" left the suite fully green, including the assertion
+  claiming exactly five sites. It caught a REWORDED existing site (count drops)
+  and nothing else, which is the weaker half of what it advertised. Derive the
+  expected count from a **structural** anchor the marker cannot influence —
+  here `update_tx.send(new_version)`, the call that actually requests the
+  update — then assert the marker matches all of them. Same shape as the
+  "metric re-derived at the call site" row below: an audit whose two operands
+  come from one source cannot report a disagreement.
+- **A skip branch in a gate is a vacuous-pass waiting to happen.** If the gate
+  can only run its check when some input is present, pin the caller that
+  supplies it. `assert_detection_healthy` skips the equality check when
+  `CANARY_EXPECTED_LATEST` is unset — correct for keeping the function pure and
+  fixture-testable, but it makes the check only as real as `cmd_preflight`, so
+  that assignment is itself pinned.
+
+### Audit
+
+Every marker a script greps for must resolve to production code at every
+occurrence, and its pin must be mutation-tested by rewording the real call and
+confirming the pin goes RED.
+
+```bash
+grep -n '^MARKER_' scripts/auto-update-canary.sh
+# then, for each marker, confirm no occurrence is a comment:
+grep -n "<marker>" crates/core/src/bin/commands/auto_update.rs \
+                   crates/core/src/bin/freenet.rs
+```
+
+Source-level regression pins live in
+`scripts/auto-update-canary_test.sh`: `pin_warn_literal` for the parse-failure
+arms, the INFO-level checks on `MARKER_CHECK_COMPLETE` and
+`MARKER_LATEST_SEEN`, the trigger-site COUNT pin, and the pin that
+`cmd_preflight` still arms the equality check. The gate's own WIRING — that the
+canary still runs, and still runs before `--draft=false` — is pinned by
+`scripts/release_canary_wiring_test.sh`.
+
 ## Self-satisfying `include_str!` source-scrape pins
 
 A source-scrape pin — a test that `include_str!`s its own crate's source
@@ -106,3 +204,112 @@ not known to work.
 ```bash
 grep -rn 'include_str!("' crates/core/src/bin/ | grep -v assets
 ```
+
+## SIGPIPE under `pipefail`: a present marker reads as absent
+
+In a script that sets `set -o pipefail`, piping a producer into a consumer that
+**short-circuits** — `grep -q`, `head -n`, `read` — makes the producer die with
+SIGPIPE (exit 141) as soon as the consumer stops reading. `pipefail` then
+promotes 141 to the pipeline's status. So:
+
+```bash
+set -uo pipefail
+if printf '%s' "$logs" | grep -qF "$MARKER"; then   # WRONG
+```
+
+reports **false for a marker that is present**. The `if` is not testing whether
+the marker is there; it is testing whether the producer finished writing.
+
+Three properties make this worse than an ordinary bug:
+
+**It is volume-dependent, so it is intermittent.** Below the 64 KB pipe buffer
+the producer finishes before `grep -q` exits and nothing happens. Above it, the
+producer is still writing and takes the signal. Every small fixture passes; the
+failure waits for a real input. Measured on #5236's canary, same log content
+with only trailing volume varied: 1 KB → `rc=0`; 200 KB → `rc=1` with a wrong
+diagnosis; a real 3.65 MB node log → `grep -acF` finds the line (count 1) while
+the piped `grep -q` exits **141** and the direct `grep -q` exits 0.
+
+**It corrupts the diagnosis, not just the verdict.** The gate blamed
+"the startup update check never ran" — pointing the next reader at auto-update
+detection when the fault was a shell pipeline. It hit 2 of 3 real preflight
+runs and `cmd_preflight` does not retry an rc=1, so a healthy release was
+blocked by an error about the wrong subsystem.
+
+**Framing decides whether it fires, invisibly.** Whether the consumer can
+short-circuit early depends on where the match falls and how the stream is
+split into lines, neither of which is visible at the call site. Same file, same
+consumer, match on line 1 of a 165 KB source:
+
+```bash
+sed 's/\\$//' "$AU" | grep -qF 'Auto-update'                # rc=141
+sed 's/\\$//' "$AU" | tr -d '[:space:]' | grep -qF '…'      # rc=0
+```
+
+The second is safe only because `tr` deletes every newline, leaving one line
+grep must read to EOF before it can report. `pin_marker` depended on that
+accident without knowing it — deleting the `tr` as a "simplification" would
+have armed the hazard on every source pin in the file at once.
+
+### Repeat offender history
+
+| Site | How it broke |
+|------|--------------|
+| `node_decided_to_update`, `node_check_settled` (`scripts/auto-update-canary.sh`) | Diagnosed and fixed when the mechanism was first found. Latent — canary logs never got large enough. |
+| Four checks in `assert_detection_healthy`, same file, same commit | NOT fixed by that pass, and not latent: Gate A's normal path leaves the node logging ~33 KB/s until it is killed seconds later, so the markers sit far behind the buffer. |
+| `pin_marker` (`scripts/auto-update-canary_test.sh`) | Safe only by accident of an intervening `tr -d '[:space:]'`, as above. |
+| `printf '%s' "$WRONG_OUT" \| grep -qF` (`scripts/auto-update-canary_lifecycle_test.sh`) | Latent; would have reported "the wrong diagnosis" for the right one. |
+
+### The rule
+
+**When you fix this, fix every instance in the repo, not the one you were
+reading.** That is the actual lesson of #5236: the same commit correctly
+diagnosed the mechanism, wrote the explanation down in a comment, fixed two
+helpers — and left four call sites inside the very function those helpers
+serve. Grep first, fix the set, then write the comment.
+
+Safe forms, in order of preference:
+
+- **Grep the file directly** rather than slurping it into a variable and piping
+  it: `grep -aqF -- "$needle" "$dir"/*.log`. No pipe, no producer to kill.
+- **Match the variable with a bash glob**: `[[ "$out" == *"$needle"* ]]`. Also
+  cheaper than forking grep.
+- **Take a count and test it**: `[ "$(grep -acF …)" -gt 0 ]` — the pipeline's
+  value is used, not its status.
+- **`|| true`** where the producer's status genuinely does not matter — but
+  prefer one of the above, since `|| true` also swallows real errors.
+
+`head` in a command substitution (`v="$(cmd | head -1)"`) is the same class but
+is normally fine: the value is used and the status discarded. It is only a
+hazard when something consumes the pipeline's status.
+
+### Audit
+
+```bash
+# which scripts are exposed at all
+grep -ln 'pipefail' scripts/*.sh
+
+# candidate sites; cross-reference against that list
+grep -rnE '\|[[:space:]]*(grep[[:space:]]+-[a-zA-Z]*q|head[[:space:]]|read[[:space:]])' scripts/
+```
+
+A hit matters when the pipeline's **status** is consumed — an `if`/`elif`,
+`&&`/`||`, a `while` condition, or a function whose last command it is. A hit
+whose stdout is captured and whose status is ignored is benign.
+
+The release-gate scripts that set `pipefail` are pinned against regression by
+`scripts/auto-update-canary_test.sh` ("no 'pipe into grep -q' …"), which fails
+if the form reappears in any of them. Regression tests in the same file drive
+>64 KB fixtures through `assert_detection_healthy` in both directions, since a
+small-fixture test cannot see this class at all.
+
+**The rest of `scripts/` is NOT pinned and has not been audited site by site.**
+As of #5236 the greps above return 20 scripts setting `pipefail` and ~69
+candidate sites across all of `scripts/`. Two that look worth a closer read
+when someone next touches those files, neither investigated here:
+`deploy-to-gateways.sh`'s health check pipes 100 journalctl lines into
+`grep -q` and consumes the status, and `deploy-local-gateway.sh` pipes
+`systemctl list-unit-files` (47 KB on one ordinary host, and it grows with the
+machine) into `grep -q` at five sites. Both would fail in the safe direction —
+reporting a healthy service as unhealthy — which is precisely the direction
+that survives unnoticed.

.github/workflows/ci.yml

@@ -185,14 +185,56 @@ jobs:
       - name: Self-test merge-queue concurrency guard
         run: bash scripts/merge_group_concurrency_test.sh
 
+      # Regression gate for the release driver's cross-compile wait: under
+      # `set -euo pipefail` a bare `var=$(gh ...)` aborts the whole driver on a
+      # transient GitHub blip, AFTER the release publishes but BEFORE the
+      # gateways are updated and the release is announced. Pins that a failing
+      # `gh` makes the poll loop retry, while a genuinely failed attach job and
+      # an unknown publish gate still stop the release loudly.
+      - name: Self-test release wait_for_binaries gh resilience
+        run: bash scripts/release_wait_for_binaries_test.sh
+
+      # Pins the WIRING of the blocking pre-flight canary, which is what
+      # makes it a gate at all: it must run inside attach-to-release, after
+      # the assets upload and BEFORE `gh release edit --draft=false`, and
+      # not be disabled in place. Also pins release.sh's ATTACH_JOB_NAME
+      # against the job's name in cross-compile.yml -- the driver polls for
+      # that string and a rename on either side is otherwise silent.
+      - name: Self-test release canary wiring
+        run: bash scripts/release_canary_wiring_test.sh
+
+      # Regression gate for #5221/#5222: auto-update was broken fleet-wide for
+      # TWO releases (v0.2.120, v0.2.121) and nothing noticed, because every
+      # signal was one-sided — the release built, published, installed and ran.
+      # scripts/auto-update-canary.sh is the release canary that catches it;
+      # this pins its two-sided assertion. The load-bearing cases are the
+      # VACUOUS ones: a log with no update check, and a node running with
+      # auto-update disabled, both contain no error line and would sail past a
+      # "grep for the error" check. Fixtures are verbatim log lines from real
+      # v0.2.119 (healthy) and v0.2.121 (broken) runs.
+      - name: Self-test auto-update release canary
+        run: bash scripts/auto-update-canary_test.sh
+
+      # Lifecycle half of the same canary: drives the real `run_node_until_check`
+      # and `cmd_preflight` against a FAKE node binary. The pure test above
+      # cannot see process management, and that is where the two real bugs were
+      # — a gate that failed even a healthy binary, and a node left running
+      # after the gate returned (holding its ports, starving the next attempt).
+      # Case 4 is the load-bearing one: reintroduce the leak and it fails.
+      - name: Self-test auto-update canary lifecycle
+        run: bash scripts/auto-update-canary_lifecycle_test.sh
+
       # install.sh now sets up a supervised service by default (issue #4073) so
       # new nodes auto-update. The system-vs-user + lingering decision is the
       # load-bearing new logic; these smoke tests pin it without needing real
       # root/sudo. shellcheck guards the installer/uninstaller scripts too.
       - name: Install shellcheck
         run: sudo apt-get update && sudo apt-get install -y shellcheck
+      # The canary scripts are linted here too. They were claimed shellcheck
+      # clean when they landed but were not actually in this list, so nothing
+      # held them to it. `-x` because the two test scripts `source` the canary.
       - name: Lint install/uninstall scripts (shellcheck)
-        run: shellcheck scripts/install.sh scripts/uninstall.sh scripts/test-install-sh.sh scripts/test-uninstall-sh.sh
+        run: shellcheck -x scripts/install.sh scripts/uninstall.sh scripts/test-install-sh.sh scripts/test-uninstall-sh.sh scripts/auto-update-canary.sh scripts/auto-update-canary_test.sh scripts/auto-update-canary_lifecycle_test.sh scripts/release_wait_for_binaries_test.sh scripts/release_canary_wiring_test.sh scripts/release.sh
       - name: Self-test install.sh service-mode decision
         run: sh scripts/test-install-sh.sh
       - name: Self-test uninstall.sh