DRU-379 - Druks sandbox client: set_expiry / release unchanged - #409
DRU-379 - Druks sandbox client: set_expiry / release unchanged#409druks-operator-treadstone[bot] wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Verdict: approve
Verification
All five acceptance criteria pass, verified directly against the diff (git diff 6b350ce591652c72b91a7254bd40985508bcca0c...16c4b2607774b9378fe2d9bdcbe56fae34c382c2), which touches exactly two files: backend/druks/sandbox/client.py (+17) and backend/tests/test_sandbox_lifecycle.py (+47/-1).
- AC1 (method shape/lifecycle) — pass.
client.py:238-253:set_expiry(self, *, host_id: str, expires_at: datetime) -> Noneobtainsapi = self._api(), callsawait api.renew_host(host_id, expires_at=expires_at), and closes viafinally: await api.aclose()— identical lifecycle idiom torelease/list_hosts. - AC2 (SandboxNotFoundError propagates) — pass.
test_set_expiry_surfaces_missing_hostsetsrenew_raisesto aSandboxNotFoundErrorinstance and assertspytest.raises(SandboxNotFoundError)withexcinfo.value is missing— an identity check confirming unmodified propagation, not swallowed or remapped toHostGone. - AC3 (expires_at forwarded verbatim) — pass.
test_set_expiry_forwards_expires_at_to_sdk_renewassertsapi.renewed == [("host-xyz", expires_at)]with no clamp/override in the method body. - AC4 (release unchanged, no migration/durable_runs change) — pass.
release()body is byte-identical to base; no file underbackend/migrations/was added; reattach key logic untouched. - AC5 (docstring) — pass. The docstring states clipping is how idle-hold ends, that drukbox's janitor still reaps at expiry, and that
releaseremains the hard delete.
CI: both underlying checks (backend, frontend) that the verification profile's "checks" name covers are green for this head SHA, so ruff check backend, frontend lint, frontend build, pytest backend/, and frontend test are all recorded as passing via CI.
No blocking findings, no open findings carried forward (round 1).
Code review
Advisory-only pass, clean-room (no ticket/AC visibility). One medium finding:
set_expirycurrently has zero callers outside its own tests (grep -rn "set_expiry" backendoutside the test file returns only the definition atclient.py:238). The plan's own scope note confirms this is deliberate — wiring an idle-hold caller is explicitly deferred to a follow-up ticket — but flagging it so the primitive doesn't linger unused if that follow-up slips. Filed as DRU-412, a child of DRU-379 (Linear auto-linked it to the existing DRU-380 follow-up, which already covers wiring the caller).
Everything else checked clean: the method mirrors the established _api()/try-finally aclose() pattern used by sibling methods, SandboxNotFoundError handling is consistent with how release (swallows) and attach (remaps to HostGone) each own their respective error semantics, the SDK's renew_host signature matches the fake's signature exactly, both new tests assert on behavior rather than implementation/prose, and no out-of-scope files were touched.
Linear ticket: DRU-379
Plan
DRU-379 — Druks sandbox
Client.set_expiry; release unchangedUnderstanding
Idle-hold needs to clip a sandbox host's lease down without deleting the VM. The SDK
already exposes
SandboxAPI.renew_host(host_id, *, expires_at=None)→POST /hosts/{id}/renew(DRU-378, Done). This ticket adds a thin Druks-client wrapper. Scope is one method plus tests;
no callers are wired here (that's DRU-380).
Change (all in
backend/druks/sandbox/)client.py— addClient.set_expiry(host_id, expires_at)next torelease. It createsits own
SandboxAPIviaself._api(), callsapi.renew_host(host_id, expires_at=expires_at),and
aclose()s in afinally— mirroring the lifecycle idiom ofrelease/list_hosts.Follow the repo's keyword-only idiom for the params.
SandboxNotFoundErroris already imported.set_expirydoes not catchSandboxNotFoundError: a missing hostpropagates it unchanged. Do not map it to
HostGone(that mapping isattach's, for stalereattach refs; the ticket pins
SandboxNotFoundErroras the surfaced "gone" signal here).reaps the host at expiry, and
release()stays a harddelete_host.Do not
durable_runs; reattach stays{workflow_id}:sandbox(no migration).now + SANDBOX_HOST_LEASE_SECONDSisexplicitly optional and left out — pass
expires_atstraight through (see rejected).renew_hostalready exists.Tests (
backend/tests/test_sandbox_lifecycle.py)Extend
_FakeAPIwith arenew_hostrecording hook (+ optionalrenew_raises). Cover:set_expiryforwards theexpires_attorenew_host; aSandboxNotFoundErrorfromrenew_hostsurfaces out ofset_expiry; existingreleasedelete behavior is untouched.Out of scope
Wiring
set_expiryinto an idle-hold caller (DRU-380) and any lease-policy/clip-extend logic(DRU-377).
Acceptance Criteria
Client.set_expiryis added in backend/druks/sandbox/client.py, accepts a host_id and an expires_at, and callsapi.renew_host(host_id, expires_at=expires_at)on a SandboxAPI it obtains viaself._api()and closes withaclose()in a finally (matching sibling methods).renew_hostraisesSandboxNotFoundError,set_expirypropagates it unchanged — it is not swallowed and not remapped toHostGone.set_expiryon a fake API whoserenew_hostraisesSandboxNotFoundErrorre-raisesSandboxNotFoundError.set_expiryforwards the caller-suppliedexpires_attorenew_host(no client-side clamp/override).expires_atpassed into the fakerenew_hostand asserts it equals the value passed toset_expiry.release()is unchanged — still callsdelete_hostand swallows already-gone/failed deletes; no host-id column is added todurable_runsand no new migration is introduced.releasebody and reattach key untouched; no file under backend/migrations/ added; existingtest_release_calls_sdk_deletestill passes.set_expirycarries a docstring stating that clipping expiry is how idle-hold ends, that Drukbox's janitor still reaps at expiry, and thatrelease()remains a hard delete.