Skip to content

Commit 58ff6d8

Browse files
committed
Test(reverseproxy): fix flaky finisher test race under -race
TestReverseProxy_Finisher_{Allow,Deny} read finisherStub.seen immediately after http.Get returned. The reverse proxy sets FlushInterval=-1, so the streamed response reaches the client before the handler's deferred RunFinish (which dispatches OnFinish) executes — a pre-existing race that flaked CI under -race (~1 in 20 locally). Poll for the expected finishers with a short timeout (waitSeen) instead of reading once, and store finisherStub.seen LAST in OnFinish (after outcome) so observing seen==true guarantees outcome is visible. The negative check (after-deny must NOT fire) runs after the positive waits, by which point the single RunFinish dispatch is complete. Verified: 50× -race runs of the finisher tests and the full authlib -race suite are green. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
1 parent 2eb3eb1 commit 58ff6d8

1 file changed

Lines changed: 25 additions & 4 deletions

File tree

authbridge/authlib/listener/reverseproxy/finisher_test.go

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,27 @@ import (
66
"net/http/httptest"
77
"sync/atomic"
88
"testing"
9+
"time"
910

1011
"github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline"
1112
"github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins/plugintesting"
1213
)
1314

15+
// waitSeen polls b until it's true, up to ~1s. OnFinish runs in the
16+
// reverse-proxy handler's deferred RunFinish, which fires only after the
17+
// streamed response (NewServer sets FlushInterval=-1) has already reached the
18+
// client — so http.Get can return before OnFinish has run. Poll rather than
19+
// read immediately, which otherwise races (flaky under -race).
20+
func waitSeen(b *atomic.Bool) bool {
21+
for i := 0; i < 1000; i++ {
22+
if b.Load() {
23+
return true
24+
}
25+
time.Sleep(time.Millisecond)
26+
}
27+
return false
28+
}
29+
1430
// finisherStub is a minimal Plugin + Finisher used by these tests to
1531
// observe OnFinish dispatch and the Outcome the listener derived.
1632
type finisherStub struct {
@@ -34,11 +50,13 @@ func (p *finisherStub) OnResponse(context.Context, *pipeline.Context) pipeline.A
3450
return pipeline.Action{Type: pipeline.Continue}
3551
}
3652
func (p *finisherStub) OnFinish(_ context.Context, pctx *pipeline.Context) {
37-
p.seen.Store(true)
3853
if o := pctx.Outcome(); o != nil {
3954
cp := *o
4055
p.outcome.Store(&cp)
4156
}
57+
// Store seen LAST so a reader that observes seen==true is guaranteed to
58+
// also see the outcome that was set just above.
59+
p.seen.Store(true)
4260
}
4361

4462
func pipelineWith(t *testing.T, plugins ...pipeline.Plugin) *pipeline.Holder {
@@ -73,7 +91,7 @@ func TestReverseProxy_Finisher_Allow(t *testing.T) {
7391
}
7492
resp.Body.Close()
7593

76-
if !f.seen.Load() {
94+
if !waitSeen(&f.seen) {
7795
t.Fatal("OnFinish did not fire")
7896
}
7997
o := f.outcome.Load()
@@ -133,12 +151,15 @@ func TestReverseProxy_Finisher_Deny(t *testing.T) {
133151
t.Errorf("HTTP status = %d, want 403", resp.StatusCode)
134152
}
135153

136-
if !before.seen.Load() {
154+
if !waitSeen(&before.seen) {
137155
t.Error("before-deny.OnFinish should have fired (OnRequest ran before denial)")
138156
}
139-
if !denier.seen.Load() {
157+
if !waitSeen(&denier.seen) {
140158
t.Error("denier.OnFinish should have fired (OnRequest ran and produced the deny)")
141159
}
160+
// before/denier have fired, so the single RunFinish dispatch is complete;
161+
// after-deny was never dispatched (its OnRequest never ran), so its
162+
// OnFinish must not have fired.
142163
if after.seen.Load() {
143164
t.Error("after-deny.OnFinish should NOT have fired (OnRequest never ran)")
144165
}

0 commit comments

Comments
 (0)