fix(nat): reserve a synced SNAT session on the rule the active matched (#6211)

[psaab]

Closes #6211

The defect

The HA standby picked the source-NAT rule for a peer-synced reservation by "first rule whose pool CONTAINS the translated address". The active node picked its rule by zone/policy match.

Two source-NAT rules can carry the SAME public pool address in SEPARATE allocators — the allocator is shared per SourceNatRule::allocator_key (pool name + addresses + port range), so distinct pool_names with a common member address give one address two independent PortAllocators. Under that config the standby's reservation landed in a different allocator than the active used for the same session, so after a failover a new local flow matching the OTHER rule missed the collision guard: the reverse-identity token sat in the wrong allocator, reintroducing the reverse-path ambiguity the token exists to prevent.

Pre-existing and LOW severity — byte-identical to the shipped port-bearing arm (#4388/#5336); #6210 mirrored it for the address-only case (#5338) without introducing it. Single-rule and non-overlapping-pool configs were never affected.

The design fork: LOCAL, not a wire change

The issue asked whether the rule identity is already available on the standby or needs adding to the sync payload. It is already available. Every input the active's rule match consumes is synced today:

Match axis	Available on the standby?
from zone / to zone	Yes — ingress_zone_id/egress_zone_id on the wire
source / destination address	Yes — the session key
L4 (match destination-port, match application)	Yes — the session key
#3096 interface / routing-instance scope	No — node-local

Both-sides grep confirms the zone carriage in production, not just in tests:

• Go sender — pkg/dataplane/userspace/manager_ha.go, both buildSessionSyncRequestV4 (~L1585) and buildSessionSyncRequestV6 (~L1669): req.IngressZoneID = val.IngressZone / req.EgressZoneID = val.EgressZone.
• Wire struct — pkg/dataplane/userspace/protocol_ha.go SessionSyncRequest.IngressZoneID/EgressZoneID ⇄ userspace-dp/src/protocol/control.rs SessionSyncRequest.ingress_zone_id/egress_zone_id. Tags agree (ingress_zone_id/egress_zone_id).
• Rust receiver — server/helpers/session_sync.rs build_synced_session_entry prefers the IDs and falls back to the legacy name strings via zone_name_to_id, landing them in SessionMetadata::ingress_zone/egress_zone.

No field was added, so protocol_wire_v1.json needs no regeneration (verified: no schema change).

Mixed-version peers. An old peer that carries neither a usable zone id nor a resolvable zone name resolves to None and takes the pre-#6211 first-pool-match — the safe direction the issue called for, never a failed reservation.

Reusing the existing precedent rather than inventing a scheme

The PolicyCounterStore stable-rule_id mechanism (session/entry.rs, session/README.md) solves "a positional id frozen onto a session goes stale after renumbering". That is the right shape if the identity must ride the wire. Here it need not: the standby is not carrying a stale id, it is re-deriving from scratch, and the selecting inputs are already synced. So instead of a second identity scheme, reserve_synced_source_nat_allocation re-runs the active's own predicate.

SourceNatRule::matches is split into shared zone_matches / l4_matches / address_matches axes plus a new matches_ignoring_scope, so the standby and the packet path cannot drift on what "matches" means. The flow key the reservation already built is byte-identical to the active's SNAT-match tuple (original source, POST-DNAT destination, original ports — nat_match_flow.forward_key in poll_descriptor), and, like match_source_nat_result_for_tuple, the narrowed pass takes the FIRST matching rule in snapshot order (that order IS the #4161 Junos specificity precedence).

Why the scope axis is ignored rather than rejected

NatScopeCtx derives from the LOCAL ifindex_to_config_name / ifindex_to_routing_instance maps keyed on the active node's ingress/egress ifindices, which a synced entry does not carry. Rejecting an interface-scoped rule the standby cannot refute would push the selection PAST the rule the active actually used and onto a later one — strictly worse than the first-pool-match it replaces. Ignoring the axis only declines to narrow on it; every other axis still narrows, and the pre-#6211 selection narrowed on none.

Why the fallback is unconditional

Both passes share one reserve_synced_on_first_pool_owner body, so the pool-mode gate, address-index math, address-only vs port-bearing arms and per-rule fall-through cannot diverge. Pass 2 (the pre-#6211 behaviour) runs whenever pass 1 reserves nothing — unresolvable zone pair, no confirmable match owning the address (NAT config drift), or every candidate refusing. No configuration ends up with FEWER reservations than before: the narrowing can only move a reservation to a better-justified allocator, never remove one.

Validation

RED on the parent, before the fix

A probe on ad9591177 with the current (pre-fix) signature — a dmz->wan rule ordered ahead of a lan->wan rule over a shared pool address, and a lan->wan synced session:

test nat::tests_pool::red_probe_6211_overlapping_pools_wrong_allocator ... FAILED
panicked at src/nat/tests_pool.rs:4537:5:
the lan->wan rule's allocator (the one the ACTIVE used) must hold the reservation
test result: FAILED. 0 passed; 1 failed

cargo test --release exit 101. The reservation landed in the dmz->wan allocator.

GREEN after, with every guard watched to fail

9 tests: 2 fail-on-revert (port-bearing + address-only arms), 4 negative controls, 3 axis guards. A 6-case mutation matrix distinguishes them:

Mutation	RED
M1 disable pass 1 (revert to first-pool-match)	follows_active_zone_match, address_only_token_follows_active_zone_match, narrows_on_l4_match, narrows_on_post_dnat_destination
M2 use scope-checked matches with an empty NatScopeCtx	ignores_unconfirmable_interface_scope
M3 delete the pass-2 fallback	without_zone_pair_falls_back_to_first_pool_match, unmatched_zone_pair_still_reserves, single_rule_is_zone_pair_invariant, non_overlapping_pools_is_zone_pair_invariant
M4 drop the L4 axis from matches_ignoring_scope	narrows_on_l4_match
M5 narrow on the PRE-DNAT destination	narrows_on_post_dnat_destination
M6 drop the zone axis from matches_ignoring_scope	follows_active_zone_match, address_only_token_follows_active_zone_match

The two invariance controls assert across BOTH None and Some(("lan","wan")), so single-rule and non-overlapping-pool configs are pinned identical either way — a fix that changed them would be over-reaching, and M3 proves those assertions are load-bearing rather than decorative.

Suite exit codes

Run	Exit
cargo test --release (full)	101 — 4241 passed, 1 failed (see below)
cargo test --release --bin xpf-userspace-dp nat::	0 — 275 passed
cargo test --release --bin xpf-userspace-dp session_glue	0 — 98 passed
go test ./pkg/refactoraudit/	0

The one full-suite failure is afxdp::ha::tests::current_generation_install_and_delete_still_apply_on_poisoned_shared_mutex — a pre-existing flake, not this change:

• It passes in isolation (exit 0).
• The afxdp::ha::tests module alone fails 4 of 6 consecutive runs, with a varying failure set — sometimes also stale_generation_install_refused_on_poisoned_shared_mutex, which this PR does not touch.
• Its assertion is a before/after delta on the process-global session_delete_stale_ignored_total, which a sibling test in the same module deliberately increments — so the delta is contaminated whenever they interleave.
• This diff touches no file under afxdp/ha/.
• The test/ha: five ha_tests share two process-global stale-ignored counters — cross-test race fails the suite ~60% of runs #6819 fix for exactly this process-global-counter flake is not in base ad9591177.

Fold r1 (54854039e) — a SECOND production change, stated here because the title does not cover it

The rev6876 gate found that the two-pass selection introduced a permanent standby pool-port leak, and fixing it required a change to the RELEASE path that the PR title ("reserve ... on the rule the active matched") does not describe. Stating it explicitly rather than leaving the diff wider than the title:

Selection is no longer a pure function of rules, so a session re-upserted after the selection outcome changes (zone delete/renumber → synced_zones becomes None; a rule-set from zone / match edit moves pass 1's candidate set) reserves a SECOND time in a different, independent allocator — reserve_flow's idempotence is per-allocator. Every live session re-upserts on HA session-sync reconnect and on a post-delete-journal-overflow resync. release_source_nat_allocation stopped at the first allocator reporting released, stranding the other forever; nothing reaps it and it counts against max_tracked_flows to eventual AllocatorExhausted.

release_source_nat_allocation now frees from EVERY pool-mode rule instead of breaking at the first hit. It cannot over-free — release_flow / rollback_flow return false unless the stored translated tuple matches — and single-reservation cases are bit-identical.

Fold also binds the previously-uncovered production call site (handle_upsert_synced), corrects the session_glue/README.md sentence that asserted the opposite of the leak, and bounds the scope in code + docs: #5144 hard-rejects the motivating duplicate-pool config at strict commit, so the live surface is only pre-#5144 persisted configs and the tolerant load / peer-sync path.

Fold mutations, each RED on exactly one test: restore the release break → the leak test only; call site passes None → the call-site test only; invert ingress/egress in the helper → the call-site test only. Under both call-site mutations all nine original tests stay GREEN.

Docs

• docs/session-sync-architecture.md — a "Rule selection (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)" bullet in the synced-reservation section.
• userspace-dp/src/afxdp/session_glue/README.md — records that the release path is unaffected: release_source_nat_allocation scans every pool-mode rule and stops at the first allocator reporting the flow released, so it locates the reservation wherever the reserve put it (it does not assume the same rule-selection rule).

Notes for the reviewer

• This touches HA session sync, so it needs make test-failover before merge. Not run here — cluster tooling is the lead's to schedule.
• userspace-dp/src/nat/source.rs grows 1765 → 1895 lines. Tier is unchanged ([WATCH], 1500–1999), but it is now ~105 lines from the [REFACTOR] threshold.

[psaab]

Hostile gate at 592a04821 — MERGE-NEEDS-MINOR. No fail-open introduced; the cross-node rule identity is sound.

Correcting my own scoping first: I measured this PR with a Go-scoped filter and read "zero non-comment production-Go lines". True but vacuous — the change is 100% Rust (nat/source.rs, nat/mod.rs, session_glue/commands/upsert_synced.rs).

Identity is not an index or ordinal. The standby receives the inputs the active's matcher consumed and re-runs the matcher: the zone pair (ingress_zone_id/egress_zone_id, on the wire since #919/#922) plus the 5-tuple. StableZoneID (pkg/config/zoneid.go:38) is FNV-1a/64 of the zone NAME alone — never the rest of the zone set, compile order, or allocation history — so adding or renaming a zone cannot renumber another. The stamped pair and the matched pair come from the same two variables at poll_descriptor/mod.rs:1448-1459 / :2408-2409, so they cannot diverge.

The drift worry does not hold: pass 1's candidate set is a strict subset of pass 2's and both take first-match, so pass 1 can only select at the same or a later index — never earlier, never none. And a reservation on a wrong allocator is weakly better than none, which leaves the port reusable in every allocator. Monotone: strictly stronger than what shipped, in every config. Unresolvable zone pair or no matching rule both fall through to the pre-#6211 path at source.rs:1046.

The fixture makes X and Y genuinely differ — overlapping_pool_rules_6211() gives snat-dmz/pool-dmz and snat-lan/pool-lan sharing one address, and because allocator_key() includes pool_name, the two allocators are separate rather than a shared Arc. 8-cell mutation grid, all assertions, compile_errors=0.

Folding:

• MINOR 1 — the release-sweep rationale at source.rs:882-886 calls it "a cold teardown path", but the same body backs rollback_source_nat_allocation from five packet-path sites including the admission-refusal arm (poll_descriptor/mod.rs:2374), i.e. the flood regime. Per refused SNAT'ed flow the lock count goes from (owning index + 1) to K. Mechanism only — no throughput measurement was taken and none is claimed.
• MINOR 2 — "at most one allocator could ever hold a given flow" holds only against an unchanged rules; parse_source_nat_rules_with_previous carries allocators over keyed on allocator_key() alone.

Noted, not blocking: NAT64 synced sessions never reach pass 1 (cross-family key, identical to master) and the #6211 fixtures are IPv4-only.

[psaab]

Gate state at 567aaedd5 — 2 of 3 legs in, parent mutation proof PASSED

Recording this on the PR so the state survives the conversation it was produced in.

Parent mutation proof — a 6-cell DISCRIMINATING matrix

Each mutation was required to turn a specific subset RED, not merely "something". Measured on the nine *_6211 tests in nat::tests_pool:

mutation	RED	GREEN
M1 disable pass 1 entirely (revert to pre-#6211 first-pool-match)	4	5
M2 use the scope-checked matches with an empty scope ctx	1	8
M3 remove the fallback	4	5
M4 drop the L4 axis	1	8
M5 key the destination pre-DNAT instead of post	1	8
M6 drop the zone axis	2	7

The value is in which cells fire, not the counts: M4 kills only narrows_on_l4_match_6211; M5 kills only narrows_on_post_dnat_destination_6211; M2 kills only ignores_unconfirmable_interface_scope_6211; M6 kills exactly the two zone-match tests; M3 kills exactly the four invariant/fallback tests. Every mutation is distinguished by the test whose name describes that axis, and no mutation reddens the whole suite — which is what separates a real matrix from an over-strong mutation that reddens everything and proves nothing.

The matrix was measured at 6d881ebf8. It still holds at this head: git diff 6d881ebf8 567aaedd5 -- userspace-dp/src/nat/source.rs shows comment-only changes, so the production logic under mutation is unchanged.

Independent AGY leg at 567aaedd5 — MERGE-READY

Given a narrow brief over named files (no tree search), it answered four questions with quoted code:

• Release completeness. Every reservation reserve_synced_source_nat_allocation can take is releasable, because release_source_nat_allocation_with_mode sweeps all pool-mode rules (for rule in rules, source.rs:906-915) rather than stopping at the first match. It constructs the identical flow key and translated tuple as the reserve path.
• Double-take. A re-upsert cannot strand a reservation: reserve_flow short-circuits for the same flow key, and if selection moved, the all-rules sweep still frees both.
• Wrong-allocator selection. Consequence is a possible double-allocated port, not a leak — the sweep frees the wrongly-chosen allocator at teardown.
• Claim audit, source.rs:870-899. All five universal claims (every, only, cannot, all) check out against the code.

On the one place the two legs read differently

The earlier hostile leg called a wrong-allocator reservation "weakly better than none"; AGY describes the same state as "a double-allocated port". Both are right about the mechanism and they are not in conflict — AGY is describing the residual, not a regression. Before #6211 no reservation was taken, so the port was free in every allocator; after it, the port is held in at least one. The set of allocators that can collide shrinks; it does not grow. The change is monotone in every config, which is why neither leg treats it as blocking.

Folded this round

Both MINORs from the hostile leg are closed at 9ba61f27c, and the fix put the caveat in the shipping artifact rather than in a review comment: the release-sweep rationale now states that this body also backs rollback_source_nat_allocation from five packet-path call sites including the admission-refusal arm (the flood regime), that the sweep costs K allocator locks instead of (owning index + 1), and — explicitly — "this is a mechanism statement: no throughput measurement was taken and none is claimed."

Codex leg still running; merge is gated on it.

[psaab]

Independent Codex leg at 567aaedd5: DO-NOT-MERGE — seven blocking findings, and I have verified the central one myself

This overturns the gate state I posted earlier. My mutation matrix and the AGY leg both passed, and both were answering a narrower question than this PR needs to answer. The matrix proved that rule-identity selection is bound — which it is. Neither leg asked whether the reservation survives the lifecycle, and that is where the defects are.

Codex could not write its cargo target dir in its sandbox and says so: its per-assertion mutation table is explicitly labelled static prediction, not measurement. I weighted it accordingly and verified the load-bearing finding directly.

Finding 2, verified firsthand — the reservation has no refcount across worker replicas

Every synced entry is sent to every worker (ha/session_import.rs:267) and each worker has its own session table. The allocator is Arc-backed and shared. I read both halves:

allocator.rs:1732-1734 — reserve_flow is idempotent with no refcount:

if let Some(existing) = live.live_by_flow.get(&flow).copied() {
    if existing.translated == translated {
        return true;          // already reserved — nothing to do
    }

allocator.rs:1386-1392 — release_flow removes unconditionally:

let Some(existing) = live.live_by_flow.get(&flow).copied() else { return false };
if existing.translated != translated { return false; }
live.live_by_flow.remove(&flow);

So N workers reserving one synced flow collapse to one live_by_flow entry, and the first worker to age out its replica — or to handle DeleteSynced — removes it while the other N−1 still hold and forward. There is no refcount and no all-workers-retired barrier. Codex's schedule: two workers, packets hash only to W0 after promotion, W1 crosses its idle timeout, W1 frees the sole global reservation, W0 keeps forwarding, a new flow can take the port. Affects source NAT and NAT64.

This is worth being precise about, because the earlier AGY leg answered "can one session take two reservations?" correctly — no — and that correct answer concealed the symmetric defect. The take side is 1:N-collapsed; the release side is 1:1. Idempotence is not the safety property here, it is the mechanism.

Finding 1 — a transient collision installs the session with no reservation at all

The helper does not reserve on the matched rule; it reserves on the first candidate whose allocator currently accepts the tuple (source.rs:1060, :1129). If an unrelated flow G holds that tuple when synced flow F arrives, F installs with no reservation; when G retires the port frees, and after failover a new flow can take it while F is live. With an overlapping later rule B the standby instead records F in B, so allocator selection differs across HA nodes under identical configuration, zones and tuple. That is an unclosed instance of the original #6211 failure this PR set out to close.

Findings 3–7, summarised

3. Accepted same-key replacement discards the previous accounting identity (install.rs:295, :322); the handler reserves only the new decision.
4. DeleteSynced can be lost across worker shutdown — the coordinator removes the key from shared authority first (session_import.rs:350) and queues the delete after (:373), so a worker that stops in between never releases, and the carried allocator holds used_ports=1 until process restart. A contained worker panic makes it deterministic.
5. The full-reconcile tunnel-remap purge runs after workers stop (reconcile/snapshot.rs:565), same permanent-retention outcome.
6. allocator_key() is pool name + addresses + port range only (source.rs:327), so a match-only edit leaves a reservation in an allocator that can no longer match it, while the rule that now owns the flow is free to reissue the tuple. Distinct keys can also collapse when a pool is renamed onto another's identity, discarding the live state of one.
7. NAT64 is not merely at pre-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 behaviour — it has its own selection bug and a permanent double-reserve. The active picks a NAT64 allocator by original IPv6 prefix (nat64.rs:1009); the standby ignores the prefix and takes the first NAT64 pool that accepts. Two prefixes sharing one pool address therefore diverge across nodes. Strict commit rejects that overlap, but lenient load and peer-sync warn and install it. And the delete loop stops after the first successful release (nat64.rs:1261), so a replay that reserved in both A and B leaves B held forever.

The claim items

The rollback-sweep correction from the previous round is confirmed real — but two of its cited line numbers (2634, 4902) are stale by ten lines; the true sites are 2644 and 4912. And "at most one allocator under unchanged rules" is still false: selection depends on allocator occupancy, not just on rules, so a refuse-then-accept sequence puts one flow in two allocators with the rule set untouched. The source sweep frees both, so it is not unreleasable — but the invariant as stated, and "every pre-#6211 config", are wrong.

Disposition

Findings 1 and 2 reopen the exact defect class #6211 exists to close, so they block here rather than being filed. The remainder may be separable — the lane should propose a scope split before implementing rather than attempting all seven in one round, and should say explicitly if any of them is a design fork rather than a bounded fix.

Superseding my earlier "waiting on Codex only" note: this PR is not near-bar.

[psaab]

Scoping decision: F1 is PRE-EXISTING on master and does not block here — split to #6979

Correcting the disposition I posted with the Codex verdict. That leg framed F1 as "an unclosed instance of the original #6211 failure", which reads as this PR failed to close what it set out to close. I checked it against master rather than against the PR's own diff, and that charge does not hold.

git show origin/master:userspace-dp/src/nat/source.rs — the reserve loop at 892-951 has four continues past refusing rules and breaks only on success, and master's own comment at :892 says a collision "leaves the rule untouched and tries the next". So master already installs a synced session with no reservation when every rule refuses, and which allocator holds a flow already depends on occupancy rather than on rules alone. With two passes instead of one, this PR is if anything less likely to end with no reservation.

F1, F7b, F3, F4, F5 and F6 are therefore tracked in #6979 with that verification recorded, so the attribution does not drift back later.

What stays here — this PR's own debt

F2 — the missing multi-worker refcount. Confirmed, and the characterisation is sharper than the original finding: this is not a collision window, it is the expected steady state after any failover carrying a synced SNAT session that outlives the inactivity timeout. Pre-failover the active's periodic re-UpsertSynced keeps all N worker 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 the first to expire in reap_expired_sessions (worker/loop_body/mod.rs:1626) frees a port the still-forwarding worker is using. N is 6 on the reference cluster. Traced through every link, not executed — the guard is what converts it to evidence.

It is a bounded fix, not a design fork: both production release sites are already per-worker (session_glue/mod.rs:563, commands/delete_synced.rs:38) and so is the reap, with coordinator/status.rs:662 being #[cfg(test)]. Take is N and release is already N; the reservation simply cannot distinguish one holder finishing from all of them. An all-workers-retired barrier would be a fork and is not required.

Implementing as a holder set (worker-id bitmask) on LiveAllocation rather than a bare counter. A bare counter makes worker-exit drain load-bearing in a way it is not today: a worker exiting without draining leaves the count above zero permanently, with no way to identify whose contribution to drop. A per-worker bit is deterministically clearable by that worker's teardown, so the #6979 items degrade to "that worker's bit is cleared" instead of "this port is unrecoverable". Both directions are fail-closed; this one does not buy a permanent-leak mode to fix a collision. The mask width must be made structurally impossible to exceed — a set-no-bit take paired with a clear-no-bit release would reintroduce the exact bug via the fix.

F7a — the NAT64 release's first-hit break (nat64.rs:1261) stays here. It is the exact break this PR removed from the source-NAT release, left in the parallel path, while this PR's own comment at source.rs:869-886 explains why it is wrong and calls the resulting retention permanent. Whatever that path's history, fixing one of two parallel paths and leaving the other contradicting the PR's stated reasoning is an inconsistency this PR created.

The invariant at source.rs:862 is rewritten rather than re-scoped: its premise "Before #6211 the reserve was a pure function of rules" is false on master, and master documents that it is false.

Two stale citations: 2634/4902 → 2644/4912 (the other three are correct; the two stale ones point at ordinary comment lines).