This repository was archived by the owner on Jul 5, 2026. It is now read-only.
actions: ActionManager finished-event must start SET to match count=0 invariant - #279
Open
Maurotb wants to merge 1 commit into
Open
actions: ActionManager finished-event must start SET to match count=0 invariant#279Maurotb wants to merge 1 commit into
Maurotb wants to merge 1 commit into
Conversation
… invariant `ActionManager.__init__` creates `_ongoing_actions_finished_event` with `asyncio.Event()` (default UNSET) while `_ongoing_actions_count` starts at 0. The intended invariant is "the event is set iff `_ongoing_actions_count == 0`" — so at construction the event must start SET. Otherwise an immediate `_maybe_wait_for_ongoing_actions_to_finish` blocks forever. This bites when a node's first `pre_action` (e.g. `tts_say`) early-returns without incrementing the counter: empty/whitespace text after templating (a conditional that collapses to an empty string is the common case), a handler exception caught before `_increment_ongoing_actions_count`, etc. With the counter at 0 and no later decrement to set the event, the next `set_node` hangs on the wait — the LLM never registers the new node's tools, and any context aggregator pinned to the previous node's tool names ends up in a hallucination loop. Two-line fix: call `.set()` on the event right after construction. Adds two pin tests: * `test_finished_event_is_set_at_construction` — pins the invariant itself (count == 0 ↔ event set). * `test_tts_action_with_empty_text_does_not_hang_set_node` — reproduces the concrete trigger end-to-end: empty-text `tts_say` followed by `_maybe_wait_for_ongoing_actions_to_finish` returns immediately instead of hanging on `asyncio.wait_for(..., timeout=1.0)`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ran into a hard-to-debug hang where
set_nodewould block forever after a node whose firstpre_actionwas atts_saywith an empty text — the empty text was the result of upstream string interpolation collapsing to"".Traced it to
ActionManager.__init__:The invariant elsewhere in the class is "event is set iff
_ongoing_actions_count == 0" — see_increment_ongoing_actions_count(clears the event) and_decrement_ongoing_actions_count(sets it when count hits 0). At construction the count is 0, so the event should start SET to keep the invariant. As-is, an immediate_maybe_wait_for_ongoing_actions_to_finishblocks onasyncio.Event.wait()forever.Normally this isn't observable because every action increments-then-decrements, so the event gets set by the first decrement. But if the very first action's handler early-returns without incrementing —
_handle_tts_actiondoes exactly that on empty text:— then the event stays UNSET forever and the wait hangs. The user-visible symptom is the next
set_nodenever completing: tools never re-register, the LLM keeps seeing the old node's tool names, and you get a hallucination loop on tool calls that "exist" but aren't actually available.Two-line fix: call
.set()on the event right after construction so the invariant holds at t=0. Behaviour for the well-formed case (every action increments then decrements) is unchanged — the first_incrementclears the event before any wait could happen.Test plan
test_finished_event_is_set_at_construction— pins the invariant itself (count == 0 ↔ event set).test_tts_action_with_empty_text_does_not_hang_set_node— reproduces the concrete trigger end-to-end: an empty-texttts_sayfollowed by_maybe_wait_for_ongoing_actions_to_finishreturns immediately instead of hanging (asserted withasyncio.wait_for(..., timeout=1.0)).pytest tests/test_actions.py→ 14 passed (12 existing + 2 new).ruff check+ruff format --checkclean.Compat
Behaviour-narrowing for the broken case (event correctly reflects "no ongoing actions" at boot), no change for the well-formed case (counter increments before any wait, event gets cleared as usual). No new state, no new dependency. The only callers that could notice are tests/code that explicitly assert
event.is_set() is Falseimmediately afterActionManager(...)— I couldn't find any.