(4)-masquerade: coverage, and the bugs it found by daniel-noland · Pull Request #1700 · githedgehog/dataplane

daniel-noland · 2026-08-06T02:28:26Z

Warning

AI assisted, not yet ready for external review by other humans.
Please do not spend review time on this yet. It is pushed to run CI and to keep
the stack visible, not to attract review. The dont-merge label stays on until
that changes.

A coverage pass over the masquerade allocator, and what it turned up: three live bugs, and a
path no test had ever entered. Coverage
was measured with just coverage-style cargo llvm-cov runs with the bolero properties given
10s each, then read for cold paths.

What it turned up

Three defects in how claimed blocks and addresses are handled — a fully claimed address
taking a whole region out of service, a drifting count of usable port blocks, and an ordinary
policy conflict reported as allocator corruption. All three are caused by the claims (2)-masquerade: port forwarding claims #1698
introduces, so the fixes now live in (2)-masquerade: port forwarding claims #1698 rather than here; review found they were merely
sequenced late, not dependency-forced. The tests that pin them stay here, along with the
coverage work that found them.
The flow carry-over path had never run. A hole in the suite rather than a bug in the
allocator, and the one that let the other two hide. Every reconfiguration test applied an
identical configuration, which the writer answers by keeping the allocator it has. So
re_reserve_ip_and_port was entered by no test at all, including for the source-VPC change
earlier in this stack. Instrumenting it counted zero entries before, two after.

Coverage added

IPv6 end to end (the offset mapping IPv4 skips entirely), a region allocated dry for real
(~129k allocations, about a second, which pins the well-known exclusion and the ordering
between addresses), and the paths that drop a flow when its configuration goes away. Dead
conversions that nothing called were deleted rather than tested.

Mutation testing earned its keep twice here: two tests passed on first write and were still
wrong, and the notes on them say so.

nat crate coverage went from 92.0% to 92.4% region measured with a clean profile (the working
figure during development read 93.3%, inflated by stale profile data from earlier runs -- see the
just coverage fix below), with flows.rs 48% to 92% and natip.rs 49% to 95%. The later test
commits on this branch take masquerade production code to 88.6% region and 66.5% branch.

Added while reviewing the stack

Fixes

The three claimed-block fixes moved to (2)-masquerade: port forwarding claims #1698 — telling a block claimed in full apart from
broken bookkeeping, keeping used-up addresses out of the pool (sixteen dropped packets for a
claimed /25, measured; now zero), and keeping the usable-block count honest. They sit with the
change that creates those states so no PR in the stack ships them live; (2)-masquerade: port forwarding claims #1698 describes them.
Flow timeouts stretch under an emulator (moved to (2)-masquerade: port forwarding claims #1698, where the test that trips it lives)
and the exhaustion tests narrow their port space there too. Between them these take
check/miri/powerpc64 from hanging past twenty-five minutes to green.
just coverage produces a report again — it was broken in three places and no workflow runs
it, which is how it stayed broken.
just miri::test keeps the rustflags .cargo/config.toml sets. Setting RUSTFLAGS in the
environment replaces build.rustflags rather than adding to it, so miri was checking a different
build of tokio from every other job, and every cfg(emulated) site warned.

Tests

Claims a carried-over flow meets; an address brought in by a reservation keeping them; a port block
coming back while its address stays; an address retired for good; the two config changes that end a
flow (its port claimed, its peering gone); the error classifications pinned against each other; an
IPv6 question for the claims; and a connection walked through an ordinary close in both directions.

Three existing tests could not fail and were mended: one property compared 24 with 24 on every
input, another discarded every error but the one it named, and the concurrent suite dropped a
successful reservation as a temporary, releasing the port it was meant to hold.

The concurrent suite's two oracle fixes have moved down to #1699, where the runtime fix they
assert against lives: they cannot hold before it, and there they double as a regression test for
that PR's own change.

The allocator self-deadlock has moved down to #1696, along with a fourth site review found
after it: IpAllocator::fmt upgrades the same weak address references while holding the pool read
guard, so printing the table could wedge it against a flow ending. That is a pre-existing defect
and dependency-free, and leaving it at the top left every lower PR's threaded CI exposed to the
hang it causes -- a six-hour one, in the run that first surfaced it. Both sites and their two
Shuttle regressions now sit at the bottom of the stack. #1696 describes them.

Every fix here is mutation-verified. Mutation testing also caught three claims of mine that were
wrong before they landed, including one in a commit message.

Coverage for masquerade production code went from 87.4% to 88.6% region and 63.3% to 66.5% branch,
measured with the repaired recipe. Printers are deliberately left uncovered.

Stack

Merge bottom to top. Each PR is based on the one above it in this list.

(1)-masquerade: allocate each public pair once #1696 — (1)-masquerade: allocate each public pair once
(2)-masquerade: port forwarding claims #1698 — (2)-masquerade: port forwarding claims
(3)-masquerade: the block-handover races, and honest concurrent oracles #1699 — (3)-masquerade: the block-handover races, and honest concurrent oracles
(4)-masquerade: coverage, and the bugs it found #1700 — (4)-masquerade: coverage, and the bugs it found ← you are here

#1697 was folded into #1696 and closed; its one commit belonged next to the other pool-table work.

Every commit in the stack builds and passes cargo nextest run and cargo clippy --all-targets
on its own.

coderabbitai · 2026-08-06T02:28:34Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b956875f-1e37-4510-8796-3ce2ee3595c6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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

Copilot

Pull request overview

Improves correctness and test coverage of the masquerade NAT allocator (IPv4/IPv6, config re-apply flow carry-over, exhaustion/claims edge cases), incorporating bugs uncovered by coverage + mutation/property testing and making emulator (miri/qemu) runs more reliable.

Changes:

Add targeted reconfiguration tests to exercise allocator rebuild + flow re-reservation and to validate flow drop behavior when config removes/changes masquerade coverage.
Fix allocator behavior around fully-claimed addresses/blocks (port-forwarding claims + well-known exclusions) by pre-excluding unusable addresses/blocks and keeping bookkeeping consistent.
Extend allocator test suite to cover IPv6 end-to-end and multiple exhaustion/reuse scenarios; adjust miri runner flags to match global build cfgs.

Reviewed changes

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

Show a summary per file

File	Description
nat/src/masquerade/test.rs	Adds reconfig tests and overlay builders; updates packet helper to vary source ports.
nat/src/masquerade/natip.rs	Removes unused NatIp extraction helpers from `Net`.
nat/src/masquerade/apalloc/test_alloc.rs	Adds IPv6 allocator context and end-to-end IPv6 allocation + reservation tests.
nat/src/masquerade/apalloc/reserved.rs	Adds address-level “unusable spans” computation and shared block-usability predicates; bolero oracle tests.
nat/src/masquerade/apalloc/port_alloc.rs	Pre-excludes unusable port blocks and derives usable-block count from block state; improves reservation error classification.
nat/src/masquerade/apalloc/pool_fuzz.rs	Strengthens properties for freeing/reuse, re-reservation correctness, and multiple exhaustion paths; adds deterministic regression tests.
nat/src/masquerade/apalloc/concurrent_fuzz.rs	Ensures successful reservations are held for run duration; tightens expectations around carried pairs.
nat/src/masquerade/apalloc/alloc.rs	Adds bounded multi-address allocation with retirement of unusable IPs; pre-excludes fully-claimed addresses from pool bitmap.
miri.just	Sets explicit `RUSTFLAGS` to preserve cfg/check-cfg/tokio_unstable expectations for miri runs.

+fn build_overlay_without_masquerade() -> Overlay {
+    let mut vpc_table = VpcTable::new();
+    let _ = vpc_table.add(Vpc::new("VPC-1", "AAAAA", 100).expect("Failed to add VPC"));
+    let _ = vpc_table.add(Vpc::new("VPC-3", "CCCCC", 300).expect("Failed to add VPC"));
+
+    let peering13 = VpcPeering::with_default_group(
+        "VPC-1--VPC-3",
+        VpcManifest::new("VPC-1").exposing(VpcExpose::empty().ip("1.1.0.0/16".into())),
+        VpcManifest::new("VPC-3").exposing(VpcExpose::empty().ip("3.3.3.0/24".into())),
+    );
+
+    let mut peering_table = VpcPeeringTable::new();
+    peering_table.add(peering13).expect("Failed to add peering");
+
+    Overlay::new(vpc_table, peering_table)
+}


Nothing exercised re-reservation. The writer keeps the allocator it has when handed a configuration equal to the current one, and only advances the flows' generation, so every reconfiguration test took that shortcut: one applies an identical configuration, and the other changes the public range, which invalidates the flow before re-reservation is reached. That left the whole carry-over path untested end to end, including the source discriminant added to the re-reservation lookup. An earlier commit on this stack claimed to cover it and did not; measuring coverage is what showed the file at under half, and instrumenting the function confirmed no test entered it at all. Change the configuration in a way that leaves the flows valid: one more masquerade expose, disjoint from everything already there. The writer then has to build a new allocator and carry the flows into it, and the two VPCs sharing a private address make a lookup that ignores which VPC a flow came from land in the wrong pool. The assertions are on behaviour rather than on a log line: both flows are translated exactly as before, and a flow arriving after the change is not handed a pair a carried-over flow still holds, which is what the re-reservation exists to guarantee. Confirmed to bite. The function is entered twice, once per carried flow, where it was entered not at all before; and passing the destination discriminant in place of the source fails the test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

NatIp declared from_src_addr and from_dst_addr, and both were implemented for each address family, and nothing has ever called any of them. A trait method that is implemented counts as used, so the compiler had nothing to say about it; measuring coverage is what showed half the file cold. Delete them rather than write tests for code with no callers. The trait is private to the crate, so nothing outside can miss them. from_src_addr and from_dst_addr went earlier in this stack, having no callers. Their doc comments stayed, describing methods that are no longer there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Nothing exercised the IPv6 path. It does not merely substitute for IPv4 here: its pool bitmap is indexed from the start of a region through an offset mapping, where IPv4 uses the address bits directly, so the arithmetic that turns an offset into an address and an address back into an offset had no coverage at all. Add a masquerade expose over IPv6 and drive it from configuration through allocation, and through the carry-over that turns a held address back into an offset. Both tests passed the moment they were written, which is not evidence of much, so I shifted the offset mapping by one to see whether they would notice. They did not: every address moves together under a constant shift, and asserting only that an address lies inside the declared range cannot see that. They now pin the first allocation to the first address of the range, which does see it, and fails as it should. The claim sweep is generic over the address family too, and had only ever been asked an IPv4 question. Every other test of port-forwarding claims is IPv4, and the two are not the same code. An IPv6 pool indexes its bitmap by an offset from the start of its region rather than by the address itself, and working out which addresses a claim covers means comparing v6 addresses through a conversion IPv4 skips. Coverage bore that out: claim_bounds had never been instantiated for Ipv6Addr at all. One address claimed end to end, which must be kept out of the pool entirely, and one claimed above its first usable block, whose remaining ports must still be handed out and whose claimed ones must not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The property tests reach exhaustion by claiming ports away, which is a model of a full pool rather than a full pool. Doing it the long way takes about 64k allocations to move off a single address, which is exactly why a handful of allocations never leaves the first one, and why the paths that move between addresses and finally give up had no coverage. It costs about a second, which buys the whole ladder: every port of an address, the move to the next when it runs out, the last port of the last address, the refusal after that, and the region becoming servable again once everything is given back. The counts are asserted rather than the shape alone. An address yields 65536 ports less the well-known range masquerade keeps off, the move happens exactly when the first address runs out and not before, and the region serves precisely the addresses and ports it holds. That pins the well-known exclusion and the ordering between addresses at the same time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The invalidation side of a configuration change was the last of the carry-over logic left cold. A flow that outlives the configuration that authorised it goes on translating traffic, so these are the paths where being wrong is worse than dropping a packet. Two are covered here. A flow whose own source address is no longer masqueraded is dropped even though the public range it holds is still exposed, so checking only the address it was given is not enough. And taking masquerade out of the configuration entirely drops every masqueraded flow, which is a different route through the writer: it throws the allocator away rather than building a new one. Neither test isolates a single guard, and the first cannot. The source is checked against the exposes, and re-reservation independently looks the pool up by source address and finds none; the pools are built from those same exposes, so the two conditions coincide and disabling either leaves the flow dropped by the other. What the test pins is the outcome. Traffic after masquerade is removed is refused for want of an allocator rather than by the filter, which is what the assertion says, since the allocator is gone rather than the flow being ruled out. Two more ways a change ends a flow, on the same fixture. Coverage showed the branches that decide whether a live flow survives a config change to be the least exercised part of the masquerade code, and they are among the most consequential: getting one wrong either drops traffic that should have continued or keeps a flow on a pair the configuration has given to something else. A flow whose port has been claimed. Port forwarding maps a public address and port statically, so once it has claimed one that pair belongs to it, and a flow left holding it would collide with the forwarding on the reverse key. Re-reservation has to fail and the flow has to go, which is the failing branch of re_reserve_ip_and_port and the else that follows. Masquerade and port forwarding overlapping on one public range is an allowed combination -- validate_expose_collisions says so explicitly, on the grounds that each implies a direction -- so this is a configuration an operator can write, not a contrivance. A flow whose peering has gone. The peering lookup fails before any expose is examined, so it is a different branch from the flow whose expose or whose source has gone, which were already covered. Both carry a control: another config change that does not affect the flow, or another flow the change does not touch, so neither can pass on a change that simply drops everything. The first cannot see whether the refusal was reported as a port reservation failure or as broken bookkeeping, since the flow is dropped either way. It says so, and points at the pool-level test that can. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…d weak tests Four changes to the pool suite, all about what it could not see. A flow carried across a config change is refused the same way whether the claim around its port covers part of the block or the whole of it. The partial case also pins the claims being clipped into a block a reservation brings in: without that, the port would be handed straight back to the flow that must not have it. An address a reservation brings into a pool keeps the claims on it. That address goes on to serve later allocations exactly as a drawn one does, so arriving unencumbered let masquerade hand out the very ports port forwarding is statically mapping. Both alloc.rs and port_alloc.rs carry comments warning of this; neither had a test, and reintroducing it failed nothing. The cost of a run of claimed addresses is now stated as a test rather than left as a remark in a commit message. An allocation gives up after MAX_ADDRESSES_PER_ALLOCATION addresses that cannot serve, so a long run sheds one allocation per bound's worth of them. The test pins that it is paid once rather than once per packet, which is the part that matters: were the addresses tried to stop being taken out of the pool, a region behind a claimed run would drop every packet forever, and the counts would not converge. freed_allocations_become_available_again could not fail. An address holds tens of thousands of ports, so two dozen allocations succeed whether or not anything was ever released, and comparing the counts compared 24 with 24 on every input. It now asks for the same answers, which pins the address bitmap and the count of usable blocks. It still does not pin the freeing of an individual port, and now says so. The re-reservation property discarded every error but one, so it would have passed on pools that refused every flow, and on an allocator reporting its own bookkeeping broken. Where the new configuration still declares the address there is nothing that may refuse it, so anything but NoPoolFound is now a failure. Also drops a reference to find_masquerade_portfw_overlap, which this stack deleted, along with the defect it described. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Mutation testing caught the previous commit overclaiming. Comparing what the pools serve before and after everything is freed was said to pin the count of usable blocks; it does not. Releasing the last port of an address releases the address, and the port allocator belongs to the address, so the blocks and their bookkeeping are built afresh next time whatever the last round did to them. Neither leaving a freed block marked non-free nor dropping the increment to the count of usable blocks failed any test in the suite. Holding the address is what makes that level observable. Take every port one address has, give back exactly one block's worth while keeping the rest, and the block that came back has to be the one that serves next, for the whole of itself and no further. Both mutations above fail this. The count is the one that drifted in the first place, where a block ruled out was marked unusable without being taken off it; this pins the other direction, that a block coming back is counted back in. The comment on the older property now says what it does reach, which is the pool's bitmap of addresses and nothing below it. The other half of the same lifecycle: an address that never comes back. Retiring an address and deallocating one pull in opposite directions on the same bitmap, and deallocation runs on the drop path where it cannot know why the address left. The set of unusable offsets is what keeps the first from being undone by the second. Keeping fully claimed addresses out of the pool when it is built removed the only way a configuration could reach retire_from_pool, so coverage now shows it never running: what is left is an address emptied by another thread between being drawn and being drawn upon, which no test can arrange. Ask it directly instead. The ordinary deallocation is checked alongside it, so the test says the address stays out because it was retired rather than because giving addresses back stopped working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Taking an address dry means about 64k allocations, a second or so natively. Under miri the same walk runs for tens of minutes: a run was killed after twenty-five, still inside a_region_can_be_allocated_dry, which is why check/miri has been hanging rather than merely failing. Cutting the walk short would take the point out of the tests, which exist because a handful of allocations never leaves the first address and the paths between addresses went untested for exactly that reason. Narrow the port space instead: a claim leaves two blocks of each address allocatable, and the same ladder is walked over it -- every port of an address, the move to the next, the last port of the last, and the refusal after that. About a thousand allocations rather than a hundred and thirty thousand. Natively nothing changes: with all 252 blocks left allocatable the claim is empty and the tests do what they did before. The narrowing is not enough on its own; two more things were in the way. Setting RUSTFLAGS in the environment replaces `build.rustflags` from .cargo/config.toml rather than adding to it, and the miri recipe builds its own value from an empty string. Both flags that live there were therefore silently dropped for miri runs alone. Losing --check-cfg=cfg(emulated) is why every site using that cfg warns under miri and nowhere else. Losing --cfg=tokio_unstable is the quieter half: miri was checking a different build of tokio from the one every other job builds, which is not what a job that exists to find undefined behaviour should be doing. nix/profiles.nix carries the same two in common.RUSTFLAGS for the qemu path and gets this right already, so the two now say the same thing. Verified by removing the flags again: one unexpected_cfgs warning with them gone, none with them present, and the whole workspace passes under miri either way. Presenting an address past what a pool can index means a region wider than a u32, and so a bitmap of 2^32 entries. Natively that is quick. Under miri it took 382 seconds, which was 382 of the 405 the whole workspace job spent: one test was very nearly the entire cost of running miri at all. Nothing about it wants a model checker. It is address arithmetic and an error path, and the refusal it checks comes from the offset mapping, which can be asked directly for nothing. So miri skips it and gets a unit test of the mapping instead, covering the same refusal either side of the boundary. Skipped under `miri` rather than `emulated`, so qemu-user still runs the whole thing -- big-endian is exactly where address arithmetic wants checking, and a few seconds there is affordable. `just miri::test -p dataplane-nat` goes from 297 seconds to 45. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Three things stood between the recipe and a number. `cargo llvm-cov clean` removes CARGO_LLVM_COV_TARGET_DIR outright, and the nextest run was then handed that same path as --extract-to, which does not create it. The recipe died on "error canonicalizing destination directory" before a single test ran. Past that, it replayed the nextest archive that `just test` uses. Coverage needs -Cinstrument-coverage at compile time and the archive is built without it, so every test ran, no profile data was written, and the report failed with "not found *.profraw files" -- looking like it had worked right up until the end. It builds its own instrumented binaries now. Teaching the nix build to produce an instrumented archive would be the faster answer, and is worth doing if this ever moves into CI, which today it does not: no workflow runs this recipe, which is how it came to be broken in three places at once without anyone noticing. The custom target and build directories went with it. cargo-llvm-cov did not honour them -- `show-env` reported neither what the recipe exported nor what .cargo/config.toml sets -- and the --profile="" workaround existed only to paper over the layout they were supposed to produce. Arguments now go straight to nextest, so `just coverage -p dataplane-nat` scopes it to one package and a bare `just coverage` covers the workspace. A summary is printed at the end rather than only written to disk. Keeping `clean` matters for more than tidiness: stale profraw from an earlier run merges into the report and quietly inflates it. Measuring the nat crate before and after this fix gave 93.3% and 92.4% for the same tree, the difference being data left over from previous attempts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

AllocatorError is classified twice over, once by is_exhaustion and once by the conversion into DoneReason, and both matches are exhaustive on purpose so that a new error has to be given an answer rather than inheriting one. Coverage found one of the ten variants exercised through either: NatUnsupportedProto, NatOutOfResources and NatFailure were never produced by any test at all. A table for each, plus the property that matters more than either: the two agree. is_exhaustion says as much in its own comment -- that it draws the line DoneReason already draws, where exactly these become NatOutOfResources -- and nothing enforced it. They are separate matches, so an error added to one and forgotten in the other is precisely how they would part, and the cost of that is a caller moving on to the next allocator on an error that means the allocator is broken. The list of errors and the exhaustive match that names them are built by one macro from one list of rows, which is what keeps the list complete: a new variant stops this module compiling until it is given a row, and a row carries the value as well as the name, so it joins the list at the same moment. Naming it then leaves it without an expected outcome, which the outcome table fails on. Written out separately, only the match would be forced. A variant could take its arm, be left out of the list, and never reach any of these tests -- an eleventh classified by both production matches but absent from the list left all four green, and a count against a literal did not catch it either, since nothing said what the literal should be. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Only the reset path was covered, and reset is the one that skips the closing sequence entirely: the flow goes straight to Reset from wherever it was. A graceful close is four further transitions, each deciding how long the flow is then kept, and coverage showed none of them had ever run for masquerade -- neither the client's FIN nor the server's had reached next_flow_status_tcp. Both directions, since they are different paths: a close begun by the client goes by way of CClosing and CHalfClose, one begun by the server by way of SClosing and SHalfClose, and only the last ack is shared. Neither test sleeps. What is being checked is the sequence of states, and asserting on flow expiry as well would tie them to the wall clock for nothing -- which is exactly what made the miri job fail earlier in this stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…bout The generator drew every claim starting inside the window, so the sweep's left-edge clipping -- a port-forwarded prefix beginning below the region and reaching into it -- was never exercised, while the right edge was. A prefix is under no obligation to sit inside the region being asked about, and the pool passes the sweep exactly the indexable slice of one. Claims now begin up to half a window below it, so both overhangs clip against the oracle at every address. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…ng discriminant A masqueraded packet carries a source and destination VPC discriminant, and several places asserted that by unwrapping into unreachable!(). An assertion that is right costs nothing; one that is wrong takes down every flow on the box because a single packet, or a single flow, was not what the code assumed. All of these can drop instead, and dropping reports the same problem at a survivable cost. The one that mattered is flows.rs. It runs over every flow in the table while a configuration is applied, and nothing upstream checks that a flow can say which VPCs it belongs to. A flow that could not would have aborted the process in the middle of the config change. It is now refused carry-over and invalidated, which is what happens to any flow that cannot be carried, and the config apply continues. In nf.rs this is defence rather than a fix, and worth being clear about: process_packet already checks both discriminants and drops with Unroutable, so the sites downstream were guarded. What they were not is self-contained -- an unreachable!() whose reachability depends on a check several frames up is one refactor away from being an abort -- so they take the discriminants through one helper that answers with an error. The new MasqueradeError::MissingDiscriminant reaches the packet as NatFailure. In icmp_handling.rs the source discriminant was only ever used to make a log line, so its absence now costs a slightly duller log rather than the process. The masquerade state downcast beside it is guarded upstream by nat_state.is_some(), which does not establish the type, so it drops too. No test: the entry guard in process_packet carries debug_assert!(false), so a packet without a discriminant panics any debug build before it reaches the code below, and a flow without one cannot be built through the public API. The change is about what a release build does when an assumption fails, which is precisely what a debug build refuses to model. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The pool-level guarantee behind the Clone removal in #1696. That change makes the sequence which broke it unwriteable, so what is left to check is the guarantee itself: a pair a live allocation holds cannot be reserved, and can be once it is released. Lives here rather than with the fix because it needs the claims argument `pool_sets_for_specs` grows in #1698. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

daniel-noland requested a review from a team as a code owner August 6, 2026 02:28

daniel-noland requested review from mvachhar and removed request for a team August 6, 2026 02:28

daniel-noland added the dont-merge Do not merge this Pull Request label Aug 6, 2026

daniel-noland changed the title ~~masquerade: allocator coverage, and the bugs it found~~ (5)-masquerade: allocator coverage, and the bugs it found Aug 6, 2026

daniel-noland marked this pull request as draft August 6, 2026 02:32

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from 206dbaa to 687a54e Compare August 6, 2026 04:31

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from cfaeb7b to c210324 Compare August 6, 2026 04:31

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from 687a54e to a348ddd Compare August 6, 2026 04:36

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch 2 times, most recently from 0e81b6d to f8cd135 Compare August 6, 2026 05:07

daniel-noland added ci:+vlab Enable VLAB tests ci:+release Enable VLAB release tests labels Aug 6, 2026

daniel-noland requested a review from Copilot August 6, 2026 05:15

daniel-noland self-assigned this Aug 6, 2026

Copilot started reviewing on behalf of daniel-noland August 6, 2026 05:15 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from a348ddd to bf7198e Compare August 6, 2026 20:45

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from 767afd8 to b8e77fa Compare August 6, 2026 20:45

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from bf7198e to dd99d23 Compare August 6, 2026 21:25

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from b8e77fa to b63b0b3 Compare August 6, 2026 21:25

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from dd99d23 to 03071be Compare August 6, 2026 21:47

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from b63b0b3 to d2d17de Compare August 6, 2026 21:47

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from 03071be to da2c429 Compare August 6, 2026 22:29

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from d2d17de to 450631a Compare August 6, 2026 22:29

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from 0b06b6e to a23149c Compare August 6, 2026 22:49

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from f17751e to 466b14b Compare August 6, 2026 22:49

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from a23149c to a1348f9 Compare August 6, 2026 23:11

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from 466b14b to 0534812 Compare August 6, 2026 23:11

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from a1348f9 to 7d12391 Compare August 6, 2026 23:51

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch 3 times, most recently from e7aa5d1 to b967c94 Compare August 7, 2026 00:28

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch 2 times, most recently from 03d977d to dcfa3a1 Compare August 7, 2026 01:07

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch 2 times, most recently from 8e60212 to 058c8b5 Compare August 7, 2026 01:30

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from dcfa3a1 to 5a47ebc Compare August 7, 2026 01:30

daniel-noland and others added 14 commits August 6, 2026 19:45

daniel-noland changed the base branch from pr/daniel-noland/pool-table-lookup to pr/daniel-noland/port-bitmap-free August 7, 2026 01:50

daniel-noland force-pushed the pr/daniel-noland/allocator-coverage branch from 058c8b5 to 10ce616 Compare August 7, 2026 01:50

daniel-noland changed the title ~~(5)-masquerade: allocator coverage, and the bugs it found~~ (4)-masquerade: coverage, and the bugs it found Aug 7, 2026

Navigation Menu

Uh oh!

(4)-masquerade: coverage, and the bugs it found - #1700

(4)-masquerade: coverage, and the bugs it found#1700
daniel-noland wants to merge 14 commits into
pr/daniel-noland/port-bitmap-freefrom
pr/daniel-noland/allocator-coverage

daniel-noland commented Aug 6, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

daniel-noland commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Added while reviewing the stack

Stack

Uh oh!

coderabbitai Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniel-noland commented Aug 6, 2026 •

edited

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading