Stop the SSE connect-snapshot test racing a queued duplicate (#2503)

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.
This commit is contained in:
rcourtman
2026-10-05 10:26:56 +01:00
committed by GitHub
2 changed files with 21 additions and 1 deletions
+5 -1
View File
@@ -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()
+16
View File
@@ -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 {