[Feat] SandboxManager 구현#32
Open
chowon442 wants to merge 2 commits into
Open
Conversation
Daytona 샌드박스 생성과 cleanup/delete 수명주기를 관리하는 SandboxManager를 추가합니다. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDaytona 샌드박스 래핑 클래스 ChangesCodeExecutor 및 SandboxManager 구현
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/proovy_agent/common/sandbox/manager.py (1)
53-53: 💤 Low value
noqa: ASYNC109주석이 필요한지 재검토하세요.ASYNC109 규칙은 async 함수에 timeout 매개변수가 있지만
asyncio.wait_for()나asyncio.timeout()을 사용하지 않을 때 경고합니다. 그런데 이 함수는 72번 줄에서asyncio.wait_for(_destroy(), timeout=timeout)으로 timeout을 올바르게 사용하고 있으므로, 이 noqa 주석이 불필요할 수 있습니다.🔍 noqa 주석 제거 제안
async def destroy_executor( self, executor: CodeExecutor, *, - timeout: float = 10.0, # noqa: ASYNC109 + timeout: float = 10.0, ) -> None:Ruff를 다시 실행하여 실제로 ASYNC109 경고가 발생하는지 확인하세요. 경고가 없다면 noqa 주석을 제거하는 것이 코드를 더 깔끔하게 만듭니다.
🤖 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 `@src/proovy_agent/common/sandbox/manager.py` at line 53, The ASYNC109 suppression on the timeout parameter appears unnecessary because the function actually uses asyncio.wait_for(_destroy(), timeout=timeout) (see the timeout argument usage), so run Ruff to confirm whether ASYNC109 is reported; if Ruff reports no ASYNC109, remove the "noqa: ASYNC109" comment from the timeout parameter in the function signature (the parameter on the async function that calls asyncio.wait_for/_destroy) so the code is cleaner, otherwise keep the noqa.
🤖 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 `@src/proovy_agent/common/sandbox/executor.py`:
- Line 13: 파일의 sandbox 매개변수와 관련 프로퍼티에 Any를 사용해 타입 안전성이 손실되었으니,
src/proovy_agent/common/sandbox/executor.py의 함수/클래스에서 사용된 sandbox 타입 표기(예: 함수
매개변수 sandbox 및 관련 프로퍼티/멤버)를 Any에서 Daytona의 AsyncSandbox로 바꾸고 파일 상단에 from daytona
import AsyncSandbox를 추가하여 정확한 타입을 사용하도록 수정하세요; 구체적으로 AsyncSandbox를 참조하는 위치는
sandbox 매개변수(현재 선언된 sandbox: Any)와 해당 프로퍼티 선언(현재 Any로 표기된 부분)을 찾아 AsyncSandbox로
교체하면 됩니다.
In `@tests/common/sandbox/test_manager.py`:
- Around line 155-164: The test must assert the call order (cleanup before
delete) for SandboxManager.destroy_executor: modify FakeSandbox and
FakeCodeExecutor to append ordered events to a shared list (e.g.,
events.append("executor.cleanup") in FakeCodeExecutor.cleanup and
events.append("sandbox.delete") in FakeSandbox.delete), pass that shared list
into the test's instances, call sandbox_manager.destroy_executor(executor), then
assert events == ["executor.cleanup", "sandbox.delete"] in addition to the
existing cleaned/deleted checks so the cleanup→delete ordering is explicitly
verified.
- Around line 198-207: The test currently only ensures no exception on timeout;
update test_destroy_executor_outer_timeout_returns_without_raising to also
assert that the cleanup/delete did not complete after the short timeout by
checking the executor and sandbox state (e.g., assert executor.cleaned is False
or sandbox.deleted is False) for the SlowCleanupExecutor/FakeSandbox used and
the call to manager.SandboxManager.destroy_executor; if cleanup completion is
set inside cleanup, consider exposing a cleanup_started or cleaned flag on
SlowCleanupExecutor to make this assertion reliable.
---
Nitpick comments:
In `@src/proovy_agent/common/sandbox/manager.py`:
- Line 53: The ASYNC109 suppression on the timeout parameter appears unnecessary
because the function actually uses asyncio.wait_for(_destroy(), timeout=timeout)
(see the timeout argument usage), so run Ruff to confirm whether ASYNC109 is
reported; if Ruff reports no ASYNC109, remove the "noqa: ASYNC109" comment from
the timeout parameter in the function signature (the parameter on the async
function that calls asyncio.wait_for/_destroy) so the code is cleaner, otherwise
keep the noqa.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 425f3f8b-6080-4ba4-ac6c-773092c600aa
📒 Files selected for processing (3)
src/proovy_agent/common/sandbox/executor.pysrc/proovy_agent/common/sandbox/manager.pytests/common/sandbox/test_manager.py
CodeExecutor의 sandbox 타입을 구체화하고 destroy_executor 테스트에서 cleanup/delete 순서와 timeout 상태를 검증합니다. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
1 participant
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.
📌 관련 이슈
🏷️ PR 타입
📝 작업 내용
SandboxManager.create_executor()를 추가해SandboxConfig기반 Daytona sandbox 생성과CodeExecutor래핑을 처리했습니다.SandboxManager.destroy_executor()를 추가해 executor cleanup 실패 여부와 무관하게 sandbox delete를 시도하고, 단계별 timeout으로 teardown hang을 방지했습니다.CodeExecutorscaffold와SandboxManager단위 테스트를 추가했습니다.📸 스크린샷
uv run pytest tests/common/sandbox/test_manager.py -v→ 7 passeduv run pytest tests/common/sandbox -v→ 27 passeduv run ruff check src/proovy_agent/common/sandbox tests/common/sandbox→ passeduv run pytest→ 31 passed✅ 체크리스트
📎 기타 참고사항
CodeExecutor는 #17에서 필요한 최소 scaffold만 포함했습니다.run_python(), context 관리, E2E 통합 테스트는 [Feat] CodeExecutor 구현 + 통합 테스트 (executor.py) #18 범위로 분리했습니다.AsyncSandbox.delete가 존재해SandboxManager.destroy_executor()는sandbox.delete()를 사용합니다.Summary by CodeRabbit
릴리스 노트
새로운 기능
테스트