Refactor error handling in backup upload and folder creation processes#342
Conversation
| @@ -0,0 +1,3 @@ | |||
| export const INITIAL_RATE_LIMIT_DELAY_MS = 30_000; | |||
There was a problem hiding this comment.
You moved this from a module we created to a legacy structure, why?
| const tree = await container.get(RemoteTreeBuilder).run(user.root_folder_id, user.rootFolderId); | ||
| const [tree, allRemoteItems] = await Promise.all([ | ||
| container.get(RemoteTreeBuilder).run(user.root_folder_id, user.rootFolderId), | ||
| container.get(RemoteItemsGenerator).getAll(), |
There was a problem hiding this comment.
Why a promise all and not wait for the treeBuilder to finish?
There was a problem hiding this comment.
You don't have to wait for the tree to be ready; both tasks can be done at the same time without interfering with each other.
|
|
||
| stopWatching(); | ||
| if (error) { | ||
| throw error; |
There was a problem hiding this comment.
Why you throw an error here but not on L45?
| try { | ||
| const uploadedContentsId = await uploader(); | ||
| return { data: uploadedContentsId }; | ||
| } catch (uploadError) { | ||
| return { error: mapEnvironmentUploadError(uploadError as Error & { status?: unknown }) }; |
There was a problem hiding this comment.
This makes the code harder to read, maybe you could create a helper function or even ensure that uploaded is not going to throw, or even if necessary wrap it in the tryCatch method utility
There was a problem hiding this comment.
I've reorganized the code to make it easier to read; the current function was too long.
| folderId: fileFolderId, | ||
| folderUuid: folder.uuid, | ||
| }); | ||
| const { data: persistedFile, error: persistedError } = await retryWithBackoff( |
There was a problem hiding this comment.
Why did you wrap TemporalFileUploader but not here?
There was a problem hiding this comment.
The responsibilities of the original function have been broken down into smaller ones to improve readability
| controller.signal, | ||
| ); | ||
|
|
||
| if (error) throw error; |
There was a problem hiding this comment.
I would not trow an error here because is not being handled on
renameController -> rename -> handleTemporalFileUploadOnRename -> uploadTemporalFileOnRename -> TemporalFileUploader.run
I assume it should be like the same on the other callers
I woud handle it as soon as the exception happens so that we can control properly this behaviour and not let the exception without being handled, what do you think?
There was a problem hiding this comment.
This function is auxiliary and throws an exception that is handled in the main run function, which is the one that actually uses this class. The exception is useful for stopping the watcher regardless of whether an error occurs; if we avoided throwing the exception, we would have to stop the watch when an error occurs and also at the end of the function if it is successful—both separately.
| FolderCreatedAt.fromString(dto.createdAt), | ||
| FolderUpdatedAt.fromString(dto.updatedAt), | ||
| ); | ||
| throw new Error(`Could not create folder ${folderPath.value}: ${error.cause}`); |
There was a problem hiding this comment.
Thats fine because it was there on previous version
… file uploads and folder creations
d1d2674 to
ec6a676
Compare
|



What is Changed / Added
Bug fix: Race condition causing
FolderNotFoundErroron large uploadsWhen uploading a folder with hundreds of files, some uploads were failing with
FolderNotFoundErroreven though the folder clearly existed on the server. The root cause was a race condition between folder creation and the periodic remote sync.Here's what was happening: when FUSE calls
mkdir,FolderCreatorcreates the folder via the API and immediately adds it to the in-memory repository. However, the folder is not yet in the SQLite sync store — the incremental sync had already completed its API call before the folder was created. When the nextREMOTE_CHANGES_SYNCHEDevent fires shortly after,FolderRepositorySynchronizerrebuilds the in-memory repo from the SQLite snapshot, notices the new folder is missing, and deletes it. Any files being uploaded into that folder at that moment then fail to find their parent.The fix adds a second argument to
FolderRepositorySynchronizer.run():allRemoteFolderIds, the full set of folder IDs present in the SQLite store (all statuses, not justEXISTS). A folder is now only evicted from the in-memory repo if it is both absent from the EXISTS remote tree and confirmed in the SQLite store — the logic being that if a folder is not in SQLite at all, it must have been created locally and not yet picked up by the incremental sync, so it should be left alone.updateVirtualDriveContainerwas updated to fetch the EXISTS-filtered tree and the fullRemoteItemsGeneratorsnapshot in parallel and pass the resulting ID set down to the synchronizer.RemoteItemsGeneratorwas also changed from.private()to public in the DI container so it can be resolved directly.Improvement: Retry on transient errors (429 / 5xx) for folder creation
Folder creation was silently failing on any 5xx or 429 response.
HttpRemoteFileSystem.persist()had a manual recursive retry only for 400s, and everything else fell through to a genericUNHANDLEDerror thatFolderCreatorwould simply throw — no retry, no backoff.To fix this,
persist()now maps all HTTP error codes to typedDriveDesktopErrorcauses (INTERNAL_SERVER_ERRORfor 5xx,RATE_LIMITEDfor 429 with the parsedRetry-Aftervalue, etc.), aligning it with howSDKRemoteFileSystemhandles errors on the file side.FolderCreatorthen wraps the remote call inretryWithBackoffusing the sharedcreateTransientErrorHandler, which applies exponential backoff forRATE_LIMITEDandINTERNAL_SERVER_ERRORand fails immediately for anything non-recoverable likeBAD_REQUESTorUNKNOWN. The old recursiveattempt-based retry insideHttpRemoteFileSystemis removed entirely.