From ab1a79e5bf78c57f71df0892151ee2d80af0c2cc Mon Sep 17 00:00:00 2001 From: Ajit Mehrotra Date: Fri, 27 Mar 2026 17:43:37 -0400 Subject: [PATCH 1/5] fix(server-identity): mirror webgui identity updates - Purpose: align the API server identity mutation with the webgui Identification flow and remove the dead onboarding-only identity path. - Before: the API sent a partial emcmd payload, kept an unused onboarding stub around, and collapsed the real emcmd failure into a generic GraphQL error. - Problem: identity updates behaved differently from the webgui path, onboarding still carried a stale implementation, and debugging transport failures was much harder than it needed to be. - Now: ServerService sends the full Identification-style payload, onboarding no longer carries the unused applyServerIdentity path, and GraphQL preserves the underlying emcmd error message in extensions.cause. - How: added webgui-style server context fields to updateServerIdentity, expanded regression coverage around payload shape and persistence side effects, and removed the orphaned onboarding helper plus its stale specs. --- .../customization/onboarding.service.spec.ts | 354 +----------------- .../customization/onboarding.service.ts | 56 --- .../resolvers/servers/server.service.spec.ts | 171 +++++++-- .../graph/resolvers/servers/server.service.ts | 41 +- 4 files changed, 166 insertions(+), 456 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts b/api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts index a2e1a134ff..56f98d4b5d 100644 --- a/api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts +++ b/api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts @@ -802,7 +802,7 @@ describe('OnboardingService', () => { }); // --- Spy on subsequent steps to ensure they are still called --- - // We already mock fs.writeFile, so we can check calls to userDynamixCfg and identCfg + // We already mock fs.writeFile, so we can check calls to userDynamixCfg const applyDisplaySettingsSpy = vi.spyOn(service as any, 'applyDisplaySettings'); const updateCfgFileSpy = vi.spyOn(service as any, 'updateCfgFile'); @@ -978,7 +978,6 @@ describe('OnboardingService', () => { (service as any).activationDir = activationDir; (service as any).configFile = userDynamixCfg; (service as any).caseModelCfg = caseModelCfg; - (service as any).identCfg = identCfg; (service as any).activationData = plainToInstance(ActivationCode, { ...mockActivationData }); (service as any).activationJsonPath = activationJsonPath; (service as any).materializedPartnerMedia = { @@ -988,7 +987,6 @@ describe('OnboardingService', () => { // Mock necessary file reads/writes vi.mocked(fs.readFile).mockImplementation(async (p) => { if (p === userDynamixCfg) return ini.stringify({ display: { existing: 'value' } }); - if (p === identCfg) return ini.stringify({ NAME: 'OldName' }); if (p === caseModelCfg) return 'old-model.png'; // Simulate file not found for updateCfgFile tests where it matters // If activation JSON is read here, return mock data @@ -1361,353 +1359,6 @@ describe('OnboardingService', () => { expect(fs.writeFile).toHaveBeenCalledWith(caseModelCfg, 'mid-tower'); expect(loggerLogSpy).toHaveBeenCalledWith(`Case model set to mid-tower in ${caseModelCfg}`); }); - - it('applyServerIdentity should call emcmd directly', async () => { - const updateSpy = vi.spyOn(service as any, 'updateCfgFile'); - - // Capture the emcmd call parameters - let emcmdParams; - vi.mocked(emcmd).mockImplementation(async (params, options) => { - emcmdParams = { params, options }; - return { body: '', ok: true } as any; - }); - - const promise = (service as any).applyServerIdentity(); - await vi.runAllTimers(); - await promise; - - // We no longer update the config file before calling emcmd - expect(updateSpy).not.toHaveBeenCalled(); - - // Verify emcmd was called with expected parameters using inline snapshot - expect(emcmdParams).toMatchInlineSnapshot(` - { - "options": { - "waitForToken": true, - }, - "params": { - "COMMENT": "Partner Comment", - "NAME": "PartnerServer", - "SYS_MODEL": "PartnerModel", - "changeNames": "Apply", - "server_addr": "", - "server_name": "", - }, - } - `); - - expect(loggerLogSpy).toHaveBeenCalledWith('emcmd executed successfully.'); - }, 10000); - - it('applyServerIdentity should skip if no relevant activation data', async () => { - const updateSpy = vi.spyOn(service as any, 'updateCfgFile'); - // Simulate empty DTO - (service as any).activationData = plainToInstance(ActivationCode, {}); - await (service as any).applyServerIdentity(); - expect(updateSpy).not.toHaveBeenCalled(); - expect(emcmd).not.toHaveBeenCalled(); - expect(loggerLogSpy).toHaveBeenCalledWith( - 'No server identity information found in activation data.' - ); - }); - - it('applyServerIdentity should skip if activation data has no relevant fields', async () => { - const updateSpy = vi.spyOn(service as any, 'updateCfgFile'); - // Simulate DTO with non-identity fields - (service as any).activationData = plainToInstance(ActivationCode, { - branding: { theme: 'white' }, - }); - await (service as any).applyServerIdentity(); - expect(updateSpy).not.toHaveBeenCalled(); - expect(emcmd).not.toHaveBeenCalled(); - expect(loggerLogSpy).toHaveBeenCalledWith( - 'No server identity information found in activation data.' - ); - }); - - it('applyServerIdentity should log error on emcmd failure', async () => { - const emcmdError = new Error('Failed to call emcmd'); - - // Set up activation data directly - (service as any).activationData = plainToInstance(ActivationCode, { - system: { - serverName: 'PartnerServer', - model: 'PartnerModel', - comment: 'Partner Comment', - }, - }); - - // Mock emcmd to throw - vi.mocked(emcmd).mockRejectedValue(emcmdError); - - // Clear previous log calls - loggerErrorSpy.mockClear(); - - // Call the method directly - await (service as any).applyServerIdentity(); - - // Verify the error was logged - expect(emcmd).toHaveBeenCalled(); - expect(loggerErrorSpy).toHaveBeenCalledWith( - 'Error applying server identity: %o', - emcmdError - ); - }, 10000); - - it('applyServerIdentity should apply comment even when name/model are absent', async () => { - (service as any).activationData = plainToInstance(ActivationCode, { - system: { - comment: 'Partner Comment', - }, - }); - - let commentOnlyParams: Record | undefined; - vi.mocked(emcmd).mockImplementation(async (params) => { - commentOnlyParams = params as Record; - return { body: '', ok: true } as any; - }); - - await (service as any).applyServerIdentity(); - - expect(emcmd).toHaveBeenCalled(); - expect(commentOnlyParams).toMatchObject({ - COMMENT: 'Partner Comment', - changeNames: 'Apply', - server_addr: '', - server_name: '', - }); - expect(commentOnlyParams).not.toHaveProperty('NAME'); - expect(commentOnlyParams).not.toHaveProperty('SYS_MODEL'); - }); - - it('applyServerIdentity should omit comment when activation data does not provide one', async () => { - (service as any).activationData = plainToInstance(ActivationCode, { - system: { - serverName: 'PartnerServer', - model: 'PartnerModel', - }, - }); - - let paramsWithoutComment: Record | undefined; - vi.mocked(emcmd).mockImplementation(async (params) => { - paramsWithoutComment = params as Record; - return { body: '', ok: true } as any; - }); - - await (service as any).applyServerIdentity(); - - expect(emcmd).toHaveBeenCalled(); - expect(paramsWithoutComment).toMatchObject({ - NAME: 'PartnerServer', - SYS_MODEL: 'PartnerModel', - }); - expect(paramsWithoutComment).not.toHaveProperty('COMMENT'); - }); - - it('applyServerIdentity should allow explicitly empty comments from activation data', async () => { - (service as any).activationData = plainToInstance(ActivationCode, { - system: { - comment: '', - }, - }); - - let emptyCommentParams: Record | undefined; - vi.mocked(emcmd).mockImplementation(async (params) => { - emptyCommentParams = params as Record; - return { body: '', ok: true } as any; - }); - - await (service as any).applyServerIdentity(); - - expect(emcmd).toHaveBeenCalled(); - expect(emptyCommentParams).toMatchObject({ - COMMENT: '', - changeNames: 'Apply', - server_addr: '', - server_name: '', - }); - }); - - it.each([ - { - caseName: 'name only', - system: { serverName: 'PartnerServer' }, - expected: { NAME: 'PartnerServer' }, - omitted: ['SYS_MODEL', 'COMMENT'], - }, - { - caseName: 'model only', - system: { model: 'PartnerModel' }, - expected: { SYS_MODEL: 'PartnerModel' }, - omitted: ['NAME', 'COMMENT'], - }, - { - caseName: 'comment only', - system: { comment: 'Partner Comment' }, - expected: { COMMENT: 'Partner Comment' }, - omitted: ['NAME', 'SYS_MODEL'], - }, - { - caseName: 'explicit empty comment', - system: { comment: '' }, - expected: { COMMENT: '' }, - omitted: ['NAME', 'SYS_MODEL'], - }, - ])( - 'applyServerIdentity should map partial identity fields correctly ($caseName)', - async (scenario) => { - (service as any).activationData = plainToInstance(ActivationCode, { - system: scenario.system, - }); - - let params: Record | undefined; - vi.mocked(emcmd).mockImplementation(async (incomingParams) => { - params = incomingParams as Record; - return { body: '', ok: true } as any; - }); - - await (service as any).applyServerIdentity(); - - expect(emcmd).toHaveBeenCalledTimes(1); - expect(params).toMatchObject({ - ...scenario.expected, - changeNames: 'Apply', - server_addr: '', - server_name: '', - }); - scenario.omitted.forEach((key) => { - expect(params).not.toHaveProperty(key); - }); - } - ); - - it('applyServerIdentity should truncate serverName if too long', async () => { - const longServerName = 'ThisServerNameIsWayTooLongForUnraid'; // Length > 16 - const truncatedServerName = longServerName.slice(0, 15); // Expected truncated length - // Simulate DTO with long serverName after plainToClass - - const testActivationParser = await plainToInstance(ActivationCode, { - ...mockActivationData, - system: { ...mockActivationData.system, serverName: longServerName }, - }); - - expect(testActivationParser.system?.serverName).toBe(truncatedServerName); - }); - - it('applyServerIdentity should sanitize and truncate activation comments', async () => { - const unsafeLongComment = `${'"\\'.repeat(40)}${'A'.repeat(100)}`; - const parsedActivation = plainToInstance(ActivationCode, { - system: { - comment: unsafeLongComment, - }, - }); - - expect(parsedActivation.system?.comment).toBeDefined(); - expect(parsedActivation.system?.comment).not.toMatch(/["\\]/); - expect(parsedActivation.system?.comment!.length).toBeLessThanOrEqual(64); - }); - - it('applyServerIdentity should send sanitized identity values from transformed activation data', async () => { - const unsafeIdentity = plainToInstance(ActivationCode, { - system: { - serverName: 'Par"t\\nerServer', - model: 'Pa"rt\\nerModel', - comment: 'Partn"er\\Comment', - }, - }); - (service as any).activationData = unsafeIdentity; - - let params: Record | undefined; - vi.mocked(emcmd).mockImplementation(async (incomingParams) => { - params = incomingParams as Record; - return { body: '', ok: true } as any; - }); - - await (service as any).applyServerIdentity(); - - expect(emcmd).toHaveBeenCalledTimes(1); - expect(params).toMatchObject({ - NAME: 'PartnerServer', - SYS_MODEL: 'PartnerModel', - COMMENT: 'PartnerComment', - }); - expect(params?.NAME).not.toMatch(/["\\]/); - expect(params?.SYS_MODEL).not.toMatch(/["\\]/); - expect(params?.COMMENT).not.toMatch(/["\\]/); - }); - - it('should correctly pass server_https parameter based on nginx state', async () => { - // Mock getters.emhttp to include nginx with sslEnabled=true - const mockEmhttpWithSsl = { - nginx: { sslEnabled: true }, - var: { name: 'Tower', sysModel: 'Custom', comment: 'Default' }, - }; - vi.mocked(getters.emhttp).mockReturnValue(mockEmhttpWithSsl as any); - - // Set up the service's activationData field directly - (service as any).activationData = plainToInstance(ActivationCode, { - system: { - serverName: 'PartnerServer', - model: 'PartnerModel', - comment: 'Partner Comment', - }, - }); - - // Mock emcmd and capture the params for snapshot testing - let sslEnabledParams; - vi.mocked(emcmd).mockImplementation(async (params) => { - sslEnabledParams = params; - return { body: '', ok: true } as any; - }); - - // Call the method directly to test SSL enabled case - await (service as any).applyServerIdentity(); - - // Verify emcmd was called - expect(emcmd).toHaveBeenCalled(); - // Use toMatchInlineSnapshot to compare the params - expect(sslEnabledParams).toMatchInlineSnapshot(` - { - "COMMENT": "Partner Comment", - "NAME": "PartnerServer", - "SYS_MODEL": "PartnerModel", - "changeNames": "Apply", - "server_addr": "", - "server_name": "", - } - `); - - // Now test with SSL disabled - const mockEmhttpNoSsl = { - nginx: { sslEnabled: false }, - var: { name: 'Tower', sysModel: 'Custom', comment: 'Default' }, - }; - vi.mocked(getters.emhttp).mockReturnValue(mockEmhttpNoSsl as any); - - // Update the mock to capture params for the second call - let sslDisabledParams; - vi.mocked(emcmd).mockImplementation(async (params) => { - sslDisabledParams = params; - return { body: '', ok: true } as any; - }); - - // Call again to test SSL disabled case - await (service as any).applyServerIdentity(); - - // Verify emcmd was called again - expect(emcmd).toHaveBeenCalled(); - // Use toMatchInlineSnapshot to compare the params - expect(sslDisabledParams).toMatchInlineSnapshot(` - { - "COMMENT": "Partner Comment", - "NAME": "PartnerServer", - "SYS_MODEL": "PartnerModel", - "changeNames": "Apply", - "server_addr": "", - "server_name": "", - } - `); - }, 10000); }); }); @@ -1722,7 +1373,6 @@ describe('applyActivationCustomizations specific tests', () => { const activationDir = mockPaths.activationBase; const userDynamixCfg = mockPaths['dynamix-config'][1]; const caseModelCfg = mockPaths.boot.caseModelConfig; - const identCfg = mockPaths.identConfig; const bannerSource = mockPaths.activation.banner; const bannerTarget = mockPaths.webgui.banner; const caseModelSource = mockPaths.activation.caseModel; @@ -1787,7 +1437,6 @@ describe('applyActivationCustomizations specific tests', () => { (service as any).activationDir = activationDir; (service as any).configFile = userDynamixCfg; (service as any).caseModelCfg = caseModelCfg; - (service as any).identCfg = identCfg; (service as any).activationData = plainToInstance(ActivationCode, { ...mockActivationData }); // Default mocks for dependencies, override in specific tests if needed @@ -1797,7 +1446,6 @@ describe('applyActivationCustomizations specific tests', () => { vi.mocked(fs.access).mockResolvedValue(undefined); // Assume dirs/files accessible by default vi.mocked(fs.readFile).mockImplementation(async (p) => { if (p === userDynamixCfg) return ini.stringify({}); - if (p === identCfg) return ini.stringify({}); if (p === caseModelCfg) return ''; // Assume empty or non-existent return ''; }); diff --git a/api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts b/api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts index 1b22ed2c11..1d08bea764 100644 --- a/api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts +++ b/api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts @@ -9,7 +9,6 @@ import * as ini from 'ini'; import coerce from 'semver/functions/coerce.js'; import gte from 'semver/functions/gte.js'; -import { emcmd } from '@app/core/utils/clients/emcmd.js'; import { fileExists } from '@app/core/utils/files/file-exists.js'; import { safelySerializeObjectToIni } from '@app/core/utils/files/safe-ini-serializer.js'; import { loadDynamixConfigFromDiskSync } from '@app/store/actions/load-dynamix-config-file.js'; @@ -43,7 +42,6 @@ export class OnboardingService implements OnModuleInit { private activationDir!: string; private configFile!: string; private caseModelCfg!: string; - private identCfg!: string; private activationJsonPath: string | null = null; private materializedPartnerMedia: Record<'banner' | 'caseModel', boolean> = { banner: false, @@ -76,7 +74,6 @@ export class OnboardingService implements OnModuleInit { this.activationDir = paths.activationBase; this.configFile = paths['dynamix-config']?.[1]; this.caseModelCfg = paths.boot?.caseModelConfig; - this.identCfg = paths.identConfig; this.logger.log('OnboardingService initialized with paths from store.'); @@ -917,59 +914,6 @@ export class OnboardingService implements OnModuleInit { } } - private async applyServerIdentity() { - if (!this.activationData) { - this.logger.warn('No activation data available for server identity setup.'); - return; - } - - this.logger.log('Applying server identity...'); - // Ideally, get current values from Redux store instead of var.ini - // Assuming EmhttpState type provides structure for emhttp slice. Adjust if necessary. - // Using optional chaining ?. in case emhttp or var is not defined in the state yet. - const currentEmhttpState = getters.emhttp(); - const currentName = currentEmhttpState?.var?.name || ''; - // Skip sending sysModel to emcmd for now - const currentSysModel = ''; - const currentComment = currentEmhttpState?.var?.comment || ''; - - this.logger.debug( - `Current identity - Name: ${currentName}, Model: ${currentSysModel}, Comment: ${currentComment}` - ); - - const { serverName, model: sysModel, comment } = this.activationData.system || {}; - const paramsToUpdate: Record = { - ...(serverName && { NAME: serverName }), - ...(sysModel && { SYS_MODEL: sysModel }), - ...(comment !== undefined && { COMMENT: comment }), - }; - - if (Object.keys(paramsToUpdate).length === 0) { - this.logger.log('No server identity information found in activation data.'); - return; - } - - this.logger.log('Updating server identity:', paramsToUpdate); - - try { - // Trigger emhttp update via emcmd - const updateParams = { - ...paramsToUpdate, - changeNames: 'Apply', - // Can be null string - server_name: '', - // Can be null string - server_addr: '', - }; - this.logger.log(`Calling emcmd with params: %o`, updateParams); - await emcmd(updateParams, { waitForToken: true }); - - this.logger.log('emcmd executed successfully.'); - } catch (error: unknown) { - this.logger.error('Error applying server identity: %o', error); - } - } - // Helper function to update .cfg files (like dynamix.cfg or ident.cfg) using the ini library private async updateCfgFile( filePath: string, diff --git a/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts b/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts index b24bcd2455..393eecac49 100644 --- a/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts +++ b/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts @@ -1,5 +1,9 @@ +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + import { GraphQLError } from 'graphql'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { emcmd } from '@app/core/utils/clients/emcmd.js'; import { getters } from '@app/store/index.js'; @@ -17,10 +21,14 @@ vi.mock('@app/store/index.js', () => ({ describe('ServerService', () => { let service: ServerService; + let tempDirectory: string; + let identConfigPath: string; - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks(); service = new ServerService(); + tempDirectory = await mkdtemp(join(tmpdir(), 'server-service-')); + identConfigPath = join(tempDirectory, 'boot/config/ident.cfg'); vi.mocked(getters.emhttp).mockReturnValue({ var: { @@ -29,10 +37,27 @@ describe('ServerService', () => { regGuid: 'GUID-123', port: '80', comment: 'Tower comment', + sysModel: 'Model X100', }, networks: [{ ipaddr: ['192.168.1.10'] }], - } as unknown as ReturnType); + nginx: { + sslEnabled: true, + lanName: 'tower.local', + lanIp: '192.168.1.10', + }, + } as ReturnType); vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited>); + + await mkdir(join(tempDirectory, 'boot/config'), { recursive: true }); + await writeFile( + identConfigPath, + 'NAME="Tower"\nCOMMENT="Tower comment"\nSYS_MODEL="Model X100"\nEXTRA="keep-me"\n', + 'utf8' + ); + }); + + afterEach(async () => { + await rm(tempDirectory, { recursive: true, force: true }); }); it('throws for invalid server name characters', async () => { @@ -87,8 +112,18 @@ describe('ServerService', () => { name: 'Tower', mdState: 'STARTED', fsState: 'Started', + regGuid: 'GUID-123', + port: '80', + comment: 'Tower comment', + sysModel: 'Model X100', + }, + networks: [{ ipaddr: ['192.168.1.10'] }], + nginx: { + sslEnabled: true, + lanName: 'tower.local', + lanIp: '192.168.1.10', }, - } as any); + } as ReturnType); await expect(service.updateServerIdentity('NewTower', 'desc')).rejects.toThrow( 'The array must be stopped to change the server name.' @@ -100,7 +135,7 @@ describe('ServerService', () => { }); }); - it('allows name change when mdState is STOPPED even if fsState is not Stopped (internal boot)', async () => { + it('allows name change when mdState is STOPPED even if fsState is not Stopped', async () => { vi.mocked(getters.emhttp).mockReturnValue({ var: { name: 'Tower', @@ -109,27 +144,50 @@ describe('ServerService', () => { regGuid: 'GUID-123', port: '80', comment: '', + sysModel: '', }, networks: [{ ipaddr: ['192.168.1.10'] }], - } as unknown as ReturnType); + nginx: { + sslEnabled: true, + lanName: 'tower.local', + lanIp: '192.168.1.10', + }, + } as ReturnType); await expect(service.updateServerIdentity('NewTower', 'desc')).resolves.toMatchObject({ name: 'NewTower', }); }); - it('calls emcmd with expected params and returns optimistic server', async () => { - const result = await service.updateServerIdentity('Tower', 'Primary host'); + it('sends the Identification.page payload and persists ident.cfg', async () => { + vi.mocked(emcmd).mockImplementation(async (params) => { + await writeFile( + identConfigPath, + `NAME="${params.NAME}"\nCOMMENT="${params.COMMENT}"\nSYS_MODEL="${params.SYS_MODEL}"\nEXTRA="keep-me"\n`, + 'utf8' + ); + return { ok: true } as Awaited>; + }); + + const result = await service.updateServerIdentity('Test1e', 'Test server1e', 'Model X200'); expect(emcmd).toHaveBeenCalledWith( { changeNames: 'Apply', - NAME: 'Tower', - COMMENT: 'Primary host', + server_https: 'on', + server_name: 'tower.local', + server_addr: '192.168.1.10', + NAME: 'Test1e', + COMMENT: 'Test server1e', + SYS_MODEL: 'Model X200', }, { waitForToken: true } ); + await expect(readFile(identConfigPath, 'utf8')).resolves.toBe( + 'NAME="Test1e"\nCOMMENT="Test server1e"\nSYS_MODEL="Model X200"\nEXTRA="keep-me"\n' + ); + expect(result).toEqual({ id: 'local', owner: { @@ -140,8 +198,8 @@ describe('ServerService', () => { }, guid: 'GUID-123', apikey: '', - name: 'Tower', - comment: 'Primary host', + name: 'Test1e', + comment: 'Test server1e', status: 'ONLINE', wanip: '', lanip: '192.168.1.10', @@ -150,10 +208,42 @@ describe('ServerService', () => { }); }); + it('preserves current comment and model when omitted, like the webgui form does', async () => { + vi.mocked(emcmd).mockImplementation(async (params) => { + await writeFile( + identConfigPath, + `NAME="${params.NAME}"\nCOMMENT="${params.COMMENT}"\nSYS_MODEL="${params.SYS_MODEL}"\nEXTRA="keep-me"\n`, + 'utf8' + ); + return { ok: true } as Awaited>; + }); + + await service.updateServerIdentity('TowerRenamed'); + + expect(emcmd).toHaveBeenCalledWith( + { + changeNames: 'Apply', + server_https: 'on', + server_name: 'tower.local', + server_addr: '192.168.1.10', + NAME: 'TowerRenamed', + COMMENT: 'Tower comment', + SYS_MODEL: 'Model X100', + }, + { waitForToken: true } + ); + + await expect(readFile(identConfigPath, 'utf8')).resolves.toBe( + 'NAME="TowerRenamed"\nCOMMENT="Tower comment"\nSYS_MODEL="Model X100"\nEXTRA="keep-me"\n' + ); + }); + it('skips emcmd when identity values are unchanged', async () => { - const result = await service.updateServerIdentity('Tower', 'Tower comment'); + const before = await readFile(identConfigPath, 'utf8'); + const result = await service.updateServerIdentity('Tower', 'Tower comment', 'Model X100'); expect(emcmd).not.toHaveBeenCalled(); + await expect(readFile(identConfigPath, 'utf8')).resolves.toBe(before); expect(result).toMatchObject({ name: 'Tower', comment: 'Tower comment', @@ -161,7 +251,7 @@ describe('ServerService', () => { }); }); - it('skips emcmd when sysModel is unchanged', async () => { + it('writes server_https as empty when ssl is disabled', async () => { vi.mocked(getters.emhttp).mockReturnValue({ var: { name: 'Tower', @@ -169,35 +259,50 @@ describe('ServerService', () => { regGuid: 'GUID-123', port: '80', comment: 'Tower comment', - sysModel: 'Model X200', + sysModel: 'Model X100', }, networks: [{ ipaddr: ['192.168.1.10'] }], - } as unknown as ReturnType); - - await service.updateServerIdentity('Tower', 'Tower comment', 'Model X200'); - - expect(emcmd).not.toHaveBeenCalled(); - }); + nginx: { + sslEnabled: false, + lanName: 'tower.local', + lanIp: '192.168.1.10', + }, + } as ReturnType); - it('includes SYS_MODEL when provided', async () => { - await service.updateServerIdentity('Tower', 'Primary host', 'Model X200'); + await service.updateServerIdentity('Tower', 'Primary host', 'Model X100'); expect(emcmd).toHaveBeenCalledWith( - { - changeNames: 'Apply', - NAME: 'Tower', - COMMENT: 'Primary host', - SYS_MODEL: 'Model X200', - }, + expect.objectContaining({ + server_https: '', + }), { waitForToken: true } ); }); - it('throws generic failure when emcmd fails', async () => { + it('throws generic failure when emcmd fails and leaves ident.cfg untouched', async () => { vi.mocked(emcmd).mockRejectedValue(new Error('socket failure')); + const before = await readFile(identConfigPath, 'utf8'); - await expect(service.updateServerIdentity('Tower', 'Primary host')).rejects.toThrow( - 'Failed to update server identity' - ); + await expect(service.updateServerIdentity('Tower', 'Primary host')).rejects.toMatchObject({ + message: 'Failed to update server identity', + extensions: { + cause: 'socket failure', + }, + }); + + await expect(readFile(identConfigPath, 'utf8')).resolves.toBe(before); + }); + + it('throws generic failure when emcmd fails to persist identity', async () => { + vi.mocked(emcmd).mockImplementation(async () => { + throw new Error('ident persistence failed'); + }); + + await expect(service.updateServerIdentity('Tower', 'Primary host')).rejects.toMatchObject({ + message: 'Failed to update server identity', + extensions: { + cause: 'ident persistence failed', + }, + }); }); }); diff --git a/api/src/unraid-api/graph/resolvers/servers/server.service.ts b/api/src/unraid-api/graph/resolvers/servers/server.service.ts index 561d865dca..deff6b0c22 100644 --- a/api/src/unraid-api/graph/resolvers/servers/server.service.ts +++ b/api/src/unraid-api/graph/resolvers/servers/server.service.ts @@ -15,6 +15,23 @@ import { export class ServerService { private readonly logger = new Logger(ServerService.name); + private buildIdentityUpdateParams( + emhttpState: ReturnType, + name: string, + comment: string, + sysModel: string + ): Record { + return { + changeNames: 'Apply', + server_https: emhttpState.nginx?.sslEnabled ? 'on' : '', + server_name: emhttpState.nginx?.lanName || 'localhost', + server_addr: emhttpState.nginx?.lanIp || '127.0.0.1', + NAME: name, + COMMENT: comment, + SYS_MODEL: sysModel, + }; + } + private buildServerResponse( emhttpState: ReturnType, name: string, @@ -109,27 +126,23 @@ export class ServerService { } } - const params: Record = { - changeNames: 'Apply', - NAME: name, - }; - - if (comment !== undefined) { - params.COMMENT = comment; - } - if (sysModel !== undefined) { - params.SYS_MODEL = sysModel; - } + const params = this.buildIdentityUpdateParams(currentEmhttp, name, nextComment, nextSysModel); try { await emcmd(params, { waitForToken: true }); this.logger.log('Server identity updated successfully via emcmd.'); const latestEmhttp = getters.emhttp(); - const responseComment = comment ?? latestEmhttp.var?.comment ?? currentComment; - return this.buildServerResponse(latestEmhttp, name, responseComment); + return this.buildServerResponse(latestEmhttp, name, nextComment); } catch (error) { this.logger.error('Failed to update server identity', error); - throw new GraphQLError('Failed to update server identity'); + throw new GraphQLError('Failed to update server identity', { + extensions: { + cause: + error instanceof Error && error.message + ? error.message + : 'Unknown server identity update failure', + }, + }); } } } From d34772e0308a7e8a669a0569818305f3c433278c Mon Sep 17 00:00:00 2001 From: Ajit Mehrotra Date: Fri, 27 Mar 2026 17:46:53 -0400 Subject: [PATCH 2/5] fix(emcmd): use curl for emhttp socket updates - Purpose: make emcmd use the same curl-over-unix-socket style transport that the webgui uses for internal /update calls. - Before: emcmd used got against the emhttp unix socket, which could apply a command successfully and still fail afterward with low-level parse errors like 'Expected HTTP/, RTSP/ or ICE/'. - Problem: callers such as updateServerIdentity could report a GraphQL error even after emhttp had already applied the setting change. - Now: emcmd shells out to curl over the unix socket, preserves the existing stdout-is-error contract, and returns a response object with body/stderr metadata for callers. - How: replaced the got socket POST with execa('curl', ... --unix-socket ... http://localhost/update), added explicit curl exit-code handling, and added direct unit coverage for successful calls, emhttp body errors, socket transport failures, and CSRF token fallback. --- api/src/core/utils/clients/emcmd.spec.ts | 135 +++++++++++++++++++++++ api/src/core/utils/clients/emcmd.ts | 44 +++++--- 2 files changed, 166 insertions(+), 13 deletions(-) create mode 100644 api/src/core/utils/clients/emcmd.spec.ts diff --git a/api/src/core/utils/clients/emcmd.spec.ts b/api/src/core/utils/clients/emcmd.spec.ts new file mode 100644 index 0000000000..bac10af2ec --- /dev/null +++ b/api/src/core/utils/clients/emcmd.spec.ts @@ -0,0 +1,135 @@ +import { readFile } from 'node:fs/promises'; + +import { execa } from 'execa'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { emcmd } from '@app/core/utils/clients/emcmd.js'; +import { getters, store } from '@app/store/index.js'; + +vi.mock('node:fs/promises', async () => { + const actual = await vi.importActual('node:fs/promises'); + return { + ...actual, + readFile: vi.fn(), + }; +}); + +vi.mock('execa', () => ({ + execa: vi.fn(), +})); + +vi.mock('@app/store/index.js', () => ({ + getters: { + emhttp: vi.fn(), + paths: vi.fn(), + }, + store: { + dispatch: vi.fn(), + }, +})); + +describe('emcmd', () => { + beforeEach(() => { + vi.clearAllMocks(); + + vi.mocked(getters.paths).mockReturnValue({ + 'emhttpd-socket': '/var/run/emhttpd.socket', + } as ReturnType); + + vi.mocked(getters.emhttp).mockReturnValue({ + var: { + csrfToken: 'state-token', + }, + } as ReturnType); + + vi.mocked(execa).mockResolvedValue({ + stdout: '', + stderr: '', + exitCode: 0, + command: 'curl', + escapedCommand: 'curl', + failed: false, + timedOut: false, + isCanceled: false, + killed: false, + } as Awaited>); + }); + + it('uses curl over the emhttp unix socket', async () => { + const result = await emcmd({ + changeNames: 'Apply', + NAME: 'Hello', + }); + + expect(execa).toHaveBeenCalledWith( + 'curl', + [ + '--silent', + '--show-error', + '--unix-socket', + '/var/run/emhttpd.socket', + '--data', + 'changeNames=Apply&NAME=Hello&csrf_token=state-token', + 'http://localhost/update', + ], + { reject: false } + ); + + expect(result).toMatchObject({ + body: '', + stderr: '', + exitCode: 0, + }); + }); + + it('throws emhttp output when curl returns a non-empty body', async () => { + vi.mocked(execa).mockResolvedValue({ + stdout: '', + stderr: '', + exitCode: 0, + command: 'curl', + escapedCommand: 'curl', + failed: false, + timedOut: false, + isCanceled: false, + killed: false, + } as Awaited>); + + await expect(emcmd({ changeNames: 'Apply' })).rejects.toThrow( + '' + ); + }); + + it('throws curl stderr when the socket transport fails', async () => { + vi.mocked(execa).mockResolvedValue({ + stdout: '', + stderr: 'curl: (56) Recv failure', + exitCode: 56, + command: 'curl', + escapedCommand: 'curl', + failed: true, + timedOut: false, + isCanceled: false, + killed: false, + } as Awaited>); + + await expect(emcmd({ changeNames: 'Apply' })).rejects.toThrow('curl: (56) Recv failure'); + }); + + it('falls back to var.ini for the csrf token before retrying state loads', async () => { + vi.mocked(getters.emhttp).mockReturnValue({ + var: {}, + } as ReturnType); + vi.mocked(readFile).mockResolvedValue('csrf_token="ini-token"\n'); + + await emcmd({ changeNames: 'Apply' }); + + expect(readFile).toHaveBeenCalledWith('/var/local/emhttp/var.ini', 'utf-8'); + expect(store.dispatch).not.toHaveBeenCalled(); + expect(execa).toHaveBeenCalledWith( + 'curl', + expect.arrayContaining(['--data', 'changeNames=Apply&csrf_token=ini-token']), + { reject: false } + ); + }); +}); diff --git a/api/src/core/utils/clients/emcmd.ts b/api/src/core/utils/clients/emcmd.ts index 3e8ac14d8b..f368b19c3b 100644 --- a/api/src/core/utils/clients/emcmd.ts +++ b/api/src/core/utils/clients/emcmd.ts @@ -1,6 +1,6 @@ import { readFile } from 'node:fs/promises'; -import { got } from 'got'; +import { execa } from 'execa'; import * as ini from 'ini'; import retry from 'p-retry'; @@ -112,26 +112,44 @@ export const emcmd = async ( }); params.append('csrf_token', csrfToken ?? ''); - const response = await got.post(`http://unix:${socketPath}:/update`, { - enableUnixSockets: true, - body: params.toString(), - headers: { - 'content-type': 'application/x-www-form-urlencoded', - }, - throwHttpErrors: false, - }); + const response = await execa( + 'curl', + [ + '--silent', + '--show-error', + '--unix-socket', + socketPath, + '--data', + params.toString(), + 'http://localhost/update', + ], + { + reject: false, + } + ); - if (response.statusCode >= 400) { - throw new Error(`emcmd request failed with status ${response.statusCode}`); + if (response.exitCode !== 0) { + const curlError = response.stderr?.trim() || response.stdout?.trim(); + throw new Error(curlError || `emcmd curl failed with exit code ${response.exitCode}`); } - const trimmedBody = response.body?.trim(); + const trimmedBody = response.stdout?.trim(); if (trimmedBody) { throw new Error(trimmedBody); } appLogger.debug('emcmd executed successfully'); - return response; + return { + body: response.stdout, + stderr: response.stderr, + exitCode: response.exitCode, + command: response.command, + escapedCommand: response.escapedCommand, + failed: response.failed, + timedOut: response.timedOut, + isCanceled: response.isCanceled, + killed: response.killed, + }; } catch (error: unknown) { if (hasErrorCode(error) && error.code === 'ENOENT') { if (error instanceof Error) { From 8ea5cb4f1ef4e4a388c8ca7ab75179a1f90281d1 Mon Sep 17 00:00:00 2001 From: Ajit Mehrotra Date: Fri, 27 Mar 2026 18:19:53 -0400 Subject: [PATCH 3/5] fix(emcmd): restore got unix socket transport - Purpose: revert the curl-based emhttp transport experiment and return emcmd to the original got unix-socket client.\n- Before: emcmd posted to /update through curl and needed transport-specific flags like --http0.9 to get past emhttpd response parsing.\n- Problem: the curl path changed the transport surface area without actually solving the real bug, which is that emhttp can succeed while returning a non-empty response body that our success detection misclassifies.\n- Now: emcmd uses got.post(..., { enableUnixSockets: true }) again, matching the original helper shape while preserving the focused tests around csrf loading and response handling.\n- How: swap execa/curl back to got, restore the response object contract, and update the emcmd spec to assert the got unix-socket request path. --- api/src/core/utils/clients/emcmd.spec.ts | 96 +++++++++--------------- api/src/core/utils/clients/emcmd.ts | 44 ++++------- 2 files changed, 49 insertions(+), 91 deletions(-) diff --git a/api/src/core/utils/clients/emcmd.spec.ts b/api/src/core/utils/clients/emcmd.spec.ts index bac10af2ec..d1dfac5ca9 100644 --- a/api/src/core/utils/clients/emcmd.spec.ts +++ b/api/src/core/utils/clients/emcmd.spec.ts @@ -1,6 +1,6 @@ import { readFile } from 'node:fs/promises'; -import { execa } from 'execa'; +import { got } from 'got'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { emcmd } from '@app/core/utils/clients/emcmd.js'; @@ -14,8 +14,10 @@ vi.mock('node:fs/promises', async () => { }; }); -vi.mock('execa', () => ({ - execa: vi.fn(), +vi.mock('got', () => ({ + got: { + post: vi.fn(), + }, })); vi.mock('@app/store/index.js', () => ({ @@ -42,78 +44,51 @@ describe('emcmd', () => { }, } as ReturnType); - vi.mocked(execa).mockResolvedValue({ - stdout: '', - stderr: '', - exitCode: 0, - command: 'curl', - escapedCommand: 'curl', - failed: false, - timedOut: false, - isCanceled: false, - killed: false, - } as Awaited>); + vi.mocked(got.post).mockResolvedValue({ + body: '', + statusCode: 200, + } as Awaited>); }); - it('uses curl over the emhttp unix socket', async () => { + it('uses got over the emhttp unix socket', async () => { const result = await emcmd({ changeNames: 'Apply', NAME: 'Hello', }); - expect(execa).toHaveBeenCalledWith( - 'curl', - [ - '--silent', - '--show-error', - '--unix-socket', - '/var/run/emhttpd.socket', - '--data', - 'changeNames=Apply&NAME=Hello&csrf_token=state-token', - 'http://localhost/update', - ], - { reject: false } - ); + expect(got.post).toHaveBeenCalledWith('http://unix:/var/run/emhttpd.socket:/update', { + enableUnixSockets: true, + body: 'changeNames=Apply&NAME=Hello&csrf_token=state-token', + headers: { + 'content-type': 'application/x-www-form-urlencoded', + }, + throwHttpErrors: false, + }); expect(result).toMatchObject({ body: '', - stderr: '', - exitCode: 0, + statusCode: 200, }); }); - it('throws emhttp output when curl returns a non-empty body', async () => { - vi.mocked(execa).mockResolvedValue({ - stdout: '', - stderr: '', - exitCode: 0, - command: 'curl', - escapedCommand: 'curl', - failed: false, - timedOut: false, - isCanceled: false, - killed: false, - } as Awaited>); + it('throws emhttp output when got returns a non-empty body', async () => { + vi.mocked(got.post).mockResolvedValue({ + body: '', + statusCode: 200, + } as Awaited>); await expect(emcmd({ changeNames: 'Apply' })).rejects.toThrow( '' ); }); - it('throws curl stderr when the socket transport fails', async () => { - vi.mocked(execa).mockResolvedValue({ - stdout: '', - stderr: 'curl: (56) Recv failure', - exitCode: 56, - command: 'curl', - escapedCommand: 'curl', - failed: true, - timedOut: false, - isCanceled: false, - killed: false, - } as Awaited>); - - await expect(emcmd({ changeNames: 'Apply' })).rejects.toThrow('curl: (56) Recv failure'); + it('throws on http failures reported by got', async () => { + vi.mocked(got.post).mockResolvedValue({ + body: '', + statusCode: 500, + } as Awaited>); + + await expect(emcmd({ changeNames: 'Apply' })).rejects.toThrow(); }); it('falls back to var.ini for the csrf token before retrying state loads', async () => { @@ -126,10 +101,11 @@ describe('emcmd', () => { expect(readFile).toHaveBeenCalledWith('/var/local/emhttp/var.ini', 'utf-8'); expect(store.dispatch).not.toHaveBeenCalled(); - expect(execa).toHaveBeenCalledWith( - 'curl', - expect.arrayContaining(['--data', 'changeNames=Apply&csrf_token=ini-token']), - { reject: false } + expect(got.post).toHaveBeenCalledWith( + 'http://unix:/var/run/emhttpd.socket:/update', + expect.objectContaining({ + body: 'changeNames=Apply&csrf_token=ini-token', + }) ); }); }); diff --git a/api/src/core/utils/clients/emcmd.ts b/api/src/core/utils/clients/emcmd.ts index f368b19c3b..3e8ac14d8b 100644 --- a/api/src/core/utils/clients/emcmd.ts +++ b/api/src/core/utils/clients/emcmd.ts @@ -1,6 +1,6 @@ import { readFile } from 'node:fs/promises'; -import { execa } from 'execa'; +import { got } from 'got'; import * as ini from 'ini'; import retry from 'p-retry'; @@ -112,44 +112,26 @@ export const emcmd = async ( }); params.append('csrf_token', csrfToken ?? ''); - const response = await execa( - 'curl', - [ - '--silent', - '--show-error', - '--unix-socket', - socketPath, - '--data', - params.toString(), - 'http://localhost/update', - ], - { - reject: false, - } - ); + const response = await got.post(`http://unix:${socketPath}:/update`, { + enableUnixSockets: true, + body: params.toString(), + headers: { + 'content-type': 'application/x-www-form-urlencoded', + }, + throwHttpErrors: false, + }); - if (response.exitCode !== 0) { - const curlError = response.stderr?.trim() || response.stdout?.trim(); - throw new Error(curlError || `emcmd curl failed with exit code ${response.exitCode}`); + if (response.statusCode >= 400) { + throw new Error(`emcmd request failed with status ${response.statusCode}`); } - const trimmedBody = response.stdout?.trim(); + const trimmedBody = response.body?.trim(); if (trimmedBody) { throw new Error(trimmedBody); } appLogger.debug('emcmd executed successfully'); - return { - body: response.stdout, - stderr: response.stderr, - exitCode: response.exitCode, - command: response.command, - escapedCommand: response.escapedCommand, - failed: response.failed, - timedOut: response.timedOut, - isCanceled: response.isCanceled, - killed: response.killed, - }; + return response; } catch (error: unknown) { if (hasErrorCode(error) && error.code === 'ENOENT') { if (error instanceof Error) { From a02a339fdb77ed90a8c40fd9c4b3734cbea54ca9 Mon Sep 17 00:00:00 2001 From: Ajit Mehrotra Date: Fri, 27 Mar 2026 18:22:59 -0400 Subject: [PATCH 4/5] fix(server-identity): verify ident persistence before failing - Purpose: make updateServerIdentity decide success from the persisted ident.cfg state instead of trusting the emcmd response alone.\n- Before: the mutation failed immediately on emcmd transport or response errors, even in cases where emhttp had already updated ident.cfg and the change would survive reboot.\n- Problem: users saw a GraphQL error for an update that actually persisted, while the stale-file case and the persisted-success case were both collapsed into the same generic failure path.\n- Now: the resolver always checks ident.cfg after the emcmd attempt, returns success when the requested identity is present on disk, and only throws when persistence verification fails or the file still contains the old values.\n- How: read getters.paths().identConfig with ini parsing, compare NAME/COMMENT/SYS_MODEL against the requested identity, preserve the original emcmd error as context when persistence did not happen, and add regression tests for the persisted-success, stale-file, and missing-file cases. --- .../resolvers/servers/server.service.spec.ts | 56 ++++++++++++++-- .../graph/resolvers/servers/server.service.ts | 66 ++++++++++++++++++- 2 files changed, 114 insertions(+), 8 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts b/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts index 393eecac49..a9a1802003 100644 --- a/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts +++ b/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts @@ -16,6 +16,7 @@ vi.mock('@app/core/utils/clients/emcmd.js', () => ({ vi.mock('@app/store/index.js', () => ({ getters: { emhttp: vi.fn(), + paths: vi.fn(), }, })); @@ -46,7 +47,9 @@ describe('ServerService', () => { lanIp: '192.168.1.10', }, } as ReturnType); - vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited>); + vi.mocked(getters.paths).mockReturnValue({ + identConfig: identConfigPath, + } as ReturnType); await mkdir(join(tempDirectory, 'boot/config'), { recursive: true }); await writeFile( @@ -54,6 +57,15 @@ describe('ServerService', () => { 'NAME="Tower"\nCOMMENT="Tower comment"\nSYS_MODEL="Model X100"\nEXTRA="keep-me"\n', 'utf8' ); + + vi.mocked(emcmd).mockImplementation(async (params) => { + await writeFile( + identConfigPath, + `NAME="${params.NAME}"\nCOMMENT="${params.COMMENT}"\nSYS_MODEL="${params.SYS_MODEL}"\nEXTRA="keep-me"\n`, + 'utf8' + ); + return { ok: true } as Awaited>; + }); }); afterEach(async () => { @@ -279,7 +291,23 @@ describe('ServerService', () => { ); }); - it('throws generic failure when emcmd fails and leaves ident.cfg untouched', async () => { + it('returns success when emcmd errors but ident.cfg has the requested identity', async () => { + vi.mocked(emcmd).mockImplementation(async () => { + await writeFile( + identConfigPath, + 'NAME="Tower"\nCOMMENT="Primary host"\nSYS_MODEL="Model X100"\nEXTRA="keep-me"\n', + 'utf8' + ); + throw new Error('socket failure'); + }); + + await expect(service.updateServerIdentity('Tower', 'Primary host')).resolves.toMatchObject({ + name: 'Tower', + comment: 'Primary host', + }); + }); + + it('throws generic failure when emcmd fails and ident.cfg stays unchanged', async () => { vi.mocked(emcmd).mockRejectedValue(new Error('socket failure')); const before = await readFile(identConfigPath, 'utf8'); @@ -293,15 +321,31 @@ describe('ServerService', () => { await expect(readFile(identConfigPath, 'utf8')).resolves.toBe(before); }); - it('throws generic failure when emcmd fails to persist identity', async () => { - vi.mocked(emcmd).mockImplementation(async () => { - throw new Error('ident persistence failed'); + it('throws when emcmd succeeds but ident.cfg does not contain the requested identity', async () => { + vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited>); + + await expect(service.updateServerIdentity('Tower', 'Primary host')).rejects.toMatchObject({ + message: 'Failed to update server identity', + extensions: { + cause: 'ident.cfg was not updated with the requested identity', + persistedIdentity: { + name: 'Tower', + comment: 'Tower comment', + sysModel: 'Model X100', + }, + }, }); + }); + + it('throws when persisted identity cannot be verified', async () => { + vi.mocked(getters.paths).mockReturnValue({ + identConfig: join(tempDirectory, 'boot/config/missing-ident.cfg'), + } as ReturnType); await expect(service.updateServerIdentity('Tower', 'Primary host')).rejects.toMatchObject({ message: 'Failed to update server identity', extensions: { - cause: 'ident persistence failed', + cause: expect.stringContaining('ENOENT'), }, }); }); diff --git a/api/src/unraid-api/graph/resolvers/servers/server.service.ts b/api/src/unraid-api/graph/resolvers/servers/server.service.ts index deff6b0c22..85ab7297ec 100644 --- a/api/src/unraid-api/graph/resolvers/servers/server.service.ts +++ b/api/src/unraid-api/graph/resolvers/servers/server.service.ts @@ -1,6 +1,8 @@ import { Injectable, Logger } from '@nestjs/common'; +import { readFile } from 'node:fs/promises'; import { GraphQLError } from 'graphql'; +import * as ini from 'ini'; import { emcmd } from '@app/core/utils/clients/emcmd.js'; import { getters } from '@app/store/index.js'; @@ -15,6 +17,31 @@ import { export class ServerService { private readonly logger = new Logger(ServerService.name); + private async readPersistedIdentity(): Promise<{ + name: string; + comment: string; + sysModel: string; + }> { + const identConfigPath = getters.paths().identConfig; + + if (!identConfigPath) { + throw new Error('ident.cfg path not found'); + } + + const contents = await readFile(identConfigPath, 'utf8'); + const parsed = ini.parse(contents) as { + NAME?: string; + COMMENT?: string; + SYS_MODEL?: string; + }; + + return { + name: parsed.NAME ?? '', + comment: parsed.COMMENT ?? '', + sysModel: parsed.SYS_MODEL ?? '', + }; + } + private buildIdentityUpdateParams( emhttpState: ReturnType, name: string, @@ -127,20 +154,55 @@ export class ServerService { } const params = this.buildIdentityUpdateParams(currentEmhttp, name, nextComment, nextSysModel); + let emcmdError: unknown; try { await emcmd(params, { waitForToken: true }); this.logger.log('Server identity updated successfully via emcmd.'); + } catch (error) { + emcmdError = error; + this.logger.error('emcmd reported a server identity update failure', error); + } + + try { + const persistedIdentity = await this.readPersistedIdentity(); + const persistedMatches = + persistedIdentity.name === name && + persistedIdentity.comment === nextComment && + persistedIdentity.sysModel === nextSysModel; + + if (!persistedMatches) { + throw new GraphQLError('Failed to update server identity', { + extensions: { + cause: + emcmdError instanceof Error && emcmdError.message + ? emcmdError.message + : 'ident.cfg was not updated with the requested identity', + persistedIdentity, + }, + }); + } + + if (emcmdError) { + this.logger.warn( + 'emcmd reported an error, but ident.cfg contains the requested server identity.' + ); + } + const latestEmhttp = getters.emhttp(); return this.buildServerResponse(latestEmhttp, name, nextComment); } catch (error) { - this.logger.error('Failed to update server identity', error); + if (error instanceof GraphQLError) { + throw error; + } + + this.logger.error('Failed to verify persisted server identity', error); throw new GraphQLError('Failed to update server identity', { extensions: { cause: error instanceof Error && error.message ? error.message - : 'Unknown server identity update failure', + : 'Unknown server identity persistence verification failure', }, }); } From b5c2d58ba01a452fc3b296256581233251ce6300 Mon Sep 17 00:00:00 2001 From: Ajit Mehrotra Date: Fri, 27 Mar 2026 18:31:52 -0400 Subject: [PATCH 5/5] test(server-identity): tighten emhttp mock typing - Purpose: make the server identity regression spec pass the full API type-check and package test matrix.\n- Before: the spec mocked getters.emhttp() with partial object literals cast directly to the emhttp slice type, which TypeScript rejected during pnpm type-check.\n- Problem: the branch-specific tests were green, but the full api/type-check gate still failed, so the package was not actually in a releasable state.\n- Now: the spec uses a typed helper that returns a full emhttp slice shape, and the array state cases use the real ArrayState enum values.\n- How: add a createEmhttpState() helper in the spec, reuse it across scenarios, and keep the mock data focused while satisfying the store slice contract. --- .../resolvers/servers/server.service.spec.ts | 120 +++++++++--------- 1 file changed, 58 insertions(+), 62 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts b/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts index a9a1802003..2fc2f1e287 100644 --- a/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts +++ b/api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts @@ -7,6 +7,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { emcmd } from '@app/core/utils/clients/emcmd.js'; import { getters } from '@app/store/index.js'; +import { type SliceState } from '@app/store/modules/emhttp.js'; +import { FileLoadStatus } from '@app/store/types.js'; +import { ArrayState } from '@app/unraid-api/graph/resolvers/array/array.model.js'; import { ServerService } from '@app/unraid-api/graph/resolvers/servers/server.service.js'; vi.mock('@app/core/utils/clients/emcmd.js', () => ({ @@ -20,6 +23,45 @@ vi.mock('@app/store/index.js', () => ({ }, })); +const createEmhttpState = ({ + name = 'Tower', + comment = 'Tower comment', + sysModel = 'Model X100', + fsState = 'Stopped', + mdState, + sslEnabled = true, +}: { + name?: string; + comment?: string; + sysModel?: string; + fsState?: string; + mdState?: SliceState['var']['mdState']; + sslEnabled?: boolean; +} = {}): SliceState => ({ + status: FileLoadStatus.LOADED, + var: { + name, + comment, + sysModel, + fsState, + mdState, + regGuid: 'GUID-123', + port: 80, + } as unknown as SliceState['var'], + devices: [], + networks: [{ ipaddr: ['192.168.1.10'] }] as unknown as SliceState['networks'], + nginx: { + sslEnabled, + lanName: 'tower.local', + lanIp: '192.168.1.10', + } as unknown as SliceState['nginx'], + shares: [], + disks: [], + users: [], + smbShares: [], + nfsShares: [], +}); + describe('ServerService', () => { let service: ServerService; let tempDirectory: string; @@ -31,22 +73,7 @@ describe('ServerService', () => { tempDirectory = await mkdtemp(join(tmpdir(), 'server-service-')); identConfigPath = join(tempDirectory, 'boot/config/ident.cfg'); - vi.mocked(getters.emhttp).mockReturnValue({ - var: { - name: 'Tower', - fsState: 'Stopped', - regGuid: 'GUID-123', - port: '80', - comment: 'Tower comment', - sysModel: 'Model X100', - }, - networks: [{ ipaddr: ['192.168.1.10'] }], - nginx: { - sslEnabled: true, - lanName: 'tower.local', - lanIp: '192.168.1.10', - }, - } as ReturnType); + vi.mocked(getters.emhttp).mockReturnValue(createEmhttpState()); vi.mocked(getters.paths).mockReturnValue({ identConfig: identConfigPath, } as ReturnType); @@ -119,23 +146,12 @@ describe('ServerService', () => { }); it('requires stopped array only when name changes', async () => { - vi.mocked(getters.emhttp).mockReturnValue({ - var: { - name: 'Tower', - mdState: 'STARTED', + vi.mocked(getters.emhttp).mockReturnValue( + createEmhttpState({ fsState: 'Started', - regGuid: 'GUID-123', - port: '80', - comment: 'Tower comment', - sysModel: 'Model X100', - }, - networks: [{ ipaddr: ['192.168.1.10'] }], - nginx: { - sslEnabled: true, - lanName: 'tower.local', - lanIp: '192.168.1.10', - }, - } as ReturnType); + mdState: ArrayState.STARTED, + }) + ); await expect(service.updateServerIdentity('NewTower', 'desc')).rejects.toThrow( 'The array must be stopped to change the server name.' @@ -148,23 +164,14 @@ describe('ServerService', () => { }); it('allows name change when mdState is STOPPED even if fsState is not Stopped', async () => { - vi.mocked(getters.emhttp).mockReturnValue({ - var: { - name: 'Tower', - mdState: 'STOPPED', - fsState: 'Started', - regGuid: 'GUID-123', - port: '80', + vi.mocked(getters.emhttp).mockReturnValue( + createEmhttpState({ comment: '', sysModel: '', - }, - networks: [{ ipaddr: ['192.168.1.10'] }], - nginx: { - sslEnabled: true, - lanName: 'tower.local', - lanIp: '192.168.1.10', - }, - } as ReturnType); + fsState: 'Started', + mdState: ArrayState.STOPPED, + }) + ); await expect(service.updateServerIdentity('NewTower', 'desc')).resolves.toMatchObject({ name: 'NewTower', @@ -264,22 +271,11 @@ describe('ServerService', () => { }); it('writes server_https as empty when ssl is disabled', async () => { - vi.mocked(getters.emhttp).mockReturnValue({ - var: { - name: 'Tower', - fsState: 'Stopped', - regGuid: 'GUID-123', - port: '80', - comment: 'Tower comment', - sysModel: 'Model X100', - }, - networks: [{ ipaddr: ['192.168.1.10'] }], - nginx: { + vi.mocked(getters.emhttp).mockReturnValue( + createEmhttpState({ sslEnabled: false, - lanName: 'tower.local', - lanIp: '192.168.1.10', - }, - } as ReturnType); + }) + ); await service.updateServerIdentity('Tower', 'Primary host', 'Model X100');