feat: auto-delete task history on extension load #9262
Conversation
dosubot
bot
added
size:XL
Re-review complete for TODO
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
hannesrudolph
force-pushed
the
chore/retention-purge-checkpoint-cleanup
branch
from
784a70d to
67c7629
Compare
hannesrudolph
force-pushed
the
chore/retention-purge-checkpoint-cleanup
branch
from
d9b68d5 to
42d73fe
Compare
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
hannesrudolph
force-pushed
the
chore/retention-purge-checkpoint-cleanup
branch
from
84853c9 to
cc67f71
Compare
heyseth
left a comment
heyseth
left a comment
There was a problem hiding this comment.
Executive Summary
This PR introduces an auto-delete task history feature that purges old tasks on extension reload. The implementation is well-architected, type-safe, and thoroughly tested. The code demonstrates solid engineering practices with proper error handling, i18n support, and integration with existing deletion logic.
Strengths
- ✅Consistency: Excellent reuse of existing
deleteTaskWithId()logic ensures checkpoints, shadow repos, and state are cleaned up alongside files. - ✅Reliability: Sequential deletion prevents race conditions, and robust retry logic handles stubborn filesystem operations.
- ✅Quality: Full i18n support across 17 languages and type-safe schema validation with Zod.
Suggested Enhancements (Code Included)
I have included specific code snippets in the comments to address performance and user trust:
1. Performance: Non-blocking Activation (Critical)
The current implementation awaits the purge process during the activate phase. On machines with large histories or slow disks, this will block the extension startup.
- Fix: I provided a snippet to wrap the logic in a background "fire-and-forget" task (
void (async ...)), ensuring the extension activates immediately. - Optimization: Added an early return check for
retention === "never"to skip the logic entirely for the majority of users.
2. UX: Visibility & Trust
Silent deletion of user data can be alarming if a user expects to find a task.
- Fix: I added a Toast Notification logic to the background task. It only triggers if files were actually deleted and provides a direct link to Settings.
3. Testing: Edge Case Coverage
Since we are permanently deleting files, I identified a gap in testing regarding "Orphan Checkpoints" and "Legacy Mtime Fallback."
- Fix: I drafted 4 specific test cases to verify we strictly target garbage data and do not accidentally delete recent, valid tasks that lack metadata.
Conclusion
This is a high-value maintenance feature. Once the activation blocking issue is resolved (using the background task pattern I suggested), this logic is solid and ready for production!
hannesrudolph
force-pushed
the
chore/retention-purge-checkpoint-cleanup
branch
from
cc67f71 to
ab6d3ab
Compare
hannesrudolph
force-pushed
the
chore/retention-purge-checkpoint-cleanup
branch
from
098aa38 to
18894da
Compare
There was a problem hiding this comment.
All previously flagged issues have been addressed in the latest commits. The implementation is clean and well-structured:
- Sequential task deletion prevents race conditions
- i18n translations added to multiple locales
- Tests properly mock storage module
- UI copy accurately describes activation behavior
- Validation occurs appropriately at use time
No net new issues identified. Approving this PR.
- Remove expensive recursive getDirectorySize() that caused UI freeze with ~9000 tasks - Now only counts top-level task directories via single fs.readdir() call - Make task count on-demand (user must click refresh) instead of auto-trigger on settings load - Update types, i18n strings, and tests accordingly
This PR was never merged, so no need for backwards compatibility. Import formatBytes directly from its source file.
| "format": "{{size}} ({{count}} Aufgaben)", | ||
| "formatSingular": "{{size}} (1 Aufgabe)", | ||
| "empty": "Keine Aufgaben gespeichert", | ||
| "refresh": "Aktualisieren", |
There was a problem hiding this comment.
Non-English locales still define settings:taskHistoryStorage.* as storage-size strings (e.g. format, calculating, error), but About.tsx now reads clickToCount, count, and countSingular. This will likely show missing-key fallbacks in most languages unless the locale JSONs are updated to match the new contract.
Fix it with Roo Code or mention @roomote and request a fix.
| const startTime = Date.now() | ||
| const result = await calculateTaskStorageSize("/global/storage") | ||
| const elapsed = Date.now() - startTime | ||
|
|
||
| expect(result.taskCount).toBe(9000) | ||
| // Should complete quickly since we're not recursing into directories | ||
| expect(elapsed).toBeLessThan(100) // Should be nearly instant | ||
| }) |
There was a problem hiding this comment.
This test is timing-based ("< 100ms") and can be flaky on slower CI or under load even if the implementation is correct. A more deterministic check is to assert we only do a single top-level readdir call (no recursion).
Fix it with Roo Code or mention @roomote and request a fix.
| try { | ||
| const entries = await fs.readdir(tasksDir, { withFileTypes: true }) | ||
| taskCount = entries.filter((d) => d.isDirectory()).length | ||
| } catch { | ||
| // Tasks directory doesn't exist yet | ||
| log?.(`[TaskStorageSize] Tasks directory not found at ${tasksDir}`) | ||
| return defaultResult | ||
| } |
There was a problem hiding this comment.
The catch here treats all readdir failures as "tasks directory not found", which is misleading for permission/IO errors and makes debugging harder. Logging the error message preserves the current behavior while keeping diagnostics accurate.
| try { | |
| const entries = await fs.readdir(tasksDir, { withFileTypes: true }) | |
| taskCount = entries.filter((d) => d.isDirectory()).length | |
| } catch { | |
| // Tasks directory doesn't exist yet | |
| log?.(`[TaskStorageSize] Tasks directory not found at ${tasksDir}`) | |
| return defaultResult | |
| } | |
| } catch (e) { | |
| // Tasks directory doesn't exist yet (or is unreadable) | |
| log?.( | |
| `[TaskStorageSize] Failed to read tasks directory at ${tasksDir}: ${e instanceof Error ? e.message : String(e)}`, | |
| ) | |
| return defaultResult | |
| } |
Fix it with Roo Code or mention @roomote and request a fix.
| const startTime = Date.now() | ||
| const result = await calculateTaskStorageSize("/global/storage") | ||
| const elapsed = Date.now() - startTime | ||
|
|
||
| expect(result.taskCount).toBe(9000) | ||
| // Should complete quickly since we're not recursing into directories | ||
| expect(elapsed).toBeLessThan(100) // Should be nearly instant | ||
| }) |
There was a problem hiding this comment.
This test is timing-based ("< 100ms") and can be flaky on slower CI or under load even if the implementation is correct. A more deterministic check is to assert we only do a single top-level readdir call (no recursion).
| const startTime = Date.now() | |
| const result = await calculateTaskStorageSize("/global/storage") | |
| const elapsed = Date.now() - startTime | |
| expect(result.taskCount).toBe(9000) | |
| // Should complete quickly since we're not recursing into directories | |
| expect(elapsed).toBeLessThan(100) // Should be nearly instant | |
| }) | |
| const result = await calculateTaskStorageSize("/global/storage") | |
| expect(result.taskCount).toBe(9000) | |
| // Deterministic: only one top-level directory listing (no recursion) | |
| expect(mockReaddir).toHaveBeenCalledTimes(1) |
Fix it with Roo Code or mention @roomote and request a fix.
The formatBytes tests are not relevant since we no longer calculate size.
…ence race condition
- Add parallel metadata reads with 50 concurrent operations - Add parallel deletions with 10 concurrent operations - Batch read all metadata first, then filter, then delete - Reduce retry attempts from 3 to 1 (remove aggressive retries with sleeps) - Extract helper functions for cleaner code (readTaskMetadata, pathExists, removeDir) This significantly improves purge performance for users with many tasks (~9000+).
- Add purgeOldCheckpoints() function with hardcoded 30-day threshold - Add startBackgroundCheckpointPurge() for fire-and-forget activation - Culls only checkpoints/ subdirectory, preserves task history - Runs silently on extension activation (no user notification) - Coexists with existing taskHistoryRetention setting - Add 6 new tests for checkpoint culling functionality
| logv(`[Retention] Phase 3: Deleting ${tasksToDelete.length} tasks (concurrency: ${DELETION_CONCURRENCY})`) | ||
| const deleteLimit = pLimit(DELETION_CONCURRENCY) | ||
|
|
||
| const deleteResults = await Promise.all( |
There was a problem hiding this comment.
This purge still deletes tasks in parallel (Promise.all + p-limit), which can reintroduce the earlier race when the provider updates taskHistory state per deletion. If the purge uses provider-backed deletion (deleteTaskById), consider serializing deletions (for..of) or batching state updates after filesystem cleanup to avoid lost updates.
Fix it with Roo Code or mention @roomote and request a fix.
| const startTime = Date.now() | ||
| const result = await calculateTaskStorageSize("/global/storage") | ||
| const elapsed = Date.now() - startTime | ||
|
|
||
| expect(result.taskCount).toBe(9000) | ||
| // Should complete quickly since we're not recursing into directories | ||
| expect(elapsed).toBeLessThan(100) // Should be nearly instant |
There was a problem hiding this comment.
This timing-based assertion ("< 100ms") is likely to be flaky under CI load or slower machines even when behavior is correct; a deterministic alternative is asserting the mocked readdir was called exactly once (no recursion).
| const startTime = Date.now() | |
| const result = await calculateTaskStorageSize("/global/storage") | |
| const elapsed = Date.now() - startTime | |
| expect(result.taskCount).toBe(9000) | |
| // Should complete quickly since we're not recursing into directories | |
| expect(elapsed).toBeLessThan(100) // Should be nearly instant | |
| const result = await calculateTaskStorageSize("/global/storage") | |
| expect(result.taskCount).toBe(9000) | |
| // Deterministic: only one top-level directory listing (no recursion) | |
| expect(mockReaddir).toHaveBeenCalledTimes(1) |
Fix it with Roo Code or mention @roomote and request a fix.
| } catch { | ||
| // Tasks directory doesn't exist yet | ||
| log?.(`[TaskStorageSize] Tasks directory not found at ${tasksDir}`) | ||
| return defaultResult |
There was a problem hiding this comment.
The catch here logs "Tasks directory not found" for all readdir failures, which is misleading for permission/IO errors. Logging the actual error message keeps behavior the same but makes diagnostics accurate.
| } catch { | |
| // Tasks directory doesn't exist yet | |
| log?.(`[TaskStorageSize] Tasks directory not found at ${tasksDir}`) | |
| return defaultResult | |
| } catch (e) { | |
| // Tasks directory doesn't exist yet (or is unreadable) | |
| log?.( | |
| `[TaskStorageSize] Failed to read tasks directory at ${tasksDir}: ${e instanceof Error ? e.message : String(e)}`, | |
| ) | |
| return defaultResult | |
| } |
Fix it with Roo Code or mention @roomote and request a fix.
- Log number of tasks being scanned - Log how many tasks have checkpoints - Log how many need culling (older than 30 days) - Log progress every 100 deletions during large culls
… popover - Add Settings gear icon in HistoryView header with popover for retention settings - Retention dropdown allows configuring auto-delete task history (never, 3-90 days) - Settings are saved immediately via vscode.postMessage updateSettings - Remove retention settings from About.tsx (Settings View) - Update About.tsx props and tests to remove taskHistoryRetention
…aging
- Add confirmation dialog before changing retention settings to prevent accidental changes
- Move task count display from About tab to History View settings popover
- Combine redundant description/warning into single, clear warning message
- Update task count display to be more human-friendly ('X tasks in history')
- Update all 18 locale files with consistent messaging
- Revert About.tsx to original state (no retention UI there)
hannesrudolph
changed the title
The file was left over after size calculation was removed from task-storage-size.ts for performance reasons. Knip correctly detected it as unused.
dosubot
bot
added
size:XXL
Summary
Adds automatic task history cleanup to help manage disk space. Old tasks and their associated checkpoints are automatically removed when VS Code restarts.
User-Facing Changes
New Setting: Auto-Delete Task History
Access via the gear icon in the History View header. Choose how long to keep your task history:
Safety Features
What Gets Cleaned Up
When VS Code restarts (if a retention period is set):
Full Localization
All new UI text is translated into 18 languages.
Screenshots
Closes #10850