mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
docs(lark): pin atomic publish cleanup so it cannot mask the error (#7329)
Document that a failed write, fsync, or replace in _publish_bytes_atomically leaves the previous file intact, and that temporary removal must not replace that error, including interruptions. Add site-specific regression tests for unlink and warning-log failures. The cleanup_failed_atomic_write call itself already landed on main in #7200. Co-authored-by: wannT-1 <200890097+wannT-1@users.noreply.github.com>
This commit is contained in:
parent
5bda01f524
commit
f481c7e19f
@ -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`:
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
@ -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"
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user