mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 21:26:39 +00:00
* fix(channels): run each IM message once across instances, restarts and late redeliveries Inbound IM messages were deduplicated only by the 10-minute inbound dedupe store (#7103). Discord direct messages had no key at all, a Discord thread reply got an instance-dependent key, a Discord thread was created before dedupe ran, a redelivery past the window started a second run, and two instances racing on a conversation's first messages bound it to two threads. - Discord messages are keyed (discord, guild_id or "dm", posting channel, message_id); the posting channel is the same on every instance. - MessageBus.claim_inbound lets an adapter record the manager's dedupe key before a side effect; Discord claims it on the Gateway loop before _create_thread, marks the published message dedupe_claimed, and gives the claim back when the message is never committed. - runs.wait / runs.stream / runs.create send an Idempotency-Key derived from the dedupe key, and keyed human message ids are uuid5 of it (a uuid4 per attempt made every keyed retry a 409). A reused run's status-only /wait or stream gap is resolved to this message's own turn via threads.get_state; 409s about the message's own keyed run keep the key instead of reading as a busy thread; an ended reused GitHub run gets no follow-up watcher. - A conversation's first thread binding is claimed insert-if-absent (claim_thread_id on channel_conversations and channel_thread_bindings); the loser adopts the winner and deletes the empty thread it minted. /new and /agent use still rebind. Docs: IM_CHANNEL_CONNECTIONS.md, CONFIGURATION.md, channels AGENTS.md, CHANGELOG (en/zh). * fix(channels): give back Discord dedupe claims for messages never processed Review round 1 on #7104. A Discord claim records the manager's dedupe key before the message is queued, so every path that drops the message unrun must release it, or a peer refuses the redelivery for the rest of the TTL: - The hand-off task releases the claim on cancellation too (stop() drains it while it resolves the connection identity), via await_drained so the cancellation still propagates. - ChannelManager._discard_pending_inbound releases the key of a claimed message that shutdown drops from the queue, after the grace period and on a cancelled shutdown. - The adapter tracks claims until they are committed. A release refused because stop() already closed cross-loop intake, or a hand-off cancelled before it ran, leaves the claim tracked, and stop() releases it last. * fix(gateway): keep IM run idempotency stable when /new rebinds the conversation Review round 2 on #7104. The Gateway scoped every Idempotency-Key by owner and thread. /new rebinds an IM conversation to a fresh thread, so a late redelivery of a message admitted on the old thread was requested on the new one, got a different scoped key and started a second run. - Keys from trusted internal callers (the IM channel manager) are scoped by owner only (`internal-run:` prefix); browser and API keys keep the owner-and-thread scope (`http-run:`). - A key that resolves to a run of another thread or user raises RunIdempotencyKeyMismatch, a RuntimeError subclass so in-process launchers see the same type, and start_run answers it with 409 "Idempotency-Key already used with a different request" instead of a 500. The channel manager already treats that 409 as the message's admitted run. Docs: API.md, IM_CHANNEL_CONNECTIONS.md, gateway AGENTS.md (reworded to stay under its budget), CHANGELOG (en/zh). --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
234 lines
11 KiB
Python
234 lines
11 KiB
Python
"""Integration test: two ChannelManagers share one Postgres dedupe table.
|
|
|
|
Mirrors ``test_multi_worker_run_ownership.py``'s "two managers sharing a DB"
|
|
pattern. Skipped unless a real Postgres backend is configured via
|
|
``DEDUPE_TEST_POSTGRES_URL`` (the dev container runs sqlite), so it only runs
|
|
where cross-pod dedupe actually matters — CI with Postgres, or a local run with
|
|
a throwaway Postgres. See issue #4120.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
import uuid
|
|
from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit
|
|
|
|
import pytest
|
|
import pytest_asyncio
|
|
|
|
from app.channels.dedupe_store import INBOUND_DEDUPE_TTL_SECONDS, PostgresInboundDedupeStore
|
|
|
|
POSTGRES_URL = os.environ.get("DEDUPE_TEST_POSTGRES_URL")
|
|
|
|
# libpq/psycopg-only query parameters that asyncpg's ``connect()`` rejects. CI
|
|
# hands over ``TEST_POSTGRES_URI`` as ``postgresql://...?sslmode=disable``; left
|
|
# in place these raise ``TypeError: connect() got an unexpected keyword argument
|
|
# 'sslmode'`` once the URL is routed to the asyncpg driver, so strip them here.
|
|
_LIBPQ_ONLY_QUERY_KEYS = {"sslmode", "channel_binding"}
|
|
|
|
|
|
def _asyncpg_url(url: str | None) -> str | None:
|
|
"""Normalize a sync ``postgresql://`` URL to the async driver SQLAlchemy needs.
|
|
|
|
``init_engine`` builds an *async* engine, so a bare ``postgresql://`` would fall
|
|
back to the synchronous psycopg2 driver and fail. Accept either form so the test
|
|
runs whether CI hands over ``TEST_POSTGRES_URI`` (``postgresql://...``) or a
|
|
caller passes ``postgresql+asyncpg://...`` directly. libpq-only query params
|
|
(e.g. ``sslmode``) are dropped because asyncpg does not understand them.
|
|
"""
|
|
if not url:
|
|
return url
|
|
if url.startswith("postgresql://"):
|
|
url = "postgresql+asyncpg://" + url[len("postgresql://") :]
|
|
parts = urlsplit(url)
|
|
if parts.query:
|
|
kept = [(k, v) for k, v in parse_qsl(parts.query, keep_blank_values=True) if k not in _LIBPQ_ONLY_QUERY_KEYS]
|
|
url = urlunsplit(parts._replace(query=urlencode(kept)))
|
|
return url
|
|
|
|
|
|
pytestmark = pytest.mark.skipif(
|
|
not POSTGRES_URL,
|
|
reason="requires DEDUPE_TEST_POSTGRES_URL (real Postgres for cross-pod dedupe)",
|
|
)
|
|
|
|
|
|
@pytest_asyncio.fixture(autouse=True)
|
|
async def _postgres_engine():
|
|
"""Initialize the Postgres engine within the test's event loop.
|
|
|
|
Doing this inside the loop (not at import time) avoids the "connection
|
|
attached to a different loop" error that arises when the engine is built in
|
|
a loop that pytest-asyncio later closes.
|
|
"""
|
|
from deerflow.persistence.engine import close_engine, init_engine
|
|
|
|
await init_engine("postgres", url=_asyncpg_url(POSTGRES_URL))
|
|
try:
|
|
yield
|
|
finally:
|
|
await close_engine()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_two_stores_share_dedupe_state_across_pods():
|
|
from deerflow.persistence.base import Base
|
|
from deerflow.persistence.engine import get_engine, get_session_factory
|
|
from deerflow.persistence.webhook_delivery.model import WebhookDeliveryRow
|
|
|
|
engine = get_engine()
|
|
sf = get_session_factory()
|
|
# Ensure the table exists regardless of whether the alembic migration ran.
|
|
async with engine.begin() as conn:
|
|
await conn.run_sync(Base.metadata.create_all, tables=[WebhookDeliveryRow.__table__], checkfirst=True)
|
|
|
|
store_a = PostgresInboundDedupeStore(session_factory=sf)
|
|
store_b = PostgresInboundDedupeStore(session_factory=sf)
|
|
unique = uuid.uuid4().hex
|
|
key = ("github", "repo", "repo", f"d-{unique}:uA:agentX")
|
|
try:
|
|
# First pod records the delivery and proceeds.
|
|
assert await store_a.try_record(key) is False
|
|
# A redelivery landing on the second pod hits the same table -> duplicate.
|
|
assert await store_b.try_record(key) is True
|
|
finally:
|
|
await store_a.release(key)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_manager_injects_shared_store_and_dedupes_cross_pod():
|
|
"""End-to-end: two ChannelManagers wired to the *same* Postgres store drop a
|
|
redelivery routed to the second manager, exactly as the production dispatch
|
|
loop would (``if await self._is_duplicate_inbound(msg): continue``)."""
|
|
from app.channels.manager import ChannelManager
|
|
from app.channels.message_bus import InboundMessage, MessageBus
|
|
from app.channels.store import JsonChannelStore
|
|
from deerflow.persistence.engine import get_session_factory
|
|
|
|
sf = get_session_factory()
|
|
shared_store = PostgresInboundDedupeStore(session_factory=sf)
|
|
manager_a = ChannelManager(bus=MessageBus(), store=JsonChannelStore(), inbound_dedupe_store=shared_store)
|
|
manager_b = ChannelManager(bus=MessageBus(), store=JsonChannelStore(), inbound_dedupe_store=shared_store)
|
|
|
|
msg = InboundMessage(
|
|
channel_name="github",
|
|
chat_id="repo",
|
|
user_id="alice",
|
|
text="@bot review",
|
|
topic_id="7:agent",
|
|
workspace_id="repo",
|
|
metadata={"message_id": "d-crosspod:agent"},
|
|
)
|
|
try:
|
|
# First manager records the delivery and lets it through.
|
|
assert await manager_a._is_duplicate_inbound(msg) is False
|
|
# Same redelivery on the second manager hits the shared table -> dropped.
|
|
assert await manager_b._is_duplicate_inbound(msg) is True
|
|
finally:
|
|
await manager_a._release_inbound_dedupe_key(msg)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_expired_unreleased_row_is_reclaimed_on_next_redelivery():
|
|
"""P1 end-to-end: a row past the TTL that was never released must be
|
|
reclaimed so the next redelivery is re-admitted (not dropped forever)."""
|
|
from sqlalchemy import text
|
|
|
|
from deerflow.persistence.base import Base
|
|
from deerflow.persistence.engine import get_engine, get_session_factory
|
|
from deerflow.persistence.webhook_delivery.model import WebhookDeliveryRow
|
|
|
|
engine = get_engine()
|
|
sf = get_session_factory()
|
|
async with engine.begin() as conn:
|
|
await conn.run_sync(Base.metadata.create_all, tables=[WebhookDeliveryRow.__table__], checkfirst=True)
|
|
|
|
store = PostgresInboundDedupeStore(session_factory=sf)
|
|
unique = uuid.uuid4().hex
|
|
channel, workspace_id, chat_id, message_id = ("github", "repo", "repo", f"d-{unique}:expired")
|
|
key = (channel, workspace_id, chat_id, message_id)
|
|
try:
|
|
# Seed an already-expired, unreleased row for this key.
|
|
async with sf() as session:
|
|
async with session.begin():
|
|
await session.execute(
|
|
text("INSERT INTO webhook_deliveries (channel, workspace_id, chat_id, message_id, first_seen) VALUES (:c, :w, :ch, :m, now() - make_interval(secs => :age))"),
|
|
{"c": channel, "w": workspace_id, "ch": chat_id, "m": message_id, "age": INBOUND_DEDUPE_TTL_SECONDS + 1},
|
|
)
|
|
# The expired row is reclaimed -> redelivery re-admitted (not a duplicate).
|
|
assert await store.try_record(key) is False
|
|
# Its first_seen is refreshed to ~now (well within the TTL).
|
|
async with sf() as session:
|
|
row = (
|
|
await session.execute(
|
|
text("SELECT (now() - first_seen) < make_interval(secs => :ttl) AS fresh FROM webhook_deliveries WHERE channel = :c AND workspace_id = :w AND chat_id = :ch AND message_id = :m"),
|
|
{"c": channel, "w": workspace_id, "ch": chat_id, "m": message_id, "ttl": INBOUND_DEDUPE_TTL_SECONDS},
|
|
)
|
|
).fetchone()
|
|
assert row is not None and row[0] is True
|
|
finally:
|
|
await store.release(key)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_two_managers_share_dedupe_for_a_discord_direct_message():
|
|
"""A Discord DM has no guild id and used to get no dedupe key, so every
|
|
instance whose session received the MESSAGE_CREATE ran it."""
|
|
from app.channels.manager import ChannelManager
|
|
from app.channels.message_bus import InboundMessage, MessageBus
|
|
from app.channels.store import JsonChannelStore
|
|
from deerflow.persistence.base import Base
|
|
from deerflow.persistence.engine import get_engine, get_session_factory
|
|
from deerflow.persistence.webhook_delivery.model import WebhookDeliveryRow
|
|
|
|
async with get_engine().begin() as conn:
|
|
await conn.run_sync(Base.metadata.create_all, tables=[WebhookDeliveryRow.__table__], checkfirst=True)
|
|
shared_store = PostgresInboundDedupeStore(session_factory=get_session_factory())
|
|
manager_a = ChannelManager(bus=MessageBus(), store=JsonChannelStore(), inbound_dedupe_store=shared_store)
|
|
manager_b = ChannelManager(bus=MessageBus(), store=JsonChannelStore(), inbound_dedupe_store=shared_store)
|
|
dm_channel = str(uuid.uuid4().int)[:19]
|
|
|
|
def delivery() -> InboundMessage:
|
|
return InboundMessage(
|
|
channel_name="discord",
|
|
chat_id=dm_channel,
|
|
user_id="42",
|
|
text="hi",
|
|
topic_id=dm_channel,
|
|
metadata={"guild_id": None, "channel_id": dm_channel, "message_id": "1300000000000000001"},
|
|
)
|
|
|
|
try:
|
|
assert await manager_a._is_duplicate_inbound(delivery()) is False
|
|
assert await manager_b._is_duplicate_inbound(delivery()) is True
|
|
finally:
|
|
await manager_a._release_inbound_dedupe_key(delivery())
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_two_managers_share_dedupe_for_a_reply_in_a_thread_only_one_tracks():
|
|
"""The instance tracking Discord thread T routes a reply in T to its parent
|
|
channel; the other treats T as an orphan. One message, one run."""
|
|
from app.channels.manager import ChannelManager
|
|
from app.channels.message_bus import InboundMessage, MessageBus
|
|
from app.channels.store import JsonChannelStore
|
|
from deerflow.persistence.base import Base
|
|
from deerflow.persistence.engine import get_engine, get_session_factory
|
|
from deerflow.persistence.webhook_delivery.model import WebhookDeliveryRow
|
|
|
|
async with get_engine().begin() as conn:
|
|
await conn.run_sync(Base.metadata.create_all, tables=[WebhookDeliveryRow.__table__], checkfirst=True)
|
|
shared_store = PostgresInboundDedupeStore(session_factory=get_session_factory())
|
|
manager_a = ChannelManager(bus=MessageBus(), store=JsonChannelStore(), inbound_dedupe_store=shared_store)
|
|
manager_b = ChannelManager(bus=MessageBus(), store=JsonChannelStore(), inbound_dedupe_store=shared_store)
|
|
thread_t = str(uuid.uuid4().int)[:19]
|
|
metadata = {"guild_id": "guild-1", "channel_id": thread_t, "message_id": "1300000000000000002"}
|
|
tracked = InboundMessage(channel_name="discord", chat_id="parent-1", user_id="42", text="hi", topic_id=thread_t, metadata=dict(metadata))
|
|
orphaned = InboundMessage(channel_name="discord", chat_id=thread_t, user_id="42", text="hi", topic_id="new-thread", metadata=dict(metadata))
|
|
|
|
try:
|
|
assert await manager_a._is_duplicate_inbound(tracked) is False
|
|
assert await manager_b._is_duplicate_inbound(orphaned) is True
|
|
finally:
|
|
await manager_a._release_inbound_dedupe_key(tracked)
|