fix(isotp): fault a waiting receive when the service goes, guard a send after dispose, validate the protocol timers - #261
Conversation
…nd after dispose, validate the protocol timers Three defects from the 2026-09-30 deep review (I6, I7, I8). A channel opened over a shared service with leaveOpen outlived the service when its owner disposed it first. The reader task ended quietly, and a ReceiveAsync waiting on the inbox waited for ever, while a send failed at once. The channel now puts the reason (an ObjectDisposedException for the bus service) into the inbox as a fault, raises it on BackgroundExceptionOccurred and completes the inbox behind it. Dispose can fall between SendAsync's disposed check and the post of the send to the actor. Its cleanup is then queued ahead of the send, finds nothing in flight, and the send that runs afterwards began on a disposed channel: a frame on the bus and, on an owned actor that is already gone, a call nobody completes. BeginSendOnLoop now has the guard HandleReceivedFrame already had. NAs, NBs and NCr are validated where LocalStMin already was: a negative one made the deadline scheduler throw after the send was on the wire or the reception begun, leaving it without a deadline. They must be greater than zero. Each fix has a regression test that fails with that fix removed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Receive paths ( Tests cover multi-waiter faulting, buffered-then-loss ordering, discard races, cancellation without consuming buffered PDUs, subscription faults, and a Reviewed by Cursor Bugbot for commit 4c3b569. 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: 89ce021c49
ℹ️ 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}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review on #261: when the shared service went, the reason was enqueued as a fault item. In the bounded DropOldest inbox that item could push the oldest finished PDU out, and with several blocked receivers only one saw the real failure while the others got the generic completion error. The inbox is now completed with the error: buffered PDUs stay readable, then every receiver, present and later, gets the ObjectDisposedException. A reassembly under way is dropped without an item. Also takes the service in the test under a using, as the code scanner asked. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…y maps its variable Code scanning (cs/linq/missed-select) on #261. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…hat decided nothing Codecov on #261. The reader failing, an injected actor that is already gone, and a reassembly under way when the service goes now have tests; the disposed guard in the actor callback and the special case in the catch did not change any outcome and are gone. 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: 5b49889287
ℹ️ 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".
Review on #261: DiscardPendingPdus(arrivedBefore) drains the inbox and writes the items from after its stamp back; once the inbox is completed with the loss that write fails and the retained PDUs were silently dropped, although the UDS client calls it exactly then. They now move to a fresh inbox completed with the same failure. The actor in the test is a using declaration, as code scanning asked. 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: 4ae8e1fd96
ℹ️ 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".
Reviews on #261: a reader meeting the old, drained and failure-completed inbox in the middle of a discard threw the loss before the retained PDUs arrived. The writable replacement is now published first and completed with the failure after the retained items are in. The discard test waits for the loss to be reported before it discards: it raced the reader noticing the service went, and the interleaving where the discard came first passed without exercising the replacement at all. 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: e43b4bbf08
ℹ️ 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".
![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 e43b4bb. Configure here.
…ing it Reviews on #261: moving the retained PDUs to a replacement inbox after a service loss needed the handoff serialised against readers and against Dispose, and each fix opened the next race. The inbox is no longer completed when the service goes (only Dispose completes it). The failure is recorded and the receivers are woken; a receive takes what is buffered first and throws the loss only on an empty inbox, for every receiver. DiscardPendingPdus is back to its original form: the inbox stays open, so what it retains is written back as before. 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: 2aa5c96ba3
ℹ️ 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".
…retained PDU first Review on #261: a discard takes the PDUs it retains out of the inbox and writes them back. After a service loss a receiver meeting the inbox empty in that gap threw the loss ahead of them. A flag marks the gap; a receiver waits it out and looks once more before it throws. The streaming receive's end on dispose is covered as well. 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: 18712a878d
ℹ️ 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".
Review on #261: the receive loop read the inbox before it reached any wait on the token, so a token that was already canceled still returned a buffered PDU (and the streaming receive drained them), where the wait on the token threw before. The token is checked before each read. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… second loop pass Codecov on #261: the second read was only reachable through a race. As a second pass of the loop it gives the same guarantee and is run by every receive that ends in the loss. 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: adf5b1d932
ℹ️ 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".
Review on #261: a caller pumping the subscription (GetReceptionsInProgress, a settle, a discard) holds the pump lock from taking a frame to posting it; the reader's post of the loss ran without it and could overtake that frame, so a receiver threw the loss before a PDU that had been received. The loss is posted under the same lock. 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: 4c3b56953a
ℹ️ 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".
2 participants
What does this change?
Closes three ISO-TP defects from the 2026-09-30 deep review (I6, I7, I8; mechanism and file/line are in the review). A receive waiting on a channel whose shared service is disposed under it now faults instead of waiting for ever, a send can no longer begin on a channel disposed between the check and the post, and non-positive
NAs/NBs/NCrare rejected atOpen.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)Also run:
dotnet format --verify-no-changes,dotnet pack+eng/verify-packages.py.Notes
NAs,NBsorNCrequal to zero were accepted before and fired on the next loop iteration; they are now rejected withArgumentOutOfRangeException, as are negative values (the same call as for J1939-TP's timers in fix(j1939tp): bind BAM steps to their session, keep T3 on a CTS hold, validate timers #258).Disposeat exactly the point between the disposed check and the post; the race cannot be hit deterministically any other way.🤖 Generated with Claude Code