fix(loadbalancer): clean up NFQLB targets before unlinking shm

[bszirtes]

fix(loadbalancer): clean up NFQLB targets and flows before unlinking shm

Split out of #281 at review request — this is 1a + 1d only. See
Relationship to #281 at the bottom for what follows separately.

The bug

DeleteInstance unlinked the shm segment before deactivating the
instance's targets:

1. delete(nfqlb.instances, name) — release the fwmark offset immediately
2. nfqlb.mu.Unlock()
3. nfqlb delete --shm=<name> — unlink the shm segment
4. loop deleteTargetNoLock → nfqlb deactivate --shm=<name>
5. list + delete flows

Step 4 cannot work after step 3. nfqlb deactivate has to mmap the segment,
which no longer exists, so it fails with FAILED mapSharedData. And
deleteTargetNoLock returns early on that error — before its
doDeletePolicyRoute loop — so the ip rule and routing table for every
target leak.

This is not a race and not restart-dependent. It is the always-taken path on
ordinary teardown of any DistributionGroup that has active targets. Reproduced
on a kind cluster at 30/30 rounds: one failed cleanup per DG deletion.

Releasing the offset in step 1 compounds it. getOffset packs ranges and
reuses freed ones, so the next DistributionGroup can be allocated the same
fwmark range and inherit live policy routes pointing at the previous DG's Pod
IPs.

Why it matters beyond the leak

Policy rules and routing tables accumulate in the LB Pod's netns on every DG
deletion. Nothing reclaims them within the process lifetime — CleanupStaleRules
only runs at startup. Under steady-state DG churn the tables grow without bound,
and the offset reuse means a new DG can silently route to a dead Pod.

The fix

Reorder to targets/routes → flows → shm unlink, and release the offset only
once every step has succeeded, so a partial failure is retried instead of
silently losing the instance while its kernel state survives.

Also drops a stale comment that justified an early nfqlb.mu unlock by
reference to a heal() function that does not exist in this repo.
DeleteInstance now holds nfqlb.mu for its full duration, matching
AddInstance, which already did.

Reviewer note — lock scope. Holding nfqlb.mu across the exec calls
means a slow or hung nfqlb subprocess blocks all instance management. The
controller already serialises these (MaxConcurrentReconciles unset → 1, plus
c.mu), so this is not a new serialisation point, but it does widen the window.
Relevant to #270, which reproduces nfqlb init hanging indefinitely under
concurrent access — a timeout on these calls is tracked separately.

cleanupDistributionGroup error handling

Previously the DeleteInstance error was logged and dropped, and the tracking
maps were cleared unconditionally. That made the failure unrecoverable:
reconcileNFQLBInstance only acts on DGs absent from c.instances, and a
deleted DG never reappears to re-trigger it, so nothing would ever call
DeleteInstance again. The error is now returned for requeue and the maps are
cleared only on success.

The readiness-file removal is attempted independently of the NFQLB cleanup.
Serialising them would let a stuck DeleteInstance keep a deleted DG's VIP
advertised over BGP indefinitely, since readiness.Manager.IsReady() keys off
file presence.

Background: why the ordering is load-bearing

nfqlb does not forward packets; it only marks them, and the kernel forwards
via policy routing. The fwmark value is the routing table ID (routing.go
sets rule.Table = fwMark and rule.Mark = uint32(fwMark)).

packet → nftables (dst is a VIP) → NFQUEUE → nfqlb
         nfqlb matches a flow → Maglev lookup in /dev/shm/<DG-name>
         → picks target slot → sets fwmark = instance.offset + slot
                                    ↓
         ip rule "from all fwmark 5002 lookup 5002"
         table 5002: "default via <podIP>"  → forwarded

So every fwmark nfqlb can emit needs a matching ip rule + routing table,
created by AddTarget and removed by DeleteTarget. Removing them requires a
successful deactivate, which requires the segment to still exist.

Tests

All three were verified to fail against the previous code before being
committed — they are regression tests, not coverage padding.

internal/nfqlb — two specs:

• cleans up policy routes and keeps the offset reserved when the shm unlink
  fails: injects a failing unlink, asserts routes were still deleted and the
  offset was not handed to the next instance.

• unlinks the shm segment only after targets and flows are cleaned up:
  observes the real call order. The unlink and flowList both shell out via
  exec.CommandContext and bypass the injectable execCmd, so the only way to
  place the unlink relative to the deactivate is to have the fake binary log
  its own invocations — execCmd is deliberately not injected here so
  deactivate lands in the same ordered log.

  My first version of this spec used a stub that always succeeded, which made
  it pass against the buggy ordering too. Rewritten to actually discriminate.

internal/controller/loadbalancer — one spec asserting a failed
DeleteInstance is returned for requeue, keeps the DG tracked, and still
removes the readiness file. The mock gains a deleteInstanceErr injection point.

Relationship to #281

#281 bundled five changes found in one investigation. Per review, they are being
split. This PR is 1a (DeleteInstance ordering) + 1d
(cleanupDistributionGroup error propagation), which the reviewer agreed could
go together.