Keep NAT tuples exclusive across overlapping pools

[daniel-noland]

Reworked and consolidated version of the stack ending in #1700

Summary

• Keep masquerade tuple allocation exclusive across overlapping public ranges and reused private address space.
• Lease an overlapping public tuple only while port-forwarded flows are live. Clients of one listener share the lease, and allocator replacement carries it forward.
• Retry port-block handover races and keep weak-map cleanup from removing a replacement block.
• Add IPv6, exhaustion, carry-over, flow-invalidation, graceful-close, error-mapping, Shuttle, and Miri coverage.

Configured port-forward rules reserve no masquerade capacity. Well-known ports and tuples outside masquerade pools need no lease.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c72a3f0b-b7cb-4502-87db-50604a71d062

📥 Commits

Reviewing files that changed from the base of the PR and between 43b320a and 1b089d7.

📒 Files selected for processing (1)

• nat/src/portfw/test.rs

---

📝 Walkthrough

Walkthrough

The PR refactors NAT allocation around disjoint address regions and VPC-aware pool keys. It adds shared port-forward leases, flow reconciliation, concurrency-model tests, Bolero fuzzing recipes, sanitizer guidance, and direct nextest coverage reporting.

Changes

NAT allocator and flow integration

Layer / File(s)	Summary
Region-based pool model nat/src/masquerade/apalloc/*	Overlapping public ranges now use disjoint regions and source/destination VPC-aware pool keys.
Address and port allocation nat/src/masquerade/apalloc/alloc.rs, nat/src/masquerade/apalloc/port_alloc.rs	Allocation preserves non-exhaustion errors, bounds IPv6 mappings, and hardens bitmap and weak-entry updates.
Flow reconciliation and port-forward leases nat/src/masquerade/flows.rs, nat/src/portfw/*, nat/src/masquerade/nf.rs	Port forwarding reserves shared NAT leases. Masquerade flow replacement reconciles active flows and validates VPC discriminants.
Allocator and flow validation nat/src/masquerade/*/test*.rs, nat/src/test.rs	Tests cover overlapping pools, IPv4 and IPv6 allocation, allocator replacement, lease persistence, flow invalidation, and TCP closure.
Concurrent allocator validation nat/src/masquerade/apalloc/concurrent_fuzz.rs, nat/src/masquerade/apalloc/display.rs	Concurrent scenarios exercise allocation, release, reservation races, pool formatting, and stale weak-entry cleanup.

Concurrency and test tooling

Layer / File(s)	Summary
Model-test macro API concurrency-macros/src/lib.rs, concurrency/src/macros.rs, flow-entry/src/flow_table/concurrent_fuzz.rs	The public model_test macro emits backend-specific test leaves and is used by the concurrency fuzz test.
Fuzzing and coverage workflows justfile, development/code/running-tests.md, miri.just, .gitignore	Fuzz target discovery, configurable campaigns, sanitizer commands, direct nextest coverage, Miri flags, and worker-log handling are documented or configured.

Possibly related PRs

• githedgehog/dataplane#1548: Related NAT and port-forwarding flow integration.
• githedgehog/dataplane#1696: Related allocator, pooling, fuzzing, and concurrency changes.
• githedgehog/dataplane#1702: Related masquerade flow reconciliation and allocator update handling.

Suggested reviewers: mvachhar, fredi-raspall

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.23% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: exclusive NAT tuple allocation across overlapping pools.
Description check	✅ Passed	The description directly explains exclusive tuple allocation, leases, race handling, and expanded test coverage.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR reworks NAT masquerade/port-forwarding interactions to correctly manage overlapping public tuple capacity at runtime (via leases) while significantly expanding allocator correctness, concurrency, and fuzz/test coverage.

Changes:

• Integrate port-forwarding with the masquerade allocator by leasing overlapping public tuples only while port-forward flows are live (and carrying leases across allocator replacement).
• Rebuild masquerade pool construction to safely handle overlapping public ranges and shared private address space across VPCs (region decomposition + per-(src,dst) pool keys).
• Add extensive unit, property, and concurrency tests (including Shuttle/Miri-oriented fixes and new just helpers for fuzzing/coverage).

Reviewed changes

Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
nat/src/test.rs	Updates pipeline setup for the new port-forwarder allocator handle and adds overlap regression tests.
nat/src/portfw/test.rs	Adjusts port-forward tests for the new allocator dependency.
nat/src/portfw/nf.rs	Reserves/maintains masquerade tuple leases for port-forwarded flows and updates flow state accordingly.
nat/src/portfw/mod.rs	Re-exports lease update helper for cross-module reconciliation.
nat/src/portfw/flow_state.rs	Introduces PublicTuple + lease tracking and provides lease migration helper for live flows.
nat/src/masquerade/test.rs	Adds broader coverage: graceful close, allocator replacement carry-over, overlapping prefix scenarios, etc.
nat/src/masquerade/state.rs	Removes Clone from masquerade state to prevent accidental allocation duplication in tests.
nat/src/masquerade/nf.rs	Adds discriminant validation, adjusts allocation call signature, and scales timeouts for emulated tests.
nat/src/masquerade/natip.rs	Simplifies NatIp trait and adds explicit to_addr_bits to avoid method-resolution pitfalls.
nat/src/masquerade/mod.rs	Exposes allocator reader internally for port-forward integration.
nat/src/masquerade/icmp_handling.rs	Hardens ICMP error handling against missing state/discriminants.
nat/src/masquerade/flows.rs	Reconciles both masquerade allocations and port-forward leases across allocator changes/removal.
nat/src/masquerade/apalloc/test_alloc.rs	Adds/extends allocator tests for overlap, IPv6, port-forward leasing, and region behavior.
nat/src/masquerade/apalloc/setup.rs	Refactors pool construction: gather exposes, decompose overlaps into shared regions, build pool sets.
nat/src/masquerade/apalloc/region.rs	New region decomposition logic + property tests to guarantee disjoint/maximal regions and ordering.
nat/src/masquerade/apalloc/port_alloc.rs	Improves reservation/lookup race handling and makes accounting errors observable.
nat/src/masquerade/apalloc/pool_fuzz.rs	New property fuzz tests for uniqueness, bounds, reuse, carry-over, and IPv6 edge cases.
nat/src/masquerade/apalloc/natip_with_bitmap.rs	Updates IPv6 reverse mapping to return errors instead of panicking.
nat/src/masquerade/apalloc/display.rs	Adds PoolSet display and avoids allocator deadlocks while formatting.
nat/src/masquerade/apalloc/concurrent_fuzz.rs	New concurrent fuzz suite for allocator publication/replacement and race regressions.
nat/src/masquerade/apalloc/alloc.rs	Adds PoolSet abstraction, region-based pools, better error handling, and deadlock avoidance.
nat/src/masquerade/allocator_writer.rs	Switches to unified reconciliation flow and ensures allocator removal detaches state appropriately.
nat/src/masquerade/allocation.rs	Adds is_exhaustion() classification with tests ensuring consistent error→DoneReason mapping.
miri.just	Ensures Miri runs with consistent flags and avoids incremental compilation sharing across workers.
justfile	Adds fuzz/fuzz-list helpers and simplifies coverage workflow.
flow-entry/src/flow_table/concurrent_fuzz.rs	Marks stress test as #[concurrency::model_test] for backend-specific nextest filtering.
development/code/running-tests.md	Documents running a real libfuzzer campaign via just fuzz.
dataplane/src/packet_processor/mod.rs	Wires allocator handle into the port-forwarder stage construction.
concurrency/src/macros.rs	Re-exports the new model_test macro.
concurrency-macros/src/lib.rs	Implements #[model_test] and clarifies backend leaf naming for nextest filters.
acl-filter/src/tests.rs	Updates end-to-end tests for the port-forwarder allocator handle.
.gitignore	Ignores libfuzzer worker logs (fuzz-*.log).

Suppressed comments (1)

nat/src/portfw/flow_state.rs:84

confidence: 8
tags: [other]

Same concern as `new_snat`: `PortFwState` is publicly re-exported, so making `new_dnat` `pub(crate)` breaks external callers. Keep it `pub` (or, if construction must be internal, consider also making the type non-public/re-exported to keep the API consistent).

pub(crate) fn new_dnat(

</details>



---

💡 <a href="/githedgehog/dataplane/new/main?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a `code-review` agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (12)

nat/src/masquerade/test.rs (1)

332-471: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a parameterized overlay builder.

The four builders repeat the same VPC table, peering names, and VPC-3 expose. Only the VPC-1 and VPC-2 exposes differ. A single helper that takes the exposes per peering would remove about 100 lines and keep the scenarios readable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/test.rs` around lines 332 - 471, Replace the duplicated
setup in build_overlay_shared_private_prefix,
build_overlay_shared_private_prefix_extended,
build_overlay_shared_private_prefix_narrowed, and
build_overlay_without_masquerade with a parameterized helper accepting the VPC-1
and VPC-2 exposes (including optional VPC-2/peering configuration as needed).
Keep the shared VPC table, peering names, VPC-3 expose, and Overlay construction
in that helper, then have each scenario builder provide only its differing
exposes.

nat/src/masquerade/apalloc/concurrent_fuzz.rs (1)

303-303: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Guard the modulo against an empty pool list.

step % published.pools.len() panics if pools is empty. Line 340 already guards the same pattern with survivors.len().max(1). pool_sets_for_specs currently returns one pool set per spec, and specs is never empty, so this is not reachable today. Applying the same guard keeps the two sites consistent and protects against a future change in pool_sets_for_specs.

♻️ Proposed change

             PacketOp::Allocate => {
-                let owner = step % published.pools.len();
+                let Some(owner) = published
+                    .pools
+                    .len()
+                    .checked_rem(published.pools.len())
+                    .map(|_| step % published.pools.len())
+                else {
+                    continue;
+                };
                 match published.pools[owner].allocate(false) {

A simpler form is also acceptable:

if published.pools.is_empty() {
    continue;
}
let owner = step % published.pools.len();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/apalloc/concurrent_fuzz.rs` at line 303, Guard the owner
calculation around published.pools in the relevant loop so an empty pool list
skips the iteration before modulo, or otherwise use a nonzero length consistent
with the existing survivors.len().max(1) guard. Preserve the current owner
selection for non-empty pools.

nat/src/masquerade/allocator_writer.rs (1)

128-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Update the doc comment: the disabled-masquerade path no longer cancels every flow.

The doc comment on update_nat_allocator states that flows using the allocator "will be cancelled". remove_allocator_from_flows now invalidates only flows carrying NAT state and clears the port-forward lease on the rest, so port-forward flows survive. The debug! message on line 130 says "will invalidate flows" and has the same problem.

Align both texts with the new behaviour.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/allocator_writer.rs` around lines 128 - 135, Update the
doc comment for update_nat_allocator and the debug! message in its
disabled-masquerading branch to describe the actual remove_allocator_from_flows
behavior: NAT-state flows are invalidated, while other flows have their
port-forward lease cleared and survive.

nat/src/masquerade/apalloc/setup.rs (1)

131-140: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Build specs once per destination VPC, not once per protocol.

specs does not depend on protocol. The current code clones every expose's public_ranges three times per destination VPC. Move the construction above the protocol loop.

♻️ Proposed refactor

     for (dst_vpc_id, exposes) in groups {
+        let specs: Vec<PoolSpec> = exposes
+            .iter()
+            .map(|expose| PoolSpec {
+                public_ranges: expose.public_ranges.clone(),
+                idle_timeout: expose.idle_timeout,
+            })
+            .collect();
         // Allocations for TCP, for example, do not affect allocations for UDP or for ICMP: the
         // space made of addresses and L4 ports or identifiers is distinct for each protocol. So
         // each region backs one allocator per protocol, over the same addresses.
         for protocol in [NextHeader::TCP, NextHeader::UDP, icmp_proto] {
-            let specs: Vec<PoolSpec> = exposes
-                .iter()
-                .map(|expose| PoolSpec {
-                    public_ranges: expose.public_ranges.clone(),
-                    idle_timeout: expose.idle_timeout,
-                })
-                .collect();
-
             let pool_sets = pool_sets_for_specs::<J>(&specs, protocol, randomize);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/apalloc/setup.rs` around lines 131 - 140, Move the `specs`
construction from inside the protocol loop to immediately before `for protocol
in [NextHeader::TCP, NextHeader::UDP, icmp_proto]`, then reuse that single
collection for each call to `pool_sets_for_specs`. Keep the existing `PoolSpec`
mapping and protocol-specific processing unchanged.

nat/src/masquerade/nf.rs (1)

86-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

discriminants duplicates the checks in process_packet, and the two paths report different outcomes.

process_packet (lines 492-505) already rejects a packet without either discriminant and sets DoneReason::Unroutable. discriminants therefore never returns MissingDiscriminant on that path, and MissingDiscriminant maps to DoneReason::NatFailure. The same malformed input would be reported two ways depending on which check runs.

Remove the two pre-checks in process_packet and let masquerade_packet surface MissingDiscriminant, or keep the pre-checks and align the DoneReason.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/nf.rs` around lines 86 - 95, Remove the duplicate source
and destination VPC discriminant checks in process_packet and rely on
masquerade_packet’s discriminants validation to return MissingDiscriminant
consistently. Preserve the existing error propagation so malformed packets
receive the corresponding single DoneReason rather than diverging based on which
check runs.

nat/src/masquerade/apalloc/port_alloc.rs (1)

316-330: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

The retry loop spins without yielding.

find_block_for_port repeats immediately when the free flag and the map entry disagree. The window it waits for is closed by another thread that runs between the CAS in try_to_reserve_block and the insert in allocate_block_for_reservation. Four tight iterations can complete before that thread is rescheduled, which turns a recoverable race into PortReservationFailed.

Add std::hint::spin_loop() (or a thread::yield_now() on the last attempts) between iterations to widen the window.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/apalloc/port_alloc.rs` around lines 316 - 330, Update the
retry loop in find_block_for_port to yield between failed block lookups, after
logging the detected free-flag/map-entry race and before the next iteration. Use
std::hint::spin_loop() or thread::yield_now(), while preserving the existing
retry limit and PortReservationFailed outcome.

nat/src/masquerade/flows.rs (1)

176-184: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Skip the lease update when check_masquerading_flow has just invalidated the flow.

for_each_flow_filtered tests is_active before the closure runs, but check_masquerading_flow can call invalidate_pair inside it. update_port_forward_lease then reserves a public tuple for a flow that is already dead. The lease is released when the flow state is dropped, so this is not a permanent reservation, but during reconciliation the tuple is unavailable to other flows.

♻️ Proposed change

         |flow_key, flow_info| {
             check_masquerading_flow(flow_key, flow_info, new_allocator);
+            if !flow_info.is_active() {
+                return;
+            }
             if let Err(error) = update_port_forward_lease(flow_info, Some(new_allocator)) {
                 error!("Failed to reserve a live port-forward tuple: {error}");
                 flow_info.invalidate_pair();
             }
         },

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/flows.rs` around lines 176 - 184, In the closure passed to
for_each_flow_filtered, check whether flow_info remains active after
check_masquerading_flow and before calling update_port_forward_lease. Skip the
lease update for flows invalidated by check_masquerading_flow, while preserving
the existing error handling for still-active flows.

nat/src/masquerade/apalloc/mod.rs (2)

337-347: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Stale lease entries are only pruned when a reservation happens.

leases.retain runs once per 256 calls to reserve_port_forward. If port-forward traffic stops, the expired Weak<Allocation> entries stay in the map until the next reservation. The map is bounded by the number of distinct forwarded tuples, so this is not a leak of unbounded size, but the memory is held for an unbounded time.

Consider also removing the entry when Weak::upgrade fails on the lookup path just below.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/apalloc/mod.rs` around lines 337 - 347, Update the lease
lookup path around the existing leases.get(...).and_then(Weak::upgrade) check to
remove the corresponding key from leases when Weak::upgrade fails. Preserve
returning the upgraded lease when it exists, and ensure stale entries are
deleted during lookup rather than relying solely on the periodic retain cleanup.

---

203-215: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider indexing public regions instead of scanning the whole table.

public_allocator iterates every entry of the pool table and every region of every matching pool set. The same region is visited once per private prefix that shares the pool set. This runs on the first packet of each port-forward flow and once per live port-forward flow during allocator replacement, so the cost grows with the number of exposes multiplied by the number of live port-forward flows.

A per-destination-VPC map from public interval to allocator, built once in build_pools, would make the lookup logarithmic and remove the duplicate visits.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/apalloc/mod.rs` around lines 203 - 215, Replace the
full-table scan in public_allocator with a per-destination-VPC public-region
index built once by build_pools. Index each public interval to its allocator,
then perform a logarithmic interval lookup keyed by protocol and dst_vpcd while
preserving the existing address-containment behavior.

nat/src/masquerade/apalloc/test_alloc.rs (1)

469-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Negative reserve_port assertions accept the wrong error. Both tests prove that a port-forward lease blocks a masquerade reservation, but they only check is_err(). reserve_port also returns NoPoolFound when the pool lookup misses and Denied for a well-known port. A regression that breaks the pool lookup, or that stops carrying the lease into the replacement allocator, would leave both tests green.

• nat/src/masquerade/apalloc/test_alloc.rs#L469-L495: replace both is_err() assertions with a match on the AllocatorError variant that a live lease produces.
• nat/src/test.rs#L221-L243: make tuple_is_reserved return the error variant comparison instead of is_err(), so the post-replacement assertion proves lease carry-over rather than any failure.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/masquerade/apalloc/test_alloc.rs` around lines 469 - 495, The
negative reserve_port assertions in nat/src/masquerade/apalloc/test_alloc.rs
lines 469-495 must match the AllocatorError variant produced when a live lease
blocks reservation, rather than accepting any error; update both assertions
while preserving the successful post-drop reservation. In nat/src/test.rs lines
221-243, update tuple_is_reserved to compare the returned error variant so the
replacement-allocator check specifically verifies lease carry-over.

nat/src/portfw/test.rs (1)

207-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The allocator slot stays empty, so the lease path is never exercised.

NatAllocatorWriter::new() creates an empty slot, and setup_pipeline never calls update_nat_allocator. Therefore self.allocator.get() returns None in do_port_forwarding, and both the reservation at nat/src/portfw/nf.rs Lines 128-143 and update_port_forward_lease take the no-allocator branch in every test in this module.

Add one test that installs a masquerade config whose public range covers 70.71.72.73, then assert that the forward and reverse flows hold the same lease and that masquerade cannot reserve the same tuple while the flows live. nat/src/masquerade/apalloc/test_alloc.rs Lines 454-496 shows the expected sharing and release behaviour.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/portfw/test.rs` around lines 207 - 213, Add a port-forwarding test
that updates the NatAllocatorWriter with a masquerade configuration whose public
range includes 70.71.72.73 before running setup_pipeline. Assert the forward and
reverse flows share the same lease, and verify masquerade cannot reserve that
tuple while both flows remain active, following the sharing and release pattern
in the allocator tests.

nat/src/portfw/flow_state.rs (1)

145-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The bool result is not used by the only caller.

nat/src/portfw/nf.rs Lines 205-211 match only on Err. The Ok(false) case, which means the flow carries no port-forwarding state, is silently ignored. Consider returning Result<(), AllocatorError> and logging the missing-state case here, or handling Ok(false) in the caller.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nat/src/portfw/flow_state.rs` around lines 145 - 164, Update
update_port_forward_lease and its sole caller to remove the unused boolean
result, either by returning Result<(), AllocatorError> and logging the missing
port-forwarding state within the function, or by explicitly handling the
no-state outcome in the caller’s match. Ensure allocator errors remain
propagated and missing state is no longer silently ignored.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@acl-filter/src/tests.rs`:
- Around line 938-939: Use a single NatAllocatorWriter in the test setup,
configure it with update_nat_allocator, and pass readers from that same writer
to both PortForwarder and Masquerade. Remove the second allocator and ensure the
shared writer is retained in PipelineHandles so both stages observe and reserve
the same allocation state.

---

Nitpick comments:
In `@nat/src/masquerade/allocator_writer.rs`:
- Around line 128-135: Update the doc comment for update_nat_allocator and the
debug! message in its disabled-masquerading branch to describe the actual
remove_allocator_from_flows behavior: NAT-state flows are invalidated, while
other flows have their port-forward lease cleared and survive.

In `@nat/src/masquerade/apalloc/concurrent_fuzz.rs`:
- Line 303: Guard the owner calculation around published.pools in the relevant
loop so an empty pool list skips the iteration before modulo, or otherwise use a
nonzero length consistent with the existing survivors.len().max(1) guard.
Preserve the current owner selection for non-empty pools.

In `@nat/src/masquerade/apalloc/mod.rs`:
- Around line 337-347: Update the lease lookup path around the existing
leases.get(...).and_then(Weak::upgrade) check to remove the corresponding key
from leases when Weak::upgrade fails. Preserve returning the upgraded lease when
it exists, and ensure stale entries are deleted during lookup rather than
relying solely on the periodic retain cleanup.
- Around line 203-215: Replace the full-table scan in public_allocator with a
per-destination-VPC public-region index built once by build_pools. Index each
public interval to its allocator, then perform a logarithmic interval lookup
keyed by protocol and dst_vpcd while preserving the existing address-containment
behavior.

In `@nat/src/masquerade/apalloc/port_alloc.rs`:
- Around line 316-330: Update the retry loop in find_block_for_port to yield
between failed block lookups, after logging the detected free-flag/map-entry
race and before the next iteration. Use std::hint::spin_loop() or
thread::yield_now(), while preserving the existing retry limit and
PortReservationFailed outcome.

In `@nat/src/masquerade/apalloc/setup.rs`:
- Around line 131-140: Move the `specs` construction from inside the protocol
loop to immediately before `for protocol in [NextHeader::TCP, NextHeader::UDP,
icmp_proto]`, then reuse that single collection for each call to
`pool_sets_for_specs`. Keep the existing `PoolSpec` mapping and
protocol-specific processing unchanged.

In `@nat/src/masquerade/apalloc/test_alloc.rs`:
- Around line 469-495: The negative reserve_port assertions in
nat/src/masquerade/apalloc/test_alloc.rs lines 469-495 must match the
AllocatorError variant produced when a live lease blocks reservation, rather
than accepting any error; update both assertions while preserving the successful
post-drop reservation. In nat/src/test.rs lines 221-243, update
tuple_is_reserved to compare the returned error variant so the
replacement-allocator check specifically verifies lease carry-over.

In `@nat/src/masquerade/flows.rs`:
- Around line 176-184: In the closure passed to for_each_flow_filtered, check
whether flow_info remains active after check_masquerading_flow and before
calling update_port_forward_lease. Skip the lease update for flows invalidated
by check_masquerading_flow, while preserving the existing error handling for
still-active flows.

In `@nat/src/masquerade/nf.rs`:
- Around line 86-95: Remove the duplicate source and destination VPC
discriminant checks in process_packet and rely on masquerade_packet’s
discriminants validation to return MissingDiscriminant consistently. Preserve
the existing error propagation so malformed packets receive the corresponding
single DoneReason rather than diverging based on which check runs.

In `@nat/src/masquerade/test.rs`:
- Around line 332-471: Replace the duplicated setup in
build_overlay_shared_private_prefix,
build_overlay_shared_private_prefix_extended,
build_overlay_shared_private_prefix_narrowed, and
build_overlay_without_masquerade with a parameterized helper accepting the VPC-1
and VPC-2 exposes (including optional VPC-2/peering configuration as needed).
Keep the shared VPC table, peering names, VPC-3 expose, and Overlay construction
in that helper, then have each scenario builder provide only its differing
exposes.

In `@nat/src/portfw/flow_state.rs`:
- Around line 145-164: Update update_port_forward_lease and its sole caller to
remove the unused boolean result, either by returning Result<(), AllocatorError>
and logging the missing port-forwarding state within the function, or by
explicitly handling the no-state outcome in the caller’s match. Ensure allocator
errors remain propagated and missing state is no longer silently ignored.

In `@nat/src/portfw/test.rs`:
- Around line 207-213: Add a port-forwarding test that updates the
NatAllocatorWriter with a masquerade configuration whose public range includes
70.71.72.73 before running setup_pipeline. Assert the forward and reverse flows
share the same lease, and verify masquerade cannot reserve that tuple while both
flows remain active, following the sharing and release pattern in the allocator
tests.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ac5b349e-0f9e-420b-b042-3a5d4c91c7ac

📥 Commits

Reviewing files that changed from the base of the PR and between 4f4dd98 and 8fae1a9.

📒 Files selected for processing (33)

• .gitignore
• acl-filter/src/tests.rs
• concurrency-macros/src/lib.rs
• concurrency/src/macros.rs
• dataplane/src/packet_processor/mod.rs
• development/code/running-tests.md
• flow-entry/src/flow_table/concurrent_fuzz.rs
• justfile
• miri.just
• nat/src/masquerade/allocation.rs
• nat/src/masquerade/allocator_writer.rs
• nat/src/masquerade/apalloc/alloc.rs
• nat/src/masquerade/apalloc/concurrent_fuzz.rs
• nat/src/masquerade/apalloc/display.rs
• nat/src/masquerade/apalloc/mod.rs
• nat/src/masquerade/apalloc/natip_with_bitmap.rs
• nat/src/masquerade/apalloc/pool_fuzz.rs
• nat/src/masquerade/apalloc/port_alloc.rs
• nat/src/masquerade/apalloc/region.rs
• nat/src/masquerade/apalloc/setup.rs
• nat/src/masquerade/apalloc/test_alloc.rs
• nat/src/masquerade/flows.rs
• nat/src/masquerade/icmp_handling.rs
• nat/src/masquerade/mod.rs
• nat/src/masquerade/natip.rs
• nat/src/masquerade/nf.rs
• nat/src/masquerade/state.rs
• nat/src/masquerade/test.rs
• nat/src/portfw/flow_state.rs
• nat/src/portfw/mod.rs
• nat/src/portfw/nf.rs
• nat/src/portfw/test.rs
• nat/src/test.rs

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (2)

nat/src/portfw/nf.rs:210

confidence: 7
tags: [logic]

If `update_port_forward_lease` fails after both flows have been inserted, the code invalidates the pair but leaves the entries in the flow table. Because the port-forward state can still hold the (old) lease until the table reaps the flow, this can unnecessarily pin the reserved public tuple.

Consider removing both flow entries from the flow table on this error path so the lease is released immediately.
**nat/src/portfw/nf.rs:200**
* ```yaml
confidence: 8
tags: [logic]

If inserting the reverse port-forwarding flow fails, the forward flow is only invalidated but not removed from the flow table. Since port-forward flows now carry a public-tuple lease, leaving the forward flow resident can keep the lease (and therefore masquerade capacity) held until the flow table later reaps it.

Prefer removing the forward entry from the flow table on this error path to ensure any attached lease is released promptly and to avoid leaving a one-sided, invalidated flow in the table.

This issue also appears on line 206 of the same file.