[RAPTOR-19755] feat(workload): pull an existing workload's code before up deploys from an empty dir - #856
Conversation
|
🎫 Jira: |
f39805e to
bf02da4
Compare
…om an empty dir Binding to an existing source-built workload has no download step: config writes the live spec into the manifest, never the code the artifact was built from. So up run from a directory that does not already hold that code rolled a new version from whatever was there — an empty tree became an empty artifact that failed the build with 'cannot detect project runtime', minutes in and after the workload had been started. up now pulls the workload's current code into an empty project directory before it reads the working tree, so the deploy builds from the real thing. A directory that already looks like a project is deployed as-is; one that holds files that are neither a recognised project nor linked to the workload is refused rather than allowed to overwrite the workload's code. Only runs that mint a version look at this; retune, start, and unchanged runs are untouched. Pull-only for now: the redundant rebuild of identical code on the first deploy after a pull is a follow-up (seed the sync base to skip it). RAPTOR-19755 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bf02da4 to
60374aa
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 60374aa. Configure here.
| if err := downloadInto(client, codeRef, dir, path); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Failed pull leaves a deployable tree
High Severity
pullCode writes catalog files in place and downloadInto leaves a truncated file on failure, with no rollback of earlier files. A retry then sees a project marker, skips seeding, and deploys the partial tree as if it were the workload's code.
Additional Locations (1)
Triggered by project rule: Bugbot Rules for DataRobot CLI
Reviewed by Cursor Bugbot for commit 60374aa. Configure here.
| if err := downloadInto(client, codeRef, dir, path); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Pull skips case-collision checks
Medium Severity
pullCode validates each path with SafeRelPath then writes with os.Create, but never runs fileops.DetectCaseCollisions. On macOS and Windows two catalog keys that differ only by case silently overwrite one another, so the seeded tree is missing files the sync engine would have refused to clobber.
Triggered by project rule: Bugbot Rules for DataRobot CLI
Reviewed by Cursor Bugbot for commit 60374aa. Configure here.
Code OwnershipWorkload Cli
Review requested from the teams above. Labels will be removed automatically upon approval. |
ajalon1
left a comment
ajalon1
left a comment
There was a problem hiding this comment.
Reviewing the seed logic that pulls an existing workload's code before up deploys from an empty directory. Two issues in seed.go below; one note on already-merged wizard code (PR #848) that this PR exercises.
Not inline-anchorable (already-merged PR #848, model.go): acceptDirectory rebuilds the draft on a directory change but does not reset f.nameGiven. On back-navigation (pick candidate → type name on screenName → Escape → pick a different candidate), the new directory's suggested name is shown as a pre-filled value instead of a placeholder, and pressing Enter silently names the workload after the directory (e.g. "src"). The nameGiven field exists specifically to prevent this. Fix: f.nameGiven = f.answers.Name != "" inside the chosen != f.detected.Dir branch. Worth a follow-up since the seed logic relies on the wizard's directory screen.
| file, err := os.Create(dst) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot create %s: %w", path, err) | ||
| } | ||
|
|
||
| defer func() { _ = file.Close() }() | ||
|
|
||
| if _, _, err := client.DownloadFile(codeRef.CatalogID, codeRef.CatalogVersionID, path, file); err != nil { |
There was a problem hiding this comment.
[P1] Partial download left on disk breaks retries and can corrupt the build
os.Create truncates the file, and on a mid-stream DownloadFile error the function returns without removing the partial file (the deferred Close only closes it). On retry, if the partial is a root marker (e.g. a half-written pyproject.toml), SuspectDir() returns false, seeding is skipped, and the deploy builds from the corrupt file; if it's not a root marker, looksEmpty returns false and the run is refused with a misleading error the user can only resolve by manually deleting files. Fix: os.Remove(dst) on the error path, or write to a temp file and rename on success.
👨🏽🚀 Also, honestly, couldn't you reuse the code from internal/workload/sync/downloadOne? You'd have to extract it somewhere shared and somehow not use *Engine.
There was a problem hiding this comment.
Per the 🤖 :
func DownloadFiles(client filesapi.Client, dir, catalogID, versionID string, files []FileAction) error
func DownloadOne(client filesapi.Client, dir, catalogID, versionID string, fa FileAction) error| return err | ||
| } | ||
|
|
||
| files, err := filesClientFn().AllFiles(codeRef.CatalogID, codeRef.CatalogVersionID) |
There was a problem hiding this comment.
[P2] Unnecessary AllFiles call can block deploys of recognized projects
seedFromLiveArtifact calls artifactCodeRef (GetArtifact) and AllFiles before seedLocalDir checks SuspectDir(). For a recognized-but-unlinked project (has a Dockerfile/pyproject.toml, not linked), seedApplies is true so both network calls fire, then seedLocalDir returns nil immediately — the AllFiles result is discarded. AllFiles is a new network dependency on a path that previously had none; a transient failure blocks a deploy that needed no seeding. Fix: check SuspectDir() / looksEmpty before the network calls.
…he local dir, and reject case collisions Addresses the review on datarobot-oss#856. - Atomicity (P1/High): downloadInto removes the partial file on any download/close failure and verifies the streamed bytes against the catalog's size and hash. A half-written root marker no longer survives to make a retry skip seeding and build from a corrupt tree. Mirrors the sync engine's downloadOne. - Ordering (P2): seedFromLiveArtifact now decides from the project directory (already-a-project / refuse / empty) before reading the live artifact, so a transient files-API failure cannot block a deploy that never needed seeding. The GetArtifact + AllFiles reads happen only on the empty-directory pull path. - Case collisions (Medium): pullCode runs fileops.DetectCaseCollisions before writing, so two catalog keys differing only by case are refused up front instead of silently overwriting one another on macOS/Windows. Adds four unit tests: partial-file cleanup, size-mismatch rejection, case-collision refusal, and no-artifact-read for a recognised project. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…code pull Follow-up to the review: rather than seed.go carrying its own verified, partial-safe downloader, extract the sync pipeline's as the shared implementation both call sites use. - sync: downloadOne/downloadFiles keep their *Engine signatures as thin wrappers over new exported DownloadOne/DownloadFiles that take a filesapi.Client and a destination dir instead of reaching into *Engine. No behaviour change for the pipeline; the parallel walk, panic recovery, size/hash verification, and partial-file cleanup are unchanged and still covered by the sync tests. - up/seed: pullCode converts the catalog's FileMeta set to []FileAction and calls sync.DownloadFiles, deleting its own downloadInto/ verifyDownload (~90 lines). The seed-specific guards stay: SafeRelPath with a workload-framed message and the case-collision refusal before any write. The test fake's pulled slice is now mutex-guarded because the reused downloader pulls in parallel. task lint clean on all GOOS; go test -race green across the workload tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oad can't seed an incomplete tree Self-review follow-up on the code pull. - Set-level atomicity: the reused sync downloader pulls in parallel and stops on the first error, but the files that already landed stayed on disk. If that subset held a root marker (pyproject.toml arrived, app.py did not), the next run's SuspectDir() went false, seeding was skipped, and the deploy built from the incomplete tree — the exact failure the pull exists to prevent. The per-file cleanup did not cover it. pullCode now snapshots the directory before the pull and, on any failure, removes every entry the pull added, returning the (previously empty) directory to its prior state so the retry pulls the whole set again. - Refusal message no longer claims the workload "already has code this run would replace": after gating on the local directory before the network, that is unread on this path and false for an artifact built from an empty tree. It now states only what the deploy would do. - pullLiveCode builds the files client once and threads it into pullCode instead of each constructing its own. Adds TestSeed_RevertsAPartialPullOnFailure. task lint clean on all GOOS; go test -race green across the workload tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
RATIONALE
Binding to an existing source-built workload has no download step.
dr workload configwrites the live spec into.datarobot.yaml, but never the code the artifact was built from — sodr workload upfrom a directory that doesn't already hold that code rolls a new version from whatever is there. An empty directory becomes an empty artifact that fails the build with "cannot detect project runtime", minutes in and after the workload has already been started. Found live (RAPTOR-19755): a deploy synced a single.drignoreand 422'd at build time.Ticket: RAPTOR-19755
CHANGES
up(a roll, or a create from source —plan.MintsVersion()), and only then,uplooks at the project directory before it reads the working tree:filesapidownload +ExtractCodeRef; no new API surface.Scope / follow-up
Pull-only for now. After a pull, the immediately-following deploy rebuilds an identical version once (the working tree matches what's already running). Seeding the sync base to skip that redundant rebuild is a deliberate follow-up — it means reaching into the sync engine's three-way state, which is not worth perturbing in the same change. Not done here: an
up-level integration test that drives a full source roll — the roll fixtures are all published-image based, and the seed logic itself is covered by six unit tests over the decision tree, with theMintsVersiongate proven by the existing roll/retune suite passing unchanged.🤖 Generated with Claude Code
Note
Medium Risk
Changes the core
upapply path and writes to the project directory from remote code before deploy; mistakes could block deploys or pull the wrong tree, though scope is narrow and failures happen before workload mutation.Overview
Fixes RAPTOR-19755: binding with
dr workload configonly writes the manifest, sodr workload upfrom an empty or wrong directory could mint a version from that tree and fail late (or overwrite live code).On version-minting runs only (
plan.MintsVersion()),applynow callsseedIfMintingbefore starts, rolls, or other mutations. For source-built, bound, unlinked projects it resolves the live artifact’s code catalog via existingfilesapi/ExtractCodeRef, then either downloads into a directory that looks empty (CLI residue like.datarobot.yaml/.drignorestill counts as empty), leaves a directory that already looks like a real project, or refuses with a clear error when unrelated files would replace the workload’s code. Retune, start-only, published-image, linked, and artifact-without-code cases are no-ops.Adds
seed.goand six unit tests over that decision tree.Reviewed by Cursor Bugbot for commit 60374aa. Configure here.