fix: sse error handling #107
Conversation
to prevent instances where we try to reconnect forever and spam the server.
fix: bug where client crashes when http error code received. chore: lint errors
luxscious
changed the title
There was a problem hiding this comment.
Pull request overview
Improves SSE robustness by propagating read-loop failures to the error handler and introducing an exponential backoff strategy for SSE reconnection.
Changes:
- Wraps
SSEClient.start()and the read loop intry/except/finally, forwarding failures to the configured error handler. - Adds exponential backoff + delayed reconnection thread orchestration in
EnvironmentConfigManager.sse_error. - Fixes
use_new_configboolean precedence for correct SSE URL change detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| devcycle_python_sdk/managers/sse_manager.py | Wraps SSE read loop in error handling and forwards exceptions to the error callback; fixes URL-change condition precedence. |
| devcycle_python_sdk/managers/config_manager.py | Adds exponential backoff reconnection logic and SSE connection recreation/teardown handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@JamieSinn I've opened a new pull request, #108, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def close(self): | ||
| self._polling_enabled = False | ||
| if self._sse_manager is not None and self._sse_manager.client is not None: | ||
| self._sse_manager.client.close() |
There was a problem hiding this comment.
close() only closes the current SSE client, but any already-scheduled reconnect thread (spawned from sse_error) can still wake up later and call _recreate_sse_connection(), re-opening SSE after the manager is supposedly closed. Add a shutdown/closed guard (or check _polling_enabled) in sse_error / _delayed_sse_reconnect / _recreate_sse_connection, and set it in close() so reconnection is suppressed during/after shutdown.
| logger.debug(f"DevCyle: SSE connection error: {error.error}") | ||
| current_time = time.time() | ||
|
|
||
| if self._sse_reconnecting: | ||
| logger.debug("DevCyle: Reconnection already in progress, skipping") | ||
| return |
There was a problem hiding this comment.
Typo in log prefix: "DevCyle" should be "DevCycle" for consistency/searchability.
| logger.debug( | ||
| f"DevCyle: Within backoff period, scheduling reconnection in {delay_seconds:.1f}s" | ||
| ) |
There was a problem hiding this comment.
Typo in log prefix: "DevCyle" should be "DevCycle" for consistency/searchability.
| logger.debug( | ||
| f"DevCyle: Attempting SSE reconnection (attempt #{self._sse_reconnect_attempts}, " | ||
| f"backoff: {delay_seconds:.1f}s)" | ||
| ) |
There was a problem hiding this comment.
Typo in log prefix: "DevCyle" should be "DevCycle" for consistency/searchability.
| self._sse_reconnecting = False | ||
| self._last_reconnect_attempt_time = None | ||
| else: | ||
| logger.debug("DevCyle: SSE keepalive received") |
There was a problem hiding this comment.
Typo in log prefix: "DevCyle" should be "DevCycle" for consistency/searchability.
| def sse_error(self, error: ld_eventsource.actions.Fault): | ||
| self._sse_connected = False | ||
| logger.debug(f"DevCycle: Received SSE error: {error}") | ||
| logger.debug(f"DevCyle: SSE connection error: {error.error}") | ||
| current_time = time.time() | ||
|
|
||
| if self._sse_reconnecting: | ||
| logger.debug("DevCyle: Reconnection already in progress, skipping") | ||
| return | ||
|
|
||
| # Calculate exponential backoff interval (capped at max) | ||
| backoff_interval = min( | ||
| self._min_reconnect_interval * (2**self._sse_reconnect_attempts), | ||
| self._max_reconnect_interval, | ||
| ) | ||
|
|
||
| # Check if we need to wait for remaining backoff time | ||
| delay_seconds = backoff_interval | ||
| if self._last_reconnect_attempt_time is not None: | ||
| time_since_last_attempt = current_time - self._last_reconnect_attempt_time | ||
| if time_since_last_attempt < backoff_interval: | ||
| delay_seconds = backoff_interval - time_since_last_attempt | ||
| logger.debug( | ||
| f"DevCyle: Within backoff period, scheduling reconnection in {delay_seconds:.1f}s" | ||
| ) | ||
|
|
||
| self._sse_reconnecting = True | ||
| self._sse_reconnect_attempts += 1 | ||
|
|
||
| logger.debug( | ||
| f"DevCyle: Attempting SSE reconnection (attempt #{self._sse_reconnect_attempts}, " | ||
| f"backoff: {delay_seconds:.1f}s)" | ||
| ) | ||
|
|
||
| reconnect_thread = threading.Thread( | ||
| target=self._delayed_sse_reconnect, args=(delay_seconds,), daemon=True | ||
| ) | ||
| reconnect_thread.start() |
There was a problem hiding this comment.
New SSE exponential-backoff reconnection behavior (attempt counting, delay calculation, and suppression of concurrent reconnects) isn’t covered by tests. Add unit tests for sse_error that validate the computed delay/attempt increments (mocking time.time()/threading.Thread/time.sleep) and that successful reconnect resets attempts.
feat: add exponential backoff strategy for SSE reconnection
fix: wrap client.start() in try catch