diff --git a/backend/packages/harness/deerflow/skills/mutations/publication.py b/backend/packages/harness/deerflow/skills/mutations/publication.py index 8191d5fb5a..f291f27ab5 100644 --- a/backend/packages/harness/deerflow/skills/mutations/publication.py +++ b/backend/packages/harness/deerflow/skills/mutations/publication.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging import os import stat @@ -12,6 +13,8 @@ from deerflow.skills.mutations.assets import capture_package, open_directory_cha from deerflow.skills.mutations.codec import operation_packages from deerflow.skills.mutations.repository import supersede_views +logger = logging.getLogger(__name__) + def replace_main(root, content: bytes, *, operation_id: str) -> None: """Use the validated directory descriptor throughout; preserve mode bits.""" @@ -48,6 +51,12 @@ def replace_main(root, content: bytes, *, operation_id: str) -> None: os.unlink(temporary, dir_fd=parent) except FileNotFoundError: pass + except OSError: + # The publication outcome is already decided; a leftover + # temp is reported here and detected again by + # ``assert_temporary_absent``, so it must not replace that + # outcome with the cleanup error. + logger.warning("Failed to remove skill mutation temporary %s", temporary, exc_info=True) finally: os.close(directory) if parent is not None: diff --git a/backend/tests/test_skill_publication_cleanup.py b/backend/tests/test_skill_publication_cleanup.py new file mode 100644 index 0000000000..b5dbe7bf7b --- /dev/null +++ b/backend/tests/test_skill_publication_cleanup.py @@ -0,0 +1,60 @@ +"""A failed discard must not replace the outcome of a skill publication. + +``replace_main`` publishes ``SKILL.md`` through a ``.host-mutation-*`` sibling and +discards it on exit. That discard only tolerated ``FileNotFoundError``, so any +other ``OSError`` replaced the publish error — or turned a decided outcome into a +cleanup failure. A leftover temp is still caught later by +``assert_temporary_absent``, so reporting is enough here. +""" + +from __future__ import annotations + +import os + +import pytest + +# ``replace_main`` walks directory descriptors with ``O_DIRECTORY``/``O_NOFOLLOW``. +pytestmark = pytest.mark.skipif(os.name != "posix", reason="host skill mutation tests require POSIX filesystem primitives") + + +def _make_skill_dir(tmp_path): + root = tmp_path / "skills" / "example" + root.mkdir(parents=True) + (root / "SKILL.md").write_text("original\n", encoding="utf-8") + return root + + +def test_replace_main_keeps_the_publish_error_when_cleanup_fails(tmp_path, monkeypatch, caplog): + from deerflow.skills.mutations import publication + + root = _make_skill_dir(tmp_path) + real_unlink = publication.os.unlink + + def failing_fsync(_fd): + raise OSError("disk error") + + def failing_unlink(path, *args, **kwargs): + if str(path).startswith(".host-mutation-"): + raise PermissionError("temporary is still open") + return real_unlink(path, *args, **kwargs) + + monkeypatch.setattr(publication.os, "fsync", failing_fsync) + monkeypatch.setattr(publication.os, "unlink", failing_unlink) + + with caplog.at_level("WARNING", logger="deerflow.skills.mutations.publication"): + with pytest.raises(OSError, match="disk error"): + publication.replace_main(root, b"replacement\n", operation_id="cleanup") + + assert (root / "SKILL.md").read_text(encoding="utf-8") == "original\n" + assert "Failed to remove skill mutation temporary" in caplog.text + + +def test_replace_main_still_publishes_and_removes_the_temporary(tmp_path): + from deerflow.skills.mutations import publication + + root = _make_skill_dir(tmp_path) + + publication.replace_main(root, b"replacement\n", operation_id="clean") + + assert (root / "SKILL.md").read_text(encoding="utf-8") == "replacement\n" + assert [p.name for p in root.parent.iterdir() if p.name.startswith(".host-mutation-")] == []