From ca4e2db1f29126a1fa3784af563832edda64b0ca Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Wed, 9 Jul 2025 13:16:53 -0400 Subject: [PATCH] fix: event emitter setup for writing status (#1496) ## Summary by CodeRabbit * **Chores** * Updated permissions to allow additional Bash command patterns in the configuration. * Improved connection status updates by triggering them via event listeners during application bootstrap. * Adjusted module provider registrations to reflect service relocation within the application structure. * **Tests** * Added comprehensive unit and integration tests for connection status writing and cleanup behaviors. --- .claude/settings.local.json | 5 +- .../connect-status-writer.config.spec.ts | 158 +++++++++++++++++ .../connect-status-writer.integration.spec.ts | 167 ++++++++++++++++++ .../connect-status-writer.service.spec.ts | 140 +++++++++++++++ .../connect-status-writer.service.ts | 46 ++--- .../unraid-api-plugin-connect/src/index.ts | 3 +- .../src/mothership-proxy/mothership.module.ts | 4 +- 7 files changed, 496 insertions(+), 27 deletions(-) create mode 100644 packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts create mode 100644 packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts create mode 100644 packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.ts rename packages/unraid-api-plugin-connect/src/{config => connection-status}/connect-status-writer.service.ts (68%) diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 3022f1b0b..0b7ab7293 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -11,7 +11,10 @@ "Bash(pnpm type-check:*)", "Bash(pnpm lint:*)", "Bash(pnpm --filter ./api lint)", - "Bash(mv:*)" + "Bash(mv:*)", + "Bash(ls:*)", + "mcp__ide__getDiagnostics", + "Bash(pnpm --filter \"*connect*\" test connect-status-writer.service.spec)" ] }, "enableAllProjectMcpServers": false diff --git a/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts new file mode 100644 index 000000000..b110ea3f0 --- /dev/null +++ b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts @@ -0,0 +1,158 @@ +import { ConfigService } from '@nestjs/config'; +import { access, constants, mkdir, readFile, rm } from 'fs/promises'; +import { join } from 'path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { ConfigType } from '../config/connect.config.js'; +import { ConnectStatusWriterService } from './connect-status-writer.service.js'; + +describe('ConnectStatusWriterService Config Behavior', () => { + let service: ConnectStatusWriterService; + let configService: ConfigService; + const testDir = '/tmp/connect-status-config-test'; + const testFilePath = join(testDir, 'connectStatus.json'); + + // Simulate config changes + let configStore: any = {}; + + beforeEach(async () => { + vi.clearAllMocks(); + + // Reset config store + configStore = {}; + + // Create test directory + await mkdir(testDir, { recursive: true }); + + // Create a ConfigService mock that behaves like the real one + configService = { + get: vi.fn().mockImplementation((key: string) => { + console.log(`ConfigService.get('${key}') called, returning:`, configStore[key]); + return configStore[key]; + }), + set: vi.fn().mockImplementation((key: string, value: any) => { + console.log(`ConfigService.set('${key}', ${JSON.stringify(value)}) called`); + configStore[key] = value; + }), + } as unknown as ConfigService; + + service = new ConnectStatusWriterService(configService); + + // Override the status file path to use our test location + Object.defineProperty(service, 'statusFilePath', { + get: () => testFilePath, + }); + }); + + afterEach(async () => { + await service.onModuleDestroy(); + await rm(testDir, { recursive: true, force: true }); + }); + + it('should write status when config is updated directly', async () => { + // Initialize service - should write PRE_INIT + await service.onApplicationBootstrap(); + await new Promise(resolve => setTimeout(resolve, 50)); + + let content = await readFile(testFilePath, 'utf-8'); + let data = JSON.parse(content); + console.log('Initial status:', data); + expect(data.connectionStatus).toBe('PRE_INIT'); + + // Update config directly (simulating what ConnectionService does) + console.log('\n=== Updating config to CONNECTED ==='); + configService.set('connect.mothership', { + status: 'CONNECTED', + error: null, + lastPing: Date.now(), + }); + + // Call the writeStatus method directly (since @OnEvent handles the event) + await service['writeStatus'](); + + content = await readFile(testFilePath, 'utf-8'); + data = JSON.parse(content); + console.log('Status after config update:', data); + expect(data.connectionStatus).toBe('CONNECTED'); + }); + + it('should test the actual flow with multiple status updates', async () => { + await service.onApplicationBootstrap(); + await new Promise(resolve => setTimeout(resolve, 50)); + + const statusUpdates = [ + { status: 'CONNECTING', error: null, lastPing: null }, + { status: 'CONNECTED', error: null, lastPing: Date.now() }, + { status: 'DISCONNECTED', error: 'Lost connection', lastPing: Date.now() - 10000 }, + { status: 'RECONNECTING', error: null, lastPing: Date.now() - 10000 }, + { status: 'CONNECTED', error: null, lastPing: Date.now() }, + ]; + + for (const update of statusUpdates) { + console.log(`\n=== Updating to ${update.status} ===`); + + // Update config + configService.set('connect.mothership', update); + + // Call writeStatus directly + await service['writeStatus'](); + + const content = await readFile(testFilePath, 'utf-8'); + const data = JSON.parse(content); + console.log(`Status file shows: ${data.connectionStatus}`); + expect(data.connectionStatus).toBe(update.status); + } + }); + + it('should handle case where config is not set before event', async () => { + await service.onApplicationBootstrap(); + await new Promise(resolve => setTimeout(resolve, 50)); + + // Delete the config + delete configStore['connect.mothership']; + + // Call writeStatus without config + console.log('\n=== Calling writeStatus with no config ==='); + await service['writeStatus'](); + + const content = await readFile(testFilePath, 'utf-8'); + const data = JSON.parse(content); + console.log('Status with no config:', data); + expect(data.connectionStatus).toBe('PRE_INIT'); + + // Now set config and call writeStatus again + console.log('\n=== Setting config and calling writeStatus ==='); + configService.set('connect.mothership', { + status: 'CONNECTED', + error: null, + lastPing: Date.now(), + }); + await service['writeStatus'](); + + const content2 = await readFile(testFilePath, 'utf-8'); + const data2 = JSON.parse(content2); + console.log('Status after setting config:', data2); + expect(data2.connectionStatus).toBe('CONNECTED'); + }); + + describe('cleanup on shutdown', () => { + it('should delete status file on module destroy', async () => { + await service.onApplicationBootstrap(); + await new Promise(resolve => setTimeout(resolve, 50)); + + // Verify file exists + await expect(access(testFilePath, constants.F_OK)).resolves.not.toThrow(); + + // Cleanup + await service.onModuleDestroy(); + + // Verify file is deleted + await expect(access(testFilePath, constants.F_OK)).rejects.toThrow(); + }); + + it('should handle cleanup when file does not exist', async () => { + // Don't bootstrap (so no file is written) + await expect(service.onModuleDestroy()).resolves.not.toThrow(); + }); + }); +}); \ No newline at end of file diff --git a/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts new file mode 100644 index 000000000..c75caa2fc --- /dev/null +++ b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts @@ -0,0 +1,167 @@ +import { ConfigService } from '@nestjs/config'; +import { access, constants, mkdir, readFile, rm } from 'fs/promises'; +import { join } from 'path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { ConfigType } from '../config/connect.config.js'; +import { ConnectStatusWriterService } from './connect-status-writer.service.js'; + +describe('ConnectStatusWriterService Integration', () => { + let service: ConnectStatusWriterService; + let configService: ConfigService; + const testDir = '/tmp/connect-status-test'; + const testFilePath = join(testDir, 'connectStatus.json'); + + beforeEach(async () => { + vi.clearAllMocks(); + + // Create test directory + await mkdir(testDir, { recursive: true }); + + configService = { + get: vi.fn().mockImplementation((key: string) => { + console.log(`ConfigService.get called with key: ${key}`); + return { + status: 'CONNECTED', + error: null, + lastPing: Date.now(), + }; + }), + } as unknown as ConfigService; + + service = new ConnectStatusWriterService(configService); + + // Override the status file path to use our test location + Object.defineProperty(service, 'statusFilePath', { + get: () => testFilePath, + }); + }); + + afterEach(async () => { + await service.onModuleDestroy(); + await rm(testDir, { recursive: true, force: true }); + }); + + it('should write initial PRE_INIT status, then update on event', async () => { + // First, mock the config to return undefined (no connection metadata) + vi.mocked(configService.get).mockReturnValue(undefined); + + console.log('=== Starting onApplicationBootstrap ==='); + await service.onApplicationBootstrap(); + + // Wait a bit for the initial write to complete + await new Promise(resolve => setTimeout(resolve, 50)); + + // Read initial status + const initialContent = await readFile(testFilePath, 'utf-8'); + const initialData = JSON.parse(initialContent); + console.log('Initial status written:', initialData); + + expect(initialData.connectionStatus).toBe('PRE_INIT'); + expect(initialData.error).toBeNull(); + expect(initialData.lastPing).toBeNull(); + + // Now update the mock to return CONNECTED status + vi.mocked(configService.get).mockReturnValue({ + status: 'CONNECTED', + error: null, + lastPing: 1234567890, + }); + + console.log('=== Calling writeStatus directly ==='); + await service['writeStatus'](); + + // Read updated status + const updatedContent = await readFile(testFilePath, 'utf-8'); + const updatedData = JSON.parse(updatedContent); + console.log('Updated status after writeStatus:', updatedData); + + expect(updatedData.connectionStatus).toBe('CONNECTED'); + expect(updatedData.lastPing).toBe(1234567890); + }); + + it('should handle rapid status changes correctly', async () => { + const statusChanges = [ + { status: 'PRE_INIT', error: null, lastPing: null }, + { status: 'CONNECTING', error: null, lastPing: null }, + { status: 'CONNECTED', error: null, lastPing: Date.now() }, + { status: 'DISCONNECTED', error: 'Connection lost', lastPing: Date.now() - 5000 }, + { status: 'CONNECTED', error: null, lastPing: Date.now() }, + ]; + + let changeIndex = 0; + vi.mocked(configService.get).mockImplementation(() => { + const change = statusChanges[changeIndex]; + console.log(`Returning status ${changeIndex}: ${change.status}`); + return change; + }); + + await service.onApplicationBootstrap(); + await new Promise(resolve => setTimeout(resolve, 50)); + + // Simulate the final status change + changeIndex = statusChanges.length - 1; + console.log(`=== Calling writeStatus for final status: ${statusChanges[changeIndex].status} ===`); + await service['writeStatus'](); + + // Read final status + const finalContent = await readFile(testFilePath, 'utf-8'); + const finalData = JSON.parse(finalContent); + console.log('Final status after status change:', finalData); + + // Should have the last status + expect(finalData.connectionStatus).toBe('CONNECTED'); + expect(finalData.error).toBeNull(); + }); + + it('should handle multiple write calls correctly', async () => { + const writes: number[] = []; + const originalWriteStatus = service['writeStatus'].bind(service); + + service['writeStatus'] = async function() { + const timestamp = Date.now(); + writes.push(timestamp); + console.log(`writeStatus called at ${timestamp}`); + return originalWriteStatus(); + }; + + await service.onApplicationBootstrap(); + await new Promise(resolve => setTimeout(resolve, 50)); + + const initialWrites = writes.length; + console.log(`Initial writes: ${initialWrites}`); + + // Make multiple write calls + for (let i = 0; i < 3; i++) { + console.log(`Calling writeStatus ${i}`); + await service['writeStatus'](); + } + + console.log(`Total writes: ${writes.length}`); + console.log('Write timestamps:', writes); + + // Should have initial write + 3 additional writes + expect(writes.length).toBe(initialWrites + 3); + }); + + describe('cleanup on shutdown', () => { + it('should delete status file on module destroy', async () => { + await service.onApplicationBootstrap(); + await new Promise(resolve => setTimeout(resolve, 50)); + + // Verify file exists + await expect(access(testFilePath, constants.F_OK)).resolves.not.toThrow(); + + // Cleanup + await service.onModuleDestroy(); + + // Verify file is deleted + await expect(access(testFilePath, constants.F_OK)).rejects.toThrow(); + }); + + it('should handle cleanup gracefully when file does not exist', async () => { + // Don't bootstrap (so no file is created) + await expect(service.onModuleDestroy()).resolves.not.toThrow(); + }); + }); +}); \ No newline at end of file diff --git a/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.ts b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.ts new file mode 100644 index 000000000..920b6394c --- /dev/null +++ b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.ts @@ -0,0 +1,140 @@ +import { ConfigService } from '@nestjs/config'; +import { unlink, writeFile } from 'fs/promises'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { ConfigType } from '../config/connect.config.js'; +import { ConnectStatusWriterService } from './connect-status-writer.service.js'; + +vi.mock('fs/promises', () => ({ + writeFile: vi.fn(), + unlink: vi.fn(), +})); + +describe('ConnectStatusWriterService', () => { + let service: ConnectStatusWriterService; + let configService: ConfigService; + let writeFileMock: ReturnType; + let unlinkMock: ReturnType; + + beforeEach(async () => { + vi.clearAllMocks(); + vi.useFakeTimers(); + + writeFileMock = vi.mocked(writeFile); + unlinkMock = vi.mocked(unlink); + + configService = { + get: vi.fn().mockReturnValue({ + status: 'CONNECTED', + error: null, + lastPing: Date.now(), + }), + } as unknown as ConfigService; + + service = new ConnectStatusWriterService(configService); + }); + + afterEach(async () => { + vi.useRealTimers(); + }); + + describe('onApplicationBootstrap', () => { + it('should write initial status on bootstrap', async () => { + await service.onApplicationBootstrap(); + + expect(writeFileMock).toHaveBeenCalledTimes(1); + expect(writeFileMock).toHaveBeenCalledWith( + '/var/local/emhttp/connectStatus.json', + expect.stringContaining('CONNECTED') + ); + }); + + it('should handle event-driven status changes', async () => { + await service.onApplicationBootstrap(); + writeFileMock.mockClear(); + + // The service uses @OnEvent decorator, so we need to call the method directly + await service['writeStatus'](); + + expect(writeFileMock).toHaveBeenCalledTimes(1); + }); + }); + + describe('write content', () => { + it('should write correct JSON structure with all fields', async () => { + const mockMetadata = { + status: 'CONNECTED', + error: 'Some error', + lastPing: 1234567890, + }; + + vi.mocked(configService.get).mockReturnValue(mockMetadata); + + await service.onApplicationBootstrap(); + + const writeCall = writeFileMock.mock.calls[0]; + const writtenData = JSON.parse(writeCall[1] as string); + + expect(writtenData).toMatchObject({ + connectionStatus: 'CONNECTED', + error: 'Some error', + lastPing: 1234567890, + allowedOrigins: '', + }); + expect(writtenData.timestamp).toBeDefined(); + expect(typeof writtenData.timestamp).toBe('number'); + }); + + it('should handle missing connection metadata', async () => { + vi.mocked(configService.get).mockReturnValue(undefined); + + await service.onApplicationBootstrap(); + + const writeCall = writeFileMock.mock.calls[0]; + const writtenData = JSON.parse(writeCall[1] as string); + + expect(writtenData).toMatchObject({ + connectionStatus: 'PRE_INIT', + error: null, + lastPing: null, + allowedOrigins: '', + }); + }); + }); + + describe('error handling', () => { + it('should handle write errors gracefully', async () => { + writeFileMock.mockRejectedValue(new Error('Write failed')); + + await expect(service.onApplicationBootstrap()).resolves.not.toThrow(); + + // Test direct write error handling + await expect(service['writeStatus']()).resolves.not.toThrow(); + }); + }); + + describe('cleanup on shutdown', () => { + it('should delete status file on module destroy', async () => { + await service.onModuleDestroy(); + + expect(unlinkMock).toHaveBeenCalledTimes(1); + expect(unlinkMock).toHaveBeenCalledWith('/var/local/emhttp/connectStatus.json'); + }); + + it('should handle file deletion errors gracefully', async () => { + unlinkMock.mockRejectedValue(new Error('File not found')); + + await expect(service.onModuleDestroy()).resolves.not.toThrow(); + + expect(unlinkMock).toHaveBeenCalledTimes(1); + }); + + it('should ensure file is deleted even if it was never written', async () => { + // Don't bootstrap (so no file is written) + await service.onModuleDestroy(); + + expect(unlinkMock).toHaveBeenCalledTimes(1); + expect(unlinkMock).toHaveBeenCalledWith('/var/local/emhttp/connectStatus.json'); + }); + }); +}); \ No newline at end of file diff --git a/packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts similarity index 68% rename from packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts rename to packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts index cd5e48f38..3b9547a5c 100644 --- a/packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts +++ b/packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts @@ -1,11 +1,14 @@ -import { Injectable, Logger, OnModuleInit } from '@nestjs/common'; +import { Injectable, Logger, OnApplicationBootstrap, OnModuleDestroy } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; +import { OnEvent } from '@nestjs/event-emitter'; +import { unlink } from 'fs/promises'; import { writeFile } from 'fs/promises'; -import { ConnectionMetadata, ConfigType } from './connect.config.js'; +import { ConfigType, ConnectionMetadata } from '../config/connect.config.js'; +import { EVENTS } from '../helper/nest-tokens.js'; @Injectable() -export class ConnectStatusWriterService implements OnModuleInit { +export class ConnectStatusWriterService implements OnApplicationBootstrap, OnModuleDestroy { constructor(private readonly configService: ConfigService) {} private logger = new Logger(ConnectStatusWriterService.name); @@ -15,30 +18,27 @@ export class ConnectStatusWriterService implements OnModuleInit { return '/var/local/emhttp/connectStatus.json'; } - async onModuleInit() { + async onApplicationBootstrap() { this.logger.verbose(`Status file path: ${this.statusFilePath}`); - + // Write initial status await this.writeStatus(); - - // Listen for changes to connection status - this.configService.changes$.subscribe({ - next: async (change) => { - const connectionChanged = change.path && change.path.startsWith('connect.mothership'); - if (connectionChanged) { - await this.writeStatus(); - } - }, - error: (err) => { - this.logger.error('Error receiving config changes:', err); - }, - }); } + async onModuleDestroy() { + try { + await unlink(this.statusFilePath); + this.logger.verbose(`Status file deleted: ${this.statusFilePath}`); + } catch (error) { + this.logger.debug(`Could not delete status file: ${error}`); + } + } + + @OnEvent(EVENTS.MOTHERSHIP_CONNECTION_STATUS_CHANGED, { async: true }) private async writeStatus() { try { const connectionMetadata = this.configService.get('connect.mothership'); - + // Try to get allowed origins from the store let allowedOrigins = ''; try { @@ -48,22 +48,22 @@ export class ConnectStatusWriterService implements OnModuleInit { } catch (error) { this.logger.debug('Could not get allowed origins:', error); } - + const statusData = { connectionStatus: connectionMetadata?.status || 'PRE_INIT', error: connectionMetadata?.error || null, lastPing: connectionMetadata?.lastPing || null, allowedOrigins: allowedOrigins, - timestamp: Date.now() + timestamp: Date.now(), }; const data = JSON.stringify(statusData, null, 2); this.logger.verbose(`Writing connection status: ${data}`); - + await writeFile(this.statusFilePath, data); this.logger.verbose(`Status written to ${this.statusFilePath}`); } catch (error) { this.logger.error(error, `Error writing status to '${this.statusFilePath}'`); } } -} \ No newline at end of file +} diff --git a/packages/unraid-api-plugin-connect/src/index.ts b/packages/unraid-api-plugin-connect/src/index.ts index 1181a7646..21748673f 100644 --- a/packages/unraid-api-plugin-connect/src/index.ts +++ b/packages/unraid-api-plugin-connect/src/index.ts @@ -3,7 +3,6 @@ import { ConfigModule, ConfigService } from '@nestjs/config'; import { ConnectConfigPersister } from './config/config.persistence.js'; import { configFeature } from './config/connect.config.js'; -import { ConnectStatusWriterService } from './config/connect-status-writer.service.js'; import { MothershipModule } from './mothership-proxy/mothership.module.js'; import { ConnectModule } from './unraid-connect/connect.module.js'; @@ -11,7 +10,7 @@ export const adapter = 'nestjs'; @Module({ imports: [ConfigModule.forFeature(configFeature), ConnectModule, MothershipModule], - providers: [ConnectConfigPersister, ConnectStatusWriterService], + providers: [ConnectConfigPersister], exports: [], }) class ConnectPluginModule { diff --git a/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts b/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts index 48bf6a538..75bde5acb 100644 --- a/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts +++ b/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts @@ -3,18 +3,20 @@ import { Module } from '@nestjs/common'; import { ConnectApiKeyService } from '../authn/connect-api-key.service.js'; import { CloudResolver } from '../connection-status/cloud.resolver.js'; import { CloudService } from '../connection-status/cloud.service.js'; +import { ConnectStatusWriterService } from '../connection-status/connect-status-writer.service.js'; import { TimeoutCheckerJob } from '../connection-status/timeout-checker.job.js'; import { InternalClientService } from '../internal-rpc/internal.client.js'; import { RemoteAccessModule } from '../remote-access/remote-access.module.js'; import { MothershipConnectionService } from './connection.service.js'; import { MothershipGraphqlClientService } from './graphql.client.js'; import { MothershipSubscriptionHandler } from './mothership-subscription.handler.js'; -import { MothershipHandler } from './mothership.events.js'; import { MothershipController } from './mothership.controller.js'; +import { MothershipHandler } from './mothership.events.js'; @Module({ imports: [RemoteAccessModule], providers: [ + ConnectStatusWriterService, ConnectApiKeyService, MothershipConnectionService, MothershipGraphqlClientService,