From e6f719e59156a5f097ee56307ca0cb995000f13f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:11:27 +0200 Subject: [PATCH 1/4] chore(reliability): recover() in fire-and-forget goroutines (closes #672) Add deferred recover() to the three unprotected background goroutines surfaced in the #669/#670 sibling audit: - internal/auth/service_apikeys.go: UpdateLastUsed async goroutine - internal/api/db_rate_limiter.go: cleanup async goroutine - internal/api/handler_accounts.go: GCP ts.Token() goroutine; panic now also sends an error back on tokenChan so the caller returns promptly rather than blocking until the 15s deadline fires Matches the existing pattern in ri_utilization_cache.go and scheduler.go. --- internal/api/db_rate_limiter.go | 5 +++++ internal/api/handler_accounts.go | 6 ++++++ internal/auth/service_apikeys.go | 5 +++++ 3 files changed, 16 insertions(+) diff --git a/internal/api/db_rate_limiter.go b/internal/api/db_rate_limiter.go index 58fb2034f..e0ead7a1f 100644 --- a/internal/api/db_rate_limiter.go +++ b/internal/api/db_rate_limiter.go @@ -169,6 +169,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/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 { From 0f26f5b7b0a76aef78266a9ac43522d12e9f1658 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 16:36:14 +0200 Subject: [PATCH 2/4] test(reliability): assert fire-and-forget goroutine panic is recovered (refs #672) Add TestValidateUserAPIKey_UpdateLastUsedPanicIsRecovered to internal/auth to prove the recover() in the UpdateLastUsed fire-and-forget goroutine works. The test injects a panicking MockStore.UpdateAPIKeyLastUsed callback and uses a done channel (closed by the mock Run callback before panicking) to wait for goroutine execution without time.Sleep. Without the recover() the injected panic crashes the test binary; with it the process survives and the test passes. --- internal/auth/service_apikeys_test.go | 53 +++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index 364ba838e..0434e5c14 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -986,6 +986,59 @@ 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} + + 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 + + mockStore.AssertExpectations(t) +} + func TestService_validateAPIKeyPermissions(t *testing.T) { ctx := context.Background() From ca118a9a37683bd046a376a6964dd16b63f7e456 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 23:40:07 +0200 Subject: [PATCH 3/4] fix(reliability): cover two missed fire-and-forget goroutines (refs #672) Add recover() to StartCleanupWorker (db_rate_limiter) and expireStaleExecutionsAsync (handler_history), both fire-and-forget goroutines that were absent from the original sweep. Also use t.Cleanup for AssertExpectations in the panic-recovery test, consistent with every other mock in the test file. --- internal/api/db_rate_limiter.go | 5 +++++ internal/api/handler_history.go | 12 +++++++++--- internal/auth/service_apikeys_test.go | 3 +-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/internal/api/db_rate_limiter.go b/internal/api/db_rate_limiter.go index e0ead7a1f..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 { diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index d6779670e..ba7908066 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,8 +244,7 @@ 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] + for _, exec := range staleExecs { _, err := h.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "expired", nil) if err != nil { logging.Warnf("history: expire sweep for execution %s failed: %v", exec.ExecutionID, err) diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index 0434e5c14..beb142dc3 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -1013,6 +1013,7 @@ func TestValidateUserAPIKey_UpdateLastUsedPanicIsRecovered(t *testing.T) { 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) @@ -1035,8 +1036,6 @@ func TestValidateUserAPIKey_UpdateLastUsedPanicIsRecovered(t *testing.T) { // Block until the goroutine has executed (and panicked + recovered). // Reaching this line means the process survived -- recover() caught the panic. <-done - - mockStore.AssertExpectations(t) } func TestService_validateAPIKeyPermissions(t *testing.T) { From a3a294d68ca32bd001d6f4c34b16e2db233f3dfa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 20:09:46 +0200 Subject: [PATCH 4/4] fix(lint): avoid rangeValCopy in expireStaleExecutionsSweep Use index-based loop to avoid copying the PurchaseExecution struct (304 bytes) on each iteration, as flagged by gocritic. --- internal/api/handler_history.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index ba7908066..f1cfcba97 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -244,10 +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 _, exec := range staleExecs { - _, 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) } } }