feat(islo): add opt-in idle pausing - #1706
Merged
Merged
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review blockedAutomated review did not run, so no review verdict was produced. Reason: The input-safety check rejected material in this revision. No detected value, path, or scanner output is reproduced here. ClawSweeper will not retry this unchanged revision. Next step: If this is a genuine credential, remove and rotate it. If it is an intentional test fixture, a maintainer must review and qualify it. |
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
Keep default creation unchanged, map opted-in idle timeouts without overflow, and reuse provider readiness after lease admission and session binding. Preserve lifecycle checks on reclaim, failure recovery, and contributor credit. Co-authored-by: zozo123 <yossi.eliaz@incredibuild.com>
Preserve the idle-pause feature while incorporating the current main help fix.
steipete
changed the title
Contributor
|
Landed in 980e1c0, preserving @zozo123’s contribution and adding the changelog entry. Idle pausing is explicitly opt-in; default creation remains unchanged. The maintainer update fixes duration rounding overflow and routes paused reuse through the existing readiness owner after session binding, so failures keep a recoverable claim/session. Live proof used two sequential, newly owned Islo sandboxes: the opted-in sandbox automatically paused; the baseline CLI failed to execute its marker on that paused resource; the candidate resumed it and printed its marker. Manual pause followed by reuse with the option unset also succeeded. Both sandboxes were deleted normally, subsequent reads returned not-found, and local claims and proof processes were cleaned up. This was synthetic On the actual merged source, 55 focused race-test cases/subtests passed with no skips or failures, along with All post-merge workflows passed on that exact commit: CI (12 jobs), Connector E2E (five passed, desktop-resize skipped), CodeQL (four analyses), and Pages (build and deploy). No rerun was required. These are workflow results, not a replacement for the separate scanner’s no-verdict outcome documented above. |
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
2 participants
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.
Summary
Add an explicit Islo idle-pause opt-in through
islo.idlePause,CRABBOX_ISLO_IDLE_PAUSE, and--islo-idle-pause. Unset or false keeps the existing create request unchanged. Opted-in creation rounds the idle timeout up safely, sendspause_after_idlewithauto_resume=never, and adds no deletion or absolute-pause deadline.Paused reuse shares the provider's existing readiness behavior. Plain leases check readiness after the run session is bound; enrolled leases retain their admitted readiness path. Resume failures preserve the recovery session and claim instead of bypassing finalization. Explicit reclaim checks a reported incompatible idle policy before publishing a claim; legacy responses without policy metadata remain supported without claiming the requested policy is enforced.
This maintainer update preserves @zozo123's contribution and branch history, fixes duration overflow, incorporates current main, and consolidates repetitive reclaim tests. Activity accounting remains an explicit operational caveat: opting in may pause long-running or externally accessed workloads.
Verification
The current-main integration passed 55 tests and subtests, with no skips or failures:
The built CLI's help delta contains only the new opt-in flag. Managed review found no actionable P0 findings; that is a scoped review, not a general correctness certificate.
Live provider proof
A source-blind validator exercised the real CLIs against two newly owned Islo sandboxes on 2026-09-14, using Ubuntu 26.04, 2 vCPUs, 4096 MB memory, and 20 GB disk. The baseline was built from
ca745fd8; the candidate was built fromfea91f01. Both artifact hashes were verified before and after. The default control was deleted before the opted-in case was created.The opted-in creation used the actual command below; executable paths and lease IDs in this receipt are redacted:
Only status reads followed readiness. The provider automatically changed the sandbox from running to paused, observed about 70 seconds after warmup returned. On that same paused lease, with the same fixture/configuration and
--keep --no-sync:RESOURCE_UNAVAILABLE; no marker; lease remained paused and claimed.CANDIDATE_PAUSED_MARKER; running state and retained claim.MANUAL_PAUSED_UNSET_OK; running state and retained claim.SANDBOX_NOT_FOUND; local claims empty.All controller/CLI processes were reaped and the credential task window closed. This proves bounded synthetic
--no-syncworkload recovery, not production workload coverage. The CLI does not expose lifecycle policy fields: hidden provider-side immutability was not directly observed. Application-side absence of policy updates is established by the reused-run path and SDK pause/resume requests (which carry only the sandbox name), alongside the request/reclaim regression tests.CI, Connector E2E, Release Check, and Docs UI Proof passed on the exact candidate head. CodeQL default setup excludes fork PRs; it was not a passed PR scan and remains a post-merge check.
ClawSweeper produced no code-review verdict because its input scanner refused the source. No rejected packet/log was inspected, and no scanner retry was requested. The separate managed reviews had already completed before this notice was observed; no admin override or merge-protection bypass is requested.