fix(si_server): idempotent start_session, suppress benign 'session already active' (#5J, #5H)#1635
Conversation
- Modified `AccessibilityEngine::start_session` and `enable` to be idempotent, returning the existing session status instead of an error when a session is already active. - Added a unit test `start_session_is_idempotent` to verify the new behavior. - Updated `src/core/jsonrpc.rs` to suppress Sentry error reporting for the benign "session already active" condition. - Updated `src/openhuman/screen_intelligence/server.rs` to log "session already active" at `info` level instead of `error` in the embedded server loop. Closes OPENHUMAN-TAURI-5J, OPENHUMAN-TAURI-5H. Co-authored-by: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Signed-off-by: oxoxDev <nikhil@tinyhumans.ai>
📝 WalkthroughWalkthroughThe PR makes session starts idempotent in the Screen Intelligence system by refactoring ChangesSession Idempotency for Screen Intelligence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/screen_intelligence/tests.rs (1)
862-890: ⚡ Quick winStrengthen the idempotency assertion to verify session reuse.
Right now the test only checks
Ok + active; it would still pass if a second call recreated a new session. Assert stable identity fields from first and second results (e.g.,started_at_ms/expires_at_ms) to lock in true idempotency behavior.Proposed test hardening
- let started = engine + let first_start = engine .start_session(StartSessionParams { consent: true, ttl_secs: Some(60), screen_monitoring: Some(true), }) .await; - if started.is_err() { + let first = match first_start { + Ok(s) => s, + 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; - } + } + }; @@ - let second_start = engine + let second = 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, + .await + .expect("Second start_session should be Ok"); + assert!( + second.active, "Second start_session should return active session status" ); + assert_eq!( + second.started_at_ms, first.started_at_ms, + "Second start_session should reuse existing session" + ); + assert_eq!( + second.expires_at_ms, first.expires_at_ms, + "Second start_session should keep existing session expiry" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/screen_intelligence/tests.rs` around lines 862 - 890, The test currently only asserts that the second call to engine.start_session(StartSessionParams { ... }) returns Ok and active, which doesn't ensure the same session was reused; modify the test to capture the first successful result (started.unwrap()) and the second result (second_start.unwrap()), then assert that stable identity/timing fields match (for example compare started.started_at_ms and second.started_at_ms and started.expires_at_ms and second.expires_at_ms) to ensure idempotent reuse rather than creation of a new session.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/jsonrpc.rs`:
- Around line 114-119: The JSON-RPC transport (rpc_handler) contains
domain-specific branching using is_benign_si_error and special-case logging;
remove that domain logic and instead rely on StructuredRpcError metadata (e.g.,
expected_user_state) emitted by controllers/handlers. Replace the
is_benign_si_error checks in src/core/jsonrpc.rs with generic handling that
inspects StructuredRpcError metadata on the error returned from
rpc_handler/handler invocation and forwards that metadata to the caller/logging
without embedding Screen Intelligence logic in rpc transport; update
callers/handlers to set expected_user_state where needed and ensure rpc_handler
only reads generic metadata fields from StructuredRpcError.
---
Nitpick comments:
In `@src/openhuman/screen_intelligence/tests.rs`:
- Around line 862-890: The test currently only asserts that the second call to
engine.start_session(StartSessionParams { ... }) returns Ok and active, which
doesn't ensure the same session was reused; modify the test to capture the first
successful result (started.unwrap()) and the second result
(second_start.unwrap()), then assert that stable identity/timing fields match
(for example compare started.started_at_ms and second.started_at_ms and
started.expires_at_ms and second.expires_at_ms) to ensure idempotent reuse
rather than creation of a new session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e2d7416-20f0-43fe-bc2e-18f5074bd024
📒 Files selected for processing (4)
src/core/jsonrpc.rssrc/openhuman/screen_intelligence/engine.rssrc/openhuman/screen_intelligence/server.rssrc/openhuman/screen_intelligence/tests.rs
…hrough expected_user_state (tinyhumansai#1635 CR) The screen_intelligence engine's start_session is now idempotent (returns the existing session on duplicate start, locked by start_session_is_idempotent test), so no RPC controller path surfaces "session already active" as an Err to the transport layer anymore. Removing the SI-specific branch keeps src/core/jsonrpc.rs free of domain logic per openhuman/CLAUDE.md. The embedded-autostart log demote in screen_intelligence/server.rs:368 is untouched — that fires at process log level, not RPC. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in 73739f2: moved the 'session already active' skip-Sentry decision out of Since So the right fix per The embedded-autostart log demote in Verified locally:
|
2 participants
Summary
AccessibilityEngine::start_sessionandenableidempotent: a duplicate start now returns the existing session status instead of erroring.src/core/jsonrpc.rsrpc-error funnel andsrc/openhuman/screen_intelligence/server.rsembedded autostart loop).start_session_is_idempotentlocking the new behaviour.Problem
error!level on every duplicate-start.feedback_validation_test_targetand the existingis_expected_macos_only_failuregate inserver.rs, the right pattern here is the same: detect the benign condition and demote.Solution
start_session/enable(src/openhuman/screen_intelligence/engine.rs): on duplicate start, return the currentSessionStatusasOk(...)instead ofErr("session already active"). The wire shape stays the same for callers that handle success; the legacy "already active" error string is preserved on the small set of remaining edge paths so the suppression check below catches them.src/core/jsonrpc.rs: addis_benign_si_error(msg)(mirrorsis_session_expired_errorshape). The post-fix(threads): typed ThreadNotFound error + skip Sentry for stale-thread RPCs (OPENHUMAN-TAURI-4H, -60) #1570report_error_or_expectedchain gets a newelse ifbranch — benign SI errors emittracing::info!with the method tag instead ofreport_error.src/openhuman/screen_intelligence/server.rs: the embedded-server autostart loop, which already has theis_expected_macos_only_failurefilter from cluster J, now also demotese.contains(\"session already active\")toinfo!.src/openhuman/screen_intelligence/tests.rsaddsstart_session_is_idempotent— callsstart_sessiontwice, asserts second call returnsOkwith the same session id.Submission Checklist
start_session_is_idempotentcovers the happy path; existingpanic_stop_behavior_stops_sessionandsession_lifecycle_transitions_and_ttl_expirycover the error/edge paths.structured_rpc_error_envelope_passes_through_generic_dispatch).Closesin## Related.Impact
Ok/Errsemantics preserved; only the level of the log + Sentry emission changes for the benign case.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Summary by CodeRabbit
Bug Fixes
Tests