[jayzuccarelli]

Adds a process-wide Harbor sandbox supervisor. run_rollout adopts each trial after Trial.create, so the process can cap concurrent sandbox creation (OPENENV_HARBOR_MAX_SANDBOX_STARTS), tear down only the sandboxes it started on SIGINT/SIGTERM, and avoid leaking a sandbox when a rollout is cancelled mid-start. Once cleanup begins, new starts are refused instead of slipping past the cleanup snapshot. Docs (harbor.md, the harbor env README, RFC 012) describe the env var and shutdown behavior; unit tests cover ownership, idempotent stop, limits, cancellation and signal chaining, with optional Docker integration tests.

Prepared with AI assistance (Claude Code); I reviewed the change and ran the tests.

Known gap: the docker test for cancelling during sandbox creation passes with or without the supervisor, so that leak path is covered by a unit test of the stop-after-start ordering rather than reproduced end to end. Happy to tighten it if you have a reliable way to trigger it.

---

Note

Medium Risk
Touches process-wide signal handling and async sandbox lifecycle across threads and event loops; bugs could affect shutdown behavior or leak sandboxes, though scope is limited to adopted trials.

Overview
Adds a process-wide SandboxSupervisor that run_rollout adopts into after each Trial.create, wrapping agent_environment.start/stop without replacing Harbor's normal teardown.

Concurrent creation is bounded separately from rollout concurrency via OPENENV_HARBOR_MAX_SANDBOX_STARTS (unset/0 = unlimited). Starts shield through cancellation so stop() can wait for in-flight creation and avoid orphaned sandboxes.

On SIGINT/SIGTERM, the supervisor stops only sandboxes this process started, then chains to the previous handler (including uvicorn loop handlers). The Harbor server registers handlers at app startup because UI rollouts start sandboxes from worker threads.

Docs (harbor guide, env README, RFC 012) and tests (unit, serving startup, optional Docker) cover ownership, limits, signal chaining, and retry/idempotent cleanup.

^{Reviewed by Cursor Bugbot for commit 1097a60. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Stale Bugbot comment from a previous run.

[jayzuccarelli]

@sergiopaniego saw the Harbor docs going in today. When you have time, could you take a look at this one and #1312 (a small trial_uri decoding fix in the same area)? Happy to rebase or split either.

[cursor]

Stale Bugbot comment from a previous run.

[burtenshaw]

Pushed a scoped cleanup fix in 42950b8. A failed real_stop previously removed the sandbox from _owned in finally, while stop_task permanently cached the failure. Cleanup now retains ownership until success and allows a later caller to retry failed/cancelled tasks. In-flight calls still share one shielded cleanup and successful cleanup stays cached.

Four regression cases failed on 81a1228 and pass with the fix: direct retry, retry through aclose, a cancelled cleanup attempt, and cancellation of a caller during a shared retry. Focused supervisor tests: 17 passed; broader supervisor/runner/rollout selection: 50 passed, 9 skipped (Harbor unavailable). Touched-file Ruff/usort and diff checks pass. No live Linux Docker test was run.

One separate lifecycle gap remains: _stop_once cancels a start after _START_GRACE_S but does not wait for cancellation to finish before real_stop. A deterministic fake-backend probe (grace set to zero, start catches cancellation and waits on an event, stop returns, then the event permits creation) ends with a live sandbox and owned == 0. This confirms the ordering gap, not that a specific live provider behaves that way. Resolving how to supervise late completion within bounded shutdown needs further lifecycle work; the 60-second signal cleanup deadline is also shorter than the 120-second start grace. Keeping this PR in needs-review pending the broader cancellation/leak evidence. The two prior review threads were already resolved and were not duplicated or changed.

[jayzuccarelli]

Thanks @burtenshaw for the cleanup fix and the regression cases. Merged main to clear the conflict; ready from my side.

[cursor]

Stale Bugbot comment from a previous run.

[burtenshaw]

Thanks for merging main. Two items from my 26 October comment are still open, so this isn't ready to merge yet.

1. A sandbox created while its start is being cancelled is never deleted. _stop_once cancels a start that outlasts _START_GRACE_S and then calls real_stop straight away. If the backend finishes creating the sandbox while the start unwinds, the stop has already run and owned is 0, so nothing deletes it. The test below fails at 3e83887: the log is start, stop, created.

Regression test

async def test_stop_deletes_a_sandbox_created_while_its_start_is_cancelled(monkeypatch):
    # The backend finishes creating the sandbox while its start unwinds.
    monkeypatch.setattr(supervisor_mod, "_START_GRACE_S", 0)
    release = asyncio.Event()

    class SlowCancelEnv(FakeEnv):
        async def start(self, force_build: bool) -> None:
            self.log.append((self.name, "start"))
            try:
                await asyncio.sleep(10)
            except asyncio.CancelledError:
                await release.wait()
                self.log.append((self.name, "created"))
                raise

    log: list = []
    sup = supervisor_mod.SandboxSupervisor()
    env = SlowCancelEnv(log)
    sup.adopt(make_trial(env))
    starting = asyncio.ensure_future(env.start(force_build=False))
    await asyncio.sleep(0.01)
    stopping = asyncio.ensure_future(env.stop(delete=True))
    await asyncio.sleep(0.01)
    release.set()
    await asyncio.wait_for(stopping, 1)
    with pytest.raises(asyncio.CancelledError):
        await starting

    # The stop must come after the creation, or the sandbox outlives cleanup.
    assert log == [("env", "start"), ("env", "created"), ("env", "stop", True)]
    assert sup.owned == 0

One way to fix it is to wait for the cancelled start to finish before stopping. With this change the test above and the 17 existing supervisor tests pass:

                 if not box.start_task.done():
                     box.start_task.cancel()
-                elif not box.start_task.cancelled():
+                    # The backend can finish creating the sandbox while the start unwinds.
+                    await asyncio.wait({box.start_task})
+                if not box.start_task.cancelled():

The trade-off is that a start which ignores cancellation now delays its own trial's stop instead of leaking. aclose and the signal path stay bounded by their own timeouts. If you'd rather bound this wait too, the sandbox has to stay in _owned when the wait runs out, so a later cleanup can still delete it.

2. On a signal, the cleanup deadline is shorter than the start grace. Signal cleanup waits _CLEANUP_TIMEOUT_S (60 s), but _stop_once first waits up to _START_GRACE_S (120 s) for a start to finish. A sandbox whose start began shortly before SIGTERM is therefore never stopped. Either the cleanup path should use a grace that fits inside the deadline, or the deadline should cover the grace.

[jayzuccarelli]

Thanks, both fixed. Item 1 uses your regression test verbatim and your fix: stop now waits for a cancelled start to unwind, so the sandbox is deleted after it is created. For item 2, the signal deadline now covers the grace: 120 s for the start plus 60 s to stop. A second signal still skips the wait.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 1097a60. Configure here.}

[cursor]

Hung start cancel blocks sandbox teardown

Medium Severity

After _START_GRACE_S, _stop_once cancels an in-flight start and then asyncio.waits for it with no timeout. A start that ignores cancellation, or whose CancelledError handler keeps creating, now blocks real_stop indefinitely, which is the hang _START_GRACE_S was meant to prevent. Signal cleanup still gives up at _CLEANUP_TIMEOUT_S, so the process can exit while that wait is running and the sandbox is never deleted.

Additional Locations (1)

• src/openenv/harbor/supervisor.py#L32-L37

 

^{Reviewed by Cursor Bugbot for commit 1097a60. Configure here.}

[burtenshaw]

Verified at 1097a60, thanks for the quick fixes. The regression test from my earlier comment passes now, and so do the supervisor and hosted-serving suites (31 tests). Signal cleanup now outlasts the start grace, and the new test covers the startup hook that installs the handlers for UI rollouts. CI is green.