From 6141222ab013e6a7e9b4b4df8f98885bdb9c71b7 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 5 May 2026 11:24:43 -0700 Subject: [PATCH] fix(test/s3/policy): allocate fresh admin port per subtest (#9332) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(test/s3/policy): allocate fresh admin port per subtest startMiniCluster ran weed mini in-process and explicitly assigned master/volume/filer/s3 ports allocated by MustAllocatePorts, but it left -admin.port and -admin.port.grpc unset, so each subtest reused the hardcoded defaults 23646 / 33646. The package's subtests run sequentially within the same go test process. The previous subtest's admin goroutine is still bound to 23646 by the time the next subtest spins up its own mini, so the new admin can never bind, mini.go's waitForAdminServerReady hits its 240-attempt cap, and glog.Fatalf kills the test binary. This has been the dominant cause of "admin server did not become ready" flakes across recent IAM PRs. Allocate two extra ports for admin and pass them through. The other subprocess-based tests (s3tables/*) are not affected because each launches weed mini in a fresh OS process. * fix(mini): make admin readiness wait context-aware waitForAdminServerReady polled for 240 attempts × 500ms regardless of whether the surrounding mini context was cancelled. When mini is run in-process from a test harness (test/s3/policy/...) and the test calls its cancel func, the leftover wait keeps spinning for the full two minutes and then glog.Fatalf's, terminating the entire test binary — including any sibling subtest that has since started its own mini. Thread the existing miniClientsCtx through the wait so a Stop / cancel returns context.Canceled immediately. The caller (startMiniAdminWithWorker) treats a context-cancelled outcome as a graceful shutdown signal and logs+returns instead of fataling. --- test/s3/policy/policy_test.go | 11 ++++++++++- weed/command/mini.go | 27 +++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/test/s3/policy/policy_test.go b/test/s3/policy/policy_test.go index 6e0df7d47..488f52884 100644 --- a/test/s3/policy/policy_test.go +++ b/test/s3/policy/policy_test.go @@ -704,11 +704,18 @@ func uniqueName(prefix string) string { // --- Test setup helpers --- func startMiniCluster(t *testing.T) (*TestCluster, error) { - ports := testutil.MustAllocatePorts(t, 8) + // Allocate two extra ports for admin HTTP + gRPC. Without explicit + // values mini falls back to 23646/33646, which collide across the + // sequential subtests in this package: each subtest spins up a fresh + // mini in a goroutine, but the previous mini's admin goroutine is + // still binding 23646 — so the next test's admin can never become + // ready and mini.go fatals on its readiness check. + ports := testutil.MustAllocatePorts(t, 10) masterPort, masterGrpcPort := ports[0], ports[1] volumePort, volumeGrpcPort := ports[2], ports[3] filerPort, filerGrpcPort := ports[4], ports[5] s3Port, s3GrpcPort := ports[6], ports[7] + adminPort, adminGrpcPort := ports[8], ports[9] // Manually-managed temp dir (not t.TempDir()) so we control removal order: // the dir is removed inside Stop() AFTER the mini goroutine has fully @@ -779,6 +786,8 @@ enabled = true "-filer.port.grpc=" + strconv.Itoa(filerGrpcPort), "-s3.port=" + strconv.Itoa(s3Port), "-s3.port.grpc=" + strconv.Itoa(s3GrpcPort), + "-admin.port=" + strconv.Itoa(adminPort), + "-admin.port.grpc=" + strconv.Itoa(adminGrpcPort), "-webdav.port=0", "-admin.ui=false", "-master.volumeSizeLimitMB=32", diff --git a/weed/command/mini.go b/weed/command/mini.go index eb45c8648..860053752 100644 --- a/weed/command/mini.go +++ b/weed/command/mini.go @@ -1253,7 +1253,15 @@ func startMiniAdminWithWorker(allServicesReady chan struct{}) { // Wait for admin server's HTTP port to be ready before launching worker adminAddr := fmt.Sprintf("http://%s:%d", bindIp, *miniAdminOptions.port) glog.V(1).Infof("Waiting for admin server to be ready at %s...", adminAddr) - if err := waitForAdminServerReady(adminAddr); err != nil { + if err := waitForAdminServerReady(ctx, adminAddr); err != nil { + // If the parent context was cancelled (e.g. a previous in-process + // mini run is being torn down), bail out gracefully instead of + // fataling — the test harness uses `cmd.Run` directly so a Fatalf + // would terminate the entire test binary. + if ctx.Err() != nil { + glog.Warningf("Admin server readiness wait aborted: %v", err) + return + } glog.Fatalf("Admin server readiness check failed: %v", err) } @@ -1272,8 +1280,12 @@ func startMiniAdminWithWorker(allServicesReady chan struct{}) { waitForWorkerReady(workerGrpcAddr) } -// waitForAdminServerReady pings the admin server HTTP endpoint to check if it's ready -func waitForAdminServerReady(adminAddr string) error { +// waitForAdminServerReady pings the admin server HTTP endpoint to check if it's ready. +// Returns ctx.Err() (typically context.Canceled) if the parent context is +// cancelled mid-poll so a torn-down mini doesn't keep spinning for the full +// timeout — relevant when tests embed mini in-process and reuse the same +// goroutine pool across subtests. +func waitForAdminServerReady(ctx context.Context, adminAddr string) error { healthAddr := getHealthCheckAddr(fmt.Sprintf("%s/health", adminAddr)) // 240 * 500ms = 120 seconds max wait. The previous 30-second ceiling was // too tight on busy CI runners where master + filer + volume + admin all @@ -1287,6 +1299,9 @@ func waitForAdminServerReady(adminAddr string) error { } for attempt < maxAttempts { + if err := ctx.Err(); err != nil { + return err + } resp, err := client.Get(healthAddr) if err == nil { resp.Body.Close() @@ -1294,7 +1309,11 @@ func waitForAdminServerReady(adminAddr string) error { return nil } attempt++ - time.Sleep(500 * time.Millisecond) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(500 * time.Millisecond): + } } return fmt.Errorf("admin server did not become ready at %s after %d attempts", adminAddr, maxAttempts)