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
Address-claim and fire-and-forget Cannot Claim sends drop the extra Reviewed by Cursor Bugbot for commit fe277da. 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".
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 8b1bf4c. Configure here.
…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".
| contendingSa: peerSa, contendingName: peerName); | ||
| SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); | ||
| ScheduleCannotClaim(); | ||
| pending.Tcs.TrySetException(new J1939CannotClaimException(pending.PreferredAddress)); |
There was a problem hiding this comment.
When a non-arbitrary claim loses with a nonzero backoff, this faults ClaimAddressAsync immediately even though ScheduleCannotClaim has only armed a timer. If the caller disposes the node after catching J1939CannotClaimException—for example by leaving a using scope—the actor is disposed before the timer fires and the _disposed == 0 guard suppresses the frame entirely. This violates the documented contract that Cannot Claim is broadcast before the exception is thrown; keep the operation pending until the delayed send has at least been initiated.
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 959)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