From be7eaa81818f735a6eeec0f941e7f5a83cc3bbd0 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 13 Oct 2026 15:55:34 +0200 Subject: [PATCH 1/3] refresh-consumer: compare mtimes locale-independently so an unchanged secret doesn't restart its unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `refresh_unit` read the credential file's mtime with `stat -c %.6Y | tr -d .`. coreutils prints that fraction with the locale's decimal separator, so on a host whose locale uses a comma (muede-lpt2: `1791232888,193159`) `tr` stripped nothing, the `-le` against the unit's start errored, the "unchanged" early return was skipped, and every run try-restarted the consumer. Every unit on a store-refresh timer was restarted every ~10 minutes; for `swarm-bao-ci-runner` that is `gitea-runner-swarm`, which killed every CI job running at the time. The mtime is now read with `date -r … +%s%6N`, the same digit-only format the start time already used (`date -d … +%s%6N` on systemd's fixed English timestamp), so neither side has a separator for a locale to change. A value that still does not read as digits — an unparsable start time, a missing file — now restarts nothing and logs at `err`. Skipping on doubt rather than restarting on doubt: the file's mtime is kept, so a genuinely changed credential still restarts its consumer on the first run that can read both times, whereas restarting on doubt restarts it on every run, which is this bug. Both reads also fall back to empty instead of aborting the caller's `set -e` script. `script-test-refresh-consumer` runs the rendered helper under `de_DE.UTF-8` against a stub `systemctl`, and checks first that `stat` really prints a comma there. The helper on main fails 14 of its assertions. Fixes #5230 --- nix/checks.nix | 3 + nix/host-modules/lib/refresh-consumer.nix | 12 +- nix/script-tests/refresh-consumer.nix | 19 ++++ nix/script-tests/refresh-consumer.sh | 130 ++++++++++++++++++++++ 4 files changed, 162 insertions(+), 2 deletions(-) create mode 100644 nix/script-tests/refresh-consumer.nix create mode 100644 nix/script-tests/refresh-consumer.sh diff --git a/nix/checks.nix b/nix/checks.nix index bc04772b..6551916c 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -55,6 +55,9 @@ in inherit pkgs self nixosSystem; inherit (pkgs) lib; }; + # Runs ./host-modules/lib/refresh-consumer.nix's restart decision under a + # comma-decimal locale, against a stub `systemctl`. + script-test-refresh-consumer = import ./script-tests/refresh-consumer.nix { inherit pkgs; }; module-eval-nats-tls = import ./module-eval/nats-tls.nix { inherit pkgs self nixosSystem; inherit (pkgs) lib; diff --git a/nix/host-modules/lib/refresh-consumer.nix b/nix/host-modules/lib/refresh-consumer.nix index c27923df..5ba06fc0 100644 --- a/nix/host-modules/lib/refresh-consumer.nix +++ b/nix/host-modules/lib/refresh-consumer.nix @@ -46,9 +46,17 @@ # otherwise read as today's midnight. started="$(systemctl "$@" show --timestamp=us+utc -p ActiveEnterTimestamp --value "$unit")" if [ -n "$started" ]; then - started_us="$(date -u -d "$started" +%s%6N)" + started_us="$(date -u -d "$started" +%s%6N)" || started_us= + fi + written_us="$(date -u -r "$path" +%s%6N)" || written_us= + # Both are bare microsecond counts, with no decimal separator for a + # locale to change. One that is not is logged at `err` and nothing is + # restarted: the mtime stays, so the first run that can read both still + # restarts , where restarting on doubt restarts it on every run. + if [[ ! "$started_us" =~ ^[0-9]+$ || ! "$written_us" =~ ^[0-9]+$ ]]; then + echo "<3>cannot compare the mtime of $path ('$written_us') with the last start of $unit ('$started') — leaving $unit as it is" >&2 + return 0 fi - written_us="$(stat -c %.6Y "$path" | tr -d .)" if [ "$written_us" -le "$started_us" ]; then return 0 fi diff --git a/nix/script-tests/refresh-consumer.nix b/nix/script-tests/refresh-consumer.nix new file mode 100644 index 00000000..396efd55 --- /dev/null +++ b/nix/script-tests/refresh-consumer.nix @@ -0,0 +1,19 @@ +# `checks.script-test-refresh-consumer` — runs ../host-modules/lib/refresh-consumer.nix's +# `refresh_unit` and `refresh_consumer` against a stub `systemctl`, under a +# locale whose decimal separator is a comma. The cases are in +# ./refresh-consumer.sh. +# +# What runs is the rendered helper, sourced as its callers' scripts include it. +{ + pkgs, + ... +}: +pkgs.runCommand "hyperhive-script-test-refresh-consumer" + { + HELPER = pkgs.writeText "refresh-consumer.sh" (import ../host-modules/lib/refresh-consumer.nix { }); + LOCALE_ARCHIVE = "${pkgs.glibcLocales}/lib/locale/locale-archive"; + } + '' + ${pkgs.bash}/bin/bash ${./refresh-consumer.sh} + touch "$out" + '' diff --git a/nix/script-tests/refresh-consumer.sh b/nix/script-tests/refresh-consumer.sh new file mode 100644 index 00000000..68c665aa --- /dev/null +++ b/nix/script-tests/refresh-consumer.sh @@ -0,0 +1,130 @@ +# Cases for ./refresh-consumer.nix. Each case sets the credential file's mtime +# and the consumer's last start, runs the helper with a stub `systemctl` that +# records every call it is asked to act on, and compares that record and what +# the helper wrote to stderr. +set -uo pipefail + +failures=0 +count=0 + +fail() { + echo "FAILED: $case: $*" >&2 + failures=$((failures + 1)) +} + +dir=$(mktemp -d) +mkdir "$dir/bin" +calls=$dir/calls +path=$dir/token +: >"$path" + +# `show` prints $STARTED as systemd's `--timestamp=us+utc` does; `is-active` +# and `is-failed` answer from $ACTIVE and $FAILED; every other verb is +# appended to $calls. +cat >"$dir/bin/systemctl" <>"$calls" ;; +esac +STUB +chmod +x "$dir/bin/systemctl" +export PATH="$dir/bin:$PATH" ACTIVE=1 FAILED=0 STARTED= + +export LC_ALL=de_DE.UTF-8 + +# shellcheck source=/dev/null +. "$HELPER" + +# The locale the cases run under has to print the comma the helper must not +# depend on, or none of them tests it. +case="setup" +touch -d @1791232888.193159 "$path" +got=$(stat -c %.6Y "$path") +if [ "$got" != "1791232888,193159" ]; then fail "stat under $LC_ALL printed '$got', wanted 1791232888,193159"; fi + +# systemd's `--timestamp=us+utc` rendering of .. +systemd_ts() { + LC_ALL=C date -u -d "@$1" '+%a %Y-%m-%d %H:%M:%S.%6N UTC' +} + +# check +# +check() { + case=$1 + local want_calls=$2 want_err=$3 got err + shift 3 + count=$((count + 1)) + : >"$calls" + # `set -e` as the callers' NixOS `script`s run: a failed read must not end the + # caller. Not under `||` or `if`, which would switch `set -e` off. + local rc + ( + set -e + "$@" >/dev/null 2>"$dir/err" + ) + rc=$? + if [ "$rc" != 0 ]; then fail "exited $rc"; fi + err=$(cat "$dir/err") + got=$(cat "$calls") + if [ "$got" != "$want_calls" ]; then + fail "systemctl was asked:"$'\n'"$got"$'\n'"wanted:"$'\n'"$want_calls" + fi + # A failed read also leaves `date`'s own message ahead of the helper's line. + if [ -n "$want_err" ]; then err=$(printf '%s\n' "$err" | tail -n 1); fi + if [ "$err" != "$want_err" ]; then + fail "stderr:"$'\n'"$err"$'\n'"wanted:"$'\n'"$want_err" + fi +} + +touch -d @1791232888.193159 "$path" + +STARTED=$(systemd_ts 1791232900.000001) +check "a file older than the start restarts nothing" "" "" \ + refresh_unit gitea-runner-swarm.service "$path" +check "nor through a container" "" "" \ + refresh_consumer swarm-ci-runner gitea-runner-swarm.service "$path" + +STARTED=$(systemd_ts 1791232888.193159) +check "a file written in the start's microsecond restarts nothing" "" "" \ + refresh_unit gitea-runner-swarm.service "$path" + +STARTED=$(systemd_ts 1791232888.193158) +check "a file one microsecond newer than the start restarts the unit" \ + "try-restart --no-block gitea-runner-swarm.service" "" \ + refresh_unit gitea-runner-swarm.service "$path" +check "through a container, with --machine" \ + "--machine=swarm-ci-runner try-restart --no-block gitea-runner-swarm.service" "" \ + refresh_consumer swarm-ci-runner gitea-runner-swarm.service "$path" + +FAILED=1 +check "a failed unit is reset and started" \ + "reset-failed gitea-runner-swarm.service"$'\n'"start --no-block gitea-runner-swarm.service" "" \ + refresh_unit gitea-runner-swarm.service "$path" +FAILED=0 + +STARTED= +check "a unit never active is restarted" \ + "try-restart --no-block gitea-runner-swarm.service" "" \ + refresh_unit gitea-runner-swarm.service "$path" + +ACTIVE=0 +check "nothing while the container is down" "" "" \ + refresh_consumer swarm-ci-runner gitea-runner-swarm.service "$path" +ACTIVE=1 + +STARTED="not a time" +check "an unreadable start restarts nothing and says so at err" "" \ + "<3>cannot compare the mtime of $path ('1791232888193159') with the last start of gitea-runner-swarm.service ('not a time') — leaving gitea-runner-swarm.service as it is" \ + refresh_unit gitea-runner-swarm.service "$path" + +STARTED=$(systemd_ts 1791232900.000001) +check "a missing file restarts nothing and says so at err" "" \ + "<3>cannot compare the mtime of $dir/missing ('') with the last start of gitea-runner-swarm.service ('$STARTED') — leaving gitea-runner-swarm.service as it is" \ + refresh_unit gitea-runner-swarm.service "$dir/missing" + +echo "$count cases, $failures failed" +[ "$failures" = 0 ] From 8d554477caf393a42176a1c1978ba6b8f86ccffa Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 13 Oct 2026 16:00:37 +0200 Subject: [PATCH 2/3] swarm-bao-agent-pki: decide openbao's restart on the shared locale-independent compare `swarm-bao-agent-pki` carried its own copy of the mtime-vs-start compare, with the same `stat -c %.6Y | tr -d .` read: on a comma-locale host the `-le` errored and, under `seal = "pkcs11"`, openbao was restarted on every run of the unit. The compare moves out of `refresh_unit` into `written_after_start` (0 newer, 1 not newer, 2 unreadable and logged at `err`), and both `refresh_unit` and the agent-pki unit call it. The agent-pki unit cannot call `refresh_unit` itself: it restarts with `restart`, not `try-restart`, runs a fail-closed check of the bundle between the compare and the restart, and under shamir prints the step instead of restarting. It acts only on 0, so an unreadable time skips the restart as `refresh_unit` does. `script-test-refresh-consumer` covers the three return codes in the agent-pki call shape; `module-eval-bao-basics` asserts both seal arms decide on that call and that no `%.6Y` read is left in the unit. Refs #5230 --- nix/host-modules/lib/refresh-consumer.nix | 38 ++++++++++++++--------- nix/host-modules/swarm-bao.nix | 10 +++--- nix/module-eval/bao-basics.nix | 14 +++++++-- nix/script-tests/refresh-consumer.nix | 6 ++-- nix/script-tests/refresh-consumer.sh | 23 ++++++++++++++ 5 files changed, 67 insertions(+), 24 deletions(-) diff --git a/nix/host-modules/lib/refresh-consumer.nix b/nix/host-modules/lib/refresh-consumer.nix index 5ba06fc0..8f74567e 100644 --- a/nix/host-modules/lib/refresh-consumer.nix +++ b/nix/host-modules/lib/refresh-consumer.nix @@ -31,15 +31,15 @@ [ ! -f "$1" ] || [ "$(< "$1")" != "$2" ] } - # [systemctl option…]. Nothing when last entered - # `active` after was last written. Otherwise a running is - # restarted and a failed one — it may have hit its start limit without the - # credential — is reset and started; one stopped on purpose stays stopped. - # The options are passed to every `systemctl` call, which is how - # `refresh_consumer` reaches into a container. `--no-block` because a - # caller ordered `Before=` its consumer must not wait on a job that waits - # on the caller (see `swarm-services-cert`'s propagation in ../hive-tls.nix). - refresh_unit() { + # [systemctl option…]. Returns 0 when was last + # written after last entered `active` (or never has), 1 when + # it was not, and 2, logged at `err`, when either time does not read. Both + # are bare microsecond counts, with no decimal separator for a locale to + # change. A caller acts only on 0: the mtime stays, so the first run that + # can read both still acts, where acting on doubt acts on every run. The + # options are passed to `systemctl`, which is how a caller reaches into a + # container. + written_after_start() { local unit="$1" path="$2" started started_us=0 written_us shift 2 # Empty for a unit that has never been active, which `date` would @@ -49,17 +49,25 @@ started_us="$(date -u -d "$started" +%s%6N)" || started_us= fi written_us="$(date -u -r "$path" +%s%6N)" || written_us= - # Both are bare microsecond counts, with no decimal separator for a - # locale to change. One that is not is logged at `err` and nothing is - # restarted: the mtime stays, so the first run that can read both still - # restarts , where restarting on doubt restarts it on every run. if [[ ! "$started_us" =~ ^[0-9]+$ || ! "$written_us" =~ ^[0-9]+$ ]]; then echo "<3>cannot compare the mtime of $path ('$written_us') with the last start of $unit ('$started') — leaving $unit as it is" >&2 - return 0 + return 2 fi if [ "$written_us" -le "$started_us" ]; then - return 0 + return 1 fi + } + + # [systemctl option…]. Nothing unless `written_after_start`. + # Otherwise a running is restarted and a failed one — it may have + # hit its start limit without the credential — is reset and started; one + # stopped on purpose stays stopped. `--no-block` because a caller ordered + # `Before=` its consumer must not wait on a job that waits on the caller + # (see `swarm-services-cert`'s propagation in ../hive-tls.nix). + refresh_unit() { + local unit="$1" path="$2" + shift 2 + written_after_start "$unit" "$path" "$@" || return 0 if systemctl "$@" is-failed --quiet "$unit"; then echo "$path changed after $unit last started, and $unit had failed — starting it" systemctl "$@" reset-failed "$unit" diff --git a/nix/host-modules/swarm-bao.nix b/nix/host-modules/swarm-bao.nix index 5fd0df35..db792c8b 100644 --- a/nix/host-modules/swarm-bao.nix +++ b/nix/host-modules/swarm-bao.nix @@ -3429,6 +3429,7 @@ in echo "wrote ${agentCaCachePath}" fi + ${refreshConsumer} ${composeListenerBundle} compose_listener_bundle @@ -3437,11 +3438,12 @@ in echo "openbao in $machine is not running; it reads ${listenerClientCaPath} when it starts" exit 0 fi - started="$(systemctl --machine="$machine" show --timestamp=us+utc -p ActiveEnterTimestamp --value openbao.service)" - started_us="$(date -u -d "$started" +%s%6N)" - written_us="$(stat -c %.6Y ${listenerClientCaPath} | tr -d .)" - if [ "$written_us" -le "$started_us" ]; then + newer=0 + written_after_start openbao.service ${listenerClientCaPath} --machine="$machine" || newer=$? + if [ "$newer" = 1 ]; then echo "openbao started after ${listenerClientCaPath} last changed; nothing to pick up" + fi + if [ "$newer" != 0 ]; then exit 0 fi diff --git a/nix/module-eval/bao-basics.nix b/nix/module-eval/bao-basics.nix index a1d74de4..c4614e67 100644 --- a/nix/module-eval/bao-basics.nix +++ b/nix/module-eval/bao-basics.nix @@ -269,16 +269,26 @@ let { # openbao reads its client-CA file only at start, so picking up the # agent CA is a restart: automatic where the store unseals itself, - # printed where a human has to. The shamir arm is the control. + # printed where a human has to. The shamir arm is the control. Both + # decide on the shared locale-independent compare + # (../host-modules/lib/refresh-consumer.nix, run by + # `script-test-refresh-consumer`) and act only on its 0. name = "the agent PKI unit restarts openbao under pkcs11 and only prints the step under shamir"; ok = let restart = ''systemctl --machine="$machine" restart openbao.service''; + compare = '' + written_after_start openbao.service ${tlsDir}/listener-client-ca.pem --machine="$machine" || newer=$? + ''; p = baoPkcs11.systemd.services.swarm-bao-agent-pki.script; sh = baoShamir.systemd.services.swarm-bao-agent-pki.script; in lib.hasInfix restart p - && lib.hasInfix ''if [ "$written_us" -le "$started_us" ]; then'' p + && lib.all (s: lib.hasInfix compare s && lib.hasInfix ''if [ "$newer" != 0 ]; then'' s) [ + p + sh + ] + && !(lib.hasInfix "%.6Y" p) && !(lib.hasInfix restart sh) && lib.hasInfix "# then unseal" sh; } diff --git a/nix/script-tests/refresh-consumer.nix b/nix/script-tests/refresh-consumer.nix index 396efd55..601ced46 100644 --- a/nix/script-tests/refresh-consumer.nix +++ b/nix/script-tests/refresh-consumer.nix @@ -1,7 +1,7 @@ # `checks.script-test-refresh-consumer` — runs ../host-modules/lib/refresh-consumer.nix's -# `refresh_unit` and `refresh_consumer` against a stub `systemctl`, under a -# locale whose decimal separator is a comma. The cases are in -# ./refresh-consumer.sh. +# `written_after_start`, `refresh_unit` and `refresh_consumer` against a stub +# `systemctl`, under a locale whose decimal separator is a comma. The cases +# are in ./refresh-consumer.sh. # # What runs is the rendered helper, sourced as its callers' scripts include it. { diff --git a/nix/script-tests/refresh-consumer.sh b/nix/script-tests/refresh-consumer.sh index 68c665aa..55b55293 100644 --- a/nix/script-tests/refresh-consumer.sh +++ b/nix/script-tests/refresh-consumer.sh @@ -126,5 +126,28 @@ check "a missing file restarts nothing and says so at err" "" \ "<3>cannot compare the mtime of $dir/missing ('') with the last start of gitea-runner-swarm.service ('$STARTED') — leaving gitea-runner-swarm.service as it is" \ refresh_unit gitea-runner-swarm.service "$dir/missing" +# compare . `written_after_start` as +# `swarm-bao-agent-pki` calls it, which acts only on 0. +compare() { + case=$1 + count=$((count + 1)) + local rc + ( + set -e + written_after_start openbao.service "$3" --machine=swarm-bao 2>/dev/null + ) + rc=$? + if [ "$rc" != "$2" ]; then fail "returned $rc, wanted $2"; fi +} + +STARTED=$(systemd_ts 1791232900.000001) +compare "the compare is 1 for a file older than the start" 1 "$path" +STARTED=$(systemd_ts 1791232888.193158) +compare "the compare is 0 for a file newer than the start" 0 "$path" +STARTED="not a time" +compare "the compare is 2 for an unreadable start" 2 "$path" +STARTED=$(systemd_ts 1791232900.000001) +compare "the compare is 2 for a missing file" 2 "$dir/missing" + echo "$count cases, $failures failed" [ "$failures" = 0 ] From be9b2475a9c4c9ffed144aa47de98fa52fce120e Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 13 Oct 2026 16:36:24 +0200 Subject: [PATCH 3/3] docs: say what refresh-consumer does when it cannot read a timestamp --- docs/swarm/credentials.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/swarm/credentials.md b/docs/swarm/credentials.md index a1aaf9b7..cfee085e 100644 --- a/docs/swarm/credentials.md +++ b/docs/swarm/credentials.md @@ -174,6 +174,7 @@ single attempt, not for the retries: an attempt that fails ends that start, so the dependent starts with whatever is already on disk. When a later attempt lands a changed value, the homeserver, Grafana and collector readers restart the service that loaded it (`nix/host-modules/lib/refresh-consumer.nix`). +When the fetch unit can't read the credential file's mtime or the service's start time, it logs `cannot compare the mtime of … — leaving as it is` at `err` in its own journal, and the service keeps running on the old value until the next timer tick. ## Progressive enhancement