fix(test): size the periodic rate bound by its error term instead of picking a statistic

[dborgards]

What does this change?

J1939NodeTests.StartPeriodicSend_SingleFrame_FiresAtConfiguredPeriod asserted a median gap of at least 0.9 × period. That is the third statistic this test has used and, like the two before it, it was chosen by intuition rather than sized against anything. It fails runs where the scheduler was correct.

This is the debt from #101 — the median is what #101 merged — and it comes before new work.

Why the median is wrong, and what replaces it

Every emission lands on its own grid slot, late by however long the host stalled: t(i) = slot(i)·P + late(i). The gaps telescope, so

mean gap = (slots spanned / gaps) · P  +  (late(last) − late(first)) / gaps

The first term is at least the period, because slots are distinct. The second term is the entire problem, and it is bounded by the number of gaps and nothing else. That disposes of all three attempts at once:

statistic	error behaviour
median band [0.7, 1.6]×P (original)	upper bound broken by coalescing to 2 × period, which the scheduler documents
per-gap slot floor	broken by a late tick legitimately shortening the distance to the next anchor
plain mean over 9 gaps	divides a cold start by nine — a 239 ms late first tick lands at 106.8 ms
median (currently on main)	no error term to shrink at all; it tracks the shape of the jitter rather than its size, so it cannot be sized

So: 21 samples, the first gap discarded as the only one measured from a cold schedule, and the mean of the remaining twenty. The sample count is what makes the assertion sound, which is why it is commented as not being a free parameter. The collection budget moves off ShortTimeout for the same reason — 21 samples at 120 ms need 2.5 s even when nothing is dropped.

The evidence that it fixes the observed failures

This is deterministic rather than a re-run, because it replays the gap sequences #92 actually recorded from macOS, against both statistics (bound 108 ms):

recorded run	gaps	plain mean (true rate)	median — on main	trimmed mean — this branch
15e2094	83, 132, 73, 173, 67, 188, 62, 105, 185	118.7 ✅	105.0 ❌	123.1 ✅
8db8b5c	182, 107, 70, 117, 131, 108, 181, 84, 95	119.4 ✅	107.7 ❌	111.6 ✅

In both, the mean says the scheduler held the requested rate to within about 1 %. The median rejected them anyway — the first because nine alternating gaps have one more short than long, the second by 0.27 ms. (The medians are as CI reported them; my own arithmetic on the rounded integers above gives 105.0 and 108.0.)

Mutation results, including one that corrects the record

Run against this branch's test, each mutation built and run on its own:

mutation	result
period halved	caught — mean 60 ms vs bound 108
rate 15 % fast	caught — mean 102 ms vs bound 108
skipped ticks released back to back	passed — the mutation is a no-op here

The third needs correcting rather than reporting, because I have claimed on #92 that this test catches it. It does not, and the reason is not the test: OnTick drops any tick whose predecessor is still in flight, so a tick fired immediately after an emission produces no frame at all. The mutation cannot express itself against the current scheduler, under load or otherwise — I ran it 3/3 under an 8× CPU overload and it passed there too.

The assertion's power is real, and separating the two took one more mutation: with the burst and the in-flight guard removed, gaps come out 116, 0, 120, 0, …, mean 57 ms, and the test fails. So the bound detects bursting; the single-line burst mutation simply never reaches it.

What my local load test does not show

Twelve runs of this branch's test under an 8× CPU overload: 12 passed. Twelve runs of main's median version under the same load: also 12 passed. This container does not reproduce the macOS failure mode, so those runs are not evidence that the change fixes anything — the recorded-gap table above is. Stating it because a reader trying to verify locally will get the same non-result.

Type of change

• feat — new behaviour (minor release)
• fix / perf — bug or performance fix (patch release)
• docs / test / refactor / chore / ci — no release
• Breaking change

Test-only: fix(test):, so no release. No production code is touched.

Checklist

• dotnet build CanKit.Pro.sln -c Release -p:CI=true succeeds — 0 warnings, 0 errors
• dotnet test CanKit.Pro.sln -c Release --no-build --framework net10.0 passes — 614/614
• dotnet format CanKit.Pro.sln --verify-no-changes clean
• dotnet pack into a fresh directory + python3 eng/verify-packages.py — 9 packages checked
• Public API changes are documented with XML comments — no API change
• New behaviour is covered by a test — this is the test; mutation results above
• The requirement or ADR this relates to is referenced — Three wall-clock-dependent tests fail intermittently across all three CI legs #92, test(j1939): stop asserting timing these two tests cannot measure #101

macos-latest may be red as on any pull request; #92 counts the population, and this branch touches one of its members without claiming to fix the rest.

🤖 Generated with Claude Code

https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj

---

Generated by Claude Code

[cursor]

PR Summary

Low Risk
Test-only change to J1939 periodic-send timing assertions; production behavior is unchanged.

Overview
StartPeriodicSend_SingleFrame_FiresAtConfiguredPeriod no longer uses a median inter-emission gap (≥ 0.9× the 120 ms period). It now drops the first gap as cold-start warm-up and asserts the mean of the remaining gaps meets the same floor, with comments tying sample count to the telescoping lateness error term.

requiredSamples rises from 10 to 22 so twenty post-trim gaps shrink endpoint error enough for the 10% bound to stay meaningful. The collection deadline switches from ShortTimeout to 4 × period × requiredSamples so slow or coalescing CI runners can finish gathering emissions without timing out early.

Test-only; no scheduler or product code changes.

^{Reviewed by Cursor Bugbot for commit dc47793. Bugbot is set up for automated code reviews on this repo. Configure here.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36195635ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dborgards]

macOS is red, and the interesting part is which test passed

One comment for the macOS situation on this branch; I will not repeat it per head.

What failed on dc47793 (job), 613/614:

Expected a <CanKit.Pro.Uds.UdsTimeoutException> to be thrown, but no exception was thrown.
  at UdsClientTests.TimedOut_Request_Does_Not_Poison_Next_Same_Service_Transaction (UdsClientTests.cs:425)

Why it is not this branch's. The diff is one test method in J1939NodeTests.cs. It touches no production code and no other test. This failure is in CanKit.Pro.Uds against a deadline in ProtocolActor's scheduler — no shared path, and it is #92's step-1 candidate product bug, now at its fifth recorded occurrence across five commits and two operating systems.

And the test this pull request exists to fix passed on that same run. That is the first macOS evidence for the change rather than a replay of recorded gaps: StartPeriodicSend_SingleFrame_FiresAtConfiguredPeriod was red on three consecutive heads of #104 with the median statistic, and it is green here on the runner that produced those failures. One green run is not proof — the test was intermittent before, and #92 exists because a single observation on this suite means little — but it is the right sign, and it is the only leg that could have produced it.

Nothing to port. No fix exists for the UDS case: #92 step 1 is an investigation that has not been started, and its whole point is that it is not yet known whether the client or the test is at fault. Guessing at it inside a pull request about a J1939 timing assertion would be exactly the widening the working agreement forbids.

I have re-run the failed job once. If it is red again, that is the same finding and still not this branch's.

---

Generated by Claude Code