feat: make automation sandbox cleanup delay configurable

[hieptl]

Summary

Automation-generated conversations are archived the instant a run finishes, and there is no way to get back into one. complete_run and both watchdog terminal paths delete the run's sandbox immediately unless the automation's keep_alive is true, and a conversation whose sandbox is gone renders as archived and read-only with no resume path. So when an automation fails or does something unexpected, the conversation that would explain it is already gone.

This adds a deployment-level delay. With AUTOMATION_SANDBOX_CLEANUP_DELAY_SECONDS set, a finished run's sandbox is paused rather than deleted, and the watchdog deletes it once the delay has passed. The conversation stays open and resumable for that window. The default is 0, which keeps today's behaviour exactly.

Pausing rather than simply keeping the sandbox alive is deliberate: paused sandboxes do not count toward the per-user running-sandbox cap, so a finished automation can no longer cost a user an interactive conversation slot, which is the reason the immediate delete was there in the first place.

Linear: OHE-2833

Changes

• config.py — ServiceSettings.sandbox_cleanup_delay_seconds (AUTOMATION_SANDBOX_CLEANUP_DELAY_SECONDS, int >= 0, default 0), documented in the settings docstring next to the other retention knobs.
• models.py + migration 024 — nullable automation_runs.sandbox_cleanup_due_at, with a partial index (WHERE sandbox_cleanup_due_at IS NOT NULL) on both PostgreSQL and SQLite, since the column is NULL on nearly every row.
• utils/sandbox.py — pause_sandbox, a best-effort POST /api/v1/sandboxes/{id}/pause built on the same shape as the existing cleanup_sandbox.
• router.py / complete_run — when a delay is configured, the due timestamp is written inside the same optimistic-lock UPDATE as status and completed_at, so it cannot be lost, and it is re-applied on the watchdog-timeout reconcile path. The fire-and-forget task then pauses instead of deleting. cancel_run is untouched and still deletes immediately, since cancelling is explicit.
• watchdog.py — both terminal sites defer through a shared _defer_sandbox_cleanup, and a new cleanup_due_sandboxes sweep runs in the existing janitor loop (cloud mode only), deleting due sandboxes in bounded batches through the run's own backend. It clears the stamp after one attempt whatever the outcome, matching the best-effort semantics of the immediate path; the runtime's TTL reaper remains the backstop, so a sandbox that refuses to delete cannot pin the sweep.
• AGENTS.md — the sandbox-cleanup bullet now describes both paths.

Behaviour

Delay	When the run finishes	Later
0 (default)	sandbox deleted immediately, as today	—
> 0	sandbox paused, sandbox_cleanup_due_at = completed_at + delay	watchdog deletes it once due

A few things worth knowing:

• The delay is measured from run completion, not last activity, so resuming the conversation does not extend it.
• keep_alive=true automations, which includes every continue_conversation trigger, are never stamped or paused; that path is unchanged.
• If the configured delay exceeds the runtime's dead-sandbox TTL, the runtime reaper gets there first. The sweep's delete then 404s, is logged, and the stamp is cleared.
• No frontend change is needed: the UI already resumes a paused sandbox when the conversation is opened.

Notes for reviewers

• The migration was numbered 024 rather than 023 because the org-scoped git sync work (feat: scope git sync to organizations so cloud deployments can sync #429) landed 023 on main in the meantime. alembic heads reports a single head.
• Draft Add configurable delay for sandbox cleanup after automation runs complete #33 proposed an earlier take on the same issue: a minutes-based variable defaulting to 60, a cleanup_at column, and no pause step. This PR goes with seconds and a default of 0, so existing deployments keep their current behaviour until an operator opts in, and pauses the sandbox so the delay does not consume sandbox slots.
• The matching Replicated installer option lives in OpenHands-Cloud (PR #1244) and should ship after this.

Testing

Run against current main (which now includes the org-scoped git sync change):

• Full suite with the CI flags (--cov=openhands/automation --cov-fail-under=76): 1750 passed, 7 skipped, coverage 76.10%, floor met.
• pre-commit on every touched file — ruff format, ruff lint, pycodestyle, pyright: all pass.
• alembic heads reports a single head (024). Against SQLite: upgrade head → downgrade 023 → upgrade head is clean, leaving the column and the partial index in place.

New tests cover the behaviour this adds:

• tests/test_router.py — the deferred path pauses and stamps the due time; keep_alive=true still wins over a configured delay.
• tests/test_watchdog.py — both terminal sites defer instead of deleting, the sweep touches only rows that are actually due, the stamp is cleared even when deletion fails, and the sweep joins the janitor cycle in cloud mode only.
• tests/test_sandbox.py — pause_sandbox against a 200, a 404, and a transport failure.

[github-actions]

Warning

Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[tofarr]

Review superseded — please see the full review below.

This placeholder was created by an AI agent (OpenHands) on behalf of the reviewer due to a tooling mishap posting the file path instead of its contents. The actual review content is in the subsequent comment.

[tofarr]

Code Review: PR #459 — feat: make automation sandbox cleanup delay configurable

Taste Rating: 🟢 Good taste

This is a well-designed change. The core insight — pause the sandbox rather than keep it alive, so finished automations don't consume running-sandbox slots while remaining resumable — is exactly the kind of data-structure/behavior choice that eliminates special cases. Stamping sandbox_cleanup_due_at inside the same optimistic-lock UPDATE as the terminal status transition, defaulting to 0 (zero behavior change), partial-indexing the NULL-heavy column, and matching the best-effort semantics of the existing immediate-cleanup path are all the right calls. The migration chain is clean (023 → 024, single head), tests assert on real DB state rather than just mocked calls, and CI is green.

No critical issues. A few observations below, none blocking.

---

[IMPROVEMENT OPPORTUNITIES]

• openhands/automation/config.py / router.py / watchdog.py — local-mode inconsistency (minor). The setting is documented as "cloud mode only," and the cleanup_due_sandboxes sweep is correctly gated behind if not settings.is_local_mode. But the stamping + pausing in complete_run and _verify_and_mark_run is not gated to cloud mode. If a local-mode operator sets AUTOMATION_SANDBOX_CLEANUP_DELAY_SECONDS > 0, runs get stamped and pause_sandbox is called against the local agent server (which may not implement /pause), the pause fails silently, and — because the sweep never runs locally — the sandbox_cleanup_due_at stamp is never cleared. It's harmless (lingering timestamps, local cleanup_after_verification is a no-op), but it's an inconsistency between the documented contract and the behavior. Consider either gating the stamp/pause on not settings.is_local_mode or noting that local mode ignores the value. Not blocking — the docstring already signals cloud-only intent.

• openhands/automation/watchdog.py, _verify_and_mark_run (both terminal sites) — in-transaction HTTP call. _defer_sandbox_cleanup issues pause_sandbox (a network round-trip with a 30s timeout) inside the DB session, before mark_stale_runs commits. This holds the transaction open across the pause call. This is consistent with the pre-existing pattern — the immediate backend.cleanup_after_verification(run_id) is also called inside the session — so it's not a regression. Worth being aware of if delays creep in, but no action needed for this PR.

• openhands/automation/watchdog.py, _verify_and_mark_run — duplicated branch. The if settings.sandbox_cleanup_delay_seconds > 0: _defer... else: cleanup... block appears at both terminal sites (verified-exit and unverifiable-timeout). It's 4 lines each, so a helper would barely save anything, but if a third terminal path ever appears it'd be worth extracting. Acceptable as-is.

---

[STYLE NOTES]

• Skipped. Comments are informative and explain why (pause-vs-keep-alive rationale, partial-index justification, reconcile semantics), not what. No noise.

---

[TESTING GAPS]

• None material. Tests cover the deferred path (pause + stamp), keep_alive=true override, both watchdog terminal sites, sweep only-touches-due-rows, stamp-cleared-on-failure, and pause_sandbox 200/404/transport-failure. They assert on real DB state (sandbox_cleanup_due_at, completed_at, status) rather than only mocked calls. The router reconcile path (watchdog-marks-FAILED-then-callback-flips-to-COMPLETED with a delay configured) isn't explicitly tested for the re-stamp, but it's covered by the existing reconcile tests + the values dict reuse, and the behavior is defensible.

---

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  • Default 0 preserves today's behavior exactly — no existing deployment changes unless an operator opts in.
  • The new path only activates when sandbox_cleanup_delay_seconds > 0 and keep_alive is not True, both explicitly checked.
  • Best-effort semantics match the immediate-cleanup path; the runtime TTL reaper remains the backstop, so a sandbox that won't pause or delete cannot pin the sweep or leak forever.
  • Migration is additive (nullable column + partial index) with a clean downgrade.
  • CI green (unit-tests, backend, build all pass).

VERDICT: ✅ Worth merging. Core logic is sound, well-tested, and the design choice (pause over keep-alive) is the right one.

KEY INSIGHT: Stamping the due timestamp inside the optimistic-lock terminal UPDATE — rather than as a separate step — is what makes this race-safe against the watchdog/callback concurrency that already exists in this codebase.

---

This review was generated by an AI agent (OpenHands) on behalf of the reviewer.

[tofarr]

🍰

[openhands-release-bot]

🚀 Released in 1.12.0.