From 3de354efda07c09e4e3b11fd4aa0b70a7539a719 Mon Sep 17 00:00:00 2001 From: Volodymyr Onofriichuk Date: Thu, 27 Aug 2026 17:06:03 +0300 Subject: [PATCH 1/3] [RAPTOR-19530] feat(workload): wait out a rollout in flight instead of refusing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dr workload up` already waits out a workload that is still moving under its own power (RAPTOR-19686). It did not wait out the one transition that check structurally cannot see: a workload being replaced reports itself running for the whole of the swap, so a rollout in flight reached guardRollout and was refused with "wait for it to settle before starting another" — the same dead end a settling workload used to have, and work the command can do itself. lookSettled now asks the replacement route before it builds the plan. A swap already under way is named on stderr, waited out, and the workload re-read, so the plan is built against where the swap landed rather than against state the platform was already moving. The rollout is waited first and the status second because a swap that lands leaves the workload coming up: one pass settles both. A rollout that ends failed never promotes, so the version that was serving is still serving and the workload is deployable. The run says what it saw and plans against what the failure left behind rather than handing back the same "run it again" refusal. A timeout or an unreadable route stops the run and names where the rollout got to. The guards at the apply sites stay refusals. A rollout that appears after the pre-plan read is somebody deploying concurrently, and waiting there would apply a plan built against state that has since moved — for --lock, it would take a one-way lock on the outgoing version. --dry-run says a deploy would wait and changes nothing; --detach explains that this wait precedes the deploy, as it already does for a settling workload. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/workload/up/cmd.go | 8 +- internal/workload/up/run.go | 177 ++++++++++++++++- internal/workload/up/run_test.go | 314 ++++++++++++++++++++++++++++++- 3 files changed, 489 insertions(+), 10 deletions(-) diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index da891c70..b6ea41dd 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -217,9 +217,11 @@ sends the sizing with the rollout, so the new version comes up with it. A workload that is not ready to be deployed onto is dealt with rather than refused. One still starting or stopping is waited out and then re-read, so the -plan is built against where it landed. A stopped one is started and then -reconciled in the same run, which is one command whether the file asks for a -start alone or for a start and a new version. +plan is built against where it landed, and so is one already being rolled onto +a new version, which reports itself running for the whole of the swap. A +stopped one is started and then reconciled in the same run, which is one +command whether the file asks for a start alone or for a start and a new +version. Examples: dr workload up diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index b9c708b5..e2a1109f 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -54,6 +54,7 @@ var ( listBuildsFn = workload.ListArtifactBuilds getCredentialFn = workload.GetCredential findCredentialFn = workload.FindCredentialNamed + activeReplacementFn = workload.GetActiveReplacement guardReplacementFn = workload.RefuseActiveReplacement startReplacementFn = workload.StartReplacement waitReplacementFn = workload.WaitForReplacement @@ -327,13 +328,187 @@ func guardRollout(workloadID, consequence string) error { // lookSettled is the live read a plan is built from: the workload as it is, // once it has stopped moving. +// +// Two things can be moving, and they are asked about in this order because the +// rollout is the coarser of them. A swap that lands leaves the workload coming +// up, so waiting the rollout out first and the status second settles both in +// one pass; the other order would return with the workload still provisioning +// and hand deployable a state it has to refuse. Neither wait needs the plan, +// which is the point: a deploy arriving mid-transition is early rather than +// wrong. func lookSettled(workloadID string, opts Options) (Live, error) { found, err := Look(workloadID) if err != nil { return Live{}, err } - return awaitSteady(found, opts) + replaced, err := awaitReplaced(found, opts) + if err != nil { + return replaced, err + } + + return awaitSteady(replaced, opts) +} + +// awaitReplaced waits out a swap somebody else already started, and hands back +// the workload as it is once that swap has landed. +// +// This is the transition awaitSteady structurally cannot see: a workload being +// replaced reports itself running for the whole of the swap, so the only way to +// know is to ask the replacement route. It used to be asked as a refusal, which +// left the same dead end a settling workload used to have — "wait for it to +// settle before starting another" is work the command can do itself. +// +// The re-read afterwards is not a formality. A rollout that completes moves the +// workload onto a different artifact, so a plan built against the state as it +// was would roll a version the platform had already installed, or report drift +// the swap had just closed. +// +// The guards at the apply sites stay refusals and are not made redundant by +// this. A rollout that appears after this read is somebody deploying +// concurrently, and waiting there would apply a plan built against state that +// has since moved. +// +// A dry run never waits, for the reason it never waits on a settling workload: +// blocking a preview for the poll timeout is the opposite of what a preview is +// for. +func awaitReplaced(live Live, opts Options) (Live, error) { + if !replaceable(live) { + return live, nil + } + + active, err := activeReplacementFn(live.WorkloadID) + if err != nil { + return live, fmt.Errorf( + "cannot tell whether workload %s already has a rollout in progress, so nothing was deployed: %w", + live.WorkloadID, err) + } + + // A settled record stays readable for a while after the rollout ends, and + // waiting on one would block every deploy for as long as it lingers. + if active == nil || workload.IsTerminalReplacementStatus(active.Status) { + return live, nil + } + + report := newReporter(opts.Stderr, opts.Spinner) + + report.say(" %s\n", tui.HintStyle.Render(replacingNote(live.WorkloadID, active))) + + if opts.DryRun { + report.say(" %s\n", tui.HintStyle.Render( + "A deploy would wait for this rollout to finish and plan against where it lands.")) + + return live, nil + } + + if opts.Detach { + // --detach is about not waiting for the deploy to serve. This wait is + // before the deploy: what to apply cannot be known until the swap has + // landed. Saying so beats blocking in silence. + report.say(" %s\n", tui.HintStyle.Render( + "Waiting for it to land before planning; --detach applies to the deploy.")) + } + + var settled *workload.Replacement + + err = report.run("Waiting for the rollout already in progress", func() error { + // Seeded with the record just read, which is what lets the wait tell a + // rollout the platform settled before the first poll landed from one + // that was never there at all. + replacement, waitErr := waitReplacementFn( + live.WorkloadID, active, opts.PollInterval, opts.PollTimeout, nil) + settled = replacement + + return waitErr + }) + if err != nil { + if failure := replacedFailed(live, settled, err); failure != nil { + return live, failure + } + + report.say(" %s\n", tui.WarnStyle.Render(fmt.Sprintf( + "⚠ That rollout ended as %s, so the workload is still running the version it was.", + settled.Status))) + } + + return Look(live.WorkloadID) +} + +// replaceable says whether there is a workload for the replacement route to +// answer about. +// +// A workload the platform does not have cannot be being replaced, and the route +// answers the same 404 for "no such workload" as it does for "nothing in +// flight", so asking about one would be a round trip whose answer could not be +// read either way. A terminated workload is refused by deployable regardless, +// so the question is only cost there too. +func replaceable(live Live) bool { + if live.WorkloadID == "" { + return false + } + + switch live.State { + case StateUnbound, StateMissing, StateTerminated: + return false + + case StateStopped, StateSettling, StateRunning, StateErrored: + return true + + default: + return true + } +} + +// replacingNote says what is in flight, carrying only the fields the platform +// filled in. +// +// The artifact is one of them because a settings-only rollout moves the +// workload onto the artifact it is already running and the platform returns no +// candidate for it, so naming one unconditionally would print an empty field +// mid-sentence. +// +// It is said before the wait, not after. Without a terminal there is no spinner +// and a phase prints nothing until it ends, so a CI log would otherwise show +// nothing at all for as long as the swap takes. +func replacingNote(workloadID string, active *workload.Replacement) string { + note := "Workload " + workloadID + " is being replaced" + + if active.ArtifactID != "" { + note += " onto artifact " + active.ArtifactID + } + + if active.Status != "" { + note += ", status " + active.Status + } + + return note + "." +} + +// replacedFailed is the verdict on a wait that did not come back clean, and +// returns nil for the one case the run carries on from. +// +// A rollout that ends failed never promotes: the version that was serving is +// still serving, so the workload is deployable and this deploy is the natural +// remedy. Refusing there would hand back the same "run it again" dead end this +// wait exists to remove, so the caller notes what happened and plans against +// what the failure left behind. +// +// Everything else is a wait that told us nothing — a timeout or a failed poll — +// and names where the rollout got to, which is what decides whether to wait +// longer or go and look at the platform. +func replacedFailed(live Live, settled *workload.Replacement, err error) error { + if settled != nil && workload.IsFailedReplacementStatus(settled.Status) { + return nil + } + + where := "did not finish rolling out" + if settled != nil && settled.Status != "" { + where = "was still " + settled.Status + } + + return fmt.Errorf( + "the rollout of workload %s %s, so nothing was deployed; check 'dr workload status %s': %w", + live.WorkloadID, where, live.WorkloadID, err) } // awaitSteady waits out a workload that is still moving, and hands back what diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index 87fc65aa..e45169de 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -206,10 +206,12 @@ type fakes struct { // checkEndpoint is the one GET a deploy ends with. checkEndpoint func(string) (int, error) - // The roll track: refuse to queue a second swap, start one, follow it. - guard func(string) error - replace func(string, string, json.RawMessage) (*workload.Replacement, error) - waitReplace func(string, *workload.Replacement, time.Duration, time.Duration, + // The roll track: read what is already in flight, refuse to queue a second + // swap, start one, follow it. + activeReplacement func(string) (*workload.Replacement, error) + guard func(string) error + replace func(string, string, json.RawMessage) (*workload.Replacement, error) + waitReplace func(string, *workload.Replacement, time.Duration, time.Duration, func(*workload.Replacement)) (*workload.Replacement, error) // settings is the in-place path: a change that moved only the sizing. @@ -324,7 +326,12 @@ func install(t *testing.T, f fakes) { swap(t, &listBuildsFn, f.builds) // Nothing stands in the way of a rollout unless a test says so, because - // the quiet answer is the one every other roll test wants. + // the quiet answer is the one every other roll test wants. That covers the + // pre-plan read as well as the guards: a test wiring neither is saying its + // workload has no swap in flight. + force(t, &activeReplacementFn, func(string) (*workload.Replacement, error) { return nil, nil }) + swap(t, &activeReplacementFn, f.activeReplacement) + force(t, &guardReplacementFn, func(string) error { return nil }) swap(t, &guardReplacementFn, f.guard) swap(t, &startReplacementFn, f.replace) @@ -558,7 +565,13 @@ func TestRun_LockWithNothingToDoStillLocks(t *testing.T) { // that artifact. Locking cannot be undone, so a lost race would leave the // outgoing version permanent and the rollout unable to complete. deployable // cannot catch it: the workload reports itself running for the whole of a swap. -func TestRun_LockWithNothingToDoWaitsForARolloutInFlight(t *testing.T) { +// +// This is the backstop rather than the ordinary answer. A swap that was already +// under way when the workload was read is waited out before the plan is built, +// so what reaches this guard is one that started after that read, which is +// somebody else deploying concurrently. Refusing is the only safe verdict +// there: waiting would take a one-way lock on a plan that is already stale. +func TestRun_LockWithNothingToDoRefusesARolloutThatStartedLate(t *testing.T) { install(t, fakes{ workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, artifactD: func(string) (workload.Document, error) { return draftArtifact(t), nil }, @@ -1187,6 +1200,295 @@ func TestRun_SettlingIntoErroredIsNotReportedAsUpToDate(t *testing.T) { } } +// A rollout already in flight used to be refused outright, which is the same +// dead end a settling workload was: "wait for it to settle, then deploy" is +// work the command can do itself. It is the one transition awaitSteady cannot +// see, because the workload reports itself running for the whole of a swap. +// +// The re-read is the point. Planning against the state as it was would roll a +// version onto a workload the platform is already moving, so the fixture +// carries drift on the first read and none on the second: the swap that was in +// flight is what closed the gap, and the deploy has nothing left to do. +func TestRun_RolloutInFlightIsWaitedOutAndThenDeployed(t *testing.T) { + var ( + artifacts int + waitedFor string + seeded *workload.Replacement + ) + + active := &workload.Replacement{ + ID: "rep-1", WorkloadID: "68b0c1d2e3f4a5b6c7d8e9f0", + ArtifactID: "68a0000000000000000000a2", Status: "switching", + } + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { + artifacts++ + + d := doc(t, liveArtifactJSON) + if artifacts == 1 { + // Drift only the first read sees: a port the file does not name. + group := d["spec"].(map[string]any)["containerGroups"].([]any)[0].(map[string]any) + group["containers"].([]any)[0].(map[string]any)["port"] = float64(8001) + } + + return d, nil + }, + activeReplacement: func(string) (*workload.Replacement, error) { return active, nil }, + waitReplace: func(id string, started *workload.Replacement, _, _ time.Duration, + _ func(*workload.Replacement), + ) (*workload.Replacement, error) { + waitedFor, seeded = id, started + + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusCompleted}, nil + }, + replace: func(string, string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("the swap already in flight is what closed the gap; nothing was left to roll") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + + assert.Equal(t, "68b0c1d2e3f4a5b6c7d8e9f0", waitedFor) + assert.Same(t, active, seeded, + "the record just read is the seed, so the wait can tell a rollout that finished early "+ + "from one that was never there") + assert.Equal(t, 2, artifacts, "the workload is re-read, because a swap lands somewhere new") + assert.Equal(t, ActionUnchanged, result.Action, + "the plan is built against where the swap landed, and the swap is what closed the gap") + + assert.Contains(t, stderr, "is being replaced") + assert.Contains(t, stderr, "68a0000000000000000000a2", "the note names what is being rolled on") + assert.Contains(t, stderr, "switching") + assert.Contains(t, stderr, "Waiting for the rollout already in progress") +} + +// A rollout that ends failed never promotes, so the version that was serving is +// still serving and the workload is deployable. Refusing here would strand the +// user on the same "run it again" dead end; the run says what it saw and plans +// against the state the failure left behind. +func TestRun_RolloutThatEndsFailedIsNotedAndTheRunContinues(t *testing.T) { + var started string + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return stoppedWorkload(t), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(id string, _ *workload.Replacement, _, _ time.Duration, + _ func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusFailed}, + errors.New("replacement for workload " + id + " ended with status failed") + }, + start: func(id string) (*workload.WorkloadOperationResponse, error) { + started = id + + return &workload.WorkloadOperationResponse{WorkloadID: id}, nil + }, + wait: func(id string, _ workload.Serving, _, _ time.Duration, _ func(*workload.Workload)) (*workload.Workload, error) { + return running(id), nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err, "a rollout somebody else lost is not this deploy's failure") + + assert.Equal(t, "68b0c1d2e3f4a5b6c7d8e9f0", started, "the run went on to do what the file asked") + assert.Equal(t, ActionStarted, result.Action) + assert.Contains(t, stderr, "ended as failed") + assert.Contains(t, stderr, "still running the version it was") +} + +// A rollout still going when the wait gave up has not been deployed onto. The +// message names where it got to, because "still switching after 30m" is what +// decides whether to wait longer or go and look at the platform. +func TestRun_RolloutThatNeverLandsStopsBeforeMutating(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return stoppedWorkload(t), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(id string, _ *workload.Replacement, _, _ time.Duration, + _ func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, + errors.New("timeout waiting for replacement on workload " + id + " after 30m0s") + }, + start: func(string) (*workload.WorkloadOperationResponse, error) { + t.Fatal("a run that never got a settled state must not have changed anything") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, _, err := runIn(t, bound, Options{NonInteractive: true}) + require.Error(t, err) + assert.Contains(t, err.Error(), "was still switching") + assert.Contains(t, err.Error(), "dr workload status 68b0c1d2e3f4a5b6c7d8e9f0") + + // The binding survives the failure, for the same reason it does when a + // workload never settles: losing the id is how a deploy becomes unfindable. + assert.Equal(t, "68b0c1d2e3f4a5b6c7d8e9f0", result.WorkloadID) +} + +// A read that cannot answer is not a rollout. The two want different words: one +// is a state to wait out, the other is a question that did not get asked. +func TestRun_UnreadableRolloutStateStopsTheRun(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return stoppedWorkload(t), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return nil, errors.New("500 Internal Server Error") + }, + start: func(string) (*workload.WorkloadOperationResponse, error) { + t.Fatal("a run that could not read the rollout state must not have changed anything") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, _, err := runIn(t, bound, Options{NonInteractive: true}) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot tell whether") +} + +// A preview must not block for the poll timeout. It changes nothing, so the +// honest answer is the plan as things stand plus a note that a deploy would +// wait: the same bargain a settling workload gets. +func TestRun_DryRunWithARolloutInFlightDoesNotWait(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + t.Fatal("a preview must not block for the poll timeout") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, stderr, err := runIn(t, bound, Options{NonInteractive: true, DryRun: true}) + require.NoError(t, err) + + assert.Contains(t, stderr, "is being replaced") + assert.Contains(t, stderr, "plan against where it lands") +} + +// --detach is about not waiting for the deploy to serve, and this wait comes +// before the deploy: what to apply cannot be known until the swap lands. +// Blocking is right, blocking in silence is not. +func TestRun_DetachedRunSaysWhyItIsWaitingForTheRollout(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusCompleted}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, stderr, err := runIn(t, bound, Options{NonInteractive: true, Detach: true}) + require.NoError(t, err) + assert.Contains(t, stderr, "--detach applies to the deploy") +} + +// The counterpart to the backstop above, and the reason the wait is placed +// before the plan rather than at the apply sites. A `--lock` run whose swap was +// already under way when the workload was read waits it out and locks whatever +// the swap left serving, instead of refusing a run whose only fault was +// arriving early. Getting this order wrong is not cosmetic: locking is one-way, +// so a lock taken against the pre-swap read would make the outgoing version +// permanent. +func TestRun_LockWaitsOutARolloutThatWasAlreadyInFlight(t *testing.T) { + var ( + locked string + artifacts int + ) + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { + artifacts++ + + return draftArtifact(t), nil + }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusCompleted}, nil + }, + lock: func(artifactID string) (*workload.Artifact, error) { + assert.Equal(t, 2, artifacts, + "the lock is one-way, so it lands on what the swap left serving, not on the pre-swap read") + + locked = artifactID + + return &workload.Artifact{ID: artifactID, Status: workload.ArtifactStatusLocked}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true, Lock: true}) + require.NoError(t, err) + + assert.NotEmpty(t, locked) + assert.True(t, result.Locked) + assert.Contains(t, stderr, "Waiting for the rollout already in progress") +} + +// A manifest with nothing to resolve has no workload to ask about, and the +// replacement route answers the same 404 for "no such workload" as it does for +// "nothing in flight". Asking would be a round trip whose answer cannot be +// read either way. +func TestRun_ARunWithNoLiveWorkloadNeverAsksAboutARollout(t *testing.T) { + install(t, fakes{ + activeReplacement: func(string) (*workload.Replacement, error) { + t.Fatal("there is no workload to be replaced") + + return nil, nil + }, + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(id string, _ workload.Serving, _, _ time.Duration, _ func(*workload.Workload)) (*workload.Workload, error) { + return running(id), nil + }, + }) + + result, _, err := runIn(t, unboundImageManifest, Options{NonInteractive: true}) + require.NoError(t, err) + assert.Equal(t, ActionCreated, result.Action) +} + // TestRun_MissingWorkloadIsRecreated is the reported bug: a workload deleted // out from under the manifest used to leave the user with a binding only a // hand edit could clear. It is drift, so the deploy recreates and rebinds. From 74d2dfef4372f2adc86d874f54c49a85d7c715d0 Mon Sep 17 00:00:00 2001 From: Volodymyr Onofriichuk Date: Thu, 27 Aug 2026 18:51:07 +0300 Subject: [PATCH 2/3] fix(workload): re-read after a settled swap; do not call a mid-swap preview up to date MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the pre-plan rollout wait, both confirmed. A terminal replacement record was returned as-is, so a swap that landed in the window between the workload read and the replacement read was never re-fetched. The plan was then built against the artifact being rolled off, awaitSteady was skipped because that snapshot still said running, and --lock made the outgoing version permanent: lock() locks result.ArtifactID, which is seeded from exactly that snapshot, and locking cannot be undone. A terminal record now re-reads. A record that says nothing is in flight still costs no second read, since it is no evidence a swap just happened. The residual is stated in the code rather than pretended away: a swap whose record is collected inside that same window reads as nil and is not re-read. Closing it would mean re-reading on every deploy, doubling the workload GET on the quiet path to catch a window narrower than the one this covers. A --dry-run during a swap left the state as running, so an empty plan printed "Already up to date" directly beneath the note saying a deploy would wait and plan against where the swap lands — a preview contradicting itself. The plan is computed against a workload the platform is already moving, so an empty one says the swap has not landed, not that there is nothing to do. The preview now reduces that to StateSettling, which is the same answer a workload halfway through stopping already gets, and settledVerdict says so. Synthesising the state is confined to the preview: every state that decides the shape of a plan already groups settling with running (actsOnState, creates), so only the verdict moves, and a real run never arrives there because it waits and re-reads instead. StateSettling's doc records that it now covers both. Co-Authored-By: Claude Opus 5 (1M context) --- internal/workload/up/run.go | 34 ++++++++- internal/workload/up/run_test.go | 120 +++++++++++++++++++++++++++++++ internal/workload/up/state.go | 12 +++- 3 files changed, 161 insertions(+), 5 deletions(-) diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index e2a1109f..1862c5ef 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -384,12 +384,27 @@ func awaitReplaced(live Live, opts Options) (Live, error) { live.WorkloadID, err) } - // A settled record stays readable for a while after the rollout ends, and - // waiting on one would block every deploy for as long as it lingers. - if active == nil || workload.IsTerminalReplacementStatus(active.Status) { + if active == nil { return live, nil } + // A settled record stays readable for a while after the rollout ends, so + // waiting on one would block every deploy for as long as it lingers. It is + // still re-read rather than returned as it stands: a terminal record means a + // swap landed recently, possibly in the window between the read above and + // this one, and the snapshot from before it names the outgoing artifact. + // Planning against that would roll a version the platform had just + // installed, and a --lock run would make the version being rolled off + // permanent, which cannot be undone. + // + // The residual, stated rather than pretended away: a swap whose record is + // collected inside that same window reads as nil above and is not re-read. + // Closing it means re-reading on every deploy, which doubles the workload + // GET on the quiet path to catch a window narrower than the one this covers. + if workload.IsTerminalReplacementStatus(active.Status) { + return Look(live.WorkloadID) + } + report := newReporter(opts.Stderr, opts.Spinner) report.say(" %s\n", tui.HintStyle.Render(replacingNote(live.WorkloadID, active))) @@ -398,6 +413,19 @@ func awaitReplaced(live Live, opts Options) (Live, error) { report.say(" %s\n", tui.HintStyle.Render( "A deploy would wait for this rollout to finish and plan against where it lands.")) + // The plan below is computed against a workload the platform is already + // moving, so an empty one is not "up to date": the swap decides what + // differs, and it has not landed. StateSettling is what says so, and it + // is the same answer for the same reason a workload halfway through + // stopping gets. Without it the preview prints "Already up to date" + // directly beneath the note above, contradicting it. + // + // Safe to synthesise because it is confined to the preview. Every state + // that decides the shape of a plan already groups settling with running + // (see actsOnState and creates), so only the verdict moves; and a real + // run never arrives here with it, because it waits and re-reads instead. + live.State = StateSettling + return live, nil } diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index e45169de..0ef3c719 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -1467,6 +1467,126 @@ func TestRun_LockWaitsOutARolloutThatWasAlreadyInFlight(t *testing.T) { assert.Contains(t, stderr, "Waiting for the rollout already in progress") } +// The narrow race the pre-plan read opens, and the reason a terminal record is +// re-read rather than returned as it stands. The swap lands between the +// workload read and the replacement read, so the route answers "completed" and +// there is nothing to wait for — but the snapshot in hand is from before the +// swap and names the artifact being rolled off. +// +// --lock is where that costs something that cannot be taken back: the lock +// lands on result.ArtifactID, which is seeded from that snapshot, so a run +// missing the re-read makes the outgoing version permanent. +func TestRun_SwapThatLandsBeforeTheRolloutReadIsStillReRead(t *testing.T) { + const ( + outgoing = "68a0000000000000000000a1" + incoming = "68a0000000000000000000b2" + ) + + var ( + looks int + locked string + ) + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { + looks++ + + d := doc(t, liveWorkloadJSON) + if looks > 1 { + // The swap has landed by the time anything re-reads. + d["artifactId"] = incoming + } + + return d, nil + }, + artifactD: func(id string) (workload.Document, error) { + d := draftArtifact(t) + d["id"] = id + + return d, nil + }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ + ID: "rep-1", ArtifactID: incoming, Status: workload.ReplacementStatusCompleted, + }, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + t.Fatal("a settled record is nothing to wait for") + + return nil, nil + }, + lock: func(artifactID string) (*workload.Artifact, error) { + locked = artifactID + + return &workload.Artifact{ID: artifactID, Status: workload.ArtifactStatusLocked}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, _, err := runIn(t, bound, Options{NonInteractive: true, Lock: true}) + require.NoError(t, err) + + assert.Equal(t, 2, looks, "a terminal record means a swap landed, so the snapshot is re-read") + assert.Equal(t, incoming, locked, + "locking is one-way, so it must land on what the swap installed, never on the version rolled off") + assert.Equal(t, incoming, result.ArtifactID) +} + +// A record that says nothing is in flight is no evidence a swap just happened, +// so the quiet path stays one workload read. Without this the fix above would +// double the read on every deploy to close a window it cannot see anyway. +func TestRun_NoRolloutRecordCostsNoSecondRead(t *testing.T) { + var looks int + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { + looks++ + + return doc(t, liveWorkloadJSON), nil + }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, _, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + assert.Equal(t, 1, looks, "nothing in flight is nothing to re-read") +} + +// A preview during a swap must not print the one verdict it cannot support. The +// plan is computed against a workload the platform is already moving, so an +// empty one says the swap has not landed yet, not that there is nothing to do — +// and "Already up to date" directly beneath "a deploy would wait" is a preview +// contradicting itself. +// +// The fixture matches the live state field for field, which is what makes the +// plan empty and is exactly the shape that used to print the wrong answer. +func TestRun_DryRunDuringARolloutDoesNotClaimUpToDate(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", ArtifactID: "68a0…b2", Status: "promoting"}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true, DryRun: true}) + require.NoError(t, err) + + assert.NotContains(t, stderr, "Already up to date", + "the swap decides what differs, and it has not landed") + assert.Contains(t, stderr, "still settling") + assert.Contains(t, stderr, "plan against where it lands") + assert.Equal(t, "settling", result.Status, + "the envelope reports the state, and a workload mid-swap is not settled") +} + // A manifest with nothing to resolve has no workload to ask about, and the // replacement route answers the same 404 for "no such workload" as it does for // "nothing in flight". Asking would be a round trip whose answer cannot be diff --git a/internal/workload/up/state.go b/internal/workload/up/state.go index 5624ef8a..2e4d7524 100644 --- a/internal/workload/up/state.go +++ b/internal/workload/up/state.go @@ -66,8 +66,16 @@ const ( // to be running. StateStopped - // StateSettling is a workload still moving under its own power. `up` - // waits for it rather than acting on a state that is about to change. + // StateSettling is a workload still moving. `up` waits for it rather than + // acting on a state that is about to change. + // + // Mostly that is a workload moving under its own power, which is what + // stateFor reads off the status. A swap already in flight is the other + // case and cannot be read that way, because a workload being replaced + // reports itself running throughout; awaitReplaced asks the replacement + // route and reduces it to this state on the one path that does not wait, + // so a preview says what a deploy would do rather than calling a workload + // mid-transition up to date. StateSettling // StateRunning is the ordinary case: reconcile against it. From 70c110d6e4e491e9ae53e629c9109b1f1c8e1e6f Mon Sep 17 00:00:00 2001 From: Volodymyr Onofriichuk Date: Mon, 31 Aug 2026 14:45:03 +0300 Subject: [PATCH 3/3] [RAPTOR-19530] fix(workload): one hint per wait, and a debug trail for the silent paths Review on #853. A --dry-run onto a workload with a rollout in flight said the same thing twice. awaitReplaced printed its note, then reduced the workload to StateSettling so the empty plan below would not be called up to date -- and awaitSteady, seeing that state under --dry-run, printed its own near-identical hint underneath: A deploy would wait for this rollout to finish and plan against where it lands. A deploy would wait for this to finish and plan against where it lands. That state is synthetic, put there for the verdict rather than read off the platform, so awaitSteady had nothing left to say about it. awaitReplaced now reports that it previewed, and lookSettled stops there. A real run never returns that flag: it waits and re-reads instead of previewing. The existing test only asserted the hint was present, which two copies satisfy. It now counts them. Two decisions in the new code were invisible even under --debug, and both are ones you would go looking for after the fact: - nothing in flight, plan against the workload as read -- the ordinary path, and the only one that prints nothing at all. Without the line, a deploy that planned against a stale artifact looks the same whether the route answered "nothing" or was never asked. - the rollout had already settled, so re-read rather than wait -- the narrow race between the workload read and the replacement read. A plan that looks like it rolled the wrong artifact is either this re-read having happened or not. Co-Authored-By: Claude Opus 5 (1M context) --- internal/workload/up/run.go | 51 +++++++++++++++++++++++++------- internal/workload/up/run_test.go | 8 ++++- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index 1862c5ef..6b5d0ca8 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -25,6 +25,7 @@ import ( "time" "github.com/datarobot/cli/internal/drapi" + "github.com/datarobot/cli/internal/log" "github.com/datarobot/cli/internal/workload" "github.com/datarobot/cli/internal/workload/ignore" "github.com/datarobot/cli/internal/workload/manifest" @@ -342,11 +343,19 @@ func lookSettled(workloadID string, opts Options) (Live, error) { return Live{}, err } - replaced, err := awaitReplaced(found, opts) + replaced, previewed, err := awaitReplaced(found, opts) if err != nil { return replaced, err } + // A preview that has already said a deploy would wait for the rollout says + // nothing further. The settling state it hands back is synthetic — put there + // so an empty plan is not called up to date — and awaitSteady would read it + // as a second transition and print the same sentence about the same wait. + if previewed { + return replaced, nil + } + return awaitSteady(replaced, opts) } @@ -371,21 +380,32 @@ func lookSettled(workloadID string, opts Options) (Live, error) { // // A dry run never waits, for the reason it never waits on a settling workload: // blocking a preview for the poll timeout is the opposite of what a preview is -// for. -func awaitReplaced(live Live, opts Options) (Live, error) { +// for. The bool it returns is that case and only that case: true means the +// preview has already said a deploy would wait here, so the caller stops rather +// than letting awaitSteady say it again about the settling state synthesised +// below. A real run never returns true, because it waits instead of previewing. +func awaitReplaced(live Live, opts Options) (Live, bool, error) { if !replaceable(live) { - return live, nil + return live, false, nil } active, err := activeReplacementFn(live.WorkloadID) if err != nil { - return live, fmt.Errorf( + return live, false, fmt.Errorf( "cannot tell whether workload %s already has a rollout in progress, so nothing was deployed: %w", live.WorkloadID, err) } if active == nil { - return live, nil + // The ordinary path, and the only one that says nothing to the user. + // Logged so a --debug transcript shows the route was asked at all: a + // deploy that planned against a stale artifact looks the same here + // whether the answer was "nothing in flight" or the question was never + // put, and those have different causes. + log.Debug("no rollout in flight; planning against the workload as read", + "workload_id", live.WorkloadID) + + return live, false, nil } // A settled record stays readable for a while after the rollout ends, so @@ -402,7 +422,16 @@ func awaitReplaced(live Live, opts Options) (Live, error) { // Closing it means re-reading on every deploy, which doubles the workload // GET on the quiet path to catch a window narrower than the one this covers. if workload.IsTerminalReplacementStatus(active.Status) { - return Look(live.WorkloadID) + // The one decision here that is neither waited on nor said out loud, so + // the debug log is the only place it can be seen. It matters after the + // fact: a plan that looks like it rolled the wrong artifact is either + // this re-read having happened or it not having happened. + log.Debug("the rollout already settled; re-reading before planning", + "workload_id", live.WorkloadID, "replacement_id", active.ID, "status", active.Status) + + refreshed, err := Look(live.WorkloadID) + + return refreshed, false, err } report := newReporter(opts.Stderr, opts.Spinner) @@ -426,7 +455,7 @@ func awaitReplaced(live Live, opts Options) (Live, error) { // run never arrives here with it, because it waits and re-reads instead. live.State = StateSettling - return live, nil + return live, true, nil } if opts.Detach { @@ -451,7 +480,7 @@ func awaitReplaced(live Live, opts Options) (Live, error) { }) if err != nil { if failure := replacedFailed(live, settled, err); failure != nil { - return live, failure + return live, false, failure } report.say(" %s\n", tui.WarnStyle.Render(fmt.Sprintf( @@ -459,7 +488,9 @@ func awaitReplaced(live Live, opts Options) (Live, error) { settled.Status))) } - return Look(live.WorkloadID) + refreshed, err := Look(live.WorkloadID) + + return refreshed, false, err } // replaceable says whether there is a workload for the replacement route to diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index 0ef3c719..617c50b2 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -1392,7 +1392,13 @@ func TestRun_DryRunWithARolloutInFlightDoesNotWait(t *testing.T) { require.NoError(t, err) assert.Contains(t, stderr, "is being replaced") - assert.Contains(t, stderr, "plan against where it lands") + assert.Contains(t, stderr, "A deploy would wait for this rollout to finish") + + // One wait pending, one sentence about it. The settling state this preview + // leaves behind is synthetic — awaitReplaced put it there so an empty plan + // is not called up to date — so awaitSteady must not read it as a second + // transition and print its own near-identical hint underneath. + assert.Equal(t, 1, strings.Count(stderr, "plan against where it lands")) } // --detach is about not waiting for the deploy to serve, and this wait comes