Skip to content

fix: finish Azure fixed claims after external resource cleanup - #2572

Merged
roboclaw-bot merged 4 commits into
mainfrom
galiniliev/azure-orphan-claim-recovery
Sep 26, 2026
Merged

roboclaw-bot merged 4 commits into
mainfrom
galiniliev/azure-orphan-claim-recovery

Conversation

@galiniliev

@galiniliev galiniliev commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes Azure fixed-lease stop getting stuck after external cleanup removes the VM and companions while the retained local claim has no original cleanup binding.

User Impact

stop --force --provider azure --id <canonical-cbx-id> can finish that existing claim after verifying that the VM, NIC, public IP, managed OS disk, and quarantine NSG are all absent. Any remaining resource, including an owned or untagged disk, blocks recovery and leaves the claim available for retry. Inspect and clean up remaining resources in Azure first. No configuration, secrets, or claim migration changes are required.

Absence checks are point-in-time observations, not a lock on other Azure writers. Resources recreated after their checks may still exist when the local claim becomes terminal; terminal retries do not inspect Azure again. Inspect and clean up any recreated resources directly in Azure.

Why This Change Was Made

Recovery without a historical binding now performs only Azure GET requests and uses the existing fixed-claim lifecycle and terminal receipt. It verifies absence both during preparation and before local completion, rejects non-native or ambiguous not-found errors, and does not initiate from automatic cleanup.

This narrows the feature to absence-only claim recovery. The destructive orphan branch and orphan-v1 binding are removed. Azure's unconditional name-addressed companion DELETE cannot guard against a replacement after attestation; the new path therefore grants no Azure deletion authority. The existing v1 cleanup path is unchanged. Remaining-resource errors give external-cleanup guidance. Landing preparation adds a maintainer changelog entry crediting @galiniliev and documents the point-in-time recovery limit; the Go implementation and test sources remain unchanged from the live-tested revision.

Evidence

Validated revision 66d66b2f6ad68cc9755597dd135e179ae84ceab2 on macOS arm64 with Go 1.26.5. The live-tested binary SHA256 is 0a29dc51f8fba38177e7c5f50e24d6b4aafac0778e05612d778c24b5b325ba8d. Passed:

go test -race ./internal/cli ./internal/providers/azure -run 'TestAzureOrphanCleanup|TestAzureDeleteResources|TestFixedAzure|TestAzureCleanup' -count=1 -timeout=90s
go vet ./internal/cli ./internal/providers/azure
go build -trimpath -o /tmp/crabbox-pr2572-platinum/crabbox ./cmd/crabbox
git diff --check

Committed SDK-transport regressions cover all five resource kinds, owned/foreign/untagged remaining resources, replacement between preparation and completion, VM reappearance during verification, authorization and transport failures, ambiguous 404s, missing fixed identity, and automatic-cleanup exclusion. The transport fails on every non-GET request. The CLI lifecycle regression verifies interruption retains the existing claim without a new binding, a retry publishes the terminal receipt, and a terminal retry does not repeat provider work. Existing v1 interrupted-cleanup tests remain in place.

Real Azure CLI proof, September 26, 2026 UTC: built this production CLI and ran the actual stop --force command with an isolated, seeded existing-format fixed claim against a disposable resource group. A real quarantine NSG had a foreign lease tag; the VM and other companion names were absent. No VM was provisioned, so this is live recovery proof from representative retained state, not a live VM-create/delete lifecycle smoke. The released v0.66.0 macOS arm64 binary was downloaded and verified against its published SHA256 checksum.

Invocation / observation Result
Released v0.66.0, initial retained claim Exit 1: durable cleanup binding unavailable; claim remains acquired
Revised CLI, foreign NSG still present Exit 1: quarantine NSG still exists; claim remains acquired
Independent Azure NSG readback Immutable GUID, ETag, and tags unchanged
Roll back to v0.66.0 with retained claim Same original missing-binding refusal; existing claim format readable
Fixture owner externally removes NSG, revised CLI retries Exit 0; existing-format terminal receipt is released
Revised CLI retries terminal receipt Exit 0
Roll back to v0.66.0 with terminal receipt Exit 0
Fixture cleanup Disposable resource group deleted

Redacted command result:

Azure orphan recovery refused: quarantine NSG for <resource-name> still exists;
inspect and clean up remaining resources in Azure, then retry stop

new-absence-recovers: exit=0; claim=released; no cleanup binding dialect
new-terminal-retry: exit=0; claim=released; no cleanup binding dialect
rollback-terminal-retry: exit=0; claim=released; no cleanup binding dialect

Rollback before completion retains the old version's original recovery limitation; it does not strand a new persisted dialect. Neither the claim schema nor the old companion cleanup dialect changes. Azure reads do not create a global writer lock: resources created after an absence observation are never deleted by this recovery path. Simulated transport tests prove the no-mutation boundary; the live NSG readback proves rejection leaves the observed resource unchanged.

Landing preparation

Prepared at 49e8af640678c6032a3f70140f744218944cd17a on Linux amd64 with Go 1.26.5 and Node 24.21.0. This follow-up changes only the stop documentation and maintainer changelog; Go implementation and test sources are identical to the live-tested revision above. No additional live Azure run was performed. Passed:

scripts/check-docs.sh
go test -race -p 2 ./internal/cli -run Azure -count=1 -timeout=90s
go test -race -p 2 ./internal/providers/azure -count=1 -timeout=90s
go vet -p 2 ./internal/cli ./internal/providers/azure
git diff --check origin/main...HEAD

Validation used an isolated environment without inherited provider settings. Documentation checks covered all 62 commands and 253 Markdown files; all 20 generated-docs tests passed. The prepared head merges without conflicts with main at 4bd59bb9; branch history was not rewritten.

Review and landing preparation session.

@clawsweeper

clawsweeper Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 26, 2026
@clawsweeper

clawsweeper Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 26, 2026, 10:12 AM ET / 14:12 UTC (Revision 4).

ClawSweeper review

What this changes

The PR lets Azure force-stop finish an existing fixed-lease claim after external cleanup by checking that the VM and named companion resources are absent, and adds documentation and regression coverage.

Merge readiness

⛔ Blocked before merge - 3 items remain

Current main and v0.66.0 still refuse this recovery case, while this PR has distinct work supported by a live Azure run. The remaining blocker is an explicit owner decision on the documented point-in-time absence guarantee.

Priority: P2
Reviewed head: 49e8af640678c6032a3f70140f744218944cd17a
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) Focused read-only code, regression coverage, and relevant live Azure and rollback observations support a good patch, subject to the owner contract decision.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The captured PR body describes the production stop command on macOS arm64 against a disposable Azure resource group: a foreign security group blocked the retained claim, external removal allowed a terminal receipt, and v0.66.0 read the existing claim states on rollback. The Go behavior is unchanged in the later documentation-only head; no new stored dialect is written, and the reported rollback checks support existing-state compatibility.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured PR body describes the production stop command on macOS arm64 against a disposable Azure resource group: a foreign security group blocked the retained claim, external removal allowed a terminal receipt, and v0.66.0 read the existing claim states on rollback. The Go behavior is unchanged in the later documentation-only head; no new stored dialect is written, and the reported rollback checks support existing-state compatibility.
Evidence reviewed 9 items Current-main gap: Current main rejects an absent Azure VM when the retained claim has no durable cleanup binding.
Released behavior: The latest release, v0.66.0, contains the same missing-binding refusal.
Introduced recovery path: The pinned PR delta enables absence recovery for explicit owned-server preparation while leaving automatic cleanup on its prior path.
Findings None None.
Security None None.

How this fits together

Crabbox's Go CLI resolves an exact local lease claim before its Azure client checks cloud resources. The fixed-lease engine retains the claim on a failed check or writes a terminal receipt after successful verification.

flowchart LR
A[Force-stop request] --> B[Exact local claim]
B --> C[Azure resource reads]
C --> D{All resources absent?}
D -- Yes --> E[Terminal receipt]
D -- No --> F[Claim retained for retry]
Loading

Decision needed

Question Recommendation
Is point-in-time Azure absence verification sufficient to publish a terminal receipt for explicit force-stop when another Azure writer could recreate a resource immediately afterward? Accept point-in-time recovery: Approve this bounded explicit recovery with the documented instruction to inspect Azure for resources recreated after verification.

Why: Separate Azure reads and the local receipt cannot be atomic; accepting the resulting resource-tracking limit is an operator-contract choice.

Before merge

  • Resolve merge risk (P1) - Azure reads are sequential. A resource recreated after its GET can remain after the local claim becomes terminal, and terminal retries will not inspect it; the PR documents this limit, but explicit Azure fixed-lease owner acceptance is not visible.
  • Complete next step (P2) - Have the Azure fixed-lease owner explicitly accept the point-in-time absence guarantee and its post-check resource recreation consequence before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Go line delta production +63/−3; tests +296/−0 Production growth is confined to Azure absence verification and has focused transport and lifecycle coverage.

Merge-risk options

Maintainer options:

  1. Accept the bounded guarantee (recommended)
    The Azure fixed-lease owner can explicitly accept terminal completion after the final point-in-time reads and the documented operator cleanup responsibility.
  2. Pause for a stronger contract
    Keep the PR open if terminal completion must remain reliable against concurrent Azure resource recreation.

Technical review

Best possible solution:

Ship the narrow read-only force-stop recovery with its existing claim format and operator guidance once the Azure fixed-lease owner accepts the point-in-time terminal contract.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main and v0.66.0 refuse a missing cleanup binding, and the PR reports that refusal in a released CLI run with a retained claim. This review did not run Azure.

Is this the best way to solve the issue?

Yes for the proposed recovery case: read-only checks avoid deleting a replacement resource, and the reported rollback checks support the existing claim format. The terminal guarantee still needs owner acceptance.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4bd59bb96b6a.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This is a bounded Azure force-stop recovery improvement with provider-specific impact.
  • merge-risk: 🚨 other: A resource recreated after its absence read could outlive the newly terminal local claim.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured PR body describes the production stop command on macOS arm64 against a disposable Azure resource group: a foreign security group blocked the retained claim, external removal allowed a terminal receipt, and v0.66.0 read the existing claim states on rollback. The Go behavior is unchanged in the later documentation-only head; no new stored dialect is written, and the reported rollback checks support existing-state compatibility.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured PR body describes the production stop command on macOS arm64 against a disposable Azure resource group: a foreign security group blocked the retained claim, external removal allowed a terminal receipt, and v0.66.0 read the existing claim states on rollback. The Go behavior is unchanged in the later documentation-only head; no new stored dialect is written, and the reported rollback checks support existing-state compatibility.

Evidence

What I checked:

  • Current-main gap: Current main rejects an absent Azure VM when the retained claim has no durable cleanup binding. (internal/cli/azure.go:1899, 4bd59bb96b6a)
  • Released behavior: The latest release, v0.66.0, contains the same missing-binding refusal. (internal/cli/azure.go:1899, 145cd6eca3c6)
  • Introduced recovery path: The pinned PR delta enables absence recovery for explicit owned-server preparation while leaving automatic cleanup on its prior path. (internal/cli/azure.go:1899, 49e8af640678)
  • Final-effect boundary: The new recovery checks the named companions and VM through GET requests, rejects present resources and ambiguous errors, and performs no Azure deletion. The existing engine writes the terminal claim only after the provider operation succeeds. (internal/cli/azure_orphan_cleanup.go:16, 49e8af640678)
  • Point-in-time limit: The new documentation says a resource recreated after its absence check may remain after the claim becomes terminal; terminal retries do not inspect Azure again. (docs/commands/stop.md:328, 49e8af640678)
  • Real behavior and upgrade proof: The captured PR body reports a production macOS arm64 CLI run against a disposable Azure resource group: a foreign security group blocked completion without mutation, its external removal allowed a terminal receipt, and the released binary read both retained and terminal claim states. The later head changes only documentation and changelog files. (49e8af640678)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • galiniliev: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Obtain Azure fixed-lease owner acceptance of the point-in-time absence contract.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-09-26T02:31:32.993Z sha 0d03421 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-26T02:43:36.579Z sha 0d03421 :: needs real behavior proof before merge. :: [P1] Prevent deletion of a replacement orphan companion | [P3] Leave the changelog entry to maintainers
  • reviewed 2026-09-26T03:01:12.863Z sha 66d66b2 :: blocked before merge. :: none

@galiniliev
galiniliev requested review from steipete and removed request for steipete September 26, 2026 02:41
@galiniliev galiniliev changed the title fix: recover Azure orphan companions after VM deletion fix: finish Azure fixed claims after external resource cleanup Sep 26, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 26, 2026
Prepare #2572 for landing with explicit point-in-time absence semantics and a maintainer changelog entry.

Co-authored-by: galiniliev <5711535+galiniliev@users.noreply.github.com>
@roboclaw-bot
roboclaw-bot merged commit c4a8488 into main Sep 26, 2026
33 checks passed
@roboclaw-bot
roboclaw-bot deleted the galiniliev/azure-orphan-claim-recovery branch September 26, 2026 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants