feat: clean stale and orphaned indexes - #180
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR replaces the purge workflow with ChangesIndex cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CleanCommand
participant LumenDataDirectory
participant SQLiteIndexMetadata
User->>CleanCommand: Run lumen clean
CleanCommand->>LumenDataDirectory: Scan index directories
CleanCommand->>SQLiteIndexMetadata: Read access and index timestamps
CleanCommand->>LumenDataDirectory: Remove stale unlocked indexes
CleanCommand-->>User: Report removals and skips
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/clean.go (1)
88-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute interactive status through
tui.Progress.The coding guidelines require interactive commands including
cleanto send progress and status totui.Progresson stderr. This sweep writes status lines directly withfmt.Fprintfon the stderr writer. The summary on stdout at Line 130 already matches the guideline.The injected
io.Writerpair keepscleanIndexestestable, so keep the writers and emit throughtui.ProgressinrunClean, or confirm thattui.Progressis intended only for long-running progress reporting.As per coding guidelines: "Interactive commands such as
index,clean, andsearchmust send progress and status totui.Progresson stderr, completion summaries to stdout withfmt.Printf, and errors to stderr withfmt.Fprintf(os.Stderr, ...)."🤖 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 `@cmd/clean.go` around lines 88 - 131, Route the interactive status messages in cleanIndexes, including lock skips, removal failures, and successful removals, through tui.Progress on stderr instead of direct fmt.Fprintf calls. Preserve the injected stdout and stderr writers for testability, keep the completion summary on stdout, and leave error reporting semantics unchanged.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 `@cmd/clean.go`:
- Line 94: Bound the positive days value in runClean before calculating cutoff,
using a maximum that keeps time.Duration(days) * 24 * time.Hour from
overflowing; preserve the existing negative-value validation and ensure larger
retention windows are rejected or safely clamped rather than allowing cutoff to
wrap and mark every index stale.
- Around line 107-125: Update the cleanup flow around isIndexStale and
os.RemoveAll to call indexlock.TryAcquire for the database lock instead of
checking indexlock.IsHeld. Retain each successfully acquired lock through the
stale check and hashDir deletion, release it on every path, and treat
acquisition failure as the existing held-lock skip case.
In `@README.md`:
- Line 401: Update the descriptions of `lumen clean --days 0` in README.md lines
401-401 and skills/reindex/SKILL.md lines 22-23 to state that it removes all
eligible cached indexes while preserving indexes held by active indexers through
active locks.
---
Nitpick comments:
In `@cmd/clean.go`:
- Around line 88-131: Route the interactive status messages in cleanIndexes,
including lock skips, removal failures, and successful removals, through
tui.Progress on stderr instead of direct fmt.Fprintf calls. Preserve the
injected stdout and stderr writers for testability, keep the completion summary
on stdout, and leave error reporting semantics unchanged.
🪄 Autofix
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 Plus
Run ID: 98c2f1df-b5c6-40c4-aca5-364ae8891ff9
📒 Files selected for processing (9)
CLAUDE.mdREADME.mdcmd/clean.gocmd/clean_test.gocmd/purge.gocmd/purge_test.gointernal/store/store.gointernal/store/store_test.goskills/reindex/SKILL.md
💤 Files with no reviewable changes (2)
- cmd/purge.go
- cmd/purge_test.go
| return fmt.Errorf("read data dir: %w", err) | ||
| } | ||
|
|
||
| cutoff := now.Add(-time.Duration(days) * 24 * time.Hour) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bound --days to prevent a time.Duration overflow that deletes every index.
time.Duration holds nanoseconds in an int64, so it saturates at about 106751 days. time.Duration(days) * 24 * time.Hour overflows for a larger value. The cutoff then wraps to a time far in the future, isIndexStale marks every index stale, and the sweep deletes all of them.
A larger retention window must never delete more. runClean already rejects negative values at Line 72. Add an upper bound there or clamp the cutoff here.
🐛 Proposed fix in runClean
if days < 0 {
return fmt.Errorf("--days must not be negative, got %d", days)
}
+ const maxCleanDays = 100000 // keeps days*24h inside time.Duration's range
+ if days > maxCleanDays {
+ return fmt.Errorf("--days must not exceed %d, got %d", maxCleanDays, days)
+ }🤖 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 `@cmd/clean.go` at line 94, Bound the positive days value in runClean before
calculating cutoff, using a maximum that keeps time.Duration(days) * 24 *
time.Hour from overflowing; preserve the existing negative-value validation and
ensure larger retention windows are rejected or safely clamped rather than
allowing cutoff to wrap and mark every index stale.
| if indexlock.IsHeld(indexlock.LockPathForDB(dbPath)) { | ||
| _, _ = fmt.Fprintf(stderr, "Keeping %s: an indexer is currently running.\n", entry.Name()) | ||
| skipped++ | ||
| continue | ||
| } | ||
|
|
||
| stale, reason := isIndexStale(dbPath, days, cutoff) | ||
| if !stale { | ||
| skipped++ | ||
| continue | ||
| } | ||
| if err := os.RemoveAll(hashDir); err != nil { | ||
| _, _ = fmt.Fprintf(stderr, "Failed to remove %s: %v\n", hashDir, err) | ||
| if firstErr == nil { | ||
| firstErr = fmt.Errorf("remove %s: %w", hashDir, err) | ||
| } | ||
| skipped++ | ||
| continue | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the indexlock API: TryAcquire, IsHeld, Release, and where LockPathForDB places the lock file.
rg -nP --type=go -B 2 -A 25 'func (TryAcquire|IsHeld|LockPathForDB)\(|func \(.*Lock\) Release\(' internal/indexlockRepository: ory/lumen
Length of output: 3971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cleanIndexes implementation =="
sed -n '80,135p' cmd/clean.go
echo
echo "== relevant indexlock tests =="
rg -n --type=go -B 4 -A 20 'TryAcquire|IsHeld|lock file|lock path|LockPathForDB|flock' cmd/clean_test.go internal/indexlock
echo
echo "== lock implementations =="
sed -n '1,90p' internal/indexlock/lock.goRepository: ory/lumen
Length of output: 20577
Hold the index lock while cleaning the index directory.
indexlock.IsHeld checks the lock before os.RemoveAll deletes hashDir. An indexer can acquire the lock in that window, so clean can remove the index files it is currently writing to.
Use indexlock.TryAcquire(indexlock.LockPathForDB(dbPath)) instead and keep any acquired lock for the whole stale/delete decision. cmd/clean_test.go already exercises the held-lock case with TryAcquire.
🤖 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 `@cmd/clean.go` around lines 107 - 125, Update the cleanup flow around
isIndexStale and os.RemoveAll to call indexlock.TryAcquire for the database lock
instead of checking indexlock.IsHeld. Retain each successfully acquired lock
through the stale check and hashDir deletion, release it on every path, and
treat acquisition failure as the existing held-lock skip case.
| ```bash | ||
| lumen clean # remove indexes unused for 30 days or whose project is gone | ||
| lumen clean --days 7 # tighten the cutoff to a week | ||
| lumen clean --days 0 # remove every cached index on this host |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
lumen clean --days 0 is not an unconditional full wipe.
cmd/clean.go:84-133 preserves indexes with active locks. Update both descriptions to state that the command removes all eligible indexes while keeping indexes used by active indexers.
README.md#L401-L401: replace “remove every cached index” with the eligible-index behavior.skills/reindex/SKILL.md#L22-L23: replace “deletes every cached index” with the same active-lock exception.
📍 Affects 2 files
README.md#L401-L401(this comment)skills/reindex/SKILL.md#L22-L23
🤖 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 `@README.md` at line 401, Update the descriptions of `lumen clean --days 0` in
README.md lines 401-401 and skills/reindex/SKILL.md lines 22-23 to state that it
removes all eligible cached indexes while preserving indexes held by active
indexers through active locks.
1 participant
Replaces lumen purge with lumen clean, which removes orphaned indexes and indexes unused beyond a configurable cutoff while preserving indexes that are actively being written. Records last-access timestamps when stores open, with read-only metadata scans and a last-indexed fallback for legacy indexes. Updates command documentation and reindex guidance, with expanded cleanup and store coverage. Tested with make test and make lint.
Summary by CodeRabbit
New Features
lumen cleanto remove stale or orphaned indexes.--days 0for all unlocked indexes.Documentation
lumen cleanandlumen index --force.