feat(pod,serverless): add --wait and --wait-timeout to create (CON-689) - #317
Conversation
aef6b19 to
aaa3750
Compare
lukepiette
left a comment
There was a problem hiding this comment.
Approving. The part that matters most on a flag-addition PR — the default path — is verified byte-compatible: all 147 deletions decompose into helper extractions and gofmt-only hunks (each diffed against main), no-wait stdout/exit codes/non-blocking behavior are unchanged for both creates, and the shared-helper refactors (sshconnect, project ssh, internal/api/client.go split) preserve caller behavior — including quietly fixing a real main bug where a poll succeeding at the deadline still reported timeout, and a nil-pod SIGSEGV. The pod-ready predicate (RUNNING + port 22 mapped + TCP connect + SSH- banner) is the right strength, keeping the billed resource on timeout with a machine-readable id is the right call, and the fake-clock tests mean none of this sleeps in CI.
Comments (none blocking):
- Ctrl-C window after readiness:
defer stop()inwaitForPodSSHderegisters the signal handler beforepodDetailsWithSSHruns its up-to-~4s of context-blind re-read sleeps. A Ctrl-C in that window exits 130 with empty stdout — no JSON error object, noid— the one path where the pod is orphaned as far as machine-readable output goes. Keeping the signal ctx alive through the re-read (and threading it into the backoff sleeps) closes it. - Asymmetric fail-fast: an endpoint deleted before
/healthever answers 404s "transiently" forever and burns the full 10m budget, because the resource-gone fail-fast only arms after one successful read. Narrow corner, but it is asymmetric with the pod path's two-consecutive-miss rule. - The endpoint-ready predicate (
ready > 0 || running > 0) is soft sincerunningis written at scheduling time — disclosed in help/README with a backend follow-up filed; holding that follow-up to account is worth doing. - Coordinate land order with #315/#316: #315 also refactors
cmd/pod/get.goandinternal/sshconnect/sshconnect.go(real merge conflicts, not just textual), and #316 adds the sibling raw/healthclient this PR's body promises to consolidate with. Landing #315 → #317 → #316 (or agreeing on any explicit order) beats three independent rebases onto moved code.
397dffc to
90bab7a
Compare
… read (CON-689) create returns when a resource is scheduled, not when it is usable. add the machinery for an opt-in wait: - internal/waitfor: one bounded poll loop with injectable clock, throttled stderr progress and a typed error carrying the resource id, the last known state and a stable code (wait_timeout / wait_interrupted). - internal/waitfor.ProbeSSH: tcp connect plus ssh banner. verified against prod that a cpu pod running alpine has a public port 22 listed in runtime.ports while the connection is refused, so port allocation is not readiness. no handshake, so --wait works without a configured ssh key. - internal/api.GetEndpointHealth: the invoke service's live worker counts, which is the only readiness signal for an endpoint (includeWorkers is historical). - internal/sshconnect.PublicSSHPort: the port-22 lookup BuildConnection already did, now shared. - internal/duration: the pod list --since parser, moved so --wait-timeout reuses it instead of adding a third duration parser. also runs gofmt over api/endpoint.go and cmd/pod/list.go, which were already unformatted on main.
pod create --wait blocks until the pod's public port 22 answers with an ssh banner, then prints the same payload as 'pod get' (the create response has no ssh info, and a pod you can connect to is the point of waiting). serverless create --wait blocks until /health reports a ready or running worker. it requires --workers-min >= 1: at 0 runpod starts no worker until a request arrives, so the wait could only ever time out. refuse up front rather than silently billing a warm worker the user did not ask for. --wait-timeout defaults to 10m. on timeout or ctrl-c the resource is kept, the exit code is non-zero and the error names the id, the last known state and the delete command. progress goes to stderr on a 15s cadence; stdout stays a single json object. --wait cannot be combined with --ssh=false, and warns on cpu pods, which are created over rest and so never get runpod-managed ssh. the legacy project ssh loop now calls the shared wait too, dropping its re-poll-inside-the-condition bug and its post-loop timeout check that could fire on a poll that had just succeeded. its stdout line and 1s/5m timings are unchanged.
four cases: the cpu timeout path (an image with no sshd, which prod still gives a public port 22 — the exact state the wait must not read as ready), the gpu success path asserting one json object with a live ssh command, the free workers-min refusal, and a warm-worker endpoint wait. every paid resource is torn down in t.Cleanup, endpoint before template. runCLI now honours RUNPODCTL_BIN so a run can target a 'go build' output instead of overwriting the installed ~/go/bin/runpodctl.
… resources (CON-689)
review found three ways --wait misbehaved once it was actually waiting.
a single bad poll ended the whole wait, and it surfaced with the underlying
transport code. verified against a fake control plane: /health 404s an endpoint
id the invoke service has not propagated yet, so `serverless create --wait`
could exit ~0.04s after create with `{"code":"not_found","status":404}` for an
endpoint that exists and is billing a warm worker; one graphql blip did the same
to `pod create --wait` with `network_error`, the one code readme documents as
"transient, retry" — an agent following that would buy a second pod. poll errors
are now the current state, not the end of the wait; only failures that cannot
resolve (unauthorized, forbidden, no_credentials, bad_request) stop it.
a pod that can never become ready burned the whole budget: a terminal
desiredStatus polled for the full 10m default, and a pod terminated out of band
mid-wait read as "pod not listed yet" for the rest of it while the error claimed
it was "still billing". both now end the wait at once (conflict / not_found).
`pod create --wait` could exit 0 with `"ssh": {"error": "ssh info unavailable"}`:
the post-wait re-read swallows a graphql failure into that blob, so the one field
the flag exists to produce went missing with a success exit code. the re-read now
retries and then fails loudly, naming the address that did answer.
also: --wait on community cloud without --public-ip is warned about the way the
cpu path already was (no publicly mapped port 22 to probe, so it could only time
out), and errors that leave a resource behind carry its id in the error object's
new `id` field instead of only in prose.
…the workers-min claim (CON-689)
second review round. all of these were reproduced through the real binary
against a fake control plane before the fix and re-run after.
- isFatalPollError now also consults the http status (400/401/403). the pod
wait's only api call is graphql GetPods(), and every graphql failure is an
*api.GraphQLError whose ErrorCode() is the constant "graphql_error", so none
of fatalPollCodes was reachable there: a bad key burned the whole budget
while the pod billed and reported wait_timeout. 15.05s -> 0.41s.
- serverless create --wait no longer refuses --workers-min 0. the premise was
wrong: ai-api floors workersStandby to 5 whenever workersMax > 1 regardless
of workersMin (pkg/graphql/aiapi.go finalEndpoint), worker.Sync fills it with
cache workers (pkg/worker/sync.go) and every /health read triggers a Sync
(pkg/loader/aiapi.go), and /health counts a cached worker as ready. six prod
endpoints with workersMin unset report ready 1-5. it now warns, like the
other satisfiable-but-often-not combinations, and the refuted claim is out of
the error string, README and AGENTS.md.
- the endpoint poller had no fatal case, so an endpoint deleted out of band
burned the full budget while the pod path failed fast. a /health 404 after a
successful read is now fatal not_found; a 404 before the first read
(propagation lag) and any 5xx stay transient.
- the endpoint detail string now reports `running`, and the success line carries
the detail, so a run says which counter satisfied it. running is written at
scheduling time (runpod-backend rentPod.ts), so an unattributed "ready after"
was not evidence of anything.
- the pod poller takes two consecutive missing reads before declaring a pod
deleted. one short list read is an unknown state like every other tolerated
anomaly, and the error asserted the pod "was terminated".
- FindPodConnection/ListConnections skip nil entries. graphql lists are
nullable, and --wait re-reads that list right after reporting success: a null
entry panicked with SIGSEGV and exit 2, replacing the json error object with
a stack trace.
- duration.Parse range-checks the product, not just the operand. 106752d and up
wrapped negative, so an out-of-range --wait-timeout was silently replaced by
the 10m default (and pod list --since 200000d returned []).
- the post-wait re-read passes includeMachine=true, so --wait no longer hands
back less than a plain create (graphql selected machine { gpuDisplayName
location }).
- two fail-fast tests set interval and timeout both to an hour, so a regression
hung until go test panicked at 10m; they now pass a 300ms budget.
State.Err had no production reader (cmd/project discards the state and tracks its own last error), addrOrUnknown's empty branch was unreachable (the poller always writes the address before reporting ready), and sshInfoError's 'unknown' fallback could not fire. TestUntilAppliesDefaults replaces the assert-nothing defaults test: DefaultTimeout, DefaultInterval and DefaultProgressEvery no longer survive mutation (verified).
…een resource (CON-689) review follow-ups: - the signal handler was registered inside waitForPodSSH, so `defer stop()` deregistered it before the post-wait re-read ran its sleeps. a ctrl-c in that window took the default disposition: exit 130, empty stdout, no error object, no pod id -- the one path that loses a billed pod. the wait and the re-read now live in one function (waitForReadyPod) under a single registration, and the re-read's backoff is cancellable, so an interrupt there reports wait_interrupted with the pod id and the delete command. the first read still always happens: a ctrl-c landing exactly as ssh came up should still get the payload it waited for. - new waitfor.SignalContext, used by both create waits: signal.NotifyContext keeps the handler armed after the first delivery, so a second ctrl-c was swallowed while an uncancellable api call finished (up to 30s per call). the registration is released as soon as the first signal lands, so the first ctrl-c still produces the error object and a second one always exits. both call sites assert it, not just the helper: reverting either to notifyWaitSignals fails a test. - the not-found fail-fast only armed after one successful read, so a resource that was never readable -- an endpoint that never propagated to /health, a pod terminated before it was ever listed -- was treated as lag for the whole budget and then reported wait_timeout, which tells an agent to retry a create that already succeeded. both waits now give a never-seen resource 12 consecutive misses (~1 min at the default interval) and then a not_found. - neither message claims the resource never existed: the id came from a create that succeeded, and a caller told otherwise buys a second billed resource. the README not_found row says the same, since that is the code an agent branches on before deciding whether anything needs cleaning up. - a read failure between two misses resets the run in both pollers, so "consecutive" means consecutive -- otherwise a miss, a blip and a second miss added up to not_found for a live pod. - waitfor.Interrupted returns the same *waitfor.Error type Until does, so a consumer type-tests an interrupted wait once instead of per phase; the miss bounds sit with the other wait constants and are pinned against the numbers README/AGENTS.md quote.
90bab7a to
a402fb3
Compare
Adds an opt-in
--waittopod createandserverless createso create returns when the resource is usable, not when it is scheduled — today a pod reportsRUNNINGminutes before sshd answers, so agents sit in a poll loop guessing.Linear: CON-689
What changed
pod create --waitreturns when the pod's public port 22 accepts a TCP connection and answers with an SSH banner, then prints thepod getshape (so the payload carries the livesshblock).serverless create --waitreturns when/healthreports areadyorrunningworker.--wait-timeout(default10m; reuses the existing7d-aware duration parser, now shared ininternal/durationand range-checked).internal/waitfor: one bounded poll loop with an injectable clock, throttled stderr progress, and a typed error carrying the last known state. Progress is a caller-suppliedio.Writer, so the loop structurally cannot touch stdout.PodSSHConnectionloop (cmd/project/ssh.go) now calls the shared helper — its stdout line, wording and timings are byte-identical to main (diff in details), and it loses a bug where a poll that succeeded at the deadline still reported timeout.idfield, so await_timeoutnames the billed resource as data, not prose. Two new codes:wait_timeout,wait_interrupted.Notable decisions
runtime.portsmeans reachable; it doesn't — docker publishes the binding at container start regardless of whether anything listens (measured: public port 22 in ~25s whilencwas refused). The legacy loop was still extracted and reused as asked. No handshake/key check, so--waitcan succeed on an image whose sshd never got your key — disclosed in help and README./healthready > 0 || running > 0. Neither counter is as strong as "a worker is ready" (readycounts a flashboot-cached EXITED worker;runningis written at scheduling time), but/healthexposes nothing stronger, andGetEndpoint(includeWorkers)returns historical records. The success line names which counter fired. Backend ask filed as follow-up.--workers-min 0warns instead of refusing. An earlier refusal rested on a false premise (review-refuted at the source: standby workers are floored to 5 wheneverworkersMax > 1, and the CLI's own polling drives provisioning) — live probe showed 6 endpoints withworkersMinunset all reportingready > 0.Full deviation analysis and review triage (2 rounds, 28 findings: 20 fixed, 1 rejected, rest disclosed/nits)
Ten deviations from CON-689's wording, each re-derived by an independent reviewer from
ai-api/runpod-backend/hostsource or free live probes. Beyond the decisions above:pod create --waitprints thepod getshape, not the create response — neither create response carries an ssh command, and a connectable pod is the flag's purpose. The re-read now passesincludeMachine=true; the residualenv/portstype difference vs graphql is disclosed.--compute-type CPUand community-without---public-ipwarn instead of failing (satisfiable but often not); only--ssh=falseis refused (genuinely unsatisfiable).Key review fixes: auth fail-fast was unreachable on the pod path (all graphql failures carried
graphql_error; a bad key burned the full budget — now 0.4s fatal via HTTP status); a deleted endpoint burned the whole 10m (a/health404 after a successful read is now fatal in ~10s; 404 before first read and any 5xx stay transient);FindPodConnectionnil-deref on apods:[null]element right after--waitsuccess (SIGSEGV → nil guard);duration.Parsesilently going negative at>= 106752d(now range-checked, also fixespod list --since 200000d); one transient emptymyself{pods}read ending the wait as "terminated" (now two consecutive misses); e2e cleanup registered before each create sot.Fatalfpaths can't orphan a billed resource.Rejected (1): a "blocker" mutation report where a concurrent process had left an edit in the shared worktree mid-review; moot — that guard is gone.
Nits later fixed in
16e96ba: unusedState.Errdeleted, two unreachable fallback branches removed, and the defaults test strengthened soDefaultTimeout/DefaultInterval/DefaultProgressEveryno longer survive mutation. Still skipped: legacy 5s account-wide poll, re-read backoff ignoring the wait context (~4s worst-case overshoot, inherited from the legacy loop).Testing
internal/waitfor95%+,internal/duration100%. Gates:gofmt/go vet(incl. e2e tags) clean,go test ./...all 16 packages, docs regenerated. Every substantive review finding was reproduced through the real binary against a fake control plane before fixing, and re-run after.Evidence: fake-control-plane repros, live prod e2e with cleanup proof, legacy byte-diff
Fake control plane (free):
Live prod e2e (round 1; paid paths not re-bought — round 2 verification was free: source reads, read-only probes, fake plane):
Legacy
project/execbehavior, base vs head against an unreachable graphql: stdout and stderr byte-identical.Follow-ups
--wait/--wait-timeoutsemantics, whatreadyactually proves, thepod getoutput shape, the new codes +idfield/healthcounter does; today's predicate is the strongest thing a client can observeinternal/api.GetEndpointHealthmay collide with CON-688'sserverless health; whichever lands second should reuse the other