Implement client ownership split for store lifecycle management #241
Conversation
Stores now only manage the lifecycle of clients they create internally.
When a user provides a client to a store constructor, the store no longer
enters/exits the client's context manager or closes it. This enables:
- Clear ownership semantics ("you create it, you manage it")
- Prevention of double-context entry issues
- Client reuse across multiple stores
- Better alignment with Python resource management patterns
Changes:
- Add _client_provided_by_user attribute to BaseContextManagerStore
- Update __aexit__ and close() to conditionally call _close()
- Update all affected stores (Redis, MongoDB, DynamoDB, Valkey, Memcached,
Disk, DuckDB, RocksDB, Elasticsearch) to track client ownership
- Remove duplicate __aexit__ override in RocksDB store
- Update all docstrings to clarify lifecycle management expectations
- Generate corresponding sync package changes
This is a breaking change for users who relied on stores closing
user-provided clients, but improves correctness and enables important
use cases like connection pooling and client reuse.
Closes #232
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: William Easton <[email protected]>
📝 WalkthroughWalkthroughAdds a stable_api flag to BaseStore and introduces a client_provided_by_user lifecycle model. Context-manager stores use (Async)ExitStack to register and run cleanup only for resources the store created; subclasses compute/forward ownership and stop closing client resources they did not create. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Store
participant Exit as ExitStack
participant Client as UnderlyingClient
Caller->>Store: __init__(client=?)
alt caller provided client
Store->>Store: client_provided_by_user = true
Store->>Client: store keeps reference (no ownership)
else store creates client
Store->>Client: create client
Store->>Store: client_provided_by_user = false
end
Caller->>Store: async/sync enter / call APIs
alt store-owned client
Store->>Exit: _ensure_exit_stack_entered()
Exit->>Client: enter client context / register cleanup
else user-provided client
Note right of Store: Do NOT enter/exit/close underlying client
end
Caller->>Store: close / __aexit__ / __del__
Store->>Exit: if entered -> close / run callbacks (cleanup owned clients)
Store->>Store: skip closing user-provided client
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Test Failure AnalysisSummary: Two RocksDB tests are failing on Windows because the store is closing user-provided database instances, leaving lock files that prevent cleanup. Root Cause: The PR correctly added the
Error from logs: Suggested Solution: Update both File: Line 86-88, replace: @override
async def _close(self) -> None:
self._close_and_flush()With: @override
async def _close(self) -> None:
if not self._client_provided_by_user:
self._close_and_flush()Line 192-193, replace: def __del__(self) -> None:
self._close_and_flush()With: def __del__(self) -> None:
if not getattr(self, "_client_provided_by_user", False):
self._close_and_flush()File: Apply the same changes to the sync version (lines 84-85 and 185-186). Detailed AnalysisTest ScenarioThe failing test ( temp_dir = TemporaryDirectory()
db_path = Path(temp_dir.name) / "db_test_db"
db_path.mkdir(parents=True, exist_ok=True)
opts = Options()
opts.create_if_missing(True)
db = Rdict(str(db_path), options=opts)
store = RocksDBStore(db=db) # User provides db
store.put(collection="test", key="db_test", value={"test": "value"})
result = store.get(collection="test", key="db_test")
store.close() # Works correctly - base class checks flag
temp_dir.cleanup() # Fails - db was closed by __del__Execution Flow
Why Windows FailsOn Windows, file locks are mandatory and strictly enforced. When RocksDB closes a database, it may still hold file handles briefly, preventing deletion. On Unix systems, this is less of an issue due to different file locking semantics. Comparison with DuckDBStoreThe @override
async def _close(self) -> None:
"""Close the DuckDB connection."""
if not self._is_closed and not self._client_provided_by_user: # ✅ Checks flag
self._connection.close()
self._is_closed = True
def __del__(self) -> None:
"""Clean up the DuckDB connection on deletion."""
try:
if not self._is_closed and not self._client_provided_by_user and hasattr(self, "_connection"): # ✅ Checks flag
self._connection.close()
self._is_closed = True
except Exception: # noqa: S110
passRocksDBStore needs the same treatment. Related FilesTest File:
Async Store:
Sync Store:
Base Class:
|
|
@claude could we simplify this by using |
|
Claude finished @strawgate's task —— View job Analyzing simplification proposal
Answer: Yes, this simplification would work!Current Pattern: if client:
self._client = client
self._client_provided_by_user = True # Set explicitly
elif url:
self._client = create_client(url)
self._client_provided_by_user = False # Set explicitly
else:
self._client = create_default_client()
self._client_provided_by_user = False # Set explicitlyProposed Simplification: # In BaseContextManagerStore.__init__():
self._client_provided_by_user = False
# In each store's __init__():
if client:
self._client = client
self._client_provided_by_user = True # Only set when user provides client
elif url:
self._client = create_client(url)
# No need to set flag - already False from base class
else:
self._client = create_default_client()
# No need to set flag - already False from base classWhy This Works
Implementation DetailsAdd to class BaseContextManagerStore(BaseStore, ABC):
"""An abstract base class for context manager stores.
Stores that accept a client parameter should set `_client_provided_by_user = True` when
a client is provided by the user. This ensures the store does not manage the lifecycle
of user-provided clients (i.e., does not close them).
"""
_client_provided_by_user: bool
def __init__(self, **kwargs):
"""Initialize the context manager store with default client ownership."""
self._client_provided_by_user = False
super().__init__(**kwargs)Then in each store, remove all Benefits
RecommendationImplement this simplification. It's a clean refactoring that reduces duplication and makes the code more maintainable. The base class |
- Initialize _client_provided_by_user = False in BaseContextManagerStore.__init__() - Remove redundant = False assignments from all store implementations - Add ownership checks in _close() methods to prevent closing user-provided clients - Add ownership checks in __del__() methods where applicable (RocksDB, Disk) - Fixes test failures where stores were closing user-provided clients This simplifies the ownership pattern: stores only set the flag to True when users provide a client, relying on the base class default of False for internally-created clients. Co-authored-by: William Easton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
45-86: Critical: Initialize_client_provided_by_userflag in all code paths.When a
directoryis provided (instead ofdisk_cache), the_client_provided_by_userflag is never initialized. This will cause anAttributeErrorat runtime when_close()(line 124) or__del__()(line 128) attempts to access the undefined attribute.Apply this diff to initialize the flag to
Falseby default and set it toTrueonly when a user-provided client is detected:def __init__( self, *, disk_cache: Cache | None = None, directory: Path | str | None = None, max_size: int | None = None, default_collection: str | None = None, ) -> None: """Initialize the disk store. Args: disk_cache: An existing diskcache Cache instance to use. If provided, the store will not manage the cache's lifecycle (will not close it). The caller is responsible for managing the cache's lifecycle. directory: The directory to use for the disk store. max_size: The maximum size of the disk store. default_collection: The default collection to use if no collection is provided. """ + # Initialize to False; will be set to True if disk_cache is provided + self._client_provided_by_user = False + if disk_cache is not None and directory is not None: msg = "Provide only one of disk_cache or directory" raise ValueError(msg) if disk_cache is None and directory is None: msg = "Either disk_cache or directory must be provided" raise ValueError(msg) if disk_cache: self._cache = disk_cache self._client_provided_by_user = True elif directory: directory = Path(directory) directory.mkdir(parents=True, exist_ok=True) if max_size is not None and max_size > 0: self._cache = Cache(directory=directory, size_limit=max_size) else: self._cache = Cache(directory=directory, eviction_policy="none") self._stable_api = True super().__init__(default_collection=default_collection)Based on PR comments and learnings.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
1-3: Critical: Changes made to auto-generated file will be overwritten.This file is auto-generated from the original
store.pyfile. Any changes made here will be lost whenbuild_sync_library.pyruns again. Please ensure the corresponding changes are made to the original source file instead.
70-82: Critical: Missing initialization of_client_provided_by_userin else branch.When a client is provided,
_client_provided_by_useris correctly set toTrue(line 72). However, when no client is provided (else branch, lines 73-78), the attribute is not initialized. This will cause anAttributeErroron line 163 when_close()attempts to accessself._client_provided_by_user.Apply this diff to initialize the attribute in both branches:
else: # redis client accepts URL addresses: list[NodeAddress] = [NodeAddress(host=host, port=port)] credentials: ServerCredentials | None = ServerCredentials(password=password, username=username) if password else None self._client_config = GlideClientConfiguration(addresses=addresses, database_id=db, credentials=credentials) self._connected_client = None + self._client_provided_by_user = FalseNote: As per the PR objectives, this aligns with strawgate's suggestion to "initialize the boolean flag... to false... to avoid needing getattr checks."
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
68-79: Critical: Missing initialization of_client_provided_by_userwhen directory is provided.When
directoryis provided (line 71), the code creates a cache internally but never sets_client_provided_by_user = False. This will cause anAttributeErrorwhen_close()(line 127) or__del__()(line 131) attempts to access this attribute.Apply this diff to initialize the flag in the directory branch:
elif directory: directory = Path(directory) directory.mkdir(parents=True, exist_ok=True) if max_size is not None and max_size > 0: self._cache = Cache(directory=directory, size_limit=max_size) else: self._cache = Cache(directory=directory, eviction_policy="none") + + self._client_provided_by_user = Falsekey-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
207-213: Critical: Initialize_client_provided_by_usertoFalsewhen store creates the client.When the store creates its own client from a URL, the
_client_provided_by_userattribute is never set. This will cause anAttributeErrorat line 481 when_closetries to access it directly.Additionally, lines 211-213 are unreachable dead code since the condition is already validated at lines 200-202.
Apply this diff to fix both issues:
elif url: self._client = Elasticsearch( hosts=[url], api_key=api_key, http_compress=True, request_timeout=10, retry_on_timeout=True, max_retries=3 ) + self._client_provided_by_user = False - else: - msg = "Either elasticsearch_client or url must be provided" - raise ValueError(msg)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
232-241: Critical: Missing attribute initialization causes AttributeError.When a
urlis provided (line 235-238),self._client_provided_by_useris never set. The_close()method (line 555) directly accesses this attribute without usinggetattr, causing anAttributeErrorwhen the store created its own client.Apply this diff to initialize the attribute in all code paths:
if elasticsearch_client: self._client = elasticsearch_client self._client_provided_by_user = True elif url: self._client = AsyncElasticsearch( hosts=[url], api_key=api_key, http_compress=True, request_timeout=10, retry_on_timeout=True, max_retries=3 ) + self._client_provided_by_user = False else: msg = "Either elasticsearch_client or url must be provided" raise ValueError(msg)Alternatively, per the PR comments suggesting initialization at the class level, you could initialize the flag at the beginning of
__init__:def __init__( self, *, elasticsearch_client: AsyncElasticsearch | None = None, url: str | None = None, api_key: str | None = None, index_prefix: str, default_collection: str | None = None, key_sanitization_strategy: SanitizationStrategy | None = None, collection_sanitization_strategy: SanitizationStrategy | None = None, ) -> None: """Initialize the elasticsearch store. ... """ + self._client_provided_by_user = False + if elasticsearch_client is None and url is None: msg = "Either elasticsearch_client or url must be provided" raise ValueError(msg) if elasticsearch_client: self._client = elasticsearch_client self._client_provided_by_user = True elif url: self._client = AsyncElasticsearch( hosts=[url], api_key=api_key, http_compress=True, request_timeout=10, retry_on_timeout=True, max_retries=3 ) - else: - msg = "Either elasticsearch_client or url must be provided" - raise ValueError(msg)The second approach aligns with the PR comment suggestion and eliminates the redundant else-branch check.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
145-177: Critical: Initialize_client_provided_by_userflag toFalse.The flag is set to
Truewhen a client is provided (line 172) but is never initialized toFalsewhen the store creates its own client (viaurlor default). This will causeAttributeErrorwhen the flag is accessed in lines 191, 200, and 318.Apply this diff to initialize the flag:
def __init__( self, *, client: MongoClient[dict[str, Any]] | None = None, url: str | None = None, db_name: str | None = None, coll_name: str | None = None, default_collection: str | None = None, collection_sanitization_strategy: SanitizationStrategy | None = None, ) -> None: """Initialize the MongoDB store. Values are stored as native BSON dictionaries for better query support and performance. Args: client: The MongoDB client to use (mutually exclusive with url). If provided, the store will not manage the client's lifecycle (will not enter/exit its context manager or close it). The caller is responsible for managing the client's lifecycle. url: The url of the MongoDB cluster (mutually exclusive with client). db_name: The name of the MongoDB database. coll_name: The name of the MongoDB collection. default_collection: The default collection to use if no collection is provided. collection_sanitization_strategy: The sanitization strategy to use for collections. """ + self._client_provided_by_user = False if client: self._client = client self._client_provided_by_user = True elif url: self._client = MongoClient(url) else: # Defaults to localhost self._client = MongoClient()key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
70-83: Critical:AttributeErrorwhen client is created internally.The
_client_provided_by_userattribute is only set toTruewhen a client is provided (line 72), but is never initialized toFalsewhen the store creates the client internally (lines 73-83). This causes anAttributeErrorwhen_close()accessesself._client_provided_by_userat line 214.Apply this diff to initialize the flag at the start of
__init__:) -> None: """Initialize the Redis store. Args: client: An existing Redis client to use. If provided, the store will not manage the client's lifecycle (will not close it). The caller is responsible for managing the client's lifecycle. url: Redis URL (e.g., redis://localhost:6379/0). host: Redis host. Defaults to localhost. port: Redis port. Defaults to 6379. db: Redis database number. Defaults to 0. password: Redis password. Defaults to None. default_collection: The default collection to use if no collection is provided. """ + self._client_provided_by_user = False + if client: self._client = client self._client_provided_by_user = Truekey-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (2)
367-375: Add defensive check for_client_provided_by_userin cleanup.While the
hasattrcheck for_connectionis good defensive programming, the direct access to_client_provided_by_userat line 370 will raiseAttributeErrorif the attribute wasn't initialized. The broad exception suppression at line 373 will hide this error, making debugging difficult.Consider using
getattrwith a default value for defensive cleanup:def __del__(self) -> None: """Clean up the DuckDB connection on deletion.""" try: - if not self._is_closed and not self._client_provided_by_user and hasattr(self, "_connection"): + client_provided = getattr(self, "_client_provided_by_user", False) + if not self._is_closed and not client_provided and hasattr(self, "_connection"): self._connection.close() self._is_closed = True except Exception: # noqa: S110This ensures cleanup works correctly even if initialization failed partway through
__init__.
144-157: Critical:_client_provided_by_userflag is overwritten when user provides a connection.When a user provides a connection, the store sets
_client_provided_by_user = Trueat line 146. However, callingsuper().__init__()at line 169 causes the base class to unconditionally reset this flag toFalse(base.py line 439). This breaks the intended behavior where user-provided connections should not be closed by the store.Solution: Set the flag after calling
super().__init__():self._table_name = table_name self._stable_api = False + # Initialize base class first (which sets _client_provided_by_user = False) super().__init__(default_collection=default_collection, seed=seed) + + # Then override if user provided a connection + if connection is not None: + self._client_provided_by_user = TrueThen remove the premature assignment at line 146:
if connection is not None: self._connection = connection - self._client_provided_by_user = Truekey-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
124-137: Critical: Double aexit called on client.When the store creates the client (_client_provided_by_user is False), the client's
__aexit__is called twice:
- Line 134:
super().__aexit__()→BaseContextManagerStore.__aexit__()→self._close()→ line 266:self._client.__aexit__(None, None, None)- Line 137:
self._client.__aexit__(exc_type, exc_value, traceback)Calling
__aexit__twice on the same context manager is incorrect and may cause errors or undefined behavior. This could be the root cause of the Windows test failures mentioned in the PR objectives.Fix: Remove the client context management from
__aexit__since_closealready handles it:@override async def __aexit__( self, exc_type: type[BaseException] | None, exc_value: BaseException | None, traceback: TracebackType | None ) -> None: await super().__aexit__(exc_type, exc_value, traceback) - # Only exit the client's context manager if the store created it - if not self._client_provided_by_user and self._client: - await self._client.__aexit__(exc_type, exc_value, traceback)The base class
__aexit__will properly handle cleanup through_close().
♻️ Duplicate comments (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
317-325: Missing AttributeError guard and redundant parentheses in del.Two issues:
Redundant parentheses (already noted in previous review):
(not self._client_provided_by_user)should benot self._client_provided_by_userAttributeError risk (new): If
__init__fails before callingsuper().__init__(), the flag won't exist and line 320 will raiseAttributeError. The existinghasattr(self, "_connection")check doesn't cover_client_provided_by_user.Apply this diff:
try: - if not self._is_closed and (not self._client_provided_by_user) and hasattr(self, "_connection"): + if not self._is_closed and not getattr(self, "_client_provided_by_user", False) and hasattr(self, "_connection"): self._connection.close() self._is_closed = True
313-315: Remove redundant parentheses.This was already noted in a previous review.
Apply this diff:
- if not self._is_closed and (not self._client_provided_by_user): + if not self._is_closed and not self._client_provided_by_user:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
key-value/key-value-aio/src/key_value/aio/stores/base.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_client(95-101)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_close_and_flush(86-90)
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (16)
key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
setup(148-170)_close(459-465)close(453-456)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_close(126-128)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
_close(361-365)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_close(263-266)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
_close(554-556)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
_close(154-156)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_close(345-347)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
_close(222-224)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
_close(85-87)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_close(166-168)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_close(123-125)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
_close(480-482)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_close(317-319)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
_close(213-215)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_close(82-84)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_close(162-164)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
_close_and_flush(89-93)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(453-456)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(423-426)
key-value/key-value-aio/src/key_value/aio/stores/base.py (16)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_close(126-128)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
_close(361-365)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_close(263-266)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
_close(554-556)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
_close(154-156)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_close(345-347)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
_close(222-224)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
_close(85-87)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_close(166-168)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
_close(429-435)close(423-426)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_close(123-125)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
_close(480-482)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_close(317-319)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
_close(213-215)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_close(82-84)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_close(162-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
🔇 Additional comments (34)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (3)
123-125: Logic looks correct, assuming initialization fix is applied.The conditional close logic properly implements the ownership semantics: only closing the cache when the store created it internally. Once the
_client_provided_by_userflag is properly initialized in__init__(per the previous comment), this implementation will work correctly.Note: The direct attribute access (without
getattr) differs from the base class pattern shown in the relevant snippets, but this is acceptable per the PR comments stating "getattr is unnecessary if the attribute is defined on the class."
127-129: Finalizer logic is consistent with_close().The
__del__method correctly implements the same ownership semantics as_close(), ensuring the cache is only closed if the store created it. Once the initialization fix is applied, this will work correctly.Note: While the ownership logic is correct, be aware that
__del__finalizers can be called at interpreter shutdown or in multi-threaded scenarios, which may lead to issues if the cache is accessed concurrently. However, this pattern pre-exists this PR and is not introduced by the ownership changes.
56-58: Excellent documentation of ownership semantics.The docstring clearly communicates that providing a
disk_cachetransfers lifecycle management responsibility to the caller. This aligns perfectly with the PR objectives to establish clear ownership semantics.key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
163-164: Lifecycle management logic is correct.The
_close()implementation correctly closes the client only when the store created it internally (_client_provided_by_userisFalse). This aligns with the PR objectives for clear ownership semantics.The direct attribute access (without
getattr) is consistent with the PR objective to simplify by always initializing the flag, though this depends on fixing the missing initialization in the else branch (see previous comment).
57-69: Excellent documentation of lifecycle semantics.The docstring clearly communicates the new ownership model: when a client is provided, the caller retains lifecycle responsibility. This is essential for preventing the double-context entry issues and enabling client reuse across multiple stores, as outlined in the PR objectives.
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
53-55: Documentation clearly explains lifecycle ownership.The docstring update appropriately clarifies that when a disk_cache is provided, the caller retains lifecycle management responsibility.
127-128: Direct attribute access requires guaranteed initialization.The code accesses
_client_provided_by_userdirectly. Once the missing initialization in the directory branch (line 71) is fixed, this will work correctly. However, consider usinggetattr(self, "_client_provided_by_user", False)as defensive coding to handle edge cases where__init__might fail before setting the attribute.Note: The base class's
close()method (shown in relevant code snippets) usesgetattrfor this reason.key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (2)
190-192: LGTM! Clear documentation of lifecycle ownership.The docstring correctly explains that when a client is provided, the store will not manage its lifecycle and the caller retains responsibility.
204-206: LGTM! Correct ownership flag for user-provided clients.The flag is properly set when the user provides an Elasticsearch client, ensuring the store won't attempt to close it.
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
218-220: LGTM: Clear documentation of lifecycle ownership.The docstring update accurately documents the ownership semantics for user-provided clients.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
160-162: LGTM: Clear documentation of lifecycle ownership.The docstring accurately describes the new ownership semantics, making it clear that when a user provides a client, they remain responsible for its lifecycle management.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
60-62: LGTM: Clear documentation of lifecycle ownership.The docstring accurately describes that user-provided clients are not managed by the store and the caller retains responsibility for lifecycle management.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (4)
7-7: LGTM! Import cleanup aligns with removal of aexit.The removal of
Anyfrom imports is correct since it's no longer needed after removing the__aexit__override.
51-53: LGTM! Clear documentation of ownership semantics.The docstring clearly communicates that user-provided database instances are not managed by the store.
65-67: LGTM! Flag setting follows expected pattern.The flag is correctly set when a user-provided database is detected. The default
Falsevalue is initialized byBaseContextManagerStore.__init__called viasuper().__init__()at line 79.
83-84: LGTM! Lifecycle guard correctly implements ownership semantics.The
_closemethod properly checks the ownership flag before closing the database.key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (4)
4-4: LGTM! Import cleanup is consistent with sync version.
54-56: LGTM! Documentation matches sync version and clearly states ownership semantics.
68-70: LGTM! Flag initialization follows the expected pattern.
86-87: LGTM! Lifecycle guard in _close is correct.key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
400-412: LGTM! Clear initialization pattern established.The base class now properly initializes
_client_provided_by_usertoFalsein__init__, and the docstring provides clear guidance for subclass implementers.
429-434: LGTM! Clear contract documentation for subclasses.The docstring accurately describes when
_closewill be called, helping subclass implementers understand the lifecycle contract.key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (3)
95-96: LGTM! Clear lifecycle documentation.
135-137: LGTM! Lifecycle ownership clearly documented.
147-149: LGTM! Flag initialization is consistent with other stores.key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (2)
92-93: LGTM! Clear ownership documentation.The docstring updates clearly communicate the new lifecycle management semantics: when a user provides a connection, the store will not manage its lifecycle. This aligns well with the PR objectives.
Also applies to: 132-134
361-365: Lifecycle logic is correct but depends on proper initialization.The logic correctly closes the connection only when the store owns it. However, this depends on
_client_provided_by_userbeing properly initialized (see previous comment about line 144-157).key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
63-65: LGTM! Clear lifecycle ownership documentation.The docstring correctly documents the new ownership semantics: when a client is provided, the store does not manage its lifecycle.
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
162-164: LGTM - Clear lifecycle documentation.The docstring accurately describes the lifecycle management behavior for user-provided clients.
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
428-440: LGTM - Clear lifecycle semantics established.The base class properly initializes the
_client_provided_by_userflag and documents the lifecycle expectations.
460-464: LGTM - Clear documentation of _close semantics.key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
55-70: LGTM - Lifecycle documentation and flag management are correct.The docstring clearly describes lifecycle expectations, and the flag is properly set when a client is provided. This store correctly relies on
BaseContextManagerStorefor lifecycle management.key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
57-69: LGTM - Clean lifecycle management implementation.The docstring and flag initialization correctly implement the new ownership semantics.
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
93-107: LGTM - Documentation and flag initialization are correct.
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py
Outdated
Show resolved
Hide resolved
Test Failure AnalysisSummary: Two tests are failing due to pytest worker crashes on Windows: Root Cause: Both tests caused pytest-xdist workers to crash with "node down: Not properly terminated" errors. This appears to be an infrastructure/flakiness issue rather than a logic error introduced by the PR changes. The error logs show: Evidence this is unrelated to the PR:
Suggested Solution: Option 1 (Recommended): Re-run the workflow to see if these are transient failures
Option 2: If failures persist, investigate the specific tests:
Both involve concurrent operations which may have timing-sensitive code. Detailed AnalysisTest Results Summary
Failed Tests1. test_passthrough_cache.py::test_not_unbounded
2. test_filetree.py::test_concurrent_operations
Worker Crash PatternThe pytest-xdist scheduler then encountered an internal error trying to reschedule work: This suggests the worker crash recovery mechanism in pytest-xdist hit an edge case. Why This is Likely Unrelated to PR ChangesYour PR changes focus on:
These changes affect:
The failing tests:
Neither test is directly exercising the code paths modified by your PR. Comparison with Previous RunsThe original failing tests (RocksDB ownership tests) are now passing, which confirms your PR fixes are working correctly. These new failures appeared in a subsequent run, suggesting they may be:
Related FilesTest Files:
Workflow Run: https://github.com/strawgate/py-key-value/actions/runs/19280534141 |
|
@claude can you review the PR feedback for cleanup opportunities? |
|
Claude finished @strawgate's task —— View job Review of CodeRabbit's PR Feedback
SummaryI've reviewed CodeRabbit's feedback and analyzed the current code state. Most of CodeRabbit's suggestions are now outdated or incorrect based on the changes that have already been implemented. Here's what I found: ✅ Already Fixed IssuesThe following critical issues flagged by CodeRabbit have already been addressed in the current code:
All stores now properly check 🔍 Valid Cleanup Opportunities1. Remove Redundant
|
|
@claude instead of setting a magic attribute for this and store stability, should we just pass them as arguments to the constructor? That should clean this up a bit |
|
Claude finished @strawgate's task —— View job Analyzing architecture suggestion
Analysis: Constructor Arguments vs Magic AttributesCurrent ImplementationThe PR currently uses "magic" instance attributes that are set conditionally in subclass
Example from RedisStore: def __init__(self, *, client=None, url=None, ...):
if client:
self._client = client
self._client_provided_by_user = True # Magic attribute
else:
self._client = Redis(...) # Store creates it
self._stable_api = True # Another magic attribute
super().__init__(...)Proposed Change: Constructor ArgumentsPass these as explicit arguments to the base class constructor: class BaseStore:
def __init__(self, *, client_provided_by_user: bool = False, stable_api: bool = False, **kwargs):
self._client_provided_by_user = client_provided_by_user
self._stable_api = stable_api
super().__init__(**kwargs)
class RedisStore(BaseContextManagerStore):
def __init__(self, *, client=None, ...):
if client:
self._client = client
client_provided = True
else:
self._client = Redis(...)
client_provided = False
super().__init__(
client_provided_by_user=client_provided,
stable_api=True,
...
)Recommendation: Yes, use constructor arguments ✅This change would be a significant improvement for several reasons: Pros:
Cons:
Implementation Impact:Minimal - only ~10 stores need updating:
Pattern is straightforward: # Before
if client:
self._client = client
self._client_provided_by_user = True
super().__init__(...)
# After
client_provided = client is not None
if client:
self._client = client
else:
self._client = create_client()
super().__init__(client_provided_by_user=client_provided, stable_api=True, ...)Suggested Next StepsIf you'd like to proceed with this refactoring:
This would make the code cleaner and more maintainable going forward. Would you like me to implement this change? |
|
@claude please proceed |
|
Claude finished @strawgate's task —— View job Refactoring completeSuccessfully refactored Tasks:
Changes MadeBase Classes:
Store Implementations Updated:
Benefits:
Example Before: if client:
self._client = client
self._client_provided_by_user = True # Set before super().__init__()
super().__init__() # Base class resets it to False! ❌Example After: client_provided = client is not None
if client:
self._client = client
else:
self._client = create_client()
super().__init__(client_provided_by_user=client_provided) # ✅| |
…arguments Changes: - Updated BaseStore to accept stable_api parameter (default: False) - Updated BaseContextManagerStore to accept client_provided_by_user parameter (default: False) - Removed magic attribute setting in favor of explicit constructor arguments - Updated all store implementations to pass these flags to base class constructors - Removed redundant getattr calls in base class methods since attributes are always initialized - Fixes initialization order bug where base class __init__ was resetting flags This eliminates the bug where stores would set _client_provided_by_user = True, then call super().__init__() which would reset it back to False, causing user-provided clients to be incorrectly closed during cleanup. Stores updated: - Redis, DynamoDB, MongoDB, Elasticsearch, Memcached - Valkey, RocksDB, Disk, DuckDB, Memory, MultiDiskStore Co-authored-by: William Easton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (17)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
317-325: Redundant parentheses and check in del.Two issues:
- Redundant parentheses around
not self._client_provided_by_user- The
_client_provided_by_usercheck is redundant in__del__context when the connection exists- Consider using
getattrfor_client_provided_by_userlike you do withhasattrfor_connectionto handle early initialization failuresApply this diff for better safety:
def __del__(self) -> None: """Clean up the DuckDB connection on deletion.""" try: - if not self._is_closed and (not self._client_provided_by_user) and hasattr(self, "_connection"): + if not self._is_closed and not getattr(self, "_client_provided_by_user", False) and hasattr(self, "_connection"): self._connection.close() self._is_closed = True except Exception: # noqa: S110 # Suppress errors during cleanup to avoid issues during interpreter shutdown pass
311-315: Redundant check and parentheses in _close.Two issues:
- The
_client_provided_by_usercheck is redundant since the base class already guards calls to_close()(see base.py lines 431-434)- Redundant parentheses around the condition
Apply this diff to simplify:
- if not self._is_closed and (not self._client_provided_by_user): + if not self._is_closed: self._connection.close() self._is_closed = Truekey-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
162-163: Redundant flag check in _close.The
_client_provided_by_usercheck is redundant since the base class already verifies this before calling_close()(see base.py lines 431-434). Only the null check for_connected_clientis necessary.Apply this diff to simplify:
- if self._connected_client is not None and (not self._client_provided_by_user): + if self._connected_client is not None: self._client.close()key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
227-228: Redundant flag check in _close.The
_client_provided_by_usercheck is redundant since the base class already guards calls to_close()(see base.py lines 461-464).Apply this diff to simplify:
- if not self._client_provided_by_user: - await self._client.aclose() + await self._client.aclose()key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (2)
203-208: Critical: Potential double-cleanup of client resources.When the store creates the client:
- Line 205:
super().__aexit__()eventually calls_close()(base.py line 461-464)- Which calls
self._client.close()(line 349)- Then line 208 calls
self._client.__aexit__()This results in both
close()and__aexit__()being called on the same client, which could cause failures if__aexit__()is invoked afterclose().Remove the client lifecycle management from
__aexit__:@override async def __aexit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None: await super().__aexit__(exc_type, exc_val, exc_tb) - # Only exit the client's context manager if the store created it - if not self._client_provided_by_user: - await self._client.__aexit__(exc_type, exc_val, exc_tb)And update
_closeto call__aexit__for symmetry with__aenter__:@override async def _close(self) -> None: - if not self._client_provided_by_user: - await self._client.close() + await self._client.__aexit__(None, None, None)
347-349: Redundant flag check in _close.The
_client_provided_by_usercheck is redundant because the base class already guards calls to_close()(see base.py lines 461-464).Apply this diff (assuming the double-cleanup fix is applied):
@override async def _close(self) -> None: - if not self._client_provided_by_user: - await self._client.close() + await self._client.__aexit__(None, None, None)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
184-186: Missing AttributeError guard in del.If
__init__fails beforesuper().__init__()is called (e.g., during path validation), the_client_provided_by_userattribute won't exist, causingAttributeErrorduring object finalization.Apply this diff to add a safe attribute check:
def __del__(self) -> None: - if not self._client_provided_by_user: + if not getattr(self, "_client_provided_by_user", False): self._close_and_flush()key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
170-171: Optional: Redundant flag check in _close.The
_client_provided_by_usercheck is redundant sinceBaseContextManagerStorealready checks this flag in bothclose()and__aexit__()before calling_close(). The check forself._connected_client is not Noneshould remain.If you prefer to remove the redundancy:
@override async def _close(self) -> None: - if self._connected_client is not None and not self._client_provided_by_user: + if self._connected_client is not None: await self._client.close()Based on PR objectives feedback.
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
196-198: Add defensive guard in del to prevent AttributeError.If
__init__fails beforesuper().__init__()is called (line 83), the_client_provided_by_userattribute won't exist, causing__del__to raiseAttributeErrorduring cleanup.Apply this diff to add a defensive check:
def __del__(self) -> None: - if not self._client_provided_by_user: + if not getattr(self, "_client_provided_by_user", False): self._close_and_flush()Alternatively, wrap the entire
__del__body in a try-except block like the DuckDB store does.Based on past review feedback.
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
269-270: Optional: Redundant flag check in _close.The
_client_provided_by_usercheck is redundant since the base class already guards the call to_close(). Theself._clientcheck should remain.After verifying the pattern is correct:
@override async def _close(self) -> None: """Close the DynamoDB client.""" - if self._client and not self._client_provided_by_user: + if self._client: await self._client.__aexit__(None, None, None)Based on past review feedback.
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
157-158: Optional: Redundant flag check in _close.The
_client_provided_by_usercheck is redundant sinceBaseContextManagerStorealready checks this flag before calling_close(). The base class docstring explicitly states: "This method is only called if the store created the client itself."Following the template method pattern:
@override async def _close(self) -> None: - if not self._client_provided_by_user: - await self._client.close() + await self._client.close()Based on past review feedback.
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
557-558: Optional: Remove redundant ownership check in _close.The base class
close()and__aexit__()methods already check_client_provided_by_userbefore calling_close(). The base class docstring states: "This method is only called if the store created the client itself."Following the template method pattern:
@override async def _close(self) -> None: - if not self._client_provided_by_user: - await self._client.close() + await self._client.close()Based on past review feedback.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
322-324: Remove redundant ownership check in_close().The base class
close()method already checks_client_provided_by_userand only calls_close()when the store owns the client (see lines 431-434 in base.py). The conditional check here duplicates that logic.Apply this diff to simplify
_close():@override def _close(self) -> None: - if not self._client_provided_by_user: - self._client.close() + self._client.close()Based on learnings
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
482-484: Remove redundant ownership check in_close().The base class
close()method already checks_client_provided_by_userand only calls_close()when the store owns the client (see lines 431-434 in base.py). Re-checking the flag here duplicates that logic.Apply this diff to simplify
_close():@override def _close(self) -> None: - if not self._client_provided_by_user: - self._client.close() + self._client.close()Based on learnings
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
129-131: Remove redundant ownership check in_close().The base class
close()method already checks_client_provided_by_userand only calls_close()when the store owns the client (see lines 461-464 in base.py). The conditional check here duplicates that logic.Apply this diff to simplify
_close():@override async def _close(self) -> None: - if not self._client_provided_by_user: - self._cache.close() + self._cache.close()Based on learnings
133-135: Use defensive attribute access in__del__().If
__init__fails before callingsuper().__init__()(e.g., validation errors at lines 60-66),_client_provided_by_userwon't be set. Accessing it directly in__del__will raiseAttributeError, suppressing the original exception and making debugging difficult.Apply this diff to use safe attribute access:
def __del__(self) -> None: - if not self._client_provided_by_user: - self._cache.close() + if not getattr(self, "_client_provided_by_user", False): + if hasattr(self, "_cache"): + self._cache.close()Based on learnings
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
213-215: Remove redundant ownership check in_close().The base class
close()method already checks_client_provided_by_userand only calls_close()when the store owns the client (see lines 431-434 in base.py). The conditional check here duplicates that logic.Apply this diff to simplify
_close():@override def _close(self) -> None: - if not self._client_provided_by_user: - self._client.close() + self._client.close()Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
key-value/key-value-aio/src/key_value/aio/stores/base.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (5)
key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
setup(150-172)_close(468-474)close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_close(122-124)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_close(322-324)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
_close(213-215)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_close(161-163)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (6)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
_close(438-444)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_close(122-124)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
_close(482-484)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_close(322-324)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
_close(213-215)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_close(161-163)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
_close(468-474)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (2)
_close(83-85)_close_and_flush(87-91)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (4)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-aio/src/key_value/aio/stores/base.py (9)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_close(129-131)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
_close(366-370)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_close(267-270)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
_close(556-558)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
_close(156-158)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_close(347-349)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
_close(226-228)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
_close(89-91)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_close(169-171)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
🔇 Additional comments (45)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (3)
57-69: LGTM: Clear lifecycle documentation.The docstring clearly documents the lifecycle ownership semantics when a client is provided by the user.
70-70: LGTM: Correct client ownership tracking.The flag correctly captures whether the client was provided by the user.
81-81: LGTM: Correct base class initialization.The parameters are correctly passed to the base class constructor.
key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (3)
57-59: LGTM: Clear lifecycle documentation.The docstring clearly communicates lifecycle ownership semantics.
67-67: LGTM: Correct ownership tracking.The flag is correctly derived from the client parameter.
91-95: LGTM: Correct base initialization.The parameters are properly passed to the base class.
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (4)
162-164: LGTM: Clear lifecycle documentation.The docstring properly documents the lifecycle ownership behavior.
172-172: LGTM: Correct ownership tracking.The flag is correctly set based on whether a client is provided.
189-193: LGTM: Correct base initialization.The parameters are properly passed to the base class.
196-201: LGTM: Correct context manager entry.The store correctly guards entry into the client's context manager based on ownership.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (5)
94-94: LGTM: Stable API flag addition.The
stable_apiparameter enables stores to signal API stability status.
107-109: LGTM: Clear documentation.The docstring properly explains the stable API behavior.
125-128: LGTM: Correct stability warning implementation.The warning is appropriately issued for unstable APIs.
401-444: LGTM: Well-designed lifecycle management.The base class correctly implements lifecycle ownership semantics:
- The
_client_provided_by_userflag is properly initialized and used__exit__andclose()appropriately guard_close()calls- Documentation clearly explains the ownership model
This design enables clean separation of lifecycle responsibility between the store and its callers.
438-443: LGTM: Clear documentation.The docstring accurately describes when
_close()is called.key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
115-115: LGTM: Correct base initialization.The MemoryStore appropriately marks itself as having a stable API.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (4)
7-7: LGTM: Correct import cleanup.Removed unused
Anyimport.
51-53: LGTM: Clear lifecycle documentation.The docstring properly documents lifecycle ownership.
65-65: LGTM: Correct ownership tracking.The flag correctly tracks whether the database was provided.
80-80: LGTM: Correct base initialization.The parameters are properly passed to the base class.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (3)
56-58: LGTM: Clear lifecycle documentation.The docstring properly documents lifecycle ownership semantics.
71-71: LGTM: Correct ownership tracking.The flag correctly tracks whether the cache was provided.
85-85: LGTM: Correct base initialization.The parameters are properly passed to the base class, marking DiskStore as stable.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (4)
95-96: LGTM: Clear lifecycle documentation.The docstring properly documents lifecycle ownership for the overload.
135-137: LGTM: Clear lifecycle documentation.The docstring properly documents lifecycle ownership in the main constructor.
147-147: LGTM: Correct ownership tracking.The flag correctly tracks whether the connection was provided.
170-170: LGTM: Correct base initialization.The parameters are properly passed to the base class.
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
439-465: LGTM: Past review feedback addressed.The defensive
getattrchecks have been successfully removed from lines 459 and 464. The_client_provided_by_userattribute is now initialized in__init__(line 448) and accessed directly in both__aexit__andclose(), which is the correct pattern.key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
118-122: LGTM: Correct delegation of stable_api to base class.The change correctly delegates the
stable_api=Trueflag to the base class constructor instead of setting it manually. This aligns with the base class changes and ensures consistent stability signaling.key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
127-141: LGTM: Correct lifecycle management for aioboto3 client.The overrides of
__aenter__and__aexit__correctly manage the aioboto3 client's context manager lifecycle only when the store creates the client. This ensures user-provided clients are not double-entered or double-exited.key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
71-82: LGTM: Correct client ownership initialization.The initialization correctly computes the
client_providedflag and passes it to the base class constructor. This ensures proper lifecycle management for both user-provided and store-created clients.key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
232-258: LGTM: Correct client lifecycle initialization.The initialization correctly determines whether the Elasticsearch client was provided by the user and passes this information to the base class for proper lifecycle management.
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (2)
144-173: LGTM: Correct client ownership initialization.The initialization correctly computes
client_providedand passes it to the base class constructor, addressing the initialization order concerns from previous reviews. The pattern of computing the flag before callingsuper().__init__(..., client_provided_by_user=client_provided)is the correct approach.
372-380: LGTM: Defensive del implementation.The
__del__method correctly wraps all cleanup logic in a try-except block to prevent errors during interpreter shutdown. While_client_provided_by_usermight not exist if__init__fails early, the exception handling ensures this won't cause crashes.key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
104-107: LGTM! Clean initialization refactoring.The change correctly passes
stable_api=Truethrough the base class constructor, aligning with the new initialization pattern across the codebase.key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (3)
170-170: LGTM! Correct lifecycle flag initialization.The flag correctly captures whether the client was provided by the user.
187-191: LGTM! Proper base class initialization.The lifecycle flag is correctly passed to the base class, enabling centralized ownership management.
195-206: LGTM! Context manager guards are necessary.The guards in
__enter__and__exit__are correct and necessary to prevent entering/exiting the context of user-provided clients.key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (2)
204-204: LGTM! Correct lifecycle flag initialization.The flag correctly captures whether the Elasticsearch client was provided by the user.
225-230: LGTM! Proper base class initialization.The lifecycle flag is correctly passed to the base class for centralized ownership management.
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
68-68: LGTM! Correct lifecycle flag initialization.The flag correctly captures whether the disk cache was provided by the user.
82-86: LGTM! Proper base class initialization.The lifecycle flag and stable API flag are correctly passed to the base class.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (2)
70-70: LGTM! Correct lifecycle flag initialization.The flag correctly captures whether the Redis client was provided by the user.
88-88: LGTM! Proper base class initialization.Both the lifecycle flag and stable API flag are correctly passed to the base class.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
107-107: LGTM! Clean initialization refactoring.The change correctly passes
stable_api=Truethrough the base class constructor, consistent with the new initialization pattern.
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py
Outdated
Show resolved
Hide resolved
|
@claude please fix the test failures and the PR Feedback. Should _is_closed be part of the context manager base store logic? What do you think? |
|
Claude finished @strawgate's task —— View job Test Failures and PR Feedback - Fixed
Summary of Changes1. Removed Redundant Ownership Checks in CodeRabbit correctly identified that the base class Fixed in these stores (both async and sync):
2. Added Defensive Added defensive attribute access in
3. Fixed RocksDB Test Failure The Windows test failure was caused by the test not closing the user-provided database before cleanup: # Added before temp_dir.cleanup()
db.close()This ensures the database lock file is released before the temp directory tries to cleanup.
|
Test Failure AnalysisSummary: The RocksDB tests are failing on Windows during cleanup with Root Cause: The issue is in The problem occurs because:
This is a Windows-specific issue with RocksDB file locking behavior, not an issue with the PR changes themselves. Suggested Solution: The test needs to be modified to handle Windows' delayed file handle release. Here are the recommended fixes:
Detailed AnalysisFailed Tests
Error LogThe error occurs during Why This HappensRocksDB creates a LOCK file to prevent concurrent access to the database. On Windows, even after calling Why Only
|
… fix test - Remove redundant _client_provided_by_user checks from all store _close() methods since base class already guards these calls - Add defensive getattr in __del__ methods to prevent AttributeError during cleanup - Fix RocksDB test to properly close user-provided database before temp cleanup - This addresses CodeRabbit feedback and fixes Windows test failures Stores updated: - RocksDB, Disk, DuckDB (both async and sync) - Redis, Valkey, MongoDB, Elasticsearch, Memcached, DynamoDB (async only) Co-authored-by: William Easton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
135-141: Prevent double-closing the DynamoDB client
BaseContextManagerStore.__aexit__()already runsawait self.close(), so_close()fires first and executesself._client.__aexit__(None, None, None). When control returns here the guard still passes, so we invoke__aexit__a second time with the real exception tuple, double-entering the aioboto3 context manager and provoking cleanup failures. Move the client shutdown ahead of thesuper()call and clear_clientso the base layer no longer sees anything to close.@@ - await super().__aexit__(exc_type, exc_value, traceback) - # Only exit the client's context manager if the store created it - if not self._client_provided_by_user and self._client: - await self._client.__aexit__(exc_type, exc_value, traceback) + if not self._client_provided_by_user and self._client: + client = self._client + self._client = None + await client.__aexit__(exc_type, exc_value, traceback) + await super().__aexit__(exc_type, exc_value, traceback)
♻️ Duplicate comments (1)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
203-208: Critical: Double-cleanup of client resources persists.When the store creates the client (
_client_provided_by_useris False):
- Line 205:
super().__aexit__()triggers base class cleanup, which calls_close()- Line 348:
_close()callsself._client.close()- Line 208: Then
__aexit__callsself._client.__aexit__()This results in both
close()and__aexit__()being called on the same client, causing double-cleanup. This issue was previously flagged but has not been resolved.Recommended fix: Remove the client lifecycle management from
__aexit__and handle it only in_closeto maintain symmetry with__aenter__:@override async def __aexit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None: await super().__aexit__(exc_type, exc_val, exc_tb) - # Only exit the client's context manager if the store created it - if not self._client_provided_by_user: - await self._client.__aexit__(exc_type, exc_val, exc_tb)And update
_closeto call__aexit__instead ofclose():@override async def _close(self) -> None: - await self._client.close() + await self._client.__aexit__(None, None, None)Also applies to: 347-348
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(3 hunks)key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(3 hunks)key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
_close_and_flush(92-96)
key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(144-148)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_client(94-100)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_client(96-102)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (4)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (4)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_close_and_flush(86-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-shared)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
🔇 Additional comments (55)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (4)
218-220: LGTM! Clear lifecycle documentation.The docstring update explicitly documents the ownership semantics for user-provided clients, making it clear that the store will not manage the lifecycle of externally provided clients. This aligns perfectly with the PR objectives.
232-232: LGTM! Correct ownership detection.The logic correctly determines whether the client was provided by the user or will be created internally by the store. This boolean is then properly used to inform the base class about lifecycle ownership.
253-258: LGTM! Proper ownership propagation.The
client_provided_by_userparameter correctly propagates the ownership information to the base class, enabling the base store to handle lifecycle management appropriately based on client ownership.
555-557: LGTM! Correct template method implementation.The
_close()method correctly implements the cleanup action without redundant ownership checks. The base classclose()method handles the ownership logic, and this method is only called when the store owns the client. The past review feedback has been properly addressed.key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (4)
57-59: Docstring clearly documents the new ownership semantics.The updated documentation explicitly states that a provided client won't be closed by the store and the caller remains responsible for lifecycle management. This aligns well with the "you create it, you manage it" principle from the PR objectives.
67-67: Clean flag initialization for ownership tracking.The
client_providedflag correctly captures whether the user supplied their own client, enabling proper lifecycle delegation to the base class.
91-95: Proper delegation of lifecycle management to base class.The initialization correctly passes
client_provided_by_userandstable_apito the base class, enabling it to handle lifecycle decisions. This cleanly separates ownership concerns from the store implementation.
226-227: _close correctly simplified per previous feedback.The unconditional close is correct because the base class now guards calls to
_close()whenclient_provided_by_useris True. This properly separates concerns and avoids duplicate responsibility.key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (3)
63-65: LGTM: Clear lifecycle ownership documentation.The docstring clearly communicates the ownership semantics — when a client is provided, the store will not manage its lifecycle.
78-82: LGTM: Correct ownership flag propagation.The
client_provided_by_userflag is correctly computed and passed to the base class, ensuring proper lifecycle management.
156-157: LGTM: Correct unconditional close in template method.The
_close()method correctly closes the client unconditionally. The base class'sclose()method already guards this call based on the_client_provided_by_userflag, so_close()is only invoked when the store created the client.key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (2)
55-67: LGTM! Clear lifecycle management documentation.The docstring effectively communicates the ownership semantics: when a client is provided, the caller retains lifecycle responsibility. This aligns perfectly with the PR's "you create it, you manage it" principle.
68-83: LGTM! Correct ownership tracking and delegation.The implementation properly:
- Tracks whether the client was provided by the caller (line 68)
- Passes ownership information to the base class (line 81)
- Delegates lifecycle control to
BaseContextManagerStoreThis ensures the base class
close()method (which checks_client_provided_by_userbefore calling_close()) correctly handles the lifecycle.key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (4)
92-93: LGTM: Clear lifecycle ownership documentation.The docstring clearly states that providing a connection means the caller is responsible for its lifecycle. This aligns with the PR's goal of "you create it, you manage it" semantics.
132-134: LGTM: Consistent lifecycle documentation.The main
__init__docstring properly documents the lifecycle ownership semantics, consistent with the overload signature.
144-144: LGTM: Correct initialization order avoids past critical bug.The flag is computed locally and passed to
super().__init__(), which correctly avoids the critical initialization-order bug mentioned in past reviews where settingself._client_provided_by_user = Truebefore callingsuper().__init__()would cause the base class to reset it toFalse. This pattern ensures the base class initializes the attribute correctly.Also applies to: 168-173
368-370: LGTM: Redundant check correctly removed.The
_client_provided_by_usercheck has been properly removed from_close()as suggested in past reviews. The base classclose()method already guards calls to_close()with this check, so it was redundant here. The_is_closedflag remains to prevent double-closing.key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (4)
162-164: LGTM: Clear lifecycle documentation.The updated docstring accurately explains the new ownership semantics—when a client is provided by the user, the store will not manage its lifecycle.
172-172: LGTM: Clean flag detection.The
client_providedflag correctly identifies whether the user supplied a client.
189-193: LGTM: Correct propagation to base class.The
client_provided_by_userflag is properly passed to the base class, enabling it to guard lifecycle operations appropriately.
195-201: LGTM: Conditional context entry.The
__aenter__method correctly enters the client's context manager only when the store created it, preserving user-provided client independence.key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (3)
60-70: LGTM! Clear lifecycle documentation and correct flag computation.The docstring clearly communicates the lifecycle ownership semantics, and the
client_providedflag is computed correctly.
88-88: LGTM! Lifecycle flag correctly propagated to base class.The
client_provided_by_userparameter is correctly passed to the base class, enabling conditional lifecycle management.
213-214: LGTM! Clean_close()implementation without redundant checks.The
_close()method correctly delegates the ownership decision to the base class'sclose()method and focuses solely on cleanup logic.key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (4)
160-170: LGTM! Lifecycle semantics clearly documented and flag computed correctly.The docstring and
client_providedflag computation follow the established pattern consistently.
187-191: LGTM! Base class initialization with lifecycle flag.The
client_provided_by_userparameter is correctly passed to enable conditional lifecycle management.
194-206: LGTM! Context manager methods correctly respect client ownership.The
__enter__and__exit__methods appropriately manage the client's context only when the store created it, preventing double-context-entry issues for user-provided clients.
322-323: LGTM! Clean_close()implementation.The method correctly delegates ownership decisions to the base class and focuses on cleanup logic only.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (3)
57-70: LGTM! Comprehensive documentation and correct lifecycle flag.The docstring thoroughly documents the lifecycle responsibility and all parameters, and the
client_providedflag is computed correctly.
81-81: LGTM! Lifecycle flag propagated to base class.The initialization correctly passes
client_provided_by_userto enable conditional lifecycle management.
161-163: LGTM! Defensive null check in_close().The check for
_connected_client is not Noneis appropriate defensive programming given the lazy initialization pattern (client is set in_setup()), not a redundant ownership check.key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (3)
56-71: LGTM! Lifecycle documentation and flag computation.The docstring clearly describes the lifecycle responsibility, and the
client_providedflag is computed correctly.
85-85: LGTM! Correct lifecycle propagation and clean_close()implementation.The initialization correctly passes the ownership flag, and
_close()appropriately delegates ownership decisions to the base class.Also applies to: 122-123
126-127: LGTM! Safe attribute access in__del__.The method correctly uses
getattrandhasattrto preventAttributeErrorduring finalization if__init__failed early.key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (3)
190-204: LGTM! Lifecycle documentation and flag computation.The docstring clearly communicates the lifecycle responsibility, and the
client_providedflag is computed correctly.
225-230: LGTM! Lifecycle flag correctly propagated.The
client_provided_by_userparameter is correctly passed to enable conditional lifecycle management.
482-483: LGTM! Clean_close()implementation.The method correctly delegates ownership decisions to the base class and focuses solely on cleanup logic.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (3)
95-96: LGTM! Comprehensive lifecycle documentation and correct flag.The docstrings clearly communicate lifecycle responsibility in both the overload and main implementation, and the
client_providedflag is computed correctly.Also applies to: 135-147
170-170: LGTM! Lifecycle flag propagated and defensive close implementation.The initialization correctly passes
client_provided_by_user, and the_is_closedcheck in_close()appropriately prevents double-close errors.Also applies to: 311-315
317-325: LGTM! Robust__del__implementation with safe attribute access.The method correctly uses
getattrandhasattrto preventAttributeErrorduring finalization, and appropriately suppresses exceptions during interpreter shutdown.key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (1)
65-66: LGTM! Correct lifecycle management in test.The test correctly demonstrates the new ownership semantics by explicitly closing the user-provided database before cleanup, aligning with the "you create it, you manage it" principle.
key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (1)
62-63: LGTM! Correct lifecycle management in async test.The test correctly demonstrates the new ownership semantics by explicitly closing the user-provided database before cleanup, consistent with the "you create it, you manage it" principle.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (5)
7-7: LGTM: Import cleanup.The unused typing imports have been removed while retaining
overload, which is correctly used for the constructor overloads.
51-53: LGTM: Clear lifecycle documentation.The docstring update clearly communicates the ownership semantics: when a user provides a
dbinstance, they remain responsible for its lifecycle.
65-66: LGTM: Client ownership detection.The flag correctly identifies whether the user provided the database instance, enabling appropriate lifecycle management downstream.
80-80: LGTM: Lifecycle flag propagation.The
client_provided_by_userflag is correctly passed to the base class, enabling centralized lifecycle management.
83-84: LGTM: Clean _close implementation.The method correctly delegates to
_close_and_flush()without redundant ownership checks, since the base class guards calls to_close().key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (5)
4-4: LGTM: Import cleanup.Unused typing imports removed while retaining
overloadfor the constructor signatures.
54-56: LGTM: Clear lifecycle documentation.The docstring correctly documents that user-provided database instances are not managed by the store.
68-69: LGTM: Client ownership detection.The flag correctly determines whether the database instance was provided by the caller.
83-86: LGTM: Lifecycle flag propagation.The ownership flag is correctly passed to
BaseContextManagerStore, enabling proper lifecycle management.
89-90: LGTM: Clean _close implementation.The method appropriately delegates to
_close_and_flush()without redundant ownership checks. The base class handles lifecycle guards.key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (3)
53-55: Clear lifecycle documentation.The docstring accurately describes the new ownership semantics—when a user provides
disk_cache, they retain responsibility for its lifecycle. This helps prevent misuse and aligns with the PR's goal of clear ownership boundaries.
82-86: Correct propagation of ownership semantics.The base class initialization correctly receives the
client_provided_by_userflag andstable_api=True, implementing the new ownership model. Once the exception-safety issue at line 68 is addressed, this will properly communicate ownership to the base class.
133-134: Defensive cleanup pattern is correct but depends on early attribute assignment.The implementation properly addresses the past review comment by using
getattrandhasattrto handle partial initialization failures. However, this defensive pattern only works correctly ifself._client_provided_by_useris set early (beforeself._cache), as recommended at line 68. Without that fix, thegetattr(..., False)default could cause the destructor to close a user-provided cache whensuper().__init__()fails.Note: The PR objectives mention removing
getattrusage if the attribute is defined on the class. However,getattrremains necessary here because__del__can be called even if__init__fails before reachingsuper().__init__(), in which case the base class never sets_client_provided_by_user. The defensivegetattr(..., False)correctly handles this scenario—provided the instance attribute is set early per the recommendation at line 68.
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py
Outdated
Show resolved
Hide resolved
Test Failure AnalysisSummary: Two Windows worker crashes on Python 3.12 in Root Cause: This is a manifestation of issue #166 ("MultiDiskStore on Windows test flaky"). The
Both tests were automatically retried by pytest-xdist:
This is NOT related to the PR's changes - the client ownership refactoring doesn't affect the Evidence it's unrelated to this PR:
Suggested Solution: This is not a blocker for merging this PR. The failures are flaky and passed on retry. However, to improve overall test reliability:
Detailed AnalysisTest Results
Worker Crash PatternWhat
|
|
@claude please address the open pr feedback |
|
Claude finished @strawgate's task —— View job Addressing PR Feedback - Complete ✅Tasks:
Changes Made
Notes
| |
- MongoDB: Remove duplicate client cleanup from __aexit__, update _close() to call __aexit__(None, None, None) for symmetry with __aenter__ - DynamoDB: Move client cleanup before super().__aexit__() to prevent double-cleanup, set _client = None after closing - RocksDB: Add try-except in __del__ for best-effort cleanup with appropriate noqa comments - DuckDB: Use getattr for _is_closed check in __del__ for consistency - Disk: Set _client_provided_by_user early for exception safety - Memcached: Change truthiness check to explicit 'is not None' for consistency - Valkey: Fix _close() to close the correct client (_connected_client) and set it to None for idempotency Co-authored-by: William Easton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
144-173: Critical: User-provided connection may be closed if init fails partway through.If
__init__fails after line 147 (where a user-provided connection is assigned toself._connection) but before line 168 (wheresuper().__init__()sets_client_provided_by_user), the__del__cleanup method will default_client_provided_by_usertoFalseand incorrectly close the user's connection.Scenario:
- User provides a connection → assigned at line 147
- Validation or setup fails at lines 159-166 (e.g., invalid
table_name)__del__runs:getattr(self, "_client_provided_by_user", False)→False__del__closes the user's connection (violates ownership semantics)Apply this diff to set the flag immediately, before any operation that could fail:
if connection is not None and database_path is not None: msg = "Provide only one of connection or database_path" raise ValueError(msg) client_provided = connection is not None + # Set this immediately to ensure __del__ has correct ownership info even if __init__ fails + self._client_provided_by_user = client_provided if connection is not None: self._connection = connectionThen update
super().__init__()to only set the flag if not already present, or document that the base class must respect pre-set values.key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
70-81: Consider explicitly initializing_client_configtoNone.The
client_providedflag and the base class initialization are correct. However, when a client is provided by the user,_client_configis never initialized, leaving it as an uninitialized attribute. While safe in practice (it's only accessed in_setup()which checks_connected_clientfirst), explicitly setting it toNonewould make the code more robust and align with the type annotation.Apply this diff to initialize
_client_config:if client is not None: self._connected_client = client + self._client_config = None else:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_client(96-102)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_client(94-100)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (4)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (4)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(146-150)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
_close_and_flush(92-96)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_client(96-102)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_client(94-100)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_close_and_flush(86-90)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(146-150)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(432-435)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(462-465)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (32)
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (4)
63-65: LGTM! Clear lifecycle ownership documentation.The docstring clearly explains that user-provided clients are not managed by the store, which aligns perfectly with the PR's objective of implementing clear ownership semantics.
71-76: LGTM! Correct client ownership determination.The logic correctly distinguishes between user-provided and store-created clients. The None-check pattern is now consistent throughout (both line 71 and line 73 use
is not None), addressing the style concern from previous reviews.
81-81: LGTM! Proper flag passing to base class.The
client_provided_by_userflag is correctly passed to the base class constructor, ensuring the base class can properly track ownership and only close clients created by the store. This resolves the flag-overwriting issue mentioned in previous reviews.
156-157: LGTM! Correct template method implementation.The
_close()method now unconditionally closes the client, correctly following the template method pattern. The base class guards the call to_close()based on the_client_provided_by_userflag, so this method is only invoked when the store created the client. This resolves the redundant-check issue from previous reviews.key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (4)
53-55: LGTM! Clear ownership documentation.The docstring correctly documents that when
disk_cacheis provided, the store will not manage its lifecycle. This clearly communicates the ownership semantics to callers.
68-69: Exception safety correctly implemented.Setting
_client_provided_by_userimmediately after computing it (and before setting_cache) correctly addresses the previous feedback about exception safety. This ensures__del__can reliably determine ownership even if initialization fails partway through.
83-87: LGTM! Proper base class initialization.The consolidated
super().__init__()call correctly passesclient_provided_by_userto the base class, enabling consistent lifecycle management across the store hierarchy.
133-135: LGTM! Defensive cleanup correctly implemented.The
__del__method correctly uses defensive attribute access withgetattrandhasattrto handle all initialization failure modes. The logic properly respects ownership: only closing the cache when the store created it and the attribute exists. This addresses the previous feedback about defensive attribute access.key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (4)
4-4: LGTM: Appropriate import for overload decorators.The
overloadimport is correctly added to support the multiple constructor signatures defined at lines 26 and 35.
54-56: LGTM: Clear documentation of lifecycle ownership.The docstring explicitly states the new ownership semantics, making it clear that user-provided database instances remain the caller's responsibility.
68-69: LGTM: Correct ownership determination.The
client_providedflag correctly reflects whether the database was supplied by the caller, properly distinguishing between user-provided and store-created clients.
83-86: LGTM: Proper ownership propagation to base class.The
client_provided_by_userparameter correctly propagates ownership semantics to the base class, enabling it to conditionally manage the client's lifecycle.key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (4)
55-67: LGTM! Clear lifecycle documentation.The docstring effectively communicates the ownership semantics, making it clear that user-provided clients remain the caller's responsibility while store-created clients are managed by the store.
68-68: LGTM! Clean ownership tracking.The boolean flag clearly captures whether the client was provided by the user, enabling proper lifecycle management downstream.
79-83: LGTM! Proper base class initialization.The consolidated
super().__init__call correctly propagates theclient_provided_by_userflag, enabling the base class to enforce lifecycle ownership semantics. This aligns with the PR's goal of centralizing lifecycle management decisions.
170-172: LGTM! Correct and idempotent cleanup.The
_close()implementation properly addresses both past review comments:
- The redundant
_client_provided_by_usercheck has been removed (the base class already guards this)- The
_connected_client = Noneassignment has been added, making the method idempotentThe defensive check for
Noneis appropriate and prevents double-close issues. Since the base class only invokes_close()for store-created clients, this correctly implements the ownership semantics.key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (3)
92-93: LGTM: Docstrings clearly document lifecycle semantics.The docstrings correctly clarify that when a connection is provided, the store will NOT manage its lifecycle. This aligns with the PR's "you create it, you manage it" ownership model.
Also applies to: 132-134
368-370: LGTM: Correctly simplified _close() per past feedback.The
_client_provided_by_usercheck has been appropriately removed since the base class'sclose()method already guards the call to_close(). The_is_closedcheck remains to prevent double-closing.
375-379: LGTM: del uses consistent defensive checks per past feedback.The
__del__method now uses defensivegetattrfor all three attributes (_is_closed,_client_provided_by_user, and_connectionviahasattr), addressing the inconsistency flagged in past reviews.However, this defensive approach relies on
_client_provided_by_userbeing set correctly during initialization. The critical issue I flagged at lines 144-173 must be fixed to ensure this cleanup logic is safe when__init__fails partway through.key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (3)
7-7: LGTM: Overload import added for type hints.The
overloadimport is correctly added to support the overloaded__init__signatures at lines 29 and 38.
51-53: LGTM: Docstring clearly states lifecycle ownership.The updated docstring correctly documents that when a
dbis provided, the store will not manage its lifecycle, aligning with the PR objectives.
83-84: LGTM: Simplified _close() method.The redundant
_client_provided_by_usercheck has been correctly removed, as noted in past review feedback. The base class already guards calls to_close(), so unconditionally delegating to_close_and_flush()is appropriate.key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
57-69: LGTM! Clear lifecycle documentation.The docstring clearly documents the new ownership semantics, explicitly stating that user-provided clients are not managed by the store.
162-164: LGTM! Correct defensive cleanup logic.The check
if self._connected_client is not Noneis a proper safety guard. Since the base class only calls_close()when the store created the client (_client_provided_by_user=False), this condition prevents attempting to closeNonein cases where_setup()was never called (e.g., context manager never entered). Clearing the reference afterward is good practice.key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (3)
56-58: LGTM! Clear lifecycle ownership documentation.The docstring explicitly documents that providing a disk_cache transfers lifecycle responsibility to the caller, which aligns perfectly with the PR's ownership semantics.
86-86: LGTM! Proper base class initialization.The consolidated
super().__init__call correctly passes all lifecycle-related parameters (client_provided_by_user,stable_api) along with the default collection.
123-124: LGTM! Properly simplified per past review.The redundant
_client_provided_by_usercheck has been correctly removed since the base class already guards calls to_close().key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
93-95: LGTM: Client ownership flag correctly implemented.The
client_providedflag is properly computed and passed to the base class, and__aenter__()correctly skips entering the client's context manager when a user-provided client is detected. The docstring clearly communicates the lifecycle responsibilities.Also applies to: 105-106, 121-124, 129-130
156-158: Review comment is incorrect — the code is not redundant.The lines at 156-158 are not dead code. While
_setup()is called from__aenter__()viasetup(), thesetup()method is also called independently from several public API methods includingsetup_collection(),collections(),destroy(), andcull(). When these methods are called directly without going through__aenter__(), the client has not yet been entered.The check
if not self._clientat line 156 is a necessary fallback that handles cases where_setup()is invoked outside the context manager flow. Without it, operations that callsetup()directly would fail if the user didn't provide a client, since_raw_clientwould never have been entered.Likely an incorrect or invalid review comment.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (3)
160-162: LGTM! Clear documentation of ownership semantics.The docstring update properly documents that user-provided clients are not managed by the store, aligning with the PR's goal of clear ownership semantics.
170-171: LGTM! Correct tracking and propagation of client ownership.The implementation correctly tracks whether a client was provided by the user and propagates this information to the base class, enabling conditional lifecycle management.
Also applies to: 187-191
195-199: LGTM! Correct conditional context manager entry.The
__enter__method correctly enters the client's context manager only when the store created the client, preventing double-context-entry issues when users provide their own clients.
| # Exit the client's context manager before calling super().__aexit__() to avoid double-cleanup | ||
| if not self._client_provided_by_user and self._client: | ||
| client = self._client | ||
| self._client = None | ||
| await client.__aexit__(exc_type, exc_value, traceback) | ||
| await super().__aexit__(exc_type, exc_value, traceback) |
There was a problem hiding this comment.
Client cleanup contradicts past review feedback.
The past review comment explicitly requested moving cleanup logic to _close() and removing the _client_provided_by_user check (because the base class handles that). However, the current implementation keeps cleanup in __aexit__() with the check still present.
This design has two issues:
- It contradicts the explicit guidance from the previous review (marked as "✅ Addressed")
- If the base class calls
_close()fromclose()(non-context-manager path), cleanup won't occur since_close()is now empty
Move the cleanup logic to _close() as previously requested:
@override
async def __aexit__(
self, exc_type: type[BaseException] | None, exc_value: BaseException | None, traceback: TracebackType | None
) -> None:
- # Exit the client's context manager before calling super().__aexit__() to avoid double-cleanup
- if not self._client_provided_by_user and self._client:
- client = self._client
- self._client = None
- await client.__aexit__(exc_type, exc_value, traceback)
await super().__aexit__(exc_type, exc_value, traceback) @override
async def _close(self) -> None:
"""Close the DynamoDB client."""
- # Client cleanup is handled in __aexit__
+ if self._client:
+ await self._client.__aexit__(None, None, None)Based on past review comments.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py around
lines 138 to 143, the client cleanup is still performed in __aexit__ with a
_client_provided_by_user check; move that cleanup into _close() as requested:
remove the _client_provided_by_user conditional entirely, implement _close() to
capture self._client, set self._client = None, and await
client.__aexit__(exc_type=None, exc_value=None, traceback=None) (or simply await
client.__aexit__(None, None, None)) to perform cleanup; leave __aexit__ to only
call await super().__aexit__(...) (no client cleanup) so both context-manager
and non-context-manager paths use _close().
| if not getattr(self, "_client_provided_by_user", False): | ||
| try: # noqa: SIM105 | ||
| self._close_and_flush() | ||
| except (AttributeError, Exception): # noqa: S110 | ||
| pass # Best-effort cleanup during finalization |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Addresses previous feedback with defensive finalization.
The implementation correctly addresses past review concerns by using getattr for safe attribute checking and wrapping cleanup in try-except for best-effort finalization.
Minor nitpick: Catching (AttributeError, Exception) is redundant since Exception already includes AttributeError. However, being explicit about the expected exception type is acceptable for defensive finalization code.
If you prefer to simplify:
- except (AttributeError, Exception): # noqa: S110
+ except Exception: # noqa: S110
pass # Best-effort cleanup during finalization📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not getattr(self, "_client_provided_by_user", False): | |
| try: # noqa: SIM105 | |
| self._close_and_flush() | |
| except (AttributeError, Exception): # noqa: S110 | |
| pass # Best-effort cleanup during finalization | |
| if not getattr(self, "_client_provided_by_user", False): | |
| try: # noqa: SIM105 | |
| self._close_and_flush() | |
| except Exception: # noqa: S110 | |
| pass # Best-effort cleanup during finalization |
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py around
lines 196 to 200, the except clause redundantly lists AttributeError alongside
Exception; replace the tuple except (AttributeError, Exception) with a single
except Exception (optionally capture it as except Exception as e if you want to
log or inspect) to remove redundancy while preserving the best-effort cleanup
behavior during finalization.
| client_provided = disk_cache is not None | ||
| self._client_provided_by_user = client_provided |
There was a problem hiding this comment.
🧩 Analysis chain
Potential redundant assignment.
Line 72 sets self._client_provided_by_user before passing client_provided_by_user to the base class at line 86. If the base class __init__ sets this attribute from the parameter (which is the typical pattern), this assignment is redundant.
Verify whether the base class sets _client_provided_by_user from the parameter. If it does, remove line 72:
client_provided = disk_cache is not None
-self._client_provided_by_user = client_provided
If the base class does NOT set it, then this is correct as-is.
🏁 Script executed:
#!/bin/bash
# Description: Check if the base class __init__ sets _client_provided_by_user from the parameter
# Search for the base class __init__ method to see how it handles the client_provided_by_user parameter
ast-grep --pattern $'class BaseContextManagerStore:
$$$
def __init__($$$, client_provided_by_user, $$$):
$$$
$$$'
# Also search for assignments to _client_provided_by_user in the base classes
rg -n "_client_provided_by_user\s*=" --type=py key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py key-value/key-value-aio/src/key_value/aio/stores/base.pyLength of output: 260
Remove redundant assignment at line 72.
The base class __init__ methods (both sync and aio versions) already set self._client_provided_by_user from the parameter passed at line 86. The assignment at line 72 is overwritten and unnecessary.
client_provided = disk_cache is not None
-self._client_provided_by_user = client_provided📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client_provided = disk_cache is not None | |
| self._client_provided_by_user = client_provided | |
| client_provided = disk_cache is not None |
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py
around lines 71 to 72, remove the redundant assignment of
self._client_provided_by_user (the line setting it from client_provided) because
the base class __init__ already sets this attribute later (see line 86); simply
delete the assignment at line 72 and keep the client_provided local variable if
still used, ensuring no other code in this method relies on the overwritten
assignment.
| if not getattr(self, "_client_provided_by_user", False) and hasattr(self, "_cache"): | ||
| self._cache.close() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider exception handling in finalizer.
The attribute guards correctly handle early __init__ failures. However, if _cache.close() raises an exception during finalization, it will be suppressed but logged as a warning by Python.
For maximum robustness, consider wrapping the close call:
def __del__(self) -> None:
if not getattr(self, "_client_provided_by_user", False) and hasattr(self, "_cache"):
- self._cache.close()
+ try:
+ self._cache.close()
+ except Exception:
+ pass # Suppress exceptions during finalizationThis is optional since diskcache.Cache.close() is unlikely to raise exceptions in practice.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not getattr(self, "_client_provided_by_user", False) and hasattr(self, "_cache"): | |
| self._cache.close() | |
| if not getattr(self, "_client_provided_by_user", False) and hasattr(self, "_cache"): | |
| try: | |
| self._cache.close() | |
| except Exception: | |
| pass # Suppress exceptions during finalization |
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py
around lines 127-128, the finalizer calls self._cache.close() without exception
handling, so if close() raises during object finalization the exception may be
logged or interfere with shutdown; wrap the close call in a try/except Exception
block to catch and suppress any errors (optionally logging them via the module
logger or warnings.warn) so finalization never raises — keep the existing
attribute guards and ensure the except block is minimal and safe for use in
__del__.
| if ( | ||
| not getattr(self, "_is_closed", False) | ||
| and (not getattr(self, "_client_provided_by_user", False)) | ||
| and hasattr(self, "_connection") | ||
| ): |
There was a problem hiding this comment.
Prevent closing user-managed connections on constructor failure
If the constructor raises before BaseStore.__init__ runs (e.g., invalid table_name), _client_provided_by_user is never set. In that path getattr(..., False) evaluates to False, so __del__ will close the caller’s DuckDB connection even though the store promised not to manage user-provided clients. That breaks the new ownership contract and can corrupt the caller’s reuse of the connection. Please persist the ownership flag before any potential early exit (and then rely on the real attribute in __del__) so we never close external connections unexpectedly.
Apply this diff to set the flag eagerly and rely on it in the destructor:
@@
- client_provided = connection is not None
+ client_provided = connection is not None
+ self._client_provided_by_user = client_provided
@@
- try:
- if (
- not getattr(self, "_is_closed", False)
- and (not getattr(self, "_client_provided_by_user", False))
- and hasattr(self, "_connection")
- ):
+ try:
+ if (
+ not getattr(self, "_is_closed", False)
+ and not getattr(self, "_client_provided_by_user", False)
+ and hasattr(self, "_connection")
+ ):
self._connection.close()
self._is_closed = True(After setting the attribute up front you can optionally tighten the getattr usage or replace it entirely since the attribute will always exist.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py
around lines 320-324, the constructor can raise before BaseStore.__init__ sets
_client_provided_by_user, so __del__ may mistakenly close a user-supplied DuckDB
connection; set self._client_provided_by_user = bool(client) (or appropriate
truthy check) as the first thing in the constructor before any validation or
operations that can raise, and then update the destructor to rely on that
attribute (or remove the getattr fallback) so we never close external
connections on constructor failure.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py
Outdated
Show resolved
Hide resolved
| client_provided = db is not None | ||
|
|
There was a problem hiding this comment.
Critical: Ownership flag must be set before calling super() to prevent lifecycle bug.
The current implementation computes client_provided as a local variable (line 65) and passes it to super().__init__() (line 80), which should store it in _client_provided_by_user. However, if super().__init__() raises an exception before setting the attribute, __del__ will incorrectly default to False via getattr and attempt to close a user-provided database.
Failure scenario:
- User provides
db→client_provided = True - Lines 67-76 store the user's db in
self._db - Line 80:
super().__init__()fails before setting_client_provided_by_user __del__executes:getattr(self, "_client_provided_by_user", False)returnsFalse- Store incorrectly closes the user's database
Apply this diff to set the flag as an instance variable before calling super():
- client_provided = db is not None
-
+ self._client_provided_by_user = db is not None
+
if db:
self._db = db
elif path:
@@ -77,7 +77,7 @@
self._is_closed = False
- super().__init__(default_collection=default_collection, client_provided_by_user=client_provided)
+ super().__init__(default_collection=default_collection, client_provided_by_user=self._client_provided_by_user)
@override
def _close(self) -> None:
@@ -183,7 +183,7 @@
def __del__(self) -> None:
- if not getattr(self, "_client_provided_by_user", False):
+ if not self._client_provided_by_user:
try: # noqa: SIM105
self._close_and_flush()
except (AttributeError, Exception): # noqa: S110This ensures the ownership flag is available for cleanup even if the base class initialization fails, and eliminates the need for getattr.
Also applies to: 80-80, 184-188
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py
around lines 65-80 and similarly at 184-188, set the ownership flag as an
instance attribute (self._client_provided_by_user = True or False) before
calling super().__init__ so the flag exists even if base-class initialization
raises; then call super().__init__(), and update __del__ to read the attribute
directly (remove the getattr fallback) so the destructor won’t accidentally
close a user-provided DB; apply the same pre-super assignment pattern at the
other noted location(s).
|
@claude please address the PR feedback. Did we document this behavior yet? When we're provided a client, we expect that client to be ready to use (entered, etc). When we build a client, we control the lifespan, which gives us the following options:
Do you think we should even bother with the |
|
Claude finished @strawgate's task —— View job Addressing PR Feedback ✅Tasks:
Changes MadeFixed critical double-cleanup issues identified by CodeRabbit: MongoDB Store (
DynamoDB Store (
Why these changes:
Documentation QuestionQ: "Did we document this behavior yet? When we're provided a client, we expect that client to be ready to use (entered, etc)." A: Yes, this is documented in the docstrings. For example, in """
Args:
client: The MongoDB client to use (mutually exclusive with url). If provided, the store
will not manage the client's lifecycle (will not enter/exit its context manager or
close it). The caller is responsible for managing the client's lifecycle.This clearly states:
This means the caller must ensure the client is ready to use (already entered if it's a context manager). This pattern is consistent across all stores that accept client parameters. Recommendation: Consider adding a note to the main README or a "Lifecycle Management" section in the docs to explain this pattern globally, rather than just in individual docstrings.
|
|
@claude we don't have to use the exit stack as a context manager, we can just use it to hold callbacks, etc and then exit and close both just exit the stack that way stores don't need to implement exit or close, just prepare the exit stack with the cleanup callback? What do you think |
|
Claude finished @strawgate's task —— View job Analysis of ExitStack Callback Approach
Excellent Idea! This Would Significantly Simplify the CodeYou're absolutely right - using Current Problems This Solves
How the ExitStack Callback Pattern WorksInstead of using ExitStack as a context manager, you use it as a cleanup registry: class BaseContextManagerStore(BaseStore, ABC):
_exit_stack: AsyncExitStack | None
_client_provided_by_user: bool
def __init__(self, *, client_provided_by_user: bool = False, **kwargs: Any) -> None:
self._client_provided_by_user = client_provided_by_user
self._exit_stack = None
super().__init__(**kwargs)
async def __aenter__(self) -> Self:
# Create the exit stack when entering
self._exit_stack = AsyncExitStack()
# Let subclasses register their cleanup callbacks
await self._register_cleanup_callbacks(self._exit_stack)
await self.setup()
return self
async def __aexit__(self, exc_type, exc_value, traceback) -> None:
# Exit stack handles all cleanup in correct order
if self._exit_stack:
await self._exit_stack.aclose()
self._exit_stack = None
async def close(self) -> None:
# Same cleanup path as __aexit__
if self._exit_stack:
await self._exit_stack.aclose()
self._exit_stack = None
@abstractmethod
async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None:
"""Register cleanup callbacks with the exit stack.
Subclasses should only register cleanup for resources THEY create.
If client_provided_by_user is True, don't register client cleanup.
"""
...Store Implementation Becomes TrivialMongoDB Example: async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None:
# Only register cleanup if we created the client
if not self._client_provided_by_user:
# Enter the client's context and register its exit
await stack.enter_async_context(self._client)That's it! No DynamoDB Example: async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None:
if not self._client_provided_by_user:
# Enter raw client context
self._client = await stack.enter_async_context(self._raw_client)RocksDB/Disk (non-context-manager clients): async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None:
if not self._client_provided_by_user:
# Register a callback for cleanup
stack.callback(self._db.close)
# or for async: stack.push_async_callback(self._cache.aclose)Key Benefits
Edge Cases Handled ElegantlyEarly initialization failure:
Non-context-manager cleanup:
Mixed resource types:
Implementation Plan
Minor Considerations
Actually, on point 3 - this might be a limitation: if users call methods like Recommendation: Implement This RefactoringThis is a genuinely better design that:
The only downside is it's a significant refactoring, but it would make the codebase much more maintainable going forward. Would you like me to implement this pattern as a follow-up to this PR, or incorporate it into this PR? | |
|
@claude do we need _register_cleanup_callbacks or can the store just do that in its own aenter? Can the context manager mixin also override _setup to enter the store if it's not already entered? Would that work too? Is it a good idea? If so please proceed with updating the PR |
|
Claude finished @strawgate's task —— View job Analyzing simplification proposal
Current Implementation AnalysisThe current implementation has stores like MongoDB and DynamoDB that:
This creates complexity with flag tracking and coordination between Technical Analysis of Proposed SimplificationYour Questions:
Why the current ExitStack callback approach is still best: Option 1: Stores handle cleanup in class MongoDBStore:
async def __aenter__(self):
if not self._client_provided_by_user:
await self._client.__aenter__()
# How do we ensure cleanup happens?
# Can't register with an exit stack we don't have access to
await super().__aenter__()
return selfProblems:
Option 2: Context manager mixin overrides class BaseContextManagerStore:
async def _setup(self):
# Auto-enter the store here?
await super()._setup()Problems:
Option 3: ExitStack callback pattern (previous discussion) class BaseContextManagerStore:
async def __aenter__(self):
self._exit_stack = AsyncExitStack()
await self._exit_stack.__aenter__()
await self._register_cleanup_callbacks(self._exit_stack)
await self.setup()
return self
async def __aexit__(self, exc_type, exc_value, traceback):
if self._exit_stack:
await self._exit_stack.aclose()
self._exit_stack = None
async def close(self):
if self._exit_stack:
await self._exit_stack.aclose()
self._exit_stack = None
class MongoDBStore:
async def _register_cleanup_callbacks(self, stack):
if not self._client_provided_by_user:
await stack.enter_async_context(self._client)Why ExitStack is superior:
RecommendationI recommend implementing the ExitStack callback pattern from the previous discussion. Your proposal to have stores handle cleanup in
The ExitStack pattern solves all these issues by centralizing lifecycle management in the base class and making stores simply declare what needs cleanup. Should I proceed with implementing the ExitStack pattern? |
|
@claude Please make a PR.
I think we want users to user async with but if they don't we still want to do async setup and so I was thinking in the context manager mixin we could detect if we're supposed to manage the client and if it is not entered we would call aenter from _setup |
|
Claude finished @strawgate's task —— View job Implementing automatic client entry from
|
Implements automatic client context entry at the base class level, allowing stores to work correctly even when not used with `async with`. Changes: - Add `_enter_client_context()` helper in BaseContextManagerStore - Add `_get_client_for_context()` override point for stores to specify their client - Add `_get_entered_client()` helper to retrieve the entered client - Call `_enter_client_context()` in both `__aenter__()` and `setup()` - Update MongoDB and DynamoDB stores to use the new base class pattern - Remove manual `__aenter__`/`__aexit__` overrides from store implementations This ensures that clients requiring context manager entry (like MongoDB's AsyncMongoClient and DynamoDB's aioboto3 client) are properly entered whether the store is used with `async with` or called directly. Fixes #232 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
key-value/key-value-aio/src/key_value/aio/stores/base.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(6 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (6)
key-value/key-value-aio/src/key_value/aio/stores/base.py (7)
_get_client_for_context(459-470)_enter_client_context(472-491)_get_entered_client(493-499)setup(150-172)setup(519-529)_close(532-538)close(514-517)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (2)
_get_client_for_context(194-196)_close(312-317)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_close(83-84)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_close(123-124)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_close(161-164)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
_close(213-214)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
_get_client_for_context(431-442)close(484-487)
key-value/key-value-aio/src/key_value/aio/stores/base.py (4)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
_get_client_for_context(126-128)_close(257-265)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (2)
_get_client_for_context(196-198)_close(337-342)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (6)
_get_client_for_context(431-442)_enter_client_context(444-463)setup(156-178)setup(489-499)_close(502-508)close(484-487)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (2)
_get_client_for_context(194-196)_close(312-317)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_get_client_for_context(459-470)_get_entered_client(493-499)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_get_client_for_context(196-198)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_get_client_for_context(459-470)close(514-517)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_get_client_for_context(126-128)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_get_client_for_context(194-196)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
🔇 Additional comments (25)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (4)
160-162: LGTM: Clear lifecycle documentation.The docstring correctly documents that user-provided clients are not managed by the store.
170-191: LGTM: Ownership tracking implemented correctly.The store correctly detects whether a client was provided and propagates this to the base class via
client_provided_by_user.
194-196: LGTM: Client context exposure.Correctly returns the MongoDB client for base class context management.
312-317: LGTM: Proper cleanup with symmetry.The
_close()implementation correctly handles both context manager and direct close paths:
- If context was entered, exits it via
__exit__- Otherwise, closes the client directly (only for store-created clients)
This maintains symmetry with
__enter__and prevents double-cleanup.key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (4)
162-164: LGTM: Clear lifecycle documentation.Docstring correctly specifies that user-provided clients are not managed by the store.
172-192: LGTM: Ownership tracking and initialization.Correctly detects client provenance and forwards the ownership flag to the base class.
196-198: LGTM: Client context exposure.Returns the async MongoDB client for base class context management.
337-342: LGTM: Async cleanup with proper symmetry.The async
_close()correctly mirrors the sync implementation, handling both context manager exit and direct close paths.key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (4)
92-94: LGTM: Clear lifecycle documentation.Docstring correctly documents client lifecycle ownership semantics.
104-123: LGTM: Ownership tracking.Correctly detects and propagates client ownership to the base class.
126-128: LGTM: Raw client exposure.Returns
_raw_client(the context manager) when it exists.
141-145: LGTM: Entered client retrieval.Correctly retrieves the entered client result from the base class context management.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (6)
94-108: LGTM: Stability flag added.The
stable_apiparameter correctly gates the stability warning.
401-429: LGTM: Client ownership infrastructure.The base class correctly initializes ownership tracking attributes and accepts the
client_provided_by_userparameter.
431-442: LGTM: Client context exposure hook.Provides the extension point for stores to return their context-manager client.
465-471: LGTM: Entered client accessor.Provides access to the entered client result.
473-499: LGTM: Context entry orchestration.The base class correctly enters the client context in both
__enter__andsetup()paths, with proper guards in__exit__andclose().
501-508: LGTM: Clear _close contract.The docstring correctly specifies that
_close()is only called for store-owned clients.key-value/key-value-aio/src/key_value/aio/stores/base.py (7)
88-102: LGTM: Stability flag added.The
stable_apiparameter correctly controls the stability warning.
429-457: LGTM: Client ownership infrastructure.The async base class correctly initializes ownership tracking and accepts the
client_provided_by_userparameter.
459-470: LGTM: Client context exposure hook.Provides the async extension point for stores to return their context-manager client.
472-491: LGTM: Async context entry.Correctly checks for
__aenter__and enters the client's async context manager.
493-499: LGTM: Entered client accessor.Provides access to the entered client result.
501-529: LGTM: Async context entry orchestration.The async base class correctly enters the client context in both
__aenter__andsetup()paths, with proper ownership guards.
531-538: LGTM: Clear _close contract.The docstring correctly documents that
_close()is only called for store-owned clients.
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py
Outdated
Show resolved
Hide resolved
| def _enter_client_context(self) -> Any: | ||
| """Enter the client's context manager if needed. | ||
| This is called automatically by the base class in either __aenter__() or setup(), | ||
| whichever happens first. Stores should not override this method directly; instead, | ||
| they should override _get_client_for_context() to return the client to enter. | ||
| Returns: | ||
| The result of entering the client's context manager (typically the client itself). | ||
| """ | ||
| if self._client_provided_by_user or self._client_context_entered: | ||
| return None | ||
|
|
||
| client = self._get_client_for_context() | ||
| if client is not None and hasattr(client, "__aenter__"): | ||
| result = client.__enter__() | ||
| self._client_context_entered = True | ||
| self._entered_client_result = result | ||
| return result | ||
| return None |
There was a problem hiding this comment.
Critical: Wrong context manager protocol check.
Line 458 checks hasattr(client, "__aenter__") in sync code. Synchronous context managers use __enter__/__exit__, not __aenter__/__aexit__ (which are for async).
This causes the context entry to silently fail—_enter_client_context() will return None for all sync clients, leaving them never entered.
Apply this diff:
def _enter_client_context(self) -> Any:
"""Enter the client's context manager if needed.
This is called automatically by the base class in either __aenter__() or setup(),
whichever happens first. Stores should not override this method directly; instead,
they should override _get_client_for_context() to return the client to enter.
Returns:
The result of entering the client's context manager (typically the client itself).
"""
if self._client_provided_by_user or self._client_context_entered:
return None
client = self._get_client_for_context()
- if client is not None and hasattr(client, "__aenter__"):
+ if client is not None and hasattr(client, "__enter__"):
result = client.__enter__()
self._client_context_entered = True
self._entered_client_result = result
return result
return None🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py around
lines 444 to 463, the code incorrectly checks for the async protocol
(__aenter__) when entering a synchronous client context; update the check to
look for the synchronous protocol by testing hasattr(client, "__enter__") and,
if present, call client.__enter__() to obtain the entered result, then set
self._client_context_entered and self._entered_client_result as currently done;
keep the early-return when client is provided by user or already entered.
|
@claude looks like this only got halfway done, let's remove _enter_client_context please, let's just have setup call aenter and have __aenter add aexit to the callbacks or something like that that's a bit cleaner using the exit stacks also please avoid hasattr |
|
Claude finished @strawgate's task —— View job Refactoring client lifecycle to use exit stacks
|
Simplified client lifecycle management by: - Removed _enter_client_context() method from base classes - Replaced manual context management with AsyncExitStack/ExitStack - Added _ensure_exit_stack() method that creates exit stack and registers client contexts - Removed complex _close() methods from MongoDB and DynamoDB stores - Exit stack now handles all cleanup automatically through its internal stack 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
key-value/key-value-aio/src/key_value/aio/stores/base.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(6 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (6)
_get_client_for_context(460-471)_get_entered_client(473-479)_ensure_exit_stack(481-500)setup(151-173)setup(522-532)close(516-520)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_get_client_for_context(126-128)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_get_client_for_context(196-198)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_get_client_for_context(194-196)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
_get_client_for_context(460-471)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_get_client_for_context(126-128)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
_get_client_for_context(432-443)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_get_client_for_context(194-196)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
_get_client_for_context(432-443)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_get_client_for_context(460-471)_get_entered_client(473-479)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_get_client_for_context(196-198)
key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_get_client_for_context(126-128)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_get_client_for_context(196-198)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (5)
_get_client_for_context(432-443)_ensure_exit_stack(453-472)setup(157-179)setup(492-502)close(486-490)
🪛 GitHub Actions: Run Tests
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
[error] 463-463: Command 'uv run basedpyright .' failed. Pyright error: Type of 'enter' is unknown (reportUnknownMemberType) in file base.py:463.
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
[failure] 489-489:
Cannot access attribute "close" for class "AsyncExitStack[bool | None]"
Attribute "close" is unknown (reportAttributeAccessIssue)
[failure] 489-489:
Type of "close" is unknown (reportUnknownMemberType)
[failure] 483-483:
Cannot access attribute "close" for class "AsyncExitStack[bool | None]"
Attribute "close" is unknown (reportAttributeAccessIssue)
[failure] 483-483:
Type of "close" is unknown (reportUnknownMemberType)
[failure] 463-463:
Cannot access attribute "enter" for class "AsyncExitStack[bool | None]"
Attribute "enter" is unknown (reportAttributeAccessIssue)
[failure] 463-463:
Type of "enter" is unknown (reportUnknownMemberType)
🔇 Additional comments (17)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (3)
77-123: Ownership model correctly implemented.The constructor properly determines client ownership, passes the flag to the base class, and documents the lifecycle semantics in the docstring. When a client is provided, the store uses it directly without managing its lifecycle. When creating a client internally, the store defers initialization to
_setupafter the base class enters the context.
138-145: Correctly delegates context management to base class.The store retrieves the entered client from the base class (which handles context entry via the exit stack) rather than managing entry itself. This properly implements the ownership model where the base class enters clients it owns.
256-256: Cleanup delegation to exit stack is cleaner than per-store_close().The exit-stack-based lifecycle management eliminates boilerplate and centralizes cleanup in the base class. This is a better approach than the earlier suggestion to implement
_close()in each store.key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (5)
162-164: LGTM: Clear documentation of lifecycle ownership.The updated docstring correctly states that user-provided clients are not managed by the store.
172-172: LGTM: Clean boolean flag derivation.Simple and clear.
192-192: LGTM: Correct ownership flag propagation.Passes the client ownership flag to the base class for lifecycle management.
196-198: LGTM: Straightforward hook implementation.Returns
self._clientfor base class context management. The base class guards context entry with the_client_provided_by_userflag, so even though this always returns the client, user-provided clients won't be entered/exited.
336-336: LGTM: Exit stack handles cleanup.The comment correctly reflects that the base class exit stack now handles all client lifecycle, eliminating the need for a
_closeoverride.key-value/key-value-aio/src/key_value/aio/stores/base.py (9)
89-89: LGTM: Stability warning for unstable stores.The
stable_apiparameter and warning mechanism help set user expectations.Also applies to: 102-104, 120-123
431-440: LGTM: Clear ownership documentation.The docstring clearly explains when stores manage client lifecycle and how subclasses should integrate via
_get_client_for_context.
442-458: LGTM: Clean initialization of lifecycle fields.Fields are initialized correctly and the
**kwargsforwarding allows flexibility for subclass constructors.
460-471: LGTM: Hook for client context management.Subclasses override this to return clients that need context entry. The default
Noneis correct for stores without such clients.
473-479: LGTM: Accessor for entered client result.Provides access to the result of
enter_async_context, useful if the entered client differs from the raw client.
481-500: LGTM: Exit stack creation and client registration.The exit stack pattern correctly handles client context entry when the store owns the client. The
hasattrcheck guards against clients without async context manager support.
508-514: LGTM: Idempotent exit stack cleanup.The
Nonecheck makes cleanup idempotent, correctly handling cases whereclose()is called before__aexit__.
516-520: LGTM: Manual cleanup for non-context-manager usage.Allows users who call methods directly (without
async with) to manually clean up resources.
522-532: LGTM: Ensures client context for direct setup calls.Guarantees that if
setupis called directly (withoutasync with), the client context is still entered. This prevents errors when stores are used without context managers.
| async def __aenter__(self) -> Self: | ||
| # Create exit stack and enter client context | ||
| await self._ensure_exit_stack() | ||
| await self.setup() | ||
| return self |
There was a problem hiding this comment.
Critical: Exit stack leaks if __aenter__ raises.
If _ensure_exit_stack or setup raises after the exit stack is created and entered, the exit stack will not be cleaned up because Python's context manager protocol does not call __aexit__ when __aenter__ raises.
Apply this diff to clean up the exit stack on exception:
async def __aenter__(self) -> Self:
- # Create exit stack and enter client context
- await self._ensure_exit_stack()
- await self.setup()
+ # Create exit stack and enter client context
+ try:
+ await self._ensure_exit_stack()
+ await self.setup()
+ except:
+ # Clean up exit stack if initialization fails
+ if self._exit_stack is not None:
+ await self._exit_stack.aclose()
+ self._exit_stack = None
+ raise
return self| def _get_client_for_context(self) -> Any | None: | ||
| """Return the raw client (context manager) for context management.""" | ||
| return self._raw_client if hasattr(self, "_raw_client") else None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace fragile hasattr check with explicit None assignment.
Using attribute existence as a proxy for ownership is implicit and fragile. If _raw_client is later set to None instead of remaining undefined, this breaks.
In __init__, always define _raw_client:
if client:
self._client = client
+ self._raw_client = None
else:
session: Session = aioboto3.Session(Then simplify _get_client_for_context:
@override
def _get_client_for_context(self) -> Any | None:
"""Return the raw client (context manager) for context management."""
- return self._raw_client if hasattr(self, "_raw_client") else None
+ return self._raw_clientCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py around
lines 126 to 128, the method uses hasattr(self, "_raw_client") which is fragile
if _raw_client is ever set to None; instead, in __init__ always initialize
self._raw_client = None and then simplify _get_client_for_context to simply
return self._raw_client (no hasattr check). Ensure __init__ sets the attribute
before any other methods are called so the method can rely on explicit None
rather than attribute existence.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
Outdated
Show resolved
Hide resolved
Test Failure AnalysisSummary: Static analysis (type checking) is failing because stores have Root Cause: The latest commit (ab49e98) refactored Suggested Solution: Remove the Async stores (key-value-aio):
Sync stores (key-value-sync) - will be auto-generated: Detailed AnalysisError MessagesFrom the async job (key-value-aio): From the sync job (key-value-sync): What ChangedThe
The exit stack approach is cleaner, but the store implementations weren't updated to remove their Additional Sync IssuesThe sync version also has errors about Related Files
|
|
@claude let's have the stores add their items to the exit stack and handle their own clients let's not pass clients back to the base store |
|
Claude finished @strawgate's task —— View job Refactoring exit stack pattern
|
Previously, stores returned clients via `_get_client_for_context()` and the base class managed them. Now stores directly register their cleanup callbacks with the exit stack via `_register_cleanup_callbacks(stack)`. This gives stores more control over their cleanup registration and simplifies the base class by removing an abstraction layer. Also updated codegen to map AsyncExitStack -> ExitStack and enter_async_context -> enter_context for sync code generation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
51-80: Critical: Ownership flag must be set before super().init() to prevent lifecycle bug.The current implementation computes
client_providedas a local variable (line 65) and passes it tosuper().__init__()(line 80). However, ifsuper().__init__()raises before setting_client_provided_by_user,__del__will usegetattr(line 183) which defaults toFalse, incorrectly attempting to close a user-provided database.This exact issue was flagged in past review comments but remains unaddressed.
- client_provided = db is not None - + self._client_provided_by_user = db is not None + if db: self._db = db elif path: ... self._is_closed = False - super().__init__(default_collection=default_collection, client_provided_by_user=client_provided) + super().__init__(default_collection=default_collection, client_provided_by_user=self._client_provided_by_user)And update
__del__:def __del__(self) -> None: - if not getattr(self, "_client_provided_by_user", False): + if not self._client_provided_by_user: try: # noqa: SIM105 self._close_and_flush() except (AttributeError, Exception): # noqa: S110 pass # Best-effort cleanup during finalizationkey-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
147-174: Set_client_provided_by_userbefore any code that can raise
client_providedis computed but never stored onselfuntilsuper().__init__()runs. If validation below raises (e.g., badtable_name), the destructor later runs with_client_provided_by_usermissing,getattr(..., False)falls back toFalse, and we incorrectly close the caller’s DuckDB connection. Persist the flag immediately after it’s computed so ownership is known even when__init__aborts early.client_provided = connection is not None + self._client_provided_by_user = client_provided @@ - super().__init__(default_collection=default_collection, seed=seed, client_provided_by_user=client_provided, stable_api=False) + super().__init__( + default_collection=default_collection, + seed=seed, + client_provided_by_user=client_provided, + stable_api=False, + )key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
144-173: Persist ownership flag before any potential failureSame risk here: if the initializer raises (bad
table_name, connect error, etc.) beforesuper().__init__()runs,_client_provided_by_usernever lands onself. During finalization thegetattr(..., False)fallback makes us treat a user-supplied connection as store-owned and close it. Store the flag onselfimmediately after you compute it.client_provided = connection is not None + self._client_provided_by_user = client_provided @@ - super().__init__( - default_collection=default_collection, - seed=seed, - client_provided_by_user=client_provided, - stable_api=False, - ) + super().__init__( + default_collection=default_collection, + seed=seed, + client_provided_by_user=client_provided, + stable_api=False, + )key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
141-149: Remove obsolete lifecycle methods; exit stack handles cleanup.Per PR objectives ("remove per-store aexit/close boilerplate"),
_closeand__del__are no longer needed when using the exit stack pattern. The base class__exit__andclosemethods now handle cleanup by closing the exit stack, which calls registered callbacks.Apply this diff:
def _sync_close(self) -> None: + """Close all disk caches.""" for cache in self._cache.values(): cache.close() - def _close(self) -> None: - self._sync_close() - - def __del__(self) -> None: - self._sync_close()Keep
_sync_closeas an implementation detail called by the cleanup callback registered in_register_cleanup_callbacks.
♻️ Duplicate comments (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
493-497: Critical: Exit stack leaks if aenter raises.If
_ensure_exit_stack()orsetup()raises, the exit stack is not cleaned up because Python's context manager protocol does not call__aexit__when__aenter__raises. This was flagged in past review comments but remains unaddressed.async def __aenter__(self) -> Self: - # Create exit stack and enter client context - await self._ensure_exit_stack() - await self.setup() + # Create exit stack and enter client context + try: + await self._ensure_exit_stack() + await self.setup() + except: + # Clean up exit stack if initialization fails + if self._exit_stack is not None: + await self._exit_stack.aclose() + self._exit_stack = None + raise return selfkey-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
127-130: Fragile hasattr pattern for _raw_client.Using
hasattr(self, "_raw_client")is fragile—if_raw_clientis ever explicitly set toNoneduring initialization, this check breaks. The past review comment (marked as addressed) suggested always initializing_raw_clientin__init__.In
__init__(around line 108), always define_raw_client:if client: self._client = client + self._raw_client = None else: session: Session = aioboto3.Session(...)Then simplify the check:
async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None: """Register DynamoDB client cleanup with the exit stack.""" - if hasattr(self, "_raw_client"): + if self._raw_client is not None: self._client = await stack.enter_async_context(self._raw_client)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
key-value/key-value-aio/src/key_value/aio/stores/base.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(3 hunks)scripts/build_sync_library.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(133-137)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(477-481)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (3)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(477-481)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
_register_cleanup_callbacks(458-473)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_register_cleanup_callbacks(197-199)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
_close_and_flush(91-95)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_register_cleanup_callbacks(127-130)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_register_cleanup_callbacks(197-199)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(507-511)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(477-481)
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_client(96-102)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_client(94-100)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (5)
_register_cleanup_callbacks(458-473)_ensure_exit_stack(475-491)setup(151-173)setup(513-523)close(507-511)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_register_cleanup_callbacks(127-130)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_register_cleanup_callbacks(197-199)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_register_cleanup_callbacks(195-197)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(133-137)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(507-511)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
_register_cleanup_callbacks(458-473)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_register_cleanup_callbacks(127-130)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_register_cleanup_callbacks(195-197)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (3)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(507-511)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_close_and_flush(85-89)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
_register_cleanup_callbacks(430-445)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (28)
scripts/build_sync_library.py (1)
227-227: LGTM! Mappings are correct.Both mappings correctly translate async context management constructs to their sync equivalents and support the AsyncExitStack-based lifecycle management introduced in this PR.
Also applies to: 242-242
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
104-107: Stable API flag wiring looks solid.
Routing thestable_api=Trueflag throughBaseContextManagerStorekeeps the lifecycle contract centralized without relying on private attributes.key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
55-83: Clear ownership semantics and safe teardown.
The docstring clarifies caller responsibilities, theclient_providedflag cleanly informs the base class, and_close()now idempotently disposes only store-managed clients.Also applies to: 168-171
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
53-87: RocksDB ownership handling aligns with the base contract.
Passingclient_provided_by_userkeeps cleanup centralized, while the defensive__del__ensures only store-owned databases are flushed and closed.Also applies to: 194-199
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
52-87: Disk cache lifecycle is now explicit and safe.
Early ownership flag assignment plus the guarded finalizer prevent accidental closure of caller-supplied caches while keeping store-managed caches cleaned up.Also applies to: 132-134
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
218-258: Elasticsearch client ownership clarified and respected.
The constructor flag cleanly tells the base class when to skip teardown, and_close()focuses solely on store-created clients.Also applies to: 555-556
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (2)
63-82: LGTM: Clean ownership semantics implementation.The lifecycle ownership changes are correctly implemented. The store now computes
client_providedand passes it to the base class, ensuring user-provided clients are not managed by the store.
155-156: LGTM: _close correctly delegates to base class contract.The
_closeimplementation unconditionally closes the client, relying on the base class to guard invocation based on ownership. This follows the template method pattern correctly.key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
105-124: LGTM: Ownership flag correctly computed and propagated.The
client_providedflag is properly derived and passed to the base class, ensuring lifecycle semantics are respected.key-value/key-value-aio/src/key_value/aio/stores/base.py (5)
89-123: LGTM: stable_api parameter correctly integrated.The
stable_apiparameter is properly stored and triggers a stability warning whenFalse. This provides clear user guidance about API stability.
430-456: LGTM: Clean ownership semantics in base class.The
BaseContextManagerStorenow explicitly tracks client ownership viaclient_provided_by_userand delegates lifecycle management to anAsyncExitStack. The docstring clearly documents the contract for subclasses.
458-491: LGTM: Exit stack pattern enables centralized cleanup.The
_register_cleanup_callbackshook and_ensure_exit_stackmethod provide a clean extension point for stores to register cleanup logic. The pattern of only registering callbacks whenclient_provided_by_user=Falsecorrectly implements ownership semantics.
499-511: LGTM: Exit stack cleanup correctly implemented.Both
__aexit__andclose()properly clean up the exit stack when it exists, setting it toNoneafterward to prevent double-cleanup.
513-523: LGTM: setup() ensures exit stack for non-context-manager usage.Calling
_ensure_exit_stack()insetup()ensures stores work correctly even when not used withasync with, enabling imperative usage patterns while maintaining proper lifecycle management.key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (2)
57-95: LGTM: Ownership semantics correctly implemented.The Redis store properly computes
client_providedand forwards it to the base class, maintaining consistency with other stores in this PR.
225-226: LGTM: _close delegates to base class contract.The method unconditionally closes the client, correctly relying on the base class to guard invocation based on ownership.
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (2)
163-194: LGTM: Clean ownership implementation.The MongoDB store correctly derives
client_providedand passes it to the base class. The docstring clearly documents that user-provided clients are not managed.
197-199: LGTM: Direct exit stack integration without fragile checks.Unlike the DynamoDB store, this implementation directly enters the client context without
hasattrchecks. The client is always defined by this point, making the code more robust.key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (2)
190-230: LGTM: Ownership semantics consistent with async stores.The sync Elasticsearch store correctly implements the same ownership pattern as async stores, computing
client_providedand forwarding it to the base class.
481-482: LGTM: _close unconditionally closes client.Following the base class contract, the method relies on the base class to guard invocation based on ownership.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (2)
60-88: LGTM: Consistent ownership implementation.The sync Redis store follows the same pattern as its async counterpart, correctly propagating client ownership to the base class.
212-213: LGTM: _close follows base class contract.The method unconditionally closes the client, delegating ownership checks to the base class.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
183-187: Best-effort cleanup pattern is appropriate for del.The try/except wrapper in
__del__correctly implements best-effort cleanup, suppressing exceptions that could occur during finalization when attributes may be partially initialized.However, this should be updated to use direct attribute access once the critical issue above is fixed.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (5)
95-95: LGTM: stable_api parameter addition.Clean addition of the stability flag with appropriate warning when False. The initialization and storage are straightforward.
Also applies to: 108-126
403-428: LGTM: client ownership model well documented.The docstring clearly explains the ownership semantics and when stores should pass
client_provided_by_user=True. Constructor properly initializes both the ownership flag and exit stack.
447-463: LGTM: exit stack lifecycle correctly implemented.The lazy initialization of the exit stack and conditional cleanup registration based on
client_provided_by_usercorrectly implement the ownership model.
465-481: LGTM: context manager protocol correctly delegates to exit stack.Both
__enter__/__exit__andclose()properly delegate lifecycle management to the exit stack, centralizing cleanup as intended by the PR.
483-493: LGTM: setup override ensures exit stack initialization.This ensures stores work correctly even when not used as context managers, by guaranteeing the exit stack and client context entry happen before
_setup()is called.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py
Show resolved
Hide resolved
Test Failure AnalysisSummary: Tests are failing on Windows with Root Cause: The recent refactoring (commit 3b465db) changed the lifecycle management pattern from calling
The stores still have their
Suggested Solution: For each of the 7 affected stores, add a Example for RocksDB ( async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None:
"""Register RocksDB cleanup with the exit stack."""
stack.push_async_callback(self._close)Example for DuckDB ( async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None:
"""Register DuckDB connection cleanup with the exit stack."""
stack.push_async_callback(self._close)After adding these methods to all 7 stores, run Detailed AnalysisError PatternAll failing tests show the same pattern: This happens during test teardown when pytest's What ChangedThe refactoring in commit 3b465db changed Before: async def __aexit__(self, exc_type, exc_value, traceback) -> None:
await self._close() # Calls _close() directlyAfter: async def __aexit__(self, exc_type, exc_value, traceback) -> None:
if self._exit_stack is not None:
await self._exit_stack.aclose() # Closes exit stack, which calls registered callbacksStores must now register their cleanup via MongoDB ( async def _register_cleanup_callbacks(self, stack: AsyncExitStack) -> None:
"""Register MongoDB client cleanup with the exit stack."""
await stack.enter_async_context(self._client)But the other 7 stores were not updated. Test Fixture BehaviorThe test fixture
Both paths fail because neither properly triggers cleanup in the affected stores. Related FilesBase Class:
Stores Needing Updates:
Reference Implementations:
|
|
@claude i dont want abstract methods on base for registering cleanup or getting the client, i just want base to provide the async exit stack and to handle exiting it on _close and aexit, the stores will put things into the exit stack if they need to. We should initialize the exit stack in the base init |
|
Claude finished @strawgate's task —— View job Refactoring Complete ✅I've successfully refactored the exit stack lifecycle management pattern as requested: Changes MadeBaseContextManagerStore Refactoring:
Store Updates:
Build Status
CommitPushed commit The implementation now follows the simplified pattern where stores directly manage adding things to the exit stack if needed, and the base class just handles closing it. |
Simplified the BaseContextManagerStore lifecycle management pattern by: - Removing abstract _register_cleanup_callbacks method - Initializing AsyncExitStack in __init__ instead of lazily - Allowing stores to directly register cleanup callbacks via _exit_stack - Stores now call _exit_stack.push_async_callback() or _exit_stack.callback() in their _setup() method to register cleanup operations This gives stores direct control over exit stack management while keeping the base class responsible for entering and exiting the stack. Updated all stores that inherit from BaseContextManagerStore: - MongoDB, DynamoDB: register client context manager - Redis, Valkey, Memcached, Elasticsearch: register async close callbacks - RocksDB, Disk, MultiDisk, DuckDB: register sync close callbacks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Test Failure AnalysisSummary: The static analysis is failing because the codegen script is not translating Root Cause: The latest commit (b7d0949) successfully refactored stores to register cleanup callbacks directly with the exit stack. However, the Suggested Solution: Add a mapping to the codegen script to translate Files to modify:
Add this line to the "push_async_callback": "callback",After making this change:
Detailed AnalysisError MessagesFrom the failed workflow run: Plus similar errors for Redis and Memcached stores. What ChangedThe refactoring correctly implemented the exit stack pattern in async stores:
The Codegen GapThe
Python Context Manager APIFrom the Python docs:
The synchronous equivalent of Affected StoresAll stores using
Related FilesCodegen Script:
Async Stores (correct):
Generated Sync Stores (incorrect, will be fixed after codegen):
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
131-139: Fix partial‑initialization bug: DuckDBStore may close user‑provided connections on failed init.For normal operation the ownership semantics are correct, but there is a subtle edge case when a caller passes an existing DuckDB connection:
client_providedis computed,_connectionand_is_closedare set, thensuper().__init__(..., client_provided_by_user=client_provided, ...)is called.- If
super().__init__raises before assigning_client_provided_by_user, the instance is only partially initialized.- During object finalization,
__del__sees_is_closed == False,_connectionpresent, andgetattr(self, "_client_provided_by_user", False)returning the defaultFalse, so it closes the connection even though it was supplied (and still owned) by the caller.To make this exception‑safe and consistent with
DiskStore, eagerly set the instance flag before any code that might raise and before the base initializer:@@ - if connection is not None and database_path is not None: + if connection is not None and database_path is not None: msg = "Provide only one of connection or database_path" raise ValueError(msg) - client_provided = connection is not None + client_provided = connection is not None + # Ensure ownership information is available even if initialization fails partway + self._client_provided_by_user = client_provided if connection is not None: self._connection = connection @@ - super().__init__( - default_collection=default_collection, - seed=seed, - client_provided_by_user=client_provided, - stable_api=False, - ) + super().__init__( + default_collection=default_collection, + seed=seed, + client_provided_by_user=client_provided, + stable_api=False, + )This preserves the existing behavior for fully initialized instances while ensuring that user‑provided connections are never closed by a partially constructed store.
Also applies to: 144-145, 168-173, 266-275, 375-383
♻️ Duplicate comments (4)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
71-72: Remove redundant assignment.Line 72 sets
self._client_provided_by_userbut the base class__init__at line 86 will set this attribute from theclient_provided_by_userparameter. The assignment is redundant and should be removed.Apply this diff:
client_provided = disk_cache is not None - self._client_provided_by_user = client_providedKeep the
client_providedlocal variable for passing tosuper().__init__()at line 86.key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
51-56: Still possible to close user-provided RocksDB on constructor failureThis reintroduces the ownership bug noted in a prior review:
client_providedis only a local variable (Line [65]) passed tosuper().__init__(Line [80]), while__del__(Lines [186]-[191]) usesgetattr(self, "_client_provided_by_user", False).If
dbis user-provided andBaseStore.__init__raises afterself._dbis set but before it stores_client_provided_by_user,getattr(..., False)will returnFalse, and__del__will incorrectly call_close_and_flush()on the caller’s database.To make ownership robust even when
super().__init__fails, persist the flag onselfbefore any code that might raise, and have__del__read it directly:def __init__(self, *, db: Rdict | None = None, path: Path | str | None = None, default_collection: str | None = None) -> None: """Initialize the RocksDB store. @@ - if db is not None and path is not None: + self._client_provided_by_user = db is not None + + if db is not None and path is not None: msg = "Provide only one of db or path" raise ValueError(msg) @@ - self._is_closed = False - - super().__init__(default_collection=default_collection, client_provided_by_user=client_provided) + self._is_closed = False + + super().__init__( + default_collection=default_collection, + client_provided_by_user=self._client_provided_by_user, + ) @@ def __del__(self) -> None: - if not getattr(self, "_client_provided_by_user", False): + if not self._client_provided_by_user: try: # noqa: SIM105 self._close_and_flush() except (AttributeError, Exception): # noqa: S110 pass # Best-effort cleanup during finalizationThis way
__del__will never close a user-providedRdict, even if base-class initialization fails.Also applies to: 65-65, 80-80, 186-191
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
95-97: DuckDBStore can still close user-supplied connections on constructor failureThe docstrings (Lines [95]-[97] and [135]-[137]) promise that user-provided connections are not managed by the store. However, as with RocksDB,
client_providedis only local (Line [147]) and__del__(Lines [323]-[327]) checksgetattr(self, "_client_provided_by_user", False).If a caller passes a
connectionandBaseStore.__init__raises afterself._connectionis assigned but before_client_provided_by_useris stored,getattr(..., False)returnsFalseand__del__may close the caller’s connection, violating the ownership contract.You can make this safe by eagerly persisting the flag and relying on it in
__del__:def __init__( @@ - """ - if connection is not None and database_path is not None: + """ + self._client_provided_by_user = connection is not None + + if connection is not None and database_path is not None: msg = "Provide only one of connection or database_path" raise ValueError(msg) @@ - client_provided = connection is not None - @@ - super().__init__(default_collection=default_collection, seed=seed, client_provided_by_user=client_provided, stable_api=False) + super().__init__( + default_collection=default_collection, + seed=seed, + client_provided_by_user=self._client_provided_by_user, + stable_api=False, + ) @@ def __del__(self) -> None: """Clean up the DuckDB connection on deletion.""" try: if ( not getattr(self, "_is_closed", False) - and (not getattr(self, "_client_provided_by_user", False)) + and not self._client_provided_by_user and hasattr(self, "_connection") ): self._connection.close() self._is_closed = TrueThis ensures that user-managed connections are never closed by the store, even when the constructor fails part-way through.
Also applies to: 135-137, 147-147, 170-170, 323-327
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
466-470: Critical: Exit stack leaks if__aenter__raises.If
_ensure_exit_stack_enteredorsetupraises after the exit stack is entered, the exit stack will not be cleaned up because Python's context manager protocol does not call__aexit__when__aenter__raises.Apply this diff to clean up the exit stack on exception:
async def __aenter__(self) -> Self: - # Enter the exit stack - await self._ensure_exit_stack_entered() - await self.setup() + # Enter the exit stack + try: + await self._ensure_exit_stack_entered() + await self.setup() + except: + # Clean up exit stack if initialization fails + if self._exit_stack_entered: + await self._exit_stack.aclose() + self._exit_stack_entered = False + raise return self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
key-value/key-value-aio/src/key_value/aio/stores/base.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(6 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(6 hunks)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(6 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
_setup(127-128)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_setup(90-93)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (2)
_setup(83-87)_close_and_flush(89-93)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_client(100-106)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(450-454)
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
_setup(127-128)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_setup(90-93)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (2)
_setup(110-112)_sync_close(146-148)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (6)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (3)
_setup(133-134)close(450-454)_delete_store(500-502)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_setup(89-92)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
_setup(233-240)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_setup(194-197)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_setup(83-87)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
_setup(84-95)_client(98-104)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (7)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_setup(127-128)close(480-484)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_setup(90-93)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
_setup(85-88)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_setup(196-199)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
_setup(222-225)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (2)
_setup(86-97)_client(100-106)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
_setup(233-240)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
_setup(133-134)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_setup(89-92)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (3)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_setup(127-128)close(480-484)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_setup(89-92)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (7)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
_setup(127-128)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_setup(90-93)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_setup(133-173)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
_setup(261-268)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
_setup(85-88)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
_setup(222-225)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_setup(194-197)
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (2)
_client(100-106)_setup(86-97)key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_setup(127-128)close(480-484)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
_setup(222-225)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
close_connection(238-241)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(480-484)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(450-454)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
_client(100-106)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
close_connection(269-272)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(450-454)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (6)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
disk_cache(31-32)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
disk_cache(34-35)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
_setup(221-252)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_setup(194-197)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_setup(83-87)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
close(450-454)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (6)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
_setup(133-134)close(450-454)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_setup(89-92)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_setup(194-197)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
_setup(209-212)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_setup(83-87)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
_setup(84-95)_client(98-104)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (4)
_ensure_exit_stack_entered(432-436)setup(157-179)setup(456-465)close(450-454)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (5)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
_setup(133-134)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_setup(89-92)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
_setup(233-240)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
_setup(83-87)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (2)
_setup(84-95)_client(98-104)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (4)
_ensure_exit_stack_entered(460-464)setup(151-173)setup(486-495)close(480-484)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
_client(98-104)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
close(480-484)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (2)
_setup(89-93)_close_and_flush(95-99)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_setup(89-92)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
_setup(221-252)
key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (8)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_setup(127-128)_delete_store(530-532)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
_setup(90-93)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_setup(133-173)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
_setup(261-268)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (2)
_setup(85-88)_delete_store(157-159)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_setup(196-199)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (2)
_setup(86-97)_client(100-106)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (2)
_setup(209-212)_delete_store(215-216)
🪛 GitHub Actions: Run Tests
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
[error] 236-236: pyright: Type of "push_async_callback" is unknown (reportUnknownMemberType)
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py
[failure] 95-95:
Cannot access attribute "push_async_callback" for class "ExitStack[bool | None]"
Attribute "push_async_callback" is unknown (reportAttributeAccessIssue)
[failure] 95-95:
Type of "push_async_callback" is unknown (reportUnknownMemberType)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
[failure] 236-236:
Cannot access attribute "push_async_callback" for class "ExitStack[bool | None]"
Attribute "push_async_callback" is unknown (reportAttributeAccessIssue)
[failure] 236-236:
Type of "push_async_callback" is unknown (reportUnknownMemberType)
🔇 Additional comments (28)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
160-197: LGTM! Lifecycle ownership correctly implemented.The client ownership detection and propagation to the base class is correct. The
_setupmethod properly usesenter_contextfor the sync MongoClient context manager, ensuring cleanup only for store-owned clients.key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
104-112: LGTM! Correct use of synchronous callback.The
_setupmethod correctly usescallback()(notpush_async_callback()) to register the synchronous_sync_closemethod, which is appropriate since the cleanup routine is not async.key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
63-88: LGTM! Async lifecycle correctly implemented.The client ownership detection and async cleanup registration via
push_async_callbackare correct for the async aiomcache client.key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
54-93: LGTM! Correct use of synchronous callback for sync cleanup.The
_setupmethod correctly usescallback()to register the synchronous_close_and_flushmethod. The ownership detection and propagation are correct.key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
162-199: LGTM! Async context manager correctly entered.The
_setupmethod correctly usesenter_async_contextfor the AsyncMongoClient, ensuring proper async lifecycle management for store-owned clients.key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (2)
57-95: LGTM! Client ownership correctly tracked.The client ownership detection and propagation to the base class with
stable_api=Trueare correct.
222-225: LGTM! Async cleanup correctly registered.The
_setupmethod correctly usespush_async_callbackto register the asyncaclose()method for store-owned Redis clients.key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
88-92: LGTM! Cleanup correctly registered.The
_setupmethod correctly usescallback()to register the synchronousclose()method on the diskcache Cache for store-owned caches.key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
52-55: Ownership tracking and cleanup wiring look correct.The disk cache ownership flag is computed once, stored on the instance before
super().__init__, and then used consistently in_setupand__del__so that only store-created caches are registered on the exit stack or closed in the finalizer. This matches the new “caller‑owns provided client” contract and is exception‑safe for partial initialization.Also applies to: 68-70, 83-87, 89-93, 135-137
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
107-112: Exit‑stack integration for cache cleanup is aligned with the new model.Registering
_sync_closeon the base exit stack ensures all per‑collection caches are torn down when the store is closed, while keeping ownership unambiguous (the store always owns the caches). The change cleanly centralizes lifecycle into the base context‑manager flow.key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
217-220: Elasticsearch client ownership and teardown are correctly separated.The constructor cleanly distinguishes between user‑provided and internally created clients, forwards that via
client_provided_by_user, and_setuponly registers an async close callback when the store owns the client. That satisfies the “you create it, you manage it” requirement without leaking internally created clients.Also applies to: 232-233, 253-258, 260-265
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
55-67: Valkey client lifecycle handling matches the shared ownership model.The store correctly treats a passed‑in
BaseClientas caller‑owned (no exit‑stack cleanup) while creating, tracking, and registering cleanup for its own client instances in_setup. Theclient_provided_by_userflag is the single source of truth, which keeps the ownership semantics clear and consistent with other stores.Also applies to: 68-69, 79-83, 85-98
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
57-81: Ownership flag wiring and docstring match new semanticsThe docstring and the
client_providedflag (Line [70]) align with the new “user-provided clients are not closed by the store” contract, and forwardingclient_provided_by_userplusstable_api=Trueviasuper().__init__(Line [81]) cleanly centralizes ownership in the base class. No changes needed here.key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
60-88: Client ownership semantics are wired correctly for RedisStoreUsing
client_provided = client is not None(Line [70]) and passing it asclient_provided_by_userintosuper().__init__(Line [88]) matches the documented behavior in Lines [60]-[62]: user-provided clients are not closed by the store, while internally created clients are managed via the base class. This is consistent with the new lifecycle model.key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (2)
7-7:overloadimport is appropriateImporting
overloadat Line [7] matches the new constructor overloads and keeps the typing story clear. No changes needed.
83-87: Exit-stack registration for RocksDB cleanup looks correct
_setup()(Lines [83]-[87]) registers_close_and_flushviaself._exit_stack.callback(...)only when the store owns the database (not self._client_provided_by_user). Combined with the_is_closedguard in_close_and_flush, this gives idempotent, centrally managed cleanup and avoids closing user-managed databases on normal shutdown.key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
235-243: Exit-stack-based connection cleanup is correctThe
_setup()block at Lines [235]-[243] correctly registers aclose_connectioncallback on the exit stack only when the store owns the connection (not self._client_provided_by_user), and uses_is_closedto make the close idempotent. This aligns with the centralized cleanup pattern used in other sync stores.key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
5-5: Ownership semantics and base wiring for user-provided DynamoDB client look goodImporting
override(Line [5]) and the updated constructor (Lines [92]-[95]) clearly state that a suppliedclientis already entered and will not be managed by the store. Computingclient_provided = client is not None(Line [104]) and passing it asclient_provided_by_userintosuper().__init__(Lines [120]-[123]) is consistent with that contract, and the comment at Line [247] correctly reflects that cleanup is handled via the base/exit stack rather than a custom_close. This matches the intended lifecycle model.Also applies to: 92-95, 104-104, 120-123, 247-247
135-137:_setup()correctly usesenter_async_contextonly for owned clientsThe
_setup()logic (Lines [135]-[137]) enters the aioboto3 raw client viaself._exit_stack.enter_async_context(...)only whennot self._client_provided_by_user. That ensures:
- Internally created clients are both entered and cleaned up through the async exit stack.
- User-provided clients are never entered or closed by the store.
This resolves prior double-
__aexit__concerns and centralizes cleanup in the base class.key-value/key-value-aio/src/key_value/aio/stores/base.py (5)
9-9: LGTM: AsyncExitStack import is appropriate for async lifecycle management.
89-89: LGTM: stable_api parameter is well-implemented with appropriate defaults and warnings.The conservative default (False) and automatic warning mechanism provide clear stability signals to users.
Also applies to: 102-103, 120-123
431-444: LGTM: Clear documentation of ownership semantics and lifecycle management.The docstring effectively communicates the "you create it, you manage it" pattern described in the PR objectives.
446-464: LGTM: Constructor and exit stack initialization are correct.The idempotent
_ensure_exit_stack_enteredproperly tracks state and safely enters the exit stack once.
472-495: LGTM: Exit stack cleanup and setup integration are correct.Both
__aexit__andcloseproperly handle exit stack teardown, and thesetupoverride ensures the exit stack is available for stores to register cleanup callbacks during_setup.key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (4)
11-11: LGTM: Correct sync equivalent (ExitStack) is now used.Past review issues regarding AsyncExitStack in sync code have been properly addressed.
95-95: LGTM: stable_api implementation mirrors async version correctly.Also applies to: 108-109, 126-129
403-416: LGTM: Sync context manager base class properly documented.
418-436: LGTM: Sync lifecycle methods correctly implement exit stack management.All methods use proper synchronous equivalents and the setup override correctly enables stores to register cleanup during
_setup.Also applies to: 444-465
| def __enter__(self) -> Self: | ||
| # Enter the exit stack | ||
| self._ensure_exit_stack_entered() | ||
| self.setup() | ||
| return self |
There was a problem hiding this comment.
Critical: Exit stack leaks if __enter__ raises.
If _ensure_exit_stack_entered or setup raises after the exit stack is entered, the exit stack will not be cleaned up because Python's context manager protocol does not call __exit__ when __enter__ raises.
Apply this diff to clean up the exit stack on exception:
def __enter__(self) -> Self:
- # Enter the exit stack
- self._ensure_exit_stack_entered()
- self.setup()
+ # Enter the exit stack
+ try:
+ self._ensure_exit_stack_entered()
+ self.setup()
+ except:
+ # Clean up exit stack if initialization fails
+ if self._exit_stack_entered:
+ self._exit_stack.close()
+ self._exit_stack_entered = False
+ raise
return self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __enter__(self) -> Self: | |
| # Enter the exit stack | |
| self._ensure_exit_stack_entered() | |
| self.setup() | |
| return self | |
| def __enter__(self) -> Self: | |
| # Enter the exit stack | |
| try: | |
| self._ensure_exit_stack_entered() | |
| self.setup() | |
| except: | |
| # Clean up exit stack if initialization fails | |
| if self._exit_stack_entered: | |
| self._exit_stack.close() | |
| self._exit_stack_entered = False | |
| raise | |
| return self |
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py around
lines 438 to 442, __enter__ currently calls self._ensure_exit_stack_entered()
then self.setup() and returns self; if either call raises after the exit stack
is entered the stack leaks. Modify __enter__ to enter the exit stack and then
wrap the subsequent setup/return in a try/except that closes/cleans up the exit
stack on any exception (call the ExitStack cleanup method) before re-raising the
error so the stack is not leaked.
| def _setup(self) -> None: | ||
| # Register client cleanup if we own the client | ||
| if not self._client_provided_by_user: | ||
| self._exit_stack.push_async_callback(self._client.close) |
There was a problem hiding this comment.
Critical: push_async_callback does not exist on sync ExitStack.
This is a sync store (in key-value-sync) but line 236 calls push_async_callback, which only exists on AsyncExitStack. This will cause an AttributeError at runtime.
The Elasticsearch client's close() method should be synchronous in the sync version. Apply this diff:
@override
def _setup(self) -> None:
# Register client cleanup if we own the client
if not self._client_provided_by_user:
- self._exit_stack.push_async_callback(self._client.close)
+ self._exit_stack.callback(self._client.close)
cluster_info = self._client.options(ignore_status=404).info()Alternatively, if the Elasticsearch client is a context manager, use:
- self._exit_stack.push_async_callback(self._client.close)
+ self._exit_stack.enter_context(self._client)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _setup(self) -> None: | |
| # Register client cleanup if we own the client | |
| if not self._client_provided_by_user: | |
| self._exit_stack.push_async_callback(self._client.close) | |
| def _setup(self) -> None: | |
| # Register client cleanup if we own the client | |
| if not self._client_provided_by_user: | |
| self._exit_stack.callback(self._client.close) |
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] 236-236: pyright: Type of "push_async_callback" is unknown (reportUnknownMemberType)
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
[failure] 236-236:
Cannot access attribute "push_async_callback" for class "ExitStack[bool | None]"
Attribute "push_async_callback" is unknown (reportAttributeAccessIssue)
[failure] 236-236:
Type of "push_async_callback" is unknown (reportUnknownMemberType)
🤖 Prompt for AI Agents
In
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
around lines 233 to 236, the sync store incorrectly calls
self._exit_stack.push_async_callback (which only exists on AsyncExitStack) and
will raise AttributeError at runtime; replace that call with a synchronous
registration (e.g., self._exit_stack.callback(self._client.close)) so the
client's close() is registered properly on the sync ExitStack, or if the
Elasticsearch client supports context management use
self._exit_stack.enter_context(self._client) instead.
| def _setup(self) -> None: | ||
| """Register client cleanup if we own the client.""" | ||
| if not self._client_provided_by_user: | ||
| self._exit_stack.push_async_callback(self._client.close) | ||
|
|
||
| @override | ||
| def _close(self) -> None: | ||
| self._client.close() | ||
| def _delete_store(self) -> bool: | ||
| return self._client.flushdb() # pyright: ignore[reportUnknownMemberType, reportAny] |
There was a problem hiding this comment.
🧩 Analysis chain
Replace push_async_callback with callback in _setup()
As in the Valkey sync store, Line [212] calls self._exit_stack.push_async_callback(self._client.close) on a sync ExitStack, which does not support that method and will fail at type-check or runtime. Use a synchronous callback instead:
@override
def _setup(self) -> None:
"""Register client cleanup if we own the client."""
- if not self._client_provided_by_user:
- self._exit_stack.push_async_callback(self._client.close)
+ if not self._client_provided_by_user:
+ self._exit_stack.callback(self._client.close)The _delete_store implementation (Lines [215]-[216]) is fine: flushing the DB is a logical store operation and independent of client ownership.
🏁 Script executed:
#!/bin/bash
# Check the imports and ExitStack usage in the Redis store file
grep -n "ExitStack\|exit_stack\|import" key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py | head -20Length of output: 796
🏁 Script executed:
#!/bin/bash
# View the _setup method and surrounding context
sed -n '205,220p' key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.pyLength of output: 451
🏁 Script executed:
#!/bin/bash
# Check if there's an async variant of ExitStack being imported
rg -n "AsyncExitStack|contextlib" key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.pyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the class definition to see where _exit_stack is initialized
rg -n "_exit_stack\s*=" key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py -A 2 -B 2Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and inspect the base store file
find . -name "base.py" -path "*/sync/code_gen/stores/*" | head -5
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Check the BaseStore/base class for _exit_stack definition
rg -n "_exit_stack|ExitStack" key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py -B 2 -A 2 | head -40Length of output: 1579
🏁 Script executed:
#!/bin/bash
# Check imports in base.py
rg -n "^from|^import" key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py | head -20Length of output: 951
---
🌐 Web query:
```web
Python contextlib ExitStack callback vs push_async_callback methods
💡 Result:
Short answer — purpose and behavior:
-
ExitStack.callback(callback, *args, **kwargs)
- Registers a plain callable (or coroutine-capable wrapper if used in AsyncExitStack?) to be called with the provided args when the stack is closed.
- Callbacks registered this way do NOT receive exception information and therefore cannot suppress exceptions. Use for simple cleanup actions. [1]
-
ExitStack.push(exit) / push_async_exit(...)
- push() registers an object’s exit method (or any callable with the same signature) so it will be called with the usual (exc_type, exc, tb) arguments when unwinding. Those callbacks CAN inspect or suppress exceptions by returning a true value. For AsyncExitStack use push_async_exit for awaitable exits. [1][2]
-
AsyncExitStack.push_async_callback(callback, *args, **kwargs)
- The async equivalent of callback(): it expects a coroutine function and will await it during stack teardown. Like callback(), it does not get exception details and cannot suppress exceptions. Use it for async cleanup tasks that don’t need exception info. [2]
When to use which:
- If your cleanup needs to know about or suppress an exception -> register an exit-style callback (push / push_async_exit).
- If your cleanup is a simple function/coroutine that just runs and doesn’t need exception info -> use callback / push_async_callback.
References:
- ExitStack and callback/push documentation. [1]
- AsyncExitStack, push_async_exit and push_async_callback documentation. [2]
Replace push_async_callback with callback in _setup()
The sync ExitStack from contextlib does not have a push_async_callback method. Line 212 incorrectly calls self._exit_stack.push_async_callback(self._client.close) on a synchronous ExitStack, which will raise an AttributeError at runtime. Use the synchronous callback method instead:
@override
def _setup(self) -> None:
"""Register client cleanup if we own the client."""
if not self._client_provided_by_user:
- self._exit_stack.push_async_callback(self._client.close)
+ self._exit_stack.callback(self._client.close)The _delete_store implementation (lines 215–216) is correct.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _setup(self) -> None: | |
| """Register client cleanup if we own the client.""" | |
| if not self._client_provided_by_user: | |
| self._exit_stack.push_async_callback(self._client.close) | |
| @override | |
| def _close(self) -> None: | |
| self._client.close() | |
| def _delete_store(self) -> bool: | |
| return self._client.flushdb() # pyright: ignore[reportUnknownMemberType, reportAny] | |
| def _setup(self) -> None: | |
| """Register client cleanup if we own the client.""" | |
| if not self._client_provided_by_user: | |
| self._exit_stack.callback(self._client.close) | |
| @override | |
| def _delete_store(self) -> bool: | |
| return self._client.flushdb() # pyright: ignore[reportUnknownMemberType, reportAny] |
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py
around lines 209-216, the sync ExitStack is incorrectly using
push_async_callback which doesn't exist; replace the call
self._exit_stack.push_async_callback(self._client.close) with the synchronous
ExitStack API call self._exit_stack.callback(self._client.close) (ensuring you
register the sync close method directly), leaving the _delete_store
implementation unchanged.
| # Register client cleanup if we own the client | ||
| if not self._client_provided_by_user: | ||
| self._exit_stack.push_async_callback(self._client.close) |
There was a problem hiding this comment.
🧩 Analysis chain
push_async_callback is invalid on sync ExitStack; use callback instead
BaseStore.close() operates on a synchronous ExitStack (it calls .close()), and static analysis already flags Line [95] because ExitStack has no push_async_callback. At runtime this will either raise AttributeError or never invoke the registered cleanup.
Use a standard synchronous callback, consistent with other sync stores:
- if not self._client_provided_by_user:
- self._exit_stack.push_async_callback(self._client.close)
+ if not self._client_provided_by_user:
+ self._exit_stack.callback(self._client.close)This preserves the “only close internally created clients” behavior while fixing the type/runtime error.
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify the file exists and inspect the problematic lines
find . -type f -name "store.py" | grep -E "valkey|key-value" | head -5
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Inspect the exact file and lines mentioned
cat "key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py" | sed -n '85,105p'Length of output: 805
🏁 Script executed:
#!/bin/bash
# Find the BaseStore class and check ExitStack type
rg -n "class BaseStore|_exit_stack|ExitStack" "key-value/key-value-sync/src/key_value/sync/code_gen/stores/" -A2 -B2 --type py | head -40Length of output: 3892
🏁 Script executed:
#!/bin/bash
# Check what ExitStack is imported (sync vs async)
rg -n "from contextlib|import.*ExitStack" "key-value/key-value-sync/src/key_value/sync/code_gen/stores/" --type pyLength of output: 103
---
🏁 Script executed:
```shell
#!/bin/bash
# Check BaseStore.close() implementation
rg -n "def close|def __exit__" "key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py" -A5 --type py
Length of output: 597
🏁 Script executed:
#!/bin/bash
# Check GlideClient.close() signature in Valkey store
rg -n "GlideClient|\.close" "key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py" -B2 -A2 --type pyLength of output: 1190
🏁 Script executed:
#!/bin/bash
# Check other sync stores for callback pattern (not push_async_callback)
rg -n "push_async_callback|\.callback\(" "key-value/key-value-sync/src/key_value/sync/code_gen/stores/" --type pyLength of output: 998
Fix push_async_callback to callback on sync ExitStack
The bug is confirmed. BaseStore.close() (line 450) calls self._exit_stack.close() synchronously, but Valkey (and Redis/Elasticsearch stores) incorrectly use push_async_callback, which doesn't exist on sync ExitStack—this will raise AttributeError at runtime.
Other sync stores (RocksDB, DuckDB, Disk) correctly use .callback(). Apply the suggested fix:
- self._exit_stack.push_async_callback(self._client.close)
+ self._exit_stack.callback(self._client.close)Note: Redis and Elasticsearch stores have the identical bug at their respective lines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Register client cleanup if we own the client | |
| if not self._client_provided_by_user: | |
| self._exit_stack.push_async_callback(self._client.close) | |
| # Register client cleanup if we own the client | |
| if not self._client_provided_by_user: | |
| self._exit_stack.callback(self._client.close) |
🧰 Tools
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
[failure] 95-95:
Cannot access attribute "push_async_callback" for class "ExitStack[bool | None]"
Attribute "push_async_callback" is unknown (reportAttributeAccessIssue)
[failure] 95-95:
Type of "push_async_callback" is unknown (reportUnknownMemberType)
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py
around lines 93 to 95, the code registers client cleanup with
self._exit_stack.push_async_callback(self._client.close) but BaseStore.close()
uses a synchronous ExitStack so push_async_callback does not exist; replace
push_async_callback with callback to register the sync close method (i.e., use
self._exit_stack.callback(self._client.close)), and apply the same change to the
equivalent lines in the Redis and Elasticsearch sync store implementations to
prevent AttributeError at runtime.
Stores now only manage the lifecycle of clients they create internally.
When a user provides a client to a store constructor, the store no longer
enters/exits the client's context manager or closes it.
This enables:
Closes #232
Generated with Claude Code) | View job run | [View branch](https://github.com/strawgate/py-key-value/tree/claude/issue-232-20251110-1652
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests