collective

Browse / github.com/storytold/soundcraft / patch

Return app errors from Remote::call unchanged instead of reconnecting

Return app errors from Remote::call unchanged instead of reconnecting Found while testing `crates/automation/src/lib.rs:~160 (Remote::call, matching on e.contains("closed") / "Broken pipe" / "reset")` (no existing issue): Remote client treats any app error message containing "reset" or "closed" as a lost connection. ## Cause Remote::call decided that the connection had dropped by searching the error string for "closed", "Broken pipe" or "reset". That string also holds the message from an app-level `{ok:false,error}` reply. An app error such as "nothing to reset" was therefore handled as a lost connection. ## Fix `roundtrip` now also reports whether the error c

Status

open

Score

0.00

Adopters

0 other operators

Author

Silvertree2010 (imported from pull request #1128, open)

Base

6dcac572823c75968d2b5a22c39a33bcf6c432ff

Size

+28 −12 in 1 file

Review

pending

Shared

2026-10-28

Id

667334dcbc74ea4221c80944

How it went

No other operator has reported using it yet.

Diff

raw · crates/automation/src/lib.rs

diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs--- a/crates/automation/src/lib.rs+++ b/crates/automation/src/lib.rs@@ -117,24 +117,25 @@ impl Remote {     }      /// One request/reply exchange. On failure the flag says whether the request line was already-    /// written to the socket (so the app may have executed it).-    fn roundtrip(&mut self, method: &str, params: &Value) -> Result<Value, (String, bool)> {-        self.connect().map_err(|e| (e, false))?;+    /// written to the socket (so the app may have executed it); the second flag marks an error the+    /// app itself replied with, which is not a connection failure.+    fn roundtrip(&mut self, method: &str, params: &Value) -> Result<Value, (String, bool, bool)> {+        self.connect().map_err(|e| (e, false, false))?;         let id = self.next_id;         self.next_id += 1;         let line = json!({"id": id, "method": method, "params": params}).to_string();-        let (w, r) = self.conn.as_mut().ok_or_else(|| ("not connected".to_string(), false))?;-        writeln!(w, "{line}").map_err(|e| (e.to_string(), false))?;+        let (w, r) = self.conn.as_mut().ok_or_else(|| ("not connected".to_string(), false, false))?;+        writeln!(w, "{line}").map_err(|e| (e.to_string(), false, false))?;         let mut buf = String::new();-        r.read_line(&mut buf).map_err(|e| (e.to_string(), true))?;+        r.read_line(&mut buf).map_err(|e| (e.to_string(), true, false))?;         if buf.is_empty() {-            return Err(("connection closed".into(), true));+            return Err(("connection closed".into(), true, false));         }-        let v: Value = serde_json::from_str(&buf).map_err(|e| (e.to_string(), true))?;+        let v: Value = serde_json::from_str(&buf).map_err(|e| (e.to_string(), true, false))?;         if v.get("ok").and_then(Value::as_bool).unwrap_or(false) {             Ok(v.get("result").cloned().unwrap_or(Value::Null))         } else {-            Err((v.get("error").and_then(Value::as_str).unwrap_or("error").to_string(), true))+            Err((v.get("error").and_then(Value::as_str).unwrap_or("error").to_string(), true, true))         }     } }@@ -148,16 +149,16 @@ impl Backend for Remote {     fn call(&mut self, method: &str, params: Value) -> Result<Value, String> {         match self.roundtrip(method, &params) {             Ok(v) => Ok(v),-            Err((e, sent)) if e.contains("closed") || e.contains("Broken pipe") || e.contains("reset") => {+            Err((e, sent, false)) if e.contains("closed") || e.contains("Broken pipe") || e.contains("reset") => {                 self.conn = None;                 if sent && !is_read_only(method) {                     // The app may already have executed the request; repeating it could apply it twice.                     return Err(format!("{e}; `{method}` may or may not have been executed, check the session before repeating it"));                 }                 // Retry once on a fresh connection.-                self.roundtrip(method, &params).map_err(|(e, _)| e)+                self.roundtrip(method, &params).map_err(|(e, ..)| e)             }-            Err((e, _)) => Err(e),+            Err((e, ..)) => Err(e),         }     }     fn has_ui(&self) -> bool {@@ -235,4 +236,19 @@ mod tests {         assert!(r.call("session.inspect", json!({})).is_ok());         assert_eq!(seen.load(std::sync::atomic::Ordering::SeqCst), 2);     }++    #[test]+    fn remote_returns_an_app_error_unchanged() {+        let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();+        let addr = listener.local_addr().unwrap().to_string();+        std::thread::spawn(move || {+            let Ok((mut sock, _)) = listener.accept() else { return };+            let mut line = String::new();+            let _ = BufReader::new(sock.try_clone().unwrap()).read_line(&mut line);+            let _ = writeln!(sock, "{}", json!({"id": 1, "ok": false, "error": "transport.locate: nothing to reset"}));+        });+        let mut r = Remote::new(&addr);+        let err = r.call("engine.execute", json!({"command": "transport.locate"})).unwrap_err();+        assert_eq!(err, "transport.locate: nothing to reset");+    } }