feat(mcp-plans): add automatic background cleanup of inactive plans - #656
Conversation
Adds automatic background cleanup of inactive plans to harnx-mcp-plans with a configurable retention period. Cleanup is performed daily in the background. Key changes: - Add --retention-days CLI flag and AGENT_PLANS_RETENTION_DAYS env var (default 14 days) - Implement supervised background task that restarts on panic - Add unit tests for deletion logic #652 Plan: harnx-mcp-plans-auto-cleanup
|
Warning Review limit reached
More reviews will be available in 34 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds inactivity-based cleanup to the harnx-mcp-plans MCP server. It enables the tokio ChangesPlan Cleanup and Task Supervision
Sequence DiagramsequenceDiagram
participant Main
participant CleanupTask as cleanup_loop<br/>(task)
participant Interval
participant BlockingFS as spawn_blocking<br/>(fs)
participant PlansDir as Plans Directory
Main->>CleanupTask: spawn cleanup_loop(dir, retention_days)
CleanupTask->>Interval: create daily interval
Interval-->>CleanupTask: first tick (immediate)
CleanupTask->>BlockingFS: run_cleanup_pass(dir, retention_days)
BlockingFS->>PlansDir: compute last_activity for each plan
BlockingFS->>PlansDir: delete plans exceeding retention
BlockingFS-->>CleanupTask: cleanup complete
Interval-->>CleanupTask: next tick (24h later)
CleanupTask->>BlockingFS: run_cleanup_pass again
Note over Main,CleanupTask: If cleanup_loop exits, Main respawns it<br/>If MCP service exits, shutdown proceeds
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/harnx-mcp-plans/src/main.rs (1)
63-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn parse errors via
anyhow::Resultinstead of callingstd::process::exit.
parse_argscurrently hard-exits in multiple branches, which bypassesmain’sanyhow::Result<()>flow and violates the repo error-handling rule. This also contributes to the current complexity gate failure on this method.Proposed direction
- fn parse_args() -> (PathBuf, u64) { + fn parse_args() -> anyhow::Result<(PathBuf, u64)> { ... - eprintln!("harnx-mcp-plans: --dir requires a path argument"); - std::process::exit(1); + anyhow::bail!("harnx-mcp-plans: --dir requires a path argument"); ... - eprintln!("harnx-mcp-plans: unknown argument: {}", other); - eprintln!("Try: harnx-mcp-plans --help"); - std::process::exit(1); + anyhow::bail!("harnx-mcp-plans: unknown argument: {other}. Try: harnx-mcp-plans --help"); ... - (plans_dir, retention_days) + Ok((plans_dir, retention_days)) }- let (plans_dir, retention_days) = parse_args(); + let (plans_dir, retention_days) = parse_args()?;As per coding guidelines: "
**/*.rs: Useanyhow::Resultandanyhow::bail!for error handling throughout the codebase".🤖 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 `@crates/harnx-mcp-plans/src/main.rs` around lines 63 - 159, parse_args currently calls std::process::exit in many branches; change it to return anyhow::Result<(PathBuf,u64)> and replace all hard exits with anyhow::bail! (or Err(anyhow!())) carrying the same human-readable messages (e.g., when missing flag args, invalid --retention-days, unknown argument, invalid AGENT_PLANS_RETENTION_DAYS value, and for --help/ -h show usage via Err or a distinct Help variant if desired), keep the same validation logic and messages, and ensure callers (main) propagate/handle the Result so process termination happens at a single top-level spot; update the function signature and all return points to use ?/bail! instead of std::process::exit.
🤖 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 `@crates/harnx-mcp-plans/src/main.rs`:
- Around line 48-55: The cleanup supervision currently respawns
server::cleanup_loop immediately inside the result = &mut cleanup_handle match
arm, which can hot-loop on deterministic failures; add a restart backoff (e.g.,
exponential with cap and small jitter) before calling tokio::spawn again.
Implement a mutable backoff Duration variable near cleanup_handle (reset to base
on successful run and double on each failure up to a max), await
tokio::time::sleep(backoff).await in the Err(e) branch (referencing
cleanup_handle, cleanup_dir.clone(), retention_days) and reset the backoff after
a successful spawn or clear on normal exit.
In `@crates/harnx-mcp-plans/src/server.rs`:
- Around line 1986-2030: The loop in async fn run_cleanup_pass uses
plan_dirs(dir) synchronously which can block the runtime; wrap the directory
listing in a blocking task instead: call tokio::task::spawn_blocking(move ||
plan_dirs(dir).collect::<Vec<_>>()) and await it to obtain a Vec<PathBuf>, then
iterate that Vec (instead of calling plan_dirs directly) so only non-blocking
tokio-await operations remain in run_cleanup_pass; keep the existing
spawn_blocking usage for plan_last_activity and remove any direct synchronous
read_dir calls from the async context (refer to run_cleanup_pass and
plan_dirs/plan_last_activity to locate the changes).
---
Outside diff comments:
In `@crates/harnx-mcp-plans/src/main.rs`:
- Around line 63-159: parse_args currently calls std::process::exit in many
branches; change it to return anyhow::Result<(PathBuf,u64)> and replace all hard
exits with anyhow::bail! (or Err(anyhow!())) carrying the same human-readable
messages (e.g., when missing flag args, invalid --retention-days, unknown
argument, invalid AGENT_PLANS_RETENTION_DAYS value, and for --help/ -h show
usage via Err or a distinct Help variant if desired), keep the same validation
logic and messages, and ensure callers (main) propagate/handle the Result so
process termination happens at a single top-level spot; update the function
signature and all return points to use ?/bail! instead of std::process::exit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90d037ab-8785-4f93-ade8-0ff7bf0653cb
📒 Files selected for processing (5)
crates/harnx-mcp-plans/Cargo.tomlcrates/harnx-mcp-plans/README.mdcrates/harnx-mcp-plans/src/main.rscrates/harnx-mcp-plans/src/server.rsdocs/solutions/async-patterns/mcp-server-background-task-supervision-2026-05-25.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…irs, parse_args returns Result
There was a problem hiding this comment.
Gates Failed
(2 files with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| main.rs | 1 advisory rule | 9.84 → 9.37 | Suppress |
| server.rs | 1 advisory rule | 4.93 → 4.85 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| } | ||
|
|
||
| fn parse_args() -> PathBuf { | ||
| fn parse_args() -> anyhow::Result<(PathBuf, u64)> { |
There was a problem hiding this comment.
❌ New issue: Complex Method
parse_args has a cyclomatic complexity of 12, threshold = 9
| async fn run_cleanup_pass(dir: &Path, retention: Duration) { | ||
| let dir_owned = dir.to_owned(); | ||
| let dirs = match tokio::task::spawn_blocking(move || plan_dirs(&dir_owned)).await { | ||
| Ok(dirs) => dirs, | ||
| Err(e) => { | ||
| eprintln!("[cleanup] error listing plans: {e}"); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| for plan_dir in dirs { | ||
| let name = plan_dir | ||
| .file_name() | ||
| .unwrap_or_default() | ||
| .to_string_lossy() | ||
| .into_owned(); | ||
| let plan_dir_for_activity = plan_dir.clone(); | ||
| let last_activity = | ||
| match tokio::task::spawn_blocking(move || plan_last_activity(&plan_dir_for_activity)) | ||
| .await | ||
| { | ||
| Ok(Ok(last_activity)) => last_activity, | ||
| Ok(Err(e)) => { | ||
| eprintln!("[cleanup] error checking plan {name}: {e}"); | ||
| continue; | ||
| } | ||
| Err(e) => { | ||
| eprintln!("[cleanup] error checking plan {name}: {e}"); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| let age = std::time::SystemTime::now() | ||
| .duration_since(last_activity) | ||
| .unwrap_or_default(); | ||
| if age <= retention { | ||
| continue; | ||
| } | ||
|
|
||
| let plan_dir_for_delete = plan_dir.clone(); | ||
| match tokio::task::spawn_blocking(move || std::fs::remove_dir_all(plan_dir_for_delete)) | ||
| .await | ||
| { | ||
| Ok(Ok(())) => { | ||
| eprintln!( | ||
| "[cleanup] deleted inactive plan {name} (inactive for {} days)", | ||
| age.as_secs() / 86_400 | ||
| ); | ||
| } | ||
| Ok(Err(e)) => eprintln!("[cleanup] error deleting plan {name}: {e}"), | ||
| Err(e) => eprintln!("[cleanup] error deleting plan {name}: {e}"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
❌ New issue: Complex Method
run_cleanup_pass has a cyclomatic complexity of 9, threshold = 9
1 participant
Adds automatic background cleanup of inactive plans to harnx-mcp-plans with a configurable retention period. Cleanup is performed daily in the background.
Key changes:
#652
Plan: harnx-mcp-plans-auto-cleanup
Summary by CodeRabbit
New Features
--retention-dayscommand-line option and environment variable support for retention configurationDocumentation