feat: add actor model to SecretManager for redaction map thread safety#354
feat: add actor model to SecretManager for redaction map thread safety#354ahmedhesham6 merged 10 commits intostakpak:betafrom
SecretManager for redaction map thread safety#354Conversation
Converted SecretManager to an actor-based architecture for improved thread-safety with benchmarks
There was a problem hiding this comment.
The actor model implementation is solid and correctly addresses the thread safety concern. Good use of Tokio channels, spawn_blocking for file I/O, and lazy loading of gitleaks config.
Suggestions
1. Unused fields in SecretManagerHandle
redact_secrets and privacy_mode fields are stored but never used. Consider either:
- Removing them
- Using them for fast-path checks before sending messages (e.g., skip sending if
!redact_secrets)
// Example fast-path optimization:
pub async fn redact_and_store_secrets(&self, content: &str, path: Option<&str>) -> Result<String, SecretManagerError> {
if !self.redact_secrets {
return Ok(content.to_string());
}
// ... send message
}2. File I/O on every save
save_session_redaction_map().await is called on every redaction operation. For frequent operations, this could be a performance bottleneck.
Consider:
- Debouncing saves (e.g., save at most once per second)
- Batching saves (save after N operations or on shutdown)
- Only saving when new secrets are actually added:
let old_len = self.redaction_map.len();
self.redaction_map.extend(redaction_result.redaction_map);
if self.redaction_map.len() > old_len {
self.save_session_redaction_map().await;
}3. Error handling context
Consider adding more context to error messages to aid debugging:
// Current
.map_err(|e| McpError::internal_error(e.to_string(), None))?;
// Suggested
.map_err(|e| McpError::internal_error(format!("Failed to redact command output: {}", e), None))?;4. Potential backpressure in format_tasks_table
This function is called from within task progress reporting loops. If the secret manager channel is full (capacity 100), the .await could cause backpressure on progress reporting.
Not a blocker, but worth monitoring. If issues arise, consider using try_send with fallback to unredacted output.
Thanks for the thorough work on this! Can you please resolve the merge conflicts?
The caller `redact_and_store_secrets_impl` uses `extend()` to merge results, so returning the full cloned map caused redundancy as we already have the old mappings.
|
Thanks for the detailed review I appreciate it! @ahmedhesham6
|
|
@abd0-omar Can you sync with beta branch? |
|
@ahmedhesham6 I've merged branch beta to the current branch |
// TODO: this is not thread safe, we need to use a mutex or an actor to protect the redaction map