fix(actor): convert timer delays and elapsed times with exact integer arithmetic - #260
Conversation
DueTimestamp multiplied delay.TotalSeconds by the source frequency in a double and took the ceiling; for about one whole millisecond in twelve (35, 70, 85, 101 ... ms) the product came out a tick above the integer and the ceiling added a tick, so a timer became due a tick late. NextTimerDelayAsync converted back through TimeSpan.FromSeconds(double), which truncates on current runtimes and lost a tick for about one millisecond in twelve (43, 51, 71, 86 ... ms). Both directions now go through decimal: a TimeSpan is a whole number of ticks and the frequency a whole number per second, so the product is exact and division by 10^7 is exact in decimal. The rounding stays upward, so a delay is still a floor. The effect on real timers is at most one source tick; it mattered on the virtual clock, where WaitUntilTimerArmedAsync matched the reported delay against the armed one exactly and failed for those values. Tests cover both Stopwatch frequencies (10^7 and 10^9) over every millisecond up to two seconds and fail on the old code. Two review nits in the same tests: the real-time delay test asserted 150 ms for a 200 ms timer, and the clean-dispose test gated on a 500 ms wall clock join. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview
Tests add Reviewed by Cursor Bugbot for commit d0902e8. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ad its own Making the actor's reported delay exact exposed the same double conversion elsewhere: on .NET Framework TimeSpan.FromSeconds(double) rounds to whole milliseconds, which had hidden that the UDS client counted its 100 ms response window as 999 999 ticks instead of 1 000 000. UdsClientTests.A_Pending_Answer_Still_On_Its_Way_Through_The_Channel_Extends_The_Window then failed on the net48 leg: the timer it waited for was armed 100 ns short of the expected delay. TickMath holds the exact conversions (rounding up for a delay or a remaining time, down for an elapsed time) and replaces the double arithmetic in the actor, the UDS response windows and elapsed time, the ISO-TP functional client and listener, the J1939 node's period and elapsed time, and the CANopen PDO inhibit time. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
dborgards
changed the title
Three tests built the deadline they expect with the same double arithmetic the library used to have: (long)(window.TotalSeconds * Frequency), which is one tick short for a 100 ms window on .NET Framework. With the library now exact, Functional_Collect_On_An_Injected_Clock_Drains_By_That_Clocks_Deadline failed on the net48 leg because its expected deadline was 999 999 ticks and the real one 1 000 000. Same fix in the actor test's Ms helper and the UDS channel double's Ticks. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
1 participant
What does this change?
Fixes finding A1 of the 2026-09-30 deep review: the actor converted a
TimeSpandelay to source ticks and back through adouble, which put about one whole millisecond in twelve a tick off in each direction. Real timers shifted by at most one source tick; the virtual clock the tests use matches the reported delay exactly, so a test with a 35 or 43 ms interval failed deterministically with a message that looked like a protocol error.The conversions now live in one internal helper,
TickMath(decimal arithmetic, exact here; up for a delay or remaining time, down for an elapsed time), and replace the double arithmetic in every package that had its own copy: actor, UDS response windows, ISO-TP functional client and listener, J1939 node, CANopen PDO inhibit time.Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no release!in the title, plus aBREAKING CHANGE:footer explaining the migration)Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds (with-p:CI=true)dotnet test CanKit.Pro.sln -c Releasepasses (net10.0,--no-build; the net48 leg runs in CI only)Also run:
dotnet format --verify-no-changes,dotnet pack+eng/verify-packages.py.Notes
UdsClientTests.A_Pending_Answer_Still_On_Its_Way_Through_The_Channel_Extends_The_Window. On .NET FrameworkTimeSpan.FromSeconds(double)rounds to whole milliseconds, which had hidden that the UDS client counted its 100 ms window as 999 999 ticks; the now-exact actor made it visible. That failure is caused by this branch, so the same conversion is fixed everywhere in this PR rather than in a follow-up.ManualTimeSourcetakes an optional frequency (default unchanged), so the tests can run at 10^7 and 10^9.🤖 Generated with Claude Code