Serialize bridge render_frame calls so parallel requests get their own frame (#3837) (#3872)

crates/automation/src/server.rs

@@ -35,6 +35,9 @@ fn join_error(e: tokio::task::JoinError) -> AutomationError {
 #[derive(Clone)]
 pub struct FilmcraftMcp {
     backend: Arc<Backend>,
+    /// Bridge `render_frame` moves the app's one playhead and screenshots the one Program monitor, so
+    /// calls (which MCP clients may send in parallel) take turns instead of seeing each other's frame.
+    render_lock: Arc<tokio::sync::Mutex<()>>,
     tool_router: ToolRouter<Self>,
 }
 
@@ -197,10 +200,10 @@ fn wrap(r: Result<Value, AutomationError>) -> Result<CallToolResult, McpError> {
 
 impl FilmcraftMcp {
     pub fn headless(session: Session) -> Self {
-        Self { backend: Arc::new(Backend::Headless(Arc::new(Mutex::new(session)))), tool_router: Self::tool_router() }
+        Self { backend: Arc::new(Backend::Headless(Arc::new(Mutex::new(session)))), render_lock: Arc::default(), tool_router: Self::tool_router() }
     }
     pub fn bridge(addr: &str) -> Result<Self, AutomationError> {
-        Ok(Self { backend: Arc::new(Backend::Bridge(Arc::new(BridgeClient::new(addr)?))), tool_router: Self::tool_router() })
+        Ok(Self { backend: Arc::new(Backend::Bridge(Arc::new(BridgeClient::new(addr)?))), render_lock: Arc::default(), tool_router: Self::tool_router() })
     }
 
     pub async fn serve_stdio(self) -> Result<(), AutomationError> {
@@ -451,6 +454,8 @@ impl FilmcraftMcp {
                 })
             }
             Backend::Bridge(b) => {
+                // held until the playhead is back, also for the no-`seconds` screenshot, which must not catch another call's seek
+                let _turn = self.render_lock.lock().await;
                 let Some(sec) = p.seconds else {
                     return self.screenshot(&b.clone(), Some("Program".into()), max).await;
                 };

crates/automation/src/server/tests.rs

@@ -401,3 +401,59 @@ async fn doc_inspect_handles_an_active_sequence_id_above_i64_max() {
     assert_eq!(v["project"]["activeSequence"].as_u64(), Some(HUGE), "{v}");
     assert_eq!(v["sequence"]["id"].as_u64(), Some(HUGE), "an id above i64::MAX must still be inspected, not dropped to a null sequence: {v}");
 }
+
+/// Bridge `render_frame` with `seconds` moves the app's single playhead, screenshots the single
+/// Program monitor and moves the playhead back. Parallel calls (the tool is annotated read-only and
+/// idempotent, so clients send them that way) interleaved those steps and got each other's frames
+/// (#3837). Each call has to return the frame at its own time.
+#[tokio::test]
+async fn parallel_bridge_render_frames_each_return_their_own_frame() {
+    use base64::Engine as _;
+    use tokio::io::{AsyncBufReadExt, AsyncWriteExt};
+    let playhead = Arc::new(std::sync::atomic::AtomicI64::new(200));
+    let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
+    let addr = listener.local_addr().unwrap().to_string();
+    let app_playhead = playhead.clone();
+    tokio::spawn(async move {
+        while let Ok((s, _)) = listener.accept().await {
+            let playhead = app_playhead.clone();
+            tokio::spawn(async move {
+                let (r, mut w) = s.into_split();
+                let mut lines = tokio::io::BufReader::new(r).lines();
+                while let Ok(Some(line)) = lines.next_line().await {
+                    let req: Value = serde_json::from_str(&line).unwrap();
+                    let params = &req["params"]["params"];
+                    let result = match (req["method"].as_str().unwrap_or_default(), req["params"]["command"].as_str().unwrap_or_default()) {
+                        (_, "sequence.inspect") => json!({"playhead": playhead.load(std::sync::atomic::Ordering::SeqCst)}),
+                        (_, "playhead.set") => {
+                            let t = params["seconds"].as_f64().map(|x| x as i64).or_else(|| params["time"].as_i64()).unwrap_or(0);
+                            playhead.store(t, std::sync::atomic::Ordering::SeqCst);
+                            Value::Null
+                        }
+                        ("ui.screenshot", _) => {
+                            // the monitor shows whatever the playhead is at when the screenshot is taken
+                            tokio::time::sleep(std::time::Duration::from_millis(30)).await;
+                            let shade = playhead.load(std::sync::atomic::Ordering::SeqCst) as u8;
+                            let img = image::RgbaImage::from_pixel(1, 1, image::Rgba([shade, 0, 0, 255]));
+                            img.save(req["params"]["path"].as_str().unwrap()).unwrap();
+                            Value::Null
+                        }
+                        _ => Value::Null,
+                    };
+                    let line = format!("{}\n", json!({"id": req["id"], "ok": true, "result": result}));
+                    w.write_all(line.as_bytes()).await.unwrap();
+                }
+            });
+        }
+    });
+    let m = FilmcraftMcp::bridge(&addr).unwrap();
+    let call = |seconds: f64| m.render_frame(Parameters(RenderParams { seconds: Some(seconds), max_side: Some(640) }));
+    let results = tokio::join!(call(5.0), call(20.0), call(35.0), call(50.0));
+    for (res, want) in [(results.0, 5u8), (results.1, 20), (results.2, 35), (results.3, 50)] {
+        let v = serde_json::to_value(res.unwrap()).unwrap();
+        let png = base64::engine::general_purpose::STANDARD.decode(v["content"][0]["data"].as_str().unwrap()).unwrap();
+        let px = image::load_from_memory(&png).unwrap().to_rgba8().get_pixel(0, 0).0;
+        assert_eq!(px[0], want, "frame at {want} s came from another call's playhead");
+    }
+    assert_eq!(playhead.load(std::sync::atomic::Ordering::SeqCst), 200, "the playhead must end where it started");
+}

docs/agents.md

@@ -83,7 +83,7 @@ claude mcp add filmcraft -- filmcraft-cli mcp
 | `project_inspect` | both | bins and items with ids, types, durations; active sequence |
 | `sequence_inspect` | both | the active sequence: tracks, clips (`start` / `end` and `sourceIn` / `sourceOut` in ticks, frames, `speed`, `reverse`: the clip plays its `sourceIn` to `sourceOut` stretch backward, `gainDb`), effects, transitions, markers (name and `comment`), playhead, selection |
 | `media_import` | both | import files by absolute path (`paths`, an array; optional `bin`, `image_sequence`) |
-| `render_frame` | both | PNG of the program frame at `seconds` (headless renders; bridge screenshots the Program monitor). Read-only: the playhead and selection are left as they were |
+| `render_frame` | both | PNG of the program frame at `seconds` (headless renders; bridge screenshots the Program monitor). Read-only: the playhead and selection are left as they were. Parallel bridge calls take turns, so each returns its own frame |
 | `ui_inspect` | bridge | UI state: tool, workspace, panels, zoom, playback, fps |
 | `ui_elements` | bridge | on-screen interactive elements with id, label and rect (`prefix` filter) |
 | `ui_click` | bridge | click by `id` or `x`,`y`; `button`, `count`, `modifiers` |