fix(j1939tp): bind BAM steps to their session, keep T3 on a CTS hold, validate timers - #258
Conversation
… validate timers Three defects from the 2026-09-30 deep review (docs/reviews/2026-09-30-deep-review.md, findings T1, T2 and the J1939-TP half of I8). A BAM chain was addressed by its (destination, PGN) key only. Cancelling a BAM and sending the same PGN again inside the packet spacing left the first session's timer armed; it then drove the new session, so TP.DT 1 went out twice, a conforming receiver aborted, and the sender reported success. Each step now carries its session and acts only while that instance is the registered one. A CTS with numPackets = 0 replaced the running timer with T4 in every state, but T4's expiry is ignored outside WaitCts. After the last TP.DT that left the send with no timer at all, holding its slot and blocking every later send to that destination. The hold now only takes effect while the sender waits for a CTS; otherwise T3 keeps running. T1..T4 and BamPacketSpacing were not validated. A negative value made Arm throw after the session was registered and the RTS sent, again leaving a send without a deadline. T1..T4 must now be greater than zero and BamPacketSpacing not negative, rejected at Open. Each fix has a regression test that was seen to fail with that fix removed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview BAM lifecycle: BAM announce/DT continuations now carry the Connection-mode CTS hold: Options: Regression tests cover BAM cancel/replace races, hold timing edge cases, and option validation. Reviewed by Cursor Bugbot for commit 570ab7b. 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: f47212c598
ℹ️ 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❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
A CTS(0) that arrives right after the last DT of a block, before its confirmation has run, was dropped by the SendingDt guard, and the confirmation then armed T3. Remember the hold (not for the message's last packet, where T3 for the EndOfMsgAck is right) and arm T4 when the block ends; a CTS that grants packets clears it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ece32923c0
ℹ️ 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 spacing timer of a BAM captures its session and so its payload; the handle was discarded, so a cancelled BAM stayed reachable until the timer fired, which with a long BamPacketSpacing is as long as that value. The session now holds the handle and EndTx releases it. Tests: the timer is gone after a cancel; a late announce confirmation of a cancelled BAM cannot start the next BAM of the same PGN (the instance check, alone); a CTS hold while the last packet is outstanding leaves T3 for the EndOfMsgAck. The hold bookkeeping from the previous commit drops a condition that never had an effect. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0f11527. Configure here.
The channel ends its sessions through TxSession.Fail on dispose, not through EndTx, so a BAM in flight on a borrowed actor kept its spacing timer, session and payload until the interval elapsed. Release it in Cancel and Fail as well, behind one method. Also drops the redundant IsCm test from the current-session check (only BAM steps call it) and adds coverage for a late TP.DT confirmation of a cancelled BAM. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…confirmation with no successor Takes back the release line in TxSession.Cancel, which nothing calls (only RxSession.Cancel is used), so it only added an uncovered line. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2 participants
What does this change?
Closes three J1939-TP defects from the 2026-09-30 deep review (findings T1, T2 and the J1939-TP half of I8); the review has the mechanism and file/line for each. A cancelled BAM no longer drives the next BAM of the same PGN (it produced a duplicate TP.DT 1 and a reported success), a CTS hold after the last TP.DT no longer strips the send of every timer, and non-positive timers are rejected at
Openinstead of hanging a send.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)Validateis internalAlso run:
dotnet format --verify-no-changes,dotnet pack+eng/verify-packages.py.Notes
T1..T4equal to zero were accepted before and fired on the next loop iteration; they are now rejected withArgumentOutOfRangeException, as are negative values. A zeroBamPacketSpacingstays valid. I classified this as a fix, not a breaking change, because zero timers have no useful meaning for the protocol timers; say if you read it differently.🤖 Generated with Claude Code