clock by daniel-noland · Pull Request #1756 · githedgehog/dataplane

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

No description provided.

coderabbitai · 2026-08-26T02:59:12Z

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

codecov · 2026-08-26T03:18:20Z

Codecov Report

❌ Patch coverage is 95.57781% with 106 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
routing/src/router/rio.rs	93.21%	24 Missing and 17 partials ⚠️
nat/src/portfw/fuzz.rs	85.36%	19 Missing and 17 partials ⚠️
config/src/external/overlay/vpcpeering.rs	93.93%	4 Missing ⚠️
stats/src/vpc_stats_fuzz.rs	98.23%	0 Missing and 4 partials ⚠️
nat/src/masquerade/expiry.rs	98.17%	2 Missing and 1 partial ⚠️
nat/src/portfw/expiry.rs	97.08%	2 Missing and 1 partial ⚠️
dataplane/src/drivers/kernel/mod.rs	0.00%	2 Missing ⚠️
nat/src/portfw/probe.rs	98.95%	0 Missing and 2 partials ⚠️
net/src/flows/flow_info_fuzz.rs	99.37%	2 Missing ⚠️
stats/src/dpstats.rs	66.66%	2 Missing ⚠️
... and 7 more

📢 Thoughts on this report? Let us know!

Anything with a timeout is untestable in a useful way if its deadline comes from the wall clock: the test either sleeps for real -- seconds of CI time per case, flaky under emulation -- or it does not test expiry at all. The flow table's expiry, the masquerade and port-forwarding timeouts, the stats delivery schedule and the FRR reconnect timers are all in that position. tokio can already pause and advance time, and the dataplane already runs on tokio. What stopped that from working was an asymmetry: deadline = std::time::Instant::now() + timeout // does not move sleep_until(tokio::time::Instant::from_std(deadline)) // does move The two agree at the moment a test pauses the clock and diverge immediately after, so a paused clock bought exactly **one** time step. A flow opened after a single `advance` is born with a deadline already in the past and is dead on arrival, which looks exactly like a masquerade bug and is not one. Three call sites define every flow lifetime, and pointing those three at `tokio::time::Instant::now()` would make multi-epoch time control work today. It would also break confusingly later, the first time someone innocently writes `Instant::now()` in a fourth place -- the symptom is a timeout test behaving strangely under a paused clock, which is a long way from the cause. So: one place the workspace reads the time, and a lint that refuses the others. `.semgrep/rules/no-std-time-direct.yaml` is the same shape as `no-std-sync-direct.yaml`, for the same reason -- clippy sees the facade's re-exports by canonical path, so the rule catches the call sites. `clock` follows `concurrency`: a cargo feature selects the backend, not a `--cfg`. That is what `concurrency` actually does, and it matters here because a `--cfg` cannot pull in an optional dependency -- tokio would become a hard dependency of everything that reads a clock. With a feature, production `net` has no tokio at all. * `clock::now()` -- `std` in production, tokio's pausable clock under `virtual`. Enabled in the dev-dependencies of every crate that reads a clock, so a test build gets the routed clock across the whole graph rather than in one crate. * `clock::system_now()` -- wall clock, deliberately **not** routed. tokio pauses its monotonic clock, not the system clock, and the values that use it are timestamps reported outwards rather than deadlines anything waits on. It lives in the facade so the lint has one chokepoint. * `Duration` is exempt and the lint says so. A duration is a plain value with no clock in it. Production cost is zero, and checked rather than assumed. From tokio 1.53.1: #[cfg(not(feature = "test-util"))] mod variant { pub(super) fn now() -> Instant { Instant::from_std(std::time::Instant::now()) } } `cargo tree -p dataplane -e features -i tokio | grep -c test-util` is 0: dev-dependency features do not unify into the production binary. 47 `Instant::now()` and 3 `SystemTime::now()` sites across 22 files and 11 crates, all migrated. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The masquerade properties deliberately stayed inside one flow lifetime, because until the workspace read its deadlines through `clock` there was no way to write these. With the deadlines and the timers on the same clock, expiry becomes an ordinary subject -- and a cheap one: a property covering a minute of flow lifetime runs in no wall clock at all, where the real-time version would cost a minute per case and be flaky under emulation. The design note asks that every piece of live state a configuration change could touch be classified. Expiry is the same question asked of time: * **preserved** -- a flow inside its lifetime keeps behaving identically. Not assumed: the cheap way to implement expiry is also the cheap way to drop something whose deadline has not passed. * **invalidated attributably** -- a flow past its lifetime stops translating and is dropped. The failure ruled out is not the drop but the *pass*: forwarding a packet still addressed to a public tuple is a leak, not a timeout. * **never resurrected** -- an expired flow does not come back, and a flow created *after* the clock moved works. That one needs three epochs and was unwritable before. Plus `traffic_extends_a_flow_past_its_first_deadline`, which is the regression test for the facade itself: `reset_expiry_unchecked` was one of the three sites reading the wall clock while the timer consuming its answer read tokio's, so a refresh under a paused clock wrote a deadline in the past and *shortened* the flow's life. `a_live_flows_tuple_is_reissued_after_its_original_deadline` reproduces a defect that is not fixed here, `#[ignore]`d so the branch stays green. Run it with `cargo test -p dataplane-nat -- --ignored reissued`. A flow is opened and refreshed every second, so it is unambiguously alive -- its replies are delivered correctly throughout. Once `MASQUERADE_ONEWAY_TIMEOUT` of virtual time has passed since it was opened, a newly opened flow is handed the same public address and port, and the replies that were reaching the first tenant start reaching the second. The threshold is exactly the one-way timeout: four seconds of advance leaves the tuple held, five reissues it. So the allocation is reclaimed on the deadline the flow was *created* with, and the refreshes that keep the flow alive do not carry it. `MasqueradeState` holds the `Allocation` in the forward entry only, which is consistent with the forward entry being reclaimed while the reverse entry survives and keeps translating -- but that is where an investigation should start, not what it has concluded. Production sets `randomize(true)`, and with randomization the same sequence draws a different port, so the collision is unlikely rather than impossible -- a matter of load and range size rather than of correctness. What is wrong in both modes is the underlying state: a flow that is still translating no longer owns the tuple it is translating to. Two tenants sharing a public tuple is a tenant isolation failure. This is the class of defect a controllable clock exists to find. Reaching it on the wall clock needs six seconds of real time per attempt and the right allocation pattern; here it is deterministic and free. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The third and last NAT flavour, and the one where the only confirmed configuration bug of this stack lived. That was found by reading code and pinned at the configuration level; this covers the stage. The direction is the point. Static NAT and masquerade translate a **source** on the way out. Port forwarding translates a **destination** on the way in, so every property reads the other half of the five-tuple and the failure modes differ in kind. A masquerade mistake leaks one tenant's traffic to another; a port-forwarding mistake delivers the outside world to an address inside a tenant that never published it. Six properties: reversibility (the reply must carry the tuple the client used, or the client drops it), containment, injectivity, permission, frame, and stability. Three more in `expiry`, on a paused clock: a published service keeps answering while time passes, a service re-established after its flow expires reaches the *same* backend -- the rule is configuration and does not expire with the flow -- and the mapping stays injective across an expiry. Two things worth recording about the harness. **The probes were judging packets that never reached the code.** A rule is keyed by `(source vpc, protocol)`, and the probe drew its protocol independently of the rule it addressed. Roughly half of them therefore matched nothing, and `nothing_is_forwarded_that_was_not_published` -- whose subject is exactly the packets that should not match -- passed for the wrong reason. The protocol now comes from the rule. Same class of mistake as the arrival-state ordering in masquerade: a harness that looks like it is testing something and is not. **The port-range guard has three gates, not one.** A port past the published range is refused independently by `RangeSet::lookup`'s upper bound, by `PortRange::contains` via `indexof`, and by the size-matched arithmetic in `map_port_to`. Opening any one, or any two, still refuses the packet; only all three together let it through. That is defence in depth rather than redundancy, and it is why the permission property looks vacuous at first -- it is not, the code is simply hard to break there. `PortForwardingExposes` gets the block-and-family treatment the other two flavours got, plus one constraint they do not have: a rule is keyed by protocol, so two exposes naming the same protocol produce two rules with the same key and the second silently replaces the first. One protocol per expose, assigned by position. The injectivity sweep is capped and strided at 256 pairs. Enumerating a rule that publishes 256 addresses over 1024 ports costs a quarter of a million packets, and that property was spending its entire budget on two configurations; it now reaches eleven. One observation, not fixed: a packet whose port is outside the published range is dropped with `DoneReason::InternalFailure`. Nothing is internally broken -- the operator did not publish that port. Attribution, not correctness. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`FlowInfo` is the state every stateful NAT flavour shares. Masquerade, port forwarding and the flow table all keep a flow alive by refreshing this object, and every timeout in the datapath is ultimately a comparison against its `expires_at`. It had no tests of its own. The network function harnesses build a configuration from an algebra of operations and judge the result by relations. This is the same idea at unit scale and much cheaper for it: `Op` is the vocabulary a flow supports -- refresh it two ways, move its status, invalidate it, let time pass -- and the headline property asserts an invariant after *every prefix* of a drawn sequence rather than after one fixed order. That shape is what the subject needs: `reset_expiry` is gated on status, `extend_expiry` on a different subset of it, and both on the clock, so a test with a fixed order would walk one path through that lattice and call it covered. * **expiry never moves backwards** -- the invariant the whole mechanism rests on. A deadline that moves earlier is a flow that dies while in use: the timer fires early, the entry is dropped, and the NAT state goes with it. This is also the invariant that was quietly violated while deadlines came from the wall clock and timers from tokio's -- the comparison inside `reset_expiry_unchecked` was between values from two different timelines. * **a refused refresh changes nothing** -- the frame condition, and load bearing: every production call site discards the result with `let _ =`, so a refusal that had already moved the deadline would be a silent write behind an error return. * **a refresh is permitted exactly when the status allows** -- stated as an iff, because a gate that is too permissive is the defect and a one-way check cannot see it. * **invalidating is idempotent and cancels the timer** -- `invalidate` is reached more than once for the same flow as a matter of course. A flow marked cancelled whose token still sleeps is an entry that lingers to its original deadline. * **a related pair refers to its partner** -- `related_pair` builds two flows that each hold a `Weak` to the other, through `Arc::new_uninit` and raw pointer writes, because neither can exist before the other. Nothing exercised that round trip: that each `Weak` upgrades, and to the *other* flow rather than to itself. * **a pair needs exactly one initiator**, and **every status survives its byte** -- the latter matters because the status is read back through a `TryFrom` that panics on an unrecognised value. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A masqueraded connection is two flow entries, forward and reverse. `refresh_masquerade_state` refreshed only the one a packet happened to hit; the partner was refreshed exactly once, on the transition into `Established`. So a connection whose traffic runs mostly one way lets the other half expire while it is still in use -- and that is the common case rather than a corner. A download is almost all reverse packets. So is a DNS response, or any session that mostly receives. It is worse than a dropped connection. `MasqueradeState` carries the `Allocation` in the **forward** entry alone, so the forward half expiring releases the address and port while the reverse half goes on translating to them. The allocator hands that tuple to another tenant, whose replies arrive at the first tenant's still-live reverse entry. Two tenants sharing one public tuple is a tenant isolation failure, not a routing one, and nothing on the packet path shows it: every reply to the first tenant keeps being delivered correctly the whole time. Only a *new* flow taking the tuple reveals it. The fix is to refresh the partner on every refresh rather than on one transition. `reset_expiry_unchecked` refuses to move a deadline earlier, so extending the partner can only lengthen its life. This is what conntrack has always done: a packet in either direction is evidence the whole connection is alive. Reaching this on the wall clock needs five seconds of real time per attempt plus the right allocation pattern -- and `randomize(true)` in production makes the reissue unlikely rather than impossible, so it would surface in the field as a rare, unreproducible cross-tenant delivery. The `#[ignore]`d reproduction is replaced by two passing properties: * `both_halves_of_a_pair_outlive_one_sided_traffic` -- the mechanism, asserted on the live flow count, because the packet path cannot see it; and * `a_live_flows_tuple_is_never_reissued` -- the outcome an operator would feel, kept separate because it would also catch a different allocator bug that released a tuple for some other reason. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Every rate the dataplane reports passes through this, and it had no tests. `rate.rs` covers the Savitzky-Golay filter beside it well -- differentiating generated polynomials against the analytic derivative, a real oracle honestly come by -- but the exponentially weighted moving average had none, and it is the piece that is a function of *time*. No runtime is needed, which is worth noticing: `update` takes the `Instant` as a parameter rather than reading a clock, so it is already dependency-injected and a property can hand it any timeline. That is the shape the rest of the workspace had to be converted to -- a function given the time it should use needs none of the machinery in `clock`. No oracle either: recomputing `data * (1 - alpha) + last * alpha` in the test would be a second copy of the thing under test. These are the properties an exponentially weighted moving average has by construction. * the first sample is the average, not something averaged against zero -- the alternative reads low for seconds after a restart, which is exactly when someone is looking; * the average never leaves the range of the samples seen (convexity); * a constant input stays constant, at every spacing -- a rate that drifts while the counter advances steadily is the most misleading thing this code could do, because it looks like real traffic; * a step is approached monotonically and never overshot; * **a longer gap weights the new sample strictly more**; and * reading the average does not change it. The time-weighting property has to be strict. Written with `<=` it is close to worthless: an implementation that ignores elapsed time altogether returns the same number from both runs, and "not further away" is true of equal values. Strict, with the inputs bounded so the difference is bigger than floating-point noise -- gaps of 1ms to 1s against a tau of 1s, at least 100ms between the two gaps, and samples at least 1 apart. Past a few multiples of tau every gap saturates to "the new sample entirely" and there is nothing left to order. Convexity has to be checked relative to the magnitude for the same kind of reason. A weighted mean of values near 44,000 lands an ulp or two outside the bound, which is thousands of times `f64::EPSILON` and not a violation of anything; an absolute tolerance there tests floating point rather than convexity. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`VpcStatsStore` is what the gateway reports about itself over gRPC: per-vpc and per-vpc-pair packet and byte counters, the latest rates, and the names those numbers are labelled with. It had no tests at all. Statistics are an odd testing target because being wrong is not an outage, which is exactly why they are worth pinning. A counter that silently wraps, a rate attributed to the wrong pair, or a name that outlives the vpc it belonged to all produce numbers an operator will act on with no way to tell they are wrong. The store has a small vocabulary -- add counts, add drops, set rates, record both at once, prune to a live set, snapshot -- so the properties are invariants over drawn sequences rather than statements about any one order. Two are relations between operations, needing no oracle at all: * `record_pair` must equal `add_pair_counts` followed by `set_pair_rates`, checked by driving two stores in parallel. A compound operation that drifts from the parts it composes is how two call sites come to disagree about the same numbers. * the pair table and the per-vpc table must not disturb one another. They look like they should be linked -- a per-vpc total ought to be the sum of its pairs -- and they are not: each is maintained independently by the caller. Writing that down is the point, because a reader who assumes the link exists will under-report, and a change that introduced it would make every caller that maintains both double count. Pruning is the sharp one. `prune_to_vpcs` keeps a pair only if its source **and** destination survive. A slip to `||` keeps half-dead pairs that report traffic to a vpc which no longer exists and which nothing will ever clean up, because the next prune has the same defect. Both directions are asserted -- keeps exactly the live set, and removes nothing that is alive -- since "exactly" is two claims and a one-sided test would pass for a prune that deleted everything. Names are pruned by the same rule for a different reason: a name outliving its vpc gets attached to whichever discriminant is allocated next, so an operator reads one tenant's traffic under another tenant's name. Saturation is a separate property from monotonicity rather than a corollary of it: reaching `u64::MAX` needs a boundary case constructed deliberately, which the drawn values monotonicity uses will not reach. One note for anyone writing async property tests here: build the runtime *outside* `bolero::check!` and enter it per iteration. A bolero body is synchronous, so blocking on a runtime from inside one is refused at run time. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

When a batch of packets is counted, the interval it covers rarely lines up with the reporting window it has to be attributed to. `TimeSlice::split_count` decides how many belong to the window that is closing and how many carry over -- and it is the only arithmetic in the collector that can silently change what the gateway reports. It was entirely uncovered, along with the `TimeSlice` impls beneath it. The invariant that matters is **conservation**: whatever the two intervals look like, the halves must sum to exactly the count that went in. A split that loses packets under-reports; one that duplicates them reports traffic that never happened. Neither surfaces as an error anywhere -- the number is simply wrong, in a shape indistinguishable from a real change in load. Two asymmetries are documented rather than asserted away, because both are real behaviours a reader would trip on. **A zero-length sample goes wholly outside.** So the function is discontinuous at zero: a one-nanosecond sample inside the window is wholly inside, while a zero-length one at the same instant is wholly outside. That is the guard against dividing by zero doing its job, but it means "growing the overlap never loses inside share" is false across that step, and the monotonicity property has to start from a non-empty sample. **A sample entirely *before* the window is salvaged into it**, not discarded -- where a sample after it carries over. Not symmetry, and deliberate: the window a late sample belongs to has already concluded and been reported, so the alternative to folding it into the current one is losing the packets. It falls out of the mirror in `split_count`, reads like a bug on the way past, and now has a property saying it is not. The disjoint short-circuit `next.start() >= self.end()` is a fast path rather than a semantic gate: delete it outright and nothing changes, because `Instant::duration_since` saturates at zero, so the general arithmetic computes `count * 0 / duration` and reaches the same answer. It would have been load bearing when `duration_since` still panicked on a negative difference. Left in place and noted where a reader will look, because what makes the boundary half-open is the arithmetic, not that branch. `stats/src/{vpc,spec,register}.rs` stay uncovered on purpose. They build metric names and label sets -- a variation on the standing "do not test printers" rule, where the test would assert that a string is the string it was constructed from. Noting the exemption is as much as is worth doing. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`rio.rs` had the worst branch coverage in the workspace, and the part of it that matters most was untested for a mundane reason: exercising the stale window costs sixty seconds of wall clock per attempt. When FRR restarts, every route we hold becomes suspect. FRR will re-send what it still believes; whatever it does not re-send was withdrawn while we were not listening. `set_stale_timeout` opens a sixty-second window and `check_stale_timeout` closes it, sweeping what did not come back. Get it wrong in one direction and withdrawn routes persist; in the other, live routes are blackholed for the length of the window. On a paused clock the window costs nothing, so the boundary can be pinned exactly rather than approached from a safe distance. This is the first thing in `routing` to use the facade for what it was built for. * `arming_the_stale_timeout_sweeps_nothing` -- the sweep happens on expiry, not on arm. * `the_stale_timeout_survives_its_own_deadline` -- `check_stale_timeout` asks `deadline < now`, so landing exactly on the deadline leaves the window open for one more poll. Only a driven clock can put a caller on that instant at all; on the wall clock the case is unreachable. * `the_stale_timeout_fires_once_and_then_disarms` -- `take_if` consumes the deadline. A plain comparison would re-sweep on every later poll, which is harmless for routes that are already gone but would keep re-deleting vrfs the control plane had since re-created. * `an_unarmed_stale_timeout_never_fires`. * `a_deleted_vrf_outlives_the_window_and_no_longer` -- the same boundary seen from the vrf table rather than from the deadline. * `an_frr_restart_opens_the_stale_window` -- and sweeps vrfs that were mid-deletion immediately, because nobody is going to finish deleting them now. * `a_refresh_with_no_peer_to_ask_is_not_marked_done` -- `NeedRefresh` means *we* restarted and must ask FRR to re-send. With no peer address there is nobody to ask, so the status deliberately stays outstanding. Transitioning to `Connected` here would silently accept a database that was never refilled. * `the_settled_cpi_states_do_nothing` -- `cpi_status_check` runs on every pass of the IO loop, so the states it ignores must be free of side effects, or the loop would re-arm the window continuously and never sweep. What remains is the IO loop and the socket paths under it -- `cli_sock_restore`, `cli_wake_on_writeable`, `reregister` -- which need a live peer on the other end of a unix socket rather than a clock. That is a harness, not a property. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The IO loop was reachable only by starting it and sleeping, so `rio.rs` had the worst branch coverage in the workspace and `cpi.rs` was barely better. Neither needed a production change to fix: the CPI and CLI sockets are ordinary unix datagram sockets, and a test can simply be the peer on the other end. `CpiPeer` binds the far end of the CPI socket and speaks dplane-rpc at the loop exactly as FRR's plugin does. `RunningRio` starts the real loop and stops it on drop. Nothing is stubbed, so readiness, the read, the decode, the dispatch and the addressed reply are all under test rather than around it -- a reply only arrives if the loop sent it back to the address the datagram came from. * `the_cpi_is_not_attended_until_it_is_unlocked` * `a_connect_over_the_cpi_socket_is_answered` * `a_request_before_any_connect_is_ignored` * `without_a_config_additions_are_refused_and_deletions_are_not` * `a_malformed_datagram_draws_a_notification_and_does_not_wedge_the_loop` * `the_cli_survives_having_its_socket_path_removed` Every CPI test times out until you know why. `CPSOCK` is registered `Interest::PRIORITY`, which mio maps to `EPOLLPRI` alone. A unix datagram socket never carries out-of-band data, so no readable event is ever raised and the loop is deaf to the CPI by construction -- not by a flag somewhere in the dispatch. `RouterCtlMsg::Unlock` reregisters it `READABLE | WRITABLE`. That is the "do not attend cpi until configured" feature, and it now has a test, because registering the socket `READABLE` "to fix a bug" would silently undo the feature and every other assertion here would still pass. Deafness turns out to defer rather than drop: a datagram sent while the CPI is unattended sits in the socket's receive queue and is served on unlock. So unlocking replays whatever arrived while we were not listening, bounded by `SO_RCVBUF` rather than by anything this code decides. The `last_pid` guard in `handle_request` is what keeps that safe. The property asserts the replay explicitly. The cli property asserts that a new client is *served*, not that the socket path reappeared. The weaker form is satisfied perfectly by a rebind that never re-registers with the poller, which answers nobody -- the same class of mistake as an inequality written non-strict: a property weaker than it reads. Noted and left alone: dropping the `deregister` of the old fd in `cli_sock_restore` changes nothing observable, because closing the fd removes it from the epoll set anyway. Defensive rather than load-bearing. `SockDir` gives each test its own directory and removes it on drop. The paths must be unique under two execution models -- `cargo test` runs every test in one process on many threads, `nextest` gives each its own process -- so a process-global counter covers the first and the pid covers the second, with no coordination between tests either way. `test_rio_ctl` moves onto the same fixture. It bound the fixed `/tmp/hh_dataplane.sock`, which is the path a *running* dataplane uses, and `open_unix_sock` unlinks before it binds; under nextest, which runs test binaries concurrently, it could pull the socket out from under a real dataplane or another copy of itself. The CPI socket could have avoided the filesystem entirely -- Linux abstract sockets have no directory entry and vanish when closed -- but the CLI socket cannot: `setup_clipath_watcher` watches the parent directory precisely because unlinking the path leaves the inode alive while the socket is open, so no `DELETE_SELF` is ever emitted. An abstract name has no parent and `Rio::new` would refuse it. Half a fixture's worth of cleanup is not worth a new production affordance on a component we intend to remove. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The frrmi's wire format is already covered in `frr::frrmi` against a `UnixStream::pair()`. What was not covered is the loop's lifecycle around it: connect, disconnect, restart. Those were three of the five functions `rio.rs` never entered. This is not a fake FRR. The frrmi does not talk to FRR. It talks to `frr-agent`, a Hedgehog component that sits beside FRR and applies configuration to it, over a Hedgehog wire format. `FakeAgent` is a `UnixListener` at the frrmi path impersonating our own agent and nothing else -- no FRR, no bgpd, no zebra, nothing GPL, and nothing that needs a container to run. Running a real FRR stack to close a unit-test gap stays off the table; that is what the vlab jobs are for. frrmi is `SOCK_STREAM`, so an accepted connection has an implicit peer and none of the addressing trouble that ruled out `socketpair` for the CPI applies. * `the_loop_connects_to_the_agent_whenever_it_appears` -- rio starts with nothing listening, so its first connect fails. That is the normal case rather than an edge one: the dataplane and the FRR container come up in whatever order they come up in, and a loop that gave up after one refusal would need a restart to recover. * `the_loop_reconnects_when_the_agent_goes_away` -- `frr-agent` restarts whenever FRR does, which is the moment the dataplane most needs to push configuration back. A loop that held the dead socket would go on believing it had a link and quietly stop applying anything. * `nonsense_from_the_agent_restarts_the_link` -- the first four octets are an announced length, so a burst of `0xff` announces an absurd message. `frr::frrmi` refuses it; this asserts what the loop does with the refusal. The connection is deliberately held open, so the restart can only have come from the refusal and not from an end-of-file. The `frrmi_connect()` inside `frrmi_restart` is not covered by any of them, and deliberately: the loop attempts a connect at the top of every pass regardless, so that call only saves the one iteration between the failure and the next pass -- latency, not correctness. Distinguishing it would need an assertion on how *fast* the reconnect happens, and an absolute timing bound measures how busy the machine is rather than what the code does. Everything here and in the previous commit deliberately avoids needing a configuration, because `RouterCtlMsg::Config` wants a `ValidatedGwConfig` and building one in a test is a substantial piece of work in its own right. That is a real gap rather than a stylistic choice: without it these tests cannot reach a route actually installed into a fib, `reapply_frr_config` after an FRR restart, or the config round trip through frrmi and out to `ShowFrrmiLastConfig`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`RouterCtlMsg::Configure` carries a `RouterConfig`, not a `ValidatedGwConfig`, and a `RouterConfig` is three lines: a non-zero genid and one vrf. `validate` objects only to duplicate vnis and to a vtep that is not set up, and `handle_configure` calls `db.set_config`, so `have_config` becomes true and the CPI starts accepting additions. That was the whole blocker. The previous two commits routed around it, which biased everything written so far toward guards and refusals -- `Add` was only ever reachable as an `Ignored`. * `a_route_the_control_plane_announces_reaches_the_fib` -- the point of the CPI: a datagram on a socket becomes a forwarding entry. Decode, dispatch, rib insertion, reconciliation and the left-right publish are all in the path of one assertion. * `a_route_the_control_plane_withdraws_leaves_the_fib` -- a withdrawal that did not take forwards at a next hop the control plane has stopped believing in, which no reconvergence elsewhere can clear. * `announcing_a_prefix_twice_leaves_one_route` -- FRR re-sends its whole table after a restart, so every prefix arrives again for one already held. A fib that grew on each pass would double every time FRR bounced. This one is a structural guard rather than a behavioural one: routes are held in a prefix-keyed trie, so duplication is not representable. Worth keeping against a change to a multimap for ECMP. `fib_v4` filters `0.0.0.0/0`, which a fresh vrf carries so that traffic with nowhere to go is dropped rather than leaked. Counting it puts every assertion off by one and would hide a withdrawal that removed the wrong route. `a_configuration_reaches_the_agent_and_its_answer_comes_back` drives the other half of the dataplane's job: hand FRR the configuration it should be routing under, and remember which generation was applied. `FakeAgent` now reads the `|length|genid|body|` frame off the wire and answers in kind. It fails if the config is never handed to the frrmi, and fails if the acknowledgement is never recorded -- which matters because `reapply_frr_config` consults exactly that after a restart. `a_large_answer_arrives_whole` announces 8192 routes and reads back an 850KiB fib listing across some four hundred chunks. It fails if the "more" octet is computed from the wrong end of the loop. It does not reach `cli_wake_on_writeable`, which is the last uncovered function in the file, and the measurements say why -- recorded next to it, because the next person to read the coverage report deserves them before spending an afternoon on it. That function runs only when a response cannot be sent in one go: the send fails with `WouldBlock`, the remaining chunks are cached, and the socket is re-armed for writability so the cache drains when the client catches up. The client's receive queue is not what bounds it. 850KiB to a client that is provably not reading -- held off by a causal barrier on the CPI socket rather than a sleep -- still never blocks the send. What bounds it is the *sender's* buffer: `open_cli_sock` sets the loop's `SndBuf` to `CLI_RX_BUFF_SIZE`, 2048 * 8192, or 16MiB, and on a unix datagram socket that is what limits outstanding unread traffic. `RcvBuf` on the client does not help -- pinned to the kernel floor of 2304 it still accepts about 100KiB, because that ceiling is the sender's too. Reaching the cache path needs roughly 16MiB of unread answer, on the order of 150,000 routes. That is not a proportionate test for one function, so it stays uncovered deliberately. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A first run of cargo-mutants over `net/src/flows/flow_info.rs` -- a file covered by seven properties -- caught 13 mutants and missed 28. Nine of the survivors were noise; nineteen were real. The one that makes the case: if new < current { return Err(FlowInfoError::TimeoutUnchanged); } `<` to `==` was caught. `<` to `>` was caught. `<` to `<=` survived. That is the same boundary class hand-broken in `rio.rs`, where it *was* caught -- because there it was suspected and a property was written for it. Suspicion is not uniform, which is the argument for the tool: it breaks what nobody thought to break. Nor is it academic here. `reset_expiry_unchecked` is what the masquerade expiry path calls on both halves of a flow pair, which is the defect this stack already fixed once. * `the_unchecked_refreshes_move_the_deadline_exactly` -- the checked wrappers were covered by whether they *refuse*; nothing said what the unchecked ones *do* when they accept. Extension adds exactly its duration; reset lands exactly on `now + duration`; and resetting to the deadline already held is accepted rather than refused, which is the only place `<` and `<=` differ. Reachable only on a driven clock: on a wall clock two instants are never exactly equal. * `a_flow_is_active_exactly_when_its_status_says_so` -- `is_active` is what the datapath asks before using a flow. All three of its mutants survived before this. * `a_flow_built_with_a_status_has_it` -- `new_with_status` ignoring its argument would make every test that seeds a non-default state pass for the wrong reason. * `a_genid_is_remembered_and_reaches_the_partner` -- halves that disagree about their generation get swept apart, one retired and the other left translating to an allocation nobody owns. * `each_flag_predicate_answers_for_its_own_bit` -- a predicate answering for the wrong bit translates the wrong end of the flow. * `the_destination_vpc_is_remembered` -- a reader that always said `None` is indistinguishable, from the packet path, from a flow that has not been through the lookup stage. The file now has no surviving mutants. `.cargo/mutants.toml` carries two exclusion categories, both in config rather than as `#[mutants::skip]` attributes, so production crates take no dependency on the tool: * Printers, per the standing "don't test printers" rule. A mutated `fmt` that nothing notices is the rule working, not a gap. * `contract::` modules. The workspace convention puts bolero generators next to the type they generate, so cargo-mutants finds them and mutates the harness at itself; a generator drawing a different distribution is not a defect in the code under test. Plus `sysfs/**`, whose write path needs root and a real sysfs. Nothing in CI depends on this. It is run by hand over a crate or a diff, and the product is the survivor list rather than the score. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`cargo-mutants` over `nat/src/masquerade/` left 52 survivors, and 23 of them were in one file: very nearly every match guard in `next_flow_status_tcp`, plus the DNS arm of the UDP patch and the single ICMP transition. There were already four TCP tests -- `test_masquerade_tcp_establish`, `_reset`, and both close directions -- and they are not weak: each caught thirty-odd mutants elsewhere in the module. But they walk a sequence and check where it ends up, so they never discriminate *which* guard fired. Replacing a guard with `true`, or `&&` with `||`, left all four passing. Nothing forwards differently because of the status. The flow's *lifetime* follows from it, and a public address and port are held for as long as the flow lives. A machine that never reaches `Closed` conserves nothing; one that reaches it early releases a tuple that can be handed to another tenant while the connection is still running -- which is the failure this stack already fixed once, approached from the other end. The whole TCP input space is 2 actions * 10 statuses * 16 flag combinations = 320 cases. Sampling that would be perverse when enumerating it makes the coverage argument disappear. The oracle is a table, which is allowed here for a specific reason. Elsewhere the properties deliberately avoid restating the implementation, because an oracle that mirrors the code mirrors its bugs. That objection has teeth when the oracle would grow into a second dataplane. Here the oracle is the TCP close sequence, which is older than this codebase and will outlive it, so writing it down is specification rather than duplication -- and where the two disagree, the table is what should be argued about. It is derived from what the flags mean, not from what the code does, and it agrees with the implementation on all 320 cases. Two properties are stated separately from the table because they should be readable without checking it row by row: a segment carrying none of the four flags moves nothing, and `Reset` absorbs. Beyond TCP: a UDP reply from port 53, 853 or 8853 closes the flow at once -- one lookup, one reply, and the tuple is released rather than held for the ordinary UDP lifetime, which on a busy gateway is most of the port space -- and an ICMP reply moves a one-way flow to two-way and nothing else moves at all. `protocol.rs` now has no surviving mutants. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`fix(masquerade): Keep both halves of a flow pair alive` made this argument for masquerade and left the identical code in port forwarding: refresh only the half a packet happened to hit, except on the transition into Established. A pair is one connection, and a packet in either direction is evidence the whole thing is alive. Refreshing one half lets the other expire under a live connection whenever traffic runs mostly one way -- and for a published service that is the ordinary case, not a corner. A client uploading refreshes the forward half on every packet while the reverse half, which carries the translation its replies need, times out beneath it. Milder than masquerade's, deliberately said so in the comment: port forwarding maps from the rule rather than from an allocation, so nothing is released to the pool and no tenant sees another tenant's traffic. What is lost is the connection. `reset_expiry_unchecked` refuses to move a deadline earlier, so extending the partner can only lengthen its life. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Two consecutive blank lines after `const PATIENCE` fail `cargo fmt --check`, which is a CI gate (`ci::check-fmt`) and part of `just pre-flight`. The `std::time::Instant` import in the CLI display module is the last one outside `clock/`. The semgrep rule catches `::now()` calls rather than imports, so it was legal -- and an invitation to write the call the rule exists to refuse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Copied from `masquerade::fuzz` before that helper was corrected, and with the same consequence: `check!()` inside a closure returns from the closure, so under `CARGO_BOLERO_SELECT` the vacuity guard runs on every count at zero and refuses the target enumeration. With this and the masquerade fix below it, `CARGO_BOLERO_SELECT=all` over the whole crate passes: 204 tests, none failing. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`reset_expiry_unchecked` loaded the stored deadline, compared, and stored -- three steps. Two threads could therefore both pass the guard against the same stale value and the shorter write land last, which is the deadline moving *backwards*: the one thing that comparison exists to prevent. `fix(masquerade): Keep both halves of a flow pair alive`, four commits back, is what makes this ordinary rather than exotic. Before it a packet refreshed the half it hit; now both directions refresh both halves on every packet, and `extend_by` differs by status, so concurrent writes of *different* deadlines to one location are the normal case for any bidirectional flow. Losing the longer write on the forward half is the serious outcome, because that half owns the `Allocation`: it expires under a live connection and the allocator hands its public tuple to another tenant -- exactly the failure refreshing both halves was meant to stop, reached by a narrower path. The guard stays strictly greater, so resetting to the deadline already held is still accepted; `the_unchecked_refreshes_move_the_deadline_exactly` pins that boundary deliberately and rejects the `>=` spelling in under a millisecond. No regression test, and the comment says why at length: tsan cannot see an atomicity violation between two atomic operations, a 200,000-round stress test scores zero, and the model checker that would settle it needs `AtomicInstant` routed through `concurrency` first. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Copied from `masquerade::fuzz` before that floor was relaxed; same reasoning. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 1017959 to c0bc094 Compare August 26, 2026 17:30

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 1576fc2 to 6498991 Compare August 26, 2026 17:30

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from c0bc094 to caeff74 Compare August 26, 2026 19:36

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 6498991 to bd73f69 Compare August 26, 2026 19:36

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from caeff74 to fc901ba Compare August 26, 2026 20:41

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from bd73f69 to 9889d61 Compare August 26, 2026 20:41

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from fc901ba to 3d43e5b Compare August 26, 2026 21:02

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 9889d61 to 409177c Compare August 26, 2026 21:02

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 3d43e5b to 357270f Compare August 26, 2026 21:13

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 409177c to 1ecf256 Compare August 26, 2026 21:13

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 357270f to ffec6ba Compare August 27, 2026 01:29

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 1ecf256 to 6a77767 Compare August 27, 2026 01:29

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from ffec6ba to f51c0fc Compare August 27, 2026 01:41

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 6a77767 to 668c5fc Compare August 27, 2026 01:41

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from f51c0fc to 96cbbc7 Compare August 27, 2026 04:32

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 668c5fc to 99956bf Compare August 27, 2026 04:33

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 96cbbc7 to 0b54681 Compare August 27, 2026 05:10

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 99956bf to d28e0ca Compare August 27, 2026 05:10

daniel-noland mentioned this pull request Aug 27, 2026

validator #1754

Draft

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 0b54681 to 4748ecd Compare August 27, 2026 17:59

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from d28e0ca to d682c66 Compare August 27, 2026 17:59

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 4748ecd to 281d3bc Compare August 27, 2026 18:29

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from d682c66 to 17c12ab Compare August 27, 2026 18:29

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 281d3bc to d61d365 Compare August 27, 2026 19:33

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch 3 times, most recently from a3d48a8 to efb7d61 Compare August 27, 2026 21:28

daniel-noland changed the title ~~feat(clock): read the clock through a facade, and test what that makes testable~~ clock Aug 28, 2026

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 12bc5f3 to a12180c Compare August 28, 2026 05:57

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 2065c3b to 43a361f Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from a12180c to 54995e7 Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 43a361f to 81f2482 Compare August 28, 2026 07:07

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 54995e7 to 480498c Compare August 28, 2026 07:07

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from 81f2482 to e681b25 Compare August 28, 2026 07:31

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 480498c to 78c5c02 Compare August 28, 2026 07:31

daniel-noland and others added 19 commits August 28, 2026 01:42

test(nat): Drop the absolute floor here too

07e5c9a

Copied from `masquerade::fuzz` before that floor was relaxed; same reasoning. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from e681b25 to f44e856 Compare August 28, 2026 07:43

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 78c5c02 to 313696e Compare August 28, 2026 07:43

daniel-noland force-pushed the pr/daniel-noland/fuzz-nf-probes branch from f44e856 to 7e7ba95 Compare August 28, 2026 09:14

daniel-noland force-pushed the pr/daniel-noland/clock-facade branch from 313696e to 07e5c9a Compare August 28, 2026 09:14

Navigation Menu

Uh oh!

clock - #1756

clock#1756
daniel-noland wants to merge 19 commits into
pr/daniel-noland/fuzz-nf-probesfrom
pr/daniel-noland/clock-facade

daniel-noland commented Aug 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 26, 2026

Draft PR not reviewed

Uh oh!

codecov Bot commented Aug 26, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

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

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 26, 2026

Draft PR not reviewed

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

daniel-noland commented Aug 26, 2026 •

edited

Loading

codecov Bot commented Aug 26, 2026 •

edited

Loading