Capture prior OS state before tuning, and restore it exactly on revert - #24
Merged
Conversation
Closes #23. 10-os-tune.sh changed privileged machine state and wrote three files under /etc. 19-os-revert.sh "undid" that by writing fixed defaults -- THP=madvise, governor=schedutil, power at maximum, persistence off -- and by `rm -f`-ing three paths it had never proven it owned. On a machine that already had a governor policy, a THP setting, or its own 99-llm-inference.conf, that was not a rollback. It was a second round of configuration wearing a rollback's clothes, and in the file case a deletion of somebody else's work. The README promised the tuning was reversible; it was re-settable, which is a different claim. THE MODEL, in lib/ostune.sh Before the first mutation of a setting, its effective prior value is captured to /var/lib/llm-rig/os-tune.state -- root-owned, 0600, in a 0700 directory. 19-os-revert.sh restores from that file and from nothing else. Capture is append-once. A second tune must not re-record the values llm-rig itself set as the ones to restore, which is precisely how a rollback becomes a silent no-op. Capture is per setting and immediately precedes the change, so a crash halfway through leaves an exact record of what has been changed so far. A partial run is fully revertible, and there is a test that proves it by making step 5 refuse and then reverting steps 1-4. The power-limit case is worth naming. TUNING.md concludes 100% is right for this chassis. Someone who read that, disagreed, and capped their card at 120W had the decision quietly undone by a revert that "restored" the maximum -- it raised a limit that had been deliberately lowered. Per-CPU governors are captured and restored individually. A machine running different governors on different cores was being flattened to one value. An unreadable prior value is recorded as `unknown` and never "restored". Writing madvise because THP could not be read is a new decision, not a rollback. FILES A file that existed before llm-rig is backed up byte for byte, restored byte for byte, and never destroyed. If it cannot be backed up, it is not overwritten -- the tune refuses and says why. Existence is checked separately from hashing, because a path that exists and cannot be read is the moment we know least about it. An unreadable file, a directory, or a dangling symlink now refuses; previously the failed hash read as "absent" and the path was overwritten. -L as well as -e: -e follows symlinks and is false for a dangling one, which is exactly the case where a write replaces somebody's link with a regular file. A file we wrote is deleted only if it still holds what we wrote. An edit is a claim of ownership. Ownership recorded but never proven -- the hash is `unknown` because the run died between recording and hashing -- also blocks the delete. Unproven is treated as not ours. PLAN MODE Both scripts take --dry-run. It prints every intended mutation with its current value, marks the ones already correct, warns when a file of yours would be adopted, and uses no sudo at all. TESTS 35 new, in tests/cases/ostune_test.sh. OSTUNE_ROOT points at a sandbox and OSTUNE_SUDO is empty, so the scripts run their real code paths -- writing, hashing, backing up, restoring, refusing -- against a fake root. That is deliberately not mocked: a mock that always succeeds cannot show that a file came back byte for byte. Two mocks earn their keep here. cpupower actually writes the sandbox governor files, because a stub that only records the call cannot demonstrate that a revert put the previous governor back. And the dangling-symlink test uses the real ln rather than the mock on PATH, which would have created nothing and left the assertion measuring the stub. The backup-failure case is a dangling symlink rather than a chmod 000 file, because tests/isolated.sh runs inside unshare --map-root-user, where permission bits do not apply and an unreadable file is readable. 412 tests across 13 suites, ordinary and network-isolated both green. README and TUNING.md now state what the guarantee actually is, including a table of what the old revert wrote versus what it should have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ShellCheck at --severity=warning flags SC2034 for the six variables lib/ostune.sh defines for 10-os-tune.sh and 19-os-revert.sh to read: the three /etc paths, the two THP paths, and OSTUNE_LAST_NOTE. Same convention as the rest of lib/ -- a directive at the assignment, naming it a documented return channel. OSTUNE_LAST_NOTE gets one directive scoped to the whole of ostune_restore_file rather than three copies inside it, matching what fdb0a2b did in the selector. No behaviour change. CI's fixture suites were already green; this is the lint half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rammsguns
added a commit
that referenced
this pull request
Aug 13, 2026
One conflict, in README.md, and it was an insertion collision rather than a disagreement: this branch adds "Rating the models you serve" immediately before "## Configuration", and #25 adds "The llama-swap binary is pinned and verified" in the same place. Neither edits the other's text. Both sections are kept, ratings first. The ratings section continues the catalog thread the surrounding prose is already on -- it opens by answering "a quarter of the score is a neutral placeholder", which is the sentence two sections above it -- and the llama-swap section is about 40-serve.sh, so it reads as the last thing before Configuration rather than an interruption. Nothing was dropped from either side. Everything else merged clean, including the two places most likely to have collided: catalog_ratings() in lib/catalog.sh, where #27 added two Laguna rows carrying unknown ratings while this branch changes how ratings are produced, and tests/cases/catalog_test.sh.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #23.
The rollback now restores the state this machine was actually in, rather than a
set of defaults someone assumed. New library
lib/ostune.sh;10-os-tune.shand
19-os-revert.shrewritten around it.What was wrong
19-os-revert.shwrote fixed values —THP=madvise,governor=schedutil,power at maximum, persistence off — and
rm -f'd three paths under/etcithad never proven it owned. On a machine with a governor policy, a THP setting,
or its own
99-llm-inference.conf, that was not a rollback but a second roundof configuration, and in the file case a deletion.
The one worth naming: restoring the maximum power limit raises a cap someone
deliberately lowered. TUNING.md argues 100% is right for this chassis; a
reader who disagreed and capped their card at 120W had that undone by a script
advertised as an undo.
Acceptance criteria
ostune_state_putimmediately precedes every change in10-os-tune.shostune_file_status/ostune_install_file— backed up byte for byte, or refused/var/lib/llm-rig/os-tune.state,0600, in a0700directory19-os-revert.shreads only the state file; a second run reports "nothing to revert" and exits 0--dry-run/ plan mode, no sudotests/cases/ostune_test.sh, 35 testsDesign decisions worth disagreeing with
Capture is append-once. A second tune must not re-record llm-rig's own
values as the ones to restore — that is exactly how a rollback becomes a
silent no-op. The cost is that a genuine change of prior state between two
tunes is not noticed; the first capture wins.
Adopt-and-restore, not refuse, for pre-existing files. The issue allowed
either. Refusing would block the tune on any machine with its own sysctl file,
which is common enough that people would work around it by deleting theirs.
Backing up byte for byte and restoring byte for byte keeps the tune usable and
the guarantee intact. Fail-closed still applies: if the backup cannot be made,
nothing is written.
Unproven ownership blocks deletion. Ownership is recorded before the write,
with the hash filled in after. A crash in between leaves
unknown, and revertthen refuses to delete. Safe direction, mildly annoying outcome: an orphaned
file the user must remove by hand, with a message saying so.
Per-CPU governors are captured and restored individually, because a machine
can legitimately run different governors on different cores and flattening them
to one value is a change, not a restoration.
An unreadable prior value is
unknownand is never "restored". Writingmadvisebecause THP could not be read is a new decision.A real bug the tests found
Existence was being inferred from hashability, so a path that exists but cannot
be hashed — unreadable file, directory, dangling symlink — read as absent and
was overwritten. That is the moment we know least about a path. It now refuses.
-Las well as-e, since-efollows symlinks and is false for a danglingone: precisely the case where a write replaces a link with a regular file.
Tests
35 new, 412 across 13 suites, ordinary and network-isolated both green.
OSTUNE_ROOTpoints at a sandbox andOSTUNE_SUDOis empty, so the scripts runtheir real code paths — writing, hashing, backing up, restoring, refusing —
against a fake root. Deliberately not mocked: a mock that always succeeds cannot
show that a file came back byte for byte.
Two testing notes:
cpupowermock now actually writes the sandbox governor files. A stubthat only records the call cannot demonstrate that a revert put the previous
governor back.
chmod 000file, because
tests/isolated.shruns insideunshare --map-root-userwherepermission bits do not apply and an unreadable file is readable. It also uses
the real
lnrather than the mock onPATH, which would have created nothingand left the assertion measuring the stub.
Not done
No run against a real
/etcor real GPU is included — this host has not had10-os-tune.shapplied, and doing so to produce a transcript would changemachine state to demonstrate a change of machine state. The sandbox exercises
every branch; the first privileged run is still the first privileged run, and
--dry-runexists so it can be inspected before it happens.🤖 Generated with Claude Code