From e1d11000852f79cd84548ef7e67614f646ff14eb Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 2 Sep 2026 16:00:02 +0200 Subject: [PATCH] cache: do the cleanup inside synctest We did the init inside synctest but made the cleanup in the creation function which was trying to close routines with t.Cleanup which runs after synctest and try to close channels created inside synctest which cause the panic Signed-off-by: Mustafa Abdelrahman --- filters/cache/filter_test.go | 61 +++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/filters/cache/filter_test.go b/filters/cache/filter_test.go index 3ead92860c..832a08c1d8 100644 --- a/filters/cache/filter_test.go +++ b/filters/cache/filter_test.go @@ -23,7 +23,7 @@ import ( "github.com/zalando/skipper/proxy/proxytest" ) -func newTestFilter(t *testing.T, ttl, errorTTL, swrWindow time.Duration, extra ...time.Duration) *cacheFilter { +func newTestFilterWithCleanup(t *testing.T, ttl, errorTTL, swrWindow time.Duration, extra ...time.Duration) (*cacheFilter, func()) { t.Helper() spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second}) args := []any{ @@ -44,8 +44,12 @@ func newTestFilter(t *testing.T, ttl, errorTTL, swrWindow time.Duration, extra . cf.fetch = func(*http.Request) (*http.Response, error) { return nil, errors.New("no fetch stub set") } - t.Cleanup(spec.(*cacheSpec).client.Close) - t.Cleanup(func() { spec.(*cacheSpec).Close() }) + return cf, func() { spec.(*cacheSpec).Close() } +} + +func newTestFilter(t *testing.T, ttl, errorTTL, swrWindow time.Duration, extra ...time.Duration) *cacheFilter { + cf, cleanup := newTestFilterWithCleanup(t, ttl, errorTTL, swrWindow, extra...) + t.Cleanup(cleanup) return cf } @@ -54,7 +58,7 @@ func newTestFilter(t *testing.T, ttl, errorTTL, swrWindow time.Duration, extra . // Expires capping, heuristic freshness, or other RFC 9111 TTL logic. // The ttl/errorTTL/swrWindow args are accepted for call-site compatibility // but are ignored — pure RFC mode has no operator TTL. -func newTestFilterRFC(t *testing.T, _, _, _ time.Duration, _ ...time.Duration) *cacheFilter { +func newTestFilterRFCWithCleanup(t *testing.T, _, _, _ time.Duration, _ ...time.Duration) (*cacheFilter, func()) { t.Helper() spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second}) f, err := spec.CreateFilter([]any{}) @@ -65,8 +69,12 @@ func newTestFilterRFC(t *testing.T, _, _, _ time.Duration, _ ...time.Duration) * cf.fetch = func(*http.Request) (*http.Response, error) { return nil, errors.New("no fetch stub set") } - t.Cleanup(spec.(*cacheSpec).client.Close) - t.Cleanup(func() { spec.(*cacheSpec).Close() }) + return cf, func() { spec.(*cacheSpec).Close() } +} + +func newTestFilterRFC(t *testing.T, ttl, errorTTL, swrWindow time.Duration, extra ...time.Duration) *cacheFilter { + cf, cleanup := newTestFilterRFCWithCleanup(t, ttl, errorTTL, swrWindow, extra...) // dummy values + t.Cleanup(cleanup) return cf } @@ -142,7 +150,7 @@ func TestCacheFilter_KeyIsolationByAuthToken(t *testing.T) { t.Fatal(err) } f := fi.(*cacheFilter) - t.Cleanup(spec.(*cacheSpec).client.Close) + t.Cleanup(func() { spec.(*cacheSpec).Close() }) f.fetch = func(*http.Request) (*http.Response, error) { return nil, errors.New("no fetch stub set") @@ -267,7 +275,6 @@ func TestCacheFilter_Response_NoopIfStateBagKeyMissing(t *testing.T) { func TestCreateFilter_InvalidArgs(t *testing.T) { spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) t.Cleanup(func() { spec.(*cacheSpec).Close() }) cases := []struct { name string @@ -860,7 +867,8 @@ func TestCacheFilter_ConditionalRevalidation_ETag_304(t *testing.T) { var revalReq *http.Request synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + f, cleanup := newTestFilterWithCleanup(t, time.Millisecond, 15*time.Second, time.Hour) + defer cleanup() ctx1 := newCtx("GET", url, "") f.Request(ctx1) rsp := upstreamResponseCC(http.StatusOK, `{"data":"v1"}`, "max-age=300") @@ -909,7 +917,8 @@ func TestCacheFilter_ConditionalRevalidation_LastModified_304(t *testing.T) { var revalReq *http.Request synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + f, cleanup := newTestFilterWithCleanup(t, time.Millisecond, 15*time.Second, time.Hour) + defer cleanup() ctx1 := newCtx("GET", url, "") f.Request(ctx1) rsp := upstreamResponseCC(http.StatusOK, `{"data":"v1"}`, "max-age=300") @@ -944,7 +953,8 @@ func TestCacheFilter_RevalidationError_MetricIncremented(t *testing.T) { url := "https://cdn.contentful.com/spaces/abc/entries/reval-err" synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + f, cleanup := newTestFilterWithCleanup(t, time.Millisecond, 15*time.Second, time.Hour) + defer cleanup() mockMetrics := &metricstest.MockMetrics{} f.metrics = mockMetrics ctx1 := newCtx("GET", url, "") @@ -1341,7 +1351,6 @@ func TestCacheFilter_MustRevalidate_ForcesCoalesceWhenStale(t *testing.T) { func TestCacheFilter_SharedStorage_RouteIsolation(t *testing.T) { spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) t.Cleanup(func() { spec.(*cacheSpec).Close() }) makeFilter := func(t *testing.T) *cacheFilter { @@ -1868,7 +1877,8 @@ func TestCacheFilter_HopByHop_NotStored(t *testing.T) { func TestCacheFilter_304Merge_HopByHop_NotMerged(t *testing.T) { synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, 5*time.Minute, 10*time.Second, 10*time.Minute) + f, cleanup := newTestFilterWithCleanup(t, 5*time.Minute, 10*time.Second, 10*time.Minute) + defer cleanup() // 1. First request populates cache rsp1 := upstreamResponseCC(http.StatusOK, "body", "max-age=300") rsp1.Header.Set("ETag", `"v1"`) @@ -1910,7 +1920,8 @@ func TestCacheFilter_304Merge_HopByHop_NotMerged(t *testing.T) { func TestCacheFilter_Revalidate200_HopByHop_NotStored(t *testing.T) { synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, 5*time.Minute, 10*time.Second, 10*time.Minute) + f, cleanup := newTestFilterWithCleanup(t, 5*time.Minute, 10*time.Second, 10*time.Minute) + defer cleanup() // 1. First request populates cache rsp1 := upstreamResponseCC(http.StatusOK, "body", "max-age=300") rsp1.Header.Set("ETag", `"v1"`) @@ -2254,7 +2265,8 @@ func TestCacheFilter_ConditionalRequest_Stale_IfNoneMatch_304_AndRevalidates(t * // Stale entries must also honour client If-None-Match per RFC 9111 §4.3.2. // Background revalidation must still fire even when a 304 is served to the client. synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, 100*time.Millisecond, time.Second, 500*time.Millisecond) + f, cleanup := newTestFilterWithCleanup(t, 100*time.Millisecond, time.Second, 500*time.Millisecond) + defer cleanup() var revalFired atomic.Bool // Prime the cache via Request+Response so the entry is stored with ETag "v1". @@ -2638,7 +2650,6 @@ func TestCacheFilter_SMaxAge_CapsRouteTTL(t *testing.T) { func TestCacheFilter_CreateFilter_RFCArgParsing(t *testing.T) { spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: ":9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) t.Cleanup(func() { spec.(*cacheSpec).Close() }) cases := []struct { @@ -2683,7 +2694,6 @@ func TestCacheFilter_PureRFCMode_ZeroArgs_UsesUpstreamMaxAge(t *testing.T) { // cache() with no args: pure RFC mode, upstream max-age is fully authoritative, // no operator ceiling. TTL should equal upstream max-age exactly. spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: ":9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) t.Cleanup(func() { spec.(*cacheSpec).Close() }) f, err := spec.CreateFilter([]any{}) if err != nil { @@ -2718,8 +2728,7 @@ func TestCacheFilter_LRUBytesGaugeUpdatesWithoutEviction(t *testing.T) { // being inside the bubble is safe (it never actually dials out). synctest.Test(t, func(t *testing.T) { spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) - t.Cleanup(func() { spec.(*cacheSpec).Close() }) + defer spec.(*cacheSpec).Close() f, err := spec.CreateFilter([]any{"5m", "15s", "5m"}) if err != nil { t.Fatal(err) @@ -2763,7 +2772,6 @@ func TestCacheFilter_PureRFCMode_ZeroArgs_NoUpstreamDirective_NotCached(t *testi // cache() with no args: when upstream sends no Cache-Control, no Expires, // and no Last-Modified, nothing should be cached (no heuristic without Last-Modified). spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: ":9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) t.Cleanup(func() { spec.(*cacheSpec).Close() }) f, err := spec.CreateFilter([]any{}) if err != nil { @@ -2880,7 +2888,6 @@ func TestCacheFilter_RevalDropped_WhenQueueFull(t *testing.T) { func TestCacheSpec_FilterRegistry(t *testing.T) { spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) t.Cleanup(func() { spec.(*cacheSpec).Close() }) // Same args — should return same *cacheFilter pointer (registry hit) @@ -2936,7 +2943,6 @@ func TestCacheSpec_FilterRegistry_InFlightJobsSurviveRebuild(t *testing.T) { // targeting that instance are unaffected by the rebuild. spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second}) - t.Cleanup(spec.(*cacheSpec).client.Close) t.Cleanup(func() { spec.(*cacheSpec).Close() }) // Create initial filter with blocking fetch stub (same pattern as TestCacheFilter_RevalDropped_WhenQueueFull). @@ -3502,7 +3508,8 @@ func TestCacheFilter_Response_StorageSetError(t *testing.T) { func TestCacheFilter_Revalidate_304_EntryEvicted(t *testing.T) { synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + f, cleanup := newTestFilterWithCleanup(t, time.Millisecond, 15*time.Second, time.Hour) + defer cleanup() url := "https://cdn.example.com/reval-304-evicted" ctx1 := newCtx("GET", url, "") @@ -3557,7 +3564,8 @@ func TestCacheFilter_Revalidate_304_EntryEvicted(t *testing.T) { func TestCacheFilter_Revalidate_BodyReadError(t *testing.T) { synctest.Test(t, func(t *testing.T) { - f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + f, cleanup := newTestFilterWithCleanup(t, time.Millisecond, 15*time.Second, time.Hour) + defer cleanup() mockMetrics := &metricstest.MockMetrics{} f.metrics = mockMetrics url := "https://cdn.example.com/reval-body-err" @@ -3595,7 +3603,8 @@ func TestCacheFilter_Revalidate_BodyReadError(t *testing.T) { func TestCacheFilter_Revalidate_RFC_NoStore_NotStored(t *testing.T) { synctest.Test(t, func(t *testing.T) { - f := newTestFilterRFC(t, time.Millisecond, 15*time.Second, time.Hour) + f, cleanup := newTestFilterRFCWithCleanup(t, time.Millisecond, 15*time.Second, time.Hour) + defer cleanup() url := "https://cdn.example.com/reval-rfc-nostore" // Seed a cacheable entry first @@ -3646,7 +3655,7 @@ func TestCacheFilter_Revalidate_SetError_MetricIncremented(t *testing.T) { t.Fatal(err) } f := fi.(*cacheFilter) - t.Cleanup(func() { spec.(*cacheSpec).Close() }) + defer spec.(*cacheSpec).Close() url := "https://cdn.example.com/reval-set-err"