Skip to content

httpserver: make /-/reload handler cancellation-safe #5234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kimjune01 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

httpserver: make /-/reload handler cancellation-safe #5234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions httpserver/httpserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,22 @@ func Register(r *route.Router, reloadCh chan<- chan error) {
r.Get("/metrics", promhttp.Handler().ServeHTTP)

r.Post("/-/reload", func(w http.ResponseWriter, req *http.Request) {
errc := make(chan error)
defer close(errc)
errc := make(chan error, 1)

reloadCh <- errc
if err := <-errc; err != nil {
http.Error(w, fmt.Sprintf("failed to reload config: %s", err), http.StatusInternalServerError)
select {
case reloadCh <- errc:
case <-req.Context().Done():
http.Error(w, req.Context().Err().Error(), http.StatusUnprocessableEntity)
return
}

select {
case err := <-errc:
if err != nil {
http.Error(w, fmt.Sprintf("failed to reload config: %s", err), http.StatusInternalServerError)
}
case <-req.Context().Done():
http.Error(w, req.Context().Err().Error(), http.StatusUnprocessableEntity)
}
})

Expand Down
122 changes: 122 additions & 0 deletions httpserver/httpserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,136 @@
package httpserver

import (
"context"
"errors"
"net/http"
"net/http/httptest"
"testing"
"time"

"github.com/prometheus/common/route"
"github.com/stretchr/testify/require"
)

func TestReloadSuccess(t *testing.T) {
reloadCh := make(chan chan error)
router := route.New()
Register(router, reloadCh)

done := make(chan struct{})
go func() {
defer close(done)
req := httptest.NewRequest("POST", "/-/reload", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
require.Equal(t, http.StatusOK, w.Code)
}()
Comment on lines +34 to +40
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

testify require FailNow from goroutine other than test goroutine

💡 Result:

In Go’s testing and in stretchr/testify, you must not call require.* / t.FailNow (which it uses under the hood) from a goroutine other than the goroutine running the test. Evidence: - Go’s testing.T.FailNow documentation says FailNow “must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test.” It calls runtime.Goexit in the current goroutine, so it only stops that goroutine, not the whole test. [1] - testify’s require package documentation states that require “terminate current test” and “must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test.” [2] What to do instead: - Use assert.* inside goroutines (it records an error via t.Errorf-style behavior rather than invoking FailNow). This is the common workaround suggested in testify discussions: “use assert in goroutines, not require.” [3] - Or funnel the result back to the main test goroutine (e.g., via a channel) and perform require/assert in that main goroutine, so FailNow happens on the correct goroutine. [4] Why this matters: - If require calls t.FailNow from the wrong goroutine, runtime.Goexit will exit only that goroutine, so the expected assertion/termination semantics for the test can break (and can even lead to hangs in some patterns). [5][6]

Citations:

🏁 Script executed:

# Check the actual test file at the mentioned line ranges
cat -n httpserver/httpserver_test.go | sed -n '30,70p'

Repository: prometheus/alertmanager

Length of output: 1246

🏁 Script executed:

# Also check the other two ranges
cat -n httpserver/httpserver_test.go | sed -n '82,125p'

Repository: prometheus/alertmanager

Length of output: 1618

Use assert.* or move assertions outside goroutines.

require.* calls t.FailNow(), which per Go's testing documentation must be called from the goroutine running the test—calling it from another goroutine exits only that goroutine via runtime.Goexit(), leaving the test in a misleading state instead of properly failing.

All four new tests have this issue (lines 34–40, 58–65, 86–92, 115–121). Fix by either:

  1. Moving assertions outside the goroutine and capturing response objects in the main goroutine after <-done, or
  2. Using assert.* inside the goroutines to record failures without violating the threading contract.
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@httpserver/httpserver_test.go` around lines 34 - 40, The tests spawn
goroutines that call require.* (which calls t.FailNow) — replace these with
assert.* or move the assertions out of the goroutine: capture the
httptest.ResponseRecorder (w) and any response values inside the goroutine, send
them on a channel or close(done) and then after <-done perform require.Equal(t,
http.StatusOK, w.Code) in the main test goroutine; update the four occurrences
referencing the done channel, router.ServeHTTP, httptest.NewRequest and
httptest.NewRecorder so assertions run in the test goroutine or use assert.*
inside the goroutine instead.


errc := <-reloadCh
errc <- nil

select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("handler did not return")
}
}

func TestReloadError(t *testing.T) {
reloadCh := make(chan chan error)
router := route.New()
Register(router, reloadCh)

done := make(chan struct{})
go func() {
defer close(done)
req := httptest.NewRequest("POST", "/-/reload", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
require.Equal(t, http.StatusInternalServerError, w.Code)
require.Contains(t, w.Body.String(), "bad config")
}()

errc := <-reloadCh
errc <- errors.New("bad config")

select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("handler did not return")
}
}

func TestReloadClientDisconnectBeforeEnqueue(t *testing.T) {
// reloadCh is never consumed, so the handler blocks on enqueue.
// Cancelling the context should unblock it.
reloadCh := make(chan chan error)
router := route.New()
Register(router, reloadCh)

ctx, cancel := context.WithCancel(context.Background())
done := make(chan struct{})
go func() {
defer close(done)
req := httptest.NewRequest("POST", "/-/reload", nil).WithContext(ctx)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
require.Equal(t, http.StatusUnprocessableEntity, w.Code)
}()

// Give the handler time to block on reloadCh send.
time.Sleep(50 * time.Millisecond)
cancel()

select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("handler did not unblock after context cancellation")
}
}

func TestReloadClientDisconnectDuringReload(t *testing.T) {
// The handler enqueues successfully but the client disconnects
// before the reload result arrives. The buffered channel ensures
// the main goroutine (sender) does not block.
reloadCh := make(chan chan error)
router := route.New()
Register(router, reloadCh)

ctx, cancel := context.WithCancel(context.Background())
done := make(chan struct{})
go func() {
defer close(done)
req := httptest.NewRequest("POST", "/-/reload", nil).WithContext(ctx)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
require.Equal(t, http.StatusUnprocessableEntity, w.Code)
}()

errc := <-reloadCh
cancel()

select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("handler did not unblock after context cancellation")
}

// Simulate the main goroutine sending the result after the handler
// has already returned. This must not block thanks to the buffered channel.
sendDone := make(chan struct{})
go func() {
defer close(sendDone)
errc <- nil
}()

select {
case <-sendDone:
case <-time.After(5 * time.Second):
t.Fatal("sender blocked on errc — channel must be buffered")
}
}

func TestDebugHandlersWithRoutePrefix(t *testing.T) {
reloadCh := make(chan chan error)

Expand Down