Skip to content

Commit da88253

Browse files
nshalmanclaude
andcommitted
Report handler panics as panics, not client disconnects
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 29ab11c commit da88253

4 files changed

Lines changed: 124 additions & 0 deletions

File tree

CHANGELOG.adoc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515

1616
https://github.com/oxidecomputer/dropshot/compare/v0.17.1\...HEAD[Full list of commits]
1717

18+
* A handler panic that unwinds through the request task is now reported as a panic ("request handling panicked", with no status code) instead of being misreported as a client disconnection with the non-standard 499 status code. Panic propagation itself is unchanged.
19+
1820
== 0.17.1 (released 2026-06-02)
1921

2022
https://github.com/oxidecomputer/dropshot/compare/v0.17.0\...v0.17.1[Full list of commits]

dropshot/src/server.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -805,6 +805,13 @@ async fn http_request_handle_wrap<C: ServerContext>(
805805
let on_disconnect = guard((), |_| {
806806
let latency_us = start_time.elapsed().as_micros();
807807

808+
if std::thread::panicking() {
809+
error!(request_log, "request handling panicked";
810+
"latency_us" => latency_us,
811+
);
812+
return;
813+
}
814+
808815
warn!(request_log, "request handling cancelled (client disconnected)";
809816
"latency_us" => latency_us,
810817
);

dropshot/tests/integration-tests/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ mod multipart;
2121
mod openapi;
2222
mod pagination;
2323
mod pagination_schema;
24+
mod panic_handling;
2425
mod path_names;
2526
mod starter;
2627
mod streaming;
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
// Copyright 2026 Oxide Computer Company
2+
3+
//! Test cases for how panicking HTTP handlers are reported.
4+
5+
use dropshot::test_util::{ClientTestContext, read_bunyan_log};
6+
use dropshot::{
7+
ApiDescription, ConfigLogging, ConfigLoggingIfExists, ConfigLoggingLevel,
8+
HttpError, HttpResponseOk, RequestContext, ServerBuilder, endpoint,
9+
};
10+
use http::{Method, StatusCode};
11+
use tokio::io::{AsyncReadExt, AsyncWriteExt};
12+
13+
fn api() -> ApiDescription<()> {
14+
let mut api = ApiDescription::new();
15+
api.register(handler_panic).unwrap();
16+
api.register(handler_ok).unwrap();
17+
api
18+
}
19+
20+
#[endpoint {
21+
method = GET,
22+
path = "/panic",
23+
}]
24+
async fn handler_panic(
25+
_rqctx: RequestContext<()>,
26+
) -> Result<HttpResponseOk<u64>, HttpError> {
27+
panic!("oh no, a panic!");
28+
}
29+
30+
#[endpoint {
31+
method = GET,
32+
path = "/ok",
33+
}]
34+
async fn handler_ok(
35+
_rqctx: RequestContext<()>,
36+
) -> Result<HttpResponseOk<u64>, HttpError> {
37+
Ok(HttpResponseOk(1))
38+
}
39+
40+
/// A panicking handler tears down the connection without sending a response;
41+
/// the panic is reported as a panic (not as a client disconnect), and the
42+
/// server remains usable for subsequent connections.
43+
#[tokio::test]
44+
async fn test_panic_reported_as_panic() {
45+
// Log to a file so that we can verify how the panic was reported.
46+
let log_path = std::env::temp_dir().join(format!(
47+
"test_panic_reported_as_panic.{}.log",
48+
std::process::id()
49+
));
50+
let config_logging = ConfigLogging::File {
51+
level: ConfigLoggingLevel::Info,
52+
path: log_path.clone().try_into().unwrap(),
53+
if_exists: ConfigLoggingIfExists::Truncate,
54+
};
55+
let log = config_logging.to_logger("panic_reported_as_panic").unwrap();
56+
57+
let server = ServerBuilder::new(api(), (), log.clone()).start().unwrap();
58+
59+
// Speak raw HTTP so that we can observe the aborted connection instead of
60+
// an HTTP response.
61+
let mut stream =
62+
tokio::net::TcpStream::connect(server.local_addr()).await.unwrap();
63+
stream
64+
.write_all(b"GET /panic HTTP/1.1\r\nhost: test\r\n\r\n")
65+
.await
66+
.unwrap();
67+
let mut buf = Vec::new();
68+
match stream.read_to_end(&mut buf).await {
69+
// Clean EOF: the connection must have been closed with no response.
70+
Ok(_) => assert!(
71+
buf.is_empty(),
72+
"expected no response bytes, got: {:?}",
73+
String::from_utf8_lossy(&buf)
74+
),
75+
// A connection reset is an equally acceptable way to observe the
76+
// aborted connection.
77+
Err(e) => {
78+
assert_eq!(e.kind(), std::io::ErrorKind::ConnectionReset)
79+
}
80+
}
81+
82+
// The server remains usable on a fresh connection.
83+
let client = ClientTestContext::new(server.local_addr(), log.clone());
84+
client
85+
.make_request_no_body(Method::GET, "/ok", StatusCode::OK)
86+
.await
87+
.expect("server should still be usable after a handler panic");
88+
89+
// Drop our references to the logger so the async drain flushes, then
90+
// check how the panic was logged: as a panic, not as a client disconnect.
91+
server.close().await.unwrap();
92+
drop(client);
93+
drop(log);
94+
let log_records = {
95+
let mut records = Vec::new();
96+
for _ in 0..100 {
97+
records = read_bunyan_log(&log_path);
98+
if records.iter().any(|r| r.msg == "request handling panicked") {
99+
break;
100+
}
101+
std::thread::sleep(std::time::Duration::from_millis(10));
102+
}
103+
records
104+
};
105+
assert!(
106+
log_records.iter().any(|r| r.msg == "request handling panicked"),
107+
"expected the panic to be logged as a panic"
108+
);
109+
assert!(
110+
!log_records.iter().any(|r| r.msg.contains("client disconnected")),
111+
"a handler panic must not be reported as a client disconnect"
112+
);
113+
std::fs::remove_file(&log_path).unwrap();
114+
}

0 commit comments

Comments
 (0)