jackc / jackc/pgx

panic: close of closed channel — the `status` guard around `cleanupDone` is an unsynchronized test-and-set (reproducer included)

Open
#2,622 1 comment 0 reactions 0 assignees View on GitHub
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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.