fix: rework file handling so we always explicitly create, overwrite or both (#12812)

This commit is contained in:
Zack Pollard 2024-09-21 00:16:53 +01:00 committed by GitHub
parent af70111645
commit 5a1a841365
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 46 additions and 16 deletions

View File

@ -35,7 +35,9 @@ export interface IStorageRepository {
createZipStream(): ImmichZipStream;
createReadStream(filepath: string, mimeType?: string | null): Promise<ImmichReadStream>;
readFile(filepath: string, options?: FileReadOptions<Buffer>): Promise<Buffer>;
writeFile(filepath: string, buffer: Buffer): Promise<void>;
createFile(filepath: string, buffer: Buffer): Promise<void>;
createOrOverwriteFile(filepath: string, buffer: Buffer): Promise<void>;
overwriteFile(filepath: string, buffer: Buffer): Promise<void>;
realpath(filepath: string): Promise<string>;
unlink(filepath: string): Promise<void>;
unlinkDir(folder: string, options?: { recursive?: boolean; force?: boolean }): Promise<void>;

View File

@ -40,8 +40,16 @@ export class StorageRepository implements IStorageRepository {
return fs.stat(filepath);
}
writeFile(filepath: string, buffer: Buffer) {
return fs.writeFile(filepath, buffer);
createFile(filepath: string, buffer: Buffer) {
return fs.writeFile(filepath, buffer, { flag: 'wx' });
}
createOrOverwriteFile(filepath: string, buffer: Buffer) {
return fs.writeFile(filepath, buffer, { flag: 'w' });
}
overwriteFile(filepath: string, buffer: Buffer) {
return fs.writeFile(filepath, buffer, { flag: 'r+' });
}
rename(source: string, target: string) {

View File

@ -511,7 +511,7 @@ describe(MetadataService.name, () => {
await sut.handleMetadataExtraction({ id: assetStub.livePhotoMotionAsset.id });
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoMotionAsset.id]);
expect(storageMock.writeFile).not.toHaveBeenCalled();
expect(storageMock.createOrOverwriteFile).not.toHaveBeenCalled();
expect(jobMock.queue).not.toHaveBeenCalled();
expect(jobMock.queueAll).not.toHaveBeenCalled();
expect(assetMock.update).not.toHaveBeenCalledWith(
@ -581,7 +581,7 @@ describe(MetadataService.name, () => {
type: AssetType.VIDEO,
});
expect(userMock.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512);
expect(storageMock.writeFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video);
expect(storageMock.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video);
expect(assetMock.update).toHaveBeenNthCalledWith(1, {
id: assetStub.livePhotoWithOriginalFileName.id,
livePhotoVideoId: fileStub.livePhotoMotion.uuid,
@ -624,7 +624,7 @@ describe(MetadataService.name, () => {
type: AssetType.VIDEO,
});
expect(userMock.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512);
expect(storageMock.writeFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video);
expect(storageMock.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video);
expect(assetMock.update).toHaveBeenNthCalledWith(1, {
id: assetStub.livePhotoWithOriginalFileName.id,
livePhotoVideoId: fileStub.livePhotoMotion.uuid,
@ -668,7 +668,7 @@ describe(MetadataService.name, () => {
type: AssetType.VIDEO,
});
expect(userMock.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512);
expect(storageMock.writeFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video);
expect(storageMock.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video);
expect(assetMock.update).toHaveBeenNthCalledWith(1, {
id: assetStub.livePhotoWithOriginalFileName.id,
livePhotoVideoId: fileStub.livePhotoMotion.uuid,
@ -716,7 +716,7 @@ describe(MetadataService.name, () => {
await sut.handleMetadataExtraction({ id: assetStub.livePhotoStillAsset.id });
expect(assetMock.create).toHaveBeenCalledTimes(0);
expect(storageMock.writeFile).toHaveBeenCalledTimes(0);
expect(storageMock.createOrOverwriteFile).toHaveBeenCalledTimes(0);
// The still asset gets saved by handleMetadataExtraction, but not the video
expect(assetMock.update).toHaveBeenCalledTimes(1);
expect(jobMock.queue).toHaveBeenCalledTimes(0);

View File

@ -529,7 +529,7 @@ export class MetadataService {
const existsOnDisk = await this.storageRepository.checkFileExists(motionAsset.originalPath);
if (!existsOnDisk) {
this.storageCore.ensureFolders(motionAsset.originalPath);
await this.storageRepository.writeFile(motionAsset.originalPath, video);
await this.storageRepository.createFile(motionAsset.originalPath, video);
this.logger.log(`Wrote motion photo video to ${motionAsset.originalPath}`);
await this.jobRepository.queue({ name: JobName.METADATA_EXTRACTION, data: { id: motionAsset.id } });
}

View File

@ -41,6 +41,11 @@ describe(StorageService.name, () => {
expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/library');
expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/profile');
expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/thumbs');
expect(storageMock.createFile).toHaveBeenCalledWith('upload/encoded-video/.immich', expect.any(Buffer));
expect(storageMock.createFile).toHaveBeenCalledWith('upload/library/.immich', expect.any(Buffer));
expect(storageMock.createFile).toHaveBeenCalledWith('upload/profile/.immich', expect.any(Buffer));
expect(storageMock.createFile).toHaveBeenCalledWith('upload/thumbs/.immich', expect.any(Buffer));
expect(storageMock.createFile).toHaveBeenCalledWith('upload/upload/.immich', expect.any(Buffer));
});
it('should throw an error if .immich is missing', async () => {
@ -49,13 +54,13 @@ describe(StorageService.name, () => {
await expect(sut.onBootstrap()).rejects.toThrow('Failed to validate folder mount');
expect(storageMock.writeFile).not.toHaveBeenCalled();
expect(storageMock.createOrOverwriteFile).not.toHaveBeenCalled();
expect(systemMock.set).not.toHaveBeenCalled();
});
it('should throw an error if .immich is present but read-only', async () => {
systemMock.get.mockResolvedValue({ mountFiles: true });
storageMock.writeFile.mockRejectedValue(new Error("ENOENT: no such file or directory, open '/app/.immich'"));
storageMock.overwriteFile.mockRejectedValue(new Error("ENOENT: no such file or directory, open '/app/.immich'"));
await expect(sut.onBootstrap()).rejects.toThrow('Failed to validate folder mount');

View File

@ -32,7 +32,7 @@ export class StorageService {
for (const folder of Object.values(StorageFolder)) {
if (!flags.mountFiles) {
this.logger.log(`Writing initial mount file for the ${folder} folder`);
await this.verifyWriteAccess(folder);
await this.createMountFile(folder);
}
await this.verifyReadAccess(folder);
@ -81,17 +81,30 @@ export class StorageService {
}
}
private async verifyWriteAccess(folder: StorageFolder) {
private async createMountFile(folder: StorageFolder) {
const { folderPath, filePath } = this.getMountFilePaths(folder);
try {
this.storageRepository.mkdirSync(folderPath);
await this.storageRepository.writeFile(filePath, Buffer.from(`${Date.now()}`));
await this.storageRepository.createFile(filePath, Buffer.from(`${Date.now()}`));
} catch (error) {
this.logger.error(`Failed to create ${filePath}: ${error}`);
this.logger.error(
`The "${folder}" folder cannot be written to, please make sure the volume is mounted with the correct permissions`,
);
throw new ImmichStartupError(`Failed to validate folder mount (write to "<UPLOAD_LOCATION>/${folder}")`);
}
}
private async verifyWriteAccess(folder: StorageFolder) {
const { filePath } = this.getMountFilePaths(folder);
try {
await this.storageRepository.overwriteFile(filePath, Buffer.from(`${Date.now()}`));
} catch (error) {
this.logger.error(`Failed to write ${filePath}: ${error}`);
this.logger.error(
`The "${folder}" folder cannot be written to, please make sure the volume is mounted with the correct permissions`,
);
throw new ImmichStartupError(`Failed to validate folder mount (write to "<MEDIA_LOCATION>/${folder}")`);
throw new ImmichStartupError(`Failed to validate folder mount (write to "<UPLOAD_LOCATION>/${folder}")`);
}
}

View File

@ -48,7 +48,9 @@ export const newStorageRepositoryMock = (reset = true): Mocked<IStorageRepositor
createZipStream: vitest.fn(),
createReadStream: vitest.fn(),
readFile: vitest.fn(),
writeFile: vitest.fn(),
createFile: vitest.fn(),
createOrOverwriteFile: vitest.fn(),
overwriteFile: vitest.fn(),
unlink: vitest.fn(),
unlinkDir: vitest.fn().mockResolvedValue(true),
removeEmptyDirs: vitest.fn(),