fix(composio): keep chat integrations cache in sync on Windows#776
Conversation
…umansai#749) On Windows the `ComposioConnectionCreated` → `wait_for_connection_active` invalidation path often misses: the 60 s readiness poll can expire before the user finishes the hosted OAuth flow (Defender SmartScreen, slower browser launch, extra consent dialogs), so `invalidate_connected_integrations_cache` never fires and the chat runtime stays frozen on the pre-connect snapshot while Settings correctly shows "Connected" via its 5 s `listConnections` poll. Layer two cross-platform defenses on top of the existing event-bus path: 1. `composio_list_connections` (the RPC the UI polls every 5 s) now diffs the backend's active-toolkit set against every cached entry and invalidates on divergence, so chat converges to truth within one poll interval — regardless of how the OAuth round-trip completed. 2. Add a 60 s TTL on the `INTEGRATIONS_CACHE` so stale entries can't live forever even if nothing polls. 3. Grep-friendly `[composio][integrations]` logs at every decision point (cache hit / miss / expiry / divergence diff) for quick diagnosis from user-supplied debug dumps. Also covers the regression with six new unit tests (activate, disconnect, steady state, non-active rows, CONNECTED vs ACTIVE alias, TTL expiry) serialized via a shared test mutex so they don't race the existing mock-backend tests over the process-wide cache.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Review: ✅ Looks goodPR #776 — fix(composio): keep chat integrations cache in sync on Windows Verification
What was checked
Minor notes
|
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Code Review: fix(composio): keep chat integrations cache in sync on Windows
Thorough line-by-line review of the diff. Clean, well-motivated fix with good defensive layering. Approving with a few non-blocking suggestions in inline comments.
What works well
- Root cause analysis is excellent. The PR description traces the exact failure mode (Windows OAuth exceeding the 60s
wait_for_connection_activewindow) and explains why the Settings UI shows "Connected" while the chat cache is stale. - Two-layer defense is sound architecture. UI-poll-driven sync (converges within one 5s poll) and defensive TTL (caps CLI/background staleness at 60s) are complementary.
- Lock discipline is correct.
sync_cache_with_connectionsreads under a read lock, collects divergent keys, drops the lock, then takes a write lock to remove. No lock held across network calls. - Tests are deterministic. TTL test ages the entry by rewriting
cached_atrather than sleeping.CACHE_TEST_GUARDprevents races over the process-wide map. - Debug logging is grep-friendly with consistent
[composio][integrations]prefixes, structured fields, and no PII. - Test helper
conn()usingserde_json::from_valueis a smart choice — decouples tests from struct field additions.
Summary of inline comments
- Suggestion — File size (~1560 lines, 3x guideline). Consider extracting cache layer into
cache.rsas follow-up. - Suggestion —
invalidate_connected_integrations_cachelog level bumped fromdebugtoinfo— intentional for the happy-path bus subscriber call? - Nitpick —
live_active.clone()insidefilter_map— minor allocation, harmless in practice. - Nitpick — Static docstring on
INTEGRATIONS_CACHEnot updated to mention TTL. - Nitpick — Subtle cross-test sync side-effect protected by
CACHE_TEST_GUARD— worth a comment.
All non-blocking. LGTM.
|
|
||
| use crate::openhuman::context::prompt::{ConnectedIntegration, ConnectedIntegrationTool}; | ||
| use std::collections::HashMap; | ||
| use std::collections::{HashMap, HashSet}; |
There was a problem hiding this comment.
🟡 Suggestion — file size: With this PR, ops.rs grows to ~1560 lines (~3x the project's preferred ≤500-line guideline from CLAUDE.md). The file already held RPC handlers, cache logic, prompt integration discovery, and a substantial test module before this PR.
Not blocking for this fix — the new code logically belongs here. But as a follow-up, consider extracting the cache/sync layer (CachedIntegrations, INTEGRATIONS_CACHE, sync_cache_with_connections, fetch_connected_integrations, connected_toolkit_set, invalidate_*) into a sibling cache.rs file within the composio/ module. The test module for the cache tests could move with it. This would bring both files comfortably under ~800 lines and make the cache behavior easier to audit independently.
| /// Clears the entire map because the bus subscriber doesn't carry a | ||
| /// config reference. | ||
| /// new OAuth connection completes, by [`composio_list_connections`] | ||
| /// when it observes a divergence between the backend response and the |
There was a problem hiding this comment.
🟢 Nitpick: The docstring on the INTEGRATIONS_CACHE static (line 445-448 in the pre-image) still says "until explicitly invalidated" — should also mention the TTL and the UI-poll sync path for completeness. The old text didn't get updated in this PR.
(The extensive docstring on CACHE_TTL covers the full story, so this is just a consistency polish.)
| /// cached snapshot, and from tests. Clears the entire map because the | ||
| /// callers don't carry a config reference. | ||
| pub fn invalidate_connected_integrations_cache() { | ||
| if let Ok(mut guard) = INTEGRATIONS_CACHE.write() { |
There was a problem hiding this comment.
💬 Question — log level: The existing invalidate_connected_integrations_cache log was debug and is now info. This is called from the event-bus subscriber on every successful OAuth (the happy path on macOS/Linux). Was the bump to info intentional?
info is fine for the new sync_cache_with_connections divergence path (that's a notable state change worth surfacing at runtime). But for the bus subscriber's normal invalidation, debug might be more appropriate to avoid noise in production logs. Consider keeping the full-wipe path at debug and the divergence-detected path at info.
| let Ok(guard) = INTEGRATIONS_CACHE.read() else { | ||
| return; | ||
| }; | ||
| guard |
There was a problem hiding this comment.
🟢 Nitpick — allocation: live_active.clone() is called inside the filter_map closure, so it clones the full HashSet once per divergent cache key. In practice the cache usually has one key (single user context), so this is harmless. But if you want to tighten it, you could move the live_active reference comparison outside the closure and collect divergent keys as just Vec<String>, then compute the diff sets only in the write-lock block where you log. Minor — the current approach is perfectly readable.
| } | ||
|
|
||
| // ── Windows-observed sync regression coverage (issue #749) ──── | ||
| // |
There was a problem hiding this comment.
🟢 Nitpick — test isolation: The six new regression tests seed the cache with literal string keys ("windows-regression-1" through "windows-regression-6") and use clear_cache_key for surgical cleanup — good. But sync_cache_with_connections compares live_active against every entry in the cache map, not just the test's key. If a parallel mock-backend test (e.g. fetch_connected_integrations_via_mock_aggregates_tools) happens to have populated a cache entry with a different connected-toolkit set, the sync function could invalidate that entry too.
The CACHE_TEST_GUARD mutex prevents this from being a real problem today, but the invariant is subtle. A brief comment in the test module docstring noting that the guard also protects against cross-test sync side effects would help future maintainers.
2 participants
Closes #749.
Summary
On Windows, integrations show as Connected in Settings but the chat agent keeps reporting them as "not connected" / unavailable. The root cause is a cache-invalidation miss that's much more likely on Windows than on macOS/Linux:
composio_authorizepublishesComposioConnectionCreatedas soon as the backend returns aconnectUrl.ComposioConnectionCreatedSubscriberthen pollswait_for_connection_activefor up to 60 s, and only callsinvalidate_connected_integrations_cache()if it observesACTIVE/CONNECTEDwithin that window.listConnections()every 5 s (app/src/lib/composio/hooks.ts) and shows the badge correctly from the fresh backend response — but that path never touches the RustINTEGRATIONS_CACHE, so the chat runtime stays frozen on the pre-connect snapshot indefinitely.This PR layers two cross-platform defenses on top of the existing event-bus path, without changing the happy-path behavior:
composio_list_connectionsnow diffs the backend's active-toolkit set against every entry inINTEGRATIONS_CACHEand invalidates diverged entries. Because the UI polls every 5 s, the chat cache converges to truth within one poll interval regardless of how the OAuth completed or which OS the user is on.CACHE_TTL = 60 scaps the worst-case staleness even when nothing polls (CLI-only flows, backgrounded app).Also adds grep-friendly
[composio][integrations]logs at every decision point — cache hit / miss / TTL expiry / divergence diff with added+removed toolkit sets — so the next Windows regression can be traced from a user-supplied debug dump without needing a live repro.Changes
src/openhuman/composio/ops.rs:CachedIntegrations { entries, cached_at }struct;INTEGRATIONS_CACHEnow keys by config path → timestamped entry.sync_cache_with_connections()called fromcomposio_list_connections— O(n) set diff, no lock held across the decision.fetch_connected_integrations.CACHE_TEST_GUARDmutex across all cache-touching tests so the new tests don't race the existing mock-backend tests over the process-wide map.Test plan
cargo test --lib -p openhuman composio::— all 254 tests pass (31 incomposio::ops::testsincluding the 6 new regression tests).cargo fmt,cargo clippy --lib -p openhuman --no-deps— no new warnings from this PR.delegate_gmailon the next turn without restart.Acceptance criteria (from #749)
Summary by CodeRabbit
Improvements
Tests
Closes #749