Skip to content

fix(test): handle Windows file locking in integration test teardown#294

Merged
danielmeppiel merged 1 commit intomainfrom
fix/windows-integration-teardown
Mar 14, 2026
Merged

fix(test): handle Windows file locking in integration test teardown#294
danielmeppiel merged 1 commit intomainfrom
fix/windows-integration-teardown

Conversation

@danielmeppiel
Copy link
Collaborator

Summary

Fixes the Windows integration test teardown failure that blocks the v0.7.9 release pipeline.

Root Cause

test_auto_install_e2e.py::teardown_method calls shutil.rmtree(self.test_dir) which fails on Windows with PermissionError: [WinError 32] — the recently-terminated apm subprocess still holds file locks on the temp directory.

All 4 tests PASS, but the teardown error causes pytest to exit non-zero, failing the entire integration test job and blocking the release.

Changes

tests/integration/test_auto_install_e2e.py:

  • Use shutil.rmtree(ignore_errors=True) in teardown_method — CI temp dirs are ephemeral
  • Close process.stdout pipes before process.wait() to release file handles on Windows
  • Increase wait timeout from 5s to 10s for graceful subprocess shutdown

tests/integration/test_ado_e2e.py:

  • Fix venv path fallback: .venv/Scripts/apm.exe on Windows vs .venv/bin/apm on Unix

Evidence

Same error reproduced consistently across multiple pipeline runs:

Notes

On Windows, recently-terminated subprocesses may still hold file locks
on temp directories (WinError 32). Fix test_auto_install_e2e.py by:
- Using shutil.rmtree(ignore_errors=True) in teardown_method
- Closing subprocess stdout pipes before waiting
- Increasing wait timeout for graceful shutdown

Also fix test_ado_e2e.py venv path fallback for Windows (.venv/Scripts
vs .venv/bin).

Ref #185

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 14, 2026 11:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the integration E2E test harness to be more reliable on Windows by reducing teardown failures caused by lingering subprocess file locks, and by fixing the Windows venv executable path fallback for ADO tests.

Changes:

  • Make test_auto_install_e2e.py teardown resilient to Windows WinError 32 by tolerating temp-dir deletion failures and adjusting subprocess shutdown handling.
  • Close subprocess stdout pipes and increase wait timeouts to improve odds of clean shutdown.
  • Fix Windows venv fallback path in test_ado_e2e.py to use .venv/Scripts/apm.exe instead of .venv/bin/apm.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/integration/test_auto_install_e2e.py Makes teardown/subprocess shutdown more Windows-tolerant to prevent non-zero pytest exits due to temp directory locks.
tests/integration/test_ado_e2e.py Updates local venv fallback to the correct Windows entrypoint path.
Comments suppressed due to low confidence (6)

tests/integration/test_auto_install_e2e.py:196

  • In this try/except, process.stdout.close() isn’t executed if an exception is raised before reaching it, and it also isn’t closed in the except path. To reliably release Windows file handles, close stdout in a finally that runs for both success and error paths.
            for line in iter(process.stdout.readline, ''):
                if not line:
                    break
                if "Package installed and ready to run" in line:
                    process.terminate()
                    break
            process.stdout.close()
            process.wait(timeout=10)
        except:
            process.kill()
            process.wait()

tests/integration/test_auto_install_e2e.py:230

  • Same cleanup issue here: if the read loop raises before process.stdout.close(), or if the except path is taken, stdout may remain open. Closing stdout in a finally (and then ensuring terminate/kill+wait happens) is more reliable for Windows file locking.
            for line in iter(process.stdout.readline, ''):
                if not line:
                    break
                output_lines.append(line)
                # Terminate once we see execution starting (no need for full run)
                if "Executing" in line or "Package installed and ready to run" in line:
                    process.terminate()
                    break
            process.stdout.close()
            process.wait(timeout=10)
        except:
            process.kill()
            process.wait()

tests/integration/test_auto_install_e2e.py:278

  • Same pattern: process.stdout.close() is skipped if an exception occurs before that line, and stdout is not closed in the except path. To prevent lingering Windows locks, close stdout in a finally that always runs.
            for line in iter(process.stdout.readline, ''):
                if not line:
                    break
                if "Package installed and ready to run" in line:
                    process.terminate()
                    break
            process.stdout.close()
            process.wait(timeout=10)
        except:
            process.kill()
            process.wait()

tests/integration/test_auto_install_e2e.py:308

  • Same issue here: if the output-reading loop errors out, stdout may never be closed, and the except path doesn’t close it either. Consider consolidating the terminate/wait/kill logic and stdout close into a single finally block so handles are always released on Windows.
            for line in iter(process.stdout.readline, ''):
                if not line:
                    break
                output_lines.append(line)
                # Terminate once we see execution starting
                if "Executing" in line or "Auto-discovered" in line:
                    process.terminate()
                    break
            process.stdout.close()
            process.wait(timeout=10)
        except:
            process.kill()
            process.wait()

tests/integration/test_auto_install_e2e.py:355

  • Same cleanup concern: if an exception is thrown before reaching process.stdout.close(), the handle stays open and can keep files locked on Windows; additionally the except path doesn’t close stdout. Moving the close into a finally that always executes will make the teardown fix more robust.
            for line in iter(process.stdout.readline, ''):
                if not line:
                    break
                # Terminate once installation completes
                if "Package installed and ready to run" in line:
                    process.terminate()
                    break
            process.stdout.close()
            process.wait(timeout=10)
        except:
            process.kill()
            process.wait()

tests/integration/test_ado_e2e.py:44

  • On Windows, apm_path (either from shutil.which or from the repo’s .venv) can contain spaces; building full_cmd = f"{apm_path} {cmd}" and running it with shell=True will break unless the executable path is quoted. Consider either quoting apm_path explicitly in full_cmd or (preferably) avoiding shell=True and passing an argv list to subprocess.run.
        if sys.platform == "win32":
            apm_path = Path(__file__).parent.parent.parent / ".venv" / "Scripts" / "apm.exe"
        else:
            apm_path = Path(__file__).parent.parent.parent / ".venv" / "bin" / "apm"
    
    full_cmd = f"{apm_path} {cmd}"
    result = subprocess.run(
        full_cmd,
        shell=True,
        cwd=cwd,

Comment on lines 131 to 137
# Wait for graceful shutdown
process.stdout.close()
try:
process.wait(timeout=5)
process.wait(timeout=10)
except subprocess.TimeoutExpired:
process.kill()
process.wait()
Comment on lines 71 to +75
if os.path.exists(self.test_dir):
shutil.rmtree(self.test_dir)
# ignore_errors=True: on Windows, recently-terminated subprocesses
# may still hold file locks on the temp directory (WinError 32).
# CI temp dirs are ephemeral — safe to leave behind if needed.
shutil.rmtree(self.test_dir, ignore_errors=True)
@danielmeppiel danielmeppiel merged commit 1886c36 into main Mar 14, 2026
17 checks passed
@danielmeppiel danielmeppiel deleted the fix/windows-integration-teardown branch March 14, 2026 11:21
danielmeppiel added a commit that referenced this pull request Mar 14, 2026
…e in finally (#295)

Move process.stdout.close() into finally blocks so pipe handles are
released even when exceptions occur during output reading.

Replace blanket ignore_errors=True with targeted PermissionError
handling: retry once after 1s on Windows (WinError 32), re-raise on
other platforms.

Ref #185

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants