From 60c6dbf4252f5738ba535c1114d8e40e22e1ad35 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 4 Sep 2026 20:16:04 +0000 Subject: [PATCH] fix(server): raise the default control-plane idle timeout to 15m An idle client connection pins a worker, so the control plane closes it after DefaultControlPlaneIdleTimeout. That was 60s, which broke any client pausing between statements; it became 5m. 5m is still short of the gaps clients actually leave. Measured against a production deployment: the connections 5m 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 then paying a cold worker respawn to get back -- churn that reclaims nothing. Only ~4% of reaps had no client return at all, so the window is not holding many genuinely abandoned connections either. Raise it to 15m. That clears the observed cluster with margin for a slower run, and costs on the order of one continuously-held worker across the fleet against roughly 150 avoided cold restarts a day. The value deliberately sits at the ceiling the accompanying test enforces: beyond 15m an abandoned connection holds a pinned worker too long to justify as a default. Operators who want tighter density still set --idle-timeout / DUCKGRES_IDLE_TIMEOUT, and a client needing longer asks per connection via duckgres.idle_timeout, bounded by DUCKGRES_CLIENT_IDLE_TIMEOUT_MAX. This reduces how often the reap fires; it is not a correctness fix. A client whose gap can exceed any finite timeout still has to treat the reap as retryable rather than fatal. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VyKpkM46efTrRcCJNp8Bmd --- server/exports.go | 26 ++++++++++++++++++++------ server/server_test.go | 9 +++++++-- 2 files changed, 27 insertions(+), 8 deletions(-) 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 {