test(canopen): say where the block-upload deadline test aborted - #241
Conversation
Sdo_BlockUpload_Server_Deadline_Measures_Peer_Idle_Time_Not_The_Whole_Transfer failed once on CI at NotBe(CsAbort) without saying which of the 26 gaps it was or what the abort code was. The failure now reports the aborted object, the abort code, the sub-block index, the time between the abort and the last frame the peer sent, and the time into the transfer. FrameTap records each frame's arrival time so the idle time is not measured at dequeue. Refs #240 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016EcWb8FeLKbGWkDkZFYnvp
PR SummaryLow Risk Overview The shared Reviewed by Cursor Bugbot for commit 2ab4733. 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: 48e8517008
ℹ️ 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 abort diagnostics of the block-upload deadline test covered only the segment reads. A server timeout in the gap after the 25th segment is queued behind the final ACK and was consumed as the end frame, so it failed the generic end-frame assertion without the abort code or timing. Both reads now go through one check. Refs #240 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016EcWb8FeLKbGWkDkZFYnvp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ab5d89a05
ℹ️ 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".
The abort diagnostics of the block-upload deadline test recorded each peer frame after Send returned. If the test thread was descheduled in between, the server could time out and its abort arrive before that timestamp; the frame was then excluded and the idle time reported from the preceding one. Taking the timestamp before the send keeps it at or before the frame's arrival. With a 700 ms stall injected after an ACK and a 300 ms server timeout, the message read 402 ms before and reads 301 ms after. Refs #240 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016EcWb8FeLKbGWkDkZFYnvp
|
@codex review Head Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbd2d72d24
ℹ️ 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".
| peerFrames.Add(System.Diagnostics.Stopwatch.GetTimestamp()); | ||
| Send(rawBus, CanOpenCobId.SdoRx(0x02), SdoBlockFrames.BuildSubBlockAck( |
There was a problem hiding this comment.
The fresh pre-send placement in 2ab4733 only reverses the original race: if the runner is descheduled after this timestamp is added but before Send executes, the server can time out while the ACK has not yet been transmitted. Because that timestamp still precedes the abort's arrival, Last(...) treats the unsent ACK as the latest peer frame and reports roughly serverTimeout - gap (about 1.9 s here) rather than the real 2 s idle period, making a host stall look like an early deadline—the exact distinction these diagnostics are intended to establish. Record the ACK at its actual transmission or server-side observation instead.
Useful? React with 👍 / 👎.
2 participants
What does this change?
Sdo_BlockUpload_Server_Deadline_Measures_Peer_Idle_Time_Not_The_Whole_Transferfailed once on CI atNotBe(CsAbort)without saying which of the 26 gaps it was or what the abort code was (#240). The failure now reports the aborted object, the abort code, the sub-block index, the time between the abort and the last frame the peer sent, and the time into the transfer, so the next occurrence says whether the deadline fired early (a re-arm defect) or at a random point (host stall).FrameTaprecords each frame's arrival time so the idle time is not measured at dequeue.This does not fix the underlying cause, which is still unknown. The issue stays open; the commit uses
Refs #240, notCloses.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 (run with-p:CI=true, 0 warnings)dotnet test CanKit.Pro.sln -c Releasepasses (net10.0, all 1376 tests)Also run locally:
dotnet format CanKit.Pro.sln --verify-no-changesanddotnet packwitheng/verify-packages.py.🤖 Generated with Claude Code
https://claude.ai/code/session_016EcWb8FeLKbGWkDkZFYnvp
Generated by Claude Code