Skip to content

Commit 4a17903

Browse files
committed
feat(control): force-close idle pgwire connections via listener watchdog
An idle-in-transaction pgwire connection could hold its Data-Plane staging overlay indefinitely, since process_socket owns the connection loop and never times out on its own. The listener now races each connection against a bounded re-check tick and drops the socket future once it exceeds the configured idle or absolute session timeout, then runs the existing connection-end teardown to reclaim the overlay. An in-flight guard tracks per-connection statement execution so a legitimately long-running query is never idle-killed, and the idle window only starts once a statement has completed.
1 parent b51a50e commit 4a17903

11 files changed

Lines changed: 371 additions & 8 deletions

File tree

nodedb-test-support/src/pgwire_harness/start.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,14 @@ pub(super) struct StartConfig {
3535
/// Raft snapshot builder requires a routing table to resolve a group's
3636
/// vShards, so round-trip tests inject one here.
3737
pub routing: Option<nodedb_cluster::RoutingTable>,
38+
/// Idle session timeout in seconds applied to the node's `SharedState`
39+
/// before it is shared. `0` (the default) leaves the idle watchdog
40+
/// disabled; a small value (e.g. `1`) lets tests exercise the pgwire
41+
/// listener's idle-timeout force-close and overlay reclamation.
42+
pub idle_timeout_secs: u64,
43+
/// Absolute session lifetime in seconds applied to `SharedState` before it
44+
/// is shared. `0` (the default) disables the absolute cap.
45+
pub session_absolute_timeout_secs: u64,
3846
}
3947

4048
impl Default for StartConfig {
@@ -44,6 +52,8 @@ impl Default for StartConfig {
4452
lockout: None,
4553
columnar_flush_threshold: None,
4654
routing: None,
55+
idle_timeout_secs: 0,
56+
session_absolute_timeout_secs: 0,
4757
}
4858
}
4959
}
@@ -84,6 +94,20 @@ impl TestServer {
8494
.await
8595
}
8696

97+
/// Spawn a single-core NodeDB server with a short idle session timeout so
98+
/// tests can exercise the pgwire listener's idle-timeout watchdog: an
99+
/// idle-in-transaction connection is force-closed after `idle_secs`,
100+
/// triggering `on_connection_end` overlay reclamation. All other settings
101+
/// stay at their defaults (trust-mode auth, lockout disabled, no absolute
102+
/// cap).
103+
pub async fn start_with_idle_timeout(idle_secs: u64) -> Self {
104+
Self::start_with_config(StartConfig {
105+
idle_timeout_secs: idle_secs,
106+
..Default::default()
107+
})
108+
.await
109+
}
110+
87111
/// Spawn a single-core NodeDB server with a cluster routing table
88112
/// installed on `SharedState::cluster_routing`.
89113
///
@@ -146,6 +170,10 @@ impl TestServer {
146170
if let Some(routing) = cfg.routing {
147171
s.cluster_routing = Some(std::sync::Arc::new(std::sync::RwLock::new(routing)));
148172
}
173+
s.set_session_timeouts_for_test(
174+
cfg.idle_timeout_secs,
175+
cfg.session_absolute_timeout_secs,
176+
);
149177
}
150178
let shared = shared;
151179

nodedb/src/control/server/pgwire/factory.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,19 @@ impl NodeDbPgHandlerFactory {
232232
self.handler.reclaim_open_txn(addr).await;
233233
self.handler.sessions.remove(addr);
234234
}
235+
236+
/// Whether the connection at `addr` is eligible for idle timeout right now:
237+
/// its session has zero statements in flight and has been silent for at
238+
/// least `idle_ms`. Used by the pgwire listener watchdog, which owns the
239+
/// per-connection task but cannot see inside pgwire's `process_socket`
240+
/// loop. Returns `false` when the session is missing (nothing to time out).
241+
pub fn session_idle_eligible(&self, addr: &std::net::SocketAddr, idle_ms: u64) -> bool {
242+
self.handler.sessions.idle_eligible(
243+
addr,
244+
idle_ms,
245+
crate::control::server::shared::session::now_unix_ms(),
246+
)
247+
}
235248
}
236249

237250
impl PgWireServerHandlers for NodeDbPgHandlerFactory {

nodedb/src/control/server/pgwire/handler/core.rs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ use crate::control::state::SharedState;
3333
use crate::types::RequestId;
3434

3535
use super::super::types::notice_warning;
36+
use super::in_flight::InFlightGuard;
3637
use super::plan::extract_collection;
3738
use super::prepared::{NodeDbQueryParser, ParsedStatement};
3839
use crate::control::server::shared::session::{SessionStore, TransactionState};
@@ -314,6 +315,12 @@ impl SimpleQueryHandler for NodeDbPgHandler {
314315
let addr = client.socket_addr();
315316
self.sessions.ensure_session(addr);
316317

318+
// Mark this statement in flight for its whole duration so the idle
319+
// watchdog never closes a connection mid-statement; the guard also
320+
// stamps last-activity on drop, restarting the idle window at
321+
// statement completion.
322+
let _in_flight = InFlightGuard::new(&self.sessions, addr);
323+
317324
let identity = self.resolve_identity(client, &addr)?;
318325
self.enforce_database_access(&identity, &addr)?;
319326

@@ -443,10 +450,15 @@ impl ExtendedQueryHandler for NodeDbPgHandler {
443450
C::Error: Debug,
444451
PgWireError: From<<C as Sink<PgWireBackendMessage>>::Error>,
445452
{
453+
let addr = client.socket_addr();
454+
// Mark this statement in flight for the duration of execution so a
455+
// long-running prepared statement is never idle-killed; the guard
456+
// stamps last-activity on drop.
457+
let _in_flight = InFlightGuard::new(&self.sessions, addr);
458+
446459
let result = self.execute_prepared(client, portal, max_rows).await;
447460
// Mirror the simple-query path: surface any queued NOTICE messages
448461
// (e.g. `truncated_before_horizon`) before returning.
449-
let addr = client.socket_addr();
450462
for message in self.sessions.drain_notices(&addr) {
451463
let notice = notice_warning(&message);
452464
let _ = client
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
3+
//! RAII in-flight guard for pgwire statement execution.
4+
//!
5+
//! Constructed at the top of a query handler and dropped when the statement
6+
//! finishes (including early return, error, or panic-unwind). On construction
7+
//! it bumps the connection's in-flight counter; on drop it decrements it and
8+
//! stamps last-activity to "now". The listener watchdog reads that state to
9+
//! decide idle eligibility, so this guard is what guarantees a long-running
10+
//! statement is never idle-killed and that the idle window only starts once a
11+
//! statement has actually completed.
12+
13+
use std::net::SocketAddr;
14+
15+
use crate::control::server::shared::session::SessionStore;
16+
17+
/// Marks a statement as in flight for the lifetime of the guard.
18+
pub(crate) struct InFlightGuard<'a> {
19+
sessions: &'a SessionStore,
20+
addr: SocketAddr,
21+
}
22+
23+
impl<'a> InFlightGuard<'a> {
24+
/// Begin a request: increment the connection's in-flight counter.
25+
pub(crate) fn new(sessions: &'a SessionStore, addr: SocketAddr) -> Self {
26+
sessions.begin_request(&addr);
27+
Self { sessions, addr }
28+
}
29+
}
30+
31+
impl Drop for InFlightGuard<'_> {
32+
fn drop(&mut self) {
33+
// Decrement in-flight and stamp last-activity on every exit path.
34+
self.sessions.end_request(&self.addr);
35+
}
36+
}

nodedb/src/control/server/pgwire/handler/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ mod cursor_cmds;
77
mod cursor_query;
88
mod dispatch;
99
mod facet;
10+
mod in_flight;
1011
pub mod listen_notify;
1112
mod listen_notify_exec;
1213
mod live_select;

nodedb/src/control/server/pgwire/listener.rs

Lines changed: 79 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
use std::net::SocketAddr;
44
use std::sync::Arc;
5-
use std::time::Duration;
5+
use std::time::{Duration, Instant};
66

77
use tokio::net::TcpListener;
88
use tokio::sync::Semaphore;
@@ -60,6 +60,13 @@ impl PgListener {
6060
bus: crate::control::shutdown::ShutdownBus,
6161
) -> crate::Result<()> {
6262
let conn_state = Arc::clone(&state);
63+
// Session-timeout policy, read once at listener startup (config is fixed
64+
// for the process lifetime). `idle` closes a connection that has been
65+
// silent between statements for this many seconds — including an
66+
// idle-in-transaction connection holding a staged-write overlay.
67+
// `absolute` caps total connection lifetime. `0` disables either.
68+
let idle_timeout_secs = conn_state.idle_timeout_secs();
69+
let absolute_timeout_secs = conn_state.session_absolute_timeout_secs();
6370
let factory = Arc::new(NodeDbPgHandlerFactory::new(state, auth_mode));
6471

6572
// Register with the shutdown bus so the sequencer waits for us to drain
@@ -122,15 +129,27 @@ impl PgListener {
122129
info!(%peer_addr, "new pgwire connection");
123130
let factory = Arc::clone(&factory);
124131
let tls = tls_acceptor.clone();
132+
let idle = idle_timeout_secs;
133+
let absolute = absolute_timeout_secs;
125134
connections.spawn(async move {
126-
if let Err(e) =
127-
process_socket(stream, tls, Arc::clone(&factory)).await
128-
{
129-
warn!(%peer_addr, error = %e, "pgwire session error");
135+
if idle == 0 && absolute == 0 {
136+
// No session timeouts configured: run the
137+
// plain socket loop with no watchdog wakeups.
138+
if let Err(e) =
139+
process_socket(stream, tls, Arc::clone(&factory)).await
140+
{
141+
warn!(%peer_addr, error = %e, "pgwire session error");
142+
}
143+
} else {
144+
run_with_watchdog(
145+
stream, tls, &factory, peer_addr, idle, absolute,
146+
)
147+
.await;
130148
}
131149
// Reclaim any abandoned-transaction Data-Plane
132150
// overlays and drop the shared session entry now
133-
// that the connection has ended.
151+
// that the connection has ended (whether it ended
152+
// on its own or was force-closed by the watchdog).
134153
factory.on_connection_end(&peer_addr).await;
135154
drop(permit);
136155
peer_addr
@@ -191,3 +210,57 @@ impl PgListener {
191210
Ok(())
192211
}
193212
}
213+
214+
/// Run `process_socket` under an idle + absolute session-timeout watchdog.
215+
///
216+
/// pgwire's `process_socket` owns the connection loop and is hard-typed to a
217+
/// `TcpStream`, so timeouts cannot be enforced inside it. Instead we race the
218+
/// socket future against a bounded re-check tick: on each wake, close the
219+
/// connection — by dropping the future, which drops the socket — if the
220+
/// absolute lifetime is exceeded or the connection is idle-eligible (zero
221+
/// in-flight statements AND silent past the idle window). A statement in
222+
/// flight keeps the connection non-idle, so a legitimately long-running query
223+
/// is never idle-killed. The caller runs `on_connection_end` afterwards to
224+
/// reclaim any staged overlay and drop the session entry.
225+
///
226+
/// Only invoked when at least one of `idle`/`absolute` is non-zero; the
227+
/// all-disabled path skips the watchdog entirely (no wakeups).
228+
async fn run_with_watchdog(
229+
stream: tokio::net::TcpStream,
230+
tls: Option<pgwire::tokio::TlsAcceptor>,
231+
factory: &Arc<NodeDbPgHandlerFactory>,
232+
peer_addr: SocketAddr,
233+
idle: u64,
234+
absolute: u64,
235+
) {
236+
let started = Instant::now();
237+
let mut fut = std::pin::pin!(process_socket(stream, tls, Arc::clone(factory)));
238+
// Bounded re-check tick — never a busy loop. Activity may advance the idle
239+
// deadline between wakes, which is recomputed via `session_idle_eligible`.
240+
let tick = Duration::from_secs(1);
241+
loop {
242+
tokio::select! {
243+
r = &mut fut => {
244+
if let Err(e) = r {
245+
warn!(%peer_addr, error = %e, "pgwire session error");
246+
}
247+
break;
248+
}
249+
_ = tokio::time::sleep(tick) => {
250+
if absolute > 0 && started.elapsed().as_secs() >= absolute {
251+
info!(%peer_addr, absolute, "pgwire absolute session timeout, closing");
252+
break;
253+
}
254+
if idle > 0
255+
&& factory.session_idle_eligible(&peer_addr, idle.saturating_mul(1000))
256+
{
257+
info!(%peer_addr, idle, "pgwire idle session timeout, closing");
258+
break;
259+
}
260+
}
261+
}
262+
}
263+
// On a timeout break the loop exits with `fut` still pending; it is dropped
264+
// here at scope end, which closes the socket. The caller then runs the
265+
// connection-end teardown hook.
266+
}

nodedb/src/control/server/shared/session/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ pub use self::staging_gate::{
4040
DetachedTxnScope, DmlTxnCtx, InTxnRoute, StagedTagKind, StagedWriteOutcome, StagingGateError,
4141
route_in_tx_write,
4242
};
43+
pub(crate) use self::state::now_unix_ms;
4344
pub use self::state::{ConnSession, CursorState, TransactionState};
4445
pub use self::store::SessionStore;
4546
pub use self::temp_tables::TempTableEntry;

nodedb/src/control/server/shared/session/state.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,17 @@
33
//! Per-connection session state types.
44
55
use std::collections::{BTreeMap, BTreeSet, HashMap};
6+
use std::sync::atomic::{AtomicU32, AtomicU64};
7+
use std::time::{SystemTime, UNIX_EPOCH};
8+
9+
/// Milliseconds since the Unix epoch. Falls back to `0` instead of panicking
10+
/// if the system clock is set before the epoch (`duration_since` errors).
11+
pub(crate) fn now_unix_ms() -> u64 {
12+
SystemTime::now()
13+
.duration_since(UNIX_EPOCH)
14+
.map(|d| d.as_millis() as u64)
15+
.unwrap_or(0)
16+
}
617

718
use crate::types::{DatabaseId, Lsn, TenantId, TxnId, VShardId};
819
use nodedb_physical::physical_task::PhysicalTask;
@@ -150,6 +161,15 @@ pub struct ConnSession {
150161
/// GAP_FREE sequence reservations pending commit/rollback.
151162
/// On COMMIT: each reservation is finalized. On ROLLBACK: counter decremented.
152163
pub pending_sequence_reservations: Vec<crate::control::sequence::gap_free::ReservationHandle>,
164+
/// Millis-since-epoch of the last statement COMPLETION on this connection
165+
/// (also set to "now" at connection start). Read by the pgwire listener
166+
/// watchdog to decide idle eligibility: a connection is idle only when it
167+
/// has been silent (no statement completing) for the idle window.
168+
pub last_activity_ms: AtomicU64,
169+
/// Count of currently-executing statements on this connection. A connection
170+
/// is idle-eligible only when this is zero — a legitimately long-running
171+
/// statement (in flight) must never be idle-killed.
172+
pub in_flight: AtomicU32,
153173
}
154174

155175
impl ConnSession {
@@ -199,6 +219,8 @@ impl ConnSession {
199219
temp_tables: super::temp_tables::TempTableRegistry::new(),
200220
plan_cache: crate::control::server::shared::session::plan_cache::PlanCache::new(128),
201221
pending_sequence_reservations: Vec::new(),
222+
last_activity_ms: AtomicU64::new(now_unix_ms()),
223+
in_flight: AtomicU32::new(0),
202224
}
203225
}
204226
}

nodedb/src/control/server/shared/session/store.rs

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,13 @@
55
use std::collections::HashMap;
66
use std::net::SocketAddr;
77
use std::sync::RwLock;
8+
use std::sync::atomic::Ordering::Relaxed;
89

910
use nodedb_types::DatabaseId;
1011

1112
use crate::types::TenantId;
1213

13-
use super::state::{ConnSession, TransactionState};
14+
use super::state::{ConnSession, TransactionState, now_unix_ms};
1415

1516
/// Concurrent session store — keyed by socket address.
1617
pub struct SessionStore {
@@ -180,6 +181,51 @@ impl SessionStore {
180181
.identity = Some(identity);
181182
}
182183

184+
/// Record the start of a statement executing on this connection. Bumps the
185+
/// in-flight counter so the idle watchdog never closes a connection with a
186+
/// statement in progress. Creates the session entry if absent (mirrors
187+
/// `set_identity`) so the extended-query path — which can begin execution
188+
/// before `ensure_session` runs — still has its in-flight state tracked.
189+
pub fn begin_request(&self, addr: &SocketAddr) {
190+
let mut sessions = self.sessions.write().unwrap_or_else(|p| p.into_inner());
191+
sessions
192+
.entry(*addr)
193+
.or_insert_with(ConnSession::new)
194+
.in_flight
195+
.fetch_add(1, Relaxed);
196+
}
197+
198+
/// Record the completion of a statement on this connection: decrement the
199+
/// in-flight counter (saturating — never underflows if the session was
200+
/// already removed by a concurrent teardown) and stamp last-activity to
201+
/// "now" so the idle window restarts from statement completion.
202+
pub fn end_request(&self, addr: &SocketAddr) {
203+
let mut sessions = self.sessions.write().unwrap_or_else(|p| p.into_inner());
204+
if let Some(session) = sessions.get_mut(addr) {
205+
// Exclusive write lock held: no concurrent mutator, so a
206+
// load-check-store is a safe saturating decrement.
207+
if session.in_flight.load(Relaxed) > 0 {
208+
session.in_flight.fetch_sub(1, Relaxed);
209+
}
210+
session.last_activity_ms.store(now_unix_ms(), Relaxed);
211+
}
212+
}
213+
214+
/// Whether the connection at `addr` is eligible for idle timeout: the
215+
/// session exists, has zero statements in flight, and its last activity is
216+
/// at least `idle_ms` in the past relative to `now_ms`. Returns `false`
217+
/// when the session is missing — nothing to time out.
218+
pub fn idle_eligible(&self, addr: &SocketAddr, idle_ms: u64, now_ms: u64) -> bool {
219+
let sessions = self.sessions.read().unwrap_or_else(|p| p.into_inner());
220+
match sessions.get(addr) {
221+
Some(session) => {
222+
session.in_flight.load(Relaxed) == 0
223+
&& now_ms.saturating_sub(session.last_activity_ms.load(Relaxed)) >= idle_ms
224+
}
225+
None => false,
226+
}
227+
}
228+
183229
/// Reset per-session state for a `USE DATABASE` switch:
184230
/// 1. Aborts any open transaction (discards tx_buffer, resets state to Idle).
185231
/// 2. Clears all SQL-level prepared statements.

0 commit comments

Comments
 (0)