fix: [#1003] store the cache expiration with the value instead of a timer

[darakanoit]

📑 Description

Closes goravel/goravel#1003

Memory.Add and Memory.Put schedule expiration with a bare time.AfterFunc:

func (r *Memory) Add(key string, value any, t time.Duration) bool {
	if t != NoExpiration {
		time.AfterFunc(t, func() {
			r.Forget(key)
		})
	}

	_, loaded := r.instance.LoadOrStore(r.key(key), value)
	return !loaded
}

The timer knows the key, not the value it was created for. It is never stopped,
and it deletes whatever sits under the key when it fires. Three consequences:

• a failed Lock.Get releases a lock someone else holds. Add arms the
  timer before LoadOrStore and keeps it when the key is already taken, and
  Lock.Get is built on Add. A contender with a shorter TTL deletes the
  holder's lock, and a third caller then acquires it;
• a timer armed before Flush removes a key stored after it, including one
  stored with NoExpiration;
• overwriting a key with a longer TTL does not extend it: the first timer
  still fires.

memory is the default store in the skeleton, so facades.Cache() and
everything built on it is affected. The HTTP rate limiter stores its bucket with
Put(key, bucket, interval) and guards it with a lock on every request: from the
second interval on it lets every request through. TestStoreWithMemoryKeepsLimiting
fails on master with 54 requests allowed where the limit is 20.

What changed

The expiration is now stored next to the value and checked on read, the way
Laravel's Illuminate\Cache\ArrayStore does it. Nothing is scheduled, so nothing
can outlive the value it belongs to.

• Add replaces an expired item with CompareAndSwap, so two callers racing for
  an expired key cannot both win;
• a read drops an expired item with CompareAndDelete, so it cannot remove a
  value stored in the meantime;
• an expired item is reclaimed when its key is read or written again, and a
  write sweeps the whole map at most once a minute, so a key nothing reads again
  does not hold its value for the life of the process. This also removes the timers that kept expired values alive: 200k Put
  calls with a one hour TTL leave ~404k live heap objects on master and ~4k
  after the change.

Verification

	before	after
TestAddDoesNotExpireTheStoredValue	fail	pass
TestLockIsNotReleasedByAFailedGet	fail	pass
TestPutAfterFlushKeepsTheNewValue	fail	pass
TestPutExtendsTheExpiration	fail	pass
TestStoreWithMemoryKeepsLimiting	fail (54 > 20)	pass
go test ./cache/ -race	fail	pass

go test ./cache/ -race failed on master because TestIncrementWithConcurrent
and TestDecrementWithConcurrent share one err across goroutines and assert
inside them; both now count failures and assert on the test goroutine. The cache,
http, queue, schedule, session and auth suites pass.

Scope

• cache/memory.go: expiry stored with the value, timers removed
• cache/memory_test.go: regression tests for the three cases, a concurrent
  Add test, and the race fix in the two existing concurrent tests
• http/limit/store_memory_test.go: the limiter against the real memory driver

No public API changed. Nothing visible to callers changed: an expired key is
still removed in the background, by a sweep instead of a per-key timer.

✅ Checks

• Added test cases for my code

[codecov]

Codecov Report

❌ Patch coverage is 97.61905% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.73%. Comparing base (5af7aae) to head (87b3af4).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
cache/memory.go	97.61%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
+ Coverage   72.70%   72.73%   +0.02%     
==========================================
  Files         412      412              
  Lines       26786    26820      +34     
==========================================
+ Hits        19476    19507      +31     
- Misses       7308     7311       +3     
  Partials        2        2              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[goravel-coder]

🤖 Automated review

This is an AI-generated code review. Please double-check each finding before acting.

Summary

Replaces time.AfterFunc-scheduled eviction in the memory cache with an expiry stored alongside each value, enforced lazily via sync.Map atomics (LoadOrStore/CompareAndSwap/CompareAndDelete). The correctness fixes (failed Add/Lock.Get, Put after Flush, TTL extension, limiter) are sound and well-tested, but removing the timers trades them for an unbounded memory leak for keys that are never read again.

Verdict

• Must Fix: 1 · Should Fix: 4 · Nits: 5

Findings

Must Fix

1. cache/memory.go:272 Expired entries are never reclaimed unless their key is read again — with the timers gone, an expired item is only deleted by load (line 279/281) or overwritten by Add's CAS (line 47). A key written once with a TTL and never touched again (rate-limiter buckets for clients that stop calling, one-shot tokens, cold Put/Remember values) stays in instance for the life of the process; previously the timer reclaimed it at expiry. Memory has no janitor/Close lifecycle, so this is a real memory-reclaim regression in the default store. Suggestion: add a bounded periodic sweep (or amortized sweep on write) that ranges instance and CompareAndDeletes expired items; if lazy eviction is intentional, document the contract and that Memory is unsuitable for unbounded distinct-key churn.

Should Fix

1. cache/memory.go:32 Add allocates an *item and reads the clock on every call, including the failure path — newItem runs before LoadOrStore, so every rejected Add (a lock already held; Lock.BlockWithTicker polls every 10ms) allocates and calls time.Now() for nothing, versus the old code boxing the caller's value with no extra allocation. Suggestion: Load first and build fresh only once the key is absent or expired, keeping the CAS loop.
2. cache/memory.go:199 Put heap-allocates an *item even when t == NoExpiration — Forever, RememberForever, and the Add counters in Increment/Decrement (lines 59, 164) carry no expiry yet pay for a wrapper they never need. Suggestion: store the bare value when t == NoExpiration and handle both shapes with a comma-ok assertion in load.
3. cache/memory_test.go:224 TestAddWithConcurrent lacks a start barrier, so the CAS race it targets is not reliably exercised — goroutines are launched in a loop and may run after the winner already replaced the item, exiting via !cached.expired(); a non-atomic replacement could still yield exactly one success. Suggestion: gate all goroutines on a shared start := make(chan struct{}) closed after they are all launched so they contend on the same expired pointer.
4. http/limit/store_memory_test.go:41 assert.Nil does not stop the test, so a constructor error proceeds with a nil driver — if cache.NewMemory fails, memoryCache{memory} wraps a nil *Memory and store.Take panics, masking the real failure. Suggestion: use require.NoError(t, err) (same for the assert.Nil(t, err) at line 54).

Nits

1. cache/memory.go:43 Unchecked *item type assertion (also at line 278) panics on any unexpected map value — currently every writer wraps in *item, so it cannot trigger, but a future writer would panic every concurrent reader instead of degrading. Suggestion: use the comma-ok form and treat a mismatch as a miss.
2. cache/memory.go:267 Receiver r on item.expired collides with *Memory's receiver — it reads as if r were a Memory. Suggestion: rename to i and use i.expireAt.
3. cache/memory.go:281 No test distinguishes CompareAndDelete from an unconditional Delete in load — weakening the guard would still pass while deleting a value stored by a concurrent Put. Suggestion: add a test racing a Get/Has on an expired key with a Put of a fresh value and assert the fresh value survives.
4. http/limit/store_memory_test.go:63 Only an upper bound is asserted, so a limiter that rejects every request also passes — assert.LessOrEqual(count, tokens) cannot distinguish correct limiting from Take always returning ok=false. Suggestion: also assert admission (e.g. at least one round reaches tokens allowed).
5. http/limit/store_memory_test.go:10 Test imports the concrete cache package into http/limit, which otherwise depends only on contracts/cache — a test-only reverse dependency that could become an import cycle. Suggestion: move this real-driver case into the tests/ module, or use a minimal local fake.

Automated Checks

• gofmt -l ./cache/ ./http/limit/ — clean
• go vet ./cache/... ./http/limit/... — clean
• go test ./http/limit/ -run TestStoreWithMemoryKeepsLimiting -count=1 — pass

[darakanoit]

Thanks. The memory one was real, I reproduced it: 1000 keys with a 10ms TTL are still in the map after they expire, and 1000 unrelated writes afterwards do not touch them. A write now sweeps the map at most once a minute, in the background, since Memory has no shutdown hook for a janitor.

Taken as well: the Load before building the item in Add, the comma-ok assertions, the receiver on item.expired, require in the limiter test, and the lower bound on what the limiter admits.

Two I did differently. The start barrier alone did not exercise the swap. With the CompareAndSwap replaced by a plain Store the test still passed, because a single round is won before most of its goroutines have started. It now repeats the contention over 500 rounds of eight, which catches that in about fifteen percent of the rounds. The same applies to the CompareAndDelete test: the window is a few instructions wide, so the two goroutines are released together and the race is repeated 5000 times; an unconditional Delete loses a value about six times in a thousand.

Two I left alone. Storing a bare value when t == NoExpiration would put two shapes in the map, and every writer storing an *item is what makes the CompareAndSwap and CompareAndDelete simple to reason about, and it also works against the comma-ok suggestion above it. And there is no import cycle to avoid in the limiter test: cache reaches only contracts/http, and Go would break the build immediately rather than degrade quietly. tests/ is a separate module for the database suites, and a fake would remove the only thing the test is for, which is running the limiter against the real driver.

One thing the review did not catch: that limiter test was flaky. The bucket counts the interval it serves a request in, the loop counted the one it sent it in, so a request that waited for the lock across a boundary could record 21 takes against a limit of 20. It failed once under -race in a full run. Requests that cross a boundary are no longer counted; the test still fails on master with 55 against a limit of 20.

[hwbrzzl]

Thanks, @darakanoit CI failed.

[darakanoit]

My fault, and it is the tests I added rather than the cache. They stored an item with a one nanosecond TTL and treated it as expired, but time.Now() advances in milliseconds on Windows, so a round ran inside a single tick and the item still counted as live. They now store it with an expiry already in the past. Only the two windows jobs failed; everything else was green.

[hwbrzzl]

Thanks, LGTM