From 58c26e1629d618981e517001aeeb654dc70deade Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Mon, 5 Oct 2026 09:57:11 +0100 Subject: [PATCH 1/2] Stop the SSE connect-snapshot test racing a queued duplicate TestSSEBroadcaster_AddClientWritesCurrentStatusBeforeReturning failed under -race in CI on unrelated frontend PRs (2479, 2485). The test broadcast a stage and connected straight away. When the broadcast loop picked that message up after the client had registered, it queued the stage for the new client too, so the client's writer goroutine wrote the httptest recorder while the test read it without a lock. AddClient itself is correct: it writes the preamble and current status before returning, and a stage racing the connect can reach the client twice, which the update modal already ignores. The test now lets the broadcast reach an earlier client before connecting, so nothing else writes the recorder and the status in the body can only come from the synchronous snapshot write. Reproduced by injecting the CI schedule through -overlay (broadcast loop held until the client registers, caller paused as AddClient returns): the old test failed 199/200 runs with DATA RACE, the new one 0/200. A mutant that moves the snapshot write onto the writer goroutine still fails the new test 100/100. --- internal/updates/sse_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/internal/updates/sse_test.go b/internal/updates/sse_test.go index 15da113887..4e97fb6fc8 100644 --- a/internal/updates/sse_test.go +++ b/internal/updates/sse_test.go @@ -249,7 +249,23 @@ func TestSSEBroadcaster_AddClientWritesCurrentStatusBeforeReturning(t *testing.T broadcaster := NewSSEBroadcaster() defer broadcaster.Close() + // The stage was broadcast before this client connects, so let it reach + // an existing client first. While a broadcast is still in flight the + // broadcast loop can queue it for the new client as well, and that + // client's writer goroutine would then write the recorder concurrently + // with the unsynchronized reads below. + earlier := &syncFlushWriter{} + if client := broadcaster.AddClient(earlier, "client-earlier"); client == nil { + t.Fatal("AddClient returned nil") + } broadcaster.Broadcast(UpdateStatus{Status: "extracting", Progress: 40, Message: "Extracting update..."}) + deadline := time.Now().Add(2 * time.Second) + for !strings.Contains(earlier.String(), `"status":"extracting"`) { + if time.Now().After(deadline) { + t.Fatalf("broadcast never reached the earlier client: %q", earlier.String()) + } + time.Sleep(time.Millisecond) + } w := &mockFlushWriter{ResponseRecorder: httptest.NewRecorder()} if client := broadcaster.AddClient(w, "client-now"); client == nil { From 561609ec3b1d15ef06685819fa5afb1775f50f4d Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Mon, 5 Oct 2026 10:06:46 +0100 Subject: [PATCH 2/2] Correct the SSE connect-snapshot comment about racing broadcasts The comment in AddClient said a broadcast racing a connect is either in the snapshot or queued behind it. It can be both, so the client then receives that status twice, possibly with an older in-flight stage in between. Trusting the "either" is what led the connect-snapshot test to assume nothing writes the recorder after AddClient returns, which made it flaky under -race. Say what actually happens and why it is harmless: stage progress only rises and the update modal ignores backwards stages. Contract-Neutral: Comment-only correction in internal/updates/sse.go; no runtime or contract change. --- internal/updates/sse.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/updates/sse.go b/internal/updates/sse.go index 700790efb8..8f146003bd 100644 --- a/internal/updates/sse.go +++ b/internal/updates/sse.go @@ -105,7 +105,11 @@ func (b *SSEBroadcaster) AddClient(w http.ResponseWriter, clientID string) *SSEC // before the ordered writer starts, so a (re)connecting client always // sees where the update is right now instead of waiting for the next // stage change. The cache is read after registration so a broadcast - // racing this connect is either in the snapshot or queued behind it. + // racing this connect is never lost: it is in the snapshot, queued + // behind it, or both. In the last case the client receives that status + // again after the snapshot, possibly preceded by an older stage that was + // still in flight. Stage progress only rises, and the update modal + // ignores stages that move it backwards, so the repeat is harmless. b.statusMu.RLock() cachedStatus := b.cachedStatus b.statusMu.RUnlock()