Skip to content

test: rename-fallback integration test doesn't actually exercise the fallback #174

@jrob5756

Description

@jrob5756

Summary

tests/test_integration/test_install_scripts.py::test_upgrade_with_running_process_uses_rename_fallback is named for the rename-fallback path in install.ps1 but doesn't actually assert that path runs. It only checks returncode == 0 and version == "0.0.2", which both hold even when uv tool install succeeds on the first attempt without the fallback ever firing.

If Move-ConductorToolDirAside or Test-LockError in install.ps1 regress (or are removed), this test will still pass.

Why the fallback isn't triggered today

Discovered while verifying #171 on Windows with uv 0.9.14 + Python 3.13/3.14:

  • The venv python.exe in tools/conductor-cli/Scripts/ is a ~241 KB launcher.
  • sys.base_prefix points to ~/AppData/Roaming/uv/python/cpython-3.X-windows-x86_64-none/, where the real interpreter and python3xx.dll live.
  • So python -I -c "import conductor; time.sleep(120)" (what _spawn_long_running_conductor does) only keeps the launcher resident; nothing inside the venv's Scripts/ dir that uv tool install --force needs to delete is locked.
  • uv tool install --force succeeds on the first attempt, Test-LockError never sees a lock-error string, and the rename-fallback is dead code from this test's perspective.

The functional outcome (install succeeds while a venv-loaded process runs) is verified — but the fallback path itself isn't.

Repro

# Sandbox setup as in tests/test_integration/install_scripts_helpers.py
& $sandboxPython -I -c "import conductor; import time; time.sleep(120)"  # background
powershell -File install.ps1 -Source $newWheel -Force
# Result: install succeeds, "Install blocked by a file lock" never logged,
#         "Moved existing install to ...old-..." never logged.

Suggested fixes (pick one)

Option A — Make the lock real (preferred). Add a stronger locker in the test that holds a file handle inside Scripts/ with deny-delete share semantics:

# In _spawn_long_running_conductor or a sibling helper, instead of just running python:
import msvcrt
fh = open(sandbox.python_exe, "rb")
msvcrt.locking(fh.fileno(), msvcrt.LK_NBLCK, 1)

…or from PowerShell test fixtures:

[System.IO.File]::Open($sandbox.python_exe, 'Open', 'Read', 'None')

Then assert the install log contains both "Install blocked by a file lock" and "Moved existing install to", in addition to the existing return-code/version checks.

Option B — Rename to match reality. Rename to test_upgrade_succeeds_while_venv_process_running and update the docstring to reflect that it only verifies the success-with-running-process scenario, not the fallback. Add a separate unit test that monkeypatches Invoke-UvInstall to fail with "access is denied" on attempt 1 and succeed on attempt 2, and asserts the fallback log lines appear.

I'd recommend (A) — it's a real e2e and one extra Open() call.

Severity

Low. The fallback code is unchanged across recent PRs and the install script's other behavior is well-covered. This is purely a "if it breaks, we won't know" gap, not a current bug.

Context

Discovered during manual verification of #171 (fix/windows-upgrade-reliability).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions