test(redis-lua): drop flaky pool.Hits() assertion in VMReuse leak test#583
Conversation
CI on PR #582 failed with: --- FAIL: TestLua_VMReuseDoesNotLeakGlobals (0.00s) Error: "0" is not greater than or equal to "1" redis_lua_pool_test.go:112 The assertion `require.GreaterOrEqual(t, pool.Hits(), uint64(1))` requires the Script-B `pool.get(nil)` to return the exact `pooledLuaState` that Script-A put back moments earlier. sync.Pool is explicitly allowed to evict pooled items under GC pressure between Put and Get (the comment above line 80 acknowledges this non-determinism for pointer identity, but then contradicts itself by asserting a hit counter that has the SAME source of non-determinism). Under GHA's -race runtime + whatever GC triggers during the test, the Script-B Get sometimes returns a fresh allocation (miss), so Hits() stays at 0 and the assertion fails. Pool effectiveness already has its own dedicated deterministic test: TestLua_PoolRecordsReuseVsAllocation asserts hits + misses across a controlled loop, which is resilient to sync.Pool eviction. This test is about the security invariant ("no globals leak across pooled states") — the hit counter line does not contribute to that and only adds flake. Remove it; leave a comment pointing to the deterministic test. Verified with `go test -race -count=20 -run TestLua_VMReuseDoesNotLeakGlobals ./adapter/` 20/20 green and the full `-short` adapter suite 63s green.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes a non-deterministic assertion on pool.Hits() in TestLua_VMReuseDoesNotLeakGlobals to prevent CI flakes caused by sync.Pool evictions under GC pressure. Feedback was provided to correct inaccuracies in the added explanatory comment regarding line references and the behavior of related tests, including a suggestion for a more concise and accurate description.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
Summary
Drop the
require.GreaterOrEqual(t, pool.Hits(), uint64(1))line fromTestLua_VMReuseDoesNotLeakGlobals. It was flaking on CI (PR #582 run24781704678), and the assertion is inherently racy onsync.Pool.Root cause
The assertion required the Script-B
pool.get(nil)to return exactly thepooledLuaStatethat Script-A put back moments earlier. Butsync.Poolis documented to discard pooled items under GC pressure, which can easily fire between a Put and a Get on the race runtime. The test's own comment above line 80 already acknowledges this non-determinism — but then the Hits assertion at line 112 reintroduces the same source of flake.Change
TestLua_VMReuseDoesNotLeakGlobals.GLOBAL_LEAK,LEAKY_TABLEacross pooled states; whitelisted globals still intact).TestLua_PoolRecordsReuseVsAllocationwhich assertshits + missesdeterministically across a controlled loop.Verification
go test -race -count=20 -run TestLua_VMReuseDoesNotLeakGlobals ./adapter/→ 20/20 passgo test -race -count=1 -short ./adapter/...→ ok in 63smake lintcleanPair with PR #582 (multi-exec flake) to unblock the CI pipeline on
perf/raft-dispatcher-lanesandfix/idempotent-rollback-marker.