Repository navigation
Actor/Reliability: minor findings and untested paths from the 2026-10-21 deep review (A-2..A-8) #351
Description
Collected from docs/reviews/2026-10-21-deep-review/actor-reliability.md; file:line against main @ 36c97ca. The Important finding A-1 (second Dispose does not join) has its own issue.
- A-2 (docs/minor)
ClockTimeoutis orphaned by actor disposal:FinalDraindiscards not-yet-due timers, so the token never cancels andWaitUntilAsyncnever completes unless the caller's token does (ClockTimeout.cs:16-45). Both consumers link a lifetime token today (UdsClientImpl.cs:374-376,LssMaster.cs:377-390); nothing documents the requirement. Add a<remarks>. - A-3 (minor, carry-over)
BusStateMonitor.PostRecheckleaves_recheckPendinglatched whenPostthrows anything butObjectDisposedException(BusStateMonitor.cs:235-248); unreachable withProtocolActor, reachable with a foreignIProtocolActor. Release the gate in a rethrowing catch. - A-4 (docs, carry-over) The dispose-timeout
TimeoutExceptionis raised on the disposing thread (ProtocolActor.cs:510-520), concurrently with the callback still running;IProtocolActor.cs:65-73andREADME.md:78-82do not say so, and a subscriber written for the loop then mutates actor state from a second thread. - A-5 (minor/docs, carry-over)
WaitForLoopTask(ProtocolActor.cs:529-542): thecatch (AggregateException)is dead code with a false comment (the loop catches its ownOperationCanceledExceptionat:593-596); a genuinely faulted loop would be reported as "joined". Delete the catch or route the inner exception throughRaiseBackgroundException, and fix the comment. - A-6 (minor, unreachable)
TickMath.DueAtsaturation checkticks >= long.MaxValue - nowoverflows for a negativenow(TickMath.cs:32). - A-7 (docs)
ProtocolActorTests.cs:154-155gives a stale reason forWithinTenSeconds(Polyfill providesWaitAsyncon every target). - A-8 (docs)
ProtocolActor.cs:88-115: the<summary>written forTimeSourceis attached toRunningLoopCount. - Test gaps (reliability-M3/M4/M5 from the previous review, re-run as actor-M4/M5/M6): no
FaultOccurredhint test, no throwing-BusStatetest (thetry/finallyinPollTickis unexercised), no hint afterDispose(handler detach unexercised). Results of the re-run are in the review's mutation table. - Timing:
DeadlineTests.Rearm_After_The_Deadline_Already_Resolved_Returns_Falseis the one wall-clock margin in this area (500 ms real deadline, test-thread stall); move it ontoVirtualClock. - Open question:
FinalDrainfires due timers after the last mailbox item (ProtocolActor.cs:630); every deadline that re-arms in that pass produces anObjectDisposedExceptiononBackgroundExceptionOccurredduring an orderly shutdown. Acceptable noise, or discard due timers like not-yet-due ones?
🤖 Generated with Claude Code
Activity
fix(actor): report a loop fault that arrives after Dispose gave up jo…
test(actor): dispose the actor and the monitor on every path out of t…
dborgards commentedon Oct 27, 2026
Compatibility assessment of the remaining shutdown-policy question, against main d80bf71:
The requested policy is: complete accepted mailbox work, discard every timer callback that has not started once disposal begins. Replacing the current default with this policy is a behavioral breaking change. The published Actor README promises completion of accepted work and specifically discards only not-yet-due scheduled callbacks; FinalDrain deliberately still invokes due timers. Schedule callbacks can complete a caller's task or perform final protocol work. Suppressing them changes that observable result even though no C# signature changes.
A standalone public-API probe was run for DedicatedThread and ThreadPool against the current Actor assembly (Actor source identical to origin/main). Inside one actor callback it queues an accepted PostAsync, schedules a zero-delay callback, then disposes reentrantly. An external Dispose joins the actor as the positive completion witness. Both modes produce:
There are no timing sleeps or negative observation windows in this probe. Thus the shutdown noise is reproduced, and removing the due callback would also remove documented user work, not just the exception.
Per the user's condition “only without a breaking change”, the existing default is left unchanged. Marked BREAKING and left open for a major-version policy change; the previously completed minor fixes are not reimplemented. An additive, explicitly selected shutdown policy could preserve the legacy default, but would be a separate public API/design choice rather than silently changing existing actors. No blanket exception suppression, no PR or release was introduced. Local probe: /private/tmp/cankit-351-shutdown-compat/.
dborgards commentedon Oct 27, 2026
The user has now explicitly authorized the additive opt-in alternative while preserving the existing default. Implemented in #488:
ActorShutdownPolicy.DiscardPendingTimersthrough a new three-argument constructor; the old constructor andDrainDueTimersbehavior remain unchanged. Removed BREAKING for this compatible scope. Full local gates are green (3125 tests, net48, format, nine packages, engineering and traceability); generated API is additive, 18 changed executable entries have no missing/partials, and eight source mutations failed at runtime/restored. Review/CI/Codecov and the unchanged ten-minute quiet period are still required before the authorized merge.