Job runs wait for completion - #6091
Open
radakam wants to merge 23 commits into
Open
Conversation
Collaborator
Integration test reportCommit: 65db56d
16 interesting tests: 8 FAIL, 3 SKIP, 3 RECOVERED, 2 KNOWN
|
radakam
marked this pull request as ready for review
July 29, 2026 14:22
Contributor
Approval status: pending
|
radakam
force-pushed
the
job-runs-wait-for-completion
branch
from
August 3, 2026 10:57
6635efd to
26506cf
Compare
radakam
commented
Aug 3, 2026
denik
reviewed
Aug 3, 2026
denik
reviewed
Aug 3, 2026
| if canReadRemoteCache { | ||
| remoteState, ok := b.RemoteStateCache.Load(targetResourceKey) | ||
| if ok { | ||
| err = adapter.CheckSettled(remoteState) |
Contributor
There was a problem hiding this comment.
as discussed: replace this with field, possibly calculated.
The fake workspace reported every run as TERMINATED SUCCESS, overwriting the FAILED state it had just recorded for a task it executed locally. A run now reports the terminal state its tasks add up to, so a failing run can be exercised end to end locally. Tasks whose code the fake workspace does not have are left successful. An immutable deployment, for example, uploads the bundle as a snapshot zip that the fake workspace never unpacks, so there is nothing to execute; that gap is in the fake workspace, not in the job under test. Originally reviewed as #6082.
Deploying a job_run triggered the run and moved on, so a resource referencing the run's outcome saw whatever state the run happened to be in. The resource now implements the framework's WaitAfterCreate hook, which blocks until the run is terminal and republishes the settled remote state; only SUCCESS lets the deploy continue. While it waits, the deploy reports the run page URL and each state change, the way `bundle run` does, since a run can take hours. A run that does not succeed fails the deploy with the failed task, the message that task reported, and a link to the run page. Bounded by 24h, matching `bundle run`. The framework saves the run id before the wait, so a run that fails stays recorded and an unchanged config plans no second run; failed_run covers that. run_page_url is now normalized to the path form that also resolves for non-admins. invariant/configs/job_run.yml.tmpl loses its notebook task, which deploying it would now actually run, and is excluded from cloud runs: a real workspace reports a condition-task-only run as SKIPPED, and a task that does succeed would add a multi-minute cluster run to every variant of a suite that asserts plan and state invariants. Still covered locally.
The wait reimplemented what `bundle run`'s monitor already does: report the run page URL once, then each state change. Both now go through progress.JobStateTracker, which decides what a poll is worth reporting; the two callers keep their own sinks, since a concurrent deploy reports plain prefixed lines where `bundle run` reports progress events. The failure the deploy reports no longer repeats the run id that the framework's wrapper already carries; what the wait adds is the link to a run that outlives it.
Excluding job_run.yml.tmpl from cloud left job_runs with no cloud coverage at all: every test under resources/job_runs inherits Cloud=false. The behaviour this milestone adds is the one that depends most on the real Jobs API, so wait_cloud triggers a run for real and reads the outcome back out of the downstream job, the way the vector search exclusion points at a dedicated test. Serverless keeps the run to about a minute, and the deploy's progress stream stays out of the golden: a real run reports an unpredictable number of intermediate states.
The ignore_remote_changes for job_parameters assumes GetRun reports every parameter the job defines, not just the ones the run overrode. Assert that on the run wait_cloud already triggers by overriding one of two parameters and planning after the deploy.
Drop the comments that only restate the code, the duplicated note about the framework prefixing the run id, and the filler in the ones that carry a reason.
Reporting every state change made a deploy's output depend on how many states the run passed through, which varies with how long its compute takes to start. That cost the cloud test its coverage: it had to grep the deploy log instead of comparing it. The full state history is still in the log. Also record that a failed run is not run again in the changelog entry.
The message the deploy names a failed task with came from an error the test server writes itself, so nothing checked that a real workspace reports one at all. Assert it does, and that we are not falling back to the states the run reports.
Also fix jobRunServer's doc, which said it returns a server when it returns a client.
Interrupting a deploy mid-wait leaves the run going, and jobs/runs/delete rejects an active run, so destroy failed and the bundle could not be torn down without cancelling the run by hand. Delete now cancels it first, and waits for the cancellation to settle since the API cancels asynchronously. The interrupt itself was reported as a timeout, blaming the 24h bound for something the user did. It now says it was interrupted, and still links the run, whose page URL is pinned to the first poll that reported one. The run left going is what the next deploy reads, and it triggers no second run, so a reference to the outcome resolves to an empty string. Recorded in a test rather than fixed here: stopping a run the user did not ask to stop is a departure from `bundle run`, which leaves interrupted runs alive.
A real workspace reports a run whose task failed as INTERNAL_ERROR in the deprecated life_cycle_state, though status.state is TERMINATED with termination code RUN_EXECUTION_ERROR. The SDK waiter halts on INTERNAL_ERROR with an error of its own, so the deploy blamed the run for an internal failure instead of naming the task that failed and the message it reported. The wait now polls for any state runIsTerminal accepts, the definition the delete path already used, and leaves the verdict to the run's result. The Jobs API retries a task that failed and reports it once per attempt, so the same task was named twice over. Only its last attempt is reported now. The fake workspace rolls a failed task up to TERMINATED FAILED, so failed_cloud was the only test that saw either of these; both are now covered by unit tests.
Interrupting a deploy mid-wait leaves the run going and its id recorded, so the next deploy plans no change for it and serves references to it from the remote state cache. An unfinished run reports an empty result_state, and that was substituted into whatever referenced it, configuring a downstream resource with the outcome of a run that had not reached one. Resources now declare when their outputs are not final yet, through an optional CheckSettled that mirrors IsGone, and reference resolution fails with what the resource reports instead of handing out the zero value. job_runs answers with runIsTerminal, the definition the wait loop already polls for, so the two cannot drift apart, and names the run and links its page. The check sits at reference resolution rather than in DoRead or the planner, so it only fires for a bundle that actually reads the outcome. A deploy that merely carries the run keeps working, as do plan, summary and destroy, while the run finishes on its own; cancelling it would be the departure from `bundle run` that this branch already declined to make.
wait_cloud inherited Local=true, so both tests deployed the same job_run against the test server and differed only in the assertions that followed. The merged test keeps wait_output's coverage that the resolved result_state tag is not perpetual drift, and reads the tag back with `jobs get` rather than from recorded requests, so that assertion holds against a real workspace as well. Recording requests stays off: the wait polls GetRun until the run is terminal, so the recorded requests depend on how long the run takes. The _cloud suffix goes with it, since the test was never cloud-only.
radakam
force-pushed
the
job-runs-wait-for-completion
branch
from
August 4, 2026 06:46
26506cf to
af3da99
Compare
…resolve it CheckSettled kept a reference to an unfinished run from resolving to an empty outcome, but it needed an adapter method of its own and left a run that failed recorded as if it were done. Record the outcome the run has to reach instead: PrepareState always sets result_state to SUCCESS, DoRead reports the outcome the run reached, and the existing drift machinery compares the two. Anything other than SUCCESS is a recreate, so a run that failed, or that an interrupted deploy left going, is triggered again on the next deploy. ignore_remote_changes can no longer be a root rule, since that would suppress the comparison, so it now lists every jobs.RunNow field. TestJobRunIgnoresEveryRequestField keeps the list in step with the SDK.
Drop the counterfactuals, keeping only what the code does and the reasons a reader cannot get from the code itself.
The exclusion was added alongside the wait itself, on the assumption that a real workspace reports a run of condition tasks alone as SKIPPED. That state means something else: a run aborted because a previous run of the same job was already active. Nothing was ever measured against a workspace, so drop the exclusion and let cloud CI say whether the run succeeds. This is the only cloud coverage of destroying and deleting a run the backend has already removed; resources/job_runs/wait covers deploy and redeploy but never re-issues a delete.
Measured on serverless against a spark_python_task that raises: the run reports INTERNAL_ERROR in the deprecated life_cycle_state, though status.state is TERMINATED with termination code RUN_EXECUTION_ERROR; both the run and the task carry a generic message pointing at the run output; and jobs/runs/get-output splits the failure into error, the exception, and error_trace, the traceback. The fake rolled a failed task up to TERMINATED, invented "task main failed", and put its own Go error into the error field. The message a deploy names a failed task with was therefore one this server wrote rather than one a workspace reports, which is what resources/job_runs/failed_cloud exists to check.
failed_cloud inherited Local=true, so both tests deployed the same failing job_run against the test server and differed only in the assertions that followed. Now that the fake reports a failed task the way a workspace does, the deploy renders identically in both places and one golden covers both: the run's INTERNAL_ERROR and its generic message are fixed strings, and the message the task reports is the exception fail.py raises. That golden replaces failed_cloud's grep over a deploy.log kept out of it, so the assertions that only held by hand-waving at the output -- the framework prefix, the failed dependency, the order of the run URL and the final state -- are pinned against a real workspace as well. The redeploy moves to failed_redeploy: a second run costs another few minutes on cloud for a recreate the test server already proves.
Only the rendered databricks.yml is absent from the test directory and has to be ignored. The template it is rendered from and the Python file the job runs are checked in and identical in the temporary directory, so listing them only hid the fact that they are compared.
…loud failed_redeploy stayed local on the grounds that a second cloud run costs another few minutes for a recreate the test server already proves. Running it against a real workspace is worth that: the recreate deletes the run that failed and triggers a fresh one, and a real Jobs API is what decides both. Request recording goes back on so the assertions failed_redeploy carried survive the merge. The cloud leg records through the proxy, which captures the same bodies the local leg sends, and the delete now names the first run, which is what shows the failed run is discarded rather than left in the workspace.
Whether an id rendered as [MY_JOB_ID] or as a bare [NUMID] depended on nothing but whether the script happened to call read_id.py for that resource, so basic named both ids, redeploy named only the job, and job_parameters named neither. Reading three neighbouring tests meant guessing which number was which. Register the ids instead, via the assignment form that adds the replacement without printing it, and let the auto-suffix distinguish the generations of a recreated run. That turns two assertions from unverifiable into checkable: redeploy and failed_run both claimed a redeploy replaces the run, but printed the deleted run and its replacement as the same [NUMID]. They now read [MY_RUN_ID] and [MY_RUN_ID_2]. [NUMID] stays for the ?o= / ?w= workspace selector, which is not a resource id.
Comparing WaitAfterCreate's return against a fresh read, and rebasing the field checks onto it, applied to every resource, but job_runs is the only one whose remote state moves while the wait blocks: the testserver settles the others on create, so both forms assert the same thing for them. Keep the original comparison for those and take the new path only for job_runs, so the change stays where it is needed. A resource whose handler later models a create-then-settle transition will fail the plain comparison and needs adding to the exception.
Comparing WaitAfterCreate's remote against a fresh DoRead held by construction:
both build the value with makeJobRunRemote from a GetRun response, and the run
is terminal by then, so the two cannot differ. It also did not check the thing
worth checking -- an early return from the wait would leave both sides equally
non-terminal -- and the drift a lossy recorded state would cause is already
covered by resources/job_runs/{basic,wait}.
What remains is not an assertion: result_state fills in only once the run
settles, so the field checks at the end of testCRUD have to read the settled
state rather than the one from just after create.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.