Skip to content

store: remove orphaned session index entries during GC listing - #495

Open
CapThunder19 wants to merge 1 commit into
volcano-sh:mainfrom
CapThunder19:fix/gc-orphaned-index-entries
Open

CapThunder19 wants to merge 1 commit into
volcano-sh:mainfrom
CapThunder19:fix/gc-orphaned-index-entries

Conversation

@CapThunder19

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it

When an entry in session:expiry / session:last_activity has lost its session:<id> key, loadSandboxesBySessionIDs skips it but never removes it. These orphans always have the oldest scores, so they sit at the front of every GC query and take up candidate slots for good. Once 100 have built up, GC returns nothing on every cycle and expired or idle sandboxes stop being cleaned up. CodeInterpreter claims are hit hardest, since GC is the only thing that removes them.

This PR removes orphans from both indexes when they're found, using a small Lua script that first re-checks the key is still missing, so a session created at that moment can't lose its entries. Redis and Valkey both get the fix. If the cleanup fails, it only logs a warning and GC carries on as before.

Which issue(s) this PR fixes

Fixes #491

Special notes for your reviewer

  • Added tests for both backends: 100 orphans in front of a real expired session (GC now reaches it after one call), plus a check that sessions whose key exists keep their index entries. The orphan tests fail without the fix.
  • I also ran an end-to-end check with the real garbageCollector.once(): without the fix the claim was still alive after 10 cycles; with it, it was deleted in the first cycle.
  • Related to GC can starve eligible sessions behind the 100-item candidate limit #454, but the cause is different. Paging alone would skip past orphans, not remove them.
  • If Valkey runs in cluster mode, the cleanup fails because the keys can span slots. It only logs a warning, and the existing MGET in the same function already assumes one slot.

Does this PR introduce a user-facing change?

Fixed garbage collection stalling when orphaned session index entries accumulate in Redis/Valkey.

Index members whose session key no longer exists were skipped but never removed. They keep the oldest scores, so once 100 build up they fill the GC candidate window and GC stops collecting expired and idle sandboxes.

Remove them from both indexes with a Lua script that re-checks the key is still missing.

Signed-off-by: Anirudh <anirudhpatwal19@gmail.com>
Copilot AI balanced review requested due to automatic review settings October 29, 2026 07:07
@volcano-sh-bot volcano-sh-bot added the kind/bug Something isn't working label Oct 29, 2026
@volcano-sh-bot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yaozengzeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.68%. Comparing base (f521ae9) to head (b144f7a).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
pkg/store/store_redis.go 83.33% 2 Missing and 1 partial ⚠️
pkg/store/store_valkey.go 81.25% 2 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #495      +/-   ##
==========================================
+ Coverage   60.45%   61.68%   +1.23%     
==========================================
  Files          35       35              
  Lines        3616     3693      +77     
==========================================
+ Hits         2186     2278      +92     
+ Misses       1211     1187      -24     
- Partials      219      228       +9     
Flag Coverage Δ
unittests 61.68% <82.35%> (+1.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Comment thread pkg/store/store_valkey.go
return sandboxResults, nil
}

var removeOrphanedIndexEntriesValkeyScript = valkey.NewLuaScript(removeOrphanedIndexEntriesLua)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var removeOrphanedIndexEntriesValkeyScript = valkey.NewLuaScript(removeOrphanedIndexEntriesLua)
// In cluster mode, all keys accessed by this Lua script must hash to the same slot.
var removeOrphanedIndexEntriesValkeyScript = valkey.NewLuaScript(removeOrphanedIndexEntriesLua)

As noted in #75 (comment), we currently only support master/slave or single redis/valkey. Add a comment here to flag this constraint for future cluster support. The same as:

// MGet should in same slot
stingSliceResults, err := vs.cli.Do(ctx, vs.cli.B().Mget().Key(sessionIDKeys...).Build()).AsStrSlice()

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GC stalls when orphaned session index entries pile up

5 participants