fix: simplify profile data removal

src/ffpuppet/core.py

@@ -586,9 +586,9 @@ def close(self, force_close: bool = False) -> None:
             self._proc_tree = None
             self._logs.close()
             self._checks = []
-            assert self.profile
-            self.profile.remove(ignore_errors=force_close)
-            self.profile = None
+            if self.profile is not None:
+                self.profile.remove()
+                self.profile = None
         finally:
             LOG.debug("reason code: %s", r_code.name)
             self.reason = r_code

src/ffpuppet/helpers.py

@@ -6,13 +6,12 @@
 
 from contextlib import suppress
 from logging import getLogger
-from os import W_OK, access, chmod, environ
+from os import environ
 from pathlib import Path
 from platform import system
-from stat import S_IWUSR
 from subprocess import STDOUT, CalledProcessError, check_output
 from time import sleep, time
-from typing import TYPE_CHECKING, Any, Callable
+from typing import TYPE_CHECKING
 
 from psutil import Process, process_iter
 
@@ -33,7 +32,6 @@
     "certutil_available",
     "certutil_find",
     "files_in_use",
-    "onerror",
     "prepare_environment",
     "wait_on_files",
     "warn_open",
@@ -232,32 +230,6 @@ def files_in_use(files: Iterable[Path]) -> Generator[tuple[Path, int, str]]:
                                 yield open_file, proc.info["pid"], proc.info["name"]
 
 
-def onerror(
-    func: Callable[[str], Any], path: str, _exc_info: Any
-) -> None:  # pragma: no cover
-    """
-    Error handler for `shutil.rmtree`.
-
-    If the error is due to an access error (read only file)
-    it attempts to add write permission and then retries.
-
-    If the error is for another reason it re-raises the error.
-
-    Copyright Michael Foord 2004
-    Released subject to the BSD License
-    ref: http://www.voidspace.org.uk/python/recipebook.shtml#utils
-
-    Usage : `shutil.rmtree(path, onerror=onerror)`
-    """
-    if not access(path, W_OK):
-        # Is the error an access error?
-        chmod(path, S_IWUSR)
-        func(path)
-    else:
-        # this should only ever be called from an exception context
-        raise  # pylint: disable=misplaced-bare-raise
-
-
 def prepare_environment(
     target_path: Path,
     sanitizer_log: Path,

src/ffpuppet/profile.py

@@ -13,7 +13,7 @@
 from time import time
 from xml.etree import ElementTree
 
-from .helpers import certutil_available, certutil_find, onerror
+from .helpers import certutil_available, certutil_find
 
 LOG = getLogger(__name__)
 
@@ -53,8 +53,7 @@ def __init__(
                     self._install_cert(cert, certutil_find(browser_bin))
         except Exception:
             if self.path.exists():
-                # pylint: disable=deprecated-argument
-                rmtree(self.path, onerror=onerror)
+                rmtree(self.path, ignore_errors=True)
             raise
 
     def __enter__(self) -> Profile:
@@ -220,27 +219,18 @@ def invalid_prefs(self) -> Path | None:
             return self.path / "Invalidprefs.js"
         return None
 
-    def remove(self, ignore_errors: bool = True) -> None:
+    def remove(self) -> None:
         """Remove the profile from the filesystem.
 
         Args:
-            ignore_errors: Do not raise exception if error is encountered.
+            None
 
         Returns:
             None
         """
-        if self.path:
-            try:
-                # check if path exists to properly support "onerror"
-                if self.path.exists():
-                    LOG.debug("removing profile")
-                    # pylint: disable=deprecated-argument
-                    rmtree(self.path, onerror=onerror)
-            except OSError:
+        if self.path is not None:
+            LOG.debug("removing profile")
+            rmtree(self.path, ignore_errors=True)
+            if self.path.exists():
                 LOG.error("Failed to remove profile '%s'", self.path)
-                # skip raising here instead of passing ignore_errors to rmtree
-                # this way onerror is always called if there is an error
-                if not ignore_errors:
-                    raise
-            finally:
-                self.path = None
+            self.path = None

src/ffpuppet/puppet_logger.py

@@ -15,7 +15,7 @@
 from tempfile import NamedTemporaryFile, mkdtemp
 from typing import IO, TYPE_CHECKING
 
-from .helpers import onerror, warn_open
+from .helpers import warn_open
 
 if TYPE_CHECKING:
     from collections.abc import Iterator, KeysView
@@ -108,19 +108,12 @@ def clean_up(self, ignore_errors: bool = False) -> None:
         if not self.closed:
             self.close()
         if self.path is not None:
-            # try multiple attempts to remove data
-            for attempt in reversed(range(2)):
-                try:
-                    # check if path exists to properly support "onerror"
-                    if self.path.exists():
-                        # pylint: disable=deprecated-argument
-                        rmtree(self.path, ignore_errors=ignore_errors, onerror=onerror)
-                except OSError:
-                    if attempt == 0:
-                        warn_open(self.path)
-                        raise
-                    continue
-                break
+            try:
+                if self.path.exists():
+                    rmtree(self.path, ignore_errors=ignore_errors)
+            except OSError:
+                warn_open(self.path)
+                raise
         self._logs.clear()
         self.path = None
 

src/ffpuppet/test_ffpuppet.py

@@ -590,34 +590,6 @@ def _fake_create_log(src, filename, _timeout: int = 90):
 
 
 def test_ffpuppet_23(tmp_path):
-    """test rmtree error handler"""
-    # normal profile creation
-    # - just create a puppet, write a readonly file in its profile, then call close()
-    with FFPuppet() as ffp:
-        ffp.launch(TESTFF_BIN)
-        assert ffp.profile is not None
-        ro_file = ffp.profile.path / "read-only-test.txt"
-        ro_file.touch()
-        ro_file.chmod(S_IREAD)
-        ffp.close()
-        assert not ro_file.is_file()
-        ffp.clean_up()
-    # use template profile that contains a readonly file
-    profile = tmp_path / "profile"
-    profile.mkdir()
-    ro_file = profile / "read-only.txt"
-    ro_file.touch()
-    ro_file.chmod(S_IREAD)
-    with FFPuppet(use_profile=profile) as ffp:
-        ffp.launch(TESTFF_BIN)
-        assert ffp.profile is not None
-        prof_path = ffp.profile.path
-        assert prof_path.is_dir()
-        ffp.close()
-        assert not prof_path.is_dir()
-
-
-def test_ffpuppet_24(tmp_path):
     """test using a readonly prefs.js and extension"""
     prefs = tmp_path / "prefs.js"
     prefs.touch()
@@ -633,7 +605,7 @@ def test_ffpuppet_24(tmp_path):
         assert not prof_path.is_dir()
 
 
-def test_ffpuppet_25(mocker, tmp_path):
+def test_ffpuppet_24(mocker, tmp_path):
     """test _crashreports()"""
     mocker.patch(
         "ffpuppet.core.check_output", autospec=True, return_value=b"valgrind-99.0"
@@ -695,7 +667,7 @@ def close(self, force_close=False):
             assert not ffp._logs.watching
 
 
-def test_ffpuppet_26(mocker, tmp_path):
+def test_ffpuppet_25(mocker, tmp_path):
     """test build_launch_cmd()"""
     with FFPuppet() as ffp:
         cmd = ffp.build_launch_cmd("bin_path", ["test"])
@@ -743,7 +715,7 @@ def test_ffpuppet_26(mocker, tmp_path):
         assert cmd[0] == "valgrind"
 
 
-def test_ffpuppet_27():
+def test_ffpuppet_26():
     """test cpu_usage()"""
     with FFPuppet() as ffp:
         assert not any(ffp.cpu_usage())
@@ -756,7 +728,7 @@ def test_ffpuppet_27():
         assert ffp.wait(timeout=10)
 
 
-def test_ffpuppet_28(mocker):
+def test_ffpuppet_27(mocker):
     """test _dbg_sanity_check()"""
     fake_system = mocker.patch("ffpuppet.core.system", autospec=True)
     fake_chkout = mocker.patch("ffpuppet.core.check_output", autospec=True)
@@ -815,7 +787,7 @@ def test_ffpuppet_28(mocker):
         FFPuppet._dbg_sanity_check(Debugger.VALGRIND)
 
 
-def test_ffpuppet_29(mocker, tmp_path):
+def test_ffpuppet_28(mocker, tmp_path):
     """test FFPuppet.close() setting reason"""
 
     class StubbedProc(FFPuppet):
@@ -914,7 +886,7 @@ def launch(self):
         assert fake_wait_files.call_count == 0
 
 
-def test_ffpuppet_30():
+def test_ffpuppet_29():
     """test ignoring benign sanitizer logs"""
     with FFPuppet() as ffp:
         ffp.launch(TESTFF_BIN)
@@ -939,7 +911,7 @@ def test_ffpuppet_30():
         (False, FileNotFoundError),
     ],
 )
-def test_ffpuppet_31(mocker, tmp_path, bin_exists, expect_exc):
+def test_ffpuppet_30(mocker, tmp_path, bin_exists, expect_exc):
     """test Popen failure during launch"""
     bin_fake = tmp_path / "fake_bin"
     if bin_exists:
@@ -957,7 +929,7 @@ def test_ffpuppet_31(mocker, tmp_path, bin_exists, expect_exc):
 
 
 @mark.skipif(system() != "Windows", reason="Only supported on Windows")
-def test_ffpuppet_32(mocker):
+def test_ffpuppet_31(mocker):
     """test FFPuppet.launch() config_job_object code path"""
     mocker.patch("ffpuppet.core.ProcessTree", autospec=True)
     fake_bts = mocker.patch("ffpuppet.core.Bootstrapper", autospec=True)
@@ -977,7 +949,7 @@ def test_ffpuppet_32(mocker):
     assert resume_suspended.mock_calls[0] == mocker.call(789)
 
 
-def test_ffpuppet_33(mocker):
+def test_ffpuppet_32(mocker):
     """test FFPuppet.dump_coverage()"""
     with FFPuppet() as ffp:
         ffp._proc_tree = mocker.Mock(spec_set=ProcessTree)

src/ffpuppet/test_profile.py

@@ -200,15 +200,13 @@ def test_profile_05(tmp_path):
 
 
 def test_profile_06(mocker, tmp_path):
-    """test Profile.remove() failure"""
-    mocker.patch("ffpuppet.profile.rmtree", autospec=True, side_effect=OSError("test"))
+    """test Profile.remove() fail to remove data directory"""
+    mocker.patch("ffpuppet.profile.rmtree", autospec=True)
     with Profile(working_path=str(tmp_path)) as profile:
-        profile.remove(ignore_errors=True)
-    with (
-        Profile(working_path=str(tmp_path)) as profile,
-        raises(OSError, match="test"),
-    ):
-        profile.remove(ignore_errors=False)
+        path = profile.path
+        profile.remove()
+        assert profile.path is None
+        assert path.exists()
 
 
 def test_profile_07(mocker, tmp_path):

src/ffpuppet/test_puppet_logger.py

@@ -10,7 +10,6 @@
 
 from pytest import raises
 
-from .helpers import onerror
 from .puppet_logger import PuppetLogger
 
 
@@ -240,15 +239,15 @@ def test_puppet_logger_09(mocker, tmp_path):
         fake_rmtree.side_effect = OSError("test")
         with raises(OSError):
             plog.clean_up()
-        assert fake_rmtree.call_count == 2
-        fake_rmtree.assert_called_with(path, ignore_errors=False, onerror=onerror)
+        assert fake_rmtree.call_count == 1
+        fake_rmtree.assert_called_with(path, ignore_errors=False)
         assert plog.path is not None
         fake_rmtree.reset_mock()
         # test with ignore_errors=True
         fake_rmtree.side_effect = None
         plog.clean_up(ignore_errors=True)
         assert fake_rmtree.call_count == 1
-        fake_rmtree.assert_called_with(path, ignore_errors=True, onerror=onerror)
+        fake_rmtree.assert_called_with(path, ignore_errors=True)
         assert plog.path is None