fix(ui-server): dedup duplicate station identity on add (#2026) (#2032)

* 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 <hashId>.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 <jerome.benoit@sap.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* 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 <jerome.benoit@sap.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
---------

Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

diff --git a/src/charging-station/Bootstrap.ts b/src/charging-station/Bootstrap.ts
index 7a0355b32c4bacab93f76236dfcbd53a46360a10..a68b8a05d74b96c11df754fdb9b909487be3ced9 100644 (file)
--- 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 5b917236ffbdf01e19da7ebe2921c176d11a22fe..ba651581597fb32426138bac8e6e39d2126518ca 100644 (file)
--- 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 b11987fde66eb8949c4f85c62c13366101e9807c..c63b94d11366effad75c2bab5568f54141e07f8c 100644 (file)
--- 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()
+      }
+    })
+  })
 })