diff --git a/bin/prod/usb-ready b/bin/prod/usb-ready index 54cc410..b70fb67 100755 --- a/bin/prod/usb-ready +++ b/bin/prod/usb-ready @@ -36,8 +36,36 @@ fi state=$(systemctl show -p ActiveState --value "$UNIT" 2>/dev/null || echo "unknown") cond=$(systemctl show -p ConditionResult --value "$UNIT" 2>/dev/null || echo "unknown") +# The job, if systemd still has one for this unit. This is the only property +# that separates "queued, about to run" from "dispatched and skipped": +# +# case ActiveState ConditionResult Job +# skipped by its condition inactive no (empty) +# queued, not started yet inactive no 12561 +# ExecStart running activating yes 12561 +# finished active yes (empty) +# +# Measured on systemd 257 (Debian 13, the board's own) and 237 - the first two +# rows are identical in every other property, including ConditionTimestamp and +# ExecMainStartTimestamp, which are 0 for both. +# +# That mattered: reading only ActiveState and ConditionResult, this said "true" +# in the queued window, Reflash mounted /dev/sda2 while expand-usb's mkfs was +# still running inside ssh-keygen-boot, and the mount failed with "bad +# superblock" - the filesystem did not exist yet. Storage was marked FAILED and +# no host keys were written, so the board came up with no sshd (#147). +job=$(systemctl show -p Job --value "$UNIT" 2>/dev/null | awk '{print $1}') case "$state" in active|failed) echo "true" ;; # ran to completion - *) [ "$cond" = "no" ] && echo "true" || echo "false" ;; # skipped, or not yet + activating) echo "false" ;; # running right now + *) + if [ -n "$job" ]; then + echo "false" # queued: it has not run yet + elif [ "$cond" = "no" ]; then + echo "true" # dispatched and skipped + else + echo "false" + fi + ;; esac diff --git a/test/bats/getters.bats b/test/bats/getters.bats index f8d687f..277325f 100644 --- a/test/bats/getters.bats +++ b/test/bats/getters.bats @@ -45,7 +45,26 @@ teardown() { teardown_sandbox; } # Reflash holds /mnt/usb for the life of the process, so it waits for whatever # needs the drive read-write to finish rather than racing it. -# systemctl show -p X --value is called twice: ActiveState then ConditionResult. +# systemctl show -p X --value, for the three properties usb-ready reads: +# ActiveState, ConditionResult and Job. +# +# WHAT A BOARD ACTUALLY REPORTS. Stub these from the table, not from what seems +# reasonable - the previous version of this helper guessed ConditionResult=yes +# for a unit that had not started, systemd says "no", and the test passed for +# years while the real case failed (#147): +# +# case ActiveState ConditionResult Job +# skipped by its condition inactive no (empty) +# queued, not started yet inactive no 12561 +# ExecStart running activating yes 12561 +# finished active yes (empty) +# +# Measured on systemd 257 (Debian 13, what the board runs) and 237, with a +# probe unit of the same shape as ssh-keygen-boot: Type=oneshot, +# RemainAfterExit=yes, a ConditionPathExists that passes or fails. The first two +# rows differ in Job and in nothing else - ConditionTimestamp and +# ExecMainStartTimestamp are 0 for both - which is why Job is the property +# usb-ready has to read. stub_unit() { cat > "$SHIMDIR/systemctl" <> "$CALLS" case "\$*" in *ConditionResult*) echo "${2:-yes}" ;; *ActiveState*) echo "${1:-inactive}" ;; + *Job*) echo "${3:-}" ;; esac EOF chmod +x "$SHIMDIR/systemctl" @@ -74,10 +94,20 @@ with_partition() { } # The race this closes: a oneshot reads "inactive" before it starts as well as -# after a Condition skips it. Mounting in that gap let the unit steal the mount. +# after a Condition skips it, and ConditionResult is "no" in BOTH - this test +# used to stub it as "yes", which is not what systemd reports in that window, so +# it passed while the real case failed (#147). Only the queued job tells them +# apart. @test "usb-ready: false while the owning unit has not started yet" { with_partition - stub_unit inactive yes + stub_unit inactive no 12561 + run "$PROD_BIN/usb-ready" + [ "$output" = "false" ] +} + +@test "usb-ready: false while the owning unit is queued behind something else" { + with_partition + stub_unit inactive yes 12561 run "$PROD_BIN/usb-ready" [ "$output" = "false" ] } @@ -98,9 +128,10 @@ with_partition() { [ "$output" = "true" ] } +# Skipped means dispatched and decided: no job is left for it. @test "usb-ready: true when the owning unit was skipped by its condition" { with_partition - stub_unit inactive no + stub_unit inactive no "" run "$PROD_BIN/usb-ready" [ "$output" = "true" ] }