refactor(worker): report provider provisioning-failure facts, keep recovery policy in core - #2552
Conversation
|
🦞👀 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: blocked before merge. Reviewed September 26, 2026, 1:19 PM ET / 17:19 UTC (Revision 3). ClawSweeper reviewWhat this changesThe branch moves AWS and Hetzner provisioning-failure classification into provider hooks, keeps recovery decisions in the coordinator, and adds failure-transport and cancellation tests. Merge readiness⛔ Blocked before merge - 5 items remain Keep open. Current main still handles these provider failures in the coordinator, so this PR remains useful. No discrete patch defect is established, but the longer visible cleanup state and native-cloud rollout need maintainer judgment. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s coordinator receives lease requests, asks cloud providers to create resources, and stores the resulting lease and cleanup state. The shared coordinator runs on Cloudflare Workers or Node.js with PostgreSQL. flowchart LR
A[Lease request] --> B[Coordinator]
B --> C[Cloud provider]
C --> D[Failure facts]
D --> E[Recovery decision]
E --> F[Stored lease]
E --> G[Cleanup retry]
Decision needed
Why: The delayed completion is intentional and safer against uncertain allocation, but accepting its operator-visible timing and the remaining native-cloud gap requires rollout ownership. Before merge
Agent review detailsSecurityNone. 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 provider failure facts in adapters and recovery policy in core, then qualify existing-record recovery and agree on the client-visible pending state and native rollout threshold. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a refactor with a deliberate safety-behavior change, rather than a reported current-main bug. Focused source tests exercise the changed failure and cancellation paths. Is this the best way to solve the issue? Yes, subject to rollout qualification: provider classification and coordinator-owned recovery match the documented boundary. A direct existing-record recovery check would strengthen upgrade confidence. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a90a781ec5e5. LabelsLabel changes: No label 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 |
90bcee2 to
dd5f2f3
Compare
dd5f2f3 to
5e3b133
Compare
Builds on @steipete's provider-private interpretation and synthetic HTTP tests in #2478. This replacement keeps AWS RunInstances uncertainty and Hetzner structured server/key evidence behind a pure, synchronous provider hook, while the coordinator owns recovery policy. The original draft is unchanged for the coordinator to close with credit.
Removed and why:
retryPendingKeyImmediately. AWS reportsownedKeyCleanupPending; core selects immediate cleanup only when the current record has pending owned-key debt and no allocation uncertainty or recorded resource. Other cleanup retains the five-minute retry delay. A regression test supplies identical provider facts with different current allocation evidence and asserts different delays.canceledBeforeAllocationand the adapter's cancellation-context flag. Cancellation plus a missing cloud ID is not native proof of absence. Core retains dispatched-create uncertainty and request markers until existing recovery resolves it; the key-import cancellation test now verifies the inventory absence-confirmation window before cleanup completes.settlesResourceUncertaintywith the factualallocationRejectedclassification for independently established Hetzner key-only rejection evidence.The hook remains after the current-lease reread, generation fence, and existing cleanup-custody/unresolved-resource guards. All state writes, retention, custody invalidation, completion, and scheduling remain in core. Foreign-provider errors, existing custody, definitive versus uncertain outcomes, replay conflicts, and real AWS/Hetzner clients against synthetic loopback HTTP endpoints retain coverage. Coordinator documentation and the Unreleased changelog are updated.
Validation:
npm ci --prefix worker— passed, zero audit vulnerabilities.npm test --prefix worker -- --maxWorkers=2— 3,431 passed, 15 skipped, including Node runtime and production-bundle tests.node scripts/check-docs-links.mjs— passed, 253 Markdown files.go vet ./..., CLI build, and the command-docs check encountered missing shared Go-cache artifacts following host disk exhaustion. No Go source changed; the coordinator's remote Go gate and CI remain required.No live native-provider proof was performed: this host's permitted AWS configuration excludes lease creation, and Hetzner credentials are unavailable. Loopback client tests are synthetic transport proof, not native lifecycle qualification. Native proof still requires an approved AWS execution configuration or Hetzner environment.
No wire-schema, dependency, credential, configuration, or migration changes. Canceled creates can now retain unresolved cleanup status until recovery establishes allocation absence. Worker merges follow the existing coordinator deployment workflow; this PR is not merged or deployed. Rollback is a code revert and redeployment, with no data migration.