Skip to content

feat(snapshots): add eviction, lifecycle cleanup, and merge-triggered invalidation#1048

Merged
aaight merged 1 commit intodevfrom
feature/snapshot-eviction-and-cleanup
Mar 25, 2026
Merged

feat(snapshots): add eviction, lifecycle cleanup, and merge-triggered invalidation#1048
aaight merged 1 commit intodevfrom
feature/snapshot-eviction-and-cleanup

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 24, 2026

Summary

  • Extend snapshot-manager.ts with evictSnapshots() enforcing per-project TTL plus global max-count and max-size budget limits (oldest-first eviction order)
  • New snapshot-cleanup.ts module with startSnapshotCleanup() / stopSnapshotCleanup() running every 5 minutes alongside the existing orphan cleanup loop
  • worker-manager.ts starts and stops snapshot cleanup alongside orphan cleanup with no timer leaks
  • PRMergedTrigger and TrelloStatusChangedMergedTrigger call invalidateSnapshot() fire-and-forget when a work item reaches merged status
  • All cleanup paths handle missing entries gracefully

Test plan

  • evictSnapshots — TTL eviction, max-count eviction, max-size eviction, combined phase ordering, no-op when under budget, graceful handling of snapshots with no size
  • startSnapshotCleanup / stopSnapshotCleanup — idempotent start, safe stop when not started, multiple start/stop cycles
  • runSnapshotCleanup — calls evictSnapshots with routerConfig values
  • startWorkerProcessor starts snapshot cleanup; stopWorkerProcessor stops it
  • PRMergedTrigger — invalidates snapshot on merged PR, skips when PR not merged, skips when no work item linked
  • TrelloStatusChangedMergedTrigger — invalidates snapshot on card moved to merged, skips when trigger disabled

Card

https://trello.com/c/69c2921d5515c7e2c229b00b

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 24, 2026

Codecov Report

❌ Patch coverage is 93.27731% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/router/snapshot-cleanup.ts 82.05% 7 Missing ⚠️
src/router/snapshot-manager.ts 98.57% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Clean, well-structured PR that adds snapshot eviction, lifecycle cleanup, and merge-triggered invalidation. The implementation closely follows the existing orphan-cleanup pattern, tests are thorough, and all CI checks pass. LGTM.

Minor Observations (non-blocking)

  • Misleading log value in Phase 2 eviction (src/router/snapshot-manager.ts:179): The snapshotCount field logged during max-count eviction uses snapshots.size + (toRemove - i), but since the delete has already happened on line 174, this produces an inflated value rather than the pre-eviction count. For example, if you start with 8 snapshots and maxCount=5 (toRemove=3), the first eviction logs snapshotCount=10 instead of 8. Consider using snapshots.size + 1 to log the count just before this deletion, or remove the field since maxCount already communicates the threshold. Cosmetic-only — doesn't affect behavior.

  • imageSizeBytes not yet plumbed in production: registerSnapshot in container-manager.ts (line 91) is called without the new imageSizeBytes parameter, so all production snapshots will have imageSizeBytes: undefined and contribute 0 to the size budget. This means the max-size eviction phase is effectively a no-op until a follow-up PR wires the image size through. The code is correct as-is (gracefully handles undefined), just worth noting for future work.

🕵️ claude-code · claude-opus-4-6 · run details

@aaight aaight merged commit 8181383 into dev Mar 25, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants