Conversation
Greptile SummaryThis PR adds user-configurable options to the manual GC cleanup flow: the "Run Cleanup Now" dialog now lets users specify retention days per resource type and shows a live estimated-deletion preview before confirming. The backend gains a new
Confidence Score: 4/5Safe to merge with one fix: the preview count shown to users before they confirm a cleanup is incomplete. The manual cleanup logic itself is correct — runCleanup properly skips resource types not present in manualDays and applies user-supplied day overrides. The concern is in PreviewCleanup: it counts only Request records for the "requests" resource type, but the actual cleanup additionally removes Thread and Trace records for that same option. Users see an undercount in the confirmation dialog and cannot accurately assess the impact before proceeding. internal/server/gc/gc.go — the PreviewCleanup function needs to count Thread and Trace records alongside Request records for the "requests" resource type. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Frontend Dialog
participant GQL as GraphQL API
participant GC as gc.Worker
UI->>GQL: previewGcCleanup(input) [query]
GQL->>GC: PreviewCleanup(ctx, input)
GC-->>GQL: []GcCleanupPreviewItem (requests + usage_logs counts)
GQL-->>UI: preview items
Note over UI: User reviews estimate and confirms
UI->>GQL: triggerGcCleanup(input) [mutation]
GQL->>GQL: spawn goroutine (system bypass ctx)
GQL-->>UI: true (async acknowledged)
GQL->>GC: RunCleanupNow(bgCtx, input)
GC->>GC: "runCleanup(ctx, manual=true, manualDays)"
Note over GC: Deletes Requests + Threads + Traces (Threads/Traces absent from preview)
GC->>GC: cleanupUsageLogs
Reviews (2): Last reviewed commit: "feat: gc now with options, close #1618" | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces a manual garbage collection (GC) cleanup feature with a preview capability. Key changes include updating the frontend to allow users to specify retention days for request and usage logs, adding a preview of estimated deletions, and modifying the backend to support manual overrides and preview queries. Feedback focuses on refining the manual cleanup logic to prevent unintended deletions, ensuring proper cleanup of timers in the frontend to avoid memory leaks, and cleaning up unused variables in the backend validation logic.
| if option.Enabled || manual { | ||
| days := option.CleanupDays | ||
| if manual && manualDays != nil { | ||
| if d, ok := manualDays[option.ResourceType]; ok { | ||
| days = d | ||
| } | ||
| } |
There was a problem hiding this comment.
When manual is true, the current logic triggers cleanup for all resource types defined in the policy, even those not specified in manualDays or those disabled in the policy. This behavior is unexpected for a manual cleanup with specific options, as it might trigger unwanted deletions for other resource types using their default policy days. It's better to only process resource types that are either enabled in the policy (for automatic runs) or explicitly provided in the manual override map.
days, shouldRun := option.CleanupDays, option.Enabled
if manual {
days, shouldRun = manualDays[option.ResourceType]
}
if shouldRun {| const [manualUsageLogsDays, setManualUsageLogsDays] = useState(7); | ||
| const [previewItems, setPreviewItems] = useState<GcCleanupPreviewItem[]>([]); | ||
| const [isPreviewLoading, setIsPreviewLoading] = useState(false); | ||
| const previewTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); |
There was a problem hiding this comment.
The previewTimerRef is used to manage a debounce timer for fetching previews. To prevent potential memory leaks or state updates on an unmounted component, it's a good practice to clear this timer when the component unmounts.
const previewTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
useEffect(() => {
return () => {
if (previewTimerRef.current) {
clearTimeout(previewTimerRef.current);
}
};
}, []);
| if input.RequestsCleanupDays > 0 { | ||
| cutoff := time.Now().AddDate(0, 0, -input.RequestsCleanupDays) | ||
| count, err := w.Ent.Request.Query().Where(request.CreatedAtLT(cutoff)).Count(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to count requests for preview: %w", err) | ||
| } | ||
| items = append(items, GcCleanupPreviewItem{ | ||
| ResourceType: "requests", | ||
| EstimatedCount: count, | ||
| CutoffTime: cutoff, | ||
| RetentionDays: input.RequestsCleanupDays, | ||
| }) | ||
| } |
There was a problem hiding this comment.
PreviewCleanup only counts Request records for the "requests" resource type, but runCleanup deletes three entity types for that same option: requests (via cleanupRequests), threads (via cleanupThreads), and traces (via cleanupTraces). A user who opens the dialog and sees "42 Request Logs records" will then confirm a cleanup that actually deletes far more rows — threads and traces are invisible to the preview entirely. To give an accurate estimate, the preview should sum counts from all three tables (or at least make clear that threads and traces are also included).
No description provided.