From aa1db97aa40e1f6fad7ef3012c4d401b48738e6f Mon Sep 17 00:00:00 2001 From: Thomas Kpenou Date: Wed, 24 Jun 2026 16:06:50 -0400 Subject: [PATCH] test: fix flaky tests_temp teardown cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared _rmtree() helper failed intermittently in CI (StreamingFormReaderTest), with "IsADirectoryError: Is a directory: tests_temp". Two bugs: 1. The on-error handler always called os.remove(path). shutil.rmtree also invokes the handler for directories (when os.rmdir fails), where os.remove raises IsADirectoryError. Fixed by re-running the failed operation func(path) (os.unlink for files, os.rmdir for directories) — the documented pattern. 2. No tolerance for the underlying race: upload threads in some tests are still writing into the temp folder during teardown, so a file appears between rmtree's scandir and rmdir and leaves the directory non-empty. Added a retry loop (up to ~1s) that lets those threads finish before giving up. Adds regression tests covering a plain tree, a read-only file (exercises the handler), and a concurrent writer (reproduces the race). Co-Authored-By: Claude Opus 4.8 --- src/tests/test_utils.py | 46 ++++++++++------ src/tests/test_utils_rmtree_test.py | 84 +++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 15 deletions(-) create mode 100644 src/tests/test_utils_rmtree_test.py diff --git a/src/tests/test_utils.py b/src/tests/test_utils.py index 1568da5a..50673443 100644 --- a/src/tests/test_utils.py +++ b/src/tests/test_utils.py @@ -3,6 +3,7 @@ import shutil import stat import threading +import time import uuid from copy import copy from unittest.case import TestCase @@ -89,21 +90,36 @@ def cleanup(): def _rmtree(folder): - exception = None - - def on_rm_error(func, path, exc_info): - try: - os.chmod(path, stat.S_IWRITE | stat.S_IEXEC | stat.S_IREAD) - os.remove(path) - except Exception as e: - print('Failed to remove path ' + path + ': ' + str(e)) - nonlocal exception - if exception is None: - exception = e - - shutil.rmtree(folder, onerror=on_rm_error) - if exception: - raise exception + # Some tests upload files via background threads that may still be writing + # into the temp folder when teardown starts. A file appearing between + # rmtree's scandir and rmdir leaves the directory non-empty, so the removal + # fails transiently. Retry a few times to let those threads finish before + # giving up. + attempts = 20 + for attempt in range(attempts): + errors = [] + + def on_rm_error(func, path, exc_info): + try: + os.chmod(path, stat.S_IWRITE | stat.S_IEXEC | stat.S_IREAD) + # Re-run the operation that failed (os.unlink for files, + # os.rmdir for directories, ...). The previous code always + # called os.remove, which raised IsADirectoryError on dirs. + func(path) + except Exception as e: + errors.append(e) + + shutil.rmtree(folder, onerror=on_rm_error) + + if not os.path.exists(folder): + return + + if attempt < attempts - 1: + time.sleep(0.05) + + if errors: + raise errors[0] + raise OSError('Failed to remove ' + folder + ' after ' + str(attempts) + ' attempts') def set_linux(): diff --git a/src/tests/test_utils_rmtree_test.py b/src/tests/test_utils_rmtree_test.py new file mode 100644 index 00000000..03d07338 --- /dev/null +++ b/src/tests/test_utils_rmtree_test.py @@ -0,0 +1,84 @@ +import os +import stat +import threading +import time +import unittest + +from tests import test_utils + + +class RmtreeTest(unittest.TestCase): + """Regression tests for test_utils._rmtree, which used to fail in CI: + the on-error handler always called os.remove (raising IsADirectoryError on + directories), and there was no tolerance for files appearing concurrently + during teardown (upload threads still writing).""" + + def setUp(self): + self.base = os.path.join(test_utils.temp_folder, 'rmtree_test') + if os.path.exists(test_utils.temp_folder): + test_utils._rmtree(test_utils.temp_folder) + os.makedirs(self.base) + + def tearDown(self): + if os.path.exists(test_utils.temp_folder): + test_utils._rmtree(test_utils.temp_folder) + + def _write_file(self, path, content='x'): + with open(path, 'w') as f: + f.write(content) + + def test_removes_plain_tree(self): + os.makedirs(os.path.join(self.base, 'sub', 'deeper')) + self._write_file(os.path.join(self.base, 'a.txt')) + self._write_file(os.path.join(self.base, 'sub', 'b.txt')) + + test_utils._rmtree(self.base) + + self.assertFalse(os.path.exists(self.base)) + + def test_removes_read_only_file(self): + # A read-only file triggers on_rm_error; the handler must chmod and + # re-run the unlink (the original behaviour this helper was written for). + ro_file = os.path.join(self.base, 'readonly.txt') + self._write_file(ro_file) + os.chmod(ro_file, stat.S_IREAD) + + test_utils._rmtree(self.base) + + self.assertFalse(os.path.exists(self.base)) + + def test_tolerates_concurrent_writer(self): + # Reproduces the CI race: a background thread keeps creating files in the + # tree while _rmtree runs. With the buggy handler this raised + # IsADirectoryError; now the retry loop absorbs it and eventually wins + # once the writer stops (well within the ~1s retry budget). + stop = threading.Event() + + def writer(): + i = 0 + while not stop.is_set() and i < 8: + try: + os.makedirs(self.base, exist_ok=True) + self._write_file(os.path.join(self.base, 'race_%d.bin' % i)) + except OSError: + pass + i += 1 + time.sleep(0.02) + + thread = threading.Thread(target=writer) + thread.start() + try: + time.sleep(0.03) # let the writer get going before we start removing + test_utils._rmtree(self.base) + finally: + stop.set() + thread.join() + + # After the writer stopped, a final removal must leave nothing behind. + if os.path.exists(self.base): + test_utils._rmtree(self.base) + self.assertFalse(os.path.exists(self.base)) + + +if __name__ == '__main__': + unittest.main()