MCP: lost replies report uncertainty instead of replaying edits
Drop a failed control connection and reconnect for the next request, never resend an operation that may already have completed. A loopback regression applies a rectangle then drops its reply. It reproduces two edits before the fix and verifies one edit, an error, and successful inspection afterward.
This commit is contained in:
parent
650c5a00a6
commit
44f3141c3d
|
|
@ -78,17 +78,10 @@ impl Backend for Remote {
|
|||
let id = self.next_id;
|
||||
self.next_id += 1;
|
||||
let line = json!({"id": id, "method": method, "params": params}).to_string();
|
||||
// One retry with a fresh connection (the app may have restarted).
|
||||
let reply = match self.roundtrip(&line) {
|
||||
Ok(r) => r,
|
||||
Err(_) => {
|
||||
self.conn = None;
|
||||
self.roundtrip(&line).map_err(|e| {
|
||||
self.conn = None;
|
||||
format!("VectorCraft app at {} is not reachable: {e}", self.addr)
|
||||
})?
|
||||
}
|
||||
};
|
||||
let reply = self.roundtrip(&line).map_err(|e| {
|
||||
self.conn = None;
|
||||
format!("VectorCraft app at {}: `{method}` failed: {e}; operation may have completed; inspect state before retrying", self.addr)
|
||||
})?;
|
||||
let v: Value = serde_json::from_str(reply.trim()).map_err(|e| format!("bad reply from app: {e}"))?;
|
||||
if v.get("ok").and_then(Value::as_bool) == Some(true) {
|
||||
Ok(v.get("result").cloned().unwrap_or(Value::Null))
|
||||
|
|
|
|||
|
|
@ -430,6 +430,61 @@ fn remote_forwards_methods() {
|
|||
assert_eq!(seen[1]["params"], json!({"command": "object.group", "params": {}}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remote_does_not_replay_when_edit_reply_is_lost() {
|
||||
let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
listener.set_nonblocking(true).unwrap();
|
||||
let addr = listener.local_addr().unwrap().to_string();
|
||||
let h = std::thread::spawn(move || {
|
||||
let timeout = std::time::Duration::from_secs(5);
|
||||
let mut backend = Headless::with_document();
|
||||
let base = backend.call("document.inspect", json!({})).unwrap()["objects"].as_u64().unwrap();
|
||||
for connection in 0..2 {
|
||||
let deadline = std::time::Instant::now() + timeout;
|
||||
let stream = loop {
|
||||
match listener.accept() {
|
||||
Ok((stream, _)) => break stream,
|
||||
Err(e) if e.kind() == std::io::ErrorKind::WouldBlock && std::time::Instant::now() < deadline => std::thread::yield_now(),
|
||||
Err(e) => panic!("accept failed: {e}"),
|
||||
}
|
||||
};
|
||||
stream.set_nonblocking(false).unwrap();
|
||||
stream.set_read_timeout(Some(timeout)).unwrap();
|
||||
stream.set_write_timeout(Some(timeout)).unwrap();
|
||||
let mut out = stream.try_clone().unwrap();
|
||||
let mut reader = BufReader::new(stream);
|
||||
for _ in 0..2 {
|
||||
let mut line = String::new();
|
||||
reader.read_line(&mut line).unwrap();
|
||||
let msg: Value = serde_json::from_str(&line).unwrap();
|
||||
let method = msg["method"].as_str().unwrap();
|
||||
let result = backend.call(method, msg["params"].clone()).unwrap();
|
||||
if connection == 0 {
|
||||
assert_eq!(method, "engine.execute");
|
||||
break;
|
||||
}
|
||||
let reply = json!({"id": msg["id"], "ok": true, "result": result});
|
||||
writeln!(out, "{reply}").unwrap();
|
||||
if method == "document.inspect" {
|
||||
return result["objects"].as_u64().unwrap() - base;
|
||||
}
|
||||
assert_eq!(method, "engine.execute");
|
||||
}
|
||||
}
|
||||
panic!("document.inspect was not received");
|
||||
});
|
||||
|
||||
let mut remote = Remote::connect(&addr).unwrap();
|
||||
let first = remote.call("engine.execute", json!({"command": "shape.rectangle", "params": {"x": 10, "y": 10, "width": 20, "height": 20}}));
|
||||
let next = remote.call("document.inspect", json!({}));
|
||||
let edits = h.join().unwrap();
|
||||
|
||||
assert_eq!(edits, 1, "a lost reply must not duplicate the edit");
|
||||
let error = first.expect_err("the lost reply must report uncertainty");
|
||||
assert!(error.contains("operation may have completed"), "{error}");
|
||||
assert!(next.is_ok(), "the next call must reconnect: {next:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remote_connect_fails_fast() {
|
||||
// A port this test holds, bound but not listening, so connecting to it is refused. (A port
|
||||
|
|
|
|||
Loading…
Reference in New Issue