Repository navigation
Prevent stopped SSE watchers from updating reconnected clients - #206
Conversation
|
Which queue do you mean ? |
|
Sorry, that comment landed in the wrong place - please disregard it. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Snapshot replay remains unordered with concurrent watcher delivery, which can leave newly registered clients with stale state.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This pull request prevents obsolete SSE watchers from updating reconnected clients.
Changes:
- Adds watcher-generation validation under a reentrant lock.
- Removes generations after final disconnection.
- Adds namespace and resource regression tests.
| File | Description |
|---|---|
backend/apps/common/sse/manager.py |
Adds generation tracking and callback synchronization. |
backend/apps/common/sse/manager_test.py |
Tests obsolete callback rejection and snapshot preservation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for the review, and thanks @juliusvonkohout for pulling in a reviewer. The Copilot comment on manager.py is right. What happens: in register_namespace_watch, a late client is added to the client set and the cached snapshot is read under the lock, but the snapshot was only put on the client's queue after the lock was released. A watcher callback takes the same lock, so it could run in that gap, see the new client in the set, and enqueue a newer event first. The older INITIAL snapshot then landed after it and the page could show stale data. register_single_watch had the same gap. This was already the case before this PR, it just sits right next to the code I changed. Fix (5102d89): the snapshot replay now happens inside the lock, in both register_namespace_watch and register_single_watch. The queue write is put_nowait, so it does not block while the lock is held, and the lock is reentrant. A callback now has to wait until the replay is queued, so the client always gets the snapshot first and any newer event after it. Tests: two new ones in manager_test.py (namespace and single). Each makes the late client's queue start a watcher callback on another thread at the moment the snapshot is enqueued, then checks the order is INITIAL then the newer event. Both fail on the previous manager.py and pass now. I ran manager_test.py, routes_test.py and watchers_test.py directly with python3 (all OK). I did not run the full backend test suite or test against a real cluster. |
|
Pleae sign your commits properly according to the DCO. Commit sha: e24233d, Author: Charan Rathore, Committer: Charan Rathore; The sign-off is missing. |
Signed-off-by: Charan Rathore <[email protected]>
A late client was added to the client set and its snapshot captured under the lock, but the snapshot was enqueued after the lock was released. A watcher callback could enqueue a newer event in between, so the older snapshot arrived after it. Replay inside the lock instead, as the callbacks already take it, and add tests for the namespace and single-resource paths. Signed-off-by: Charan Rathore <[email protected]>
Signed-off-by: Charan Rathore <[email protected]>
Signed-off-by: Charan Rathore <[email protected]>
56e7b55 to
8965968
Compare
|
All four commits now carry a Signed-off-by line. The code is unchanged, only the commit messages were rewritten, and the DCO check passes on the new head (8965968). Prepared with AI assistance; Charan Rathore is directing and responsible for this PR. |
…bject Signed-off-by: Charan Rathore <[email protected]>
|
Please fix the tests |
|
Let me try to rerun it first |
A watcher can finish an in-flight callback after its final client disconnects. If another client has already registered under the same namespace or resource key, the obsolete callback currently reaches that new client and overwrites the replay snapshot.
Track the active watcher generation separately from its factory placeholder and ignore callbacks from earlier generations. Keep generation validation, snapshot recording, and nonblocking queue delivery under one reentrant lock. Remove the generation when the final client leaves; watcher shutdown stays outside the lock.
Tests cover namespace and single-resource disconnect/reconnect, obsolete initial and deletion events, and the snapshot returned to a later client. Both regression tests fail before the change and pass afterwards.
Validation:
Docker image build/smoke and Kubernetes integration were not run. Frontend checks were not run for this backend-only change.