Skip to content

fix(sse): cancel active streams on user logout#6033

Closed
clubanderson wants to merge 1 commit intofix/6022-6024-6026-securityfrom
fix/6029-sse-cancellation
Closed

fix(sse): cancel active streams on user logout#6033
clubanderson wants to merge 1 commit intofix/6022-6024-6026-securityfrom
fix/6029-sse-cancellation

Conversation

@clubanderson
Copy link
Copy Markdown
Collaborator

Fixes #6029. Stacked on #6031 (fix/6022-6024-6026-security).

Problem

SSE streaming endpoints (streamClusters in pkg/api/handlers/sse.go) run inside SetBodyStreamWriter callbacks that block until either the client disconnects or sseOverallDeadline (~30s) fires. When a user logged out, any in-flight SSE streams kept emitting cluster_data events until that deadline — the token was revoked but the already-established stream had no active cancellation path.

Fix

Mirrors the exec-session cancellation pattern added in #6031 for /ws/exec sessions (#6024).

  • New per-user SSE session registry in sse.go:
    • sseSessions map[uuid.UUID]map[int64]context.CancelFunc guarded by a plain sync.Mutex
    • registerSSESession / unregisterSSESession / CancelUserSSEStreams
    • Monotonic session-id generator so multiple concurrent streams for the same user are tracked independently
  • streamClusters captures the authenticated user ID before entering the deferred SetBodyStreamWriter callback (the fiber.Ctx may be reused by then, so c.Locals is not safe to read inside), then registers the stream context's cancel func in the registry with a deferred unregister for normal stream end. All 17 SSE handlers in sse.go route through streamClusters, so one registration site covers every streaming endpoint.
  • auth.Logout calls CancelUserSSEStreams(claims.UserID) immediately after the existing CancelUserExecSessions call, tearing down any live SSE streams for the user on logout.
  • streamDemoSSE is intentionally left unchanged — demo streams are instant (no blocking, no goroutines), so they need no cancellation path.

Tests

New pkg/api/handlers/sse_test.go mirroring exec_test.go:

  • TestCancelUserSSEStreams_CancelsRegisteredContexts — register two contexts, call cancel, verify both Done() and the per-user map entry is cleared
  • TestCancelUserSSEStreams_OtherUserUnaffected — register two users, cancel one, verify the other's context is still alive
  • TestUnregisterSSESession_RemovesEntry — verify per-session removal drops only the target entry and the per-user slot is removed when empty
  • TestCancelUserSSEStreams_NoSessions — must not panic on empty map

All pass:

=== RUN   TestCancelUserSSEStreams_CancelsRegisteredContexts
--- PASS
=== RUN   TestCancelUserSSEStreams_OtherUserUnaffected
--- PASS
=== RUN   TestUnregisterSSESession_RemovesEntry
--- PASS
=== RUN   TestCancelUserSSEStreams_NoSessions
--- PASS

Verification

  • go build ./... — clean
  • go test ./pkg/api/handlers/... -count=1 — passes
  • go vet ./... — clean
  • cd web && npm run build — clean (post-build safety checks pass)

Code quality

  • sync.Mutex (not RWMutex) — matches the exec-session pattern
  • uuid.UUID for user ID throughout — matches the rest of the codebase
  • New constants have comments; no magic numbers introduced
  • Unrelated SSE code untouched

Fixes #6029. SSE streaming endpoints (streamClusters in sse.go) ran
inside SetBodyStreamWriter callbacks that blocked for up to
sseOverallDeadline (~30s), so when a user logged out any in-flight
SSE streams continued to emit cluster_data events until the deadline
fired or the client disconnected.

Mirrors the exec session cancellation pattern added in #6031 for
/ws/exec sessions (#6024):

- Add a per-user SSE session registry in sse.go with
  registerSSESession / unregisterSSESession / CancelUserSSEStreams,
  guarded by a plain sync.Mutex.
- In streamClusters, capture the authenticated user ID before the
  deferred SetBodyStreamWriter callback runs (the fiber.Ctx may be
  reused by then) and register the stream context's cancel func in
  the registry, with a deferred unregister for normal stream end.
- In auth.Logout, after CancelUserExecSessions, call
  CancelUserSSEStreams(claims.UserID) so logout tears down any live
  SSE streams the user had open.
- Add sse_test.go with the four lifecycle tests mirroring
  exec_test.go: CancelsRegisteredContexts, OtherUserUnaffected,
  UnregisterSSESession_RemovesEntry, and NoSessions (no panic on
  empty map).

streamDemoSSE is left unchanged — demo streams are instant, they
do not block, so they need no cancellation path.

Signed-off-by: Andrew Anderson <[email protected]>
@kubestellar-prow kubestellar-prow Bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Apr 10, 2026
@kubestellar-prow
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign eeshaansa for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@clubanderson clubanderson added the ai-generated Pull request generated by AI label Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

👋 Hey @clubanderson — thanks for opening this PR!

🤖 This project is developed exclusively using AI coding assistants.

Please do not attempt to code anything for this project manually.
All contributions should be authored using an AI coding tool such as:

This ensures consistency in code style, architecture patterns, test coverage,
and commit quality across the entire codebase.


This is an automated message.

@kubestellar-prow kubestellar-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 10, 2026
@kubestellar-prow kubestellar-prow Bot deleted the branch fix/6022-6024-6026-security April 10, 2026 12:36
@kubestellar-prow kubestellar-prow Bot closed this Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-generated Pull request generated by AI dco-signoff: yes Indicates the PR's author has signed the DCO. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant