Job runs wait for completion - #6091
Open
radakam wants to merge 13 commits into
Open
Conversation
Collaborator
Integration test reportCommit: af3da99
8 interesting tests: 5 RECOVERED, 3 SKIP
Top 6 slowest tests (at least 2 minutes):
|
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
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.
Why
The experimental
job_runsresource returned as soon as it triggered a run, so a resource referencing the run's outcome saw a state from a moment afterrun-now, and a run that failed left the deploy reporting success.What
WaitAfterCreateblocks until the run is terminal, and onlySUCCESScontinues the deploy. Anything else fails it, naming the outcome, each task that caused the failure with the error that task reported, and a link to the run page. A failed run stays recorded, so the next deploy does not run the job again. While waiting the user gets the run page URL and every state change, through aprogress.JobStateTrackernow shared withbundle run.Interrupting a deploy mid-wait leaves the run going. Destroy cancels a run that has not finished, since
jobs/runs/deleterejects one, and the error says the wait was interrupted rather than blaming the 24h timeout. That run is also what the next deploy reads, and its outcome is still empty, so an optionalCheckSettled(mirroringIsGone) lets a resource declare its outputs unsettled: a reference to one fails with the run and a link to its page.Two supporting changes:
JobRunPageURLmoved tolibs/workspaceurlsfor the second caller, and the fake workspace rolls task outcomes up into the run state so a failing task fails the run, with a task whose code it never uploaded succeeding instead, since that gap is the server's and not the job's.Tests
Unit tests cover success,
FAILED, a named failing task,SKIPPED,INTERNAL_ERROR, an interrupted wait, polling past non-terminal reads, delete cancelling an unfinished run, andCheckSettledboth directly and through reference resolution.wait_outputandfailed_runcover the deploy against the test server;wait_cloudandfailed_cloudrun the job for real.