Skip to content

Commit b1319ed

Browse files
sbernauerclaude
andauthored
fix: Handle Redis connection errors gracefully instead of panicking (#111)
* fix: Handle Redis connection errors gracefully instead of panicking When Redis goes down (e.g. master failover), a broken pipe error would cause `get_queued_query_count` to panic due to an `.unwrap()`, which then poisoned the RwLock used in metrics callbacks, cascading into further panics and leaving pods unresponsive until the liveness probe killed them. - Replace `.unwrap()` with proper error propagation in `get_queued_query_count` - Handle poisoned locks, closed channels, and panicked threads gracefully in both metrics callbacks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Rename e to error * changelog --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent bc8554f commit b1319ed

3 files changed

Lines changed: 86 additions & 26 deletions

File tree

CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file.
44

55
## [Unreleased]
66

7+
### Fixed
8+
9+
- Handle Redis connection errors (e.g. broken pipe during master failover) gracefully instead of panicking.
10+
Previously, `get_queued_query_count` would `.unwrap()` on the Redis result, causing a panic that poisoned
11+
the metrics `RwLock`, cascading into further panics and leaving pods unresponsive ([#111]).
12+
13+
[#111]: https://github.com/stackabletech/trino-lb/pull/111
14+
715
## [0.6.0] - 2026-02-17
816

917
### Added

trino-lb-persistence/src/redis/mod.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,12 @@ pub enum Error {
116116
#[snafu(display("Invalid response from compare and set lua script. Expected either 0 or 1"))]
117117
InvalidCASScriptResponse { response: u64 },
118118

119+
#[snafu(display("Failed to get queued query count for cluster group {cluster_group:?}"))]
120+
GetQueuedQueryCount {
121+
source: RedisError,
122+
cluster_group: String,
123+
},
124+
119125
#[snafu(display("Failed to list queued queries for cluster group {cluster_group:?}"))]
120126
ListQueuedQueries {
121127
source: RedisError,
@@ -428,7 +434,7 @@ where
428434
.connection()
429435
.scard::<_, Option<u64>>(queued_query_set_name(cluster_group))
430436
.await
431-
.unwrap()
437+
.context(GetQueuedQueryCountSnafu { cluster_group })?
432438
// The set might not be there yet, as no queries have been queued for this cluster group so far.
433439
.unwrap_or_default())
434440
}

trino-lb/src/metrics.rs

Lines changed: 71 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -142,19 +142,42 @@ impl Metrics {
142142

143143
meter
144144
.register_callback(&[queued_queries_metric.as_any()], move |observer| {
145-
ping_sender.send(()).unwrap();
145+
if let Err(err) = ping_sender.send(()) {
146+
error!(error = ?err, "Failed to send ping for queued_queries metric");
147+
return;
148+
}
146149
let queued_queries = std::thread::scope(|s| {
147-
s.spawn(|| metrics_receiver.write().unwrap().blocking_recv().unwrap())
148-
.join()
149-
.unwrap()
150+
s.spawn(|| {
151+
let mut receiver = match metrics_receiver.write() {
152+
Ok(r) => r,
153+
Err(err) => {
154+
error!(error = %err, "Failed to acquire write lock for queued_queries metric");
155+
return None;
156+
}
157+
};
158+
match receiver.blocking_recv() {
159+
Some(v) => Some(v),
160+
None => {
161+
error!("queued_queries metrics channel closed");
162+
None
163+
}
164+
}
165+
})
166+
.join()
167+
.unwrap_or_else(|err| {
168+
error!(error = ?err, "queued_queries metrics thread panicked");
169+
None
170+
})
150171
});
151172

152-
for (cluster_group, queued) in queued_queries {
153-
observer.observe_u64(
154-
&queued_queries_metric,
155-
queued,
156-
[KeyValue::new("cluster-group", cluster_group)].as_ref(),
157-
);
173+
if let Some(queued_queries) = queued_queries {
174+
for (cluster_group, queued) in queued_queries {
175+
observer.observe_u64(
176+
&queued_queries_metric,
177+
queued,
178+
[KeyValue::new("cluster-group", cluster_group)].as_ref(),
179+
);
180+
}
158181
}
159182
})
160183
.context(RegisterMetricsCallbackSnafu)?;
@@ -183,24 +206,47 @@ impl Metrics {
183206
.register_callback(
184207
&[cluster_counts_per_state_metric.as_any()],
185208
move |observer| {
186-
ping_sender.send(()).unwrap();
209+
if let Err(err) = ping_sender.send(()) {
210+
error!(error = ?err, "Failed to send ping for cluster_counts_per_state metric");
211+
return;
212+
}
187213
let cluster_counts = std::thread::scope(|s| {
188-
s.spawn(|| metrics_receiver.write().unwrap().blocking_recv().unwrap())
189-
.join()
190-
.unwrap()
214+
s.spawn(|| {
215+
let mut receiver = match metrics_receiver.write() {
216+
Ok(r) => r,
217+
Err(err) => {
218+
error!(error = %err, "Failed to acquire write lock for cluster_counts_per_state metric");
219+
return None;
220+
}
221+
};
222+
match receiver.blocking_recv() {
223+
Some(v) => Some(v),
224+
None => {
225+
error!("cluster_counts_per_state metrics channel closed");
226+
None
227+
}
228+
}
229+
})
230+
.join()
231+
.unwrap_or_else(|err| {
232+
error!(error = ?err, "cluster_counts_per_state metrics thread panicked");
233+
None
234+
})
191235
});
192236

193-
for (cluster_group, counts) in cluster_counts {
194-
for (state, count) in counts {
195-
observer.observe_u64(
196-
&cluster_counts_per_state_metric,
197-
count,
198-
[
199-
KeyValue::new("cluster-group", cluster_group.clone()),
200-
KeyValue::new::<_, &str>("state", state.into()),
201-
]
202-
.as_ref(),
203-
);
237+
if let Some(cluster_counts) = cluster_counts {
238+
for (cluster_group, counts) in cluster_counts {
239+
for (state, count) in counts {
240+
observer.observe_u64(
241+
&cluster_counts_per_state_metric,
242+
count,
243+
[
244+
KeyValue::new("cluster-group", cluster_group.clone()),
245+
KeyValue::new::<_, &str>("state", state.into()),
246+
]
247+
.as_ref(),
248+
);
249+
}
204250
}
205251
}
206252
},

0 commit comments

Comments
 (0)