Skip to content
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

refactor: config init event for first config load #13930

Merged
merged 2 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions server/src/app.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,11 @@ class BaseModule implements OnModuleInit, OnModuleDestroy {
}

this.eventRepository.setup({ services });
await this.eventRepository.emit('app.bootstrap', this.worker);
await this.eventRepository.emit('app.bootstrap');
}

async onModuleDestroy() {
await this.eventRepository.emit('app.shutdown', this.worker);
await this.eventRepository.emit('app.shutdown');
await teardownTelemetry();
}
}
Expand Down
16 changes: 11 additions & 5 deletions server/src/interfaces/event.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,21 @@ import { ClassConstructor } from 'class-transformer';
import { SystemConfig } from 'src/config';
import { AssetResponseDto } from 'src/dtos/asset-response.dto';
import { ReleaseNotification, ServerVersionResponseDto } from 'src/dtos/server.dto';
import { ImmichWorker } from 'src/enum';
import { JobItem, QueueName } from 'src/interfaces/job.interface';

export const IEventRepository = 'IEventRepository';

type EventMap = {
// app events
'app.bootstrap': [ImmichWorker];
'app.shutdown': [ImmichWorker];
'app.bootstrap': [];
'app.shutdown': [];

'config.init': [{ newConfig: SystemConfig }];
// config events
'config.update': [
{
newConfig: SystemConfig;
/** When the server starts, `oldConfig` is `undefined` */
oldConfig?: SystemConfig;
oldConfig: SystemConfig;
},
];
'config.validate': [{ newConfig: SystemConfig; oldConfig: SystemConfig }];
Expand Down Expand Up @@ -89,6 +88,13 @@ export type EventItem<T extends EmitEvent> = {
server: boolean;
};

export enum BootstrapEventPriority {
zackpollard marked this conversation as resolved.
Show resolved Hide resolved
// Database service should be initialized before anything else, most other services need database access
DatabaseService = -200,
// Initialise config after other bootstrap services, stop other services from using config on bootstrap
SystemConfig = 100,
}

export interface IEventRepository {
setup(options: { services: ClassConstructor<unknown>[] }): void;
emit<T extends keyof EventMap>(event: T, ...args: ArgsOf<T>): Promise<void>;
Expand Down
24 changes: 9 additions & 15 deletions server/src/services/backup.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { PassThrough } from 'node:stream';
import { defaults, SystemConfig } from 'src/config';
import { StorageCore } from 'src/cores/storage.core';
import { ImmichWorker, StorageFolder } from 'src/enum';
import { IConfigRepository } from 'src/interfaces/config.interface';
import { IDatabaseRepository } from 'src/interfaces/database.interface';
import { IJobRepository, JobStatus } from 'src/interfaces/job.interface';
import { IProcessRepository } from 'src/interfaces/process.interface';
Expand All @@ -16,13 +17,14 @@ describe(BackupService.name, () => {
let sut: BackupService;

let databaseMock: Mocked<IDatabaseRepository>;
let configMock: Mocked<IConfigRepository>;
let jobMock: Mocked<IJobRepository>;
let processMock: Mocked<IProcessRepository>;
let storageMock: Mocked<IStorageRepository>;
let systemMock: Mocked<ISystemMetadataRepository>;

beforeEach(() => {
({ sut, databaseMock, jobMock, processMock, storageMock, systemMock } = newTestService(BackupService));
({ sut, configMock, databaseMock, jobMock, processMock, storageMock, systemMock } = newTestService(BackupService));
});

it('should work', () => {
Expand All @@ -32,35 +34,32 @@ describe(BackupService.name, () => {
describe('onBootstrapEvent', () => {
it('should init cron job and handle config changes', async () => {
databaseMock.tryLock.mockResolvedValue(true);
systemMock.get.mockResolvedValue(systemConfigStub.backupEnabled);

await sut.onBootstrap(ImmichWorker.API);
await sut.onConfigInit({ newConfig: systemConfigStub.backupEnabled as SystemConfig });

expect(jobMock.addCronJob).toHaveBeenCalled();
expect(systemMock.get).toHaveBeenCalled();
});

it('should not initialize backup database cron job when lock is taken', async () => {
systemMock.get.mockResolvedValue(systemConfigStub.backupEnabled);
databaseMock.tryLock.mockResolvedValue(false);

await sut.onBootstrap(ImmichWorker.API);
await sut.onConfigInit({ newConfig: systemConfigStub.backupEnabled as SystemConfig });

expect(jobMock.addCronJob).not.toHaveBeenCalled();
});

it('should not initialise backup database job when running on microservices', async () => {
await sut.onBootstrap(ImmichWorker.MICROSERVICES);
configMock.getWorker.mockReturnValue(ImmichWorker.MICROSERVICES);
await sut.onConfigInit({ newConfig: systemConfigStub.backupEnabled as SystemConfig });

expect(jobMock.addCronJob).not.toHaveBeenCalled();
});
});

describe('onConfigUpdateEvent', () => {
beforeEach(async () => {
systemMock.get.mockResolvedValue(defaults);
databaseMock.tryLock.mockResolvedValue(true);
await sut.onBootstrap(ImmichWorker.API);
await sut.onConfigInit({ newConfig: defaults });
});

it('should update cron job if backup is enabled', () => {
Expand All @@ -80,14 +79,9 @@ describe(BackupService.name, () => {
expect(jobMock.updateCronJob).toHaveBeenCalled();
});

it('should do nothing if oldConfig is not provided', () => {
sut.onConfigUpdate({ newConfig: systemConfigStub.backupEnabled as SystemConfig });
expect(jobMock.updateCronJob).not.toHaveBeenCalled();
});

it('should do nothing if instance does not have the backup database lock', async () => {
databaseMock.tryLock.mockResolvedValue(false);
await sut.onBootstrap(ImmichWorker.API);
await sut.onConfigInit({ newConfig: defaults });
sut.onConfigUpdate({ newConfig: systemConfigStub.backupEnabled as SystemConfig, oldConfig: defaults });
expect(jobMock.updateCronJob).not.toHaveBeenCalled();
});
Expand Down
17 changes: 9 additions & 8 deletions server/src/services/backup.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@ import { validateCronExpression } from 'src/validation';
export class BackupService extends BaseService {
private backupLock = false;

@OnEvent({ name: 'app.bootstrap' })
async onBootstrap(workerType: ImmichWorker) {
if (workerType !== ImmichWorker.API) {
@OnEvent({ name: 'config.init' })
async onConfigInit({
newConfig: {
backup: { database },
},
}: ArgOf<'config.init'>) {
if (this.worker !== ImmichWorker.API) {
return;
}
const {
backup: { database },
} = await this.getConfig({ withCache: true });

this.backupLock = await this.databaseRepository.tryLock(DatabaseLock.BackupDatabase);

Expand All @@ -36,8 +37,8 @@ export class BackupService extends BaseService {
}

@OnEvent({ name: 'config.update', server: true })
onConfigUpdate({ newConfig: { backup }, oldConfig }: ArgOf<'config.update'>) {
if (!oldConfig || !this.backupLock) {
onConfigUpdate({ newConfig: { backup } }: ArgOf<'config.update'>) {
if (!this.backupLock) {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion server/src/services/database.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
VectorExtension,
VectorIndex,
} from 'src/interfaces/database.interface';
import { BootstrapEventPriority } from 'src/interfaces/event.interface';
import { BaseService } from 'src/services/base.service';

type CreateFailedArgs = { name: string; extension: string; otherName: string };
Expand Down Expand Up @@ -64,7 +65,7 @@ const RETRY_DURATION = Duration.fromObject({ seconds: 5 });
export class DatabaseService extends BaseService {
private reconnection?: NodeJS.Timeout;

@OnEvent({ name: 'app.bootstrap', priority: -200 })
@OnEvent({ name: 'app.bootstrap', priority: BootstrapEventPriority.DatabaseService })
async onBootstrap() {
const version = await this.databaseRepository.getPostgresVersion();
const current = semver.coerce(version);
Expand Down
2 changes: 1 addition & 1 deletion server/src/services/job.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe(JobService.name, () => {

describe('onConfigUpdate', () => {
it('should update concurrency', () => {
sut.onConfigUpdate({ oldConfig: defaults, newConfig: defaults });
sut.onConfigInitOrUpdate({ newConfig: defaults });

expect(jobMock.setConcurrency).toHaveBeenCalledTimes(15);
expect(jobMock.setConcurrency).toHaveBeenNthCalledWith(5, QueueName.FACIAL_RECOGNITION, 1);
Expand Down
3 changes: 2 additions & 1 deletion server/src/services/job.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ const asJobItem = (dto: JobCreateDto): JobItem => {

@Injectable()
export class JobService extends BaseService {
@OnEvent({ name: 'config.init' })
@OnEvent({ name: 'config.update', server: true })
onConfigUpdate({ newConfig: config }: ArgOf<'config.update'>) {
onConfigInitOrUpdate({ newConfig: config }: ArgOf<'config.init'>) {
if (this.worker !== ImmichWorker.MICROSERVICES) {
return;
}
Expand Down
Loading
Loading