fix(runtime): reap the sandbox container when Caido bootstrap fails

[thejesh23]

Fixes #830

What

create_or_reuse now deletes the sandbox container it just started if the remaining bootstrap steps fail, instead of letting the exception escape with the container still running and untracked.

Why

The container is live from backend(...) (:119), but it is only registered in _SESSION_CACHE at :145. Between those two points sit session.resolve_exposed_port (:129) and bootstrap_caido (:134), either of which can raise.

cleanup reaps only cached bundles:

bundle = _SESSION_CACHE.pop(scan_id, None)
if bundle is None:
    logger.debug("cleanup(%s): no cached session", scan_id)
    return

so after a bootstrap failure cleanup(scan_id) is a no-op and nothing else holds a reference to client/session — the container is orphaned for the lifetime of the daemon, keeping its exposed port and bind mounts. bootstrap_caido exhausting its 10 guest-login retries on a loaded machine is a routine failure, so this leaks one container per failed scan start.

How

Wrap the post-start steps in try/except BaseException, delete the session best-effort, and re-raise the original exception unchanged. BaseException rather than Exception so a KeyboardInterrupt or asyncio.CancelledError during bootstrap also reaps.

The delete path is now shared with cleanup as _delete_quietly, so both teardown sites also release the docker client identically. cleanup's behaviour is unchanged.

Tests

New tests/test_session_bootstrap_failure.py, driving the real create_or_reuse with a stub backend:

• test_container_is_deleted_when_port_resolution_fails
• test_container_is_deleted_when_caido_bootstrap_fails
• test_cleanup_is_a_noop_for_an_unknown_scan — pins the behaviour that made the leak invisible

Both leak tests fail on main with AssertionError: started container was never reaped, and pass with this change. Each also asserts the original exception still propagates and that nothing is left in _SESSION_CACHE.

uv run pytest -q
2 failed, 317 passed

The 2 failures are pre-existing on main (tests/test_runner_root_prompt.py — a stale SimpleNamespace settings stub missing runtime) and unrelated; separate PR for those.

ruff format --check, ruff check and mypy strix/runtime/session_manager.py are clean.

Scope

One file plus a new test file. No signature or behaviour change on the success path.

[greptile-apps]

Greptile Summary

This PR cleans up sandbox containers when Caido startup fails. The main changes are:

• Deletes a newly started container after port-resolution or bootstrap errors.
• Shares container and Docker-client teardown with normal cleanup.
• Adds tests for bootstrap failures and unknown scan cleanup.

Confidence Score: 4/5

The cancellation cleanup path can still leave an uncached container running.

• Ordinary bootstrap exceptions now delete the started container.
• A second cancellation can interrupt the awaited delete before teardown finishes.
• Later cleanup cannot retry deletion because the session was never cached.

strix/runtime/session_manager.py

Important Files Changed

Filename	Overview
strix/runtime/session_manager.py	Adds bootstrap-failure teardown and extracts shared container and client cleanup.
tests/test_session_bootstrap_failure.py	Covers port-resolution failures, Caido bootstrap failures, and cleanup of unknown scans.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
strix/runtime/session_manager.py:149
**Second Cancellation Interrupts Teardown**

If another `Task.cancel()` arrives while `_delete_quietly` awaits `client.delete(session)`, its `CancelledError` escapes the helper and deletion can stop midway. Because this session was never cached, later `cleanup(scan_id)` is a no-op, leaving the container running and replacing the original bootstrap failure with the second cancellation.

_{Reviews (1): Last reviewed commit: "fix(runtime): reap the sandbox container..." | Re-trigger Greptile}

[greptile-apps]

Second Cancellation Interrupts Teardown

If another Task.cancel() arrives while _delete_quietly awaits client.delete(session), its CancelledError escapes the helper and deletion can stop midway. Because this session was never cached, later cleanup(scan_id) is a no-op, leaving the container running and replacing the original bootstrap failure with the second cancellation.

Prompt To Fix With AI

This is a comment left during a code review.
Path: strix/runtime/session_manager.py
Line: 149

Comment:
**Second Cancellation Interrupts Teardown**

If another `Task.cancel()` arrives while `_delete_quietly` awaits `client.delete(session)`, its `CancelledError` escapes the helper and deletion can stop midway. Because this session was never cached, later `cleanup(scan_id)` is a no-op, leaving the container running and replacing the original bootstrap failure with the second cancellation.

How can I resolve this? If you propose a fix, please make it concise.

[thejesh23]

Confirmed and fixed in 0893e65.

You're right that this reopens the exact leak the PR closes: _delete_quietly catches Exception, not CancelledError, and because the session was never cached cleanup() can't retry.

with contextlib.suppress(BaseException):
    await asyncio.shield(_delete_quietly(client, session, scan_id))
raise

shield lets the delete run to completion, and suppressing the escaping cancellation means the original bootstrap error is still what propagates rather than being replaced by the second cancellation.

Regression test is event-gated rather than timing-based: client.delete blocks on an asyncio.Event, the task is cancelled while inside the delete, then the gate is released. Fails without the shield.

[thejesh23]

Good catch — that's a real hole in this very change, fixed in 0893e65.

_delete_quietly catches Exception, not CancelledError, so a second Task.cancel() landing while it awaited client.delete() would escape and abandon the container mid-teardown. And because the session was never cached, no later cleanup() could retry it — so the leak this PR exists to close would reopen on exactly the cancellation path the except BaseException was meant to cover.

The teardown now runs under asyncio.shield, with the escaping cancellation suppressed so the original bootstrap error is still what propagates.

Added a deterministic regression test rather than a timing-based one: client.delete blocks on an asyncio.Event, the task is cancelled while it is inside the delete, then the gate is released. It fails without the shield (container abandoned when teardown was cancelled) and passes with it.

(Edited: this comment originally cited a wrong commit hash. The correct one is 0893e65.)