diff --git a/.github/workflows/ci-rust-windows.yml b/.github/workflows/ci-rust-windows.yml index 4a3145c7b1..bc9472714a 100644 --- a/.github/workflows/ci-rust-windows.yml +++ b/.github/workflows/ci-rust-windows.yml @@ -66,7 +66,7 @@ jobs: run: cargo test -p cua-driver -p cua-driver-core -p cua-driver-sdk -p cua-driver-testkit -p cursor-overlay -p cursor-theme-cli -p platform-windows -p cua-driver-uia --all-targets --no-run --locked - name: Run complete desktop-independent core tests working-directory: libs/cua-driver/rust - run: cargo test -p cua-driver-core --lib --locked + run: cargo test -p cua-driver-core --lib --test recording_shutdown_errors --locked - name: Run deferred text publication regressions working-directory: libs/cua-driver/rust run: | diff --git a/libs/cua-driver/rust/crates/cua-driver-core/src/video_ffmpeg.rs b/libs/cua-driver/rust/crates/cua-driver-core/src/video_ffmpeg.rs index ed48f7b6d0..36d108c8c0 100644 --- a/libs/cua-driver/rust/crates/cua-driver-core/src/video_ffmpeg.rs +++ b/libs/cua-driver/rust/crates/cua-driver-core/src/video_ffmpeg.rs @@ -138,51 +138,47 @@ impl FfmpegVideoBackend { impl VideoBackend for FfmpegVideoBackend { fn stop(mut self: Box) -> anyhow::Result { let elapsed = self.started_at.elapsed(); - let finalized; - if let Some(mut stdin) = self.child.stdin.take() { let _ = stdin.write_all(b"q\n"); let _ = stdin.flush(); } - let deadline = Instant::now() + Duration::from_millis(3000); - loop { + let shutdown_timeout = Duration::from_millis(3000); + let deadline = Instant::now() + shutdown_timeout; + let result = loop { match self.child.try_wait()? { + Some(status) if status.success() => break Ok(()), Some(status) => { - finalized = status.success(); - break; + let cause = status + .code() + .map_or_else(|| status.to_string(), |code| format!("code {code}")); + break Err(anyhow::anyhow!("ffmpeg exited with {cause}")); } - None => { - if Instant::now() > deadline { - // Polite shutdown stalled — force kill. mp4 will lack - // a moov atom and won't be playable; `finalized: - // false` tells the caller. - let _ = self.child.kill(); - let _ = self.child.wait(); - finalized = false; - break; - } - std::thread::sleep(Duration::from_millis(80)); + None if Instant::now() > deadline => { + let _ = self.child.kill(); + let _ = self.child.wait(); + break Err(anyhow::anyhow!( + "ffmpeg shutdown timed out after {} ms", + shutdown_timeout.as_millis() + )); } + None => std::thread::sleep(Duration::from_millis(80)), } - } - - if !finalized { - if let Some(handle) = self.stderr_thread.take() { - if let Ok(buf) = handle.join() { - let tail = String::from_utf8_lossy(&buf); - tracing::warn!(target: "recording", - "ffmpeg did not finalize cleanly. Last stderr tail:\n{tail}"); - } + }; + if let Some(handle) = self.stderr_thread.take() { + let stderr = handle.join().unwrap_or_default(); + if let Err(error) = &result { + tracing::warn!(target: "recording", + %error, + stderr_tail = %String::from_utf8_lossy(&stderr), + "ffmpeg shutdown failed"); } - } else if let Some(handle) = self.stderr_thread.take() { - let _ = handle.join(); } - + result?; Ok(VideoMetadata { path: self.output_path, duration_ms: elapsed.as_millis() as u64, - finalized, + finalized: true, }) } } diff --git a/libs/cua-driver/rust/crates/cua-driver-core/tests/fixtures/failing_encoder.rs b/libs/cua-driver/rust/crates/cua-driver-core/tests/fixtures/failing_encoder.rs new file mode 100644 index 0000000000..242dbaff8f --- /dev/null +++ b/libs/cua-driver/rust/crates/cua-driver-core/tests/fixtures/failing_encoder.rs @@ -0,0 +1,17 @@ +use std::io::{self, Read}; + +fn main() { + if std::env::args().any(|arg| arg == "-version") { + return; + } + let mut byte = [0]; + while io::stdin().read_exact(&mut byte).is_ok() { + if byte[0] == b'q' { + if std::env::var("CUA_RECORDING_ERROR_TEST_CHILD").as_deref() == Ok("timeout") { + std::thread::sleep(std::time::Duration::from_secs(30)); + } + std::process::exit(23); + } + } + std::process::exit(24); +} diff --git a/libs/cua-driver/rust/crates/cua-driver-core/tests/recording_shutdown_errors.rs b/libs/cua-driver/rust/crates/cua-driver-core/tests/recording_shutdown_errors.rs new file mode 100644 index 0000000000..0d4b7bfd47 --- /dev/null +++ b/libs/cua-driver/rust/crates/cua-driver-core/tests/recording_shutdown_errors.rs @@ -0,0 +1,116 @@ +use std::{process::Command, sync::Arc}; + +use cua_driver_core::{ + recording::RecordingSession, + recording_tools::{GetRecordingStateTool, StartRecordingTool, StopRecordingTool}, + tool::Tool, + video::set_video_backend_factory, + video_ffmpeg::FfmpegVideoBackendFactory, +}; +use serde_json::{json, Value}; + +const CHILD_MODE: &str = "CUA_RECORDING_ERROR_TEST_CHILD"; + +#[tokio::test] +async fn stop_recording_reports_encoder_exit() { + if std::env::var_os(CHILD_MODE).is_none() { + run_in_child_process("stop_recording_reports_encoder_exit", "exit"); + return; + } + let (response, state) = record_with_encoder().await; + assert_eq!(response["isError"], true); + assert!( + response["content"][0]["text"] + .as_str() + .unwrap() + .contains("ffmpeg exited with code 23"), + "{response}" + ); + assert_eq!(state["enabled"], false); + assert!(state["last_video_path"].is_null()); + assert!(state["last_error"] + .as_str() + .unwrap() + .contains("ffmpeg exited with code 23")); +} + +#[tokio::test] +async fn stop_recording_reports_shutdown_timeout() { + if std::env::var_os(CHILD_MODE).is_none() { + run_in_child_process("stop_recording_reports_shutdown_timeout", "timeout"); + return; + } + let (response, state) = record_with_encoder().await; + assert_eq!(response["isError"], true); + assert!( + response["content"][0]["text"] + .as_str() + .unwrap() + .contains("ffmpeg shutdown timed out"), + "{response}" + ); + assert_eq!(state["enabled"], false); + assert!(state["last_video_path"].is_null()); + assert!(state["last_error"] + .as_str() + .unwrap() + .contains("ffmpeg shutdown timed out")); +} + +fn run_in_child_process(test_name: &str, mode: &str) { + let directory = tempfile::tempdir().unwrap(); + let encoder = directory.path().join(if cfg!(windows) { + "ffmpeg.exe" + } else { + "ffmpeg" + }); + let compilation = Command::new("rustc") + .arg(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/failing_encoder.rs" + )) + .arg("-o") + .arg(&encoder) + .output() + .unwrap(); + assert!(compilation.status.success(), "{compilation:?}"); + let path = std::env::join_paths( + std::iter::once(directory.path().to_path_buf()) + .chain(std::env::split_paths(&std::env::var_os("PATH").unwrap())), + ) + .unwrap(); + let child = Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test_name, "--nocapture"]) + .env(CHILD_MODE, mode) + .env("PATH", path) + .output() + .unwrap(); + assert!( + child.status.success(), + "{}\n{}", + String::from_utf8_lossy(&child.stdout), + String::from_utf8_lossy(&child.stderr) + ); +} + +async fn record_with_encoder() -> (Value, Value) { + set_video_backend_factory(Box::new(FfmpegVideoBackendFactory)); + let session = Arc::new(RecordingSession::new()); + let directory = tempfile::tempdir().unwrap(); + let started = StartRecordingTool::new(session.clone()) + .invoke(json!({"output_dir": directory.path(), "record_video": true})) + .await; + assert_eq!( + started.structured_content.as_ref().unwrap()["video_active"], + true, + "{started:?}" + ); + let stopped = StopRecordingTool::new(session.clone()) + .invoke(json!({})) + .await; + let state = GetRecordingStateTool::new(session).invoke(json!({})).await; + ( + serde_json::to_value(stopped).unwrap(), + state.structured_content.unwrap(), + ) +}