service: install must be safe to re-run - #80
Merged
Conversation
Two defects, both live on main, both found while planning `fabric update`.
That command re-renders the unit on every run, so each of these would have
gone from rare to constant.
ONE. A RE-INSTALL SILENTLY DROPPED A CONFIGURED MEMORY CEILING.
`allow_shell` and `allow_exec` survive a re-install because
`resolve_allow_shell` and `resolve_allow_exec` read them back out of
`config.toml` when the option is `None`. `memory_max_mb` had no such path.
It lived only in the rendered plist or unit, and `render_*` emits it only
when `Some`, so ANY install that did not name it removed a ceiling an
operator had set earlier, without saying anything.
It is now persisted beside the other two, with `resolve_memory_max_mb` in
the same shape as its siblings.
The option becomes a real tri-state, because a ceiling is itself optional:
nothing said keeps what is persisted, `Some(None)` clears it, `Some(Some)`
sets it. Persisting a value with no way to remove it would have made the
first ceiling anybody set permanent, so `--no-memory-max-mb` is added
alongside, mirroring `--no-allow-shell`.
The install summary now prints the RESOLVED ceiling rather than what the
caller passed. Those differ exactly when a persisted value was kept, which
is the case the line exists to show.
TWO. THE LINUX RESTART KILLED ITS OWN CALLER.
`install_systemd_user` ended with `systemctl --user restart fabric.service`.
`fabric exec` runs its session inside `fabric.service`'s cgroup, so
`fabric exec hetz -- fabric service install` restarts the cgroup the caller
lives in and dies partway through. Reachable today with a command any of us
might run.
The restart is now handed to systemd to run on its own, outside the
caller's cgroup, three seconds out so the caller returns first. No `--unit`
name is passed on purpose: systemd names the transient unit itself, so two
updates close together cannot collide on a name that already exists.
The install now says `restart scheduled`, because that is what it is.
Claiming a restart that has not happened would make a failed start look
like a successful install.
BOTH TESTS WERE WATCHED FAILING, against a stub that reproduced the real
behaviour rather than against nothing:
a re-install that never mentioned the ceiling removed it
left: None, right: Some(512)
the restart is issued in place, so it kills its own caller
`SERVICE_NAME` loses its `cfg(target_os = "linux")` gate so the restart
argv and its test build everywhere. A Linux-only string cannot be tested
from a Mac, and this is the shape whose regression takes a remote machine
down.
The restart test pins the COMMAND SHAPE, not the effect. The failure mode
is that the caller dies, so a test that waited for the effect would be the
thing that got killed.
Agent: Silber.fabric
myobie
added a commit
that referenced
this pull request
Aug 25, 2026
`schedule_supervisor` built its command inline inside a `cfg(target_os = "linux")` block, so the one shape whose regression leaves a Linux machine down with no way back in could only be checked on the platform where it does the damage. The argv is now built by `supervisor_argv`, which is not cfg-gated, and a test pins it: scheduled rather than inline, invoking the supervising subcommand, and told what to restore. IT ALSO PINS THAT THE SUPERVISOR IS THE ROLLBACK BINARY. Asking a possibly-broken new binary to supervise its own installation is not supervision, and that is the sort of detail a later refactor simplifies away without noticing. I mutated it to run inline and watched it fail with "the supervisor would die with the cgroup it is meant to outlive". Same reasoning as `systemd_restart_argv` in #80. A Linux-only string cannot be tested from a Mac, and these two are the strings that strand a remote machine. Agent: Silber.fabric
myobie
added a commit
that referenced
this pull request
Aug 25, 2026
* update: the parts that decide what to trust
The verification core of `fabric update`, with no IO in it yet, so every
decision it makes is testable without a network or a filesystem.
WHAT IT REFUSES, which is the point of the module.
`--url` without `--sha256` is rejected outright. There is nothing sensible
to default to: the whole point of `--url` is that fabric does not know what
is there, so installing it unchecked would be remote code execution with
good manners. `--tag` with `--url` names two artifacts. `--sha256` without
`--url` is a mistake worth saying out loud rather than ignoring, because a
release carries its own checksum.
The archive must hold EXACTLY ONE MEMBER NAMED `fabric`. Not two, not a
directory, not `./fabric`.
WHAT THE CHECKSUM DOES AND DOES NOT DO, said in the module doc rather than
implied. With `--url` and an explicit hash it is a real check that the
bytes are the ones the caller named. On the release paths the sidecar comes
from the SAME server as the artifact, so it protects against corruption and
truncation and NOT against a compromised release. Ordinary for a release
install, and written down because "verify" reads as security to whoever
arrives next.
THE SIDECAR PARSE IS A REAL TRAP, inherited from the shell script. The
published file reads `<hash> dist/fabric-<target>.tar.gz`, carrying the
path it had on the builder. That path does not exist on the machine
installing, so `shasum -c` fails on the directory name rather than on the
bytes. Field one is taken directly.
`--check` gets THREE exit states, not two: 0 up to date, 1 update
available, 2 error. A sweep that cannot tell "the release server is
unreachable" from "an update is available" will act on the wrong one.
TWO THINGS THE TESTS TAUGHT ME, both worth more than the code they changed.
The `./fabric` case cannot be fixtured with `tar::Builder`, because it
normalises the name on the way in. The test was passing against an archive
that said `fabric`. It now builds a 512-byte header by hand AND asserts the
fixture really carries the dot-slash before testing anything, so it cannot
quietly go back to proving nothing.
And I wrote a comment claiming the parsed path would ACCEPT `./fabric`, so
comparing raw bytes was a fix. Mutating the code back proved it would not:
the crate normalises on write, not on read, and both forms reject it. The
raw comparison stays, as belt and braces rather than a fix, and the comment
now says so. A flattering explanation of a change is worse than none.
Adds `tar` and `flate2`. `reqwest`, `rustls` and `sha2` were already
compiled as transitive dependencies of iroh, so naming them costs no new
crates; the reqwest feature pin keeps `aws-lc-rs` out from beside the ring
provider iroh already uses.
Agent: Silber.fabric
* update: one command that updates a machine and cannot strand it
`fabric update` replaces this machine's fabric with a verified build, then
re-renders the service and restarts it.
WHAT IT REPLACES. A shell script that lived on one machine and was
base64-encoded across the wire to the others, plus two more copies of the
same recipe in `install.sh` and the README. Each copy knew a trap the
others did not. This is one copy, in the binary, with the traps as tests.
fabric update [--tag T] [--url U --sha256 H]
[--check] [--dry-run] [--no-restart] [--rollback]
WHAT IT REFUSES. `--url` without `--sha256`, because installing bytes
nobody checked is remote code execution with good manners and there is
nothing sensible to default to. Two sources at once. An archive that is
not exactly one member named `fabric`. A staged binary whose `--version`
disagrees with the tag that promised it. Every refusal happens before
anything is moved.
`--check` HAS THREE EXIT STATES, NOT TWO: 0 up to date, 1 update
available, 2 error. The error path is special-cased in `main` rather than
left to propagate, because propagating would exit 1, and a fleet sweep
would read an unreachable release server as work to do.
WHAT THE CHECKSUM DOES AND DOES NOT DO, in the module doc and in `--help`.
With `--url` and an explicit hash it proves the bytes are the ones the
caller named. On the release paths the sidecar comes from the SAME SERVER
as the artifact, so it catches corruption and truncation and NOT a
compromised release. A test fails if that wording disappears, because
"verify" reads as security to whoever arrives next.
IT INSTALLS AT THE PATH THE SERVICE MANAGER RUNS, read from the plist or
from `ExecStart`, not from `$PATH`. Installing at the wrong one leaves the
daemon on the old binary while `--version` reports the new.
ON LINUX THE RESTART SUPERVISES ITSELF, because nothing outside can reach
a machine whose daemon is down: `fabric exec` stops working and the tool
that would repair it is the tool that broke it. A transient unit runs the
ROLLBACK binary, the copy already proven to work there, waits for the
control socket, and puts the old binary back if the daemon does not
return. It cannot run in the updating process, which dies with the cgroup.
A BUG I ALMOST SHIPPED, found by reading rather than by a failing test.
`service::install` renders the unit from `env::current_exe()`, so
re-rendering during an update would have pointed the daemon at whatever
binary ran the update — a `target/debug` build while testing. That is the
wrong-path trap entered from the other side. `install_at` takes the exe
explicitly, and a test asserts the rendered unit names the binary it was
GIVEN and specifically not the one rendering it.
AND A TEST THAT WAS PROVING NOTHING. The `./fabric` case was passing
against an archive that said `fabric`, because `tar::Builder` normalises
the name on the way in. It now builds a 512-byte header by hand AND
asserts the fixture really carries the dot-slash before testing anything.
I also claimed in a comment that comparing raw bytes fixed a bug there;
mutating the code back proved it did not, and the comment now says so.
VERIFIED ON SILBER against the real release server: `--check` 0, an older
tag 1, a nonexistent tag 2; `--dry-run` left the binary byte-identical
with no temporary files; `--url file:///` installed a locally built
artifact through the same hash check; both refusals changed nothing.
`tar` and `flate2` are new. `reqwest`, `rustls` and `sha2` were already
compiled under iroh, so naming them costs no crates.
Agent: Silber.fabric
* update: the supervisor's shape is testable from either platform
`schedule_supervisor` built its command inline inside a
`cfg(target_os = "linux")` block, so the one shape whose regression leaves
a Linux machine down with no way back in could only be checked on the
platform where it does the damage.
The argv is now built by `supervisor_argv`, which is not cfg-gated, and a
test pins it: scheduled rather than inline, invoking the supervising
subcommand, and told what to restore.
IT ALSO PINS THAT THE SUPERVISOR IS THE ROLLBACK BINARY. Asking a
possibly-broken new binary to supervise its own installation is not
supervision, and that is the sort of detail a later refactor simplifies
away without noticing.
I mutated it to run inline and watched it fail with "the supervisor would
die with the cgroup it is meant to outlive".
Same reasoning as `systemd_restart_argv` in #80. A Linux-only string
cannot be tested from a Mac, and these two are the strings that strand a
remote machine.
Agent: Silber.fabric
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
1 participant
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.
Two defects, both live on
main, both found while planningfabric update. That command re-renders the unit on every run, so each of these would have gone from rare to constant.1. A re-install silently dropped a configured memory ceiling
allow_shellandallow_execsurvive a re-install becauseresolve_allow_shell/resolve_allow_execread them back out ofconfig.tomlwhen the option isNone.memory_max_mbhad no such path. It lived only in the rendered plist or unit, andrender_*emits it only whenSome— so any install that didn't name it removed a ceiling an operator set earlier, without saying anything.It's now persisted beside the other two, with
resolve_memory_max_mbin the same shape as its siblings.The option becomes a real tri-state, because a ceiling is itself optional: nothing said keeps what's persisted,
Some(None)clears,Some(Some)sets. Persisting a value with no way to remove it would have made the first ceiling anybody set permanent, so--no-memory-max-mbis added alongside, mirroring--no-allow-shell.The install summary now prints the resolved ceiling rather than what the caller passed. Those differ exactly when a persisted value was kept — the case the line exists to show.
2. The Linux restart killed its own caller
install_systemd_userended withsystemctl --user restart fabric.service.fabric execruns its session insidefabric.service's cgroup, sofabric exec hetz -- fabric service installrestarts the cgroup the caller lives in and dies partway through. Reachable today with a command any of us might run.The restart is now handed to systemd to run on its own, outside the caller's cgroup, three seconds out so the caller returns first. No
--unitname is passed on purpose: systemd names the transient unit itself, so two updates close together can't collide on a name that already exists.Install now reports
restart scheduled, because that's what it is. Claiming a restart that hasn't happened would make a failed start look like a successful install.Both tests were watched failing
Against a stub reproducing the real behaviour, not against nothing:
SERVICE_NAMEloses itscfg(target_os = "linux")gate so the restart argv and its test build everywhere — a Linux-only string can't be tested from a Mac, and this is the shape whose regression takes a remote machine down.The restart test pins the command shape, not the effect. The failure mode is that the caller dies, so a test waiting for the effect would be the thing that got killed.
Agent: Silber.fabric