From 644bb5d27ee93f164b65f7a3330413138e6ab5c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Sat, 15 Aug 2026 11:18:42 +0200 Subject: [PATCH 1/2] test: fix flaky TestAddModuleWorkerViaAdminApi The test POSTs a full config to the admin /load endpoint, which swaps the HTTP listener for the new config, then immediately GETs the newly added worker. A request racing that swap can hit a reset connection (EOF) before the worker ever sees it, as seen on CI (php/frankenphp#2604 CI run, and previously on #2412/#2381 per #2413 for the sibling autoscale tests). Retry only on connection-level errors, not on the request itself: once a request reaches the worker it always increments its counter, so retrying past that point would break the "requests:1" assertion. --- caddy/admin_test.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/caddy/admin_test.go b/caddy/admin_test.go index 27ece031f3..3f24285f02 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -9,6 +9,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/dunglas/frankenphp/internal/fastabs" @@ -345,6 +346,24 @@ func TestAddModuleWorkerViaAdminApi(t *testing.T) { assert.Greater(t, updatedWorkerCount, initialWorkerCount, "Worker count should have increased") assert.True(t, workerFound, fmt.Sprintf("Worker with name %q should be found", "Worker PHP Thread - "+filename)) - // Make a request to the worker to verify it's working - tester.AssertGetResponse("http://localhost:"+testPort+"/worker-with-counter.php", http.StatusOK, "requests:1") + // The /load above swaps the HTTP listener for the new config; a request + // racing that swap can see a reset connection before the worker ever + // receives it, so retry on connection-level failures only (a request + // that reaches the worker always counts, so retrying past that point + // would throw off the "requests:1" assertion below). + workerURL := "http://localhost:" + testPort + "/worker-with-counter.php" + var getResp *http.Response + for i := 0; i < 20; i++ { + getResp, err = http.Get(workerURL) + if err == nil { + break + } + time.Sleep(50 * time.Millisecond) + } + require.NoError(t, err) + defer func() { require.NoError(t, getResp.Body.Close()) }() + body, err := io.ReadAll(getResp.Body) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, getResp.StatusCode) + assert.Equal(t, "requests:1", string(body)) } From 9f746dc9b46ab1a9f77a99ab06b69de6c57cb909 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Sat, 15 Aug 2026 17:00:56 +0200 Subject: [PATCH 2/2] test: use require.Eventually for the post-reload retry Matches the existing retry idiom used elsewhere in this codebase (frankenphp_test.go, finishrequest_realserver_test.go, worker_internal_test.go, caddy/watcher_test.go, caddy/caddy_test.go) instead of a bespoke for-loop. --- caddy/admin_test.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/caddy/admin_test.go b/caddy/admin_test.go index 3f24285f02..7129bab103 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -353,14 +353,10 @@ func TestAddModuleWorkerViaAdminApi(t *testing.T) { // would throw off the "requests:1" assertion below). workerURL := "http://localhost:" + testPort + "/worker-with-counter.php" var getResp *http.Response - for i := 0; i < 20; i++ { + require.Eventually(t, func() bool { getResp, err = http.Get(workerURL) - if err == nil { - break - } - time.Sleep(50 * time.Millisecond) - } - require.NoError(t, err) + return err == nil + }, 1*time.Second, 50*time.Millisecond) defer func() { require.NoError(t, getResp.Body.Close()) }() body, err := io.ReadAll(getResp.Body) require.NoError(t, err)