fix(tencentcloud): preserve acquisition causes and bound readiness

[steipete]

Summary

Consolidate two Tencent acquisition policies behind existing shared helpers, keeping the changes in separate commits:

• JoinAcquireCleanupError preserves primary and termination error causes and primary typed exit-code precedence. Reported cleanup failure explicitly vetoes a fresh-instance retry; successful rollback retains the existing bootstrap retry.
• PollReadiness owns the five-minute elapsed IP-readiness budget and three-second wait. Cooperative reads now receive the bounded observation context, waits cannot overshoot the budget, and caller cancellation retains its custom cause without changing the canonical public cancellation diagnostic.

Tencent still owns public-IPv4-only readiness (regardless of state), immediate API-error handling, completed-response precedence, and the exact timeout message/exit code 5. A completed ready response or typed API error can win at coincident cancellation; this is not a promise to reject every response from a client that ignores context. Lifecycle timestamp clocks remain separate from the readiness budget.

Termination order, its independent 45-second cleanup context, tags, claims, and local-key removal are unchanged. This does not qualify recovery-state retention. Docs and the maintainer-added Unreleased entry describe both fixes.

Verification

• Cleanup baseline: eight regression cases fail across both keep modes, exposing lost causes/exit codes. A fault-injected bootstrap-shaped cleanup error also causes two allocations; this tests the policy boundary, not an observed Tencent API reply.
• Readiness baseline: three cancellation cases lose their cause or perform an unnecessary read; a cooperative read reaches six minutes, and the no-IP retry loop stops at five minutes three seconds. Existing response/readiness controls pass.
• Full combined Tencent/shared race suites and vet pass. testing/synctest exercises the actual five-minute/three-second constants without production timing knobs. The successful-rollback control still retries; the new combined cancellation-plus-cleanup test preserves the private cause without exposing its text or allocating twice.
• Complete independent Codex review of the integrated candidate found no actionable issues through P2. A separate read-only integration audit found no dropped tests or merge artifacts.
• The exact integrated CLI binary was rebuilt and rerun against the loopback Tencent API and inert SSH host-key rejection. This exercises the real transport and acquisition/rollback path, with both diagnostics and a single create/terminate sequence. Temporary fixture state and listeners were removed. This is controlled integration evidence, not a native cloud lifecycle pass or a wall-clock five-minute timing measurement.

Field-selected before/after CLI results:

[
  {
    "phase": "before",
    "source": "e6c63a5970e43402fea33768d465da6f52fddb64",
    "sha256": "8a7be9fa074e63771d3b2b446df4af4bcb432fd4c84d096ef1394740f25db9e2",
    "exit": 1,
    "actions": [
      "DescribeInstances",
      "RunInstances",
      "DescribeInstances",
      "TerminateInstances"
    ],
    "primaryDiagnostic": true,
    "cleanupDiagnostic": true,
    "retryVetoWarning": false
  },
  {
    "phase": "after",
    "source": "08aef04d28c2d7f2bacd6878b23469ddac59b0b5",
    "sha256": "e5dda9de653928f7b7d29275b6a4a64c6791279f52ed2c91947dc66bd6352d16",
    "exit": 1,
    "actions": [
      "DescribeInstances",
      "RunInstances",
      "DescribeInstances",
      "TerminateInstances"
    ],
    "primaryDiagnostic": true,
    "cleanupDiagnostic": true,
    "retryVetoWarning": true
  }
]

Qualification status

This remains a draft pending native Tencent lifecycle proof. Known approved credential locations were unavailable in the previous check; no credentials or cloud account permissions were changed. The prior proof remains attributed to its original source; the results above are from the integrated head. No workflow, dependencies, release machinery, or provider mutation authority changes.

[clawsweeper]

🦞👀
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]

Codex review: blocked before merge. Reviewed September 25, 2026, 1:12 PM ET / 17:12 UTC (Revision 4).

ClawSweeper review

What this changes

The Tencent Cloud provider preserves acquisition and rollback error causes, stops a fresh-instance retry after reported termination failure, and bounds public-IP readiness, with tests and operator documentation.

Merge readiness

⛔ Blocked before merge - 4 items remain

Keep open: current main and v0.66.0 still have the older Tencent Cloud behavior, and this PR provides a distinct fix. The patch has no identified correctness finding; merge readiness depends on an explicit decision about the retry policy and the pending native qualification.

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

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	The focused patch has source-bound production-path proof and strong tests, with a conditional compatibility choice and native qualification still awaiting owner resolution.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The PR's before/after CLI trace exercises the production Tencent acquisition and rollback path through a loopback API and inert SSH rejection; the after run shows the new retry-veto diagnostic, and the proof commit's production blobs match this head. Fake-client and synctest coverage supplements readiness behavior; native Tencent lifecycle and wall-clock qualification remain unresolved. No stored-data contract changes.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR's before/after CLI trace exercises the production Tencent acquisition and rollback path through a loopback API and inert SSH rejection; the after run shows the new retry-veto diagnostic, and the proof commit's production blobs match this head. Fake-client and synctest coverage supplements readiness behavior; native Tencent lifecycle and wall-clock qualification remain unresolved. No stored-data contract changes.
Evidence reviewed	9 items	Introduced Tencent behavior: The PR calls the shared cleanup-error helper after failed termination and the shared readiness poll for the five-minute public-IP wait. Current-main gap: The pinned main revision still formats the primary error as text after cleanup failure and uses the older readiness poll, so the Tencent fix is absent. Release check: The latest identified release, v0.66.0, retains the older Tencent cleanup and readiness code.
Findings	None	None.
Security	None	None.

How this fits together

Crabbox's Tencent Cloud provider turns a CLI lease request into a cloud instance, waits for its public IP and SSH access, then returns a claimed lease. If acquisition fails, it attempts termination and decides whether to retry.

flowchart LR
A[CLI lease request] --> B[Tencent Cloud provider]
B --> C[Create cloud instance]
C --> D[Poll public IP]
D --> E[Check SSH and claim]
E -->|Ready| F[Usable lease]
E -->|Failure| G[Terminate and assess retry]
G -->|Cleanup succeeded| B
G -->|Cleanup failed| H[Error for operator]

Loading

Decision needed

Question	Recommendation
Should Tencent Cloud acquisition stop fresh-instance retries whenever termination reports failure, and should this change require a native Tencent lifecycle run before merge?	Qualify and accept the veto: Run a guarded Tencent lifecycle check, then approve the stop-on-failed-termination policy and its operator recovery guidance.

Why: The retry veto deliberately changes a conditional recovery path, while the current proof exercises a loopback transport rather than a Tencent account; accepting that upgrade behavior and qualification threshold requires provider-owner intent.

Before merge

• Resolve merge risk (P1) - For a bootstrap-shaped typed termination error, this PR stops a fresh-instance retry that the prior Tencent wrapper could allow. An operator may need to inspect the instance whose termination was not confirmed; the compatibility policy needs explicit owner acceptance.
• Resolve merge risk (P1) - The PR still identifies native Tencent lifecycle qualification as pending. A guarded run or explicit waiver is needed before treating the loopback evidence as sufficient qualification for the cloud operation.
• Complete next step (P2) - Have the Tencent provider owner explicitly accept the retry and exit-code upgrade behavior, then complete a guarded native lifecycle run or record an explicit waiver.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	production +18/−24 lines; tests +201/−2 lines	The provider change uses existing shared helpers and adds focused regression coverage without growing production code.

Merge-risk options

Maintainer options:

1. Qualify and accept fail-closed acquisition (recommended)
   Run a guarded Tencent lifecycle check, then explicitly approve stopping retries after reported termination failure and document how operators inspect an unconfirmed instance.
2. Waive the native run
   Explicitly accept the retry-policy change and the remaining native-qualification gap using the loopback and regression evidence.

Technical review

Best possible solution:

Ship the bounded wait and cause-preserving rollback with clear operator guidance for a termination whose outcome is unconfirmed.

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

Yes. Current-main source exposes the lost primary cause and unbounded readiness read, and the captured baseline tests and source-matched loopback CLI trace give a focused reproduction path; I did not execute the path in this read-only review.

Is this the best way to solve the issue?

Yes for reusing the existing shared helpers to preserve causes and bound polling. The conditional no-retry policy still requires explicit owner acceptance.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5fa10bdaee68.

Labels

Label changes:

No label changes.

Label justifications:

• P2: This is a bounded Tencent Cloud acquisition and readiness repair with limited provider-specific impact.
• merge-risk: 🚨 compatibility: The retry veto and preserved typed exit causes can change how an existing Tencent acquisition failure ends.
• 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 PR's before/after CLI trace exercises the production Tencent acquisition and rollback path through a loopback API and inert SSH rejection; the after run shows the new retry-veto diagnostic, and the proof commit's production blobs match this head. Fake-client and synctest coverage supplements readiness behavior; native Tencent lifecycle and wall-clock qualification remain unresolved. No stored-data contract changes.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR's before/after CLI trace exercises the production Tencent acquisition and rollback path through a loopback API and inert SSH rejection; the after run shows the new retry-veto diagnostic, and the proof commit's production blobs match this head. Fake-client and synctest coverage supplements readiness behavior; native Tencent lifecycle and wall-clock qualification remain unresolved. No stored-data contract changes.

Evidence

What I checked:

• Introduced Tencent behavior: The PR calls the shared cleanup-error helper after failed termination and the shared readiness poll for the five-minute public-IP wait. (internal/providers/tencentcloud/backend.go:122, a8211c4252ce)
• Current-main gap: The pinned main revision still formats the primary error as text after cleanup failure and uses the older readiness poll, so the Tencent fix is absent. (internal/providers/tencentcloud/backend.go:121, 5fa10bdaee68)
• Release check: The latest identified release, v0.66.0, retains the older Tencent cleanup and readiness code. (internal/providers/tencentcloud/backend.go:121, 145cd6eca3c6)
• Shared policy and upgrade effect: The existing shared helper vetoes retry for a reported cleanup failure; the old Tencent wrapper could instead classify a bootstrap-shaped typed cleanup error as retryable. The PR's injected control demonstrates that conditional change. (internal/providers/shared/direct.go:148, a8211c4252ce)
• Readiness boundary: The shared poll passes the bounded context to the fetch, checks cancellation around observations and waits, and lets the provider preserve completed-response and diagnostic policy. (internal/providers/shared/observer.go:65, a8211c4252ce)
• Regression coverage: The added tests cover retained causes and exit codes, the retry veto, successful-rollback retry, cancellation, elapsed read and sleep bounds, and completed-response precedence. (internal/providers/tencentcloud/provider_test.go:348, a8211c4252ce)

Likely related people:

• steipete: 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.

• Resolve the prior native Tencent qualification request with a guarded lifecycle run or explicit owner waiver.
• Record explicit owner acceptance of the stop-on-failed-termination upgrade behavior.

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-21T20:50:33.826Z sha e4d0c1c :: needs changes before merge. :: none
• reviewed 2026-09-21T21:04:08.094Z sha 08aef04 :: needs changes before merge. :: none
• reviewed 2026-09-25T05:24:09.473Z sha aea8543 :: blocked before merge. :: none