From 568dd5e373870d0829c485b0ee1b1933f7f3b2ad Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:13:17 +0200 Subject: [PATCH 1/9] fix(j1939): back off before Cannot Claim and a re-claim, fault a second claim in arbitration, and send without a pool hop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- src/CanKit.Pro.J1939/IJ1939Node.cs | 6 + src/CanKit.Pro.J1939/J1939NodeImpl.cs | 130 ++++++++++++------ src/CanKit.Pro.J1939/J1939NodeOptions.cs | 6 +- src/CanKit.Pro.J1939/README.md | 7 +- .../TestCases/J1939/J1939NodeTests.cs | 85 +++++++++++- 5 files changed, 180 insertions(+), 54 deletions(-) diff --git a/src/CanKit.Pro.J1939/IJ1939Node.cs b/src/CanKit.Pro.J1939/IJ1939Node.cs index b239d3e..beecf0d 100644 --- a/src/CanKit.Pro.J1939/IJ1939Node.cs +++ b/src/CanKit.Pro.J1939/IJ1939Node.cs @@ -79,9 +79,15 @@ public interface IJ1939Node : IDisposable, IAsyncDisposable /// per SAE J1939-81 §4.4.3.4 and throw /// (SRS FR-J1939-004). /// + /// The Cannot Claim, and an arbitrary-address node's next claim after losing, go out after + /// the pseudo-random 0..153 ms backoff of SAE J1939-81 §4.4.4.3 -- the NAME's bytes summed, + /// modulo 255, times 0.6 ms -- so two nodes colliding on an address do not answer in + /// lockstep (#58). /// /// The preferred address was lost to a /// higher-priority NAME and no fallback was available. + /// A claim is still in arbitration: it is + /// not silently replaced -- await it, or cancel it, before claiming again (#58). Task ClaimAddressAsync(byte preferredAddress, CancellationToken cancellationToken = default); /// diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index c4c363e..24c6862 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -307,14 +307,51 @@ private void BeginClaim(byte preferredAddress, TaskCompletionSource tcs return; } - // Cancel any previous in-flight claim. - _pendingClaim?.Deadline?.Dispose(); - _pendingClaim?.Tcs.TrySetCanceled(); - _pendingClaim?.CtRegistration.Dispose(); + // A claim still in arbitration is not silently replaced: the caller who started it + // is waiting on it, and a second one is a programming error (#58). A re-claim on a + // node that holds an address, or holds none, is what this method is for. A pending + // claim whose task has already completed -- cancelled by its caller, whose cancel is + // still on its way to this loop -- is over, and is swept here so that awaiting the + // cancellation and claiming again is race-free. + if (_pendingClaim is { } inFlight) + { + if (!inFlight.Tcs.Task.IsCompleted) + { + ctr.Dispose(); + tcs.TrySetException(new InvalidOperationException( + $"An address claim for SA 0x{inFlight.PreferredAddress:X2} is in arbitration; await it, or cancel it, before claiming again.")); + return; + } + _pendingClaim = null; + inFlight.Deadline?.Dispose(); + inFlight.CtRegistration.Dispose(); + } BeginClaimRound(preferredAddress, tcs, ctr); } + // J1939-81 §4.4.4.3: a node that lost arbitration delays its Cannot Claim, and an + // arbitrary-address node its next claim, by a pseudo-random 0..153 ms derived from its + // NAME -- the eight bytes summed, modulo 255, times 0.6 ms -- so two nodes colliding on an + // address do not answer in lockstep for ever (#58). Fixed by the NAME, so a test can + // choose it; scheduled on the actor, so the state it acts on is the state it read. + private TimeSpan ClaimBackoff + { + get + { + int sum = 0; + foreach (var b in _name.ToBytes()) sum += b; + return TimeSpan.FromMilliseconds((sum % 255) * 0.6); + } + } + + private void AfterClaimBackoff(Action onLoop) + { + var delay = ClaimBackoff; + if (delay <= TimeSpan.Zero) { onLoop(); return; } + _deadlines.Arm(delay, () => { if (_disposed == 0) onLoop(); }); + } + // Starts (or restarts, for the arbitrary-address fallback) a single arbitration round for // `preferredAddress` on the actor loop. Unlike BeginClaim it does not cancel the caller's // pending claim — the same TCS (and the scan state) is carried across retries. @@ -532,7 +569,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) { // The caller's TCS and its cancellation registration stay alive across // retries; only the arbitration round is restarted. - BeginClaimRound(nextCandidate, pending.Tcs, pending.CtRegistration, scanStart); + AfterClaimBackoff(() => BeginClaimRound(nextCandidate, pending.Tcs, pending.CtRegistration, scanStart)); return; } @@ -543,7 +580,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.CannotClaim, address: null, contendingSa: peerSa, contendingName: peerName); - SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + AfterClaimBackoff(() => SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress)); pending.Tcs.TrySetException(new J1939CannotClaimException(pending.PreferredAddress)); return; } @@ -572,7 +609,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _ = tcs.Task.ContinueWith(t => RaiseBackgroundException(t.Exception!.GetBaseException()), CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.Default); - BeginClaimRound(nextCandidate, tcs, default, scanStart); + AfterClaimBackoff(() => BeginClaimRound(nextCandidate, tcs, default, scanStart)); return; } // Broadcast Cannot-Claim and transition. @@ -580,7 +617,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.CannotClaim, address: null, contendingSa: peerSa, contendingName: peerName); - SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + AfterClaimBackoff(() => SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress)); } else { @@ -631,31 +668,36 @@ private void TransmitAddressClaimConfirmed(byte sourceAddress) var payload = BuildAddressClaimPayload(); uint canId = J1939Id.ComposePgn(_options.ClaimPriority, J1939Pgn.AddressClaimed, sourceAddress, destinationAddress: J1939Pgn.GlobalAddress); - byte preferred = sourceAddress; - _ = Task.Run(async () => - { - try - { - using var frame = CanFrame.Classic(unchecked((int)canId), payload, isExtendedFrame: true); - var confirmation = await _service.SendConfirmed(frame).ConfigureAwait(false); - if (!confirmation.Confirmed) - { - var ex = new J1939NodeException( - $"J1939 address claim TX failed (id=0x{canId:X8}): {confirmation.FailureReason}."); - try { _actor.Post(() => OnClaimAnnounceTxFailed(preferred, ex)); } - catch (ObjectDisposedException) { } - return; - } + // Called directly rather than through Task.Run: SendConfirmed's synchronous part is + // the driver hand-off, and its continuation already runs off this loop; a pool hop + // in front of it bought nothing and cost one per claim round -- a full arbitrary- + // address scan is 240 of them (#58). + _ = TransmitAddressClaimConfirmedAsync(canId, payload, sourceAddress); + } - try { _actor.Post(() => OnClaimAnnounceTxConfirmed(preferred)); } - catch (ObjectDisposedException) { } - } - catch (Exception ex) + private async Task TransmitAddressClaimConfirmedAsync(uint canId, byte[] payload, byte preferred) + { + try + { + using var frame = CanFrame.Classic(unchecked((int)canId), payload, isExtendedFrame: true); + var confirmation = await _service.SendConfirmed(frame).ConfigureAwait(false); + if (!confirmation.Confirmed) { + var ex = new J1939NodeException( + $"J1939 address claim TX failed (id=0x{canId:X8}): {confirmation.FailureReason}."); try { _actor.Post(() => OnClaimAnnounceTxFailed(preferred, ex)); } catch (ObjectDisposedException) { } + return; } - }); + + try { _actor.Post(() => OnClaimAnnounceTxConfirmed(preferred)); } + catch (ObjectDisposedException) { } + } + catch (Exception ex) + { + try { _actor.Post(() => OnClaimAnnounceTxFailed(preferred, ex)); } + catch (ObjectDisposedException) { } + } } private void SetClaimState(J1939ClaimState state, byte? address, byte? contendingSa, @@ -887,22 +929,24 @@ private void TransmitFrame(uint canId, byte[] payload) { // Fire-and-forget: address-claim traffic doesn't need a task, but we still want a // background exception if the driver rejects it. SendConfirmed is used consistently - // with the rest of the CanKit.Pro stack. - _ = Task.Run(async () => + // with the rest of the CanKit.Pro stack. No Task.Run hop in front of it (#58). + _ = TransmitFrameAsync(canId, payload); + } + + private async Task TransmitFrameAsync(uint canId, byte[] payload) + { + try { - try - { - using var frame = CanFrame.Classic(unchecked((int)canId), payload, isExtendedFrame: true); - var confirmation = await _service.SendConfirmed(frame).ConfigureAwait(false); - if (!confirmation.Confirmed) - RaiseBackgroundException(new J1939NodeException( - $"J1939 frame TX failed (id=0x{canId:X8}): {confirmation.FailureReason}.")); - } - catch (Exception ex) - { - RaiseBackgroundException(ex); - } - }); + using var frame = CanFrame.Classic(unchecked((int)canId), payload, isExtendedFrame: true); + var confirmation = await _service.SendConfirmed(frame).ConfigureAwait(false); + if (!confirmation.Confirmed) + RaiseBackgroundException(new J1939NodeException( + $"J1939 frame TX failed (id=0x{canId:X8}): {confirmation.FailureReason}.")); + } + catch (Exception ex) + { + RaiseBackgroundException(ex); + } } // ========================================================================================= diff --git a/src/CanKit.Pro.J1939/J1939NodeOptions.cs b/src/CanKit.Pro.J1939/J1939NodeOptions.cs index 5db6a45..efb3085 100644 --- a/src/CanKit.Pro.J1939/J1939NodeOptions.cs +++ b/src/CanKit.Pro.J1939/J1939NodeOptions.cs @@ -58,9 +58,9 @@ public J1939NodeOptions(J1939Name name) public int ReceiveBufferCapacity { get; init; } = 128; /// - /// Options forwarded to the shared for multi-frame payloads - /// (SRS FR-J1939-006). Defaults to a fresh . Ignored when the - /// caller supplies their own pre-built channel to the factory. + /// Options for the the node opens for multi-frame payloads + /// (SRS FR-J1939-006). Defaults to a fresh . The node always + /// opens that channel itself; no factory overload takes a pre-built one (#58). /// public J1939TpOptions TransportOptions { get; init; } = new J1939TpOptions(); diff --git a/src/CanKit.Pro.J1939/README.md b/src/CanKit.Pro.J1939/README.md index 5928620..2a231ff 100644 --- a/src/CanKit.Pro.J1939/README.md +++ b/src/CanKit.Pro.J1939/README.md @@ -23,7 +23,12 @@ FR-J1939-001..006 (Must) and FR-J1939-007 (Should). 0xEE00 from SA = 0xFE) when the field is exhausted. Governed by `J1939NodeOptions.EnableArbitraryAddressClaiming` (default: derived from the NAME's Arbitrary Address Capable bit). A move after a successful claim - is announced through `AddressClaimChanged`; nobody awaits it. + is announced through `AddressClaimChanged`; nobody awaits it. The Cannot + Claim, and the next claim after losing, go out after the pseudo-random + 0..153 ms backoff of SAE J1939-81 §4.4.4.3 (the NAME's bytes summed, modulo + 255, times 0.6 ms), so two nodes colliding on an address do not answer in + lockstep; and a second `ClaimAddressAsync` while one is in arbitration faults + with `InvalidOperationException` rather than silently cancelling the first (#58). - **Request for Address Claimed** (SAE J1939-81 §4.2.2): a Request for PGN 0xEE00 is answered by the node itself — with its Address Claimed while it holds or arbitrates an address, with Cannot Claim while it holds none — so diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index c8d03bd..feddd36 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -306,6 +306,74 @@ public async Task CannotClaim_BroadcastsWithNullSourceAddress() J1939Pgn.IsAddressClaim(decomposed.Pgn).Should().BeTrue(); } + // #58 (SAE J1939-81 §4.4.4.3): a node that lost arbitration sends its Cannot Claim after a + // pseudo-random 0..153 ms derived from its NAME -- the bytes summed, modulo 255, times + // 0.6 ms -- so two nodes colliding on an address do not answer in lockstep. The loser's + // NAME here gives 78 ms; the gap between the winner's re-announcement, which is what the + // loser answers, and the Cannot Claim is at least that, a lower bound a loaded host only + // raises. + [Fact] + public async Task CannotClaim_Is_Sent_After_The_Names_Pseudo_Random_Backoff() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var busC = Open(session, 2); // spectator + + long reannouncedAt = 0, cannotClaimedAt = 0; + var winnerClaimed = new[] { false }; + var cannotClaimSeen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + busC.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (!J1939Pgn.IsAddressClaim(fields.Pgn)) return; + if (fields.SourceAddress == 0x60 && Interlocked.Read(ref reannouncedAt) == 0 && Volatile.Read(ref winnerClaimed[0])) + Interlocked.Exchange(ref reannouncedAt, Stopwatch.GetTimestamp()); + if (fields.SourceAddress == J1939Pgn.NullAddress) + { + Interlocked.Exchange(ref cannotClaimedAt, Stopwatch.GetTimestamp()); + cannotClaimSeen.TrySetResult(true); + } + }; + + var loserName = Name(0x0000E0); // byte sum mod 255 = 130: 78 ms + var loserBackoff = TimeSpan.FromMilliseconds((loserName.ToBytes().Sum(b => (int)b) % 255) * 0.6); + loserBackoff.Should().Be(TimeSpan.FromMilliseconds(78), "the NAME was chosen for it"); + + using var winner = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(200) }); + using var loser = J1939Node.Open(busB, new J1939NodeOptions(loserName) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(200) }); + + await winner.ClaimAddressAsync(0x60).WithTimeout(ShortTimeout); + Volatile.Write(ref winnerClaimed[0], true); + Func act = () => loser.ClaimAddressAsync(0x60).WithTimeout(ShortTimeout); + await act.Should().ThrowAsync(); + await cannotClaimSeen.Task.AsTaskWithTimeout(ShortTimeout); + + Interlocked.Read(ref reannouncedAt).Should().NotBe(0, "the winner re-announced its claim, which is what the loser lost to"); + var gap = TimeSpan.FromSeconds((Interlocked.Read(ref cannotClaimedAt) - Interlocked.Read(ref reannouncedAt)) / (double)Stopwatch.Frequency); + gap.Should().BeGreaterThanOrEqualTo(loserBackoff - TimeSpan.FromMilliseconds(5), + "the Cannot Claim waited the NAME's backoff after the claim it lost to"); + } + + // #58: a second ClaimAddressAsync while one is in arbitration faults instead of silently + // cancelling the first, whose caller is waiting on it. + [Fact] + public async Task A_Second_Claim_During_Arbitration_Faults_And_Leaves_The_First_Alone() + { + var session = NewSession(); + using var bus = Open(session, 0); + using var node = J1939Node.Open(bus, new J1939NodeOptions(Name(0x000030)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(200) }); + + var first = node.ClaimAddressAsync(0x70); + Func second = () => node.ClaimAddressAsync(0x71).WithTimeout(ShortTimeout); + await second.Should().ThrowAsync(); + + await first.WithTimeout(ShortTimeout); + node.Address.Should().Be(0x70); + node.ClaimState.Should().Be(J1939ClaimState.Claimed); + } + // --------------------------------------------------------------------------------------- // #34 (SAE J1939-81 §4.2.2): a Request for PGN 0xEE00 is answered by the node itself — // with its Address Claimed while it holds an address, with Cannot-Claim (SA 0xFE) while it @@ -1772,13 +1840,14 @@ public async Task A_Broadcast_Echo_Arriving_While_The_Node_Holds_No_Address_Is_S await claimB.WithTimeout(ShortTimeout); } - // #121 across claim rounds: a re-claim replaced while in flight, then completed for a third + // #121 across claim rounds: a re-claim cancelled while in flight, then completed for a third // address, and the echo of the frame sent under the first address still arrives after all of // it. The ledger is not touched by claim rounds at all, so nothing here can forget the frame; - // the test pins that the replacement does not either (#119 had a marker that a second round - // wiped). + // the test pins that the cancellation does not either (#119 had a marker that a second round + // wiped). Since #58 a second claim no longer replaces one in flight -- it faults -- so the + // first is cancelled by its caller here. [Fact] - public async Task An_Echo_Sent_Under_The_Vacated_Address_Is_Not_Raised_After_A_Replaced_Reclaim() + public async Task An_Echo_Sent_Under_The_Vacated_Address_Is_Not_Raised_After_A_Cancelled_Reclaim() { using var bus = ControllableBus.DeferredEchoCapable(NewSession()); using var node = J1939Node.Open(bus, new J1939NodeOptions(Name(1)) @@ -1801,12 +1870,14 @@ public async Task An_Echo_Sent_Under_The_Vacated_Address_Is_Not_Raised_After_A_R destinationAddress: J1939Pgn.GlobalAddress)); await bus.DeferredEchoes.WaitForEnqueuedAsync(2, ShortTimeout); - var superseded = node.ClaimAddressAsync(0x22); + using var cancel = new CancellationTokenSource(); + var superseded = node.ClaimAddressAsync(0x22, cancel.Token); await bus.DeferredEchoes.WaitForEnqueuedAsync(3, ShortTimeout); - var reclaim = node.ClaimAddressAsync(0x23); - await bus.DeferredEchoes.WaitForEnqueuedAsync(4, ShortTimeout); + cancel.Cancel(); Func awaitSuperseded = () => superseded.WithTimeout(ShortTimeout); await awaitSuperseded.Should().ThrowAsync(); + var reclaim = node.ClaimAddressAsync(0x23); + await bus.DeferredEchoes.WaitForEnqueuedAsync(4, ShortTimeout); bus.DeferredEchoes.DiscardNext().Should().BeTrue(); // the broadcast's echo, held back bus.DeferredEchoes.ReleaseAll(); // both claims' announcements From 8b1bf4cef41257bfa7b57a0b324cc3e8d87296a0 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:29:43 +0200 Subject: [PATCH 2/9] fix(j1939): invalidate a lost address at once, keep the claim in hand 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 --- src/CanKit.Pro.J1939/J1939NodeImpl.cs | 57 +++++++-- .../TestCases/J1939/J1939NodeTests.cs | 112 +++++++++++++++++- 2 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index 24c6862..ae8afcf 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -345,6 +345,15 @@ private TimeSpan ClaimBackoff } } + // The delayed Cannot Claim of a lost arbitration: dropped if a new claim has started + // meanwhile -- its announcement is the newer word on the bus, and a Cannot Claim after it + // would retract it (Codex on #153). + private void SendCannotClaimIfStillUnclaimed() + { + if ((J1939ClaimState)Volatile.Read(ref _claimStateStore) != J1939ClaimState.CannotClaim) return; + SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + } + private void AfterClaimBackoff(Action onLoop) { var delay = ClaimBackoff; @@ -560,19 +569,35 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) // Arbitrary-address fallback (FR-J1939-004 / SAE J1939-81 §4.5): retry with the // next candidate from the arbitrary address field before giving up with // Cannot-Claim. - _pendingClaim = null; pending.Deadline?.Dispose(); + pending.Deadline = null; var scanStart = pending.ArbitraryScanStart; if (ArbitraryClaimingEnabled && TryGetNextArbitraryCandidate(pending.PreferredAddress, ref scanStart, out var nextCandidate)) { // The caller's TCS and its cancellation registration stay alive across - // retries; only the arbitration round is restarted. - AfterClaimBackoff(() => BeginClaimRound(nextCandidate, pending.Tcs, pending.CtRegistration, scanStart)); + // retries; only the arbitration round is restarted. The pending claim stays + // registered through the backoff, so a second ClaimAddressAsync meanwhile + // meets the in-flight guard, and the delayed round runs only if it is still + // the claim in hand -- not one cancelled or replaced meanwhile (Codex on + // #153). The lost address is no longer announced as ours meanwhile. + WriteAddress(null); + SetClaimState(J1939ClaimState.Claiming, nextCandidate, contendingSa: null, contendingName: null); + var retry = new PendingClaim(nextCandidate, pending.Tcs, deadline: null, pending.CtRegistration) + { + ArbitraryScanStart = scanStart, + }; + _pendingClaim = retry; // the next candidate is what is being claimed now + AfterClaimBackoff(() => + { + if (!ReferenceEquals(_pendingClaim, retry) || retry.Tcs.Task.IsCompleted) return; + BeginClaimRound(nextCandidate, retry.Tcs, retry.CtRegistration, scanStart); + }); return; } + _pendingClaim = null; pending.CtRegistration.Dispose(); WriteAddress(null); // TP channel goes back to placeholder 0xFE — no directed TP traffic reaches @@ -580,7 +605,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.CannotClaim, address: null, contendingSa: peerSa, contendingName: peerName); - AfterClaimBackoff(() => SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress)); + AfterClaimBackoff(SendCannotClaimIfStillUnclaimed); pending.Tcs.TrySetException(new J1939CannotClaimException(pending.PreferredAddress)); return; } @@ -609,7 +634,21 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _ = tcs.Task.ContinueWith(t => RaiseBackgroundException(t.Exception!.GetBaseException()), CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.Default); - AfterClaimBackoff(() => BeginClaimRound(nextCandidate, tcs, default, scanStart)); + // The lost address is the peer's from this instant: invalidated now, so no + // application traffic goes out under it during the backoff; only the next + // round waits. Registered as the claim in hand meanwhile, so a + // ClaimAddressAsync during the backoff meets the in-flight guard, and the + // delayed round runs only if it still is (Codex on #153). + WriteAddress(null); + RebindTransportOnLoop(J1939Pgn.NullAddress); + SetClaimState(J1939ClaimState.Claiming, nextCandidate, contendingSa: null, contendingName: null); + var unseated = new PendingClaim(nextCandidate, tcs, deadline: null, default) { ArbitraryScanStart = scanStart }; + _pendingClaim = unseated; + AfterClaimBackoff(() => + { + if (!ReferenceEquals(_pendingClaim, unseated) || tcs.Task.IsCompleted) return; + BeginClaimRound(nextCandidate, tcs, default, scanStart); + }); return; } // Broadcast Cannot-Claim and transition. @@ -617,7 +656,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.CannotClaim, address: null, contendingSa: peerSa, contendingName: peerName); - AfterClaimBackoff(() => SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress)); + AfterClaimBackoff(SendCannotClaimIfStillUnclaimed); } else { @@ -686,17 +725,17 @@ private async Task TransmitAddressClaimConfirmedAsync(uint canId, byte[] payload var ex = new J1939NodeException( $"J1939 address claim TX failed (id=0x{canId:X8}): {confirmation.FailureReason}."); try { _actor.Post(() => OnClaimAnnounceTxFailed(preferred, ex)); } - catch (ObjectDisposedException) { } + catch (ObjectDisposedException) { /* the node was disposed: no loop to tell */ } return; } try { _actor.Post(() => OnClaimAnnounceTxConfirmed(preferred)); } - catch (ObjectDisposedException) { } + catch (ObjectDisposedException) { /* the node was disposed: no loop to tell */ } } catch (Exception ex) { try { _actor.Post(() => OnClaimAnnounceTxFailed(preferred, ex)); } - catch (ObjectDisposedException) { } + catch (ObjectDisposedException) { /* the node was disposed: no loop to tell */ } } } diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index feddd36..428a904 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -356,6 +356,113 @@ public async Task CannotClaim_Is_Sent_After_The_Names_Pseudo_Random_Backoff() "the Cannot Claim waited the NAME's backoff after the claim it lost to"); } + // Codex and Bugbot on #153: an arbitrary-address node unseated from a claimed address loses + // the address the instant the winning claim is heard; only its next claim waits the + // backoff. Measured from the winner's claim frame to the node's Claiming transition: at + // once, against a NAME whose backoff is 150 ms -- the reading a node that invalidated only + // when the delayed round ran would give -- with 75 ms to either. + [Fact] + public async Task An_Unseated_Node_Loses_The_Address_At_Once_And_Waits_Only_To_Reclaim() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + const byte contended = 0x40; + + var ownerName = Name(0x000158); // backoff 150 ms + ((ownerName.ToBytes().Sum(b => (int)b) % 255) * 0.6).Should().Be(150); + using var owner = J1939Node.Open(busA, new J1939NodeOptions(ownerName) + { + ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80), + EnableArbitraryAddressClaiming = true, + }); + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) + { + ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80), + }); + await owner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + + long winnerClaimAt = 0; + var claiming = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + busA.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == contended && Interlocked.Read(ref winnerClaimAt) == 0 && !e.IsEcho) + Interlocked.Exchange(ref winnerClaimAt, Stopwatch.GetTimestamp()); + }; + owner.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.Claiming) claiming.TrySetResult(Stopwatch.GetTimestamp()); }; + + await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + var claimingAt = await claiming.Task.AsTaskWithTimeout(ShortTimeout); + owner.Address.Should().BeNull("the address is the winner's from the instant its claim was heard"); + Interlocked.Read(ref winnerClaimAt).Should().NotBe(0); + var reaction = TimeSpan.FromSeconds((claimingAt - Interlocked.Read(ref winnerClaimAt)) / (double)Stopwatch.Frequency); + reaction.Should().BeLessThan(TimeSpan.FromMilliseconds(75), "the invalidation does not wait the backoff"); + } + + // Codex and Bugbot on #153: a ClaimAddressAsync during the backoff before a re-claim meets + // the in-flight guard -- the claim in hand stays registered through the backoff -- and the + // re-claim completes for its caller. + [Fact] + public async Task A_Claim_During_The_Backoff_Before_A_Reclaim_Faults_And_The_Reclaim_Completes() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + const byte contended = 0x81; + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + using var node = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000158)) // backoff 150 ms + { + ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80), + EnableArbitraryAddressClaiming = true, + }); + + var backingOff = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + node.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.Claiming && e.Address == contended + 1) backingOff.TrySetResult(true); }; + + var first = node.ClaimAddressAsync(contended); // lost to the winner; the scan moves on after the backoff + await backingOff.Task.AsTaskWithTimeout(ShortTimeout); + Func second = () => node.ClaimAddressAsync(0x90).WithTimeout(ShortTimeout); + await second.Should().ThrowAsync(); + + await first.WithTimeout(ShortTimeout); + node.Address.Should().Be((byte)(contended + 1)); + } + + // Codex and Bugbot on #153: a loser that claims again before its delayed Cannot Claim went + // out does not have it go out -- the new claim's announcement is the newer word on the bus. + [Fact] + public async Task A_Delayed_Cannot_Claim_Is_Dropped_Once_A_New_Claim_Has_Started() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var busC = Open(session, 2); // spectator + + int cannotClaims = 0; + busC.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == J1939Pgn.NullAddress) Interlocked.Increment(ref cannotClaims); + }; + + using var winner = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + using var loser = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000158)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 150 ms + await winner.ClaimAddressAsync(0x60).WithTimeout(ShortTimeout); + + Func lost = () => loser.ClaimAddressAsync(0x60).WithTimeout(ShortTimeout); + await lost.Should().ThrowAsync(); + await loser.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); // within the backoff, and through the new arbitration + + await Task.Delay(300); // past the backoff, with room + Volatile.Read(ref cannotClaims).Should().Be(0, "the Cannot Claim was overtaken by the new claim"); + loser.Address.Should().Be(0x61); + } + // #58: a second ClaimAddressAsync while one is in arbitration faults instead of silently // cancelling the first, whose caller is waiting on it. [Fact] @@ -517,7 +624,10 @@ public async Task AddressClaim_ArbitraryFallback_ExhaustsField_ThenCannotClaim() using var busB = Open(session, 1); using var busC = Open(session, 2); // spectator for the final Cannot-Claim broadcast - var nodeName = Name(0x0000BB); + // A NAME whose §4.4.4.3 backoff is zero (bytes summed, modulo 255 == 0): the scan + // pays the backoff before every one of its 240 rounds, and this test's subject is the + // exhaustion, not the delay -- with a 56 ms backoff it took 13 s more (#153). + var nodeName = Name(0x00015D); var peerName = Name(0x000001); var opts = new J1939NodeOptions(nodeName) { From 99cc8099d28c8efe1c6fbb9559454c5ab0c75bb1 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:41:54 +0200 Subject: [PATCH 3/9] fix(j1939): tie a delayed Cannot Claim to its loss, and start a backing-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 --- src/CanKit.Pro.J1939/J1939NodeImpl.cs | 87 +++++++++++++------ .../TestCases/J1939/J1939NodeTests.cs | 84 ++++++++++++++++++ 2 files changed, 144 insertions(+), 27 deletions(-) diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index ae8afcf..04991d8 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -307,6 +307,10 @@ private void BeginClaim(byte preferredAddress, TaskCompletionSource tcs return; } + // A Cannot Claim still waiting its backoff is overtaken by this claim (Codex on #153). + _cannotClaimBackoff?.Dispose(); + _cannotClaimBackoff = null; + // A claim still in arbitration is not silently replaced: the caller who started it // is waiting on it, and a second one is a programming error (#58). A re-claim on a // node that holds an address, or holds none, is what this method is for. A pending @@ -345,20 +349,46 @@ private TimeSpan ClaimBackoff } } - // The delayed Cannot Claim of a lost arbitration: dropped if a new claim has started - // meanwhile -- its announcement is the newer word on the bus, and a Cannot Claim after it - // would retract it (Codex on #153). - private void SendCannotClaimIfStillUnclaimed() + // The delayed Cannot Claim of a lost arbitration, tied to the loss that scheduled it: a + // new claim cancels it -- its announcement is the newer word on the bus, and a Cannot + // Claim after it would retract it -- and a second loss reschedules a full backoff rather + // than inheriting the remainder of the first (Codex on #153, twice). The state check is + // the second line. + private IDeadline? _cannotClaimBackoff; + + private void ScheduleCannotClaim() { - if ((J1939ClaimState)Volatile.Read(ref _claimStateStore) != J1939ClaimState.CannotClaim) return; - SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + _cannotClaimBackoff?.Dispose(); + _cannotClaimBackoff = AfterClaimBackoff(() => + { + _cannotClaimBackoff = null; + if ((J1939ClaimState)Volatile.Read(ref _claimStateStore) != J1939ClaimState.CannotClaim) return; + SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + }); } - private void AfterClaimBackoff(Action onLoop) + private IDeadline? AfterClaimBackoff(Action onLoop) { var delay = ClaimBackoff; - if (delay <= TimeSpan.Zero) { onLoop(); return; } - _deadlines.Arm(delay, () => { if (_disposed == 0) onLoop(); }); + if (delay <= TimeSpan.Zero) { onLoop(); return null; } + return _deadlines.Arm(delay, () => { if (_disposed == 0) onLoop(); }); + } + + // Registers `retry` as the claim in hand and starts its round after the backoff; a Request + // for Address Claimed or a contest for the candidate starts it at once (Codex on #153). + private void BeginClaimRoundAfterBackoff(PendingClaim retry, byte candidate, byte? scanStart) + { + _pendingClaim = retry; + void Start() + { + if (!ReferenceEquals(_pendingClaim, retry) || retry.Tcs.Task.IsCompleted) return; + retry.StartRound = null; + retry.Deadline?.Dispose(); + retry.Deadline = null; + BeginClaimRound(candidate, retry.Tcs, retry.CtRegistration, scanStart); + } + retry.StartRound = Start; + retry.Deadline = AfterClaimBackoff(Start); } // Starts (or restarts, for the arbitrary-address fallback) a single arbitration round for @@ -588,12 +618,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) { ArbitraryScanStart = scanStart, }; - _pendingClaim = retry; // the next candidate is what is being claimed now - AfterClaimBackoff(() => - { - if (!ReferenceEquals(_pendingClaim, retry) || retry.Tcs.Task.IsCompleted) return; - BeginClaimRound(nextCandidate, retry.Tcs, retry.CtRegistration, scanStart); - }); + BeginClaimRoundAfterBackoff(retry, nextCandidate, scanStart); return; } @@ -605,14 +630,17 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.CannotClaim, address: null, contendingSa: peerSa, contendingName: peerName); - AfterClaimBackoff(SendCannotClaimIfStillUnclaimed); + ScheduleCannotClaim(); pending.Tcs.TrySetException(new J1939CannotClaimException(pending.PreferredAddress)); return; } // Peer's NAME is >= ours: they lose. Re-announce our own claim so they hear it, - // then keep waiting on our deadline. - SendAddressClaimFrame(sourceAddress: pending.PreferredAddress); + // then keep waiting on our deadline. A round still waiting its backoff has nothing + // to re-announce yet: it starts now, and its announcement is the answer (Codex on + // #153). + if (pending.BackingOff) pending.StartRound!(); + else SendAddressClaimFrame(sourceAddress: pending.PreferredAddress); return; } @@ -643,12 +671,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.Claiming, nextCandidate, contendingSa: null, contendingName: null); var unseated = new PendingClaim(nextCandidate, tcs, deadline: null, default) { ArbitraryScanStart = scanStart }; - _pendingClaim = unseated; - AfterClaimBackoff(() => - { - if (!ReferenceEquals(_pendingClaim, unseated) || tcs.Task.IsCompleted) return; - BeginClaimRound(nextCandidate, tcs, default, scanStart); - }); + BeginClaimRoundAfterBackoff(unseated, nextCandidate, scanStart); return; } // Broadcast Cannot-Claim and transition. @@ -656,7 +679,7 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.CannotClaim, address: null, contendingSa: peerSa, contendingName: peerName); - AfterClaimBackoff(SendCannotClaimIfStillUnclaimed); + ScheduleCannotClaim(); } else { @@ -675,8 +698,11 @@ private void AnswerRequestForAddressClaimed() break; case J1939ClaimState.Claiming when _pendingClaim is { } pending: // Arbitrating: the claim for the preferred address is the answer, and a peer - // that hears it contests it now rather than after the window (§4.4.3). - SendAddressClaimFrame(sourceAddress: pending.PreferredAddress); + // that hears it contests it now rather than after the window (§4.4.3). A round + // still waiting its backoff has announced nothing; the request is the moment to + // -- it starts the round, which announces (Codex on #153). + if (pending.BackingOff) pending.StartRound!(); + else SendAddressClaimFrame(sourceAddress: pending.PreferredAddress); break; default: SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); @@ -1339,6 +1365,13 @@ public PendingClaim(byte preferredAddress, TaskCompletionSource tcs, ID /// null while no fallback round has run yet. Carried across retries via /// BeginClaimRound. public byte? ArbitraryScanStart { get; set; } + /// + /// The round waits the §4.4.4.3 backoff and has announced nothing yet; + /// is the backoff, and starts the round -- now, when a Request for + /// Address Claimed or a contest for the candidate makes waiting pointless (Codex on #153). + /// + public Action? StartRound { get; set; } + public bool BackingOff => StartRound is not null; } /// diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index 428a904..98d64ac 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -463,6 +463,90 @@ public async Task A_Delayed_Cannot_Claim_Is_Dropped_Once_A_New_Claim_Has_Started loser.Address.Should().Be(0x61); } + // Codex on #153: the delayed Cannot Claim belongs to the loss that scheduled it. A second + // claim cancels the first's; when the second loses too, its Cannot Claim waits a full + // backoff from its own loss rather than the remainder of the first's -- measured from the + // second J1939CannotClaimException to the first SA 0xFE frame, a lower bound a loaded host + // only raises. + [Fact] + public async Task A_Second_Loss_Waits_Its_Own_Full_Backoff_Before_Cannot_Claim() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var busC = Open(session, 2); // spectator + + long cannotClaimAt = 0; + var cannotClaimSeen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + busC.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == J1939Pgn.NullAddress + && Interlocked.CompareExchange(ref cannotClaimAt, Stopwatch.GetTimestamp(), 0) == 0) + cannotClaimSeen.TrySetResult(true); + }; + + using var winner = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + using var loser = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000158)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 150 ms + await winner.ClaimAddressAsync(0x60).WithTimeout(ShortTimeout); + await Task.Delay(100); + await winner.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); // the winner holds 0x61 now; both losses are to it + + Func first = () => loser.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); + await first.Should().ThrowAsync(); + await Task.Delay(100); // most of the first backoff + Func second = () => loser.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); + await second.Should().ThrowAsync(); + var secondLossAt = Stopwatch.GetTimestamp(); + + await cannotClaimSeen.Task.AsTaskWithTimeout(ShortTimeout); + var gap = TimeSpan.FromSeconds((Interlocked.Read(ref cannotClaimAt) - secondLossAt) / (double)Stopwatch.Frequency); + gap.Should().BeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(145), + "the first loss's Cannot Claim was cancelled by the second claim, and the second loss waits its own backoff"); + } + + // Codex on #153: a round waiting its backoff has announced nothing, and answers a Request + // for Address Claimed by starting -- one announcement, now -- rather than by an + // announcement of its own with the round's to follow. + [Fact] + public async Task A_Request_During_The_Backoff_Starts_The_Round_With_A_Single_Announcement() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + const byte contended = 0x81; + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + using var node = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000158)) // backoff 150 ms + { + ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80), + EnableArbitraryAddressClaiming = true, + }); + + int candidateClaims = 0; + busB.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == contended + 1) Interlocked.Increment(ref candidateClaims); + }; + var backingOff = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + node.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.Claiming && e.Address == contended + 1) backingOff.TrySetResult(true); }; + + var claim = node.ClaimAddressAsync(contended); + await backingOff.Task.AsTaskWithTimeout(ShortTimeout); + busB.Transmit(CanFrame.Classic( + (int)J1939Id.ComposePgn(6, J1939Pgn.Request, sourceAddress: 0x20, destinationAddress: J1939Pgn.GlobalAddress), + new byte[] { 0x00, 0xEE, 0x00 }, isExtendedFrame: true)); + + await claim.WithTimeout(ShortTimeout); + node.Address.Should().Be((byte)(contended + 1)); + await Task.Delay(300); // past the backoff: a round that still fired would announce again + Volatile.Read(ref candidateClaims).Should().Be(1, "the request started the round, whose announcement is the answer, and nothing announced twice"); + } + // #58: a second ClaimAddressAsync while one is in arbitration faults instead of silently // cancelling the first, whose caller is waiting on it. [Fact] From 35c879935d8d51397a280ef17b70f6db7ed0f3c8 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:57:27 +0200 Subject: [PATCH 4/9] fix(j1939): take the claim backoff from the low byte of the NAME's sum, and read the lost address in the handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/CanKit.Pro.J1939/IJ1939Node.cs | 4 +-- src/CanKit.Pro.J1939/J1939NodeImpl.cs | 10 ++++--- src/CanKit.Pro.J1939/README.md | 4 +-- .../TestCases/J1939/J1939NodeTests.cs | 28 +++++++++++-------- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/CanKit.Pro.J1939/IJ1939Node.cs b/src/CanKit.Pro.J1939/IJ1939Node.cs index beecf0d..812541e 100644 --- a/src/CanKit.Pro.J1939/IJ1939Node.cs +++ b/src/CanKit.Pro.J1939/IJ1939Node.cs @@ -80,8 +80,8 @@ public interface IJ1939Node : IDisposable, IAsyncDisposable /// (SRS FR-J1939-004). /// /// The Cannot Claim, and an arbitrary-address node's next claim after losing, go out after - /// the pseudo-random 0..153 ms backoff of SAE J1939-81 §4.4.4.3 -- the NAME's bytes summed, - /// modulo 255, times 0.6 ms -- so two nodes colliding on an address do not answer in + /// the pseudo-random 0..153 ms backoff of SAE J1939-81 §4.4.4.3 -- the low byte of the + /// NAME's bytes summed, times 0.6 ms -- so two nodes colliding on an address do not answer in /// lockstep (#58). /// /// The preferred address was lost to a diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index 04991d8..bf98192 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -336,16 +336,18 @@ private void BeginClaim(byte preferredAddress, TaskCompletionSource tcs // J1939-81 §4.4.4.3: a node that lost arbitration delays its Cannot Claim, and an // arbitrary-address node its next claim, by a pseudo-random 0..153 ms derived from its - // NAME -- the eight bytes summed, modulo 255, times 0.6 ms -- so two nodes colliding on an - // address do not answer in lockstep for ever (#58). Fixed by the NAME, so a test can - // choose it; scheduled on the actor, so the state it acts on is the state it read. + // NAME -- the low byte of the eight bytes' sum, times 0.6 ms, so 255 slots reach the + // 153 ms endpoint (Codex on #153: modulo 255 mapped a sum of 255 to zero) -- so two nodes + // colliding on an address do not answer in lockstep for ever (#58). Fixed by the NAME, so + // a test can choose it; scheduled on the actor, so the state it acts on is the state it + // read. private TimeSpan ClaimBackoff { get { int sum = 0; foreach (var b in _name.ToBytes()) sum += b; - return TimeSpan.FromMilliseconds((sum % 255) * 0.6); + return TimeSpan.FromMilliseconds((sum & 0xFF) * 0.6); } } diff --git a/src/CanKit.Pro.J1939/README.md b/src/CanKit.Pro.J1939/README.md index 2a231ff..2ef04f1 100644 --- a/src/CanKit.Pro.J1939/README.md +++ b/src/CanKit.Pro.J1939/README.md @@ -25,8 +25,8 @@ FR-J1939-001..006 (Must) and FR-J1939-007 (Should). the NAME's Arbitrary Address Capable bit). A move after a successful claim is announced through `AddressClaimChanged`; nobody awaits it. The Cannot Claim, and the next claim after losing, go out after the pseudo-random - 0..153 ms backoff of SAE J1939-81 §4.4.4.3 (the NAME's bytes summed, modulo - 255, times 0.6 ms), so two nodes colliding on an address do not answer in + 0..153 ms backoff of SAE J1939-81 §4.4.4.3 (the low byte of the NAME's bytes + summed, times 0.6 ms), so two nodes colliding on an address do not answer in lockstep; and a second `ClaimAddressAsync` while one is in arbitration faults with `InvalidOperationException` rather than silently cancelling the first (#58). - **Request for Address Claimed** (SAE J1939-81 §4.2.2): a Request for PGN diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index 98d64ac..4a3dd18 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -307,9 +307,10 @@ public async Task CannotClaim_BroadcastsWithNullSourceAddress() } // #58 (SAE J1939-81 §4.4.4.3): a node that lost arbitration sends its Cannot Claim after a - // pseudo-random 0..153 ms derived from its NAME -- the bytes summed, modulo 255, times + // pseudo-random 0..153 ms derived from its NAME -- the low byte of the bytes' sum, times // 0.6 ms -- so two nodes colliding on an address do not answer in lockstep. The loser's - // NAME here gives 78 ms; the gap between the winner's re-announcement, which is what the + // NAME here sums to exactly 255, the 153 ms endpoint that a modulo 255 would have mapped + // to zero (Codex on #153); the gap between the winner's re-announcement, which is what the // loser answers, and the Cannot Claim is at least that, a lower bound a loaded host only // raises. [Fact] @@ -337,9 +338,9 @@ public async Task CannotClaim_Is_Sent_After_The_Names_Pseudo_Random_Backoff() } }; - var loserName = Name(0x0000E0); // byte sum mod 255 = 130: 78 ms - var loserBackoff = TimeSpan.FromMilliseconds((loserName.ToBytes().Sum(b => (int)b) % 255) * 0.6); - loserBackoff.Should().Be(TimeSpan.FromMilliseconds(78), "the NAME was chosen for it"); + var loserName = Name(0x00015D); // byte sum 255: the 153 ms endpoint + loserName.ToBytes().Sum(b => (int)b).Should().Be(255, "the NAME was chosen for it"); + var loserBackoff = TimeSpan.FromMilliseconds(153); using var winner = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(200) }); using var loser = J1939Node.Open(busB, new J1939NodeOptions(loserName) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(200) }); @@ -370,7 +371,7 @@ public async Task An_Unseated_Node_Loses_The_Address_At_Once_And_Waits_Only_To_R const byte contended = 0x40; var ownerName = Name(0x000158); // backoff 150 ms - ((ownerName.ToBytes().Sum(b => (int)b) % 255) * 0.6).Should().Be(150); + ((ownerName.ToBytes().Sum(b => (int)b) & 0xFF) * 0.6).Should().Be(150); using var owner = J1939Node.Open(busA, new J1939NodeOptions(ownerName) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80), @@ -383,7 +384,10 @@ public async Task An_Unseated_Node_Loses_The_Address_At_Once_And_Waits_Only_To_R await owner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); long winnerClaimAt = 0; - var claiming = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + // What the handler reads is read in the actor's step that made the transition; read + // after the await, the address could be the re-claimed one already on a host that + // schedules the continuation late (macOS CI on #153). + var claiming = new TaskCompletionSource<(long At, byte? Address)>(TaskCreationOptions.RunContinuationsAsynchronously); busA.FrameObserved += (_, e) => { if (!e.CanFrame.IsExtendedFrame) return; @@ -391,11 +395,11 @@ public async Task An_Unseated_Node_Loses_The_Address_At_Once_And_Waits_Only_To_R if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == contended && Interlocked.Read(ref winnerClaimAt) == 0 && !e.IsEcho) Interlocked.Exchange(ref winnerClaimAt, Stopwatch.GetTimestamp()); }; - owner.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.Claiming) claiming.TrySetResult(Stopwatch.GetTimestamp()); }; + owner.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.Claiming) claiming.TrySetResult((Stopwatch.GetTimestamp(), owner.Address)); }; await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); - var claimingAt = await claiming.Task.AsTaskWithTimeout(ShortTimeout); - owner.Address.Should().BeNull("the address is the winner's from the instant its claim was heard"); + var (claimingAt, addressThen) = await claiming.Task.AsTaskWithTimeout(ShortTimeout); + addressThen.Should().BeNull("the address is the winner's from the instant its claim was heard"); Interlocked.Read(ref winnerClaimAt).Should().NotBe(0); var reaction = TimeSpan.FromSeconds((claimingAt - Interlocked.Read(ref winnerClaimAt)) / (double)Stopwatch.Frequency); reaction.Should().BeLessThan(TimeSpan.FromMilliseconds(75), "the invalidation does not wait the backoff"); @@ -708,10 +712,10 @@ public async Task AddressClaim_ArbitraryFallback_ExhaustsField_ThenCannotClaim() using var busB = Open(session, 1); using var busC = Open(session, 2); // spectator for the final Cannot-Claim broadcast - // A NAME whose §4.4.4.3 backoff is zero (bytes summed, modulo 255 == 0): the scan + // A NAME whose §4.4.4.3 backoff is zero (the bytes sum to 256, low byte 0): the scan // pays the backoff before every one of its 240 rounds, and this test's subject is the // exhaustion, not the delay -- with a 56 ms backoff it took 13 s more (#153). - var nodeName = Name(0x00015D); + var nodeName = Name(0x00005F); var peerName = Name(0x000001); var opts = new J1939NodeOptions(nodeName) { From fe277dad5cef2f1b6c680be26c1e2655202ad746 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:03:45 +0200 Subject: [PATCH 5/9] fix(j1939): answer a Request for Address Claimed with the Cannot Claim 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 --- src/CanKit.Pro.J1939/J1939NodeImpl.cs | 33 +++++++++++---- src/CanKit.Pro.J1939/README.md | 7 +++- .../TestCases/J1939/J1939NodeTests.cs | 42 +++++++++++++++++++ 3 files changed, 73 insertions(+), 9 deletions(-) diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index bf98192..c79a876 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -361,12 +361,31 @@ private TimeSpan ClaimBackoff private void ScheduleCannotClaim() { _cannotClaimBackoff?.Dispose(); - _cannotClaimBackoff = AfterClaimBackoff(() => - { - _cannotClaimBackoff = null; - if ((J1939ClaimState)Volatile.Read(ref _claimStateStore) != J1939ClaimState.CannotClaim) return; - SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); - }); + _cannotClaimBackoff = AfterClaimBackoff(SendCannotClaimIfStillDue); + } + + // The answer a node without an address owes a Request for Address Claimed waits the same + // backoff, and for a reason the loss path only shares: a Cannot Claim carries the null + // address, so two nodes answering one global request at the same instant put identical + // CAN IDs with different NAMEs on the bus, which arbitration cannot separate (SAE + // J1939-81 §4.4.4.3; Codex on #153). An answer already waiting is the answer -- sending + // now would both bypass that delay and make the armed one a second copy. + private void AnswerRequestWithCannotClaim() + { + if (_cannotClaimBackoff is not null) return; + _cannotClaimBackoff = AfterClaimBackoff(SendCannotClaimIfStillDue); + } + + private void SendCannotClaimIfStillDue() + { + _cannotClaimBackoff = null; + // Claimed: the claim is the newer word and a Cannot Claim would retract it. Claiming: + // the round in hand announces, and a claim overtook this one (Codex on #153). Neither + // is reachable without the arming path having disposed the handle; this is the second + // line. NotClaimed passes, because that is a node that never claimed answering a scan. + var state = (J1939ClaimState)Volatile.Read(ref _claimStateStore); + if (state is J1939ClaimState.Claimed or J1939ClaimState.Claiming) return; + SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); } private IDeadline? AfterClaimBackoff(Action onLoop) @@ -707,7 +726,7 @@ private void AnswerRequestForAddressClaimed() else SendAddressClaimFrame(sourceAddress: pending.PreferredAddress); break; default: - SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + AnswerRequestWithCannotClaim(); break; } } diff --git a/src/CanKit.Pro.J1939/README.md b/src/CanKit.Pro.J1939/README.md index 2ef04f1..0b958e3 100644 --- a/src/CanKit.Pro.J1939/README.md +++ b/src/CanKit.Pro.J1939/README.md @@ -32,8 +32,11 @@ FR-J1939-001..006 (Must) and FR-J1939-007 (Should). - **Request for Address Claimed** (SAE J1939-81 §4.2.2): a Request for PGN 0xEE00 is answered by the node itself — with its Address Claimed while it holds or arbitrates an address, with Cannot Claim while it holds none — so - a network-management tool scanning the bus sees it. The request still - reaches `MessageReceived`. + a network-management tool scanning the bus sees it. A Cannot Claim answer + waits the §4.4.4.3 backoff below and shares it with the one a lost claim + owes, since every Cannot Claim carries the same null source address; a + claim still waiting its backoff answers by starting its round. The request + still reaches `MessageReceived`. - **Request-PGN** (PGN 0xEA00) send and receive (**FR-J1939-005**). - **Auto-routing** to J1939-TP for payloads > 8 bytes; direct 29-bit frames for payloads ≤ 8 bytes (**FR-J1939-006**). diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index 4a3dd18..961b4d1 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -551,6 +551,48 @@ public async Task A_Request_During_The_Backoff_Starts_The_Round_With_A_Single_An Volatile.Read(ref candidateClaims).Should().Be(1, "the request started the round, whose announcement is the answer, and nothing announced twice"); } + // Codex on #153: a Cannot Claim carries the null address, so two nodes answering the same + // global Request for Address Claimed at the same instant put identical CAN IDs with + // different NAMEs on the bus. The answer a losing node owes therefore waits the same + // §4.4.4.3 backoff as the Cannot Claim of the loss itself -- and a request arriving inside + // that backoff is answered by that one frame, not by an immediate second copy. + [Fact] + public async Task A_Request_During_The_Cannot_Claim_Backoff_Is_Answered_By_That_One_Frame() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + const byte contended = 0x62; + + int cannotClaims = 0; + long firstCannotClaimAt = 0; + busB.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (!J1939Pgn.IsAddressClaim(fields.Pgn) || fields.SourceAddress != J1939Pgn.NullAddress) return; + Interlocked.CompareExchange(ref firstCannotClaimAt, Stopwatch.GetTimestamp(), 0); + Interlocked.Increment(ref cannotClaims); + }; + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + using var loser = J1939Node.Open(busA, new J1939NodeOptions(Name(0x00015D)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 153 ms + + Func act = () => loser.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + await act.Should().ThrowAsync(); + var lostAt = Stopwatch.GetTimestamp(); // the loss is known; its Cannot Claim is waiting + busB.Transmit(CanFrame.Classic( + (int)J1939Id.ComposePgn(6, J1939Pgn.Request, sourceAddress: 0x20, destinationAddress: J1939Pgn.GlobalAddress), + new byte[] { 0x00, 0xEE, 0x00 }, isExtendedFrame: true)); + + await Task.Delay(500); // past the backoff, with room for a second copy to show up + Volatile.Read(ref cannotClaims).Should().Be(1, "the answer already waiting is the answer to the request"); + var waited = TimeSpan.FromSeconds((Interlocked.Read(ref firstCannotClaimAt) - lostAt) / (double)Stopwatch.Frequency); + waited.Should().BeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(100), + "the request did not shortcut the backoff -- a lower bound on the 153 ms a loaded host only lengthens"); + } + // #58: a second ClaimAddressAsync while one is in arbitration faults instead of silently // cancelling the first, whose caller is waiting on it. [Fact] From 059c42d33c53c8011c169e03a2f863ca78a65775 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Wed, 23 Sep 2026 06:04:37 +0200 Subject: [PATCH 6/9] fix(j1939): fault a lost claim only once its Cannot Claim is on the bus 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 --- src/CanKit.Pro.J1939/IJ1939Node.cs | 4 +- src/CanKit.Pro.J1939/J1939NodeImpl.cs | 30 +++++- src/CanKit.Pro.J1939/README.md | 4 +- .../TestCases/J1939/J1939NodeTests.cs | 95 ++++++++++++++++--- 4 files changed, 115 insertions(+), 18 deletions(-) diff --git a/src/CanKit.Pro.J1939/IJ1939Node.cs b/src/CanKit.Pro.J1939/IJ1939Node.cs index 812541e..837444b 100644 --- a/src/CanKit.Pro.J1939/IJ1939Node.cs +++ b/src/CanKit.Pro.J1939/IJ1939Node.cs @@ -82,7 +82,9 @@ public interface IJ1939Node : IDisposable, IAsyncDisposable /// The Cannot Claim, and an arbitrary-address node's next claim after losing, go out after /// the pseudo-random 0..153 ms backoff of SAE J1939-81 §4.4.4.3 -- the low byte of the /// NAME's bytes summed, times 0.6 ms -- so two nodes colliding on an address do not answer in - /// lockstep (#58). + /// lockstep (#58). The task faults after that Cannot Claim has gone out, in the order step + /// 3 gives, so disposing the node on the exception cannot suppress the frame; a further + /// claim started before it drops the Cannot Claim and faults this one at once. /// /// The preferred address was lost to a /// higher-priority NAME and no fallback was available. diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index c79a876..c6ae575 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -308,8 +308,11 @@ private void BeginClaim(byte preferredAddress, TaskCompletionSource tcs } // A Cannot Claim still waiting its backoff is overtaken by this claim (Codex on #153). + // Dropping it settles the loss that scheduled it just as finally as sending it would, + // so the caller waiting on that loss is answered here. _cannotClaimBackoff?.Dispose(); _cannotClaimBackoff = null; + CompleteLostClaim(); // A claim still in arbitration is not silently replaced: the caller who started it // is waiting on it, and a second one is a programming error (#58). A re-claim on a @@ -384,8 +387,24 @@ private void SendCannotClaimIfStillDue() // is reachable without the arming path having disposed the handle; this is the second // line. NotClaimed passes, because that is a node that never claimed answering a scan. var state = (J1939ClaimState)Volatile.Read(ref _claimStateStore); - if (state is J1939ClaimState.Claimed or J1939ClaimState.Claiming) return; - SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + if (state is not (J1939ClaimState.Claimed or J1939ClaimState.Claiming)) + SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + CompleteLostClaim(); + } + + // The claim of a loss that owes the bus a Cannot Claim: it faults once that frame has gone + // out, not when the loss became known. A caller that disposes the node on the exception -- + // a `using` scope ending on it -- would otherwise tear the actor down inside the backoff, + // and the frame the loss owes would never be sent at all (Codex on #153). + private PendingClaim? _lostClaim; + + private void CompleteLostClaim() + { + var lost = _lostClaim; + if (lost is null) return; + _lostClaim = null; + lost.CtRegistration.Dispose(); + lost.Tcs.TrySetException(new J1939CannotClaimException(lost.PreferredAddress)); } private IDeadline? AfterClaimBackoff(Action onLoop) @@ -644,15 +663,15 @@ private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) } _pendingClaim = null; - pending.CtRegistration.Dispose(); WriteAddress(null); // TP channel goes back to placeholder 0xFE — no directed TP traffic reaches // us while unclaimed. RebindTransportOnLoop(J1939Pgn.NullAddress); SetClaimState(J1939ClaimState.CannotClaim, address: null, contendingSa: peerSa, contendingName: peerName); + // The caller is answered by the backoff, once the Cannot Claim is on the bus. + _lostClaim = pending; ScheduleCannotClaim(); - pending.Tcs.TrySetException(new J1939CannotClaimException(pending.PreferredAddress)); return; } @@ -1295,6 +1314,9 @@ public void Dispose() { _actor.Post(() => { + // A loss whose Cannot Claim never got its backoff still answers its caller: + // the claim failed, and no dispose makes that less true (Codex on #153). + CompleteLostClaim(); var pending = _pendingClaim; if (pending is not null) { diff --git a/src/CanKit.Pro.J1939/README.md b/src/CanKit.Pro.J1939/README.md index 0b958e3..7e6aa9e 100644 --- a/src/CanKit.Pro.J1939/README.md +++ b/src/CanKit.Pro.J1939/README.md @@ -36,7 +36,9 @@ FR-J1939-001..006 (Must) and FR-J1939-007 (Should). waits the §4.4.4.3 backoff below and shares it with the one a lost claim owes, since every Cannot Claim carries the same null source address; a claim still waiting its backoff answers by starting its round. The request - still reaches `MessageReceived`. + still reaches `MessageReceived`. `ClaimAddressAsync` faults only once the + Cannot Claim it owes has gone out, so a caller that disposes the node on the + exception cannot suppress it. - **Request-PGN** (PGN 0xEA00) send and receive (**FR-J1939-005**). - **Auto-routing** to J1939-TP for payloads > 8 bytes; direct 29-bit frames for payloads ≤ 8 bytes (**FR-J1939-006**). diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index 961b4d1..15e9b95 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -458,10 +458,18 @@ public async Task A_Delayed_Cannot_Claim_Is_Dropped_Once_A_New_Claim_Has_Started using var loser = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000158)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 150 ms await winner.ClaimAddressAsync(0x60).WithTimeout(ShortTimeout); - Func lost = () => loser.ClaimAddressAsync(0x60).WithTimeout(ShortTimeout); - await lost.Should().ThrowAsync(); + var lossSeen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + loser.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.CannotClaim) lossSeen.TrySetResult(true); }; + + // The claim faults only once its Cannot Claim is on the bus, so the loss is awaited + // through the state change here -- the exception would come too late to claim inside + // the backoff it is waiting for. + var lost = loser.ClaimAddressAsync(0x60); + await lossSeen.Task.AsTaskWithTimeout(ShortTimeout); await loser.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); // within the backoff, and through the new arbitration + Func awaitLost = () => lost.WithTimeout(ShortTimeout); + await awaitLost.Should().ThrowAsync("dropping the Cannot Claim settles the loss that owed it"); await Task.Delay(300); // past the backoff, with room Volatile.Read(ref cannotClaims).Should().Be(0, "the Cannot Claim was overtaken by the new claim"); loser.Address.Should().Be(0x61); @@ -470,8 +478,8 @@ public async Task A_Delayed_Cannot_Claim_Is_Dropped_Once_A_New_Claim_Has_Started // Codex on #153: the delayed Cannot Claim belongs to the loss that scheduled it. A second // claim cancels the first's; when the second loses too, its Cannot Claim waits a full // backoff from its own loss rather than the remainder of the first's -- measured from the - // second J1939CannotClaimException to the first SA 0xFE frame, a lower bound a loaded host - // only raises. + // second transition to CannotClaim, which is the instant the second loss is known, to the + // one SA 0xFE frame, a lower bound a loaded host only raises. [Fact] public async Task A_Second_Loss_Waits_Its_Own_Full_Backoff_Before_Cannot_Claim() { @@ -497,15 +505,27 @@ public async Task A_Second_Loss_Waits_Its_Own_Full_Backoff_Before_Cannot_Claim() await Task.Delay(100); await winner.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); // the winner holds 0x61 now; both losses are to it - Func first = () => loser.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); - await first.Should().ThrowAsync(); + int losses = 0; + long secondLossAt = 0; + var lossSeen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + loser.AddressClaimChanged += (_, e) => + { + if (e.State != J1939ClaimState.CannotClaim) return; + if (Interlocked.Increment(ref losses) == 2) Interlocked.Exchange(ref secondLossAt, Stopwatch.GetTimestamp()); + lossSeen.TrySetResult(true); + }; + + var first = loser.ClaimAddressAsync(0x61); // faults when its Cannot Claim goes out, or is dropped + await lossSeen.Task.AsTaskWithTimeout(ShortTimeout); await Task.Delay(100); // most of the first backoff Func second = () => loser.ClaimAddressAsync(0x61).WithTimeout(ShortTimeout); await second.Should().ThrowAsync(); - var secondLossAt = Stopwatch.GetTimestamp(); + Func awaitFirst = () => first.WithTimeout(ShortTimeout); + await awaitFirst.Should().ThrowAsync(); + Interlocked.Read(ref secondLossAt).Should().NotBe(0, "the second claim lost too"); await cannotClaimSeen.Task.AsTaskWithTimeout(ShortTimeout); - var gap = TimeSpan.FromSeconds((Interlocked.Read(ref cannotClaimAt) - secondLossAt) / (double)Stopwatch.Frequency); + var gap = TimeSpan.FromSeconds((Interlocked.Read(ref cannotClaimAt) - Interlocked.Read(ref secondLossAt)) / (double)Stopwatch.Frequency); gap.Should().BeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(145), "the first loss's Cannot Claim was cancelled by the second claim, and the second loss waits its own backoff"); } @@ -551,6 +571,45 @@ public async Task A_Request_During_The_Backoff_Starts_The_Round_With_A_Single_An Volatile.Read(ref candidateClaims).Should().Be(1, "the request started the round, whose announcement is the answer, and nothing announced twice"); } + // Codex on #153: the Cannot Claim of a loss is delayed, the exception was not, and a caller + // that ends a `using` scope on that exception disposed the node inside the backoff -- the + // frame the loss owed the bus was then never sent at all. ClaimAddressAsync now faults + // once the frame has gone out, so the loser here disposes as soon as it can and the + // spectator still sees it. + [Fact] + public async Task A_Lost_Claim_Faults_Only_Once_Its_Cannot_Claim_Is_On_The_Bus() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var busC = Open(session, 2); // spectator: it outlives the loser + + int cannotClaims = 0; + busC.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == J1939Pgn.NullAddress) Interlocked.Increment(ref cannotClaims); + }; + + using var winner = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); + var loser = J1939Node.Open(busB, new J1939NodeOptions(Name(0x00015D)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 153 ms + try + { + Func act = () => loser.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); + await act.Should().ThrowAsync(); + } + finally + { + loser.Dispose(); // the scope a caller ends on the exception + } + + await Task.Delay(300); // a backoff that survived the dispose would have fired by now + Volatile.Read(ref cannotClaims).Should().Be(1, + "the claim faulted only after its Cannot Claim went out, so disposing on the exception cannot suppress it"); + } + // Codex on #153: a Cannot Claim carries the null address, so two nodes answering the same // global Request for Address Claimed at the same instant put identical CAN IDs with // different NAMEs on the bus. The answer a losing node owes therefore waits the same @@ -579,16 +638,28 @@ public async Task A_Request_During_The_Cannot_Claim_Backoff_Is_Answered_By_That_ await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); using var loser = J1939Node.Open(busA, new J1939NodeOptions(Name(0x00015D)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 153 ms - Func act = () => loser.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); - await act.Should().ThrowAsync(); - var lostAt = Stopwatch.GetTimestamp(); // the loss is known; its Cannot Claim is waiting + long lostAt = 0; + var lossSeen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + loser.AddressClaimChanged += (_, e) => + { + if (e.State != J1939ClaimState.CannotClaim) return; + Interlocked.CompareExchange(ref lostAt, Stopwatch.GetTimestamp(), 0); + lossSeen.TrySetResult(true); + }; + + // The claim's exception arrives with the Cannot Claim itself, so the request has to be + // sent from the loss, which is the instant the backoff starts. + var lost = loser.ClaimAddressAsync(contended); + await lossSeen.Task.AsTaskWithTimeout(ShortTimeout); busB.Transmit(CanFrame.Classic( (int)J1939Id.ComposePgn(6, J1939Pgn.Request, sourceAddress: 0x20, destinationAddress: J1939Pgn.GlobalAddress), new byte[] { 0x00, 0xEE, 0x00 }, isExtendedFrame: true)); + Func awaitLost = () => lost.WithTimeout(ShortTimeout); + await awaitLost.Should().ThrowAsync(); await Task.Delay(500); // past the backoff, with room for a second copy to show up Volatile.Read(ref cannotClaims).Should().Be(1, "the answer already waiting is the answer to the request"); - var waited = TimeSpan.FromSeconds((Interlocked.Read(ref firstCannotClaimAt) - lostAt) / (double)Stopwatch.Frequency); + var waited = TimeSpan.FromSeconds((Interlocked.Read(ref firstCannotClaimAt) - Interlocked.Read(ref lostAt)) / (double)Stopwatch.Frequency); waited.Should().BeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(100), "the request did not shortcut the backoff -- a lower bound on the 153 ms a loaded host only lengthens"); } From 207fa04bfe5fbbcdb79fab49d1b46b9da522f229 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:39:42 +0200 Subject: [PATCH 7/9] test(j1939): cover the address-claim paths Codecov left open 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 --- .../TestCases/J1939/J1939NodeTests.cs | 452 +++++++++++++++++- 1 file changed, 445 insertions(+), 7 deletions(-) diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index 15e9b95..ae493fc 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -594,16 +594,11 @@ public async Task A_Lost_Claim_Faults_Only_Once_Its_Cannot_Claim_Is_On_The_Bus() using var winner = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); await winner.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); - var loser = J1939Node.Open(busB, new J1939NodeOptions(Name(0x00015D)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 153 ms - try + using (var loser = J1939Node.Open(busB, new J1939NodeOptions(Name(0x00015D)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) })) // backoff 153 ms { Func act = () => loser.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); await act.Should().ThrowAsync(); - } - finally - { - loser.Dispose(); // the scope a caller ends on the exception - } + } // the using a caller ends on the exception await Task.Delay(300); // a backoff that survived the dispose would have fired by now Volatile.Read(ref cannotClaims).Should().Be(1, @@ -682,6 +677,322 @@ public async Task A_Second_Claim_During_Arbitration_Faults_And_Leaves_The_First_ node.ClaimState.Should().Be(J1939ClaimState.Claimed); } + // The in-flight guard's other arm: a claim whose caller already cancelled it, whose cancel + // has not yet been applied on the loop, is over. Claiming again sweeps it instead of + // faulting. Both posts are made from the loop, so the new claim is queued ahead of the + // cancel and runs while the cancelled task is already complete and the announce has not + // been confirmed -- the pending claim's deadline is still null. + [Fact] + public async Task A_Cancelled_Claim_Not_Yet_Confirmed_Is_Swept_When_Claiming_Again() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var raw = new CanBusService(busA); + using var scripted = new ScriptedClaimBus(raw, ScriptedClaimBus.Script.HoldFirstClaim); + using var node = J1939Node.Open(scripted, new J1939NodeOptions(Name(1)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + using var cts = new CancellationTokenSource(); + + var first = node.ClaimAddressAsync(0x10, cts.Token); + await scripted.Held.AsTaskWithTimeout(ShortTimeout); + + var ran = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + Task? second = null; + node.MessageReceived += (_, msg) => + { + if (msg.Pgn != 0xFEE9u || second is not null) return; + second = node.ClaimAddressAsync(0x11); // queued ahead of the cancel + cts.Cancel(); + ran.TrySetResult(true); + }; + busB.Transmit(CanFrame.Classic( + (int)J1939Id.ComposePgn(6, 0xFEE9u, sourceAddress: 0x22), + new byte[] { 0x11, 0x22 }, isExtendedFrame: true)); + + await ran.Task.AsTaskWithTimeout(ShortTimeout); + await second!.WithTimeout(ShortTimeout); + node.Address.Should().Be(0x11); + Func awaitFirst = () => first.WithTimeout(ShortTimeout); + await awaitFirst.Should().ThrowAsync(); + scripted.ReleaseConfirmed(); + } + + // Same sweep once the cancelled claim is the one waiting out a re-claim backoff, so the + // deadline in hand is the backoff timer rather than null. + [Fact] + public async Task A_Cancelled_Claim_Waiting_Out_Its_Backoff_Is_Swept_When_Claiming_Again() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + const byte contended = 0x81; + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + using var node = J1939Node.Open(busA, new J1939NodeOptions(Name(0x000158)) + { + ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80), + EnableArbitraryAddressClaiming = true, + }); + using var cts = new CancellationTokenSource(); + + var backingOff = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var ran = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + Task? second = null; + node.AddressClaimChanged += (_, e) => + { + if (e.State == J1939ClaimState.Claiming && e.Address == contended + 1) + backingOff.TrySetResult(true); + }; + node.MessageReceived += (_, msg) => + { + if (msg.Pgn != 0xFEE9u || second is not null) return; + second = node.ClaimAddressAsync(0x40); + cts.Cancel(); + ran.TrySetResult(true); + }; + + var first = node.ClaimAddressAsync(contended, cts.Token); + await backingOff.Task.AsTaskWithTimeout(ShortTimeout); + // The Claiming transition is raised before the backoff claim is registered. The frame + // is handled on a later turn, which is the first moment the deadline is the backoff. + busB.Transmit(CanFrame.Classic( + (int)J1939Id.ComposePgn(6, 0xFEE9u, sourceAddress: 0x22), + new byte[] { 0x11, 0x22 }, isExtendedFrame: true)); + + await ran.Task.AsTaskWithTimeout(ShortTimeout); + await second!.WithTimeout(ShortTimeout); + node.Address.Should().Be(0x40); + Func awaitFirst = () => first.WithTimeout(ShortTimeout); + await awaitFirst.Should().ThrowAsync(); + } + + // A Request for Address Claimed while a round is in arbitration -- announced, not waiting + // out a backoff -- is answered by sending that claim again. + [Fact] + public async Task A_Request_During_Arbitration_Is_Answered_By_Reannouncing_The_Claim() + { + using var clock = new VirtualClock(); + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var service = new CanBusService(busA); + var actor = clock.NewActor(); + var timeout = TimeSpan.FromMilliseconds(200); + using var node = new J1939NodeImpl(service, new J1939NodeOptions(Name(1)) { ClaimAnnounceTimeout = timeout }, ownsService: false, actor); + + const byte preferred = 0x40; + int claims = 0; + var first = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var second = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + busB.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (!J1939Pgn.IsAddressClaim(fields.Pgn) || fields.SourceAddress != preferred) return; + var n = Interlocked.Increment(ref claims); + if (n == 1) first.TrySetResult(true); + if (n == 2) second.TrySetResult(true); + }; + + var claim = node.ClaimAddressAsync(preferred); + await first.Task.AsTaskWithTimeout(ShortTimeout); + await clock.WaitUntilTimerArmedAsync(actor, timeout, ShortTimeout); + busB.Transmit(CanFrame.Classic( + (int)J1939Id.ComposePgn(6, J1939Pgn.Request, sourceAddress: 0x20, destinationAddress: J1939Pgn.GlobalAddress), + new byte[] { 0x00, 0xEE, 0x00 }, isExtendedFrame: true)); + await second.Task.AsTaskWithTimeout(ShortTimeout); + Volatile.Read(ref claims).Should().Be(2, "the request re-announced the claim already in arbitration"); + + await clock.AdvanceAsync(timeout); + await claim.WithTimeout(ShortTimeout); + node.Address.Should().Be(preferred); + } + + // A peer that claims the candidate we are backing off toward, and loses the NAME comparison, + // starts that round now. The announcement is on the bus while the backoff timer is still + // armed, which a round that waited the backoff out would not have sent yet. + [Fact] + public async Task A_Weaker_Claim_During_The_Backoff_Starts_The_Round() + { + using var clock = new VirtualClock(); + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var service = new CanBusService(busA); + var actor = clock.NewActor(); + var backoff = TimeSpan.FromMilliseconds(150); + const byte contended = 0x81; + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(contended).WithTimeout(ShortTimeout); + + var nodeName = Name(0x000158); // backoff 150 ms + using var node = new J1939NodeImpl(service, new J1939NodeOptions(nodeName) + { + ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80), + EnableArbitraryAddressClaiming = true, + }, ownsService: false, actor); + + var backingOff = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + node.AddressClaimChanged += (_, e) => + { + if (e.State == J1939ClaimState.Claiming && e.Address == contended + 1) backingOff.TrySetResult(true); + }; + var announced = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + busB.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == contended + 1 + && e.CanFrame.Data.Length >= 8 + && J1939Name.FromBytes(e.CanFrame.Data.ToArray()).Value == nodeName.Value) + announced.TrySetResult(true); + }; + + var claim = node.ClaimAddressAsync(contended); + await backingOff.Task.AsTaskWithTimeout(ShortTimeout); + await clock.WaitUntilTimerArmedAsync(actor, backoff, ShortTimeout); + + var weaker = Name(0x000400); // numerically higher: loses to nodeName + busB.Transmit(CanFrame.Classic( + (int)J1939Id.ComposePgn(6, J1939Pgn.AddressClaimed, sourceAddress: (byte)(contended + 1), destinationAddress: J1939Pgn.GlobalAddress), + weaker.ToBytes(), isExtendedFrame: true)); + + await announced.Task.AsTaskWithTimeout(ShortTimeout); + await clock.WaitUntilTimerArmedAsync(actor, TimeSpan.FromMilliseconds(80), ShortTimeout); + await clock.AdvanceAsync(TimeSpan.FromMilliseconds(80)); + await claim.WithTimeout(ShortTimeout); + node.Address.Should().Be((byte)(contended + 1)); + } + + // Disposing during the backoff leaves the timer armed on an injected actor. When it fires, + // the node is already disposed and the Cannot Claim is not sent. + [Fact] + public async Task A_Backoff_That_Fires_After_Dispose_Sends_No_Cannot_Claim() + { + using var clock = new VirtualClock(); + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var busC = Open(session, 2); + using var service = new CanBusService(busA); + var actor = clock.NewActor(); + var backoff = TimeSpan.FromMilliseconds(150); + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); + + int cannotClaims = 0; + busC.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn) && fields.SourceAddress == J1939Pgn.NullAddress) + Interlocked.Increment(ref cannotClaims); + }; + + var node = new J1939NodeImpl(service, new J1939NodeOptions(Name(0x000158)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }, ownsService: false, actor); + try + { + var lost = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + node.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.CannotClaim) lost.TrySetResult(true); }; + var claim = node.ClaimAddressAsync(0x63); + await lost.Task.AsTaskWithTimeout(ShortTimeout); + await clock.WaitUntilTimerArmedAsync(actor, backoff, ShortTimeout); + + node.Dispose(); + await clock.AdvanceAsync(backoff); + Volatile.Read(ref cannotClaims).Should().Be(0, "a backoff that fires after dispose does not send the Cannot Claim"); + Func awaitClaim = () => claim.WithTimeout(ShortTimeout); + await awaitClaim.Should().ThrowAsync(); + } + finally + { + node.Dispose(); + } + } + + // The announce's confirmation is a failure: the claim faults and the address is not taken. + [Fact] + public async Task A_Rejected_Address_Claim_Transmit_Faults_The_Claim() + { + var session = NewSession(); + using var bus = Open(session, 0); + using var raw = new CanBusService(bus); + using var scripted = new ScriptedClaimBus(raw, ScriptedClaimBus.Script.RejectClaims); + using var node = J1939Node.Open(scripted, new J1939NodeOptions(Name(1)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + + Func act = () => node.ClaimAddressAsync(0x10).WithTimeout(ShortTimeout); + var thrown = await act.Should().ThrowAsync(); + thrown.Which.Should().NotBeOfType(); + node.ClaimState.Should().Be(J1939ClaimState.NotClaimed); + node.Address.Should().BeNull(); + } + + // The announce's confirmation throws: the claim faults with that exception wrapped. + [Fact] + public async Task A_Throwing_Address_Claim_Transmit_Faults_The_Claim() + { + var session = NewSession(); + using var bus = Open(session, 0); + using var raw = new CanBusService(bus); + using var scripted = new ScriptedClaimBus(raw, ScriptedClaimBus.Script.ThrowOnClaims); + using var node = J1939Node.Open(scripted, new J1939NodeOptions(Name(1)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + + Func act = () => node.ClaimAddressAsync(0x10).WithTimeout(ShortTimeout); + var thrown = await act.Should().ThrowAsync(); + thrown.Which.InnerException.Should().BeOfType(); + node.ClaimState.Should().Be(J1939ClaimState.NotClaimed); + } + + // The confirmation arrives after the node has been disposed: posting it back finds no loop. + [Theory] + [InlineData(ScriptedClaimBus.ReleaseKind.Confirmed)] + [InlineData(ScriptedClaimBus.ReleaseKind.Rejected)] + [InlineData(ScriptedClaimBus.ReleaseKind.Throw)] + public async Task A_Claim_Confirmation_That_Arrives_After_Dispose_Is_Dropped(ScriptedClaimBus.ReleaseKind release) + { + var session = NewSession(); + using var bus = Open(session, 0); + using var raw = new CanBusService(bus); + using var scripted = new ScriptedClaimBus(raw, ScriptedClaimBus.Script.HoldFirstClaim); + using var node = J1939Node.Open(scripted, new J1939NodeOptions(Name(1)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + + var claim = node.ClaimAddressAsync(0x10); + await scripted.Held.AsTaskWithTimeout(ShortTimeout); + node.Dispose(); + scripted.Release(release); + Func act = () => claim.WithTimeout(ShortTimeout); + await act.Should().ThrowAsync(); + } + + // A Cannot Claim is fire-and-forget. A driver that rejects it, or throws, surfaces on the + // background channel; the claim itself still faults as a loss. + [Theory] + [InlineData(ScriptedClaimBus.Script.RejectCannotClaim, typeof(J1939NodeException))] + [InlineData(ScriptedClaimBus.Script.ThrowOnCannotClaim, typeof(InvalidOperationException))] + public async Task A_Failed_Cannot_Claim_Transmit_Surfaces_In_The_Background(ScriptedClaimBus.Script script, Type exceptionType) + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var raw = new CanBusService(busA); + using var scripted = new ScriptedClaimBus(raw, script); + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); + using var node = J1939Node.Open(scripted, new J1939NodeOptions(Name(0x00005F)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); // backoff 0 + + var background = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + node.BackgroundExceptionOccurred += (_, ex) => background.TrySetResult(ex); + + Func act = () => node.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); + await act.Should().ThrowAsync(); + var surfaced = await background.Task.AsTaskWithTimeout(ShortTimeout); + surfaced.Should().BeOfType(exceptionType); + } + // --------------------------------------------------------------------------------------- // #34 (SAE J1939-81 §4.2.2): a Request for PGN 0xEE00 is answered by the node itself — // with its Address Claimed while it holds an address, with Cannot-Claim (SA 0xFE) while it @@ -2662,6 +2973,133 @@ public async Task StartPeriodicSend_SingleFrame_ReclaimResumesUnderNewSa() } } +/// +/// Test double over a real : address-claim transmits can be rejected, +/// thrown, or parked until the test releases them. Everything else is forwarded. +/// +public sealed class ScriptedClaimBus : ICanBusService +{ + public enum Script + { + HoldFirstClaim, + RejectClaims, + ThrowOnClaims, + RejectCannotClaim, + ThrowOnCannotClaim, + } + + public enum ReleaseKind + { + Confirmed, + Rejected, + Throw, + } + + private readonly ICanBusService _inner; + private readonly Script _script; + private readonly TaskCompletionSource _held = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _release = new(TaskCreationOptions.RunContinuationsAsynchronously); + private int _heldOnce; + + public ScriptedClaimBus(ICanBusService inner, Script script) + { + _inner = inner; + _script = script; + } + + public Task Held => _held.Task; + + public void Release(ReleaseKind how) => _release.TrySetResult(how); + + public void ReleaseConfirmed() => Release(ReleaseKind.Confirmed); + + public ICanBus Bus => _inner.Bus; + public int SubscriptionCount => _inner.SubscriptionCount; + + public event EventHandler? BackgroundExceptionOccurred + { + add => _inner.BackgroundExceptionOccurred += value; + remove => _inner.BackgroundExceptionOccurred -= value; + } + + public ISubscription Subscribe(Func? predicate = null, int? bufferCapacity = null, bool includeEcho = false) + => _inner.Subscribe(predicate, bufferCapacity, includeEcho); + + public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) + => _inner.Subscribe(filter, bufferCapacity, includeEcho); + + public IReadOnlyList FindOverlappingFilterSubscriptions() + => _inner.FindOverlappingFilterSubscriptions(); + + public async Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, CancellationToken cancellationToken = default) + { + if (frame.IsExtendedFrame) + { + var fields = J1939Id.Decompose((uint)frame.ID); + if (J1939Pgn.IsAddressClaim(fields.Pgn)) + { + var outcome = Outcome(fields.SourceAddress); + if (outcome == ClaimOutcome.Hold) + { + _held.TrySetResult(true); + switch (await _release.Task.ConfigureAwait(false)) + { + case ReleaseKind.Confirmed: + return Accepted(); + case ReleaseKind.Rejected: + return Rejected(); + default: + throw new InvalidOperationException("address claim transmit failed"); + } + } + if (outcome == ClaimOutcome.Reject) return Rejected(); + if (outcome == ClaimOutcome.Throw) throw new InvalidOperationException("address claim transmit failed"); + } + } + + return await _inner.SendConfirmed(frame, timeout, cancellationToken).ConfigureAwait(false); + } + + public void Dispose() { /* the test owns the inner service */ } + + private ClaimOutcome Outcome(byte sourceAddress) + { + switch (_script) + { + case Script.HoldFirstClaim: + return Interlocked.Exchange(ref _heldOnce, 1) == 0 ? ClaimOutcome.Hold : ClaimOutcome.Forward; + case Script.RejectClaims: + return ClaimOutcome.Reject; + case Script.ThrowOnClaims: + return ClaimOutcome.Throw; + case Script.RejectCannotClaim: + return sourceAddress == J1939Pgn.NullAddress ? ClaimOutcome.Reject : ClaimOutcome.Forward; + case Script.ThrowOnCannotClaim: + return sourceAddress == J1939Pgn.NullAddress ? ClaimOutcome.Throw : ClaimOutcome.Forward; + default: + return ClaimOutcome.Forward; + } + } + + private enum ClaimOutcome { Forward, Hold, Reject, Throw } + + private static TxConfirmation Accepted() => new TxConfirmation + { + Confirmed = true, + IsApproximated = true, + Timestamp = DateTime.UtcNow, + FailureReason = TxConfirmFailureReason.None, + }; + + private static TxConfirmation Rejected() => new TxConfirmation + { + Confirmed = false, + IsApproximated = false, + Timestamp = DateTime.UtcNow, + FailureReason = TxConfirmFailureReason.Rejected, + }; +} + internal static class J1939NodeTestExtensions { public static async Task AsTaskWithTimeout(this Task task, TimeSpan timeout) From e2f32e28b44cd2d28940cbc5bc4d67307ccbdc76 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:45:00 +0200 Subject: [PATCH 8/9] test(j1939): dispose the backoff node with using 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 --- .../TestCases/J1939/J1939NodeTests.cs | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index ae493fc..0e03f3c 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -892,25 +892,20 @@ public async Task A_Backoff_That_Fires_After_Dispose_Sends_No_Cannot_Claim() Interlocked.Increment(ref cannotClaims); }; - var node = new J1939NodeImpl(service, new J1939NodeOptions(Name(0x000158)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }, ownsService: false, actor); - try + Task claim; + using (var node = new J1939NodeImpl(service, new J1939NodeOptions(Name(0x000158)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }, ownsService: false, actor)) { var lost = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); node.AddressClaimChanged += (_, e) => { if (e.State == J1939ClaimState.CannotClaim) lost.TrySetResult(true); }; - var claim = node.ClaimAddressAsync(0x63); + claim = node.ClaimAddressAsync(0x63); await lost.Task.AsTaskWithTimeout(ShortTimeout); await clock.WaitUntilTimerArmedAsync(actor, backoff, ShortTimeout); - - node.Dispose(); - await clock.AdvanceAsync(backoff); - Volatile.Read(ref cannotClaims).Should().Be(0, "a backoff that fires after dispose does not send the Cannot Claim"); - Func awaitClaim = () => claim.WithTimeout(ShortTimeout); - await awaitClaim.Should().ThrowAsync(); - } - finally - { - node.Dispose(); } + + await clock.AdvanceAsync(backoff); + Volatile.Read(ref cannotClaims).Should().Be(0, "a backoff that fires after dispose does not send the Cannot Claim"); + Func awaitClaim = () => claim.WithTimeout(ShortTimeout); + await awaitClaim.Should().ThrowAsync(); } // The announce's confirmation is a failure: the claim faults and the address is not taken. From b6c4ee64eae1c4e90a2c22cd763a6def1c0be945 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Wed, 23 Sep 2026 20:04:42 +0200 Subject: [PATCH 9/9] fix(j1939): fault a lost claim only after the Cannot Claim handoff 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 --- src/CanKit.Pro.J1939/J1939NodeImpl.cs | 50 +++++++++++---- .../TestCases/J1939/J1939NodeTests.cs | 61 ++++++++++++++++++- 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index c6ae575..e3c0d53 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -309,10 +309,13 @@ private void BeginClaim(byte preferredAddress, TaskCompletionSource tcs // A Cannot Claim still waiting its backoff is overtaken by this claim (Codex on #153). // Dropping it settles the loss that scheduled it just as finally as sending it would, - // so the caller waiting on that loss is answered here. + // so the caller waiting on that loss is answered here. One already handed to the driver + // is not: that caller is answered when the handoff finishes, and faulting it here would + // let a dispose on the exception suppress the frame. _cannotClaimBackoff?.Dispose(); _cannotClaimBackoff = null; - CompleteLostClaim(); + if (!_cannotClaimHandoffPending) + CompleteLostClaim(); // A claim still in arbitration is not silently replaced: the caller who started it // is waiting on it, and a second one is a programming error (#58). A re-claim on a @@ -388,16 +391,33 @@ private void SendCannotClaimIfStillDue() // line. NotClaimed passes, because that is a node that never claimed answering a scan. var state = (J1939ClaimState)Volatile.Read(ref _claimStateStore); if (state is not (J1939ClaimState.Claimed or J1939ClaimState.Claiming)) - SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress); + { + // Fault only after SendConfirmed has handed the frame to the driver. Completing + // the loss beside the fire-and-forget start lets a caller dispose on the exception + // while that handoff is still pending, and the frame is never sent (Codex on #153). + _cannotClaimHandoffPending = true; + SendAddressClaimFrame(sourceAddress: J1939Pgn.NullAddress, afterHandoff: () => + { + _cannotClaimHandoffPending = false; + CompleteLostClaim(); + }); + return; + } CompleteLostClaim(); } - // The claim of a loss that owes the bus a Cannot Claim: it faults once that frame has gone - // out, not when the loss became known. A caller that disposes the node on the exception -- - // a `using` scope ending on it -- would otherwise tear the actor down inside the backoff, - // and the frame the loss owes would never be sent at all (Codex on #153). + // The claim of a loss that owes the bus a Cannot Claim: it faults once that frame has been + // handed to the driver, not when the loss became known. A caller that disposes the node on + // the exception -- a `using` scope ending on it -- would otherwise tear the actor down + // inside the backoff, or inside the send, and the frame the loss owes would never be sent + // at all (Codex on #153). private PendingClaim? _lostClaim; + // Set on the actor for the interval between starting the Cannot Claim send and the + // handoff continuation. A claim that starts in that interval must not answer the loss: + // the frame is already with the driver. + private bool _cannotClaimHandoffPending; + private void CompleteLostClaim() { var lost = _lostClaim; @@ -750,15 +770,17 @@ private void AnswerRequestForAddressClaimed() } } - private void SendAddressClaimFrame(byte sourceAddress) + private void SendAddressClaimFrame(byte sourceAddress, Action? afterHandoff = null) { // 8-byte little-endian NAME payload. PGN 0xEE00 is PDU1 with PS = 0xFF (global). // Re-announcements / Cannot-Claim remain fire-and-forget; the initial claim path uses // TransmitAddressClaimConfirmed so ClaimAddressAsync cannot succeed without TX confirm. + // `afterHandoff` runs back on the actor once SendConfirmed has returned, which is after + // the driver accepted the frame -- the loss that owes a Cannot Claim faults from there. var payload = BuildAddressClaimPayload(); uint canId = J1939Id.ComposePgn(_options.ClaimPriority, J1939Pgn.AddressClaimed, sourceAddress, destinationAddress: J1939Pgn.GlobalAddress); - TransmitFrame(canId, payload); + TransmitFrame(canId, payload, afterHandoff); } private byte[] BuildAddressClaimPayload() => _name.ToBytes(); @@ -1030,15 +1052,15 @@ public IDisposable StartPeriodicSend(J1939Message message, TimeSpan period) // Wire helpers // ========================================================================================= - private void TransmitFrame(uint canId, byte[] payload) + private void TransmitFrame(uint canId, byte[] payload, Action? afterHandoff = null) { // Fire-and-forget: address-claim traffic doesn't need a task, but we still want a // background exception if the driver rejects it. SendConfirmed is used consistently // with the rest of the CanKit.Pro stack. No Task.Run hop in front of it (#58). - _ = TransmitFrameAsync(canId, payload); + _ = TransmitFrameAsync(canId, payload, afterHandoff); } - private async Task TransmitFrameAsync(uint canId, byte[] payload) + private async Task TransmitFrameAsync(uint canId, byte[] payload, Action? afterHandoff) { try { @@ -1052,6 +1074,10 @@ private async Task TransmitFrameAsync(uint canId, byte[] payload) { RaiseBackgroundException(ex); } + + if (afterHandoff is null) return; + try { _actor.Post(afterHandoff); } + catch (ObjectDisposedException) { /* the node was disposed: no loop to tell */ } } // ========================================================================================= diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs index 0e03f3c..b34aa91 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939/J1939NodeTests.cs @@ -962,6 +962,57 @@ public async Task A_Claim_Confirmation_That_Arrives_After_Dispose_Is_Dropped(Scr await act.Should().ThrowAsync(); } + // Codex on #153: the loss must not fault on the same actor turn that only started the + // Cannot Claim. SendConfirmed here does not return until released, so the handoff has not + // happened. A post queued behind that turn sees the claim still incomplete -- completing + // it beside the fire-and-forget start would already have faulted it. Releasing forwards + // the frame, and only then does the claim fault. + [Fact] + public async Task A_Lost_Claim_Faults_Only_After_The_Cannot_Claim_Handoff() + { + var session = NewSession(); + using var busA = Open(session, 0); + using var busB = Open(session, 1); + using var busC = Open(session, 2); + using var raw = new CanBusService(busA); + using var scripted = new ScriptedClaimBus(raw, ScriptedClaimBus.Script.HoldCannotClaim); + using var actor = new ProtocolActor(); + + using var winner = J1939Node.Open(busB, new J1939NodeOptions(Name(0x000010)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }); + await winner.ClaimAddressAsync(0x63).WithTimeout(ShortTimeout); + using var node = new J1939NodeImpl(scripted, new J1939NodeOptions(Name(0x00005F)) { ClaimAnnounceTimeout = TimeSpan.FromMilliseconds(80) }, ownsService: false, actor); + + var onBus = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var reclaimed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + busC.FrameObserved += (_, e) => + { + if (!e.CanFrame.IsExtendedFrame) return; + var fields = J1939Id.Decompose((uint)e.CanFrame.ID); + if (!J1939Pgn.IsAddressClaim(fields.Pgn)) return; + if (fields.SourceAddress == J1939Pgn.NullAddress) onBus.TrySetResult(true); + else if (fields.SourceAddress == 0x11) reclaimed.TrySetResult(true); + }; + + var claim = node.ClaimAddressAsync(0x63); + await scripted.Held.AsTaskWithTimeout(ShortTimeout); + // The turn that started the send has finished: a loss completed beside that start is + // already faulted, and one completed from the send's continuation is not. + await actor.PostAsync(() => { }).WithTimeout(ShortTimeout); + claim.IsCompleted.Should().BeFalse("the loss is not faulted while the Cannot Claim handoff is still pending"); + + // A claim started while that handoff is outstanding does not answer the loss either. + // Answering it here would let the caller dispose on the exception and suppress the frame. + var reclaim = node.ClaimAddressAsync(0x11); + await reclaimed.Task.AsTaskWithTimeout(ShortTimeout); + claim.IsCompleted.Should().BeFalse("a claim started during the handoff does not fault the loss"); + + scripted.ReleaseConfirmed(); + await onBus.Task.AsTaskWithTimeout(ShortTimeout); + Func act = () => claim.WithTimeout(ShortTimeout); + await act.Should().ThrowAsync(); + await reclaim.WithTimeout(ShortTimeout); + } + // A Cannot Claim is fire-and-forget. A driver that rejects it, or throws, surfaces on the // background channel; the claim itself still faults as a loss. [Theory] @@ -2977,6 +3028,7 @@ public sealed class ScriptedClaimBus : ICanBusService public enum Script { HoldFirstClaim, + HoldCannotClaim, RejectClaims, ThrowOnClaims, RejectCannotClaim, @@ -3047,6 +3099,11 @@ public async Task SendConfirmed(CanFrame frame, TimeSpan? timeou throw new InvalidOperationException("address claim transmit failed"); } } + if (outcome == ClaimOutcome.HoldThenForward) + { + _held.TrySetResult(true); + await _release.Task.ConfigureAwait(false); + } if (outcome == ClaimOutcome.Reject) return Rejected(); if (outcome == ClaimOutcome.Throw) throw new InvalidOperationException("address claim transmit failed"); } @@ -3063,6 +3120,8 @@ private ClaimOutcome Outcome(byte sourceAddress) { case Script.HoldFirstClaim: return Interlocked.Exchange(ref _heldOnce, 1) == 0 ? ClaimOutcome.Hold : ClaimOutcome.Forward; + case Script.HoldCannotClaim: + return sourceAddress == J1939Pgn.NullAddress ? ClaimOutcome.HoldThenForward : ClaimOutcome.Forward; case Script.RejectClaims: return ClaimOutcome.Reject; case Script.ThrowOnClaims: @@ -3076,7 +3135,7 @@ private ClaimOutcome Outcome(byte sourceAddress) } } - private enum ClaimOutcome { Forward, Hold, Reject, Throw } + private enum ClaimOutcome { Forward, Hold, HoldThenForward, Reject, Throw } private static TxConfirmation Accepted() => new TxConfirmation {