Skip to content

fix(sandbox): default the pool size when tearing down, as startup does - #18629

Open
marmar9615-cloud wants to merge 2 commits into
infiniflow:mainfrom
marmar9615-cloud:fix/sandbox-stop-pool-size-default
Open

fix(sandbox): default the pool size when tearing down, as startup does#18629
marmar9615-cloud wants to merge 2 commits into
infiniflow:mainfrom
marmar9615-cloud:fix/sandbox-stop-pool-size-default

Conversation

@marmar9615-cloud

Copy link
Copy Markdown
Contributor

Summary

Startup and teardown disagree about how many sandbox containers exist.

scripts/start.sh:26 applies a default whether or not .env supplies the value, and :32 applies the same one when .env is absent:

SANDBOX_EXECUTOR_MANAGER_POOL_SIZE="${SANDBOX_EXECUTOR_MANAGER_POOL_SIZE:-5}" # Default to 5 if not set in .env

scripts/stop.sh:29 and the clean target in Makefile both source .env and then use the variable bare:

source .env
for i in $(seq 0 $((SANDBOX_EXECUTOR_MANAGER_POOL_SIZE - 1))); do

If .env exists but does not set SANDBOX_EXECUTOR_MANAGER_POOL_SIZE, the arithmetic evaluates to -1, so the range is 0..-1 and the pool containers start.sh created are not removed. Both paths then print their success line regardless — ✅ Stopping and cleanup complete — so nothing signals that the cleanup did not happen.

This mirrors the default start.sh already applies. An .env that sets the value is unaffected:

.env loop bound before loop bound after
omits the variable -1 4 (indices 0-4, matching start.sh)
sets it to 8 7 7

Scope note: this is deliberately only the missing default. stop.sh also skips cleanup entirely when .env is absent while start.sh proceeds on defaults, but changing that alters behavior rather than restoring the two into agreement, so it is left alone here.

.env.example does ship the variable at line 3, so a .env copied from it is already fine. This is about the case where it has been trimmed or hand-written, where the current failure is silent.

Verified against upstream/main at e9cace9a. bash -n agent/sandbox/scripts/stop.sh passes and make -n clean expands as intended.

start.sh defaults SANDBOX_EXECUTOR_MANAGER_POOL_SIZE to 5 whether or not .env
supplies it, so it creates and cleans indices 0..4. stop.sh and the Makefile
clean target source .env and then use the variable bare, so an .env that omits
it leaves the range 0..-1 and the containers startup created are not removed,
while both still report cleanup complete.

Mirror the default start.sh already applies. An .env that sets the value is
unaffected.
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. 🐞 bug Something isn't working, pull request that fix bug. labels Aug 21, 2026
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 54ad7445-8ad2-4c8d-b453-97621b257604

📥 Commits

Reviewing files that changed from the base of the PR and between e9cace9 and f4ce03b.

📒 Files selected for processing (2)
  • agent/sandbox/Makefile
  • agent/sandbox/scripts/stop.sh

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Sandbox cleanup now defaults SANDBOX_EXECUTOR_MANAGER_POOL_SIZE to 5 in both cleanup entry points before container removal.

Changes

Sandbox cleanup

Layer / File(s) Summary
Default cleanup pool size
agent/sandbox/Makefile, agent/sandbox/scripts/stop.sh
Both cleanup paths set SANDBOX_EXECUTOR_MANAGER_POOL_SIZE to 5 when it is unset or empty.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to f4ce0

The change aligns teardown defaults with startup so sandbox containers are cleaned up when the pool size is omitted; no actionable merge-blocking risk remains after normal checks and review.

Poem

A bunny checks the cleanup queue,
Five little workers know what to do.
Empty settings no longer delay,
Sandbox containers hop away.
make clean keeps the path bright.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the sandbox teardown fix and its alignment with startup behavior.
Description check ✅ Passed The description includes the required Summary section and clearly explains the issue, scope, expected behavior, and verification performed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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

Labels

🐞 bug Something isn't working, pull request that fix bug. size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant