Skip to content

Commit 604115c

Browse files
fix(jobs): treat a webhook reset after a full response as delivered (#259)
`read_status` failed the delivery on any read error, so a server that sent its complete HTTP response and then closed abruptly — e.g. a Windows peer resetting the socket (os error 10053) right after `write_all` — surfaced as "failed to read webhook response" even though the status had already arrived. This flaked `webhook_http_post_uses_validated_socket_addr` on Windows CI. Parse whatever arrived when a read errors: if a terminated status line is already present the delivery succeeded, so return it; only propagate the error when no status was read. `parse_status_line` requires a line terminator so a truncated first line is never misread as a status. The test server now half-closes (`shutdown(Write)`) and drains before drop so the client reads the full response before the socket closes. Adds unit coverage for both the reset-after-response and reset-with-no- status paths. Co-authored-by: Claude <noreply@anthropic.com>
1 parent df3039e commit 604115c

1 file changed

Lines changed: 87 additions & 13 deletions

File tree

src/automation/job_webhook.rs

Lines changed: 87 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -249,26 +249,48 @@ fn read_status<R: Read>(reader: &mut R) -> Result<u16> {
249249
let mut bytes = Vec::new();
250250
let mut buf = [0_u8; 512];
251251
loop {
252-
let n = reader.read(&mut buf).map_err(|e| TraceDecayError::Config {
253-
message: format!("failed to read webhook response: {e}"),
254-
})?;
255-
if n == 0 {
256-
break;
257-
}
258-
bytes.extend_from_slice(&buf[..n]);
259-
if bytes.windows(4).any(|window| window == b"\r\n\r\n") || bytes.len() > 8192 {
260-
break;
252+
match reader.read(&mut buf) {
253+
Ok(0) => break,
254+
Ok(n) => {
255+
bytes.extend_from_slice(&buf[..n]);
256+
if bytes.windows(4).any(|window| window == b"\r\n\r\n") || bytes.len() > 8192 {
257+
break;
258+
}
259+
}
260+
// A server that sends its full response and then closes abruptly can
261+
// surface the close as a read error before we observe the end of the
262+
// headers — e.g. a Windows peer resets the connection (os error
263+
// 10053) right after `write_all`. If a complete status line already
264+
// arrived the delivery succeeded, so parse what we have rather than
265+
// discarding it; only propagate the error when no status was read.
266+
Err(e) => {
267+
if parse_status_line(&bytes).is_some() {
268+
break;
269+
}
270+
return Err(TraceDecayError::Config {
271+
message: format!("failed to read webhook response: {e}"),
272+
});
273+
}
261274
}
262275
}
263-
let header = String::from_utf8_lossy(&bytes);
276+
parse_status_line(&bytes).ok_or_else(|| TraceDecayError::Config {
277+
message: "webhook response did not include an HTTP status".to_string(),
278+
})
279+
}
280+
281+
/// Parses the numeric HTTP status from a (possibly partial) response head.
282+
/// Returns `None` until a terminated status line has arrived so a truncated
283+
/// first line is never mistaken for a status code.
284+
fn parse_status_line(bytes: &[u8]) -> Option<u16> {
285+
if !bytes.contains(&b'\n') {
286+
return None;
287+
}
288+
let header = String::from_utf8_lossy(bytes);
264289
header
265290
.lines()
266291
.next()
267292
.and_then(|line| line.split_whitespace().nth(1))
268293
.and_then(|status| status.parse::<u16>().ok())
269-
.ok_or_else(|| TraceDecayError::Config {
270-
message: "webhook response did not include an HTTP status".to_string(),
271-
})
272294
}
273295

274296
fn request_target(url: &Url) -> String {
@@ -329,6 +351,50 @@ mod tests {
329351
use std::sync::mpsc;
330352
use std::thread;
331353

354+
/// A reader that yields a canned buffer once, then fails every subsequent
355+
/// read with `ConnectionAborted` — models a peer that sends its full
356+
/// response and then resets the socket (Windows os error 10053).
357+
struct ResetAfterResponse {
358+
response: Vec<u8>,
359+
sent: bool,
360+
}
361+
362+
impl Read for ResetAfterResponse {
363+
fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
364+
if self.sent {
365+
return Err(std::io::Error::new(
366+
std::io::ErrorKind::ConnectionAborted,
367+
"connection aborted",
368+
));
369+
}
370+
self.sent = true;
371+
let n = self.response.len().min(buf.len());
372+
buf[..n].copy_from_slice(&self.response[..n]);
373+
Ok(n)
374+
}
375+
}
376+
377+
#[test]
378+
fn read_status_accepts_a_complete_response_before_a_connection_reset() {
379+
let mut reader = ResetAfterResponse {
380+
response: b"HTTP/1.1 202 Accepted\r\nContent-Length: 0\r\n".to_vec(),
381+
sent: false,
382+
};
383+
// The trailing `\r\n\r\n` never arrives before the reset, so the loop
384+
// hits the read error — but a full status line was already received.
385+
assert_eq!(read_status(&mut reader).unwrap(), 202);
386+
}
387+
388+
#[test]
389+
fn read_status_propagates_a_reset_with_no_status_line() {
390+
// `sent: true` makes the very first read fail, so no bytes arrive.
391+
let mut reader = ResetAfterResponse {
392+
response: Vec::new(),
393+
sent: true,
394+
};
395+
assert!(read_status(&mut reader).is_err());
396+
}
397+
332398
#[test]
333399
fn ipv6_embedded_ipv4_targets_are_blocked() -> std::result::Result<(), std::net::AddrParseError>
334400
{
@@ -374,6 +440,14 @@ mod tests {
374440
}
375441
let _ = tx.send((peer, String::from_utf8_lossy(&request).to_string()));
376442
let _ = stream.write_all(b"HTTP/1.1 202 Accepted\r\nContent-Length: 0\r\n\r\n");
443+
let _ = stream.flush();
444+
// Close gracefully: signal end-of-response with a half-close and let
445+
// the client read the full response before the socket is dropped. A
446+
// bare drop can send an RST on some platforms (Windows os error
447+
// 10053), aborting the client mid-read.
448+
let _ = stream.shutdown(std::net::Shutdown::Write);
449+
let mut drain = [0_u8; 64];
450+
while stream.read(&mut drain).map(|n| n > 0).unwrap_or(false) {}
377451
});
378452

379453
let url = Url::parse("http://webhook.example.test/hook?token=abc")?;

0 commit comments

Comments
 (0)