diff --git a/backend/packages/harness/deerflow/AGENTS.md b/backend/packages/harness/deerflow/AGENTS.md index 4badff8de5..d6999c6384 100644 --- a/backend/packages/harness/deerflow/AGENTS.md +++ b/backend/packages/harness/deerflow/AGENTS.md @@ -39,8 +39,12 @@ skill bodies and manifest together, keeping earlier writes private. A write failure leaves the previous pack unchanged and discards staging; replacement moves the old target to a backup first and restores it on an ordinary publication error. Atomic writes fsync only their temporary files, so -whole-tree durability after power loss is not guaranteed. Callers own final -file modes: runtime artifacts executable, skill bodies and manifests read-only. +whole-tree durability after power loss is not guaranteed. +`_publish_bytes_atomically` removes a failed temporary through +`cleanup_failed_atomic_write`, so an unlink or warning-log failure cannot +replace the original publication error, including interruptions. Callers own +final file modes: runtime artifacts executable, skill bodies and manifests +read-only. Coverage: `tests/test_lark_cli_atomic_publish.py`. App registration and direct app switching replace the per-user Lark credential tree transactionally. Clear the old OAuth data before `lark-cli config init`: diff --git a/backend/packages/harness/deerflow/integrations/lark_cli.py b/backend/packages/harness/deerflow/integrations/lark_cli.py index 9dfdfd8e06..cfc788966c 100644 --- a/backend/packages/harness/deerflow/integrations/lark_cli.py +++ b/backend/packages/harness/deerflow/integrations/lark_cli.py @@ -1272,6 +1272,8 @@ def _publish_bytes_atomically(path: Path, content: bytes) -> None: staged skill files and manifests become read-only through ``make_skill_tree_sandbox_readable``. The parent directory is not fsynced, so this helper alone does not guarantee publication survives power loss. + A failed write, fsync, or replace leaves the previous file intact. + Removing the temporary is best-effort and must not replace that error. """ fd, temporary_name = tempfile.mkstemp(dir=str(path.parent), prefix=f".{path.name}.", suffix=".tmp") temporary = Path(temporary_name) diff --git a/backend/tests/AGENTS.md b/backend/tests/AGENTS.md index 71dd0e9538..fdd2ac7fdc 100644 --- a/backend/tests/AGENTS.md +++ b/backend/tests/AGENTS.md @@ -195,7 +195,10 @@ the real installer: the previous installed tree must stay byte-identical while the write is still in staging and after it fails, with no staging or backup tree left behind. Do not replace these behavioral checks with assertions about source formatting or duplicate the runtime publication control in -`test_lark_cli_atomic_publish.py`. +`test_lark_cli_atomic_publish.py`. That runtime publisher must also keep the +original publish error when temporary cleanup or its warning log fails, +including interruptions. A successful publish still replaces the bytes, leaves +no temporary file, and keeps the temporary file's mode; callers chmod afterward. ## Channel staging cleanup diff --git a/backend/tests/test_lark_cli_atomic_publish.py b/backend/tests/test_lark_cli_atomic_publish.py index d874b89f52..3cd8dcaefa 100644 --- a/backend/tests/test_lark_cli_atomic_publish.py +++ b/backend/tests/test_lark_cli_atomic_publish.py @@ -3,7 +3,10 @@ from __future__ import annotations +import logging import os +import stat +from pathlib import Path import pytest @@ -35,6 +38,77 @@ def test_a_failed_publish_keeps_the_previous_binary(tmp_path, monkeypatch): assert list(tmp_path.glob("*.tmp")) == [] +@pytest.mark.parametrize("primary_type", [OSError, KeyboardInterrupt]) +@pytest.mark.parametrize("failure", ["none", "unlink", "logging"]) +def test_publish_preserves_original_error_when_cleanup_fails(tmp_path, monkeypatch, caplog, primary_type, failure): + target = tmp_path / "lark-cli" + target.write_bytes(b"old runtime") + primary = primary_type("publish failed") + staged: list[Path] = [] + real_replace = os.replace + real_unlink = Path.unlink + + def replace(source, destination): + staged.append(Path(source)) + if Path(destination) == target: + raise primary + return real_replace(source, destination) + + monkeypatch.setattr(os, "replace", replace) + if failure in {"unlink", "logging"}: + + def unlink(path, *args, **kwargs): + if staged and path == staged[0]: + raise OSError("temporary unlink failed") + return real_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", unlink) + if failure == "logging": + + def boom_log(*args, **kwargs): + raise RuntimeError("log handler failed") + + monkeypatch.setattr("deerflow.utils.atomic_write.logger.warning", boom_log) + + try: + with caplog.at_level(logging.WARNING, logger="deerflow.utils.atomic_write"): + with pytest.raises(primary_type) as raised: + _publish_bytes_atomically(target, b"new runtime") + + assert raised.value is primary + assert "temporary unlink failed" not in str(raised.value) + assert target.read_bytes() == b"old runtime" + assert len(staged) == 1 + if failure == "none": + assert not staged[0].exists() + assert "Failed to remove temporary output file" not in caplog.text + else: + assert staged[0].read_bytes() == b"new runtime" + if failure == "unlink": + assert "Failed to remove temporary output file" in caplog.text + assert str(staged[0]) in caplog.text + assert "temporary unlink failed" in caplog.text + finally: + if staged: + real_unlink(staged[0], missing_ok=True) + + +def test_success_replaces_bytes_and_keeps_the_temporary_mode(tmp_path): + target = tmp_path / "linux-amd64" / "lark-cli" + target.parent.mkdir() + target.write_bytes(b"old runtime") + if os.name != "nt": + target.chmod(0o755) + + _publish_bytes_atomically(target, b"new runtime") + + assert target.read_bytes() == b"new runtime" + if os.name != "nt": + # mkstemp publishes mode 0o600; callers chmod the final path afterward. + assert stat.S_IMODE(target.stat().st_mode) == 0o600 + assert [path for path in target.parent.iterdir() if path.suffix == ".tmp"] == [] + + def test_truncating_write_would_have_destroyed_it(tmp_path): """Control: the previous implementation loses the binary in the same failure.""" target = tmp_path / "lark-cli"