Codex/sandbox workspace archive - #1039
Open
FicoHu wants to merge 26 commits into
Open
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughThe pull request introduces DB-backed sandbox workspace archival and restoration capabilities. A new Changes
Sequence DiagramssequenceDiagram
participant Client
participant ExecutionSvc as Execution Service
participant DB as Database
participant ArchiveSvc as Archive Service
participant RuntimeClient as Runtime Client
Client->>ExecutionSvc: create_sandbox()
ExecutionSvc->>DB: Open SessionLocal transaction
ExecutionSvc->>ArchiveSvc: prepare_restore_metadata(db)
ArchiveSvc->>RuntimeClient: Resolve runtime client
ArchiveSvc->>RuntimeClient: Check archive availability
ArchiveSvc->>DB: Load task metadata
ArchiveSvc-->>ExecutionSvc: Return task/restore target
ExecutionSvc->>RuntimeClient: create_sandbox via API
RuntimeClient-->>ExecutionSvc: Return sandbox object
ExecutionSvc->>ArchiveSvc: restore_sandbox_after_create(sandbox)
ArchiveSvc->>RuntimeClient: restore_workspace(executor context)
RuntimeClient-->>ArchiveSvc: Restoration result
ArchiveSvc-->>ExecutionSvc: Return restore result
ExecutionSvc->>DB: Commit transaction
ExecutionSvc->>DB: Close session
ExecutionSvc-->>Client: CreateSandboxResponse with metadata
sequenceDiagram
participant Client
participant ExecutionSvc as Execution Service
participant DB as Database
participant ArchiveSvc as Archive Service
participant RuntimeClient as Runtime Client
participant SandboxMgr as Sandbox Manager API
Client->>ExecutionSvc: delete_sandbox(sandbox_id)
ExecutionSvc->>DB: Open SessionLocal session
ExecutionSvc->>ArchiveSvc: archive_sandbox_before_delete(db, sandbox_id)
ArchiveSvc->>RuntimeClient: Resolve runtime client
ArchiveSvc->>RuntimeClient: get_sandbox(sandbox_id)
RuntimeClient-->>ArchiveSvc: Sandbox pod details
ArchiveSvc->>DB: Load task from sandbox metadata
ArchiveSvc->>RuntimeClient: archive_workspace(executor context)
RuntimeClient-->>ArchiveSvc: Archive result
ArchiveSvc->>DB: Commit archive metadata
ArchiveSvc-->>ExecutionSvc: Archive complete (or swallow warning)
ExecutionSvc->>SandboxMgr: HTTP DELETE sandbox
SandboxMgr-->>ExecutionSvc: Deletion response
ExecutionSvc->>DB: Close session
ExecutionSvc-->>Client: Deletion result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/tests/services/test_sandbox_workspace_archive.py (1)
58-82: 💤 Low valueMinor: Unnecessary
@pytest.mark.asynciodecorator.
prepare_restore_metadatais a synchronous method (noasync def, noawaitin test). The@pytest.mark.asynciodecorator on line 58 is unnecessary here and could be removed for clarity.Suggested fix
-@pytest.mark.asyncio -async def test_prepare_restore_metadata_sets_skip_git_clone_when_archive_available(): +def test_prepare_restore_metadata_sets_skip_git_clone_when_archive_available():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/services/test_sandbox_workspace_archive.py` around lines 58 - 82, The test function test_prepare_restore_metadata_sets_skip_git_clone_when_archive_available is marked with `@pytest.mark.asyncio` but calls the synchronous SandboxWorkspaceArchiveService.prepare_restore_metadata (no async/await); remove the unnecessary decorator from the test declaration so it runs as a normal sync test, leaving the rest of the test (patches of service._load_task and archive_service.check_archive_available, the call to service.prepare_restore_metadata, and the assertions) unchanged.backend/app/services/sandbox_workspace_archive.py (1)
93-97: ⚡ Quick winMissing return type annotation for
archive_sandbox_before_delete.The method returns
archive_info(fromarchive_service.archive_workspace) orNone, but lacks a return type hint. Per coding guidelines, public functions should have type hints.Suggested fix
+ from app.services.workspace_archive.archive_service import ArchiveInfo + async def archive_sandbox_before_delete( self, db: Session, sandbox_id: str, - ): + ) -> Optional["ArchiveInfo"]: """Archive a running task-backed sandbox before it is deleted."""Alternatively, if
ArchiveInfoimport causes cycles, use a string annotation orAny:async def archive_sandbox_before_delete( self, db: Session, sandbox_id: str, - ): + ) -> Optional[Any]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/sandbox_workspace_archive.py` around lines 93 - 97, Add an explicit return type to archive_sandbox_before_delete: it can return the ArchiveInfo from archive_service.archive_workspace or None, so annotate it as Optional[ArchiveInfo] (or use a forward-string annotation "ArchiveInfo" or typing.Any if importing ArchiveInfo would introduce an import cycle). Update the function signature for archive_sandbox_before_delete to include the chosen return type and add the necessary typing import (Optional) or use the string/Any approach to avoid cyclic imports.backend/app/services/execution/__init__.py (1)
209-222: 💤 Low valueRedundant
passstatement after logging.Line 220 has
passafter the warning log which is unnecessary.Suggested fix
except Exception as archive_error: logger.warning( "Failed to archive sandbox %s before deletion: %s", sandbox_id, archive_error, ) - pass finally: db.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/__init__.py` around lines 209 - 222, The except block in the try/except around SessionLocal usage contains a redundant "pass" after logging; remove the unnecessary "pass" so the except only logs the error (logger.warning(...) for sandbox_workspace_archive_service.archive_sandbox_before_delete) and then allows the finally block to run (db.close()); ensure no other logic depends on the pass before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/services/execution/__init__.py`:
- Around line 209-222: The except block in the try/except around SessionLocal
usage contains a redundant "pass" after logging; remove the unnecessary "pass"
so the except only logs the error (logger.warning(...) for
sandbox_workspace_archive_service.archive_sandbox_before_delete) and then allows
the finally block to run (db.close()); ensure no other logic depends on the pass
before committing.
In `@backend/app/services/sandbox_workspace_archive.py`:
- Around line 93-97: Add an explicit return type to
archive_sandbox_before_delete: it can return the ArchiveInfo from
archive_service.archive_workspace or None, so annotate it as
Optional[ArchiveInfo] (or use a forward-string annotation "ArchiveInfo" or
typing.Any if importing ArchiveInfo would introduce an import cycle). Update the
function signature for archive_sandbox_before_delete to include the chosen
return type and add the necessary typing import (Optional) or use the string/Any
approach to avoid cyclic imports.
In `@backend/tests/services/test_sandbox_workspace_archive.py`:
- Around line 58-82: The test function
test_prepare_restore_metadata_sets_skip_git_clone_when_archive_available is
marked with `@pytest.mark.asyncio` but calls the synchronous
SandboxWorkspaceArchiveService.prepare_restore_metadata (no async/await); remove
the unnecessary decorator from the test declaration so it runs as a normal sync
test, leaving the rest of the test (patches of service._load_task and
archive_service.check_archive_available, the call to
service.prepare_restore_metadata, and the assertions) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6643df6a-fa4a-4f60-bb58-4b883aae821d
📒 Files selected for processing (7)
backend/app/services/execution/__init__.pybackend/app/services/sandbox_workspace_archive.pybackend/tests/services/execution/test_sandbox_manager_client.pybackend/tests/services/test_sandbox_workspace_archive.pyexecutor_manager/routers/sandbox.pyexecutor_manager/schemas/sandbox.pyexecutor_manager/tests/routers/test_sandbox_routes.py
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.
Summary by CodeRabbit