Skip to content

fix(reliability): coalesce BusStateMonitor's error-frame rechecks - #87

Merged
dborgards merged 1 commit into
mainfrom
fix/busstate-storm-coalescing
Sep 10, 2026
Merged

dborgards merged 1 commit into
mainfrom
fix/busstate-storm-coalescing

Conversation

@dborgards

Copy link
Copy Markdown
Owner

Closes #22.

BusStateMonitor subscribes ErrorFrameReceived/FaultOccurred as low-latency hints and posted
one recheck per hint. A bus-off or error-passive storm raises those thousands of times per second —
far faster than the loop drains them — so a transient bus fault became an unbounded mailbox backlog
that starves exactly the protocol work a BusOff exists to abort (FR-RAW-020..022), and fed
straight into the unbounded drain of #21.

What is coalesced

Hint posts, not state transitions. At most one un-run hint recheck is outstanding in the
mailbox at any time; hints arriving while one is queued are dropped rather than posted
(Interlocked.CompareExchange gate, released by the recheck itself).

This is lossless with respect to what the monitor reports, because a recheck is a sample of a
level
— ICanBus.BusState is a plain getter with no change event — and not the delivery of a
queued event. N back-to-back samples of an unchanged level yield exactly what one sample yields.
What is dropped is redundant mailbox traffic.

The gate is released before the sample is taken. That ordering is the correctness argument: a
hint raised while the recheck is in flight then claims the gate again and posts a follow-up, so the
last hint of a storm is always succeeded by a sample taken after it. Releasing afterwards would let
that hint be dropped and push the state change it announced out to the next poll tick — turning the
hints' latency guarantee back into a poll-interval one at the exact moment it matters most. There
is a dedicated test for that ordering (see below).

What is deliberately preserved, and what is not

Preserved:

  • The transition sequence. Every edge the monitor samples is still raised as its own
    StateChanged, in order, with Previous chained to the last reported state. A subscriber never
    sees a gap, a re-ordering, or a wrong Previous.
  • CurrentState convergence. Because the gate is released before sampling, the final state of
    a storm is always observed by a hint-driven recheck, not merely by the next poll.
  • The poll floor. The self-rearming 50 ms poll is untouched — hints remain a pure latency
    optimisation, and an adapter that refuses the subscriptions still degrades to poll-only.

Not preserved (and never was):

  • A per-error-frame count. An error frame is not a state transition, and nothing public ever
    exposed a count of them. BusStateMonitor's only observable is StateChanged.
  • Intermediate levels of a cascade faster than the sampling rate. If the controller passes
    through ErrWarning and ErrPassive on its way to BusOff between two samples, one
    ErrActive → BusOff edge is reported. This was already true before this change — a hint says
    "something happened", not "this transition happened", so whether the intermediate levels were
    caught depended on whether a redundant sample happened to land on them. It is also exactly what
    happens today on the poll-only path (AllowErrorInfo=false). The honest statement is that the
    monitor is a sampler; the observable difference is that the storm no longer buys extra samples by
    flooding the mailbox.

Why no opt-out was added: the existing public knob already covers the only legitimate need. A
consumer who wants finer sampling granularity shortens pollInterval, which raises the sample rate
deterministically. An opt-out would instead be a switch labelled "please flood my mailbox", whose
sampling benefit is incidental and load-dependent, and it would grow the public surface of a
shipping package to preserve a side effect of a bug.

Public API

Unchanged. No new type, member, parameter or overload; ApiApprovals/CanKit.Pro.Reliability.approved.txt
is untouched, and both netstandard2.0 and net10.0 assets build as before. No new clock or
timing primitive was needed, so nothing here depends on TickCount64 or anything else missing on
netstandard2.0.

Tests

Three tests in BusStateMonitorTests, driven through a new private MailboxActor double — an
IProtocolActor that only queues on Post, drains when the test says so, and returns a Schedule
handle that never becomes due. That removes the self-rearming poll from the picture, so every post
counted is hint-driven and no assertion depends on machine speed. There is no Task.Delay and no
wall-clock synchronisation anywhere in them.

  1. An_Error_Frame_Storm_Coalesces_Into_One_Pending_Actor_Post — 1000 error frames in, exactly one
    post and a mailbox depth of one out.
  2. The_Coalesced_Recheck_Reports_The_Transition_And_Reopens_The_Gate — the storm's transition is
    still reported exactly once with the right Previous/Current, and a second storm after the
    recheck has run posts again (the gate is a window, not a one-shot latch).
  3. A_Hint_Arriving_While_The_Recheck_Is_In_Flight_Posts_A_Follow_Up — a hint raised from inside
    the StateChanged handler, i.e. in the one window where release ordering decides whether a
    transition can be lost, must post a follow-up that observes it.

How the tests were verified to fail without the fix

  • Reverted BusStateMonitor.cs to origin/main and re-ran: tests 1 and 2 go red with
    found 1000 against expected 1 (post count, and drained work items). Test 3 passes there, as
    it must — with no coalescing at all nothing can be swallowed.
  • Test 3 guards the release ordering, so it was verified against a mutant of the fix instead:
    swapping HintRecheckOnLoop to release the gate after RecheckOnLoop() makes it go red with
    Expected actor.MailboxDepth to be 1 ... but found 0, i.e. the in-flight hint was swallowed.
    The other two tests stay green under that mutant, so each of the three fails for its own reason.

Full suite: dotnet test CanKit.Pro.sln -c Release → 412 passed, 0 failed. The net48 leg was
compile-checked with dotnet build tests/CanKit.Pro.Tests -f net48 -p:CanKitProTestNetFrameworkLeg=true -c Release
(0 warnings, 0 errors); it cannot be executed on macOS.

Test-infrastructure notes

  • ControllableBus.RaiseErrorFrame(...) added — ErrorFrameReceived had no way to be raised, and
    its #pragma warning disable CS0067 // Never raised: nothing in CanKit.Pro subscribes to these
    comment was already stale, since BusStateMonitor does subscribe to it. Moved out of that group.
  • StubErrorInfo added: CanKit's own ICanErrorInfo implementation is internal to CanKit.Core,
    so there is nothing public to construct, and passing null into an event whose contract says it
    carries error info would quietly excuse a subscriber that dereferences it.

Relationship to #21

Complementary, and no files overlap. This PR reduces what is produced into the mailbox; #21
bounds how it is drained. The produce side is the right place for this particular fix: the
information content of N hint posts is identical to that of one, so dropping them costs nothing
that a drain-side bound could give back — a bounded drain would still have to carry, order and
eventually discard 1000 identical work items per storm-millisecond.

Defect noticed but not fixed

dotnet format CanKit.Pro.sln --verify-no-changes fails on main (whitespace in
TestCases/Uds/UdsTransferTests.cs, import ordering in UdsTransferTests.cs, UdsClientTests.cs
and Nfr006ErrorArchitectureTests.cs). Pre-existing, unrelated to this change, and left alone.

🤖 Generated with Claude Code

BusStateMonitor subscribes ErrorFrameReceived/FaultOccurred as low-latency
hints and posted one recheck per hint. A bus-off or error-passive storm raises
those thousands of times per second, far faster than the loop can drain them,
so a transient bus fault turned into an unbounded mailbox backlog — starving
exactly the protocol work the state change exists to abort (FR-RAW-020..022).

At most one un-run hint recheck is now outstanding: hints arriving while one is
queued are dropped instead of posted. That is lossless for what the monitor
reports, because a recheck is a sample of a level (ICanBus.BusState is a plain
getter), not the delivery of a queued event — N back-to-back samples of an
unchanged level say what one says. The gate is released before the sample is
taken, so a hint racing an in-flight recheck posts a follow-up and the last hint
of a storm is always succeeded by a sample taken after it; releasing afterwards
would push that hint's state change out to the next poll tick.

What is deliberately not preserved is a per-error-frame count: an error frame is
not a state transition, and the hints were never a transition log. Every edge
the monitor samples is still raised individually and in order, with Previous
chained to the last reported state. Sampling granularity is unchanged and still
tuned the way it always was, via pollInterval.

Public surface is untouched: no new type, member or parameter.

Tests drive the monitor through a queue-only IProtocolActor double whose
Schedule never becomes due, so the poll is out of the picture and post counts
are exact without any wall-clock waiting.

Closes #22

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches concurrency and actor mailbox behavior on the bus-degradation path (FR-RAW-051); behavior is narrowed (fewer posts) with explicit ordering guarantees and tests, but mis-coalescing could delay state observation.

Overview
Fixes mailbox flooding during CAN error-frame storms (#22): BusStateMonitor used to Post one actor recheck per ErrorFrameReceived/FaultOccurred hint, which could enqueue thousands of redundant jobs per second and starve protocol work on a BusOff.

Coalescing caps hint-driven traffic at one outstanding recheck via an Interlocked gate (_recheckPending). Extra hints while a recheck is queued are dropped; HintRecheckOnLoop releases the gate before sampling BusState, so hints that arrive during an in-flight recheck can still post a follow-up. Poll-based monitoring and public API are unchanged; docs in BusStateMonitor and the Reliability README describe the semantics.

Tests add ControllableBus.RaiseErrorFrame, StubErrorInfo, a deterministic MailboxActor test double, and three storm/coalescing tests (post count, transition reporting, in-flight follow-up).

Reviewed by Cursor Bugbot for commit a68994e. Bugbot is set up for automated code reviews on this repo. Configure here.

@dborgards
dborgards merged commit ae87bd7 into main Sep 10, 2026
11 checks passed
@dborgards
dborgards deleted the fix/busstate-storm-coalescing branch September 10, 2026 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BusStateMonitor: one actor post per error frame floods the mailbox during a bus-off storm

1 participant