Add end-to-end conductor parity tests, and fix the divergence they found - #763
Merged
wbarnha merged 1 commit intoAug 7, 2026
Conversation
The topic conductor is the per-message inner loop of a worker, and it exists twice: `ConductorHandler` in the extension, and the `on_message` closure from `ConductorCompiler.build`. Neither was covered -- the existing conductor tests replace the handler with an AsyncMock and assert it was called, so the fan-out, event reuse, buffer-pressure callbacks, full-queue path and decode-error propagation were untested on both sides. Both handlers take `(conductor, tp, channels)` and are awaited with a Message, so they can be driven over the same input and compared. tests/unit/transport/test_conductor_parity.py does that for each of those paths and diffs a full record of the outcome: which events reached which channels, refcount and acked state, decode counts, and every sensor and consumer callback. Both implementations run against the *same* conductor and the same `channels` set, one after the other, rather than two separately-built environments. `channels` is a set of Topic objects hashed by identity, so two separate sets iterate in unrelated orders, and anything order-sensitive -- which channel decodes first, which ones a mid-fan-out decode error reaches -- would differ for reasons unrelated to the implementations. Sharing the set removes that variable; `reset()` clears queues and recorded callbacks between runs. ## What it found `ConductorHandler` never reused a decoded event. The conductor is supposed to deserialize once and reuse it for every channel whose `(key_type, value_type)` matches, but `event_keyid` was only ever assigned from `_decode()`, which returned it *unchanged* on the first pass. It stayed None forever, the reuse branch was dead, and every subscribed channel re-deserialized the payload. That masked a second fault. Had the keyid ever been set, a mismatched pair fell off the end of `_decode` and returned a bare None, which unpacking into two names raises TypeError on. Fixing the reuse alone would have turned a silent inefficiency into a crash on any topic whose subscribers declare different key or value types -- confirmed by building that partial fix and watching the new heterogeneous-keyid test fail with `TypeError: 'NoneType' object is not iterable` at the unpack. This is the same double-bug shape as `_try_get_quick_value` in streams.pyx, arrived at independently: a dead optimization whose deadness concealed that it was also wrong. It was not only a performance difference. A channel whose event is reused never calls `decode`, so a channel that would have failed to deserialize raised no error under the pure-Python conductor and raised one under the extension -- changing which channels received the message and how many acks it got. The fix ports conductor.py's loop faithfully: `event`/`event_keyid` stay pinned to the first channel, and a channel with a different pair gets its own `dest_event` without displacing the pinned one. `_decode` is gone; `keyid` and `dest_event` were already declared in `__call__` and unused, which suggests this is what it was meant to be. ## Verification 16 parity tests, covering fan-out over 1/2/3 channels, no subscribers, batches, event reuse for matching keyids, per-channel decode for differing keyids, decode errors (whole fan-out and single channel), the full-queue path and the pressure callbacks. Before the fix 6 of them failed, all tracing to that one root cause; after it, all pass. Full suite green in every configuration: extensions built (2270 passed), absent (2207 passed, parity skipped), NO_CYTHON=1, and free-threaded 3.14t under PYTHON_GIL=0 (2274 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8qT5E3rnSXvw7ibNLXrVr
This was referenced Aug 7, 2026
Member
Author
|
Folded into #762 — the whole series is now one branch, so there is no stack to land in order. The commit ( Generated by Claude Code |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Closes the gap left by #762: the topic conductor — the per-message inner loop of a worker — exists twice and neither copy was covered. The existing conductor tests replace the handler with an
AsyncMockand assert it was called, so the fan-out, event reuse, buffer-pressure callbacks, full-queue path and decode-error propagation were untested on both sides.Both handlers take
(conductor, tp, channels)and are awaited with aMessage, so they can be driven over the same input and compared.tests/unit/transport/test_conductor_parity.pydoes that for each of those paths and diffs a full record of the outcome: which events reached which channels, refcount and acked state, decode counts, and every sensor and consumer callback.Both implementations run against the same conductor and the same
channelsset, one after the other, rather than two separately-built environments.channelsis a set ofTopicobjects hashed by identity, so two separate sets iterate in unrelated orders — anything order-sensitive (which channel decodes first, which ones a mid-fan-out decode error reaches) would then differ for reasons unrelated to the implementations. Sharing the set removes that variable;reset()clears queues and recorded callbacks between runs.What it found
ConductorHandlernever reused a decoded event. The conductor is supposed to deserialize once and reuse it for every channel whose(key_type, value_type)matches, butevent_keyidwas only ever assigned from_decode(), which returned it unchanged on the first pass. It stayedNoneforever, the reuse branch was dead, and every subscribed channel re-deserialized the payload.That masked a second fault. Had the keyid ever been set, a mismatched pair fell off the end of
_decodeand returned a bareNone— which unpacking into two names raisesTypeErroron. Fixing the reuse alone would have turned a silent inefficiency into a crash on any topic whose subscribers declare different key or value types. Confirmed by building that partial fix and watching the new heterogeneous-keyid test fail withTypeError: 'NoneType' object is not iterableat the unpack.This is the same double-bug shape as
_try_get_quick_valueinstreams.pyx(#762), arrived at independently: a dead optimization whose deadness concealed that it was also wrong.It was not only a performance difference. A channel whose event is reused never calls
decode, so a channel that would have failed to deserialize raised no error under the pure-Python conductor and raised one under the extension — changing which channels received the message and how many acks it got.The fix ports
conductor.py's loop faithfully:event/event_keyidstay pinned to the first channel, and a channel with a different pair gets its owndest_eventwithout displacing the pinned one._decodeis gone;keyidanddest_eventwere already declared in__call__and unused, which suggests this is what it was meant to be.Verification
16 parity tests: fan-out over 1/2/3 channels, no subscribers, batches, event reuse for matching keyids, per-channel decode for differing keyids, decode errors (whole fan-out and single channel), the full-queue path, and the pressure callbacks.
Before the fix 6 failed, all tracing to that one root cause; after it, all pass. Full suite green in every configuration:
FAUST_REQUIRE_CYTHON=1)NO_CYTHON=1PYTHON_GIL=0flake8/black/isortclean; docs build clean.Note on the base branch
Based on
claude/faust-free-threaded-support-tpmqh2(#762), notmaster, because it depends on that branch's in-place extension build andFAUST_REQUIRE_CYTHONguard. Without those, CI never imports the compiled conductor and every comparison here would run the pure-Python implementation against itself — the same reason the parity tests in #751 note they otherwise "just run the pure-Python one twice". Retarget tomasteronce #762 lands.Generated by Claude Code