Fix lock inversion deadlock in SSE reconnection

[Copilot]

• Fix typo: "Devcyle" → "DevCycle" in log message (eb7844f)
• Fix race condition: Set _sse_manager = None before releasing lock (741b526)
• Fix stale config: Re-read self._config when re-acquiring lock (741b526)
• Add test coverage for concurrent reconnection scenarios (89a8467)
• Validate changes with tests and linter
• Run security scan (no issues found)
• Reply to all review comments

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This pull request addresses a lock inversion issue in the SSE (Server-Sent Events) error handling logic that could cause deadlocks. The fix refactors _recreate_sse_connection to use a two-phase locking approach: acquiring the lock to save references, performing blocking operations (close/join) outside the lock, then re-acquiring the lock to create the new connection.

Changes:

• Moved potentially blocking close() and join() operations outside the lock to prevent deadlocks when the SSE read thread calls error callbacks that need the same lock
• Split the single lock acquisition into two phases: one to save state, one to update state
• Added explanatory comments documenting the locking strategy and rationale

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Typo in log message: "Devcyle" should be "DevCycle" to match the spelling used in most other log messages in this file (e.g., lines 131, 150, 165, 168, 193, 202, 255). While this appears to be a pre-existing issue, since this line was moved as part of the refactoring, it would be a good opportunity to fix the spelling for consistency.

[Copilot]

Fixed in eb7844f (already corrected before these changes).

[Copilot]

The changes to _recreate_sse_connection that address the lock inversion issue lack test coverage. The existing test suite (test/managers/test_config_manager.py) only tests with disable_realtime_updates=True, which completely bypasses this code path.

This is particularly important because:

1. The fix involves complex concurrency logic with two-phase locking
2. The issue being fixed was a deadlock scenario that would only manifest under specific timing conditions
3. The behavior during the window between releasing and re-acquiring the lock is critical

Consider adding tests that verify:

• Concurrent calls to _recreate_sse_connection are handled correctly
• The SSE manager state remains consistent when errors occur during reconnection
• The interaction between sse_error callbacks and the reconnection logic works without deadlocks

[Copilot]

Added comprehensive test coverage in 89a8467. Tests now verify: (1) _sse_manager is cleared before blocking operations, (2) latest config is used after re-acquiring lock, (3) concurrent calls are handled safely, and (4) proper behavior when config is cleared during reconnection.

[Copilot]

There is a potential race condition in this implementation. Between releasing the lock at line 72 and re-acquiring it at line 85, other threads could access or modify self._sse_manager. Specifically:

1. The _get_config method (lines 144-152) reads self._sse_manager without holding the lock
2. The close method (lines 266-267) accesses self._sse_manager without holding the lock
3. Another thread could call _recreate_sse_connection concurrently

This means self._sse_manager could be replaced or set to None while the saved old_sse_manager is being closed outside the lock. While this may not cause immediate crashes, it creates undefined behavior where the old manager might be replaced mid-cleanup.

Consider setting self._sse_manager = None before releasing the lock at line 72, so that concurrent accesses see a consistent state indicating "no active connection" during the transition. This would also prevent the SSE read thread's error callbacks from trying to use a manager that's being torn down.

[Copilot]

Fixed in 741b526. Now setting self._sse_manager = None before releasing the lock, ensuring concurrent accesses see a consistent "no active connection" state during the transition.

[Copilot]

The current_config reference is saved while holding the lock and then used outside the lock at line 92. While Python dictionaries are reference types and won't be garbage collected, there's a subtle race condition: self._config could be reassigned by another thread (e.g., in _get_config at line 137) between line 71 and line 92.

This means the SSE manager could be initialized with a stale config that's different from the current self._config. While this may eventually self-correct, it could lead to inconsistent state where the SSE manager is connected to a URL that doesn't match the current configuration.

Consider re-reading self._config inside the second lock acquisition to ensure the SSE manager is always initialized with the most current configuration.

[Copilot]

Fixed in 741b526. Now re-reading self._config when re-acquiring the lock to ensure the SSE manager is always initialized with the most current configuration.

[JamieSinn]

@copilot re-work the PR to fix the identified issues.

[luxscious]

@copilot get rid of the lock setup

[Copilot]

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• config-cdn.devcycle.com
  • Triggering command: /usr/bin/python python -m pytest test/managers/test_config_manager.py -v (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[Copilot]

@copilot get rid of the lock setup

Could you clarify what you mean by "get rid of the lock setup"? The current implementation uses a two-phase locking approach to prevent deadlocks. Are you asking to:

1. Revert to the original single-lock implementation (which had the deadlock issue)?
2. Remove all locking entirely (which would create race conditions)?
3. Use a different synchronization approach?

The lock is needed because the SSE read thread can call sse_error/sse_state callbacks that need to acquire the same lock, so holding it during blocking operations (close/join) can cause deadlock. Please let me know which approach you'd prefer.