cockroachdb / cockroachdb/cockroach
util: singleflight wedges callers on panic, ctxgroup.Go has no recover, stopper handle never recovers
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Summary:**
Three general-purpose utilities behave badly when a panic passes through them. All three are used throughout SQL execution and job code. Part of #174944.
**Findings:**
- `singleflight.Group.Do`: `g.m[key]` and `c.c` — the flight is published into the map and `fn` runs inline on the caller's goroutine, but the cleanup `delete(g.m, key)` and `close(c.c)` are plain statements with another call between them. A panic in `fn` leaves a stale flight whose channel is never closed, so **every later caller for that key blocks forever**. Reachable via the node-global hydrated-descriptor cache, keyed by descriptor ID and version, so the effect is that all queries touching a table with UDT columns hang until the node restarts. No log, no metric.
- `ctxgroup.Group.Go`: a bare pass-through to `errgroup.Group.Go` with no recover, unlike its sibling `GoCtx`. A panic in a `.Go` goroutine is invisible to any recovery boundary no matter how that boundary is designed, and it is easy to reach for `.Go` out of habit. Seven non-test call sites in job code. Note also that `GoCtx` re-raises only at the next `Wait()`, so a fire-and-forget group swallows the panic entirely — upstream `errgroup` declines to recover for exactly this reason, citing the risk of "deadlocks that hide the panic entirely".
- `stop.activeHandle.Release`: calls `hdl.s.recover(ctx)` as a plain statement. Per the Go spec `recover()` only stops a panic when called *directly* by the deferred function, and `Release` is the deferred function, so this never recovers and `logcrash.ReportPanicWithGlobalSettings` never fires. The comment on `Stopper.recover` explicitly warns that it "must be called with `defer s.recover()`"; the four other call sites do. Panics from every `RunAsyncTask` / `RunAsyncTaskEx` goroutine — the most common way we spawn background work — therefore crash the node *without* a crash report. Task-count bookkeeping is unaffected, since Go still runs the rest of the deferred function body.
**Next Steps:**
- [ ] Defer both cleanup statements in `singleflight.Group.Do`
- [ ] Convert the seven job-code `ctxgroup.Group.Go` call sites to `.GoCtx`, or document why not
- [ ] Inline the `recover()` in `activeHandle.Release`, or move the four cleanup steps above the `recover` call
Epic: none
Jira issue: CRDB-68131
Contributor guide
Research direction
Start by reading singleflight.Group.Do, ctxgroup.Group.Go and GoCtx, and stop.activeHandle.Release alongside Stopper.recover. Trace the seven non-test Group.Go call sites and the RunAsyncTask/RunAsyncTaskEx paths before running the relevant package tests. Done means panic paths no longer leave callers blocked, silently disappear, or crash without a report.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 43/100