Review Client Ownership

[strawgate]

We have an established pattern where store constructors can take a client directly or the parameters required for us to create a client.

Currently we "own" the client once it's passed to us. We should consider requiring the user to handle setup/teardown of clients they provide. This means we would not enter the client context manager, etc.

We need to establish the pros/cons of this approach, we can ignore that it's a breaking change for now as the stores are not yet stable.

[strawgate]

@claude We have an established pattern where store constructors can take a client directly or the parameters required for us to create a client.

Currently we "own" the client once it's passed to us. We should consider requiring the user to handle setup/teardown of clients they provide. This means we would not enter the client context manager, etc.

We need to establish the pros/cons of this approach, we can ignore that it's a breaking change for now as the stores are not yet stable.

[claude]

Claude finished @strawgate's task —— View job

---

Client Ownership Analysis

I've analyzed the current client ownership pattern across all store implementations. Here's my comprehensive assessment:

Current Implementation

Pattern:

• Stores accept either a client directly OR parameters to create a client
• Once passed, stores "own" the client regardless of who created it
• For BaseContextManagerStore implementations:
  • When user provides a client: Store still enters/exits the client's context manager in __aenter__/__aexit__
  • When store creates a client: Store manages full lifecycle
  • The _close() method is called on exit, closing the client

Example from RedisStore:

def __init__(self, *, client: Redis | None = None, url: str | None = None, ...):
    if client:
        self._client = client  # User-provided client
    elif url:
        self._client = Redis(...)  # Store creates client
    else:
        self._client = Redis(...)  # Store creates client

async def _close(self) -> None:
    await self._client.aclose()  # Closes client regardless of who created it

Affected Stores:

• Redis, Valkey, MongoDB, DynamoDB, Memcached, Disk, DuckDB, RocksDB, MultiDiskStore, Elasticsearch

---

Proposed Change

New Pattern:

• If user provides a client: Store does NOT manage its lifecycle (no context manager entry, no close)
• If store creates a client: Store fully manages lifecycle (setup, context, teardown)

---

Pros & Cons Analysis

✅ Pros of Proposed Approach (User Manages External Clients)

1. Clear Ownership Semantics

   • Follows "you create it, you manage it" principle
   • Aligns with Python resource management best practices
   • Reduces confusion about who is responsible for cleanup

2. Prevents Double-Context Issues

   • Currently, if a user enters a client's context and passes it to store, the store enters it again
   • Example issue:

     async with Redis(...) as client:
         store = RedisStore(client=client)
         async with store:  # Double context entry!
             ...

   • This can cause issues with some client libraries

3. Enables Client Reuse

   • Users can share one client across multiple stores
   • Reduces connection overhead
   • Better for connection pooling
   • Example:

     async with Redis(...) as client:
         store1 = RedisStore(client=client, default_collection="cache1")
         store2 = RedisStore(client=client, default_collection="cache2")
         # Both stores use same connection pool

4. Simplifies Store Implementation

   • Stores don't need to track client ownership
   • Clearer separation of concerns
   • Less conditional logic in __aenter__/__aexit__

5. More Predictable Behavior

   • Users know external resources won't be closed unexpectedly
   • Easier to debug resource lifecycle issues
   • Aligns with principle of least surprise

6. Better for Testing

   • Mock clients won't be closed by the store
   • Easier to share test fixtures across tests
   • More control over client lifecycle in test scenarios

---

⚠️ Cons of Proposed Approach

1. Breaking Change

   • Existing code that relies on store closing user-provided clients will break
   • Users need to explicitly manage client lifecycle
   • Migration path required

2. More Boilerplate for Simple Cases

   • Current pattern is convenient for one-off usage:

     # Current (convenient)
     async with RedisStore(client=my_client) as store:
         await store.put(...)
     
     # Proposed (more verbose)
     async with my_client:
         store = RedisStore(client=my_client)
         await store.put(...)

3. Potential for Resource Leaks

   • If users forget to close their clients
   • However, this is the user's responsibility and a common pattern in Python

4. Inconsistent with Some Python Patterns

   • Some libraries (e.g., requests.Session wrappers) do take ownership
   • However, async context managers typically follow "explicit is better than implicit"

5. Documentation Overhead

   • Need clear docs explaining ownership semantics
   • Need examples for both patterns
   • Potential for user confusion during transition

6. Mixed Behavior in Context Manager

   • Store's context manager would do different things based on how it was initialized:
     • With client params: Manages client lifecycle
     • With client object: Only manages store setup/teardown
   • Could be confusing, though precedent exists (e.g., aiohttp.ClientSession can wrap existing connector)

---

🎯 Recommendation

Proceed with the proposed change because:

1. Correctness over Convenience: The current pattern can cause subtle bugs with double context entry
2. Scalability: Client reuse is important for performance in production
3. Pythonic: "Explicit is better than implicit" - clear ownership is better
4. Future-proof: As stores stabilize, having clear semantics is critical

Implementation Strategy:

1. Add a transition mechanism (optional):

   def __init__(self, *, client: Redis | None = None, own_client: bool = True, ...):
       self._external_client = client is not None and not own_client

   • Default own_client=True maintains current behavior (with deprecation warning)
   • Eventually remove own_client parameter and make it automatic

2. Update Documentation:

   • Clear examples for both patterns
   • Emphasize ownership semantics
   • Migration guide

3. Update Tests:

   • Add tests for both client creation modes
   • Verify client is not closed when user-provided
   • Verify client IS closed when store-created

---

📋 Implementation Checklist

If proceeding, here's what needs to change:

• Update BaseContextManagerStore.__aenter__ to check client ownership
• Update BaseContextManagerStore.__aexit__ to check client ownership
• Update individual stores (Redis, DynamoDB, MongoDB, Valkey, etc.)
• Add client ownership tracking to each store
• Update all docstrings to clarify ownership semantics
• Update documentation (stores.md, examples)
• Add tests for both ownership modes
• Update existing tests that may rely on old behavior
• Add migration guide
• Consider adding deprecation warnings (optional transition period)

---

[strawgate]

@claude let's proceed with the split and we'll skip the own_client compatibility transition part

[claude]

Claude finished @strawgate's task —— View job • claude/issue-232-20251110-1652 • Create PR ➔

---

Client Ownership Split Implementation Complete

I've successfully implemented the client ownership split pattern where stores only manage the lifecycle of clients they create, not user-provided clients.

Summary of Changes

BaseContextManagerStore (base.py):

• Added _client_provided_by_user attribute to track ownership
• Updated __aexit__ and close() to only call _close() if store created the client
• Updated docstrings to clarify lifecycle management

Updated Stores:

• Redis (redis/store.py): Set _client_provided_by_user based on whether client parameter was provided
• MongoDB (mongodb/store.py): Set flag + updated custom __aenter__/__aexit__ to only manage client context if store created it
• DynamoDB (dynamodb/store.py): Set flag + updated custom __aenter__/__aexit__ to only manage client context if store created it
• Valkey (valkey/store.py): Set _client_provided_by_user based on whether client parameter was provided
• Memcached (memcached/store.py): Set _client_provided_by_user based on whether client parameter was provided
• Elasticsearch (elasticsearch/store.py): Set _client_provided_by_user based on whether client parameter was provided
• Disk (disk/store.py): Set _client_provided_by_user based on whether disk_cache parameter was provided
• DuckDB (duckdb/store.py): Set _client_provided_by_user based on whether connection parameter was provided, removed outdated ownership comment, updated _close() and __del__() methods
• RocksDB (rocksdb/store.py): Set _client_provided_by_user based on whether db parameter was provided, removed duplicate __aexit__ override

All docstrings were updated to clarify that user-provided clients are not managed by the store.

Benefits

This change provides:

• ✅ Clear ownership semantics following "you create it, you manage it" principle
• ✅ Prevention of double-context entry issues
• ✅ Ability to reuse clients across multiple stores
• ✅ Better connection pooling support
• ✅ Alignment with Python resource management best practices

Note

There are 8 pre-existing type warnings in the generated sync Redis store code related to Redis library types. These are unrelated to the client ownership changes and exist due to the codegen not perfectly handling Redis's ResponseT type in the sync version.

---