nat: free a synced NAT reservation only when the last worker releases it (#6211 F2)

[psaab]

Stacked on #6876 (base fix/6211-synced-snat-rule-identity @ ad439e5a0). Advances #6211 — this is the F2 holder-set item; it does not close the issue.

The defect

An HA-synced session is pushed to every worker's session table (afxdp/ha/session_import.rs fans UpsertSynced out to each worker's command queue), while the source-NAT / NAT64 allocator is a single shared Arc. So N workers reserve the same (flow, translated) against one allocator, and each releases it independently — the GC reap, the replicated DeleteSynced, and the alias purge all run per worker.

• nat/allocator.rs — reserve_flow returned true and did nothing when the flow already held that exact translated tuple. Idempotent, no refcount.
• nat/allocator.rs — release_flow removed the record unconditionally once the tuple matched.

The N reserves therefore collapsed into one record, and the first worker to let go freed a (pool_addr, port) the other N-1 were still forwarding through — the allocator then handed it to the next local flow. That is a NAT source collision / session-hijack surface.

This is not a race window. Pre-failover the active's periodic re-UpsertSynced keeps all N replicas alive. Post-failover that refresh stops and RSS lands traffic on exactly one worker, so the other N-1 idle out with nothing refreshing them, and whichever expires first in reap_expired_sessions frees the live worker's port. It is the expected steady state after any failover carrying a synced SNAT session older than the inactivity timeout. N is 6 on the reference cluster.

The fix — a holder set, and why not a counter

LiveAllocation.holders: u128, one bit per worker_id. The bit is OR-ed in at reserve_flow's and reserve_address_only's idempotent early return.

That site is load-bearing for two reasons at once: it is where workers 2..N land and the path an already-holding worker takes on every refresh (each HA session-sync reconnect, each periodic re-upsert). OR is idempotent there where increment is not — a counter would climb without bound on every re-sync and never drain to zero, so the port would never be freed at all. That is the failure a bare refcount ships, and it is what the refresh cell below pins.

LiveAllocation stays Clone, Copy (both read paths use .copied()); no Vec/HashSet enters the per-flow record.

Semantics:

• holders == 0 — an untracked local allocation. RSS steers a 5-tuple to exactly one worker, so a local allocation has a single holder by construction; first release frees, bit-identical to today.
• non-zero mask — frees only when the last holder's bit clears.
• an Untracked release of a tracked reservation keeps it. Deliberate direction: an under-release leaks a bounded, observable pool port (AllocatorExhausted), whereas an over-release is exactly the bug being closed.

Completeness is compile-enforced, not asserted

The threading is 20 production call sites. Rather than claim it is complete, the untracked entry points (release_source_nat_allocation, reserve_synced_source_nat_allocation, rollback_source_nat_allocation and their NAT64 twins) are now #[cfg(test)]. A production path that forgot to thread its worker id fails the release build. cargo build --release is green with those entry points excluded from the crate, which is the proof.

worker_id comes from WorkerLaunchPlan::worker_id — the worker's own identity established at spawn, not read off a BindingWorker slot — threaded through apply_worker_commands, reap_expired_sessions, resolve_flow_session_decision, delete_terminal_filtered_session, purge_translated_synced_hit, the DSCP-revalidation purge, and the nine packet-path rollbacks (poll_descriptor already received _worker_id as an inert parameter; it is now live).

The width, and where the bound is enforced

MAX_NAT_HOLDER_WORKERS = 128, tied to the mask width by const _: () = assert!(u128::BITS == MAX_NAT_HOLDER_WORKERS); so the two cannot drift apart.

Enforced at the mint site: replan_bindings_from_candidates refuses the whole plan (fail-closed — no bindings, no forwarding) if any queue_id % workers would exceed it. A silent set-no-bit take paired with a clear-no-bit release would reintroduce the original over-release through its own fix.

It is deliberately not a cap on --workers. queue_count is the per-interface RX-queue minimum, computed independently of workers, and the id is queue_id % workers with queue_id < queue_count — so minted ids span [0, min(queue_count, workers)). Capping --workers would false-refuse a safe box: --workers 200 on a 16-queue NIC mints ids 0..15. A dedicated cell pins that.

A CLI validator mirroring validate_ring_entries_arg was considered and dropped: any check on the raw --workers value can only refuse configurations the mint-site check would allow, which is the false-refusal above.

Validation

Fail-on-revert, RED first

Reverting only the three-line load-bearing hunk (the holders |= holder.bit() in reserve_flow's idempotent early return) via edit — diff against the pristine copy confirms that is the sole delta:

test nat::tests_pool::synced_reservation_survives_first_worker_retire_6211_f2 ... FAILED
panicked at src/nat/tests_pool.rs:6565:5:
#6211 F2: worker 1 still holds this synced session, so worker 0's retire must NOT
free (203.0.113.1, 10000) — freeing it hands a live worker's NAT source tuple to
the next local flow
test result: FAILED. 7 passed; 1 failed; 0 ignored; 4290 filtered out

RED_RC=101, and 0 error[ lines — an assertion failure, not a build break. Exactly one cell went RED.

What stayed GREEN under that revert

Cell	Role	Under revert
synced_reservation_frees_on_last_worker_retire_6211_f2	last holder must free (leak guard)	GREEN
synced_reservation_refresh_by_one_worker_does_not_accumulate_holders_6211_f2	the cell a bare counter fails — 8 re-reserves by one worker, one retire frees	GREEN
local_allocation_still_frees_on_first_release_6211_f2	over-reach guard — a local allocation must NOT become refcounted	GREEN
synced_address_only_token_survives_first_worker_retire_6211_f2	#5338 address-only arm	GREEN
replan_refuses_worker_ids_beyond_the_nat_holder_mask_6211_f2	mint-site refusal	GREEN
replan_accepts_the_widest_representable_worker_id_6211_f2	boundary accepted (defeats "refuse everything")	GREEN
replan_accepts_huge_worker_count_on_a_small_queue_nic_6211_f2	false-refusal control — --workers 200 on 16 queues accepted	GREEN

The address-only binder staying GREEN is informative rather than weak: it binds a different production site (reserve_address_only's own early return), which this mutation did not touch. The two binders are independent.

The two-worker fixtures use two workers by construction — a single-worker fixture cannot express this property and would pass forever.

Suite

Run	Exit
cargo test --release (full, exit code captured unpiped)	0 — 4296 passed, 0 failed, 2 ignored
cargo test --release --no-run	0
go build ./...	0

The pre-existing afxdp::ha::tests process-global-counter flake documented in #6876 did not fire in this run. 0 Go files differ from ad439e5a0; the repo's gofmt -l hits are pre-existing and verified byte-identical to the PR head.

Docs

• docs/session-sync-architecture.md — a "Per-worker holder set (nat/HA: synced source-NAT rule-selection picks first-pool-match, can diverge from the active node's zone/policy match under overlapping pool addresses #6211 F2)" bullet and a "Worker-id bound (nat/HA: synced source-NAT rule-selection picks first-pool-match, can diverge from the active node's zone/policy match under overlapping pool addresses #6211 F2)" bullet in the synced-reservation section.
• userspace-dp/src/afxdp/session_glue/README.md — records that every release site in the module now takes a worker_id and why the untracked twins are #[cfg(test)].
• _Log.md per project convention.

For the reviewer — attack these first

1. The stale-tuple replace path in reserve_flow. When a synced decision's translated tuple changes for the same flow key, the first worker to process it frees tuple A while workers 2..N still hold A, then re-inserts with a fresh mask. I argue this is not a regression (pre-fix, that path freed A immediately too, and the same fanned-out command updates every worker to B), but it is the least obvious corner.
2. delete_terminal_half takes worker_id FIRST; the other threaded functions take it LAST. The coordinator asked for first-position throughout. I placed it last on the wide functions so ~40 test call-site updates became mechanical appends rather than positional inserts, which materially cut mis-ordering risk; u32 cannot silently swap with the neighbouring c_int/u64/usize params. Happy to normalise to first-position on request.
3. HA session sync is touched, so this needs make test-failover before merge. Not run here — cluster tooling is the lead's to schedule.