diff --git a/internal/api/db_rate_limiter.go b/internal/api/db_rate_limiter.go index 58fb2034f..029af0411 100644 --- a/internal/api/db_rate_limiter.go +++ b/internal/api/db_rate_limiter.go @@ -57,6 +57,11 @@ func NewDBRateLimiter(pool *pgxpool.Pool) *DBRateLimiter { // tests with a short-lived context. func (rl *DBRateLimiter) StartCleanupWorker(ctx context.Context) { go func() { + defer func() { + if r := recover(); r != nil { + logging.Warnf("db_rate_limiter: StartCleanupWorker goroutine panic: %v", r) + } + }() ticker := time.NewTicker(dbRateLimiterScheduledCleanupInterval) defer ticker.Stop() for { @@ -169,6 +174,11 @@ func (rl *DBRateLimiter) maybeCleanup() { // Run cleanup in background go func() { defer rl.cleanupRunning.Store(false) + defer func() { + if r := recover(); r != nil { + logging.Warnf("db_rate_limiter: cleanup goroutine panic: %v", r) + } + }() rl.cleanup() }() } diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 542caec39..0ea530c7f 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -881,6 +881,12 @@ func gcpTokenExchangeAttempt(ctx context.Context, ts oauth2.TokenSource) (Accoun defer cancel() tokenChan := make(chan tokenResult, 1) go func() { + defer func() { + if r := recover(); r != nil { + logging.Warnf("handler_accounts: gcp token-exchange goroutine panic: %v", r) + tokenChan <- tokenResult{err: fmt.Errorf("token exchange panic: %v", r)} + } + }() tok, err := ts.Token() tokenChan <- tokenResult{tok: tok, err: err} }() diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index d6779670e..f1cfcba97 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -227,7 +227,14 @@ func (h *Handler) expireStaleExecutions(staleExecs []config.PurchaseExecution) { h.expireStaleExecutionsSweep(staleExecs) return } - go h.expireStaleExecutionsSweep(staleExecs) + go func() { + defer func() { + if r := recover(); r != nil { + logging.Warnf("history: expireStaleExecutionsAsync goroutine panic: %v", r) + } + }() + h.expireStaleExecutionsSweep(staleExecs) + }() } // expireStaleExecutionsSweep is the shared sweep body for both branches of @@ -237,11 +244,10 @@ func (h *Handler) expireStaleExecutions(staleExecs []config.PurchaseExecution) { // should not abort the best-effort transitions. func (h *Handler) expireStaleExecutionsSweep(staleExecs []config.PurchaseExecution) { ctx := context.Background() - for _rvc := range staleExecs { - exec := staleExecs[_rvc] - _, err := h.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "expired", nil) + for i := range staleExecs { + _, err := h.config.TransitionExecutionStatus(ctx, staleExecs[i].ExecutionID, []string{"pending", "notified"}, "expired", nil) if err != nil { - logging.Warnf("history: expire sweep for execution %s failed: %v", exec.ExecutionID, err) + logging.Warnf("history: expire sweep for execution %s failed: %v", staleExecs[i].ExecutionID, err) } } } diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index b8e2a5b94..79a80a3a3 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -291,6 +291,11 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA keyID := key.ID go func() { if _, sfErr, _ := s.lastUsedSFG.Do(keyID, func() (any, error) { + defer func() { + if r := recover(); r != nil { + logging.Warnf("service_apikeys: UpdateLastUsed goroutine panic: %v", r) + } + }() updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() if err := s.UpdateLastUsed(updateCtx, keyID); err != nil { diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index 364ba838e..beb142dc3 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -986,6 +986,58 @@ func TestService_HasAPIKeyPermissionForConstraintsAPI_OwnerCapEnforced(t *testin }) } +// TestValidateUserAPIKey_UpdateLastUsedPanicIsRecovered proves that a panic +// inside the fire-and-forget UpdateLastUsed goroutine is caught by the +// recover() added in issue #672. Without the recover(), the panic would crash +// the entire test binary (an unrecovered goroutine panic terminates the +// process). The test uses a channel -- closed by the mock Run callback just +// before panicking -- to wait for goroutine execution without time.Sleep. +func TestValidateUserAPIKey_UpdateLastUsedPanicIsRecovered(t *testing.T) { + ctx := context.Background() + + apiKey := "test-api-key-panic-123" + hash := sha256.Sum256([]byte(apiKey)) + keyHash := base64.RawURLEncoding.EncodeToString(hash[:]) + + user := &User{ + ID: "user-panic", + Email: "panic@example.com", + Active: true, + } + apiKeyRecord := &UserAPIKey{ + ID: "key-panic", + UserID: "user-panic", + KeyHash: keyHash, + IsActive: true, + } + + mockStore := new(MockStore) + service := &Service{store: mockStore} + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(apiKeyRecord, nil) + mockStore.On("GetUserByID", mock.Anything, "user-panic").Return(user, nil) + + // done is closed by the Run callback immediately before panicking so the + // test can wait for goroutine execution without time.Sleep. + done := make(chan struct{}) + mockStore.On("UpdateAPIKeyLastUsed", mock.Anything, "key-panic"). + Run(func(args mock.Arguments) { + close(done) + panic("injected panic for #672 regression test") + }). + Return(nil) + + resultKey, resultUser, err := service.ValidateUserAPIKey(ctx, apiKey) + require.NoError(t, err) + require.Equal(t, apiKeyRecord, resultKey) + require.Equal(t, user, resultUser) + + // Block until the goroutine has executed (and panicked + recovered). + // Reaching this line means the process survived -- recover() caught the panic. + <-done +} + func TestService_validateAPIKeyPermissions(t *testing.T) { ctx := context.Background()