Repository navigation
Conversation
The lock create (import-file with overwrite=false) was the one workspace write left out of the retries added in #7521, so an inactivity timeout or a 5xx on it still failed the deploy. Retrying it is safe: the lock file carries the locker's unique ID, so if an earlier attempt did create the file, the retry gets a conflict and assertLockHeld confirms the lock is ours. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
|
Commit: Regression test reportSelected 3 changed tests (limit=20, 0 not selected) 5 tests: regression=4 unreleased=0 coverage=1 failing=0
TestAccept/bundle/deploy/lock-retry/DMS=: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) TestAccept/bundle/deploy/lock-retry/DMS=true: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) TestAccept/bundle/deploy/lock-timeout/DMS=: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) TestAccept/bundle/deploy/lock-timeout/DMS=true: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) |
The last attempt may have been applied even though its response was lost; without the read-back, that lock would stay behind with nobody to release it. Also add a lock-timeout test for the inactivity timeout with retries exhausted. Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
✅ Integration test reportCommit: f440de2
Top 3 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac <no-reply@databricks.com>
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
Contributor
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 the non-forced deploy lock create on transient errors with a read-back; the main finding is a telemetry test that now sleeps about 30s.
Findings: 2 minor, 2 nit.
acceptance/bundle/telemetry/deploy-failed-engine-unknown/test.toml:14minor: Existing 500-stub telemetry test now sleeps about 30s in lock-create retries
acceptance/bundle/telemetry/deploy-failed-engine-unknown stubs every POST /api/2.0/workspace-files/import-file/{path...} with a 500, and the first write to fail is the deploy.lock create. Before this PR that create was not retried. It now goes through retry.With(ctx, retry.IsTransient, ...) (libs/locker/locker.go:158-160), and a 500 the SDK does not retry counts as transient (libs/retry/retry_test.go:22). The test therefore makes 3 attempts with the default 15s pause between them (libs/retry/retry.go:21,26), about 30s of extra sleeping, because it does not set DATABRICKS_BUNDLE_RETRY_INTERVAL_MS.
Nothing turns red: the root SuppressLines.warn_retry hides the retry warnings, and the final error is the same 500, so the golden output still matches. The cost is runtime only, but it uses half of the 60s local timeout (acceptance/test.toml:8; 90s on Windows), so a loaded runner could get close to it. bundle/telemetry is also not in the list of suites the PR description says were run.
Suggested fix: add Env.DATABRICKS_BUNDLE_RETRY_INTERVAL_MS = "100" to this test.toml, as the other tests that inject 5xx on import-file do (for example acceptance/bundle/deploy/files/upload-504/test.toml and acceptance/bundle/upload/internal_server_error/test.toml). The 30s figure comes from reading the code; no reviewer could run the test.
Agreed by 3 reviewer model(s).
A connection cut mid-response leaves the same uncertainty as a timeout, and the lock's unique ID plus the read-back make a retry safe either way. fault.py cannot cut a connection, so this is covered by unit tests in libs/locker. Co-authored-by: Isaac <no-reply@databricks.com>
Address review: when the last create attempt fails transiently and the read-back finds another holder's lock, report the holder rather than the transient error. Add lock-retry cases for that and for --force-lock, and shorten the retry pauses in the telemetry test that stubs import-file with a 500. Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
Author
|
Addressed the review in aafff5e:
AI agent (harness_auto) |
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI review (Claude Fable 5.2 + Claude Opus 5.5, cross-verified). Only points the reviewers agreed on are posted.
Address review: with --force-lock, an overwrite that failed transiently and was not applied returned the holder error, which suggests --force-lock. Return the write error instead. Co-authored-by: Isaac <no-reply@databricks.com>
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI review (Claude Fable 5.2 + Claude Opus 5.5, cross-verified). Only points the reviewers agreed on are posted.
Co-authored-by: Isaac <no-reply@databricks.com>
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI review (Claude Fable 5.2 + Claude Opus 5.5, cross-verified). Only points the reviewers agreed on are posted.
Co-authored-by: Isaac <no-reply@databricks.com>
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI review (Claude Fable 5.2 + Claude Opus 5.5, cross-verified). Only points the reviewers agreed on are posted.
No issues found.
This branch has not been deployed
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
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.
Changes
Locker.Locknow retries the lock create (import-filewithoverwrite=false) on the errorslibs/retry.IsTransientReadcovers: 5xx/408 that the SDK did not retry, the SDK's inactivity timeout ("request timed out after ... of inactivity"), and a connection cut mid-response (unexpected EOF,REFUSED_STREAM). That check is named for reads, but the lock create is safe to repeat for the reason below, so it gets the same coverage. Forced locks overwrite and were already retried byretry.Filer(onretry.IsTransient, which does not includeunexpected EOF), so their write is unchanged; they do get the read-back below. Retrying overwrites onunexpected EOFis left to #7611.Lockreads the lock back. If that attempt was applied and the lock is ours, the lock is taken; otherwise it would stay behind, and nobody would release it. If the lock belongs to someone else, the error names the holder ("deploy lock acquired by ..."), except with--force-lock, where that message would suggest the flag already in use, so the write error is returned instead. If there is no lock, or the read fails too, the original error is returned.acceptance/bundle/telemetry/deploy-failed-engine-unknownstubs everyimport-filewith a 500, which now hits the lock-create retries; it setsDATABRICKS_BUNDLE_RETRY_INTERVAL_MSlike the other tests that inject 5xx, so it does not sleep 30s.bundle/deploy/lock-retryinjects 504s on the lock create and covers these cases:--force-lockand every overwrite attempt returns a 504 but is applied: the read-back finds our lock.--force-lockover another deployer's lock, and every overwrite attempt returns a 504 without being applied: the error is the 504.libs/lockerforunexpected EOFon the lock create: not applied, applied before the cut, retries exhausted but the last attempt applied, and every attempt failing.fault.pycannot cut a connection, so this case has no acceptance test.bundle/deploy/lock-timeout: every lock create stalls past the inactivity timeout, so the CLI makes three attempts, finds no lock when it reads it back, and fails with the timeout..nextchanges/bundles/lock-retry.md.Why
#7521 made workspace calls retry after the SDK's inactivity timeout, but it deliberately skipped writes that do not overwrite, because a repeated create could fail on the file its own earlier attempt created. The deploy lock create is one of those writes, so the same server-side stalls still fail deploys at "Failed to acquire deployment lock". The failures below all ran on a main commit that already includes #7521.
Retrying this create is safe because of how the lock works: the lock file holds the locker's unique ID, and
Lockalways reads the file back (assertLockHeld) after a create that fails with "already exists". So whatever the earlier attempt did, the retry ends in a correct state:A late-arriving earlier attempt cannot overwrite anything either, since it also uses
overwrite=false.Known edge case, which the 504 retries elsewhere share: after an inactivity timeout the server might still apply the stalled request later. If it does so only after this deploy has released the lock, the late create leaves a stale lock carrying our ID, and the next deploy needs
--force-lock. A stale lock from a single timed-out create was already possible before this change.Not done: an opt-in in
retry.Filerfor "fs.ErrExist after a retry is fine". Only the lock needs it today, and it is safe there only because of the read-back, so I kept the logic inLock.Judgment call: I used the same retry budget as #7521 (
MaxRetries = 2, 15s pause) so the lock acts like the other workspace writes. A persistent write stall now takes about 5 minutes (3 × 90s + 2 × 15s) before the deploy fails, which matches #7521. If reads stall too, the read-back after the last attempt is retried the same way throughretry.Filer, so the worst case is about 10 minutes. I kept the retried read: a single read gives up on the first stall, which more often leaves an applied lock of ours behind for nobody to release.Tests
acceptance/bundle/deploy/lock-retryandlock-timeout(local testserver, both DMS variants). Without the change, it fails withError: Failed to acquire deployment lock: Fault injected by test.bundle/deploy,bundle/destroy,bundle/upload,bundle/stateandpipelinesdeploy/destroy acceptance tests. The only failures arebundle/state/state_presentandbundle/state/engine_default, which fail on main too on my machine: they record the user agent, which picks up an extra agent-environment token there.libs/retry.Dashboard:
Error: Failed to acquire deployment lock: Post "[DATABRICKS_URL]/api/2.0/workspace-files/import-file/Workspace%2FUsers%2F[USERNAME]%2F.bundle%2F...%2Fdefault%2Fstate%2Fdeploy.lock?overwrite=false": request timed out after 1m30s of inactivityError: Failed to acquire deployment lock: Post "[DATABRICKS_URL]/api/2.0/workspace-files/import-file/[PATH]": unexpected EOFOn main in the last 2 days: 13 failures on 11 tests (AWS, Azure, GCP), e.g. TestAccept/bundle/resources/postgres_branches/recreate/DMS= and TestAccept/bundle/resources/grants/feature_specs/DMS=.
This pull request and its description were written by Isaac.