ymrtech/nix-config

Fork 0

deploy: a PR-branch dispatch reverts every other flake input (non-main refs deploy the branch's stale flake.lock) #253

New issue

Closed

opened 2026-09-15 17:19:14 -06:00 by hermes · 1 comment

hermes commented

2026-09-15 17:19:14 -06:00

Collaborator

What happens

A workflow_dispatch on a non-main ref deploys that branch's flake.lock, which is a copy of the branch's base. Any input main has moved since is silently reverted on the target host.

Every host job runs (nix-deploy.yml:155, 271, 387, 515, 532):

sudo nixos-rebuild switch --flake "git+https://.../nix-config?rev=${{ github.sha }}#<host>" --refresh

github.sha is the dispatched ref's head. For a PR branch that is the branch's base lock, not main's.

Observed 2026-09-15 (why this was found)

clientsv PR #67 (per-peer bandwidth accounting) merged as 244dc35; the clientsv CD chain bumped clientsv in nix-config main (eb3d329, 16:45). Deploy #1435 (22:49Z) shipped it and the new collector seeded normally. Then two NixOS Deploy runs dispatching the dns/public: AdGuard Home web UI… PR branch landed on public:

run	ref	nix-config commit	clientsv pin in that commit	effect
#1435	main	`1c0b6df`	`244dc35`	collector running, 282 buckets seeded
#1439	PR branch	`d593139`	`92d02cd`	reverted
#1445/1446	PR branch	`6fc34fd`	`92d02cd`	reverted

Evidence: the running binary is /nix/store/50xzjkag…-clientsv-0.1.0/bin/clientsv with 0 occurrences of peer_traffic_hourly; clientsv.service last restarted 16:58; the rollup cursor (peer_traffic_meta.last_sample_at) is stuck at 22:49:41Z; and the branch commits' flake.lock pins clientsv@92d02cd (curl …/raw/commit/6fc34fd/flake.lock) while main pins 244dc35.

Neither d593139 nor 6fc34fd touched flake.lock relative to main — the branches simply carried a stale copy.

Blast radius

Not specific to clientsv: every input main has bumped since a branch's base is reverted on the deployed host — nixpkgs, nix-config-private, hardware, hermes-agent, this repo's own modules … The deploy looks green (health gate only checks critical services), so the revert is silent. It also contradicts the design note in deploy.yml's record-deployed-state step ("prod = main always … a concurrent deploy CONVERGES instead of reverting").

This is not a preview-only path. The provisioning flow dispatches hosts=vpn on a per-device branch, so every customer provisioning deploy runs the branch lock too.

Why the existing guardrail didn't catch it

The deployed.jsonl drift watchdog (readlink/cron, Sun 04:35) compares the deployed nix-config sha against main's head — weekly, and sha-level. Here the harm was a reverted input inside an otherwise-plausible deploy.

Proposed fix (implemented in the linked PR)

A non-main ref must never revert another input. Rule: if the branch did not itself modify flake.lock, main's lock is authoritative for it — deploy the branch's tree with main's lock. If the branch did modify flake.lock (an intentional input bump), deploy it as-is.

Shape: one step in the scope job (checkout → merge-base origin/main HEAD → if flake.lock is untouched on the branch, take origin/main -- flake.lock, commit, push to a per-ref deploy-lock/<ref> branch and expose that rev). The host jobs use that rev instead of github.sha. Deliberately best-effort: the provisioning dispatch must never depend on reaching main, so any failure falls back to today's behaviour with a loud warning rather than blocking the deploy.

Alternatives considered: refusing the dispatch outright (breaks automatic provisioning), and --override-input per input (needs per-input URL synthesis; only covers root inputs).

Immediate mitigation (until the fix lands)

Merge origin/main into a PR branch before dispatching a deploy from it — the branch lock then matches main and nothing reverts. The AdGuard branch in question is in exactly that state now, and main currently pins clientsv@244dc35, so its next deploy restores the collector.

Impact of the current state (for the record)

No accounting data is lost: the collector's gap recovery is capped at 6h, and it seeds from 72h of VM history on a cold start, so a restart within 6h of the stall back-fills the whole gap.

## What happens A `workflow_dispatch` on a **non-main ref** deploys that branch's `flake.lock`, which is a copy of the branch's *base*. Any input main has moved since is silently reverted on the target host. Every host job runs (nix-deploy.yml:155, 271, 387, 515, 532): ``` sudo nixos-rebuild switch --flake "git+https://.../nix-config?rev=${{ github.sha }}#<host>" --refresh ``` `github.sha` is the dispatched ref's head. For a PR branch that is the branch's base lock, not main's. ## Observed 2026-09-15 (why this was found) `clientsv` PR #67 (per-peer bandwidth accounting) merged as `244dc35`; the clientsv CD chain bumped `clientsv` in nix-config main (`eb3d329`, 16:45). Deploy #1435 (22:49Z) shipped it and the new collector seeded normally. Then two `NixOS Deploy` runs dispatching the **`dns/public: AdGuard Home web UI…` PR branch** landed on public: | run | ref | nix-config commit | clientsv pin in that commit | effect | |---|---|---|---|---| | #1435 | main | `1c0b6df` | `244dc35` | collector running, 282 buckets seeded | | #1439 | PR branch | `d593139` | `92d02cd` | reverted | | #1445/1446 | PR branch | `6fc34fd` | `92d02cd` | reverted | Evidence: the running binary is `/nix/store/50xzjkag…-clientsv-0.1.0/bin/clientsv` with **0** occurrences of `peer_traffic_hourly`; `clientsv.service` last restarted 16:58; the rollup cursor (`peer_traffic_meta.last_sample_at`) is stuck at `22:49:41Z`; and the branch commits' `flake.lock` pins `clientsv@92d02cd` (`curl …/raw/commit/6fc34fd/flake.lock`) while main pins `244dc35`. Neither `d593139` nor `6fc34fd` touched `flake.lock` relative to main — the branches simply carried a stale copy. ## Blast radius Not specific to clientsv: **every** input main has bumped since a branch's base is reverted on the deployed host — nixpkgs, nix-config-private, hardware, hermes-agent, this repo's own modules … The deploy looks green (health gate only checks critical services), so the revert is silent. It also contradicts the design note in `deploy.yml`'s record-deployed-state step ("prod = main always … a concurrent deploy CONVERGES instead of reverting"). **This is not a preview-only path.** The provisioning flow dispatches `hosts=vpn` on a per-device branch, so every customer provisioning deploy runs the branch lock too. ## Why the existing guardrail didn't catch it The `deployed.jsonl` drift watchdog (`readlink`/cron, Sun 04:35) compares the deployed nix-config **sha** against main's head — weekly, and sha-level. Here the harm was a *reverted input inside* an otherwise-plausible deploy. ## Proposed fix (implemented in the linked PR) A non-main ref must never revert another input. Rule: **if the branch did not itself modify `flake.lock`, main's lock is authoritative for it** — deploy the branch's tree with main's lock. If the branch *did* modify `flake.lock` (an intentional input bump), deploy it as-is. Shape: one step in the `scope` job (checkout → `merge-base origin/main HEAD` → if `flake.lock` is untouched on the branch, take `origin/main -- flake.lock`, commit, push to a per-ref `deploy-lock/<ref>` branch and expose that rev). The host jobs use that rev instead of `github.sha`. Deliberately **best-effort**: the provisioning dispatch must never depend on reaching main, so any failure falls back to today's behaviour with a loud warning rather than blocking the deploy. Alternatives considered: refusing the dispatch outright (breaks automatic provisioning), and `--override-input` per input (needs per-input URL synthesis; only covers root inputs). ## Immediate mitigation (until the fix lands) Merge `origin/main` into a PR branch before dispatching a deploy from it — the branch lock then matches main and nothing reverts. The AdGuard branch in question is in exactly that state now, and main currently pins `clientsv@244dc35`, so its next deploy restores the collector. ## Impact of the current state (for the record) No accounting data is lost: the collector's gap recovery is capped at 6h, and it seeds from 72h of VM history on a cold start, so a restart within 6h of the stall back-fills the whole gap.

hermes referenced this issue from a commit

2026-09-15 17:25:57 -06:00

deploy: never let a non-main ref change flake inputs (issue #253)

hermes referenced this issue from a pull request that will close it,

2026-09-15 17:26:05 -06:00

deploy: never let a non-main ref change flake inputs (guard + fail-closed on regression) #255

hermes referenced this issue from a commit

2026-09-15 17:27:26 -06:00

deploy: reconcile flake.lock to main for guard-test/stale (flake-input guard, issue #253)

hermes referenced this issue from a commit

2026-09-15 17:30:27 -06:00

deploy: never let a non-main ref change flake inputs (issue #253)

hermes commented

2026-09-15 17:30:40 -06:00

Author

Collaborator

Implemented in #255 (rebased onto main after #250/#251 touched the same workflow).

One refinement since the original analysis, based on your steer that touching inputs is rare and owned by the auto-update pipeline:

non-main ref	outcome
no input touched	reconcile to main's pins (the normal case)
adds an input	allowed — rare but legitimate; the new node survives
removes an input	allowed — cleanup of something unused can't regress anything still in use, and the reconcile does not resurrect it
changes an existing input	FAIL — these belong to the weekly flake-update pipeline on main

The decision is made against the ref's merge base, not main: comparing to main would misread a merely-stale copy as intent (verified — feeding main as the "base" produces changed:[clientsv, nix-config-private] and a wrong refusal, while the real base yields {added:[],removed:[],changed:[]} and reconciles).

Fail-closed, with allow_input_lock_change=true as the emergency bypass. For the record, the fail-closed choice means a provisioning dispatch now fails rather than silently downgrading inputs if git.ymrtech.com is unreachable — the bypass is the escape hatch.

End-to-end, deploy-free: a throwaway branch forked before the clientsv bump (lock pinned clientsv@92d02cd, main's 244dc35, and the branch itself untouched flake.lock — the incident's exact shape) was dispatched with an invalid hosts value. The scope job aborted the run so nothing deployed, while the guard really ran: it pushed deploy-lock/guard-test/stale, whose lock has clientsv@244dc35 with all 30 shared nodes at main's pins. Both throwaway branches were deleted afterwards.

That exercise also caught three real bugs in my first cut, before any of this could ship:

$m|has(.key) re-binds . to $m → has(null), a hard error on every non-main deploy;
input values may be a bare node string or [node,...];
flake.lock's top-level root is a node name (a string) — the root node itself lives in .nodes.

Current production note: public was running the pre-#67 clientsv from the AdGuard PR-branch deploy, but the #254 merge's own post-merge deploy (run #1454) has since restored main's pin — the collector is running again (bandwidth rollup: collecting every 15m0s, binary contains the rollup strings) and the stall was inside the 6h gap-recovery window, so no accounting was lost. Any further AdGuard-branch deploy will revert it again until either this PR lands or that branch merges main.

Implemented in #255 (rebased onto main after #250/#251 touched the same workflow). One refinement since the original analysis, based on your steer that touching inputs is rare and owned by the auto-update pipeline: | non-main ref | outcome | |---|---| | no input touched | **reconcile** to main's pins (the normal case) | | **adds** an input | allowed — rare but legitimate; the new node survives | | **removes** an input | allowed — cleanup of something unused can't regress anything still in use, and the reconcile does not resurrect it | | **changes** an existing input | **FAIL** — these belong to the weekly flake-update pipeline on main | The decision is made against the ref's **merge base**, not main: comparing to main would misread a merely-stale copy as intent (verified — feeding main as the "base" produces `changed:[clientsv, nix-config-private]` and a wrong refusal, while the real base yields `{added:[],removed:[],changed:[]}` and reconciles). Fail-closed, with `allow_input_lock_change=true` as the emergency bypass. For the record, the fail-closed choice means a provisioning dispatch now fails rather than silently downgrading inputs if git.ymrtech.com is unreachable — the bypass is the escape hatch. **End-to-end, deploy-free:** a throwaway branch forked *before* the clientsv bump (lock pinned `clientsv@92d02cd`, main's `244dc35`, and the branch itself untouched `flake.lock` — the incident's exact shape) was dispatched with an invalid `hosts` value. The scope job aborted the run so **nothing deployed**, while the guard really ran: it pushed `deploy-lock/guard-test/stale`, whose lock has `clientsv@244dc35` with all 30 shared nodes at main's pins. Both throwaway branches were deleted afterwards. That exercise also caught three real bugs in my first cut, before any of this could ship: - `$m|has(.key)` re-binds `.` to `$m` → `has(null)`, a hard error on every non-main deploy; - input values may be a bare node string *or* `[node,...]`; - `flake.lock`'s top-level `root` is a node **name** (a string) — the root node itself lives in `.nodes`. Current production note: public was running the pre-#67 clientsv from the AdGuard PR-branch deploy, but the #254 merge's own post-merge deploy (run #1454) has since restored main's pin — the collector is running again (`bandwidth rollup: collecting every 15m0s`, binary contains the rollup strings) and the stall was inside the 6h gap-recovery window, so no accounting was lost. Any *further* AdGuard-branch deploy will revert it again until either this PR lands or that branch merges main.