From 44c2a0a237750147d52cbddd7884cdfe4d04eb95 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Sat, 18 Jul 2026 23:32:20 +0200 Subject: [PATCH] fix(ui-server): dedup duplicate station identity on add (#2026) (#2032) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * fix(ui-server): dedup duplicate station identity on add (#2026) hashId is a deterministic content hash of template identity fields + composed chargingStationId, with no uniqueness check on add. Two stations from a template with identical identity fields (e.g. fixedName: true) collide on one hashId. AbstractUIServer.setChargingStationData was last-writer-wins by timestamp keyed by hashId, so the twin silently overwrote the first station in the UI registry; a targeted broadcast command then completed success on the first worker's reply while the collided twin was orphaned. Add post-creation identity dedup at the registry write choke point: setChargingStationData returns a discriminated 'set' | 'stale' | 'collision' outcome. A write whose hashId already maps to a station with a different templateIndex is rejected as 'collision' without overwriting, guarding every worker event (added/started/stopped/updated) uniformly. A restart re-emit reuses the same templateIndex and still updates by timestamp, so there is no regression. workerEventAdded logs the rejected collision at error level instead of silently reporting the twin as added. getHashId output is unchanged (byte-identical), so persisted .json configuration, the registry map key, broadcast routing and stats resolve untouched. No OCPP PDU / message-format change; simulator station-registry lifecycle only. Tests: a twin (same hashId, different templateIndex) is rejected and does not overwrite; a restart re-emit (same identity, newer timestamp) still updates; a stale same-identity re-emit is dropped; a targeted broadcast reports no false success after a collision. Mutation-verified: reverting the guard fails the twin/false-success tests while the restart/stale tests stay green. Duplicate-identity aggregation rework and hash-algorithm / per-instance-salt changes are out of scope (per issue). Causally linked to #2027: an orphaned twin is exactly the worker #2027 cannot individually terminate. Closes #2026. Signed-off-by: Jérôme Benoit Co-authored-by: Claude Opus 4.8 * fix(ui-server): discriminate station identity by (templateName, templateIndex) (#2026) Strengthen the add-time identity dedup after a multi-reviewer cross-validation. The initial guard discriminated on templateIndex alone, but getHashId does not hash the template file name and the index space is per-templateName: two identity-clone template files (copy + rename, both fixedName, same baseName and identity fields) can produce the same hashId AND the same templateIndex. The templateIndex-only guard treated them as the same station and silently overwrote one, reintroducing the last-writer-wins bug for cross-template collisions. Discriminate on the full physical-station identity, the (templateName, templateIndex) pair: a cached entry differing in either field is a collision. A legitimate restart re-emit keeps both fields and still updates by timestamp (no regression). Sync the SetChargingStationDataOutcome doc to the pair, add templateName to the rejection log so an equal-index clone collision is diagnosable, and add a test for the same-templateIndex / different-templateName case. Mutation-verified: reverting the templateName clause fails only the new clone-file test while the same-template twin, restart, stale, and false-success tests stay green. Signed-off-by: Jérôme Benoit Co-authored-by: Claude Opus 4.8 --------- Signed-off-by: Jérôme Benoit Co-authored-by: Claude Opus 4.8 --- src/charging-station/Bootstrap.ts | 18 ++- .../ui-server/AbstractUIServer.ts | 29 ++++- .../ui-services/AbstractUIService.test.ts | 117 ++++++++++++++++++ 3 files changed, 157 insertions(+), 7 deletions(-) diff --git a/src/charging-station/Bootstrap.ts b/src/charging-station/Bootstrap.ts index 7a0355b3..a68b8a05 100644 --- a/src/charging-station/Bootstrap.ts +++ b/src/charging-station/Bootstrap.ts @@ -724,7 +724,17 @@ export class Bootstrap extends EventEmitter implements IBootstrap { } private readonly workerEventAdded = (data: ChargingStationData): void => { - if (this.uiServer.setChargingStationData(data.stationInfo.hashId, data)) { + const outcome = this.uiServer.setChargingStationData(data.stationInfo.hashId, data) + if (outcome === 'collision') { + logger.error( + `${this.logPrefix()} ${moduleName}.workerEventAdded: Rejected charging station ${ + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + data.stationInfo.chargingStationId + } (hashId: ${data.stationInfo.hashId}, templateName: ${data.stationInfo.templateName}, templateIndex: ${data.stationInfo.templateIndex.toString()}): a different station identity already maps to this hashId; the added station is orphaned from the UI registry` + ) + return + } + if (outcome === 'set') { this.uiServer.scheduleClientNotification() } logger.info( @@ -773,7 +783,7 @@ export class Bootstrap extends EventEmitter implements IBootstrap { } private readonly workerEventStarted = (data: ChargingStationData): void => { - if (this.uiServer.setChargingStationData(data.stationInfo.hashId, data)) { + if (this.uiServer.setChargingStationData(data.stationInfo.hashId, data) === 'set') { this.uiServer.scheduleClientNotification() } const templateStatistics = this.templateStatistics.get(data.stationInfo.templateName) @@ -789,7 +799,7 @@ export class Bootstrap extends EventEmitter implements IBootstrap { } private readonly workerEventStopped = (data: ChargingStationData): void => { - if (this.uiServer.setChargingStationData(data.stationInfo.hashId, data)) { + if (this.uiServer.setChargingStationData(data.stationInfo.hashId, data) === 'set') { this.uiServer.scheduleClientNotification() } const templateStatistics = this.templateStatistics.get(data.stationInfo.templateName) @@ -805,7 +815,7 @@ export class Bootstrap extends EventEmitter implements IBootstrap { } private readonly workerEventUpdated = (data: ChargingStationData): void => { - if (this.uiServer.setChargingStationData(data.stationInfo.hashId, data)) { + if (this.uiServer.setChargingStationData(data.stationInfo.hashId, data) === 'set') { this.uiServer.scheduleClientNotification() } } diff --git a/src/charging-station/ui-server/AbstractUIServer.ts b/src/charging-station/ui-server/AbstractUIServer.ts index 5b917236..ba651581 100644 --- a/src/charging-station/ui-server/AbstractUIServer.ts +++ b/src/charging-station/ui-server/AbstractUIServer.ts @@ -51,6 +51,19 @@ import { } from './UIServerSecurity.js' import { getUsernameAndPasswordFromAuthorizationToken, HttpMethod } from './UIServerUtils.js' +/** + * Outcome of {@link AbstractUIServer.setChargingStationData}: + * - `set`: stored (new `hashId`, or a newer/equal-timestamp update of the same + * physical station identity, including a legitimate restart re-emit). + * - `stale`: an older-timestamp update of the same physical station identity; + * dropped. + * - `collision`: a different physical station identity — a distinct + * `(templateName, templateIndex)` pair — already maps to this deterministic + * `hashId`; rejected without overwriting so the registered station is not + * silently shadowed (the caller surfaces the rejected twin). + */ +export type SetChargingStationDataOutcome = 'collision' | 'set' | 'stale' + /** * Outcome of {@link AbstractUIServer.runRequestPrologue}. * @@ -333,13 +346,23 @@ export abstract class AbstractUIServer { */ public abstract sendResponse (response: ProtocolResponse): void - public setChargingStationData (hashId: string, data: ChargingStationData): boolean { + public setChargingStationData ( + hashId: string, + data: ChargingStationData + ): SetChargingStationDataOutcome { const cachedData = this.chargingStations.get(hashId) + if ( + cachedData != null && + (cachedData.stationInfo.templateName !== data.stationInfo.templateName || + cachedData.stationInfo.templateIndex !== data.stationInfo.templateIndex) + ) { + return 'collision' + } if (cachedData == null || data.timestamp >= cachedData.timestamp) { this.chargingStations.set(hashId, data) - return true + return 'set' } - return false + return 'stale' } public setChargingStationTemplates (templates: string[] | undefined): void { diff --git a/tests/charging-station/ui-server/ui-services/AbstractUIService.test.ts b/tests/charging-station/ui-server/ui-services/AbstractUIService.test.ts index b11987fd..c63b94d1 100644 --- a/tests/charging-station/ui-server/ui-services/AbstractUIService.test.ts +++ b/tests/charging-station/ui-server/ui-services/AbstractUIService.test.ts @@ -11,6 +11,7 @@ import type { AbstractUIService } from '../../../../src/charging-station/ui-serv import type { BroadcastChannelResponse, BroadcastChannelResponsePayload, + ChargingStationData, UUIDv4, } from '../../../../src/types/index.js' @@ -96,6 +97,34 @@ const emitWorkerResponse = ( channel.onmessage({ data: [uuid, responsePayload] }) } +/** + * Build charging station data with an explicit `templateIndex` under `hashId`, + * to exercise identity-collision dedup deterministically. + * @param hashId - Deterministic content hash shared by colliding twins. + * @param templateIndex - Stable per-instance discriminator within a template. + * @param timestamp - Registry merge timestamp in milliseconds. + * @param templateName - Owning template name; differs for identity-clone template files. + * @returns Charging station data for the described identity. + */ +const createStationDataWithIndex = ( + hashId: string, + templateIndex: number, + timestamp: number, + templateName = 'collision-template' +): ChargingStationData => + createMockChargingStationData(hashId, { + stationInfo: { + baseName: 'collision-base', + chargePointModel: 'TestModel', + chargePointVendor: 'TestVendor', + chargingStationId: hashId, + hashId, + templateIndex, + templateName, + }, + timestamp, + }) + await describe('AbstractUIService', async () => { afterEach(() => { standardCleanup() @@ -962,4 +991,92 @@ await describe('AbstractUIService', async () => { service.stop() } }) + + await describe('AbstractUIServer identity-collision dedup (issue #2026)', async () => { + await it('should reject a twin whose hashId collides with a different templateIndex', () => { + const server = new TestableUIWebSocketServer(createMockUIServerConfiguration()) + const hashId = 'collision-hash' + assert.strictEqual( + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 1, 1000)), + 'set' + ) + assert.strictEqual( + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 2, 2000)), + 'collision' + ) + assert.strictEqual(server.getChargingStationData(hashId)?.stationInfo.templateIndex, 1) + assert.strictEqual(server.getChargingStationsCount(), 1) + }) + + await it('should reject an identity-clone twin sharing hashId and templateIndex but a different templateName', () => { + const server = new TestableUIWebSocketServer(createMockUIServerConfiguration()) + const hashId = 'clone-file-hash' + assert.strictEqual( + server.setChargingStationData( + hashId, + createStationDataWithIndex(hashId, 1, 1000, 'template-a') + ), + 'set' + ) + assert.strictEqual( + server.setChargingStationData( + hashId, + createStationDataWithIndex(hashId, 1, 2000, 'template-b') + ), + 'collision' + ) + assert.strictEqual( + server.getChargingStationData(hashId)?.stationInfo.templateName, + 'template-a' + ) + assert.strictEqual(server.getChargingStationsCount(), 1) + }) + + await it('should still update the registry for a restart re-emit with a newer timestamp', () => { + const server = new TestableUIWebSocketServer(createMockUIServerConfiguration()) + const hashId = 'restart-hash' + assert.strictEqual( + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 1, 1000)), + 'set' + ) + assert.strictEqual( + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 1, 2000)), + 'set' + ) + assert.strictEqual(server.getChargingStationData(hashId)?.timestamp, 2000) + }) + + await it('should drop a stale same-identity re-emit with an older timestamp', () => { + const server = new TestableUIWebSocketServer(createMockUIServerConfiguration()) + const hashId = 'stale-hash' + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 1, 2000)) + assert.strictEqual( + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 1, 1000)), + 'stale' + ) + assert.strictEqual(server.getChargingStationData(hashId)?.timestamp, 2000) + }) + + await it('should not report false success for a targeted broadcast after a twin collision', async () => { + const server = new TestableUIWebSocketServer(createMockUIServerConfiguration()) + server.testRegisterProtocolVersionUIService(ProtocolVersion['0.0.1']) + const service = server.getUIService(ProtocolVersion['0.0.1']) + if (service == null) { + assert.fail('Expected UI service to be registered') + } + const hashId = 'broadcast-collision-hash' + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 1, 1000)) + server.setChargingStationData(hashId, createStationDataWithIndex(hashId, 2, 2000)) + try { + await registerTransportStopRequestFor(service, TEST_UUID, [hashId]) + assert.strictEqual(service.getBroadcastChannelOutstandingResponseCount(TEST_UUID), 1) + emitWorkerResponse(service, { hashId, status: ResponseStatus.SUCCESS }, TEST_UUID) + assert.strictEqual(service.getBroadcastChannelOutstandingResponseCount(TEST_UUID), 0) + assert.strictEqual(server.getChargingStationsCount(), 1) + assert.strictEqual(server.getChargingStationData(hashId)?.stationInfo.templateIndex, 1) + } finally { + service.stop() + } + }) + }) }) -- 2.53.0