M8: a control request that times out says whether it was applied (#1673) (#1692)

The UI starts a request through its gate (a new Running state) just before running it and never runs one the transport already cancelled. At the timeout the reply says it was dropped and not applied, or that it is still running and will take effect; a reply that just made it is delivered. Only screenshots and input blame the window.

apps/figcraft/src/control_server.rs

@@ -28,6 +28,9 @@ enum WaitFailure {
     Timeout,
     /// The UI side dropped its sender before replying (the app is closing).
     Disconnected,
+    /// The UI started the request before the timeout and is still running it: it takes effect
+    /// when it finishes, though nobody waits for its reply (#1673).
+    StillRunning,
 }
 
 /// Wait for the UI's reply, and settle a timeout under the same gate the UI's final commit uses.
@@ -46,10 +49,30 @@ fn wait_for_reply(rx: &std::sync::mpsc::Receiver<ControlReply>, gate: &RequestGa
 fn after_timeout(rx: &std::sync::mpsc::Receiver<ControlReply>, gate: &RequestGate) -> Result<ControlReply, WaitFailure> {
     match gate.cancel_if_open() {
         RequestState::Replied => rx.try_recv().map_err(|_| WaitFailure::Disconnected),
+        // It may have finished just now: its reply is the truth then.
+        RequestState::Running => rx.try_recv().map_err(|_| WaitFailure::StillRunning),
         RequestState::Open | RequestState::Cancelled => Err(WaitFailure::Timeout),
     }
 }
 
+/// 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 {
+    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::Timeout if needs_frame => {
+            "timed out waiting for the UI: screenshots and input need the FigCraft window on screen (is it minimised or covered?)"
+        }
+        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"
+        }
+    }
+}
+
 /// Deferred requests for a hidden window must not make the UI's queue grow without a bound.
 pub fn drain_queue(queue: &Queue, target: &mut Vec<ControlRequest>) {
     target.retain(|request| !request.is_cancelled());
@@ -171,6 +194,7 @@ fn serve_with_events(mut out: TcpStream, queue: Queue, ctx: CtxSlot, events: Arc
                     writeln!(out, "{}", json!({"id": id, "ok": false, "error": "control request queue is full"}))?;
                     continue;
                 }
+                let needs_frame = matches!(method.as_str(), "ui.screenshot" | "ui.input");
                 pending.push(ControlRequest { id: id.clone(), method, params, reply: tx, state: gate.clone() });
                 drop(pending);
                 if let Some(c) = ctx.lock().unwrap_or_else(std::sync::PoisonError::into_inner).as_ref() {
@@ -181,12 +205,7 @@ fn serve_with_events(mut out: TcpStream, queue: Queue, ctx: CtxSlot, events: Arc
                         Ok(res) => json!({"id": r.id, "ok": true, "result": res}),
                         Err(e) => json!({"id": r.id, "ok": false, "error": e}),
                     },
-                    Err(WaitFailure::Timeout) => {
-                        json!({"id": id, "ok": false, "error": "timed out waiting for the UI: screenshots and input need the FigCraft window on screen (is it minimised or covered?)"})
-                    }
-                    Err(WaitFailure::Disconnected) => {
-                        json!({"id": id, "ok": false, "error": "the FigCraft window closed before answering"})
-                    }
+                    Err(failure) => json!({"id": id, "ok": false, "error": timeout_message(&failure, needs_frame)}),
                 }
             }
             Err(e) => {

apps/figcraft/src/control_server_tests.rs

@@ -162,6 +162,38 @@ fn a_disconnected_reply_channel_is_not_reported_as_a_timeout() {
     assert_eq!(gate.state(), RequestState::Open, "a closed app is not a cancel decision");
 }
 
+/// #1673: a request the UI already started can't be cancelled any more. Its timeout says it is
+/// still running and will take effect, not that the window is hidden; one that finished just as
+/// the timeout fired gets its real reply.
+#[test]
+fn a_started_request_times_out_as_still_running() {
+    let gate = RequestGate::new();
+    assert!(gate.start(), "an open request starts");
+    let (tx, rx) = mpsc::channel::<ControlReply>();
+    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);
+    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");
+}
+
+/// #1673: a request the transport gave up on before the UI started it never starts, and its
+/// timeout says it wasn't applied; only requests that need a drawn frame blame the window.
+#[test]
+fn a_request_cancelled_before_it_started_never_runs() {
+    let gate = RequestGate::new();
+    let (_tx, rx) = mpsc::channel::<ControlReply>();
+    let Err(failure) = wait_for_reply(&rx, &gate, Duration::ZERO) else { panic!("no reply was sent") };
+    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");
+}
+
 #[test]
 fn idle_and_partial_frame_connections_expire() {
     for bytes in [b"".as_slice(), b"{\"id\":"] {

crates/ui-egui/src/control.rs

@@ -48,8 +48,8 @@ pub struct ControlReply {
 /// Which side has finished a control request.
 ///
 /// The transport and the UI race to settle one request: while it is `Open`, either may cancel it
-/// (the transport's timeout) or commit it (the UI's final reply). Once settled, neither side may
-/// change it.
+/// (the transport's timeout) or commit it (the UI's final reply) or, for a request the UI answers
+/// at once, start it (`Running`). Once settled or started, the transport can't cancel it.
 #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
 pub enum RequestState {
     #[default]
@@ -58,6 +58,9 @@ pub enum RequestState {
     Cancelled,
     /// The UI wrote the file (if any) and queued the reply before releasing the gate.
     Replied,
+    /// The UI started a request it answers at once (an edit, an inspection); the transport can no
+    /// longer cancel it, so a timeout now means "still running, it will take effect" (#1673).
+    Running,
 }
 
 /// The gate the transport's timeout and the UI's final reply share for one request.
@@ -102,6 +105,17 @@ impl RequestGate {
         *state
     }
 
+    /// Start a request the UI answers at once, unless the transport already gave up on it: then
+    /// it must not run at all. `false` when it was cancelled (or already settled).
+    pub fn start(&self) -> bool {
+        let mut state = self.state.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
+        if *state != RequestState::Open {
+            return false;
+        }
+        *state = RequestState::Running;
+        true
+    }
+
     /// Run the final commit (file write and reply enqueue) only while the request is still open,
     /// then mark it replied before releasing the gate. `None` means the transport cancelled it or
     /// it was already committed; the closure must not have run then.
@@ -131,6 +145,13 @@ impl ControlRequest {
     pub fn is_cancelled(&self) -> bool {
         self.state.is_cancelled()
     }
+
+    /// Mark the request started just before the UI runs it; `false` when the transport timed out
+    /// first, and then it must not run. `ui.input` is acknowledged by a later frame, so it stays
+    /// open (cancellable) like a screenshot.
+    fn begin(&self) -> bool {
+        self.method == "ui.input" || self.state.start()
+    }
 }
 
 /// One screenshot's completion slot.
@@ -519,6 +540,9 @@ pub fn background(app: &mut FigApp, ctx: &Context) {
             let _ = r.reply.send(ControlReply { id: r.id, result: Err(error) });
             continue;
         }
+        if !r.begin() {
+            continue;
+        }
         // Outside a frame, so outside the frame's guard: within the window's swap, a panic still
         // ends in recovery copies and leaves every window its own state.
         let res = crate::windows::as_window(app, target, |app| {
@@ -719,6 +743,9 @@ fn route_queued(app: &mut FigApp, ctx: &Context, viewport: egui::ViewportId, now
         admit(app, r, target, now);
         return;
     }
+    if !r.begin() {
+        return;
+    }
     // Every control-channel gesture is fenced from the user's system clipboard (the flag is
     // restored so a batch already in a frame's raw input keeps its own provenance).
     let before = app.control_synthetic;

crates/ui-egui/src/idle_tests.rs

@@ -344,6 +344,47 @@ fn cjk_names_are_looked_for_in_edits_not_on_every_pan() {
     assert_eq!(h.app.cjk_needed(&ctx, 0), crate::cjk::HANGUL);
 }
 
+/// #1673: the UI runs a queued request only if the transport hasn't given up on it, and marks it
+/// started first, so a timeout can tell "not applied" from "still running". Both from a frame and
+/// from a hidden window's logic pass.
+#[test]
+fn a_request_the_transport_gave_up_on_is_never_applied() {
+    let (mut h, text) = document();
+    h.settle();
+    let (tx, rx) = std::sync::mpsc::channel();
+    let rename = |id: u64, name: &str| ControlRequest {
+        id: json!(id),
+        method: "engine.execute".into(),
+        params: json!({"command": "node.set", "params": {"ids": [text.clone()], "props": {"name": name}}}),
+        reply: tx.clone(),
+        state: Default::default(),
+    };
+    let node = figcraft_doc::NodeId(text.as_u64().unwrap());
+    let name = |h: &Harness| h.app.session.doc().unwrap().node(node).unwrap().name.clone();
+    let before = name(&h);
+    let mut hidden = RawInput::default();
+    hidden.viewports.insert(ViewportId::ROOT, egui::ViewportInfo { minimized: Some(true), ..Default::default() });
+    for on_screen in [true, false] {
+        let gave_up = rename(1, "late");
+        gave_up.state.cancel_if_open();
+        let wanted = rename(2, "wanted");
+        let gate = wanted.state.clone();
+        h.app.control_queue.extend([gave_up, wanted]);
+        if on_screen {
+            h.frame(vec![]);
+        } else {
+            let ctx = h.ctx.clone();
+            let _ = ctx.run_logic(&hidden, |ctx| crate::control::background(&mut h.app, ctx));
+        }
+        let reply = rx.try_recv().expect("the wanted request is answered");
+        assert_eq!(reply.id, json!(2), "on screen: {on_screen}");
+        assert!(rx.try_recv().is_err(), "nothing answers the cancelled one");
+        assert_eq!(gate.state(), crate::RequestState::Running, "started before it ran");
+        assert_eq!(name(&h), "wanted");
+        h.app.session.execute("node.set", &json!({"ids": [text.clone()], "props": {"name": before.clone()}})).unwrap();
+    }
+}
+
 /// Asserts a hidden window answers edits and inspection at once, while a screenshot is
 /// parked with a deadline and times out once instead of waiting for a frame.
 #[test]

docs/control-protocol.md

@@ -17,7 +17,11 @@ reply:    {"id": 1, "ok": true, "result": ...}
 ```
 
 Requests are queued and run on the UI thread at the start of the next frame, so replies reflect the
-real UI state. A request that the UI doesn't answer within 60 s gets a timeout error. A line that is
+real UI state. A request that the UI doesn't answer within 60 s gets a timeout error that says what
+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
 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).
@@ -35,8 +39,8 @@ before the newline** and drops the connection on a longer one (see [MCP server](
 Both the socket queue and the UI queue hold at most **8 requests** each, including requests waiting
 for another window's frame. A full socket queue replies with `"control request queue is full"`
 instead of dispatching the new request. These limits apply to the opt-in loopback control channel;
-requests still queued after the UI timeout are discarded, including requests deferred to a hidden
-window. A command already running is not interrupted.
+requests still queued after the UI timeout are discarded (never run), including requests deferred to
+a hidden window. A command already running is not interrupted: it takes effect when it finishes.
 
 A minimised or covered window (the main one or another, see Windows below) draws no frames, but
 still answers every method that acts on the app's state (`engine.execute`, `document.inspect`,