ENG-822 - Sandbox provisioning failures are transient but unclassified — the in-run retry never covers them

[druks-operator]

Linear ticket: ENG-822

Plan

Approach

Use the existing harness failure taxonomy rather than adding a second classified-error family. HarnessSandboxError in backend/druks/harnesses/exceptions.py already owns the required transient retry schedule, but its sandbox code cannot satisfy the operator-pinned sandbox_provisioning classification. Add a provisioning-specific subclass beside it that overrides only code, inheriting the established retry metadata.

Translate failures at the shared Client boundary, not behind an agent-context check. This applies consistently to agents, doctor.py, and browser callers; their existing broad reporting/wrapping behavior requires no caller changes.

Implementation

• In backend/druks/harnesses/exceptions.py, add the HarnessSandboxError specialization with code = "sandbox_provisioning"; do not duplicate retry or retry_delays.
• In backend/druks/sandbox/client.py, translate only the current SDK's transient control-plane shapes:
  • SandboxProvisioningError and SandboxUnavailableError from host creation.
  • SandboxUnavailableError from host lookup while attaching.
    Preserve each original exception as the cause and leave the SandboxAPIError base and its fatal subclasses untouched.
• Refine Client.acquire's existing post-create rollback path so a failure establishing that the fresh VM is reachable and usable during SSH/helper setup is classified as sandbox provisioning after cleanup. Do not classify cancellation or unrelated local/programming failures.
• Leave backend/druks/sandbox/exceptions.py unchanged: it remains the sandbox layer's own SandboxError vocabulary, while the client boundary emits the harness taxonomy consumed by orchestration.
• Leave Agent.__call__ and _execute_run unchanged. The new subtype automatically enters their existing bounded transient retry and classified run-failure branches.
• Preserve the logical keys already supplied by backend/druks/agents.py and Workflow._ensure_host; retries re-enter _run with the same workflow/agent identity, so no new key generation or persistence is needed.
• Extend backend/tests/test_sandbox_lifecycle.py for create/attach SDK translation, fresh-host SSH/helper failure classification after rollback, cause preservation, and representative fatal passthrough.
• Extend backend/tests/test_agents.py for recovery and exhaustion through the durable and in-step retry paths, capturing stable keys for ephemeral and workflow-reused acquisition.
• Extend backend/tests/test_run_state.py for persistence of exhausted provisioning failures as sandbox_provisioning.

Scope boundaries

No schema, migration, API, frontend, documentation, or non-agent caller changes are required. Out of scope: drukbox-side fixes (error formatting or create_vm timeout headroom), and retrying genuinely fatal provisioning errors such as bad images or authentication failures.

Verification

Run focused tests for the three changed backend test modules, then the backend-only profile: uv run pytest backend/, uv run ruff check backend, and uv run ruff format --check backend.

Ruled out

• Defining the classified failure in backend/druks/sandbox/exceptions.py: that module contains the sandbox layer's own SandboxError family and would mix unrelated bases while reversing the established sandbox-to-harness translation direction.
• Reusing HarnessSandboxError directly: retries would work, but exhausted runs would retain the generic sandbox code instead of the required sandbox_provisioning classification.
• Catching the SandboxAPIError base as transient: this would retry authentication, validation, conflict, and generic response failures that cannot recover without changed credentials or inputs.
• Leaving acquire-time SSH/helper reachability failures unclassified: the host would be rolled back, but the raw error would still bypass agent retry and produce an empty run failure code—the same dead-run symptom this change addresses.
• Adding a separate sandbox retry loop or retrying the whole workflow: this would duplicate the existing bounded retry policy and could repeat orchestration beyond the failed agent attempt.
• Generating a new idempotency key per attempt: an ambiguous create timeout could provision duplicate hosts instead of resolving to the existing logical acquire.
• Gating translation on agent execution: acquire, provision, and attach are shared boundaries, and caller-specific gating would make identical provisioning failures depend on who invoked the client.
• Changing drukbox error formatting or timeout budgets here: those service-side concerns are independently tracked by ENG-821 and do not repair Druks's missing classification route.

Acceptance Criteria

• AC1: create_host failures represented by drukbox SandboxProvisioningError or SandboxUnavailableError, and get_host attach failures represented by SandboxUnavailableError, are translated at the shared sandbox-client boundary into a provisioning-specific HarnessSandboxError subclass with code="sandbox_provisioning"; the existing transient retry policy is inherited and the SDK exception is preserved as the cause.
  • Verification: Focused tests in backend/tests/test_sandbox_lifecycle.py exercise create and attach translation and inspect the resulting type, code, retry classification, and exception cause.
• AC2: When a freshly created host cannot become usable during acquire-time SSH/helper setup, Client.acquire performs its existing rollback—closing the sandbox, deleting the host, and removing its key—then raises the same classified provisioning failure from the original reachability/setup error. Cancellation and unrelated local/programming errors remain unchanged.
  • Verification: A sandbox lifecycle test simulates failure while reaching the new VM and asserts rollback plus classified propagation with the original cause.
• AC3: A transient provisioning failure during an agent run is handled by the existing bounded retry/backoff paths, including durable agent steps and agents invoked inside an enclosing step; a later successful attempt proceeds. Every attempt for one logical acquire receives the same idempotency key for both ephemeral and workflow-reused sandboxes.
  • Verification: Focused tests in backend/tests/test_agents.py simulate failure then recovery through both retry paths, assert the applicable sleep mechanism, and capture unchanged keys across ephemeral and reused-host attempts.
• AC4: After transient provisioning retries exhaust, the propagated classified error still has code="sandbox_provisioning", and _execute_run persists Run.failure_code as sandbox_provisioning rather than an empty string.
  • Verification: Agent tests cover bounded exhaustion, and backend/tests/test_run_state.py verifies persistence of the classified failure code.
• AC5: Authentication, validation, conflict, generic response, and other non-transient drukbox SDK failures are not converted into provisioning failures and do not trigger automatic agent retry.
  • Verification: A representative fatal-error test asserts the original SDK error propagates after one provisioning attempt with no retry sleep.

[druks-reviewer]

Verification — PASS (round 1)

Diff: backend/druks/harnesses/exceptions.py (+13), backend/druks/sandbox/client.py (+54/-11), plus test-only changes to backend/tests/test_agents.py (+180), backend/tests/test_run_state.py (+19), backend/tests/test_sandbox_lifecycle.py (+133/-1). No schema, migration, API, frontend, or dependency/lockfile changes — matches the plan's declared scope.

AC1 — PASS. HarnessSandboxProvisioningError(HarnessSandboxError) added at harnesses/exceptions.py with code="sandbox_provisioning", inheriting the transient retry/retry_delays without redeclaring them. Client.acquire translates create_host's SandboxProvisioningError/SandboxUnavailableError; Client.attach translates get_host's SandboxUnavailableError. Both preserve the SDK exception via raise ... from exc. Covered by test_acquire_translates_transient_create_failures and test_attach_translates_unavailable_lookup_failure, asserting type, code, retry metadata, and __cause__.

AC2 — PASS. client.py's acquire rollback (aclose, _best_effort_delete, key unlink) runs unconditionally in except BaseException, then reclassifies only if the error is in _ACQUIRE_SETUP_REACHABILITY_ERRORS (SandboxError, asyncssh.Error, OSError, TimeoutError) — CancelledError re-raises unchanged. Covered by test_acquire_classifies_setup_reachability_failure_after_rollback and test_acquire_setup_cancellation_propagates_unclassified.

AC3 — PASS. agents.py/workflows.py are unchanged, so the new subtype flows through the existing bounded transient retry unmodified. Idempotency keys are stable by construction (keyed on workflow/step identity, not attempt count) for both ephemeral and workflow-reused acquisition, verified across durable-step and in-step retry paths.

AC4 — PASS. Exhausted retries propagate code == "sandbox_provisioning", and _execute_run persists Run.failure_code == "sandbox_provisioning" (verified in test_run_state.py), replacing the previous empty-string dead-run symptom.

AC5 — PASS. Fatal SandboxAPIError subclasses (auth, validation, conflict, etc.) are untouched — only the two transient shapes are caught — so fatal errors propagate after a single attempt with no retry sleep.

Checks: uv run ruff check backend pass · uv run ruff format --check backend pass · uv run pytest backend/ pass (1254 passed) · Backend CI (On Pull Request Backend / checks) green for 777d1894. Frontend lint/build/test checks correctly did not trigger — the PR touches no frontend/** files and those workflows are path-filtered.

No blocking findings.

Open findings (non-blocking)

• Low — _ACQUIRE_SETUP_REACHABILITY_ERRORS in client.py includes asyncssh.Error, OSError, and TimeoutError alongside SandboxError, but new tests only exercise the SandboxError and CancelledError branches; the other three types are untested. Low risk given the generic isinstance check, but worth a follow-up test if convenient.
• Low — pre-existing, out of this diff's scope: the rollback calls inside the except BaseException block in client.py aren't themselves guarded, so a raise during rollback could mask the original error before reclassification. This shape predates the PR and isn't part of the plan's stated scope.

Code review

No medium or high findings. The clean-room lens read the full diff and all five touched files end to end:

• HarnessSandboxProvisioningError follows the exact existing wrapping pattern used at sandbox/host.py (catch SandboxError, re-raise as a HarnessError subclass with raise ... from exc). No new retry/dispatch mechanism was invented — the generic except HarnessError loop in agents.py already dispatches on .retry/.retry_delays.
• Every other caller of Client.acquire/attach/provision/ephemeral (doctor.py, browser/login.py, workflows.py::_ensure_host, browser/sessions.py) was checked — none depended on the previously-unwrapped raw SDK errors propagating, so wrapping doesn't change their behavior.
• The reachability-error tuple is deliberately narrow and backed by a dedicated cancellation test proving CancelledError isn't misclassified.
• New tests assert behavior (retry counts, sleep delays, idempotency-key stability, failure_code persistence) rather than implementation/mock-call shape — consistent with sibling HarnessOverloadedError/HarnessRateLimitError tests.
• No edits outside the stated scope.

One low, non-blocking cosmetic note: a couple of test functions use function-local import asyncio/import asyncssh rather than a module-level import — consistent with this file's existing convention elsewhere, not worth a required change.

Low-only findings, so no follow-up sub-issue was filed.

[druks-reviewer]

Verification

Verdict: PASS — all five acceptance criteria of the plan (revision 2) are satisfied by this diff (implementation revision 1, round 1).

• AC1 (SDK translation at the Client boundary): backend/druks/harnesses/exceptions.py adds HarnessSandboxProvisioningError(HarnessSandboxError), overriding only code = "sandbox_provisioning" and inheriting retry/retry_delays (TRANSIENT, (60, 300)) as specified. client.py's create_host path catches exactly (SandboxProvisioningError, SandboxUnavailableError) and re-raises with from exc (cause preserved); attach/get_host catches only SandboxUnavailableError. SandboxAPIError and its fatal subclasses are untouched. Covered by test_acquire_translates_transient_create_failures and test_attach_translates_unavailable_lookup_failure in test_sandbox_lifecycle.py — pass.
• AC2 (acquire-time SSH/helper rollback + classification): the existing rollback (aclose, _best_effort_delete, key unlink) runs unconditionally; only the re-raise is refined via a narrow _ACQUIRE_SETUP_REACHABILITY_ERRORS tuple. CancelledError (a BaseException) correctly falls through unclassified. Covered by test_acquire_classifies_setup_reachability_failure_after_rollback and test_acquire_setup_cancellation_propagates_unclassified — pass.
• AC3 (in-run retry, stable idempotency key): Agent.__call__, the in-step retry loop, and Workflow._ensure_host are unchanged, as planned — the new error type flows through existing HarnessError/Retry.TRANSIENT machinery. Keys are deterministic (f"{workflow_id}:{step}", f"{workflow_id}:sandbox") and unaffected by retry count. Covered by three focused tests in test_agents.py (durable retry, in-step retry, reused-host key stability) — pass.
• AC4 (exhaustion → classified failure_code persisted): _execute_run's existing generic HarnessError handling passes exc.code through unmodified. test_provisioning_failure_exhausts_retries_with_classified_code and test_an_exhausted_provisioning_failure_stamps_its_code (test_run_state.py) confirm Run.failure_code == "sandbox_provisioning" — pass.
• AC5 (fatal SDK errors excluded from retry): representative fatal-error tests (SandboxAuthError at the lifecycle layer, SandboxValidationError at the agent layer) confirm no rollback attempt, no retry sleep, error propagates as-is — pass.

Verification profile:

• uv run ruff check backend — pass ("All checks passed!")
• uv run ruff format --check backend — pass (342 files formatted)
• uv run pytest backend/ — pass (1254 passed; focused modules also pass in isolation, 72 passed)
• GitHub check "On Pull Request Backend / checks" — SUCCESS for head_sha 777d189
• Frontend lint/build/test — not applicable; no frontend files touched by this diff

Findings: none blocking. Six-sweep review (AC coverage, plan-specified approach, test coverage, dependency/lockfile changes, input validation/error boundaries, out-of-scope side effects) found nothing that contradicts the plan, the ticket, or the "ruled out" list.

Open findings (advisory, non-blocking): two observations from the verification pass, both superseded by the more concrete write-up under Code review below — see that section and the filed follow-up (ENG-866).

Code review

Clean-room code-review lens (diff-only, no ticket/plan context) found no high-severity issues. Two non-blocking findings:

1. Medium — backend/druks/sandbox/client.py, Client.acquire's create_host-time exception handler doesn't call delete_host on translation, unlike the sibling post-create rollback path a few lines below. Likely intentional (the exception carries no host id to delete), but undocumented. See inline comment. Filed as follow-up ENG-866 (child of ENG-822) since this is a genuine question worth confirming, not something this diff needs to resolve.
2. Low — backend/druks/sandbox/exceptions.py's SandboxUnreachable docstring says "terminal for the affected run," but the new setup-reachability handling in client.py reclassifies it as transient-retryable at one call site. Defensible given the different context (early "never became usable" vs. established-run tail-loop), but the shared docstring no longer reflects both use sites. Noted for whoever next touches that file; no action required now.

Everything else: the new exception fits the existing harness taxonomy exactly, SDK exception narrowing correctly preserves fatal passthrough and cancellation, new tests are behavior-focused (assert retry counts/keys/final codes, not call sequences) and follow the shape of existing sibling tests, no dead code, no duplicated abstractions, no out-of-scope edits.

[druks-reviewer]

Advisory (non-blocking), from the code-review lens: this except (SandboxProvisioningError, SandboxUnavailableError) handler on the create_host path wraps and re-raises HarnessSandboxProvisioningError without calling delete_host, unlike the sibling post-create setup-failure rollback path a bit further down (which calls _best_effort_delete). The drukbox SDK's docstring for SandboxProvisioningError notes the host row stays in error state server-side and suggests calling delete_host to release partial provider state.

This may well be intentional — the exception only carries a detail string with no host id, so there may be nothing to delete from here, and the ticket's safety argument already relies on idempotency-key dedup on the next create_host rather than immediate cleanup. But the diff doesn't document that assumption anywhere. A one-line comment confirming it (or a note if it's a real gap) would help the next reader.

Filed as a non-blocking follow-up: ENG-866.

[czpython]

I would prefer except _ACQUIRE_SETUP_REACHABILITY_ERRORS with a shortened version of this comment, and its ok to repeat the sandbox aclose and other stuff, to avoid the isinstance which just does what except does

[czpython]

trim this comment and use simplified english

[czpython]

trim this docstring and use simplified english

[druks-reviewer]

Hi @czpython — three style/structure changes picked up from your review, implementing them now:

1. client.py line 139 — replace isinstance with two except clauses. Split the current except BaseException + isinstance(error, _ACQUIRE_SETUP_REACHABILITY_ERRORS) guard into separate except _ACQUIRE_SETUP_REACHABILITY_ERRORS and except BaseException clauses, repeating the rollback (aclose, _best_effort_delete, unlink) in each so the branching is done by the exception machinery instead of isinstance. The comment above will be shortened accordingly.

2. client.py line 30 — trim the _ACQUIRE_SETUP_REACHABILITY_ERRORS comment. Shorter, plain language.

3. harnesses/exceptions.py line 66 — trim the HarnessSandboxProvisioningError docstring. Shorter, plain language.

No logic changes — same rollback behavior and same classification, just cleaner structure and prose. Will post an updated commit.

Addressed all three inline style comments in 067ff78: split the acquire rollback into two explicit except clauses (reachability vs. BaseException), trimmed the _ACQUIRE_SETUP_REACHABILITY_ERRORS block comment, and trimmed the HarnessSandboxProvisioningError docstring. No logic changes.

[druks-reviewer]

Verdict: PASS — round 2 (implementation revision 2) satisfies all five acceptance criteria of plan revision 2, and correctly applies the three style-only edits czpython requested in the round-1 CHANGES_REQUESTED review.

What changed this round: backend/druks/sandbox/client.py and backend/druks/harnesses/exceptions.py only, 15 insertions / 27 deletions, no test files touched — exactly matching the "three edits; no logic changes, no new tests" instruction.

1. Client.acquire's setup-failure handler was split from a single except BaseException as error: + isinstance(error, _ACQUIRE_SETUP_REACHABILITY_ERRORS) guard into two explicit clauses: except _ACQUIRE_SETUP_REACHABILITY_ERRORS as error: (rollback, then raise HarnessSandboxProvisioningError(...) from error) and except BaseException: (identical rollback, then bare raise). Verified byte-identical rollback (sandbox.aclose(), self._best_effort_delete(api, record.id), key_path.unlink(missing_ok=True)) in both branches, and that asyncio.CancelledError still falls to the second clause and propagates unclassified, since it's a BaseException not in the reachability tuple.
2. The _ACQUIRE_SETUP_REACHABILITY_ERRORS block comment was trimmed to two plain-English lines.
3. The HarnessSandboxProvisioningError docstring was trimmed to two plain-English lines.

The sibling create_host-time exception handling a few lines above is untouched, as required.

Acceptance criteria: AC1–AC5 all PASS. AC1/AC2 cover the exact code refactored this round and were re-verified behaviorally identical by diff reading and by the still-passing round-1 tests (test_acquire_classifies_setup_reachability_failure_after_rollback, test_acquire_setup_cancellation_propagates_unclassified, test_acquire_releases_host_when_helper_upload_fails, test_acquire_translates_transient_create_failures, test_acquire_passes_through_fatal_create_failure). AC3–AC5 cover agent-level retry/idempotency/exhaustion behavior untouched by this refactor, confirmed via test_provisioning_failure_recovers_through_durable_retry, test_provisioning_failure_recovers_in_step_retry, test_reused_host_retry_presents_a_stable_idempotency_key, test_provisioning_failure_exhausts_retries_with_classified_code, test_an_exhausted_provisioning_failure_stamps_its_code, test_fatal_sdk_error_does_not_trigger_agent_retry.

Verification: uv run ruff check backend clean; uv run ruff format --check backend clean (342 files); full uv run pytest backend/ — 1254 passed, 0 failures; GitHub check "On Pull Request Backend / checks" is green for head 067ff78. No frontend files touched, so frontend lint/build/test commands are not applicable this round.

No regressions introduced by this round; no unaddressed prior blockers (round 1 was itself a clean pass with no blocking findings).

Open findings

Carried forward from round 1, unchanged by this round's diff and not re-raised as blocking:

• (medium, non-blocking) Client.acquire's create_host-time exception handler doesn't call delete_host before re-raising HarnessSandboxProvisioningError, unlike the sibling post-create setup-failure rollback path. Already filed as follow-up ENG-866 (child of ENG-822).
• (low, non-blocking) SandboxUnreachable's docstring in backend/druks/sandbox/exceptions.py says "terminal for the affected run," but this PR's setup-time reachability path now reclassifies it as transient-retryable at one call site. No action required; noted for whoever next touches that file.

Code review

Clean-room review of the round-2 diff reported no findings. The refactor is behaviorally equivalent to round 1's approved code: the two-except-clause form correctly preserves cancellation/unrelated-error passthrough, mirrors the existing except-clause classification style already used a few lines away in attach() (except SandboxNotFoundError / except SandboxUnavailableError), and the comment/docstring trims remove implementation narration while keeping the why. No new abstractions, no scope creep, no dead code, no test changes. No follow-up issue filed — no medium/high findings this round.