Uh oh!
There was an error while loading. Please reload this page.
fix(fleet): harden Windows sharing-violation retries and TUI directory picker - #487
Merged
Merged
Conversation
…y picker Extract the Windows-only PermissionError retry loop out of _replace_with_retry into a shared _retry_on_windows_sharing_violation helper, and reuse it for record deletion (_safe_unlink) and quarantine rename (_delete_if_unchanged) so those paths also absorb a transient sharing violation from a concurrent reader instead of failing outright. FileNotFoundError is treated as "already gone" and never retried. Also fixes the Fleet TUI's directory picker so the tree's highlighted node only mirrors into the input while the tree has focus, and updates associated tests, docs, and changelog.
Blocking fixes: - records.py: `_restore_if_absent` now routes `os.link` through the bounded Windows sharing-violation retry helper, closing the gap where a failed restore could orphan a live run's record as an unswept `.prune-*` file (AGENTS.md updated to match). - tui/actions.py, CHANGELOG.md, docs/fleet.md, test comment: corrected the false claim that the automatic root `NodeHighlighted` is caused by the background directory load -- it fires at mount, from `Tree`'s reactive initialisation, independent of any directory I/O. - tui/actions.py, docs/fleet.md: corrected the false "Enter in the input is the only accept path" claims -- a single click (or Enter) on a tree node also accepts via `NodeSelected` -> `DirectorySelected`; docs now describe both accept paths. Recommendations applied: - Guard `_retry_on_windows_sharing_violation` against a mistuned zero-valued retry constant silently reporting success without calling `op`; annotated the constant `Final[int]`. - Corrected the false "os.unlink is patchable, Path.unlink isn't" rationale in `_safe_unlink`'s docstring. - Fixed the CHANGELOG entry describing the self-cleanup retry (it understated the two-call rename+unlink sequence and was missing a relative pronoun). - Dropped `exc_info=True` (which produced an unattributed traceback on stderr/status output with no installed handlers) from the three new warning logs, interpolating the exception and stating the consequence instead. - Fixed the `Raises:` section on `_retry_on_windows_sharing_violation` to stop contradicting itself, and added an inline comment marking the deliberate `except PermissionError` (not `OSError`) choice. - Reworded the FILE_SHARE_DELETE rationale to lead with the observable source/destination contention asymmetry rather than presenting it as the sole cause. Recommendations skipped (see review response for reasoning): new `caplog`-based tests for the warning paths, gating the retry off in the bulk-prune scan, the KeyboardInterrupt handling change in cli/run.py, the `_replace_with_retry` inline-vs-wrapper judgement call, and the test hardening for global os.unlink/os.rename fakes -- each is a reasonable follow-up but changes behavior or adds test/production surface beyond what the blocking findings required. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 24, 2026 14:56
`test_tree_highlight_while_focused_mirrors_into_the_input` failed on Windows CI on both runs of this PR while passing on Linux, at `assert tree.has_focus` right after `tree.focus()`. The race is in the test, not the modal. `DirectoryPickerModal.on_mount` focuses `#dir-path`, and `Widget.focus()` defers the real work to `App.call_later`, so "the screen is composed" and "the modal's mount-time focus has landed" are two different moments. The test focused the tree in between: its own `call_later` ran first, the modal's arrived afterwards, and the input won -- leaving the tree unfocused. A single `pilot.pause()` closed that window on Linux but not on Windows. `Pilot.pause()` returns once `textual._wait.wait_for_idle` judges the process idle by comparing `time.process_time()` against wall clock, and Windows reports process time in ~15.6ms ticks, so the comparison reads no CPU used and returns after its first sleep however much work is queued. Reproduced locally by stubbing `wait_for_idle` to a no-op: the same test fails with the same assertion, and `screen.focused` is still `None` after the push. `_push` now takes the pilot and waits (via the existing `wait_for` condition helper) until the modal is mounted *and* its input actually holds focus, so every test in the class starts from a settled modal; the now-redundant `pilot.pause()` after each push is dropped. The focus assertion itself becomes a `wait_for` too, since `has_focus` is a reactive the tree only sets once it processes the `Focus` message. Test-only change. All 11 picker tests pass both normally and under the stubbed-idle emulation of Windows' weaker pause. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_replace_with_retryinto a shared_retry_on_windows_sharing_violationhelper insrc/conductor/fleet/records.py, and reused it for_safe_unlink(record deletion) and_delete_if_unchanged(quarantine rename) so those paths also tolerate a transientPermissionErrorfrom a concurrent reader on Windows instead of failing outright.FileNotFoundErroris treated as "already gone" and is never retried.DirectoryPickerModalso the directory tree's highlighted node only mirrors into the input while the tree has focus.docs/fleet.md), andCHANGELOG.md.Closes#486
Test plan
uv run pytest tests/test_fleet/test_records.py tests/test_fleet/test_tui_actions.py tests/test_fleet/test_tui_runs.py tests/test_fleet/test_tui_theme.py