Add BlobStorageOptions to support no_tmp_dir parameter #65
Conversation
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
Member
|
@bearcage-dayjob thanks for raising this, I was able to reproduce it. I am working on a refactored version of this change based on what you have provided. |
Author
|
Awesome, thanks for taking a look! |
This commit adds configurable support for the no_tmp_dir URL parameter
in gocloud.dev/blob/fileblob to address cross-filesystem rename issues.
Background:
The blob storage backend uses atomic rename operations (create temp file
in os.TempDir(), then rename to final destination). This fails with
"invalid cross-device link" errors when the temp directory (typically /tmp)
is on a different filesystem than the storage directory. This commonly
occurs in containerized environments where /tmp is on an overlay filesystem
and the cache directory is on a mounted volume.
Solution:
Added a BlobStorageOptions struct with a NoTempDir field that, when set to
true, adds the ?no_tmp_dir=true parameter to file:// URLs. This tells
fileblob to create temporary files in the same directory as the final
destination, avoiding cross-filesystem renames.
Changes:
- Added BlobStorageOptions struct with NoTempDir bool field
- Updated NewBlobStorage() to accept optional *BlobStorageOptions parameter
- Modified GetDefaultStorageURL() to accept noTempDir bool and append URL parameter
- Added helper functions for URL parameter manipulation
- Updated all existing callers to pass nil for backward compatibility
- Added comprehensive tests for new functionality
- All existing tests pass, confirming backward compatibility
Usage:
// Default behavior (backward compatible)
storage, err := buildkitelogs.NewBlobStorage(ctx, storageURL, nil)
// Enable no_tmp_dir to avoid cross-filesystem issues
opts := &buildkitelogs.BlobStorageOptions{NoTempDir: true}
storage, err := buildkitelogs.NewBlobStorage(ctx, storageURL, opts)
Trade-offs:
When NoTempDir=true, temporary files are created in the storage directory
instead of /tmp. This may result in stranded .tmp files if the process
crashes before cleanup runs, but prevents cross-filesystem rename failures.
Downstream consumers (like the MCP server or CLI tools) can now opt into
using no_tmp_dir by passing BlobStorageOptions when creating blob storage.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
wolfeidau
force-pushed
the
fix-cross-filesystem-renames
branch
from
575a6cf to
6ddfb04
Compare
wolfeidau
approved these changes
Nov 10, 2025
Member
wolfeidau
left a comment
wolfeidau
left a comment
There was a problem hiding this comment.
👍🏻 looks good with some ammendments
catkins
reviewed
Nov 10, 2025
| storageURL, err := GetDefaultStorageURL(storageURL) | ||
| // | ||
| // The opts parameter allows configuring blob storage behavior. Pass nil to use default options. | ||
| func NewBlobStorage(ctx context.Context, storageURL string, opts *BlobStorageOptions) (*BlobStorage, error) { |
Collaborator
There was a problem hiding this comment.
i've got a mild preference for using functional options rather than nil-able options struct, but its very mild
catkins
approved these changes
Nov 10, 2025
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.
This commit adds configurable support for the no_tmp_dir URL parameter in
gocloud.dev/blob/fileblobto address cross-filesystem rename issues.The blob storage backend uses atomic rename operations (create temp file in os.TempDir(), then rename to final destination). This fails with "invalid cross-device link" errors when
/tmpis on a different filesystem than the storage directory, which happens to be the case for my workspace VM.This diff adds a BlobStorageOptions struct with a NoTempDir field that, when set to true, adds the ?no_tmp_dir=true parameter to file:// URLs, which tells fileblob to create temporary files in the same directory as the final destination. This can result in some untidiness in
.tmpin the cache directory, but there's not really a universal solution to that. We could probably add some sort of "cleanup old ctime records on fetch" task to tidy up the user's log cache in the future, if this becomes a problem.