fix(isotp): complete the PDU inbox on the actor so a Dispose cannot drop what a discard retains - #275
Conversation
…rop what a discard retains DiscardPendingPdus takes the inbox items out and writes the retained ones back on the actor. Dispose completed the inbox from the caller's thread, and between the take and the write-back the write failed: the PDUs the discard promised to keep were lost. The inbox is completed by the cleanup Dispose already posts to the actor (directly where no actor is left to run a discard, and in place when Dispose runs on the actor), so it is completed behind a running discard (#262). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Receive shutdown no longer completes the bounded channel’s writer on Producers ( Tests add ISO-TP coverage for dispose-during-discard, receive completion while the actor is blocked, and post-dispose producer writes. A J1939 periodic-send test stops relying on elapsed-time bounds and asserts cancellation via completion counters and no background failures. Reviewed by Cursor Bugbot for commit 3252406. 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 595ed907b3
ℹ️ 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! |
… completing its writer Completing the writer from the disposing thread raced a discard's write-back; completing it on the actor made the wake of the receivers depend on the actor getting round to it. The end is a flag and a signal beside the inbox now, as the loss of the bus service already is: a receiver takes what is buffered and meets the end on an empty inbox, a discard writes back into an inbox that is never closed, and Dispose lets the receivers go at once from any thread (#262). 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: 51046d49f5
ℹ️ 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".
…hing 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: ec0ecf7a1d
ℹ️ 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".
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 ec0ecf7. Configure here.
…st(j1939): judge the cancelled emission by its outcome The producers write into the inbox under the same gate the disposal takes to set the closed flag, so nothing is queued after it and what was queued before is there when a receiver meets it. The J1939 test that a disposed schedule cancels its emission measured a stopwatch against the bus's confirm timeout, and a cancellation took 620 ms to be observed on a Windows runner; it now checks that the emission ends without reporting the timeout. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
What does this change?
#262:
DiscardPendingPdus(arrivedBefore)takes every item out of the PDU inbox on the actor and writes the ones that arrived at or after the stamp back.Disposecompleted the inbox from the caller's thread, unsynchronised with that. ADisposebetween the take and the write-back made everyTryWritefail, and the PDUs the discard promised to keep were lost.Decided differently from the issue's first option, after two review rounds on this pull request: completing the inbox on the actor made the receivers' wake depend on the actor getting round to it (and an in-place completion still raced a discard already queued behind it). The inbox's writer is not completed at all any more. The end is a flag and a signal beside the inbox, as the loss of the bus service has been since #261: a receiver takes what is buffered and meets the end on an empty inbox, a discard writes back into an inbox that is never closed, and
Disposesets the flag and the signal from whichever thread it runs on, so the receivers are let go at once and no actor has to run. A PDU that a discard retained can still be read after the disposal.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; net48 only runs in CI)Mutation check: completing the inbox from the disposing thread again fails the new test, which disposes from another thread while a discard has the retained PDU out of the inbox (the hook for that moment,
DiscardGapObserver, exists from #261). The test holds the discard for 300 ms to let the dispose get as far as it will, so it can only pass falsely on a very slow host.Closes #262.
🤖 Generated with Claude Code