DRU-380 - Add sandbox hold - #421
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Verdict: approve
Verification
All six acceptance criteria pass against 6b350ce591652c72b91a7254bd40985508bcca0c...3163b711ef5c2c5dcf5ad8390d7657ca0216306e, round 1 (implementation revision 1).
- AC1 — pass.
Gate.wait(workflows.py:294-301) takeshold_sandbox: bool | timedelta | None = False, forwards it to_park(:341-343);_park(:342-347) accepts the same param, defaultFalse. - AC2 — pass.
_park's falsy branch callsworkflow._reap_run()(:352-355). Verified bytest_park_without_hold_releases_the_warm_host. - AC3 — pass. Truthy
hold_sandbox+ a warm host routes to_hold_host: callsset_expiry, neverrelease, retainsself._host. Verified bytest_park_with_hold_clips_the_lease_and_keeps_the_host. - AC4 — pass.
_hold_host(:879-896) clips viamin(current expiry, now+span)for bothTrue(rotate threshold) and atimedelta. Verified bytest_hold_true_clips_to_one_more_worst_case_call,test_hold_never_outlasts_the_lease_drukbox_granted,test_hold_timedelta_clips_to_the_requested_span. - AC5 — pass.
review()(:829) still calls_parkwith no hold arg, so it reaps. No-warm-host hold case covered bytest_hold_without_a_warm_host_touches_nothing. - AC6 — pass. No change to
_lease_host's idempotency-keyed re-lease path. Same-instance reuse and restarted-worker re-provision each covered by a dedicated test.
Checks: uv run ruff check backend — pass (clean locally; GitHub backend check green for head_sha). uv run pytest backend/ — pass (backend check green for head_sha; the 11 new/existing tests in test_warm_host_rotation.py pass locally). Frontend build/test — not_run, diff is backend-only.
Open findings (non-blocking)
- [High, informational]
sandbox_client.set_expirydoesn't yet exist on the realClient(backend/druks/sandbox/client.py) — only the test fake defines it. This is the explicitly plan-scoped DRU-379 (#409) dependency: this PR intentionally doesn't re-addset_expiryto avoid a merge collision, and no caller currently passeshold_sandbox=True, so nothing exercises this path in production yet. But the hold capability this PR ships won't actually work until #409 lands and wires the real method. - [High, informational]
_hold_hostclips the lease server-side but never updates the in-memoryself._host.expires_at(read-only, derived fromself.record), so_lease_host's rotation check can compare against a stale, pre-clip expiry after a hold — a same-worker resume in the gap between the real clip and the stale threshold could try to reuse an already-reaped host. Not exercised by the new tests (none advance simulated time between hold and the follow-up lease check).
Both items are inline on the diff and filed together as DRU-413, a child of DRU-380, for follow-up once DRU-379 lands.
Code review
An independent clean-room lens (no access to the plan/ticket) reviewed the diff for maintainability. It found the same set_expiry gap above as its one high finding, and confirmed: no reuse/duplication issues, idiomatic fit with the neighboring _reap_run/_lease_host methods, tests targeting behavior rather than implementation, no dead branches, no secret/log leakage, and no out-of-scope edits — both changed files are squarely scoped to the sandbox-hold feature. It noted one low-severity nit (the bool | timedelta | None signature never distinguishes None from False) but didn't treat it as a separate finding.
| expires_at = datetime.now(UTC) + span | ||
| if self._host.expires_at: | ||
| expires_at = min(self._host.expires_at, expires_at) | ||
| await sandbox_client.set_expiry(host_id=self._host.id, expires_at=expires_at) |
There was a problem hiding this comment.
Code-review lens (advisory, non-blocking): sandbox_client.set_expiry doesn't exist on the real Client in backend/druks/sandbox/client.py — only the test's _FakeSandboxClient defines it. This is the known, plan-scoped DRU-379 (#409) dependency, so it's not a defect in this diff, but it does mean the hold path will AttributeError in production until #409 lands and wires the real method. Filed as follow-up DRU-413.
| host, self._host = self._host, None | ||
| await sandbox_client.release(host_id=host.id) | ||
|
|
||
| async def _hold_host(self, hold: bool | timedelta) -> None: |
There was a problem hiding this comment.
Verification lens open finding (non-blocking — ACs pass as literally specified and tested): _hold_host clips the lease via set_expiry but never updates self._host's local expires_at (it's a read-only property derived from self.record). _lease_host's rotate check then compares against the stale, pre-clip expiry, so a same-worker resume in the window between the real clipped expiry and the stale threshold could reuse a host_id the provider already reaped, surfacing as HostGone rather than a clean cold re-provision. Not exercised by the new tests since none let simulated time pass between the hold and the follow-up _lease_host call. Worth addressing alongside the set_expiry wiring — folded into DRU-413.
Linear ticket: DRU-380
Plan
Add sandbox hold (opt-in warm-VM retention across a park)
Goal. Let a warm-VM run parking on a gate keep its host (clipping the lease) instead of reaping it, so a resume reattaches warm.
review()and terminal exit still reap. Default preserves today's behavior.What the repo already gives us
_park(backend/druks/workflows.py:333) unconditionallyawait workflow._reap_run()before suspending onDBOS.recv_async._reap_run(:859) deletes the VM and nullsself._host.Gate.wait(:294) calls_park;review()(:815) calls_parkdirectly (so the flag must live on_park).f"{workflow_id}:sandbox"in_lease_host(:836), which rotates when remaining lease< SANDBOX_HOST_ROTATE_BEFORE_SECONDS(75 min; constants atsandbox/constants.py:6-8).finally: await instance._reap_run()(workflows.py:1084) already reaps on every exit.Client.set_expiry(host_id, expires_at)from DRU-379 (PR DRU-379 - Druks sandbox client: set_expiry / release unchanged #409, still In Review — not present in this checkout). See scope note below.Change (one module + tests)
Gate.wait— addhold_sandbox: bool | timedelta | None = False; forward it to_park._park— addhold_sandboxparam (defaultFalse). Replace the unconditional_reap_run()with a hold-aware branch: falsy →_reap_run()(today); truthy andself._hostset → clip the lease via a new_hold_host(hold_sandbox)that callssandbox_client.set_expiry(...)and retainsself._host. No warm host held → nothing to hold (no set_expiry, no delete)._hold_host(mirror_reap_run) computes the clipped expiry:True→min(current expiry, now + SANDBOX_HOST_ROTATE_BEFORE_SECONDS)(one worst-case call still fits);timedelta→min(current expiry, now + hold).review()— unchanged; passes no hold, so it still reaps.self._host; a restarted worker re-leases via the existing{workflow_id}:sandboxidempotency key (warm if the clipped host survives, cold if Drukbox reaped it).backend/tests/test_warm_host_rotation.py's fake (_FakeSandboxClient+set_expiry/deletedtracking) to cover the hold decision and the follow-on_lease_host.Scope / risk
Client.set_expiry. This PR does not re-add it (that would collide with PR DRU-379 - Druks sandbox client: set_expiry / release unchanged #409). Production wiring callssandbox_client.set_expiry; tests patch a fake client that exposes it. Assumes DRU-379 lands first.durable_runshost-id column (reattach stays the idempotency key), no new reconciler/janitor, no Drukbox cap. No app currently passeshold_sandbox=True; this PR only adds the capability + the set_expiry caller path.Acceptance Criteria
Gate.waitaccepts a keyword parameterhold_sandbox: bool | timedelta | None = Falseand forwards it to_park;_parkaccepts ahold_sandboxparameter defaulting toFalse.Gate.waitand_parkin backend/druks/workflows.py and the call fromwaitto_park.hold_sandboxis falsy (False/None), a park releases (deletes) any warm host exactly as today —_parkreaps via_reap_run/sandbox_client.releaseand does not callset_expiry.hold_sandboxis truthy and a warm host is held,_parkcallssandbox_client.set_expiryfor that host and never deletes it, and retainsworkflow._host(not nulled) across the suspend.workflow._hoststill set after the hold path runs.set_expiryis: forhold_sandbox=True, no later thannow + SANDBOX_HOST_ROTATE_BEFORE_SECONDSand no later than the host's current expiry; for atimedelta,min(current expiry, now + hold).expires_atargument to the fakeset_expirymatches the clip for both True and a timedelta input.review()continues to park without a hold and therefore still reaps the warm host (noset_expiry); parking with a hold when no warm host is held (e.g.steps_reuse_sandbox=Falseor_host is None) issues neither a delete nor aset_expiry.review()'s_parkcall (no hold arg) and a unit test that a hold park with_host is Nonerecords no set_expiry and no delete._host, a same-instance_lease_hostreturns the same host id without provisioning; with_hostcleared and the clipped lease lapsed,_lease_hostprovisions exactly once under the{workflow_id}:sandboxkey (no second host on a pre-expiry resume)._lease_hostmirroring test_warm_host_rotation.py: retained-host reuse, and re-provision-once after clip/expiry.