Skip to content

fix(postgres): retry transient Accept errors in the data-plane loop instead of exiting - #57

Merged
andybons merged 1 commit into
mainfrom
fix-postgres-accept-retry
Jul 15, 2026
Merged

fix(postgres): retry transient Accept errors in the data-plane loop instead of exiting#57
andybons merged 1 commit into
mainfrom
fix-postgres-accept-retry

Conversation

@andybons

Copy link
Copy Markdown
Contributor

What

Sibling of #56. PostgresServer.acceptLoop exited permanently on any Accept() error — a transient one (EMFILE/ENFILE under fd exhaustion, ECONNABORTED; realistic for a DB proxy holding many long-lived relay connections) would kill the Postgres listener until process restart.

Now it mirrors Demux.acceptLoop (and net/http.Server.Serve) exactly: clean exit only on our own shutdown (s.closed); otherwise capped exponential backoff (5ms→1s), WARN, and retry, with the backoff reset after a successful accept. It reuses the same demuxAcceptRetryBaseDelay/demuxAcceptRetryMaxDelay constants (same package) so the two loops stay identical. Like the demux fix, it doesn't gate on the deprecated net.Error.Temporary().

This was the out-of-scope note on #56 — its blast radius was unchanged by that PR, so it's fixed here separately.

Tests

Test-first, mirroring the demux tests (and reusing the scriptedAcceptListener helper): a listener that fails 3× with a simulated EMFILE then hands out a real conn — proves the loop retries and the conn is handled (SSLRequest → S); plus a clean-exit-on-Stop() test. Both mutation-checked (revert retry → red; remove shutdown check → spin-red). go test -race ./..., vet, gofmt clean.

CHANGELOG v0.19.1.

…nstead 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.
@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @andybons's task in 1m 36s —— View job


Code Review

Summary: This is a clean, correct fix. PostgresServer.acceptLoop (proxy/postgres.go:579-620) now retries transient Accept errors with capped exponential backoff (5ms → 1s) instead of returning permanently, and exits cleanly only when s.closed is set by beginClose — an exact, line-for-line mirror of the already-merged Demux.acceptLoop (proxy/demux.go:310-337), reusing the same demuxAcceptRetryBaseDelay/demuxAcceptRetryMaxDelay constants so the two loops can't drift.

Verified:

  • Shutdown race is preserved. The s.closed.Load() check on the error path (postgres.go:584) correctly relies on beginClose setting closed before closing the listener (postgres.go:504-511), so a real shutdown is always observed before the resulting Accept error is inspected — no window for the retry path to misclassify a shutdown as transient.
  • Backoff reset is correct. backoff = 0 on every successful Accept (postgres.go:603), so a burst of transient errors followed by recovery doesn't leave a stale backoff inflating the delay on the next unrelated failure.
  • No net.Error.Temporary() gating — intentionally, matching the demux fix's rationale (deprecated/unreliable), with the same accepted pathological case (a truly dead listener retries once/sec forever, visibly logged at WARN) rather than a tight spin.
  • Logging is safe — only error and computed retry_in are logged (postgres.go:596-599), no connection content or credentials, consistent with this repo's logging constraints.
  • Tests are solid. TestPostgresServer_AcceptLoopRetriesTransientErrors and TestPostgresServer_AcceptLoopExitsCleanlyOnStop correctly reuse scriptedAcceptListener from demux_test.go (same package) rather than duplicating it, and each test's failure message names the specific bug it guards against (e.g., "want exactly 1: >1 means the loop spin-retried the closed-listener error"), per this repo's testing conventions in AGENTS.md.
  • Independence from the demux path confirmedDemux.acceptLoop (demux.go:310) is the sole acceptor when HTTP/Postgres share a port, so this fix specifically covers PostgresServer running its own dedicated listener (the non-multiplexed case), which is exactly the gap PR feat(proxy): multiplex HTTP and Postgres on one port when their ports coincide #56 left open.

No bugs, race conditions, or security issues found. I was unable to execute go build/go vet/go test in this sandbox (Bash tool calls were not approved), so I could not independently confirm the test run — the diff and reasoning check out from static review, but you may want to confirm CI is green before merging.

@andybons
andybons merged commit d6da2dc into main Jul 15, 2026
2 checks passed
@andybons
andybons deleted the fix-postgres-accept-retry branch July 15, 2026 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant