mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-09 21:49:37 +00:00
fix(agents): do not hide invalid config with file fallback (#4952)
* fix(agents): do not hide invalid config with file fallback * test(agents): cover invalid on-disk config fallback * fix(agents): resolve stores off the event loop * fix(agents): distinguish missing nested config from main config * fix(agents): reject missing explicit config path * test(agents): isolate config fallback test * test(agents): isolate router blocking IO coverage * test(agents): pin malformed config.yaml parse-error propagation An unparseable config.yaml used to be swallowed by the broad except Exception and silently downgrade to FileAgentStore. The narrowed except FileNotFoundError already propagates yaml.ParserError/ScannerError; pin that contract with a real on-disk config instead of monkeypatched get_app_config.
This commit is contained in:
parent
fe379c4486
commit
fb722770e4
@ -19,7 +19,7 @@ from deerflow.config.agents_config import (
|
||||
)
|
||||
from deerflow.config.app_config import get_app_config
|
||||
from deerflow.config.paths import get_paths
|
||||
from deerflow.persistence.agents import AgentExistsError, get_agent_store
|
||||
from deerflow.persistence.agents import AgentDeleteOutcome, AgentExistsError, get_agent_store
|
||||
from deerflow.runtime.user_context import get_effective_user_id
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@ -251,10 +251,14 @@ async def check_agent_name(name: str) -> dict:
|
||||
_validate_agent_name(name)
|
||||
normalized = _normalize_agent_name(name)
|
||||
user_id = get_effective_user_id()
|
||||
|
||||
# Availability is defined by the active backend and stays consistent with
|
||||
# create()'s conflict rule (file: per-user or legacy dir; db: a row). The
|
||||
# exists() probe is filesystem IO / a DB round trip, so keep it off the loop.
|
||||
exists = await asyncio.to_thread(get_agent_store().exists, normalized, user_id=user_id)
|
||||
def _exists() -> bool:
|
||||
return get_agent_store().exists(normalized, user_id=user_id)
|
||||
|
||||
exists = await asyncio.to_thread(_exists)
|
||||
return {"available": not exists, "name": normalized}
|
||||
|
||||
|
||||
@ -334,11 +338,10 @@ async def create_agent_endpoint(request: AgentCreateRequest) -> AgentResponse:
|
||||
# model / model_settings / thinking_enabled / reasoning_effort (issue #4336).
|
||||
_apply_model_behavior(config_data, request)
|
||||
|
||||
store = get_agent_store()
|
||||
|
||||
def _create_agent() -> AgentResponse:
|
||||
# Worker thread: existence checks + persistence (file IO or a DB round
|
||||
# trip) must stay off the event loop.
|
||||
store = get_agent_store()
|
||||
store.create(normalized_name, config_data, request.soul, user_id=user_id)
|
||||
logger.info("Created agent '%s'", normalized_name)
|
||||
agent_cfg = load_agent_config(normalized_name, user_id=user_id)
|
||||
@ -453,11 +456,14 @@ async def update_agent(name: str, request: AgentUpdateRequest) -> AgentResponse:
|
||||
for key, value in preserve_non_managed_fields(agent_cfg).items():
|
||||
updated.setdefault(key, value)
|
||||
|
||||
store = get_agent_store()
|
||||
# Persist config (when changed) and/or soul (when provided) off the
|
||||
# event loop. A no-change PATCH commits nothing and re-reads current state.
|
||||
if updated is not None or request.soul is not None:
|
||||
await asyncio.to_thread(store.update, name, updated, request.soul, user_id=user_id)
|
||||
|
||||
def _update_agent() -> None:
|
||||
get_agent_store().update(name, updated, request.soul, user_id=user_id)
|
||||
|
||||
await asyncio.to_thread(_update_agent)
|
||||
|
||||
logger.info(f"Updated agent '{name}'")
|
||||
|
||||
@ -560,11 +566,12 @@ async def delete_agent(name: str) -> None:
|
||||
_validate_agent_name(name)
|
||||
name = _normalize_agent_name(name)
|
||||
user_id = get_effective_user_id()
|
||||
store = get_agent_store()
|
||||
|
||||
try:
|
||||
# Off the event loop: file rmtree or a DB delete plus memory cleanup.
|
||||
outcome = await asyncio.to_thread(store.delete, name, user_id=user_id)
|
||||
def _delete_agent() -> AgentDeleteOutcome:
|
||||
return get_agent_store().delete(name, user_id=user_id)
|
||||
|
||||
outcome = await asyncio.to_thread(_delete_agent)
|
||||
except Exception as e:
|
||||
logger.error(f"Failed to delete agent '{name}': {e}", exc_info=True)
|
||||
raise HTTPException(status_code=500, detail=f"Failed to delete agent: {str(e)}")
|
||||
|
||||
@ -8,6 +8,7 @@ persistence layer.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from deerflow.persistence.agents.base import (
|
||||
@ -60,10 +61,13 @@ def make_agent_store(config: AppConfig) -> AgentStore:
|
||||
def get_agent_store() -> AgentStore:
|
||||
"""Return the store for the current process's configuration.
|
||||
|
||||
Defaults to the file backend when no app config can be resolved — the free
|
||||
functions in ``agents_config`` must keep working in lightweight contexts
|
||||
(CLI, tests, tools) that never load a full ``config.yaml``. Only an
|
||||
explicit ``agent_storage.backend: db`` diverges from the file default.
|
||||
Defaults to the file backend only when search mode cannot find the main app
|
||||
config — the free functions in ``agents_config`` must keep working in
|
||||
lightweight contexts (CLI, tests, tools) that never load a full
|
||||
``config.yaml``. A missing explicit ``DEER_FLOW_CONFIG_PATH`` or a missing
|
||||
nested config is an operator error and propagates instead of falling back.
|
||||
Only an explicit ``agent_storage.backend: db`` diverges from the file
|
||||
default once configuration resolves successfully.
|
||||
|
||||
Cross-process invariant (the ``db`` backend's whole point): the per-run
|
||||
agent build runs in the **graph subprocess**, a different process from the
|
||||
@ -76,12 +80,20 @@ def get_agent_store() -> AgentStore:
|
||||
downgraded to node-local ``file``. Pinned by
|
||||
``test_get_agent_store_resolves_db_backend_from_on_disk_config``.
|
||||
"""
|
||||
from deerflow.config.app_config import get_app_config
|
||||
from deerflow.config.app_config import AppConfig, get_app_config
|
||||
|
||||
try:
|
||||
config = get_app_config()
|
||||
except Exception: # noqa: BLE001 — no resolvable config → file default
|
||||
return _file_store()
|
||||
except FileNotFoundError:
|
||||
if os.getenv("DEER_FLOW_CONFIG_PATH"):
|
||||
raise
|
||||
# ``get_app_config()`` also loads optional nested config files. Only
|
||||
# fall back when the main config itself is absent.
|
||||
try:
|
||||
AppConfig.resolve_config_path()
|
||||
except FileNotFoundError:
|
||||
return _file_store()
|
||||
raise
|
||||
return make_agent_store(config)
|
||||
|
||||
|
||||
|
||||
@ -1,11 +1,10 @@
|
||||
"""Regression anchors: the custom-agent router must not block the event loop.
|
||||
|
||||
``app.gateway.routers.agents.create_agent_endpoint`` and ``delete_agent`` are
|
||||
async route handlers that resolve the agent directory (``Paths.base_dir`` calls
|
||||
``Path.resolve``), probe it (``Path.exists``), and create/remove it (``mkdir``,
|
||||
config/SOUL writes, ``shutil.rmtree``) — all blocking IO. Both offload that work
|
||||
via ``asyncio.to_thread``; if any of it regresses back onto the event loop, the
|
||||
strict Blockbuster gate raises ``BlockingError`` and these tests fail.
|
||||
The custom-agent handlers resolve the active store, access agent directories,
|
||||
and may perform filesystem or database IO. Create/read/check/update/delete all
|
||||
offload their complete blocking operations via ``asyncio.to_thread``; if any of
|
||||
that work regresses onto the event loop, the strict Blockbuster gate raises
|
||||
``BlockingError`` and these tests fail.
|
||||
|
||||
Imports live at module scope so the one-time FastAPI app construction (which
|
||||
reads files while building OpenAPI schemas) happens at collection time, not on
|
||||
@ -23,11 +22,13 @@ import pytest
|
||||
|
||||
from app.gateway.routers.agents import (
|
||||
AgentCreateRequest,
|
||||
AgentUpdateRequest,
|
||||
check_agent_name,
|
||||
create_agent_endpoint,
|
||||
delete_agent,
|
||||
get_agent,
|
||||
list_agents,
|
||||
update_agent,
|
||||
)
|
||||
from deerflow.config.agents_api_config import load_agents_api_config_from_dict
|
||||
from deerflow.config.paths import get_paths
|
||||
@ -36,6 +37,13 @@ from deerflow.runtime.user_context import get_effective_user_id
|
||||
pytestmark = pytest.mark.asyncio
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _isolate_agent_store_config(tmp_path: Path, monkeypatch) -> None:
|
||||
monkeypatch.delenv("DEER_FLOW_CONFIG_PATH", raising=False)
|
||||
monkeypatch.setenv("DEER_FLOW_PROJECT_ROOT", str(tmp_path))
|
||||
monkeypatch.setattr("deerflow.config.app_config._legacy_config_candidates", lambda: ())
|
||||
|
||||
|
||||
async def test_create_agent_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None:
|
||||
monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path))
|
||||
monkeypatch.setattr("deerflow.config.paths._paths", None)
|
||||
@ -92,3 +100,17 @@ async def test_read_endpoints_do_not_block_event_loop(tmp_path: Path, monkeypatc
|
||||
assert (await check_agent_name("never-created-agent"))["available"] is True
|
||||
finally:
|
||||
load_agents_api_config_from_dict({})
|
||||
|
||||
|
||||
async def test_update_agent_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None:
|
||||
monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path))
|
||||
monkeypatch.setattr("deerflow.config.paths._paths", None)
|
||||
load_agents_api_config_from_dict({"enabled": True})
|
||||
try:
|
||||
await create_agent_endpoint(AgentCreateRequest(name="loop-update-agent", soul="Original soul"))
|
||||
|
||||
response = await update_agent("loop-update-agent", AgentUpdateRequest(description="Updated description"))
|
||||
|
||||
assert response.description == "Updated description"
|
||||
finally:
|
||||
load_agents_api_config_from_dict({})
|
||||
|
||||
@ -263,9 +263,82 @@ def test_get_agent_store_falls_back_to_file_without_config(tmp_path, monkeypatch
|
||||
"""The ``except -> file`` fallback is for genuinely unresolvable config only
|
||||
(CLI/tests); it must not fire when a config exists — that asymmetry is what
|
||||
keeps a misconfigured graph process from silently downgrading db to file."""
|
||||
monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(tmp_path / "does-not-exist.yaml"))
|
||||
monkeypatch.delenv("DEER_FLOW_CONFIG_PATH", raising=False)
|
||||
monkeypatch.setenv("DEER_FLOW_PROJECT_ROOT", str(tmp_path))
|
||||
from deerflow.config import app_config
|
||||
|
||||
monkeypatch.setattr(app_config, "_legacy_config_candidates", lambda: ())
|
||||
try:
|
||||
reset_app_config()
|
||||
assert isinstance(get_agent_store(), FileAgentStore)
|
||||
finally:
|
||||
reset_app_config()
|
||||
|
||||
|
||||
def test_get_agent_store_does_not_fallback_when_explicit_config_is_missing(tmp_path, monkeypatch):
|
||||
"""An explicit config path is an operator assertion and must fail closed."""
|
||||
monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(tmp_path / "does-not-exist.yaml"))
|
||||
try:
|
||||
reset_app_config()
|
||||
with pytest.raises(FileNotFoundError, match="DEER_FLOW_CONFIG_PATH"):
|
||||
get_agent_store()
|
||||
finally:
|
||||
reset_app_config()
|
||||
|
||||
|
||||
def test_get_agent_store_does_not_hide_invalid_config(monkeypatch):
|
||||
"""Only missing config falls back; config errors must reach the caller."""
|
||||
from deerflow.config import app_config
|
||||
|
||||
def raise_invalid_config():
|
||||
raise ValueError("invalid config")
|
||||
|
||||
monkeypatch.setattr(app_config, "get_app_config", raise_invalid_config)
|
||||
with pytest.raises(ValueError, match="invalid config"):
|
||||
get_agent_store()
|
||||
|
||||
|
||||
def test_get_agent_store_propagates_invalid_on_disk_config(tmp_path, monkeypatch):
|
||||
"""A present config with an invalid backend must fail instead of falling back."""
|
||||
cfg_path = tmp_path / "config.yaml"
|
||||
_write_min_config(cfg_path, {"agent_storage": {"backend": "invalid"}})
|
||||
monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(cfg_path))
|
||||
try:
|
||||
reset_app_config()
|
||||
with pytest.raises(ValueError, match="agent_storage.backend"):
|
||||
get_agent_store()
|
||||
finally:
|
||||
reset_app_config()
|
||||
|
||||
|
||||
def test_get_agent_store_propagates_malformed_yaml(tmp_path, monkeypatch):
|
||||
"""An unparseable config.yaml must surface the parse error, not fall back to file."""
|
||||
cfg_path = tmp_path / "config.yaml"
|
||||
cfg_path.write_text("agent_storage: [oops\n", encoding="utf-8")
|
||||
monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(cfg_path))
|
||||
try:
|
||||
reset_app_config()
|
||||
with pytest.raises(yaml.YAMLError): # ParserError/ScannerError, previously swallowed
|
||||
get_agent_store()
|
||||
finally:
|
||||
reset_app_config()
|
||||
|
||||
|
||||
def test_get_agent_store_does_not_fallback_when_extensions_config_is_missing(tmp_path, monkeypatch):
|
||||
"""A missing nested config must not look like a missing main config."""
|
||||
cfg_path = tmp_path / "config.yaml"
|
||||
_write_min_config(
|
||||
cfg_path,
|
||||
{
|
||||
"agent_storage": {"backend": "db"},
|
||||
"database": {"backend": "sqlite", "sqlite_dir": str(tmp_path / "db")},
|
||||
},
|
||||
)
|
||||
monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(cfg_path))
|
||||
monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(tmp_path / "missing-extensions.json"))
|
||||
try:
|
||||
reset_app_config()
|
||||
with pytest.raises(FileNotFoundError, match="Extensions config"):
|
||||
get_agent_store()
|
||||
finally:
|
||||
reset_app_config()
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user