fix(bus): synchronous handler registration to de-flake round-trip tests - #59
Merged
Merged
Conversation
…rips `Bus::handle_requests` registered its handler on a detached `tokio::spawn` and returned before the spawned task had run. A request dialed to that topic in the intervening window found no handler in `Inbox::dispatch_request`, which silently returns `Ok(None)` (no reply) — so the asker just timed out. The direct-dial round-trip tests papered over the race with a fixed `sleep(200ms)` (and the in-memory unit test with four `yield_now()`s): a timing crutch that can still lose the race on a loaded CI runner. Root-cause fix: the `handlers` map only ever holds its lock for a brief `get().cloned()` / `insert()` (never across an `.await`), so it does not need `tokio::sync::RwLock`. Switch it to `std::sync::RwLock`, make `Inbox::register_handler` synchronous, and have `handle_requests` install the handler before it returns. No spawn, no window, no sleep. - inbox.rs: `handlers` -> `std::sync::RwLock`; `register_handler` is now a sync `fn`; add `handler_count()` diagnostic accessor. - bus.rs: `handle_requests` registers synchronously; drop the 4x `yield_now()` crutch in the in-memory round-trip test. - Remove the now-needless `sleep(200ms)` from the deterministic direct-dial round-trips (bus_roundtrip.rs, mcp_stdio.rs). Regression test `handle_requests_registers_synchronously_no_spawn_race` asserts the handler is registered the instant `handle_requests` returns, with no yield in between. On a current-thread runtime this deterministically fails on the old spawn-based code (count 0) and passes now (count 1). Refs #52 (de-flake portion). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What this PR does
Hardens the #52 de-flake by fixing the root-cause nondeterminism the
direct-dial round-trip tests were papering over with a fixed
sleep.Bus::handle_requestsregistered its handler on a detachedtokio::spawnand returned before that task had run. A request dialed to the topic in the
gap found no handler in
Inbox::dispatch_request, which silently returnsOk(None)(no reply) — the asker just times out. On a loaded CI runner thesleep(200ms)crutch can lose that race; that is precisely the flake class#52 set out to eliminate.
handlersmap only ever holds its lock for a briefget().cloned()/insert()— never across an.await— so it does notneed
tokio::sync::RwLock. Switch it tostd::sync::RwLock, makeregister_handlera synchronousfn, and add ahandler_count()diagnostic accessor.
handle_requestsnow installs the handler before it returns(no spawn, no window). Drops the 4x
yield_now()crutch in the in-memoryround-trip unit test.
sleep(200ms)from the deterministicdirect-dial round-trips (
bus_roundtrip.rs,mcp_stdio.rs).Test plan
handle_requests_registers_synchronously_no_spawn_race(agent-mesh-bus/src/bus.rs): asserts the handler is registered the instant
handle_requestsreturns, with no yield between. Verified itdeterministically fails on the old spawn-based code (
handler_count0vs 1) and passes now.
stable without the timing crutch:
request_reply_roundtrip_via_direct_dial_no_mdns— 30/30mcp_server_direct_addr_round_trips_to_live_responder— 30/30request_reply_roundtrip_over_in_memory_transport(yields removed) — 30/30just check(workspace fmt + clippy -D warnings + test) andjust cov-ci(
--fail-under-lines 75) green locally via the pre-push hook.pyo3feature build clippy-clean.
Out of scope
amesh send --addr/--pubkey,mesh_requestaddr+pubkey) and the initial tiering ofthe mDNS tests already landed on main via feat(cli): direct-address dial for amesh send + pubkey in listen banner (#52) #53/feat(mcp): direct-address dial for mesh_request + de-flake mcp_stdio round-trip (#52) #55/feat(mcp): mesh_publish tool — surface publish_to / publish_to_direct (#56) #57; this PR only
hardens their determinism.
#[ignore]d — they uniquely exercise real multicast discovery.Refs #52 — de-flake portion. The issue's feature scope appears already
complete on main; leaving the close decision to a human rather than
auto-closing.