feat(rawcan)!: harden TX-confirm echo matching and replace the filter-overlap tuple

[dborgards]

What does this change?

Three RawCan issues that all edit CanBusService.cs and ICanBusService.cs, so they land together
rather than as three pull requests inheriting each other's conflicts (CLAUDE.md § Pull requests,
and the sequencing note on #82).

Closes #24 — an expired pending send no longer poisons the echo FIFO.
Closes #53 — the collected minor findings, six of seven acted on; the seventh is argued below.
Closes #82 — FindOverlappingFilterSubscriptions() returns a named FilterOverlap.

Type of change

• feat — new behaviour (minor release)
• fix / perf — bug or performance fix (patch release)
• docs / test / refactor / chore / ci — no release
• Breaking change (! in the title, plus a BREAKING CHANGE: footer explaining the migration)

Two commits carry ! and a BREAKING CHANGE: footer — feat(rawcan)! for #82 and fix(rawcan)!
for the filter validation. .releaserc.json maps breaking → minor, per ADR 0001, so the release
is a minor bump.

#24 — the expired pending send

Three parts, as the issue sets out:

• The cancellation/timeout registration unlinks the entry under _pendingGate, before completing
  the TCS, instead of leaving it to SendWithEchoConfirmAsync's finally. Because the TCS is
  RunContinuationsAsynchronously, that finally runs a scheduling turn later on a pool thread —
  and until it did, the resolved entry was still the FIFO head for its key and swallowed the next
  byte-identical send's echo.
• TryMatchEcho skips and unlinks entries whose TCS is already completed, rather than taking
  list.First unconditionally.
• PendingKey includes the frame kind and the identifying flags.

The finally is not removed. It is still the cleanup for the paths that never reach the
registration (rejection, an exception out of Transmit), and removing a guard because a new one
appears to shadow it is the failure mode this repository has paid for before.

Flags: Ext | Rtr | Error, not all of them — a deliberate deviation from the issue. Brs and
Esi are link-layer transmission attributes; an adapter may report an echo whose BRS/ESI differ
from what the caller requested (ESI reflects the controller's error state). Two sends differing in
nothing else are interchangeable for confirmation purposes — the FIFO hands each exactly one echo —
so including those bits could only turn a matched echo into a spurious timeout, which is the failure
this issue is about. Worth a second opinion if you disagree.

The test the issue asks for

ControllableBus gained an OnTransmitting hook, which runs on the transmitting thread while
CanBusService holds _pendingGate. That is what makes the poisoning case deterministic instead
of a race: the first send is cancelled from inside the second send's Transmit, so the first send's
own async cleanup blocks on the lock we are holding and cannot possibly unlink the entry before the
second send's echo is matched a few lines later, inside the same call. Without the fix the stale
entry is guaranteed to be the FIFO head at that moment, not merely likely.

(#93 already added DeferredEchoQueue and made the FIFO-order test real; that half of the issue's
test-gap note is done. This is the remaining half.)

#53 — the collected findings

#	Finding	Done
1	Transmit runs under _pendingGate	No — see below
2	TryMatchEcho allocates a byte[] per echo frame	Yes
3	Payload copied once per matching subscription	Yes (the L3/L4 second copies stay — see below)
4	CanIdFilter.Range/Mask do not validate against the ID space	Yes
5	Interlocked.Exchange on a volatile field	Yes
6	Callback Subscribe swallows onNext failures, leaks the pump task	Yes
7	ProtocolErrorCodes occupies CanKitErrorCode 6002..6005	Partly — a tripwire, not a move

1. Transmit under _pendingGate — left as it is

This finding predates #93, which moved Transmit into the lock on purpose. Holding it across
register + transmit is what makes the pending FIFO's order equal transmission order, and that is the
whole of FR-RAW-031's "no cross-matching of byte-identical concurrent sends" — with the lock
released in between, two threads can register in one order and transmit in the other. An echo-mode
adapter also re-enters this same lock from inside Transmit already, and a driver that genuinely
blocks in Transmit is unusable for every other consumer of ICanBus too, since the same call is on
their dispatch path. The rationale is now recorded next to the lock (docs(rawcan) commit) so the
finding is not re-raised from a closed issue.

3. The L3/L4 second copies stay

The per-subscription copy is now one copy per frame, shared by every subscription that buffers it.
That makes the IsoTpChannel/J1939TpChannel/CanOpenNode copies more load-bearing, not less:
each protocol layer now gets a buffer shared with every other matching subscription, and dropping
its copy would also require MemoryMarshal.TryGetArray to get a byte[] back out of a
ReadOnlyMemory<byte> — trading a bounded, provably safe allocation for an aliasing invariant spread
across three packages. What the issue is right about is the comment: only IsoTpChannel had one,
and it described a hazard (the adapter's RX lease) that the subscription had already removed. It now
says why the copy is actually there.

One existing test asserted the opposite of the new behaviour — ReferenceEquals(segA.Array, segB.Array).Should().BeFalse("each subscription buffers its own copy") — as the setup for proving
that CanFrameEvent equality compares bytes rather than memory segments. That guard is not dropped:
it becomes a direct test over two arrays built in the test, so the distinct-buffer case stays covered
no matter how many copies the demux makes, and the two-subscription test keeps asserting equal
events.

4. Filter validation is a behaviour break

Range and Mask now throw ArgumentOutOfRangeException for bounds or acceptance pairs outside the
target ID space. Marked breaking with a footer, because code that built such a filter now gets an
exception where it previously got silence — though "silence" meant matching nothing at all, which is
the bug. Three existing tests drove out-of-space filters through the factories to reach the clipping
in Overlaps; they become construction-rejection tests. The clipping and the full-width bit walk are
kept — they are what makes Overlaps correct independently of how a filter was built, and an
acceptance mask reaching above the ID space is still legal (it constrains those bits to zero).

6. onError is an API addition, not just a fix

ICanBusService.BackgroundExceptionOccurred is an event on the interface, and only the declaring
type can raise one — so for any implementation other than CanBusService this extension had nowhere
to put a failing handler and dropped it. There is no way to fix that without a channel the caller
supplies, hence the optional onError. It takes precedence over the service event when given. The
pump loop is also wrapped so a failure of the enumeration itself is reported rather than left on a
task that Dispose may already have stopped waiting for. The 2 s join stays bounded: an onNext
that never returns cannot be cancelled from here, and waiting longer only moves the hang into the
caller's Dispose.

7. The error codes are not moved

Verified against the pinned CanKit 0.5.6 by reflecting over the packaged enum: the 6000 range holds
only TransportOperationFailed = 6001. The numbers are chosen so that an upstream adoption of these
four codes deletes ProtocolErrorCodes and changes nothing for callers, which is what
docs/upstream-candidates.md § 1 promises; any other range gives that up for a collision risk that
is smaller but not gone, since CanKit allocates in 1000-blocks up to 9000 and could grow into
whatever we picked. What the finding is right about is that a collision would be silent — an
upstream 6002 would make ProtocolTimeout render an unrelated name and compare equal to an unrelated
failure. So it gets a tripwire test that fails on the CanKit bump which claims one of the four, while
that bump is still a one-line version change under review. Flagging this as the judgement call I am
least certain about: if you would rather relocate the numbers, that is a small follow-up.

#82 — FilterOverlap

FindOverlappingFilterSubscriptions() returns IReadOnlyList<FilterOverlap>. The method is
replaced in place, no parallel method and no [Obsolete], per ADR 0001 and your supersession
comment on the issue — the additive route would have introduced the first [Obsolete] member into a
clean surface with no major version scheduled to remove it. The [return: TupleElementNames]
attribute is gone from the approved API surface.

The method keeps its name: the ADR entry names the tuple as the thing being replaced, the name is
accurate for what it returns, and renaming would churn every doc and call site for nothing. Say the
word if you want FindFilterOverlaps() instead.

The type carries the overlapping ID range, as the issue and your comment suggest:
LowestSharedId/HighestSharedId, exact for range filters and an inclusive hull for acceptance-mask
filters (which accept a scattered set — documented on the type). It also destructures to (A, B), so
foreach (var (a, b) in ...) in the docs keeps working.

Computing the range is a generalisation of the search Overlaps already ran rather than a second
one: the bit walk now returns the smallest or largest satisfying ID instead of only whether one
exists, all four range/mask combinations reduce to one call of it, and Overlaps delegates — one
implementation, not two that can drift. Rewriting that search is the riskiest edit in this PR, so it
is checked against a brute-force sweep of the entire 11-bit ID space for all 169 ordered pairs of
13 filters, on top of the existing overlap tests, which all pass unchanged.

Moved with the signature: five ICanBusService test fakes, docs/getting-started.md,
src/CanKit.Pro.Addressing/README.md, and the FR-RAW-041 rows in the SRS and arc42 documents.

Verified-to-fail evidence

Every test added here was run against the unfixed code first. Nothing below passed before its fix.

Test	Without the fix
Echo_Bus_Does_Not_Let_A_Resolved_Send_Consume_A_Later_Identical_Echo	fails, 5 runs out of 5 — second.Confirmed false (the echo went to the cancelled entry)
Echo_Bus_Does_Not_Confirm_A_Standard_Send_From_An_Extended_Echo_With_The_Same_Id	fails — the extended echo confirms the standard send
Echo_Bus_Does_Not_Confirm_A_Classic_Send_From_A_Can_Fd_Echo_With_The_Same_Id	fails — the CAN-FD echo confirms the Classic send
Two_Subscriptions_Receiving_The_Same_Frame_Share_One_Payload_Copy	fails — two distinct arrays
Echo_Lookup_Does_Not_Copy_The_Payload_Of_Every_Echo_Frame	fails — 88 B allocated per unmatched echo frame against a 64 B budget
Range_Rejects_Bounds_Outside_The_Standard_11Bit_Space	fails — no exception thrown
Range_Rejects_Bounds_Outside_The_Extended_29Bit_Space	fails — no exception thrown
Mask_Rejects_A_Code_Requiring_A_Bit_Outside_The_Id_Space	fails — no exception thrown
Callback_Subscribe_Reports_A_Handler_Failure_Through_OnError_For_A_Foreign_Service	fails — times out waiting for the report
Callback_Subscribe_OnError_Takes_Precedence_Over_The_Service_Fault_Event	fails — times out waiting for the report
Overlap_And_Reported_Range_Agree_With_A_Brute_Force_Sweep_Of_The_Id_Space, An_Overlap_Between_Mask_Filters_Reports_The_Hull_Of_The_Shared_Ids, FindOverlappingFilterSubscriptions_Reports_Overlapping_Registered_Subscriptions	all three fail when the high witness's bit preference is inverted

Two of these deserve their caveat stated rather than buried:

• The two onError tests cannot be compiled against the unfixed extension at all, since the
  parameter does not exist. They were verified by disabling the routing branch instead, which is the
  behaviour they assert.
• The_Borrowed_Protocol_Error_Codes_Are_Still_Unclaimed_Upstream is a tripwire, not a regression
  test: there is no fix it could fail without, because nothing is being fixed. Its teeth were checked
  by pointing it at 6001, which upstream does define — it fails with the message the real case
  would print.

Gate

Run in this worktree on .NET SDK 10.0.112, at 3e9a8b2 — i.e. after merging current main
(a723b3c, #97) into the branch, so these are the numbers for the merge result rather than for a
stale base. The merge was clean; #97 touches CANopen only.

dotnet build CanKit.Pro.sln -c Release -p:CI=true    →  0 Warning(s), 0 Error(s)
dotnet test  CanKit.Pro.sln -c Release --framework net10.0
                                                     →  Passed: 459, Failed: 0, Skipped: 0
dotnet format CanKit.Pro.sln --verify-no-changes     →  clean (exit 0)
dotnet pack + python3 eng/verify-packages.py <fresh> →  9 package(s) checked, all ok

459 tests. Before the merge the same gate read 449 passed at 1bba7d7, against 440 on the old base —
nine of the ten added here, plus the tenth from #97's side of the merge; dotnet pack was verified
at 1bba7d7.

No flaky failures were seen across the roughly fifteen full and partial suite runs this branch took
(#92), so nothing to report there.

tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt was replaced with the generated
.received.txt both times it changed, never hand-edited.

Checklist

• dotnet build CanKit.Pro.sln -c Release succeeds
• dotnet test CanKit.Pro.sln -c Release passes
• Public API changes are documented with XML comments
• New behaviour is covered by a test
• The requirement or ADR this relates to is referenced (FR-RAW-031, FR-RAW-033, FR-RAW-013,
  FR-RAW-014, FR-RAW-041, NFR-006, ADR-7, ADR 0001)

Closes #24
Closes #53
Closes #82

🤖 Generated with Claude Code

https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj

[cursor]

PR Summary

Medium Risk
Breaking API on overlap discovery and filter construction, plus changes to echo FIFO matching and shared RX buffers that affect concurrent SendConfirmed and multi-subscription dispatch on the hot path.

Overview
RawCan gets several coordinated fixes and API updates around TX confirmation, subscription diagnostics, and dispatch efficiency.

Breaking: FindOverlappingFilterSubscriptions() now returns FilterOverlap (subscriptions A/B plus LowestSharedId/HighestSharedId) instead of (First, Second) tuples. CanIdFilter.Range and Mask throw when bounds or acceptance pairs can never match the chosen ID space (e.g. 29-bit IDs without Extend).

TX echo matching (#24, FR-RAW-031): Pending sends are keyed by ID, payload, frame kind, and identity flags (Ext/Rtr/Error, not BRS/ESI). TryMatchEcho walks the FIFO, skipping and unlinking entries whose confirmation task already completed, so cancelled or timed-out sends no longer steal later identical echoes. Timeout/cancellation completes the task without taking _pendingGate (avoiding blocking on another send inside Transmit). Echo lookup borrows the payload instead of allocating per frame.

Dispatch / callbacks: Matching subscriptions on one frame share a single owned payload copy. Callback Subscribe adds optional onError (required for non-CanBusService implementations) and reports pump failures instead of leaving faulted background tasks unobserved.

Overlap math: CanIdFilter.Overlaps delegates to an extended bit walk that returns min/max shared IDs; docs and tests (including brute-force 11-bit sweeps) follow.

Misc: IsoTpChannel RX copy comment clarified; ProtocolErrorCodes upstream collision tripwire test; InternalsVisibleTo for direct PendingKey tests.

^{Reviewed by Cursor Bugbot for commit 3a0cedd. Bugbot is set up for automated code reviews on this repo. Configure here.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[dborgards]

Codecov: one real gap closed, three branches argued

The patch report named 7 lines across four files. I measured them locally rather than reading the percentages, and they are not the same kind of thing — one was a genuine hole, three are defensive branches that cannot be reached deterministically. Taking each in turn, since "95.2 %" says nothing useful on its own.

Closed: CanBusServiceExtensions.cs (2 missing, 1 partial)

The two ArgumentNullException guards on the callback Subscribe overload had no test at all. That is a real hole, not a formality: without them a null service or onNext reaches the pump task and surfaces later as a NullReferenceException on a background thread, with nothing pointing at the call that caused it. Both are now asserted by parameter name. This file is gone from the report.

CanIdFilter.cs — an unreachable guard, and a test that is worth having anyway

I first assumed the half-covered branch was the preferHigh ternary, since that is the riskiest code here, and went looking for a filter pair that would reach it. That assumption was wrong. The flagged branch is one line above:

var remaining = bit >= 31 ? uint.MaxValue : (1u << (bit + 1)) - 1;

bit >= 31 cannot be reached from here. Search starts with both bounds tight, and getting both loose costs at least one bit, so the fill-in always runs at bit <= 30. The preferHigh ternary a line below was already fully covered.

The guard stays, and now says why in a comment: C# masks the shift count, so 1u << 32 evaluates to 1 rather than overflowing, and losing the guard would make the fill-in quietly produce a one-bit remaining mask instead of failing loudly. A branch that exists so a future refactor cannot introduce a silent wrong answer is worth its half-covered line.

The filter pair I found while chasing the wrong hypothesis turned out to be worth keeping on its own merits, so it is in: range [0x000, 0x040] against a mask requiring bits 4..7 clear shares only 0x000..0x00F, so HighestSharedId is 0x00F — not the range's own upper bound. That is the shape where the high walk must abandon the high bound rather than ride it down, and the brute-force sweep never produced it. The test cross-checks both bounds against an enumeration of the shared set, so it cannot agree with a wrong implementation. It does not move the coverage number, and I would rather say that than imply it did.

CanBusService.cs (1 missing, 1 partial) — the guard #24 made unreachable

The uncovered branch is TryMatchEcho skipping an entry whose TCS is already completed. It is unreachable by construction, and that is the point of this PR: every resolution path now unlinks under _pendingGate before completing the TCS, so while TryMatchEcho holds that lock a linked entry always has an incomplete TCS. The dispose path nulls Node before completing too.

Reaching it in a test would mean deliberately constructing the broken invariant through internals — PendingSend is internal and _pending is private, so it would need InternalsVisibleTo added to CanKit.Pro.RawCan purely to assert a state the code says cannot occur. That trades a real widening of the assembly's surface for a test that pins defensive code rather than behaviour. The behaviour that is reachable — an expired send not consuming a later identical echo — is covered by Echo_Bus_Does_Not_Let_A_Resolved_Send_Consume_A_Later_Identical_Echo, which fails 5 out of 5 without the fix.

PendingSend.cs (1 partial) — reachable only on a hash collision

PendingKey.Equals short-circuits on _id, _flags and _frameKind before comparing the payload. The unhit arms are those early exits, which a dictionary only takes when two different keys land in the same bucket. Since the hash folds all four components, the extended-vs-standard and Classic-vs-CAN-FD tests never reach Equals at all — they diverge at the hash. Producing a deterministic collision would mean reverse-engineering the hash in a test, and that test would break the moment the hash changed, without the equality contract having changed at all.

---

Net: 597/597, build 0 warnings / 0 errors with CI=true, format clean. Pushed as e2a8a09, on top of the main merge (e6afc0a, #98).

If you would rather have the two unreachable branches covered than argued, the honest route is InternalsVisibleTo on CanKit.Pro.RawCan plus direct unit tests on PendingKey and the FIFO — say the word and I will do it as a separate change, since it widens the assembly surface and does not belong in this one.

---

Generated by Claude Code

[dborgards]

CI: macos-latest red on e2a8a09 — not this PR's, but the one failure worth a second look

What failed. One test, UdsClientTests.TimedOut_Request_Does_Not_Poison_Next_Same_Service_Transaction:

Expected a <UdsTimeoutException> to be thrown, but no exception was thrown.

596 of 597 passed. Everything else on this head is green so far.

Why it is not this PR's. The push that produced this head added exactly two tests and one comment — nothing under CanKit.Pro.Uds, CanKit.Pro.Actor or the deadline scheduler. The whole PR touches RawCan, one ISO-TP comment, docs and tests. And the failing assertion is about a UDS client deadline losing a race to a late response, which no part of this diff reaches.

But I am not filing this as "just the flake", because #92 explicitly says it might not be. This is the third test in #92's original table, and the only one that issue suspects is a product bug rather than a tolerance problem:

A 170 ms margin is not tight, so "the runner was slow" does not explain it on its own — the response cannot arrive early. What can produce the observed result is the opposite of slowness at the point that matters: if the actor loop is starved for longer than 250 ms, then by the time it runs again both the expired deadline and the response are waiting, and the response wins the race.

That hypothesis had exactly one observation behind it. It now has two, on unrelated branches, which is worth something: it makes "one unlucky runner" a weaker explanation than it was.

Worth stating plainly given where it landed: #92 notes this is the same shape as #24 — an expired thing that should be dead still winning against what came after it — and #24 is what this PR fixes. It is not the same defect. #24 is the L2 echo FIFO; this is the L4 deadline-versus-response race inside ProtocolActor. Nothing in this PR fixes or worsens it. But the recurrence is a second argument for #92's suggested order, which puts the UDS case first precisely because it is the one that might be a real bug.

What I did. Attempted the one re-run this situation allows; GitHub refused with 403 This workflow is already running because windows-latest and ubuntu-latest are still in flight. I will re-run the failed job once the run completes, and I am not treating this PR as green until it is.

No fix to port, and I am not touching the test. Making the assertion survive a starved actor loop would hide the very signal #92 wants characterised. The durable fix — driving the scheduler from a virtual clock the test advances — is test-infrastructure work across several suites and has no business in a PR about RawCan echo matching.

Recorded on #92 as the seventh occurrence, and the first repeat of any single test.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2a8a09a13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dborgards]

Codecov, measured again on 2a87013

The report above still lists CanBusServiceExtensions.cs at 92.30 %, which contradicts what I wrote earlier. Measured locally on the current head, it does not:

File	Line	Branch
CanBusServiceExtensions.cs	1.00	1.00
FilterOverlap.cs	1.00	1.00
CanIdFilter.cs	0.987	0.938
PendingSend.cs	0.950	0.500

So the Extensions row is Codecov lag — its comment was refreshed while the CI run for this head was still going, so a new patch definition met an older upload. The overall figure did move (95.2 % → 96.6 %), which is the CanBusService.cs items dropping off: the cancellation test added in 2a87013 reaches the TryMatchEcho skip that was unreachable before, because that loop is now the mechanism rather than a guard behind an eager unlink. A finding fixed a coverage hole as a side effect, which is the right order of events.

What remains is the two branches I argued before, unchanged and still argued:

• CanIdFilter.cs — the bit >= 31 guard on the shift, unreachable from its only call site because the walk starts with both bounds tight. Documented in place.
• PendingSend.cs — PendingKey.Equals short-circuits, reachable only on a hash-bucket collision, plus the Equals(object?) override that a Dictionary<PendingKey, …> never calls because it goes through the generic IEquatable path.

Neither is worth a test that reverse-engineers a hash or asserts a state the code says cannot occur. If Codecov still shows the Extensions row after the current run finishes uploading, that is worth a second look — I will check on the next pass rather than assume.

---

Generated by Claude Code

[dborgards]

Correction: Codecov was right about CanBusServiceExtensions.cs, my measurement was wrong

I said that row was stale reporting and that the file measured 100 % locally. It did not. The script I checked with de-duplicated coverage entries by filename — and Cobertura emits one entry per type, so it read the outer static class, saw 1.00, and never looked at the nested CallbackSubscription, which is where the uncovered lines live. The bot was reporting a real gap for two rounds while I explained it away.

What was uncovered is behaviour this PR introduces and the description above explicitly claims:

The pump loop is also wrapped so a failure of the enumeration itself is reported rather than left on a task that Dispose may already have stopped waiting for.

That outer catch had no test. Every existing onError case throws inside the handler, which the inner catch takes — so the outer one was never entered by anything in the suite. A claimed feature with no coverage, which is exactly the kind of thing this PR spent effort on elsewhere.

Fixed in a37a20a, with a service whose subscription throws on enumeration:

• Callback_Subscribe_Reports_A_Failure_Of_The_Frame_Stream_Itself — the stream's own failure reaches onError.
• Callback_Subscribe_Drops_A_Stream_Failure_It_Has_Nowhere_To_Report — with no onError and a service that is not CanBusService, there is nowhere to send it (the interface's fault event is not ours to raise), so it is dropped quietly rather than surfacing on a pool thread; Dispose stays idempotent afterwards.

CanBusServiceExtensions.cs is now fully covered, lines and branches, across all nested types — measured with the corrected script, not the one that produced the wrong answer.

The other two rows are unaffected by that bug — neither CanIdFilter.cs nor PendingSend.cs has nested types, so those measurements were right and the arguments stand: an unreachable shift guard, and equality short-circuits reachable only on a hash-bucket collision.

Gate: 600/600, build 0 warnings / 0 errors with CI=true, format clean.

Worth recording why this went wrong twice: both times I treated a disagreement between the tool and my own number as the tool being behind, rather than checking my instrument. The first round the intersection used stale line numbers; the second the aggregation dropped nested types. A bot that keeps reporting the same thing after you have "fixed" it is evidence about the measurement, not about the bot.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d213e001d5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dborgards]

macos-latest red again on f75b40f — same test, still not this PR's

UdsClientTests.TimedOut_Request_Does_Not_Poison_Next_Same_Service_Transaction, 599/600, "Expected a <UdsTimeoutException> to be thrown, but no exception was thrown". Same failure as on e2a8a09; the reasoning I gave there still holds, so this is an update rather than a restatement.

Two things make it not attributable to this diff:

• Subsystem. This PR touches CanKit.Pro.RawCan's TX-confirm FIFO and its dispatch path. The assertion is about a UDS client deadline expiring in ProtocolActor's scheduler — a different package, and nothing in the diff is on the path between them.

• Pattern. Four consecutive heads of this branch alternate:

  head	macos-latest
  e2a8a09	fail
  a37a20a	pass
  d213e00	pass
  f75b40f	fail

  A regression introduced by a commit does not switch itself off for two heads and back on for the third. Timing sensitivity on a shared macOS runner does exactly that.

I have spent the one allowed re-run on this head (the earlier attempt was refused with 403 This workflow is already running while the other legs were still in flight). ubuntu-latest and windows-latest are green on f75b40f, including net48, and the suite is 600/600 locally.

The test itself is untouched and stays untouched — it is asserting the right thing, and #92 now carries the argument that the right thing may not be happening. I have recorded this occurrence there as the third failure and the second on a single branch, which is what pushes the reading past "flake": three instances of one shape in one subsystem (#24's expired pending send, the check-then-act in TryMatchEcho fixed in this PR, and this) start to look like a missing invariant rather than a coincidence — an expiry must be observed before anything arriving after it, and claiming must be one atomic transition rather than a test followed by an act.

If the re-run is green, this PR is ready on its own merits. If it is red again on the same test, that is further evidence for #92 and still not a reason to hold this diff.

---

Generated by Claude Code

[dborgards]

Correction to my last comment: the re-run settles this, and not the way I framed it

I said the UDS test was the thing to watch. The re-run of macos-latest on the same commit f75b40f failed — but with a different pair of tests, and UdsClientTests passed:

attempt	failures on f75b40f
1	UdsClientTests.TimedOut_Request_Does_Not_Poison_Next_Same_Service_Transaction
2	J1939NodeTests.ClaimAddressAsync_CancelDuringArbitration_TearsDownPendingClaim, J1939NodeTests.StartPeriodicSend_MultiFrame_KeepsFixedRate_Without_SendTime_Drift

Two runs of one commit, disjoint failure sets. That is nondeterminism by definition, and it is not about this diff.

More to the point, macos-latest is red on main right now. Run 34743182101 on e6afc0a (the #98 merge, this PR's base) fails J1939NodeTests.StartPeriodicSend_SingleFrame_FiresAtConfiguredPeriod — 582/583. So the base branch carries the failure independently of #100, which is the one case where a red leg is genuinely not this PR's to root-cause.

I have spent the one allowed re-run. There is no fix on main to port, because main is where the problem is. Nothing in the diff — RawCan's TX-confirm FIFO and dispatch — is on the path to a J1939 periodic-send timer or a UDS deadline.

What the three failures actually have in common

All three are wall-clock assertions on a shared, loaded macOS runner, and at least one is a straightforward test defect rather than jitter:

for (int i = 0; i < 20 && node.ClaimState != J1939ClaimState.Claiming; i++)
    await Task.Delay(10);
node.ClaimState.Should().Be(J1939ClaimState.Claiming);   // found Claimed

The test means to observe a 500 ms arbitration window from the inside. Its own setup budget is nominally 200 ms but is bounded by nothing — twenty Task.Delay(10) hops on a stalled runner overshoot 500 ms easily, and by then the arbitration timer has legitimately committed the address. The product did the right thing; the test arrived late and blamed it. Same family as the Thread.Sleep I removed from my own test earlier in this PR.

The two periodic-send tests are harder — they assert grid alignment and median inter-arrival, so timing is the observable — but a 299 ms off-grid gap against a 180 ms tolerance is the runner, not a drifting scheduler.

Where this leaves #100 and what I am doing next

This PR stands on its own: ubuntu-latest and windows-latest green on f75b40f including net48, format/version/release-config green, 600/600 locally, Codecov 98.68%, all review threads resolved.

The macOS failures are a main problem and I am treating them as one — recorded on #92, which I am widening from "one flaky UDS test" to what the evidence now shows: a family of wall-clock-sensitive tests, currently red on the base branch. I am not touching any of these tests from this PR, and I will not fix them by loosening a tolerance — that converts a test that sometimes tells the truth into one that never does.

---

Generated by Claude Code

[dborgards]

Codecov: both remaining items closed, not argued — 3a0cedd

Fair criticism. I explained these two away three times instead of fixing them, and one of my reasons for not fixing them was simply wrong. Patch coverage is 0 missing, 0 partial now.

CanIdFilter.cs — the branch is gone, not covered

- var remaining = bit >= 31 ? uint.MaxValue : (1u << (bit + 1)) - 1;
+ var remaining = (uint)((1UL << (bit + 1)) - 1);

The guard existed because C# masks the shift count: 1u << 32 is not an overflow, it evaluates to 1, so a 32-bit shift would silently produce a one-bit mask instead of all-ones. A 64-bit shift has room for the carry, so one expression is correct for every bit in 0..31 and there is no arm left to cover. Verified equivalent to the old ternary for all 32 values.

This is better code regardless of coverage — the reader no longer has to know about shift masking to see that it is right. I should have reached for it instead of writing seven lines of comment defending a half-covered ternary.

PendingSend.cs — PendingKey.Equals now tested directly

This needed InternalsVisibleTo on CanKit.Pro.RawCan. My objection that this "widens the assembly surface" did not survive checking: CanKit.Pro.CANopen and CanKit.Pro.J1939Tp both already do exactly this. It is the house convention, not a concession.

The gap was real, and why it stayed open is the interesting part: PendingKey is only ever reached through a Dictionary, and a dictionary compares hashes first — so two keys differing in ID, flags or frame kind land in different buckets and Equals is never called. The bus-level echo tests cover the behaviour and structurally cannot touch the comparison that implements it. A bug in one of those short-circuit arms would surface only as a hash collision resolving to the wrong send: rare, non-deterministic, and diagnosed on the wire hours later — precisely the failure mode this key was introduced to prevent. So "unreachable in practice" was exactly backwards as a reason to skip the test.

Fourteen cases in PendingKeyTests.cs:

• each identity component varied on its own — ID, Ext, Rtr, Error, frame kind, payload content, payload length, empty payload — with symmetry asserted
• Brs and Esi deliberately not changing identity, which is the counterpart claim the type's own remarks make and which nothing checked
• ForEchoLookup agreeing with ForPendingSend for the same frame (if they disagreed, no echo would ever confirm anything)
• ForPendingSend copying the payload, so a recycled adapter buffer cannot repoint a stored key
• the Equals(object?) override agreeing with the typed one
• the distinctness the FIFO relies on, over a real dictionary

Measured

file	patch lines missing	partial
CanBusService.cs	0	0
CanBusServiceExtensions.cs	0	0
CanIdFilter.cs	0	0
FilterOverlap.cs	0	0
PendingSend.cs	0	0
Subscription.cs	0	0
IsoTpChannel.cs	0	0

Intersected against the lines this PR actually adds or changes, from the same tree the coverage was collected on. The measurement aggregates per filename across all types including nested ones — the bug that produced my two earlier wrong answers.

CanIdFilter.cs still has one uncovered line and one partial branch overall (lines 197 and 181), but both are pre-existing and outside this diff, which is why they do not appear above and did not appear in the patch report either.

Gate: build 0 warnings / 0 errors with CI=true, suite 614/614, format clean, 9 packages verified, release config loadable.

---

Generated by Claude Code

[dborgards]

CodeQL on 3a0cedd: 1 error + 1 warning fixed, 3 notes stood down — 83b7c59

Both blocking alerts were on the same two assertions in the new PendingKeyTests:

severity	line	rule
error	129	Equals on incomparable types — PendingKey vs String
warning	130	Null argument to Equals(object) — never true

Both rules are right about the code and wrong about the intent: those assertions exist to pin the obj is PendingKey guard in the Equals(object?) override — a foreign reference and null must come back false rather than throwing or matching — so "this comparison is always false" is the property under test. The rules are aimed at accidental comparisons in production code, where they earn their keep.

Holding the operands in object? locals says what the assertions mean, "some reference that is not a PendingKey", instead of leaving a string literal in a signature that takes object. Runtime behaviour is identical, so the guard's false arm stays covered — 14/14 in PendingKeyTests, and patch coverage re-measured at 0 missing / 0 partial across every line this PR touches.

Not changed: the three Generic catch clause notes on CanBusServiceExtensions.cs (lines 110, 122, 132). Catching Exception there is the contract — an arbitrary caller-supplied onNext or onError must not be able to take the delivery pump down with it, and FR-RAW-011's isolation guarantee depends on exactly that. Each of the three already says so in place. Narrowing them would mean choosing which subscriber bugs are allowed to kill an unrelated subscription, which is the opposite of what this layer promises. They are notes, not blocking, and I would rather leave them visible than suppress them.

Also worth recording, since it is the first time on this branch: macos-latest was green on 3a0cedd, along with ubuntu, windows (incl. net48), format, version, release-config, pack and Bugbot. That is consistent with the reading on #92 — those failures track runner load, not this diff — and #101 fixes the two tests that were red on main.

Gate on 83b7c59: build 0 warnings / 0 errors with CI=true, suite 614/614, format clean.

---

Generated by Claude Code

[dborgards]

83b7c59: everything green except macos-latest, which failed on a fifth distinct test

CodeQL is green — the two alerts are gone and the three Generic catch clause notes are informational as expected. codecov/patch green, ubuntu-latest green, windows-latest green (incl. net48), plus build, format, version, release-config and Analyze C#.

macos-latest failed 613/614 on:

J1939TpTests.Parallel_Bam_And_TwoCm_Sessions_Do_Not_Interfere
System.TimeoutException : Operation timed out after 00:00:05.
  at J1939TpTests.WithTimeout(Task task, TimeSpan timeout) J1939TpTests.cs:1443

Not this PR's, and this time it is easy to say why without hand-waving: the test lives in CanKit.Pro.J1939Tp and asserts that a BAM session and two CM sessions running in parallel complete within a hard 5-second budget. This PR touches CanKit.Pro.RawCan's TX-confirm FIFO and dispatch. Nothing in the diff is on the path to a J1939 transport-protocol session, and the failure mode is a wall-clock budget expiring, not an assertion about behaviour.

No fix exists to port. #101 fixes two macOS-red tests — StartPeriodicSend_SingleFrame_FiresAtConfiguredPeriod and ClaimAddressAsync_CancelDuringArbitration_TearsDownPendingClaim — and this is neither of them. I am deliberately not adding it there: #101's argument is that those two asserted properties they could not measure, whereas this one is a hard timeout on a parallel-session test in a different package. Folding a third, differently-shaped problem into it would weaken the case for both. It belongs to #92 step 2, where the durable answer is a virtual clock rather than a larger number.

This is now the fifth distinct test in this family, and the pattern that matters is that it is a different one each time:

head	failing test(s) on macos-latest
e2a8a09	UdsClientTests.TimedOut_Request_Does_Not_Poison_Next_…
f75b40f attempt 1	UdsClientTests.TimedOut_Request_Does_Not_Poison_Next_…
f75b40f attempt 2	J1939NodeTests.ClaimAddressAsync_CancelDuringArbitration…, J1939NodeTests.StartPeriodicSend_MultiFrame_…
3a0cedd	(green)
83b7c59	J1939TpTests.Parallel_Bam_And_TwoCm_Sessions_Do_Not_Interfere

Five heads of one branch, five different outcomes including one clean pass, and main itself red on a sixth test. A defect introduced by this diff does not behave like that.

The one allowed re-run is unspent on this head, and I am spending it now. Recorded on #92 as the fifth distinct test.

---

Generated by Claude Code

[dborgards]

Re-run green — macos-latest passed on attempt 2 of 83b7c59, 614/614.

All 12 checks now green on this head: ubuntu-latest, windows-latest (incl. net48), macos-latest, CodeQL, codecov/patch, build, format, version, validate release config, Analyze C#, pack (dry run), Cursor Bugbot.

That is also the second data point closing out the argument above: 3a0cedd passed macOS clean, 83b7c59 failed once on J1939TpTests.Parallel_Bam_And_TwoCm_Sessions_Do_Not_Interfere and then passed on the same commit. Same binary, different outcome — the failure tracks runner load, not the diff. Recorded on #92; no further re-runs will be spent here.

Nothing left outstanding on this PR: all review threads resolved, patch coverage 0 missing / 0 partial, suite 614/614 locally.

---

Generated by Claude Code