-
Notifications
You must be signed in to change notification settings - Fork 4
Refactor error handling in backup upload and folder creation processes #342
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
Changes from all commits
0d0e2cc
7311206
0c1478b
28e4eb0
ec6a676
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export const INITIAL_RATE_LIMIT_DELAY_MS = 30_000; | ||
| export const INITIAL_SERVER_ERROR_DELAY_MS = 1_000; | ||
| export const MAX_BACKOFF_MS = 480_000; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import { logger } from '@internxt/drive-desktop-core/build/backend'; | ||
| import { DriveDesktopError } from '../../../context/shared/domain/errors/DriveDesktopError'; | ||
| import { extractPropertyFromStringyfiedJson } from '../../../shared/extract-property-from-json'; | ||
| import { INITIAL_RATE_LIMIT_DELAY_MS, INITIAL_SERVER_ERROR_DELAY_MS, MAX_BACKOFF_MS } from './constants'; | ||
|
|
||
| export function parseRetryAfterMs(message?: string) { | ||
| const retryAfterSeconds = extractPropertyFromStringyfiedJson(message ?? '', 'retry_after'); | ||
| return typeof retryAfterSeconds === 'number' ? retryAfterSeconds * 1000 : INITIAL_RATE_LIMIT_DELAY_MS; | ||
| } | ||
|
|
||
| export function mapEnvironmentUploadError(err: Error & { code?: unknown; status?: unknown }): DriveDesktopError { | ||
| if (err.code === 'EACCES' || err.code === 'EPERM') { | ||
| return new DriveDesktopError('ACTION_NOT_PERMITTED', err.message); | ||
| } | ||
| if (err.message === 'Max space used') { | ||
| return new DriveDesktopError('NOT_ENOUGH_SPACE'); | ||
| } | ||
| if (typeof err.status === 'number') { | ||
| if (err.status === 429) { | ||
| return new DriveDesktopError('RATE_LIMITED', String(parseRetryAfterMs(err.message))); | ||
| } | ||
| if (err.status >= 500) { | ||
| return new DriveDesktopError('INTERNAL_SERVER_ERROR'); | ||
| } | ||
| } | ||
| return new DriveDesktopError('UNKNOWN', err.message); | ||
| } | ||
|
|
||
| function exponentialBackoff(attempts: number, baseMs: number) { | ||
| return Math.min(baseMs * Math.pow(2, attempts - 1), MAX_BACKOFF_MS); | ||
| } | ||
|
|
||
| type Props = { | ||
| tag: 'BACKUPS' | 'SYNC-ENGINE'; | ||
| context: string; | ||
| path: string; | ||
| }; | ||
|
|
||
| export function createTransientErrorHandler({ tag, context, path }: Props) { | ||
| let transientAttempts = 0; | ||
|
|
||
| return (error: DriveDesktopError): number | null => { | ||
| if (error.cause === 'RATE_LIMITED' || error.cause === 'INTERNAL_SERVER_ERROR') { | ||
| transientAttempts++; | ||
|
|
||
| const baseDelayMs = | ||
| error.cause === 'RATE_LIMITED' | ||
| ? Number(error.message) || INITIAL_RATE_LIMIT_DELAY_MS | ||
| : INITIAL_SERVER_ERROR_DELAY_MS; | ||
|
|
||
| const delayMs = exponentialBackoff(transientAttempts, baseDelayMs); | ||
|
|
||
| logger.debug({ | ||
| tag, | ||
| msg: `[${context}]`, | ||
| cause: error.cause, | ||
| attempt: transientAttempts, | ||
| delayMs, | ||
| path, | ||
| }); | ||
|
|
||
| return delayMs; | ||
| } | ||
|
|
||
| return null; | ||
| }; | ||
| } |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1 @@ | ||
| export const DEFAULT_CONCURRENCY = 6; | ||
| export const MAX_RETRIES = 3; | ||
| export const RETRY_DELAYS_MS = [1000, 2000, 4000]; | ||
| export const INITIAL_RATE_LIMIT_DELAY_MS = 30_000; | ||
| export const MAX_BACKOFF_MS = 480_000; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,28 @@ | ||
| import { RemoteTreeBuilder } from '../../../../context/virtual-drive/remoteTree/application/RemoteTreeBuilder'; | ||
| import { RemoteItemsGenerator } from '../../../../context/virtual-drive/remoteTree/domain/RemoteItemsGenerator'; | ||
| import { FolderRepositorySynchronizer } from '../../../../context/virtual-drive/folders/application/FolderRepositorySynchronizer/FolderRepositorySynchronizer'; | ||
| import { FileRepositorySynchronizer } from '../../../../context/virtual-drive/files/application/FileRepositorySynchronizer'; | ||
| import { StorageRemoteChangesSyncher } from '../../../../context/storage/StorageFiles/application/sync/StorageRemoteChangesSyncher'; | ||
| import { ServerFolderStatus } from '../../../../context/shared/domain/ServerFolder'; | ||
| import { logger } from '@internxt/drive-desktop-core/build/backend'; | ||
| import { User } from '../../../../apps/main/types'; | ||
| import { Container } from 'diod'; | ||
|
|
||
| // This is the old src/apps/drive/fuse/FuseApp.update | ||
| export async function updateVirtualDriveContainer({ container, user }: { container: Container; user: User }) { | ||
| try { | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why a promise all and not wait for the treeBuilder to finish?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| ]); | ||
|
|
||
| const deletedFolderIds = new Set( | ||
| allRemoteItems.folders.filter((f) => f.status !== ServerFolderStatus.EXISTS).map((f) => f.id), | ||
| ); | ||
|
|
||
| await Promise.all([ | ||
| container.get(FileRepositorySynchronizer).run(tree.files), | ||
| container.get(FolderRepositorySynchronizer).run(tree.folders), | ||
| container.get(FolderRepositorySynchronizer).run(tree.folders, deletedFolderIds), | ||
| container.get(StorageRemoteChangesSyncher).run(), | ||
| ]); | ||
| logger.debug({ msg: '[VIRTUAL DRIVE] Tree updated successfully' }); | ||
|
|
||
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.
You moved this from a module we created to a legacy structure, why?
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.
It has already moved