Repository navigation
Conversation
… Go tests
Replace the Go test sleeps that stood in for async synchronization with
waits that cannot lose to a slow scheduler:
- internal/testutil/goroutines: a dependency-free helper that reads
runtime.Stack(all) to list goroutines started after a mark. A go
statement is in the dump as soon as it executes, so a negative check
("this read spawned nothing") no longer needs a sleep window.
- scheduler: the Lambda and unmarked-read "no background refresh"
tests check the goroutine snapshot plus the GetGlobalConfig call log
instead of sleeping 20ms; the fan-out limit test parks every worker
and reads the in-flight count once errgroup.Go is blocked on the
limit, instead of a 5ms "overlap" sleep that passed on a serial loop.
- api: the RI-utilization SWR tests gate the fetcher on a channel and
wait with require.Eventually (no elapsed-time bound, no 250ms/200ms
sleeps); the singleflight test releases only after all 10 refreshes
are inside Group.Do. The DB rate limiter cleanup tests wait for the
cleanup goroutine to exit instead of sleeping.
- purchase: the multi-account parallelism test uses a two-party
barrier instead of a 300ms sleep and an elapsed-time bound.
- server: the scheduled-task integration test counts the collect call
instead of sleeping 100ms.
Sleeps that test real elapsed time (TTL windows, DB-clock ages) or
poll an external system with a generous deadline are kept. The
frontend setTimeout waits are left for the UI owner.
Refs LeanerCloud#36
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Several Go tests used
time.Sleepas a stand-in for synchronization: wait "long enough" for a goroutine to fire (or not fire), for a background write to land, or for workers to overlap. Those tests either flake under CI load or, worse, pass when the behavior they guard is broken. The 20ms negative windows in the scheduler tests are false-green against a slow wrongly-spawned goroutine, and the 5ms fan-out "overlap" sleep passed against a serial loop.Fix (test code only, no production change)
internal/testutil/goroutines: a small helper with no dependencies (a separate package, liketestutil/lambdatimeout, soschedulerandpurchasetests can import it without a cycle). It parsesruntime.Stack(all=true), which stops the world. A goroutine shows up in that dump as soon as itsgostatement runs, scheduled or not, and stays until its function returns.Mark.Spawned()/Mark.StartedBy()/Mark.Started()list goroutines started after a mark (goroutine IDs are never reused). It has its own unit tests, including one for a goroutine that hasn't been scheduled yet and one for nested spawns.TestScheduler_ListRecommendations_LambdaSkipsBackgroundRefresh,TestListRecommendations_UnmarkedColdStart_NeverMarksOrCollects,TestListRecommendations_UnmarkedStaleCache_NeverRefreshes: the 20ms sleep is gone. The newassertNoBackgroundCollecttakes a goroutine snapshot first and then checks theGetGlobalConfigcall log. A wrongly spawned collect is either still alive (it's in the snapshot) or already finished (and then it calledGetGlobalConfig, which the mock records whether or not it was expected). There's no window left for it to slip through.TestFanOutPerAccount_RespectsParallelismLimit: every fake call now parks on a channel. The test waits until the fan-out goroutine is blocked inerrgroup.(*Group).Goon the limit's semaphore and every worker it started has enteredfn, then reads the in-flight count, which must be exactly 3. It also assertspeak == 3(the old<= 3passed on a serial loop).TestRIUtilizationCache_StaleOnNonLambdaServesStaleAndKicksRefresh, plus its integration twinTestRIUtilizationCache_Integration_StaleNonLambdaServesStaleAndRefreshes: the fetcher blocks until the test releases it, sogetOrFetchreturning at all proves SWR served the stale copy. That replaces theelapsed < 50ms/< 100msbounds.require.Eventually(10s, 1ms/20ms polls) replaces the sleep-poll loops.TestRIUtilizationCache_SingleflightCollapsesConcurrentRefreshes: the 250ms head-start sleep and the 200ms settle sleep are gone. The test releases the fetch only once all 10 refreshes are accounted for: each one either called the fetcher or is parked insingleflight.(*Group).Doon the in-flight call'sWaitGroupas a duplicate. After that it waits for the fresh row to land and for every refresh goroutine to return.TestDBRateLimiter_maybeCleanup_ReadyToCleanupused to assert nothing; it now checkslastCleanupwas stamped and waits for the cleanup goroutine to exit and releasecleanupRunning.TestDBRateLimiter_StartCleanupWorker_StopsOnContextCancelwas vacuous: itsdonechannel closed as soon asStartCleanupWorkerreturned. It now checks a worker goroutine was started and waits for that goroutine to exit aftercancel(). It no longer shortensdbRateLimiterScheduledCleanupInterval. The worker reads that variable on its own goroutine, and once the sleep is gone, restoring it int.Cleanupis a race the race detector reports (1 in 20 under-race). A tick isn't observable with a nil pool anyway.TestExecuteMultiAccount_RunsAccountsInParallel: a two-party barrier inPurchaseCommitmentreplaces the 300ms sleep and theelapsed < 500msbound. Parallel calls meet at once; a serial loop leaves the first call alone at the barrier until it gives up after 10s, and the test fails on the overlap count.TestScheduledTaskIntegration(integration tag): drops the 100ms "simulate actual work" sleep and asserts the collect ran exactly once.The 10s limits are only there so a broken build fails instead of hanging; no test asserts on elapsed time.
Refs #36
This is partial: the frontend waits are left for the UI owner (below).
Verification
All evidence below is unit/mock/fixture level, plus one real-Postgres integration test (testcontainers). Tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.Build, vet, lint:
Unit packages touched,
-count=1:./internal/testutil/goroutines/,./internal/scheduler/,./internal/purchase/,./internal/server/,./internal/api/and./internal/mocks/allok.Stability,
-race -count=20on every converted test, all pass:An earlier
-count=20integration run on a busy shared machine gave 19 PASS and 1 FAIL. The failure was inside the container/migration setup (setupRICacheIntegration, line 35), before any of the changed code runs. A later-count=5run on the final code passed 5/5.Mutation probes. Each one changes production code, runs the converted test, and restores the file with
git checkout:!s.isLambdafrom the refresh guard inListRecommendationsgo func(){ sleep 50ms; CollectRecommendations }()mayCollectfrom the cold-start conditionmayCollectfrom the refresh guardg.SetLimit(-1)(no limit) infanOutPerAccountg.SetLimit(limit+1)g.SetLimit(1)(serial)if true || c.isLambda)kickBackgroundRefreshcallcleanupRunninglastCleanupnot stampedexecuteMultiAccountconcurrency forced to 1Fail-before evidence for the false greens: on
origin/main, with the 50ms-delayed-spawn mutations applied, the old Lambda and unmarked-read tests passed 60/60 (-count=20each). Withg.SetLimit(1), the old fan-out test passed 20/20.Conflicts with in-flight branches:
git merge-treeof this branch againstfork/fix/issue-111,fix/issue-326,fix/issue-343,fix/issue-353andfix/issue-84(#1099) is clean. #111 editsscheduler_test.goonly around lines 465-600, and #353 edits onlyTestVersionEndpointintests/e2e/e2e_test.go. No conversion had to be deferred.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Sleeps kept on purpose
internal/api/inmemory_rate_limiter_test.go:113,:201InMemoryRateLimitercallstime.Now()directly with no clock to inject. A sleep is a lower bound, so "sleep 2x the window, then expect expiry" can't flake. (See residual risk below.)internal/database/advisory_lock_holder_integration_test.go:93>= 1s): this is real elapsed time on the DB clock.internal/config/store_postgres_no_submission_integration_test.go:72updated_atis set by a DB trigger; the row has to age past the 1s floor on the DB clock.internal/server/http_scheduled_context_test.go:96WriteTimeout(100ms); the 400ms is the behavior under test.internal/api/ladder_resolve_race_integration_test.go:113,:117;internal/purchase/plan_step_fail_atomic_integration_test.go:247internal/server/http_shutdown_test.go:81,:115;http_scheduled_context_test.go:173;internal/purchase/plan_step_fail_atomic_integration_test.go:151;internal/config/store_postgres_ri_exchange_approved_reservation_integration_test.go:158;internal/server/scheduledauth/kidguard_test.go:216pg_stat_activity, or go-oidc's in-flight fetch. They are therequire.Eventuallypattern written out; converting them would be cosmetic.tests/e2e/e2e_test.go:60Out of scope / residual risk (not changed here)
TestInMemoryRateLimiter_Allow_WindowExpiryasserts that the second request is blocked inside a 10ms window. A pause longer than 10ms between the first and secondAllowwould let it through. A real fix needs a clock seam inInMemoryRateLimiter, which is a production change and was left alone.TestStartHTTPServerWaitsForBackgroundInitOnShutdownuses a 300mstime.Afternegative window ("did not return while init is parked"). It's not a sleep, but it has the same false-green shape against a slow wrong return.[chan send+errgroup.(*Group).Go(,singleflight.(*Group).Do(+sync.(*WaitGroup).Wait(). If anx/syncupgrade changes those internals, the tests fail loudly (theEventuallytimes out). They can't silently pass.Frontend follow-up for the UI owner
No
frontend/changes here. The frontend Jest suites now have 112 fixed non-zero waits (await new Promise(resolve => setTimeout(resolve, N))): 49 at 50ms, 54 at 10ms, 6 at 20ms and 3 at 100ms. The issue counted ~70. There are also 219 zero-delaysetTimeout(resolve, 0)macrotask flushes. Non-zero waits per file:frontend/src/__tests__/auth.test.ts(incl. lines 126, 162 from the issue)frontend/src/__tests__/plans.test.tsfrontend/src/__tests__/auth-mfa-enroll.test.tsfrontend/src/__tests__/history-retry-button.test.tsfrontend/src/__tests__/savings-history.test.tsfrontend/src/__tests__/history-approve-button.test.tsfrontend/src/__tests__/history-cancel-button.test.tsfrontend/src/__tests__/history-approval-queue.test.tsfrontend/src/__tests__/dashboard.test.tsfrontend/src/__tests__/plans-service-ids.test.tsfrontend/src/__tests__/plans-range-validation.test.tsfrontend/src/__tests__/riexchange.test.tsSuggested approach, in order of preference:
mockResolvedValuepromise andawaitit, then flush microtasks withawait Promise.resolve()), or use@testing-library'swaitFor(() => expect(api.login).toHaveBeenCalled...)if it's added.jest.useFakeTimers()withawait jest.advanceTimersByTimeAsync(ms); 16 suites already use fake timers.flushPromises()helper would replace most of the zero-delay flushes.Labels
A maintainer, please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/m.🤖 Generated with Claude Code