From 9f3fda9dde0a5af768c0399804d9d758261e6d23 Mon Sep 17 00:00:00 2001 From: Alex Tran Date: Wed, 20 Nov 2024 13:47:12 -0600 Subject: [PATCH 1/6] feat(server): clean up interrupted upload files --- .../src/middleware/file-upload.interceptor.ts | 20 +++++++++++++++++++ server/src/services/storage.service.ts | 4 ++++ 2 files changed, 24 insertions(+) diff --git a/server/src/middleware/file-upload.interceptor.ts b/server/src/middleware/file-upload.interceptor.ts index 075a7f504636a..04c1e577b0d15 100644 --- a/server/src/middleware/file-upload.interceptor.ts +++ b/server/src/middleware/file-upload.interceptor.ts @@ -11,6 +11,7 @@ import { RouteKey } from 'src/enum'; import { ILoggerRepository } from 'src/interfaces/logger.interface'; import { AuthRequest } from 'src/middleware/auth.guard'; import { AssetMediaService, UploadFile } from 'src/services/asset-media.service'; +import { StorageService } from 'src/services/storage.service'; export interface UploadFiles { assetData: ImmichFile[]; @@ -81,6 +82,7 @@ export class FileUploadInterceptor implements NestInterceptor { constructor( private reflect: Reflector, private assetService: AssetMediaService, + private storageService: StorageService, @Inject(ILoggerRepository) private logger: ILoggerRepository, ) { this.logger.setContext(FileUploadInterceptor.name); @@ -141,6 +143,14 @@ export class FileUploadInterceptor implements NestInterceptor { private handleFile(request: AuthRequest, file: Express.Multer.File, callback: Callback>) { (file as ImmichMulterFile).uuid = randomUUID(); + + request.on('error', (error) => { + if (error.message === 'aborted') { + this.handleAbortedRequest(request, file); + return; + } + }); + if (!this.isAssetUploadFile(file)) { this.defaultStorage._handleFile(request, file, callback); return; @@ -158,6 +168,16 @@ export class FileUploadInterceptor implements NestInterceptor { }); } + private handleAbortedRequest(request: AuthRequest, file: Express.Multer.File) { + const uploadFilename = this.assetService.getUploadFilename(asRequest(request, file)); + const uploadFolder = this.assetService.getUploadFolder(asRequest(request, file)); + const uploadPath = `${uploadFolder}/${uploadFilename}`; + + this.storageService.deleteFiles([uploadPath]).catch((error) => { + this.logger.warn(`Failed to delete aborted file: ${uploadPath}`, error); + }); + } + private removeFile(request: AuthRequest, file: Express.Multer.File, callback: (error: Error | null) => void) { this.defaultStorage._removeFile(request, file, callback); } diff --git a/server/src/services/storage.service.ts b/server/src/services/storage.service.ts index ce26df486977e..04fd6cec071dc 100644 --- a/server/src/services/storage.service.ts +++ b/server/src/services/storage.service.ts @@ -84,6 +84,10 @@ export class StorageService extends BaseService { return JobStatus.SUCCESS; } + async deleteFiles(files: string[]) { + await this.jobRepository.queue({ name: JobName.DELETE_FILES, data: { files } }); + } + private async verifyReadAccess(folder: StorageFolder) { const { internalPath, externalPath } = this.getMountFilePaths(folder); try { From 46765489ad909bf22d47071b33be4ab409b85c93 Mon Sep 17 00:00:00 2001 From: Alex Tran Date: Wed, 20 Nov 2024 14:30:31 -0600 Subject: [PATCH 2/6] pr feedback --- .../src/middleware/file-upload.interceptor.ts | 33 ++----------------- .../src/services/asset-media.service.spec.ts | 25 ++++++++++++++ server/src/services/asset-media.service.ts | 11 ++++++- server/src/services/storage.service.ts | 4 --- server/src/utils/asset.util.ts | 22 +++++++++++++ 5 files changed, 59 insertions(+), 36 deletions(-) diff --git a/server/src/middleware/file-upload.interceptor.ts b/server/src/middleware/file-upload.interceptor.ts index 04c1e577b0d15..d6aeb031aa901 100644 --- a/server/src/middleware/file-upload.interceptor.ts +++ b/server/src/middleware/file-upload.interceptor.ts @@ -11,7 +11,7 @@ import { RouteKey } from 'src/enum'; import { ILoggerRepository } from 'src/interfaces/logger.interface'; import { AuthRequest } from 'src/middleware/auth.guard'; import { AssetMediaService, UploadFile } from 'src/services/asset-media.service'; -import { StorageService } from 'src/services/storage.service'; +import { asRequest, mapToUploadFile } from 'src/utils/asset.util'; export interface UploadFiles { assetData: ImmichFile[]; @@ -36,16 +36,6 @@ export interface ImmichFile extends Express.Multer.File { checksum: Buffer; } -export function mapToUploadFile(file: ImmichFile): UploadFile { - return { - uuid: file.uuid, - checksum: file.checksum, - originalPath: file.path, - originalName: Buffer.from(file.originalname, 'latin1').toString('utf8'), - size: file.size, - }; -} - type DiskStorageCallback = (error: Error | null, result: string) => void; type ImmichMulterFile = Express.Multer.File & { uuid: string }; @@ -63,14 +53,6 @@ const callbackify = (target: (...arguments_: any[]) => T, callback: Callback< } }; -const asRequest = (request: AuthRequest, file: Express.Multer.File) => { - return { - auth: request.user || null, - fieldName: file.fieldname as UploadFieldName, - file: mapToUploadFile(file as ImmichFile), - }; -}; - @Injectable() export class FileUploadInterceptor implements NestInterceptor { private handlers: { @@ -82,7 +64,6 @@ export class FileUploadInterceptor implements NestInterceptor { constructor( private reflect: Reflector, private assetService: AssetMediaService, - private storageService: StorageService, @Inject(ILoggerRepository) private logger: ILoggerRepository, ) { this.logger.setContext(FileUploadInterceptor.name); @@ -146,7 +127,7 @@ export class FileUploadInterceptor implements NestInterceptor { request.on('error', (error) => { if (error.message === 'aborted') { - this.handleAbortedRequest(request, file); + this.assetService.onUploadError(request, file).catch(this.logger.error); return; } }); @@ -168,16 +149,6 @@ export class FileUploadInterceptor implements NestInterceptor { }); } - private handleAbortedRequest(request: AuthRequest, file: Express.Multer.File) { - const uploadFilename = this.assetService.getUploadFilename(asRequest(request, file)); - const uploadFolder = this.assetService.getUploadFolder(asRequest(request, file)); - const uploadPath = `${uploadFolder}/${uploadFilename}`; - - this.storageService.deleteFiles([uploadPath]).catch((error) => { - this.logger.warn(`Failed to delete aborted file: ${uploadPath}`, error); - }); - } - private removeFile(request: AuthRequest, file: Express.Multer.File, callback: (error: Error | null) => void) { this.defaultStorage._removeFile(request, file, callback); } diff --git a/server/src/services/asset-media.service.spec.ts b/server/src/services/asset-media.service.spec.ts index c269739935e01..8da249c8d3f20 100644 --- a/server/src/services/asset-media.service.spec.ts +++ b/server/src/services/asset-media.service.spec.ts @@ -14,6 +14,7 @@ import { IAssetRepository } from 'src/interfaces/asset.interface'; import { IJobRepository, JobName } from 'src/interfaces/job.interface'; import { IStorageRepository } from 'src/interfaces/storage.interface'; import { IUserRepository } from 'src/interfaces/user.interface'; +import { AuthRequest } from 'src/middleware/auth.guard'; import { AssetMediaService } from 'src/services/asset-media.service'; import { ImmichFileResponse } from 'src/utils/file'; import { assetStub } from 'test/fixtures/asset.stub'; @@ -879,4 +880,28 @@ describe(AssetMediaService.name, () => { expect(assetMock.getByChecksums).toHaveBeenCalledWith(authStub.admin.user.id, [file1, file2]); }); }); + + describe('onUploadError', () => { + it('should queue a job to delete the uploaded file', async () => { + const request = { user: authStub.user1 } as unknown as AuthRequest; + console.log('request', request); + const file = { + fieldname: UploadFieldName.ASSET_DATA, + originalname: 'image.jpg', + mimetype: 'image/jpeg', + buffer: Buffer.from(''), + size: 1000, + uuid: 'random-uuid', + checksum: Buffer.from('checksum', 'utf8'), + originalPath: 'upload/upload/user-id/ra/nd/random-uuid.jpg', + } as unknown as Express.Multer.File; + + await sut.onUploadError(request, file); + + expect(jobMock.queue).toHaveBeenCalledWith({ + name: JobName.DELETE_FILES, + data: { files: ['upload/upload/user-id/ra/nd/random-uuid.jpg'] }, + }); + }); + }); }); diff --git a/server/src/services/asset-media.service.ts b/server/src/services/asset-media.service.ts index 70f4905de31e4..2424c93e44a15 100644 --- a/server/src/services/asset-media.service.ts +++ b/server/src/services/asset-media.service.ts @@ -23,9 +23,10 @@ import { AuthDto } from 'src/dtos/auth.dto'; import { ASSET_CHECKSUM_CONSTRAINT, AssetEntity } from 'src/entities/asset.entity'; import { AssetStatus, AssetType, CacheControl, Permission, StorageFolder } from 'src/enum'; import { JobName } from 'src/interfaces/job.interface'; +import { AuthRequest } from 'src/middleware/auth.guard'; import { BaseService } from 'src/services/base.service'; import { requireUploadAccess } from 'src/utils/access'; -import { getAssetFiles, onBeforeLink } from 'src/utils/asset.util'; +import { asRequest, getAssetFiles, onBeforeLink } from 'src/utils/asset.util'; import { ImmichFileResponse } from 'src/utils/file'; import { mimeTypes } from 'src/utils/mime-types'; import { fromChecksum } from 'src/utils/request'; @@ -118,6 +119,14 @@ export class AssetMediaService extends BaseService { return folder; } + async onUploadError(request: AuthRequest, file: Express.Multer.File) { + const uploadFilename = this.getUploadFilename(asRequest(request, file)); + const uploadFolder = this.getUploadFolder(asRequest(request, file)); + const uploadPath = `${uploadFolder}/${uploadFilename}`; + + await this.jobRepository.queue({ name: JobName.DELETE_FILES, data: { files: [uploadPath] } }); + } + async uploadAsset( auth: AuthDto, dto: AssetMediaCreateDto, diff --git a/server/src/services/storage.service.ts b/server/src/services/storage.service.ts index 04fd6cec071dc..ce26df486977e 100644 --- a/server/src/services/storage.service.ts +++ b/server/src/services/storage.service.ts @@ -84,10 +84,6 @@ export class StorageService extends BaseService { return JobStatus.SUCCESS; } - async deleteFiles(files: string[]) { - await this.jobRepository.queue({ name: JobName.DELETE_FILES, data: { files } }); - } - private async verifyReadAccess(folder: StorageFolder) { const { internalPath, externalPath } = this.getMountFilePaths(folder); try { diff --git a/server/src/utils/asset.util.ts b/server/src/utils/asset.util.ts index 44c291e139766..f8bed5485f8b1 100644 --- a/server/src/utils/asset.util.ts +++ b/server/src/utils/asset.util.ts @@ -1,6 +1,7 @@ import { BadRequestException } from '@nestjs/common'; import { StorageCore } from 'src/cores/storage.core'; import { BulkIdErrorReason, BulkIdResponseDto } from 'src/dtos/asset-ids.response.dto'; +import { UploadFieldName } from 'src/dtos/asset-media.dto'; import { AuthDto } from 'src/dtos/auth.dto'; import { AssetFileEntity } from 'src/entities/asset-files.entity'; import { AssetFileType, AssetType, Permission } from 'src/enum'; @@ -8,6 +9,9 @@ import { IAccessRepository } from 'src/interfaces/access.interface'; import { IAssetRepository } from 'src/interfaces/asset.interface'; import { IEventRepository } from 'src/interfaces/event.interface'; import { IPartnerRepository } from 'src/interfaces/partner.interface'; +import { AuthRequest } from 'src/middleware/auth.guard'; +import { ImmichFile } from 'src/middleware/file-upload.interceptor'; +import { UploadFile } from 'src/services/asset-media.service'; import { checkAccess } from 'src/utils/access'; export interface IBulkAsset { @@ -181,3 +185,21 @@ export const onAfterUnlink = async ( await assetRepository.update({ id: livePhotoVideoId, isVisible: true }); await eventRepository.emit('asset.show', { assetId: livePhotoVideoId, userId }); }; + +export function mapToUploadFile(file: ImmichFile): UploadFile { + return { + uuid: file.uuid, + checksum: file.checksum, + originalPath: file.path, + originalName: Buffer.from(file.originalname, 'latin1').toString('utf8'), + size: file.size, + }; +} + +export const asRequest = (request: AuthRequest, file: Express.Multer.File) => { + return { + auth: request.user || null, + fieldName: file.fieldname as UploadFieldName, + file: mapToUploadFile(file as ImmichFile), + }; +}; From baf5d0c545a57f17db7fe96d54d8b61a33220a20 Mon Sep 17 00:00:00 2001 From: Alex Tran Date: Wed, 20 Nov 2024 14:36:02 -0600 Subject: [PATCH 3/6] remove console.log --- server/src/services/asset-media.service.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/services/asset-media.service.spec.ts b/server/src/services/asset-media.service.spec.ts index 8da249c8d3f20..d68140367d702 100644 --- a/server/src/services/asset-media.service.spec.ts +++ b/server/src/services/asset-media.service.spec.ts @@ -883,8 +883,8 @@ describe(AssetMediaService.name, () => { describe('onUploadError', () => { it('should queue a job to delete the uploaded file', async () => { - const request = { user: authStub.user1 } as unknown as AuthRequest; - console.log('request', request); + const request = { user: authStub.user1 } as AuthRequest; + const file = { fieldname: UploadFieldName.ASSET_DATA, originalname: 'image.jpg', From 5d3dacf2a6e6790778ea3ea536f2bed94c880a63 Mon Sep 17 00:00:00 2001 From: Alex Tran Date: Wed, 20 Nov 2024 14:37:54 -0600 Subject: [PATCH 4/6] handle all errors --- server/src/middleware/file-upload.interceptor.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/server/src/middleware/file-upload.interceptor.ts b/server/src/middleware/file-upload.interceptor.ts index d6aeb031aa901..8ba10ad94c95b 100644 --- a/server/src/middleware/file-upload.interceptor.ts +++ b/server/src/middleware/file-upload.interceptor.ts @@ -126,10 +126,9 @@ export class FileUploadInterceptor implements NestInterceptor { (file as ImmichMulterFile).uuid = randomUUID(); request.on('error', (error) => { - if (error.message === 'aborted') { - this.assetService.onUploadError(request, file).catch(this.logger.error); - return; - } + this.logger.warn('Request error while uploading file, cleaning up', error); + this.assetService.onUploadError(request, file).catch(this.logger.error); + return; }); if (!this.isAssetUploadFile(file)) { @@ -138,7 +137,6 @@ export class FileUploadInterceptor implements NestInterceptor { } const hash = createHash('sha1'); - file.stream.on('data', (chunk) => hash.update(chunk)); this.defaultStorage._handleFile(request, file, (error, info) => { if (error) { hash.destroy(); From ed52a72231173cdf793c0e2bf10dc86d9fbdfdc5 Mon Sep 17 00:00:00 2001 From: Alex Tran Date: Wed, 20 Nov 2024 14:48:05 -0600 Subject: [PATCH 5/6] remove return in callback function --- server/src/middleware/file-upload.interceptor.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/middleware/file-upload.interceptor.ts b/server/src/middleware/file-upload.interceptor.ts index 8ba10ad94c95b..d0b04b50a2e5c 100644 --- a/server/src/middleware/file-upload.interceptor.ts +++ b/server/src/middleware/file-upload.interceptor.ts @@ -128,7 +128,6 @@ export class FileUploadInterceptor implements NestInterceptor { request.on('error', (error) => { this.logger.warn('Request error while uploading file, cleaning up', error); this.assetService.onUploadError(request, file).catch(this.logger.error); - return; }); if (!this.isAssetUploadFile(file)) { From 9fe25fa7b9bbc4369c2b30da65d6a280ec552285 Mon Sep 17 00:00:00 2001 From: Alex Tran Date: Wed, 20 Nov 2024 14:50:36 -0600 Subject: [PATCH 6/6] programming in bed is a bad idea --- server/src/middleware/file-upload.interceptor.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/middleware/file-upload.interceptor.ts b/server/src/middleware/file-upload.interceptor.ts index d0b04b50a2e5c..108a187e67b8c 100644 --- a/server/src/middleware/file-upload.interceptor.ts +++ b/server/src/middleware/file-upload.interceptor.ts @@ -136,6 +136,7 @@ export class FileUploadInterceptor implements NestInterceptor { } const hash = createHash('sha1'); + file.stream.on('data', (chunk) => hash.update(chunk)); this.defaultStorage._handleFile(request, file, (error, info) => { if (error) { hash.destroy();