fix(oom-staged): a re-run whose rebuild fails restored a STALE backup over /etc/nixos - #1459
fix(oom-staged): a re-run whose rebuild fails restored a STALE backup over /etc/nixos#1459ZacxDev wants to merge 1 commit into
Conversation
… over /etc/nixos MEASURED DATA LOSS in the staged script #1415 merged. Found by the round-2 delta audit of that PR, reproduced by executing the script rather than reading it, and it is a defect the round-1 fix pass INTRODUCED. THE DEFECT `BACKUP="${CFG}.bak.tmux-oom"` is a FIXED name (correctly — the timestamped spelling it replaced added one file per run to a directory already holding 18). But the backup is only taken on the branch that WIRES the import, while `trap restore ERR` covers the whole script including the closing `nixos-rebuild switch`. And `restore()` asked only whether the backup FILE EXISTS. So on a re-run — which takes the already-wired path and creates no backup — any rebuild failure, for any unrelated reason, found the PREVIOUS run's file and cp'd it over the live config: run 1 wires the import, writes $BACKUP, rebuild OK, $BACKUP survives operator hand-edits /etc/nixos/configuration.nix run 2 already wired -> no backup taken; rebuild fails -> ERR trap "FAILED - restoring ... from ...bak.tmux-oom" import present: 0 <- the live, correctly-applied import GONE operator edit present: 0 <- an unrelated edit GONE config byte-identical to the pre-wiring backup: YES `cp -a` restores the old mtime as well, so there is no tell. The message says "restoring", which reads as correct unwinding. WHY THE EXISTING TEST COULD NOT SEE IT `test_restore_does_not_claim_to_restore_a_backup_that_does_not_exist` asserted the literal `-f "$BACKUP"` appeared inside `restore()`. That is a guard on the SOURCE SPELLING, and the spelling it pinned is exactly what makes the bug fire: `-f` is TRUE precisely because a stale backup exists. It read as coverage while providing none. THE FIX * `restore()` gates on BACKUP_TAKEN_THIS_RUN, not on file existence. A run that changed nothing restores nothing. * the backup moves to the one branch that actually writes $CFG, immediately before the write. Taken earlier, a refusal that modified nothing still deposited a .bak file — which then armed the stale restore on the next run. TESTS: BEHAVIOURAL, because the structural form is what missed this Three tests execute the real script with only its environment couplings replaced (paths, the $EUID test, the selector pre-flight, nixos-rebuild); the trap, the backup and every branch are untouched. Every substitution is asserted to have applied, so a harness that patched nothing cannot report a pass. * a re-run whose rebuild fails must NOT restore a stale backup * POSITIVE CONTROL: a FIRST run whose rebuild fails MUST still roll back * a refusal that modifies nothing leaves no backup behind MUTATION: 3 mutants, 3 killed, each by its own named assertion, control green. M1 restore() back to a bare `-f "$BACKUP"` KILLED ("RESTORED A STALE BACKUP ... destroyed an unrelated operator edit") and the positive control still PASSED under M1, so the new test discriminates rather than merely asserting restore never runs. M2 backup moved back above the awk KILLED ("a refusal that modified nothing left a backup behind") M3 BACKUP_TAKEN_THIS_RUN pinned to 0 KILLED ("a FAILED first run must restore the config it modified") M2 SURVIVED on the first sweep, and that was a real finding about the TEST, not noise: the fixture used a config with no `imports =` at all, which exits at the count guard ABOVE both the backup and the awk — so early-cp and late-cp behave identically and the guarded branch never executes. The fixture now uses `imports =` present exactly once with no `[` after it, which is the only shape that reaches the awk-found-nothing refusal. Also recorded: an earlier M2 attempt scored SURVIVED without running at all, because the mutating `python3` was not on PATH and the failure was not read; the sweep now verifies the mutant differs from the original before scoring it. The module docstring claimed this script's behaviour "is not reachable from the suite". Its privileged EFFECT is not; its CONTROL FLOW is, and that claim is what let a spelled guard stand in for a real one. Corrected in the same commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013jdbmhCKa6edhTmiADsziR Claude-Session-Id: 097b404c-db17-4472-bd37-dc90cf8fa675
|
Closing in favour of #1464, which deletes the script this PR fixes. Operator decision after a round-0 (requirements & deletion) audit of this PR. The fix here What decided it:
The counter-argument is recorded in #1464's body rather than dropped, because it is real and Two findings from this PR's own review that die with it, noted so they are not re-derived: the The measured cause of the actual incident is fixed and live on both hosts — |
fix(oom-staged): a re-run whose rebuild fails restored a STALE backup over /etc/nixos
MEASURED DATA LOSS in the staged script #1415 merged. Found by the round-2
delta audit of that PR, reproduced by executing the script rather than reading
it, and it is a defect the round-1 fix pass INTRODUCED.
THE DEFECT
BACKUP="${CFG}.bak.tmux-oom"is a FIXED name (correctly — the timestampedspelling it replaced added one file per run to a directory already holding 18).
But the backup is only taken on the branch that WIRES the import, while
trap restore ERRcovers the whole script including the closingnixos-rebuild switch. Andrestore()asked only whether the backup FILEEXISTS.
So on a re-run — which takes the already-wired path and creates no backup — any
rebuild failure, for any unrelated reason, found the PREVIOUS run's file and
cp'd it over the live config:
run 1 wires the import, writes $BACKUP, rebuild OK, $BACKUP survives
operator hand-edits /etc/nixos/configuration.nix
run 2 already wired -> no backup taken; rebuild fails -> ERR trap
"FAILED - restoring ... from ...bak.tmux-oom"
import present: 0 <- the live, correctly-applied import GONE
operator edit present: 0 <- an unrelated edit GONE
config byte-identical to the pre-wiring backup: YES
cp -arestores the old mtime as well, so there is no tell. The message says"restoring", which reads as correct unwinding.
WHY THE EXISTING TEST COULD NOT SEE IT
test_restore_does_not_claim_to_restore_a_backup_that_does_not_existassertedthe literal
-f "$BACKUP"appeared insiderestore(). That is a guard on theSOURCE SPELLING, and the spelling it pinned is exactly what makes the bug fire:
-fis TRUE precisely because a stale backup exists. It read as coverage whileproviding none.
THE FIX
restore()gates on BACKUP_TAKEN_THIS_RUN, not on file existence. A runthat changed nothing restores nothing.
before the write. Taken earlier, a refusal that modified nothing still
deposited a .bak file — which then armed the stale restore on the next run.
TESTS: BEHAVIOURAL, because the structural form is what missed this
Three tests execute the real script with only its environment couplings
replaced (paths, the $EUID test, the selector pre-flight, nixos-rebuild); the
trap, the backup and every branch are untouched. Every substitution is asserted
to have applied, so a harness that patched nothing cannot report a pass.
MUTATION: 3 mutants, 3 killed, each by its own named assertion, control green.
M1 restore() back to a bare
-f "$BACKUP"KILLED ("RESTORED A STALEBACKUP ... destroyed an unrelated operator edit")
and the positive control still PASSED under M1, so the new test
discriminates rather than merely asserting restore never runs.
M2 backup moved back above the awk KILLED ("a refusal that
modified nothing left a backup behind")
M3 BACKUP_TAKEN_THIS_RUN pinned to 0 KILLED ("a FAILED first run
must restore the config it modified")
M2 SURVIVED on the first sweep, and that was a real finding about the TEST, not
noise: the fixture used a config with no
imports =at all, which exits at thecount guard ABOVE both the backup and the awk — so early-cp and late-cp behave
identically and the guarded branch never executes. The fixture now uses
imports =present exactly once with no[after it, which is the only shapethat reaches the awk-found-nothing refusal. Also recorded: an earlier M2 attempt
scored SURVIVED without running at all, because the mutating
python3was noton PATH and the failure was not read; the sweep now verifies the mutant differs
from the original before scoring it.
The module docstring claimed this script's behaviour "is not reachable from the
suite". Its privileged EFFECT is not; its CONTROL FLOW is, and that claim is
what let a spelled guard stand in for a real one. Corrected in the same commit.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_013jdbmhCKa6edhTmiADsziR