feat: let delegated providers keep a lease alive - #1707
Open
zozo123 wants to merge 1 commit into
Open
Conversation
`crabbox heartbeat` only worked for SSH-lease backends: internal/cli/heartbeat.go asserted backend.(SSHLeaseBackend) and otherwise exited 2. A provider that delegates execution had no way to keep its lease alive even when its API can do so cheaply, and the islo adapter's Touch only rewrote a local label, so it never reached the provider at all. Add a provider-neutral optional capability, `LeaseHeartbeatBackend`, plus a `FeatureHeartbeat` feature so the capability is advertised and discoverable through `crabbox providers`. The request carries only the lease ID: there is nothing generic to negotiate, and a smaller contract is easier for the next delegated provider to implement. Backends that do not implement the capability fall through to the untouched SSH assertion with the same message and the same exit code, so existing behaviour is unchanged. The gate is a spec condition rather than a global config check, so a provider that can never hold a coordinator-registered lease is not excluded by an unrelated broker setting. `--idle-timeout` is refused on this path rather than silently ignored: neither plane exposes a lifecycle update, so a delegated heartbeat cannot change an idle window, and accepting the flag would have reported a number that was not in force. Implement it for islo as the first consumer. There is no dedicated heartbeat endpoint, so a heartbeat is one `true` exec, which exits 0 immediately and mutates nothing, making it safe to replay. Streams are discarded so a successful heartbeat is silent. Both the read and the exec are given a bounded context so a hung control plane cannot stall a heartbeat loop. The reported idle window is read from the live sandbox rather than from Crabbox config, because the provider echoes the sandbox's own lifecycle policy and that is the only idle number describing this lease; when the sandbox carries no such policy the result reports no idle timeout and warns, rather than printing an unrelated config default. A paused sandbox is refused rather than exec'd. An exec against a paused sandbox resumes it and the resume is billed: on a tenant with no credit the same call is rejected with HTTP 402 BILLING_NOT_ALLOWED, "Insufficient credit balance to resume a sandbox". Refusing means a heartbeat can never be the thing that starts billing compute. This path issues no create and no lifecycle write of any kind, so it cannot affect an absolute deletion deadline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 4:31 PM ET / 20:31 UTC. ClawSweeper reviewWhat this changesThe branch adds a provider-owned lease-heartbeat capability so delegated providers, initially Islo, can record lease activity without a Crabbox-managed SSH lease. Merge readiness⛔ Blocked until stronger real behavior proof is added - 3 items remain Keep this PR open: the implementation is coherent and current main still lacks the delegated heartbeat path, but the supplied live A/B output does not demonstrate the new CLI command succeeding against a claimed running Islo sandbox. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s heartbeat command refreshes lease activity through either a coordinator, direct SSH provider, or provider adapter. This change sends delegated-provider identifiers to an adapter-owned activity call and renders the adapter’s reported lease state. flowchart LR
A[CLI heartbeat request] --> B[Provider selection]
B --> C{Coordinator lease possible?}
C -->|Yes| D[Coordinator heartbeat]
C -->|No| E[Delegated provider heartbeat]
E --> F[Islo sandbox API]
D --> G[Heartbeat result]
F --> G
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the provider-neutral capability and add a redacted terminal transcript or runtime log that connects the real claimed Islo sandbox, CLI heartbeat invocation, successful result, and observed lifecycle effect. Do we have a high-confidence way to reproduce the issue? Yes. Current main’s command path only accepts an SSH lease backend, while the branch adds the Islo adapter path; the supplied live A/B describes the provider activity scenario but has not shown the actual CLI result. Is this the best way to solve the issue? Yes, subject to real-path proof. The optional provider-neutral capability follows the repository’s provider boundary and keeps Islo-specific lifecycle behavior in its adapter. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 200089463ff5. 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
History |
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:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
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
crabbox heartbeatonly worked for SSH-lease backends —internal/cli/heartbeat.goassertedbackend.(SSHLeaseBackend)and otherwise exited 2. A delegated provider had no way to keep its lease alive evenwhen its API can do so cheaply. For islo specifically,
Touchonly rewrote a local label, so a "heartbeat" neverreached the provider and an idle lease paused anyway.
Change
Adds a provider-neutral optional
LeaseHeartbeatBackendcapability plus aFeatureHeartbeatfeature so it isdiscoverable via
crabbox providers. The request carries only the lease ID — there is nothing generic to negotiate,and a smaller contract is easier for the next delegated provider to implement. Backends that do not implement it
fall through to the untouched SSH assertion with the same message and exit code.
--idle-timeoutis refused on this path rather than silently ignored: no lifecycle-update operation exists, soa delegated heartbeat cannot change an idle window.
islo is the first consumer. There is no dedicated heartbeat endpoint, so a heartbeat is one
trueexec — exits 0immediately, mutates nothing, safe to replay. Streams are discarded so success is silent, and both the read and the
exec get a bounded context. The reported idle window is read from the live sandbox, not from Crabbox config.
A paused sandbox is refused rather than exec'd: an exec against a paused sandbox resumes it, and the resume is
billed, so a heartbeat must never be the thing that starts billing compute.
Evidence
Tests were mutation-checked — 12 mutations, each confirmed to fail, including reverting the gate to the config-only
condition and swapping the discarded streams for real writers.
Live behaviour — a controlled A/B against a real sandbox with
pause_after_idle = 60s(created and deleted for thetest):
Control, same 60s policy with no execs, paused at t+90s. So the exec measurably defers the pause, and the pause
still arrives once heartbeats stop.
Separately, on a tenant with no credit the same exec-against-paused returns
402 BILLING_NOT_ALLOWED — "Insufficient credit balance to resume a sandbox", which is why a paused sandbox isrefused here.