-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Windows desktop reliability (logs) + onboarding bits #1797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,17 +6,55 @@ use cap_rendering::{ | |
| FrameRenderer, ProjectRecordingsMeta, ProjectUniforms, RenderSegment, RenderVideoConstants, | ||
| RendererLayers, ZoomFocusInterpolator, spring_mass_damper::SpringMassDamperSimulationConfig, | ||
| }; | ||
| use futures::FutureExt; | ||
| use image::codecs::jpeg::JpegEncoder; | ||
| use serde::{Deserialize, Serialize}; | ||
| use specta::Type; | ||
| use std::any::Any; | ||
| use std::panic::AssertUnwindSafe; | ||
| use std::{ | ||
| path::{Path, PathBuf}, | ||
| sync::{ | ||
| Arc, | ||
| atomic::{AtomicBool, Ordering}, | ||
| }, | ||
| }; | ||
| use tracing::{info, instrument}; | ||
| use tracing::{error, info, instrument}; | ||
|
|
||
| fn panic_message(panic: Box<dyn Any + Send>) -> String { | ||
| if let Some(msg) = panic.downcast_ref::<&str>() { | ||
| msg.to_string() | ||
| } else if let Some(msg) = panic.downcast_ref::<String>() { | ||
| msg.clone() | ||
| } else { | ||
| "unknown panic".to_string() | ||
| } | ||
| } | ||
|
|
||
| async fn run_protected_export( | ||
| project_path: &Path, | ||
| settings: &ExportSettings, | ||
| progress: &tauri::ipc::Channel<FramesRendered>, | ||
| force_ffmpeg: bool, | ||
| ) -> Result<PathBuf, String> { | ||
| match AssertUnwindSafe(do_export(project_path, settings, progress, force_ffmpeg)) | ||
| .catch_unwind() | ||
| .await | ||
| { | ||
| Ok(result) => result, | ||
| Err(panic) => { | ||
| let panic_msg = panic_message(panic); | ||
| error!( | ||
| target: "cap_desktop_export", | ||
| panic = %panic_msg, | ||
| "export task panicked" | ||
| ); | ||
| let message = format!("Export task panicked: {panic_msg}"); | ||
| sentry::capture_message(&message, sentry::Level::Error); | ||
| Err(message) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+41
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src-tauri/src/export.rs
Line: 41-57
Comment:
**`AssertUnwindSafe` on retry path**
`AssertUnwindSafe` is correct here — caught panics return `Err` and the export is abandoned rather than retried into potentially corrupted state. One thing worth double-checking: the retry in `export_video` (with `force_ffmpeg: true`) runs immediately after a panic in the GPU path. If the panic leaked an exclusive lock on a WGPU device or left a GPU command encoder in an open state, the retry (even on the software/ffmpeg path) may encounter those residual locks. If you see intermittent hangs on the retry leg during Windows testing, that would be the likely cause.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| struct ExportActiveGuard<'a>(&'a AtomicBool); | ||
|
|
||
|
|
@@ -193,7 +231,7 @@ pub async fn export_video( | |
| wait_for_export_preview_idle(&ed.export_preview_active).await; | ||
| } | ||
|
|
||
| let result = do_export(&project_path, &settings, &progress, force_ffmpeg).await; | ||
| let result = run_protected_export(&project_path, &settings, &progress, force_ffmpeg).await; | ||
|
|
||
| match result { | ||
| Ok(path) => { | ||
|
|
@@ -206,7 +244,8 @@ pub async fn export_video( | |
| e | ||
| ); | ||
|
|
||
| let retry_result = do_export(&project_path, &settings, &progress, true).await; | ||
| let retry_result = | ||
| run_protected_export(&project_path, &settings, &progress, true).await; | ||
|
|
||
| match retry_result { | ||
| Ok(path) => { | ||
|
|
@@ -351,6 +390,35 @@ pub async fn generate_export_preview( | |
| project_path: PathBuf, | ||
| frame_time: f64, | ||
| settings: ExportPreviewSettings, | ||
| ) -> Result<ExportPreviewResult, String> { | ||
| match AssertUnwindSafe(generate_export_preview_inner( | ||
| project_path, | ||
| frame_time, | ||
| settings, | ||
| )) | ||
| .catch_unwind() | ||
| .await | ||
| { | ||
| Ok(result) => result, | ||
| Err(panic) => { | ||
| let panic_msg = panic_message(panic); | ||
| error!( | ||
| target: "cap_desktop_export", | ||
| panic = %panic_msg, | ||
| "generate_export_preview panicked" | ||
| ); | ||
| let message = format!("Export preview panicked: {panic_msg}"); | ||
| sentry::capture_message(&message, sentry::Level::Error); | ||
| Err(message) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[instrument(skip_all)] | ||
| async fn generate_export_preview_inner( | ||
| project_path: PathBuf, | ||
| frame_time: f64, | ||
| settings: ExportPreviewSettings, | ||
| ) -> Result<ExportPreviewResult, String> { | ||
| use base64::{Engine, engine::general_purpose::STANDARD}; | ||
| use cap_editor::create_segments; | ||
|
|
@@ -593,6 +661,33 @@ pub async fn generate_export_preview_fast( | |
| editor: WindowEditorInstance, | ||
| frame_time: f64, | ||
| settings: ExportPreviewSettings, | ||
| ) -> Result<ExportPreviewResult, String> { | ||
| match AssertUnwindSafe(generate_export_preview_fast_inner( | ||
| editor, frame_time, settings, | ||
| )) | ||
| .catch_unwind() | ||
| .await | ||
| { | ||
| Ok(result) => result, | ||
| Err(panic) => { | ||
| let panic_msg = panic_message(panic); | ||
| error!( | ||
| target: "cap_desktop_export", | ||
| panic = %panic_msg, | ||
| "generate_export_preview_fast panicked" | ||
| ); | ||
| let message = format!("Export preview panicked: {panic_msg}"); | ||
| sentry::capture_message(&message, sentry::Level::Error); | ||
| Err(message) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[instrument(skip_all)] | ||
| async fn generate_export_preview_fast_inner( | ||
| editor: WindowEditorInstance, | ||
| frame_time: f64, | ||
| settings: ExportPreviewSettings, | ||
| ) -> Result<ExportPreviewResult, String> { | ||
| use base64::{Engine, engine::general_purpose::STANDARD}; | ||
| use std::time::Instant; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,13 @@ fn main() { | |
| std::env::set_var("RUST_LOG", "trace"); | ||
| } | ||
|
|
||
| #[cfg(not(debug_assertions))] | ||
| if std::env::var_os("RUST_BACKTRACE").is_none() { | ||
| unsafe { | ||
| std::env::set_var("RUST_BACKTRACE", "full"); | ||
| } | ||
| } | ||
|
|
||
| // We have to hold onto the ClientInitGuard until the very end | ||
| let _sentry_guard = std::option_env!("CAP_DESKTOP_SENTRY_URL").map(|url| { | ||
| let sentry_client = sentry::init(( | ||
|
|
@@ -72,7 +79,6 @@ fn main() { | |
| }); | ||
|
|
||
| let file_appender = tracing_appender::rolling::daily(&logs_dir, "cap-desktop.log"); | ||
| let (non_blocking, _logger_guard) = tracing_appender::non_blocking(file_appender); | ||
|
|
||
| let (otel_layer, _tracer) = if cfg!(debug_assertions) { | ||
| use opentelemetry::trace::TracerProvider; | ||
|
|
@@ -125,10 +131,12 @@ fn main() { | |
| tracing_subscriber::fmt::layer() | ||
| .with_ansi(false) | ||
| .with_target(true) | ||
| .with_writer(non_blocking), | ||
| .with_writer(file_appender), | ||
| ) | ||
| .init(); | ||
|
|
||
| install_panic_hook(logs_dir.clone()); | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| sentry::configure_scope(|scope| { | ||
| scope.set_user(Some(sentry::User { | ||
|
|
@@ -143,3 +151,72 @@ fn main() { | |
| .expect("Failed to build multi threaded tokio runtime") | ||
| .block_on(cap_desktop_lib::run(handle, logs_dir)); | ||
| } | ||
|
|
||
| fn install_panic_hook(logs_dir: std::path::PathBuf) { | ||
| let prev = std::panic::take_hook(); | ||
| let panics_log = logs_dir.join("panics.log"); | ||
| std::panic::set_hook(Box::new(move |info| { | ||
| let location = info | ||
| .location() | ||
| .map(|l| format!("{}:{}:{}", l.file(), l.line(), l.column())) | ||
| .unwrap_or_else(|| "<unknown>".to_string()); | ||
| let message = info | ||
| .payload() | ||
|
Comment on lines
+163
to
+168
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src-tauri/src/main.rs
Line: 159-164
Comment:
**`RUST_BACKTRACE` has no effect on `Backtrace::force_capture()`**
`std::backtrace::Backtrace::force_capture()` (used in the panic hook) always captures a backtrace regardless of the `RUST_BACKTRACE` environment variable — that is the point of `force_capture` vs `capture`. The env-var block therefore only affects the *default* Rust panic handler (`prev(info)` call at the end of the hook). This works as intended for the default handler, but the block is misleading and could be removed without changing what ends up in `panics.log`.
How can I resolve this? If you propose a fix, please make it concise. |
||
| .downcast_ref::<&str>() | ||
| .map(|s| (*s).to_string()) | ||
| .or_else(|| info.payload().downcast_ref::<String>().cloned()) | ||
| .unwrap_or_else(|| "<no message>".to_string()); | ||
| let backtrace = std::backtrace::Backtrace::force_capture(); | ||
| let thread = std::thread::current(); | ||
| let thread_name = thread.name().unwrap_or("<unnamed>").to_string(); | ||
| let timestamp = chrono::Utc::now().to_rfc3339(); | ||
| let pid = std::process::id(); | ||
|
|
||
| write_panic_record( | ||
| &panics_log, | ||
| ×tamp, | ||
| pid, | ||
| &thread_name, | ||
| &location, | ||
| &message, | ||
| &backtrace, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Replacing Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src-tauri/src/main.rs
Line: 182
Comment:
**Synchronous file writer in async context**
Replacing `tracing_appender::non_blocking` with a direct `RollingFileAppender` makes every `tracing` call block the calling thread until the write is flushed to disk. Inside Tokio worker threads (recording pipeline, export, preview rendering) this can stall the runtime and introduce latency spikes. If the goal is to ensure no logs are dropped on Windows, consider keeping `non_blocking` but explicitly flushing the guard on shutdown, or using `tracing_appender::non_blocking` with `lossy = false`.
How can I resolve this? If you propose a fix, please make it concise. |
||
| ); | ||
|
|
||
| tracing::error!( | ||
| target: "cap_desktop_panic", | ||
| location = %location, | ||
| thread = %thread_name, | ||
| message = %message, | ||
| backtrace = %backtrace, | ||
| "panic" | ||
| ); | ||
| eprintln!( | ||
| "[cap-desktop panic] thread '{thread_name}' at {location}: {message}\nbacktrace:\n{backtrace}" | ||
| ); | ||
| prev(info); | ||
| })); | ||
| } | ||
|
|
||
| fn write_panic_record( | ||
| path: &std::path::Path, | ||
| timestamp: &str, | ||
| pid: u32, | ||
| thread_name: &str, | ||
| location: &str, | ||
| message: &str, | ||
| backtrace: &std::backtrace::Backtrace, | ||
| ) { | ||
| use std::io::Write; | ||
| let Ok(mut file) = std::fs::OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(path) | ||
| else { | ||
| return; | ||
| }; | ||
| let _ = writeln!( | ||
| file, | ||
| "[{timestamp}] pid={pid} thread='{thread_name}' at {location}: {message}\n{backtrace}\n----" | ||
| ); | ||
| let _ = file.flush(); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently returns the panic payload as a user-facing
Err(String), which can leak internal details (paths, codec strings, etc.) into the UI.Consider returning a generic error and only sending the panic payload to logs/Sentry.