Docs: turbopuffer ContainsAnyToken vs Terraphim graph/embedding search

[AlexMikhalev]

Adds a short design note mapping turbopuffer's ContainsAnyToken (token-level OR filter) to Terraphim's multi-term OR filtering and explains how that differs from TerraphimGraph/rolegraph concept matching.\n\nRefs: https://turbopuffer.com/docs/query#param-ContainsAnyToken

[claude]

Code Review: PR #489

Overview

This PR implements substantial functionality for the terraphim_rlm crate (~5,000 LOC added), including session management, query loops, validation, logging, and MCP tools integration. While the PR title suggests documentation, this is actually a major feature implementation.

---

🎯 Critical Issues

1. PR Title/Scope Mismatch

Title: "Docs: turbopuffer ContainsAnyToken vs Terraphim graph/embedding search"
Reality: Major implementation of RLM system with 6 new modules (4,729 LOC) + snapshot management enhancements

Recommendation: Retitle to something like:

feat(rlm): implement session-aware RLM with query loops, validation, and MCP tools

- Add session management with snapshot/rollback support
- Implement query loop orchestration (LLM → parse → execute → feedback)
- Add knowledge graph validation with strictness levels
- Implement trajectory logging for debugging
- Add MCP tools integration (search, VM management)
- Include turbopuffer query parity documentation

Phase 3-4 of terraphim_rlm implementation (54 tests passing)

2. PR Should Be Split

For easier review and safer merging, consider splitting into:

• PR 1: Firecracker snapshot management enhancements (firecracker.rs)
• PR 2: Core RLM orchestration (rlm.rs, session.rs, query_loop.rs)
• PR 3: Validation & logging (validator.rs, logger.rs)
• PR 4: MCP tools & parser (mcp_tools.rs, parser.rs)
• PR 5: Documentation (turbopuffer-query-parity.md)

---

✅ Strengths

1. Excellent Test Coverage

   • 60 test functions added (#[tokio::test])
   • Good mix of unit and integration tests
   • Tests cover snapshot management, session lifecycle, validation

2. Strong Documentation

   • Comprehensive module-level doc comments
   • Good inline documentation
   • Architecture diagrams in comments
   • Clear examples in doc strings

3. Safety & Error Handling

   • No unsafe blocks - excellent safety practices
   • Proper use of Result types throughout
   • Good error propagation with ? operator
   • Custom error types with RlmError

4. Good Rust Idioms

   • Proper use of Arc/RwLock for shared state
   • Async/await patterns with tokio
   • Type-safe session IDs with newtype pattern
   • Proper trait abstractions

---

⚠️ Issues to Address

Code Quality

TODO Comments (3 found - should track these):

// crates/terraphim_rlm/src/executor/firecracker.rs
// TODO: Create actual VmPoolManager with VmManager
// TODO: Call snapshot_manager.list_snapshots() when we have mutable access
// TODO: Stop and cleanup VMs via VmManager

Action: Create GitHub issues for these TODOs and reference them.

Panic in Test Code:

// Found in tests
_ => panic\!("Expected QueryLlmBatched"),

Recommendation: Use assert\! or matches\! instead for better test failure messages:

assert\!(matches\!(result, QueryLlmBatched(_)), "Expected QueryLlmBatched");

Architecture & Design

1. Snapshot Management API Change
The trait signature change from:

async fn create_snapshot(&self, name: &str) -> Result<SnapshotId>

to:

async fn create_snapshot(&self, session_id: &SessionId, name: &str) -> Result<SnapshotId>

is a breaking change.

Recommendation:

• Document this in CHANGELOG
• Verify all implementations are updated
• Consider deprecation path if there are other implementations

2. Duplicate Snapshot Name Check

if session_snapshots.iter().any(|s| s.name == name) {
    return Err(RlmError::SnapshotCreationFailed { ... });
}

Good catch! This prevents data corruption from name collisions.

3. Rollback Implementation (firecracker.rs:217-239)
The rollback logic looks solid:

• Checks for current snapshot
• Logs warnings appropriately
• Graceful degradation if no snapshot exists

Minor suggestion: Consider returning a RollbackResult enum:

pub enum RollbackResult {
    Restored(SnapshotId),
    NoSnapshotAvailable,
}

This makes the no-op case explicit to callers.

Performance

Lock Contention (firecracker.rs:52-57):

snapshots: parking_lot::RwLock<HashMap<SessionId, Vec<SnapshotId>>>,
session_vms: parking_lot::RwLock<HashMap<SessionId, String>>,
current_snapshot: parking_lot::RwLock<HashMap<SessionId, SnapshotId>>,

Analysis: Three separate locks for session-related data. Under high concurrency, this could cause contention.

Suggestion: Consider consolidating into a single lock:

struct SessionState {
    vm_ip: Option<String>,
    snapshots: Vec<SnapshotId>,
    current_snapshot: Option<SnapshotId>,
}
session_state: parking_lot::RwLock<HashMap<SessionId, SessionState>>

Tradeoff: This reduces lock contention but makes the critical section slightly larger. Profile before optimizing.

Security

1. VM Resource Cleanup (firecracker.rs:592-594):

// TODO: Cleanup VM pool
// - Return VMs to pool or destroy overflow VMs

Concern: Incomplete cleanup could leak VM resources or allow session state to persist across users.

Action Required:

• Implement proper VM cleanup before merge
• Add tests for cleanup behavior
• Verify no state leaks between sessions

2. Snapshot Limit Enforcement (firecracker.rs:439-443):
Good! The max snapshots check prevents resource exhaustion attacks.

3. Session Isolation:
Code correctly uses session_id as isolation boundary. Good separation.

Dependencies

New dependencies added:

terraphim_types = { path = "../terraphim_types", optional = true }
terraphim_rolegraph = { path = "../terraphim_rolegraph", optional = true }
fcctl-core = { path = "../../../firecracker-rust/fcctl-core" }

Questions:

1. Why are terraphim_types and terraphim_rolegraph optional? What features gate them?
2. fcctl-core uses relative path ../../../ - is this crate part of the workspace?

---

📝 Testing

Coverage: Excellent! 60 tests added covering:

• ✅ Snapshot creation/restoration/deletion
• ✅ Snapshot limits per session
• ✅ Duplicate name rejection
• ✅ Bulk session cleanup
• ✅ Rollback scenarios

Suggestions for Additional Tests:

1. Concurrency test: Multiple sessions creating snapshots simultaneously
2. Error propagation: Verify VM failures properly rollback
3. Resource limits: Test behavior at max_snapshots_per_session boundary
4. Long-running sessions: Test session expiry and cleanup

---

🔧 Suggested Improvements

1. Error Messages

Current:

RlmError::SnapshotNotFound { snapshot_id: id.to_string() }

Better:

RlmError::SnapshotNotFound { 
    snapshot_id: id.id.clone(),
    snapshot_name: id.name.clone(),
    session_id: id.session_id,
}

This helps debugging by showing all context.

2. Logging Consistency

Mix of log::info\!, log::debug\!, log::warn\!. Consider:

• debug: Internal state changes ("Would create Firecracker VM snapshot")
• info: User-visible operations ("Created snapshot 'checkpoint_1'")
• warn: Degraded functionality ("No VM assigned, restore is a no-op")
• error: Failures requiring attention

Current usage looks mostly good, but review for consistency.

3. Documentation

The turbopuffer-query-parity.md doc is excellent! Clear explanations of:

• What ContainsAnyToken means
• Mapping to Terraphim's OR filtering
• Differences (tokenization, field awareness)
• Future alignment opportunities

Suggestion: Add a section on Performance Implications:

• BM25 scoring overhead vs simple token matching
• Graph traversal costs for TerraphimGraph
• When to use which approach

---

🎯 Action Items Before Merge

Must Fix:

• Update PR title and description to match actual changes
• Implement VM cleanup (remove TODO at firecracker.rs:592)
• Document breaking change in snapshot API signature
• Create GitHub issues for remaining TODOs
• Verify CI passes all checks

Should Fix:

• Replace panic! with assert! in tests
• Add concurrency tests for snapshot management
• Document why terraphim_types/rolegraph are optional dependencies
• Add performance implications section to turbopuffer doc

Nice to Have:

• Consider splitting PR for easier review
• Add RollbackResult enum for explicit no-op handling
• Profile lock contention under load
• Consolidate session state locks if contention observed

---

📊 Verdict

Quality: High - well-structured code with good practices
Test Coverage: Excellent - 60 tests covering key scenarios
Documentation: Very good - comprehensive docs and examples
Risk: Medium - large change surface, but well-tested

Recommendation:
✅ Approve with conditions - address "Must Fix" items before merge.

This is solid engineering work. The main concerns are:

1. PR scope clarity (title/description)
2. Incomplete VM cleanup (security concern)
3. Size makes review challenging (consider splitting)

Once the VM cleanup TODO is resolved and the PR is properly titled, this is ready to merge.

---

🙏 Positive Callouts

• Excellent work on the snapshot management system
• The validation framework with strictness levels is well-designed
• Query loop architecture is clean and maintainable
• Documentation quality is exceptional
• Test coverage demonstrates thorough thinking about edge cases

Great work on Phase 3 implementation! 🚀

---

Review generated with analysis of 19 changed files, 5,729 lines added, following project CLAUDE.md guidelines.