mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-22 20:46:20 +00:00
fix(channels): preserve thread-create lock generation (#5480)
This commit is contained in:
parent
53dde30d4a
commit
4387dce7be
@ -28,6 +28,7 @@ Bridges external messaging platforms (Feishu, Slack, Telegram, Discord, DingTalk
|
|||||||
2. `ChannelManager._dispatch_loop()` consumes from queue
|
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=<id>; ` (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`
|
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=<id>; ` (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
|
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`)
|
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
|
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.
|
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.
|
||||||
|
|||||||
@ -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.config.paths import make_safe_user_id
|
||||||
from deerflow.runtime import END_SENTINEL, StreamBridge
|
from deerflow.runtime import END_SENTINEL, StreamBridge
|
||||||
from deerflow.runtime.goal import parse_goal_command
|
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.runtime.user_context import get_effective_user_id
|
||||||
from deerflow.skills.slash import parse_slash_skill_reference
|
from deerflow.skills.slash import parse_slash_skill_reference
|
||||||
from deerflow.skills.storage import get_or_new_skill_storage
|
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
|
# same thread before every turn; None distinguishes a checked default
|
||||||
# thread from a thread that has not been inspected yet.
|
# thread from a thread that has not been inspected yet.
|
||||||
self._thread_agent_names: dict[str, str | None] = {}
|
self._thread_agent_names: dict[str, str | None] = {}
|
||||||
# Per-conversation locks so concurrent inbound messages for the same
|
# Waiter-aware per-conversation locks prevent concurrent inbound messages
|
||||||
# chat don't race to create duplicate threads (see _get_or_create_thread).
|
# from creating duplicate threads. Participants are checked out before
|
||||||
self._thread_create_locks: dict[tuple[str, str, str | None], asyncio.Lock] = {}
|
# 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
|
# Per-thread run locks for channels that want in-manager serialization
|
||||||
# instead of surfacing the runtime's generic busy reply.
|
# instead of surfacing the runtime's generic busy reply.
|
||||||
self._serialized_thread_runs: dict[tuple[str, str], _SerializedThreadRunState] = {}
|
self._serialized_thread_runs: dict[tuple[str, str], _SerializedThreadRunState] = {}
|
||||||
@ -2326,20 +2329,13 @@ class ChannelManager:
|
|||||||
return thread_id, False
|
return thread_id, False
|
||||||
|
|
||||||
key = (msg.channel_name, msg.chat_id, msg.topic_id)
|
key = (msg.channel_name, msg.chat_id, msg.topic_id)
|
||||||
lock = self._thread_create_locks.setdefault(key, asyncio.Lock())
|
async with self._thread_create_locks.hold(key):
|
||||||
try:
|
# A concurrent message for the same chat may have created the
|
||||||
async with lock:
|
# thread while we were waiting on the lock.
|
||||||
# A concurrent message for the same chat may have created the
|
thread_id = await self._lookup_thread_id(msg)
|
||||||
# thread while we were waiting on the lock.
|
if thread_id:
|
||||||
thread_id = await self._lookup_thread_id(msg)
|
return thread_id, False
|
||||||
if thread_id:
|
return await self._create_thread(client, msg), True
|
||||||
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 def _update_thread_channel_metadata(self, client, msg: InboundMessage, thread_id: str) -> None:
|
async def _update_thread_channel_metadata(self, client, msg: InboundMessage, thread_id: str) -> None:
|
||||||
"""Best-effort source metadata backfill for existing IM-created threads."""
|
"""Best-effort source metadata backfill for existing IM-created threads."""
|
||||||
|
|||||||
@ -768,6 +768,105 @@ class TestChannelManager:
|
|||||||
|
|
||||||
_run(go())
|
_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):
|
def test_fetch_gateway_includes_internal_auth_headers(self, monkeypatch):
|
||||||
from app.channels.manager import ChannelManager
|
from app.channels.manager import ChannelManager
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user