Control channel: a request its sender gave up on is not run later

A request that waited more than 60 s was answered with a timeout and left in
the queue, so `control::poll` ran it on the next UI pass and dropped the reply.
The client was told its command had failed while the command was applied: an
agent that retried after the error ran it twice, and one that gave up left an
unexpected change in the workbook.

`ControlRequest` now carries who has it -- pending, taken by the app, or
abandoned by its sender -- as the report suggests. `poll` claims each request
and skips one it cannot claim; the connection thread abandons the request when
its reply times out, and says which happened:

  still queued     timed out waiting for the UI (is the window minimised?)
  already started  timed out waiting for the UI, which had already started the
                   request, so it may still finish; its outcome is unknown

The second case cannot be called a failure without guessing. This is the shape
DeckCraft settled on for the same bug (its #719), so the two apps now agree.

Fix: #1914

apps/gridcraft/src/control_server.rs

@@ -119,14 +119,23 @@ fn serve(stream: TcpStream, tx: Sender<ControlRequest>, ctx: egui::Context) {
                 Line::Reject(e) => e,
                 Line::Request { id, method, params } => {
                     let (req, rx) = ControlRequest::new(method, params);
+                    let state = std::sync::Arc::clone(&req.state);
                     if tx.send(req).is_err() {
                         break;
                     }
                     // Wake the UI thread: egui only runs frames when something happens.
                     ctx.request_repaint();
-                    let mut reply = rx
-                        .recv_timeout(Duration::from_secs(60))
-                        .unwrap_or_else(|_| json!({"ok": false, "error": "timed out waiting for the UI (is the window minimised?)"}));
+                    let mut reply = rx.recv_timeout(Duration::from_secs(60)).unwrap_or_else(|_| {
+                        // Give up on the request as well as on its reply. While it is still
+                        // queued this keeps it from running late -- the client was told it had
+                        // failed, and an agent that retried after the error ran it twice (#1914);
+                        // if the app had already started it, its outcome is unknown instead.
+                        if ControlRequest::abandon(&state) {
+                            json!({"ok": false, "error": "timed out waiting for the UI (is the window minimised?)"})
+                        } else {
+                            json!({"ok": false, "error": "timed out waiting for the UI, which had already started the request, so it may still finish; its outcome is unknown"})
+                        }
+                    });
                     if let Some(o) = reply.as_object_mut() {
                         o.insert("id".into(), id);
                     }

crates/ui-egui/src/control.rs

@@ -29,12 +29,35 @@ pub struct ControlRequest {
     pub method: String,
     pub params: Json,
     pub reply: Sender<ControlResponse>,
+    /// Who has the request: [`PENDING`], [`TAKEN`] (the app runs it) or [`ABANDONED`] (the sender
+    /// gave up waiting, so the app must not run it late: a client told its request timed out had
+    /// it applied anyway on the next UI pass, and an agent that retried ran it twice (#1914).
+    pub state: std::sync::Arc<std::sync::atomic::AtomicU8>,
 }
 
+/// Queued, not yet run.
+pub const PENDING: u8 = 0;
+/// The app has started it: it will answer.
+pub const TAKEN: u8 = 1;
+/// Its sender gave up waiting (the reply timeout expired): it is dropped unrun.
+pub const ABANDONED: u8 = 2;
+
 impl ControlRequest {
     pub fn new(method: impl Into<String>, params: Json) -> (Self, std::sync::mpsc::Receiver<ControlResponse>) {
         let (tx, rx) = std::sync::mpsc::channel();
-        (Self { method: method.into(), params, reply: tx }, rx)
+        (Self { method: method.into(), params, reply: tx, state: Default::default() }, rx)
+    }
+
+    /// Claim the request for running; false when its sender already gave up on it.
+    pub fn take(&self) -> bool {
+        use std::sync::atomic::Ordering;
+        self.state.compare_exchange(PENDING, TAKEN, Ordering::SeqCst, Ordering::SeqCst).is_ok() || self.state.load(Ordering::SeqCst) == TAKEN
+    }
+
+    /// Give up on the request; false when the app has already started it (it will answer).
+    pub fn abandon(state: &std::sync::atomic::AtomicU8) -> bool {
+        use std::sync::atomic::Ordering;
+        state.compare_exchange(PENDING, ABANDONED, Ordering::SeqCst, Ordering::SeqCst).is_ok() || state.load(Ordering::SeqCst) == ABANDONED
     }
 }
 
@@ -391,6 +414,11 @@ pub fn handle(app: &mut SheetApp, ctx: &egui::Context, method: &str, p: &Json) -
 pub fn poll(app: &mut SheetApp, ctx: &egui::Context) {
     let Some(rx) = app.control_rx.take() else { return };
     while let Ok(req) = rx.try_recv() {
+        // A request whose sender stopped waiting is dropped unrun: running it would apply a
+        // command the client was told had failed, and its reply would go nowhere (#1914).
+        if !req.take() {
+            continue;
+        }
         let outcome = handle(app, ctx, &req.method, &req.params);
         match outcome {
             Outcome::Done(v) => {

crates/ui-egui/tests/control_timeout.rs

@@ -0,0 +1,61 @@
+//! A control request whose sender stopped waiting is dropped unrun (#1914).
+
+use gridcraft_engine::Session;
+use gridcraft_engine::core::CellRef;
+use gridcraft_ui_egui::{ControlRequest, SheetApp};
+use serde_json::json;
+
+fn app() -> (SheetApp, egui::Context) {
+    let mut s = Session::new();
+    s.new_workbook();
+    (SheetApp::new(s, Default::default()), egui::Context::default())
+}
+
+fn a1(app: &SheetApp) -> String {
+    let d = app.session.active().unwrap();
+    d.wb.active().unwrap().cell(CellRef::parse("A1").unwrap()).map(|c| format!("{:?}", c.value)).unwrap_or_else(|| "empty".into())
+}
+
+/// `set` writes A1, so a request that did run is visible in the workbook.
+fn set_a1(text: &str) -> serde_json::Value {
+    json!({"command": "cell.set", "params": {"cell": "A1", "input": text}})
+}
+
+#[test]
+fn a_request_its_sender_gave_up_on_does_not_run_later() {
+    let (mut app, ctx) = app();
+    let (tx, rx) = std::sync::mpsc::channel();
+    app.control_rx = Some(rx);
+
+    // One request the sender is still waiting for, and one it gave up on after its reply timed
+    // out. Before the fix both ran on the next UI pass: the client was told its command had
+    // failed and the command was applied anyway, so an agent that retried ran it twice.
+    let (kept, kept_rx) = ControlRequest::new("engine.execute", set_a1("kept"));
+    tx.send(kept).unwrap();
+    gridcraft_ui_egui::control::poll(&mut app, &ctx);
+    assert!(kept_rx.try_recv().is_ok(), "a request still waited for is answered");
+    assert_eq!(a1(&app), "Text(\"kept\")");
+
+    let (dropped, dropped_rx) = ControlRequest::new("engine.execute", set_a1("gave up"));
+    assert!(ControlRequest::abandon(&dropped.state), "the sender gives up while it is queued");
+    tx.send(dropped).unwrap();
+    gridcraft_ui_egui::control::poll(&mut app, &ctx);
+    assert_eq!(a1(&app), "Text(\"kept\")", "the abandoned request did not run");
+    assert!(dropped_rx.try_recv().is_err(), "and nothing was replied to it");
+}
+
+/// Which of the two sides wins the race is settled once: a request the app has started is not
+/// abandoned (the app will answer, so the sender reports an unknown outcome instead of a failure),
+/// and one already abandoned is never started.
+#[test]
+fn taking_and_abandoning_a_request_settle_once() {
+    let (taken, _rx) = ControlRequest::new("engine.execute", json!({}));
+    assert!(taken.take(), "a pending request can be taken");
+    assert!(taken.take(), "and taking it again is the same answer");
+    assert!(!ControlRequest::abandon(&taken.state), "it cannot then be abandoned");
+
+    let (given_up, _rx) = ControlRequest::new("engine.execute", json!({}));
+    assert!(ControlRequest::abandon(&given_up.state), "a pending request can be abandoned");
+    assert!(ControlRequest::abandon(&given_up.state), "and again is the same answer");
+    assert!(!given_up.take(), "it cannot then be taken");
+}