test(uds): run the P2/P2* and suppressed-window tests on a virtual clock - #189
Merged
Merged
Conversation
UdsClientImpl can now take an internal ProtocolActor, through a new internal UdsClient.Create(channel, clock, ...) overload. The timings move onto it: - Timestamps: the P2 and P2* budget, the suppressed-response windows and the stale-PDU discard now read Now() and Ticks() on the actor's time source, instead of Stopwatch. - Waits: the P2/P2* receive timeout, the suppressed-window wait and the busy-repeat delay are timers on that actor (CancelAfter, DelayAsync). The timer fires on the actor loop, and the cancellation it triggers is handed to the thread pool rather than run there. Stamps alone are not enough. An earlier attempt on this branch put only the stamps on the clock and kept real timeout sources for the waits. The order of a late answer and the next transmit was then decided on the wall clock, and three converted tests stopped catching the defects they exist for (recorded on #171). With the waits on the clock as well, a window ends only when the test moves the clock. The in-progress recheck stays a real timer. It polls the channel's state rather than enforcing a protocol deadline. With no clock injected, which is always the case through the public API, both helpers fall back to real timers and Stopwatch. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
Five UDS tests move onto a ClockPair: the client's channel runs on a VirtualClock actor, and the ECU stays on the wall clock. Each test is a lockstep script: 1. The ECU holds each answer on a gate. 2. The test moves the clock to the instant the answer belongs to. 3. The test releases the gate and waits until the client's actor has taken the frame in. 4. The test waits for the exact interval the client arms, which distinguishes the correct client from each mutant. Converted: - A_Late_Negative_Response_To_A_Suppressed_Send_Is_Not_The_Next_Requests - Suppressed_Send_Windows_Are_Kept_Per_Service - A_Cancelled_Wait_Keeps_The_Rest_Of_The_Window - A_Pending_Answer_Consumed_As_Another_Requests_Stray_Still_Extends_Its_Window (red on macOS for #188) - P2_Ends_With_The_First_Frame_Of_A_MultiFrame_Response (red on macOS for #185 and #188). N_Cr now runs on the virtual clock, so real CF gaps can no longer expire it. P2 is made to expire mid-transfer by moving the clock once the First Frame is in. Mutation checks, each turning exactly its own test red: - no suppressed-window wait: all four window tests - a cancelled wait forgets its window: the cancelled-wait test - no routing of a stray 0x78: the pending-answer test - one window shared by all services: the per-service test - P2 measured against the last frame: the P2 test Supporting changes: - SimulatedUdsEcu gains a Delay hook. Its paced answers default to Task.Delay. - FrameConsumptionCountingBusService now also counts frames drained with TryRead, which is how the ISO-TP channel's pump reads. A frame counts as consumed once the reader asks for the next one. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
On the actor's clock, the source a timer cancels holds nothing that needs disposing. Leaving it undisposed means a cancellation the timer has already handed to the thread pool can no longer find it disposed, so the exception that case needed caught is gone. The busy-repeat delay now waits on the clock with a timer and a completion source, which is the helper UdsFunctionalClient already uses. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
Schedule(delay) measures the delay from a clock reading it takes itself. A caller whose deadline is already fixed has to turn it into a delay from its own, earlier reading. If the clock moves in between, the timer lands late by that whole move. On a VirtualClock a test advances in steps, so the timer can end up past the last advance and never fire. The internal ScheduleAt(dueTimestamp, callback) arms the instant as it is. Schedule and ScheduleAt share one insertion path. The public API is unchanged. The new test was mutation-checked: making ScheduleAt land late by the clock's move turns it red. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
On the injected clock, the P2/P2* receive timeout and the suppressed-window slices were armed as "remaining from now". The remaining time was computed on a thread-pool continuation, and the actor measured it again from its own reading. When a test moved the clock between the two readings, the timer was armed that much too late and never fired. This happened under load in A_Cancelled_Wait_Keeps_The_Rest_Of_The_Window: 2 hangs in 40 runs alongside a concurrent test process. Both timeouts are now armed at the deadline itself: budgetStart + P2 for the receive, and the window's own end for a slice. They go through ProtocolActor.ScheduleAt. After the change the same stress passed 60/60. The production path without an injected clock is unchanged: a real CancellationTokenSource timer for the remaining time. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
…virtual clock Converts the remaining fourteen P2/P2*/suppressed-window tests in UdsClientTests.cs from wall-clock pacing (Thread.Sleep/Task.Delay/Stopwatch bounds) onto the ClockPair pattern introduced by a6e699a, or onto a bare VirtualClock actor for the two that use a StarvedReaderBusService double instead of SimulatedUdsEcu. Two small helpers were added: OrderedGates (an EcuSteps sibling that gates SimulatedUdsEcu.Delay calls by order rather than by duration, for scenarios that reuse the same delay length more than once) and a non-generic Within(Task) overload. Converted, with each one's mutation and the observed result (both original and converted red on it, verified before every revert; `git diff src/` was empty before this commit): - P2_Is_Not_Extended_By_A_MultiFrame_Transfer_For_Another_Service Mutation: ResponseBeganInTime (UdsClientImpl.cs) drops the response-SID check, so any in-progress reception -- not just this request's own -- extends the wait. Original: BeLessThan(1500ms) failed at ~2.6s. Converted: initially passed (the dropped real-time bound had made the exception-type-only assertion insufficient, since the mutant also eventually throws UdsTimeoutException once the ECU's STmin-paced, real-time transfer finishes). A later commit on this branch replaces the wall-clock bound this was first fixed with by a frozen transfer the mutant can only escape by cancellation. - A_Pending_Answer_To_A_Suppressed_Send_Extends_Its_Window_By_P2Star - A_Queued_Pending_Answer_Still_Extends_A_Window_That_Has_Run_Out Mutation: ExtendOnPending's own-service branch (UdsClientImpl.cs) never extends (`return false` unconditionally). Both: original failed on the request count / the elapsed-time floor; converted failed with WaitUntilTimerArmedAsync unable to find the extended interval ("the earliest is none"). - A_Queued_Pending_Answer_From_After_The_Windows_End_Does_Not_Revive_It - A_Pending_Answer_From_After_A_Windows_End_Does_Not_Revive_It - A_Pending_Answer_From_After_A_Windows_End_Heard_In_A_Wait_Out_Does_Not_Revive_It Mutation: ExtendOnPending's own-service guard (UdsClientImpl.cs) drops the "arrived after the window's own end" check, so a late 0x78 always revives it. Original: BeLessThan(1s) failed at ~1.4-1.8s. Converted: hung past its own clock choreography and was cancelled by the caller's token (OperationCanceledException). - A_Pending_Answer_For_Another_Service_Heard_During_A_Wait_Extends_That_Services_Window Mutation: ExtendOnPending's another-service branch (UdsClientImpl.cs) is a no-op. Original failed with a UdsNegativeResponseException (the stale negative was taken as the reset request's own answer); converted failed on WaitUntilTimerArmedAsync. - A_Pending_Answer_Still_On_Its_Way_Through_The_Channel_Extends_The_Window Mutation: the zero-remaining branch of WaitOutSuppressedResponseWindowAsync (UdsClientImpl.cs) skips SettleAsync() before deciding the inbox is empty. Original: BeGreaterThanOrEqualTo(600ms) failed at ~280ms. Converted: threw UdsTimeoutException too early (the window closed before WaitUntilTimerArmedAsync ever saw the extended interval, so the second wait step failed). - A_Suppressed_Send_The_Channel_Refuses_Leaves_No_Window Mutation: SendWithoutResponseAsync's RefusedBeforeTransmission catch (UdsClientImpl.cs) no longer calls SuppressedResponseWindows.Restore, leaving the provisional window in place. Original: BeLessThan(1s) failed at ~2s. Converted: hung waiting out the leaked window and was cancelled by Within's own timeout. - A_Suppressed_Send_The_Channel_Refuses_Leaves_An_Earlier_Window_As_It_Was Mutation: SuppressedResponseWindows.Restore ignores `had` and always removes the window. Original: BeGreaterThanOrEqualTo(1900ms) failed at ~54ms. Converted: WaitUntilTimerArmedAsync never saw the 2s window (the earlier window was wiped). Each of the two Restore mutations was checked against both tests to confirm it broke only the one it targets. - A_Pending_Answer_Still_On_Its_Way_Is_Routed_By_An_Aborted_Requests_Discard Mutation: RouteStrayPending (UdsClientImpl.cs) never extends any window. Original: BeGreaterThanOrEqualTo(600ms) failed at ~199ms. Converted: WaitUntilTimerArmedAsync never saw the extension. (A first attempt mutated DiscardStalePdusAsync's own SettleAsync call, which the original test caught but the converted one did not -- DiscardPendingPdus's own internal pump made that call redundant on the virtual clock. RouteStrayPending is the mutation that isolates the property the test actually names.) - A_Stale_Pending_Answer_Queued_Before_A_Suppressed_Send_Does_Not_Extend_Its_Window Mutation: SendWithoutResponseAsync's pre-send discard call (UdsClientImpl.cs) is skipped. Original: BeLessThan(1s) failed at ~2s. Converted: WaitUntilTimerArmedAsync never saw the un-extended (100 ms) interval it expects. - Client_Times_Out_With_P2Star_When_Ecu_Sends_Only_ResponsePending - ResponsePending_Restarts_P2Star_And_Returns_Final_Response Mutation: the NRC-0x78 branch of ExchangeOnceAsync (UdsClientImpl.cs) no longer resets budgetStart/notBefore/timeout/timerKind, so the client never leaves its initial P2 budget. Original: Timer assertion / P2 timeout instead of a positive result. Converted: WaitUntilTimerArmedAsync never saw the restarted P2* interval. Stress-testing this change surfaced a hang in A_Cancelled_Wait_Keeps_The_Rest_Of_The_Window and A_Pending_Answer_Consumed_As_Another_Requests_Stray_Still_Extends_Its_Window. Both tests were converted earlier on this branch, so the defect is this branch's own: the injected clock's timeouts were armed as a delay from a fresh reading and could land past the test's last advance. It is fixed on this branch by arming them at their deadlines (ProtocolActor.ScheduleAt). Left unconverted, with reasons: - TimedOut_Request_Does_Not_Poison_Next_Same_Service_Transaction, ResponsePending_Loop_Aborts_When_Exceeding_MaxResponsePendingCount: excluded by the task (not P2/P2*/window timing, or already deliberately real-clock). Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
- P2_Is_Not_Extended_By_A_MultiFrame_Transfer_For_Another_Service no
longer bounds the client with a real Stopwatch.
- The ECU sends only the First Frame of the unrelated transfer, so the
reception stays in progress. The client's N_Cr is on the virtual
clock and cannot end it.
- P2 then runs out on the clock, and a correct client times out.
- A client fooled into waiting for the transfer never returns. The
test's own token then ends it with a cancellation rather than a
timeout.
- Mutation checked: with ResponseBeganInTime's response-SID check
dropped, the test is red on exactly that cancellation.
- EcuSteps now honours the ECU's cancellation token. Under a mutation
that stops a 0x78 from restarting P2*, a failed test disposed its
ECU while the ECU loop was parked on a gate nobody opens. The
disposal hung, and so did the whole test run. The same mutation now
turns its three tests red in 32 s.
- New A_Busy_Repeat_Waits_Its_Delay_On_The_Clients_Clock and
A_Busy_Repeat_Delay_On_The_Clients_Clock_Ends_On_Cancellation cover
the busy-repeat delay on the injected clock.
- A delay that ignores the clock turns both red.
- A delay deaf to the token turns the cancellation test red.
Refs #171
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
PR SummaryMedium Risk Overview Actor timers: Tests: Adds Reviewed by Cursor Bugbot for commit ae072fa. 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! |
Codecov on #189 reported three partial lines: - The internal UdsClient.Create(channel, clock, ...) overload was never called with a null channel or with omitted options. - DelayAsync's wall-clock branch was never reached, because no existing test uses a non-zero BusyRepeatRequestDelay without an injected clock. Three small tests cover those paths. The wall-clock one asserts only that the repeated request succeeds. How long the delay lasts is what A_Busy_Repeat_Waits_Its_Delay_On_The_Clients_Clock proves. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
This is the next step of #171. The UDS P2/P2* and suppressed-window tests were the ones turning unrelated PRs red on macOS:
P2_Ends_With_The_First_Frame…on #185 and #188,A_Pending_Answer_Consumed…on #188. They now run on aVirtualClockand no longer depend on how the host schedules.Product code (internal only, no public API change):
UdsClientImpltakes an optional injectedProtocolActor, through a new internalUdsClient.Create(channel, clock, …)overload. When a clock is injected, both the timestamps and the waits run on it:ProtocolActor.ScheduleAt. An earlier revision armed a delay computed from a fresh clock reading. When a test moved the clock in between, the timer landed past the last advance and never fired: 2 hangs in 40 runs under load. Arming at the deadline removed them: 60/60 afterwards.CancellationTokenSourcetimers andStopwatch.Why stamps alone were not enough. An earlier attempt, dropped before #186, put only the timestamps on the clock. The window wait was still a real timer, so whether a late answer or the next transmit came first was decided on the wall clock. Three converted tests then stopped catching the defects they exist for. Details are on #171.
Tests: 21 UDS tests converted or added. Each is a lockstep script:
Mutation checks: every converted test was checked. The mutation, and the fact that it turns the test red, are recorded per test in the commit messages. A re-check against the final product code, over the whole
UdsClientTestsclass:Harness changes:
SimulatedUdsEcugets aDelayhook. The default is stillTask.Delay.FrameConsumptionCountingBusServicealso counts frames drained throughTryRead, which is how the ISO-TP channel's pump reads.EcuStepshonours cancellation, so a failing test cannot hang the run while it disposes.Found along the way, not caused by this branch:
UdsExpiredDeadlineTests.Q_A_Stray_Pending_From_Before_The_Handoff…fails occasionally under load. It failed 1 of 8 runs onmainas well. I'll record it on #171.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; the net48 test leg also compiles)dotnet test CanKit.Pro.sln -c Releasepasses (net10.0, locally; theUdsClientTestsclass also 8/8 under concurrent load)FR-RAW-031,ADR-7), if any (Replace wall-clock category-2 test sleeps (macOS flake risk) #171)🤖 Generated with Claude Code
https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
Generated by Claude Code