chore: Fix test deadlock and cyclomatic complexity. - #66
Merged
Conversation
…helpers (*Server).Handler mixed HTTP content negotiation into the streaming loop, and the deepest branch of (*Server).run inlined the choice between ReplayWithContext and Replay. Moving each into a small named function makes the remaining control flow easier to follow and brings both functions back under the gocyclo limit (Handler 31 -> 28, run 30 -> 29). No behavior change; this is pure code motion.
…lient bytes Replayed batch events are flushed once per batch, so nothing a producer sends mid-batch is client-visible while it still holds the batch open. The disconnect tests previously positioned the producer by waiting for "data: first" to reach the client, which can never happen for a producer that deliberately blocks mid-batch; the tests hung, and the first one deadlocked the test binary because its failure path skipped releasing the producer, leaving the deferred httptest.Server.Close waiting on the still-running handler. The repos now close a firstDelivered channel when their first unbuffered send completes -- which proves the handler has consumed the event and is reading the batch -- and tests synchronize on that instead. The normal-delivery tests still assert the full batch (including "data: first") reaches the client once the batch completes.
kinyoklion
changed the title
kinyoklion
changed the title
kinyoklion
marked this pull request as ready for review
July 28, 2026 23:15
keelerm84
approved these changes
Jul 29, 2026
kinyoklion
added a commit
to launchdarkly/ld-relay
that referenced
this pull request
Jul 29, 2026
…onnects (#774) ## Summary The server-side stream replay path (`serverSideEnvStreamRepository.Replay`, backing `/sdk/stream` for FDv2 and `/all` for FDv1) leaks a goroutine per SDK client that disconnects mid-replay. `Replay` returns an unbuffered channel and spawns a producer goroutine that sends the replay events on it. Under backpressure — a slow or stalled SDK client — the producer parks on `out <- event` while the `eventsource` connection handler is busy writing to the socket. If the client then disconnects, the handler stops reading the channel, and because `Replay` has no cancellation hook, the producer is stranded on that send until the process exits. The goroutine and the replay payload it holds leak. ## Change Adopt the new optional `eventsource.RepositoryWithContext` extension: - `serverSideEnvStreamRepository` now implements `ReplayWithContext(ctx, channel, id)`. The `eventsource` server calls it with the subscribing request's context, which is cancelled on disconnect. The send loop `select`s on `ctx.Done()`, so the producer returns immediately instead of blocking on a send nobody will receive. - `Replay` is retained (it delegates to the same logic with a background context) to satisfy the `eventsource.Repository` interface; the server prefers `ReplayWithContext` when a repository implements it. The shared `replay` helper keeps the existing `IsInitialized` short-circuit and singleflight behavior unchanged. ## Dependency This depends on launchdarkly/eventsource#63, which adds `RepositoryWithContext` (plus a handler-side background drain that unblocks any `Repository` producer, even those that don't adopt the context). That change is now released: `go.mod` points at the tagged **eventsource v1.11.2**, which contains #63 along with the once-per-batch replay flush (launchdarkly/eventsource#64) and the CI fixes (launchdarkly/eventsource#66). The earlier pseudo-version pin of the PR branch is gone. ## Testing - New unit test: `ReplayWithContext` stops producing (channel closes) promptly when the subscriber's context is cancelled without a reader — before context propagation this producer would block forever. - `go test -race ./internal/streams/...` and `./relay/...` pass, re-run against the released v1.11.2. - End to end against `mockld` (a ~3MB, 6500-flag dataset) with a non-reading client that stalls the socket: - Preconditions reproduced: the producer goroutine parks on the channel send while the handler blocks in a socket write (real TCP backpressure). - **Fixed build:** the replay producer goroutine exits within ~0.6s of a client FIN. - **Stock build (eventsource v1.11.0):** the producer is still blocked on the channel send 3s after the client disconnects (handler already gone) — the leak. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches hot-path SSE replay for all server-side SDK connections; behavior change is limited to cleanup on disconnect, but large replay payloads and timing-sensitive send loops warrant careful review. > > **Overview** > Fixes a **goroutine leak** on server-side SDK streams (`/sdk/stream`, `/all`) when a client disconnects while replay is still sending on an unbuffered channel—the producer could block forever on `out <- event` with no reader. > > `serverSideEnvStreamRepository` now implements **`eventsource.RepositoryWithContext`**: shared `replay` logic uses the subscribe request context (cancelled on disconnect), bails before building the snapshot if already cancelled, and **`select`s on `ctx.Done()`** when sending events. Legacy **`Replay`** delegates to the same helper with `context.Background()`. > > **Dependencies:** `github.com/launchdarkly/eventsource` **v1.11.0 → v1.11.2** (adds `RepositoryWithContext`); `klauspost/compress` patch bump in lockfile. > > Adds a unit test that cancels context mid-replay with no consumer and asserts the channel closes without delivering events. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 9997de9. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI on
mainis red for two independent reasons after #63 and #64 merged; each PR was green alone but their combination broke both the lint job and the test jobs. This PR fixes both.1. Test deadlock (the 10-minute Windows timeouts, reproducible on Linux). #64 changed replayed batches to flush once per batch instead of once per event. The #63 disconnect tests positioned their producer by waiting for
data: firstto reach the client mid-batch — which can never happen once mid-batch writes are no longer flushed, since the producer deliberately holds the batch open. 10 of the 16 tests inserver_replay_disconnect_test.gofail this way, and the first one deadlocks the whole test binary: itst.Fatalfskips the producer-release step, so the deferredhttptest.Server.Closewaits forever on the still-running handler. (#63's CI predated #64's merge, so neither PR saw this.)The fix is test-only: the test repositories close a
firstDeliveredchannel when their first unbuffered send completes — which proves the handler has consumed the event and is reading the batch — and the tests synchronize on that instead of on client-visible bytes. The normal-delivery tests still assert the full batch, includingdata: first, reaches the client once the batch completes. The production code needs no changes; the drain and context-cancellation logic composes with per-batch flushing as-is.2. gocyclo (the Linux lint failure). The two merges combined pushed
(*Server).Handlerto cyclomatic complexity 31, over golangci-lint's default limit of 30, and left(*Server).runat exactly 30. Rather than raising the limit, this extracts two self-contained pieces into named functions — pure code motion, no behavior change:writeStreamHeaders: the SSE response-header setup and gzip negotiation that opened theHandlerclosure (Handler 31 -> 28).replay: the choice betweenReplayWithContextand the context-lessReplayfallback, previously inlined at the deepest nesting level ofrun(run 30 -> 29).Verified with the repo's
make lint(same golangci-lint version as CI) and the full suite green under-race, including all 10 previously hanging/failing tests.