Repository navigation
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Drain returns only non-expired items; an empty result means the | ||
| // sweeper has evicted the expired ones. | ||
| items, drainErr := storage.Drain(ctx) | ||
| assert.Equal(t, nil, drainErr) | ||
|
|
||
| if len(items) == 0 { | ||
| return |
There was a problem hiding this comment.
TestSweeper_EvictsExpiredEntries checks whether Drain returns no live entries, not whether the sweeper removed expired entries from memory. As the comment explains, Drain excludes expired entries, so this check can pass without any sweep. Assert that Cleanup runs on the tracked caches, or inspect the stored entry count without calling Drain.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/sms-gateway/cache/sweeper_test.go
Line: 48-54
Comment:
**Test misses broken cleanup**
`TestSweeper_EvictsExpiredEntries` checks whether `Drain` returns no live entries, not whether the sweeper removed expired entries from memory. As the comment explains, `Drain` excludes expired entries, so this check can pass without any sweep. Assert that `Cleanup` runs on the tracked caches, or inspect the stored entry count without calling `Drain`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| fx.Provide(func(factory cachefx.Factory) Factory { | ||
| return factory.WithName("sms-gateway") | ||
| return &trackingFactory{ | ||
| Factory: factory.WithName("sms-gateway"), | ||
|
|
||
| cachesMu: sync.Mutex{}, | ||
| caches: []cache.Cache{}, | ||
| } | ||
| }), |
There was a problem hiding this comment.
The new sweeper misses the cache used for one-time codes. otp.Module creates it through cachefx.Factory, which bypasses trackingFactory.New. If the server uses the memory cache, unused codes remain allocated after they expire because CleanupAll never visits this cache and Storage.Cleanup has no caller. Route it through the tracked factory or give it its own cleanup job.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/sms-gateway/cache/module.go
Line: 17-24
Comment:
**One-time codes miss cleanup**
The new sweeper misses the cache used for one-time codes. `otp.Module` creates it through `cachefx.Factory`, which bypasses `trackingFactory.New`. If the server uses the memory cache, unused codes remain allocated after they expire because `CleanupAll` never visits this cache and `Storage.Cleanup` has no caller. Route it through the tracked factory or give it its own cleanup job.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| func (f *trackingFactory) WithName(prefix string) cachefx.Factory { | ||
| return &trackingFactory{ | ||
| Factory: f.Factory.WithName(prefix), | ||
|
|
||
| cachesMu: sync.Mutex{}, | ||
| caches: []cache.Cache{}, | ||
| } |
There was a problem hiding this comment.
trackingFactory.WithName creates a separate cache list. Caches created through the returned factory are invisible to the existing sweeper. No current caller uses this path, but a caller that adds a name prefix will silently lose periodic cleanup. Share the list and its lock between the parent and named factories.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/sms-gateway/cache/sweeper.go
Line: 57-63
Comment:
**Named caches lose cleanup**
`trackingFactory.WithName` creates a separate cache list. Caches created through the returned factory are invisible to the existing sweeper. No current caller uses this path, but a caller that adds a name prefix will silently lose periodic cleanup. Share the list and its lock between the parent and named factories.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
🤖 Pull request artifacts
|
This branch has not been deployed
1 participant
The PR appears safe to merge, with non-blocking gaps in cleanup coverage and its tests.
Findings
Fix with agent prompt
Summary
Adds a factory wrapper that records cache instances and a sweeper that calls
CleanupAllevery minute. Registers the sweeper with the app lifecycle and adds tests and dependency updates.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Tracked factory] --> B[Recorded cache instances] C[Minute tick] --> D[Sweeper] D --> E[CleanupAll] E --> B F[Raw cachefx factory] --> G[One-time code cache] A --> H[WithName] H --> I[Separate cache list]Reviews (1) · Last reviewed commit: "[cache] periodic cache clenup" · Reviewed by Greptile