Skip to content

Destroy grading sandboxes at pipeline cleanup #279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ArthurCRodrigues merged 2 commits into from
Apr 13, 2026
Merged

Destroy grading sandboxes at pipeline cleanup #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
893da98
Destroy grading sandboxes at pipeline cleanup
ArthurCRodrigues Apr 13, 2026
1cdfb2e
fix docs ci config
ArthurCRodrigues Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions .github/workflows/docs-pages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,6 @@ on:
- 'mkdocs.yml'
- 'requirements-docs.txt'
- '.github/workflows/docs-pages.yml'
pull_request:
paths:
- 'docs/**'
- 'mkdocs.yml'
- 'requirements-docs.txt'
- '.github/workflows/docs-pages.yml'
workflow_dispatch:

permissions:
Expand Down Expand Up @@ -44,17 +38,14 @@ jobs:
run: mkdocs build --strict

- name: Setup Pages
if: github.event_name != 'pull_request'
uses: actions/configure-pages@v5

- name: Upload Pages artifact
if: github.event_name != 'pull_request'
uses: actions/upload-pages-artifact@v3
with:
path: ./site

deploy:
if: github.event_name != 'pull_request'
needs: build
runs-on: ubuntu-latest
environment:
Expand Down
9 changes: 5 additions & 4 deletions autograder/autograder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def run(self, submission: Submission):

pipeline_execution.finish_execution() # Generates GradingResult object in pipeline execution

# Cleanup: Release sandbox back to pool if it was created
# Cleanup: Destroy sandbox if it was created
self._cleanup_sandbox(pipeline_execution)

Comment on lines 87 to 91

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finish_execution() can raise (e.g., missing grade step data / feedback step produced None), which would skip sandbox destruction because cleanup is not in a finally. Wrap finish_execution()/return in try/finally (or move cleanup to finally) so sandbox destruction truly runs even when finalization errors out or the pipeline is INTERRUPTED.

Copilot uses AI. Check for mistakes.
logger.info(
Expand All @@ -98,16 +98,17 @@ def run(self, submission: Submission):
return pipeline_execution

def _cleanup_sandbox(self, pipeline_execution: PipelineExecution) -> None:
"""Release sandbox back to pool after pipeline execution."""
"""Destroy sandbox after pipeline execution to avoid cross-submission reuse."""
try:
sandbox = pipeline_execution.sandbox
if sandbox:
from sandbox_manager.manager import get_sandbox_manager
manager = get_sandbox_manager()
language = pipeline_execution.submission.language
manager.release_sandbox(language, sandbox)
manager.destroy_sandbox(language, sandbox)
pipeline_execution.sandbox = None
logger.info(
"Sandbox released: external_user_id=%s, language=%s",
"Sandbox destroyed: external_user_id=%s, language=%s",
pipeline_execution.submission.user_id,
language.value if language else "none",
)
Expand Down
2 changes: 1 addition & 1 deletion docs/core/pipeline-architecture.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ LOAD_TEMPLATE -> BUILD_TREE -> SANDBOX -> PRE_FLIGHT -> AI_BATCH -> GRADE -> FOC
2. Each step receives the same `PipelineExecution` and appends one `StepResult`.
3. If a step fails, execution stops early.
4. `finish_execution()` assembles `GradingResult` from grade/focus/feedback artifacts.
5. Sandbox cleanup runs at the end.
5. Sandbox cleanup runs at the end and destroys any sandbox used by the submission.

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement says sandbox cleanup runs at the end and destroys the sandbox. Today AutograderPipeline.run() calls cleanup after finish_execution() without a finally, so destruction may be skipped if finalization raises. Align this doc with the actual guarantee (or adjust code to make the guarantee true).

Suggested change
5. Sandbox cleanup runs at the end and destroys any sandbox used by the submission.
5. In the normal execution flow, sandbox cleanup is called after finalization to destroy any sandbox used by the submission.

Copilot uses AI. Check for mistakes.

## Core data contract

Expand Down
4 changes: 2 additions & 2 deletions docs/pipeline/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Key behaviors:
- After each step, the pipeline checks the latest `StepResult`. If it failed, the pipeline sets its status to `FAILED` and breaks.
- Unhandled exceptions set the status to `INTERRUPTED`.
- After all steps (or early exit), `finish_execution()` is called to assemble the final `GradingResult`.
- Sandbox cleanup always runs at the end, regardless of success or failure.
- Sandbox cleanup always runs at the end, regardless of success or failure, and destroys the sandbox used by that submission.

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs state sandbox cleanup “always runs” at the end. In AutograderPipeline.run(), cleanup currently happens after finish_execution() without a finally, so any exception during finalization can skip destruction. Either update the docs to reflect “best effort” cleanup or (preferably) change the implementation to guarantee cleanup via try/finally.

Suggested change
- Sandbox cleanup always runs at the end, regardless of success or failure, and destroys the sandbox used by that submission.
- Sandbox cleanup is attempted at the end as a best-effort step and normally destroys the sandbox used by that submission, but it is not guaranteed if finalization raises an exception before cleanup is reached.

Copilot uses AI. Check for mistakes.

### PipelineExecution

Expand Down Expand Up @@ -191,7 +191,7 @@ When a step fails:
2. The pipeline detects the failure via `get_previous_step()` and calls `set_failure()`.
3. No further steps are executed.
4. `finish_execution()` is called — since the status is `FAILED`, `result` remains `None`.
5. Sandbox cleanup always runs: if a Sandbox step created a sandbox, it is released back to the pool regardless of pipeline outcome.
5. Sandbox cleanup always runs: if a Sandbox step created a sandbox, it is destroyed regardless of pipeline outcome, and the pool replenishes with a fresh container.

For unhandled exceptions, the status is set to `INTERRUPTED` and the same cleanup logic applies.

Expand Down
12 changes: 6 additions & 6 deletions tests/unit/pipeline/test_sandbox_cleanup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def pipeline_exec(submission):

@patch("sandbox_manager.manager.get_sandbox_manager")
def test_cleanup_sandbox_called_after_pipeline_run(mock_get_manager, pipeline_exec):
"""Verifies that _cleanup_sandbox is called and releases the sandbox correctly."""
"""Verifies that _cleanup_sandbox is called and destroys the sandbox correctly."""
mock_manager = mock_get_manager.return_value
mock_sandbox = MagicMock(spec=SandboxContainer)

Expand All @@ -39,11 +39,11 @@ def test_cleanup_sandbox_called_after_pipeline_run(mock_get_manager, pipeline_ex
# Let's test _cleanup_sandbox directly to verify it uses the property
pipeline._cleanup_sandbox(pipeline_exec)
Comment on lines 27 to 40

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test’s docstring says it verifies cleanup “after pipeline run”, but the test calls _cleanup_sandbox() directly and never exercises AutograderPipeline.run() to ensure cleanup is invoked on real execution paths. Either adjust the wording or add a minimal step + real run() invocation to validate the integration behavior being described.

Copilot uses AI. Check for mistakes.

mock_manager.release_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox)
mock_manager.destroy_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox)
Comment on lines 40 to +42

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new behavior includes clearing the sandbox reference (pipeline_execution.sandbox = None), but the tests only assert destroy_sandbox() was called. Add an assertion that pipeline_exec.sandbox is None after cleanup so the “clear sandbox reference after cleanup” requirement is covered.

Copilot uses AI. Check for mistakes.

@patch("sandbox_manager.manager.get_sandbox_manager")
def test_cleanup_sandbox_even_on_step_failure(mock_get_manager, pipeline_exec):
"""Ensures that sandbox cleanup occurs even if the pipeline status is FAILED."""
"""Ensures sandbox destruction occurs even if the pipeline status is FAILED."""
mock_manager = mock_get_manager.return_value
mock_sandbox = MagicMock(spec=SandboxContainer)

Expand All @@ -55,16 +55,16 @@ def test_cleanup_sandbox_even_on_step_failure(mock_get_manager, pipeline_exec):
pipeline._cleanup_sandbox(pipeline_exec)

# Cleanup should still happen
mock_manager.release_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox)
mock_manager.destroy_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox)

@patch("sandbox_manager.manager.get_sandbox_manager")
def test_cleanup_no_sandbox_no_call(mock_get_manager, pipeline_exec):
"""Verifies that no sandbox release call is made if no sandbox is attached."""
"""Verifies that no sandbox destroy call is made if no sandbox is attached."""
mock_manager = mock_get_manager.return_value

pipeline_exec.sandbox = None

pipeline = AutograderPipeline()
pipeline._cleanup_sandbox(pipeline_exec)

mock_manager.release_sandbox.assert_not_called()
mock_manager.destroy_sandbox.assert_not_called()
Loading