feat(python-sdk): harden cache, refresh, and network runtime behavior - #5
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds network timeout and retry logic, implements an automatic configuration refresh mechanism, and introduces an LRU cache for visitor data. Feedback identifies a race condition in the cache update logic, suggests using standard library functions for query string construction, recommends simplifying redundant conditional logic, and points out unreachable code in the HTTP client.
| with self._lock: | ||
| self._bucketed_visitors[store_key] = updated | ||
| self._bucketed_visitors.move_to_end(store_key) | ||
| while len(self._bucketed_visitors) > self._local_store_limit: | ||
| self._bucketed_visitors.popitem(last=False) |
There was a problem hiding this comment.
Adding this lock is a good step towards thread safety. However, a race condition still exists because the read operation on line 87 (via get_data) is outside this lock. This means another thread could modify the data between the read and this write, leading to lost updates.
To ensure atomicity and prevent data loss in concurrent scenarios, the entire read-modify-write cycle for the in-memory cache should be performed within a single lock.
| query += f"environment={self._environment}" | ||
| if self._cache_level == "low": | ||
| query += "_conv_low_cache=1" | ||
| query += ("&" if self._environment else "") + "_conv_low_cache=1" |
There was a problem hiding this comment.
While this line fixes a bug in query string construction, the overall approach of manually building the query string is fragile. Consider refactoring the query-building logic (lines 237-241) to use urllib.parse.urlencode. This would involve creating a dictionary of parameters and would be more robust and readable.
For example:
params = {}
if self._environment:
params['environment'] = self._environment
if self._cache_level == 'low':
params['_conv_low_cache'] = '1'
query = ''
if params:
query = '?' + urllib.parse.urlencode(params)| if not had_data and initial: | ||
| self._initialized = True | ||
| self._event_manager.fire(SystemEvents.READY, None, None, True) | ||
| elif had_data: | ||
| self._event_manager.fire(SystemEvents.CONFIG_UPDATED, None, None, True) | ||
| else: | ||
| self._initialized = True | ||
| self._event_manager.fire(SystemEvents.READY, None, None, True) |
There was a problem hiding this comment.
The logic in this if/elif/else block can be simplified. The first if block (lines 71-73) and the else block (lines 76-78) contain identical code. They can be combined into a single else clause for when had_data is false.
if had_data:
self._event_manager.fire(SystemEvents.CONFIG_UPDATED, None, None, True)
else:
self._initialized = True
self._event_manager.fire(SystemEvents.READY, None, None, True)| time.sleep(retry_backoff) | ||
| if last_error: | ||
| raise last_error | ||
| raise RuntimeError("HTTP request failed without an error") |
There was a problem hiding this comment.
This RuntimeError appears to be unreachable. The for loop will always execute at least once, and it will either return a response or raise an exception on the final attempt. Therefore, the program flow should never reach this line. It can be safely removed.
1 participant
Summary
This PR hardens the Python SDK for long-running and production-style runtimes. It adds bounded in-memory caching, safer HTTP request behavior, datastore queue passthrough support, and background config refresh lifecycle management.
What’s Included
Added bounded, thread-safe in-memory visitor cache
cache.max_entriesImproved datastore integration
release_queue()passthrough supportrelease_queue()andreleaseQueue()on external datastore implementationsImproved network/runtime safety
environmentis also presentAdded background config refresh lifecycle handling
sdkKey-based initializationclose()support to stop refresh timers and clean up API queue timersConfiguration Added
cache.max_entriesnetwork.requestTimeoutnetwork.configRetriesnetwork.trackRetriesnetwork.retryBackoffTesting
Added/updated tests for:
Verification:
.venv/bin/pytest -q76 passedWhy This PR
This PR focuses on runtime hardening rather than new SDK surface area. The goal is to make the Python SDK safer for long-running services and production deployments while preserving behavior already implemented in earlier PRs.