Extract the repeated dnsmasq pidfile path literals in internal/networ...

[colonelpanik]

Goal

Extract the repeated dnsmasq pidfile path literals in internal/network/provision.go (fmt.Sprintf("/var/run/litevirt-dnsmasq-%s.pid", bridge) at lines 101, 245, 305 and fmt.Sprintf("/var/run/litevirt-dnsmasq-vni%d.pid", vni) at lines 191, 335) into one or two small named helpers in internal/network, and use them at every call site.

Plan

Plan: extract the dnsmasq pidfile path literals in internal/network

Goal

Replace the hand-rolled fmt.Sprintf dnsmasq pidfile paths in
internal/network/provision.go with two small unexported helpers in
internal/network, and use them at every call site.

This is a pure extraction. The produced strings must stay byte-for-byte identical
to what internal/network/dnsmasq.go's pidfile-based stop/kill logic
(StopDHCP, procIsOurDnsmasq, readPidFile, the drift-restart path) and
internal/network/dnsmasq_test.go already expect. No path-format change, no
behaviour change, no new exported API.

Current state

The literals live in exactly one non-test file. Verified with
grep -rn "litevirt-dnsmasq" --include=*.go .:

provision.go	context	current expression
101	Provision, "bridge" case, DHCP start	fmt.Sprintf("/var/run/litevirt-dnsmasq-%s.pid", bridge)
191	Provision, "vxlan" case, DHCP start on the elected gateway host	fmt.Sprintf("/var/run/litevirt-dnsmasq-vni%d.pid", vni)
245	Provision, "isolated" case, DHCP start	fmt.Sprintf("/var/run/litevirt-dnsmasq-%s.pid", bridge)
305	Deprovision, "bridge" case, StopDHCP	fmt.Sprintf("/var/run/litevirt-dnsmasq-%s.pid", bridge)
335	Deprovision, "vxlan" case, StopDHCP	fmt.Sprintf("/var/run/litevirt-dnsmasq-vni%d.pid", vni)
346	Deprovision, "isolated" case, StopDHCP	fmt.Sprintf("/var/run/litevirt-dnsmasq-%s.pid", bridge)

There are six sites, not the five named in the task. Line 346 (the
"isolated" branch of Deprovision) is the same %s-with-bridge form and is
included — "every call site" is the requirement, and leaving one behind would
defeat the point of the extraction.

Nothing outside internal/network constructs one of these paths.
StartDHCP/StopDHCP are exported but their only callers are inside this
package (provision.go and dnsmasq.go itself), so both helpers stay
unexported, as the constraint requires.

Design

Add to internal/network/dnsmasq.go, immediately after the existing
dnsmasqLeaseDir const (~line 65). That file already owns every consumer of a
pidfile path and the sibling "where does dnsmasq's on-disk state live" fact, so
the two path-shape decisions end up adjacent and reviewable together.

// dnsmasqPidFile returns the pidfile path for the dnsmasq instance keyed on
// `key`. It is the ONLY place this path is constructed: StopDHCP signals the
// recorded PID, procIsOurDnsmasq matches the live `--pid-file=<path>` argv
// element, and the pkill backstop matches `pid-file=<path>` — so a path built
// two different ways silently orphans a running dnsmasq instead of stopping it.
//
// `key` is the bridge name for bridge and isolated networks. VXLAN networks key
// on the VNI token instead (see dnsmasqPidFileVNI) — NOT on the bridge name.
func dnsmasqPidFile(key string) string {
	return "/var/run/litevirt-dnsmasq-" + key + ".pid"
}

// dnsmasqPidFileVNI returns the pidfile path for a VXLAN network's dnsmasq. Its
// key is "vni<N>", deliberately NOT vxlanBridgeName(vni) ("br-vni<N>"): the
// path predates this helper and is what the running fleet's dnsmasq processes
// already carry in their command line. Re-keying it on the bridge name would
// make StopDHCP/StartDHCP look at a path no live process uses — the old dnsmasq
// keeps holding the bridge gateway IP:53 and the replacement dies on bind.
func dnsmasqPidFileVNI(vni int) string {
	return dnsmasqPidFile(fmt.Sprintf("vni%d", vni))
}

Then, at each of the six sites, replace only the right-hand side; keep the local
pidFile := binding so the diff is six one-line changes:

• 101, 245, 305, 346 → pidFile := dnsmasqPidFile(bridge)
• 191, 335 → pidFile := dnsmasqPidFileVNI(vni)

Why the VNI helper delegates rather than holding its own literal

Delegating leaves exactly one path literal in the package, which is the whole
point of the change. The alternative — a second independent
fmt.Sprintf("/var/run/litevirt-dnsmasq-vni%d.pid", vni) inside
dnsmasqPidFileVNI — reads marginally more directly but leaves two copies of
/var/run/litevirt-dnsmasq- that can drift apart, which is the defect being
removed. "vni" + strconv.Itoa(vni) composed through the base helper is
byte-identical to the old vni%d form for every int (%d and Itoa agree,
including negatives, which cannot occur here — Provision rejects vni == 0 and
the VNI comes from a validated compose field).

The base helper's parameter is named key, not bridge, because two of six
callers pass a VNI token. Naming it bridge would make those two call sites read
as a bug.

Byte-identity argument

• dnsmasqPidFile(bridge) ≡ fmt.Sprintf("/var/run/litevirt-dnsmasq-%s.pid", bridge)
  — %s on a string is the identity, so concatenation is equivalent.
• dnsmasqPidFileVNI(vni) ≡ fmt.Sprintf("/var/run/litevirt-dnsmasq-vni%d.pid", vni)
  — expands to "/var/run/litevirt-dnsmasq-" + "vni" + itoa(vni) + ".pid".
• /var/run is preserved verbatim. It is not normalised to /run (they are
  the same directory via symlink on modern Linux, but procIsOurDnsmasq and the
  pkill backstop match the argv string, so a live dnsmasq started by the
  previous binary with --pid-file=/var/run/... would stop matching).

Files to change

1. internal/network/dnsmasq.go — add the two helpers (~14 lines incl. comments).
   fmt is already imported.
2. internal/network/provision.go — six one-line replacements. fmt stays
   imported (used by fmt.Errorf throughout), so no import churn and no
   goimports movement.
3. internal/network/provision_test.go — new call-site tests (below).
4. internal/network/dnsmasq_test.go — new helper unit test (below).

No other file changes. No docs, schema, ledger, proto, or CLI surface is touched,
so make ci-guards is unaffected (it still must pass, and will — there is no new
config key, flag, doc reference, SQL builder, or litevirt_* identifier).

Tests

Unit tier, in internal/network. This is package-local logic with no multi-node
failure mode, so per CLAUDE.md's tier table tests/fleet/ is not the right
reach here — a fleet node never spawns dnsmasq (it runs libvirtfake), so a
fleet test could not observe these strings at all.

1. TestDnsmasqPidFile (new, in dnsmasq_test.go)

Pins the exact output of both helpers and the relationship between them:

dnsmasqPidFile("br0")          == "/var/run/litevirt-dnsmasq-br0.pid"
dnsmasqPidFile("br-iso-web")   == "/var/run/litevirt-dnsmasq-br-iso-web.pid"
dnsmasqPidFileVNI(500)         == "/var/run/litevirt-dnsmasq-vni500.pid"

Plus the load-bearing negative assertion:

dnsmasqPidFileVNI(500) != dnsmasqPidFile(vxlanBridgeName(500))

That last line is the guard against the one way this refactor can go wrong later:
a "simplification" that notices a bridge variable in scope at the VXLAN sites
and collapses both helpers into one. It fails loudly with a comment explaining
the upgrade hazard.

Also assert round-tripping through the consumer, so the test proves the helper
output is usable by the stop/kill logic rather than merely equal to a string:

cmdlineHasPidFile([]byte("dnsmasq\x00--pid-file="+dnsmasqPidFile("br0")), dnsmasqPidFile("br0")) == true
cmdlineHasPidFile([]byte("dnsmasq\x00--pid-file="+dnsmasqPidFileVNI(500)), dnsmasqPidFileVNI(500)) == true

2. TestProvision_DnsmasqPidFilePaths (new, in provision_test.go)

A helper-unit test alone would not catch "helper added, but one call site still
hand-rolls its own path". Every one of the six sites is independently observable,
so each gets an assertion.

The three Provision sites (101, 191, 245) — stub startDHCPFunc to capture
the pidFile argument (the existing pattern at provision_test.go:79 and
:181) and stub execCommand:

subtest	NetworkDef	expected captured pidFile
bridge	{Type:"bridge", Interface:"lv-pf-br0", Subnet:"10.88.0.0/24"}	/var/run/litevirt-dnsmasq-lv-pf-br0.pid
vxlan	{Type:"vxlan", VNI:4242, Underlay:"eth0", Subnet:"10.89.0.0/24"}	/var/run/litevirt-dnsmasq-vni4242.pid
isolated	{Type:"isolated", Subnet:"10.90.0.0/24"}, network "pfnet"	/var/run/litevirt-dnsmasq-br-iso-pfnet.pid

Reachability of each DHCP start, checked against the code:

• bridge: lv-pf-br0 does not exist on the host, so BridgeExists (a real
  net.InterfaceByName, not execCommand) is false → bridgePreExisted false →
  the Subnet != "" && (!bridgePreExisted || def.DHCP) gate opens. Keep the name
  ≤15 chars or EnsureBridge rejects it.
• vxlan: set Underlay explicitly so defaultRouteInterface() (which would
  return "" under the exec stub and abort with "could not auto-detect") is not
  consulted. Provision upserts this host's own VTEP, so isGatewayHost sees a
  single-host list and elects it → DHCP starts. This path sleeps 500 ms for CRDT
  convergence, same as the existing TestProvision_WithSubnet.
• isolated: the non-HostIsolation branch calls
  corrosion.DeleteHostFWIntent, so this subtest needs a real
  corrosion.NewTestClient() + InitSchema (as all three do).

The three Deprovision sites (305, 335, 346) — these call StopDHCP
directly, not through a func var, so they are observed one level down:
StopDHCP's pkill backstop runs execCommand("pkill", "-f", "pid-file="+pidFile),
and execCommand is stubbed. Assert the captured call set contains exactly:

subtest	NetworkDef	expected pkill argument
bridge	{Type:"bridge", Interface:"lv-pf-br0", Subnet:"10.88.0.0/24"}	pid-file=/var/run/litevirt-dnsmasq-lv-pf-br0.pid
vxlan	{Type:"vxlan", VNI:4242, Subnet:"10.89.0.0/24"}	pid-file=/var/run/litevirt-dnsmasq-vni4242.pid
isolated	{Type:"isolated", Subnet:"10.90.0.0/24"}, network "pfnet"	pid-file=/var/run/litevirt-dnsmasq-br-iso-pfnet.pid

Deprovision can be called with a nil db here — its only db use is guarded by
if db != nil. Every other side effect (RemoveHostIsolation, RemoveSNAT,
RemoveProxyARP, RemoveNAT, RemoveIRB, DeprovisionVXLAN, ip link del)
goes through execCommand and is therefore fully stubbed.

Test-hygiene notes for whoever writes these

• execCommand and startDHCPFunc are package-level vars. Subtests must not
  call t.Parallel(), and each must defer the restore
  (execCommand = defaultExec, startDHCPFunc = StartDHCP) exactly like the
  existing tests.
• StopDHCP also does a real os.ReadFile/os.Remove on the path (only the
  pkill goes through the stub). On a non-root dev box both fail with ENOENT and
  are ignored. Use deliberately unreal names (lv-pf-br0, VNI 4242, network
  pfnet) so the test cannot delete a real pidfile if someone runs
  go test ./internal/network/ as root on a live litevirt host.
• IsolatedBridgeName("pfnet") → "br-iso-pfnet" (12 chars, under the hash
  fallback threshold), so the expected string is stable and readable. Do not pick
  a long network name here or the bridge becomes a sha1 prefix.

Mutation verification

Per CLAUDE.md, each assertion is proven by breaking the property and confirming
red, then restoring. Three mutations, each targeting a different failure the tests
are supposed to catch:

1. Path prefix drift — change the helper to "/run/litevirt-dnsmasq-".
   Expected red: TestDnsmasqPidFile and all six call-site assertions. Proves
   the tests pin the literal path rather than just "whatever the helper returns".
2. The collapse — change dnsmasqPidFileVNI to
   return dnsmasqPidFile(vxlanBridgeName(vni)). Expected red: the
   != assertion in TestDnsmasqPidFile plus the two vxlan subtests. Proves the
   negative assertion is not vacuous — this is the mutation that matters most,
   because it is the plausible future mistake.
3. A missed call site — revert site 191 to a hand-rolled
   fmt.Sprintf("/var/run/litevirt-dnsmasq-vni-%d.pid", vni) (note the extra
   hyphen). Expected red: the vxlan Provision subtest only. Proves the
   call-site tests observe the actual call sites, not just the helper. Repeat for
   one Deprovision site (335) to prove the pkill-based observation works.

Record the observed red output for at least mutations 2 and 3 before restoring —
those are the two that make the test suite non-vacuous.

Verification

go build ./... && go vet ./...
go test ./internal/network/
go test ./...
make ci-guards

Plus the mechanical check that the extraction is complete — after the change this
must print exactly one line (the helper in dnsmasq.go):

grep -rn "litevirt-dnsmasq-" --include=*.go internal/ | grep -v _test.go

Notes for the reviewer

• The VXLAN pidfile is keyed on the VNI, not the bridge name. vni500 vs
  br-vni500. Both Provision's and Deprovision's vxlan branches have a
  bridge variable in scope (bridge, err := EnsureVXLAN(...) /
  bridge := vxlanBridgeName(vni)), which makes dnsmasqPidFile(bridge) look
  like the obvious tidy-up at those two sites. It is not: it changes the path.
  The concrete blast radius is an upgrade — a node running the current binary
  has live dnsmasq processes whose command line carries
  --pid-file=/var/run/litevirt-dnsmasq-vni500.pid. A new binary looking at
  .../litevirt-dnsmasq-br-vni500.pid would find no pidfile, skip the
  already-running short-circuit, fail procIsOurDnsmasq, miss with the pkill
  backstop, and then spawn a second dnsmasq that dies on bind against the
  survivor still holding the bridge gateway IP:53 — DHCP outage on every VXLAN
  network with a subnet, and StartDHCP's two-attempt retry does not help. This
  is why the two helpers stay distinct and why the != assertion exists.
• dnsmasq_test.go's existing literals stay literal, on purpose.
  TestDnsmasqArgs_ExcludesLoopback (:84, :90), TestDnsmasqArgs_V6EnablesRA
  (:151), and TestCmdlineHasPidFile (:15, :21) pass pidfile paths in as
  inputs to dnsmasqArgs / cmdlineHasPidFile; they are testing argument
  construction and argv matching, not path derivation. Rewriting them to call
  dnsmasqPidFile would make them tautological — a path-format regression would
  move both sides of the comparison and the tests would still pass. Leaving them
  as independent literals is what keeps them a real guard on the constraint.
  (TestCmdlineHasPidFile uses /run/..., not /var/run/...; that is fine and
  intentional — it only needs some path shape to test exact-argv matching.)
• /var/run is load-bearing as a string. See the byte-identity section: the
  stop/kill paths match argv text, so /run is not a safe "cleanup".
• No behaviour change is intended anywhere. The DHCP-start gates, the NAT
  intent decoupling (b7e46c7, pinned by
  TestProvision_Bridge_PreExistingNAT_NoDHCP), the gateway election, and the
  drift-restart logic are all untouched. If a reviewer sees any of those move,
  that is out of scope.
• Out of scope, deliberately: moving these paths to a configurable run
  directory (internal/lb uses /run/litevirt/lb with an injectable runDir —
  a tempting symmetry, but it changes the on-disk contract and breaks the
  byte-identity constraint), unifying with dnsmasqLeaseDir, and threading a
  pidfile through StartDHCP's signature so callers cannot get it wrong.
• Commit: refactor(network): extract the dnsmasq pidfile path into one helper.
  PLAN.md is already gitignored (.gitignore:71), so it will not appear in the
  diff.

Deviations from the plan as written

Two, both cosmetic; the design, the six call sites, and every test are as
specified.

1. The completeness grep prints two lines, not one. The plan's check
   (grep -rn "litevirt-dnsmasq-" --include=*.go internal/ | grep -v _test.go)
   also matches dnsmasq.go:88, a comment line inside
   dnsmasqPidFileVNI's doc block that names .../litevirt-dnsmasq-br-vni500.pid
   as the concrete counter-example for the collapse hazard. The plan's intent —
   one place where the path is constructed — holds exactly; excluding comment
   lines prints the single dnsmasq.go:80 return. The counter-example path was
   kept because a reader hitting that comment benefits from seeing the wrong path
   spelled out, which is the whole point of the warning.

2. Pre-existing gofmt non-compliance was left alone. gofmt -l internal/network/ flags dnsmasq.go, provision_test.go, bridge.go,
   ipam.go and network_extra_test.go. All of it predates this change and is
   identical at HEAD (verified per-file against git show HEAD:<file> | gofmt -d): in dnsmasq.go a missing // separator in the comment block above
   dnsmasqLeaseDir, in provision_test.go four def,"10.0.0.1" calls missing a
   space. The added code introduces zero new gofmt findings. Fixing the rest
   would have put unrelated churn in a pure-extraction diff.

Mutation verification (observed, then restored)

All three mutations behaved exactly as predicted.

1. Prefix drift (/run/ for /var/run/) → red: all 3 TestDnsmasqPidFile
   equality assertions and all six call-site assertions.
2. The collapse (dnsmasqPidFileVNI → dnsmasqPidFile(vxlanBridgeName(vni))) →
   red: dnsmasq_test.go:64 (VXLAN pidfile must key on the VNI token, not the bridge name; both produced "/var/run/litevirt-dnsmasq-br-vni500.pid"), the
   vni500 equality assertion, plus provision_vxlan and deprovision_vxlan
   only — the bridge and isolated subtests correctly stayed green. The negative
   assertion is not vacuous.
3. A missed call site (vni-%d, extra hyphen), applied one site at a time:
   site 191 → only provision_vxlan red (got .../litevirt-dnsmasq-vni-4242.pid);
   site 335 → only deprovision_vxlan red, via the pkill pattern
   (pid-file=/var/run/litevirt-dnsmasq-vni-4242.pid), confirming the
   one-level-down observation of the Deprovision sites actually works.

go build ./..., go vet ./..., go test ./... and make ci-guards (incl.
stmtshapecheck, ledger-drift, writecheck, docs truth) all pass.

Final Codex review

No blocking findings remained.

---

Opened by overseer. The plan and the code each converged to zero blocking findings from Codex before this pull request was created.