Repository navigation
feat: guard direct lease expiry and gate remote allocations - #2806
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5387877dba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false | ||
| } | ||
|
|
||
| func validateDirectLeaseCreation(cfg Config) error { |
There was a problem hiding this comment.
watch
When directLeases: deny is configured and crabbox watch is invoked without --id, watchWithBackend still calls backend.Acquire without ever invoking this validator. Subsequent iterations use the acquired ID and therefore also bypass the new-run check in run, allowing the command to create exactly the remote direct lease this machine policy is intended to prohibit; apply the gate to the lease-creation branch of watch while preserving existing-lease reuse.
Useful? React with 👍 / 👎.
| "ssh_keys": []any{key.ID}, | ||
| "tags": tags, | ||
| "user_data": core.CloudInitUserData(cfg, publicKey), | ||
| "user_data": core.CloudInitUserData(core.WithGuestLeaseExpiry(cfg, labelsFromTags(tags)), publicKey), |
There was a problem hiding this comment.
This user-data snapshot is built from DigitalOcean's provisioning tags, while acquisition later changes only the provider tags and returned server to ready. A successful standalone warmup does not call touchLeaseWithGuestExpiry, so if no later command reaches the lease, its guest file remains in provisioning and the guard grants the one-hour provisioning grace after expires_at instead of reclaiming at the ready lease's deadline. Refresh the file-backed snapshot after successful acquisition/ready publication; the same issue affects the other file-backed integrations initialized from provisioning labels.
Useful? React with 👍 / 👎.
5387877 to
7b9637a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b9637afc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch cfg.Provider { | ||
| case "gcp", "aws", "azure": | ||
| return cfg.Provider | ||
| case "hetzner", "digitalocean", "vultr", "linode", "ovh", "scaleway", "nebius", "lambda", "tencentcloud", "incus", "multipass", "firecracker": | ||
| return "file" |
There was a problem hiding this comment.
This switch makes provider coverage depend on a hard-coded list in core, while guestExpiryScript also selects provider-specific metadata and reclaim behavior there. Adding or changing a provider now requires editing internal/cli, and an otherwise compatible shared-cloud-init provider silently receives no guard. Expose this through a provider capability so each adapter owns its metadata source and reclaim semantics.
AGENTS.md reference: AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 949e71cfed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := RefreshGuestLeaseExpiry(ctx, target, GuestLeaseExpiryFromLabels(server.Labels)); err != nil { | ||
| return server, fmt.Errorf("refresh guest lease expiry: %w", err) |
There was a problem hiding this comment.
When a file-backed provider touch succeeds but the subsequent SSH refresh fails—for example during a transient SSH or sudo failure—the provider deadline has already advanced while /etc/crabbox/lease-expiry retains its old deadline. Callers such as run and interactive connect downgrade this returned error to a warning and continue, so the guest guard can power off an active lease at the pre-touch deadline even though provider metadata records the renewal. Retry or otherwise reconcile the guest snapshot before allowing the touch to remain partially successful.
Useful? React with 👍 / 👎.
| if labels == nil || labels["crabbox"] != "true" { | ||
| return false, "missing labels" | ||
| } | ||
| if strings.EqualFold(labels["keep"], "true") { | ||
| return false, "keep=true" | ||
| } | ||
| state := strings.ToLower(labels["state"]) |
There was a problem hiding this comment.
Removing the shared keep=true exemption does not apply the advertised expiry behavior to provider-specific cleanup paths: multipass.shouldCleanup and firecracker.shouldCleanupRecord still return immediately for every kept lease before evaluating expiry. Consequently, expired kept Multipass instances and Firecracker VMs remain indefinitely during crabbox cleanup, despite the new policy and documentation saying that keep only suppresses post-run release.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b94a8835f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| labels := DirectLeaseLabels(cfg, leaseID, slug, "hetzner", "", keep, now) | ||
| cfg = WithGuestLeaseExpiry(cfg, labels) |
There was a problem hiding this comment.
When a Hetzner bootstrap takes longer than a short configured idle timeout, this snapshot records state=leased, so the newly installed timer applies the deadline immediately instead of the one-hour provisioning grace and can power off the VM while Acquire is still waiting for SSH. Build the guest snapshot with state=provisioning, as the other provisioning-aware integrations do, and publish the ready state after acquisition. This is distinct from the existing ready-publication comment because Hetzner can be reclaimed before acquisition completes.
Useful? React with 👍 / 👎.
| func directLeaseExempt(provider string) bool { | ||
| switch provider { | ||
| case "ssh", "local-container", "docker-sandbox", "multipass", "firecracker", "lume", "tart", "hyperv", "windows-sandbox", "apple-container", "apple-machine", "apple-vm", "anthropic-sandbox-runtime", "mxc": |
There was a problem hiding this comment.
This provider-name switch makes core own provider-specific allocation semantics: every new local-only backend is rejected under directLeases: deny until this unrelated core list is updated, while adapters cannot declare the exemption themselves. Expose an allocation/locality capability on the provider contract and let each adapter own it, keeping the policy evaluator provider-neutral.
AGENTS.md reference: AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
Keep AWS raw-gzip and Hetzner MIME transport intact through a shared encoder, bind the Hetzner round-trip fixture to allocation labels, and bound Tencent Cloud compressed user data. Compact the shared guard and use the exported expiry refresh hook directly. Normalize Sealos native ownership and recheck live expiry before deletion. Update ownership fixtures and test renewed kept leases, expired kept leases, and non-mutating cleanup dry runs. Verified targeted failing tests and affected guard/transport/size regressions with GOTOOLCHAIN=go1.26.5 across seven packages. Documentation links pass. Codex autoreview against the origin/main merge base is scoped-clean through P2.
Wait for the client channel-close acknowledgement before closing TCP, so native Windows OpenSSH receives the fixture exit status instead of intermittently returning 255 after a successful refresh. The targeted DigitalOcean heartbeat/status test passes with GOTOOLCHAIN=go1.26.5. Codex autoreview is scoped-clean through P2.
77976d9 to
0e3ab66
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e3ab66715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Quantity: 1, | ||
| SSHKeyNames: []string{key.Name}, | ||
| UserData: core.CloudInitUserData(cfg, publicKey), | ||
| UserData: core.CloudInitUserData(core.WithGuestLeaseExpiry(cfg, core.DirectLeaseLabels(cfg, leaseID, slug, providerName, "", keep, now)), publicKey), |
There was a problem hiding this comment.
When a Lambda lease has an idle timeout shorter than its bootstrap time (especially two minutes or less), DirectLeaseLabels writes state=leased into the new guest snapshot. The timer starts two minutes after boot, and leased receives no provisioning grace, so it can power off the instance while Acquire is still waiting for SSH. Build the initial snapshot with state=provisioning, as the other provisioning-aware provider paths do.
Useful? React with 👍 / 👎.
1 participant
Problem
Direct leases could outlive the CLI indefinitely. Only GCP had a guest expiry guard, and the manual sweep skipped kept machines forever while allowing running/provisioning machines another 12 hours. There was also no machine-level policy to prevent new direct remote allocations.
Change
Install a shared root-owned Linux expiry script and systemd service/timer on the shared cloud-init path. It runs every two minutes, requires
crabbox=true, treats metadata as data, and fails closed on missing ownership. No credentials are written into guard files. Brokered leases and Windows/macOS targets do not receive the guard.compute/tagsList/etc/crabbox/lease-expiry/etc/crabbox/lease-expiry/etc/crabbox/lease-expiryThe file-backed bootstrap records
crabbox,state,keep, andexpires_at.RefreshGuestLeaseExpiryis the hook for file-backed lifetime updates; ordinary CLI touches use it too. Updates validate the complete SSH payload before atomically renaming a mode-0600 file. Readers evaluate one complete snapshot. This change does not add anextendcommand.The CLI sweep and shell guard share tested decision cases: kept leases expire, running/provisioning grace is one hour, and unowned/unlabeled machines remain untouched.
failed,released, andexpiredremain immediately reclaimable.keeponly suppresses automatic release afterrun.Add
directLeases: allow|deny(default allow),CRABBOX_DIRECT_LEASES,directLeaseAllow, and comma-separatedCRABBOX_DIRECT_LEASE_ALLOW. Denial is checked before direct acquisition for new runs, warmup/prewarm, fixed IDs, checkpoint forks, and shards. Existing lease operations remain available. A repository can set deny, but cannot loosen an inherited deny or grant allowlist exceptions; those belong to trusted user/system config or environment. Admission policy stays out of the existing controller identity contract and fixed-resource fingerprints.The local/static exemptions are
ssh,local-container,docker-sandbox,apple-container,apple-machine,apple-vm,anthropic-sandbox-runtime,mxc,multipass,firecracker,lume,tart,hyperv, andwindows-sandbox. Providers that can target remote hosts, including Incus and Parallels, are not automatically exempt.Limits
Poweroff stops the workload; it does not necessarily stop billing. Hetzner still needs
crabbox cleanupto delete the server, and Azure guest shutdown does not deallocate the VM. Retained disks or other resources may remain billable. GCP keeps its existing self-delete permission requirement.Custom/template bootstraps such as Proxmox, XCP-ng, Parallels, and Apple VM, plus hosted/delegated/static providers without the shared bootstrap, remain uncovered. The lifecycle/provider docs enumerate the complete covered and uncovered sets. No worker code changed, and no live provider calls were used for proof.
Complete excluded provider list (no shared Linux bootstrap)
agent-sandbox,anthropic-sandbox-runtime,apple-container,apple-machine,apple-vm,ascii-box,aws-lambda-microvm,azure-dynamic-sessions.azure-sandbox,blacksmith-testbox,blaxel,boxd,cloud-run-sandbox,cloudflare,cloudflare-dynamic-workers,cloudflare-sandbox.coder,codesandbox,crownest,cua,cubesandbox,daytona,docker-sandbox,e2b.exe-dev,external,fastapi-cloud,freestyle,github-codespaces,hostinger,hyperv,islo.kubevirt,local-container,lume,machine0,modal,morph,mxc,namespace-devbox.namespace-instance,nomad,nvidia-brev,opencomputer,opensandbox,orgo,parallels,phala.proxmox,railway,runpod,sealos-devbox,semaphore,serve,smolvm,sprites.ssh,superserve,tart,tenki,tensorlake,unikraft-cloud,upstash-box,vast.vercel-sandbox,wandb,windows-sandbox,xcp-ng.Verification
All Go commands use
GOTOOLCHAIN=go1.26.5.gofmt -l internal cmd: empty.go vet ./...: passed.go build -trimpath -o bin/crabbox ./cmd/crabbox: passed.-count=1 -timeout=10m.go test -count=1 -timeout=15minvocation covering AWS, Azure, GCP, Hetzner, DigitalOcean, Vultr, Linode, OVH, Scaleway, Nebius, Lambda, Tencent Cloud, Incus, Multipass, and Firecracker. The new refresh exposed missing SSH fixtures in DigitalOcean and Multipass heartbeat tests. Those fixtures now verify the guest payload; both full packages passed on the affected-package rerun (77.465s and 10.633s). All other 13 packages passed initially.node scripts/check-command-docs.mjs: 64 command docs passed.node scripts/check-docs-links.mjs: 261 Markdown files passed.TestGuestExpiryGuardProviderswhen ShellCheck is installed.origin/mainmerge base (80498fbc8112f85934002f1e43b93b465324c768):scoped-clean, no accepted/actionable P0–P2 findings. Review covered ownership, shell/metadata parsing, expiry semantics, config trust, broker exclusion, AWS launch options, and atomic refresh; provider integration is also covered by the tests above.The earlier over-broad CLI selection ran for 1175.272 seconds under heavy host load and failed unchanged
TestSSHTransportCapturedConfigPreventsAliasReassignmenton its shared 15-second context (captured command after reassignment: context deadline exceeded). The maintainer ruled this environmental; it was not changed or rerun as a broad selection. PR CI is the broad proof. Earlier scratch-filesystem experiments preserved all assertions; no scratch volumes remain. No live provider calls were made.Exact focused command and proof tails