cockroachdb / cockroachdb/cockroach

util: singleflight wedges callers on panic, ctxgroup.Go has no recover, stopper handle never recovers

Open
#174,960 1 comment 0 reactions 0 assignees View on GitHub
C-bug O-agent T-kv
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.