From 291a7374223a17ca70fbeb6436ea3c851f9e0dee Mon Sep 17 00:00:00 2001 From: Nick <126941599+coo1white@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:41:44 +0700 Subject: [PATCH] test: stop four concurrency tests from failing on a busy machine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four pass on a quiet box and fail on a loaded one, each for its own reason. None of them is testing the clock, so none of them should be failing on it. - ollama_cloud_usage_test.go: the second caller was released with `close(release)` right after its goroutine was started, not after it had reached the singleflight group. When the first refresh won that race, the second became a new singleflight execution, re-read the account, saw the LastAttemptAt the first one had just written, and came back with the 30-second manual-refresh 429 at line 685. It now counts account loads and waits for the second caller's own load, which happens right before it joins the group. Adds a counting GetByID to the test repo. - gateway_hotpath_optimization_test.go: a 20ms sleep was meant to let all 12 callers reach the cache before the loader was released. A caller that arrived after the load had finished got a hit, not a miss, so the miss count came out 11 of 12. It now waits on the miss counter itself, which is the value the test asserts on. - token_refresh_pool_health_test.go: the floor was `configuredSpacing` minus 10ms, i.e. 40ms out of 50ms. Each start timestamp is taken after the rate gate releases the goroutine, so scheduler delay can compress one observed gap with the gate behaving correctly — seen at 37ms and again at 13ms. The floor is now a tenth of the configured spacing. Measured with providerQPS=20, 8 attempts, concurrency 2: gate at 50ms gives a minimum gap of 49.97ms, gate at 0 gives 22µs. So an unpaced gate sits three orders of magnitude under the 5ms floor and is still caught, while jitter has room to move. A comment warns against replacing this with an assertion on the total span of the starts: the span is set by how long each attempt takes under the concurrency limit, not by the gate — 471ms paced against 241ms unpaced — so a span check passes with the gate disabled. - prompt_guard_test.go: the bound only has to show the failover shared the first endpoint's 70ms deadline and did not take the second endpoint's own 500ms one. An unshared deadline lands near 535ms, so 350ms still fails loudly (seen: 224ms against a 180ms bound). Tests only; no product code is touched. Each fix was checked in both directions: it passes with the behaviour intact, and it still fails when the behaviour is broken on purpose (for the QPS one, by swapping the shared rate gate for a zero-interval one). --- .../securityaudit/prompt_guard_test.go | 7 +++++- .../gateway_hotpath_optimization_test.go | 10 +++++++- .../service/ollama_cloud_usage_test.go | 20 ++++++++++++++++ .../service/token_refresh_pool_health_test.go | 24 ++++++++++++++++++- 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/backend/internal/securityaudit/prompt_guard_test.go b/backend/internal/securityaudit/prompt_guard_test.go index 76e9dea05e..2346857ea5 100644 --- a/backend/internal/securityaudit/prompt_guard_test.go +++ b/backend/internal/securityaudit/prompt_guard_test.go @@ -222,7 +222,12 @@ func TestGuardEvaluatorFlagSharedDeadlineFailClosedAndContextCancel(t *testing.T elapsed := time.Since(started) require.Error(t, err) require.Equal(t, 2, calls) - require.Less(t, elapsed, 180*time.Millisecond) + // The bound only has to prove the failover shared the first endpoint's + // 70ms deadline instead of taking the second endpoint's own 500ms one. + // An unshared deadline lands at ~535ms, so 350ms still fails loudly + // while leaving room for scheduler delay on a busy CI machine. A + // tighter bound made this test flaky, not stricter. + require.Less(t, elapsed, 350*time.Millisecond) require.GreaterOrEqual(t, elapsed, 50*time.Millisecond) require.Equal(t, int64(1), metrics.Snapshot().Failovers) require.Equal(t, int64(1), metrics.Snapshot().Timeouts) diff --git a/backend/internal/service/gateway_hotpath_optimization_test.go b/backend/internal/service/gateway_hotpath_optimization_test.go index e5bf49b8b7..13fc32165f 100644 --- a/backend/internal/service/gateway_hotpath_optimization_test.go +++ b/backend/internal/service/gateway_hotpath_optimization_test.go @@ -219,7 +219,15 @@ func TestGetUserGroupRateMultiplier_UsesCacheAndSingleflight(t *testing.T) { } close(start) - time.Sleep(20 * time.Millisecond) + // Wait for every caller to have recorded its cache miss before releasing the + // loader. A fixed sleep raced here: a goroutine that reached the cache after + // the singleflight load had already finished got a hit instead of a miss, and + // the miss assertion below saw 11 of 12. The miss counter is the observable + // that says "all callers are now inside the singleflight group". + require.Eventually(t, func() bool { + _, miss, _, _, _ := GatewayUserGroupRateCacheStats() + return miss == int64(concurrent) + }, 5*time.Second, time.Millisecond, "all callers must miss the cache before the loader is released") close(unblock) wg.Wait() diff --git a/backend/internal/service/ollama_cloud_usage_test.go b/backend/internal/service/ollama_cloud_usage_test.go index d315d7eb41..4b4a5aadb6 100644 --- a/backend/internal/service/ollama_cloud_usage_test.go +++ b/backend/internal/service/ollama_cloud_usage_test.go @@ -36,6 +36,14 @@ type ollamaUsageTestRepo struct { disableAutoAttempts atomic.Int64 disableAutoCalls atomic.Int64 groupResolveCalls atomic.Int64 + getByIDCalls atomic.Int64 +} + +// GetByID counts loads so a test can wait for a caller to reach the point just +// before the singleflight group, instead of guessing with a sleep. +func (r *ollamaUsageTestRepo) GetByID(ctx context.Context, id int64) (*Account, error) { + r.getByIDCalls.Add(1) + return r.upstreamBillingProbeAccountRepo.GetByID(ctx, id) } func (r *ollamaUsageTestRepo) ListOllamaCloudUsageGroupAccounts(_ context.Context, anchors []*Account) ([]Account, error) { @@ -805,7 +813,19 @@ func TestOllamaCloudUsageRefreshSingleflightAndRunnerDeduplicateSharedGroup(t *t errs := make(chan error, 2) go func() { _, err := svc.Refresh(context.Background(), first.ID); errs <- err }() <-started + // The first caller is now parked in the stub, having loaded the account twice + // (once to build the group key, once inside the singleflight function). + loadsBeforeSecond := repo.getByIDCalls.Load() go func() { _, err := svc.Refresh(context.Background(), second.ID); errs <- err }() + // Only release the first caller once the second one has loaded its own + // account, which happens immediately before it joins the singleflight group. + // Releasing right after starting the goroutine raced: if the first refresh + // finished first, the second became a fresh singleflight execution, re-read + // the account, saw the LastAttemptAt just written, and failed with the 30s + // manual-refresh 429 instead of sharing the in-flight result. + require.Eventually(t, func() bool { + return repo.getByIDCalls.Load() > loadsBeforeSecond + }, 5*time.Second, time.Millisecond, "the second caller must reach the singleflight group before the first is released") close(release) require.NoError(t, <-errs) require.NoError(t, <-errs) diff --git a/backend/internal/service/token_refresh_pool_health_test.go b/backend/internal/service/token_refresh_pool_health_test.go index f4950d879f..a9bd760d95 100644 --- a/backend/internal/service/token_refresh_pool_health_test.go +++ b/backend/internal/service/token_refresh_pool_health_test.go @@ -630,7 +630,29 @@ func TestTokenRefreshService_SaturatedProviderPreservesConcurrencyAndActualQPSSt starts := refresher.startsSnapshot() require.Len(t, starts, attemptCount) configuredSpacing := time.Second / time.Duration(providerQPS) - minimumObservedSpacing := configuredSpacing - 10*time.Millisecond + + // The floor is a tenth of the configured spacing, not spacing minus a few + // milliseconds. Each start timestamp is taken after the rate gate releases + // the goroutine, so scheduler delay can compress one observed gap without + // the gate having done anything wrong: this assertion failed on a loaded + // machine at 13ms and again at 37ms against a 40ms floor, both times with + // the gate pacing correctly. + // + // A tenth still separates the two cases by a wide margin. Measured with + // providerQPS=20 (50ms spacing), 8 attempts, providerConcurrency=2: + // + // gate at 50ms -> minimum observed gap 49.97ms + // gate at 0 -> minimum observed gap 22µs + // + // So an unpaced gate lands three orders of magnitude below the 5ms floor + // and is still caught, while jitter on a busy machine has room to move. + // + // Do not swap this for an assertion on the total span of the starts: the + // span is dominated by how long each attempt takes under + // providerConcurrency, not by the gate. Same measurement — 471ms paced + // against 241ms unpaced — so a span check passes with the gate disabled and + // tests nothing. + minimumObservedSpacing := configuredSpacing / 10 actualMinimumSpacing := starts[1].Sub(starts[0]) for i := 1; i < len(starts); i++ { spacing := starts[i].Sub(starts[i-1])