Repository navigation
fix(gateway): keep custom-skill rollback filesystem IO off the event loop - #5751
Conversation
…loop Closes bytedance#5747. Three blocking sites remained on the Gateway event loop in the rollback route and its shared response reader, all offloaded through asyncio.to_thread following the bytedance#5729 pattern: - storage.validate_skill_markdown_content() creates a temp dir and writes SKILL.md into it (site 1) - the post-scan read of the file being replaced (site 2, named by bytedance#5729's test docstring) - _read_custom_skill_response(): load_skills() walks every skill directory and read_custom_skill() opens SKILL.md — O(installed skills) loop time paid per accepted rollback and per GET /skills/custom/{name} (site 3) A blocking_io regression drives the full accepted-rollback path (past the security scan) under the strict gate; the two pre-existing cases only covered the pre-scan branches.
willem-bd
left a comment
There was a problem hiding this comment.
Validated at head 53925d4: the three offloads match #5747's documented sites exactly, the 404/read ordering in _read_custom_skill_response is preserved, and the new blocking_io regression is real — reverting the three to_thread wraps locally makes test_rollback_accepted_path_does_not_block_event_loop fail with BlockingError: Blocking call to os.mkdir, while the suite passes as committed (3 passed, plus 29 in the behavior suite; ruff clean). Two notes below, neither blocking: the sibling update route still has the same on-loop calls the issue believed were already offloaded, and a small typing nit on the new closure's annotation.
| if target_content is None: | ||
| raise HTTPException(status_code=400, detail="Selected history entry has no previous content to roll back to") | ||
| storage.validate_skill_markdown_content(skill_name, target_content) | ||
| await asyncio.to_thread(storage.validate_skill_markdown_content, skill_name, target_content) |
There was a problem hiding this comment.
Out of scope for this rollback PR but worth pinning down, since both the issue and the PR description lean on it: the claim that "update_custom_skill ... run[s] it through asyncio.to_thread" only holds for the agent tool path (skill_manage_tool.py offloads all three of its validate_skill_markdown_content calls). The HTTP update_custom_skill route in this same file still calls storage.validate_skill_markdown_content(...) (line 538) and prev_content = storage.read_custom_skill(...) (line 543) directly on the event loop, and nothing in tests/blocking_io/ drives that route — so an accepted edit still pays the temp-dir blocking cost with no gate to catch it. Suggest a follow-up issue to offload those two and add the missing anchor.
| skills = storage.load_skills(enabled_only=False) | ||
| skill = next((s for s in skills if s.name == skill_name and s.category == SkillCategory.CUSTOM), None) | ||
|
|
||
| def _load_response_parts() -> tuple[object, str | None]: |
There was a problem hiding this comment.
Nit: tuple[object, str | None] erases the skill element type, so the _skill_to_response(skill) call below is no longer statically checked. Skill is already imported in this module, so tuple[Skill | None, str | None] keeps the check for free.
Review nit on bytedance#5751: tuple[object, ...] erased the element type for the _skill_to_response call; Skill is already imported in this module.
Closes #5747. Follows the breadcrumb #5729 left in its test docstring.
Why
#5747 documents three filesystem operations still running directly on the Gateway event loop in the custom-skill rollback route and its shared response reader. The largest (
load_skills()in_read_custom_skill_response) walks every installed skill directory and scales with the number of skills — measured at ~580 ms with 200 skills, paid on every accepted rollback and everyGET /skills/custom/{name}. #5729 already established the pattern (offload viaasyncio.to_thread) and named the post-scan read as remaining work.What changed
storage.validate_skill_markdown_content(...)runs throughasyncio.to_thread(site 1 — mirrors the sibling installer path).skill_file.exists()+read_text) runs throughasyncio.to_thread(site 2 — the read fix(skills): rollback reads its history file off the event loop #5729 named)._read_custom_skill_response()loads skills and readsSKILL.mdin a singleasyncio.to_threadclosure (site 3), preserving the exact 404/exception ordering:read_custom_skillstill only runs when the skill was found.Surface area
frontend/backend/applanggraph.json, or prompt changedocker/or sandboxed execution (Gateway runtime hygiene; no sandbox change)skills/backend/pyproject.tomlorfrontend/package.json(say what it buys us)Validation
tests/blocking_io/test_skills_custom_router.py— 3 passed (two pre-existing anchors + the new accepted-path regression).test_skills_custom_router.py,test_skills_router_authz.py,test_user_scoped_skill_storage.py,test_skill_storage_lifecycle.py) — 87 passed.ruff check ./ruff format --check .clean.tests/blocking_io/(channel runtime config store ×3, lark auth route ×1) reproduce identically on cleanmainand are out of this PR's scope.AI assistance
Tool(s) used: ZCode (AI coding agent)
How you used it: implemented the three
asyncio.to_threadoffloads and the blocking_io regression from the issue's documented sites; reviewed the full diff before committing.