mirror of
https://github.com/storytold/figcraft.git
synced 2026-11-14 19:20:56 +00:00
A request line can carry timeoutMs beside id and method, capped at 600,000 ms; without it the wait stays 60 s and older apps ignore the field. figcraft-cli mcp --attach PORT --timeout SECONDS sends it with every request and reads the answers that much longer. A timeout's message says how long the request waited.
This commit is contained in:
+2
-1
File diff suppressed because one or more lines are too long
@@ -209,6 +209,9 @@ pub struct McpArgs {
|
||||
/// Refuses the automation roots
|
||||
#[arg(long, value_name = "PORT", value_parser = attach, conflicts_with_all = ["automation_read_root", "automation_write_root"])]
|
||||
pub attach: Option<String>,
|
||||
/// How long the attached app may take over each request, in seconds (1–600; default 60)
|
||||
#[arg(long, value_name = "SECONDS", requires = "attach", value_parser = clap::value_parser!(u64).range(1..=600))]
|
||||
pub timeout: Option<u64>,
|
||||
#[command(flatten)]
|
||||
pub roots: RootArgs,
|
||||
}
|
||||
@@ -458,6 +461,8 @@ mod tests {
|
||||
assert_eq!(a.attach.as_deref(), Some("127.0.0.1:7979"));
|
||||
let Command::Mcp(a) = ok(&["mcp", "--attach=localhost:7979"]) else { panic!("mcp") };
|
||||
assert_eq!(a.attach.as_deref(), Some("localhost:7979"));
|
||||
let Command::Mcp(a) = ok(&["mcp", "--attach", "7979", "--timeout", "300"]) else { panic!("mcp") };
|
||||
assert_eq!(a.timeout, Some(300));
|
||||
let Command::Mcp(a) = ok(&["mcp", "--automation-read-root", "in", "--automation-write-root", "out"]) else { panic!("mcp") };
|
||||
assert_eq!((a.roots.automation_read_root, a.roots.automation_write_root), (vec!["in".to_owned()], vec!["out".to_owned()]));
|
||||
let Command::Plugin { command: PluginCommand::Publish(a) } = ok(&["plugin", "publish", "dist/counter", "--server", "ws://h:47900/"]) else {
|
||||
@@ -509,6 +514,9 @@ mod tests {
|
||||
(&["perf", "--layers", "10,x"], ValueValidation),
|
||||
(&["perf", "--layers", "10", "--layers", "20"], ArgumentConflict),
|
||||
(&["mcp", "--attach", "notaport"], ValueValidation),
|
||||
(&["mcp", "--attach", "7979", "--timeout", "0"], ValueValidation),
|
||||
(&["mcp", "--attach", "7979", "--timeout", "601"], ValueValidation),
|
||||
(&["mcp", "--timeout", "300"], MissingRequiredArgument),
|
||||
(&["mcp", "--attach", "7979", "--automation-read-root", "."], ArgumentConflict),
|
||||
(&["mcp", "--automation-write-root"], InvalidValue),
|
||||
(&["mcp", "stray"], UnknownArgument),
|
||||
|
||||
@@ -91,7 +91,7 @@ fn run(cli: Cli) -> CliResult<ExitCode> {
|
||||
Ok(ExitCode::SUCCESS)
|
||||
}
|
||||
// `--attach` conflicts with the roots: the app runs the commands, only its own roots can confine them.
|
||||
Command::Mcp(a) => mcp(a.attach.as_deref(), roots_of(&a.roots)?),
|
||||
Command::Mcp(a) => mcp(a.attach.as_deref(), a.timeout, roots_of(&a.roots)?),
|
||||
Command::Plugin { command: PluginCommand::Publish(a) } => publish_plugin(&a.folder, &a.server),
|
||||
Command::Connect { command: ConnectCommand::Publish(a) } => confined(&a.roots, || files::connect_publish(&a.file, &a.mappings, a.dry_run)),
|
||||
Command::Perf(a) => {
|
||||
@@ -148,12 +148,15 @@ fn publish_plugin(folder: &str, server: &str) -> CliResult<ExitCode> {
|
||||
Ok(ExitCode::SUCCESS)
|
||||
}
|
||||
|
||||
fn mcp(attach: Option<&str>, roots: Option<figcraft_engine::file_access::Roots>) -> CliResult<ExitCode> {
|
||||
fn mcp(attach: Option<&str>, timeout: Option<u64>, roots: Option<figcraft_engine::file_access::Roots>) -> CliResult<ExitCode> {
|
||||
let backend: Box<dyn figcraft_mcp::Backend> = match attach {
|
||||
Some(addr) => {
|
||||
let b = figcraft_mcp::Attached::connect(addr)
|
||||
.map_err(|e| format!("can't reach the FigCraft app at {addr}: {e} (start it with `figcraft --control PORT`)"))?;
|
||||
Box::new(b)
|
||||
let no_app = |e: std::io::Error| format!("can't reach the FigCraft app at {addr}: {e} (start it with `figcraft --control PORT`)");
|
||||
let b = figcraft_mcp::Attached::connect(addr).map_err(no_app)?;
|
||||
match timeout {
|
||||
Some(secs) => Box::new(b.with_timeout(std::time::Duration::from_secs(secs)).map_err(no_app)?),
|
||||
None => Box::new(b),
|
||||
}
|
||||
}
|
||||
None => Box::new(figcraft_mcp::Headless::new()),
|
||||
};
|
||||
|
||||
@@ -17,6 +17,8 @@ const MAX_LINE: usize = 32 * 1024 * 1024;
|
||||
const MAX_CONNECTIONS: usize = 8;
|
||||
const MAX_QUEUED_REQUESTS: usize = 8;
|
||||
const TIMEOUT: Duration = Duration::from_secs(60);
|
||||
/// The longest a request's `timeoutMs` can make it wait: a client can raise the wait, never lift it.
|
||||
const MAX_TIMEOUT: Duration = Duration::from_secs(600);
|
||||
/// How long the accepting thread waits after a failed accept. Accepting can keep failing, for
|
||||
/// example while the process has no file descriptors left, and retrying at once would keep a
|
||||
/// core busy until it recovers.
|
||||
@@ -55,20 +57,27 @@ fn after_timeout(rx: &std::sync::mpsc::Receiver<ControlReply>, gate: &RequestGat
|
||||
}
|
||||
}
|
||||
|
||||
/// What a request that got no reply is told. Only a request that needs a drawn frame can be held
|
||||
/// up by the window being out of sight; any other was either never started (the app was busy, so
|
||||
/// it was dropped and never applied) or is still running and will be applied (#1673).
|
||||
fn timeout_message(failure: &WaitFailure, needs_frame: bool) -> &'static str {
|
||||
/// How long to wait for the reply to request `v`: its `timeoutMs` (a whole number, at most
|
||||
/// [`MAX_TIMEOUT`]), else `default` (#1696).
|
||||
fn request_timeout(v: &Value, default: Duration) -> Duration {
|
||||
v.get("timeoutMs").and_then(Value::as_u64).map_or(default, |ms| Duration::from_millis(ms).min(MAX_TIMEOUT))
|
||||
}
|
||||
|
||||
/// What a request that got no reply after `waited` is told. Only a request that needs a drawn frame
|
||||
/// can be held up by the window being out of sight; any other was either never started (the app
|
||||
/// was busy, so it was dropped and never applied) or is still running and will be applied (#1673).
|
||||
fn timeout_message(failure: &WaitFailure, needs_frame: bool, waited: Duration) -> String {
|
||||
let secs = waited.as_secs_f64().ceil();
|
||||
match failure {
|
||||
WaitFailure::Disconnected => "the FigCraft window closed before answering",
|
||||
WaitFailure::StillRunning => {
|
||||
"the app is still running this request after 60 s: it will take effect when it finishes, but its reply is lost. Check the document before retrying"
|
||||
}
|
||||
WaitFailure::Disconnected => "the FigCraft window closed before answering".into(),
|
||||
WaitFailure::StillRunning => format!(
|
||||
"the app is still running this request after {secs} s: it will take effect when it finishes, but its reply is lost. Check the document before retrying"
|
||||
),
|
||||
WaitFailure::Timeout if needs_frame => {
|
||||
"timed out waiting for the UI: screenshots and input need the FigCraft window on screen (is it minimised or covered?)"
|
||||
"timed out waiting for the UI: screenshots and input need the FigCraft window on screen (is it minimised or covered?)".into()
|
||||
}
|
||||
WaitFailure::Timeout => {
|
||||
"timed out after 60 s before the app started this request (it was busy, or its window isn't drawing): it was not applied"
|
||||
format!("timed out after {secs} s before the app started this request (it was busy, or its window isn't drawing): it was not applied")
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -185,6 +194,7 @@ fn serve_with_events(mut out: TcpStream, queue: Queue, ctx: CtxSlot, events: Arc
|
||||
writeln!(out, "{reply}")?;
|
||||
continue;
|
||||
}
|
||||
let waits = request_timeout(&v, timeout);
|
||||
let (tx, rx) = std::sync::mpsc::channel();
|
||||
let gate = RequestGate::new();
|
||||
let mut pending = queue.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
@@ -200,12 +210,12 @@ fn serve_with_events(mut out: TcpStream, queue: Queue, ctx: CtxSlot, events: Arc
|
||||
if let Some(c) = ctx.lock().unwrap_or_else(std::sync::PoisonError::into_inner).as_ref() {
|
||||
c.request_repaint();
|
||||
}
|
||||
match wait_for_reply(&rx, &gate, timeout) {
|
||||
match wait_for_reply(&rx, &gate, waits) {
|
||||
Ok(r) => match r.result {
|
||||
Ok(res) => json!({"id": r.id, "ok": true, "result": res}),
|
||||
Err(e) => json!({"id": r.id, "ok": false, "error": e}),
|
||||
},
|
||||
Err(failure) => json!({"id": id, "ok": false, "error": timeout_message(&failure, needs_frame)}),
|
||||
Err(failure) => json!({"id": id, "ok": false, "error": timeout_message(&failure, needs_frame, waits)}),
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
|
||||
@@ -173,7 +173,7 @@ fn a_started_request_times_out_as_still_running() {
|
||||
let Err(failure) = wait_for_reply(&rx, &gate, Duration::ZERO) else { panic!("no reply was sent") };
|
||||
assert_eq!(failure, WaitFailure::StillRunning);
|
||||
assert_eq!(gate.state(), RequestState::Running, "the timeout doesn't cancel a running request");
|
||||
let message = timeout_message(&failure, false);
|
||||
let message = timeout_message(&failure, false, Duration::from_secs(60));
|
||||
assert!(message.contains("will take effect") && !message.contains("window"), "{message}");
|
||||
tx.send(ControlReply { id: json!(3), result: Ok(json!({"done": true})) }).unwrap();
|
||||
assert_eq!(wait_for_reply(&rx, &gate, Duration::ZERO).unwrap().id, json!(3), "a reply that made it is the truth");
|
||||
@@ -189,9 +189,64 @@ fn a_request_cancelled_before_it_started_never_runs() {
|
||||
assert_eq!(failure, WaitFailure::Timeout);
|
||||
assert!(!gate.start(), "the UI must not run a cancelled request");
|
||||
assert_eq!(gate.state(), RequestState::Cancelled);
|
||||
assert!(timeout_message(&failure, false).contains("not applied"));
|
||||
assert!(!timeout_message(&failure, false).contains("window on screen"));
|
||||
assert!(timeout_message(&failure, true).contains("window on screen"), "screenshots and input still need the window");
|
||||
let message = |needs_frame| timeout_message(&failure, needs_frame, Duration::from_secs(60));
|
||||
assert!(message(false).contains("not applied"));
|
||||
assert!(!message(false).contains("window on screen"));
|
||||
assert!(message(true).contains("window on screen"), "screenshots and input still need the window");
|
||||
}
|
||||
|
||||
/// #1696: a request's `timeoutMs` sets its wait, capped; anything but a whole number leaves the
|
||||
/// default.
|
||||
#[test]
|
||||
fn a_request_sets_its_own_timeout_within_the_cap() {
|
||||
let default = Duration::from_secs(60);
|
||||
let wait = |v: Value| request_timeout(&v, default);
|
||||
assert_eq!(wait(json!({"id": 1, "method": "engine.execute"})), default, "older clients keep the default");
|
||||
assert_eq!(wait(json!({"timeoutMs": 300_000})), Duration::from_secs(300));
|
||||
assert_eq!(wait(json!({"timeoutMs": 500})), Duration::from_millis(500), "a shorter wait too");
|
||||
assert_eq!(wait(json!({"timeoutMs": u64::MAX})), MAX_TIMEOUT, "never unbounded");
|
||||
for junk in [json!({"timeoutMs": "600000"}), json!({"timeoutMs": -1}), json!({"timeoutMs": 1.5}), json!({"timeoutMs": null})] {
|
||||
assert_eq!(wait(junk), default);
|
||||
}
|
||||
}
|
||||
|
||||
/// #1696: over a connection, a raised `timeoutMs` outlasts the connection's default.
|
||||
#[test]
|
||||
fn a_raised_timeout_outlasts_the_default() {
|
||||
let (mut client, server) = pair();
|
||||
let pending = queue();
|
||||
let q = pending.clone();
|
||||
// Written first: the connection's 50 ms is also how long it waits for a line.
|
||||
client.write_all(b"{\"id\":1,\"method\":\"ui.inspect\",\"timeoutMs\":5000}\n").unwrap();
|
||||
let worker = std::thread::spawn(move || serve(server, q, context(), Duration::from_millis(50)));
|
||||
let mut reader = BufReader::new(client);
|
||||
let start = Instant::now();
|
||||
let r = loop {
|
||||
if let Some(r) = pending.lock().unwrap().pop() {
|
||||
break r;
|
||||
}
|
||||
assert!(start.elapsed() < Duration::from_secs(5));
|
||||
std::thread::sleep(Duration::from_millis(1));
|
||||
};
|
||||
// Well past the connection's 50 ms: the request's own wait still holds.
|
||||
std::thread::sleep(Duration::from_millis(200));
|
||||
r.reply.send(ControlReply { id: r.id, result: Ok(json!("late")) }).unwrap();
|
||||
assert_eq!(reply(&mut reader), json!({"id": 1, "ok": true, "result": "late"}));
|
||||
drop(reader);
|
||||
worker.join().unwrap().ok();
|
||||
}
|
||||
|
||||
/// #1696: the timeout's message says how long the request waited, not always 60 s.
|
||||
#[test]
|
||||
fn a_timeout_names_the_wait_the_request_asked_for() {
|
||||
let (mut client, server) = pair();
|
||||
let worker = std::thread::spawn(move || serve(server, queue(), context(), Duration::from_secs(5)));
|
||||
client.write_all(b"{\"id\":2,\"method\":\"ui.inspect\",\"timeoutMs\":100}\n").unwrap();
|
||||
let mut reader = BufReader::new(client);
|
||||
let error = reply(&mut reader)["error"].as_str().unwrap().to_string();
|
||||
assert!(error.contains("after 1 s") && error.contains("not applied"), "{error}");
|
||||
reader.get_ref().shutdown(Shutdown::Write).unwrap();
|
||||
worker.join().unwrap().unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -87,16 +87,26 @@ pub struct Attached {
|
||||
next_id: u64,
|
||||
/// [`MAX_REPLY`]; smaller in tests.
|
||||
max_reply: usize,
|
||||
/// How long the app is asked to wait for each request; `None`: its own 60 s.
|
||||
timeout: Option<Duration>,
|
||||
}
|
||||
|
||||
impl Attached {
|
||||
/// Connect to `addr` (e.g. `127.0.0.1:7979`), failing fast when nothing is listening.
|
||||
pub fn connect(addr: &str) -> std::io::Result<Self> {
|
||||
let mut r = Self { addr: addr.to_string(), conn: None, next_id: 1, max_reply: MAX_REPLY };
|
||||
let mut r = Self { addr: addr.to_string(), conn: None, next_id: 1, max_reply: MAX_REPLY, timeout: None };
|
||||
r.reconnect()?;
|
||||
Ok(r)
|
||||
}
|
||||
|
||||
/// Ask the app to wait up to `timeout` for each request instead of its 60 s (it caps this; an
|
||||
/// app from before this ignores it and answers within its 60 s), and read its answers as long.
|
||||
pub fn with_timeout(mut self, timeout: Duration) -> std::io::Result<Self> {
|
||||
self.timeout = Some(timeout);
|
||||
self.reconnect()?;
|
||||
Ok(self)
|
||||
}
|
||||
|
||||
/// Connect to `127.0.0.1:port`.
|
||||
pub fn port(port: u16) -> std::io::Result<Self> {
|
||||
Self::connect(&format!("127.0.0.1:{port}"))
|
||||
@@ -113,8 +123,8 @@ impl Attached {
|
||||
match TcpStream::connect_timeout(&sa, Duration::from_millis(800)) {
|
||||
Ok(s) => {
|
||||
s.set_nodelay(true).ok();
|
||||
// The app answers within 60 s (its own timeout); leave headroom.
|
||||
s.set_read_timeout(Some(Duration::from_secs(90))).ok();
|
||||
// The app answers within its timeout; leave headroom.
|
||||
s.set_read_timeout(Some(self.timeout.unwrap_or(Duration::from_secs(60)).saturating_add(Duration::from_secs(30)))).ok();
|
||||
let read = s.try_clone()?;
|
||||
self.conn = Some((BufReader::new(read), s));
|
||||
return Ok(());
|
||||
@@ -165,7 +175,11 @@ impl Backend for Attached {
|
||||
fn call(&mut self, method: &str, params: Value) -> Result<Value, String> {
|
||||
let id = self.next_id;
|
||||
self.next_id += 1;
|
||||
let line = json!({"id": id, "method": method, "params": params}).to_string();
|
||||
let mut line = json!({"id": id, "method": method, "params": params});
|
||||
if let (Some(t), Some(fields)) = (self.timeout, line.as_object_mut()) {
|
||||
fields.insert("timeoutMs".into(), json!(u64::try_from(t.as_millis()).unwrap_or(u64::MAX)));
|
||||
}
|
||||
let line = line.to_string();
|
||||
if self.conn.is_none() || self.stale() {
|
||||
self.reconnect().map_err(|e| {
|
||||
format!("FigCraft app at {} is not reachable: {e} (request was not sent; start it with `figcraft --control PORT`)", self.addr)
|
||||
|
||||
@@ -43,6 +43,33 @@ fn ok_reply(text: &str) -> Vec<u8> {
|
||||
format!("{}\n", json!({"id": 1, "ok": true, "result": text})).into_bytes()
|
||||
}
|
||||
|
||||
/// #1696: a raised timeout goes with every request; without one the line is as it always was.
|
||||
#[test]
|
||||
fn a_raised_timeout_is_asked_for_on_each_request() {
|
||||
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
let port = listener.local_addr().unwrap().port();
|
||||
// Reads the requests of two calls; `with_timeout`'s dropped connection brings none.
|
||||
let app = std::thread::spawn(move || {
|
||||
let mut lines = Vec::new();
|
||||
while lines.len() < 2 {
|
||||
let (stream, _) = listener.accept().unwrap();
|
||||
let mut out = stream.try_clone().unwrap();
|
||||
let mut line = String::new();
|
||||
if BufReader::new(stream).read_line(&mut line).unwrap() > 0 {
|
||||
out.write_all(&ok_reply("fine")).unwrap();
|
||||
lines.push(serde_json::from_str::<serde_json::Value>(&line).unwrap());
|
||||
}
|
||||
}
|
||||
lines
|
||||
});
|
||||
Attached::port(port).unwrap().call("engine.commands", json!({})).unwrap();
|
||||
let mut long = Attached::port(port).unwrap().with_timeout(Duration::from_secs(300)).unwrap();
|
||||
long.call("engine.commands", json!({})).unwrap();
|
||||
let lines = app.join().unwrap();
|
||||
assert_eq!(lines[0].get("timeoutMs"), None, "{:?}", lines[0]);
|
||||
assert_eq!(lines[1]["timeoutMs"], 300_000, "{:?}", lines[1]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_oversized_reply_is_refused_and_the_next_call_reconnects() {
|
||||
let (port, done, fake) = fake_app(vec![vec![b'x'; 200], ok_reply("fine")]);
|
||||
|
||||
+2
-1
@@ -280,7 +280,8 @@ Serve MCP on stdio: headless by default, or attached to a running app started wi
|
||||
folders) confine the files the agent's calls read (inside a read or write root) and write (inside a
|
||||
write root); everything else is refused ([mcp.md › Confining file access](mcp.md#confining-file-access)).
|
||||
They apply to the headless session only, and that is why `--attach` refuses them: an attached server
|
||||
is confined by the app it attaches to, from the flags that app was started with. The headless
|
||||
is confined by the app it attaches to, from the flags that app was started with. With `--attach`,
|
||||
`--timeout SECONDS` (1–600, default 60) is how long the app may take over each request. The headless
|
||||
session can host, join and publish
|
||||
live sessions (`collab.*`: without a key on a server open to the public, with `collab.serverInfo`,
|
||||
`collab.manageFile` for the owner token and `collab.report`) and use plugin directories
|
||||
|
||||
@@ -21,8 +21,12 @@ real UI state. A request that the UI doesn't answer within 60 s gets a timeout e
|
||||
happened to it: one the app never started (it was busy, or its window wasn't drawing) was dropped
|
||||
and not applied; one it had started (a long `batch`, say) can't be stopped, so the error says it is
|
||||
still running and will take effect, and to check the document before retrying; only `ui.screenshot`
|
||||
and `ui.input`, which need a drawn frame, point at the window being minimised or covered. A line that is
|
||||
not JSON gets `{"id": null, "ok": false, "error": "bad JSON: …"}` and ends the connection (so an
|
||||
and `ui.input`, which need a drawn frame, point at the window being minimised or covered. A request
|
||||
that knows it will take longer (a big `batch`, say) can set its own wait with `timeoutMs` beside
|
||||
`id` and `method`, in whole milliseconds, up to 600 000 (10 minutes): `{"id": 1, "method":
|
||||
"engine.execute", "params": {...}, "timeoutMs": 300000}`. Anything else is ignored and the wait
|
||||
stays 60 s, as it is in an app from before this. A screenshot keeps its own 60 s deadline. A line
|
||||
that is not JSON gets `{"id": null, "ok": false, "error": "bad JSON: …"}` and ends the connection (so an
|
||||
HTTP request a web page makes the browser send here never runs). The implementation is in
|
||||
`crates/ui-egui/src/control.rs` (methods) and `apps/figcraft/src/control_server.rs` (socket).
|
||||
|
||||
|
||||
+3
-1
@@ -17,7 +17,9 @@ The server lives in `crates/mcp` (`figcraft-mcp`) and is started by the CLI:
|
||||
| `figcraft-cli mcp` | **Headless**: an in-process engine session with one untitled document. No window; renders with the CPU renderer. |
|
||||
| `figcraft-cli mcp --attach <port>` | **Attached**: drives a running FigCraft app through its control channel (`figcraft --control <port>`). The user sees every change live. |
|
||||
|
||||
`--attach` also takes `host:port`. The control channel only listens on 127.0.0.1.
|
||||
`--attach` also takes `host:port`. The control channel only listens on 127.0.0.1. The app takes up
|
||||
to 60 s over each request; `--timeout SECONDS` (1–600) raises that for a client that sends big
|
||||
batches: `figcraft-cli mcp --attach 7979 --timeout 300`. Headless has no such limit.
|
||||
|
||||
While an attached agent works, the app shows where, as it shows a person in a live session: its
|
||||
avatar, in a colour of its own and named as its comments are signed ("Claude (agent)", see
|
||||
|
||||
+2
-1
File diff suppressed because one or more lines are too long
Reference in New Issue
Block a user