Conversation
Resume and activate returned as soon as the API accepted the request, and first-image --wait could poll until Ctrl-C. --timeout (default 0) now caps those waits via context.
|
Important Review skippedAuto reviews are disabled on this repository. To trigger a review, include ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
secretfader
left a comment
secretfader
left a comment
There was a problem hiding this comment.
2 bugs, 3 risks, 2 nits. Server behaviour checked against Runware/serverless main.
apps_invoke.go:L73: nit:--syncPOST is not under--timeout. It can block ~120s (server write timeout) before the bounded poll starts. WrapInvokeSyncin the wait ctx (task id is already known, so--task-idretries it), or say in help that--timeoutcovers the poll only.wait.go:L20: nit: a negative--timeoutsilently means no limit, and--timeoutwithout--waitis silently ignored. Reject both in flag validation.- test gap: nothing drives
deploy --waitinto an error, which is how the L387 panic got through.TestWaitForApp_TimeoutcoverswaitForApponly. Add a deploy test with aninitializingapp and--timeout 50ms.
| if err != nil { | ||
| spin.Stop() | ||
| return err | ||
| return waitTimeoutErr(err, "application "+app.AppId, timeout) |
There was a problem hiding this comment.
bug: waitForAppDeploy returns a nil app on any error, so app.AppId panics here. Every failed deploy --wait (timeout, Ctrl-C, GetApp 5xx) crashes with SIGSEGV. Reproduced: --wait --timeout 50ms against an initializing app. Capture appID := app.AppId before the call and use it here and in the report closure.
| spin.Stop() | ||
| return err | ||
| } | ||
| if wait && !serverlessapi.AppDeployTerminal(app.Status) { |
There was a problem hiding this comment.
bug: activate on a live app answers active (SetActiveVersion keeps active; the roll never leaves active and restores the old pin if it fails). AppDeployTerminal(active) is true, so --wait never polls and exits 0 before workers roll. A failed rollback looks like success. The API has no rollout-progress field (events are free text), so --wait cannot follow this case. Skip the wait on active and print that the roll runs in the background, and fix the help text. Or drop --wait from activate until the API exposes rollout state.
| if !wait { | ||
| return nil | ||
| } | ||
| return appFailedErr(cmd.Context(), client, app) |
There was a problem hiding this comment.
risk: activate on a stopped app answers stopped (pin recorded, rolls on resume). With --wait, appFailedErr exits 1 with "ended in status stopped" though the call succeeded. stopping polls until stopped, then fails the same way. Treat stopped/stopping as success with a note.
| } | ||
|
|
||
| func waitTimeoutErr(err error, subject string, timeout time.Duration) error { | ||
| if err == nil || !errors.Is(err, context.DeadlineExceeded) { |
There was a problem hiding this comment.
risk: errors.Is(err, context.DeadlineExceeded) also matches the client's 30s per-request Client.Timeout (net/http timeoutError.Is). One slow GetApp mid-wait reports "timed out ... after 5m" early, and with no --timeout says "timed out waiting" when no limit was set. Rewrite only when the wait ctx itself expired (waitCtx.Err() == context.DeadlineExceeded). The timeout == 0 branch then goes away.
| if canCompare { | ||
| settled, err := waitForSubmittedVersion(cmd.Context(), client, app.AppId, versionBefore, pollInterval) | ||
| settled, err := waitForSubmittedVersion(waitCtx, client, app.AppId, versionBefore, pollInterval) | ||
| if errors.Is(err, context.DeadlineExceeded) { |
There was a problem hiding this comment.
risk: same match here turns one 30s request timeout inside waitForSubmittedVersion into a failed deploy (exit 1, app not printed), even without --timeout. Before this PR that error only skipped the endpoint-change warning. Gate on waitCtx.Err().
2 participants
Summary
--wait/--timeout/--poll-intervaltoapps resumeandapps versions activate. Those commands previously returned as soon as the API accepted the request (202 / initializing) and left rollout to the operator.--timeouttodeployandapps invokeso first-image and task waits can be bounded. Default is0(no limit);WaitApp/WaitTaskstill end on a terminal status or a cancelled context, so Ctrl-C keeps working.context.WithTimeout.stopanddeletestay fire-and-forget.Test plan
runware serverless deploy ./app.py --id my-app --gpu-type h100 --wait --timeout 10mpolls until active/failed or times outrunware serverless apps invoke my-app infer --wait --timeout 2m -f payload.jsontimes out withtimed out waiting for task … after 2mif the task stays pendingrunware serverless apps resume my-app --wait --timeout 5mwaits for active/failedrunware serverless apps versions activate my-app 2 --wait --timeout 5mwaits for rolloutapps stop/apps deletehave no--wait/--timeout--waitwithout--timeoutstill polls until a terminal status; Ctrl-C cancels