From f71e75bf36d22e4026149b50318449e896f4db13 Mon Sep 17 00:00:00 2001 From: Alexandru MAXIMCIUC Date: Sat, 17 Jan 2026 23:56:08 +0200 Subject: [PATCH 1/5] enhance file lock management in AppendOnlyFSBucket tests --- python/tests/test_append_only_fs_bucket.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/python/tests/test_append_only_fs_bucket.py b/python/tests/test_append_only_fs_bucket.py index 1d5df52..279febd 100644 --- a/python/tests/test_append_only_fs_bucket.py +++ b/python/tests/test_append_only_fs_bucket.py @@ -7,6 +7,7 @@ from bucketbase import MemoryBucket from bucketbase.fs_bucket import AppendOnlyFSBucket, FSBucket +from bucketbase.named_lock_manager import FileLockManager class TestAppendOnlyFSBucket(unittest.TestCase): @@ -44,7 +45,8 @@ def put_object(self2, name, content): # put object is expected to create a lock file before calling base_bucket.put_object, and remove it after bucket_in_test.put_object(object_name, content) - self.assertFalse(lock_file_path.exists()) + verifier_manager = FileLockManager(self.locks_path) + self.assertTrue(verifier_manager.get_lock(object_name).acquire(timeout=0.1), "Lock should have been released after put_object") self.assertEqual(base_bucket_put_calls, [(object_name, content)]) def test_put_object_twice_raises_exception(self): @@ -89,9 +91,12 @@ def test_lock_object_creates_lock_and_unlock_releases(self): # Check if the lock file was created lock_file_path = self.locks_path / (object_name.replace(bucket_in_test.SEP, FSBucket.TEMP_SEP) + ".lock") self.assertTrue(lock_file_path.exists()) + verifier_manager = FileLockManager(self.locks_path) + with self.assertRaises(TimeoutError): + verifier_manager.get_lock(object_name).acquire(timeout=0.1) bucket_in_test._unlock_object(object_name) - self.assertFalse(lock_file_path.exists()) + self.assertTrue(verifier_manager.get_lock(object_name).acquire(timeout=0.1), "Lock should have been released after _unlock_object") def test_unlocking_unlocked_object_raises_assertion(self): bucket_in_test = AppendOnlyFSBucket(self.base_bucket, self.locks_path) @@ -213,3 +218,7 @@ def thread2_func(): t2.join() self.assertListEqual(results, ["thread1_done", "thread2_file_exists"]) self.assertEqual(bucket_in_test.get_object(object_name), content1) + + +if __name__ == "__main__": + unittest.main() From 2df20c8b3051f66271aaf9a925c76c44ed4111cc Mon Sep 17 00:00:00 2001 From: Alexandru MAXIMCIUC Date: Sun, 18 Jan 2026 03:02:50 +0200 Subject: [PATCH 2/5] enhance FileLockManager tests to verify lock acquisition behavior with stale files --- python/tests/test_namedlock.py | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/python/tests/test_namedlock.py b/python/tests/test_namedlock.py index fe7725a..90913ca 100644 --- a/python/tests/test_namedlock.py +++ b/python/tests/test_namedlock.py @@ -71,16 +71,19 @@ def test_sanitizes_path_characters(self): lock1.acquire() lock2.acquire() - # Verify both lock files were created with sanitized names - files = list(self.temp_dir.glob("*")) - sanitized_names = [f.name for f in files] + verifier_manager = FileLockManager(self.temp_dir) + with self.assertRaises(TimeoutError): + verifier_manager.get_lock(name1).acquire(timeout=0.1) + with self.assertRaises(TimeoutError): + verifier_manager.get_lock(name2).acquire(timeout=0.1) - self.assertIn("path#with#slashes.lock", sanitized_names) - self.assertIn("path#with#slashes2.lock", sanitized_names) lock1.release() lock2.release() - self.assertEqual(0, len(list(self.temp_dir.glob("*")))) + self.assertTrue(verifier_manager.get_lock(name1).acquire(timeout=0.1)) + verifier_manager.get_lock(name1).release() + self.assertTrue(verifier_manager.get_lock(name2).acquire(timeout=0.1)) + verifier_manager.get_lock(name2).release() def test_lock_directory_created(self): # Delete the directory to test creation @@ -93,3 +96,18 @@ def test_lock_directory_created(self): self.assertTrue(self.temp_dir.exists()) self.assertTrue(self.temp_dir.is_dir()) + + def test_possibly_stale_lock_file_does_not_block_new_manager(self): + name = "some_lock" + lock1 = self.lock_manager.get_lock(name) + lock1.acquire() + lock1.release() + + new_manager = FileLockManager(self.temp_dir) + lock2 = new_manager.get_lock(name) + self.assertTrue(lock2.acquire(timeout=0.1)) + lock2.release() + + +if __name__ == "__main__": + unittest.main() From 2c8416c8130549b443f4c7f0e3517380b342e81a Mon Sep 17 00:00:00 2001 From: Alexandru MAXIMCIUC Date: Sun, 18 Jan 2026 03:19:43 +0200 Subject: [PATCH 3/5] use UUID for unique suffix in IBucketTester --- python/tests/bucket_tester.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/tests/bucket_tester.py b/python/tests/bucket_tester.py index 9e054d5..6cb6823 100644 --- a/python/tests/bucket_tester.py +++ b/python/tests/bucket_tester.py @@ -10,6 +10,7 @@ from queue import Queue from typing import BinaryIO from unittest import TestCase +import uuid import pyarrow as pa import pyarrow.parquet as pq @@ -87,7 +88,7 @@ def __init__(self, storage: IBucket, test_case: TestCase) -> None: self.storage = storage self.test_case = test_case # Next is a unique suffix to be used in the names of dirs and files, so they will be unique - self.us = f"{iTSms.now() % 100_000_000:08d}" + self.us = uuid.uuid4().hex def cleanup(self) -> None: self.storage.remove_prefix(f"dir{self.us}") From 3c4412834399318a8cfae18040a63e16992224d0 Mon Sep 17 00:00:00 2001 From: Alexandru MAXIMCIUC Date: Sun, 18 Jan 2026 12:47:26 +0200 Subject: [PATCH 4/5] refactor lock acquisition tests to use temporary variables for clarity and ensure proper lock release --- python/tests/test_append_only_fs_bucket.py | 8 ++++++-- python/tests/test_namedlock.py | 11 +++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/python/tests/test_append_only_fs_bucket.py b/python/tests/test_append_only_fs_bucket.py index 279febd..8130bd6 100644 --- a/python/tests/test_append_only_fs_bucket.py +++ b/python/tests/test_append_only_fs_bucket.py @@ -46,7 +46,9 @@ def put_object(self2, name, content): bucket_in_test.put_object(object_name, content) verifier_manager = FileLockManager(self.locks_path) - self.assertTrue(verifier_manager.get_lock(object_name).acquire(timeout=0.1), "Lock should have been released after put_object") + v_lock = verifier_manager.get_lock(object_name) + self.assertTrue(v_lock.acquire(timeout=0.1), "Lock should have been released after put_object") + v_lock.release() self.assertEqual(base_bucket_put_calls, [(object_name, content)]) def test_put_object_twice_raises_exception(self): @@ -96,7 +98,9 @@ def test_lock_object_creates_lock_and_unlock_releases(self): verifier_manager.get_lock(object_name).acquire(timeout=0.1) bucket_in_test._unlock_object(object_name) - self.assertTrue(verifier_manager.get_lock(object_name).acquire(timeout=0.1), "Lock should have been released after _unlock_object") + v_lock = verifier_manager.get_lock(object_name) + self.assertTrue(v_lock.acquire(timeout=0.1), "Lock should have been released after _unlock_object") + v_lock.release() def test_unlocking_unlocked_object_raises_assertion(self): bucket_in_test = AppendOnlyFSBucket(self.base_bucket, self.locks_path) diff --git a/python/tests/test_namedlock.py b/python/tests/test_namedlock.py index 90913ca..07ca58b 100644 --- a/python/tests/test_namedlock.py +++ b/python/tests/test_namedlock.py @@ -72,6 +72,7 @@ def test_sanitizes_path_characters(self): lock2.acquire() verifier_manager = FileLockManager(self.temp_dir) + # We expect TimeoutError because filelock.Timeout is a subclass of it. with self.assertRaises(TimeoutError): verifier_manager.get_lock(name1).acquire(timeout=0.1) with self.assertRaises(TimeoutError): @@ -80,10 +81,12 @@ def test_sanitizes_path_characters(self): lock1.release() lock2.release() - self.assertTrue(verifier_manager.get_lock(name1).acquire(timeout=0.1)) - verifier_manager.get_lock(name1).release() - self.assertTrue(verifier_manager.get_lock(name2).acquire(timeout=0.1)) - verifier_manager.get_lock(name2).release() + v_lock1 = verifier_manager.get_lock(name1) + self.assertTrue(v_lock1.acquire(timeout=0.1)) + v_lock1.release() + v_lock2 = verifier_manager.get_lock(name2) + self.assertTrue(v_lock2.acquire(timeout=0.1)) + v_lock2.release() def test_lock_directory_created(self): # Delete the directory to test creation From 78e5c7c4e19704a42cbab11fbb647568ddbc6621 Mon Sep 17 00:00:00 2001 From: Alexandru MAXIMCIUC Date: Sun, 18 Jan 2026 13:32:58 +0200 Subject: [PATCH 5/5] [copilot review] --- python/tests/test_append_only_fs_bucket.py | 23 +++++++++--------- python/tests/test_namedlock.py | 28 ++++++++++++---------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/python/tests/test_append_only_fs_bucket.py b/python/tests/test_append_only_fs_bucket.py index 8130bd6..fab1089 100644 --- a/python/tests/test_append_only_fs_bucket.py +++ b/python/tests/test_append_only_fs_bucket.py @@ -45,11 +45,14 @@ def put_object(self2, name, content): # put object is expected to create a lock file before calling base_bucket.put_object, and remove it after bucket_in_test.put_object(object_name, content) - verifier_manager = FileLockManager(self.locks_path) - v_lock = verifier_manager.get_lock(object_name) - self.assertTrue(v_lock.acquire(timeout=0.1), "Lock should have been released after put_object") - v_lock.release() - self.assertEqual(base_bucket_put_calls, [(object_name, content)]) + lock_verifier = FileLockManager(self.locks_path) + v_lock = lock_verifier.get_lock(object_name) + try: + acquired = v_lock.acquire(timeout=0.1) + self.assertTrue(acquired, "Lock should have been released after put_object") + self.assertEqual(base_bucket_put_calls, [(object_name, content)]) + finally: + v_lock.release() def test_put_object_twice_raises_exception(self): bucket_in_test = AppendOnlyFSBucket(self.base_bucket, self.locks_path) @@ -90,15 +93,13 @@ def test_lock_object_creates_lock_and_unlock_releases(self): # Attempt to lock the object bucket_in_test._lock_object(object_name) - # Check if the lock file was created - lock_file_path = self.locks_path / (object_name.replace(bucket_in_test.SEP, FSBucket.TEMP_SEP) + ".lock") - self.assertTrue(lock_file_path.exists()) - verifier_manager = FileLockManager(self.locks_path) + # Verify the lock is actually held by attempting acquisition with another manager + lock_verifier = FileLockManager(self.locks_path) with self.assertRaises(TimeoutError): - verifier_manager.get_lock(object_name).acquire(timeout=0.1) + lock_verifier.get_lock(object_name).acquire(timeout=0.1) bucket_in_test._unlock_object(object_name) - v_lock = verifier_manager.get_lock(object_name) + v_lock = lock_verifier.get_lock(object_name) self.assertTrue(v_lock.acquire(timeout=0.1), "Lock should have been released after _unlock_object") v_lock.release() diff --git a/python/tests/test_namedlock.py b/python/tests/test_namedlock.py index 07ca58b..95dd969 100644 --- a/python/tests/test_namedlock.py +++ b/python/tests/test_namedlock.py @@ -71,20 +71,22 @@ def test_sanitizes_path_characters(self): lock1.acquire() lock2.acquire() - verifier_manager = FileLockManager(self.temp_dir) - # We expect TimeoutError because filelock.Timeout is a subclass of it. - with self.assertRaises(TimeoutError): - verifier_manager.get_lock(name1).acquire(timeout=0.1) - with self.assertRaises(TimeoutError): - verifier_manager.get_lock(name2).acquire(timeout=0.1) - - lock1.release() - lock2.release() - - v_lock1 = verifier_manager.get_lock(name1) + lock_verifier = FileLockManager(self.temp_dir) + try: + # Attempting to acquire already-held locks should timeout, raising TimeoutError + # (filelock.Timeout is a subclass of TimeoutError). + with self.assertRaises(TimeoutError): + lock_verifier.get_lock(name1).acquire(timeout=0.1) + with self.assertRaises(TimeoutError): + lock_verifier.get_lock(name2).acquire(timeout=0.1) + finally: + lock1.release() + lock2.release() + + v_lock1 = lock_verifier.get_lock(name1) self.assertTrue(v_lock1.acquire(timeout=0.1)) v_lock1.release() - v_lock2 = verifier_manager.get_lock(name2) + v_lock2 = lock_verifier.get_lock(name2) self.assertTrue(v_lock2.acquire(timeout=0.1)) v_lock2.release() @@ -100,7 +102,7 @@ def test_lock_directory_created(self): self.assertTrue(self.temp_dir.exists()) self.assertTrue(self.temp_dir.is_dir()) - def test_possibly_stale_lock_file_does_not_block_new_manager(self): + def test_released_lock_can_be_reacquired_by_new_manager(self): name = "some_lock" lock1 = self.lock_manager.get_lock(name) lock1.acquire()