Repository navigation
feat(memory-sync): wake Composio periodic loop on resume + gate on per-source toggle (#2831) - #3417
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdd a process-wide resume Notify and fire it when the scheduler gate unpauses (including sign-in). Make the Composio periodic loop wait on either its interval or the resume Notify, reset cadence on resume, and use a per-tick snapshot plus decide_periodic_source to gate each Composio connection by registry-enabled state or toolkit defaults. Tests added for notify behavior and periodic decision logic. ChangesResume Notification and Periodic Gating
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/openhuman/scheduler_gate/gate.rs (1)
285-287: ⚡ Quick winLog when the gate actually emits the resume signal.
These are the new wakeup branches, but nothing with the
[scheduler_gate]prefix currently says thatnotify_one()fired or which transition caused it. Adebug!breadcrumb here would make resume-latency issues diagnosable.As per coding guidelines, "Include debug logging with verbose diagnostics on all new/changed flows: log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors with stable grep-friendly prefixes (
[domain],[rpc])."Also applies to: 355-357
🤖 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/scheduler_gate/gate.rs` around lines 285 - 287, Add a debug log right before/after calling resume_notify().notify_one() to record that the gate emitted a resume signal and which transition triggered it; specifically in the branch that checks the resumed boolean (the resumed variable and the resume_notify() call) emit a debug! with the `[scheduler_gate]` prefix and include the transition name/identifier and resumed=true context. Apply the same pattern to the other wakeup branch around the second resume_notify().notify_one() (the code referenced at the other location) so both wakeup paths log a concise, grep-friendly breadcrumb when notify_one() is invoked.Source: Coding guidelines
src/openhuman/memory_sync/composio/periodic.rs (1)
298-305: ⚡ Quick winLog the resume-notify branch before rebasing the ticker.
This is the new behavior change, but there's no
[composio:periodic]breadcrumb showing whether a tick came from the 20-minute timer or fromresume_notify(). Adebug!here would make wakeups and burst-prevention behavior observable.As per coding guidelines, "Include debug logging with verbose diagnostics on all new/changed flows: log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors with stable grep-friendly prefixes (
[domain],[rpc])."🤖 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/memory_sync/composio/periodic.rs` around lines 298 - 305, Add a debug log entry in the resume-notified branch so wakeups are observable: inside the tokio::select! branch that matches resume.notified() (before calling ticker.reset()), emit a debug! with a stable grep-friendly prefix like "[composio:periodic]" and a message noting the branch (e.g. "resume-notify wakeup - rebasing ticker") and any useful context (timestamp or task id if available). Locate the resume.notified() match and insert the debug logging immediately before calling ticker.reset() to ensure the log records that this wakeup came from resume_notify() rather than the periodic tick.Source: Coding guidelines
🤖 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/openhuman/memory_sync/composio/periodic.rs`:
- Around line 456-471: The current code builds composio_sources from
list_sources().await.unwrap_or_default(), which masks errors and effectively
treats failures as "all sources enabled"; instead, use the already-loaded config
snapshot for this tick (config.memory_sources) to build the index: iterate over
config.memory_sources, filter entries where kind == SourceKind::Composio, map
entries with a connection_id to (connection_id.clone(), MemorySourceEntry) and
collect into the HashMap used by composio_sources so that decide_periodic_source
gets explicit per-source state rather than defaulting on a failed registry read;
update the variable/comprehension that defines composio_sources and remove
reliance on list_sources() for this snapshot.
---
Nitpick comments:
In `@src/openhuman/memory_sync/composio/periodic.rs`:
- Around line 298-305: Add a debug log entry in the resume-notified branch so
wakeups are observable: inside the tokio::select! branch that matches
resume.notified() (before calling ticker.reset()), emit a debug! with a stable
grep-friendly prefix like "[composio:periodic]" and a message noting the branch
(e.g. "resume-notify wakeup - rebasing ticker") and any useful context
(timestamp or task id if available). Locate the resume.notified() match and
insert the debug logging immediately before calling ticker.reset() to ensure the
log records that this wakeup came from resume_notify() rather than the periodic
tick.
In `@src/openhuman/scheduler_gate/gate.rs`:
- Around line 285-287: Add a debug log right before/after calling
resume_notify().notify_one() to record that the gate emitted a resume signal and
which transition triggered it; specifically in the branch that checks the
resumed boolean (the resumed variable and the resume_notify() call) emit a
debug! with the `[scheduler_gate]` prefix and include the transition
name/identifier and resumed=true context. Apply the same pattern to the other
wakeup branch around the second resume_notify().notify_one() (the code
referenced at the other location) so both wakeup paths log a concise,
grep-friendly breadcrumb when notify_one() is invoked.
🪄 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: 346ecc51-7203-47f0-a03d-ecd11b4acb2a
📒 Files selected for processing (2)
src/openhuman/memory_sync/composio/periodic.rssrc/openhuman/scheduler_gate/gate.rs
…r-source toggle (tinyhumansai#2831) Pause via the scheduler gate (tinyhumansai#2825) left up to 20 min resume latency: when Memory Tree is toggled back on (or the user signs back in), the periodic loop kept its original interval cadence and the next real tick could be ~20 min away. - Add a process-wide resume `Notify` to `scheduler_gate::gate`, fired on transitions out of `Policy::Paused` in `update_config` (Memory Tree toggled on) and on `set_signed_out` true->false (sign-in). Over-notifying is safe: the woken tick re-checks `current_policy()` and no-ops if still paused. - `periodic::run_loop` now `select!`s over the ticker and the resume notify; on wake it runs a tick immediately and `reset()`s the ticker so rapid toggles don't bunch-fire. - Gate the periodic loop on the per-source `enabled` toggle: a source switched off now stops syncing in the background (matching the manual sync paths, which already honour `enabled`). Previously the loop read the registry only for caps and synced disabled sources anyway, uncapped. No registry row yet -> sync with conservative per-toolkit defaults (never uncapped, never skip). Tests: decide_periodic_source (disabled skip / enabled caps / default caps), resume_notify singleton + parked-waiter wake. Closes tinyhumansai#2831 Co-Authored-By: Claude <noreply@anthropic.com>
tinyhumansai#2831) Seeds STATE directly (the test module can reach it) and drives a deterministic Off -> AlwaysOn transition plus set_signed_out true->false, asserting each fires the resume notify. Lifts changed-line diff coverage on the touched files above the 80% gate (gate.rs to 100%). Co-Authored-By: Claude <noreply@anthropic.com>
…oolkits (tinyhumansai#2831) tinyhumansai#3443 (merged from upstream) added multiple account connections per toolkit. The periodic loop already handles this correctly because it keys the per-source gate on connection_id (not toolkit) and walks every connection — so two accounts of the same toolkit are gated independently. Add a regression test pinning that connection-id keying: enabled account syncs with its caps, a disabled sibling account is skipped, an unregistered sibling falls back to bounded defaults. Co-Authored-By: Claude <noreply@anthropic.com>
…ig snapshot (tinyhumansai#2831) CodeRabbit review: `list_sources().await.unwrap_or_default()` did a second config read per tick and collapsed any transient load error into an empty map — which made every disabled source fall through to the `decide_periodic_source(None, ..)` default-caps path, silently re-enabling background sync for sources the user switched off. Build the index from the already-loaded `config.memory_sources` snapshot instead: fail-closed (a disabled row stays disabled even if a read would have failed) and one fewer config load. Co-Authored-By: Claude <noreply@anthropic.com>
a2bf8db to
886b350
Compare
…r-source toggle (tinyhumansai#2831) (tinyhumansai#3417)\n\n
Summary
Notifytoscheduler_gate::gate, fired when the gate transitions out ofPolicy::Paused— Memory Tree toggled back on (update_config) or sign-in (set_signed_outtrue→false).composio::periodic::run_loopnowselect!s over the 20-min ticker and the resume notify, so background sync restarts within seconds instead of at the next ≤20-min boundary. On wake it runs a tick immediately andreset()s the ticker so rapid toggles don't bunch-fire.enabledtoggle: a source switched off now stops syncing in the background (matching the manual sync paths). No registry row yet → sync with conservative per-toolkit defaults (never uncapped, never skipped).Original issue request (#2831)
feat(memory-sync): wake Composio periodic loop early via Notify when Memory Tree toggled back on — click to expand the original ask
How this PR maps to the acceptance criteria:
scheduler_gate::gate::resume_notify()fired fromupdate_configon un-pause;run_loopselect!s on it and runs a tick immediatelyticker.reset()on the wake arm +notify_one()'s single stored permit collapse repeated wakes to one tickperiodic_pause_reason()Step-0 check unchanged; woken ticks re-check it and no-op if still pausedset_signed_out(true→false)firesresume_notify()Problem
#2825 paused the periodic Composio loop via a scheduler-gate check, but resume latency was up to 20 min: the
tokio::time::intervalkept its cadence and each paused tick was a fast no-op, so re-enabling Memory Tree (or signing back in) didn't sync until the next boundary. Painful for onboarding, dev iteration, and re-login recovery (the explicit follow-up #2825 asked for).Separately, while tracing the loop: the periodic scheduler gated on the Composio connection status (
is_active()) and consulted thememory_sourcesregistry only for caps (.unwrap_or((None, None))). A source the user toggled off therefore kept syncing in the background — and uncapped (None= uncapped). The manual paths (memory_sources::sync_source,memory_sources_sync_all) already early-return on!enabled; the periodic loop was the only sync entry point that ignored the toggle.Solution
scheduler_gate/gate.rs): newRESUME_NOTIFYsingleton +resume_notify().update_configfiresnotify_one()on a paused→running transition;set_signed_outfires on true→false. Over-notifying is safe by design — the woken tick re-checkscurrent_policy()and no-ops if still effectively paused, so we avoid computing the precise combined (config × signed-out) edge.composio/periodic.rs):tokio::select!overticker.tick()andresume.notified();ticker.reset()on the wake arm.notify_one's single stored permit + the reset bound rapid toggles to one extra tick.list_sources, indexed by connection id) and resolve via a puredecide_periodic_sourcehelper — disabled → skip, enabled → its caps, no-row →memory_sync_defaults_for_toolkit(bounded). The no-match path defaults to sync-bounded, never skip, so a missing/mismatched row degrades safely instead of going dark.Submission Checklist
decide_periodic_source(disabled-skip / enabled-caps / default-caps),resume_notifysingleton + parked-waiter wake.cargo-llvm-cov+diff-coveragainstupstream/main: 85% total (gate.rs 100%, periodic.rs 73.9%). Passes the gate.periodic.rsmisses are therun_loopselect!+ the connection-loop body (only reachable with a live Composio client / the infinite loop); the decision logic they wrap is unit-tested.docs/TEST-COVERAGE-MATRIX.md(likelyN/A: behaviour-only change).Closes #NNN.Impact
Validation status
Automated:
cargo-llvm-cov+diff-cover→ 85% diff coverage on changed lines (≥80% gate passes; gate.rs 100%, periodic.rs 73.9%). Therun_loopselect!arm and the per-connection loop body are integration-level (live Composio client / infinite loop) and not reachable from lib unit tests; the decision logic they wrap (decide_periodic_source,effective_interval_secs,connection_is_due) is unit-tested, including a multi-account regression test.Manual smoke (not run — deferred): behaviours to verify on a live signed-in instance — Memory Tree off→on resumes a Composio tick within seconds (not at the 20-min boundary); rapid off-on-off-on doesn't bunch-fire; sign-out→in wakes the loop; a single source toggled off stops background sync while others continue (manual sync still works).
Related
AI Authored PR Metadata
Linear Issue
Commit & Branch
feat/2831-composio-autosync-wakeValidation Run
pnpm --filter openhuman-app format:check— N/A (no app/ changes)pnpm typecheck— N/A (no app/ changes)cargo test --libperiodic + scheduler_gate modules (22 + 7 pass)cargo fmt --checkclean;cargo check --lib+--testscleanBehavior Changes
Parity Contract
effective_interval_secs/connection_is_due) unchanged; pause short-circuit (periodic_pause_reason) unchanged.Summary by CodeRabbit
New Features
Bug Fixes
Tests