Repository navigation
Conversation
The Apps API rejects update and delete with 400 BAD_REQUEST while the app compute is in a transitional state, and the compute can enter UPDATING on its own shortly after a create or update returned. On such a 400, wait for the compute to settle and retry. Co-authored-by: Isaac <no-reply@databricks.com>
cloud: TestAccept/bundle/apps/job_permissions TestAccept/bundle/invariant/no_drift/DMS=true/INPUT_CONFIG=app.yml.tmpl Co-authored-by: Isaac <no-reply@databricks.com>
…h started state Retry on the specific 400 messages even if the compute settled before the first read, use retry.With for the attempt limit and pause, cap each wait at 20 minutes, treat an app gone while waiting as deleted, and refresh the started state used by lifecycle after waiting. Add a DoUpdate test. cloud: TestAccept/bundle/apps/job_permissions TestAccept/bundle/invariant/no_drift/DMS=true/INPUT_CONFIG=app.yml.tmpl Co-authored-by: Isaac <no-reply@databricks.com>
✅ Integration test reportCommit: 489b214
Top 6 slowest tests (at least 2 minutes):
|
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
There was a problem hiding this comment.
🤖 AI review from review-factory: 3 independent reviewer models (Claude Opus 5.5, Claude Sonnet 5.5, Claude Fable 5.2) with cross-verification. Only findings that reviewers agreed on are posted.
Retries direct-engine app update and delete after waiting for compute to leave UPDATING/STARTING/STOPPING; reviewers raised only minor test gaps and a silent wait.
Findings: 3 minor, 2 nit.
| } | ||
| // The plan-time compute state is stale if the compute moved while we waited. | ||
| if settled != nil { | ||
| alreadyStarted = !isComputeStopped(settled) |
There was a problem hiding this comment.
minor: The started-state refresh after a settle wait has no test
After a settle wait, doUpdate sets alreadyStarted = !isComputeStopped(settled) (app.go:220-223) and passes it to manageLifecycle (app.go:235). That value decides whether Apps.Start, Apps.Stop or a plain deploy happens, so a wrong value issues an unneeded stop or skips a needed start. No test reaches it. TestAppDoUpdate_RetriesWhileComputeSettles (app_test.go:379-380) passes an AppState without Lifecycle and a PlanEntry with no remote state, so manageLifecycle returns at app.go:255-257 before it reads the flag. Deleting or inverting the three lines leaves every test green. A search of app_test.go for Lifecycle, /start, /stop, STARTING and STOPPING finds nothing.
Suggested cases for the update test:
- Plan-time remote
started=true, the first update rejected with a STOPPING message, GET returns STOPPING then STOPPED, andlifecycle.started: true. Assert thatPOST /api/2.0/apps/{name}/startis called. This also exercises a non-UPDATING alternative of the message regex. - Plan-time
started=true, the compute settles to STOPPED, and the config hasstarted: false. Assert that no/stopcall is made. - The mirror case: plan-time STOPPED with
started: true, the app settles to ACTIVE. Assert that no/startcall is made.
There is also no unit test for an update whose app disappears (404) during the wait. That path returns the wrapped app is gone: ... error (app.go:406) rather than nil, unlike DoDelete, which checks errAppGoneWhileSettling.
Agreed by 3 reviewer model(s).
| if !isComputeTransitionalStateError(err) { | ||
| return false | ||
| } | ||
| settled, waitErr = r.waitForComputeToSettle(ctx, name) |
There was a problem hiding this comment.
minor: The compute wait runs silently inside retry.With's check, and retry.With then adds another 15s sleep
waitForComputeToSettle runs inside the check callback of retry.With (app.go:395-400), and nothing in app.go:392-438 logs before or during the wait. retry.With evaluates check first, then calls log.Warnf(ctx, "%s", retryMessage(err)), then sleeps interval(ctx) (libs/retry/retry.go:101-114; default 15s at retry.go:26). A bundle deploy or destroy that hits this path can therefore show no output for up to computeSettleTimeout (20 minutes) per wait, with up to two waits per call, which looks like a hang. The warning that follows is retrying after 400 Bad Request from DELETE /api/2.0/apps/<name> (retry.go:128). It names only the status code and endpoint, not the compute state, and appears after the wait has finished. The fixed 15s sleep is then taken even though the compute has already settled.
Suggestions:
- Log one line before the wait, for example
app %s compute is %s; waiting up to 20m before retrying, so users can tell why the command has paused. - Optionally, do the wait outside
checkso the message comes first and the redundant sleep goes away. Keepingretry.Withand its pause is also acceptable.
Acceptance tests suppress Warn: retrying after lines (acceptance/test.toml:32), so a separate message should not change golden output.
Agreed by 3 reviewer model(s).
| if apierr.IsMissing(waitErr) { | ||
| return nil, fmt.Errorf("%w: %w", errAppGoneWhileSettling, waitErr) | ||
| } | ||
| return nil, errors.Join(err, waitErr) |
There was a problem hiding this comment.
minor: The paths where the wait fails or the retries run out are untested
retryWhileComputeSettles has several outcomes after a wait fails or the API keeps rejecting the call, and only the 404 case is tested (the delete case "treats an app gone while waiting as deleted"). Not covered:
- The wait fails with something other than 404, such as a GET error or the 20-minute timeout. The function returns
errors.Join(err, waitErr)(app.go:404-409). Callers inbundle/direct/apply.goclassify the returned error withapierr.IsMissing,errors.Is(err, apierr.ErrPermissionDenied)andretry.IsTransient(apply.go:134-142 Recreate, 174-179 Update, 302-318 Delete). Which of the two joined errors they see matters.retry.IsTransient(libs/retry/retry.go:45-58) checksisInactivityTimeoutbefore the first*apierr.APIErrorin the chain, so a joined SDK inactivity timeout from the GET can make the outerretry.WithinUpdate/Recreaterun the wholeDoUpdate/DoDeleteagain, even though the 400 would not be retried. - The API keeps returning the transitional 400 on all attempts. The last 400 is returned as is. The existing delete cases fail the first DELETE and succeed afterwards (app_test.go:334-342), so exhaustion is never reached.
- An update, as opposed to a delete, whose app disappears during the wait.
doUpdatereturns theapp is gone: ...error, which wraps a 404.
A unit test for the first two would pin down what the apply layer receives. The first needs a GET handler that returns 500 on the poll. The second needs a DELETE handler that always returns the UPDATING message while GET returns ACTIVE.
Agreed by 3 reviewer model(s).
| // its compute is in UPDATING state" and "Cannot delete app X as it is not terminal with state | ||
| // UPDATING". DELETING is left out: a delete of a DELETING app is handled by IsGone and | ||
| // deleteConfirmedGone, and waiting would not help. | ||
| var computeTransitionalStateError = regexp.MustCompile(`(?:is in|with state) (?:UPDATING|STARTING|STOPPING)\b`) |
There was a problem hiding this comment.
nit: The STARTING/STOPPING message matches have no cited backend message and no test
The comment at app.go:369-373 quotes only the two UPDATING messages ("compute is in UPDATING state" and "not terminal with state UPDATING"), which come from the dashboard errors. The regex is (?:is in|with state) (?:UPDATING|STARTING|STOPPING)\b (app.go:374). All retry cases in app_test.go (lines 254-255 and 283-354) use the UPDATING message, or a DELETING / "Invalid app name." negative case. The STARTING|STOPPING alternatives are therefore unverified against the real backend wording, yet the PR description and the changelog fragment say they are retried. Waiting on those states in isComputeTransitional is reasonable, but matching an error message needs a real message to match. Either quote an observed STARTING/STOPPING 400 in the comment and add a table case for it, or narrow the regex to what has been observed. AGENTS.md asks to avoid speculative flexibility.
Agreed by 3 reviewer model(s).
| wantGets: 1, | ||
| }, | ||
| { | ||
| name: "treats an app gone while waiting as deleted", |
There was a problem hiding this comment.
nit: The "app gone while waiting" case implies zero GETs, but one GET is made
In the test helper, if len(states) == 0 returns a 404 at app_test.go:263-268 before *gets++ at :270. The "app gone while waiting" case (app_test.go:306-310) sets wantDeletes: 1 and no wantGets, so the expected count defaults to 0 even though waitForComputeToSettle makes one GET. The test still passes for the right reason, because without the GET DoDelete would return the 400. But the count reads as "no GET happened". Please count the 404 response too and set wantGets: 1.
Agreed by 3 reviewer model(s).
Changes
In the direct engine, app update (
CreateUpdate) and delete now retry when the Apps API rejects them with 400 BAD_REQUEST because the compute is in a transitional state ("compute is in UPDATING state", "not terminal with state UPDATING"; STARTING and STOPPING are matched too). Before each retry the CLI polls the app until the compute leaves UPDATING/STARTING/STOPPING (at most 20 minutes per wait). The attempt limit and pause come fromlibs/retry.With.IsGoneand the apply-timedeleteConfirmedGonealready handle it.lifecycleuses, because the plan-time value may be stale.Why
The Apps API rejects update and delete while the app compute is in a transitional state, and the compute can enter UPDATING on its own a few seconds after a create or update returned. Cloud runs on main hit this in two tests: the second deploy right after the create fails, and so does the destroy that follows.
Decision: the retry is triggered by the error message, not only by the compute state read afterwards. Otherwise a compute that settled before the follow-up read would not be retried, and that is exactly when a retry succeeds. Other 400s fail at once, without an extra GET. #7460 adds a similar wait for
bundle run; the two could share a helper once both land.Tests
bundle/direct/dresources/app_test.go: delete and update retry after UPDATING, a delete is retried when the compute settled before the first read, a delete succeeds when the app is gone while waiting, and DELETING and other 400s are not retried.bundle/apps,bundle/resources/apps,bundle/run/app*,bundle/invariantpass with no output changes.testme-aws:bundle/apps/job_permissions/DMS=trueandbundle/invariant/no_drift/DMS=true/INPUT_CONFIG=app.yml.tmplpass. The race is rare (2 failures each in 14 days), so one run doesn't reproduce it; the last commit names these tests for the PR's cloud run.Dashboard
Error: cannot delete resources.apps.[NAME]: deleting id=test[UNIQUE_NAME]: Cannot delete app test[UNIQUE_NAME] as it is not terminal with state UPDATING, and was updated less than 20 minutes ago. Please wait before trying again. (400 BAD_REQUEST)Error: cannot update resources.apps.[NAME]: updating id=app-[UNIQUE_NAME]: Cannot update app app-[UNIQUE_NAME] as its compute is in UPDATING state. App compute needs to be ACTIVE or STOPPED to update. (400 BAD_REQUEST)Tests: TestAccept/bundle/apps/job_permissions/DMS=true, TestAccept/bundle/invariant/no_drift/DMS=true/INPUT_CONFIG=app.yml.tmpl/READPLAN=
This pull request and its description were written by Isaac.