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

daniel-noland · 2026-08-28T02:52:10Z

No description provided.

coderabbitai · 2026-08-28T02:52:22Z

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-28T02:58:44Z

Codecov Report

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

Files with missing lines	Patch %	Lines
dataplane/src/packet_processor/fuzz.rs	87.74%	42 Missing and 33 partials ⚠️
clock/src/virtual_time.rs	92.36%	8 Missing and 3 partials ⚠️
tracectl/src/stamp.rs	93.61%	1 Missing and 2 partials ⚠️
clock/src/lib.rs	90.90%	1 Missing and 1 partial ⚠️
stats/src/rate.rs	95.83%	2 Missing ⚠️
acl/tests/property_predicate.rs	98.03%	1 Missing ⚠️
mgmt/src/tests/mgmt.rs	92.30%	1 Missing ⚠️
net/src/headers/view.rs	95.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

tokio's pausable clock is per runtime and is read through the calling thread's runtime context, so a second runtime is a second timeline and a thread with no runtime silently answers from the wall clock. Measured against tokio 1.53.1: after a one-hour advance an unentered thread reads an hour behind an entered one, and a second paused runtime sits 599.999989s behind the first. The check is scoped to a live `Paused` rather than latched, because tokio's own `DID_PAUSE_CLOCK` latches for the life of the process and a latched check would refuse every innocent reader in any binary that shares one -- which plain `cargo test` and edition-2024's merged doctests both do. That leaves one accepted false positive, under `cargo test` only; the panic message says so. `cargo nextest`, which CI and `just miri` both use, gives each test its own process and has no such window. `--cfg wall_clock` turns the routing off without touching the eleven dev-dependency declarations that switch `virtual` on, so a property written against `advance` is the test under both clocks. Features are additive and cannot be subtracted from the command line, which is why this is a cfg. Both expiry suites take the shared `Paused`, rather than each building its own runtime and its own four-yield advance. The check is scoped to a live `Paused` for the same reason it is not latched: ten independent expiry properties run in parallel threads that share nothing, and refusing a second clock outright would fail all ten while catching nothing. What must not happen is one *property* building two, which is a claim about a single test rather than about the process. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Once anything in the process has paused the virtual clock, tokio routes every read through the calling thread's runtime context -- and a `Drop` running during thread-local teardown may find that context already destroyed, which tokio answers with a panic inside a destructor. That is not catchable: `fatal runtime error: thread local panicked on drop, aborting`, SIGABRT. Reproduced against tokio 1.53.1. Nothing in the workspace does this today. The rule exists because the connection between "an expiry test three crates away" and "CI aborted with no test name" is not one anyone will make from the failure alone. Anchored on `fn drop(&mut self)` rather than `impl Drop for $T`, because opengrep's Rust support matches the latter against any impl block -- verified against a fixture, where the trait name did no work and three of four cases were false positives. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`tracing_subscriber`'s default timer is `SystemTime`, so a line emitted at virtual T+1h carried the real time and correlating a log against an expiry was guesswork exactly when the log is all that is left to read. Under a routed clock the stamp becomes an offset on that clock. It cannot be a wall time -- `clock::now()` is monotonic and has no epoch -- but "how far into the test is this line" is the more useful question in a test log, and it is the only stamp that agrees with what the code under test believes. `clock::checked_now` exists for this: a log timestamp must not panic, and `clock::now()` would on a thread that cannot see the driven clock. Such a line is stamped `T+?off-clock`, which is itself the finding. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The *preserved* disposition at pipeline scale: time passing under a flow does not disturb it, so long as it stays inside its lifetime. Until the workspace read its deadlines through `clock` this could not be written -- the waits were on tokio's clock and the deadlines on `std`'s, so every flow created after the first advance was born already expired. The advance goes between rounds, never during one. `advance` is async and only the driving thread may call it, so a worker could not move the clock anyway; and a clock that jumped mid-pipeline would measure the scheduler rather than the code. The waits are drawn, so the advance composes with the picks as another dimension of one input rather than as something the property decides. Raising the wait cap past `MASQUERADE_ONEWAY_TIMEOUT` fails with `the reply of a delivered flow did not reach the wire: Dropped(Filtered)`, which is what confirms the advance reaches the flow deadlines at all. Also gates the clock's refusal on `NEXTEST_EXECUTION_MODE=process-per-test`. The check is sound only when the paused section owns every thread in the process; under `cargo test` it does not, and this property pausing while another builds a fabric on its main thread failed the packet-processor suite 10 times out of 10. `CLOCK_STRICT=1` reproduces the strict behaviour there, and still fails that suite, so the gate is the only thing softening it. It is a stopgap: the check wants to be scoped to a thread tree, which needs a mechanism this does not have. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The mechanism the runner gate was standing in for. `add_spawn_hook` runs a closure on the parent at spawn time and another on the child before its body, so a spawned thread inherits both the runtime context and the flag saying it is inside a driven-clock world. The check is then scoped to a *tree* rather than to a process, which is the granularity it always wanted: - an unrelated test sharing the process is simply not in the tree, so the false positive is gone by construction -- the packet-processor suite goes from 0/10 to 10/10 under `cargo test`, with no `NEXTEST_EXECUTION_MODE` gate; - a worker that would have forgotten to enter the runtime is entered for it, so forgetting stops being possible rather than merely detected; - what remains for the panic is the case inheritance cannot reach -- a thread created outside `std::thread`, by DPDK's EAL or a C library -- which is exactly where a silent wall-clock reading would be least expected. Registration is per thread, not per process. `SPAWN_HOOKS` is itself a thread-local list that children inherit, and a `Once` looked right and was not: the first test to build a `Paused` consumed it, and every later test's threads inherited nothing. The handle is leaked per spawn for the same class of reason -- caching one for the process sent every later test's threads to the first test's clock. `build.rs` probes for the feature rather than checking the release channel, so a toolchain that predates it degrades to checking only the thread holding the clock instead of failing to build. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`FlowTable::insert` spawns a timer task holding an `Arc` to the table, and bolero's `for_each` takes a *synchronous* closure -- so inside `#[tokio::test]` the body awaits nothing for the whole run, the runtime is never polled, and not one of those tasks ever executes. They accumulate, each pinning an entire per-case flow table. A thirty-minute fuzz run died on it, with seven `oom-*` artifacts against libFuzzer's 2GB default. Instrumenting `num_alive_tasks` against RSS shows the mechanism directly: four tasks and ~600KB per case, rising linearly to 2,572 tasks and 1.5GB over 700 cases with no plateau. Twelve of the forty-three properties were affected; after the change each reaches a steady state instead (`every_shape...` 2,572 -> 522 tasks, 1,491MB -> 359MB flat). Three of the twelve carry the largest corpora of any target in the tree, so they are the ones a long fuzzing run leans on hardest. The leak is the lesser half. A timer that never runs is a flow that never expires, so anything these properties appeared to say about a flow outliving its deadline was saying nothing. `Runtime::enter` is not enough -- it supplies a context but drives nothing, and `block_on` cannot be called from within one, which is why these are now plain `#[test]`. `settled` takes a `FnOnce()` rather than wrapping the closure `for_each` is handed: wrapping the closure reads better and does not compile, because the `|(a, b)|` destructuring makes the input type ambiguous and inference settles on an unsized tuple. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two faults, both of which had to go before `just fuzz` could reach this crate: the vacuity guard ran during cargo-bolero's target-selection pass and refused the selection, and the runtime the properties entered was never driven, so the flow timers spawned under it accumulated until the process ran out of memory. Neither had been seen because neither is reachable from `cargo test`, and no corpus for a nat target has ever existed -- the crate has thirteen properties that have never had a fuzzing engine behind them. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…named An empty `sanitize` does not mean "no sanitizer": cargo-bolero substitutes its own default of `address`. The check therefore passed the one combination it exists to refuse -- rust instrumented against an uninstrumented sysroot -- and refused `NONE`, which is the setting that matches such a sysroot. The empty default is announced rather than refused, so that the tree's existing invocations keep working while a run that reports nothing stops reading as a run that found nothing. EOF2 git log --oneline -3 Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A `check!()` written inside a closure is named `...::{{closure}}`, which matches no test, so none of these eleven could be selected by `cargo bolero` however they were spelled on the command line. The paused clock now wraps each case instead of the whole property, which is also what keeps the spawned timers from piling up. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`bolero-libfuzzer` calls `bolero_engine::any::run` but declares only the `cache` feature, so `any` arrives solely when something else in the graph pulls in `bolero/std`. A workspace build unifies that in and hides it; building one package alone does not, which is why these three could not be fuzzed at all. Fixed here rather than in the fork so the tree builds today. The declaration in `bolero-libfuzzer` is still wrong and is the place it belongs. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The shards existed to spread one property over many test processes, but all twenty-four called a helper that held the `check!()`, so bolero registered the helper's name twenty-four times and none of the shards could be selected. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Last of the properties that `cargo bolero list` reported and could not run. Each was a helper holding the `check!()` for several tests, so one target was registered under the helper's name and no test could be selected by it. With these the workspace has no unaddressable targets left: 596 of 596. `config`'s also had to absorb `census()`, which existed only because `check!()` expands to a bare `return` and so cannot sit in a function with a return type. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two faults, both invisible because nothing linked the two halves together. `cargo-bolero` puts a RUSTFLAGS value in the environment, and cargo ignores `.cargo/config.toml`'s `rustflags` whenever that variable is set rather than merging the two, so every fuzz build lost `--cfg=tokio_unstable` and both `--check-cfg` registrations: 43 `unexpected cfg` warnings a build, now none. And sancov instruments every binary in the package while only bolero-linked ones carry a runtime defining the symbols, so a package with a non-bolero test binary could not link at all unless a sanitizer happened to supply them. That is why the default appeared to work, why `-p` looked like a fix, and why it surfaced only when `sanitize=NONE` was asked for. Every package in the workspace can now be fuzzed without a sanitizer, which is about 3.5x quicker. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Fuzzing was modelled as a profile, but a profile is a choice and this is a set: coverage and fuzzing are both bundles of flags added to rustc, clang and the link, they compose with each other and with the sanitizers, and nothing about them is an optimisation level. `instrumentations` now reads exactly like `sanitizers` -- split, sorted, mapped over -- and takes `none`, `coverage`, `fuzz`, or both. The cargo profile stays, because cargo profiles are the only lever for `debug-assertions` and `overflow-checks`, but it is named for what it is rather than for its first consumer: coverage wants the same bargain. Sorting the sets keeps a marker file from depending on the order somebody typed a comma-separated list, which is the shape of bug the sanitize guard already had. Containers refuse to build under instrumentation. The tag was never the problem: instrumentation appears nowhere in `version`, so such an image silently takes a clean image's tag -- and there is no reason to ship a measuring device. Sanitizer images are unaffected and still build; those are worth deploying. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three faults, one per layer, none of which existed without the others. rdma-core's own build-time executables are instrumented with no runtime to call, so they are allowed to leave the symbols unresolved -- they are throwaway, we consume the static archives, and the objects that do reach our binary take their sancov symbols from the libFuzzer the fuzz target links. That is also why the link flags here are empty: naming a runtime would drag the whole of libFuzzer, libstdc++ and libm into every executable a C dependency happens to produce. DPDK failed differently. sancov emits a module constructor per translation unit and reaches it from `.init_array`; thin LTO does not count that as a live reference, discards the section and leaves the relocation dangling. So fuzz instrumentation gives up LTO, and only fuzz instrumentation does. The overlay could not see `instrumentations` at all; it does now. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`LIVE` is a count because nothing refuses a second `Paused` -- deliberately, and the type says why. `IN_WORLD` was a flag, so one thread holding two cleared it on the inner one's drop and `armed()` stayed false for the rest of the outer one's life. The guard silently stops guarding, which is the one failure this module exists to prevent. The module documentation also still described the draft that refused a second `Paused`, contradicting the type's own account two hundred lines below it. The type is right; the module doc now says the same thing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`fix: Give every remaining shared fuzz driver a target name per test` took the last use of `RefUnwindSafe` out of both integration tests and left the imports behind. `just lint` runs clippy over all targets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…ere too The per-process `LazyLock` holding the tokio runtime goes through the facade. Everything holding the *paused clock* keeps `std::sync` and says why. A facade `LazyLock` there stops the virtual clock working -- the nat properties went from four seconds to thirty-two, which is them really sleeping. These are the harness's own bookkeeping, one section per process, outside anything a model checker should schedule; `clock` keeps `std::sync` for the same reason and has `concurrency` as a dev-dependency deliberately, to keep the edge out of the real graph. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from fa0e8f2 to b164a0f Compare August 28, 2026 03:05

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from 5843caa to 6f106ac Compare August 28, 2026 03:05

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from b164a0f to 3cd5687 Compare August 28, 2026 03:34

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from 6f106ac to 320c6aa Compare August 28, 2026 03:34

daniel-noland changed the title ~~feat(clock): give a test one clock it drives, and fix what that exposed~~ test-clock Aug 28, 2026

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from 320c6aa to d3c348d Compare August 28, 2026 04:22

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch 2 times, most recently from 1ec0be0 to de8e45f Compare August 28, 2026 05:11

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch 2 times, most recently from be78275 to 4969a2f Compare August 28, 2026 05:28

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from de8e45f to bea6549 Compare August 28, 2026 05:32

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from 4969a2f to 26e6361 Compare August 28, 2026 05:47

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from bea6549 to f850a96 Compare August 28, 2026 05:48

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from 26e6361 to 3ee9131 Compare August 28, 2026 06:18

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from f850a96 to 9398527 Compare August 28, 2026 06:21

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from 3ee9131 to fcd1d11 Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 9398527 to d497a39 Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from fcd1d11 to b26928b Compare August 28, 2026 07:08

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from d497a39 to 3e43aea Compare August 28, 2026 07:08

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from b26928b to 84d66cc Compare August 28, 2026 07:31

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 3e43aea to 610dcf1 Compare August 28, 2026 07:31

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from 84d66cc to a842024 Compare August 28, 2026 07:43

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 610dcf1 to 698f663 Compare August 28, 2026 07:43

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from a842024 to c50fd96 Compare August 28, 2026 09:14

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 698f663 to 26b967b Compare August 28, 2026 09:14

daniel-noland and others added 3 commits August 28, 2026 11:14

daniel-noland and others added 15 commits August 28, 2026 11:14

daniel-noland force-pushed the pr/daniel-noland/config-algebra branch from c50fd96 to ccbfc9c Compare August 28, 2026 17:16

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 26b967b to 8968292 Compare August 28, 2026 17:16

Navigation Menu

Uh oh!

test-clock - #1773

test-clock#1773
daniel-noland wants to merge 18 commits into
pr/daniel-noland/config-algebrafrom
pr/daniel-noland/driven-clock

daniel-noland commented Aug 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 28, 2026

Draft PR not reviewed

Uh oh!

codecov Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

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

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 28, 2026

Draft PR not reviewed

Uh oh!

codecov Bot commented Aug 28, 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 28, 2026 •

edited

Loading

codecov Bot commented Aug 28, 2026 •

edited

Loading