Destroy grading sandboxes at pipeline cleanup

[ArthurCRodrigues]

Summary

• change pipeline sandbox cleanup to destroy used sandboxes instead of returning them to idle
• clear sandbox reference after cleanup
• update sandbox cleanup unit tests to assert destroy behavior
• update pipeline documentation to reflect destroy-on-cleanup semantics

Why

Returning used sandboxes to idle can retain cross-submission workspace state and increase risk of vulnerability exploration and workdir pollution.

Closes #278

[Copilot]

Pull request overview

Updates the autograder pipeline’s end-of-run sandbox lifecycle to destroy the sandbox used for a submission (instead of returning it to the idle pool), reducing cross-submission state retention risk.

Changes:

• Switch pipeline cleanup from release_sandbox to destroy_sandbox and clear the sandbox reference.
• Update unit tests to assert destroy-on-cleanup behavior.
• Update pipeline documentation to describe destroy-on-cleanup semantics.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
autograder/autograder.py	Changes _cleanup_sandbox to destroy sandboxes and clears pipeline_execution.sandbox.
tests/unit/pipeline/test_sandbox_cleanup.py	Updates assertions to expect destroy_sandbox() instead of release_sandbox().
docs/pipeline/README.md	Documents destroy-on-cleanup behavior as a pipeline guarantee.
docs/core/pipeline-architecture.md	Updates architecture summary to mention sandbox destruction during cleanup.

Comments suppressed due to low confidence (1)

autograder/autograder.py:121

• pipeline_execution.sandbox is only cleared after a successful destroy_sandbox() call. If destroy_sandbox() raises, the exception handler logs and leaves the sandbox reference attached, contradicting the “clear sandbox reference after cleanup” goal and risking repeated cleanup attempts. Clear the reference in a finally once you’ve captured the sandbox to clean up.

        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.destroy_sandbox(language, sandbox)
                pipeline_execution.sandbox = None
                logger.info(
                    "Sandbox destroyed: external_user_id=%s, language=%s",
                    pipeline_execution.submission.user_id,
                    language.value if language else "none",
                )
        except Exception as e:
            # Log error but don't fail the pipeline
            logger.warning(
                "Failed to cleanup sandbox (external_user_id=%s): %s",
                pipeline_execution.submission.user_id,
                str(e),
            )

[Copilot]

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]

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]

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]

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]

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.

[ArthurCRodrigues]

OK