Conversation
…nd claim in arbitration, and send without a pool hop #58, the node findings. A node that lost arbitration answered at once, so two nodes colliding on an address could answer each other in lockstep for ever; the Cannot Claim, and an arbitrary-address node's next claim, go out after SAE J1939-81 §4.4.4.3's pseudo-random 0..153 ms backoff -- the NAME's bytes summed, modulo 255, times 0.6 ms -- scheduled on the actor. A second ClaimAddressAsync while one is in arbitration silently cancelled the first, whose caller was waiting on it; it faults with InvalidOperationException now, and a pending claim already cancelled by its caller, whose cancel is still on its way to the loop, is swept so that awaiting the cancellation and claiming again is race-free. The claim and Cannot Claim sends went through Task.Run, a pool hop per round -- 240 for a full arbitrary-address scan -- in front of a SendConfirmed whose continuation already runs off the loop; they are called directly. And J1939NodeOptions.TransportOptions no longer documents a factory overload taking a pre-built channel, which does not exist. The #121 test that pinned the silent replacement cancels the first claim explicitly now. Mutation-checked: without the backoff the Cannot Claim follows the claim it lost to by half a millisecond; with the second claim cancelling the first, no exception is thrown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview
Performance: address-claim and Cannot Claim transmits drop the extra Tests add Reviewed by Cursor Bugbot for commit b6c4ee6. 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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 568dd5e373
ℹ️ 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! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… through the backoff, and drop a Cannot Claim a new claim overtook Codex and Bugbot on #153, on the backoff's three consequences. A node unseated from a claimed address kept sending on it until the delayed round ran; the address is invalidated the instant the winning claim is heard, and only the next claim waits. The pending claim was cleared for the backoff, so a second ClaimAddressAsync slipped past the in-flight guard and was then overwritten; the claim in hand -- re-registered for the next candidate -- stays through the backoff, and the delayed round runs only if it still is that claim. And a loser that claimed again before its delayed Cannot Claim went out still had it go out; it is dropped once the node has left CannotClaim. The exhaustion test uses a NAME whose backoff is zero -- the scan pays the backoff before each of its 240 rounds, and its subject is the exhaustion. Mutation-checked, one per consequence: invalidating in the delayed round makes the reaction 152 ms against a 75 ms bound; clearing the pending claim lets the second claim through; the unguarded Cannot Claim goes out. Co-Authored-By: Claude Opus 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: 8b1bf4cef4
ℹ️ 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".
…ng-off round on a request or a contest Codex and Bugbot on #153. The delayed Cannot Claim was guarded by state alone, so a second loss inherited the remainder of the first's backoff; the deadline is held, replaced by a later loss and cancelled by a new claim, so each loss waits its own. And a round waiting its backoff was treated as live arbitration: a Request for Address Claimed, or a lower-priority peer claiming the candidate, made it announce at once, without the round -- it announces by starting the round now, once, since either is the moment to. Mutation-checked: without the deadline's replacement the second loss's Cannot Claim goes out 45 ms after it; with the request answered by a frame of its own, the candidate is announced twice. Co-Authored-By: Claude Opus 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: 99cc8099d2
ℹ️ 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".
…m, and read the lost address in the handler Two findings on 99cc809 of #153. Codex: the backoff was the NAME's byte sum modulo 255, which never reaches the 153 ms endpoint the 0..153 ms range of SAE J1939-81 §4.4.4.3 documents and maps a sum of exactly 255 to zero -- a claimant that answers at once instead of last. The low byte of the sum (modulo 256) gives all 256 slots. The backoff test now uses a NAME whose bytes sum to exactly 255 and expects a gap of at least 148 ms, so the modulo-255 form fails it (found 0.5 ms); the exhaustion test moves to a NAME summing to 256 to keep its zero backoff. macOS CI: An_Unseated_Node_Loses_The_Address_At_Once_And_Waits_Only_To_Reclaim read owner.Address after awaiting the Claiming event, and on a runner that scheduled the continuation late the 150 ms backoff and the 80 ms round had already re-claimed 0x80 (found 0x80, expected null). The handler now records the address in the step that made the transition, and that is what the test asserts on. Co-Authored-By: Claude Opus 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: 35c879935d
ℹ️ 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".
…m already waiting Codex on 35c8799 of #153: in CannotClaim state a Request for Address Claimed took the default branch and sent a null-address claim at once, while the backoff armed by the loss stayed armed and sent a second copy -- a scan both bypassed the new collision-avoidance delay and got two announcements. The delay matters for the answer in its own right: a Cannot Claim carries source address 0xFE, so two nodes answering the same global request at the same instant put identical CAN IDs with different NAME payloads on the bus, which arbitration cannot separate. The answer now shares the loss's handle -- one waiting is the answer, none arms a fresh backoff -- and the state guard moves from "is CannotClaim" to "is neither Claimed nor Claiming", so a node that never claimed still answers a scan. A_Request_During_The_Cannot_Claim_Backoff_Is_Answered_By_That_One_Frame pins both halves: with the immediate send restored it sees two frames. Co-Authored-By: Claude Opus 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: fe277dad5c
ℹ️ 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".
Codex on fe277da of #153: the backoff delayed the Cannot Claim but not the J1939CannotClaimException, so a caller ending a `using` scope on that exception disposed the node inside the backoff and the frame the loss owed the bus was never sent -- the order step 3 of the ClaimAddressAsync contract documents, reversed. The losing claim is now held until its Cannot Claim has gone out and faulted there; a further claim started meanwhile drops the Cannot Claim, which settles the loss just as finally, and faults it at once; Dispose settles one still waiting. A_Lost_Claim_Faults_Only_Once_Its_Cannot_Claim_Is_On_The_Bus disposes the loser the moment the exception arrives and a spectator bus still sees the frame -- with the immediate fault restored it sees none. Three tests that awaited the exception to act inside the backoff now await the transition to CannotClaim, which is the instant the loss is known and the backoff starts; the exception is awaited at their end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The announce confirmation, a failed Cannot Claim, a request during arbitration, and a cancelled claim swept by the next one had no test. The lost-claim test now disposes with using, which is the scope CodeQL flagged. Co-authored-by: Cursor <cursoragent@cursor.com>
CodeQL flagged the try/finally: an exception from the claim or the timer wait skipped the dispose the test was asserting. The using ends once the backoff is armed, which is when the node has to be gone. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f32e28b4
ℹ️ 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".
SendAddressClaimFrame returns once the send has been started. Completing the loss on that same turn lets a caller dispose on the exception while SendConfirmed is still pending, so the frame never reaches the driver. Co-authored-by: Cursor <cursoragent@cursor.com>
| catch (Exception ex) | ||
| { | ||
| RaiseBackgroundException(ex); | ||
| } |
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 b6c4ee6. Configure here.
| { | ||
| _cannotClaimHandoffPending = false; | ||
| CompleteLostClaim(); | ||
| }); |
There was a problem hiding this comment.
Handoff can complete the wrong loss
High Severity
The Cannot Claim afterHandoff always completes the current _lostClaim, not the loss that started that send. A further ClaimAddressAsync during the handoff is allowed to leave that slot set, so a second loss overwrites it. The first ClaimAddressAsync then never completes, and the second is faulted when the earlier frame finishes rather than its own.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b6c4ee6. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6c4ee64ea
ℹ️ 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".
| SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress, afterHandoff: () => | ||
| { | ||
| _cannotClaimHandoffPending = false; | ||
| CompleteLostClaim(); |
There was a problem hiding this comment.
When claim A's Cannot Claim is still awaiting SendConfirmed, BeginClaim deliberately permits claim B; if B also loses before A's continuation runs, _lostClaim = pending overwrites A with B. This parameterless callback then completes B from A's handoff—before B's own backoff/send—while A's task is no longer referenced and never completes. Capture the specific lost claim or a generation in each continuation instead of completing whichever claim is currently stored.
Useful? React with 👍 / 👎.
This branch has not been deployed
2 participants
What does this change?
Closes #58 — the node findings; the transport ones landed on #152 and the docs bullet on #145.
ClaimAddressAsyncwhile one is in arbitration faults withInvalidOperationExceptioninstead of silently cancelling the first, whose caller is waiting on it. A pending claim its caller already cancelled — the cancel still on its way to the loop — is swept, so awaiting the cancellation and claiming again is race-free. The J1939: the self-traffic guard is bounded by the arbitration window, not by the transmission it exists for #121 test that pinned the silent replacement cancels the first claim explicitly now.Task.Runin front of aSendConfirmedwhose continuation already runs off the loop — 240 hops for a full arbitrary-address scan. They are called directly.J1939NodeOptions.TransportOptionsno longer documents a factory overload taking a pre-built channel, which does not exist.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) — a second claim in arbitration faulted the first before and faults the second now; a behaviour that was a defect, not a documented contractChecklist
dotnet build CanKit.Pro.sln -c Releasesucceedsdotnet test CanKit.Pro.sln -c Releasepasses (net10.0, all 960)IJ1939Node.ClaimAddressAsyncdocuments the backoff and the new exceptionCannotClaim_Is_Sent_After_The_Names_Pseudo_Random_Backoff(a NAME chosen for a 78 ms backoff; the gap after the claim it lost to is a lower bound),A_Second_Claim_During_Arbitration_Faults_And_Leaves_The_First_Alone,An_Unseated_Node_Loses_The_Address_At_Once_And_Waits_Only_To_Reclaim,A_Claim_During_The_Backoff_Before_A_Reclaim_Faults_And_The_Reclaim_Completes,A_Delayed_Cannot_Claim_Is_Dropped_Once_A_New_Claim_Has_Started,A_Second_Loss_Waits_Its_Own_Full_Backoff_Before_Cannot_Claim,A_Request_During_The_Backoff_Starts_The_Round_With_A_Single_Announcement; each fails with its change reverted (mutation-checked). The pool hop is by construction.🤖 Generated with Claude Code