Skip to content

Commit b51a50e

Browse files
committed
fix(control): reclaim pgwire transaction overlays on connection teardown
An abandoned pgwire transaction's Data-Plane staging overlays leaked forever when a connection ended (disconnect, timeout, EOF) without an explicit COMMIT/ROLLBACK, since nothing dispatched DropTxnOverlay for it. Stash the resolved identity per connection and reclaim any open transaction via the existing rollback path on connection end.
1 parent e57e68e commit b51a50e

8 files changed

Lines changed: 208 additions & 3 deletions

File tree

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,9 @@ impl TestServer {
166166
core_dir: dir.path().to_path_buf(),
167167
core_array_catalog: shared.array_catalog.clone(),
168168
event_producer,
169-
core_metrics: None,
169+
// Share the Control-Plane `SystemMetrics` so the Data-Plane
170+
// core updates the same `active_txn_overlays` gauge tests read.
171+
core_metrics: shared.system_metrics.clone(),
170172
governor: shared.governor.clone(),
171173
replay: None,
172174
graph_tuning: nodedb_types::config::tuning::GraphTuning::default(),

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,14 @@ impl NodeDbPgHandlerFactory {
224224
state,
225225
}
226226
}
227+
228+
/// Reclaim an abandoned transaction's overlays and drop the shared session
229+
/// entry when a pgwire connection ends. Idempotent — a no-op when the
230+
/// connection had no open transaction.
231+
pub async fn on_connection_end(&self, addr: &std::net::SocketAddr) {
232+
self.handler.reclaim_open_txn(addr).await;
233+
self.handler.sessions.remove(addr);
234+
}
227235
}
228236

229237
impl PgWireServerHandlers for NodeDbPgHandlerFactory {

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,11 @@ impl NodeDbPgHandler {
146146
}
147147
}
148148

149+
// Stash the identity in force for this query so a connection torn down
150+
// mid-transaction can reclaim its Data-Plane staging overlays without a
151+
// live query — `on_connection_end` reads it back to drive `run_rollback`.
152+
self.sessions.set_identity(addr, identity.clone());
153+
149154
Ok(identity)
150155
}
151156

nodedb/src/control/server/pgwire/handler/transaction_cmds/begin_rollback.rs

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use pgwire::api::results::{Response, Tag};
99
use pgwire::error::{ErrorInfo, PgWireError, PgWireResult};
1010

1111
use crate::control::security::identity::AuthenticatedIdentity;
12-
use crate::control::server::shared::session::lifecycle;
12+
use crate::control::server::shared::session::{TransactionState, lifecycle};
1313

1414
use super::super::core::NodeDbPgHandler;
1515
use super::commit::PgwireTxnDp;
@@ -46,4 +46,26 @@ impl NodeDbPgHandler {
4646
lifecycle::run_rollback(&self.sessions, addr, identity, &self.state, &dp).await;
4747
Ok(vec![Response::Execution(Tag::new("ROLLBACK"))])
4848
}
49+
50+
/// Reclaim an abandoned transaction's Data-Plane staging overlays when a
51+
/// pgwire connection ends without COMMIT/ROLLBACK.
52+
///
53+
/// A no-op when the connection had no open transaction (the common case).
54+
/// Otherwise drives the same neutral `run_rollback` path as an explicit
55+
/// ROLLBACK, using the identity stashed by `resolve_identity` on the last
56+
/// query — without it the overlays (keyed by `txn_id` per staged vShard)
57+
/// would leak for the process lifetime.
58+
pub(in crate::control::server::pgwire) async fn reclaim_open_txn(
59+
&self,
60+
addr: &std::net::SocketAddr,
61+
) {
62+
if self.sessions.transaction_state(addr) == TransactionState::Idle {
63+
return;
64+
}
65+
let Some(identity) = self.sessions.identity(addr) else {
66+
return;
67+
};
68+
let dp = PgwireTxnDp { handler: self };
69+
lifecycle::run_rollback(&self.sessions, addr, &identity, &self.state, &dp).await;
70+
}
4971
}

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,15 @@ impl PgListener {
123123
let factory = Arc::clone(&factory);
124124
let tls = tls_acceptor.clone();
125125
connections.spawn(async move {
126-
if let Err(e) = process_socket(stream, tls, factory).await {
126+
if let Err(e) =
127+
process_socket(stream, tls, Arc::clone(&factory)).await
128+
{
127129
warn!(%peer_addr, error = %e, "pgwire session error");
128130
}
131+
// Reclaim any abandoned-transaction Data-Plane
132+
// overlays and drop the shared session entry now
133+
// that the connection has ended.
134+
factory.on_connection_end(&peer_addr).await;
129135
drop(permit);
130136
peer_addr
131137
});

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,15 @@ pub struct ConnSession {
7272
/// with `42501` before this field is written), so the identity-bound
7373
/// invariant continues to hold for tenant-scoped users.
7474
pub effective_tenant_id: Option<TenantId>,
75+
/// Authenticated identity resolved for queries on this connection.
76+
///
77+
/// Stashed by `resolve_identity` (the per-query auth chokepoint) so that a
78+
/// connection torn down mid-transaction can reclaim its Data-Plane staging
79+
/// overlays without a live query in flight — `run_rollback` requires the
80+
/// identity (tenant + username) to dispatch `MetaOp::DropTxnOverlay` and to
81+
/// audit any GAP_FREE reservation rollback. `None` until the first query
82+
/// resolves an identity on this connection.
83+
pub identity: Option<crate::control::security::identity::AuthenticatedIdentity>,
7584
/// Session parameters set via SET commands.
7685
pub parameters: HashMap<String, String>,
7786
/// Buffered write tasks accumulated between BEGIN and COMMIT.
@@ -171,6 +180,7 @@ impl ConnSession {
171180
tx_state: TransactionState::Idle,
172181
current_database: None,
173182
effective_tenant_id: None,
183+
identity: None,
174184
parameters,
175185
tx_buffer: Vec::new(),
176186
tx_snapshot_lsn: None,

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,36 @@ impl SessionStore {
150150
}
151151
}
152152

153+
/// Read the identity resolved for queries on this connection, if any.
154+
/// Returns `None` when no query has resolved an identity yet (the session
155+
/// never issued a statement past auth) or the session does not exist.
156+
pub fn identity(
157+
&self,
158+
addr: &SocketAddr,
159+
) -> Option<crate::control::security::identity::AuthenticatedIdentity> {
160+
let sessions = self.sessions.read().unwrap_or_else(|p| p.into_inner());
161+
sessions.get(addr).and_then(|s| s.identity.clone())
162+
}
163+
164+
/// Stash the identity resolved for queries on this connection. Called from
165+
/// the per-query auth chokepoint (`resolve_identity`) so a connection torn
166+
/// down mid-transaction can reclaim its Data-Plane overlays without a live
167+
/// query. Overwrites any prior value — the identity in force for the most
168+
/// recent query is the one teardown must use. Creates the session entry if
169+
/// absent so the extended-query path (which resolves identity before
170+
/// `ensure_session`) still records it.
171+
pub fn set_identity(
172+
&self,
173+
addr: &SocketAddr,
174+
identity: crate::control::security::identity::AuthenticatedIdentity,
175+
) {
176+
let mut sessions = self.sessions.write().unwrap_or_else(|p| p.into_inner());
177+
sessions
178+
.entry(*addr)
179+
.or_insert_with(ConnSession::new)
180+
.identity = Some(identity);
181+
}
182+
153183
/// Reset per-session state for a `USE DATABASE` switch:
154184
/// 1. Aborts any open transaction (discards tx_buffer, resets state to Idle).
155185
/// 2. Clears all SQL-level prepared statements.
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
3+
//! Regression test: an abandoned PGWIRE-protocol transaction's Data-Plane
4+
//! staging overlays (`txn_overlays` / `graph_txn_overlays`, keyed by
5+
//! `txn_id`) leaked forever if the connection ended (abrupt disconnect,
6+
//! idle/absolute timeout, or clean EOF) without a `COMMIT`/`ROLLBACK` --
7+
//! nothing dispatched `MetaOp::DropTxnOverlay` for it. The pgwire listener
8+
//! now calls `NodeDbPgHandlerFactory::on_connection_end` on every connection
9+
//! exit, which reclaims any still-open transaction's overlay via
10+
//! `lifecycle::run_rollback` and drops the shared session entry.
11+
//!
12+
//! This test proves reclamation (rather than just "row not visible", which
13+
//! cannot distinguish reclaimed from leaked-but-invisible) via the
14+
//! `active_txn_overlays` gauge on `SystemMetrics`: it rises when the
15+
//! transaction's first staged write materializes the overlay, and must fall
16+
//! back to baseline once the abandoned connection is torn down.
17+
18+
mod common;
19+
20+
use std::sync::atomic::Ordering;
21+
use std::time::{Duration, Instant};
22+
23+
use common::pgwire_harness::TestServer;
24+
25+
/// Poll `active_txn_overlays` until `pred` is satisfied or `deadline` elapses.
26+
/// Returns the last observed value; callers assert against it so a timeout
27+
/// panics with the actual value instead of a bare "timed out".
28+
async fn poll_gauge(server: &TestServer, deadline: Duration, pred: impl Fn(u64) -> bool) -> u64 {
29+
let metrics = server
30+
.shared
31+
.system_metrics
32+
.as_ref()
33+
.expect("system_metrics must be wired into the pgwire harness");
34+
let start = Instant::now();
35+
loop {
36+
let value = metrics.active_txn_overlays.load(Ordering::Relaxed);
37+
if pred(value) || start.elapsed() >= deadline {
38+
return value;
39+
}
40+
tokio::time::sleep(Duration::from_millis(20)).await;
41+
}
42+
}
43+
44+
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
45+
async fn pgwire_abandoned_txn_overlay_reclaimed_on_teardown() {
46+
let server = TestServer::start().await;
47+
48+
let baseline = server
49+
.shared
50+
.system_metrics
51+
.as_ref()
52+
.expect("system_metrics must be wired into the pgwire harness")
53+
.active_txn_overlays
54+
.load(Ordering::Relaxed);
55+
assert_eq!(baseline, 0, "gauge must start at zero: {baseline}");
56+
57+
// Open a SEPARATE connection the test OWNS (not `server.client`, which the
58+
// harness owns) so we can close it mid-transaction. tokio-postgres runs the
59+
// socket in a spawned task; we keep its JoinHandle to abort it later.
60+
let conn_str = format!(
61+
"host=127.0.0.1 port={} user=nodedb dbname=nodedb",
62+
server.pg_port
63+
);
64+
let (client, connection) = tokio_postgres::connect(&conn_str, tokio_postgres::NoTls)
65+
.await
66+
.expect("owned pgwire connection must connect");
67+
let conn_handle = tokio::spawn(async move {
68+
let _ = connection.await;
69+
});
70+
71+
client
72+
.batch_execute(
73+
"CREATE COLLECTION pg_txn_overlay_teardown (id STRING PRIMARY KEY, n INT) \
74+
WITH (engine='document_schemaless')",
75+
)
76+
.await
77+
.expect("CREATE must succeed");
78+
79+
client
80+
.batch_execute("BEGIN")
81+
.await
82+
.expect("BEGIN must succeed");
83+
client
84+
.batch_execute("INSERT INTO pg_txn_overlay_teardown (id, n) VALUES ('a', 1)")
85+
.await
86+
.expect("in-tx INSERT must succeed");
87+
88+
// The staged write must have materialized the overlay, bumping the gauge
89+
// above baseline -- confirms there is something to reclaim.
90+
let after_stage = poll_gauge(&server, Duration::from_secs(5), |v| v > baseline).await;
91+
assert!(
92+
after_stage > baseline,
93+
"staged write must raise active_txn_overlays above baseline {baseline}, got {after_stage}"
94+
);
95+
96+
// Abruptly abandon the connection -- no COMMIT/ROLLBACK. Dropping the
97+
// Client resolves the Connection future; aborting its task guarantees the
98+
// socket is dropped so the server sees EOF and runs `on_connection_end`.
99+
drop(client);
100+
conn_handle.abort();
101+
let _ = conn_handle.await;
102+
103+
// The abandoned transaction's overlay must be reclaimed on teardown,
104+
// bringing the gauge back down to baseline.
105+
let after_teardown = poll_gauge(&server, Duration::from_secs(5), |v| v == baseline).await;
106+
assert_eq!(
107+
after_teardown, baseline,
108+
"abandoned txn overlay must be reclaimed on connection teardown, \
109+
active_txn_overlays still {after_teardown} (baseline {baseline})"
110+
);
111+
112+
// Belt-and-suspenders: the harness-owned autocommit connection must not see
113+
// the staged row -- it was never committed.
114+
let rows = server
115+
.query_text("SELECT n FROM pg_txn_overlay_teardown WHERE id = 'a'")
116+
.await
117+
.expect("post-teardown SELECT must succeed");
118+
assert!(
119+
rows.is_empty(),
120+
"the never-committed staged row must not be visible: {rows:?}"
121+
);
122+
}

0 commit comments

Comments
 (0)