From 9363ec9d54ea0feb3c441c0b71773d324eb78b32 Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Thu, 14 May 2026 02:57:52 +0530 Subject: [PATCH] fix(si_server): idempotent start_session, suppress benign 'session already active' (#5J, #5H) (#1635) Signed-off-by: oxoxDev Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) --- src/openhuman/screen_intelligence/engine.rs | 76 ++++++++++++--------- src/openhuman/screen_intelligence/server.rs | 2 + src/openhuman/screen_intelligence/tests.rs | 45 ++++++++++++ 3 files changed, 90 insertions(+), 33 deletions(-) diff --git a/src/openhuman/screen_intelligence/engine.rs b/src/openhuman/screen_intelligence/engine.rs index 80381849e..b99361159 100644 --- a/src/openhuman/screen_intelligence/engine.rs +++ b/src/openhuman/screen_intelligence/engine.rs @@ -77,12 +77,13 @@ impl AccessibilityEngine { } } - if !spawned_new_session { - return Ok(self.status().await.session); + let status = self.status().await; + if spawned_new_session { + self.spawn_workers().await; + Ok(self.status().await.session) + } else { + Ok(status.session) } - - self.spawn_workers().await; - Ok(self.status().await.session) } pub async fn start_session( @@ -101,40 +102,49 @@ impl AccessibilityEngine { .unwrap_or(ScreenIntelligenceConfig::default().session_ttl_secs) .clamp(30, 3600); + let mut spawned_new_session = false; { let mut state = self.inner.lock().await; if state.session.is_some() { - return Err("session already active".to_string()); + tracing::debug!( + "[screen_intelligence] start_session requested while session already active" + ); + } else { + state.permissions = detect_permissions(); + if state.permissions.accessibility != PermissionState::Granted { + return Err("accessibility permission is not granted".to_string()); + } + + let screen_monitoring_requested = params.screen_monitoring.unwrap_or(true); + if screen_monitoring_requested + && state.permissions.screen_recording != PermissionState::Granted + { + return Err("screen recording permission is not granted".to_string()); + } + + let now = now_ms(); + let expires_at_ms = now + (ttl_secs as i64 * 1000); + state.features.screen_monitoring = screen_monitoring_requested; + + state.session = Some(new_session_runtime( + &state.config, + now, + expires_at_ms, + ttl_secs, + )); + state.last_event = Some("session_started".to_string()); + state.last_error = None; + spawned_new_session = true; } - - state.permissions = detect_permissions(); - if state.permissions.accessibility != PermissionState::Granted { - return Err("accessibility permission is not granted".to_string()); - } - - let screen_monitoring_requested = params.screen_monitoring.unwrap_or(true); - if screen_monitoring_requested - && state.permissions.screen_recording != PermissionState::Granted - { - return Err("screen recording permission is not granted".to_string()); - } - - let now = now_ms(); - let expires_at_ms = now + (ttl_secs as i64 * 1000); - state.features.screen_monitoring = screen_monitoring_requested; - - state.session = Some(new_session_runtime( - &state.config, - now, - expires_at_ms, - ttl_secs, - )); - state.last_event = Some("session_started".to_string()); - state.last_error = None; } - self.spawn_workers().await; - Ok(self.status().await.session) + let status = self.status().await; + if spawned_new_session { + self.spawn_workers().await; + Ok(self.status().await.session) + } else { + Ok(status.session) + } } pub async fn disable(&self, reason: Option) -> SessionStatus { diff --git a/src/openhuman/screen_intelligence/server.rs b/src/openhuman/screen_intelligence/server.rs index 6abd0d6ae..3838081f5 100644 --- a/src/openhuman/screen_intelligence/server.rs +++ b/src/openhuman/screen_intelligence/server.rs @@ -365,6 +365,8 @@ pub async fn start_if_enabled(app_config: &Config) { if let Err(e) = server.run(&config_for_run).await { if is_expected_benign_start_session_failure(&e) { info!("{LOG_PREFIX} embedded server autostart skipped: {e}"); + } else if e.contains("session already active") { + info!("{LOG_PREFIX} embedded server session already active (benign)"); } else { error!("{LOG_PREFIX} embedded server exited with error: {e}"); } diff --git a/src/openhuman/screen_intelligence/tests.rs b/src/openhuman/screen_intelligence/tests.rs index 701127ab7..8771e2aae 100644 --- a/src/openhuman/screen_intelligence/tests.rs +++ b/src/openhuman/screen_intelligence/tests.rs @@ -844,3 +844,48 @@ async fn analyze_and_persist_frame_rejects_disabled_local_ai() { "unexpected error when local ai is disabled: {err}" ); } + +#[tokio::test] +async fn start_session_is_idempotent() { + if !cfg!(target_os = "macos") { + return; + } + + let engine = Arc::new(AccessibilityEngine { + inner: Mutex::new(EngineState::new(ScreenIntelligenceConfig { + baseline_fps: 6.0, + session_ttl_secs: 60, + ..Default::default() + })), + }); + + // First start + let started = engine + .start_session(StartSessionParams { + consent: true, + ttl_secs: Some(60), + screen_monitoring: Some(true), + }) + .await; + + if started.is_err() { + // If we can't start the first session (e.g. no permissions in test env), + // we can't test idempotency properly here, but it shouldn't fail the test. + return; + } + + // Second start - should succeed and return the same session status (active: true) + let second_start = engine + .start_session(StartSessionParams { + consent: true, + ttl_secs: Some(60), + screen_monitoring: Some(true), + }) + .await; + + assert!(second_start.is_ok(), "Second start_session should be Ok"); + assert!( + second_start.unwrap().active, + "Second start_session should return active session status" + ); +}