enhance file lock management in AppendOnlyFSBucket tests

[amaximciuc]

Title

Enhances file lock management testing with more robust lock acquisition verification.

Ticket / Task

somewhat relalted #164

What & Why

This PR enhances file lock management testing in AppendOnlyFSBucket by replacing file existence checks with more robust lock acquisition verification. Instead of simply checking if lock files exist or don't exist, the tests now verify that locks can actually be acquired (when they should be released) or cannot be acquired (when they should be held).

solves the CI issues we've been having.

Scope

• New feature
• Bug fix
• Refactor / tech-debt
• Test / tooling only

Checklist (self-review)

See Section 5.

Risk / Impact

• Latency critical path? ☐ Yes [x] No
• External API contract change? ☐ Yes [x] No
• Migration steps required? ☐ Yes [x] No

Screenshots / Logs / Benchmarks

(only if relevant)

Author Self-Review Checklist

• Single Ticket — PR addresses only one business requirement / Trello card.
• Minimal Diff — no unrelated refactors, commented-out code, or debug prints.
• Compiles & Tests Pass — pytest -q / mvn test clean locally.
• No Dead Code — every new unit is called or covered by tests.
• Naming & Clarity — identifiers are self-explanatory; no overloaded meanings.
• Docs Updated — README, wiki, or docstrings updated where behaviour changed.
• Performance Tagged — if touching hot path, attach micro-benchmarks or profiler diff.
• Config & Secrets — no plaintext credentials; configs externalised.
• Rollback Ready — change can be reverted with git revert without dependency hell.
• Checklist Acknowledged — I would merge this myself if I were the reviewer.
• LLM Review — PR has been reviewed by the approved LLM tool and suggestions addressed.
  • IDE Type Checker — PyCharm/Pylance recommendations were addressed where applicable.
  • Optional mypy — (Recommended) Ran python/scripts/run_mypy.bat locally with no critical errors

Resources:

• PR guidelines

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Tests were updated to verify lock lifecycles at runtime by importing FileLockManager and using short-timeout acquisitions to assert lock-held and lock-released states instead of relying on direct filesystem assertions. test_put_object_creates_lock and test_lock_object_creates_lock_and_unlock_releases now use acquisition/timeout checks and explicit release verification. Named-lock tests were refactored to use acquisition-based semantics and include a new test ensuring a released lock can be reacquired by a new manager. IBucketTester’s unique suffix generation switched from a time-based 8-digit format to uuid.uuid4().hex. An main entry point was added for running tests. No public API changes.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	PR description covers core requirements but has incomplete and unchecked items in the self-review checklist that undermine confidence.	Check the 'Single Ticket' and 'No Dead Code' boxes if true, add IDE Type Checker and mypy validation details, and clarify the CI issues being solved for full transparency.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly addresses the primary change: enhancing file lock management verification in AppendOnlyFSBucket tests by replacing filesystem assertions with FileLockManager-based synchronization checks.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR enhances file lock management testing in AppendOnlyFSBucket by replacing file existence checks with more robust lock acquisition verification. Instead of simply checking if lock files exist or don't exist, the tests now verify that locks can actually be acquired (when they should be released) or cannot be acquired (when they should be held).

Changes:

• Introduced FileLockManager import for test-side lock verification
• Replaced lock_file_path.exists() checks with actual lock acquisition attempts using timeout
• Added if __name__ == "__main__" block for direct test execution

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@python/tests/test_append_only_fs_bucket.py`:
- Around line 48-49: The test acquires the verifier lock via
verifier_manager.get_lock(object_name).acquire(timeout=0.1) but never releases
it, which can leave the file locked and break cleanup; change the test to obtain
the lock object with verifier_manager.get_lock(object_name), call
lock.acquire(timeout=0.1) and then always call lock.release() in a finally block
(or use a context manager if the lock type supports it) so the lock is released
regardless of assertions or exceptions.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@python/tests/test_namedlock.py`:
- Around line 74-78: The test asserts TimeoutError but filelock.FileLock.acquire
raises filelock.Timeout; update the test or library to match: either change the
test to expect filelock.Timeout when calling
verifier_manager.get_lock(name1).acquire(...) and
verifier_manager.get_lock(name2).acquire(...), or modify the lock wrapper (e.g.,
FileLockForPath.acquire or FileLockManager.get_lock) to catch filelock.Timeout
and re-raise a built-in TimeoutError so existing tests remain valid; reference
FileLockManager and the acquire implementation on the FileLock wrapper to locate
where to adjust behavior.

🧹 Nitpick comments (1)

python/tests/test_namedlock.py (1)

83-86: 0.1s timeout is razor‑thin — test could flake on slow CI.

Line 83-86 and Line 108 use timeout=0.1, which is risky under load. Consider bumping to a safer value or centralizing a test constant.

🔧 Possible tweak

+LOCK_TIMEOUT = 1.0
 ...
-        self.assertTrue(verifier_manager.get_lock(name1).acquire(timeout=0.1))
+        self.assertTrue(verifier_manager.get_lock(name1).acquire(timeout=LOCK_TIMEOUT))
         verifier_manager.get_lock(name1).release()
-        self.assertTrue(verifier_manager.get_lock(name2).acquire(timeout=0.1))
+        self.assertTrue(verifier_manager.get_lock(name2).acquire(timeout=LOCK_TIMEOUT))
         verifier_manager.get_lock(name2).release()
 ...
-        self.assertTrue(lock2.acquire(timeout=0.1))
+        self.assertTrue(lock2.acquire(timeout=LOCK_TIMEOUT))

Also applies to: 108-109

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[asuiu]

Great change! Definitely LGTM. Thanks!