[Bug] Two routers share one Conversation across threads, so every caller gets every task's messages (#2054)

[ayaangazali]

Part of #2054 — the two structures that are a data race. The nine sequential-reuse leaks in that issue are a separate concern and are not in this diff.

What is wrong

MultiAgentRouter and HybridHierarchicalClusterSwarm each build one Conversation in __init__ and write to it from the per-task path. Both also fan that same path across a thread pool:

• hybrid_hiearchical_peer_swarm.py:212 — run_concurrently(self.run, tasks, ...)
• multi_agent_router.py:485 — executor.submit(self.route_task, task)

So N threads append to one object, and each returns history_output_formatter(...) over it. Every caller receives all tasks' messages rather than its own.

Evidence

The same shape in isolation, 8 concurrent callers, printing how many messages each one sees:

shared instance -> [1, 2, 3, 4, 5, 6, 7, 8]
per-call        -> [1, 1, 1, 1, 1, 1, 1, 1]

The shared column is the current behaviour: caller 8 sees the other seven tasks.

Why not just reset it in run()

Resetting the shared Conversation at the top of run() fixes sequential reuse, which is the other half of #2054, but not this. Concurrent callers would still race on the reset itself, and one thread would clear another's in-flight history.

The conversation is per-task state, so it is constructed in the per-task entry point and threaded explicitly into the helpers that record into it (route_task, handle_single_handoff, handle_multiple_handoffs). That fixes the race and removes the sequential leak in these two, since nothing survives the call.

self.conversation is removed from both classes rather than left vestigial. No caller in the repo reads it, and leaving it would be a shared object that looks live but is never written.

Testing

No new test. tests/structs/test_multi_agent_router.py is 22 failed / 1 passed / 1 skipped on master and identical on this branch — the suite needs live credentials, so a concurrency assertion added there would not run in CI. Happy to add one if you would rather have it behind a mocked function_caller.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[kyegomez]

The diagnosis here is right and the approach is the correct one — threading a per-task Conversation rather than resetting a shared one, and your reasoning for why a reset does not fix the concurrent case is exactly right. Two things need doing before this can go in.

1. Conflicts with master

swarms/structs/multi_agent_router.py conflicts since #2087 merged. I resolved it locally to check the damage, and the good news is it is small — two hunks, both just method signatures:

handle_single_handoff:    -> None   (master)  vs  + conversation param, -> dict  (this PR)
handle_multiple_handoffs: -> None   (master)  vs  + conversation param, -> dict  (this PR)

Resolution is to take both sides: keep your conversation: Conversation parameter, keep master's -> None. The -> dict annotation was wrong — both methods return nothing, and their docstrings said so — which is why #2087 corrected it.

Everything else merged cleanly. I verified the resolved file still has all of #2087's fixes intact, none of which are on your branch:

.get("task") instead of ["task"]   2 sites   (KeyError on an absent task key)
executor.map                       1 site    (selected agents run concurrently)
resolved.append                    1 site    (one agent lookup per handoff, not two)
skip_null_tasks return guard       present

And your change is fully applied on top — grep self.conversation on the resolved file returns nothing, so the shared object really is gone.

Please resolve by merging master into your branch rather than rebasing; force pushes are not allowed on this repo.

2. The signature change breaks the tests #2087 added

This is the part that needs work beyond the conflict. On the resolved merge:

master:          22 failed, 7 passed, 1 skipped
resolved merge:  27 failed, 2 passed, 1 skipped

Five regressions, all from tests/structs/test_multi_agent_router.py, because those tests call the handlers with the old two-argument signature:

router.handle_single_handoff(
    {"handoffs": [{"agent_name": "A1", "task": None}]}, ""
)

They now need the conversation passed in. They are the tests covering skip_null_tasks, the absent-task-key fallback, concurrent execution, the unknown-agent guard and batch ordering — all worth keeping, so they should be updated rather than dropped.

3. On the missing test

Your note says a concurrency assertion would not run in CI because the suite needs live credentials. That is true of the old tests in that file, but #2087 added a _local_router() helper that builds the router with MultiAgentRouter.__new__ and agents that answer locally — no model, no key. It is already in the file you are editing. Once you rebase, an assertion like

router, _ = _local_router()
results = router.concurrent_batch_run(["a", "b", "c"])
# each result sees only its own task's messages

runs offline. Given this PR fixes a data race, a test that would have caught it is worth having.

Also worth a second look

hybrid_hiearchical_peer_swarm.py merged without conflict, but it is worth confirming the same self.conversation removal did not leave a reader behind — I only verified that for multi_agent_router.py.

Note also that #2076 is still open and edits handle_multiple_handoffs too. Whichever of the two lands second will need a small conflict resolution in the same method.

[ayaangazali]

All four done. Merged, not rebased.

1. Conflict

Merge commit 51f9beeb. Two hunks, exactly the ones you found, resolved as you suggested — my conversation: Conversation parameter, master's -> None. You were right that -> dict was wrong; both methods return nothing.

Re-ran your verification on the resolved file:

.get("task")        2 sites    present
executor.map        1 site     present
resolved.append     1 site     present
skip_null_tasks               present
self.conversation   0 matches  shared object is gone

2. The five regressions

Fixed. The tests called the handlers with the old two-argument signature; they now pass a Conversation(). Baselined both sides myself rather than trusting the number:

master:          22 failed, 7 passed, 1 skipped
this branch:     22 failed, 8 passed, 1 skipped

Same 22 (the live-credential ones), and the extra pass is the new test below. No regressions.

3. The offline test — and one thing I had to redo

Added test_concurrent_batch_run_keeps_each_task_history_separate, built on your _local_router() with a stubbed function_caller, so it runs with no model and no key.

First version was weak and I caught it: I had removed router.conversation = Conversation() from _local_router, since nothing on this branch reads it. That made the test fail on master with AttributeError: 'MultiAgentRouter' object has no attribute 'conversation' — a failure caused by my helper edit, not by the bug. A test that fails for the wrong reason is not a regression test.

Restored that line, so the helper still satisfies master's code path. The test now fails on master for the actual reason:

AssertionError: 'alpha' history leaked 'gamma':
  'user: alpha\n\nuser: beta\n\nuser: gamma\n\nA1: A1-out\n\nA1: A1-out\n\nA1: A1-out'

Three concurrent tasks, all interleaved into one history, and that whole thing returned as the result for alpha. That is the race, printed.

4. hybrid_hiearchical_peer_swarm.py

Checked as you asked. No self.conversation readers left — the conversation is created per run at :147 and threaded through route_task. Clean.

It did leave one stale line behind, which I removed: the class docstring still advertised conversation (Conversation) as an attribute the class no longer has. Same for concurrent_batch_run's Note:, which still told callers "All tasks share this router's Conversation, so their histories interleave. Prefer batch_run when the per-task history matters." That is the exact behaviour this PR removes, so leaving it would have actively misdirected the next reader. Those are the only two doc edits; no new comments anywhere in the diff.

On #2076

Noted — it also touches handle_multiple_handoffs. Whichever lands second gets the resolution; happy for that to be mine.