[Yunnglin]

Summary

Restart the per-sandbox Docker container when a cancelled exec remains running. Docker's API does not expose a reliable exec-kill operation, and the exec PID cannot safely be used inside the container PID namespace.

This prevents a cancelled command and its child processes from continuing in a sandbox that will otherwise be reused.

Test

• DOCKER_HOST=unix:///Users/yunlin/.colima/default/docker.sock PYTHONPATH=$PWD /Users/yunlin/miniconda3/envs/eval/bin/python -m pytest tests/test_tool.py -v -k 'cancellation_restarts or timeout_terminates'

The real Docker regression cancels sleep 2; touch marker, verifies that the marker is absent after waiting, and confirms that the restarted sandbox accepts a subsequent command.

[Copilot]

🟡 Changes recommended

Unresolved cancellation cleanup and notebook session handling issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes sandbox cleanup after cancelled Docker execs by restarting the affected container and adding regression coverage.

Changes:

• Replaces exec process killing with container restart logic.
• Adds tests for lingering child processes and sandbox reuse.

File summaries

File	Summary
tests/test_tool.py	Adds cancellation cleanup and sandbox reuse regression coverage.
ms_enclave/sandbox/boxes/docker_sandbox.py	Restarts containers after active cancellation; unresolved issues remain for detached descendants (critical, 2 votes) and notebook session reinitialization (moderate, 1 vote).

Review details

Suppressed comments (3)

ms_enclave/sandbox/boxes/docker_sandbox.py:209

• If either kill or start fails, this handler only logs and returns, leaving the sandbox object marked RUNNING with a stopped or otherwise unusable container. Subsequent requests (and the pool's finally path) can reuse it and fail on every exec_create; restart failures need to make the sandbox unavailable rather than being silently swallowed.

        except Exception as exc:
            logger.debug(f'Failed to restart sandbox after cancelling exec {exec_id}: {exc}')

ms_enclave/sandbox/boxes/docker_sandbox.py:206

• Restarting the container here terminates every other docker exec in the same sandbox, not only the cancelled one. The direct manager/API execution path does not serialize requests, so cancelling one request can abort unrelated concurrent work; please serialize per-sandbox executions or explicitly coordinate/invalidates concurrent work before using a container-wide restart.

            await self._run_blocking(self.container.kill)

ms_enclave/sandbox/boxes/docker_sandbox.py:207

• DockerNotebookSandbox inherits this restart path, but killing and starting its container also destroys the Kernel Gateway process and its websocket/kernel. Nothing here calls _setup_jupyter or replaces ws and kernel_id, so a cancelled compatible Docker command in a notebook sandbox leaves later notebook executions connected to stale state. Reinitialize the notebook session after restart or exclude notebook sandboxes from this path.

            logger.warning(f'Exec {exec_id} is still running; restarting sandbox container.')
            await self._run_blocking(self.container.kill)
            await self._run_blocking(self.container.start)

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.