test(canopen): wait on observables instead of sleeping - #188
Conversation
…nary WriteUnsigned and Add already serialize on the private _writeGate mutex, but nothing let a test observe that a concurrent writer had genuinely reached it. Three CanOpenCommunicationProfileTests relied on a fixed Thread.Sleep instead, assuming a concurrent "hammer" writer had reached the gate within the sleep. Internal WriteGateWaiters (incremented before lock (_writeGate), decremented once acquired) gives tests a real signal to poll instead. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
…eeps with observables Part of the #114/#171 delay-and-sleep audit ("CANopen NMT Start" and "CANopen session, wire, and pump" tables). None of these sleeps waited on a clock the node's actor had armed: they assumed a received frame had been dequeued and ApplyNmtTransition had run, an SDO session had been installed, an upload-init was on the wire, or the RPDO event pump had drained. - ICanOpenNode.State already round-trips through the node's actor, so a bounded poll on it (WaitUntilOperationalAsync) is a real barrier for "the NMT Start this test just sent has been applied" — no product change needed. Used in CanOpenDynamicMappingTests and the four Tpdo_*/Nmt_Broadcast tests in CanOpenNodeIntegrationTests that used to sleep 50-100 ms after sending Start. - A local BootupWatch (the pattern CanOpenCommunicationProfileTests and CanOpenDeviceDescriptionTests already use) replaces the sleeps that waited to consume a node's opening boot-up or a reset's boot-up (Nmt_ResetNode_EmitsBootup, Nmt_ResetCommunication_Emits..., and the heartbeat-consumer arming sleep in Heartbeat_Consumer_FiresTimeout...). - Sdo_ServerSupersede_EmitsWireAbort_ForPriorTransfer, Sdo_ClientResponseWithShortDlc_IsAcceptedAndCompletes, and Sdo_ClientSegmentedUploadResponse_OverMaxTransferBytes_AbortsOutOfMemory now wait for the server's session-install ack / the client's own init frame to be observed on the wire (FrameObserved) instead of guessing when a request "must" have been transmitted. - Sdo_Segmented_Download_Wrong_Toggle_Aborts waits for the segmented download's init-ack (scs 0x60) instead of guessing 100 ms. - Tpdo_Emission_UnderConcurrentOdWrites_NeverTears flushes the consumer's RPDO event pump with one more deterministic TPDO and waits for its own delivery (the pump is a single FIFO reader) instead of a fixed 200 ms, so a late torn payload can no longer slip past the assertions unsampled. Left unconverted, with reasons: - Every "nothing else was transmitted" wall-clock window whose effect the actor round trip cannot observe (Tpdo_ChangeOfState_DoesNotEcho_ On_RpdoUnpack's echo-quiet window, Overlapping_Emcys_..., A_Guarding_ Reply_..., the producer-tick windows) is left as-is per the audit's own guidance: shortening a green "did not happen" window only weakens it. - Nmt_ResetCommunication_EmitsBootup_And_Settles_In_PreOperational's trailing state check needed no barrier at all: PerformNmtReset sets _state = PreOperational before it emits the boot-up frame the test already awaits, on the same actor, so the state is already correct by the time bootups.Second resolves. Mutation-tested: ApplyNmtTransition (state never reaches Operational), the two EmitHeartbeat(0x00) boot-up call sites (construction and reset), AbortSupersededServerSession (stays silent), and the download- segment toggle check each caught their guarded test(s) when broken and were restored; src/ carries no leftover mutation. Ran the full CANopen + ApiApproval filter (424 tests, all green) and the three converted files' tests 10x in a row (116 tests each run, all green). Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
…aiters signal Part of the #114/#171 delay-and-sleep audit's object-dictionary write-gate sub-table. A_Redeclaration_Waits_For_The_Write_In_Flight_On_The_Entry, A_Typed_Write_Resolves_Its_Type_Under_The_Write_Gate, and A_Direct_Write_Cannot_Land_Inside_A_ConfigureTpdo_Transaction each held the ObjectDictionary's write gate from inside a WriteValidator callback and slept a fixed 100-200 ms, assuming a concurrent writer had reached the same gate by then. They now spin (SpinUntilAtTheWriteGate) on the ObjectDictionary.WriteGateWaiters seam added in the prior commit — real evidence the other writer's own lock (_writeGate) attempt is blocked on this one, not a guess at how long reaching it takes. Left unconverted: A_Save_During_An_Nmt_Reset_Stores_All_Restored_Values_ Not_A_Mix's Thread.Sleep(300) is not the same shape. It runs inside an NMT reset's Transaction(...), which already holds _writeGate for the whole restore; the concurrent save's WriteRaw call is provably blocked on that same lock the instant it is issued, regardless of the sleep's length. The sleep is margin for the "if the save were wrongly not held back it would have completed by now" direction (matching the audit's "green"), not a guess about when the save reaches the gate, so it is left as-is. Mutation-tested: making Add's write-gate lock private (no longer the shared gate) broke A_Redeclaration_Waits_For_The_Write_In_Flight_On_ The_Entry as expected, then reverted. Ran CanOpenCommunicationProfileTests (67 tests) and the full CANopen + ApiApproval filter (424 tests) green, and the three converted files' tests 10x in a row (116 tests/run) green. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
PR SummaryLow Risk Overview Production (test seam only): Tests: New helpers synchronize on real conditions—poll No public API or runtime behavior change beyond the internal waiter count used only from tests. Reviewed by Cursor Bugbot for commit 366c488. 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0bcb53a30
ℹ️ 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".
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The handler added to wait for the flush TPDO matched any RPDO from the producer. An event still queued from the 2000 emissions before it is raised to every handler subscribed when it runs, so it could complete the flush early and let the counts be sampled while later deliveries, a torn one included, were still pending (Codex on #188). The flush TPDO now carries a payload nothing else emits, and the tear check ignores it. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
CodeQL on #188 flagged the foreach in both copies of WaitUntilOperationalAsync as a missed All(...). Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00768e7948
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The RPDO flush added on this branch cannot order anything. EmitTpdo sends each TPDO through its own Task.Run, so the flush TPDO can overtake earlier ones on the wire (Codex on #188). Counting deliveries does not work either: every WriteRaw in the writer loop also emits a change-of-state TPDO, and a probe saw 16,000 to 35,000 of them arrive for the loop's 2000 triggers. With no barrier available, the 200 ms window from main is restored, with the reason recorded next to it. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
|
00768e7 is no longer the head. ff50e60 has a fresh CI run of its own, so I'm not re-running the old one. Generated by Claude Code |
|
The failing test was Why it isn't this branch's:
No fix exists yet. It is one of the UDS P2 rows on #171 that still needs a time seam that holds. Re-run: none was needed. The base merge 366c488 started fresh CI, and all legs are green on that head, macOS included (run 36351351035). Generated by Claude Code |
3 participants
What does this change?
This is the next step of #171 and covers the CANopen rows of the delay/sleep audit. The sleeps it replaces did not wait for a timer. They guessed how long something else would take to happen. The tests now wait for that event directly:
ICanOpenNode.State, which reads through the node's actor.BootupWatchconsumes the opening boot-up first, then waits for the one after the reset.ObjectDictionary.WriteGateWaiterscounter.The counter adds two interlocked operations around the gate in
WriteUnsignedandAdd. Both methods already take two locks, so the cost is small in comparison. Nothing outside the test project reads the counter.Checks that something did not happen keep their wall-clock windows. Each commit message lists the affected tests. One of them is the RPDO drain in
Tpdo_Emission_UnderConcurrentOdWrites_NeverTears, and ff50e60 has the full reasoning:EmitTpdosends each TPDO through its ownTask.Run, so no frame can mark "everything before me has arrived".Mutation checks:
WriteUnsignedmakesA_Typed_Write_Resolves_Its_Type_Under_The_Write_GateandA_Direct_Write_Cannot_Land_Inside_A_ConfigureTpdo_Transactionfail.AddmakesA_Redeclaration_Waits_For_The_Write_In_Flight_On_The_Entryfail.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, locally)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