diff --git a/flocks/cli/service_manager.py b/flocks/cli/service_manager.py index 46134d52..4f9009a0 100644 --- a/flocks/cli/service_manager.py +++ b/flocks/cli/service_manager.py @@ -328,6 +328,8 @@ def get_node_major_version() -> int | None: check=False, capture_output=True, text=True, + encoding="utf-8", + errors="replace", ) except OSError: return None @@ -528,6 +530,8 @@ def _windows_tasklist_process_name(pid: int) -> str | None: check=False, capture_output=True, text=True, + encoding="utf-8", + errors="replace", ) if completed.returncode != 0: return None @@ -946,6 +950,8 @@ def _process_list_pids() -> list[int]: check=False, capture_output=True, text=True, + encoding="utf-8", + errors="replace", ) else: completed = subprocess.run( @@ -2166,6 +2172,8 @@ def _run_windows_netstat(port: int) -> str: check=False, capture_output=True, text=True, + encoding="utf-8", + errors="replace", ) if completed.returncode != 0: return "" diff --git a/flocks/contracts/webui/store.py b/flocks/contracts/webui/store.py index f47a2f8d..3233386f 100644 --- a/flocks/contracts/webui/store.py +++ b/flocks/contracts/webui/store.py @@ -677,6 +677,8 @@ def _iter_page_dirs(self, root: Path) -> list[tuple[Path, str]]: page_dir = manifest_path.parent if page_dir == root: continue + if self._is_hidden_relative(page_dir, root): + continue page_id = self._manifest_page_id_at(manifest_path) if page_id is None: continue @@ -701,6 +703,8 @@ def _iter_workspace_dirs(self, root: Path) -> list[tuple[Path, WebUIWorkspaceMan workspace_dir = manifest_path.parent if workspace_dir == root: continue + if self._is_hidden_relative(workspace_dir, root): + continue manifest = self._read_workspace_manifest_at(workspace_dir) if manifest is None: continue @@ -764,6 +768,23 @@ def _page_dir_in_root(self, root: Path, page_id: str) -> Path: self._assert_inside_root(page_path, root) return page_path + @staticmethod + def _is_hidden_relative(path: Path, root: Path) -> bool: + """True when *path* lives under a dot-prefixed dir relative to *root*. + + The Hub installer stages WebUI packages into sibling scratch dirs + named ``..`` / ``..bak`` inside this very root + before the atomic swap. On Windows that swap can fail (WinError 5) + while the page watcher holds the tree open, leaving those dot-dirs + behind. Scans must skip them so half-written or stale copies never + surface as real pages (and duplicate the live ones). + """ + try: + rel = path.relative_to(root) + except ValueError: + return False + return any(part.startswith(".") for part in rel.parts) + @staticmethod def _assert_inside_root(path: Path, root: Path) -> None: try: diff --git a/flocks/contracts/webui/watcher.py b/flocks/contracts/webui/watcher.py index 9cbb413f..14eea47f 100644 --- a/flocks/contracts/webui/watcher.py +++ b/flocks/contracts/webui/watcher.py @@ -143,6 +143,14 @@ def _classify_event( return None if not rel.parts: return None + # Ignore events under dot-prefixed dirs. The Hub installer stages + # WebUI packages into sibling scratch dirs (``..`` / + # ``..bak``) inside this watched root before its atomic + # swap. Reacting to those writes would race the installer's own + # build — on Windows the watcher's file handles block the swap + # with a WinError 5 access-denied — and surface half-built pages. + if any(part.startswith(".") for part in rel.parts): + return None if rel.name == WORKSPACE_MANIFEST_FILE: workspace_id = self._store.workspace_id_for_path(src) diff --git a/flocks/hub/installer.py b/flocks/hub/installer.py index bcbbf240..1405dc9c 100644 --- a/flocks/hub/installer.py +++ b/flocks/hub/installer.py @@ -4,7 +4,9 @@ import asyncio import shutil +import sys import tempfile +import time from pathlib import Path from typing import Awaitable, Callable @@ -93,18 +95,11 @@ def _resolve_install_destination( def _copy_package(src: Path, dst: Path) -> None: parent = dst.parent parent.mkdir(parents=True, exist_ok=True) + _purge_stale_scratch(parent, dst.name) tmp = Path(tempfile.mkdtemp(prefix=f".{dst.name}.", dir=str(parent))) try: _copy_package_contents(src, tmp) - backup = None - if dst.exists(): - backup = parent / f".{dst.name}.bak" - if backup.exists(): - shutil.rmtree(backup) - dst.replace(backup) - tmp.replace(dst) - if backup and backup.exists(): - shutil.rmtree(backup) + _replace_dir(tmp, dst) except Exception: if tmp.exists(): shutil.rmtree(tmp, ignore_errors=True) @@ -122,6 +117,53 @@ def _copy_package_contents(src: Path, dst: Path) -> None: shutil.copy2(item, target) +def _purge_stale_scratch(parent: Path, name: str) -> None: + """Remove leftover ``..`` / ``..bak`` staging dirs. + + A failed atomic swap (see :func:`_replace_dir`) can leave scratch and + backup dirs behind next to *parent*/*name*. They are never valid + installs, but on Windows a lingering ``..bak`` blocks the next + swap, so we clear both before staging a fresh copy. + """ + if not parent.is_dir(): + return + for entry in parent.iterdir(): + stale = entry.name.startswith(f".{name}.") or entry.name == f".{name}.bak" + if not stale: + continue + try: + if entry.is_dir() and not entry.is_symlink(): + shutil.rmtree(entry, ignore_errors=True) + else: + entry.unlink() + except OSError: + pass + + +def _replace_with_retry(src: Path, dst: Path) -> None: + """``src.replace(dst)`` with a Windows access-denied backoff. + + On Windows an antivirus scan or a directory watcher (e.g. the WebUI + page watcher over ``~/.flocks/plugins/contracts/webui``) can hold a + transient handle on the freshly written tree, making the atomic swap + fail with ``PermissionError`` (WinError 5 / 32). Elsewhere the rename + is atomic and never needs retrying. + """ + if sys.platform != "win32": + src.replace(dst) + return + delay = 0.1 + for attempt in range(6): + try: + src.replace(dst) + return + except PermissionError: + if attempt == 5: + raise + time.sleep(delay) + delay = min(delay * 2, 1.0) + + def _replace_dir(src: Path, dst: Path) -> None: parent = dst.parent backup = None @@ -129,8 +171,8 @@ def _replace_dir(src: Path, dst: Path) -> None: backup = parent / f".{dst.name}.bak" if backup.exists(): shutil.rmtree(backup) - dst.replace(backup) - src.replace(dst) + _replace_with_retry(dst, backup) + _replace_with_retry(src, dst) if backup and backup.exists(): shutil.rmtree(backup) @@ -185,6 +227,7 @@ def _build_webui_pages(plugin_id: str, install_dir: Path) -> None: def _copy_webui_package_with_build(plugin_id: str, src: Path, dst: Path) -> None: parent = dst.parent parent.mkdir(parents=True, exist_ok=True) + _purge_stale_scratch(parent, dst.name) tmp = Path(tempfile.mkdtemp(prefix=f".{dst.name}.", dir=str(parent))) try: _copy_package_contents(src, tmp) diff --git a/tests/cli/test_service_manager.py b/tests/cli/test_service_manager.py index d75c1cac..dd590ac4 100644 --- a/tests/cli/test_service_manager.py +++ b/tests/cli/test_service_manager.py @@ -522,6 +522,87 @@ def test_run_windows_netstat_handles_missing_stdout(monkeypatch) -> None: assert service_manager._run_windows_netstat(5173) == "" +def test_run_windows_netstat_decodes_with_replacement(monkeypatch) -> None: + """netstat output is decoded as utf-8/replace so GBK console output on + Chinese Windows never raises UnicodeDecodeError in the reader thread.""" + seen: dict[str, object] = {} + + def fake_run(*_args, **kwargs): + seen.update(kwargs) + return SimpleNamespace(returncode=0, stdout=" TCP 127.0.0.1:5173 0.0.0.0:0 LISTENING 42\n") + + monkeypatch.setattr(service_manager.subprocess, "run", fake_run) + + assert "42" in service_manager._run_windows_netstat(5173) + assert seen.get("encoding") == "utf-8" + assert seen.get("errors") == "replace" + + +def test_windows_tasklist_process_name_decodes_with_replacement(monkeypatch) -> None: + monkeypatch.setattr(service_manager.sys, "platform", "win32") + seen: dict[str, object] = {} + + def fake_run(*_args, **kwargs): + seen.update(kwargs) + return SimpleNamespace(returncode=0, stdout='"python.exe","9436"\n') + + monkeypatch.setattr(service_manager.subprocess, "run", fake_run) + + assert service_manager._windows_tasklist_process_name(9436) == "python.exe" + assert seen.get("encoding") == "utf-8" + assert seen.get("errors") == "replace" + + +def test_process_list_pids_windows_decodes_with_replacement(monkeypatch) -> None: + monkeypatch.setattr(service_manager.sys, "platform", "win32") + seen: dict[str, object] = {} + + def fake_run(*_args, **kwargs): + seen.update(kwargs) + return SimpleNamespace(returncode=0, stdout="9436\n36056\n") + + monkeypatch.setattr(service_manager.subprocess, "run", fake_run) + + assert service_manager._process_list_pids() == [9436, 36056] + assert seen.get("encoding") == "utf-8" + assert seen.get("errors") == "replace" + + +def test_get_node_major_version_decodes_with_replacement(monkeypatch) -> None: + monkeypatch.setattr(service_manager, "resolve_node_executable", lambda: "node") + seen: dict[str, object] = {} + + def fake_run(*_args, **kwargs): + seen.update(kwargs) + return SimpleNamespace(returncode=0, stdout="v20.11.1\n") + + monkeypatch.setattr(service_manager.subprocess, "run", fake_run) + + assert service_manager.get_node_major_version() == 20 + assert seen.get("encoding") == "utf-8" + assert seen.get("errors") == "replace" + + +def test_windows_process_probes_survive_gbk_bytes(monkeypatch) -> None: + """End-to-end: real GBK-encoded bytes flowing through the decode path + must not raise UnicodeDecodeError (the restart-time reader-thread crash).""" + monkeypatch.setattr(service_manager.sys, "platform", "win32") + # 0xbb is the byte that crashed utf-8 decode in the original bug report. + gbk_stdout = "映像名称: python.exe 拒绝访问".encode("gbk").decode("utf-8", errors="replace") + + def fake_run(*_args, **kwargs): + assert kwargs.get("encoding") == "utf-8" + assert kwargs.get("errors") == "replace" + return SimpleNamespace(returncode=0, stdout=gbk_stdout) + + monkeypatch.setattr(service_manager.subprocess, "run", fake_run) + + # None of these should raise, even with mojibake stdout. + service_manager._windows_tasklist_process_name(123) + service_manager._run_windows_netstat(5173) + service_manager._process_list_pids() + + def test_port_owner_pids_warns_when_no_tool_found(monkeypatch) -> None: monkeypatch.setattr(service_manager.sys, "platform", "linux") monkeypatch.setattr(service_manager, "which", lambda _name: None) diff --git a/tests/conftest.py b/tests/conftest.py index ecb8e12c..651e714f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ """ import os +from pathlib import Path import pytest @@ -37,3 +38,51 @@ def pytest_runtest_setup(item): for marker_name, (env_var, reason) in _API_KEY_MARKERS.items(): if item.get_closest_marker(marker_name) and not os.getenv(env_var): pytest.skip(reason) + + +@pytest.fixture(autouse=True) +def _home_honors_env(monkeypatch): + """Make ``Path.home()`` follow the ``HOME`` env var on every platform. + + Many suites isolate their filesystem by ``monkeypatch.setenv("HOME", ...)`` + and then rely on production code that resolves ``Path.home() / ".flocks"`` + (hub installs, plugin roots, the WebUI contract store, ...). That works on + Linux CI, where ``Path.home()`` honors ``$HOME`` — but on Windows + ``Path.home()`` reads ``%USERPROFILE%`` and ignores ``HOME`` entirely, so + those tests silently leak into (and assert against) the *real* user home. + + This autouse fixture patches ``pathlib.Path.home`` to honor ``HOME`` (then + ``USERPROFILE``) at call time, aligning Windows with Linux CI. It is + behavior-preserving when ``HOME`` already points at the real home (the + default), so tests that never touch ``HOME`` see no change. + """ + def _home() -> Path: + candidate = os.environ.get("HOME") or os.environ.get("USERPROFILE") + if candidate: + return Path(candidate) + return Path(os.path.expanduser("~")) + + monkeypatch.setattr(Path, "home", staticmethod(_home)) + + # ``Path.home()`` covers explicit home lookups, but ``~``/``~user`` tilde + # expansion goes through ``os.path.expanduser`` (and ``Path.expanduser``), + # which on Windows keys off ``%USERPROFILE%``/``%HOMEDRIVE%%HOMEPATH%`` and + # likewise ignores ``HOME``. Route a bare ``~`` / ``~/...`` through the same + # ``HOME``-aware root so tilde-based tests are hermetic too. Anything else + # (``~otheruser``) falls back to the real implementation untouched. + _real_expanduser = os.path.expanduser + + def _expanduser(path): + text = os.fspath(path) + # Only rewrite plain ``str`` tilde paths; leave bytes and ``~user`` + # forms to the real implementation so we never change its type + # contract or raise on a bytes path. + if isinstance(text, str): + home = os.environ.get("HOME") or os.environ.get("USERPROFILE") + if home and (text == "~" or text.startswith("~/") or text.startswith("~\\")): + return home + text[1:] + return _real_expanduser(path) + + monkeypatch.setattr(os.path, "expanduser", _expanduser) + + diff --git a/tests/contracts/webui/test_store.py b/tests/contracts/webui/test_store.py index d88f3517..9f00ed09 100644 --- a/tests/contracts/webui/test_store.py +++ b/tests/contracts/webui/test_store.py @@ -109,6 +109,40 @@ def test_list_pages_scans_user_and_project_roots_with_user_priority(tmp_path): assert store.page_dir("project-page").is_relative_to(project_root) +def test_list_pages_skips_installer_scratch_dirs(tmp_path): + """Pages that exist only under the Hub installer's ``..`` / + ``..bak`` scratch dirs must never surface as real pages. + + Reproduces the Windows WinError 5 aftermath where a failed atomic swap + left the SOC pages' manifests stuck inside scratch dirs while the real + install was incomplete. + """ + user_root = tmp_path / "user" / "contracts" / "webui" + _write_page(user_root, "real-page", "Real Page") + # Manifests stranded in leftover scratch/backup dirs (no real install). + _write_page_at(user_root, ".soc_ui.abc123/soc_overview", "soc-overview", "Stranded Overview") + _write_page_at(user_root, ".soc_ui.bak/soc_dashboard", "soc-dashboard", "Stranded Dashboard") + + store = WebUIPagesStore(root=user_root, project_root=None, legacy_root=None) + + pages = store.list_pages() + assert [page.id for page in pages] == ["real-page"] + assert pages[0].title == "Real Page" + + +def test_list_workspaces_skips_installer_scratch_dirs(tmp_path): + user_root = tmp_path / "user" / "contracts" / "webui" + _write_workspace(user_root, "real_ws", "Real Workspace") + # A workspace manifest stranded under a scratch dir must be ignored. + _write_workspace(user_root / ".soc_ui.abc123", "soc_ui", "Stranded Workspace") + + store = WebUIPagesStore(root=user_root, project_root=None, legacy_root=None) + + workspaces = store.list_workspaces() + assert [workspace.id for workspace in workspaces] == ["real_ws"] + assert workspaces[0].title == "Real Workspace" + + def test_grouped_page_directory_uses_manifest_id_for_lookup(tmp_path): user_root = tmp_path / "user" / "contracts" / "webui" _write_workspace(user_root, "scene_workspace", "场景工作区") diff --git a/tests/contracts/webui/test_watcher.py b/tests/contracts/webui/test_watcher.py index a5043c6a..271bbfc4 100644 --- a/tests/contracts/webui/test_watcher.py +++ b/tests/contracts/webui/test_watcher.py @@ -104,3 +104,39 @@ def test_watcher_classifies_workspace_manifest_change(tmp_path): assert page_id == "scene_workspace" assert pending.manifest_changed + + +def test_watcher_ignores_installer_scratch_dir_events(tmp_path): + """Events under the Hub installer's ``..`` / ``..bak`` + scratch dirs must be dropped *before* any store lookup — reacting there + races the installer's atomic swap and on Windows blocks it with a + WinError 5 access-denied. + + The store is stubbed to raise if consulted, proving the dot-dir guard + short-circuits ahead of path resolution rather than relying on the + store's own scratch-dir filtering. + """ + root = tmp_path / "webui_pages" + root.mkdir(parents=True) + + class _RaisingStore: + def workspace_id_for_path(self, _path): + raise AssertionError("store should not be consulted for scratch-dir events") + + def page_id_for_path(self, _path): + raise AssertionError("store should not be consulted for scratch-dir events") + + watcher = WebUIPagesWatcher( + store=_RaisingStore(), builder=_BuilderStub(), api_runtime=_RuntimeStub() + ) + + for candidate in ( + root / ".soc_ui.abc123" / "soc_overview" / "src" / "index.tsx", + root / ".soc_ui.abc123" / "soc_overview" / "manifest.json", + root / ".soc_ui.abc123" / "soc_overview" / "dist" / "page.js", + root / ".soc_ui.bak" / "soc_overview" / "workspace.json", + ): + assert ( + watcher._classify_event(candidate, root, event_type="modified", is_directory=False) + is None + ) diff --git a/tests/hub/test_installer_swap.py b/tests/hub/test_installer_swap.py new file mode 100644 index 00000000..76bbac03 --- /dev/null +++ b/tests/hub/test_installer_swap.py @@ -0,0 +1,121 @@ +"""Unit tests for the Hub installer's Windows-safe directory swap helpers. + +These exercise the atomic-swap path in isolation (explicit ``tmp_path`` +dirs, no ``Path.home()`` dependency), covering the WinError 5 aftermath +where a directory watcher or AV scan holds a transient handle on the +freshly staged tree and blocks ``src.replace(dst)``. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from flocks.hub import installer + + +def _make_tree(path: Path, marker: str) -> None: + path.mkdir(parents=True, exist_ok=True) + (path / "manifest.json").write_text(marker, encoding="utf-8") + + +def test_replace_dir_swaps_into_place(tmp_path): + src = tmp_path / ".soc_ui.scratch" + dst = tmp_path / "soc_ui" + _make_tree(src, "new") + + installer._replace_dir(src, dst) + + assert (dst / "manifest.json").read_text(encoding="utf-8") == "new" + assert not src.exists() + assert not (tmp_path / ".soc_ui.bak").exists() + + +def test_replace_dir_overwrites_existing_and_removes_backup(tmp_path): + src = tmp_path / ".soc_ui.scratch" + dst = tmp_path / "soc_ui" + _make_tree(src, "new") + _make_tree(dst, "old") + + installer._replace_dir(src, dst) + + assert (dst / "manifest.json").read_text(encoding="utf-8") == "new" + assert not (tmp_path / ".soc_ui.bak").exists() + + +def test_replace_with_retry_recovers_from_transient_permission_error(tmp_path, monkeypatch): + """A first ``PermissionError`` (WinError 5) is retried, not surfaced.""" + monkeypatch.setattr(installer.sys, "platform", "win32") + monkeypatch.setattr(installer.time, "sleep", lambda _s: None) + + src = tmp_path / ".soc_ui.scratch" + dst = tmp_path / "soc_ui" + _make_tree(src, "new") + + real_replace = Path.replace + calls = {"n": 0} + + def flaky_replace(self, target): + calls["n"] += 1 + if calls["n"] == 1: + raise PermissionError("[WinError 5] Access is denied") + return real_replace(self, target) + + monkeypatch.setattr(Path, "replace", flaky_replace) + installer._replace_with_retry(src, dst) + + assert calls["n"] == 2 + assert (dst / "manifest.json").read_text(encoding="utf-8") == "new" + + +def test_replace_with_retry_reraises_after_exhausting_attempts(tmp_path, monkeypatch): + monkeypatch.setattr(installer.sys, "platform", "win32") + monkeypatch.setattr(installer.time, "sleep", lambda _s: None) + + src = tmp_path / ".soc_ui.scratch" + dst = tmp_path / "soc_ui" + _make_tree(src, "new") + + def always_denied(self, target): + raise PermissionError("[WinError 5] Access is denied") + + monkeypatch.setattr(Path, "replace", always_denied) + with pytest.raises(PermissionError): + installer._replace_with_retry(src, dst) + + +def test_purge_stale_scratch_removes_leftovers(tmp_path): + parent = tmp_path + _make_tree(parent / ".soc_ui.55ram7wo" / "soc_overview", "stranded") + _make_tree(parent / ".soc_ui.bak" / "soc_dashboard", "stranded") + _make_tree(parent / "soc_ui", "live") + # Unrelated dot-dir for a different plugin must be left untouched. + _make_tree(parent / ".other.bak", "keep") + + installer._purge_stale_scratch(parent, "soc_ui") + + assert not (parent / ".soc_ui.55ram7wo").exists() + assert not (parent / ".soc_ui.bak").exists() + assert (parent / "soc_ui" / "manifest.json").read_text(encoding="utf-8") == "live" + assert (parent / ".other.bak").exists() + + +def test_copy_package_purges_stale_scratch_before_staging(tmp_path): + """A prior failed install's leftovers self-heal on the next install.""" + src = tmp_path / "bundled" / "soc_ui" + _make_tree(src, "payload") + (src / "manifest.json").write_text('{"id": "soc_ui"}', encoding="utf-8") + (src / "src").mkdir() + (src / "src" / "index.tsx").write_text("export default 1;\n", encoding="utf-8") + + dst = tmp_path / "install" / "soc_ui" + dst.parent.mkdir(parents=True) + _make_tree(dst.parent / ".soc_ui.leftover" / "soc_overview", "stranded") + + installer._copy_package(src, dst) + + assert (dst / "src" / "index.tsx").is_file() + assert not (dst.parent / ".soc_ui.leftover").exists() + # ``manifest.json`` is intentionally not copied by the installer. + assert not (dst / "manifest.json").exists()