[IAmJSD]

Summary

This finishes the Go part of #36 (wall-clock sleeps used for async synchronization in tests). #1298 already removed most of the Go sleeps. This PR handles the three that were left:

• internal/api/inmemory_rate_limiter_test.go: TestInMemoryRateLimiter_Allow_WindowExpiry slept 20ms to wait out a 10ms window, and TestInMemoryRateLimiter_GarbageCollection slept 10ms to wait out a 1ms window. The limiter read time.Now() directly, so a sleep was the only way to expire a window.
• internal/server/http_shutdown_test.go: TestStartHTTPServerWaitsForBackgroundInitOnShutdown showed that "StartHTTPServer does not return while the initializer is still running" by watching for 300ms. On a slow runner a wrong implementation could pass inside that window.

Root cause and fix

Rate limiter clock. InMemoryRateLimiter now has an injectable now func() time.Time field, read through rl.clock(). This is the same pattern as Handler.now / h.clock(). Nil, the production default, means time.Now, so struct literals and NewInMemoryRateLimiter() behave exactly as before. A test-only fakeClock moves time forward with Advance.

• TestInMemoryRateLimiter_Allow_WindowExpiry now uses a one-hour window, so only the injected clock can expire it. It also pins the boundary: at exactly resetTime the request is still blocked (now.After is strict), one nanosecond later it is allowed, and the new window enforces the limit again.
• TestInMemoryRateLimiter_GarbageCollection used to assert nothing beyond "no error". It also used only 26 distinct keys, so it never reached the cap path it was named for. It now fills the map to inMemoryRateLimitMaxEntries, with one long-window entry (the oldest) and the rest on a short window. It then advances the clock past the short window and triggers the cap check. It asserts that the expired-purge step alone brings the map down to 2 entries. Oldest-first eviction would leave 401 and would drop the live entry. It also asserts that the live entry keeps its count.

Shutdown wait signal. internal/server/http.go gets a package-level shutdownInitWaitHook func(). It is nil in production, and tests set it the same way they already shorten shutdownGracePeriod. When set, it runs on the StartHTTPServer goroutine right before the select on initDone, which is after server.Shutdown and the scheduled-task stop. The test:

1. waits for the hook, which records the server goroutine's ID. If StartHTTPServer returns first, the test fails at once.
2. dials the port once. Shutdown has already run by then, so the dial must be refused. This replaces the old polling loop and still pins "drain before waiting".
3. waits, with internal/testutil/goroutines (added in test: wait on goroutines and barriers instead of wall-clock sleeps in Go tests #1298), until that goroutine is parked in [select] with internal/server.StartHTTPServer as its innermost frame. After the hook, that select is the wait for the initializer.
4. checks without blocking that returned is empty. A goroutine parked in that select cannot return until initDone closes, and initDone cannot close while the resolver is held. (The 30s grace period is the only other way out.)
5. releases the resolver and requires StartHTTPServer to return nil.

No time window decides the result. The remaining time.After(5 * time.Second) calls are failure deadlines only. The test went from about 0.30s to about 0.00s.

Alternatives considered: a hook alone, followed by a non-blocking check on returned, races with a mutant that keeps the hook and drops the wait. Inspecting goroutines alone, with no hook, cannot tell the first select (serve vs. signal) apart from the init-wait select without matching line numbers. Using both together makes the check deterministic.

Refs #36

Verification

All evidence here is local unit tests on Linux (Go 1.26.9). None of it needs a database or a cloud API. Commands were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

Fail-before: the rewritten tests use rl.now and shutdownInitWaitHook, so they do not compile against origin/main. Fail-before is therefore shown with mutation probes on the production code. Each probe was applied, run, and reverted:

Probe (production change)	Command	Result
Allow reads time.Now() instead of rl.clock()	go test ./internal/api/ -run 'InMemoryRateLimiter_(Allow_WindowExpiry|GarbageCollection)' -count=1	both FAIL ("request after the window expired must be allowed"; "expected: 2 actual: 401")
Window expires at resetTime (!now.Before instead of now.After)	go test ./internal/api/ -run InMemoryRateLimiter_Allow_WindowExpiry -count=1	FAIL ("request at exactly the reset time must still be blocked")
Expired-entry purge disabled in evictIfAtCap	go test ./internal/api/ -run InMemoryRateLimiter_GarbageCollection -count=1	FAIL ("expected: 2 actual: 401")
StartHTTPServer drops the initDone wait (hook kept)	go test ./internal/server/ -run 'TestStartHTTPServerWaitsForBackgroundInitOnShutdown$' -count=20	20/20 FAIL ("StartHTTPServer returned () instead of waiting for the background initializer")
Hook + initDone wait moved before server.Shutdown	same, -count=20	20/20 FAIL ("listener still accepting connections when StartHTTPServer reached the initializer wait")

Pass-after and stability:

go test ./internal/api/ -run 'InMemoryRateLimiter' -count=20                    # ok
go test ./internal/server/ -run 'TestStartHTTPServer(WaitsForBackgroundInitOnShutdown|ShutdownWaitIsBounded)$' -count=20   # ok
go test -race ./internal/server/ -run 'TestStartHTTPServer(WaitsForBackgroundInitOnShutdown|ShutdownWaitIsBounded)$' -count=20   # ok
go test -race ./internal/api/ -run 'InMemoryRateLimiter' -count=20              # ok
go test ./internal/api/ -count=1                                                # ok
go test ./internal/server/ -count=1                                             # ok
go test ./internal/mocks/ -count=1                                              # ok
go test ./internal/testutil/goroutines/ -count=1                                # ok
go build ./... && go vet ./... && go vet -tags integration ./...                # ok
golangci-lint run --allow-parallel-runners ./internal/api/                      # 0 issues
golangci-lint run --allow-parallel-runners ./internal/server/                   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/inmemory_rate_limiter.go internal/server/http.go   # none over 10

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ c00ef619 = origin/main 15dc8feb plus these heads, merged in this order:

Branch	Tested head
fix/issue-712-fargate	2df9723
fix/issue-41-ri-exchange-wiring	bb548fe
fix/issue-349-ci-hygiene	360ae6c
fix/issue-756-next-collection	2cb1b2f
fix/issue-36-go-clock	aaea757
fix/issue-36-fe-waits	7feb01d
fix/issue-35-purchase-asserts	fbafeb0
fix/issue-134-mfa-token	33da813

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. The full suite ran on 644bfcaf, which differs from this tree only by a test-only rename on fix/issue-134-mfa-token (a password-like constant that git-secrets flagged):

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase and internal/server all pass. Migration 000152 applies on top of main's 000151.
• Frontend: the full jest suite passes on this tree (175 suites, 4595 tests, 1 skipped).
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns 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: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run on this tree (CI runs them). Nothing was applied to any cloud.

Out of scope

• The frontend setTimeout waits in TEST-04: Wall-clock sleeps for async synchronization (~90 frontend setTimeout waits, several Go sleeps) #36 (frontend/src/__tests__/...) are handled on another branch by the UI owner. This PR changes nothing under frontend/.
• Sleeps that measure real elapsed time against a DB clock or real TTLs, and the poll loops with a generous deadline, were kept on purpose in test: wait on goroutines and barriers instead of wall-clock sleeps in Go tests #1298 and are untouched here. That includes the 10ms bind-poll in startServerWithBlockedInit, which waits for the listener to come up and is not a negative-assertion window.
• TestStartHTTPServerShutdownWaitIsBounded still uses a 200ms shutdownGracePeriod. It tests the real grace-period bound, so that elapsed time is the thing under test.

Maintainers: please mirror the issue's triage labels on this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/m.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 15dc8fe
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 351400fd-1860-4003-9fee-bcd7f8384915

Commits

Reviewing files that changed from the base of the PR and between 15dc8fe and aaea757.

Files selected for processing: 4

• internal/api/inmemory_rate_limiter.go
• internal/api/inmemory_rate_limiter_test.go
• internal/server/http.go
• internal/server/http_shutdown_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE aaea7571a1127f16b2938c07d03d7044553364b9

Blocking:

• none

Non-blocking:

• internal/server/http_shutdown_test.go blockedInStartHTTPServerSelect: the check relies on the goroutine dump format, specifically that the innermost visible frame of a parked goroutine is internal/server.StartHTTPServer(. That format is stable in Go today. I also ran the test with GOTRACEBACK=system, which shows runtime frames in tracebacks, and it still passes. If a future Go release changes the format, the test fails loudly through its 5s deadline, so there is no false green. Acceptable.
• Body claim "This PR handles the three that were left": a few Go sleeps still synchronize async work rather than measure elapsed time. For example, internal/archera/provider_cancel_test.go:111 uses slowResolver to sleep 100ms so concurrent callers overlap. That was outside the issue's listed sites, and the PR correctly says Refs #36, not Closes. Worth a line on TEST-04: Wall-clock sleeps for async synchronization (~90 frontend setTimeout waits, several Go sleeps) #36 so it isn't closed on the strength of this PR plus the frontend one.

Attacked:

• Production changes:
  • InMemoryRateLimiter.now is nil by default, and clock() falls back to time.Now. Nothing outside the tests sets it, and all reads happen under rl.mu.
  • shutdownInitWaitHook is nil in production and runs after server.Shutdown and scheduledTasks.stop, right before the initDone select.
• Shared global: no other test that runs StartHTTPServer uses t.Parallel, and each one waits for StartHTTPServer to return before cleanup, so the global write can't race. Confirmed with -race -count=30 on the TestStartHTTPServer* and InMemoryRateLimiter* tests: ok.
• Fail-before by mutation, since the new tests don't compile against main:
  • Dropping the initDone select (hook kept): TestStartHTTPServerWaitsForBackgroundInitOnShutdown fails 5/5 with "returned () instead of waiting for the background initializer".
  • Switching Allow to time.Now(): Allow_WindowExpiry fails ("request after the window expired must be allowed"), and so does GarbageCollection ("expected: 2").
  • Both restored afterwards.
• Also ran go build ./..., go vet with and without -tags integration on internal/api and internal/server, go test -race -short -count=1 ./internal/api/ ./internal/server/ (ok), and golangci-lint on both packages with and without --build-tags integration (0 issues).
• GarbageCollection really reaches the cap: it fills exactly inMemoryRateLimitMaxEntries and asserts the length before the trigger.

Body-stale:

• none

_{Reviewer: rI1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at aaea7571a1127f16b2938c07d03d7044553364b9 merged onto main (76180820), together with #1571, #1568, #1570 in that order (combined tree cf2e973e, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on aaea7571, green CI, and the combined-tree verification above. Squash-merged at exactly aaea7571a1127f16b2938c07d03d7044553364b9.