Review M2a: accept with two follow-up tasks; record models and lessons
The branch passes every check, including make verify-device on straylight and repeated timing runs under load. Reading and probing found that inferproxy does not pass an upstream close on to a client that is still sending, and that the chunked body reader delivers a stream only when the caller's buffer fills or the stream ends. Both were also gaps in the tasks and tests, so tasks 14 and 15 carry the fixes with new tests checked against the reference. The Model column is corrected: tasks 04 to 06 and 08 to 13 were Ornith. Lessons gain four implementer tips and five task-writing tips; three rules are promoted to AGENTS.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
@@ -154,3 +154,44 @@ fn an_unreachable_upstream_closes_the_client() {
|
||||
let path = start("127.0.0.1:1".to_string(), Limits::default());
|
||||
assert!(was_refused(&path));
|
||||
}
|
||||
|
||||
/// An upstream that answers at once and closes, whether or not the client has finished sending.
|
||||
/// This is what `llama-server` does, and what a server that dies mid-answer looks like.
|
||||
fn answer_and_close_upstream() -> String {
|
||||
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
let addr = listener.local_addr().unwrap().to_string();
|
||||
thread::spawn(move || {
|
||||
for stream in listener.incoming() {
|
||||
let mut stream = stream.unwrap();
|
||||
let mut first = [0u8; 1];
|
||||
let _ = stream.read(&mut first);
|
||||
stream.write_all(b"answer").unwrap();
|
||||
// dropping `stream` closes it
|
||||
}
|
||||
});
|
||||
addr
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_upstreams_close_reaches_a_client_that_is_still_sending() {
|
||||
// The HTTP client in loopd never half-closes: it keeps its sending side open until it drops
|
||||
// the connection. When the server closes, the proxy must close towards the client at once,
|
||||
// or the client only learns of a dead server from its own timeout.
|
||||
let path = start(answer_and_close_upstream(), Limits::default());
|
||||
let mut s = UnixStream::connect(&path).unwrap();
|
||||
s.set_read_timeout(Some(Duration::from_secs(2))).unwrap();
|
||||
s.write_all(b"request").unwrap();
|
||||
let started = std::time::Instant::now();
|
||||
let mut got = Vec::new();
|
||||
let result = s.read_to_end(&mut got);
|
||||
assert!(
|
||||
result.is_ok(),
|
||||
"the read must end with EOF, not a timeout: {result:?}"
|
||||
);
|
||||
assert_eq!(got, b"answer");
|
||||
assert!(
|
||||
started.elapsed() < Duration::from_millis(1000),
|
||||
"EOF took {:?}",
|
||||
started.elapsed()
|
||||
);
|
||||
}
|
||||
|
||||
@@ -308,3 +308,46 @@ fn a_missing_socket_is_a_connect_error() {
|
||||
let e: Box<dyn std::error::Error> = Box::new(HttpError::Timeout);
|
||||
assert!(!e.to_string().is_empty());
|
||||
}
|
||||
|
||||
/// A streamed body is read while it arrives. A `read` that already has data must not block
|
||||
/// waiting for more, or every event is delivered one buffer late and the last ones only when
|
||||
/// the stream ends.
|
||||
#[test]
|
||||
fn streamed_data_is_delivered_as_it_arrives() {
|
||||
let server = FakeServer::start();
|
||||
let reply = Reply::fixture("turn1");
|
||||
let piece = reply.offset_after_events(1);
|
||||
// About one event per piece, 300 ms apart: the whole stream takes over a second.
|
||||
server.route("/v1/chat/completions", vec![reply.trickle(piece, 300)]);
|
||||
let mut conn = Connection::open(&server.socket).unwrap();
|
||||
conn.set_read_timeout(Duration::from_secs(5)).unwrap();
|
||||
conn.send(&Request {
|
||||
method: "POST",
|
||||
path: "/v1/chat/completions",
|
||||
body: Some(b"{}"),
|
||||
})
|
||||
.unwrap();
|
||||
let head = conn.read_head().unwrap();
|
||||
let started = std::time::Instant::now();
|
||||
let mut body = conn.body(&head).unwrap();
|
||||
let mut arrivals = Vec::new();
|
||||
let mut buf = [0u8; 8192];
|
||||
loop {
|
||||
let n = std::io::Read::read(&mut body, &mut buf).unwrap();
|
||||
if n == 0 {
|
||||
break;
|
||||
}
|
||||
arrivals.push(started.elapsed());
|
||||
}
|
||||
let first = arrivals.first().copied().unwrap_or_default();
|
||||
let last = arrivals.last().copied().unwrap_or_default();
|
||||
assert!(
|
||||
first < Duration::from_millis(200),
|
||||
"the first data came only after {first:?}"
|
||||
);
|
||||
assert!(
|
||||
last - first >= Duration::from_millis(600),
|
||||
"everything arrived within {:?} of the first read: the reader is buffering the stream",
|
||||
last - first
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user