fix(fleet): harden Windows sharing-violation retries and TUI directory picker

CHANGELOG.md

@@ -50,6 +50,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   Conductor now reads both fields through a shared helper that tries the 2.x
   name and falls back to the 1.x one, preserving compatibility with both MCP
   1.x and 2.x.
+- **The Fleet Manager TUI's launch-directory picker no longer clobbers its
+  own prefill** (#486). Textual posts a `NodeHighlighted` event for the
+  directory tree's own root at mount, from reactive initialisation, with no
+  user interaction involved; `DirectoryPickerModal` mirrored every such
+  event into the input, silently replacing the prefilled launch directory
+  with its parent before the user ever touched the tree. The mirror now
+  fires only while the tree actually has focus.
+- **A run record could silently fail to be removed on Windows** (#486).
+  `remove_run_record` deleted a record with a single unretried `unlink`, and
+  `remove_run_record_for_current_process` renamed it into a quarantine path
+  with a single unretried `rename`; on Windows, a concurrent reader can make
+  either fail with a sharing violation, leaving a stale record behind. Both
+  paths now use the same bounded retry that `write_run_record` already used
+  for its own `os.replace`.
 
 ## [0.1.33](https://github.com/microsoft/conductor/compare/v0.1.32...v0.1.33) - 2026-08-18
 

docs/fleet.md

@@ -314,7 +314,10 @@ policy, two presentations, sharing the same underlying implementation).
 `d` (Runs) / `ctrl+d` (New run) opens a directory picker — type a path, or
 browse a tree rooted at the current directory's parent (so a sibling
 checkout is one keypress away) — and sets the TUI's **launch directory**
-for the rest of this `conductor fleet` session.
+for the rest of this `conductor fleet` session. The input is pre-filled
+with the current launch directory; browsing the tree updates it as you go,
+and either pressing Enter in the input, or pressing Enter or clicking a
+directory in the tree, accepts it.
 
 The launch directory affects two things:
 

src/conductor/fleet/records.py

@@ -50,10 +50,11 @@
 import tempfile
 import time
 import uuid
+from collections.abc import Callable
 from dataclasses import dataclass
 from datetime import datetime
 from pathlib import Path
-from typing import Any, Literal, cast, get_args
+from typing import Any, Final, Literal, cast, get_args
 
 from conductor.cli import pid as cli_pid
 from conductor.run_id import RUN_ID_PATTERN_SOURCE
@@ -70,12 +71,14 @@
 
 _VALID_MODES: frozenset[str] = frozenset(get_args(RunMode))
 
-# Bounded retry for the Windows-only `os.replace` sharing violation -- see
-# `_replace_with_retry`. Deliberately short: the contended window is a single
-# small-file read, and a genuine permission problem must still surface rather
-# than being hidden behind a long stall.
-_REPLACE_RETRIES = 10
-_REPLACE_RETRY_DELAY_SECONDS = 0.02
+# Bounded retry for the Windows-only sharing-violation family (`os.replace`
+# on write, `os.unlink` on remove, `os.rename` into quarantine on
+# self-cleanup) -- see `_retry_on_windows_sharing_violation`. Deliberately
+# short: the contended window is a single small-file read, and a genuine
+# permission problem must still surface rather than being hidden behind a
+# long stall.
+_SHARING_VIOLATION_RETRIES: Final[int] = 10
+_SHARING_VIOLATION_RETRY_DELAY_SECONDS = 0.02
 
 # The path-safe run-id contract itself now lives in ``conductor.run_id`` (the
 # leaf module ``engine/event_log.py`` also depends on, without pulling in
@@ -429,35 +432,82 @@ def run_records_dir() -> Path:
     return d
 
 
-def _replace_with_retry(tmp_name: str, filepath: Path) -> None:
-    """``os.replace`` the temp file into place, retrying briefly on Windows.
+def _retry_on_windows_sharing_violation(op: Callable[[], None]) -> None:
+    """Run ``op``, retrying briefly on Windows if it raises ``PermissionError``.
 
-    POSIX ``rename`` is atomic and never fails because a reader has the
-    destination open. Windows is different: ``os.replace`` raises
-    ``PermissionError`` (``ERROR_ACCESS_DENIED``/``ERROR_SHARING_VIOLATION``)
-    when another process holds a handle to the destination — and this record
-    is read constantly, by ``conductor status``, ``fleet list``, the TUI's
-    ~2s poll, and the ``--web-bg`` launch gate. Without the retry the write
-    fails, ``cli/run.py`` swallows it, and the run silently becomes
-    undiscoverable and unstoppable: exactly the defect the run record exists
-    to prevent, reproduced only on Windows.
+    On Windows, a concurrent reader — ``conductor status``, ``fleet list``,
+    the TUI's ~2s poll, or the ``--web-bg`` launch gate — can make
+    ``os.replace``/``os.unlink``/``os.rename`` fail with ``PermissionError``
+    (``ERROR_ACCESS_DENIED``/``ERROR_SHARING_VIOLATION``): ``os.replace``
+    contends on its *destination* (the file being written), while
+    ``os.unlink``/``os.rename`` contend on their *source* (the file being
+    read). CPython opening files without ``FILE_SHARE_DELETE`` is one
+    contributor; antivirus and indexer handles routinely hold the same kind
+    of lock. POSIX ``rename``/``unlink`` are unaffected by a concurrent
+    reader and never fail this way.
 
-    The window is a single ``read_text`` on a small file, so a short bounded
-    retry closes it in practice. A genuine permission problem still surfaces:
-    the final attempt is allowed to raise.
+    ``op`` is called with no arguments and is expected to raise on failure
+    (never return a status code) -- callers close over whatever arguments
+    the real syscall needs.
+
+    ``FileNotFoundError`` is deliberately *not* retried: it means the target
+    is already gone, which is the common case and must not cost a stall on
+    every "already gone" call site.
+
+    On non-Windows platforms this is a plain passthrough -- ``op()`` runs
+    once, with no retry loop and no ``time.sleep`` overhead.
+
+    Args:
+        op: A zero-argument callable performing the filesystem operation.
+            Raises on failure; returns nothing meaningful on success.
+
+    Raises:
+        PermissionError: On Windows, if every attempt in the retry budget
+            raised it. On other platforms, if the single call to ``op``
+            raised it.
+        BaseException: Anything else ``op`` raises propagates unchanged and
+            unretried -- only ``PermissionError``, and only on Windows, is
+            retried. ``FileNotFoundError`` in particular surfaces
+            immediately.
     """
     if sys.platform != "win32":
-        os.replace(tmp_name, filepath)
+        op()
         return
 
-    for attempt in range(_REPLACE_RETRIES):
+    for attempt in range(_SHARING_VIOLATION_RETRIES):
         try:
-            os.replace(tmp_name, filepath)
+            op()
             return
+        # Deliberately NOT `except OSError`: `FileNotFoundError` must fall
+        # straight through unretried (see the docstring above).
         except PermissionError:
-            if attempt == _REPLACE_RETRIES - 1:
+            if attempt == _SHARING_VIOLATION_RETRIES - 1:
                 raise
-            time.sleep(_REPLACE_RETRY_DELAY_SECONDS)
+            time.sleep(_SHARING_VIOLATION_RETRY_DELAY_SECONDS)
+
+    # Unreachable when `_SHARING_VIOLATION_RETRIES >= 1`: the last loop
+    # iteration either returns (success) or raises (final failure). Guards
+    # against a mistuned (or test-patched) constant silently reporting
+    # success without ever calling `op` -- see the docstring's `Raises:`.
+    raise AssertionError(
+        f"_SHARING_VIOLATION_RETRIES must be >= 1, got {_SHARING_VIOLATION_RETRIES}"
+    )
+
+
+def _replace_with_retry(tmp_name: str, filepath: Path) -> None:
+    """``os.replace`` the temp file into place, retrying briefly on Windows.
+
+    Without the retry the write fails, ``cli/run.py`` swallows it, and the
+    run silently becomes undiscoverable and unstoppable: exactly the defect
+    the run record exists to prevent, reproduced only on Windows.
+
+    The window is a single ``read_text`` on a small file, so a short bounded
+    retry closes it in practice. A genuine permission problem still surfaces:
+    the final attempt is allowed to raise. See
+    :func:`_retry_on_windows_sharing_violation` for the mechanism, shared
+    with the removal paths (:func:`_safe_unlink`, :func:`_delete_if_unchanged`).
+    """
+    _retry_on_windows_sharing_violation(lambda: os.replace(tmp_name, filepath))
 
 
 def write_run_record(record: RunRecord) -> Path:
@@ -502,23 +552,37 @@ def write_run_record(record: RunRecord) -> Path:
 def _safe_unlink(f: Path) -> bool:
     """Best-effort delete of ``f``, never raising.
 
+    On Windows, a bounded retry (:func:`_retry_on_windows_sharing_violation`)
+    absorbs a transient sharing violation from a concurrent reader (e.g.
+    ``conductor status``, ``fleet list``, the TUI's ~2s poll) before giving
+    up. Uses ``os.unlink`` rather than ``Path.unlink()`` for symmetry with
+    the other two operations routed through the same helper (``os.replace``
+    on write, ``os.rename`` into quarantine); the two are otherwise
+    equivalent.
+
     Args:
         f: Path to delete.
 
     Returns:
         True if this call's ``unlink()`` actually removed the file. False if
         the file was already absent, or an ``OSError`` (permission denied,
-        read-only filesystem, etc.) prevented removal — the latter is logged
-        but never raised, since a bulk scan (:func:`read_run_records`) must
+        read-only filesystem, a Windows sharing violation that outlasted the
+        retry budget, etc.) prevented removal — the latter is logged but
+        never raised, since a bulk scan (:func:`read_run_records`) must
         never crash on one bad file, and a caller reporting deletion status
         must not claim success for a removal that didn't happen.
     """
     try:
-        f.unlink()
+        _retry_on_windows_sharing_violation(lambda: os.unlink(f))
     except FileNotFoundError:
         return False
-    except OSError:
-        logger.warning("Could not remove run record file: %s", f, exc_info=True)
+    except OSError as e:
+        logger.warning(
+            "Could not remove run record file %s (%s); it will be retried on the "
+            "next scan and may linger in `conductor status` / `fleet list`",
+            f,
+            e,
+        )
         return False
     return True
 
@@ -540,8 +604,9 @@ def _restore_if_absent(src: Path, dst: Path) -> None:
     place in the interim and the quarantine copy is no longer needed. If
     ``src`` itself is already gone (e.g. the caller's own earlier ``stat()``
     of it failed), this is a silent no-op -- there is nothing to restore.
-    Any other failure (permission denied, read-only filesystem, etc.) is
-    logged and ``src`` is left in place; a subsequent scan may retry.
+    Any other failure (permission denied, read-only filesystem, a Windows
+    sharing violation that outlasted the retry budget, etc.) is logged and
+    ``src`` is left in place; a subsequent scan may retry.
 
     Never raises.
 
@@ -550,7 +615,7 @@ def _restore_if_absent(src: Path, dst: Path) -> None:
         dst: The original path to restore it to, iff still absent.
     """
     try:
-        os.link(src, dst)
+        _retry_on_windows_sharing_violation(lambda: os.link(src, dst))
     except FileNotFoundError:
         # `src` no longer exists -- nothing to restore.
         return
@@ -560,12 +625,14 @@ def _restore_if_absent(src: Path, dst: Path) -> None:
         # it doesn't linger as an orphaned `.prune-*` artifact.
         _safe_unlink(src)
         return
-    except OSError:
+    except OSError as e:
         logger.warning(
-            "Could not restore quarantined run record to %s; leaving %s in place",
-            dst,
+            "Could not restore quarantined run record %s to %s (%s); it will be "
+            "retried on the next scan and may linger in `conductor status` / "
+            "`fleet list`",
             src,
-            exc_info=True,
+            dst,
+            e,
         )
         return
     # `src` and `dst` now both point at the same inode (two names for one
@@ -616,22 +683,34 @@ def _delete_if_unchanged(f: Path, stat_before: os.stat_result | None) -> bool:
 
     Returns:
         True if ``f`` was actually removed by this call. False if it was
-        already gone, a concurrent replacement was detected and restored
-        (or superseded by a still-newer replacement, in which case the
-        quarantined copy is simply discarded), or the final removal itself
-        failed (e.g. permission denied) — in the last two cases the
-        original content is put back at its original path (when nothing
-        newer has since taken its place) so it isn't silently lost as an
-        orphaned quarantine file.
+        already gone, a Windows sharing violation on the quarantine rename
+        outlasted the retry budget (see
+        :func:`_retry_on_windows_sharing_violation`), a concurrent
+        replacement was detected and restored (or superseded by a
+        still-newer replacement, in which case the quarantined copy is
+        simply discarded), or the final removal itself failed (e.g.
+        permission denied) — in the quarantine-restore cases the original
+        content is put back at its original path (when nothing newer has
+        since taken its place) so it isn't silently lost as an orphaned
+        quarantine file.
     """
     if stat_before is None:
         return False
 
     quarantine = f.with_name(f".{f.name}.prune-{uuid.uuid4().hex}")
     try:
-        os.rename(f, quarantine)
-    except OSError:
+        _retry_on_windows_sharing_violation(lambda: os.rename(f, quarantine))
+    except FileNotFoundError:
         return False  # Already gone -- nothing to prune.
+    except OSError as e:
+        logger.warning(
+            "Could not quarantine run record %s for deletion (%s); it will be "
+            "retried on the next scan and may linger in `conductor status` / "
+            "`fleet list`",
+            f,
+            e,
+        )
+        return False
 
     try:
         stat_now = quarantine.stat()

src/conductor/fleet/tui/actions.py

@@ -738,10 +738,23 @@ class DirectoryPickerModal(ModalScreen[Path | None]):
     binding that cancels. Two controls that stay in sync -- typing (or
     editing) a path in ``Input#dir-path``, pre-filled with the current
     launch directory and focused on mount, or browsing
-    ``DirectoryTree#dir-tree`` -- but exactly **one** way to accept: pressing
-    Enter in the input. The tree's highlighted node is mirrored into the
-    input rather than accepted directly, so the input stays the single
-    source of truth for what Enter will submit.
+    ``DirectoryTree#dir-tree``. There are two ways to accept: pressing
+    Enter in the input, or pressing Enter or clicking a node in the tree --
+    a single click on a node's label runs ``Tree.select_cursor``, which
+    Textual turns into a ``NodeSelected`` -> ``DirectoryTree
+    .DirectorySelected`` that this modal accepts directly (see
+    ``on_directory_tree_directory_selected``). The tree's *highlighted*
+    node (moving the cursor, distinct from selecting it) is separately
+    mirrored into the input, but only *while the tree has focus* -- so
+    browsing without selecting keeps the input showing what a subsequent
+    Enter-in-the-input would submit. Textual posts a ``NodeHighlighted``
+    for the tree's own root at *mount*, with no user interaction at all --
+    reactive initialisation runs ``Tree.watch_show_root``, which assigns
+    ``cursor_line = -1``; ``validate_cursor_line`` clamps it to ``0``;
+    ``watch_cursor_line`` then posts ``NodeHighlighted`` for line 0. This is
+    plain ``Tree`` behaviour, independent of ``DirectoryTree``'s directory
+    load; without the focus gate that automatic highlight overwrote the
+    prefilled launch directory with its *parent* (issue #486).
 
     A bad path -- one that does not exist, or names a file rather than a
     directory -- is rejected *in place*: a red message line appears and the
@@ -831,15 +844,44 @@ def on_mount(self) -> None:
     def on_tree_node_highlighted(self, event: Tree.NodeHighlighted[DirEntry]) -> None:
         """Mirror the highlighted tree node's path into the input.
 
-        The input remains the single source of truth for what Enter
-        accepts -- the tree is a browsing aid, not a second accept path.
+        Highlighting (moving the cursor) is distinct from selecting: a
+        single click, or Enter, on a tree node fires ``NodeSelected`` and
+        accepts that directory directly via
+        ``on_directory_tree_directory_selected`` -- that is a second accept
+        path, not merely a browsing aid. This handler only mirrors the
+        *highlighted* node so the input reflects what browsing (without
+        selecting) would submit if Enter were pressed in the input instead.
+
+        Gated on the tree actually having focus (issue #486): Textual posts
+        a ``NodeHighlighted`` for the tree's own root at *mount*, with no
+        user interaction at all -- reactive initialisation runs
+        ``Tree.watch_show_root``, which assigns ``cursor_line = -1``;
+        ``validate_cursor_line`` clamps it to ``0``; ``watch_cursor_line``
+        then posts ``NodeHighlighted`` for line 0. This is plain ``Tree``
+        behaviour, independent of ``DirectoryTree``'s directory load, and
+        the root already carries a ``DirEntry`` from construction -- so an
+        ungated mirror silently replaced the prefilled launch directory
+        with its *parent* before the user ever touched the tree. Focus is
+        the actual discriminator between "the tree loaded" and "the user
+        browsed it": the input is focused on mount, so the automatic
+        highlight is ignored, while both mouse clicks and keyboard
+        navigation into the tree focus it first (``Screen._forward_event``
+        focuses on ``MouseDown`` before delivery; ``Tree`` is
+        ``can_focus=True``), so genuine browsing still mirrors. Do not
+        narrow this to "ignore only the root node" -- that would also
+        ignore a genuine keyboard highlight of the root.
         """
+        if not event.node.tree.has_focus:
+            return
         data = event.node.data
         if data is not None:
             self.query_one("#dir-path", Input).value = str(data.path)
 
     def on_directory_tree_directory_selected(self, event: DirectoryTree.DirectorySelected) -> None:
-        """Double-click / Enter-on-the-tree accepts that directory directly."""
+        """A single click on a tree node's label, or Enter while the tree is
+        focused, accepts that directory directly (``Tree._on_click`` ->
+        ``select_cursor`` -> ``NodeSelected`` -> ``DirectoryTree
+        .DirectorySelected``)."""
         self._accept(str(event.path))
 
     def on_input_submitted(self, event: Input.Submitted) -> None: