diff --git a/backend/app/gateway/routers/skills.py b/backend/app/gateway/routers/skills.py index 9febc1b23..6d29b4582 100644 --- a/backend/app/gateway/routers/skills.py +++ b/backend/app/gateway/routers/skills.py @@ -618,10 +618,19 @@ async def get_custom_skill_history(skill_name: str, request: Request, config: Ap async def rollback_custom_skill(skill_name: str, body: SkillRollbackRequest, request: Request, config: AppConfig = Depends(get_config)) -> CustomSkillContentResponse: await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) try: - storage = _get_user_skill_storage(config) - if not storage.custom_skill_exists(skill_name) and not storage.get_skill_history_file(skill_name).exists(): + + def _read_rollback_history() -> tuple[SkillStorage, list[dict] | None]: + # Worker thread: storage construction, the existence probes, and the + # history-file read are blocking filesystem IO that must stay off the + # event loop — the same rule get_custom_skill_history applies above. + storage = _get_user_skill_storage(config) + if not storage.custom_skill_exists(skill_name) and not storage.get_skill_history_file(skill_name).exists(): + return storage, None + return storage, storage.read_history(skill_name) + + storage, history = await asyncio.to_thread(_read_rollback_history) + if history is None: raise HTTPException(status_code=404, detail=f"Custom skill '{skill_name}' not found") - history = storage.read_history(skill_name) if not history: raise HTTPException(status_code=400, detail=f"Custom skill '{skill_name}' has no history") record = history[body.history_index] diff --git a/backend/tests/blocking_io/test_skills_custom_router.py b/backend/tests/blocking_io/test_skills_custom_router.py new file mode 100644 index 000000000..e85ac4aa1 --- /dev/null +++ b/backend/tests/blocking_io/test_skills_custom_router.py @@ -0,0 +1,86 @@ +"""Regression anchor: the custom-skill rollback route must keep its history reads off the loop. + +``rollback_custom_skill`` offloads storage construction, existence probes and +the ``custom/.history/.jsonl`` read through ``asyncio.to_thread``, matching +the adjacent ``get_custom_skill_history`` handler. History entries contain the +full previous and new skill content, so reading and parsing the entire history +on the Gateway loop would stall other requests. The post-scan current-content +read is outside this anchor's coverage and remains a separate blocking-IO fix. + +The two branches driven here both return before the awaited security scan, so +the anchor needs no scanner or model stub: the 404 branch covers construction +plus the existence probes, and the out-of-range branch covers the full +history-file read and parse. +""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path +from types import SimpleNamespace +from uuid import UUID + +import pytest +from fastapi import HTTPException, Request + +from app.gateway.routers.skills import SkillRollbackRequest, rollback_custom_skill +from deerflow.config.app_config import AppConfig +from deerflow.config.paths import get_paths +from deerflow.runtime.user_context import get_effective_user_id + +pytestmark = pytest.mark.asyncio + +_SKILL_NAME = "loop-rollback-skill" +_SKILL_MD = f"---\nname: {_SKILL_NAME}\ndescription: Anchor fixture skill.\n---\n\n# {_SKILL_NAME}\n" + + +def _custom_dir() -> Path: + return get_paths().user_custom_skills_dir(get_effective_user_id()) + + +@pytest.fixture(autouse=True) +def _isolate_paths(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + + +def _admin_request() -> Request: + # AuthMiddleware normally supplies this state; keep the real admin check. + user = SimpleNamespace(id=UUID("11111111-2222-3333-4444-555555555555"), system_role="admin") + return Request({"type": "http", "headers": [], "state": {"user": user}}) + + +def _install_skill() -> None: + skill_dir = _custom_dir() / _SKILL_NAME + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + + +def _write_history(records: list[dict]) -> None: + history_dir = _custom_dir() / ".history" + history_dir.mkdir(parents=True, exist_ok=True) + (history_dir / f"{_SKILL_NAME}.jsonl").write_text("".join(json.dumps(r) + "\n" for r in records), encoding="utf-8") + + +async def test_rollback_missing_skill_does_not_block_event_loop() -> None: + """The 404 branch must not resolve paths or probe the filesystem from the loop.""" + config = AppConfig.model_validate({"sandbox": {"use": "test"}}) + + with pytest.raises(HTTPException) as excinfo: + await rollback_custom_skill(_SKILL_NAME, SkillRollbackRequest(history_index=0), _admin_request(), config) + + assert excinfo.value.status_code == 404 + + +async def test_rollback_history_read_does_not_block_event_loop() -> None: + """The out-of-range branch reads and parses the whole history file first.""" + await asyncio.to_thread(_install_skill) + await asyncio.to_thread(_write_history, [{"action": "edit", "ts": 1, "prev_content": _SKILL_MD, "new_content": _SKILL_MD}]) + config = AppConfig.model_validate({"sandbox": {"use": "test"}}) + + with pytest.raises(HTTPException) as excinfo: + await rollback_custom_skill(_SKILL_NAME, SkillRollbackRequest(history_index=99), _admin_request(), config) + + assert excinfo.value.status_code == 400 + assert "history_index is out of range" in str(excinfo.value.detail)