feat(api): cleaner evicts sandboxes older than StaleCutoff#2640
Conversation
The Redis cleaner now removes sandboxes whose EndTime is older than StaleCutoff, in addition to pruning orphaned index entries. Recently expired sandboxes are left to the evictor to avoid races.
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit cadc7dd. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 6 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 16 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Code Review
This pull request updates the Redis sandbox cleaner to proactively remove expired sandboxes that exceed the StaleCutoff threshold, ensuring cleaner storage state. The reviewer identified a potential race condition in the new evictExpired logic, noting that storage.Remove is called without verifying the current sandbox state, which could lead to premature deletion if a sandbox's lifetime is extended concurrently.
There was a problem hiding this comment.
LGTM — small, well-tested addition with the race window already addressed by the StaleCutoff guard.
Extended reasoning...
Overview
This PR extends the Redis sandbox cleaner to also remove sandboxes whose EndTime is older than sandbox.StaleCutoff, in addition to its existing job of pruning orphaned index entries. The change is ~20 lines in cleaner.go plus two new focused tests in cleaner_test.go covering both the eviction path and the StaleCutoff race-guard.
Security risks
None. This is internal cleanup logic operating on Redis-backed sandbox state. Remove is idempotent and the operation is scoped to entries already classified as expired by ExpiredItems. Multi-pod safety is preserved (the existing idempotency contract still holds).
Level of scrutiny
Low-to-moderate. It is operational/cleanup code rather than auth or data-handling, the change is small and self-contained, and the new behavior is gated by an explicit StaleCutoff window that intentionally leaves recently-expired sandboxes to the evictor to avoid a race. The PR author addressed gemini-code-assist's race concern in the timeline by noting Redis is used as a write-through cache and the StaleCutoff window provides sufficient buffer.
Other factors
The two inline nits flagged (a stale doc comment and a misleading 'after ErrNotFound' log message) are comment-only and do not affect runtime behavior — worth fixing but not blocking. Test coverage for the new behavior is good: both the eviction path and the negative guard (recently-expired survives) are exercised.
Address PR feedback: the inline comment in RunOnce contradicted what evictExpired actually does, and the warning message referenced a non-existent ErrNotFound precondition.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4e80239d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- drop items.go line reference from RunOnce comment - promote stale-remove failure log from Warn to Error
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e33eea1afa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2de6e99 to
cadc7dd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadc7ddea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Redis cleaner now removes sandboxes whose EndTime is older than StaleCutoff, in addition to pruning orphaned index entries. Recently expired sandboxes are left to the evictor to avoid races.
The Redis cleaner now removes sandboxes whose EndTime is older than StaleCutoff, in addition to pruning orphaned index entries. Recently expired sandboxes are left to the evictor to avoid races.