networkmanager: repair pooled TAP drift instead of failing

[hky1999]

Summary

Pooled TAP endpoints whose host-side state drifted — host MAC randomized,
or the bridge attach lost when a predecessor daemon's sandbox0 was torn
down — used to abort allocation and even make daemon startup fatal:

allocate resource interface failed: pooled TAP tap.x host MAC is 02:..(random).., want 02:fd:..
recover idle pooled TAP tap.y: pooled TAP tap.y is not attached to sandbox0   → fatal at boot

The endpoint identity is fully determined by its name (IP-encoded), so
re-stamping the deterministic host MAC and re-attaching the bridge can only
converge a pooled TAP back to its expected state. This change repairs such
drift in place with a warning, instead of failing.

What changed

pkg/networkmanager/tap.go — setTapState only (+35/−12):

check	before	after
not attached to bridge	hard error (fatal at daemon recovery)	LinkSetMaster re-attach + warn
host MAC ≠ deterministic 02:fd:<ip>	hard error (allocation failure)	LinkSetHardwareAddr re-stamp + warn
ifindex drift vs lease record	hard error	refresh lease record + warn (name is the identity; ifindex is bookkeeping)
wrong link type / name ≠ IP encoding	hard error	unchanged (real identity violation)
lease guest-MAC corruption / schema too new	hard error	unchanged (no deterministic basis to repair from)

No API or behavior change for healthy pools.

Why repair is safe

• The deterministic attributes (02:fd:<ip4> host MAC, sandbox0 master)
  are pure functions of the lease IP; applying them cannot make a correct
  endpoint wrong.
• The safety argument is determinism, not the absence of a consumer:
  markUsing repairs freshly-popped idle endpoints, while load() at
  daemon startup repairs both adopted idle taps and taps leased to
  still-running sandboxes (the VMs outlive the daemon; that is the
  point of the durable leases). Re-attaching a bridge port or re-stamping
  the host-side MAC on a TAP a guest still holds open only converges the
  host side toward the expected state.
• Cases with no deterministic answer (corrupt lease payloads, foreign
  device types) still fail loudly.

Root cause (why drift exists at all)

createTapDevice is the only TAP creation site and always sets the
deterministic MAC — but creation is not atomic: LinkAdd (kernel assigns
a random MAC) → SetHardwareAddr → bridge attach → set down. netlink
tuntap creation cannot carry a MAC in the create call (kernel API
limitation, iproute2 is equally two-step), so a daemon dying inside that
window structurally leaves an orphan tap.* with a random MAC and no
bridge. The next daemon's recovery adopts host tap.* devices into the
pool — and then dies on them.

This was verified end-to-end by deterministically reenacting the crash
window: a 20-line program issues the exact same netlink calls and exits
right after LinkAdd, leaving tap.0a5b00c8 with MAC
2a:21:c6:f4:97:e0, unattached. Against that single orphan:

• old binary: daemon startup fatal — recover idle pooled TAP tap.0a5b00c8: … is not attached to sandbox0 (the production signature)
• this fix: daemon starts, logs two repairs (bridge re-attach + MAC
  2a:21:c6:f4:97:e0 → 02:fd:0a:5b:00:c8), and a sandbox restore
  through the adopted endpoint succeeds.

Since the origin (crash window) cannot be eliminated at the kernel-API
level, adoption-time repair is the correct layer to fix it.

Verification

Root cause was reproduced by injecting drift into a pooled TAP on the old
binary — the first allocation failed with the exact production error
signature; clean sequential/concurrent runs never reproduce (rules out a
race). With the fix (full matrix in the linked check doc):

scenario	old	new
unclean kill + randomized MAC + detached bridge, daemon restart	fatal at startup	self-heals, 2 repair warnings, daemon serves
randomized MAC on idle TAP → firecracker restore ×6	1st fails, exact prod signature	6/6 OK + repair warning
randomized MAC on idle TAP → runsc start ×2	same path, fails	2/2 OK + repair warning
runsc start/delete ×3 regression	—	no regression

Gates: gofmt -l empty, go vet ./..., go test ./... green.

Notes for reviewers

• Follow-up in the same family (NOT in this PR): cgroup recovery is also
  fatal after unclean termination when a stale sandbox cgroup dir is missing
  pids.max; deserves the same drop-stale/repair treatment.
• Verification environment: nested-KVM local host, sandboxd @ this branch +
  FC fork @2c6936f6a + runsc release-20260817.0-akernel.1(pinned).

Review follow-up 1 — f7eae7f (Copilot: persist the refreshed lease key)

The ifindex repair only mutated the in-memory struct, so recovery kept the
original usingInterfaces key and re-warned on every restart. Recovery now
swaps the durable key when the serialization changed.

Review follow-up 2 — f8f789f (P1: keep external lease references working)

The durable-key swap above broke the sandbox's externally held copy: the
serialized NetResource is written into the sandbox OCI annotations once at
start and later passed back verbatim to Deactivate/Release. After the
rename, both lookups missed and silently returned success, so the refreshed
lease stayed active forever — leaking its IP and pool slot after sandbox
deletion.

Release-path lookups (Recycle, Deactivate, releaseEphemeral, Discard)
now resolve misses onto the stored key by immutable identity (endpoint type,
interface name — which encodes the IP — and the IP) via resolveLeaseKey,
so the durable key can stay fresh without orphaning external references.
setTapState also routes its host mutations through the injectable
linkOperations seam (the package-level netlink wrappers are inline
candidates that gomonkey cannot patch, which made recovery untestable under
plain go test). Regression test
TestRecoveryIfindexDriftKeepsExternalLeaseReferenceWorking covers the
requested scenario: active ifindex drift → restart (key rename) → sandbox
deletion with the pre-restart annotation string — red before the fix, green
after.

End-to-end verification on a live host (real netlink devices + real durable
store; annotate → external delete/recreate of the TAP → daemon recovery →
Deactivate/Release with the stale annotation string):

commit	recovery	Deactivate(stale) / Release(stale)	final pool state
main (PR base)	fatal: pooled TAP … index is 60, lease records 59 — daemon cannot start	—	—
f7eae7f	self-heals, renames key	both return nil without doing anything	leak: using=1, idle=0
f8f789f	self-heals, renames key	both resolve by immutable identity (warning logged) and execute	recycled: using=0, idle=1

Gates re-run at f8f789f: gofmt -l empty, go vet ./...,
go test ./... green.

How startup handles a dirty tap (the full matrix)

Daemon startup (load()) classifies each host tap.* device by lease state
and repairs what has a deterministic answer:

startup sees	action now
tap leased to a running sandbox, drifted (detached bridge / randomized host MAC / stale ifindex)	repair in place + warn, LinkSetUp, lease kept; ifindex refresh also renames the durable key (external references stay resolvable per follow-up 2)
tap with no lease (adopted into the pool), drifted	same repair + warn, LinkSetDown, queued as idle
tap with an active lease but the device is missing	hard error, startup fatal — nothing to repair
identity/record violations (wrong link type, name ≠ IP encoding, corrupted guest-MAC record, schema too new, legacy pooled-veth lease)	hard error, startup fatal — no deterministic basis to repair from

At allocation, markUsing runs the same validation on freshly-popped idle
taps; an unrepairable one is quarantined (kept in the using set, counted,
never requeued) rather than handed to a sandbox.

Reproducing locally

The P1 chain (ifindex drift → restart → deletion with the stale annotation
string) is covered by the regression test added in f8f789f:

go test ./pkg/networkmanager/ -run 'TestRecoveryIfindexDriftKeepsExternalLeaseReferenceWorking|TestResolveLeaseKeyRejectsForeignIdentity' -v

For an end-to-end check against real netlink devices and the real durable
store (needs root):

# round 1: create bridge+tap, lease it, persist, hard-exit (= unclean death)
sudo env P1REPRO=allocate ./p1.test -test.run TestP1ReproHarness
# external drift: delete + recreate the tap (new ifindex, MAC re-stamped, re-attached)
sudo ip link del tap.0ac70005 && sudo ip tuntap add dev tap.0ac70005 mode tap
sudo ip link set tap.0ac70005 address 02:fd:0a:c7:00:05
sudo ip link set tap.0ac70005 master sandbox0
# round 2: daemon recovery + sandbox deletion with the stale annotation string
sudo env P1REPRO=release ./p1.test -test.run TestP1ReproHarness

Observed matrix across the three commits (main / f7eae7f / f8f789f),
also embedded in the table under follow-up 2. The MAC re-stamp and bridge
re-attach repair branches can be exercised the same way by randomizing the
MAC or detaching the bridge instead of recreating the device.

Review follow-up 3 — f1c3f7c (revised P1 comment: reject replaced active TAPs)

The reviewer sharpened the boundary and it is the better call: sandboxd never
recreates a leased TAP, so an ifindex mismatch on an ACTIVE lease means the
kernel device was replaced externally — and the owning sandbox's networking
is already broken (the guest still holds the old device). Refreshing the
lease record would relabel a broken state as healthy instead of restoring
function.

• Active-lease ifindex mismatch is now a hard error again, checked in
  load() before any repair; the durable lease is left untouched and the
  replacement is never adopted.
• The durable-key swap from follow-up 1 is reverted — recovery no longer
  mutates the stored serialization, so no rename can diverge from the sandbox
  OCI annotations. resolveLeaseKey (follow-up 2) stays as defense in depth.
• MAC re-stamp / bridge re-attach repairs unchanged; the ifindex refresh only
  runs on consumer-less paths (markUsing re-handout, Recycle), where the
  refreshed serialization is what the caller persists and later releases.
• New regression tests: active replacement rejected (load() fails, no
  LinkSetUp); idle orphan with randomized MAC + detached bridge still
  adopted with both repairs; markUsing keeps the stored key and handed-out
  string identical. Gates green.

[Copilot]

Pull request overview

This PR changes pooled TAP validation in networkmanager to repair host-side drift (missing bridge attachment, randomized host MAC, and ifindex drift) during allocation and daemon startup recovery, rather than failing hard when the TAP’s identity-by-name is still valid.

Changes:

• Re-attach pooled TAPs to sandbox0 when the master index drifted, logging a warning instead of aborting.
• Re-stamp the deterministic host MAC (02:fd:<ip4>) when the kernel-assigned/random MAC is observed, logging a warning instead of aborting.
• Treat ifindex drift as non-fatal and attempt to “refresh” the lease record (warn + update in-memory struct).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[hky1999]

Fixed in f7eae7f (durable key swap on recovery)

[hky1999]

Superseded by f1c3f7c: active-lease ifindex drift is now rejected outright at recovery (see tianyuzhou95's thread), so recovery never rewrites the durable key anymore. The remaining refresh paths (markUsing re-handout, Recycle) persist the refreshed serialization through the returned/queued string, which is what the durable key and the handed-out annotation both become.

[hky1999]

False positive: net.HardwareAddr implements Stringer; fmt %s invokes it (verified output is colon-separated), and the pre-PR code used the same pattern.

[tianyuzhou95]

Do not repair active TAP replacement by rewriting the lease key

An active TAP should not be recreated by sandboxd: createTapDevice refuses to replace an existing device, MAC and bridge repairs do not change the ifindex, and a TAP left in the LinkAdd crash window has not yet become a durable active lease. An ifindex mismatch on an active lease therefore means that the kernel device was replaced externally, which is an identity violation rather than ordinary bookkeeping drift. Please keep this case as a hard error instead of accepting the replacement.

Rewriting the key here is also not sufficient to migrate ownership. The original serialized NetResource remains in the sandbox OCI annotations and is later passed to Deactivate and Release. Those operations look up the old string, do not find it after this swap, and return success without deactivating or recycling the TAP, leaving the refreshed lease active after sandbox deletion.

The MAC and bridge self-repair can remain independent of this check. A regression test should verify that active ifindex replacement is rejected while idle orphan MAC and bridge drift are still repaired.

[hky1999]

Addressed in f1c3f7c — agreed on all three points.

• Active-lease ifindex mismatch is a hard error again, checked in load() before any repair: active pooled TAP … was replaced externally (ifindex N, durable lease records M). The durable lease is left untouched for the operator and the replacement is never adopted (no LinkSetUp on it).
• The durable-key swap from f7eae7f is reverted: with active-lease replacement rejected, recovery no longer mutates the stored serialization, so no rename can ever diverge from the sandbox OCI annotations — your second concern is gone at the root rather than patched over. resolveLeaseKey (f8f789f) stays as defense in depth for release-path lookups.
• MAC re-stamp / bridge re-attach repairs are unchanged, and the ifindex refresh now only runs on consumer-less paths (markUsing re-handout, Recycle), where the refreshed serialization is exactly the string the caller persists and later releases.
• Regression tests added per your ask: TestRecoveryRejectsExternallyReplacedActiveTap (replacement rejected) and TestRecoveryRepairsIdleOrphanMacAndBridgeDrift (idle orphan with randomized MAC + detached bridge still adopted with both repairs). Full gates green (gofmt/go vet/go test ./...).

[hky1999]

Design note: how the TAP pool is accounted, and what recovery may and may not heal

Posting the full reasoning behind this change (and the review follow-ups) in one place, since it answers several questions raised in review.

1. Pool bookkeeping

The manager keeps three ledgers:

ledger	contents	survives daemon death?
usingInterfaces (persisted to bbolt under bridgeIp)	active leases — TAPs held by live sandboxes, keyed by the full serialized NetResource	yes — this is the authority for "the IP is taken" across daemon generations
interfaces queue	idle pool — reusable TAP serializations	no — rebuilt at startup by scanning host devices
total / idleIp	capacity ceiling / free IPs	no

Only active leases are persisted: idle state is reconstructable from the host, but a live sandbox's claim must outlive the daemon that granted it. The VM processes hold their TAP fds independently, so a daemon restart never interrupts a sandbox.

Device ownership is expressed by three deterministic conventions:

1. the name encodes the IP (tap.0a580005 ⇒ 10.88.0.5) and must decode inside this daemon's exclusively configured IpRange;
2. the host MAC is a pure function of the IP (02:fd:<ip4>) — both the "ours" stamp and the repair target;
3. the endpoint is attached to this daemon's bridge sandbox0.

2. When recovery self-heals and when it refuses

The criterion is "does the repair restore function, and is it a convergence toward the deterministic expectation?" Relabeling a broken state as healthy is not healing.

situation	verdict	behavior
idle tap: randomized MAC / detached bridge	deterministic drift, repair restores the endpoint	re-stamp / re-attach + warn, adopt into idle pool
active tap: randomized MAC / detached bridge	same, and the repair genuinely restores that sandbox's connectivity	same + LinkSetUp, lease kept
idle tap: stale ifindex in the lease record	pure bookkeeping, no consumer	refresh the record
active tap: ifindex mismatch	the kernel device was replaced externally; the guest still holds the OLD device — that sandbox's networking is already dead	hard error, never adopt the replacement (f1c3f7c)
name ≠ IP encoding / wrong link type / corrupted lease payload	identity violation, no deterministic basis to repair from	hard error
active lease, device missing entirely	nothing to repair	hard error

3. The recovery algorithm (load())

read durable active leases (authority)  +  scan host devices
for every tap.* whose name decodes into our IpRange:
  ├─ active lease exists → belongs to a live sandbox:
  │     identity/schema checks → reject externally replaced device (ifindex)
  │     → repair MAC/bridge → LinkSetUp → lease kept
  └─ no lease → our idle asset (includes create-crash-window orphans):
        repair MAC/bridge → LinkSetDown → adopt into the idle pool
active lease with no device → fatal
devices whose names do not decode into our range → never touched

4. Why recovery cannot delete someone else's TAP

• Recovery never deletes taps at all — it only adopts/repairs. A create-crash-window orphan (device exists, MAC not yet stamped, no lease) is not deleted and recreated; the interrupted creation is simply finished (stamp the MAC it was always going to get, attach the bridge, queue as idle). The name already encodes the IP the device was destined for.
• The namespace is the IP range, not fingerprinting. A device is only even looked at if its name decodes into this daemon's exclusively-configured subnet. Anything else on the host — other processes, other daemons, foreign tools — is structurally invisible. This is precisely why two sandboxd daemons must not share a subnet (m2 runs 10.88, m4-gv runs 10.89); same-range deployment is a configuration-discipline matter, not something code can defend against.
• Within our range, the name IS the claim. A foreign tool creating tap.<our-ip> has, by convention, created our device; adopting (repairing) it is the correct semantics. Worst case we converge a name-colliding device into a pooled endpoint — destructive deletion never happens.
• The paths that do destroy devices are all guarded: shrink only destroys taps already in our idle queue; Discard only destroys our own lease; the deletions inside load() target the veth migration paths and are gated on "no durable lease owns it"; shutdown cleanup() sweeps exactly the two ledgers.

Residual risks, honestly: (a) same-range multi-daemon deployment — ops discipline; (b) a foreign device name-colliding into our range gets adopted — semantically ours, acceptable; (c) one of our taps renamed by external tooling becomes invisible to us — a leak, but harmless until noticed.

5. One asymmetry worth a follow-up (not this PR)

Ephemeral veth leases get a sandbox-liveness cross-check at recovery (lease whose sandbox metadata directory no longer exists is dropped, with the device cleaned up). Pooled TAP leases have no equivalent: a sandbox removed wholesale without going through Deactivate/Release leaves its lease parked as active. The orchestrator's deletion flow is the current guard. A metadata-existence check for pooled leases in load() would close this — same family as the cgroup-recovery note in the PR description.

Merge recommendation

This PR solves exactly one problem — an uncleanly killed daemon (or external tooling) leaves dirty taps in the pool, and the old code answered with "exit" or "refuse to allocate" — and both failure modes were observed in practice (daemon startup fatal during m4-gv debugging; the M5 allocation-failure signature that blocked acceptance). The final shape after the review rounds:

• deterministic drift (MAC/bridge) self-heals — the incidents above become a warning line;
• externally replaced active endpoints are refused rather than adopted (f1c3f7c);
• the durable key is never rewritten by recovery, so sandbox OCI annotations can always resolve their lease (f8f789f resolveLeaseKey kept as defense in depth);
• regression tests pin both halves of the reviewer-requested boundary (active replacement rejected; idle orphan MAC/bridge drift repaired), plus the allocation-path consistency;
• CI is 12/12 green on f1c3f7c, including the privileged network-dataplane suite and all five runtime E2E jobs; local gofmt / go vet / go test ./... green.

Behavior relative to main is a strict improvement: every previously-fatal drift case now heals except externally-replaced active endpoints, which were already fatal on main and remain so, now with an actionable message.

[tianyuzhou95]

Follow-up pushed in 6a1ef4e.

This trims the recovery change back to the intended ownership contract:

• Removed resolveLeaseKey and restored exact serialized-key lookups in Recycle, Deactivate, releaseEphemeral, and Discard. Once active ifindex replacement is rejected during recovery, normal flows no longer produce a durable-key/annotation divergence, so the identity fallback is unnecessary and could cross lease generations when an IP and deterministic interface name are reused.
• Kept the active TAP ifindex hard error added in f1c3f7c.
• Kept MAC re-stamping and bridge re-attachment for unleased orphan TAP recovery.
• Kept refreshed serialization when a consumer-less TAP is handed out or returned to the idle queue.
• Documented in README.md that sandboxd owns and may adopt TAP devices in the host network namespace whose tap.<encoded-ip> names fall within plugin.network.ip_range; other software must not use that reserved namespace.

Validation on the remote test node completed successfully: make check-fmt, make vet, and make test.

[hky1999]

ACK on 6a1ef4e — agreed with the removal, and thanks for tightening the final shape.

Since f1c3f7c removed the only path that rewrote the stored key (recovery no longer mutates the durable serialization), lookups are generation-safe by construction and resolveLeaseKey was a redundant layer rather than a guard. I also re-checked the one edge that layer used to cover — an external replacement happening mid-run, after recovery: the lease is still found by exact match (the key was never rewritten), setTapState converges on the live device by name, and Recycle queues the refreshed serialization, so the pool state converges without any identity resolution.

Verified at 6a1ef4e: the three boundary tests pass (active replacement rejected with no LinkSetUp; idle orphan MAC/bridge drift repaired and adopted; hand-out string stays identical to the stored key), local gofmt / go vet / go test ./... green, CI 12/12. The README namespace reservation is a good addition — it documents the ownership model this recovery relies on.

Ready to merge from our side.

[tianyuzhou95]

Please resolv the conflict and I will merge this pr.

[tianyuzhou95]

There is one related TAP crash-consistency gap worth fixing in this PR. Ordinary pooled Allocate() currently returns after markUsing() updates usingInterfaces and sets storeMark; keepStoring() does not flush that state until its five-second tick. If sandboxd exits after the TAP is handed to a runtime but before the flush, the runtime and TAP can survive while the durable active set does not contain the lease. On restart, load() then classifies that TAP as idle and may hand it to another sandbox.

Could this PR persist the active pooled lease synchronously before Allocate() returns, matching the write-before-handoff boundary already used by AllocateEphemeral()? A StoreRaw failure should prevent allocation from succeeding; the TAP should be returned to a proven-safe idle state, or kept quarantined if rollback cannot be completed safely. A focused regression test should inject the persistence failure and verify that no lease is returned to the caller.

[hky1999]

Follow-up pushed in daa6f1f (plus a merge of current main in def1947 resolving the README conflict — the branch's TAP namespace note and main's ACL schema v2 wording are both kept).

The TAP crash-consistency gap is fixed at the requested boundary:

• Allocate() now routes both the idle-pop fast path and the on-demand create path through a new markUsingPooled(), which persists the active set synchronously before the lease is returned — the same write-before-handoff boundary AllocateEphemeral() uses. The durable active set can no longer lag the handoff until keepStoring()'s five-second tick, so a daemon exit after handoff can no longer recover a leased TAP as idle and hand it to a second sandbox.
• A StoreRaw failure fails the allocation. rollbackPooledHandout() returns the TAP to the idle pool only after setTapState has converged it back to the idle state (link down, refreshed serialization re-queued, as Recycle does); if rollback cannot be completed safely the lease stays active, so the endpoint is quarantined and counted instead of reused. Rollback never deletes a device, matching this PR's adopt/repair-only ownership model.
• The pending storeMark is deliberately left set after a successful synchronous store: clearing it could drop a concurrent Recycle's removal from the next flush, and one redundant re-store on the next tick is harmless.
• Regression tests inject the persistence failure (flakyRawStore) and verify that no lease is returned to the caller; that the endpoint re-enters the idle pool only in the converged state (activated once, set back down, refreshed ifindex); and that an unrollbackable endpoint stays quarantined with the periodic-store mark pending. A success-path test additionally pins that the durable set already contains the refreshed lease the moment Allocate() returns.

The drift-repair and recovery semantics this PR is about are untouched (tap.go / tap_recovery.go have zero diff since 6a1ef4e); the only behavior delta on healthy pools is the durability boundary itself — one synchronous store before return, exactly the cost AllocateEphemeral() already pays.

CI is 13/13 green on daa6f1f, including all five runtime E2E matrix jobs, the incremental firecracker job, the privileged network-dataplane suite, unit tests, and vet; go test -race ./pkg/networkmanager/ is also green locally. With the conflict resolved and the crash-consistency gap closed, this looks ready to merge from our side.