fix(vast): bound SSH readiness observations and waits - #2448
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 21, 2026, 12:48 PM ET / 16:48 UTC (Revision 5). ClawSweeper reviewWhat this changesThe PR bounds Vast SSH-endpoint polling and retry waits by the existing startup deadline, preserves redacted transport error causes, and adds regression coverage and documentation. Merge readiness✅ Ready for maintainer review The fix remains necessary: current main and v0.63.0 still leave readiness observations outside the startup deadline. The synchronized head resolves the previous conflict, has sufficient real behavior proof, and introduces no actionable correctness findings. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s Vast adapter provisions GPU instances and waits for an SSH endpoint before bootstrapping tools and returning a usable lease. This polling step consumes Vast API responses and caller cancellation, then returns readiness or an error to the acquisition and cleanup flow. flowchart TD
A[Lease acquisition] --> B[Ten-minute readiness budget]
B --> C[Vast API observation]
C --> D{Observation result}
D -->|Ready endpoint| E[SSH bootstrap and lease]
D -->|Still loading| F[Cancellation-aware wait]
F --> C
D -->|Error or deadline| G[Safe error and acquisition cleanup]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep readiness timing inside the Vast adapter, using shared cancellation and display-safe error helpers without changing lease ownership or configuration. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the failure path: a Vast observation begun near the startup deadline can continue beyond it because main supplies only the parent context. This review did not execute a reproduction. Is this the best way to solve the issue? Yes. A deadline-bearing polling context and existing shared error helpers repair the established timeout contract without introducing a parallel configuration or lifecycle mechanism. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 7ef1afd0fab1. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
|
@clawsweeper re-review The main PR body now contains completed native proof for the unchanged reviewed head 8a19ef6: fresh acquisition, actual SSH readiness, successful normal destroy, independent inventory/exact-ID absence, and local claim cleanup. The private runtime and credential window are closed. The previous review captured the body before that proof was available; exact-head CI is still pending. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
@clawsweeper re-review The synchronized head is 5e04038. The main body now includes a new native run on that exact source: fresh acquisition, actual SSH readiness, successful normal destroy, independent inventory/exact-ID absence, and local claim cleanup. Both changelog entries were retained during integration; the full Vast race suite, vet, and independent review passed again. The private runtime and credential window are closed. Current-head CI remains running. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
1 participant
Summary
Vast's ten-minute SSH-endpoint wait checked the clock only after an API observation. A slow request or retry sleep could therefore outlive that budget. The client also redacted transport errors by flattening their causes into strings.
Use a deadline-bearing context for observations and waits, reuse the shared cancellation-aware sleeper, and retain transport causes through the shared display-safe error wrapper. Preserve completed ready responses, terminal native states, and API errors; do not relabel an independent client timeout as startup timeout. Lifecycle timestamps, native ownership, provisioning payloads, and cleanup rules are unchanged. Documentation and an Unreleased entry are included.
Verification
Before merge
Exact-head CI run 35626628510 passed for the synchronized source below. No credentials, dependency changes, or configuration migration are required.
Native proof on synchronized head
Source:
5e04038733d27edc8efb6866405b8cf09095a487. Binary SHA-256:2f3c164e16d297af08be1ea8f0df21b660b9fe74e27afa91dca24545c12fd0e9. After merging main and retaining both changelog entries, the full Vast race suite, vet, and complete independent review passed again. A fresh native RTX 5090 instance under the $0.50/hour offer-search cap passed account checks, acquisition through the changed readiness loop (256 seconds including bootstrap), and actual SSH readiness (ready: true,state: ready). The first normal destroy succeeded; independent full inventory and exact-ID reads confirmed no task instance remained, and the local claim was absent. Private runtime/key material was removed and the owned credential window closed. This is a new run on the synchronized source, not a relabeling of earlier proof.