diff --git a/backend/app/channels/AGENTS.md b/backend/app/channels/AGENTS.md index 357e9092c..f1c5dd3f6 100644 --- a/backend/app/channels/AGENTS.md +++ b/backend/app/channels/AGENTS.md @@ -28,6 +28,7 @@ Bridges external messaging platforms (Feishu, Slack, Telegram, Discord, DingTalk 2. `ChannelManager._dispatch_loop()` consumes from queue 3. For user-owned channel connections, incoming messages carry `connection_id`, `owner_user_id`, and `workspace_id`; `owner_user_id` becomes the DeerFlow run `user_id`, while the raw platform user id remains `channel_user_id`. The Gateway accepts `channel_user_id` only from an internally authenticated channel caller's top-level `body.context`, clears it from both free-form `body.config` sections, and writes it into runtime context only (never `configurable`, which is checkpointed). `bash_tool` exposes it to sandbox commands as the fixed env var `DEERFLOW_CHANNEL_USER_ID` — via a shell-quoted command-string prefix, NOT the `execute_command(env=...)` channel, which is reserved for request-scoped secrets and would switch `AioSandbox` onto the `bash.exec` path (image >= 1.9.3, fresh session per call). Per-call injection keeps group-chat identity correct (one thread/sandbox, many senders) **without depending on the AIO shell's session semantics**: every IM-channel command carries an explicit `export VAR=; ` (valid id) or `unset VAR; ` (empty / non-str / over the 256-char cap). The AIO no-env path reuses a persistent shell session (the reason for the class lock, #1433), so a bare command could otherwise resolve a stale id an earlier sender exported; the `unset` closes the window the length/type guard would open (a dropped id would inherit the previous sender's value). Non-IM runs (no `channel_user_id` in context) are left untouched. Not injected on the Windows local sandbox (its PowerShell/cmd.exe fallback has no `export`/`unset`). Propagates across `task` delegation: `task_tool` captures the dispatching turn's id and the subagent executor forwards it into the subagent's runtime context, same as the guardrail attribution fields. The runtime-context value is authorization-grade at the Gateway/guardrail boundary, but the exported shell variable remains informational because any bash command can overwrite its own environment; skills must not treat the shell variable itself as authenticated identity. Tests: `tests/test_gateway_services.py`, `tests/test_channel_user_id_env.py` 4. For chat: look up/create thread through Gateway's LangGraph-compatible API + - First-use creation is serialized by a waiter-aware keyed lock over `(channel_name, chat_id, topic_id)`. Holders and queued callers share one lock generation through failure and cancellation; idle entries are reclaimed only after the final participant leaves. This prevents a late inbound message from bypassing a queued creator and splitting one conversation across duplicate Gateway threads. 5. Feishu/Telegram chat: `runs.stream()` → accumulate AI text → publish multiple outbound updates (`is_final=False`) → publish final outbound (`is_final=True`) 6. Slack/Discord chat: `runs.wait()` → extract final response → publish outbound 6b. GitHub chat (`ChannelRunPolicy.fire_and_forget=True`): `runs.create()` returns once the run is `pending`; the manager does not wait for the final state and does not publish an outbound. The agent posts its own reply mid-run via `gh` from the sandbox. `ConflictError` on a busy thread still trips the standard `THREAD_BUSY_MESSAGE` path (log-only on GitHub); when the channel's policy also sets `buffer_followups_on_busy=True` (GitHub's default — see "Follow-up buffering while busy" below), the triggering message is additionally captured into a per-thread buffer instead of only logged, so a concurrent comment is not silently dropped. diff --git a/backend/app/channels/manager.py b/backend/app/channels/manager.py index c8096fb4f..454ae4846 100644 --- a/backend/app/channels/manager.py +++ b/backend/app/channels/manager.py @@ -46,6 +46,7 @@ from deerflow.config.agents_config import list_custom_agents, load_agent_config from deerflow.config.paths import make_safe_user_id from deerflow.runtime import END_SENTINEL, StreamBridge from deerflow.runtime.goal import parse_goal_command +from deerflow.runtime.keyed_lock import AsyncKeyedLockTable from deerflow.runtime.user_context import get_effective_user_id from deerflow.skills.slash import parse_slash_skill_reference from deerflow.skills.storage import get_or_new_skill_storage @@ -1233,9 +1234,11 @@ class ChannelManager: # same thread before every turn; None distinguishes a checked default # thread from a thread that has not been inspected yet. self._thread_agent_names: dict[str, str | None] = {} - # Per-conversation locks so concurrent inbound messages for the same - # chat don't race to create duplicate threads (see _get_or_create_thread). - self._thread_create_locks: dict[tuple[str, str, str | None], asyncio.Lock] = {} + # Waiter-aware per-conversation locks prevent concurrent inbound messages + # from creating duplicate threads. Participants are checked out before + # they wait, so failure or cancellation of the current creator cannot let + # a late caller bypass an already-queued creator through a new lock generation. + self._thread_create_locks = AsyncKeyedLockTable[tuple[str, str, str | None]]() # Per-thread run locks for channels that want in-manager serialization # instead of surfacing the runtime's generic busy reply. self._serialized_thread_runs: dict[tuple[str, str], _SerializedThreadRunState] = {} @@ -2326,20 +2329,13 @@ class ChannelManager: return thread_id, False key = (msg.channel_name, msg.chat_id, msg.topic_id) - lock = self._thread_create_locks.setdefault(key, asyncio.Lock()) - try: - async with lock: - # A concurrent message for the same chat may have created the - # thread while we were waiting on the lock. - thread_id = await self._lookup_thread_id(msg) - if thread_id: - return thread_id, False - return await self._create_thread(client, msg), True - finally: - # Once the thread is stored, later messages short-circuit on the - # lookup above and never reach this lock, so it's safe to drop the - # entry and keep the registry bounded to in-flight conversations. - self._thread_create_locks.pop(key, None) + async with self._thread_create_locks.hold(key): + # A concurrent message for the same chat may have created the + # thread while we were waiting on the lock. + thread_id = await self._lookup_thread_id(msg) + if thread_id: + return thread_id, False + return await self._create_thread(client, msg), True async def _update_thread_channel_metadata(self, client, msg: InboundMessage, thread_id: str) -> None: """Best-effort source metadata backfill for existing IM-created threads.""" diff --git a/backend/tests/test_channels.py b/backend/tests/test_channels.py index 4d01b7e44..381ab78a5 100644 --- a/backend/tests/test_channels.py +++ b/backend/tests/test_channels.py @@ -768,6 +768,105 @@ class TestChannelManager: _run(go()) + @pytest.mark.parametrize("first_exit", ["error", "cancel"]) + def test_thread_create_waiters_keep_one_lock_generation_after_first_aborts(self, first_exit): + """A queued creator must remain visible after the first creator aborts. + + The first creator used to remove the conversation's lock entry in its + ``finally`` block even while a second creator was queued on that lock. + A late third caller could then install a new lock and create a second + thread concurrently with the queued caller. + """ + from app.channels.manager import ChannelManager + + async def go(): + bus = MessageBus() + store = ChannelStore(path=Path(tempfile.mkdtemp()) / "store.json") + manager = ChannelManager(bus=bus, store=store) + first_create_started = asyncio.Event() + release_first_with_error = asyncio.Event() + second_lookup_started = asyncio.Event() + second_create_started = asyncio.Event() + third_lookup_started = asyncio.Event() + allow_third_lookup = asyncio.Event() + third_create_started = asyncio.Event() + release_later_creates = asyncio.Event() + lookup_counts: dict[str, int] = {} + create_calls = 0 + active_later_creates = 0 + max_active_later_creates = 0 + + async def lookup_thread_id(msg): + lookup_counts[msg.text] = lookup_counts.get(msg.text, 0) + 1 + if msg.text == "second" and lookup_counts[msg.text] == 1: + second_lookup_started.set() + if msg.text == "third" and lookup_counts[msg.text] == 1: + third_lookup_started.set() + await allow_third_lookup.wait() + return None + + async def create_thread(_client, _msg): + nonlocal create_calls, active_later_creates, max_active_later_creates + create_calls += 1 + call_number = create_calls + if call_number == 1: + first_create_started.set() + await release_first_with_error.wait() + raise RuntimeError("synthetic first-create failure") + + active_later_creates += 1 + max_active_later_creates = max(max_active_later_creates, active_later_creates) + if call_number == 2: + second_create_started.set() + else: + third_create_started.set() + try: + await release_later_creates.wait() + finally: + active_later_creates -= 1 + return f"thread-{call_number}" + + manager._lookup_thread_id = lookup_thread_id + manager._create_thread = create_thread + client = MagicMock() + first_msg = InboundMessage(channel_name="slack", chat_id="C1", user_id="U1", text="first") + second_msg = InboundMessage(channel_name="slack", chat_id="C1", user_id="U1", text="second") + third_msg = InboundMessage(channel_name="slack", chat_id="C1", user_id="U1", text="third") + + first = asyncio.create_task(manager._get_or_create_thread(client, first_msg)) + await first_create_started.wait() + second = asyncio.create_task(manager._get_or_create_thread(client, second_msg)) + await second_lookup_started.wait() + + if first_exit == "cancel": + first.cancel() + with pytest.raises(asyncio.CancelledError): + await first + else: + release_first_with_error.set() + with pytest.raises(RuntimeError, match="synthetic first-create failure"): + await first + + await second_create_started.wait() + third = asyncio.create_task(manager._get_or_create_thread(client, third_msg)) + await third_lookup_started.wait() + allow_third_lookup.set() + turn_complete = asyncio.get_running_loop().create_future() + asyncio.get_running_loop().call_soon(turn_complete.set_result, None) + await turn_complete + + bypassed_lock_generation = third_create_started.is_set() + max_active_before_release = max_active_later_creates + release_later_creates.set() + await asyncio.gather(second, third) + + assert not bypassed_lock_generation, "late caller bypassed the queued creator through a new lock generation" + assert max_active_before_release == 1 + assert max_active_later_creates == 1 + assert not manager._thread_create_locks._entries_by_loop + + _run(go()) + def test_fetch_gateway_includes_internal_auth_headers(self, monkeypatch): from app.channels.manager import ChannelManager