-
Notifications
You must be signed in to change notification settings - Fork 251
Avoid re-creating pre-existing folders. #3295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a bug fix to avoid re-creating pre-existing folders, addressing performance issues with services like Azure Files that may complain about excessive directory creation requests.
Key Changes
- Introduces a new
FolderExistedtransfer status to distinguish between folders created by the current job versus pre-existing folders - Modifies folder creation logic to return a sentinel error (
FolderCreationErrorAlreadyExists) when a folder already exists, preventing redundant creation attempts - Extends the
NewFolderCreationTrackerto create trackers for File and BlobFS destinations, not just when persisting folder properties
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ste/folderCreationTracker.go |
Core logic changes: adds folder state tracking, new status enum, and logic to avoid re-creating existing folders |
ste/folderCreationTracker_test.go |
Adds test for folder existence detection and updates existing test structure |
ste/sender-blobFS.go |
Updates doEnsureDirExists to return FolderCreationErrorAlreadyExists when path already exists |
ste/sender-azureFile.go |
Updates CreateDirToRoot to return FolderCreationErrorAlreadyExists when resource already exists |
ste/mgr-JobMgr.go |
Passes fromTo parameter to NewFolderCreationTracker constructor |
common/folderCreationTracker_interface.go |
Defines new FolderCreationErrorAlreadyExists sentinel error type |
common/fe-ste-models.go |
Adds FolderExisted() status (value 4) and adjusts Restarted() to value 5 |
ste/JobPartPlan.go |
Bumps data schema version from 19 to 20 for enum value changes |
jobsAdmin/init.go |
Includes FolderExisted status in job summary resurrection logic |
go.mod, go.sum |
Adds indirect dependencies (transitive from other packages) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, ok := f.unregisteredButCreated[folder]; ok { | ||
| f.fetchTransfer(JpptFolderIndex{PartNum: partNum, TransferIndex: transferIndex}).SetTransferStatus(common.ETransferStatus.FolderCreated(), false) | ||
| // if our transfer already has a created status, we should adopt that. | ||
| if ts := f.fetchTransfer(*state.Index).TransferStatus(); ts == ts.FolderCreated() { |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison ts == ts.FolderCreated() is comparing the variable against a method call on itself, which will always return false. This should be ts == common.ETransferStatus.FolderCreated() to properly check if the transfer status is FolderCreated.
| if ts := f.fetchTransfer(*state.Index).TransferStatus(); ts == ts.FolderCreated() { | |
| if ts := f.fetchTransfer(*state.Index).TransferStatus(); ts == common.ETransferStatus.FolderCreated() { |
| } | ||
|
|
||
| f.debugCheckState(*state) | ||
|
|
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a new state is created (line 153) for a folder that isn't already in f.contents, it needs to be stored in the map. After updating the state's Status field (lines 156-160), add f.contents[folder] = state before line 174 to persist the state. Without this, the state is lost and subsequent calls to CreateFolder for the same folder won't see the previous status, defeating the purpose of tracking folder creation.
| f.contents[folder] = state |
| // returned by doCreation on FolderCreationTracker.CreateFolder for supported folder-creators. | ||
| // This will inform the folder creation tracker to _not_ try to create the folder. |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is misleading. The error is returned after doCreation has already been called and attempted to create the folder. It signals that the folder already existed, not that creation should be skipped. Consider rewording to: "FolderCreationErrorAlreadyExists is a signalling error that should be returned by doCreation when the folder already exists, allowing the tracker to mark it as FolderExisted rather than FolderCreated."
| // returned by doCreation on FolderCreationTracker.CreateFolder for supported folder-creators. | |
| // This will inform the folder creation tracker to _not_ try to create the folder. | |
| // returned by doCreation when the folder already exists, allowing the tracker to mark it as | |
| // FolderExisted rather than FolderCreated. |
Description
Feature / Bug Fix: (Brief description of the feature or issue being addressed)
Related Links:
PBI # 26906103
Type of Change
How Has This Been Tested?
Test is written to interact with the folder creation tracker directly.