diff --git a/internal/server/app.go b/internal/server/app.go index fd92d96bf..ab624c753 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -832,8 +832,6 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da app.Auth = authSvc dbRL := api.NewDBRateLimiter(dbConn.Pool()) - // Periodic cleanup also evicts expired keys with no subsequent allowed request. - app.startRateLimiterCleanup(dbRL) app.RateLimiter = dbRL log.Println("Initialized database-backed rate limiter with shared replica counters") @@ -938,6 +936,11 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da return fmt.Errorf("failed to create API handler") } + // Periodic cleanup also evicts expired keys with no subsequent allowed + // request. Started last: on any earlier error ensureDB closes the pool, and + // a worker started before that would keep ticking against it. + app.startRateLimiterCleanup(dbRL) + return nil } diff --git a/internal/server/app_rl_cleanup_test.go b/internal/server/app_rl_cleanup_test.go index 77a0df711..ac398dc06 100644 --- a/internal/server/app_rl_cleanup_test.go +++ b/internal/server/app_rl_cleanup_test.go @@ -2,9 +2,14 @@ package server import ( "context" + "os" + "runtime" + "strings" "testing" "time" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -34,3 +39,89 @@ func TestStartRateLimiterCleanup_SurvivesRequestCtxAndStopsOnClose(t *testing.T) t.Fatal("worker ctx not canceled on Close") } } + +// dbRLWorkerCreator is the "created by" frame every goroutine launched by +// api.DBRateLimiter.StartCleanupWorker carries in a full stack dump. +const dbRLWorkerCreator = "created by github.com/LeanerCloud/cloud-commitments-platform/internal/api.(*DBRateLimiter).StartCleanupWorker" + +// liveDBRLWorkers counts the DBRateLimiter cleanup goroutines running now. +func liveDBRLWorkers() int { + buf := make([]byte, 1<<20) + for { + n := runtime.Stack(buf, true) + if n < len(buf) { + return strings.Count(string(buf[:n]), dbRLWorkerCreator) + } + buf = make([]byte, 2*len(buf)) + } +} + +// newReinitTestApp builds a pre-DB Application whose reinitializeAfterConnect +// runs without a database, AWS or secret store: the stores and services it +// wires only touch the pool when used, the encryption key comes from the raw +// env var, and the AWS config loads from empty files. +func newReinitTestApp(t *testing.T) *Application { + t.Helper() + for _, key := range []string{ + "CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN", "CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME", + "CREDENTIAL_ENCRYPTION_KEY_SECRET_ID", "CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY", + "CUDLY_SIGNING_KEY_ID", "CUDLY_SIGNING_KEY_VAULT_URL", "CUDLY_SIGNING_KEY_NAME", + "CUDLY_SIGNING_KEY_RESOURCE", "AWS_PROFILE", "AWS_LAMBDA_RUNTIME_API", + } { + t.Setenv(key, "") + } + t.Setenv("CREDENTIAL_ENCRYPTION_KEY", strings.Repeat("12", 32)) + t.Setenv("SCHEDULED_TASK_AUTH_MODE", "disabled") + t.Setenv("AWS_CONFIG_FILE", os.DevNull) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", os.DevNull) + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + t.Setenv("AWS_REGION", "us-east-1") + + app, err := NewApplicationFromDeps(testutil.TestContext(t), ApplicationConfig{ + DefaultTerm: 3, DefaultCoverage: 80, DashboardURL: "https://rl-cleanup.example.test", + }, ExternalDeps{ + EmailSender: &noopEmailSender{}, + DBConfig: &database.Config{Host: "localhost", Port: 5432, Database: "cudly_test", User: "test", Password: "test", SSLMode: "disable"}, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = app.Close() }) // idempotent; covers early test failures + return app +} + +// TestReinitializeAfterConnect_CleanupWorkerOutlivesRequestCtx pins the call +// site (#1137): the worker reinitializeAfterConnect starts must not stop when +// the request context that triggered the lazy DB connect ends (#269), and must +// stop on Close. Starting it with dbRL.StartCleanupWorker(ctx) fails this. +func TestReinitializeAfterConnect_CleanupWorkerOutlivesRequestCtx(t *testing.T) { + app := newReinitTestApp(t) + before := liveDBRLWorkers() + + reqCtx, cancelReq := context.WithCancel(testutil.TestContext(t)) + require.NoError(t, app.reinitializeAfterConnect(reqCtx, &database.Connection{})) + require.Equal(t, before+1, liveDBRLWorkers(), "reinitializeAfterConnect must start one cleanup worker") + + cancelReq() + assert.Never(t, func() bool { return liveDBRLWorkers() <= before }, + 300*time.Millisecond, 10*time.Millisecond, + "cleanup worker stopped with the request ctx; it must run on the app lifetime ctx") + + require.NoError(t, app.Close()) + assert.Eventually(t, func() bool { return liveDBRLWorkers() == before }, + 2*time.Second, 10*time.Millisecond, "cleanup worker must stop on Close") +} + +// TestReinitializeAfterConnect_NoCleanupWorkerWhenLaterStepFails: ensureDB +// closes the pool when reinitializeAfterConnect fails, so a worker started +// before the failing step would tick against a closed pool until a retry +// replaced it or Close canceled it (#1137). +func TestReinitializeAfterConnect_NoCleanupWorkerWhenLaterStepFails(t *testing.T) { + app := newReinitTestApp(t) + // A missing shared-config profile fails the AWS config load, a step that + // runs after the rate limiter is built. + t.Setenv("AWS_PROFILE", "cudly-rl-cleanup-missing-profile") + before := liveDBRLWorkers() + + err := app.reinitializeAfterConnect(testutil.TestContext(t), &database.Connection{}) + require.ErrorContains(t, err, "failed to load AWS config") + assert.Equal(t, before, liveDBRLWorkers(), "a failed reinitializeAfterConnect must not leave a cleanup worker running") +}