Skip to content

Commit 7783c27

Browse files
committed
test(redis-lua): drop flaky pool.Hits() assertion in VMReuse leak test
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.
1 parent e9a2bb9 commit 7783c27

1 file changed

Lines changed: 8 additions & 2 deletions

File tree

adapter/redis_lua_pool_test.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,14 @@ func TestLua_VMReuseDoesNotLeakGlobals(t *testing.T) {
108108
require.NoError(t, stateB.DoString(`assert(string.upper("ok") == "OK")`))
109109
pool.put(plsB)
110110

111-
// Pool should have registered at least one hit by now.
112-
require.GreaterOrEqual(t, pool.Hits(), uint64(1), "pool never reported a hit")
111+
// NOTE: we intentionally do NOT assert pool.Hits() >= 1 here.
112+
// sync.Pool may evict items between Put and Get under GC pressure
113+
// (same non-determinism acknowledged in the comment above line
114+
// 80), so the Script-B Get may be a fresh allocation even though
115+
// Script A's plsA was just Put. CI on GitHub Actions has
116+
// reproduced this as a flake. Pool effectiveness is covered
117+
// deterministically by TestLua_PoolRecordsReuseVsAllocation,
118+
// which asserts hits + misses rather than hits alone.
113119
}
114120

115121
// TestLua_VMReuseRestoresRebindsWhitelistedGlobals guards against a

0 commit comments

Comments
 (0)