[IAmJSD]

Root cause

reinitializeAfterConnect (internal/server/app.go) starts the DB rate limiter's cleanup worker through startRateLimiterCleanup. That function runs the worker on the app's lifetime context, so the worker no longer dies with the request context that triggered the lazy DB connect (#269, fixed in #1134). The existing TestStartRateLimiterCleanup_SurvivesRequestCtxAndStopsOnClose only calls startRateLimiterCleanup directly. Nothing covered the call site, so changing it back to dbRL.StartCleanupWorker(ctx) reintroduced #269 and every test still passed.

Fix

• Test (the main ask): TestReinitializeAfterConnect_CleanupWorkerOutlivesRequestCtx builds a pre-DB Application with NewApplicationFromDeps and calls reinitializeAfterConnect with a cancelable request ctx and a zero database.Connection. No step touches the pool during wiring, the encryption key comes from CREDENTIAL_ENCRYPTION_KEY, and the AWS config loads from /dev/null. The test then cancels the request ctx. It checks the real api.DBRateLimiter worker goroutine directly: it counts goroutines whose "created by" frame is (*DBRateLimiter).StartCleanupWorker in a runtime.Stack dump. Exactly one worker must start. It must still be running 300 ms after the request ctx is canceled. It must exit after Close. No production seam was added.
• Related, bounded part (the issue's optional proposal): app.startRateLimiterCleanup(dbRL) moves from right after the limiter is built to the end of reinitializeAfterConnect, after every step that can fail. Before this change, if the AWS config load or the analytics collector or API handler construction failed, ensureDB closed the pool while the worker kept ticking against it until a retry replaced it or Close stopped it. TestReinitializeAfterConnect_NoCleanupWorkerWhenLaterStepFails forces the AWS config load to fail with a missing AWS_PROFILE. It asserts the error is that one, so the test can't pass by failing at an earlier step, and that no worker is left running. app.RateLimiter = dbRL stays where it was, so nothing else changes.

Closes #1137

Verification

All evidence is unit-level: in-process Application, no Postgres, no AWS calls. Real-Postgres coverage of the rate limiter is still TestApplicationRateLimiter (integration tag). I didn't run it here.

Mutation probe, the exact revert from the issue (app.startRateLimiterCleanup(dbRL) -> dbRL.StartCleanupWorker(ctx)):

# on origin/main + new test only: passes
--- PASS: TestReinitializeAfterConnect_CleanupWorkerOutlivesRequestCtx (0.30s)

# same, with the revert applied: fails
--- FAIL: TestReinitializeAfterConnect_CleanupWorkerOutlivesRequestCtx (0.00s)
    Error: Condition satisfied
    Messages: cleanup worker stopped with the request ctx; it must run on the app lifetime ctx

# on this branch, with the revert applied at the new position: fails the same way

Failure-path test, fail before / pass after the move:

# origin/main app.go
--- FAIL: TestReinitializeAfterConnect_NoCleanupWorkerWhenLaterStepFails
    expected: 0  actual: 1
    Messages: a failed reinitializeAfterConnect must not leave a cleanup worker running
# this branch
--- PASS: TestReinitializeAfterConnect_NoCleanupWorkerWhenLaterStepFails

Commands (run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is broken; the new tests also set both to /dev/null themselves):

go build ./...
go vet ./internal/server/ && go vet -tags integration ./internal/server/
go test ./internal/server/ -count=1                       # ok
go test -race ./internal/server/ -run 'TestReinitializeAfterConnect_|TestStartRateLimiterCleanup' -count=10   # ok
go test ./internal/server/ -run 'TestReinitializeAfterConnect_|TestStartRateLimiterCleanup' -count=3 -shuffle=on  # ok
go test ./internal/mocks/ -count=1                        # ok
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/server/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/server/app.go                # silent
bash scripts/gosec-hook.sh internal/server/app.go internal/server/app_rl_cleanup_test.go   # rc=0
git-secrets --scan (fresh git init, isolated HOME, repo setup + .gitallowed) on both files  # clean
bash scripts/test-git-secrets-allowlist.sh                # 42 PASS, 0 FAIL

Because disk space on this machine is tight, only ./internal/server and ./internal/mocks were run, not the whole repo.

Notes / out of scope

• credentials.LoadKey memoizes once per process. The new tests are the first unit tests in internal/server to load the key, and both use the same env. A future unit test in this package that loads the key under a different env could be affected depending on test order. This is unchanged pre-existing behavior.
• The goroutine count matches the created by ...(*DBRateLimiter).StartCleanupWorker frame. If that method is renamed, the test fails loudly at "must start one cleanup worker" rather than passing vacuously.

Maintainer: please mirror the issue's triage labels on this PR: impact/internal, priority/p3, severity/low, urgency/this-quarter, effort/xs, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 59 minutes for your next included review.

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: 5d4ab6b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 851bf051-6a94-4867-9830-fb82ffc24581

Commits

Reviewing files that changed from the base of the PR and between 5d4ab6b and 1bb6a9a.

Files selected for processing: 2

• internal/server/app.go
• internal/server/app_rl_cleanup_test.go

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

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

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 1bb6a9a

Findings (none blocking):

• [non-blocking] internal/server/app_rl_cleanup_test.go:43-56 (liveDBRLWorkers) - The test counts worker goroutines across the whole process with runtime.Stack.
  • A goroutine left over from an earlier sequential test can still be exiting when before is sampled.
  • app_islambda_integration_test.go:56-58 starts real workers and closes them in t.Cleanup, and the new tests also compile under -tags integration.
  • So in an integration run the count can drop on its own. Either require.Equal(before+1) or the assert.Never(count<=before) check could then fail.
  • The window is tiny: 0 failures in 20 race runs and 5 shuffled runs.
• [non-blocking, pre-existing] internal/server/app.go:834-836 - When a later step fails, app.RateLimiter = dbRL (and app.Auth etc.) stay assigned to stores on the pool that ensureDB then closes (app.go:698-701). The PR does not make this worse.

Root cause:

• Base already had the right call site (startRateLimiterCleanup on lifeCtx), but no test covered it.
• The PR adds that coverage and moves the start after the last step that can fail (app.go:939-942, after the API-handler nil check).
• Every return err in reinitializeAfterConnect (app.go:800-937) now comes before the start.

Attacked:

• Callers: reinitializeAfterConnect is called only from ensureDB (app.go:698), and startRateLimiterCleanup only at app.go:942 and in tests. The DBRateLimiter worker is unchanged.
• Nil pool: the tests pass &database.Connection{}. The worker touches the pool only on a 10m tick, and panics there are recovered (db_rate_limiter.go:58-75).
• Fail-before, with the head test file on base:
  • CleanupWorkerOutlivesRequestCtx passes on base, as expected, because base's call site was already correct.
  • NoCleanupWorkerWhenLaterStepFails FAILS ("a failed reinitializeAfterConnect must not leave a cleanup worker running").
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 5 were caught:
  • M1, the call site changed to dbRL.StartCleanupWorker(ctx) (the fix(server): DB rate-limiter cleanup worker runs on a request context and never ticks #269 regression): Outlives FAILS.
  • M2, the start moved back to right after NewDBRateLimiter: NoCleanupWorker FAILS.
  • M3, the start removed: Outlives FAILS.
  • M4, Close no longer calls lifeCancel: both lifecycle tests FAIL.
  • M5, the worker ctx derived from context.Background: both lifecycle tests FAIL.
• Races: -race on the package, plus -count=20 on the new tests, found no data race.

Verified locally, on the tree merged with origin/main (00a3f40, which includes f5ad8c6), with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• Merge: git merge-tree --write-tree origin/main 1bb6a9a5 is clean.
• Static checks: go build ./... and go vet ./internal/server/, plain and with -tags integration, are OK. gofmt -l is clean.
• Unit and race runs:
  • go test -count=1 ./internal/server/: ok.
  • go test -race -count=20 -run 'TestReinitializeAfterConnect_|TestStartRateLimiterCleanup_' ./internal/server/: PASS, no races.
  • go test -race -count=1 ./internal/server/: ok.
  • -count=5 -shuffle=on on the new tests: ok.
• Integration and api runs:
  • go test -tags integration -count=1 -run 'RateLimit|Reinit' ./internal/server/ (testcontainers): ok. The full integration suite was not run.
  • go test ./internal/api/ -run 'DBRateLimiter|Cleanup': ok.
• No migrations, frontend or terraform changes.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 1bb6a9a

Independent adversarial review of #1216 (closes #1137) at 1bb6a9a, base main. The diff is a 7-line move in internal/server/app.go plus two tests in internal/server/app_rl_cleanup_test.go. It covers both parts of #1137:

• The reinitializeAfterConnect call site is now pinned against the fix(server): DB rate-limiter cleanup worker runs on a request context and never ticks #269 revert by a test that drives the real api.DBRateLimiter worker.
• The optional part is done: the worker now starts only after every step that can fail, so a failed attempt (after which ensureDB closes the pool, app.go:698-701) no longer leaves a worker on the closed pool.

Findings: no blocking findings.

• nit internal/server/app.go:833 - on a failed attempt, app.RateLimiter still points at the limiter built on the now-closed pool, as do app.Config and app.Auth. This predates the PR. ensureDB returns the error, and the next attempt replaces them, so I found no failure path.

Attacked:

• Ran go test ./internal/server/ -count=1: pass. Also -count=2 -shuffle=on: pass, so the memoized credentials.LoadKey shows no order dependence. -race -count=5 on the new and existing cleanup tests: pass. go vet (default and -tags integration): clean. gofmt -l: empty.
• Ran the new tests with an outer AWS_PROFILE pointing at a missing profile: they still pass, because the test overrides AWS_PROFILE, the config files and IMDS, so the success test cannot fail on the host's AWS setup.
• Checked that the zero database.Connection{} cannot reach the pool during the 300 ms window. The scheduled cleanup ticks every 10 minutes (internal/api/db_rate_limiter.go:35), and the constructors only store the pool.
• Checked the goroutine-count oracle. It is relative to before, and t.Setenv keeps both tests sequential. If the method were renamed, the require at line 101 fails loudly instead of passing vacuously.
• Blast radius: startRateLimiterCleanup has one production caller. No integration test asserts the start order. This PR does not touch store, SQL or migration code, so I ran no testcontainers integration tests.
• Mutation probes in a scratch copy, each reverted:
  1. The exact test(server): pin the rate-limiter cleanup start in reinitializeAfterConnect #1137 revert (dbRL.StartCleanupWorker(ctx) at the new position): OutlivesRequestCtx FAILs.
  2. Move the start back to right after the limiter is built: NoCleanupWorkerWhenLaterStepFails FAILs.
  3. Remove the start entirely: OutlivesRequestCtx FAILs.
  4. Derive the worker ctx from context.Background() instead of lifeCtx: both the existing SurvivesRequestCtxAndStopsOnClose and the new OutlivesRequestCtx FAIL.

[IAmJSD]

Local verification at 1bb6a9a510091d62b94d48de498765bf776170f2 merged onto main (2514c01a), together with #1203, #1207, #1211, #1214, #1216 in that order (combined tree 995207c1, 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 1bb6a9a5, green CI, and the combined-tree verification above. Squash-merged at exactly 1bb6a9a510091d62b94d48de498765bf776170f2.