fix(execution): recover from panics inside FanOutWithConcurrency goroutines (closes #669) - #670
Conversation
…utines (closes #669) Defence-in-depth for the per-account / per-rec fan-out helper. Before this change, any panic inside fn (nil deref, type assertion failure, slice OOB in a future refactor) propagated up the unrecovered goroutine and crashed the entire Lambda process. Consequences: - Purchase execution row stays at 'approved' with no transition to 'failed' (state machine never runs the aggregator that would record the failure). - Lambda invocation terminates abnormally; CloudWatch may not flush the final log lines. - User sees a generic Lambda invocation error rather than the actual panic detail. The fan-out helper now installs a deferred recover() in every goroutine that converts a panic into a structured Err on the per-item Result slot, logs the goroutine stack at Error level for post-mortem, and lets the parent aggregator process the failure exactly like a normal fn-returned error. Current call sites in internal/purchase/execution.go (per-account executeForAccount + per-rec processPurchaseRecommendations) have no obvious panic source today; the recover() is cheap insurance for the high-impact failure mode and covers future refactors that might introduce one. Regression test (TestFanOut_PanicInFn) panics inside fn for one of three items, asserts the panicking item carries a non-nil Err containing the panic value AND that the other two items still succeed (i.e. one goroutine's panic doesn't cascade across the fan-out). Closes #669
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds panic recovery to ChangesPanic Recovery in Fan-Out Execution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
) 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.
) 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.
) 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.
) 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.
) 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.
) 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.
) 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.
) 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.
) (#859) * 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. * 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. * 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. * 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.
Summary
Defence-in-depth for
internal/execution/FanOutWithConcurrency. Before this change, any panic insidefn(nil deref, type assertion failure, slice OOB in a future refactor) propagated up the unrecovered goroutine and crashed the entire Lambda process. Consequences:approvedwith no transition tofailed(the aggregator that would record the failure never runs).The fan-out helper now installs a deferred
recover()in every goroutine that:Erron the per-itemResultslot.fn-returned error.Why now?
Current call sites in
internal/purchase/execution.go(per-accountexecuteForAccount+ per-recprocessPurchaseRecommendations) have no obvious panic source today, so this is cheap insurance, not a bug fix. But the failure mode is catastrophic (whole-Lambda crash + stranded execution atapproved), and the cost is ~10 lines with zero perf impact. Found during the concurrency-audit pass on #667 + #632; filed as #669.What changed
internal/execution/fanout.go: added a deferredrecover()inside the goroutine launched byFanOutWithConcurrency. Captures the panic value + stack viaruntime.Stack, logs at Error, and writes a structured error toresults[idx].internal/execution/fanout_test.go: new testTestFanOut_PanicInFnpanics fromfnfor one of three items, asserts the panicking item carries anErrcontaining the panic value AND that the other two items still succeed — i.e. one goroutine's panic doesn't cascade across the fan-out.Test plan
go test ./internal/execution/... ./internal/purchase/... -count=1 -short— 174/174 passgo build ./...— cleanfailed(notapproved) with the panic visible inexec.ErrorCross-references
fix/purchases-add-execution-logging(PR fix(purchases): add execution-tagged logging to executeSinglePurchase (refs #667) #668) after that PR had already merged with only the logging commit — this PR is the orphaned recover() commit landed cleanly on its own branch.Summary by CodeRabbit