From: Jérôme Benoit Date: Thu, 9 Jul 2026 22:45:56 +0000 (+0200) Subject: fix(ocpp): guard post-stop() handler dispatch against WeakMap resurrection (#1992) X-Git-Tag: cli@v4.11.0~39 X-Git-Url: https://git.piment-noir.org/?a=commitdiff_plain;h=4fce19aef989e2c3f62989b4623cb908a5d5ae7c;p=e-mobility-charging-stations-simulator.git fix(ocpp): guard post-stop() handler dispatch against WeakMap resurrection (#1992) Closes #1970 OCPP 2.0.1 handlers dispatched from queued microtasks, unref'd retry timers, or EventEmitter-dispatched `.on(...)` listeners could run AFTER `stop()` and resurrect the `stationsState` WeakMap entry via `getOrCreateStationState` lazy-init. `sendQueuedSecurityEvents` also scheduled a retry `setTimeout(...)` whose handle was never stored, so `stop()` had no way to cancel a pending retry, and the recursive retry callback re-entered `sendQueuedSecurityEvents` — resurrecting state. Three coordinated parts. Part 1 — base plumbing (`OCPPIncomingRequestService.ts`): - Generic bound widened to `TStationState extends { stopped?: boolean } = { stopped?: boolean }`. - `stop()` template: after `resetStationState`, marks `stationState.stopped = true` instead of `stationsState.delete(cs)`. Deletion would re-enable resurrection via `getOrCreateStationState` lazy-init on any late dispatch. Keeping the sealed entry lets the getter return the sealed state and raw `.get()` null-guarded callers observe the marker and drop. The WeakMap entry is naturally collected when the ChargingStation reference is dropped. - `getOrCreateStationState` returns the sealed stopped state when `state?.stopped === true`; no fresh entry is lazy-init'd post-stop. - Both concrete state interfaces (`OCPP16StationState`, `OCPP20StationState`) get an alphabetized `stopped?: boolean` field. Part 2 — OCPP 2.0.1 call-site audit (11 sites): Two sites converted to raw `stationsState.get(cs)` + `stopped === true` null-guard, silent-drop: - `sendNotifyReportRequest` — dispatched from `.on(GET_BASE_REPORT).catch(...)` microtask; state usually created by sibling `handleRequestGetBaseReport` but external `emit(...)` paths and post-stop dispatch are covered by the null-guard. - `sendQueuedSecurityEvents` — dispatched from unref'd retry `setTimeout`; guard is the last line of defense against a fired-before-cancel race. Four sites use `getOrCreateStationState` + explicit `stopped` check (state may not exist yet on the pre-stop happy path): - `sendSecurityEventNotification` — lifecycle-entry on the invalid-cert-on-first-request edge case (`handleRequestCertificateSigned` X.509-invalid branch, `handleRequestUpdateFirmware` invalid-PEM branch). - `getCertSigningRetryManager` — public accessor; return type widened to `OCPP20CertSigningRetryManager | undefined`; two callers (`handleRequestCertificateSigned`, `OCPP20ResponseService`) updated with `?.` to tolerate the undefined return. Without this guard, a late `handleResponseSignCertificate` dispatch (a SignCertificate response can arrive between `ocppIncomingRequestService.stop(cs)` and `closeWSConnection()` in `ChargingStation.stop()`) would materialize a fresh manager on the sealed state and schedule a non-`.unref()`'d retry `setTimeout` holding the ChargingStation reference. - `simulateFirmwareUpdateLifecycle` — dispatched from `.on(UPDATE_FIRMWARE).catch(...)` microtask; guards against driving the full async lifecycle on a sealed state (which would send OCPP requests through a closed WebSocket and clobber the abort-controller fields). - `simulateLogUploadLifecycle` — symmetric, `.on(GET_LOG).catch(...)`. Five sites kept unchanged (Part 1 guard covers them): `getRestoredConnectorStatus`, `handleRequestGetBaseReport`, `handleRequestUpdateFirmware`, `savePreInoperativeStatuses`, `sendFirmwareStatusNotification`. Part 3 — retry timer discipline in `sendQueuedSecurityEvents`: - Store the retry `setTimeout` handle on `OCPP20StationState.securityEventRetryTimer`. - Cancel any previously scheduled retry before scheduling a new one. - Self-clear the handle before the recursive `sendQueuedSecurityEvents` call inside the callback. - New `cancelSecurityEventRetryTimer` helper is called from `resetStationState` (after abort-controller aborts and cert-signing retry cancel, before the two `resetActive*State` helpers). - `.unref()` preserved. Files touched (7 modified + 1 new): - `src/charging-station/ocpp/OCPPIncomingRequestService.ts` - `src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts` - `src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts` - `src/charging-station/ocpp/2.0/OCPP20ResponseService.ts` - `tests/charging-station/ocpp/OCPPIncomingRequestService-StationState.test.ts` - `tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts` - `tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-GetDiagnostics.test.ts` - `tests/charging-station/ocpp/2.0/OCPP20IncomingRequestService-PostStopResurrection.test.ts` (new) Design rationale — silent-drop over `StationStoppedError` throw: matches the "post-stop silently drop" semantic and avoids catch-block sprawl at every handler. Design rationale — return `undefined` from `getCertSigningRetryManager` over convert-with-explicit-null-manager: two callers already handle optional chaining cleanly; return-type widening is the minimum change. Verification: pnpm format / typecheck / lint pass; pnpm test 3029 pass, 0 fail; pnpm build exit 0. No `stationsState.delete` remains in `src/`; `securityEventRetryTimer` lives in exactly three logical places (interface field, retry-site store + self-clear, cancel helper called from `resetStationState`). References: #1983 (shared base plumbing), #1984 (OCPP 1.6 `deferredFirmwareUpdateTimer` timer discipline). --- diff --git a/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts b/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts index 0b6c5340..d27daec4 100644 --- a/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts +++ b/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts @@ -171,7 +171,7 @@ interface OCPP16StationState { * carry a `requestId`, so the temporal position tells the CSMS * which upload it belonged to). * 2. {@link OCPP16IncomingRequestService.resetStationState} on - * `stop()`, following the cancel-before-delete ordering documented + * `stop()`, following the cancel-before-mark ordering documented * on {@link OCPP20IncomingRequestService.resetStationState}. */ activeDiagnosticsAbortController?: AbortController @@ -226,6 +226,12 @@ interface OCPP16StationState { * invocation before the first completes. */ firmwareUpdateInProgress?: boolean + /** + * `true` after {@link OCPPIncomingRequestService.stop} has released + * per-station resources via {@link resetStationState}. Never + * set from OCPP 1.6 code directly. + */ + stopped?: boolean } /** @@ -745,15 +751,17 @@ export class OCPP16IncomingRequestService extends OCPPIncomingRequestService reportDataCache: Map securityEventQueue: QueuedSecurityEvent[] + /** + * `setTimeout` handle for the pending {@link sendQueuedSecurityEvents} + * retry. Stored so {@link resetStationState} can cancel it before the + * base template marks the entry `stopped: true`. Released by + * {@link cancelSecurityEventRetryTimer} on stop, re-schedule, or + * self-clear when the callback fires. + */ + securityEventRetryTimer?: NodeJS.Timeout + /** + * `true` after {@link OCPPIncomingRequestService.stop} has released + * per-station resources via {@link resetStationState}. Never set from + * OCPP 2.0.1 code directly. + */ + stopped?: boolean } interface QueuedSecurityEvent { @@ -666,12 +680,26 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { + // Silent-drop late `.on(GET_BASE_REPORT).catch(...)` dispatch when + // no state entry exists or the entry is sealed stopped. + const stationState = this.stationsState.get(chargingStation) + if (stationState == null || stationState.stopped === true) { + return + } const { reportBase, requestId } = request - const stationState = this.getOrCreateStationState(chargingStation) const cached = stationState.reportDataCache.get(requestId) const reportData = cached ?? this.buildReportData(chargingStation, reportBase) @@ -3710,7 +3760,13 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { - if (!isNotEmptyArray(queue) || !chargingStation.isWebSocketConnectionOpened()) { + if ( + stationState.stopped === true || + !isNotEmptyArray(queue) || + !chargingStation.isWebSocketConnectionOpened() + ) { stationState.isDrainingSecurityEvents = false return } @@ -3747,6 +3807,9 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { + if (stationState.stopped === true) { + return + } const retryCount = (event.retryCount ?? 0) + 1 if (retryCount >= OCPP20Constants.MAX_SECURITY_EVENT_SEND_ATTEMPTS) { logger.warn( @@ -3762,8 +3825,15 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { + // Store the retry handle on `stationState` so + // `resetStationState` can cancel it on `stop()`. + // Cancel any previously scheduled retry first. Unref: + // fire-and-forget retry must not block node.js exit. + this.cancelSecurityEventRetryTimer(stationState) + stationState.securityEventRetryTimer = setTimeout(() => { + // Self-clear before the recursive call: a subsequent retry + // must not observe a stale reference. + delete stationState.securityEventRetryTimer this.sendQueuedSecurityEvents(chargingStation) }, OCPP20Constants.SECURITY_EVENT_RETRY_DELAY_MS).unref() }) @@ -3812,10 +3882,16 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { const { installDateTime, location, retrieveDateTime, signature } = firmware + const stationState = this.getOrCreateStationState(chargingStation) + // Silent-drop late `.on(UPDATE_FIRMWARE).catch(...)` dispatch after + // `stop()`: the sealed state must not drive the async lifecycle + // nor clobber `activeFirmwareUpdateAbortController`. + if (stationState.stopped === true) { + return + } // Store the abort controller so a subsequent UpdateFirmware can abort this in-progress update const abortController = new AbortController() - const stationState = this.getOrCreateStationState(chargingStation) stationState.activeFirmwareUpdateAbortController = abortController stationState.activeFirmwareUpdateRequestId = requestId @@ -4087,6 +4169,11 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { const stationState = this.getOrCreateStationState(chargingStation) + // Silent-drop late `.on(GET_LOG).catch(...)` dispatch after + // `stop()`; symmetric with `simulateFirmwareUpdateLifecycle`. + if (stationState.stopped === true) { + return + } const abortController = new AbortController() stationState.activeLogUploadAbortController = abortController stationState.activeLogUploadRequestId = requestId diff --git a/src/charging-station/ocpp/2.0/OCPP20ResponseService.ts b/src/charging-station/ocpp/2.0/OCPP20ResponseService.ts index edd4931d..9ef4d0b0 100644 --- a/src/charging-station/ocpp/2.0/OCPP20ResponseService.ts +++ b/src/charging-station/ocpp/2.0/OCPP20ResponseService.ts @@ -361,9 +361,11 @@ export class OCPP20ResponseService extends OCPPResponseService { `${chargingStation.logPrefix()} ${moduleName}.handleResponseSignCertificate: SignCertificate response received, status: ${payload.status}` ) if (payload.status === GenericStatus.Accepted) { + // Optional-chain: `getCertSigningRetryManager` returns undefined + // when the station has been stopped. OCPP20IncomingRequestService.getInstance() .getCertSigningRetryManager(chargingStation) - .startRetryTimer(requestPayload.certificateType) + ?.startRetryTimer(requestPayload.certificateType) } } diff --git a/src/charging-station/ocpp/OCPPIncomingRequestService.ts b/src/charging-station/ocpp/OCPPIncomingRequestService.ts index c2d94595..95104f56 100644 --- a/src/charging-station/ocpp/OCPPIncomingRequestService.ts +++ b/src/charging-station/ocpp/OCPPIncomingRequestService.ts @@ -19,31 +19,36 @@ import { type Ajv, createAjv, validatePayload } from './OCPPServiceUtils.js' * * Provides shared plumbing for per-station lifecycle state: * - a protected {@link WeakMap} keyed by {@link ChargingStation}; - * - a lazy-init getter (`getOrCreateStationState`); - * - a concrete `stop()` template that resets and drops the entry. + * - a lazy-init getter (`getOrCreateStationState`) that returns the + * sealed stopped state instead of resurrecting a fresh entry after + * {@link stop}; + * - a concrete `stop()` template that resets and marks the entry + * `stopped: true`, preserving the WeakMap entry so late dispatch + * observes the marker and drops. * * Subclass contract: * - `createStationState` — factory returning the initial state object. * - `resetStationState` — releases any resources held by the state * (abort controllers, timer handles, retry managers). * - * The `stop()` template owns the ordering invariant (reset before delete) + * The `stop()` template owns the ordering invariant (reset before mark) * and the "only-if-present" guard. Subclasses that need additional * lifecycle cleanup should override `stop()` and call `super.stop()` * first; override `resetStationState` (not `stop()`) for state-field * reset logic. * @template TStationState - Concrete per-station state shape. - * The default `object` bound is load-bearing: it allows the bare + * The default `{ stopped?: boolean }` bound is load-bearing: the bare * `OCPPIncomingRequestService` reference in the static singleton - * registry ({@link OCPPIncomingRequestService.instances}) and in the - * `getInstance` constraint to - * resolve to `OCPPIncomingRequestService`. Removing the default - * would break both declarations with `TS2314` (generic type requires - * type arguments). The bound also matches the `WeakMap` value-type - * requirement (values must be non-primitive). + * registry ({@link OCPPIncomingRequestService.instances}) and the + * `getInstance` constraint + * resolve to `OCPPIncomingRequestService<{ stopped?: boolean }>`; + * removing the default breaks both with `TS2314`. The bound also + * matches the `WeakMap` value-type requirement (values must be + * non-primitive) and carries the marker field consumed by {@link stop} + * and {@link getOrCreateStationState}. */ export abstract class OCPPIncomingRequestService< - TStationState extends object = object + TStationState extends { stopped?: boolean } = { stopped?: boolean } > extends EventEmitter { private static readonly instances = new Map< new () => OCPPIncomingRequestService, @@ -188,24 +193,33 @@ export abstract class OCPPIncomingRequestService< * Stops the incoming-request service for the given charging station. * * Template method: subclasses SHOULD NOT override the template steps - * (WeakMap lookup, reset, delete) — override {@link resetStationState} - * for field-level cleanup. Subclasses MAY override to add - * lifecycle-scoped side effects (e.g. clearing external caches keyed - * on the station); such overrides MUST call `super.stop()` first so - * the reset-then-delete ordering is preserved. + * (WeakMap lookup, reset, mark stopped) — override + * {@link resetStationState} for field-level cleanup. Subclasses MAY + * override to add lifecycle-scoped side effects (e.g. clearing + * external caches keyed on the station); such overrides MUST call + * `super.stop()` first so the reset-then-mark ordering is preserved. * - * Exception behavior: if {@link resetStationState} throws, `WeakMap` - * eviction is skipped and a subsequent `stop()` re-invokes the hook - * on the same state. Any subclass extension after `super.stop()` is - * also skipped, as the throw propagates. Matches pre-refactor - * semantics exactly. + * The WeakMap entry is NOT deleted — it is marked `stopped: true` + * after {@link resetStationState} releases its resources. + * Deletion would re-enable resurrection via + * {@link getOrCreateStationState} lazy-init on any late handler + * dispatch. Keeping the sealed entry lets + * {@link getOrCreateStationState} return the sealed state and + * `stationsState.get(cs)` null-guarded callers observe the marker + * and drop. The WeakMap entry is naturally collected when the + * {@link ChargingStation} reference is dropped. + * + * Exception behavior: if {@link resetStationState} throws, the + * `stopped` mark is NOT set and a subsequent `stop()` re-invokes the + * hook on the same state. Any subclass extension after `super.stop()` + * is also skipped, as the throw propagates. * @param chargingStation - Target charging station. */ public stop (chargingStation: ChargingStation): void { const stationState = this.stationsState.get(chargingStation) if (stationState != null) { this.resetStationState(stationState) - this.stationsState.delete(chargingStation) + stationState.stopped = true } } @@ -219,12 +233,30 @@ export abstract class OCPPIncomingRequestService< /** * Returns the state entry for `chargingStation`, creating one on first * access via {@link createStationState}. Subsequent calls return the - * same reference until {@link stop} evicts the entry. + * same reference for the station's active lifecycle. + * + * Once {@link stop} has marked the entry `stopped: true`, + * this method returns the sealed stopped state instead of + * lazy-init'ing a fresh entry. `.stopped` is written only from + * {@link stop}, so the guard is unreachable on the pre-stop happy + * path. Callers that need "silent-drop on stop" semantics MUST NOT + * rely on this guard alone — they use `stationsState.get(cs)` with + * an inline `stopped === true` null-guard and return early, because + * writes performed through this getter still land on the sealed + * object (harmless but wasteful). * @param chargingStation - Target charging station. - * @returns The lazily-initialized per-station state. + * @returns The lazily-initialized per-station state, or the sealed + * stopped state after {@link stop}. + * @see OCPP20IncomingRequestService.sendNotifyReportRequest for the + * consume-only null-guard pattern. + * @see OCPP20IncomingRequestService.sendSecurityEventNotification for + * the lifecycle-entry pattern where creation is required. */ protected getOrCreateStationState (chargingStation: ChargingStation): TStationState { let state = this.stationsState.get(chargingStation) + if (state?.stopped === true) { + return state + } if (state == null) { state = this.createStationState() this.stationsState.set(chargingStation, state) @@ -246,12 +278,12 @@ export abstract class OCPPIncomingRequestService< /** * Hook method paired with the {@link stop} template: releases * resources held by the per-station state (abort controllers, timer - * handles, retry managers, etc.) prior to eviction from - * {@link stationsState}. + * handles, retry managers, etc.) prior to the template marking the + * entry `stopped: true` in {@link stationsState}. * - * Implementations MAY throw; on throw the base template skips - * `WeakMap` eviction and a subsequent `stop()` re-invokes this hook - * on the same partially-reset state. Implementations MUST therefore + * Implementations MAY throw; on throw the base template skips the + * `stopped` mark and a subsequent `stop()` re-invokes this hook on + * the same partially-reset state. Implementations MUST therefore * tolerate re-entry on a partially-reset state (idempotent field * clears + optional-chained aborts satisfy this). See {@link stop} * for the full exception-behavior contract. diff --git a/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts b/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts index 2fc58347..7fc1c117 100644 --- a/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts +++ b/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts @@ -356,6 +356,7 @@ await describe('OCPP16IncomingRequestService — Firmware', async () => { await describe('UPDATE_FIRMWARE deferred timer lifecycle', async () => { interface OCPP16StationStateShape { deferredFirmwareUpdateTimer?: NodeJS.Timeout + stopped?: boolean } interface PlumbingAccess { @@ -429,7 +430,12 @@ await describe('OCPP16IncomingRequestService — Firmware', async () => { assert.strictEqual(plumbing.stationsState.has(station), true) listenerService.stop(station) - assert.strictEqual(plumbing.stationsState.has(station), false) + assert.strictEqual(plumbing.stationsState.has(station), true) + assert.strictEqual(plumbing.stationsState.get(station)?.stopped, true) + assert.strictEqual( + plumbing.stationsState.get(station)?.deferredFirmwareUpdateTimer, + undefined + ) t.mock.timers.tick(120_000) await flushMicrotasks() @@ -541,7 +547,12 @@ await describe('OCPP16IncomingRequestService — Firmware', async () => { assert.strictEqual(plumbing.stationsState.has(stationB), true) listenerService.stop(stationA) - assert.strictEqual(plumbing.stationsState.has(stationA), false) + assert.strictEqual(plumbing.stationsState.has(stationA), true) + assert.strictEqual(plumbing.stationsState.get(stationA)?.stopped, true) + assert.strictEqual( + plumbing.stationsState.get(stationA)?.deferredFirmwareUpdateTimer, + undefined + ) assert.strictEqual(plumbing.stationsState.has(stationB), true) assert.notStrictEqual( plumbing.stationsState.get(stationB)?.deferredFirmwareUpdateTimer, diff --git a/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-GetDiagnostics.test.ts b/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-GetDiagnostics.test.ts index 2b948394..1c54d47b 100644 --- a/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-GetDiagnostics.test.ts +++ b/tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-GetDiagnostics.test.ts @@ -43,6 +43,7 @@ import { interface OCPP16StationStateShape { activeDiagnosticsAbortController?: AbortController diagnosticsUploadInProgress?: boolean + stopped?: boolean } interface PlumbingAccess { @@ -211,7 +212,7 @@ await describe('OCPP16IncomingRequestService — GetDiagnostics supersession', a assert.strictEqual(finalState?.diagnosticsUploadInProgress, undefined) }) - await it('should abort activeDiagnosticsAbortController and delete the WeakMap entry on stop()', () => { + await it('should abort activeDiagnosticsAbortController and mark the WeakMap entry stopped on stop()', () => { // Arrange — pre-populate an in-flight lifecycle, then stop. const { incomingRequestService, station } = context const plumbing = asPlumbing(incomingRequestService) @@ -223,14 +224,16 @@ await describe('OCPP16IncomingRequestService — GetDiagnostics supersession', a // Act incomingRequestService.stop(station) - // Assert — cancel-before-delete ordering: the signal must fire before the - // base template deletes the WeakMap entry, and the entry must be gone. + // Assert — cancel-before-mark ordering: the signal must fire before the + // base template marks the entry stopped, and the entry must be preserved + // and sealed. assert.strictEqual( inflightController.signal.aborted, true, 'stop() must signal the in-flight diagnostics AbortController' ) - assert.strictEqual(plumbing.stationsState.has(station), false) + assert.strictEqual(plumbing.stationsState.has(station), true) + assert.strictEqual(plumbing.stationsState.get(station)?.stopped, true) }) await it('should not install activeDiagnosticsAbortController for non-FTP locations', async () => { diff --git a/tests/charging-station/ocpp/2.0/OCPP20IncomingRequestService-PostStopResurrection.test.ts b/tests/charging-station/ocpp/2.0/OCPP20IncomingRequestService-PostStopResurrection.test.ts new file mode 100644 index 00000000..16496fe2 --- /dev/null +++ b/tests/charging-station/ocpp/2.0/OCPP20IncomingRequestService-PostStopResurrection.test.ts @@ -0,0 +1,380 @@ +/** + * @file Tests for OCPP20IncomingRequestService post-stop() resurrection guard + * @description Covers the OCPP 2.0.1 side of the shared post-stop + * resurrection guard: `sendSecurityEventNotification`, + * `sendQueuedSecurityEvents`, `sendNotifyReportRequest`, + * `getCertSigningRetryManager`, `simulateFirmwareUpdateLifecycle`, + * and `simulateLogUploadLifecycle` all silent-drop (or return + * undefined) when the WeakMap entry is absent or sealed + * `stopped: true`. Also exercises the `sendQueuedSecurityEvents` + * retry `setTimeout` handle discipline: store on state, self-clear + * on fire, cancel on re-schedule, cancel in `resetStationState`. + */ + +import assert from 'node:assert/strict' +import { afterEach, beforeEach, describe, it, mock } from 'node:test' + +import type { ChargingStation } from '../../../../src/charging-station/index.js' + +import { OCPP20IncomingRequestService } from '../../../../src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.js' +import { OCPPVersion } from '../../../../src/types/index.js' +import { Constants } from '../../../../src/utils/index.js' +import { + flushMicrotasks, + standardCleanup, + withMockTimers, +} from '../../../helpers/TestLifecycleHelpers.js' +import { TEST_CHARGING_STATION_BASE_NAME } from '../../ChargingStationTestConstants.js' +import { createMockChargingStation } from '../../helpers/StationHelpers.js' + +interface OCPP20StationStateShape { + activeFirmwareUpdateAbortController?: AbortController + activeFirmwareUpdateRequestId?: number + activeLogUploadAbortController?: AbortController + activeLogUploadRequestId?: number + certSigningRetryManager?: unknown + isDrainingSecurityEvents: boolean + reportDataCache: Map + securityEventQueue: QueuedSecurityEventShape[] + securityEventRetryTimer?: NodeJS.Timeout + stopped?: boolean +} + +interface PlumbingAccess { + createStationState: () => OCPP20StationStateShape + getOrCreateStationState: (chargingStation: ChargingStation) => OCPP20StationStateShape + sendNotifyReportRequest: ( + chargingStation: ChargingStation, + request: { reportBase: string; requestId: number }, + response: { status: string } + ) => Promise + sendQueuedSecurityEvents: (chargingStation: ChargingStation) => void + sendSecurityEventNotification: ( + chargingStation: ChargingStation, + type: string, + techInfo?: string + ) => void + simulateFirmwareUpdateLifecycle: ( + chargingStation: ChargingStation, + requestId: number, + firmware: unknown, + retries?: number, + retryInterval?: number + ) => Promise + simulateLogUploadLifecycle: (chargingStation: ChargingStation, requestId: number) => Promise + stationsState: WeakMap +} + +interface QueuedSecurityEventShape { + retryCount?: number + techInfo?: string + timestamp: Date + type: string +} + +const asPlumbing = (service: OCPP20IncomingRequestService): PlumbingAccess => + service as unknown as PlumbingAccess + +const createStation = (baseNameSuffix: string): ChargingStation => { + const { station } = createMockChargingStation({ + baseName: `${TEST_CHARGING_STATION_BASE_NAME}-${baseNameSuffix}`, + connectorsCount: 1, + evseConfiguration: { evsesCount: 1 }, + stationInfo: { + ocppStrictCompliance: false, + ocppVersion: OCPPVersion.VERSION_201, + }, + websocketPingInterval: Constants.DEFAULT_WS_PING_INTERVAL_SECONDS, + }) + const openStation = station as ChargingStation & { isWebSocketConnectionOpened: () => boolean } + openStation.isWebSocketConnectionOpened = () => true + return station +} + +const setRequestHandler = ( + station: ChargingStation, + handler: (...args: unknown[]) => Promise +): void => { + ;( + station.ocppRequestService as unknown as { + requestHandler: (...args: unknown[]) => Promise + } + ).requestHandler = handler +} + +await describe('OCPP20IncomingRequestService — post-stop resurrection guard', async () => { + let service: OCPP20IncomingRequestService + let plumbing: PlumbingAccess + + beforeEach(() => { + service = new OCPP20IncomingRequestService() + plumbing = asPlumbing(service) + }) + + afterEach(() => { + standardCleanup() + }) + + await describe('sendSecurityEventNotification', async () => { + await it('should create state and enqueue on a fresh station (lifecycle-entry preserved)', () => { + const station = createStation('sec-fresh') + const requestHandlerMock = mock.fn(async () => Promise.resolve({})) + setRequestHandler(station, requestHandlerMock) + + plumbing.sendSecurityEventNotification(station, 'FirmwareUpdated', 'info') + + const state = plumbing.stationsState.get(station) + assert.notStrictEqual(state, undefined) + assert.strictEqual(state?.stopped, undefined) + }) + + await it('should silent-drop after stop() without resurrecting the entry', () => { + const station = createStation('sec-post-stop') + const state = plumbing.getOrCreateStationState(station) + const queueRef = state.securityEventQueue + + service.stop(station) + assert.strictEqual(state.stopped, true) + + plumbing.sendSecurityEventNotification(station, 'FirmwareUpdated', 'info') + + assert.strictEqual(plumbing.stationsState.get(station), state) + assert.strictEqual(state.stopped, true) + assert.strictEqual(queueRef.length, 0) + }) + + await it('should push to the queue on the pre-stop happy path (regression guard)', () => { + const station = createStation('sec-pre-stop') + // Close the WebSocket so `sendQueuedSecurityEvents` bails before + // draining and the queued event stays observable. + const closedStation = station as ChargingStation & { + isWebSocketConnectionOpened: () => boolean + } + closedStation.isWebSocketConnectionOpened = () => false + const state = plumbing.getOrCreateStationState(station) + const queueLengthBefore = state.securityEventQueue.length + + plumbing.sendSecurityEventNotification(station, 'FirmwareUpdated', 'tech-info') + + assert.strictEqual(state.stopped, undefined) + assert.strictEqual(state.securityEventQueue.length, queueLengthBefore + 1) + assert.strictEqual(state.securityEventQueue[0].type, 'FirmwareUpdated') + assert.strictEqual(state.securityEventQueue[0].techInfo, 'tech-info') + }) + }) + + await describe('sendNotifyReportRequest', async () => { + await it('should silent-drop when no state entry exists', async () => { + const station = createStation('nr-no-entry') + const requestHandlerMock = mock.fn(async () => Promise.resolve({})) + setRequestHandler(station, requestHandlerMock) + + await plumbing.sendNotifyReportRequest( + station, + { reportBase: 'FullInventory', requestId: 1 }, + { status: 'Accepted' } + ) + + assert.strictEqual(requestHandlerMock.mock.callCount(), 0) + assert.strictEqual(plumbing.stationsState.has(station), false) + }) + + await it('should silent-drop after stop() without resurrecting the entry', async () => { + const station = createStation('nr-post-stop') + const state = plumbing.getOrCreateStationState(station) + const requestHandlerMock = mock.fn(async () => Promise.resolve({})) + setRequestHandler(station, requestHandlerMock) + + service.stop(station) + assert.strictEqual(state.stopped, true) + + await plumbing.sendNotifyReportRequest( + station, + { reportBase: 'FullInventory', requestId: 42 }, + { status: 'Accepted' } + ) + + assert.strictEqual(requestHandlerMock.mock.callCount(), 0) + assert.strictEqual(plumbing.stationsState.get(station), state) + }) + }) + + await describe('sendQueuedSecurityEvents retry timer discipline', async () => { + await it('should store the retry handle on stationState after a failed send', async t => { + await withMockTimers(t, ['setTimeout'], async () => { + const station = createStation('retry-store') + const requestHandlerMock = mock.fn(async () => + Promise.reject(new Error('websocket rejected')) + ) + setRequestHandler(station, requestHandlerMock) + + plumbing.sendSecurityEventNotification(station, 'TamperDetected', 'sim') + await flushMicrotasks() + await flushMicrotasks() + + const state = plumbing.stationsState.get(station) + assert.notStrictEqual(state, undefined) + assert.notStrictEqual(state?.securityEventRetryTimer, undefined) + }) + }) + + await it('should cancel the retry timer on stop() and prevent late fire', async t => { + await withMockTimers(t, ['setTimeout'], async () => { + const station = createStation('retry-cancel-on-stop') + const requestHandlerMock = mock.fn(async () => + Promise.reject(new Error('websocket rejected')) + ) + setRequestHandler(station, requestHandlerMock) + + plumbing.sendSecurityEventNotification(station, 'TamperDetected', 'sim') + await flushMicrotasks() + await flushMicrotasks() + + const state = plumbing.stationsState.get(station) + assert.notStrictEqual(state?.securityEventRetryTimer, undefined) + + service.stop(station) + + assert.strictEqual(state?.securityEventRetryTimer, undefined) + assert.strictEqual(state?.stopped, true) + + const callCountBeforeTick = requestHandlerMock.mock.callCount() + t.mock.timers.tick(60_000) + await flushMicrotasks() + await flushMicrotasks() + + assert.strictEqual(requestHandlerMock.mock.callCount(), callCountBeforeTick) + }) + }) + + await it('should self-clear the retry handle when the retry callback fires', async t => { + await withMockTimers(t, ['setTimeout'], async () => { + const station = createStation('retry-self-clear') + let rejectCount = 0 + const requestHandlerMock = mock.fn(async () => { + rejectCount += 1 + if (rejectCount === 1) { + return Promise.reject(new Error('first attempt fails')) + } + return Promise.resolve({}) + }) + setRequestHandler(station, requestHandlerMock) + + plumbing.sendSecurityEventNotification(station, 'TamperDetected', 'sim') + await flushMicrotasks() + await flushMicrotasks() + + const state = plumbing.stationsState.get(station) + assert.notStrictEqual(state?.securityEventRetryTimer, undefined) + + t.mock.timers.tick(60_000) + await flushMicrotasks() + await flushMicrotasks() + + assert.strictEqual(state?.securityEventRetryTimer, undefined) + }) + }) + + await it('should cancel the previous retry timer when a new retry is scheduled', async t => { + await withMockTimers(t, ['setTimeout'], async () => { + const station = createStation('retry-reschedule') + const requestHandlerMock = mock.fn(async () => Promise.reject(new Error('always fails'))) + setRequestHandler(station, requestHandlerMock) + + plumbing.sendSecurityEventNotification(station, 'TamperDetected', 'first') + await flushMicrotasks() + await flushMicrotasks() + + const state = plumbing.stationsState.get(station) + const firstHandle = state?.securityEventRetryTimer + assert.notStrictEqual(firstHandle, undefined) + + t.mock.timers.tick(60_000) + await flushMicrotasks() + await flushMicrotasks() + + const secondHandle = state?.securityEventRetryTimer + assert.notStrictEqual(secondHandle, undefined) + assert.notStrictEqual(secondHandle, firstHandle) + }) + }) + + await it('should silent-drop the recursive retry call after stop() even if the handle survives race', () => { + const station = createStation('retry-recursive-drop') + const state = plumbing.getOrCreateStationState(station) + const requestHandlerMock = mock.fn(async () => Promise.resolve({})) + setRequestHandler(station, requestHandlerMock) + state.securityEventQueue.push({ + retryCount: 1, + timestamp: new Date(), + type: 'TamperDetected', + }) + + service.stop(station) + + plumbing.sendQueuedSecurityEvents(station) + + assert.strictEqual(requestHandlerMock.mock.callCount(), 0) + assert.strictEqual(state.stopped, true) + }) + }) + + await describe('getCertSigningRetryManager', async () => { + await it('should return undefined after stop() to prevent leaked retry timer', () => { + const station = createStation('cert-post-stop') + plumbing.getOrCreateStationState(station) + + service.stop(station) + + const manager = service.getCertSigningRetryManager(station) + assert.strictEqual(manager, undefined) + }) + + await it('should lazily create the retry manager on the pre-stop happy path', () => { + const station = createStation('cert-pre-stop') + + const first = service.getCertSigningRetryManager(station) + const second = service.getCertSigningRetryManager(station) + + assert.notStrictEqual(first, undefined) + assert.strictEqual(first, second) + }) + }) + + await describe('simulateFirmwareUpdateLifecycle', async () => { + await it('should silent-drop after stop() without setting activeFirmwareUpdateAbortController', async () => { + const station = createStation('firmware-post-stop') + const state = plumbing.getOrCreateStationState(station) + const requestHandlerMock = mock.fn(async () => Promise.resolve({})) + setRequestHandler(station, requestHandlerMock) + + service.stop(station) + + await plumbing.simulateFirmwareUpdateLifecycle(station, 42, { + location: 'ftp://localhost/f.bin', + retrieveDateTime: new Date(), + }) + + assert.strictEqual(state.activeFirmwareUpdateAbortController, undefined) + assert.strictEqual(state.activeFirmwareUpdateRequestId, undefined) + assert.strictEqual(requestHandlerMock.mock.callCount(), 0) + }) + }) + + await describe('simulateLogUploadLifecycle', async () => { + await it('should silent-drop after stop() without setting activeLogUploadAbortController', async () => { + const station = createStation('log-post-stop') + const state = plumbing.getOrCreateStationState(station) + const requestHandlerMock = mock.fn(async () => Promise.resolve({})) + setRequestHandler(station, requestHandlerMock) + + service.stop(station) + + await plumbing.simulateLogUploadLifecycle(station, 42) + + assert.strictEqual(state.activeLogUploadAbortController, undefined) + assert.strictEqual(state.activeLogUploadRequestId, undefined) + assert.strictEqual(requestHandlerMock.mock.callCount(), 0) + }) + }) +}) diff --git a/tests/charging-station/ocpp/OCPPIncomingRequestService-StationState.test.ts b/tests/charging-station/ocpp/OCPPIncomingRequestService-StationState.test.ts index 54a745be..48f8fdd9 100644 --- a/tests/charging-station/ocpp/OCPPIncomingRequestService-StationState.test.ts +++ b/tests/charging-station/ocpp/OCPPIncomingRequestService-StationState.test.ts @@ -1,13 +1,17 @@ /** * @file Tests for OCPPIncomingRequestService per-station state plumbing (#1963) * @description Unit tests for the shared WeakMap + lazy-init + stop() template - * in `OCPPIncomingRequestService` and for the OCPP 1.6 `resetStationState` - * contract. + * in `OCPPIncomingRequestService`, the OCPP 1.6 `resetStationState` contract, + * and the post-stop resurrection guard: `stop()` marks the entry + * `stopped: true` instead of evicting the WeakMap entry, and + * `getOrCreateStationState` returns the sealed stopped state on late + * dispatch instead of lazy-init'ing a fresh entry. * - * The OCPP 2.0.1 5-statement ordering invariant (abort firmware → abort log → - * cancel retry → resetActiveFirmwareUpdateState → resetActiveLogUploadState) - * is enforced by three independent mechanisms verified out-of-band from these - * tests: (1) JSDoc rationale on `OCPP20IncomingRequestService.resetStationState` + * The OCPP 2.0.1 6-statement ordering invariant (abort firmware → abort log → + * cancel cert signing retry → cancel security-event retry → + * resetActiveFirmwareUpdateState → resetActiveLogUploadState) is enforced by + * three independent mechanisms verified out-of-band from these tests: (1) + * JSDoc rationale on `OCPP20IncomingRequestService.resetStationState` * documenting the `?.abort()` short-circuit consequence of reordering, (2) the * `no-restricted-syntax` ESLint rule preventing direct `stationsState` * mutations from subclass code, and (3) byte-identical preservation of the @@ -23,9 +27,11 @@ import { OCPP16IncomingRequestService } from '../../../src/charging-station/ocpp import { standardCleanup } from '../../helpers/TestLifecycleHelpers.js' import { createMockChargingStation } from '../helpers/StationHelpers.js' -type OCPP16StationStateShape = Record +interface OCPP16StationStateShape { + stopped?: boolean +} -interface PlumbingAccess { +interface PlumbingAccess { createStationState: () => T getOrCreateStationState: (chargingStation: ChargingStation) => T resetStationState: (state: T) => void @@ -70,13 +76,29 @@ await describe('OCPPIncomingRequestService — per-station state plumbing', asyn assert.strictEqual(plumbing.stationsState.get(stationA), first) }) - await it('should delete the WeakMap entry after stop()', () => { - plumbing.getOrCreateStationState(stationA) + await it('should mark stopped and preserve the WeakMap entry after stop()', () => { + const state = plumbing.getOrCreateStationState(stationA) assert.strictEqual(plumbing.stationsState.has(stationA), true) + assert.strictEqual(state.stopped, undefined) service.stop(stationA) - assert.strictEqual(plumbing.stationsState.has(stationA), false) + assert.strictEqual(plumbing.stationsState.has(stationA), true) + assert.strictEqual(plumbing.stationsState.get(stationA), state) + assert.strictEqual(state.stopped, true) + }) + + await it('should return the sealed stopped state from getOrCreateStationState after stop(), without re-invoking createStationState', () => { + const first = plumbing.getOrCreateStationState(stationA) + + service.stop(stationA) + + const createSpy: ReturnType = mock.method(plumbing, 'createStationState') + const afterStop = plumbing.getOrCreateStationState(stationA) + + assert.strictEqual(afterStop, first) + assert.strictEqual(afterStop.stopped, true) + assert.strictEqual(createSpy.mock.callCount(), 0) }) await it('should invoke resetStationState exactly once with the current state on stop()', () => { @@ -105,9 +127,12 @@ await describe('OCPPIncomingRequestService — per-station state plumbing', asyn service.stop(stationA) - assert.strictEqual(plumbing.stationsState.has(stationA), false) + assert.strictEqual(plumbing.stationsState.has(stationA), true) + assert.strictEqual(plumbing.stationsState.get(stationA), stateA) + assert.strictEqual(stateA.stopped, true) assert.strictEqual(plumbing.stationsState.has(stationB), true) assert.strictEqual(plumbing.stationsState.get(stationB), stateB) + assert.strictEqual(stateB.stopped, undefined) }) await it('should not mutate state in the OCPP 1.6 default resetStationState', () => { @@ -121,7 +146,7 @@ await describe('OCPPIncomingRequestService — per-station state plumbing', asyn assert.strictEqual(plumbing.stationsState.get(stationA), state) }) - await it('should not delete the WeakMap entry when resetStationState throws, and re-invoke on subsequent stop()', () => { + await it('should not mark stopped when resetStationState throws, and re-invoke on subsequent stop()', () => { const state = plumbing.getOrCreateStationState(stationA) const failure = new Error('reset failed') let shouldThrow = true @@ -139,12 +164,15 @@ await describe('OCPPIncomingRequestService — per-station state plumbing', asyn ) assert.strictEqual(plumbing.stationsState.has(stationA), true) assert.strictEqual(plumbing.stationsState.get(stationA), state) + assert.strictEqual(state.stopped, undefined) shouldThrow = false service.stop(stationA) assert.strictEqual(resetSpy.mock.callCount(), 2) assert.strictEqual(resetSpy.mock.calls[1].arguments[0], state) - assert.strictEqual(plumbing.stationsState.has(stationA), false) + assert.strictEqual(plumbing.stationsState.has(stationA), true) + assert.strictEqual(plumbing.stationsState.get(stationA), state) + assert.strictEqual(state.stopped, true) }) })