chore(reliability): recover() in fire-and-forget goroutines (closes #672)#859
chore(reliability): recover() in fire-and-forget goroutines (closes #672)#859cristim wants to merge 3 commits into
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughDeferred panic-recovery blocks are added to five fire-and-forget goroutines: the rate-limiter cleanup ticker and async cleanup, GCP token exchange, stale execution expiry, and API key last-used update. Each recovers panics and logs via logging.Warnf. A regression test validates recovery behavior for the API key goroutine. ChangesGoroutine Panic Recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Rebased onto The conflict was between two intents on the fire-and-forget
Resolution preserves both: the Verification:
No open CodeRabbit review threads were outstanding. @coderabbitai full review |
|
🧠 Learnings used✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…routine (#1111) A panic in one region's worker (nil deref, SDK type assertion failure, slice OOB) crashed the whole CLI process. Add a deferred recover() that logs the panic with region context and lets the remaining goroutines complete, mirroring the pattern applied to Lambda goroutines in PR #859 (issue #672). Closes #997 Co-authored-by: Claude <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/db_rate_limiter.go (1)
58-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRecover() at goroutine top silently kills the ticker loop forever.
The
defer recover()wraps the wholefor { select {...} }loop. Ifrl.cleanup()(line 72) panics, recovery logs it but the goroutine then returns, permanently stopping this periodic cleanup for the rest of the process lifetime — the ticker never fires again. Compare with themaybeCleanupgoroutine below (lines 175-183), wheredefer rl.cleanupRunning.Store(false)correctly re-arms that path for future attempts after a panic.Scope the recover to just the per-tick work instead, so a single panic doesn't disable the entire background worker.
🔧 Proposed fix
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 { select { case <-ctx.Done(): return case <-ticker.C: - rl.cleanup() + func() { + defer func() { + if r := recover(); r != nil { + logging.Warnf("db_rate_limiter: StartCleanupWorker goroutine panic: %v", r) + } + }() + rl.cleanup() + }() } } }() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/db_rate_limiter.go` around lines 58 - 76, The panic recovery in StartCleanupWorker currently wraps the entire ticker goroutine, so a panic inside rl.cleanup() stops the cleanup loop permanently. Move the recover logic to only guard the per-tick work inside the for/select branch in StartCleanupWorker, keeping the goroutine alive after a single cleanup panic; use the existing cleanup path in rl.cleanup() and mirror the resilience pattern used by maybeCleanup so future ticks still run.
🧹 Nitpick comments (1)
internal/api/db_rate_limiter.go (1)
60-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging a stack trace with the recovered panic.
logging.Warnf(...%v", r)only captures the panic value, not where it happened. Since these are rare, hard-to-reproduce background panics, appendingdebug.Stack()would materially help post-mortem debugging. Same applies to the other recover blocks added in this PR (handler_accounts.go, handler_history.go, service_apikeys.go).♻️ Example
+import "runtime/debug" ... defer func() { if r := recover(); r != nil { - logging.Warnf("db_rate_limiter: StartCleanupWorker goroutine panic: %v", r) + logging.Warnf("db_rate_limiter: StartCleanupWorker goroutine panic: %v\n%s", r, debug.Stack()) } }()Also applies to: 177-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/db_rate_limiter.go` around lines 60 - 64, The recover blocks in db_rate_limiter.go and the other new goroutine recover paths in handler_accounts.go, handler_history.go, and service_apikeys.go only log the panic value, which loses the call site context. Update each panic recovery helper/site (including StartCleanupWorker and the other affected functions) to append a stack trace from debug.Stack() to the warning log, keeping the existing panic value while adding the full stack for post-mortem debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/api/db_rate_limiter.go`:
- Around line 58-76: The panic recovery in StartCleanupWorker currently wraps
the entire ticker goroutine, so a panic inside rl.cleanup() stops the cleanup
loop permanently. Move the recover logic to only guard the per-tick work inside
the for/select branch in StartCleanupWorker, keeping the goroutine alive after a
single cleanup panic; use the existing cleanup path in rl.cleanup() and mirror
the resilience pattern used by maybeCleanup so future ticks still run.
---
Nitpick comments:
In `@internal/api/db_rate_limiter.go`:
- Around line 60-64: The recover blocks in db_rate_limiter.go and the other new
goroutine recover paths in handler_accounts.go, handler_history.go, and
service_apikeys.go only log the panic value, which loses the call site context.
Update each panic recovery helper/site (including StartCleanupWorker and the
other affected functions) to append a stack trace from debug.Stack() to the
warning log, keeping the existing panic value while adding the full stack for
post-mortem debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6746adc-5503-4811-8963-506140106aa9
📒 Files selected for processing (5)
internal/api/db_rate_limiter.gointernal/api/handler_accounts.gointernal/api/handler_history.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
) 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.
…d (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.
) 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.
1 participant
Summary
defer recover()to the 3 unprotected fire-and-forget goroutines identified in chore(reliability): add recover() to non-fan-out fire-and-forget goroutines (auth, rate-limiter, GCP token-exchange) #672:UpdateLastUsed(auth), rate-limitercleanup, and GCPts.Token()(accounts handler)tokenChanso the caller returns immediately rather than hanging until the 15s context deadlineri_utilization_cache.goandscheduler.go; all log vialogging.WarnfTest plan
go build ./...passesgo test ./internal/auth/... ./internal/api/...-- 1852 tests passscheduler.goandapp.gogoroutines confirmed already protected (excluded from change)cmd/multi_service.gosignal-handler goroutine confirmed trivially safe (no code path that can panic)Closes #672
Summary by CodeRabbit