fix(mobile): handle missing extension during uploads

* fix(maintenance): prevent CLI hanging on occassion
fix(maintenance): always ack messages
fix(maintenance): ensure Redis is connected first

* chore(maintenance): validate app restart responses

* chore: mock the app restart callback

* fix: ack may not exist depending on caller

* refactor: move one shot into app.repository

* fix: send correct state in one shot

* chore: log restart event

mobile/lib/services/upload.service.dart

@@ -271,12 +271,12 @@ class UploadService {
       return null;
     }
 
-    final file = await _storageRepository.getFileForAsset(asset.id);
-    if (file == null) {
+    final uploadFileResult = await prepareUploadFile(asset, isLivePhoto: entity.isLivePhoto);
+    if (uploadFileResult == null) {
       return null;
     }
 
-    final originalFileName = entity.isLivePhoto ? p.setExtension(asset.name, p.extension(file.path)) : asset.name;
+    final (:file, :originalFilename) = uploadFileResult;
 
     String metadata = UploadTaskMetadata(
       localAssetId: asset.id,
@@ -290,7 +290,7 @@ class UploadService {
         file,
         createdAt: asset.createdAt,
         modifiedAt: asset.updatedAt,
-        originalFileName: originalFileName,
+        originalFileName: originalFilename,
         deviceAssetId: asset.id,
         metadata: metadata,
         group: "group",
@@ -308,8 +308,6 @@ class UploadService {
       return null;
     }
 
-    File? file;
-
     /// iOS LivePhoto has two files: a photo and a video.
     /// They are uploaded separately, with video file being upload first, then returned with the assetId
     /// The assetId is then used as a metadata for the photo file upload task.
@@ -320,18 +318,12 @@ class UploadService {
     /// The cancel operation will only cancel the video group (normal group), the photo group will not
     /// be touched, as the video file is already uploaded.
 
-    if (entity.isLivePhoto) {
-      file = await _storageRepository.getMotionFileForAsset(asset);
-    } else {
-      file = await _storageRepository.getFileForAsset(asset.id);
-    }
-
-    if (file == null) {
+    final uploadFileResult = await prepareUploadFile(asset, isLivePhoto: entity.isLivePhoto);
+    if (uploadFileResult == null) {
       return null;
     }
 
-    final fileName = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name;
-    final originalFileName = entity.isLivePhoto ? p.setExtension(fileName, p.extension(file.path)) : fileName;
+    final (:file, :originalFilename) = uploadFileResult;
 
     String metadata = UploadTaskMetadata(
       localAssetId: asset.id,
@@ -345,7 +337,7 @@ class UploadService {
       file,
       createdAt: asset.createdAt,
       modifiedAt: asset.updatedAt,
-      originalFileName: originalFileName,
+      originalFileName: originalFilename,
       deviceAssetId: asset.id,
       metadata: metadata,
       group: group,
@@ -362,21 +354,20 @@ class UploadService {
       return null;
     }
 
-    final file = await _storageRepository.getFileForAsset(asset.id);
-    if (file == null) {
+    final result = await prepareUploadFile(asset);
+    if (result == null) {
       return null;
     }
 
     final fields = {'livePhotoVideoId': livePhotoVideoId};
 
     final requiresWiFi = _shouldRequireWiFi(asset);
-    final originalFileName = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name;
 
     return buildUploadTask(
-      file,
+      result.file,
       createdAt: asset.createdAt,
       modifiedAt: asset.updatedAt,
-      originalFileName: originalFileName,
+      originalFileName: result.originalFilename,
       deviceAssetId: asset.id,
       fields: fields,
       group: kBackupLivePhotoGroup,
@@ -398,6 +389,54 @@ class UploadService {
     return requiresWiFi;
   }
 
+  @visibleForTesting
+  Future<({File file, String originalFilename})?> prepareUploadFile(
+    LocalAsset asset, {
+    bool isLivePhoto = false,
+  }) async {
+    final file = isLivePhoto
+        ? await _storageRepository.getMotionFileForAsset(asset)
+        : await _storageRepository.getFileForAsset(asset.id);
+
+    if (file == null) {
+      return null;
+    }
+
+    final originalFilename = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name;
+
+    if (isLivePhoto) {
+      final livePhotoFilename = p.setExtension(originalFilename, p.extension(file.path));
+      return (file: file, originalFilename: livePhotoFilename);
+    }
+
+    final filenameExt = p.extension(originalFilename);
+    if (filenameExt.isNotEmpty) {
+      return (file: file, originalFilename: originalFilename);
+    }
+
+    final assetNameExt = p.extension(asset.name);
+    if (assetNameExt.isNotEmpty) {
+      final correctedFilename = p.setExtension(originalFilename, assetNameExt);
+      _logger.fine(
+        "Corrected filename $originalFilename to $correctedFilename using asset.name extension $assetNameExt",
+      );
+      return (file: file, originalFilename: correctedFilename);
+    }
+
+    final filePathExt = p.extension(file.path);
+    if (filePathExt.isEmpty) {
+      _logger.warning(
+        "Asset ${asset.id} has no file extension in any source, using original filename - $originalFilename",
+      );
+      return (file: file, originalFilename: originalFilename);
+    }
+
+    final correctedFilename = p.setExtension(originalFilename, filePathExt);
+    _logger.fine("Corrected filename $originalFilename to $correctedFilename using file path extension $filePathExt");
+
+    return (file: file, originalFilename: correctedFilename);
+  }
+
   Future<UploadTask> buildUploadTask(
     File file, {
     required String group,

mobile/test/services/upload.service_test.dart

@@ -4,6 +4,7 @@ import 'package:drift/drift.dart' hide isNull, isNotNull;
 import 'package:drift/native.dart';
 import 'package:flutter/services.dart';
 import 'package:flutter_test/flutter_test.dart';
+import 'package:immich_mobile/domain/models/asset/base_asset.model.dart';
 import 'package:immich_mobile/domain/models/store.model.dart';
 import 'package:immich_mobile/domain/services/store.service.dart';
 import 'package:immich_mobile/entities/store.entity.dart';
@@ -16,8 +17,8 @@ import 'package:mocktail/mocktail.dart';
 import '../domain/service.mock.dart';
 import '../fixtures/asset.stub.dart';
 import '../infrastructure/repository.mock.dart';
-import '../repository.mocks.dart';
 import '../mocks/asset_entity.mock.dart';
+import '../repository.mocks.dart';
 
 void main() {
   late UploadService sut;
@@ -165,4 +166,174 @@ void main() {
       verify(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).called(1);
     });
   });
+
+  group('prepareUploadFile', () {
+    test('should keep filename with existing extension unchanged', () async {
+      final asset = LocalAssetStub.image1;
+      final mockFile = File('/tmp/123.jpg');
+
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'photo.jpg');
+
+      final result = await sut.prepareUploadFile(asset);
+      expect(result, isNotNull);
+      expect(result!.file.path, equals('/tmp/123.jpg'));
+      expect(result.originalFilename, equals('photo.jpg'));
+    });
+
+    test('should use asset.name extension when original filename lacks one', () async {
+      final asset = LocalAssetStub.image1;
+      final mockFile = File('/tmp/cache/123.mov');
+
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => '2024-10-23_17-00-30');
+
+      final result = await sut.prepareUploadFile(asset);
+      expect(result, isNotNull);
+      expect(result!.originalFilename, equals('2024-10-23_17-00-30.jpg'));
+    });
+
+    test('should use file path extension as final fallback', () async {
+      final asset = LocalAssetStub.image1.copyWith(name: 'document');
+      final mockFile = File('/tmp/cache/123.mov');
+
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'document');
+
+      final result = await sut.prepareUploadFile(asset);
+      expect(result, isNotNull);
+      expect(result!.originalFilename, equals('document.mov'));
+    });
+
+    test('should handle file without extension anywhere', () async {
+      final asset = LocalAssetStub.image1.copyWith(name: 'document');
+      final mockFile = File('/tmp/temp');
+
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'document');
+
+      final result = await sut.prepareUploadFile(asset);
+      expect(result, isNotNull);
+      expect(result!.originalFilename, equals('document'));
+    });
+
+    test('should preserve existing extension even if asset.name has different one', () async {
+      final asset = LocalAssetStub.image1;
+      final mockFile = File('/tmp/123.mov');
+
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'photo.HEIC');
+
+      final result = await sut.prepareUploadFile(asset);
+      expect(result, isNotNull);
+      expect(result!.originalFilename, equals('photo.HEIC'));
+    });
+
+    test('should fall back to asset.name when getOriginalFilename returns null', () async {
+      final asset = LocalAssetStub.image1.copyWith(name: 'VID_1234.mp4');
+      final mockFile = File('/tmp/video.mov');
+
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => null);
+
+      final result = await sut.prepareUploadFile(asset);
+      expect(result, isNotNull);
+      expect(result!.originalFilename, equals('VID_1234.mp4')); // Uses asset.name directly
+    });
+
+    test('should return null when file is not found', () async {
+      final asset = LocalAssetStub.image1;
+
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => null);
+
+      final result = await sut.prepareUploadFile(asset);
+      expect(result, isNull);
+    });
+  });
+
+  group('getUploadTask with missing extensions', () {
+    test('should add extension for regular photo without extension', () async {
+      final asset = LocalAssetStub.image1;
+      final mockEntity = MockAssetEntity();
+      final mockFile = File('/path/to/file.jpg');
+
+      when(() => mockEntity.isLivePhoto).thenReturn(false);
+      when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => '2024-10-23_17-00-30');
+
+      final task = await sut.getUploadTask(asset);
+
+      expect(task, isNotNull);
+      expect(task!.fields['filename'], equals('2024-10-23_17-00-30.jpg'));
+    });
+
+    test('should preserve existing extension for regular photo', () async {
+      final asset = LocalAssetStub.image1;
+      final mockEntity = MockAssetEntity();
+      final mockFile = File('/path/to/file.jpg');
+
+      when(() => mockEntity.isLivePhoto).thenReturn(false);
+      when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'MyPhoto.HEIC');
+
+      final task = await sut.getUploadTask(asset);
+
+      expect(task, isNotNull);
+      expect(task!.fields['filename'], equals('MyPhoto.HEIC'));
+    });
+
+    test('should add extension for video without extension', () async {
+      // Create a video asset using copyWith since image2 is a video type
+      final asset = LocalAssetStub.image1.copyWith(id: 'video1', name: 'VID_20241023_170030', type: AssetType.video);
+      final mockEntity = MockAssetEntity();
+      final mockFile = File('/path/to/video.mov');
+
+      when(() => mockEntity.isLivePhoto).thenReturn(false);
+      when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'VID_20241023_170030');
+
+      final task = await sut.getUploadTask(asset);
+
+      expect(task, isNotNull);
+      expect(task!.fields['filename'], equals('VID_20241023_170030.mov'));
+    });
+  });
+
+  group('getLivePhotoUploadTask with missing extensions', () {
+    test('should add extension when live photo filename lacks one', () async {
+      final asset = LocalAssetStub.image1.copyWith(name: 'IMG_1234.heic');
+      final mockEntity = MockAssetEntity();
+      final mockFile = File('/path/to/photo.heic');
+
+      when(() => mockEntity.isLivePhoto).thenReturn(true);
+      when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'IMG_1234');
+
+      final task = await sut.getLivePhotoUploadTask(asset, 'video-id-123');
+
+      expect(task, isNotNull);
+      expect(task!.fields['filename'], equals('IMG_1234.heic'));
+      expect(task.fields['livePhotoVideoId'], equals('video-id-123'));
+    });
+
+    test('should preserve extension when live photo filename has one', () async {
+      final asset = LocalAssetStub.image1;
+      final mockEntity = MockAssetEntity();
+      final mockFile = File('/path/to/photo.heic');
+
+      when(() => mockEntity.isLivePhoto).thenReturn(true);
+      when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
+      when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
+      when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'MyLivePhoto.HEIC');
+
+      final task = await sut.getLivePhotoUploadTask(asset, 'video-id-456');
+
+      expect(task, isNotNull);
+      expect(task!.fields['filename'], equals('MyLivePhoto.HEIC'));
+    });
+  });
 }

server/src/maintenance/maintenance-websocket.repository.ts

@@ -37,7 +37,13 @@ export class MaintenanceWebsocketRepository implements OnGatewayConnection, OnGa
 
   afterInit(websocketServer: Server) {
     this.logger.log('Initialized websocket server');
-    websocketServer.on('AppRestart', () => this.appRepository.exitApp());
+
+    websocketServer.on('AppRestart', (event: ArgsOf<'AppRestart'>, ack?: (ok: 'ok') => void) => {
+      this.logger.log(`Restarting due to event... ${JSON.stringify(event)}`);
+
+      ack?.('ok');
+      this.appRepository.exitApp();
+    });
   }
 
   clientBroadcast<T extends keyof ClientEventMap>(event: T, ...data: ClientEventMap[T]) {

server/src/repositories/app.repository.ts

@@ -1,5 +1,10 @@
 import { Injectable } from '@nestjs/common';
+import { createAdapter } from '@socket.io/redis-adapter';
+import Redis from 'ioredis';
+import { Server as SocketIO } from 'socket.io';
 import { ExitCode } from 'src/enum';
+import { ConfigRepository } from 'src/repositories/config.repository';
+import { AppRestartEvent } from 'src/repositories/event.repository';
 
 @Injectable()
 export class AppRepository {
@@ -17,4 +22,26 @@ export class AppRepository {
   setCloseFn(fn: () => Promise<void>) {
     this.closeFn = fn;
   }
+
+  async sendOneShotAppRestart(state: AppRestartEvent): Promise<void> {
+    const server = new SocketIO();
+    const { redis } = new ConfigRepository().getEnv();
+    const pubClient = new Redis({ ...redis, lazyConnect: true });
+    const subClient = pubClient.duplicate();
+
+    await Promise.all([pubClient.connect(), subClient.connect()]);
+
+    server.adapter(createAdapter(pubClient, subClient));
+
+    // => corresponds to notification.service.ts#onAppRestart
+    server.emit('AppRestartV1', state, async () => {
+      const responses = await server.serverSideEmitWithAck('AppRestart', state);
+      if (responses.some((response) => response !== 'ok')) {
+        throw new Error("One or more node(s) returned a non-'ok' response to our restart request!");
+      }
+
+      pubClient.disconnect();
+      subClient.disconnect();
+    });
+  }
 }

server/src/services/cli.service.spec.ts

@@ -89,6 +89,7 @@ describe(CliService.name, () => {
         alreadyDisabled: true,
       });
 
+      expect(mocks.app.sendOneShotAppRestart).toHaveBeenCalledTimes(0);
       expect(mocks.systemMetadata.set).toHaveBeenCalledTimes(0);
       expect(mocks.event.emit).toHaveBeenCalledTimes(0);
     });
@@ -99,6 +100,7 @@ describe(CliService.name, () => {
         alreadyDisabled: false,
       });
 
+      expect(mocks.app.sendOneShotAppRestart).toHaveBeenCalled();
       expect(mocks.systemMetadata.set).toHaveBeenCalledWith(SystemMetadataKey.MaintenanceMode, {
         isMaintenanceMode: false,
       });
@@ -114,6 +116,7 @@ describe(CliService.name, () => {
         }),
       );
 
+      expect(mocks.app.sendOneShotAppRestart).toHaveBeenCalledTimes(0);
       expect(mocks.systemMetadata.set).toHaveBeenCalledTimes(0);
       expect(mocks.event.emit).toHaveBeenCalledTimes(0);
     });
@@ -126,6 +129,7 @@ describe(CliService.name, () => {
         }),
       );
 
+      expect(mocks.app.sendOneShotAppRestart).toHaveBeenCalled();
       expect(mocks.systemMetadata.set).toHaveBeenCalledWith(SystemMetadataKey.MaintenanceMode, {
         isMaintenanceMode: true,
         secret: expect.stringMatching(/^\w{128}$/),

server/src/services/cli.service.ts

@@ -5,7 +5,7 @@ import { MaintenanceAuthDto } from 'src/dtos/maintenance.dto';
 import { UserAdminResponseDto, mapUserAdmin } from 'src/dtos/user.dto';
 import { SystemMetadataKey } from 'src/enum';
 import { BaseService } from 'src/services/base.service';
-import { createMaintenanceLoginUrl, generateMaintenanceSecret, sendOneShotAppRestart } from 'src/utils/maintenance';
+import { createMaintenanceLoginUrl, generateMaintenanceSecret } from 'src/utils/maintenance';
 import { getExternalDomain } from 'src/utils/misc';
 
 @Injectable()
@@ -55,8 +55,7 @@ export class CliService extends BaseService {
 
     const state = { isMaintenanceMode: false as const };
     await this.systemMetadataRepository.set(SystemMetadataKey.MaintenanceMode, state);
-
-    sendOneShotAppRestart(state);
+    await this.appRepository.sendOneShotAppRestart(state);
 
     return {
       alreadyDisabled: false,
@@ -89,7 +88,7 @@ export class CliService extends BaseService {
       secret,
     });
 
-    sendOneShotAppRestart({
+    await this.appRepository.sendOneShotAppRestart({
       isMaintenanceMode: true,
     });
 

server/src/services/maintenance.service.ts

@@ -2,6 +2,7 @@ import { Injectable } from '@nestjs/common';
 import { OnEvent } from 'src/decorators';
 import { MaintenanceAuthDto } from 'src/dtos/maintenance.dto';
 import { SystemMetadataKey } from 'src/enum';
+import { ArgOf } from 'src/repositories/event.repository';
 import { BaseService } from 'src/services/base.service';
 import { MaintenanceModeState } from 'src/types';
 import { createMaintenanceLoginUrl, generateMaintenanceSecret, signMaintenanceJwt } from 'src/utils/maintenance';
@@ -31,7 +32,10 @@ export class MaintenanceService extends BaseService {
   }
 
   @OnEvent({ name: 'AppRestart', server: true })
-  onRestart(): void {
+  onRestart(event: ArgOf<'AppRestart'>, ack?: (ok: 'ok') => void): void {
+    this.logger.log(`Restarting due to event... ${JSON.stringify(event)}`);
+
+    ack?.('ok');
     this.appRepository.exitApp();
   }
 

server/src/utils/maintenance.ts

@@ -1,55 +1,6 @@
-import { createAdapter } from '@socket.io/redis-adapter';
-import Redis from 'ioredis';
 import { SignJWT } from 'jose';
 import { randomBytes } from 'node:crypto';
-import { Server as SocketIO } from 'socket.io';
 import { MaintenanceAuthDto } from 'src/dtos/maintenance.dto';
-import { ConfigRepository } from 'src/repositories/config.repository';
-import { AppRestartEvent } from 'src/repositories/event.repository';
-
-export function sendOneShotAppRestart(state: AppRestartEvent): void {
-  const server = new SocketIO();
-  const { redis } = new ConfigRepository().getEnv();
-  const pubClient = new Redis(redis);
-  const subClient = pubClient.duplicate();
-  server.adapter(createAdapter(pubClient, subClient));
-
-  /**
-   * Keep trying until we manage to stop Immich
-   *
-   * Sometimes there appear to be communication
-   * issues between to the other servers.
-   *
-   * This issue only occurs with this method.
-   */
-  async function tryTerminate() {
-    while (true) {
-      try {
-        const responses = await server.serverSideEmitWithAck('AppRestart', state);
-        if (responses.length > 0) {
-          return;
-        }
-      } catch (error) {
-        console.error(error);
-        console.error('Encountered an error while telling Immich to stop.');
-      }
-
-      console.info(
-        "\nIt doesn't appear that Immich stopped, trying again in a moment.\nIf Immich is already not running, you can ignore this error.",
-      );
-
-      await new Promise((r) => setTimeout(r, 1e3));
-    }
-  }
-
-  // => corresponds to notification.service.ts#onAppRestart
-  server.emit('AppRestartV1', state, () => {
-    void tryTerminate().finally(() => {
-      pubClient.disconnect();
-      subClient.disconnect();
-    });
-  });
-}
 
 export async function createMaintenanceLoginUrl(
   baseUrl: string,