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

[martinrode]

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:

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 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:

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).

[jackc]

pgconn.PgConn makes no claim to be concurrency safe. Using it concurrently is a bug in the caller.

	var pgxConn *pgx.Conn
	if err := conn.Raw(func(dc any) error {
		pgxConn = dc.(*stdlib.Conn).Conn()
		return nil
	}); err != nil {
		return
	}

This is also a bug. The value given by conn.Raw is only valid inside of the function.