panic: close of closed channel — the `status` guard around `cleanupDone` is an unsynchronized test-and-set (reproducer included)
- Dominant language
- Go
- Stars
- 14.3k
- Forks
- 1.1k
- Avg merge
- 6d 9h
- Merged PRs (30d)
- 11
Description
## Summary
`pgConn.cleanupDone` is closed from four places, each guarded only by a plain
read/write of `pgConn.status`, an ordinary `byte` field with no mutex or atomic
(`pgconn/pgconn.go:92`):
| line (v5.9.2) | site |
|---|---|
| 633-635 | `receiveMessage`, FATAL `ErrorResponse` with `OnPgError` returning false |
| 689-694 | `Close` |
| 723-729 | `asyncClose` |
| 1443-1445 | `CopyFrom`, `bufferingReceiveErr` path |
Because the guard is a non-atomic test-and-set, two goroutines can both pass it
and both close the channel:
```go
func (pgConn *PgConn) Close(ctx context.Context) error {
if pgConn.status == connStatusClosed { // read
return nil
}
pgConn.status = connStatusClosed // write
defer close(pgConn.cleanupDone)
...
return pgConn.conn.Close() // panics here, on the deferred close
}
```
The window is wide: between the guard and the deferred close there is a full
`Terminate` + flush round trip.
`Close`, `asyncClose` and the `CopyFrom` path each already handle "somebody
else closed this" incorrectly, and #2470, #2364 and #1920 each repaired one
call path. The guard itself is still unsynchronized on master, so the next
combination of paths panics again — the reproducer below hits two different
ones on v5.9.2 and v5.10.0.
## Reproducer
Needs a reachable postgres, and nothing else — dropped into an empty module,
`go mod tidy` resolves it to v5.10.0. As published below it panics within 25 s
on darwin/arm64, in 4 runs out of 4. The `-fix` variant, which differs only in
the context the transaction is begun on, survives all 3 runs.
```go
// go run . -> panics
// go run . -fix -> does not
package main
import (
"context"
"database/sql"
"flag"
"fmt"
"math/rand/v2"
"os"
"sync"
"time"
"github.com/jackc/pgx/v5"
"github.com/jackc/pgx/v5/stdlib"
)
var (
fix = flag.Bool("fix", false, "begin the transaction on an uncancellable context")
workers = flag.Int("workers", 32, "concurrent workers")
seconds = flag.Int("seconds", 25, "run time")
dsn = flag.String("dsn", "host=localhost port=5432 dbname=postgres sslmode=disable", "")
)
func main() {
flag.Parse()
db, err := sql.Open("pgx", *dsn)
if err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(2)
}
defer db.Close()
db.SetMaxOpenConns(*workers)
db.SetMaxIdleConns(*workers)
if err := db.PingContext(context.Background()); err != nil {
fmt.Fprintln(os.Stderr, "no postgres:", err)
os.Exit(2)
}
deadline := time.Now().Add(time.Duration(*seconds) * time.Second)
var wg sync.WaitGroup
for range *workers {
wg.Add(1)
go func() {
defer wg.Done()
for time.Now().Before(deadline) {
round(db)
}
}()
}
wg.Wait()
fmt.Println("survived")
}
func round(db *sql.DB) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
conn, err := db.Conn(ctx)
if err != nil {
return
}
defer conn.Close()
var pgxConn *pgx.Conn
if err := conn.Raw(func(dc any) error {
pgxConn = dc.(*stdlib.Conn).Conn()
return nil
}); err != nil {
return
}
beginCtx := ctx
if *fix {
beginCtx = context.WithoutCancel(ctx)
}
tx, err := conn.BeginTx(beginCtx, nil)
if err != nil {
return
}
if _, err := tx.ExecContext(ctx,
`CREATE TEMP TABLE IF NOT EXISTS bulk (id int, name text) ON COMMIT DROP`); err != nil {
_ = tx.Rollback()
return
}
// cancel while the COPY is streaming
time.AfterFunc(time.Duration(rand.IntN(3000))*time.Microsecond, cancel)
rows := make([][]any, 20000)
for i := range rows {
rows[i] = []any{i, "some padding value to make the copy stream take a while"}
}
_, _ = pgxConn.CopyFrom(ctx, pgx.Identifier{"bulk"}, []string{"id", "name"},
pgx.CopyFromRows(rows))
_ = tx.Rollback()
}
```
To be explicit about what the reproducer does, since it matters for the
diagnosis: the transaction is a `database/sql` `Tx`, and the bulk load runs on
the `*pgx.Conn` obtained through `sql.Conn.Raw` — the documented escape hatch,
and the only way to get COPY performance under `database/sql`. That COPY is
therefore outside the per-connection lock `database/sql` puts around every
other statement, which is what lets two closers meet.
## Stacks
The reproducer produces two distinct panics from the same defect.
`CopyFrom`'s inline close (3 of 4 runs) — the site of #2364:
```
panic: close of closed channel
github.com/jackc/pgx/v5/pgconn.(*PgConn).CopyFrom(...)
pgx@v5.9.2/pgconn/pgconn.go:1445
github.com/jackc/pgx/v5.(*copyFrom).run(...)
pgx@v5.9.2/copy_from.go:202
github.com/jackc/pgx/v5.(*Conn).CopyFrom(...)
pgx@v5.9.2/copy_from.go:275
```
`Close` from the transaction rollback (1 of 4 runs) — this is the stack that
originally took a server down and prompted the investigation:
```
panic: close of closed channel
github.com/jackc/pgx/v5/pgconn.(*PgConn).Close(...)
pgx@v5.9.2/pgconn/pgconn.go:717
github.com/jackc/pgx/v5.(*Conn).die(...)
pgx@v5.9.2/conn.go:445
github.com/jackc/pgx/v5.(*dbTx).Rollback(...)
pgx@v5.9.2/tx.go:218
github.com/jackc/pgx/v5/stdlib.wrapTx.Rollback(...)
pgx@v5.9.2/stdlib/sql.go:909
database/sql.(*Tx).rollback.func1()
/usr/local/go/src/database/sql/sql.go:2345
database/sql.(*Tx).awaitDone(...)
/usr/local/go/src/database/sql/sql.go:2221
created by database/sql.(*DB).beginDC
```
`database/sql.beginDC` unconditionally spawns `go tx.awaitDone()` for every
`BeginTx(ctx, …)`, so on cancellation that goroutine rolls back; the `rollback`
`Exec` fails on the dead context, pgx takes the error branch at `tx.go:218` and
calls `conn.die()` → `PgConn.Close()`, concurrently with the COPY's own
teardown.
## Still present on master
v5.10.0 has the identical code — `Close` at 738-745, `asyncClose` at 772-780,
`IsClosed` at 811-813, `CopyFrom`'s inline close unchanged.
`diff v5.9.2 v5.10.0 pgconn/pgconn.go` shows no change touching `cleanupDone`.
The stacks above are from v5.9.2, but the reproducer was last run on v5.10.0,
where the same two sites panic at their shifted line numbers: `pgconn.go:767`
(the `return pgConn.conn.Close()` whose deferred `close(cleanupDone)` at 744
panics) in 2 of 4 runs, and `pgconn.go:1513` (`CopyFrom`) in the other 2.
## Suggested fix
Make the transition atomic and the close single-shot, so no combination of call
paths can double-close:
```go
type PgConn struct {
...
status atomic.Uint32
cleanupOnce sync.Once
}
func (pgConn *PgConn) markClosed() bool {
return pgConn.status.Swap(connStatusClosed) != connStatusClosed
}
func (pgConn *PgConn) finishCleanup() {
pgConn.cleanupOnce.Do(func() { close(pgConn.cleanupDone) })
}
```
`sync.Once` alone removes the panic; making `status` atomic additionally fixes
the data race and makes `IsClosed()` meaningful to callers.
Adjacent, same path: with `CancelRequestContextWatcherHandler`, `HandleCancel`
runs on the `ctxwatch` goroutine and its inner goroutine reads `pgConn.conn`,
`pgConn.pid` and `pgConn.secretKey` in `CancelRequest` while the owning
goroutine may be in `Close()` calling `pgConn.conn.Close()`. That is the same
shape as the connect race fixed in v5.10.0 ("Fix data race when context is
cancelled during connect"), but on the query path.
## Version
- pgx: v5.9.2 (verified unchanged on v5.10.0)
- Go: 1.26.0
- PostgreSQL: 18.3
- OS/arch: darwin/arm64
---
Reported by Claude Opus 5 on behalf of Martin Rode (Programmfabrik GmbH, fylr).
Contributor guide
Research direction
Read pgconn/pgconn.go at PgConn.status and cleanupDone, then inspect the Close, asyncClose, and CopyFrom paths named in the issue. Run the supplied database/sql reproducer with a reachable PostgreSQL instance. Done means the reproducer no longer panics when concurrent cleanup paths meet, with the reported race addressed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, postgresql, sql
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100