[ericcurtin]

Summary

Python E2E cleanup no longer fails when Kubernetes pod termination outlasts the 30s delete deadline.

Related Issue

Closes #4685

Changes

• Add opt-in delete_timeout_seconds to Sandbox. The default is unchanged.
• Add timeout_seconds kwarg to SandboxClient.delete and SandboxSession.delete.
• The E2E fixture sets it to 120s.

Testing

• Checks appropriate to the affected code and behavior pass
• Unit tests added/updated (if applicable)
• E2E tests added/updated (if applicable)

Ran pytest python/, ruff and ty. Not run against a Kubernetes gateway.

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable)

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[ericcurtin]

@krishicks @matthewgrossman could you please triage #4685 so the issue check can pass? @drew @sjenning PTAL, and /ok to test 14c6a40de767efec65358788973c23b0491590cf if it looks good. Thank you!

[TaylorMutch]

/ok to test 14c6a40

[github-actions]

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. python-sdk — No explicit risk acceptance found

SandboxSession.delete() changed from calling SandboxClient.delete(..., allow_missing=...) to always adding timeout=None when the argument is omitted.

• Impact: Existing SandboxClient subclasses that retain the baseline delete signature now raise TypeError on an unchanged SandboxSession.delete() call.
• Action: When timeout is None, call SandboxClient.delete without the new keyword and forward it only when a caller explicitly opts in.
• PR: fix(python): add opt-in sandbox delete timeout #4697

Review details

• Execution: complete
• Candidate: PR #4697 (dac79c70d5557ecf41c9cae7e4d873dc5dffb552)
• Baseline: 55bf786453395f3fe1858a187152537b457de77d (55bf786453395f3fe1858a187152537b457de77d)
• Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

[TaylorMutch]

@ericcurtin please see the compatibility review and address the concern. Thanks!

[ericcurtin]

@TaylorMutch Fixed: timeout is now forwarded only when set. @krishicks @matthewgrossman PTAL, and /ok to test 42bcba0f0 if it looks good. Thank you!

[TaylorMutch]

/ok to test 42bcba0

[copy-pr-bot]

/ok to test 42bcba0

@TaylorMutch, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

[TaylorMutch]

/ok to test 42bcba0

[TaylorMutch]

@TaylorMutch Fixed: timeout is now forwarded only when set. @krishicks @matthewgrossman PTAL, and /ok to test 42bcba0f0 if it looks good. Thank you!

Just FYI we can't use short sha's anymore on /ok to test.

[TaylorMutch]

@ericcurtin please see the branch check failures

[ericcurtin]

@TaylorMutch Fixed the typecheck failure. /ok to test dd5dfad875d2fbed89a938f6ec72e1b7577752db if it looks good. Thank you!

[JohnnyWilson16]

Tested commit dd5dfad locally:

Backward compatibility verified: SandboxSession.delete() correctly omits the keyword when timeout=None, preserving compatibility with legacy client subclasses.
Defaults preserved: standard callers remain bound to 30s; delete_timeout_seconds=120.0 in e2e/python/conftest.py resolves the Kubernetes pod termination race.
Passing unit tests verified in python/openshell/sandbox_cleanup_test.py and python/openshell/sandbox_test.py.
Looks good to land.

[TaylorMutch]

/ok to test dd5dfad

[ericcurtin]

@TaylorMutch @mrunalp PTAL when you get a chance. Thank you!

[ericcurtin]

@drew @johntmyers PTAL when you get a chance. CI is green, only a maintainer approval is missing. Thank you!

[rhuss]

cc-review Summary

Thanks for this! It is a small, well-scoped fix for #4685, and the opt-in approach matches what the issue asks for.

This is an agent-assisted review, produced with cc-review and several reviewer agents. The failure scenarios in the comments were derived from reading the code and were not executed.

All findings are non-blocking suggestions (Minor or Notable). The two I'd look at first are the test tightening on python/openshell/sandbox_cleanup_test.py:185 and whether the PR should close #4685 before a Kubernetes run is shown. Once those are settled, this looks ready for a maintainer to me.

What Went Well

• SandboxSession.delete (python/openshell/sandbox.py:629-639) forwards timeout only when it is set, so existing SandboxClient subclasses with the old signature keep working. That cleanly answers the compatibility review, and test_sandbox_session_delete_omits_timeout_for_legacy_clients (python/openshell/sandbox_test.py:3095-3114) pins the contract.
• SandboxClient.delete (python/openshell/sandbox.py:1096) scopes the override to the single DeleteSandbox call, falls back to the client timeout, and still honors an explicit 0.
• delete_timeout_seconds defaults to None next to ready_timeout_seconds (python/openshell/sandbox.py:1602), so the SDK default stays unchanged as #4685 requires, and __exit__ keeps the delete inside try/finally, so the client is released even on DEADLINE_EXCEEDED.
• test_context_cleanup_delete_timeout_is_opt_in drives the real intercepted gRPC stub against an in-process server with a controlled delay, and the new delete_delay defaults to 0.0, so the existing cleanup tests are unaffected.
• The 120s value is scoped to the e2e fixture with a comment explaining why (e2e/python/conftest.py:98-100), instead of raising the SDK-wide default.

Findings

Severity	File	Description	Source
Minor	e2e/python/conftest.py:100	Direct sandbox_client.delete calls in other e2e tests keep the 30s default (follow-up)	gator
Notable	e2e/python/conftest.py:100	Kubernetes run for #4685 criterion 4 not shown, but the PR closes the issue	goal-alignment
Minor	python/openshell/sandbox.py:1087	Naming nit: per-call timeout vs delete_timeout_seconds / timeout_seconds	architecture
Notable	python/openshell/sandbox.py:1087	New public timeout keyword on both delete methods is not listed in the PR description	goal-alignment
Minor	python/openshell/sandbox.py:1602	Kubernetes users may not discover delete_timeout_seconds; a docs note would help	gator
Notable	python/openshell/sandbox.py:1733	wait_deleted on the ACCEPTED path keeps its fixed 60s bound (informational)	architecture
Minor	python/openshell/sandbox_cleanup_test.py:165	No case where the configured delete_timeout_seconds itself expires (#4685 criterion 3)	goal-alignment
Minor	python/openshell/sandbox_cleanup_test.py:185	Short deadline also covers __enter__, and the test never asserts DeleteSandbox ran	test-quality

All findings are posted as inline comments.

Review Details

• Findings posted: 8
• Findings reviewed and not posted: 0
• Gate outcome: PASS (no Critical or Important findings)
• Participating reviewers: architecture, goal-alignment, test-quality, gator (correctness, production and security also ran and reported no findings)

[ericcurtin]

Updated: cleanup tests now put only the exit delete under the short deadline and cover delete_timeout_seconds expiring. Documented timeout on SandboxClient.delete and added a docs note on larger delete timeouts for Kubernetes. PR body now says "Part of #4685" since no Kubernetes run was done.

[ericcurtin]

Merged main, one docs conflict resolved. Tests pass.

[ericcurtin]

@pimlock @alangou PTAL when you get a chance, and /ok to test d7a64794187ef1172d9b3c9f589d12b14b0e175a if it looks good. Main is merged and the review notes are addressed. The issue check wants a closing keyword but this covers part of #4685, so please say if you would rather I switch it. Thank you!

[ericcurtin]

@rhuss @TaylorMutch PTAL, review notes addressed. Could one of you comment ok to test 91ff960? Thank you!

[TaylorMutch]

/ok to test 91ff960

[github-actions]

Label test:e2e applied for 91ff960. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

[TaylorMutch]

Validated the Python SDK and E2E fixtures at commit 91ff960da7a0ca605968590aa1c4f9b0d5acfeaf against a local Kubernetes-backed gateway.

• Environment: two-node k3d cluster, Kubernetes v1.33.4+k3s1; isolated Helm release using the PR's chart and published gateway/supervisor/sandbox images 0.1.8-pre.3.
• e2e/python/test_exec_admission.py: 5 passed in 211.93 seconds, run serially.
• e2e/python/test_sandbox_api.py::test_sandbox_labels_and_selectors: 1 passed, covering direct client deletes.
• All five context-manager cleanup RPCs completed successfully in 31.3–33.0 seconds, exceeding the previous 30-second default while remaining within the fixture's 120-second deadline.
• Both direct deletes also succeeded using the session-scoped client's 120-second timeout.

The tests used the PR's fixtures without test-local timeout overrides. A temporary timing wrapper observed SandboxClient.delete duration and the effective deadline without changing either. This exercises the Kubernetes cleanup behavior reported in #4685, rather than relying only on Docker or mocked RPCs.

Commands, against the isolated registered Kubernetes gateway:

E2E_PARALLEL=1 uv run pytest -o python_files='test_*.py *_test.py' -n 0 e2e/python/test_exec_admission.py
E2E_PARALLEL=1 uv run pytest -o python_files='test_*.py *_test.py' -n 0 e2e/python/test_sandbox_api.py::test_sandbox_labels_and_selectors

Temporary test resources were removed afterward. The test:e2e CI rerun is still in progress.

[ericcurtin]

@TaylorMutch the two k3s E2E failures are the get.k3s.io installer checksum, fixed on main by #5223. I can merge main, but that dismisses your approval, so shall I? Thank you!