fix(sdk): SimpleBox auto_remove leaks boxes over REST (Python + Node) - #1186
Open
G4614 wants to merge 3 commits into
Open
fix(sdk): SimpleBox auto_remove leaks boxes over REST (Python + Node)#1186G4614 wants to merge 3 commits into
G4614 wants to merge 3 commits into
Conversation
`Box.__aexit__` (box_handle.rs) only ever called `handle.stop()`. Deletion
was assumed to come from `BoxOptions.auto_remove`, but that field is a
deprecated, REST-silent no-op (rest/types.rs never transmits it) - local
runtimes self-delete on stop internally, which is why this only leaks
remotely. Every box `apps/e2e/cases/test_sdk_tunnel.py` created via
SimpleBox was left `Stopped` forever on the dev cloud environment; a full
e2e run there confirmed exactly 5 leaked boxes, matching that file's 5
SimpleBox call sites 1:1.
SimpleBox.__aexit__ / SyncSimpleBox.__exit__ now explicitly call
runtime.remove() after stop() when auto_remove was requested and this
instance created the box (a box reused via reuse_existing=True may still
be open in an outer session, so an inner exit must not delete it). The
remove call retries once after a 1s backoff, since stop() can leave the
box briefly `pending` server-side and force=True does not bypass that
guard (the REST controller never reads the `force` query param on delete).
Before:
SimpleBox.__aexit__ (simplebox.py)
-> Box.__aexit__ (box_handle.rs) <- BUG: only stops, never deletes
-> LiteBox.stop -> RestBox::stop (REST: auto_remove field is dropped)
After:
SimpleBox.__aexit__ (simplebox.py)
-> Box.__aexit__ (box_handle.rs) - still just stops the VM
-> SimpleBox._remove_after_stop (simplebox.py) [new]
-> Boxlite.remove (runtime.rs) -> DELETE /boxes/{id}
retries once (1s) past the post-stop `pending` window
Regression coverage: apps/e2e/cases/test_simplebox_lifecycle.py, verified
failing against the pre-fix code and passing after, against the dev cloud
API. Re-ran test_sdk_tunnel.py (the file that originally leaked) after the
fix: dev cloud box count returned to 0.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesSimpleBox cleanup lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant SimpleBox
participant Runtime
Test->>SimpleBox: Exit context or call stop
SimpleBox->>Runtime: Stop box
SimpleBox->>Runtime: Retry removal for newly created box
Runtime-->>SimpleBox: Removal result
SimpleBox-->>Test: Return after cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
G4614
marked this pull request as ready for review
August 10, 2026 05:57
📦 BoxLite review — couldn't completepowered by BoxLite |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/e2e/cases/test_simplebox_lifecycle.py`:
- Around line 31-35: Update the get_info polling loop in the lifecycle deletion
helper to return true only for the REST not-found condition (HTTP
404/not_found). Add or propagate a typed not-found signal through the REST
runtime and Python SDK, then retry transient failures and re-raise all other
errors instead of treating every exception as deletion.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2277052-1249-4382-a740-606c93733719
📒 Files selected for processing (3)
apps/e2e/cases/test_simplebox_lifecycle.pysdks/python/boxlite/simplebox.pysdks/python/boxlite/sync_api/_simplebox.py
1 task
Same bug as the Python side, same fix. SimpleBox.stop() only ever called
this._box.stop(); autoRemove on JsBoxOptions is a deprecated field REST
runtimes silently ignore (local runtimes self-delete on stop internally,
which is why this only leaks remotely). Confirmed with a live repro
against api.dev.boxlite.ai: the pre-fix box stayed Stopped forever after
stop(); with the fix it's gone within the same 15s window.
stop() now explicitly calls runtime.remove() after stopping when
autoRemove was requested and this instance created the box (one reused
via reuseExisting may still be open in an outer session). Retries once
after a 1s backoff for the same post-stop `pending` race the Python fix
retries.
Added apps/e2e/sdks/node/e2e_simplebox_lifecycle.ts + cases/test_node_simplebox_lifecycle.py,
mirroring test_simplebox_lifecycle.py through the existing cross-language
Node e2e driver pattern (cases/test_node_tunnel.py's).
Test plan:
- [x] Two-side verified against the dev cloud API via
test_node_simplebox_lifecycle.py: fails on the pre-fix code
("box ... still present after stop() with autoRemove=true"),
passes after.
- [x] cd sdks/node && npx tsc --noEmit
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
G4614
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/e2e/sdks/node/e2e_simplebox_lifecycle.ts (1)
49-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a reused-box retention case.
The test does not verify the
reuseExisting: truecontract from this PR. Create an outer named box. Create an innerSimpleBoxwith the same name,reuseExisting: true, andautoRemove: true. Afterinner.stop(), assert that the outer box still exists. Clean up the outer box explicitly.🤖 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 `@apps/e2e/sdks/node/e2e_simplebox_lifecycle.ts` around lines 49 - 60, Extend the lifecycle test around the existing keptBox scenario with a reused-box retention case: create an outer named box, then an inner SimpleBox using the same name with reuseExisting: true and autoRemove: true; stop the inner box and assert the outer box still exists. Explicitly remove the outer box during cleanup.
🤖 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 `@apps/e2e/sdks/node/e2e_simplebox_lifecycle.ts`:
- Around line 20-24: Update the cleanup helper around runtime.getInfo() so it
returns true only when getInfo(id) returns null and propagates transport,
authorization, and other unexpected errors; at
apps/e2e/sdks/node/e2e_simplebox_lifecycle.ts lines 20-24, remove the catch-all
success path. At lines 55-60, retry only the expected transient or pending-state
failure from remove(), then throw the final failure instead of swallowing it.
---
Nitpick comments:
In `@apps/e2e/sdks/node/e2e_simplebox_lifecycle.ts`:
- Around line 49-60: Extend the lifecycle test around the existing keptBox
scenario with a reused-box retention case: create an outer named box, then an
inner SimpleBox using the same name with reuseExisting: true and autoRemove:
true; stop the inner box and assert the outer box still exists. Explicitly
remove the outer box during cleanup.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7fd773c-b4a6-4afc-b3fd-dd89fc92f10c
📒 Files selected for processing (3)
apps/e2e/cases/test_node_simplebox_lifecycle.pyapps/e2e/sdks/node/e2e_simplebox_lifecycle.tssdks/node/lib/simplebox.ts
CodeRabbit on boxlite-ai#1186: both _box_gone() (Python) and boxGone() (Node) caught *any* get_info failure and returned "gone" - a pending-state error, transport blip, or unrelated server error would make the deletion assertion pass while the box still exists, silently hiding a real leak instead of catching one. Both now only treat a REST not-found (matched by message, since neither SDK exposes a typed not-found error over REST) as deletion; every other failure is retried until the timeout, then re-raised/rethrown instead of being swallowed. The Node driver's own cleanup remove() at exit no longer swallows its error either, for the same reason. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
|
this should be implemented in core side. seems cloud didn't honor the flag |
DorianZheng
requested changes
Aug 11, 2026
DorianZheng
left a comment
DorianZheng
left a comment
Member
There was a problem hiding this comment.
Please check the root cause
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SimpleBox(Python and Node) now deletes the box on exit whenauto_remove/autoRemoveis true, instead of only stopping it (a no-op over REST that left every such boxStoppedforever on the dev cloud environment).Test plan:
apps/e2e/cases/test_simplebox_lifecycle.pyagainst the dev cloud API — fails on the pre-fix Python code, passes afterapps/e2e/cases/test_node_simplebox_lifecycle.pyagainst the dev cloud API — fails on the pre-fix Node code, passes afterapps/e2e/cases/test_sdk_tunnel.pyagainst the dev cloud API — dev box count returns to 0 after the runcd sdks/node && npx tsc --noEmitSummary by CodeRabbit
New Features
Bug Fixes
Tests