feat(proxmox): support replay-safe fixed lease IDs - #1861
Conversation
Implements replay-safe fixed Proxmox lease acquisition with durable VMID binding, exact identity validation, conflict detection, and terminal tombstones.\n\nCloses openclaw#1847
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
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: f7a1f42148
ℹ️ 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 &leaseBackend{DirectSSHBackend: shared.DirectSSHBackend{SpecValue: spec, Cfg: cfg, RT: rt, StoredLeaseKeys: true}} | ||
| } | ||
|
|
||
| func (b *leaseBackend) SupportsRequestedLeaseID() bool { return true } |
There was a problem hiding this comment.
This advertises a new user-visible warmup --lease-id capability for Proxmox, but the commit leaves CHANGELOG.md's Unreleased section empty. Record the feature there so it is included in the next release notes, as required for agent-authored user-visible features.
AGENTS.md reference: AGENTS.md:L41-L41
Useful? React with 👍 / 👎.
| User: strings.TrimSpace(cfg.SSHUser), WorkRoot: strings.TrimSpace(cfg.WorkRoot), | ||
| FullClone: cfg.Proxmox.FullClone, ServerType: strings.TrimSpace(cfg.ServerType), | ||
| TargetOS: strings.TrimSpace(cfg.TargetOS), RequestedSlug: core.NormalizeLeaseSlug(req.RequestedSlug), | ||
| Keep: req.Keep, TTLNanoseconds: cfg.TTL.Nanoseconds(), IdleNanos: cfg.IdleTimeout.Nanoseconds(), | ||
| SSHPublicKey: strings.TrimSpace(publicKey), |
There was a problem hiding this comment.
The fingerprint includes the SSH user and key but omits cfg.SSHPort and cfg.SSHFallbackPorts, even though acquireFixed later builds its readiness/returned target from those current values via sshTargetFromConfig. Replaying one fixed lease ID with a changed SSH port therefore passes intent validation, attempts the existing VM over a different route, and can rewrite the claim endpoint instead of returning the promised lease_id_conflict; hash the effective SSH routing configuration as part of the immutable intent.
Useful? React with 👍 / 👎.
| if claim.CloudImmutableID == "" { | ||
| claim.CloudNumericID = server.ID | ||
| claim.CloudImmutableID = server.ImmutableID | ||
| claim.Labels = maps.Clone(server.Labels) |
There was a problem hiding this comment.
When a clone attempt has been persisted but CreateServerWithVMID never returned successfully, CloudImmutableID remains empty and this branch binds the vmgenid of whichever matching-labeled VM is first observed. If the original VM was deleted or restored/replaced at the same VMID with its description labels preserved, replay accepts the replacement generation instead of reporting the documented identity conflict. Only bind the generation from a create proven to have completed in the current invocation, or durably attest it before permitting ambiguous replay adoption.
Useful? React with 👍 / 👎.
| const fixedProxmoxCreateIntentVersion = 1 | ||
|
|
||
| var fixedProxmoxLeaseKind = core.FixedLeaseKind{ | ||
| ClaimProvider: core.FixedProxmoxClaimProvider, |
There was a problem hiding this comment.
Using proxmox-fixed-v1 as the claim provider makes fixed claims invisible to the existing resolveNumericClaim, which filters with claim.Provider != "proxmox" rather than canonicalizing the marker. Consequently, when a fixed attempt's VM is already absent, crabbox stop --provider proxmox --id <vmid> reports not found and cannot turn the prepared claim into its terminal tombstone, even though numeric VMIDs are otherwise accepted by this backend; include the fixed marker in that lookup.
Useful? React with 👍 / 👎.
| if len(claim.Labels) == 0 { | ||
| claim.Labels = fixedProxmoxIdentityLabels(b.Cfg, claim.LeaseID, claim.Slug, claim.FixedCreateIntent.Fingerprint, node) | ||
| } | ||
| *claim = fixedProxmoxLeaseKind.TerminalClaim(*claim, time.Now().UTC()) |
There was a problem hiding this comment.
A failed clone submission can mean Proxmox accepted the asynchronous request but the client lost the response before receiving its UPID, so the VM may not yet appear in inventory. releaseFixed treats a single absent VMExistsInCluster observation as final and writes a released tombstone; the still-running clone task can then create the VM afterward, leaving an unlabeled leak that future replay and release refuse to reconcile. A prepared, unbound attempt needs task-completion evidence or another durable cancellation/reconciliation barrier before absence can be terminalized.
Useful? React with 👍 / 👎.
| if req.DryRun { | ||
| fmt.Fprintf(b.RT.Stderr, "would delete server id=%s name=%s\n", server.DisplayID(), server.Name) | ||
| continue |
There was a problem hiding this comment.
For a fixed claim, dry-run returns here before releaseFixed performs provider-scope, VMID, label, and vmgenid validation. A VM that merely copies the fixed lease label—or a replaced VM with a mismatched generation—will therefore be reported as would delete, while the real cleanup refuses it; this makes the dry-run output unreliable as a safety preview. Run the same non-mutating fixed-identity checks before printing the deletion.
Useful? React with 👍 / 👎.
Downstream live qualificationA dist-patched Linux amd64 binary built from commit Results:
The test used only generic Proxmox/Crabbox behavior. No private configuration or credentials are included in this PR. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 4, 2026, 8:50 PM ET / September 5, 2026, 00:50 UTC. ClawSweeper reviewWhat this changesAdds caller-supplied Proxmox lease IDs with persisted VM allocation intent, replay reconciliation, terminal release records, tests, and documentation. Merge readiness⛔ Blocked before merge - 15 items remain This remains useful work: neither the inspected main revision nor v0.49.1 supports fixed Proxmox lease IDs. The reported live qualification demonstrates normal replay, but source inspection confirms blocking generation-binding and release-reconciliation defects. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s direct Proxmox backend turns CLI lease requests into template clones and SSH execution targets. Local claim records bind those requests to provider resources and govern subsequent replay, release, and cleanup. flowchart TD
A[CLI lease request] --> B[Proxmox provider adapter]
B --> C[Durable local claim]
C --> D{New allocation or replay}
D --> E[Clone selected VMID]
D --> F[Check VM identity]
E --> F
F --> G[SSH lease]
G --> H[Checked release and terminal claim]
Before merge
Findings
Agent review detailsSecurityNeeds attention: An unbound attempt can acquire destructive authority from a replacement VM rather than proving the original generation. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep Proxmox-specific reconciliation in the existing fixed-lease framework, with attested generation binding, recoverable submission states, consistent ownership checks, and verified claim-format compatibility. Do we have a high-confidence way to reproduce the issue? Yes, the proposed-head defects have clear source-level triggers: replay or release an unbound prepared claim against a replacement generation, or retry after allocator failure. These paths were inspected, not executed; current main does not yet expose the capability. Is this the best way to solve the issue? No, not as submitted: reusing the provider-neutral framework is appropriate, but inventory must not create missing ownership authority and momentary absence must not erase unresolved submission state. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 530d9b586aa3. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Release-only resolution of an exact fixed Proxmox lease ID failed with exit 4 "lease/server not found" whenever Crabbox held no claim, leaving callers stuck retrying destruction of a VM that no longer exists. A completed cluster-wide inventory that shows no VM carrying the lease identity now converges as confirmed absence, so release is idempotent. Unreadable inventory, reused or conflicting VM identity, an unresolved cluster scope, and non-exact identifiers still fail closed, and no tombstone is fabricated for an ID that has no durable create intent. Refs openclaw#1847 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up commit |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: feat(proxmox): support replay-safe fixed lease IDs This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25340a9178
ℹ️ 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 req.OnAcquired != nil { | ||
| if err := req.OnAcquired(acquired); err != nil { | ||
| return LeaseTarget{}, fmt.Errorf("acknowledge fixed Proxmox acquisition: %w", err) |
There was a problem hiding this comment.
When controller identity acknowledgment is configured, this callback runs only after AcquireFixedLease has completed CreateServerWithVMID, SSH bootstrap/readiness, label updates, and acquisition-claim persistence. If the process is interrupted during those potentially long readiness steps, or if the callback itself fails, the controller never learns the created VM identity and this path neither rolls it back nor returns it to the caller, leaving the controller unable to reconcile the resource. Invoke OnAcquired once the raw VM identity is validated and before readiness side effects, as required by the AcquireRequest.OnAcquired contract, or explicitly roll back on acknowledgment failure.
Useful? React with 👍 / 👎.
| return &leaseBackend{DirectSSHBackend: shared.DirectSSHBackend{SpecValue: spec, Cfg: cfg, RT: rt, StoredLeaseKeys: true}} | ||
| } | ||
|
|
||
| func (b *leaseBackend) SupportsRequestedLeaseID() bool { return true } |
There was a problem hiding this comment.
Enabling fixed IDs also makes their claims available to normal lifecycle commands, but Proxmox Resolve still returns inventory matches without checking the fixed claim's fingerprint or CloudImmutableID. If a VM is restored or replaced at the same VMID while retaining the lease labels, status --wait and heartbeat accept it because the generic claim check compares only provider scope and CloudID, and then Touch writes lifecycle labels to the wrong generation instead of returning lease_id_conflict. Route fixed-claim resolution through fixedProxmoxAttempt and validateFixedProxmoxServer, as the acquire and release paths already do.
Useful? React with 👍 / 👎.
| } | ||
| fmt.Fprintf(b.RT.Stderr, "provisioning provider=proxmox lease=%s slug=%s node=%s template=%d vmid=%d keep=%v fixed=true\n", | ||
| leaseID, intent.Slug, cfg.Proxmox.Node, cfg.Proxmox.TemplateID, attemptVMID, req.Keep) | ||
| server, err = client.CreateServerWithVMID(ctx, cfg, publicKey, leaseID, intent.Slug, req.Keep, attemptVMID, labels) |
There was a problem hiding this comment.
This fixed-ID path reuses CreateServerWithVMID, whose post-clone error paths call cleanupClone() from a background context, including when waitTask, IP discovery, or SSH bootstrap returns because the caller canceled the command. The durable fixed claim still retains its submitted VMID, so after that deletion an identical replay finds no VM and permanently returns the unresolved-attempt conflict instead of recovering the lease; this also contradicts the documented fixed-ID guarantee that caller cancellation never releases the resource. Give fixed creation a no-cleanup mode and leave the VM for identity-checked replay or explicit release.
Useful? React with 👍 / 👎.
1 participant
Summary
--lease-idsupport to the direct Proxmox providervmgenidon replaylease_id_conflictCloses #1847
Verification
go test ./internal/providers/proxmox -run "Fixed|RequestedLease|Proxmox" -count=1go test ./internal/cli -run "Proxmox|FixedProxmox" -count=1gofmton changed Go filesgit diff --checkNotes
No configuration or secret changes. The implementation uses the existing provider-neutral fixed-acquire framework and keeps Proxmox-specific reconciliation behind the provider adapter.