Skip to content

fix(execution): FanOutWithConcurrency goroutine has no recover() — purchase panics crash the whole Lambda (P1) #669

Description

@cristim

Summary

internal/execution/fanout.go:69-74 launches one goroutine per item and has no recover(). The helper is the workhorse for both the per-account fan-out AND the per-rec fan-out inside internal/purchase/execution.go:processPurchaseRecommendations. A panic inside fn (e.g. nil deref on a mocked-but-unexpected provider response, type assertion failure, slice OOB) is fatal:

  1. defer wg.Done() and defer { <-sem }() still fire (so the fan-out helper itself doesn't deadlock)
  2. The panic propagates up the goroutine stack with no upstream recover()
  3. Go runtime crashes the entire process — Lambda invocation terminates abnormally
  4. Purchase execution row is left at approved (was set before fan-out kicked off; nothing transitions it to failed or completed)
  5. CloudWatch may not flush the final log lines before the process dies
  6. User sees a generic Lambda invocation error, not the actual panic message

This is the silent-failure trap behind issue #632 ("approve strands execution in 'approved' on interrupted sync execution"). One panic anywhere in the purchase chain (cloud SDK, our codec, our parser, mock factory in tests) can strand a real execution.

Audit of all production goroutines

File:Line recover()? Risk
internal/scheduler/scheduler.go:1021 background refresh ✅ safe
internal/server/app.go:206 migration runner ✅ safe
internal/api/ri_utilization_cache.go:151 cache refresh ✅ safe
internal/execution/fanout.go:69 purchase fan-out helper ❌ HIGH
internal/auth/service_apikeys.go:279 async UpdateLastUsed ❌ LOW (small DB UPDATE)
internal/api/db_rate_limiter.go:137 async cleanup ❌ LOW
internal/api/handler_accounts.go:811 token exchange (in-flight only) ❌ LOW (timeout-bounded)

Fix

Add a recover() deferred guard inside the goroutine in FanOutWithConcurrency that catches the panic, converts it to an error with the panic value + the per-item account ID, and writes it to the per-index results[idx] so the parent aggregator sees a clean r.Err != nil outcome and can record the failure on the execution row:

go func(idx int, accountID string) {
    defer wg.Done()
    defer func() { <-sem }()
    defer func() {
        if r := recover(); r != nil {
            // Capture the panic + stack so post-mortem doesn't require
            // re-running the failure under the debugger. Surface as an
            // Err on the result slot so the parent aggregator records
            // the failure on the execution row.
            buf := make([]byte, 4096)
            n := runtime.Stack(buf, false)
            logging.Errorf("fan-out goroutine panic (account=%s): %v\n%s", accountID, r, buf[:n])
            results[idx] = Result[T]{
                AccountID: accountID,
                Err:       fmt.Errorf("panic during fan-out (account=%s): %v", accountID, r),
            }
        }
    }()
    val, err := fn(ctx, accountID)
    results[idx] = Result[T]{AccountID: accountID, Value: val, Err: err}
}(i, id)

Optionally also add recover() to the three LOW-risk goroutines in service_apikeys.go, db_rate_limiter.go, handler_accounts.go for completeness — those don't strand execution state but a panic in any of them still kills the Lambda.

Acceptance criteria

  • FanOutWithConcurrency catches panics in fn and surfaces them as Result.Err instead of crashing the process
  • Stack trace is logged at Error level so post-mortem doesn't require a repro
  • Regression test in internal/execution/fanout_test.go that panics from fn and asserts the result slot has Err containing "panic" + the panic value
  • Purchase execution e2e: a forced panic inside executeSinglePurchase should leave the execution at failed (not approved), with the panic visible in exec.Error

Severity

P1 / high. Doesn't match the QA-reported #667 symptom directly (that one IS a context-deadline exceeded with a structured SDK error), but is the same class of bug — a sync purchase failure that leaves execution stranded. Fixing this hardens against the next purchase failure that's a panic rather than a structured error.

Cross-references

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions