Skip to content

workloadmanager: remove session when its Sandbox is deleted - #496

Open
GuTS805 wants to merge 1 commit into
volcano-sh:mainfrom
GuTS805:fix/session-cleanup-on-sandbox-delete
Open

GuTS805 wants to merge 1 commit into
volcano-sh:mainfrom
GuTS805:fix/session-cleanup-on-sandbox-delete

Conversation

@GuTS805

@GuTS805 GuTS805 commented Oct 30, 2026

Copy link
Copy Markdown

What type of PR is this?
/kind bug

What this PR does / why we need it:
When a Sandbox or SandboxClaim is deleted by anything other than GC or the session DELETE API (e.g. kubectl delete sandbox, or deleting a warm-pool CodeInterpreter), its session stays in the store. The Router keeps proxying to the old pod and returns 502/503/504 instead of 404 SESSION_NOT_FOUND, and every retry refreshes last activity, so idle GC never removes it.

This PR watches Sandbox and SandboxClaim deletions in the workload manager and removes the session named by the runtime.agentcube.io/session-id label. The entry is only removed if its kind, namespace and name still match the deleted object.

Which issue(s) this PR fixes:
Fixes #469

Special notes for your reviewer:

  • Tested on a kind cluster. Before: after kubectl delete sandbox, the key stayed in Redis, the Router returned 504, and the last-activity score kept moving. After: 404 SESSION_NOT_FOUND and the key is removed. Deleting a warm-pool CodeInterpreter also removes its claim's session.
  • Normal session DELETE and GC are unchanged. The store delete is idempotent, so a second delete is harmless.
  • If the workload manager is down when the Sandbox is deleted, the event is missed and the entry is still removed by TTL/GC as before.
  • go test -race ./pkg/workloadmanager/, golangci-lint and the e2e suite pass locally.

Does this PR introduce a user-facing change?:

Sessions whose Sandbox or SandboxClaim is deleted outside AgentCube now return 404 SESSION_NOT_FOUND instead of 502/503/504.

Store entries were only removed by GC, the create rollback and the
session DELETE API. When a Sandbox or SandboxClaim was deleted any
other way, such as kubectl delete or a CodeInterpreter deletion
cascading to its claims, the session stayed in the store. The Router
kept proxying to the old pod and returned 502/503/504 instead of
404 SESSION_NOT_FOUND, and each retry refreshed last activity, so
idle GC never picked the entry up.

Watch Sandbox and SandboxClaim deletions and remove the session named
by the session-id label, but only when the stored entry still points
at the deleted object.

Signed-off-by: GuTS805 <alok020505@gmail.com>
@volcano-sh-bot volcano-sh-bot added the kind/bug Something isn't working label Oct 30, 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 acsoto 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

@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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.74%. Comparing base (f521ae9) to head (5a3b3e8).
⚠️ Report is 22 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #496      +/-   ##
==========================================
+ Coverage   60.45%   61.74%   +1.29%     
==========================================
  Files          35       35              
  Lines        3616     3691      +75     
==========================================
+ Hits         2186     2279      +93     
+ Misses       1211     1186      -25     
- Partials      219      226       +7     
Flag Coverage Δ
unittests 61.74% <100.00%> (+1.29%) ⬆️

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.

@acsoto

acsoto commented Oct 31, 2026

Copy link
Copy Markdown
Member

generally LG

@GuTS805

GuTS805 commented Oct 31, 2026

Copy link
Copy Markdown
Author

Thanks! LMK if anything needs changing.

@zhzhuang-zju

Copy link
Copy Markdown
Contributor

Hi @GuTS805, thanks for working on this. The session endpoint may also become stale when:

  • a Pod is replaced with one that has a different IP; or
  • an adopted Sandbox is deleted while its SandboxClaim remains and binds to a new Sandbox.

In these cases, the session record may still contain the old Pod IP. Should we handle these cases in this PR as well?

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.

Session stays in the store after its Sandbox is deleted outside GC; Router returns 502 instead of SESSION_NOT_FOUND

5 participants