From ea30e411e716fe24db0c7a0642223ae10ca1770d Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:31:02 +0700 Subject: [PATCH] Fix #1725: a control request its caller timed out on is withdrawn, not run later The worker answered "timeout" after 60 s and left the request on the queue, so the app ran it whenever it next drained the queue: an edit nobody was waiting for, and a second one if the caller had retried. A request now leaves the queue once: taken by the app, or given up by its caller, whichever comes first. The timeout reply says which ("will not run" / "still running"). --- apps/vectorcraft/src/control_server.rs | 31 +++++++++++++++++++++++- crates/ui-egui/src/control.rs | 33 +++++++++++++++++++++++++- crates/ui-egui/src/lib.rs | 7 +++++- docs/control-protocol.md | 6 +++++ 4 files changed, 74 insertions(+), 3 deletions(-) diff --git a/apps/vectorcraft/src/control_server.rs b/apps/vectorcraft/src/control_server.rs index 846ebc46..21eacd61 100644 --- a/apps/vectorcraft/src/control_server.rs +++ b/apps/vectorcraft/src/control_server.rs @@ -29,6 +29,21 @@ pub const MAX_CONNECTIONS: usize = 16; /// Size in bytes of the buffer that [`write_line`] writes a reply line through. const REPLY_CHUNK: usize = 64 * 1024; +/// How long a request waits for the app's reply. +const REPLY_WAIT: Duration = Duration::from_secs(60); + +/// The reply to a request the app didn't answer in [`REPLY_WAIT`]. One it had not started is +/// withdrawn, so it never runs after its caller was told it failed; one it is running can't be +/// stopped, and the reply says its outcome isn't known. +fn timed_out(ticket: &vectorcraft_ui_egui::ControlTicket) -> Value { + let error = if ticket.give_up() { + "timeout: the app did not start the request in time; it was withdrawn and will not run" + } else { + "timeout: the app is still running the request; it may yet complete" + }; + json!({"ok": false, "error": error}) +} + #[derive(Clone, Copy)] struct ConnectionTimeouts { idle: Duration, @@ -106,9 +121,10 @@ fn spawn_connection( let _slot = slot; serve_socket(stream, timeouts, |method, params| { let (req, rrx) = ControlRequest::new(method, params); + let ticket = req.ticket(); tx.send(req).ok()?; ctx.request_repaint(); - Some(rrx.recv_timeout(Duration::from_secs(60)).unwrap_or_else(|_| json!({"ok": false, "error": "timeout"}))) + Some(rrx.recv_timeout(REPLY_WAIT).unwrap_or_else(|_| timed_out(&ticket))) }); }) } @@ -455,6 +471,19 @@ mod tests { json!({"ok": true, "result": {"layers": [{"children": paths, "id": 1, "kind": "Layer", "name": "Layer 1"}]}}) } + /// #1725: a request its caller gave up before the app took it never runs; one the app took + /// first can't be withdrawn, and the timeout says so. + #[test] + fn a_timed_out_request_is_withdrawn_unless_it_started() { + let (waiting, _rx) = ControlRequest::new("engine.execute", json!({})); + assert!(timed_out(&waiting.ticket())["error"].as_str().unwrap().contains("will not run")); + assert!(!waiting.claim(), "the app skips a withdrawn request"); + let (running, _rx) = ControlRequest::new("engine.execute", json!({})); + assert!(running.claim()); + assert!(timed_out(&running.ticket())["error"].as_str().unwrap().contains("still running")); + assert!(!running.claim(), "a request is taken once"); + } + #[test] fn requests_are_answered_in_order_with_ids() { let (r, calls) = run(b"{\"id\":1,\"method\":\"a\"}\n\n{\"id\":2,\"method\":\"b\",\"params\":{}}\r\n{\"method\":\"c\"}"); diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index ef921fdb..4cb60e21 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -24,6 +24,8 @@ //! - `app.export {path?, format?, …document.export options}`: encoded by the engine, written through //! the host; the document keeps its path. No path → `{dataBase64, format, bytes}` (as headless) +use std::sync::Arc; +use std::sync::atomic::{AtomicU8, Ordering}; use std::sync::mpsc::Sender; use serde_json::{Value, json}; @@ -63,12 +65,41 @@ pub struct ControlRequest { pub method: String, pub params: Value, pub reply: Sender, + state: Arc, +} + +/// A request's state: waiting to run, taken by the app ([`ControlRequest::claim`]), or given up +/// by its caller before that ([`Ticket::give_up`]). It leaves `QUEUED` once, one way or the other. +const QUEUED: u8 = 0; +const STARTED: u8 = 1; +const GIVEN_UP: u8 = 2; + +/// The caller's side of a queued [`ControlRequest`]. +pub struct Ticket(Arc); + +impl Ticket { + /// Stop waiting for the request → `true` when it had not started, and so never runs: a caller + /// told "timeout" can then retry without the edit being applied twice, or applied later when + /// nobody expects it (#1725). `false`: the app is running it (or has); its outcome is unknown. + pub fn give_up(&self) -> bool { + self.0.compare_exchange(QUEUED, GIVEN_UP, Ordering::SeqCst, Ordering::SeqCst).is_ok() + } } impl ControlRequest { pub fn new(method: impl Into, params: Value) -> (Self, std::sync::mpsc::Receiver) { let (tx, rx) = std::sync::mpsc::channel(); - (Self { method: method.into(), params, reply: tx }, rx) + (Self { method: method.into(), params, reply: tx, state: Arc::new(AtomicU8::new(QUEUED)) }, rx) + } + + /// The caller's handle on this request, to give it up with. + pub fn ticket(&self) -> Ticket { + Ticket(self.state.clone()) + } + + /// Take the request to run it → `false` when its caller gave it up meanwhile (don't run it). + pub fn claim(&self) -> bool { + self.state.compare_exchange(QUEUED, STARTED, Ordering::SeqCst, Ordering::SeqCst).is_ok() } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index d211c5c2..e3e953cc 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -162,7 +162,7 @@ use vectorcraft_engine::cmd::fileio; use vectorcraft_engine::file_access::{self, AutomationRoots}; use vectorcraft_engine::{Session, ViewInfo}; -pub use control::{ControlRequest, ControlResponse}; +pub use control::{ControlRequest, ControlResponse, Ticket as ControlTicket}; pub use state::{UiState, View}; pub use sysclip::{ClipboardProbeFactory, SystemClipboard}; @@ -769,6 +769,11 @@ impl VectorcraftApp { let mut handled = 0; while handled < control::MAX_CONTROL_REQUESTS_PER_FRAME { let Ok(req) = rx.try_recv() else { break }; + // Its caller stopped waiting (the app was busy or its window not drawn for too long) + // and was told it won't run: an edit applied now would come from nowhere (#1725). + if !req.claim() { + continue; + } handled += 1; let reply = req.reply.clone(); // Guarded per request: a panic must not drop the channel (taken out of `self` above). diff --git a/docs/control-protocol.md b/docs/control-protocol.md index bbbaf648..742f627e 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -43,6 +43,12 @@ and does not append a partial gesture. Clients can retry once earlier input has scheduled. These limits cover synthetic input admission, separately from socket transport and command-processing timeouts. +A request the app hasn't answered after **60 s** gets `{"ok": false, "error": "timeout: …"}`. If the +app had not started it (it was busy, or its window wasn't being drawn), the request is withdrawn +and never runs, and the error says "will not run": retrying is safe. If the app was already +running it, the error says "still running": it may yet complete, so check the document before +retrying. + ## Confining file access ```sh