Skip to content

fix: correct method names in hold_lock() context manager#2322

Closed
nightcityblade wants to merge 1 commit intotopoteretes:mainfrom
nightcityblade:fix/issue-2304
Closed

fix: correct method names in hold_lock() context manager#2322
nightcityblade wants to merge 1 commit intotopoteretes:mainfrom
nightcityblade:fix/issue-2304

Conversation

@nightcityblade
Copy link

@nightcityblade nightcityblade commented Mar 8, 2026

Description

Fix hold_lock() context manager in CacheDBInterface to call self.acquire_lock() and self.release_lock() instead of self.acquire() and self.release(). The abstract methods are defined as acquire_lock() and release_lock(), so the current code would raise AttributeError at runtime.

Fixes #2304

Acceptance Criteria

  • hold_lock() calls self.acquire_lock() and self.release_lock() correctly
  • ruff check passes

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Code refactoring
  • Other (please specify):

Screenshots

N/A

Pre-submission Checklist

  • I have tested my changes thoroughly before submitting this PR
  • This PR contains minimal changes necessary to address the issue/feature
  • My code follows the project's coding standards and style guidelines
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if applicable)
  • All new and existing tests pass
  • I have searched existing PRs to ensure this change hasn't been submitted already
  • I have linked any relevant issues in the description
  • My commits have clear and descriptive messages

DCO Affirmation

I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.

Summary by CodeRabbit

  • Refactor
    • Internal optimization to cache lock operations.

Fix hold_lock() to call self.acquire_lock() and self.release_lock()
instead of self.acquire() and self.release(), matching the abstract
method definitions in CacheDBInterface.

Fixes topoteretes#2304
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 8, 2026

Walkthrough

The hold_lock() context manager in the cache database interface has been updated to call the correct abstract method names. The method calls were renamed from acquire() and release() to acquire_lock() and release_lock() to align with the actual interface method definitions.

Changes

Cohort / File(s) Summary
Cache DB Interface Fix
cognee/infrastructure/databases/cache/cache_db_interface.py
Updated hold_lock() context manager to invoke acquire_lock() and release_lock() instead of acquire() and release(), correcting the method name mismatch with abstract method definitions.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: correcting method names in the hold_lock() context manager.
Description check ✅ Passed The PR description follows the template with all required sections completed: Description, Acceptance Criteria, Type of Change, and Pre-submission Checklist all properly filled out.
Linked Issues check ✅ Passed The PR addresses all coding requirements from issue #2304: correcting method names in hold_lock() and ensuring ruff check passes.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the method name mismatch in hold_lock(); no out-of-scope modifications are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cognee/infrastructure/databases/cache/cache_db_interface.py (1)

37-46: ⚠️ Potential issue | 🔴 Critical

RedisAdapter.hold_lock() still calls non-existent methods and must be fixed.

The interface fix is correct—CacheDBInterface.hold_lock() now properly calls self.acquire_lock() and self.release_lock(). However, RedisAdapter overrides hold_lock() (lines 187-195) and still calls self.acquire() and self.release(), which do not exist on the class. This will cause an AttributeError at runtime when using the Redis adapter.

Per issue #2304 objectives, implementations must be consistent with the corrected interface. Apply the same fix to RedisAdapter:

Fix for RedisAdapter.hold_lock()

In cognee/infrastructure/databases/cache/redis/RedisAdapter.py:

     `@contextmanager`
     def hold_lock(self):
         """
         Context manager for acquiring and releasing the Redis lock automatically. (Sync because of Kuzu)
         """
-        self.acquire()
+        self.acquire_lock()
         try:
             yield
         finally:
-            self.release()
+            self.release_lock()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cognee/infrastructure/databases/cache/cache_db_interface.py` around lines 37
- 46, RedisAdapter.hold_lock still calls non-existent methods acquire() and
release(); update the implementation to call the interface methods
acquire_lock() and release_lock() instead. Locate the RedisAdapter class and
modify its hold_lock() context manager to call self.acquire_lock() before
yielding and self.release_lock() in the finally block, matching
CacheDBInterface.hold_lock()’s behavior so the adapter implements the corrected
interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@cognee/infrastructure/databases/cache/cache_db_interface.py`:
- Around line 37-46: RedisAdapter.hold_lock still calls non-existent methods
acquire() and release(); update the implementation to call the interface methods
acquire_lock() and release_lock() instead. Locate the RedisAdapter class and
modify its hold_lock() context manager to call self.acquire_lock() before
yielding and self.release_lock() in the finally block, matching
CacheDBInterface.hold_lock()’s behavior so the adapter implements the corrected
interface.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fae323c2-1314-4fd2-9aa4-1218989db550

📥 Commits

Reviewing files that changed from the base of the PR and between c9370a8 and a0f8287.

📒 Files selected for processing (1)
  • cognee/infrastructure/databases/cache/cache_db_interface.py

@Vasilije1990 Vasilije1990 requested a review from hajdul88 March 12, 2026 10:34
@dexters1 dexters1 closed this Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix hold_lock() method name mismatch in CacheDBInterface

2 participants