From: Jérôme Benoit Date: Mon, 6 Jul 2026 17:33:06 +0000 (+0200) Subject: fix(ocpp): from-zero audit — Critical + High spec conformance and safety fixes (... X-Git-Tag: cli@v4.11.0~52 X-Git-Url: https://git.piment-noir.org/?a=commitdiff_plain;h=6da3e6f8cd22ba7aa297d574a73b261b2739db35;p=e-mobility-charging-stations-simulator.git fix(ocpp): from-zero audit — Critical + High spec conformance and safety fixes (#1961) Eleven commits from the from-zero audit (three review rounds). Fixes: - C2 (§4.2) forbid non-triggered outbound messages in Pending state under non-strict compliance - H2 emit StatusNotification(Finishing) before StopTransaction.req in the non-remote stop path - H6 (§5.11) RemoteStart auto-select skips connectors reserved for a different idTag - H9 partial: .unref() on 5 lifecycle timer sites (3 setTimeout + 2 setInterval) so they no longer block node.js shutdown; per-site safety-invariant comments - H10 partial: initialize 5 string ChargingStation fields in the constructor; drop their definite-assignment markers - H11 log the errors previously swallowed by .catch(() => undefined) in AbstractUIServer Review follow-ups: - Correct method name in stop() metrics-cleanup log prefix - Clarify runMetricsScrape catch-log wording - Correct spec-citation in stopTransactionOnConnector test title Gates: format / typecheck / lint / build / test (2951 pass / 0 fail / 6 skipped) all pass at tip ca6b0182. Skott circular-dependency count preserved at 61. Empirically-rescinded audit findings: H4, H5, H8. Deferred with concrete rationale: H7, H9 residual, H10 residual. --- diff --git a/src/charging-station/ChargingStation.ts b/src/charging-station/ChargingStation.ts index 22f2136c..47371d50 100644 --- a/src/charging-station/ChargingStation.ts +++ b/src/charging-station/ChargingStation.ts @@ -209,20 +209,20 @@ export class ChargingStation extends EventEmitter { private automaticTransactionGeneratorConfiguration?: AutomaticTransactionGeneratorConfiguration private readonly chargingStationWorkerBroadcastChannel: ChargingStationWorkerBroadcastChannel - private configurationFile!: string - private configurationFileHash!: string + private configurationFile: string + private configurationFileHash: string private configuredSupervisionUrl!: URL private readonly connectors: Map - private connectorsConfigurationHash!: string + private connectorsConfigurationHash: string private readonly evses: Map - private evsesConfigurationHash!: string + private evsesConfigurationHash: string private flushingMessageBuffer: boolean private flushMessageBufferSetInterval?: NodeJS.Timeout private readonly messageQueue: string[] private ocppIncomingRequestService!: OCPPIncomingRequestService private readonly sharedLRUCache: SharedLRUCache private stopping: boolean - private templateFileHash!: string + private templateFileHash: string private templateFileWatcher?: FSWatcher private wsConnectionRetryCount: number private wsPingSetInterval?: NodeJS.Timeout @@ -244,6 +244,11 @@ export class ChargingStation extends EventEmitter { this.sharedLRUCache = SharedLRUCache.getInstance() this.idTagsCache = IdTagsCache.getInstance() this.chargingStationWorkerBroadcastChannel = new ChargingStationWorkerBroadcastChannel(this) + this.configurationFile = '' + this.configurationFileHash = '' + this.connectorsConfigurationHash = '' + this.evsesConfigurationHash = '' + this.templateFileHash = '' this.on(ChargingStationEvents.added, () => { parentPort?.postMessage(buildAddedMessage(this)) diff --git a/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts b/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts index 62b9aa42..95921fbb 100644 --- a/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts +++ b/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts @@ -590,6 +590,8 @@ export class OCPP16IncomingRequestService extends OCPPIncomingRequestService { ) }) } else { + // Unref: fire-and-forget deferred firmware update must not + // block node.js exit. setTimeout(() => { this.updateFirmwareSimulation(chargingStation).catch((error: unknown) => { logger.error( @@ -597,7 +599,7 @@ export class OCPP16IncomingRequestService extends OCPPIncomingRequestService { error ) }) - }, retrieveDate.getTime() - now) + }, retrieveDate.getTime() - now).unref() } } ) @@ -1301,10 +1303,14 @@ export class OCPP16IncomingRequestService extends OCPPIncomingRequestService { connectorId <= chargingStation.getNumberOfConnectors(); connectorId++ ) { + const connectorStatus = chargingStation.getConnectorStatus(connectorId) + const isReservedForOther = + connectorStatus?.status === OCPP16ChargePointStatus.Reserved && + !OCPP16ServiceUtils.hasReservation(chargingStation, connectorId, commandPayload.idTag) if ( chargingStation.isConnectorAvailable(connectorId) && - chargingStation.getConnectorStatus(connectorId)?.transactionStarted === false && - !OCPP16ServiceUtils.hasReservation(chargingStation, connectorId, commandPayload.idTag) + connectorStatus?.transactionStarted === false && + !isReservedForOther ) { commandPayload.connectorId = connectorId break diff --git a/src/charging-station/ocpp/1.6/OCPP16ServiceUtils.ts b/src/charging-station/ocpp/1.6/OCPP16ServiceUtils.ts index 4f2ed6c8..1b137cf0 100644 --- a/src/charging-station/ocpp/1.6/OCPP16ServiceUtils.ts +++ b/src/charging-station/ocpp/1.6/OCPP16ServiceUtils.ts @@ -949,6 +949,13 @@ export class OCPP16ServiceUtils { connectorId: number, reason?: StopTransactionReason ): Promise { + const connectorStatus = chargingStation.getConnectorStatus(connectorId) + if (connectorStatus?.status !== OCPP16ChargePointStatus.Finishing) { + await sendAndSetConnectorStatus(chargingStation, { + connectorId, + status: OCPP16ChargePointStatus.Finishing, + } as OCPP16StatusNotificationRequest) + } const rawTransactionId = chargingStation.getConnectorStatus(connectorId)?.transactionId const transactionId = rawTransactionId != null ? convertToInt(rawTransactionId) : undefined if ( diff --git a/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts b/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts index 4ec20e2f..c8af668c 100644 --- a/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts +++ b/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts @@ -3319,6 +3319,8 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { logger.info( `${chargingStation.logPrefix()} ${moduleName}.scheduleEvseReset: Executing EVSE ${evseId.toString()} reset${hasActiveTransactions ? ' after transaction termination' : ''}` ) + // Unref: fire-and-forget pending EVSE reset must not block + // node.js exit. setTimeout(() => { const evse = chargingStation.getEvseStatus(evseId) if (evse) { @@ -3337,7 +3339,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { `${chargingStation.logPrefix()} ${moduleName}.scheduleEvseReset: EVSE ${evseId.toString()} reset completed` ) } - }, OCPP20Constants.RESET_DELAY_MS) + }, OCPP20Constants.RESET_DELAY_MS).unref() }) } @@ -3347,6 +3349,8 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { * @param evseId - The EVSE identifier to reset */ private scheduleEvseResetOnIdle (chargingStation: ChargingStation, evseId: number): void { + // Unref: idle-monitor poll must not block node.js exit; the interval + // self-clears once the EVSE is idle or its state is gone. const monitorInterval = setInterval(() => { const evse = chargingStation.getEvseStatus(evseId) if (evse != null) { @@ -3360,7 +3364,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { } else { clearInterval(monitorInterval) } - }, OCPP20Constants.RESET_IDLE_MONITOR_INTERVAL_MS) + }, OCPP20Constants.RESET_IDLE_MONITOR_INTERVAL_MS).unref() } /** @@ -3368,6 +3372,8 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { * @param chargingStation - The charging station instance */ private scheduleResetOnIdle (chargingStation: ChargingStation): void { + // Unref: idle-monitor poll must not block node.js exit; the interval + // self-clears once the station reports idle. const monitorInterval = setInterval(() => { if (this.isChargingStationIdle(chargingStation)) { clearInterval(monitorInterval) @@ -3381,7 +3387,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { ) }) } - }, OCPP20Constants.RESET_IDLE_MONITOR_INTERVAL_MS) + }, OCPP20Constants.RESET_IDLE_MONITOR_INTERVAL_MS).unref() } private selectAvailableEvse (chargingStation: ChargingStation): number | undefined { @@ -3611,9 +3617,10 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { ) queue.unshift({ ...event, retryCount }) stationState.isDrainingSecurityEvents = false + // Unref: fire-and-forget retry must not block node.js exit. setTimeout(() => { this.sendQueuedSecurityEvents(chargingStation) - }, OCPP20Constants.SECURITY_EVENT_RETRY_DELAY_MS) + }, OCPP20Constants.SECURITY_EVENT_RETRY_DELAY_MS).unref() }) } drainNextEvent() diff --git a/src/charging-station/ocpp/OCPPRequestService.ts b/src/charging-station/ocpp/OCPPRequestService.ts index 09e4e2e5..e30b5f35 100644 --- a/src/charging-station/ocpp/OCPPRequestService.ts +++ b/src/charging-station/ocpp/OCPPRequestService.ts @@ -301,7 +301,7 @@ export abstract class OCPPRequestService { chargingStation.inRejectedState()) && commandName === RequestCommand.BOOT_NOTIFICATION) || (chargingStation.stationInfo?.ocppStrictCompliance === false && - (chargingStation.inUnknownState() || chargingStation.inPendingState())) || + chargingStation.inUnknownState()) || chargingStation.inAcceptedState() || (chargingStation.inPendingState() && (params.triggerMessage === true || messageType === MessageType.CALL_RESULT_MESSAGE)) diff --git a/src/charging-station/ui-server/AbstractUIServer.ts b/src/charging-station/ui-server/AbstractUIServer.ts index 73f8fa91..6fbd884a 100644 --- a/src/charging-station/ui-server/AbstractUIServer.ts +++ b/src/charging-station/ui-server/AbstractUIServer.ts @@ -415,7 +415,12 @@ export abstract class AbstractUIServer { .finally(() => { registry.clear() }) - .catch(() => undefined) + .catch((error: unknown) => { + logger.warn( + `${this.logPrefix()} ${moduleName}.stop: metrics registry cleanup failed`, + error + ) + }) this.metricsRegistry = undefined } // detachTransport() / uiService.stop() are subclass hooks — see @@ -1268,7 +1273,12 @@ export abstract class AbstractUIServer { registry: Registry ): Promise { this.metricsScrapeChain = this.metricsScrapeChain - .catch(() => undefined) + .catch((error: unknown) => { + logger.warn( + `${this.logPrefix()} ${moduleName}.runMetricsScrape: previous scrape errored; absorbing rejection so next scrape can proceed`, + error + ) + }) .then(async () => { this.metricsSampleCount = 0 // prom-client's sync prefix runs every collect() (each writing diff --git a/tests/charging-station/ocpp/OCPPServiceOperations.test.ts b/tests/charging-station/ocpp/OCPPServiceOperations.test.ts index 525f1a93..4f24efcc 100644 --- a/tests/charging-station/ocpp/OCPPServiceOperations.test.ts +++ b/tests/charging-station/ocpp/OCPPServiceOperations.test.ts @@ -56,7 +56,7 @@ await describe('OCPPServiceOperations', async () => { }) await describe('stopTransactionOnConnector', async () => { - await it('should send StopTransaction for OCPP 1.6 stations and return accepted: true', async () => { + await it('should send StatusNotification(Finishing) then StopTransaction for OCPP 1.6 stations (matches remote-stop message sequence)', async () => { const { requestHandler, station } = createStationWithRequestHandler() requestHandler.mock.mockImplementation(async (..._args: unknown[]) => Promise.resolve({ idTagInfo: { status: 'Accepted' } }) @@ -67,10 +67,15 @@ await describe('OCPPServiceOperations', async () => { assert.strictEqual(result.accepted, true) assert.ok( - requestHandler.mock.calls.length >= 1, - 'request handler should have been called at least once' + requestHandler.mock.calls.length >= 2, + 'request handler should have been called at least twice (StatusNotification + StopTransaction)' + ) + assert.strictEqual(requestHandler.mock.calls[0].arguments[1], 'StatusNotification') + assert.strictEqual( + (requestHandler.mock.calls[0].arguments[2] as { status: string }).status, + 'Finishing' ) - assert.strictEqual(requestHandler.mock.calls[0].arguments[1], 'StopTransaction') + assert.strictEqual(requestHandler.mock.calls[1].arguments[1], 'StopTransaction') }) await it('should send TransactionEvent(Ended) for OCPP 2.0 stations and return accepted: true', async () => {