refactor: centralize provider readiness termination policy - #2456
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: needs real behavior proof before merge. Reviewed September 21, 2026, 2:01 PM ET / 18:01 UTC. ClawSweeper reviewWhat this changesThe PR shares Vast and RunPod readiness deadlines and cancellation handling while retaining each provider’s readiness checks, polling schedule, and public diagnostics. Merge readiness⛔ Blocked before merge - 2 items remain This remains a useful, bounded consolidation absent from current main. No actionable correctness defect was found; the reported native smokes support the approach, but inspectable runtime evidence is still needed before merge. Priority: P3 Review scores
Verification
How this fits togetherDuring machine acquisition, Crabbox polls provider APIs until an SSH endpoint becomes ready. The shared readiness helper returns either the ready resource or an error that the adapter uses to stop acquisition and invoke its existing cleanup path. flowchart TD
A[Machine acquisition] --> B[Provider readiness adapter]
B --> C[Shared deadline and polling]
C --> D[Provider API observation]
D --> E{Ready or stopped?}
E -->|Ready| F[SSH preparation]
E -->|Error or cancellation| G[Acquisition cleanup]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep one shared readiness termination policy with provider-specific predicates and diagnostics, supported by inspectable production-path evidence. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR consolidates existing behavior rather than reporting a new failure; source comparison and focused tests define the behavior to preserve. Is this the best way to solve the issue? Yes: extracting the matching adapter policies is a narrow solution, while leaving existing PollReady consumers unchanged avoids imposing a different cancellation contract. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6aa35cea5840. LabelsLabel 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
|
|
@clawsweeper re-review The body now includes captured results on final source 38db1ee: fresh Vast and RunPod acquisition, actual SSH, successful normal cleanup, independent inventory/exact-ID absence, and production HTTP-client deadline test output. Sensitive identities and endpoints are omitted. Current-head CI passed. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Summary
Vast and RunPod now share ordinary readiness-budget and termination policy through
PollReadiness, rather than maintaining separate versions of the same cancellation logic.The shared helper owns the child deadline, interrupted-read classification, completed-observation precedence, and cause-preserving termination wrapper. Adapters retain endpoint/state predicates, native API-error recognition, retry/backoff decisions, and public diagnostics. RunPod's jittered schedule and Vast's terminal-state handling remain local. Existing
PollReadycallers are deliberately unchanged pending their individual contract audit.Verification
38db1ee5154a38af9f4f4125f772be05a140f4e4. Binary SHA-256:dcd36be2ae2ef963ae04185aa0954ca12b06987dee2eae1b95c56d722990efd8.The separately prepared OVH consumer remains outside this PR and retains its own native-proof requirement. This is an internal consolidation with no intended user-visible change, so no new changelog entry is added.
Captured final-source runtime results
The following is a field-selected copy of the native harness output. Credentials, resource identities, endpoints, local paths, and process IDs are omitted. Native runs exercise acquisition/SSH and normal release; deadline cancellation is covered separately through the production HTTP clients against loopback servers.
{ "vast": { "source": "38db1ee5154a38af9f4f4125f772be05a140f4e4", "binarySha256": "dcd36be2ae2ef963ae04185aa0954ca12b06987dee2eae1b95c56d722990efd8", "status": "passed", "nativeReadiness": { "ready": true, "state": "ready", "actualSSH": true }, "cleanup": { "ownInstances": 0, "instanceMissingById": true, "localClaims": 0 }, "events": [ { "phase": "doctor", "exit": 0, "seconds": 2.23 }, { "phase": "warmup", "exit": 0, "seconds": 42.597 }, { "phase": "ready", "exit": 0, "seconds": 2.175 }, { "phase": "destroy_1", "exit": 0, "seconds": 1.453 } ] }, "runpod": { "source": "38db1ee5154a38af9f4f4125f772be05a140f4e4", "binarySha256": "dcd36be2ae2ef963ae04185aa0954ca12b06987dee2eae1b95c56d722990efd8", "status": "passed", "nativeReadiness": { "ready": true, "state": "ready", "actualSSH": true }, "cleanup": { "taskPodsRemaining": 0, "exactAbsent": true, "normalReleaseSucceeded": true, "localClaimAbsent": true }, "events": [ { "phase": "doctor", "exit": 0, "seconds": 2.693 }, { "phase": "warmup", "exit": 0, "seconds": 34.469 }, { "phase": "ready", "exit": 0, "seconds": 1.379 }, { "phase": "release", "exit": 0, "seconds": 1.771 } ] }, "privateRuntimesRemoved": true }Production-client deadline test output from the full race run: