feat: pause idle islo sandboxes instead of billing them - #1706
Open
zozo123 wants to merge 4 commits into
Open
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 availability
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed August 31, 2026, 2:07 PM ET / 18:07 UTC. ClawSweeper reviewWhat this changesThe PR maps Crabbox’s idle timeout to Islo’s create-time pause policy, resumes paused leases before reuse, rejects incompatible reclaims, and documents the lifecycle behavior. Merge readiness⛔ Blocked until real behavior proof is added - 5 items remain Keep open: the adapter-local implementation is coherent, but it converts the existing 30-minute global default into a provider-enforced pause for every newly created Islo sandbox while the PR documents that active workloads may also be paused. This needs an explicit compatibility decision and real Crabbox-to-Islo behavior proof before merge. Priority: P1 Review scores
Verification
How this fits togetherCrabbox’s Islo provider adapter turns CLI lease settings into remote sandbox requests. It creates or reuses sandboxes, then supplies a running sandbox to workspace sync, command execution, SSH, and share-related flows. flowchart LR
A[CLI lease settings] --> B[Islo provider adapter]
B --> C[Create lifecycle policy]
C --> D[Islo sandbox]
E[Reused lease] --> F[Check and resume sandbox]
F --> G[Sync exec and SSH]
D --> G
Decision needed
Why: The implementation deliberately changes an existing default into an external lifecycle action, and the provider's activity definition is unknown; this tradeoff cannot be resolved mechanically. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve current default runtime behavior until maintainers approve an explicit Islo idle-pause policy, then prove both an opted-in idle pause and uninterrupted active command/share behavior through the real CLI and provider. Do we have a high-confidence way to reproduce the issue? No high-confidence reproduction of the active-workload regression is available. Source proves the default mapping, but the supplied real-provider observation covers an idle sandbox rather than a Crabbox-created sandbox executing a command or serving a share past the timeout. Is this the best way to solve the issue? No. The provider-specific mapping is at the correct adapter boundary, but unconditional default-on behavior is not the safest solution while Islo's activity semantics and upgrade impact remain unproven. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 91f6710b04ad. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
The islo create request never carried a lifecycle policy: before this change
createSandbox built CreateSandboxRequest with name/image/gateway/snapshot/
vcpus/memory/disk only (internal/providers/islo/backend.go:605-635 on
origin/main), so a sandbox ran under Islo's tenant defaults and the lease's own
--idle-timeout was local-only bookkeeping. Islo accepts a lifecycle object on
create and enforces it, so the non-destructive half of the lease intent can be
pushed down: IdleTimeout -> pause_after_idle (backend.go:645).
This is on by default, not opt-in: --idle-timeout defaults to 30m and
validateLeaseDurations (internal/cli/lease_flags.go:421-429) rejects a
non-positive one, so every CLI-driven islo create now carries an idle pause
where it previously sent nothing. The comment and both docs say so, and so does
the CHANGELOG entry.
--ttl is deliberately NOT mapped to delete_after. A provider-side deletion
deadline lets Islo destroy a sandbox Crabbox still holds a lease claim on,
possibly mid-run; the closest in-tree precedent makes the same call in the other
direction, pinning Daytona's auto-delete interval off
(internal/providers/daytona/lifecycle.go:48-51) so Crabbox stays the only thing
that deletes a Crabbox lease. pause_after stays unset because Crabbox has no
generic absolute pause deadline, so no flag was invented for it.
auto_resume is pinned to "never" and Crabbox does its own resuming, so that
resuming stays an explicit decision: a resume is billable, and an explicit
`crabbox pause` should not be undone by a background policy. The reuse run path
previously went from lease resolution straight to syncWorkspace/exec with no
status check, unlike the SSH resolve path, so it now goes through the same
resume-if-paused helper (backend.go:226, resolveRunningSandbox in ssh.go) -
which also fixes reusing a lease that an explicit `crabbox pause` had paused.
pause_after_idle is enforced, not merely recorded, and control-plane reads are
not activity: a sandbox created with pause_after_idle=60 still reported
"running" at 75s and "paused" at 90s while being polled with GET /sandboxes
every 15s throughout. What else Islo counts as activity is undocumented, and
that is not a safe unknown, so it is named with its consequence in
isloLifecycleForConfig and in docs/providers/islo.md rather than papered over:
if an in-flight exec or in-VM share/tailnet traffic does not hold the idle
clock off, a run longer than --idle-timeout can be paused mid-exec. Paths that
resolve a lease recover by resuming first; PublishPeer and fetchRunFileAs do not
resolve, which the ssh.go comment and the docs both state.
Islo fixes the policy at create time and exposes no lifecycle update, so a
reclaim that would change pause_after_idle fails with exit 2 at the one place a
sandbox is already fetched for adoption (backend.go:789) instead of adopting a
lease whose advertised idle timeout is not in force. A sandbox that reports no
lifecycle (created before this, or by another tool) stays reusable, and
auto_resume drift is not a conflict because Crabbox does not rely on it.
Verified against a fake API that captures the exact create JSON: lifecycle
arrives as {"auto_resume":"never","pause_after_idle":1800} for a 30m idle
timeout, delete_after is absent whatever --ttl says, sub-second values round up,
and a reused lease reported as paused is resumed once before sync and exec.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Supersedes the "on by default, not opt-in" decision in the previous commit on
this branch. That commit derived pause_after_idle from --idle-timeout
unconditionally, and --idle-timeout defaults to 30m with a non-positive one
rejected by validateLeaseDurations, so every existing islo user got a
provider-enforced pause on their next create without asking for it. The same
commit documents that Islo does not define what counts as activity, so an
in-flight exec or a lease serving a published share could be paused mid-flight
by a change nobody selected. That unknown is a reason for the feature to be
opt-in, not a caveat to hang on a default.
islo.idlePause / --islo-idle-pause / CRABBOX_ISLO_IDLE_PAUSE now gates it,
defaulting to off and following the existing islo flag wiring
(RegisterIsloProviderFlags / ApplyIsloProviderFlags, fileIsloConfig, applyEnv).
With the knob unset, isloLifecycleForConfig returns nil and the create request
carries no lifecycle object at all - byte-for-byte the request earlier releases
sent - and --idle-timeout stays local bookkeeping. With it set, the mapping is
unchanged: pause_after_idle seconds (rounded up), auto_resume pinned to "never",
delete_after never sent.
isloLifecycleConflict is gated on the same knob. With the knob off Crabbox makes
no provider-side lifecycle claim, so refusing to adopt a sandbox that carries a
pause policy from an Islo tenant default, another tool, or an opted-in run would
turn an opt-in feature into a reclaim regression for operators who never enabled
it. With the knob on the conflict is exactly as before.
The unconditional resume-if-paused check on the reuse path stays unconditional:
`crabbox pause` and an Islo tenant default can leave a reused lease paused
whether or not the knob is set, so that is a fix in its own right rather than
mitigation for this policy.
Tests cover both sides of the knob. The create wire contract asserts that a 30m
default idle timeout with the knob unset produces `{}` plus the generated name
and no lifecycle key, and that an opted-in one produces
{"auto_resume":"never","pause_after_idle":1800}; the conflict table and a
reclaim test assert that drifted and unwanted provider policies are adoptable
with the knob off and still conflict with it on; and flag/file/env tests assert
the shipped defaults leave it off while each opt-in path turns it on and
--islo-idle-pause=false / CRABBOX_ISLO_IDLE_PAUSE=0 turn it back off.
Mutation-checked: dropping the guard in isloLifecycleForConfig fails the
default-path wire test and TestIsloIdlePauseIsOptIn; dropping the guard in
isloLifecycleConflict fails the two knob-off conflict cases and
TestIsloReclaimWithoutIdlePauseAdoptsDriftedPolicy; dropping the flag, file, or
env wiring fails the corresponding opt-in assertion.
Docs and CHANGELOG now describe opt-in behaviour, and keep the honest "what
counts as activity is unknown" note as the stated reason the knob exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
clearConfigEnv unsets every CRABBOX_ISLO_* variable so a developer's exported value cannot decide the outcome of a config test, but the new idle-pause knob was missing from that list. With CRABBOX_ISLO_IDLE_PAUSE=1 exported, applyEnv inside any config test flipped Islo.IdlePause on, which would make an opt-in-is-off assertion pass or fail for a reason unrelated to the code under test. Added it beside CRABBOX_ISLO_DISK_GB, and asserted the sealing directly in TestIsloIdlePauseDefaultsOffAndOptsInExplicitly: after clearConfigEnv, applyEnv over the shipped defaults must leave the pause off. Also softened the docs claim about Islo's idle enforcement. docs/providers/islo.md stated as bare fact that Islo enforces pause_after_idle and that control-plane reads are not activity; both come from a single manual check against the live API that nothing in this repository can reproduce - the tests here only pin the request Crabbox sends. The paragraph now dates the check and reports what it saw (pause_after_idle=60 still "running" at 75s, "paused" at 90s, polled with GET /sandboxes every 15s throughout) as an observation at a point in time rather than a documented contract, while keeping the load-bearing "what else counts as activity is undocumented" note and its consequences intact - that unknown is still the stated reason the knob is opt-in. Mutation-checked: removing "CRABBOX_ISLO_IDLE_PAUSE" from clearConfigEnv fails TestIsloIdlePauseDefaultsOffAndOptsInExplicitly under CRABBOX_ISLO_IDLE_PAUSE=1 with "clearConfigEnv must unset CRABBOX_ISLO_IDLE_PAUSE"; restoring it passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zozo123
force-pushed
the
feat/islo-lifecycle-classes
branch
from
September 1, 2026 20:23
dc1f047 to
288505e
Compare
Contributor
Author
|
@clawsweeper re-review Blocking findings addressed and the PR body updated with the fixes plus live behaviour proof. |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
TestProvidersDescribeBuiltBinaryContract pins a sha256 of `crabbox run --help` so an unintended change to the CLI surface trips a test rather than shipping. This branch adds --islo-idle-pause deliberately, so the baseline and byte count are updated to match. Hand-maintained by design: there is no regeneration script, which is what makes the tripwire meaningful.
This was referenced Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An islo sandbox held by a Crabbox lease keeps billing while nobody is using it. The adapter sent no lifecycle policy
on create, so an abandoned lease ran until someone noticed.
Change
Maps the lease idle timeout onto the provider's
pause_after_idle, so an idle sandbox pauses instead of billing.Deliberately narrow:
delete_afteris never sent. Handing a deletion deadline to the provider lets it destroy a sandbox Crabboxstill holds a claim on, possibly mid-run.
internal/providers/daytona/lifecycle.gopins auto-delete off for thesame reason.
auto_resumepinned tonever, so resuming stays Crabbox's decision. Resuming is billable, so it is worthdeciding deliberately rather than inheriting as a side effect.
Review fix (this push)
The reviewer was right that this should not be on by default. The previous version sent
pause_after_idleunconditionally from the 30-minute lease default, so existing users would get provider-side pausing they never
asked for — and, as these docs admit, what the provider counts as activity is not documented. A long-running
command or a lease serving a published share could be paused mid-flight.
Provider-side idle pausing is now opt-in. With the knob unset, no lifecycle object is sent and behaviour is
exactly as before this branch.
Evidence
Tests pin the exact JSON sent with the knob set, that nothing is sent with it unset, the omitted
delete_after,the pinned
auto_resume, and the adoption-conflict path.Live behaviour against real sandboxes (created and deleted for the test):
The two together are the point: unset changes nothing, set actually pauses.