fix: make repeated runs on the same runner idempotent - #217
Conversation
github-actions
Bot
added
the
team-developer-support
9dc3b99 to
fe6fcc2
Compare
brendaneamon
changed the title
c47d028 to
e9a1219
Compare
|
PR review by Claudius (
|
e9a1219 to
9380cbc
Compare
dbda4c1 to
4b40be4
Compare
a550a2e to
831768f
Compare
|
Downgrades shouldn't be a supported path here. The PR builds them in deliberately ( Silently keeping the installed version would make the pin lie, so failing loudly seems right:
One thing to decide explicitly: pins that don't parse as versions — nightly and commit-hash pins have no ordering, so the guard can't tell direction there. I'd let those through with a warning and a doc note rather than block them, but flagging it so it's a decision rather than an accident. |
|
One more, on token handling: a credential an admin hand-wrote into The mechanism: The action's own token is fine: it's masked via Two-part fix, both cheap:
Applies to both |
|
One more edge case, from the #191 thread itself: the workaround posted there interacts badly with the new The workaround is a step before this action that sed-deletes
The action can't distinguish a workaround line from a genuinely user-owned PAT, so I wouldn't ask for smarter stripping. Two asks instead:
|
607962e to
d9af705
Compare
The Flox packages ship their own Nix and symlink it into /usr/bin, so a runner whose disk survives the job starts its next run with a `nix` this action installed. Detection read that as a Nix the user had brought and tried to install Flox through `nix profile install` on a machine that already had it. Asking for `flox` first answers the question directly: when it is already present the installation is skipped, and `force-reinstall` or a pinned `version` still install over it. Nix settings now live in `/etc/nix/install-flox-action.conf`, replaced in full each run and pulled in by a single optional include. The token granted to a job expires with that job, so rewriting the file is what keeps a later run from being handed a dead one: Nix falls back to anonymous, rate-limited access when no token is configured, but fails with HTTP 401 when it finds one that has expired. A token the user placed in nix.conf themselves is left alone. Settings written into nix.conf by earlier versions are removed, and that removal is logged, since it is the moment a stale token stops being served. A post step deletes the file when the job ends, so the token does not outlive the job it was granted to. On the existing-Nix path the configuration is written before installing, because the flake fetch needs the token. On the package path it comes after: flox's postinst writes nix.conf only when it finds none, and the defaults it puts there include the empty build-users-group a single-user install depends on. rpm refuses to install over a package already present, so the rpm path uses `-U --replacepkgs --oldpackage`, covering a fresh install, a reinstall, and a downgrade to a pinned version alike. The config file is named for the job that owns it rather than shared, and its name handed to the post step, so a machine running several jobs at once cannot have one job's teardown delete a token another job is still using; include lines whose file is gone are pruned, so a job killed before its post step leaves nothing behind. Writes go over stdin rather than in a command line, because the runner echoes commands into the job log and nix.conf may hold a token this action did not write and cannot mask; any it finds there is registered for masking regardless. Deferring to such a token is now logged, since silence there is indistinguishable from the bug being fixed. Installing a version older than the one already present now fails before anything is installed. Flox brings its own Nix, a Nix store migrates only forward, and a Nix older than the one that last wrote the store can refuse to operate against it; no package manager declines the swap on those grounds, so the install would succeed and the machine would break later at first use, with nothing tying it to the pin. The error names the remedy: change the pin, or remove flox and /nix and install again. `--oldpackage` is gone from the rpm command so rpm refuses as well. A reference with no version ordering, a commit-hash channel for instance, cannot be judged either way and proceeds with a warning.
Neither failure can occur in this repository's CI, because every job on a hosted runner starts on a fresh VM. One CI job now runs the action twice in a single job, expiring the recorded token and planting a block of the older format in between, which puts the second run in the state a runner that keeps its disk reaches on its own. The script does the same locally against a container that stays alive across both runs. It takes a git ref, so a given revision can be exercised against that state. There is no rpm runner in the CI matrix, so the rpm branch of the install script has a check of its own, taking the command from the script rather than repeating it so the two cannot drift. It covers a fresh install, a reinstall at the same version, and a downgrade.
Nothing in the README distinguished a runner that starts fresh from one that keeps its filesystem, which is the distinction governing almost everything the action does. The new section covers why detection looks for flox rather than Nix, which file the action owns and why it is rewritten rather than appended to, that a token found in nix.conf is left alone, and that the token is removed when the job ends. The packages pre-depend on sudo and xz, and sudo is needed whichever way flox is installed because this action uses it to write the Nix configuration. Hosted images carry both and a minimal self-hosted machine may not, so the README says to provision them. CLAUDE.md is brought up to date with the same changes.
`scripts/` held two unrelated kinds of file: `install-flox.sh`, which the action executes at runtime, and tools a developer runs deliberately. Nothing distinguished them. The developer tools move to `verification/` with a README describing what each answers and what it needs; `install-flox.sh` stays where the bundle resolves it. The checks are there because CI has three standing blind spots rather than because of any one bug: a hosted runner never keeps its filesystem between jobs, the matrix never executes the rpm branch of the install script, and a job token cannot be observed expiring into a later job. A runner-registration script and a workflow to dispatch against it cover the third, which nothing here could reach before. The workflow deliberately asserts little beyond "flox still works", since what counts as correct depends on the investigation. CLAUDE.md records the three gaps so a green CI run is not mistaken for full coverage. The rpm check exercises the store after every install rather than reading `flox --version`, which passes whether or not /nix is usable and so proved nothing about the version changes it was meant to cover. It also asserts that a downgrade is refused rather than that it succeeds.
d9af705 to
fdb5e9e
Compare
2 participants
Summary
On a runner whose disk survives between jobs, this action fails on the second run and every run after it. Two defects share one cause: the action assumes it always starts on a fresh machine, so everything it writes is written append-if-absent, a pattern that is correct exactly once.
The Flox packages ship their own Nix and symlink it into
/usr/bin, so a successful first run creates/usr/bin/nix. Detection read that as a Nix the user had brought and tried to install Flox throughnix profile installon a machine that already had it. Asking whetherfloxis present answers the question directly, so that is now the first check. When Flox is already there the installation is skipped and the newflox-preinstalledoutput reports it; a newforce-reinstallinput, or a pinnedversionthat does not match what is installed, still installs over it.configureNixExtraappended to/etc/nix/nix.confand declined to write a token whenever anaccess-tokensline was already present. That guard exists to avoid clobbering a token the user set themselves, but on a persistent runner the line it found was this action's own, holding a token that expired the moment the earlier job ended. A stale token is worse than no token: Nix falls back to anonymous, rate-limited access when none is configured, but fails outright withHTTP error 401when it finds one that has expired. It also explains why passinggithub-tokenexplicitly made no difference, since the guard never consulted the input.The action now owns a file under
/etc/nix/named for the job that wrote it,install-flox-action-<run>-<attempt>-<id>.conf, with one matching!includeline innix.conf, following the precedent Flox's own postinst sets withinclude flox.conf. Per job rather than shared, because a machine can run several jobs at once and one job's teardown must not delete a token another is still using; include lines whose file is gone get pruned, so a job killed before its post step leaves nothing behind. The optional!includeform matters: a bareincludeof a missing file is fatal to Nix, and the file is removed at job end. Anaccess-tokensline the user wrote themselves is still left alone, and that deference is logged, since silence there is indistinguishable from the bug being fixed: 401s, withgithub-tokenappearing to do nothing. Settings written intonix.confby earlier versions are removed on first run, and that removal is logged too, since it is the moment a stale token stops being served.Config writes go over stdin rather than in a command line, because the runner echoes commands into the job log and
nix.confmay hold a token this action did not write. Anyaccess-tokensvalue found there is registered for masking regardless.Where configuration sits relative to installation depends on the path. On the existing-Nix path it goes first, because the flake fetch reads
access-tokensfrom disk. On the package path it comes second: Flox's postinst writes/etc/nix/nix.confonly when it finds none (hooks/after-install.bash:78), and the defaults it puts there include the emptybuild-users-groupa single-user install depends on.A post step removes the config file when the job ends, so the token granted to a job does not outlive it. That file has to stay world-readable for Nix to read it, and on a shared runner anything running in the meantime can read it too.
rpm refuses to install over a package already present, which the reinstall paths now reach, so the rpm branch uses
-U --replacepkgs.--oldpackageis deliberately absent: installing an older flox over a newer one is refused before anything is installed. Flox brings its own Nix, a Nix store migrates only forward, and no package manager declines the swap on those grounds, so the install would otherwise succeed and the machine would break later at first use with nothing tying it to the pin. A reference with no version ordering, a commit-hash channel for instance, cannot be judged either way and proceeds with a warning.configureNixSubstituteris removed: it was exported and unit-tested but never called, and its substituters are supplied directly as flags byinstallViaExistingNixand through inputs byconfigureNixExtra.Fixes #191.
How to review this
Read the four commits in order. The repo is rebase-only, so they land on
mainindividually and each stands on its own.fix(runners)src/main.jschecksfloxbeforenix;src/nixconf.jsowns the config file;src/cleanup.jsis the post step;scripts/install-flox.shgets the rpm flagstest(runners)test-repeat-runCI job and the container harnessesdocs(readme)test(verification)verification/, with a READMETwo directories, deliberately.
scripts/install-flox.shis shipped code theaction executes at runtime. Everything under
verification/is a developer toolCI never runs. They were previously mixed with nothing marking which was which.
The reviewable surface is smaller than the diff suggests. Most of the
non-generated lines are tests and harnesses;
dist/index.jsis generated bynccand not worth reading.Run the checks yourself. Docker is all the first two need:
The first runs the action twice against one long-lived container, which is a
persistent runner for these purposes; the git ref argument is how two revisions
are compared. The second covers fresh install, same-version reinstall, and that a
downgrade is refused, exercising the store after each rather than reading
flox --version, and reading the rpm command out ofinstall-flox.shso thecheck cannot drift from what the action runs.
For a genuine runner, with a scratch repository and a registration token:
Then copy
verification/self-hosted-compare.ymlinto that repository's.github/workflows/and dispatch it: two refs, twice each, chained so they stayin order on the one machine, with state printed before and after every run. Use a
scratch repository, since the runner accepts any job that repository schedules.
Why any of this is by hand. CI cannot see three things, and this bug fell
into all of them: hosted runners never keep a filesystem between jobs, the matrix
never executes the rpm branch, and a job token cannot be watched expiring into a
later job.
verification/README.mdcovers each in detail.Test plan
npm run allpasses: formatting, unit tests, and the bundlemainand passes here, withnix.confkeeping the defaults Flox's postinst writesverification/rpm-reinstall.shpasses on x86_64 and aarch64: fresh install, same-version reinstall, downgrade refused, store usable after eachtest-repeat-runpasses onubuntu-latest,ubuntu-slim, andmacos-latesttest-existing-nixstill passes for both the Determinate Systems and Cachix installers, covering the foreign-Nix pathv2.5.3produced the 401 from its own expired token and this branch healed the machine (detail)