Check Close error when generate downloader writes files#5523
Open
simonfaltum wants to merge 4 commits into
Open
Check Close error when generate downloader writes files#5523simonfaltum wants to merge 4 commits into
simonfaltum wants to merge 4 commits into
Conversation
FlushToDisk closed downloaded files via defer, discarding the Close error and logging success before the file was closed. A failed flush could leave a truncated file while the command exits 0. Close the file explicitly and propagate the error, mirroring the pattern in libs/filer/local_client.go. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
Integration test reportCommit: ef5336e
28 interesting tests: 15 SKIP, 6 flaky, 6 RECOVERED, 1 FAIL
Top 50 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| assert.Empty(t, downloader.files) | ||
| } | ||
|
|
||
| func TestDownloader_FlushToDisk(t *testing.T) { |
Contributor
There was a problem hiding this comment.
The test doesn't exercise the failure path
Member
Author
There was a problem hiding this comment.
Good catch, fixed in 246ff61. A real os.File Close failure can't be triggered portably in a test, so I extracted the copy+close logic into a writeAndClose helper and added TestWriteAndClose with a fake WriteCloser covering the failure paths: a Close error is returned, and a copy error is not masked by a Close error.
A real os.File Close error cannot be triggered portably in a test, so extract the copy-and-close logic into a helper and cover the failure paths with a fake WriteCloser: a Close error is returned, and a copy error is not masked by a Close error. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
approved these changes
Jun 12, 2026
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
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
Found during a full-repo review of the CLI. When
bundle generate(andapps import) downloads notebooks and files to disk, it closed each written file with a deferredClosewhose error was discarded, and the "File successfully saved" message was printed before the file was closed. If the OS reports a write error at close time (for example a failed flush on a full or remote filesystem), the command logs success and exits 0 while leaving a truncated file on disk.Changes
Before, a failed
Closeon a downloaded file was silently ignored; now it fails the command before the success message is printed. Inbundle/generate/downloader.go,FlushToDiskcloses the file explicitly afterio.Copyand propagates theCloseerror when the copy itself succeeded, mirroring the existing pattern inlibs/filer/local_client.go. No output changes on the happy path.Test plan
TestDownloader_FlushToDiskcovering the download-write-close path end to end (file contents verified on disk). The close-error branch itself is not portably triggerable on a realos.Filewithout dependency injection, so the test covers the restructured code path.go test ./bundle/generate/passes.TestAccept/bundle/generate/job_nested_notebooks,TestAccept/bundle/generate/python_job,TestAccept/bundle/generate/pipeline../task fmt-q,./task lint-q, and./task checkspass.This pull request and its description were written by Isaac.