Repository navigation
Revert bundle file uploads to import-file and replace nodes of the other type - #7218
Merged
Sankalp-Mittal merged 18 commits intoOct 29, 2026
Merged
Conversation
Since v1.21 the CLI decides itself whether a source file is a notebook (exact first-line header). The legacy import-file endpoint used up to v1.20 imports a file as a notebook if its first line contains "Databricks notebook source", and always imports .Rmd as an R notebook. Files where the two disagree break deployments that switch between CLI versions, which is the error reported in #7216. Add acceptance tests, with Badness, for the CLI side: - near-miss headers (BOM, trailing space, ...) are uploaded as files - .Rmd is uploaded as a file - syncing over a legacy-created notebook fails with "file already exists" - the legacy endpoint fails over a file synced by this CLI with RESOURCE_ALREADY_EXISTS: Node named '...' already exists and a Cloud test documenting /workspace/import: AUTO's 200-byte, any-line marker rule, R_MARKDOWN, and that overwrite never changes an object's type. The testserver now mirrors the legacy and AUTO classification rules, the R_MARKDOWN format and the collision responses observed on a real workspace. Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
|
Commit: Regression test reportSelected 20 changed tests (limit=20, 37 not selected) 64 tests: regression=58 unreleased=0 coverage=6 failing=0
TestAccept/air/run-submit-deps: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/air/run-submit-overrides: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/apps/app_yaml/DMS=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/apps/app_yaml/DMS=true: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/ai_runtime_code_source/DMS=/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/ai_runtime_code_source/DMS=/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): TestAccept/bundle/artifacts/ai_runtime_code_source/DMS=true/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): TestAccept/bundle/artifacts/ai_runtime_code_source/DMS=true/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): TestAccept/bundle/artifacts/artifact_upload_for_workspace/DMS=/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/artifact_upload_for_workspace/DMS=/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/artifact_upload_for_workspace/DMS=true/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/artifact_upload_for_workspace/DMS=true/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/artifact_upload_with_no_library_reference/DMS=/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/artifact_upload_with_no_library_reference/DMS=/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/artifact_upload_with_no_library_reference/DMS=true/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/artifact_upload_with_no_library_reference/DMS=true/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/upload_multiple_libraries/DMS=/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/upload_multiple_libraries/DMS=/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/upload_multiple_libraries/DMS=true/READPLAN=: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) TestAccept/bundle/artifacts/upload_multiple_libraries/DMS=true/READPLAN=1: branch ✅ | main (e9687c1) ❌ | latest (v1.22.1) ❌main (e9687c1): latest (v1.22.1): same output as main (e9687c1) 38 more failure details omitted, see the full report in the run. |
Keep a single SQL notebook with a byte order mark. One bundle test covers both version-switching failures, and the endpoint test covers AUTO vs RAW and that overwrite can't change an object's type. Drop the other header variants, .Rmd and AUTO's 200-byte rule, along with the testserver R_MARKDOWN/.Rmd handling and the ruff exclusion they needed. Co-authored-by: Isaac <no-reply@databricks.com>
Use a unique bundle name and clean up the workspace folder so the test can run against a real workspace, and drop generic comments from test.toml. Both tests pass on aws-cli with the recorded outputs. Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 640e02f
Top 7 slowest tests (at least 2 minutes):
|
…/import (#6149)" This reverts commit b4f7a43. /workspace/import classifies files differently from import-file and can't overwrite some of them, which broke deployments (#7216). Forward-fixes for changes that landed on main after #6149: - upload-504 tests (#7014) and upload/internal_server_error inject their faults on the import-file URL again; --body-contains is gone. - The multipart request recorder in prepare_server.go stays, since snapshot uploads use it. - The testserver keeps its /workspace/import modelling for `workspace import`, minus the JUPYTER format no test uses anymore. - sync-upload-edge-cases, added by #6149, is removed. Co-authored-by: Isaac <no-reply@databricks.com>
Overwrite can't change a node's type: import-file returns 400 when a file holds the name of a notebook upload, and 409 when a notebook holds the name of a file upload. Deployments made with v1.21 and v1.22 have such files, because those versions uploaded some notebooks as plain files. When an overwrite fails this way, delete the node of the other type and upload once more, so the remote matches the local file. Replace the #7216 Badness tests with bundle/sync/replace-node-type (Cloud), which syncs over both kinds of mismatch. Co-authored-by: Isaac <no-reply@databricks.com>
The revert of #7034 restored a direct comparison that errorlint now flags. Co-authored-by: Isaac <no-reply@databricks.com>
Sankalp-Mittal
changed the title
Sankalp-Mittal
marked this pull request as ready for review
October 28, 2026 14:57
Sankalp-Mittal
requested review from
andrewnester and
maksim-zuev-db
and removed request for
andrewnester and
maksim-zuev-db
October 28, 2026 14:57
The real import-file endpoint imports each entry of a .zip into the archive's parent folder instead of storing the archive (importZipFile in universe). The fake stored it as a file, so nothing tracked that behavior. Add bundle/sync/zip-extract (Cloud), and have the ssh tests upload valid placeholder archives, which the real endpoint requires too. Co-authored-by: Isaac <no-reply@databricks.com>
…16-notebook-detection # Conflicts: # acceptance/bundle/deployment/bind/pipelines/recreate/output.txt # acceptance/bundle/deployment/bind/pipelines/recreate/script # acceptance/bundle/deployment/bind/pipelines/update/output.txt # acceptance/bundle/deployment/bind/pipelines/update/script
Sankalp-Mittal
requested review from
louis-johnston-db
and removed request for
louis-johnston-db
October 28, 2026 15:06
| // RESOURCE_ALREADY_EXISTS, which no status maps to a conflict. | ||
| if errors.Is(err, apierr.ErrResourceConflict) || errors.Is(err, apierr.ErrResourceAlreadyExists) { | ||
| // This API returns 409 if the file already exists, when the object type is file | ||
| if aerr.StatusCode == http.StatusConflict { |
Contributor
There was a problem hiding this comment.
Just to confirm, this logic identical to the previous state? We did not check ErrResourceAlreadyExists here?
Contributor
Author
There was a problem hiding this comment.
Yes this is identical other than the delete node logic, the error code check was added with the workspace import PR
| return permissionError{absPath, err} | ||
| var nodePath string | ||
| var nodeType workspace.ObjectType | ||
| switch aerr.StatusCode { |
Contributor
There was a problem hiding this comment.
this seems opaque and risky? determining the node type from the HTTP status code? Should we use a regex on the error message instead?
If the regex breaks in the future, that's fine since this cleanup is us best-effort trying to overwrite the file - to prevent breaking future.
Contributor
There was a problem hiding this comment.
We also know the expected node type so an alternative is maybe rely on that? I'm completely fine with relying on the error regex here though.
Contributor
Author
There was a problem hiding this comment.
using the expected node type here is risky since that caused the bug here in the first place so I'd rather just go with the regex here
The testserver now unzips archives uploaded through import-file, like the real endpoint, so the test needs a valid archive instead of a plain string. Co-authored-by: Isaac <no-reply@databricks.com>
The status code alone is ambiguous (any 400 matched). Match the two collision messages import-file returns instead; any other error skips the cleanup, as in v1.20. Co-authored-by: Isaac <no-reply@databricks.com>
The revert restored the pre-#7034 shape of DetectWithFS, which behaves the same for files on disk. Keep the main version instead so the PR does not touch libs/notebook, and let the testserver use DetectBytes for JUPYTER imports again. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
shreyas-goenka
approved these changes
Oct 28, 2026
| if err != nil { | ||
| return false, err | ||
| } | ||
| if info.ObjectType != nodeType { |
Contributor
There was a problem hiding this comment.
this seems pointless? We determine the nodeType based on the error message but hten get node type based on GetStatus?
What are we trying to prevent here?
Contributor
Author
There was a problem hiding this comment.
there are a few edge cases that we would miss, in all the examples assume I am trying to upload x.sql
- a folder
x.sqlexists at the path and a python notebookxwe get the same error and the python notebook is deleted - a race condition when multiple writers are writing to the same workspace and both of them try to upload
x.sqlwithout this check the second one deletes the first ones upload (this should have failed but now it passes)
Contributor
There was a problem hiding this comment.
a race condition when multiple writers are writing to the same workspace and both of them try to upload x.sql
We don't need to reason about this case. DABs in general acquires an exclusive lock over the deployment before starting upload.
a folder x.sql exists at the path and a python notebook x we get the same error and the python notebook is deleted
That is by design right? The entire point is we delete x (notebook) to make way for x.sql.
Contributor
There was a problem hiding this comment.
a folder x.sql exists at the path and a python notebook x
this is probably not a legal state for the file system to end up in remotely.
The additional guard is OK though - atleast this way we know for sure that the remote object being deleted is a file or directory.
| trace $CLI workspace-settings-v2 patch-public-workspace-setting port_fwd_serverless_enabled --json '{"name": "port_fwd_serverless_enabled", "boolean_val": {"value": false}}' >/dev/null | ||
|
|
||
| # No release artifacts locally and they're never executed; stand in empty archives. | ||
| # No release artifacts locally and they're never executed; stand in placeholder archives. |
Contributor
There was a problem hiding this comment.
unnecessary diff? I would expect this PR to be a noop wrt zip files?
Contributor
Author
There was a problem hiding this comment.
nope I am still keeping the changes to the testserver wrt how it handles zips, that was the reason for the first .zip regression being missed in the first case
Contributor
Author
There was a problem hiding this comment.
Now we create zips with actual data instead of empty ones
Require the name in the message to match the upload, and only delete a notebook whose language matches the upload's extension. A folder holding x.sql also returns the 409 message, and the old check then deleted an unrelated Python notebook x. Co-authored-by: Isaac <no-reply@databricks.com>
…16-notebook-detection
Since #7241, * in a fault pattern matches within one path segment and ** across segments. Upload paths span many segments, so the faults stopped firing. Co-authored-by: Isaac <no-reply@databricks.com>
Sankalp-Mittal
enabled auto-merge
October 28, 2026 23:39
Co-authored-by: Isaac <no-reply@databricks.com>
This was referenced Oct 29, 2026
|
|
||
| # Workspace paths uploaded via /workspace/import, one per line. The path is a | ||
| # multipart form field, not part of the URL, so print_requests.py can't filter on it. | ||
| print_workspace_imports() { |
Member
There was a problem hiding this comment.
I think most of the PR is a git revert right, so this and most changes under acceptance/ are to a known stable. but would be nice to (in prep for the eventual roll-forward) we can reintroduce this abstraction to avoid having to change all script files again
Comment on lines
432
to
+433
| server.Handle("POST", validateConfigPath, func(req testserver.Request) any { return tt.response }) | ||
| server.Handle("POST", "/api/2.0/workspace/import", func(req testserver.Request) any { | ||
| server.Handle("POST", "/api/2.0/workspace-files/import-file/{path...}", func(req testserver.Request) any { |
Member
There was a problem hiding this comment.
unrelated to your change, but curious that cmd/air/*_test.go do these tests. they should really be using acceptance test for this
Member
There was a problem hiding this comment.
prefer to create archive.zip and archive_v2.zip inside script so we don't have opaque binaries committed
| >>> list /Workspace/Users/[USERNAME]/.bundle/zip-extract-[UNIQUE_NAME]/default/files/pkg | ||
| FILE /pkg/a.txt | ||
|
|
||
| === Syncing a changed archive overwrites the extracted files |
Member
There was a problem hiding this comment.
let's list again after bundle sync?
| uploadPath, opts := importRequest(absPath, body) | ||
| if overwrite { | ||
| opts = append(opts, workspace.UploadOverwrite()) | ||
| err = w.apiClient.Do(ctx, http.MethodPost, urlPath, w.workspaceIDHeaders(), nil, body, nil) |
Member
There was a problem hiding this comment.
workspace_files_client.go:173
The legacy uploader creates notebook bom from BOM-prefixed bom.sql, but sync’s unchanged detector records bom.sql → bom.sql. I reproduced: upload, remove the local file, sync again → reports one deletion but leaves notebook bom behind. The same probe passes with the base branch’s uploader.
An existing snapshot also causes unchanged, previously misclassified files to be skipped after upgrading. The follow-up needs consistent remote-path bookkeeping and coverage for upgrade → replacement → deletion. This restores a v1.20-era problem rather than introducing it entirely anew.
Restoring backend classification also changes remote filenames for BOM-prefixed notebooks. bom.sql now uploads as notebook bom, but NewSnapshotState still records bom.sql → bom.sql because the local detector rejects the BOM. Removing the local file then reports a successful deletion while leaving bom behind. I reproduced this locally; it passes with the previous uploader. Please align the snapshot mapping and extend replace-node-type to cover deleting the file after replacement.
Collaborator
Integration test reportCommit: 8afc86f
7 interesting tests: 7 flaky
Top 50 slowest tests (at least 2 minutes):
|
deco-sdk-tagging Bot
added a commit
that referenced
this pull request
Oct 29, 2026
## Release v1.23.0 ### CLI * Fix GitHub API rate limit errors in `aitools list` and plugin installs by pinning skills to `0.2.32`. ([#7159](#7159)) * `databricks ssh connect` now makes `claude` and `codex` available on dedicated clusters. ([#7190](#7190)) * `databricks ssh connect` on serverless now makes `claude` and `codex` available in IDE sessions (`--ide`) and to non-interactive commands (e.g. `databricks ssh connect -- 'claude -p "..."'`). ([#7123](#7123)) ### AI Runtime * Add workload support for the GPU_1xRTX6000 accelerator type in enabled workspaces. ([#7214](#7214)) * Show only the accelerator types available in the current workspace in `databricks air run -h config.compute`. ([#6993](#6993)) ### Bundles * Fixed `bundle validate` failing with "expected a map to index" when a job `schedule`, `trigger` or `continuous` is a reference to a whole block, such as `${resources.jobs.base.schedule}`. ([#7225](#7225)) * Added `lifecycle.depends_on` to order resources without referencing their fields. ([#7120](#7120)) * Immutable-folder deploys now refuse to deploy onto a snapshot that was modified outside the bundle (break glass); `bundle deploy --force` moves the deployment to a fresh snapshot. ([#6774](#6774)) * Fix `--include-locations` failing when a resource is missing a required field. ([#7185](#7185)) * Removed spurious "required field is not set" warnings for a field whose value is a reference to a whole block, such as `schedule: ${resources.jobs.base.schedule}`. ([#7226](#7226)) * Revert bundle file uploads to the legacy `/workspace-files/import-file` endpoint, rolling back [#6149](#6149) and [#7034](#7034) released in [v1.21.0](https://github.com/databricks/cli/releases/tag/v1.21.0). This fixes notebooks being uploaded as plain files and deployments failing (for example SQL notebooks in [#7216](#7216)). A remote file or notebook whose type differs from the local one is now deleted and replaced instead of blocking the deploy. ([#7218](#7218)) * Support selecting resources for deletion with `bundle deploy --select`. ([#7023](#7023)) * Add the `workspace.sync_mode` setting with the values `overwrite` (default), `in-place`, and `immutable`. It replaces `presets.source_linked_deployment` (use `in-place`) and `experimental.immutable_folder` (use `immutable`), which are now deprecated and emit a warning. ([#7183](#7183)) ### Dependency Updates * Bump dependencies with known vulnerabilities. ([#7213](#7213)) * Update the Go SDK to [v0.190.0](https://github.com/databricks/databricks-sdk-go/releases/tag/v0.190.0). ([#7155](#7155)) * Update the Go SDK to [v0.191.0](https://github.com/databricks/databricks-sdk-go/releases/tag/v0.191.0). ([#7211](#7211)) * Bump Node.js downloaded by the `ssh` agent shim from v24.21.0 to v24.22.2. ([#7246](#7246))
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
Fixes #7216.
import-fileendpoint again./workspace/importbehaves differently from it in ways the CLI couldn't fully work around (summary: https://docs.google.com/document/d/1yIaFyB6IoYGBuynPwQNnb6uwbcEznUZPaVY6AEl2iOw):AUTOrejects reserved asset names, can't overwrite an extracted.zip, and classifies notebooks differently. The CLI-side classification added in filer: import files the way the legacy import-file endpoint did #7034 to avoidAUTOimported some notebooks (e.g. a SQL notebook starting with a byte order mark) as plain files.import-filereturns 400 when a file holds the name of a notebook upload, and 409 when a notebook holds the name of a file upload. Deployments made with v1.21 and v1.22 have such files. When an overwrite fails with one of those two collision messages, the filer deletes the node of the other type (checked withget-status) and uploads once more, so the remote matches the local file. Any other error is returned as before.Forward-fixes for changes on main since #6149: the
upload-504tests (#7014) andupload/internal_server_errorinject faults on theimport-fileURL again, the multipart request recorder stays (snapshot uploads use it), and the testserver keeps the improvements from #6149/#7034 (its/workspace/importmodel,fault.py --body-contains, JUPYTER imports).libs/notebookis left as on main.The testserver now also extracts a
.zipsent toimport-fileinto the parent folder, like the real endpoint (importZipFilein universe), instead of storing it as a file. The ssh tests therefore upload valid placeholder archives instead of empty files.Upload errors no longer carry the
failed to upload <path>:prefix #6149 added; the path is still in theEndpoint:line, as in v1.20.Why
Discussed in Slack: revert until the backend fixes
/workspace/import, and clean up mismatched nodes client-side, since a deploy's goal is for the remote to match the local files.Tests
bundle/sync/replace-node-type(Cloud = true): a file where the upload is a notebook and a notebook where the upload is a file are both replaced. Passes locally and onaws-cli.bundle/sync/zip-extract(Cloud = true): a synced.zipis extracted into the file path and overwritten on the next sync. Passes locally and onaws-cli.deleteNodeOfOtherType. Deploying the same 32-file bundle (notebook header variants,.zip, reserved asset names, a dashboard) twice with released v1.20.0 and with this branch on dogfood gives identical object types and byte-identical contents, and a deploy lock held by someone else is rejected the same way and left in place.bom.sql), then this branch (replaced by NOTEBOOKbom), then v1.20.0 and this branch again; no errors.uvcan't resolvehatchling.golang.org/x/net v0.62.0(from Bump dependencies with known vulnerabilities #7213); the merge only touched twobind/pipelinestests, resolved by combining--nostampwith theimport-fileexclusion. Everything above ran on the commit before the merge.This pull request and its description were written by Isaac.