diff --git a/server/exports.go b/server/exports.go index 90a7dad1..f9440ff7 100644 --- a/server/exports.go +++ b/server/exports.go @@ -94,12 +94,26 @@ func NewClientConn(s *Server, conn net.Conn, reader *bufio.Reader, writer *bufio // connection rather than an idle one — and a client cannot safely replay a // write that may already have committed, so the failure reaches the user. // -// 5m keeps the reclaim bounded while covering the gaps a batch or BI client -// leaves between statements. Deployments that want tighter worker density set -// --idle-timeout / DUCKGRES_IDLE_TIMEOUT; a client that needs longer asks for -// it per connection with duckgres.idle_timeout, bounded by -// DUCKGRES_CLIENT_IDLE_TIMEOUT_MAX. -const DefaultControlPlaneIdleTimeout = 5 * time.Minute +// 5m fixed the worst of that but still sat inside the gaps clients actually +// leave. Measured over a production deployment, the connections it reaped were +// not abandoned: the client came back a median of 8s after the reap and at +// worst 58s, so every observed idle gap fell between 5m00s and 5m58s. Those +// clients were missing the threshold by seconds and paying a cold worker +// respawn to get back, which is churn with no reclaim benefit — and only ~4% +// of reaps had no client return at all, so a longer window is not exposing +// many genuinely abandoned connections. +// +// 15m clears that cluster with margin for a slower run, and costs on the order +// of one continuously-held worker across the fleet. It is deliberately at the +// ceiling the accompanying test enforces: past this, an abandoned connection +// holds a pinned worker too long to justify by default. Deployments that want +// tighter worker density set --idle-timeout / DUCKGRES_IDLE_TIMEOUT; a client +// that needs longer asks for it per connection with duckgres.idle_timeout, +// bounded by DUCKGRES_CLIENT_IDLE_TIMEOUT_MAX. +// +// This is a frequency reduction, not a correctness fix: a client whose gap can +// exceed any finite timeout still needs to treat the reap as retryable. +const DefaultControlPlaneIdleTimeout = 15 * time.Minute // NormalizeIdleTimeout resolves a configured connection idle timeout: zero means // "unset" → use zeroDefault; a negative value means "explicitly disabled" → 0 diff --git a/server/server_test.go b/server/server_test.go index a65b1299..4e61294e 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -44,9 +44,14 @@ func TestCancelQueryDoesNotLogSecretKey(t *testing.T) { // The floor is what matters here. Anything at or below a minute reintroduces // that failure; the exact value above it is a density trade-off operators can // make with --idle-timeout. +// +// 5m was still short of the gaps clients leave in practice: measured in +// production, reaped connections came back a median of 8s (worst 58s) after +// the reap, so the reaps were hitting clients that missed the threshold by +// seconds and then paid a cold respawn. 15m clears that with margin. func TestDefaultControlPlaneIdleTimeout(t *testing.T) { - if DefaultControlPlaneIdleTimeout != 5*time.Minute { - t.Errorf("DefaultControlPlaneIdleTimeout = %v, want 5m", DefaultControlPlaneIdleTimeout) + if DefaultControlPlaneIdleTimeout != 15*time.Minute { + t.Errorf("DefaultControlPlaneIdleTimeout = %v, want 15m", DefaultControlPlaneIdleTimeout) } // A client that pauses between statements must survive the pause. if DefaultControlPlaneIdleTimeout <= time.Minute {