fix: event emitter setup for writing status (#1496)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

.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

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<ConfigType, true>;
+    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<ConfigType, true>;
+
+        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();
+        });
+    });
+});

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<ConfigType, true>;
+    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<ConfigType, true>;
+
+        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();
+        });
+    });
+});

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<ConfigType, true>;
+    let writeFileMock: ReturnType<typeof vi.fn>;
+    let unlinkMock: ReturnType<typeof vi.fn>;
+
+    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<ConfigType, true>;
+
+        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');
+        });
+    });
+});

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<ConfigType, true>) {}
 
     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<ConnectionMetadata>('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}'`);
         }
     }
-}
+}

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 {

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,