Fix error reporting on Windows when an error is raised in the pager

CHANGES.md

@@ -74,6 +74,11 @@ Unreleased
   does not divide the total. Steps below that threshold are applied when the
   bar finishes, so `show_pos` renders `20/20` rather than the last multiple
   it reached. {issue}`3571` {pr}`3769`
+- An error raised while writing to the pager no longer gets replaced by
+  `PermissionError: [WinError 32]` on Windows. The temporary file backend
+  unlinked its file without closing it first, and Windows refuses to remove a
+  file the process still holds open, so the cleanup failure masked the real
+  exception. {issue}`3731` {pr}`3764`
 
 ## Version 8.4.2
 

src/click/_termui_impl.py

@@ -638,6 +638,11 @@ def _tempfilepager(
         f.close()
         subprocess.call([str(cmd_path), f.name])
     finally:
+        # An error raised while paging skips the close() above, and Windows
+        # refuses to unlink a file the process still holds open. Closing here
+        # keeps that PermissionError from replacing the original exception.
+        # close() is idempotent, so this is a no-op on the success path.
+        f.close()
         os.unlink(f.name)
 
 

tests/test_termui.py

@@ -1,6 +1,7 @@
 import contextlib
 import gc
 import io
+import os
 import platform
 import shlex
 import shutil
@@ -972,12 +973,17 @@ def _force_tempfile_pager(monkeypatch, pager_cmd="cat"):
 
     That backend is only reachable on Windows, so the platform flag and the tty
     probes are faked to exercise it from any runner.
+
+    ``PAGER`` is set to the bare command name, not to the path
+    :func:`shutil.which` resolves it to. ``pager()`` splits ``PAGER`` with
+    :func:`shlex.split` in POSIX mode, where a Windows path loses its
+    backslashes and splits on the space in ``C:\\Program Files``, leaving a
+    command that resolves to nothing. Click resolves the bare name itself.
     """
-    cmd_path = shutil.which(pager_cmd)
-    assert cmd_path is not None, f"{pager_cmd} not available"
+    assert shutil.which(pager_cmd) is not None, f"{pager_cmd} not available"
     monkeypatch.setattr(click._termui_impl, "isatty", lambda _: True)
     monkeypatch.setattr(click._termui_impl, "WIN", True)
-    monkeypatch.setitem(click._termui_impl.os.environ, "PAGER", cmd_path)
+    monkeypatch.setitem(click._termui_impl.os.environ, "PAGER", pager_cmd)
 
 
 def _page_with_get_pager_file():
@@ -1038,6 +1044,56 @@ def test_tempfile_pager_handles_ansi(monkeypatch, capfd, color, expected):
     assert out.replace("\r\n", "\n") == expected
 
 
+@pytest.mark.skipif(shutil.which("cat") is None, reason="cat not available")
+def test_tempfile_pager_closes_file_before_unlink(monkeypatch):
+    """An error while paging must not leave the temp file open at unlink time.
+
+    The ``f.close()`` on the success path sits after the ``yield``, so an
+    exception raised while the pager is open used to skip it and reach
+    ``finally: os.unlink(f.name)`` with the handle still open. Windows refuses
+    to unlink an open file, so ``PermissionError: [WinError 32]`` replaced the
+    original error and hid it (issue #3731).
+    """
+    _force_tempfile_pager(monkeypatch)
+
+    temp_files = []
+    real_named_temporary_file = tempfile.NamedTemporaryFile
+
+    def record(*args, **kwargs):
+        f = real_named_temporary_file(*args, **kwargs)
+        temp_files.append(f)
+        return f
+
+    monkeypatch.setattr(tempfile, "NamedTemporaryFile", record)
+
+    closed_at_unlink = []
+    real_unlink = os.unlink
+
+    def spy_unlink(path, **kwargs):
+        closed_at_unlink.append([f.closed for f in temp_files])
+        return real_unlink(path, **kwargs)
+
+    monkeypatch.setattr(os, "unlink", spy_unlink)
+
+    try:
+        with pytest.raises(RuntimeError, match="boom"):
+            with click.get_pager_file():
+                raise RuntimeError("boom")
+    finally:
+        # Should this invariant regress, the assertion below reports it. Close
+        # the handle anyway so the leak does not also resurface as a
+        # ResourceWarning charged to an unrelated test.
+        for f in temp_files:
+            if not f.closed:
+                f.close()
+
+    assert temp_files, "the temp file pager backend was not used"
+    assert closed_at_unlink == [[True]], (
+        "temp file still open when unlink ran; on Windows this raises "
+        "PermissionError [WinError 32] and masks the real error"
+    )
+
+
 def test_editor_unclosed_quote():
     """An unclosed quote in the editor command raises ValueError."""
     with pytest.raises(ValueError, match="No closing quotation"):