[RAPTOR-18976] feat(workload): start a stopped workload

[wojtekwdr]

Sixth of the dr workload up stack, on top of #764.

What

up means make the file true, and a workload the file describes exactly but which is switched off is not true yet. It was refused with ErrNotWired; it is now one POST and a wait.

The plan already understood this case — it reports the run as started and refuses to call a stopped workload up to date — so this is the apply catching up with what was already being printed.

Decisions worth a look

• Stopped leaves the list of undeployable states, which is now only the states that genuinely are not a starting point: gone, terminated, errored, still settling. Stopped never belonged with them; it is one call away from running.
• The start branch is checked before the roll refusal, so a workload the file agrees with gets deployed rather than turned away.
• When the file asks for more than a start, the refusal wins and nothing is started. Bringing the workload up on the version it was stopped on and then reporting a failure leaves the user worse off than refusing did: a running workload they did not ask for, on code they did not deploy.
• Nothing is written back and nothing is compiled on this path, because nothing is being changed. The workload keeps the artifact it was stopped on, which is the version this run just confirmed the file still describes.
• --detach and --lock behave as they do on a create, since both are about what happens after the workload is asked to run, not about how it was asked.

Test plan

• task test — no new failures
• task lint — clean on all three GOOS targets

Made with Cursor

[datarobot-pr-review-router]

🎫 Jira: RAPTOR-18976 — 5. dr workload up: plan against live state, apply only the difference

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit e443264. Configure here.}

[wojtekwdr]

/approve-smoke-tests

[wojtekwdr]

/approve-smoke-tests

[ajalon1]

🧑‍🏫 Coached Review — PR #765

Verdict: Approve. Clean, well-tested addition that correctly routes a stopped workload through a single POST /start instead of refusing it. The credential check repositioning is the subtlest change and it's right.

---

How this fits in the up deploy pipeline

The dr workload up pipeline has four planned phases: plan → build → roll → retune. This PR fills a gap: a workload that is stopped (not missing, not errored, not unsettled) is a workload the file already describes correctly — it just isn't running. Before this PR, up refused it with "not wired yet." After this PR, up starts it with one POST /workloads/{id}/start and waits for it to serve.

The start path is the only apply path that changes nothing about the spec: no artifact is built, no runtime is retuned, no binding is written back. The plan says "~ workload started" and the action is ActionStarted.

Sibling PRs in this stack wire the other phases: building images from Dockerfiles, rolling new artifact versions, and retuning runtime settings live.

---

Suggestion (S2)

S2-1: verifyCredentials now runs on the refusal path too — not wrong, but the comment should acknowledge it

File: internal/workload/up/run.go:185-189

Impact: A stopped workload with drift (e.g., a runtime change the code can't apply yet) will now hit the credential check before the ErrNotWired refusal. If a credential is bad, the user sees that error first instead of the "not wired" error. This is actually better — catching a bad credential early saves a round-trip — but the old comment ("Last check before the first mutation") was removed and the new comment only speaks to the start case. Someone reading this later might wonder why credentials are verified for a path that's about to be refused.

The code is correct; the comment could be more precise about covering both the start path and the refusal path.

🤖 mechanism

Before this PR, the call order in apply() was:

deployable() → [roll refusal] → [build-mode refusal] → verifyCredentials() → create()

verifyCredentials was right before create() because create() was the only mutation that could reach it. The start path and the refusal paths both returned before reaching it.

After this PR:

deployable() → verifyCredentials() → [start check] → [roll refusal] → [build-mode refusal] → create()

verifyCredentials moved up to cover the start path. But it now also fires for the refusal paths (roll, build-mode). The comment at line 185-187 says "Starting counts: it is when the container resolves its references…" which frames it as a start-path concern, but it's actually a general gate now. The code is correct — verifying credentials before any mutation (or before refusing a mutation that could be fixed) is right — but the framing undersells the generalization.

Suggested comment update:

// Credentials are verified before any mutation, and also before the
// roll/retune refusals, so a bad reference is caught in one run rather
// than two. A credential deleted while the workload was off would
// otherwise surface as a container that will not come up, minutes later.

---

Nit (N1)

All nits are in the collapsed section below.

🤖 Nits (3 items)

N1-1: startable() doc comment says "one off status" instead of "one of status"

File: internal/workload/up/run.go:273

// startable refuses the one off status that cannot be started.

Should be "one of status" or "the one status." "One-off" reads as "ad-hoc / special," which isn't the intent.

N1-2: TestRun_StartDoesNotRelockALockedArtifact has unreachable return nil, nil after t.Fatal

File: internal/workload/up/run_test.go (in the lock: fake)

lock: func(string) (*workload.Artifact, error) {
    t.Fatal("the artifact this workload runs is already locked")
    return nil, nil  // unreachable
},

Same pattern appears in other test fakes with t.Fatal + dead return. This is consistent with existing test style in the file (e.g., the create: fake in TestRun_AlreadyUpToDate), so it's a file-level convention, not a new issue.

N1-3: stoppedCredWorkloadJSON and credArtifactJSON fixture constants are grouped with the test function that uses them, but declared as a const block with two entries

File: internal/workload/up/run_test.go

The grouping is fine — the fixtures sit right above TestRun_StartVerifiesCredentialsFirst — but they're a const block containing two multi-line JSON strings. Go conventionally uses var for large composite literals and const for scalars. Not a correctness issue; both work because they're string literals.

---

Praise

• The start vs. roll ordering is exactly right. Checking ActionStarted before the roll refusal means a stopped workload whose file agrees with it gets deployed rather than turned away. And when the file asks for more than a start, the refusal wins — refusing to start on the old version and then fail is the correct call. This is a subtle ordering constraint and the code and comments both nail it.

• The startable() guard against suspended workloads is well-judged. The platform silently no-ops a start on a suspended workload, which would mean polling until timeout for a transition that never comes. Refusing early with an actionable message ("find out why it was suspended") is much better UX than a timeout.

• TestRun_StartVerifiesCredentialsFirst is the standout test. It sets up a stopped workload whose only "drift" is a credential that 404s, asserts the plan action is ActionStarted (confirming the test is on the start path, not the roll path), and then asserts the credential error fires before the start fake is ever called. This is a precise, single-responsibility test.

• The startingStatus() helper for detached starts avoids a confusing status report. Without it, a detached start would report result.Status = "stopped" (the live status before the start) alongside result.Action = "started", which reads as contradictory. Replacing it with "submitted" makes the envelope self-consistent.

• Removing State.Deployable() was the right cleanup. That method existed solely to exclude StateStopped from being deployable. Now that stopped is deployable (via start), the method had no remaining callers and removing it prevents future misuse.

[ajalon1]

🔒 Security Review — PR #765 (STRIDE)

Result: No exploitable security findings.

What was checked

STRIDE Category	Checked	Findings
Spoofing	Identity/API auth in the new start path	None — StartWorkload uses the same authenticated client as all other operations
Tampering	Injection, path traversal, mass assignment	None — no user-controlled input flows into the workloadID parameter (it comes from the platform's own document via Look()); no file writes; no string interpolation into URLs or shell commands
Repudiation	Missing audit trail	N/A — no audit log changes in this diff
Information Disclosure	Secrets, tokens, PII in output/logs	None — verifyCredentials only checks 200 vs 404, never fetches or prints credential values. The Status field carries platform status strings (e.g., "stopped", "suspended"), not secrets. The acknowledgement message from WorkloadOperationResponse.Status is a platform-generated human-readable string (e.g., "Proton is already running"), not user data.
Denial of Service	Resource exhaustion, hangs	Excluded (no business impact) — the start path is a single POST with a configurable poll timeout
Elevation of Privilege	Missing authorization checks	None — no authz gates added or bypassed. The start operation uses the same permission model as create/roll.

Key observations

1. Credential verification is safe. verifyCredentials calls getCredentialFn(ref.CredentialID) which does a GET /credentials/{id}. It checks for 200 (exists) vs 404 (missing). It never reads or prints credential values. The function was simply repositioned in apply() — it existed before this PR and the implementation is unchanged.

2. The workloadID in startWorkloadFn(result.WorkloadID) is not attacker-controlled. It originates from live.WorkloadID, which is set by Look() from the platform's own workload document. A manifest's workloadId: field tells Look() which workload to fetch, but the platform returns the ID it knows — a mismatched ID would result in a 404 from getWorkloadDocFn, which surfaces as StateMissing, which deployable() refuses before any mutation.

3. No new file paths, subprocess calls, or shell commands. The entire diff operates at the API-client layer. No os/exec, no filepath.Join with external input, no template rendering into shell contexts.

4. The Status field addition in Live carries a platform-originated string. It's used only for comparison (live.Status != workload.WorkloadStatusSuspended) and for the startingStatus() switch. It's never interpolated into a URL, file path, or command. No XSS/ injection surface.

5. No hardcoded secrets, tokens, or keys were introduced in this diff.

[chasdr]

lgtm, approve. one net-new thing plus a couple of notes.

the only start-path --lock test (TestRun_StartDoesNotRelockALockedArtifact) runs against liveArtifactJSON, which is already "status":"locked", so it only exercises the new skip branch. is the actual lock-on-start covered anywhere? i.e. unlocked artifact + --lock -> lockArtifactFn fires -> result.Locked flips? couldn't find it. worth a fixture with an unlocked artifact asserting the lock fake got called.

on bugbot's "misleading stopped-drift refusal" - think that's already handled? unwired() rewrites the subject to "a stopped workload, which is also not being started" for both the roll and the runtime-update refusal, so it doesn't claim it's running. reads like bugbot missed that hunk. dismissable unless i'm misreading.

+1 to aj on the verifyCredentials move. one nuance on top of his reword: on a stopped-with-drift or build-mode workload with a bad cred, it now surfaces the credential error instead of ErrNotWired, so the cred problem shadows the "not wired" message. flip side of catching it early, not a blocker.

[chasdr]

Reviewed — LGTM.

[chasdr]

the lock-on-start test from yesterday still isn't in here. TestRun_StartDoesNotRelockALockedArtifact runs against liveArtifactJSON ("status":"locked"), so nothing exercises lockArtifactFn firing on the start path. TestRun_LockHappensAfterTheWorkloadServes covers it on the create path. is that enough, or worth a stopped+unlocked fixture asserting the lock fake fired and result.Locked flips? not a blocker either way.

[chasdr]

reviewed at current head. detailed notes are in the comment above.