From 982478794ff486299175008f8f5f92ff2ce31478 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Wed, 15 Jul 2026 18:59:08 -0400 Subject: [PATCH] fix(postgres): retry transient Accept errors in the data-plane loop instead of exiting PostgresServer.acceptLoop had the identical unconditional-exit-on-any- Accept-error bug that PR #56 fixed in Demux.acceptLoop, and was intentionally left out of that PR's scope. This is the sibling fix: acceptLoop now mirrors Demux.acceptLoop (and, in turn, net/http.Server.Serve) by retrying any non-shutdown Accept error with a capped exponential backoff (reusing demux.go's demuxAcceptRetryBaseDelay/demuxAcceptRetryMaxDelay constants so the two loops can't drift), logging at WARN, and only exiting cleanly on an intentional Stop/Shutdown. --- CHANGELOG.md | 6 +++ proxy/postgres.go | 31 +++++++++++++-- proxy/postgres_test.go | 89 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4628ccd..023ae9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ Gatekeeper is a standalone credential-injecting TLS-intercepting proxy. It trans Gatekeeper is pre-1.0. The configuration schema and credential source interface may change between minor versions. +## v0.19.1 — 2026-07-15 + +### Fixed + +- **The Postgres data-plane accept loop no longer dies permanently on a transient `Accept` error** — `PostgresServer.acceptLoop` (`proxy/postgres.go`) treated any non-shutdown `Accept` error as fatal, logging `postgres accept loop exited` and returning for good; a transient failure (EMFILE/ENFILE under fd exhaustion, ECONNABORTED — realistic for a proxy holding many long-lived Postgres relay connections) permanently killed the data-plane listener until process restart, with no automatic recovery. This is the identical unconditional-exit-on-any-Accept-error bug `Demux.acceptLoop` had until [#56](https://github.com/majorcontext/gatekeeper/pull/56) fixed it for the shared-listener demux path, and was intentionally left out of that PR's scope as a separate follow-up. `PostgresServer.acceptLoop` now mirrors `Demux.acceptLoop` exactly, which in turn mirrors `net/http.Server.Serve`: on an `Accept` error, it exits cleanly and silently only when the server is already shutting down (`beginClose` sets the `closed` flag before closing the listener, exactly as before); any other error is treated as transient, logged once at WARN with the error and the computed backoff (never connection content or credentials), and retried after a capped exponential delay (5ms doubling to a 1s cap), which resets to zero after the next successful `Accept`. The two loops now share the same backoff constants (`demuxAcceptRetryBaseDelay`, `demuxAcceptRetryMaxDelay`, defined once in `demux.go`) so they can't drift apart, and — like the demux loop — deliberately do not gate the retry on the deprecated, unreliable `net.Error.Temporary()`; a genuinely dead-but-unclosed listener retries once per second forever with a WARN each time, the same visible, capped pathological case `net/http` and the demux loop already tolerate rather than a zero-delay spin. All other behavior is unchanged: the clean-shutdown path, per-connection goroutine dispatch via `trackConn`/`untrackConn`, and existing logging + ## v0.19.0 — 2026-07-15 ### Added diff --git a/proxy/postgres.go b/proxy/postgres.go index 5b656a0..11c40fc 100644 --- a/proxy/postgres.go +++ b/proxy/postgres.go @@ -563,7 +563,21 @@ func (s *PostgresServer) closeActiveConns() { s.mu.Unlock() } +// acceptLoop accepts connections on ln and dispatches each to handleConn. It +// mirrors Demux.acceptLoop's accept-error handling (demux.go), which in turn +// mirrors net/http.Server.Serve: a transient Accept error is retried after a +// capped exponential backoff rather than tearing down the listener, and only +// an intentional shutdown (s.closed set by beginClose before the listener is +// closed) exits the loop. Without this, a transient error (EMFILE/ENFILE +// under fd exhaustion, ECONNABORTED -- realistic for a proxy holding many +// long-lived Postgres relay connections) permanently kills the data-plane +// listener until process restart. +// +// Like Demux.acceptLoop, this does not gate the retry on the deprecated and +// unreliable net.Error.Temporary(): any error while the listener is still +// live is treated as transient and retried. func (s *PostgresServer) acceptLoop(ln net.Listener) { + var backoff time.Duration for { conn, err := ln.Accept() if err != nil { @@ -571,11 +585,22 @@ func (s *PostgresServer) acceptLoop(ln net.Listener) { // Intentional shutdown: the listener was closed. return } - slog.Error("postgres accept loop exited", + if backoff == 0 { + backoff = demuxAcceptRetryBaseDelay + } else { + backoff *= 2 + } + if backoff > demuxAcceptRetryMaxDelay { + backoff = demuxAcceptRetryMaxDelay + } + slog.Warn("postgres: transient accept error; retrying", "subsystem", "proxy", - "error", err) - return + "error", err, + "retry_in", backoff) + time.Sleep(backoff) + continue } + backoff = 0 // Register the connection under the same lock that Shutdown uses to set // closed. This makes the closed check and wg.Add atomic with respect to // shutdown: either we add to the WaitGroup before Shutdown observes the diff --git a/proxy/postgres_test.go b/proxy/postgres_test.go index ef92962..9364e51 100644 --- a/proxy/postgres_test.go +++ b/proxy/postgres_test.go @@ -1224,3 +1224,92 @@ func TestCheckNetworkPolicyHTTPPortDefaultsUnchanged(t *testing.T) { t.Error(`checkNetworkPolicy("api.github.com", 5432) = true, want false -- a portless HTTP allow pattern must not match the Postgres port`) } } + +// --- accept-loop resilience to transient Accept errors --------------------- +// +// These are the sibling of TestDemux_AcceptLoopRetriesTransientErrors and +// TestDemux_AcceptLoopExitsCleanlyOnClose in demux_test.go: PostgresServer's +// own acceptLoop had the identical unconditional-exit-on-any-Accept-error bug +// that PR #56 fixed in Demux.acceptLoop, and was intentionally left out of +// that PR's scope. scriptedAcceptListener (defined in demux_test.go, same +// package) is reused here rather than duplicated. + +// TestPostgresServer_AcceptLoopRetriesTransientErrors is the regression guard +// for PostgresServer.acceptLoop: a transient Accept error (EMFILE/ENFILE +// under fd exhaustion, ECONNABORTED -- realistic for a proxy holding many +// long-lived Postgres relay connections) must not permanently kill the +// data-plane listener. The loop must back off and retry -- mirroring +// net/http.Server.Serve and Demux.acceptLoop -- so a good connection +// arriving after a burst of transient errors is still dispatched. +func TestPostgresServer_AcceptLoopRetriesTransientErrors(t *testing.T) { + p, _ := newTestProxyWithCA(t) + srv := NewPostgresServer(p) + + // Three transient failures, then a real conn. + transient := errors.New("simulated EMFILE: too many open files") + l := newScriptedAcceptListener(3, transient) + + serverConn, clientConn := net.Pipe() + defer clientConn.Close() + l.conns <- serverConn + + if err := srv.StartListener(l); err != nil { + t.Fatalf("StartListener: %v", err) + } + t.Cleanup(srv.Stop) + + // If the loop had exited after the first transient error (the bug), the + // good conn is never Accepted from the scripted listener and handleConn + // never runs on it, so nothing ever answers the SSLRequest below and this + // read times out. The 2s bound is far above the ~35ms the three + // 5ms/10ms/20ms backoffs take. + fe := pgproto3.NewFrontend(clientConn, clientConn) + go func() { + fe.Send(&pgproto3.SSLRequest{}) + _ = fe.Flush() + }() + + _ = clientConn.SetDeadline(time.Now().Add(2 * time.Second)) + var resp [1]byte + if _, err := clientConn.Read(resp[:]); err != nil { + t.Fatalf("read SSLRequest response (proves the post-burst conn reached handleConn): %v", err) + } + if resp[0] != 'S' { + t.Errorf("SSLRequest response = %q, want 'S'", resp[0]) + } +} + +// TestPostgresServer_AcceptLoopExitsCleanlyOnStop guards the other half of +// the fix: the retry path must not swallow shutdown. When Stop calls +// beginClose, which sets closed before closing the listener, the resulting +// Accept error is gatekeeper's own doing -- the loop must return immediately +// without logging and without spin-retrying the closed-listener error. +func TestPostgresServer_AcceptLoopExitsCleanlyOnStop(t *testing.T) { + logBuf := captureSlogText(t) + p, _ := newTestProxyWithCA(t) + srv := NewPostgresServer(p) + + l := newScriptedAcceptListener(0, nil) + if err := srv.StartListener(l); err != nil { + t.Fatalf("StartListener: %v", err) + } + + // Let the accept loop reach its blocking Accept before shutting down. + time.Sleep(20 * time.Millisecond) + + srv.Stop() + + // Give the loop time to observe the close and return. A correct loop + // calls Accept exactly once more (getting net.ErrClosed), sees closed, + // and returns; a loop that treated the closed-listener error as + // transient would keep calling Accept on a ~5ms backoff, so the + // post-close count would climb past 1 within this window. + time.Sleep(80 * time.Millisecond) + + if got := l.acceptsAfterCloseCount(); got != 1 { + t.Errorf("Accept called %d times after close, want exactly 1: >1 means the loop spin-retried the closed-listener error instead of exiting on shutdown", got) + } + if s := logBuf.String(); strings.Contains(s, "transient accept error") || strings.Contains(s, "accept loop exited") { + t.Errorf("clean shutdown logged an accept error/retry line, want none: %q", s) + } +}