test(router): reproduce config-reload plan-cache retention leak#3019
test(router): reproduce config-reload plan-cache retention leak#3019asoorm wants to merge 1 commit into
Conversation
WalkthroughAdds a Go test that exercises router config reload shutdown with a plan cache, a slow fallback cache, and finalizer-based GC checks. The test includes control and retention subtests plus helpers for cache demotion and repeated GC polling. ChangesRouter reload retention test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3019 +/- ##
==========================================
- Coverage 60.27% 54.33% -5.95%
==========================================
Files 477 243 -234
Lines 62471 29465 -33006
Branches 6131 0 -6131
==========================================
- Hits 37656 16010 -21646
+ Misses 24787 11886 -12901
- Partials 28 1569 +1541 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/reload_persistent_state_leak_test.go`:
- Around line 156-163: The regression helper is asserting shutdown behavior too
broadly by manually calling planCache.Close() with the demotion callback still
active, which can mask a valid fix in graphMux.Shutdown. Update the test to
exercise the real shutdown/reload path through graphMux.Shutdown, or narrow the
assertion to only the slowplancache.Close retention contract so it does not
depend on the pre-fix demotion behavior; use the planCache and fallback setup in
reload_persistent_state_leak_test.go to keep the test aligned with the intended
contract.
- Around line 148-149: The retention test around planCache.Set and
planCache.Wait is ignoring whether the canary plan was actually admitted into
the cache. Update the test to check the boolean result returned by planCache.Set
before calling Wait, and fail the test if the canary plan is rejected so the
admission check is explicit and reliable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e78bb9f8-1498-4e1f-ac1a-ffa3fe8c2592
📒 Files selected for processing (1)
router/core/reload_persistent_state_leak_test.go
| planCache.Set(1, plan, 1) | ||
| planCache.Wait() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline router/core/reload_persistent_state_leak_test.go --view expanded || true
printf '\n== relevant file slice ==\n'
nl -ba router/core/reload_persistent_state_leak_test.go | sed -n '120,170p'
printf '\n== cache set usages in repo ==\n'
rg -n "planCache\.Set\(|\.Set\(" router/core -g '*.go' || true
printf '\n== go module references ==\n'
rg -n "ristretto|slowplancache|ReloadPersistentState" go.mod go.sum router -g '*.go' -g 'go.mod' -g 'go.sum' || trueRepository: wundergraph/cosmo
Length of output: 524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline router/core/reload_persistent_state_leak_test.go --view expanded || true
printf '\n== relevant file slice ==\n'
nl -ba router/core/reload_persistent_state_leak_test.go | sed -n '120,170p'
printf '\n== cache set usages in repo ==\n'
rg -n "planCache\.Set\(|\.Set\(" router/core -g '*.go' || true
printf '\n== go module references ==\n'
rg -n "ristretto|slowplancache|ReloadPersistentState" go.mod go.sum router -g '*.go' -g 'go.mod' -g 'go.sum' || trueRepository: wundergraph/cosmo
Length of output: 524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant file slice ==\n'
sed -n '130,160p' router/core/reload_persistent_state_leak_test.go
printf '\n== surrounding helper slice ==\n'
sed -n '100,220p' router/core/reload_persistent_state_leak_test.go
printf '\n== search for immediate Set result handling in repo ==\n'
rg -n "require\.(True|False|NoError)\(.*\.Set\(" router/core -g '*.go' || trueRepository: wundergraph/cosmo
Length of output: 5582
Assert the canary plan was admitted. ristretto.Cache.Set reports whether the item was accepted; ignoring the return value lets Wait() complete even if the plan never enters the cache, which weakens this retention check.
Proposed fix
- planCache.Set(1, plan, 1)
+ require.True(t, planCache.Set(1, plan, 1), "plan cache insertion should be accepted")
planCache.Wait()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| planCache.Set(1, plan, 1) | |
| planCache.Wait() | |
| require.True(t, planCache.Set(1, plan, 1), "plan cache insertion should be accepted") | |
| planCache.Wait() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/core/reload_persistent_state_leak_test.go` around lines 148 - 149, The
retention test around planCache.Set and planCache.Wait is ignoring whether the
canary plan was actually admitted into the cache. Update the test to check the
boolean result returned by planCache.Set before calling Wait, and fail the test
if the canary plan is rejected so the admission check is explicit and reliable.
| // ---- the reload close path (graphMux.Shutdown) ---- | ||
| // Closing the ristretto cache fires OnEvict for every remaining entry, demoting | ||
| // the plan (and its operation document) into the slowplancache fallback. | ||
| planCache.Close() | ||
| // Ensure the demoted Set has been applied before we close the fallback. | ||
| fallback.Wait() | ||
| // slowplancache.Close() only closes channels; it does NOT clear stored entries. | ||
| fallback.Close() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Avoid freezing the pre-fix shutdown behavior in the regression helper.
This manually calls planCache.Close() with the demotion callback still installed, so a valid production fix in graphMux.Shutdown() that suppresses demotion or detaches the fallback on reload would still fail here. Either drive the actual shutdown/reload path or narrow the test to the specific slowplancache.Close retention contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/core/reload_persistent_state_leak_test.go` around lines 156 - 163, The
regression helper is asserting shutdown behavior too broadly by manually calling
planCache.Close() with the demotion callback still active, which can mask a
valid fix in graphMux.Shutdown. Update the test to exercise the real
shutdown/reload path through graphMux.Shutdown, or narrow the assertion to only
the slowplancache.Close retention contract so it does not depend on the pre-fix
demotion behavior; use the planCache and fallback setup in
reload_persistent_state_leak_test.go to keep the test aligned with the intended
contract.
1 participant
What
A deterministic regression test that reproduces the memory retention on router config hot-reload: after a reload, the old query plan cache (and the operation/plan entries it holds) is retained by a Router-lifetime structure and survives GC.
The test is finalizer-based — no production load, no profiling, CI-friendly.
Root cause it covers
OnEvictcallback (when cache warmup in-memory fallback is enabled) that re-homes evicted plans intoplanFallbackCache— aslowplancache.Cache(core/graph_server.go:723-728).reloadPersistentState, which lives on the long-livedRouter, not the per-configgraphMux(core/graph_server.go:1609).graphMux.Shutdown()callsplanCache.Close(); ristrettoClose()→Clear()firesOnEvictfor every entry, dumping the whole plan cache into the fallback.slowplancache.Close()did not clear its stored map, so the Router-lifetime warmer keeps the old entries (and their AST/plan references) indefinitely.Red → green
main— the cachedoperationDocumentstays reachable viaReloadPersistentState → slowplancache, so a finalizer set on it never fires.OnRouterConfigReloadbefore hot-reload), fix(router): skip plan cache OnEvict during graphMux shutdown #3009 (skip plan-cacheOnEvictduring shutdown).A
controlsub-test (no Router-lifetime retention) passes in both states, guarding against a false-positive finalizer mechanism.Run
Why draft
Depends on the linked fix PRs landing — it is intentionally red against
mainuntil then.Closes none. Validates #3006, #3007, #3008, #3009.
Summary by CodeRabbit