Skip to content

fix: keep event order, measure delays on the monotonic clock, cancel by marking - #665

Open
JMerrill-Fairness wants to merge 8 commits into
fgmacedo:developfrom
FairPlay-AI:fix/event-order-and-monotonic-clock
Open

JMerrill-Fairness wants to merge 8 commits into
fgmacedo:developfrom
FairPlay-AI:fix/event-order-and-monotonic-clock

Conversation

@JMerrill-Fairness

@JMerrill-Fairness JMerrill-Fairness commented Oct 4, 2026 •

Copy link
Copy Markdown

Three fixes to how the engines order, time and cancel events, and a refactor that removes every suppression from the modules they touch. Each is its own commit, and they can be split into separate PRs if you prefer.

Events due at the same moment kept their order only by chance

Both queues are PriorityQueues ordered by TriggerData.execution_time alone, so triggers with equal times come out in heap order, not the order they were put. A clock coarse enough to stamp two events alike runs them out of order. With every trigger stamped alike, twenty events raised in one callback ran as 0, 2, 6, 14, 19, 18, ..., on both engines and both queues.

TriggerData now numbers each trigger as it is created (itertools.count) and compares that number after execution_time, so triggers due together keep their order.

Delays were measured on the wall clock

Triggers were stamped with time.time() plus their delay and held until time.time() reached the stamp, so a step of the wall clock moved every due time. Stepped back by a time sync, it held even an undelayed event until the clock caught up. Stepped forward, it ran delayed events early. Stamps and the due checks in both engines now use time.monotonic().

Cancelling a delayed event could disorder the others and strand its caller

EventQueue.remove() filtered the list behind the heap, and a filtered heap is not always a heap. Of ten delayed events due from 10 to 90 ms, cancelling the first ran the event due at 50 ms before the one due at 40 ms. The trigger's future was dropped with it, so on the async engine a coroutine awaiting the cancelled event's send() was never answered. Both happen on 3.2.1.

cancel_event() now marks each trigger sent with the send_id as cancelled, in a new TriggerData.cancelled field, and answers its future with None. The triggers stay where they are, so nothing rearranges the heap from outside PriorityQueue, and EventQueue.is_empty() drops cancelled triggers from the head, so the processing loop never checks whether one is due. EventQueue.remove() is renamed cancel().

The suppressions in the touched modules

No type: ignore, noqa, pragma: no cover or pragma: no branch is left in engines/base.py, engines/sync.py, engines/async_.py or statemachine.py. Each was replaced by fixing what it covered:

  • BaseEngine declared select_transitions, select_eventless_transitions, _select_transitions, _first_transition_that_matches, microstep and _exit_states as synchronous, and AsyncEngine overrode them with coroutines, which is what the four type: ignore[override] hid. They now belong to SyncEngine, and BaseEngine keeps what both engines share.
  • Each processing loop is split into its macrostep, the external events and one external event, and add_descendant_states_to_enter into its history, region and initial-state parts, which takes every function under the complexity limit.
  • Removed as unreachable: BaseEngine.empty() and pop(), which nothing called; the _run_microstep() wrappers, since microstep() already sends every error through _handle_error(); the second pass over the internal queue, since the macrostep empties it and spawning invocations puts events only on the external queue; and the checks for an exited state being None.
  • SyncEngine.processing_loop() no longer takes a caller_future it never used. Only AsyncEngine.put() attaches a future to an event, so StateChart._processing_loop() now passes one to the async engine alone.
  • is_in_final_state() takes the parallel case first, so an atomic state needs no branch of its own.
  • StateChart: a cast for the default Model, vars(self).update(), an assert that narrows initial_state (as factory.py does), and _repr_svg_() through the diagram formatter, which renders SVG the same way.

Two lines the pragmas hid now have tests: stop() still stops the engine when cancelling invocations fails, and a transition into a parallel state and a state inside one of its regions enters that state and not the region's initial one.

Tests

  • tests/test_event_ordering.py stamps every trigger with one time (by patching statemachine.event_data.monotonic) and checks the order on both engines and both queues, for 0, 1, 2 and 20 events. It failed before its fix.
  • tests/test_cancel_event.py runs every test on a clock that moves only when the test moves it.
    • Cancelling the first, a middle or the last of ten delayed events, or the last one queued, leaves the rest in due order.
    • A send_id cancels every event sent with it and nothing else. It is matched exactly: empty, whitespace, line endings, NUL, case, a zero-width space, a byte-order mark, composed against decomposed accents, and ids up to 2048 characters. It never cancels an event sent without one, and a cancel with nothing queued cancels nothing sent later.
    • The loop never reads the clock for a cancelled event, cancel_event() works from inside a callback, and a cancelled trigger is marked rather than removed.
    • A coroutine awaiting a cancelled event gets None, whether it is cancelled once or twice, and a caller that stops waiting, before or while it awaits, leaves its event cancellable.
    • The order test and the awaiting test failed before the fix.
  • tests/test_statechart_delayed.py checks that no trigger is stamped or checked on the wall clock (reading time.time fails the test), that the engines read time.monotonic, and that a delay in milliseconds comes due that many thousandths of a second later, for no delay, 0, 1, 50 and -50.
  • tests/test_statechart_delayed.py also queues a delayed event from a callback, on a clock that moves 10 ms with every reading, and checks that a coroutine that stops awaiting a delayed event, before or while it awaits, does not stop it firing in due order.
  • tests/test_invoke.py::TestEngineStopCleanup and tests/scxml/test_parallel_regions.py cover the two lines the pragmas hid.
  • Each test was checked against mutants of the code it covers: 40 in all, every one failed by at least one test without waiting for a timeout.
  • The full suite passes with --cov-fail-under=100, along with ruff, mypy and pyright, through the pre-commit hooks.

Release notes are in docs/releases/3.2.2.md.

🤖 Generated with Claude Code

JMerrill-Fairness and others added 8 commits October 4, 2026 14:00
Both queues are PriorityQueues ordered by TriggerData's execution_time
alone, so triggers with equal times came out in heap order rather than
the order they were put. Two events stamped alike by a coarse clock could
run out of order; twenty raised in one callback with one stamp ran as
0, 2, 6, 14, 19, 18, ...

TriggerData now numbers each trigger as it is created and compares that
number after execution_time, so triggers due together keep their order.
The new tests stamp every trigger with one time and check the order on
both engines and both queues.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Triggers were stamped with time.time() plus their delay and held until
time.time() reached the stamp, so a step of the wall clock moved every
due time: stepped back, it held even an undelayed event until the clock
caught up; stepped forward, it ran delayed events early. Stamps and the
due checks in both engines now use time.monotonic().

The new delayed-event test drives a fake monotonic clock in all three
modules, so a stamp on one clock checked against another would hold the
event and fail. The ordering tests now fix the monotonic clock instead
of the wall clock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EventQueue.remove() filtered the PriorityQueue's underlying list. A
filtered heap is not always a heap, so the triggers left could come out
of due order: of ten delayed events due from 10 to 90 ms, cancelling the
first ran the event due at 50 ms before the one due at 40 ms. The
trigger's future was dropped with it, so on the async engine a coroutine
awaiting the cancelled event's send() was never answered.

cancel_event() now marks each trigger sent with the send_id as cancelled
and answers its future with None. The triggers stay where they are, so
the heap is never rearranged from outside, and is_empty() drops a
cancelled trigger when it reaches the head, before the processing loop
could check whether it is due.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tests drawn from the change's own code rather than its description:

- cancel_event() called from a callback, on both engines, and a caller
  that stops waiting, before or while it awaits, leaving its event
  cancellable;
- a send_id matched exactly: empty, whitespace, line endings, NUL, case,
  a zero-width space, a byte-order mark, composed against decomposed
  accents, and ids up to 2048 characters, none of which cancels an event
  sent without one;
- a cancel with nothing queued cancels nothing sent later, two ids
  cancelled in turn cancel each, and a cancelled trigger is marked
  rather than removed;
- the order test cancels the first, a middle and the last event due, and
  the last one queued;
- no trigger is stamped or checked on the wall clock, the engines read
  time.monotonic, and a delay in milliseconds comes due that many
  thousandths of a second later, for none, 0, 1, 50 and -50;
- events due together keep their order for 0, 1, 2 and 20 of them.

Each was checked against mutants of the code it covers: 31, every one
failed by at least one test without waiting for a timeout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SonarCloud's S9083 asks for `@pytest.fixture` without parentheses, and
this project's ruff configuration (fixture-parentheses = true) asks for
them. The two clock fixtures are now `autouse=True`, which both accept:
every test in each module runs on the substituted clock, so none reads
the real one, and a test that moves the clock asks for it by name.

The send_ids that held a zero-width space, a byte-order mark and
accents as literal characters are escapes again, so the file is ASCII.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A delayed event queued from a callback, while the processing loop runs,
comes due on a clock that moves 10 ms with every reading, on both
engines. A coroutine that stops awaiting a delayed event, before or
while it awaits, does not stop the event, which fires in due order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No `type: ignore`, `noqa`, `pragma: no cover` or `pragma: no branch` is
left in base.py, sync.py, async_.py or statemachine.py.

- BaseEngine declared select_transitions, select_eventless_transitions,
  _select_transitions, _first_transition_that_matches, microstep and
  _exit_states as synchronous, and AsyncEngine overrode them with
  coroutines, which mypy rejected. They now belong to SyncEngine, and
  BaseEngine keeps what both engines share.
- Each processing loop is split into its macrostep, the external events
  and one external event, and add_descendant_states_to_enter into its
  history, region and initial-state parts.
- Removed as unreachable: BaseEngine.empty and pop, which nothing
  called; the _run_microstep wrappers, since microstep already sends
  every error through _handle_error; the second pass over the internal
  queue, since the macrostep empties it and spawning invocations puts
  events only on the external queue; and the checks for an exited state
  being None.
- is_in_final_state takes the parallel case first, so an atomic state
  needs no branch of its own.
- StateChart: a cast for the default Model, vars(self).update, an
  assert that narrows initial_state, and _repr_svg_ through the diagram
  formatter, which renders SVG the same way.

New tests cover the two lines the pragmas hid: stop() still stops the
engine when cancelling invocations fails, and a transition into a
parallel state and a state inside one of its regions enters that state
and not the region's initial one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SyncEngine.processing_loop took a caller_future it never used: only
AsyncEngine.put attaches a future to an event, so the sync engine was
always handed None. StateChart._processing_loop now passes the future
to an AsyncEngine and calls the sync loop without one, and the sync
loop no longer takes it (SonarCloud python:S1172).

The new SCXML test marks its module with `pytestmark` rather than
`@pytest.mark.scxml()`, which this project's ruff configuration
(mark-parentheses = true) and SonarCloud's python:S9083 disagree on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Oct 4, 2026

Copy link
Copy Markdown

This branch has not been deployed

No deployments
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.

1 participant