fix: flush memory persistence on channel shutdown for short-lived conversations#401
fix: flush memory persistence on channel shutdown for short-lived conversations#401l33t0 wants to merge 2 commits intospacedriveapp:mainfrom
Conversation
Short-lived conversations (common on Discord and Telegram) that never reach the message_interval threshold (default: 50 messages) would lose their context entirely because memory persistence only triggered at the interval boundary. Adds force_memory_persistence() which spawns a memory persistence branch unconditionally when a channel stops, ensuring any unsaved conversation context is committed regardless of message count. Also fixes pre-existing clippy collapsible_if lint in signal adapter. Closes spacedriveapp#363
|
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 (1)
WalkthroughAdds unconditional memory persistence on channel shutdown to persist unsaved conversation context, and refactors Signal adapter shorthand handling in message routing using a Rust let-guard to streamline control flow without changing behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/channel.rs (1)
1024-1030: Add a shutdown regression for below-threshold persistence.This fix only runs in teardown, but the tests in this module still exercise event routing rather than channel shutdown. A focused test around
message_count > 0andmemory_persistence.enabledwould keep this from regressing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1024 - 1030, Teardown now persists unsaved context only when self.message_count > 0 by calling self.force_memory_persistence(), but there is no targeted test to prevent regressions; add a focused unit test for the Channel (or agent) shutdown path that sets memory_persistence.enabled=true, sets message_count to a value > 0, invokes the channel shutdown/teardown sequence, and asserts that force_memory_persistence() (or the underlying persistence side-effect) was called; additionally add a complementary test with message_count == 0 to ensure no persistence occurs, and wire the test to observe the actual persistence effect (mock the memory persistence component or inspect persisted state) rather than relying on event routing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/agent/channel.rs`:
- Around line 1024-1030: Teardown now persists unsaved context only when
self.message_count > 0 by calling self.force_memory_persistence(), but there is
no targeted test to prevent regressions; add a focused unit test for the Channel
(or agent) shutdown path that sets memory_persistence.enabled=true, sets
message_count to a value > 0, invokes the channel shutdown/teardown sequence,
and asserts that force_memory_persistence() (or the underlying persistence
side-effect) was called; additionally add a complementary test with
message_count == 0 to ensure no persistence occurs, and wire the test to observe
the actual persistence effect (mock the memory persistence component or inspect
persisted state) rather than relying on event routing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c32f8451-020d-436b-a759-1b5bd7f804c8
📒 Files selected for processing (2)
src/agent/channel.rssrc/tools/send_message_to_another_channel.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| self.message_count = 0; | ||
|
|
||
| match spawn_memory_persistence_branch(&self.state, &self.deps).await { | ||
| Ok(branch_id) => { | ||
| self.memory_persistence_branches.insert(branch_id); |
There was a problem hiding this comment.
If spawn_memory_persistence_branch fails, clearing message_count here drops the signal that there was unsaved context. Consider only zeroing the counter after a successful spawn.
| self.message_count = 0; | |
| match spawn_memory_persistence_branch(&self.state, &self.deps).await { | |
| Ok(branch_id) => { | |
| self.memory_persistence_branches.insert(branch_id); | |
| match spawn_memory_persistence_branch(&self.state, &self.deps).await { | |
| Ok(branch_id) => { | |
| self.message_count = 0; | |
| self.memory_persistence_branches.insert(branch_id); |
| } | ||
|
|
||
| #[test] | ||
| fn memory_persistence_config_defaults_to_enabled() { |
There was a problem hiding this comment.
These new tests are only asserting config fields (or even just config.enabled) and don’t exercise the shutdown flush behavior added above. I’d either drop them (to avoid a false sense of coverage) or replace with a regression that drives the channel shutdown path and asserts a memory-persistence branch is spawned when message_count > 0 and memory_persistence.enabled.
1 participant
Summary
Short-lived conversations on Discord and Telegram were losing their context because memory persistence only triggers every N messages (default: 50). Conversations that ended before reaching the threshold never got their context saved to agent memory.
The root cause is NOT adapter-specific - all adapters share the same channel code path. The issue is that the periodic trigger is the only mechanism, and there was no flush on channel close.
force_memory_persistence()that spawns a memory persistence branch unconditionally (bypassing the message_interval threshold)message_count > 0)memory_persistence.enabledconfig flagAlso fixes a pre-existing clippy
collapsible_iflint in the Signal adapter code.Closes #363
Test plan
cargo clippy --all-targets -- -D warningspasses cleanNote
Key Changes: Adds channel shutdown hook that flushes memory persistence for short-lived conversations. The
force_memory_persistence()method spawns a memory persistence branch unconditionally on channel close if there are unsaved messages. Also refactors nested conditionals in Signal adapter code using let-guards for improved readability and Clippy compliance.Written by Tembo for commit d490b4a. This will update automatically on new commits.