From 03a8ad30717d64bbf04402f392b8eee3f4132742 Mon Sep 17 00:00:00 2001 From: IronMurphy Date: Tue, 6 Oct 2026 20:12:39 +0800 Subject: [PATCH] fix(extensions): publish a snapshot restore atomically (#6366) * fix(extensions): publish a snapshot restore atomically _FileSnapshot.restore() rolled a failed extension install back with Path.write_bytes, which opens with O_TRUNC: if the process died during the rollback, pyproject.toml / uv.lock / config.yaml were left half written and the failed install took the checkout with it. Publish through a temporary file plus os.replace, the pattern the rest of the codebase already uses. * fix(extensions): preserve snapshot permissions on rollback --------- Co-authored-by: Willem Jiang --- README.md | 8 +- .../harness/deerflow/extensions/AGENTS.md | 4 + .../harness/deerflow/extensions/manager.py | 31 ++++- .../tests/test_extension_snapshot_restore.py | 107 ++++++++++++++++++ 4 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 backend/tests/test_extension_snapshot_restore.py diff --git a/README.md b/README.md index a6674ad08c..b085590ce6 100644 --- a/README.md +++ b/README.md @@ -1641,8 +1641,12 @@ Management commands bootstrap the checkout environment without the extension gro when an installed extension's remote source or managed snapshot has become unavailable, while a fresh checkout can still create the non-extension environment from the existing lock. The manager itself owns the subsequent locked dependency transaction. -Mutations for one checkout are serialized through a process lock. The initial manager -surface is create/remove rather than in-place upgrade: to change an installed source, save +Mutations for one checkout are serialized through a process lock. +Failed extension operations restore dependency files and configuration atomically with +their original file permissions, preserving access for a Gateway that reads them through +group permissions. +The initial manager surface is create/remove rather than in-place upgrade: to change an +installed source, save its private `plugins[].config`, remove it, reinstall the new pin, and restore that config. Local-directory installs are copied into diff --git a/backend/packages/harness/deerflow/extensions/AGENTS.md b/backend/packages/harness/deerflow/extensions/AGENTS.md index 97b0db7383..44dd07a840 100644 --- a/backend/packages/harness/deerflow/extensions/AGENTS.md +++ b/backend/packages/harness/deerflow/extensions/AGENTS.md @@ -43,6 +43,10 @@ case rather than reviving a record whose package declaration may already be gone cancellation skips the recovery sync entirely — the declarations are already restored and the next locked startup sync reconciles the environment, whereas blocking an interrupt on a full dependency resolve invites a second interrupt that escapes the handler mid-transaction. +File snapshots capture the original permission bits alongside the bytes; rollback applies +that mode to a temporary file before atomically replacing the destination, including when +restoring `config.yaml` after a failed removal. A failed write, chmod, or replace leaves the +current destination untouched and removes the temporary file. Package mutation is deferred from environment mutation: after `uv add/remove` updates the declaration and lock, one `uv sync --locked --all-packages` preserves the same config-/environment-detected optional extras as normal startup. All three uv calls pin the diff --git a/backend/packages/harness/deerflow/extensions/manager.py b/backend/packages/harness/deerflow/extensions/manager.py index 854f4235dd..03b6cd4806 100644 --- a/backend/packages/harness/deerflow/extensions/manager.py +++ b/backend/packages/harness/deerflow/extensions/manager.py @@ -9,6 +9,7 @@ import os import re import shlex import shutil +import stat import subprocess import sys import tempfile @@ -96,20 +97,46 @@ class ConfiguredExtension: required: bool +def _write_bytes_atomically(path: Path, content: bytes, mode: int | None) -> None: + """Publish *content* at *path* so an interrupted restore cannot truncate it. + + ``Path.write_bytes`` opens with ``O_TRUNC``, so a rollback that dies midway + leaves pyproject.toml / uv.lock / config.yaml half written — the failed + install would then take the checkout with it. The rest of the codebase + already publishes through a temporary file plus ``os.replace``. + """ + fd, temporary = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.", suffix=".tmp") + try: + with os.fdopen(fd, "wb") as handle: + handle.write(content) + handle.flush() + if mode is not None: + os.chmod(temporary, mode) + os.fsync(handle.fileno()) + os.replace(temporary, path) + except BaseException: + Path(temporary).unlink(missing_ok=True) + raise + + @dataclass(frozen=True) class _FileSnapshot: path: Path content: bytes | None + mode: int | None @classmethod def capture(cls, path: Path) -> _FileSnapshot: - return cls(path, path.read_bytes() if path.exists() else None) + if not path.exists(): + return cls(path, None, None) + with path.open("rb") as handle: + return cls(path, handle.read(), stat.S_IMODE(os.fstat(handle.fileno()).st_mode)) def restore(self) -> None: if self.content is None: self.path.unlink(missing_ok=True) else: - self.path.write_bytes(self.content) + _write_bytes_atomically(self.path, self.content, self.mode) def _read_optional_bytes(path: Path) -> bytes | None: diff --git a/backend/tests/test_extension_snapshot_restore.py b/backend/tests/test_extension_snapshot_restore.py new file mode 100644 index 0000000000..519ffbe612 --- /dev/null +++ b/backend/tests/test_extension_snapshot_restore.py @@ -0,0 +1,107 @@ +# SPDX-License-Identifier: MIT +"""A failed extension install must not corrupt the files it rolls back.""" + +from __future__ import annotations + +import os +import stat + +import pytest + +from deerflow.extensions.manager import _FileSnapshot + + +def test_restore_keeps_the_previous_content_when_the_publish_fails(tmp_path, monkeypatch): + target = tmp_path / "pyproject.toml" + target.write_text("original\n", encoding="utf-8") + snapshot = _FileSnapshot.capture(target) + + # The install failed, so the file was rewritten; now the rollback runs. + target.write_text("half written by the failed install", encoding="utf-8") + + def _boom(*args, **kwargs): + raise OSError("device full") + + monkeypatch.setattr(os, "replace", _boom) + + with pytest.raises(OSError): + snapshot.restore() + + # write_bytes would have truncated the file before failing, taking the + # checkout with it; publishing through a temporary file cannot. + assert target.read_text(encoding="utf-8") == "half written by the failed install" + + +def test_restore_still_replaces_the_file(tmp_path): + target = tmp_path / "config.yaml" + target.write_text("original\n", encoding="utf-8") + snapshot = _FileSnapshot.capture(target) + + target.write_text("changed\n", encoding="utf-8") + snapshot.restore() + + assert target.read_text(encoding="utf-8") == "original\n" + assert list(tmp_path.glob(".*tmp")) == [] + + +def test_restore_removes_a_file_that_did_not_exist(tmp_path): + target = tmp_path / "uv.lock" + snapshot = _FileSnapshot.capture(target) + target.write_text("created by the failed install\n", encoding="utf-8") + + snapshot.restore() + + assert not target.exists() + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX file permissions") +@pytest.mark.parametrize("mode", [0o644, 0o640, 0o600]) +def test_restore_preserves_the_captured_permissions(tmp_path, mode): + target = tmp_path / "config.yaml" + target.write_text("original\n", encoding="utf-8") + target.chmod(mode) + snapshot = _FileSnapshot.capture(target) + + target.write_text("changed\n", encoding="utf-8") + target.chmod(0o660) + snapshot.restore() + + assert target.read_text(encoding="utf-8") == "original\n" + assert stat.S_IMODE(target.stat().st_mode) == mode + assert list(tmp_path.glob(".*tmp")) == [] + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX file permissions") +def test_restore_recreates_a_deleted_file_with_its_captured_permissions(tmp_path): + target = tmp_path / "uv.lock" + target.write_text("original\n", encoding="utf-8") + target.chmod(0o644) + snapshot = _FileSnapshot.capture(target) + target.unlink() + + snapshot.restore() + + assert target.read_text(encoding="utf-8") == "original\n" + assert stat.S_IMODE(target.stat().st_mode) == 0o644 + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX file permissions") +def test_restore_keeps_the_current_file_when_setting_permissions_fails(tmp_path, monkeypatch): + target = tmp_path / "config.yaml" + target.write_text("original\n", encoding="utf-8") + target.chmod(0o640) + snapshot = _FileSnapshot.capture(target) + target.write_text("changed\n", encoding="utf-8") + target.chmod(0o660) + + def _boom(*args, **kwargs): + raise OSError("cannot set permissions") + + monkeypatch.setattr(os, "chmod", _boom) + + with pytest.raises(OSError, match="cannot set permissions"): + snapshot.restore() + + assert target.read_text(encoding="utf-8") == "changed\n" + assert stat.S_IMODE(target.stat().st_mode) == 0o660 + assert list(tmp_path.glob(".*tmp")) == []