From 040ddb32f223cafe222783d6b94d643d1ff894df Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Tue, 7 Jul 2026 02:12:51 +0200 Subject: [PATCH] =?utf8?q?refactor:=20audit=20backlog=20cleanup=20?= =?utf8?q?=E2=80=94=20JSDoc=20gaps,=20physics=20warning,=20ATG=20robustnes?= =?utf8?q?s,=20comment=20sweep=20(#1962)?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * docs: fill JSDoc gaps on public/protected abstract methods Nine public/protected abstract methods across five base classes lacked JSDoc, forcing new implementers to read existing subclasses to reconstruct the contract. Add minimal-accurate JSDoc (one-line description, @param, @returns) matching the style used by neighboring already-documented methods in the same files. Covered: - Storage.close / open / storePerformanceStatistics (contract for MongoDB / MariaDB / MySQL / SQLite / JSON storage backends) - OCPPIncomingRequestService.stop - OCPPIncomingRequestService.isIncomingRequestCommandSupported - OCPPRequestService.requestHandler (contract for OCPP 1.6 / 2.0 request services) - OCPPResponseService.isRequestCommandSupported - AbstractUIServer.sendRequest / sendResponse (contract for HTTP / WebSocket / stdio UI transports) No behavior change. Gates pass (format / typecheck / lint). * fix(physics): warn on voltageOut=400 V likely intended as line-to-line voltageOut is line-to-neutral (V_LN) throughout the simulator. V_LL is derived as sqrt(3) * V_LN in a balanced 3-phase Y system (see OCPPServiceUtils.buildVoltageMeasurandValue and CoherentMeterValueBuilder). The Voltage enum offers four closed values: 110, 230, 400, 800. Users familiar with EU 3-phase infrastructure often configure voltageOut=400 V thinking of the standard 400 V line-to-line nominal, unaware that the simulator interprets it as line-to-neutral. Downstream this yields an unrealistic simulated L-L ~= 693 V that only surfaces when a meter value is inspected. Emit a targeted logger.warn once per station init when (currentOutType === AC && numberOfPhases === 3 && voltageOut === 400) so the ambiguity surfaces at station start and the message tells the user how to reach 400 V L-L (set voltageOut=230, the closest enum L-N value). No behavior change beyond the log line. Voltage.VOLTAGE_400 remains a valid L-N configuration for the fraction of deployments that truly want a 400 V L-N (rare but not unphysical), so the warning is not an error. Gates pass (format / typecheck / lint). * fix(atg): make in-flight transaction wait AbortSignal-aware The AutomaticTransactionGenerator run loop at internalStartConnector uses two long sleeps drawn from randomInt-clamped uniform distributions: - Line 319: await sleep(wait), up to maxDelayBetweenTwoTransactions - Line 345: await sleep(waitTrxEnd), up to maxDuration (potentially hours for a realistic charging session) Neither observed the connectorsStatus.get(connectorId)?.start flag while sleeping. stopConnector() flipped start=false immediately, but the run loop only saw it AFTER the current sleep expired — so a maxDuration=3600 config could keep the loop alive for an entire hour after a stop request. Introduce a small module-scope interruptibleSleep(ms, signal) primitive that races setTimeout against an AbortSignal, cleaning up whichever listener does not win. Add a per-connector AbortController map on the class, wire stopConnector() to abort the current controller, and thread the signal through both long sleeps in internalStartConnector. The controller is recreated per invocation of internalStartConnector so a stale abort from a previous run cannot short-circuit a fresh loop. The polling helpers (waitChargingStationAvailable / waitConnectorAvailable / waitRunningTransactionStopped) keep their DEFAULT_ATG_WAIT_TIME_MS sleeps — they iterate on much shorter timescales and are not the audit target. Gates pass (format / typecheck / lint). * fix(atg): validate min/max delay and duration bounds before starting the run loop The AutomaticTransactionGenerator run loop draws its between-transaction wait and in-transaction duration from randomInt(min, max + 1) of node:crypto. That primitive throws RangeError when min >= max, so a mis-configured template with minDelay > maxDelay or minDuration > maxDuration kills the async loop at the first iteration with an unhandled rejection that surfaces only as a generic 'Error while starting connector' log line — the actual mis-config value is lost. Add a private validateConfiguration() that checks both min/max invariants up front and, on violation, logs a targeted error naming the offending pair and refuses to schedule internalStartConnector. Called from startConnector() before the internalStartConnector spawn so external callers of startConnector() also benefit; start() reaches startConnectors() → startConnector() and inherits the guard. Config absence is not a violation (returns true); the run loop already defaults to 0 for missing min/max fields and no randomInt call fires. No behavior change for a valid configuration. Gates pass (format / typecheck / lint). * refactor(charging-station): remove 24 redundant restatement comments The audit's M4-M6 finding covered ~63 imperative/redundant/narrative comments across the codebase, roughly a quarter of which sat in ChargingStation.ts as one-line WHAT-restatements directly above a self-documenting method call: // Start heartbeat this.startHeartbeat() or above a specifically-named event handler: // Handle WebSocket close this.wsConnection.on('close', this.onClose.bind(this)) Delete such restatements when the next line already conveys the same information via method / event / status name. Comments that carry a non-obvious WHY, a spec reference, a race condition note, or a subtle behavioural invariant are preserved unchanged (e.g. the response- handling deferred-promise reject-is-no-op explanation at line 2472). 24 comment lines removed; no code change beyond comment deletion. Gates pass (format / typecheck / lint). * refactor(ocpp): remove 10 redundant restatement comments in request/incoming request services Continue the M4-M6 comment sweep in the OCPP request/incoming-request dispatch layer: OCPPRequestService.ts (8 deletions) - // Send error message (above sendError → internalSendMessage) - // Send response message (above sendResponse → internalSendMessage) - // Build request (above JSON.stringify of OutgoingRequest) - // Build response (above JSON.stringify of Response) - // Send a message through wsConnection (above the WebSocket Promise) - // Handle the request's response (above responseHandler call) - // Remove request from the cache (above requests.delete) - // Check if wsConnection opened (above isWebSocketConnectionOpened()) OCPPIncomingRequestService.ts (2 deletions) - // Log (above logger.error) - // Send the built response (above sendResponse call) The '// Build Error Message per OCPP-J §4.2.3: [4, messageId, errorCode, errorDescription, errorDetails]' comment carries a spec reference and the tuple shape, and stays. The '// Emit command name event to allow delayed handling only if there are listeners' comment carries the listener-count WHY and stays. No code change. Gates pass (format / typecheck / lint). * refactor(auth): remove 14 redundant restatement comments in OCPPAuthServiceImpl Continue the M4-M6 comment sweep in the auth service. Delete pure WHAT-restatement comments that immediately precede a self-descriptive line (method call, property assignment, or self-explaining condition): - // Initialize metrics tracking / Initialize default configuration (above obvious property assignments in the constructor) - // Update request metrics / Update failure metrics (above metrics counter increments) - // Update metrics based on result (above updateMetricsForResult call) - // Create a minimal request to check applicability (above testRequest object literal) - // Check if adapter reports remote availability (above adapter.isRemoteAvailable() call) - // Merge new config with existing / Validate merged configuration / Apply validated configuration (updateConfiguration WHAT-narration) - // Check for specific error patterns that indicate critical issues (above criticalPatterns array literal) - // Track successful vs failed authentication / Track strategy usage / Track cache hits/misses based on method (updateMetricsForResult WHAT-narration) Preserved for their WHY / non-obvious context: - // Note: Adapter and strategies will be initialized async via initialize() (async lifecycle note) - // Try each strategy in priority order (section marker in a 100+ line method) - // Continue to next strategy unless it's a critical error (has WHY) - // Get rate limiting stats from cache via remote strategy (describes routing indirection) - // Try local strategy first for quick cache/list lookup (fast-path rationale) - // Create a type guard to check if strategy has configure method / Use type guard instead of any cast (non-obvious TS pattern) No code change. Gates pass (format / typecheck / lint). * refactor(charging-station): remove 6 residual restatement comments in ATG and HelpersChargingProfile Final M4-M6 comment sweep pass on the two remaining files with clear pure-restatement comments: AutomaticTransactionGenerator.ts (2 deletions) - // Start transaction (above await this.startTransaction(connectorId)) - // Wait until end of transaction (above const waitTrxEnd = secondsToMilliseconds(randomInt(minDur, maxDur+1)) — the waitTrxEnd variable name is the comment) HelpersChargingProfile.ts (4 deletions) - // Check if the charging profile is active (above isWithinInterval(currentDate, {...}) condition) - // Check if the first schedule period startPeriod property is equal to 0 (above the equality check on chargingSchedulePeriod[0].startPeriod) - // Handle only one schedule period (above chargingSchedule.chargingSchedulePeriod.length === 1 branch) - // Handle the last schedule period within the charging profile duration (above the composite last-period + duration-overflow condition) Preserved WHY-comments in these two files: the § references, physics / priority-order rationale, race-condition notes, and the ATG interruptibleSleep + AbortController design comments introduced by fa2bea32 / 5a066a51. No code change. Gates pass (format / typecheck / lint). * docs(atg): document onAbort cleanup semantics in interruptibleSleep The empty JSDoc bloc above the onAbort function inside interruptibleSleep was an auto-generated placeholder from the jsdoc/require-jsdoc lint rule firing on function declarations. It read like a TODO left in place. Replace with a targeted comment covering the two non-obvious invariants this cleanup function relies on: - clearTimeout(timeout) prevents the setTimeout callback from firing after the promise has already resolved on the abort path - addEventListener({ once: true }) auto-removes the abort listener so no explicit removeEventListener is needed on the abort path No behavior change. Gates pass (format / typecheck / lint). * fix(physics): broaden voltageOut warning to Voltage.VOLTAGE_800 and harmonize with CoherentSession Round-1 review of this PR surfaced a harmonization violation: my M13 warning at ChargingStation.getStationInfoFromTemplate covered only Voltage.VOLTAGE_400 and gated on numberOfPhases===3, while the pre-existing per-session warning at CoherentSession.createCoherentSession covered Voltage.VOLTAGE_400 or Voltage.VOLTAGE_800 in AC without a phase gate. Broaden the station-init warning: - Add Voltage.VOLTAGE_800 (common L-L nominal in DC HPC / industrial systems; users configuring it as L-N in AC produce sqrt(3) * 800 ~= 1386 V simulated L-L). - Drop the numberOfPhases === 3 gate so single-phase AC configurations with 400 V / 800 V voltageOut also surface at station init (matches CoherentSession coverage). - Message shows the actual derived L-L (sqrt(3) * voltageOut) so users see the physical implausibility of both enum values. - Suggestion 'set voltageOut=230' is emitted only for the 400 case; the 800 case has no clean L-N enum alternative (462 V is not in the Voltage enum), so the message stops at the diagnostic. The two warning sites remain complementary: - ChargingStation station-init: fires once per station lifetime, regardless of coherentMeterValues flag. - CoherentSession per-session: fires per transaction, only when coherentMeterValues=true. Users with coherentMeterValues=false now get consistent enum coverage without depending on the session-level flag. Gates pass (format / typecheck / lint). * refactor(atg): memoize validateConfiguration result to dedupe per-connector log noise Round-1 review noted that validateConfiguration() fires N times for a station with N connectors (once per startConnector call inside the startConnectors loop), so an invalid ATG configuration produces N identical error log lines instead of one. Add a private configurationValidationResult field cached on first invocation and reset in stop() so a station with N connectors emits the diagnostic log line at most once per session, while still allowing a subsequent start() with a mutated configuration to re-validate from scratch. Cache lifecycle: - Initialized to undefined in the constructor. - Set to true or false on the first validateConfiguration() call after a start() (or after a fresh instance). - Cleared back to undefined at the end of stop() so the next start() observes any config changes and re-emits the log line if invalid. No behavior change for the valid-configuration path. For the invalid path, log-line count drops from N (per-connector) to 1 (per-session). Gates pass (format / typecheck / lint). * refactor(utils): extract isRandomIntBoundsValid predicate and apply in ATG Round-1 review noted that the min > max guard in AutomaticTransactionGenerator.validateConfiguration is a specific instance of a general concern: any caller of randomInt(min, max + 1) from node:crypto risks a RangeError when configuration-driven min/max values are mis-ordered. Extract a pure predicate isRandomIntBoundsValid(min, max) into src/utils/Utils.ts (co-located with other random primitives: secureRandom, getRandomFloat, getRandomFloatFluctuatedRounded, getRandomFloatRounded). The predicate carries the +1 semantics contract in its JSDoc so consumers cannot misuse it. Zero logger dependency, zero side effects, testable in isolation. Apply in ATG's validateConfiguration where the same check now goes through the named predicate for readability and reuse. The two log lines remain in ATG with their full field-name context (which the predicate intentionally does not embed to stay pure). Other randomInt(min, max + 1) sites in the codebase remain unchanged, each documented as low-risk: - OCPP16IncomingRequestService.updateFirmwareSimulation lines 1860, 1871, 1908, 1925, 1936, 1947: min/max are function-parameter defaults (maxDelay=30, minDelay=15, hardcoded and safe by construction); the internal callers respect the default ordering. - OCPPServiceUtils.buildSocMeasurandValue line 270: socMaximumValue is the Constants.SOC_MAXIMUM_PERCENT (100), socMinimumValue is the Zod- schema-validated template field (bounded by the schema). The Zod schema guarantees the invariant at load time. These sites can adopt the predicate in a follow-up config-validation sweep PR if the constraints ever loosen. No behavior change in this commit. Gates pass (format / typecheck / lint). * fix(atg): guarantee AbortController cleanup and prevent per-connector race Round-3 review (subagents #1 design/impl + #2 algo) surfaced two concurrency bugs in internalStartConnector's per-connector controller lifecycle: 1. Leak on abort/exception path: the delete at line 412 was only reached when the while-loop exited normally. Any await inside the loop that rejected (waitChargingStationAvailable, startTransaction, stopTransaction, etc.) propagated through the caller's .catch handler in startConnector — bypassing the delete and leaving the controller in connectorAbortControllers indefinitely. 2. Race in stopConnector→startConnector→cleanup ordering: if the old loop was still draining (e.g. resolving an aborted interruptibleSleep) when a new startConnector fired, the new run set(controller_new) at line 349, and shortly after the old loop reached the unconditional delete(connectorId), wiping the successor controller. A subsequent stopConnector could then not abort the new loop. Fix both with a single try/finally + conditional delete pattern: - try { while-loop } finally { conditional delete } guarantees cleanup on every exit path (normal, break, throw) without leaking. - Delete only when this.connectorAbortControllers.get(connectorId) === abortController (identity check on the run-local reference) so a successor controller cannot be wiped by a predecessor's finally. No behavior change on the happy path. Gates pass (format / typecheck / lint). * fix(atg): unconditionally reset validation cache in stop() to enable recovery Round-3 review (subagents #1 + #2) found a stuck-state bug: The cache reset was at the END of stop(), inside the branch guarded by 'if (!this.started) { return }'. So if a caller invoked startConnector() directly (without going through start()), this.started never became true, and any subsequent stop() early-returned without resetting the cache — the user was stuck with a cached-false configurationValidationResult and no path to recover. Move the reset to the TOP of stop(), before the started/stopping guards. The reset is idempotent (undefined -> undefined is a no-op), so unconditional execution has no side effect on the already-stopped path. This lets external startConnector() callers recover by calling stop() to clear the cache. Gates pass (format / typecheck / lint). * refactor(utils): rename to isValidRandomIntBounds and strengthen input validation Round-3 review (subagents #1 + #2 + harmonization #6) found three issues with the extracted predicate: 1. Naming inversion — neighbors follow subject-first (isValidDate, isNotEmptyString, isNotEmptyArray) but isRandomIntBoundsValid put the operation before the subject. Rename to isValidRandomIntBounds. 2. Under-validation — the predicate only checked min <= max, so it returned true for NaN/Infinity/non-integers/negatives even though node:crypto.randomInt throws RangeError on all of them (safe integer 0 <= min < 2^48 <= max required). NaN was accidentally rejected because 'NaN <= NaN' is false. Strengthen to reject explicitly: Number.isSafeInteger(minValue) && Number.isSafeInteger(maxValue) && minValue >= 0 && minValue <= maxValue. 3. JSDoc leaked ATG-specific '+1' convention into a general utility contract. Trim @param maxValue to just 'Upper bound (inclusive).' The +1 semantics belong in the description (kept there) and at the call site. Log messages in ATG updated to match: - randomInt(min, max) -> randomInt(min, max + 1) to reflect the actual call - Semicolon comma joining consequence clauses replaced with em-dash for readability Gates pass (format / typecheck / lint). * refactor(utils): export interruptibleSleep and apply to ATG polling helpers Round-3 review (subagent #4 harmonization) found interruptibleSleep was private to ATG and could not be reused, plus the three ATG polling helpers (waitChargingStationAvailable / waitConnectorAvailable / waitRunningTransactionStopped) called plain sleep() and were blind to the abort signal. Changes: 1. Move interruptibleSleep from AutomaticTransactionGenerator.ts to src/utils/Utils.ts next to sleep(). Export from utils/index.ts. 2. Reorder to declare 'timeout' before the 'onAbort' arrow function so there is no forward reference (subagent #1 finding). 3. Expand JSDoc: describe both cleanup paths symmetrically (timer-path explicit removeEventListener, abort-path clearTimeout + { once: true } auto-removal). Also document that the promise resolves (does not reject) on abort so callers can drop try/catch noise. 4. Add signal: AbortSignal parameter to the three ATG wait helpers. 5. Replace sleep(DEFAULT_ATG_WAIT_TIME_MS) with interruptibleSleep(DEFAULT_ATG_WAIT_TIME_MS, signal) in each helper. 6. Include !signal.aborted in each while-loop condition so the poll exits immediately when the connector is stopped mid-wait. 7. Thread abortController.signal from internalStartConnector into all three helper calls. 8. Add 'interruptible' to cspell.config.yaml. Impact: a stopConnector() call during an availability-wait now unblocks the poll immediately rather than after DEFAULT_ATG_WAIT_TIME_MS. Gates pass (format / typecheck / lint). * fix(physics): scope L-L derivation to 3-phase and harmonize voltageOut warning Round-3 review (subagents #1 impl elegance + #2 physics + #4 harmonization + #5 content) found the M13 warning had three related issues: 1. Message inaccuracy for single-phase AC: the phrase '3-phase Y-derived line-to-line = X V' fired for single-phase AC stations too (F1 had intentionally dropped the numberOfPhases===3 gate for enum coverage symmetry with CoherentSession). But sqrt(3) only applies to balanced 3-phase Y — for single-phase there is no L-L, so the number cited was physically meaningless. 2. Missing VOLTAGE_800 suggestion: only the VOLTAGE_400 case emitted a remediation hint. VOLTAGE_800 warned but did not tell the operator what to do next; 800 V L-L has no clean L-N enum equivalent, so the suggestion should say so. 3. Terminology drift vs CoherentSession: this site said 'line-to-neutral' while CoherentSession said 'line-to-neutral (phase voltage)'; missing space before V in CoherentSession's log message. Fix: - Gate the 3-phase-Y clause behind getNumberOfPhases===3 (conditional string, still fires for all AC phase counts on enum match). For 1-phase and DC, the derived-L-L clause is elided. - Emit a VOLTAGE_800 remediation: 'no standard L-N enum value exists; verify template voltageOut intent (~462 V L-N)'. - Adopt 'line-to-neutral (phase voltage)' terminology matching CoherentSession. Add space before V in CoherentSession log message. - Trim the 10-line explanatory comment block to 6 lines: keep the invariant (voltageOut is L-N; VOLTAGE_400/800 commonly mis-configured); drop the cross-site harmonization prose (belongs in commit history). Gates pass (format / typecheck / lint). * docs(charging-profile): restore structural 'last period' navigation comment Round-3 review (subagent #1 design/impl) found the M4-M6 comment sweep had over-deleted one non-restatement comment at HelpersChargingProfile line ~404: '// Handle the last schedule period within the charging profile duration'. The composite condition below (last-in-array OR next-period-would-exceed-duration) is not self-evident from the boolean expression alone; the comment aids navigation through this OCPP-specific boundary case. Restore with a tighter version: 'Last period: last in array OR next period would exceed the charging profile duration.' No code change. Gates pass (format / typecheck / lint). * docs(ocpp): use backtick-quoted `true` in @returns for consistency Round-3 review (subagent #5 content finding 3) found @returns lines in the M11 JSDocs used capitalized 'True when' while neighboring JSDocs used backtick-quoted `true`. Normalize both: - OCPPIncomingRequestService.isIncomingRequestCommandSupported - OCPPResponseService.isRequestCommandSupported No behavior change. Gates pass (format / typecheck / lint). * docs: remove/trim redundant comments per DRY (AGENTS.md no-duplication rule) User feedback: comments that repeat info already conveyed by log messages, method names, or field names violate concision + AGENTS.md 'No duplication: maintain single authoritative documentation source; reference other sources rather than copying'. Changes: 1. ChargingStation.ts:1676-1681 - DELETE the 6-line voltage warning block comment. Every fact it stated (voltageOut is L-N, 400/800 are L-L nominals) is already in the log message immediately below ('matches a line-to-line nominal value', 'treats voltageOut as line-to-neutral (phase voltage)'). The 'harmonized with CoherentSession' cross-reference is trivially discoverable via 'rg Voltage.VOLTAGE_400'. 2. AutomaticTransactionGenerator.ts stop() - trim 4-line 'Reset unconditionally' comment to 3 lines. Drop the redundant 'configurationValidationResult' field name (visible on next line) and 'this.started never transitioned to true' (obvious from the guard structure). 3. AutomaticTransactionGenerator.ts stopConnector() - trim 5-line 'Wake' comment to 3 lines. Drop the redundant second sentence about controller lifecycle (visible from the field name 'connectorAbortControllers' and its usage in internalStartConnector). 4. AutomaticTransactionGenerator.ts internalStartConnector() - trim 4-line 'Fresh AbortController' comment to 3 lines. The 'new AbortController()' on the next line already conveys freshness; only the abort-before-replace ordering (unblock lingering sleep) needs documenting. 5. AutomaticTransactionGenerator.ts try/finally - trim 5-line 'Only clear' comment to 2 lines. The try/finally structure self-documents the 'guarantees cleanup on all paths' claim; only the identity check for concurrent stopConnector→startConnector needs explanation. No behavior change. Gates pass (format / typecheck / lint). * fix(atg): close two ATG concurrency correctness gaps Round-4 review (subagents #1 design/impl HIGH, #2 algo HIGH): 1. **Concurrent-loop race on back-to-back stopConnector→startConnector** (subagent #2 finding 3): after stopConnector aborts the controller, the old internalStartConnector loop resumes from its interruptible sleep. When startConnector fires immediately after and resets connectorStatus.start=true, the old loop's while condition (which only reads the shared 'start' flag) sees true again and continues running with an already-aborted signal — subsequent interruptibleSleep calls resolve synchronously → tight-loop, or worse, the old loop calls stopConnector again and aborts the NEW controller. Fix: add !abortController.signal.aborted to the while condition. The old loop now exits immediately after abort regardless of the shared 'start' flag. 2. **waitRunningTransactionStopped stale connectorStatus snapshot** (subagent #1 finding 7): the helper fetched connectorStatus once before the loop and re-read the same object on each iteration. This works today because getConnectorStatus returns a Map reference that mutates in place, but the pattern was inconsistent with waitChargingStationAvailable / waitConnectorAvailable which call their predicates inline, and would silently break if getConnectorStatus ever returned a new object per call. Fix: inline the getConnectorStatus call in the while condition and inline it again for the one-time log message. Matches the pattern of the two sibling wait helpers. Gates pass (format / typecheck / lint). * fix(atg): move post-loop bookkeeping inside finally for exception paths Round-4 review (subagent #1 design/impl MEDIUM finding 3): the connectorStatus.stoppedDate assignment and the two 'Stopped on connector' logger calls at the end of internalStartConnector sat OUTSIDE the try/finally block. If any await inside the while loop rejected (waitChargingStationAvailable, startTransaction, stopTransaction, etc.), the finally block ran the controller cleanup — but the throw propagated past the post-loop block, leaving connectorStatus.stoppedDate undefined and skipping the 'Stopped on connector' + 'Stopped with connector status' log lines. The connector observably appeared to be still running for a stopped station. Move the four post-loop statements inside the finally block so the connector's stoppedDate is always set and the operator always sees the 'Stopped' log lines, regardless of exit path (normal exit, break, throw). The ChargingStationEvents.updated emit also relocates so the UI reflects the stopped state on every path. Gates pass (format / typecheck / lint). * docs: harmonize voltage warnings and @returns JSDoc across OCPP layer Round-4 review (subagents #3 OCPP+TS, #4 harmonization, #5 content) surfaced three harmonization drifts in JSDoc/log content: 1. ChargingStation.ts:1697 voltage warning said 'voltageOut=X V with AC output matches…' — 'with AC output' is redundant (the warning already gates on currentOutType === AC) and not spec terminology. Drop the qualifier. 2. CoherentSession.ts:103 voltage warning framed the diagnostic differently than ChargingStation.ts ("is treated as line-to-neutral by ACElectricUtils" vs "matches a line-to-line nominal value"). Also attributed the interpretation to ACElectricUtils, an implementation detail that leaked into a user-facing log. Align framing to match ChargingStation.ts and drop the ACElectricUtils attribution. 3. AutomaticTransactionGenerator.ts:302-304 abort-before-replace comment ended with 'before installing the new controller' — the 'new AbortController()' + 'connectorAbortControllers.set()' calls on the immediately following two lines restate that fact. Trim. 4. Four OCPP validate*Payload JSDocs used "@returns True if payload validation succeeds, false otherwise" — capitalized bare 'True'/'false' without backticks and no trailing period. Established pattern elsewhere in the same files uses "\ when … \ otherwise." Normalize in OCPPIncomingRequestService, OCPPRequestService, OCPPResponseService, and OCPPServiceUtils. Gates pass (format / typecheck / lint). * docs: polish batch from round-4 review Round-4 subagents flagged five small content-quality items, none blocking: 1. Utils.ts interruptibleSleep: delete two inline comment blocks that duplicate the JSDoc (timer-path cleanup and abort-path cleanup are already documented symmetrically in the JSDoc body). 2. Utils.ts isValidRandomIntBounds JSDoc: drop the last sentence that leaked call-site usage guidance into a general-utility predicate contract. Document the >= 0 constraint in @param. 3. AutomaticTransactionGenerator.ts validateConfiguration JSDoc: trim the memoization-mechanism sentence to a single clause. The full rationale already lives in the stop() reset comment. 4. AutomaticTransactionGenerator.ts validateConfiguration log messages: the > description understated the predicate rejection surface (it also rejects NaN/Infinity/non-integer/negative). Rewrite as invalid bounds matching the predicate contract. 5. Storage.ts three JSDoc @returns: backtick-quote void to match the codebase convention of quoting type names. No behavior change. Gates pass (format / typecheck / lint). * fix(ocpp/1.6): guard updateFirmwareSimulation against invalid delay bounds Round-4 review (subagent #4 harmonization finding 3): six sites in updateFirmwareSimulation call randomInt(minDelay, maxDelay + 1) using function-parameter bounds. Current callers (constructor lines 586 and 596) pass no arguments so the safe defaults (maxDelay=30, minDelay=15) apply. But the parameters are exposed, and any future caller passing config-driven values could hit a randomInt RangeError inside the async firmware simulation loop that surfaces only as a generic caught error. Add an early isValidRandomIntBounds guard immediately after the existing checkChargingStationState guard. On invalid bounds, log an error naming the offending pair and return without scheduling any simulation. Defensive-only — no behavior change for the current default-only callers. Gates pass (format / typecheck / lint). * fix(ocpp/2.0): thread abort signal through simulateFirmwareUpdateLifecycle sleeps Round-4 review (subagent #4 harmonization finding 1): the OCPP 2.0 firmware simulation lifecycle owns an AbortController (activeFirmwareUpdateAbortController set at line 3703) so a concurrent UpdateFirmware request can cancel an in-progress simulation (line 3091 aborts the previous controller). However, the eight await sleep(...) calls inside simulateFirmwareUpdateLifecycle did not observe that signal. The pattern was: await sleep(delayMs) if (checkAborted()) return which correctly exits after abort but only ONCE the sleep expires. A long sleep (retrieveDateTime scheduling, InstallDateTime scheduling) could keep the simulation running for hours after the abort. Replace each of the eight sleeps inside simulateFirmwareUpdateLifecycle with interruptibleSleep(delay, abortController.signal). The abort now wakes the sleep immediately; the subsequent checkAborted() guard observes the aborted state and returns. Callback semantics identical on the non-abort path. The ninth sleep at line 3902 (simulateLogUploadLifecycle) is left unchanged - that function does not own an AbortController and log uploads are not user-cancellable in this simulator. Gates pass (format / typecheck / lint). * fix(utils): enforce node:crypto randomInt 2^48 range limit in predicate Round-5 review (subagent OCPP+TS finding 8, HIGH): Number.isSafeInteger allows values up to 2^53 - 1, but node:crypto.randomInt rejects any call where max - min > 2^48 - 1 with a RangeError. The predicate's documented contract ('safe for randomInt') was therefore incorrect for wide ranges — the false-positive latent because current callers use small integer delays (15-30s) and SoC bounds (0-100). Add the 2^48 range check to the predicate and document it in the JSDoc. Empirically verified against Node 22: randomInt(0, 2**48) throws RangeError Number.isSafeInteger(2**48) === true Gates pass (format / typecheck / lint). * fix(ocpp): guard SOC randomInt against invalid template bounds The SoC MeterValues fallback path in buildSocMeasurandValue drew a random integer via randomInt(socMinimumValue, socMaximumValue + 1) where socMinimumValue comes from the (user-supplied) template minimumValue field and socMaximumValue is Constants.SOC_MAXIMUM_PERCENT (=100). If a template set minimumValue >= 101, randomInt would throw RangeError inside a MeterValues emission — surfacing only as a caught error in the OCPP path. Guard with isValidRandomIntBounds before the randomInt call. On invalid bounds, log a warn line naming the offending pair and fall back to socMaximumValue (100) so the measurand still produces a value. Gates pass (format / typecheck / lint). * fix(atg): gate all post-loop bookkeeping on controller ownership Round-5 review (design/impl finding 3, MEDIUM): the round-4 finally block gated only the connectorAbortControllers.delete() on the identity check (map.get === abortController). It then wrote connectorStatus.stoppedDate unconditionally, emitted the Stopped log lines, and fired the updated event. On a back-to-back stopConnector -> startConnector, the sequence is: t0: old loop finally fires t1: identity check: map.get === NEW controller (the successor already installed by startConnector); check fails, delete correctly skipped t2: connectorStatus.stoppedDate = new Date() <-- corrupts the new run's status object (shared reference in connectorsStatus) t3: 'Stopped on connector' log fires for a connector the operator just re-started Gate all four post-loop operations (delete, stoppedDate, info log, debug log, updated event) on a single isOwner boolean. Non-owner paths silently exit — the successor run owns the status object and will emit its own Stopped log when it eventually completes. Gates pass (format / typecheck / lint). * fix(ocpp/2.0): guarantee clearActiveFirmwareUpdate on every exit path Round-5 review (harmonization finding C-2, MEDIUM): simulateFirmwareUpdateLifecycle called clearActiveFirmwareUpdate only on three specific paths (empty location, signature failure, happy completion). Any other early-return path (7 abort returns) or a thrown await left activeFirmwareUpdateAbortController set. A subsequent UpdateFirmware.req would then abort() a non-existent in-progress update, and stationState.activeFirmwareUpdateRequestId would never re-populate — the station appeared stuck with a phantom in-progress firmware update. Wrap the entire body in try/finally with a single clearActiveFirmwareUpdate call in finally. Delete the three now-redundant site-local calls. Also correct the abort-controller store comment from 'cancel' to 'abort' (codebase terminology finding B1/D4 from the round-5 content review). Gates pass (format / typecheck / lint). * docs: polish batch from round-5 review Four small content-quality items surfaced by round-5 review: 1. AutomaticTransactionGenerator.waitRunningTransactionStopped: the first-iteration log line called getConnectorStatus a second time (separate from the loop condition) to read transactionId. Between the two reads the transaction could theoretically end and a new one start, logging a different id than the one that triggered the wait. Snapshot transactionId into a local const before the log. 2. AutomaticTransactionGenerator.validateConfiguration error messages and OCPP16IncomingRequestService.updateFirmwareSimulation abort message both suffixed 'for randomInt(min, max + 1)' — misleading because isValidRandomIntBounds validates raw bounds, not the +1 form. Drop the suffix; the 'invalid bounds' clause is sufficient. 3. CoherentSession voltage warning lacked the actionable remediation hint present in the ChargingStation counterpart. Append 'Set voltageOut to the L-N equivalent in the station template to resolve.' No behavior change. Gates pass (format / typecheck / lint). * fix(utils): tighten isValidRandomIntBounds boundary by one The predicate validated maxValue - minValue < 2^48, but every call site passes randomInt(min, maxValue + 1). node:crypto's randomInt requires max_exclusive - min <= 2^48 - 1, so the effective constraint on the inclusive bounds is maxValue - minValue <= 2^48 - 2. The old boundary permitted maxValue - minValue = 2^48 - 1, which after the +1 offset would throw RangeError at the randomInt call. Practical impact is nil (all real config paths are far below the limit) but the predicate is now nominally correct. Also trim JSDoc overexplanation of caller convention. * fix(ocpp): skip SoC measurand emission on invalid bounds buildSocMeasurandValue previously fabricated a synthetic 100 % SoC reading when the template's minimumValue was misconfigured (e.g. a negative float that fails isValidRandomIntBounds). Emitting 100 % looks like a valid meter reading to the CSMS and could distort billing, session-end triggers, and analytics on the operator side. Skip the measurand entirely instead — consistent with the existing null-return path when the SoC template itself is absent (line 261). The warn log still surfaces the misconfiguration. * refactor(ocpp20): extract resetActiveFirmwareUpdateState helper handleRequestUpdateFirmware's abort-and-replace path directly wrote undefined to the two activeFirmwareUpdate* fields, while the lifecycle finally routed through clearActiveFirmwareUpdate. Any future logic added to the helper (logging, metrics, invariants) would silently diverge for the abort-and-replace path. Extract resetActiveFirmwareUpdateState as the single source of truth for the field reset. clearActiveFirmwareUpdate still guards on requestId identity (protecting a successor's state from being cleared by a stale finally); handleRequestUpdateFirmware calls the reset unconditionally because the abort-and-replace semantics require it. * feat(atg): log displaced-loop exit at debug level The isOwner gate silently returned from the finally block when a concurrent stopConnector→startConnector had already installed a successor controller. Diagnosing 'why did the old loop stop?' under concurrency required knowing the silent path existed. Emit a debug line on the non-owner path so displaced-loop exits are observable without adding noise at higher log levels. Also trim the redundant second half of the ownership comment (the consequence is already implied by the WHY sentence above). * docs(coherent): align voltage warning voice with ChargingStation CoherentSession's L-L/L-N mismatch warning used passive voice ('If this value is meant as line-to-line') while the parallel warning in ChargingStation.ts uses direct second-person ('If you intended'). Align to the second-person form for cross-file consistency. * docs(utils): align isValidRandomIntBounds JSDoc notation The description body stated 'maxValue - minValue <= 2^48 - 2' while @param stated 'maxValue - minValue < 2^48 - 1'. Both are equivalent for integers but forced a reader to verify equivalence. Also drop the derivation showing how the node:crypto max_exclusive - min <= 2^48 - 1 constraint reduces to maxValue - minValue <= 2^48 - 2 — the derivation belongs in commit history, not in the live docblock. @param and description now use the same notation (<= 2^48 - 2). * docs(logs): align tense and specificity in round-6 messages Two log-message polish items surfaced by round-7: ATG.internalStartConnector displaced-loop debug log used present participle 'Exiting' while the sibling info log at line 391 uses past tense 'Stopped'. Both fire from the finally block reporting a completed event. Align to 'Exited'. OCPPServiceUtils.buildSocMeasurandValue skip-log action clause read 'skipping measurand'. Sibling messages in ATG and OCPP16 use fully qualified action noun phrases ('aborting connector startup', 'aborting firmware update simulation'). Add the 'SoC' qualifier for parallelism: 'skipping SoC measurand'. * refactor(ocpp20): harmonize firmware-state cleanup + log superseded Three coherence gaps identified by round-7 harmonization review: stop() aborted the active firmware update but wrote the delete without routing through resetActiveFirmwareUpdateState. Latent-only because the stationsState.delete makes the fields unreachable, but the pattern diverged from every other cleanup site. Route through the helper. clearActiveFirmwareUpdate silently returned when the requestId did not match the active one — the exact 'displaced' pattern the ATG round-6 fix addressed. A stale finally firing after a successor took over is diagnostically important. Emit a debug line for the superseded path. Naming distinction (reset = unconditional, clear = identity-guarded) is now self-documenting via the guard in clearActiveFirmwareUpdate. * fix(ocpp20): prevent state resurrection in clearActiveFirmwareUpdate getStationState is a lazy-init getter: on cache miss it creates an empty entry and adds it to stationsState. clearActiveFirmwareUpdate runs in the finally of simulateFirmwareUpdateLifecycle. If stop() has already deleted the station entry before the aborted lifecycle's finally fires, the lazy-init would resurrect a fresh empty entry — leaking state past shutdown. Use stationsState.get() directly and no-op silently when the entry is absent. The non-owner debug log now only fires for genuine supersession (state present, requestId mismatched). * docs: revert isValidRandomIntBounds JSDoc to match code + fix typo Round-7 aligned the JSDoc @param to '<= 2^48 - 2' while the code kept '< 2 ** 48 - 1'. Both forms are equivalent for integers but the JSDoc notation deviated from the code's literal. Revert JSDoc to '< 2^48 - 1' which mirrors the code and stays close to node:crypto's own docs phrasing ('range must be less than 2^48'). Also fix a pre-existing typo in the Constants.MAX_RANDOM_INTEGER comment: 'randomInit()' -> 'randomInt()'. * fix(ocpp20): use raw stationsState.get() in FirmwareStatusNotification trigger The TriggerMessage/FirmwareStatusNotification case at line 564 is a reporting path — it reads activeFirmwareUpdateRequestId to build the notification payload but does not otherwise mutate station state. Calling getStationState (which lazy-inits on cache miss) here means a TriggerMessage arriving for a station that has never had a firmware update creates a spurious empty state entry, and a TriggerMessage arriving between stop() and WS close would resurrect state after shutdown — same pattern as the round-8 clearActiveFirmwareUpdate fix. Use stationsState.get() directly and optional-chain the requestId read. When no state exists the notification carries requestId undefined, which is spec-compliant for status = Idle (OCPP 2.0.1 L01.FR.20: requestId is mandatory only for non-Idle statuses). * docs(utils): drop @param maxValue range-width restatement The @param maxValue clause restated 'with maxValue - minValue < 2^48 - 1', which is already stated in the description body. Trim to just the intrinsic bound (inclusive safe integer >= minValue) and let the range-width constraint live where it belongs (the body). * refactor(ocpp20): rename getStationState to getOrCreateStationState The private getter lazily creates a station state entry on cache miss. Callers seeing 'getStationState(cs)' vs 'stationsState.get(cs)' side by side could not tell that the former creates state while the latter does not — the very split rounds 8 and 9 introduced to fix state-resurrection bugs (clearActiveFirmwareUpdate, TriggerMessage/ FirmwareStatusNotification). Rename the lazy-init getter so its name carries the semantics: this.getOrCreateStationState(cs) // creates on miss - handler paths this.stationsState.get(cs) // undefined on miss - cleanup/reporting Nine call sites + one definition. Zero behavior change. * docs(utils): make @param minValue structurally symmetric with @param maxValue Round-9 trimmed @param maxValue to 'safe integer >= minValue' but left @param minValue as 'non-negative safe integer'. Both bounds now state 'safe integer >= X' — @param minValue relative to 0, @param maxValue relative to minValue. The description body still carries the rejection list (NaN, Infinity, floats, negatives, range-width). * fix(ocpp20): cross-check firmware status against active requestId in trigger FirmwareStatusNotification TriggerMessage read from two independent sources: hasFirmwareUpdateInProgress reads chargingStation.stationInfo .firmwareStatus, activeFirmwareUpdateRequestId lives on stationsState. They can diverge in real windows: 1. simulateFirmwareUpdateLifecycle sets activeFirmwareUpdateRequestId at line 3716 before its first sendFirmwareStatusNotification (line 3467). During any await between the two — notably the retrieveTime sleep at 3730 — a trigger would emit { requestId: X, status: Idle/undefined }. 2. On exception paths the finally resets activeFirmwareUpdateRequestId but leaves stationInfo.firmwareStatus at its last-set value. A trigger fired between the exception and the next UpdateFirmware.req would emit { requestId: undefined, status: } — an OCPP 2.0.1 L01.FR.20 violation (requestId mandatory for non-Idle). Gate the non-Idle firmwareStatus branch on requestId being defined — activeFirmwareUpdateRequestId is the authoritative signal for 'an update is in progress'. When it is undefined we always emit Idle, which is spec-compliant regardless of the stationInfo staleness. * refactor(ocpp): extract getOrCreateWarnedMeasurands helper The resolveEnabledMeasurands warn-once path had a 4-line inline lazy-init block on warnedInvalidMeasurands (WeakMap>). Extract it as a module-level helper mirroring the round-10 getOrCreateStationState naming. Same semantic distinction as OCPP20IncomingRequestService: - getOrCreate* — lazy-init, always returns a live reference - .get() — raw WeakMap read, may return undefined * fix(ocpp20): apply dual-gate to LogStatusNotification trigger The LogStatusNotification TriggerMessage case sent { status: Idle } unconditionally, with no requestId — losing the signal when a log upload is genuinely in progress and mirroring the same pre-fix gap round-11 closed for FirmwareStatusNotification. Round-11 established the dual-gate pattern: an active requestId in stationsState is the authoritative 'operation in progress' signal; the status is emitted from the same source of truth to guarantee consistency between requestId and status. Log upload has no stationInfo.logStatus equivalent (unlike firmware's stationInfo.firmwareStatus), so store the status alongside the requestId in the stationState. Wrap simulateLogUploadLifecycle in try/finally to guarantee cleanup on exception. The trigger now reports Uploading/Uploaded with the active requestId when in-flight, Idle otherwise — matching the firmware pattern structurally. * docs(ocpp20): enumerate all 8 in-progress statuses in hasFirmwareUpdateInProgress @returns The JSDoc @returns listed 3 statuses (Downloading, Downloaded, Installing) but the body checks 8 (also DownloadScheduled, DownloadPaused, InstallScheduled, InstallRebooting, SignatureVerified). Enumerate all 8 to match the implementation. * fix(ocpp20): guard log-upload mid-lifecycle mutation + wrap firmware JSDoc Two round-13 findings surfaced in the same file: 1. simulateLogUploadLifecycle mutated activeLogUploadStatus = Uploaded without an identity guard. If a concurrent GetLog request installed a new lifecycle during the sleep, the old lifecycle's post-sleep resume would overwrite the new lifecycle's status with a stale Uploaded, causing the TriggerMessage handler to report the wrong signal for the in-flight upload. Guard the mid-lifecycle status mutation with the same identity check used in the finally. Extract clearActiveLogUpload as a helper mirroring clearActiveFirmwareUpdate — same raw-.get() + null-guard + identity-check + debug-log-on-superseded pattern for observability parity with the firmware peer. 2. hasFirmwareUpdateInProgress @returns clause was 183 chars on one line and used unbacktick'd 'true'. Wrap the enumeration across three lines and restore the `true` backtick style established by 92a8dac1. * fix(ocpp20): implement log-upload supersession per OCPP 2.0.1 N01.FR.12 Round-14 subagent C surfaced a HIGH spec-conformance gap: when a new GetLog request arrives with a prior upload in progress, the CS was returning Accepted and letting the old lifecycle run to completion (sending a spurious Uploaded for the superseded requestId). This violates N01.FR.12 + FR.20 and fails TC_N_36_CSMS step 7 which expects LogStatusNotification(AcceptedCanceled) for the old id. handleRequestGetLog now mirrors handleRequestUpdateFirmware: - detect active upload, capture the previous requestId - reset log state so the old lifecycle exits gracefully - send LogStatusNotification(AcceptedCanceled) for the old id - return GetLogResponse with status AcceptedCanceled The GET_LOG on-hook now accepts both Accepted and AcceptedCanceled to start the new lifecycle. The mid-lifecycle Uploaded emission is now co-located inside the identity guard: a superseded lifecycle skips both the state mutation AND the wire notification, preventing the spurious Uploaded that would follow an already-emitted AcceptedCanceled. Extracted resetActiveLogUploadState mirroring resetActiveFirmwareUpdateState (round-14 subagent A finding 2) so clearActiveLogUpload delegates to the two-field wipe rather than inlining it — full structural parity with the firmware peer. * fix(ocpp20): round-15 clear-log parity + stop() log reset + JSDoc Three findings from the same review round, one file: 1. clearActiveFirmwareUpdate and clearActiveLogUpload logged the debug 'superseded requestId X (active: Y)' message even when the active field was undefined (the state was reset, not superseded). The post-supersession trace fires clearActive* with the old requestId after resetActive*State cleared the active field — the log said 'active: undefined' which reads as a bug. Tighten the else branch to fire only when a DIFFERENT non-null requestId is active; drop the String() wrapper since only the number path can reach it. 2. stop() aborted the firmware controller and reset firmware state before deleting the stationsState entry, but skipped the log upload state. The delete makes the fields unreachable so it is not a memory leak, but the asymmetry breaks the structural mirror with firmware and the aborted-but-still-running log lifecycle would see inconsistent state post-stop. Add resetActiveLogUploadState for parity. 3. handleRequestGetLog @returns still said 'Accepted status' only — stale since the round-14 supersession fix. Extend to mention both Accepted (no prior upload) and AcceptedCanceled (superseded prior upload) with a description sentence on the supersession behavior. * fix(ocpp20): cancel cert-signing retry timer in stop() + JSDoc polish Three findings from round-16 review of the round-15 changes: 1. MEDIUM: OCPP20StationState.certSigningRetryManager holds a live setTimeout whose callback closes over chargingStation. The stationsState.delete() in stop() drops the WeakMap entry but the timer keeps chargingStation alive until it fires. Add certSigningRetryManager?.cancelRetryTimer() alongside the firmware abort() call, before the resets and the delete — closes the leak and matches the abort-before-reset pattern established for firmware. 2. LOW: 'simulates a Uploading' — wrong article. Fix to 'simulates an Uploading' (starts with vowel sound). 3. LOW: The @returns clause exceeded 140 chars on a single line. Wrap onto two lines to match the ~100-char convention. * fix(ocpp20): add AbortController to log-upload lifecycle + canceled spelling Round-17 findings from rounds 13-17 flagged the log-upload/firmware structural asymmetry repeatedly: firmware had AbortController + reset, cert-signing had cancelRetryTimer + reset, log had only identity-guard + state reset. A superseded lifecycle woke from sleep on its own schedule, wasted the delay, and the aborted-but-still-running coroutine emitted spurious wire notifications until the identity guard caught it. Round-17's harmonization subagent explicitly said 'worth fixing in this PR while the pattern is fresh'. Add activeLogUploadAbortController to OCPP20StationState, create it in simulateLogUploadLifecycle, replace sleep() with interruptibleSleep threading the signal, and gate the mid-lifecycle status write and send on both signal.aborted and the identity check. abort() the controller in handleRequestGetLog supersession, in stop(), and reset it via resetActiveLogUploadState. sleep import is now unused — removed. Also fix pre-existing British/American spelling drift surfaced by round-17 content review: 'Retry timer cancelled' -> 'canceled', for consistency with the cancelRetryTimer method name and OCPP enum values (AcceptedCanceled). --- cspell.config.yaml | 1 + .../AutomaticTransactionGenerator.ts | 235 ++++++--- src/charging-station/ChargingStation.ts | 48 +- .../HelpersChargingProfile.ts | 6 +- .../meter-values/CoherentSession.ts | 2 +- .../ocpp/1.6/OCPP16IncomingRequestService.ts | 7 + .../ocpp/2.0/OCPP20CertSigningRetryManager.ts | 2 +- .../ocpp/2.0/OCPP20IncomingRequestService.ts | 476 +++++++++++------- .../ocpp/OCPPIncomingRequestService.ts | 14 +- .../ocpp/OCPPRequestService.ts | 18 +- .../ocpp/OCPPResponseService.ts | 8 +- src/charging-station/ocpp/OCPPServiceUtils.ts | 32 +- .../ocpp/auth/services/OCPPAuthServiceImpl.ts | 14 - .../ui-server/AbstractUIServer.ts | 8 + src/performance/storage/Storage.ts | 13 + src/utils/Constants.ts | 2 +- src/utils/Utils.ts | 45 ++ src/utils/index.ts | 2 + 18 files changed, 602 insertions(+), 331 deletions(-) diff --git a/cspell.config.yaml b/cspell.config.yaml index 02662796..5db4c063 100644 --- a/cspell.config.yaml +++ b/cspell.config.yaml @@ -103,3 +103,4 @@ words: - secret - emerg - REMAPPINGS + - interruptible diff --git a/src/charging-station/AutomaticTransactionGenerator.ts b/src/charging-station/AutomaticTransactionGenerator.ts index b352db03..fff64748 100644 --- a/src/charging-station/AutomaticTransactionGenerator.ts +++ b/src/charging-station/AutomaticTransactionGenerator.ts @@ -20,12 +20,13 @@ import { Constants, convertToDate, formatDurationMilliSeconds, + interruptibleSleep, isEmpty, isValidDate, + isValidRandomIntBounds, logger, logPrefix, secureRandom, - sleep, } from '../utils/index.js' import { checkChargingStationState } from './Helpers.js' import { IdTagsCache } from './IdTagsCache.js' @@ -47,6 +48,8 @@ export class AutomaticTransactionGenerator { public started: boolean private readonly chargingStation: ChargingStation + private configurationValidationResult: boolean | undefined + private readonly connectorAbortControllers: Map private starting: boolean private stopping: boolean @@ -56,6 +59,8 @@ export class AutomaticTransactionGenerator { this.stopping = false this.chargingStation = chargingStation this.connectorsStatus = new Map() + this.connectorAbortControllers = new Map() + this.configurationValidationResult = undefined this.initializeConnectorsStatus() } @@ -112,6 +117,9 @@ export class AutomaticTransactionGenerator { throw new BaseError(`Connector ${connectorId.toString()} does not exist`) } if (this.connectorsStatus.get(connectorId)?.start === false) { + if (!this.validateConfiguration()) { + return + } this.internalStartConnector(connectorId, stopAbsoluteDuration).catch((error: unknown) => logger.error( `${this.logPrefix(connectorId)} ${moduleName}.startConnector: Error while starting connector:`, @@ -126,6 +134,10 @@ export class AutomaticTransactionGenerator { } public stop (): void { + // Reset before the guards so external startConnector() callers can + // recover from a cached-false state via stop() even when start() + // was never called. + this.configurationValidationResult = undefined if (!this.started) { logger.warn(`${this.logPrefix()} ${moduleName}.stop: Already stopped`) return @@ -150,6 +162,10 @@ export class AutomaticTransactionGenerator { const connectorStatus = this.connectorsStatus.get(connectorId) if (connectorStatus?.start === true) { connectorStatus.start = false + // Wake internalStartConnector out of its interruptible sleeps so + // it observes start=false immediately rather than after the + // current wait (up to maxDuration seconds) expires. + this.connectorAbortControllers.get(connectorId)?.abort() } else if (connectorStatus?.start === false) { logger.warn( `${this.logPrefix(connectorId)} ${moduleName}.stopConnector: Already stopped on connector` @@ -290,6 +306,12 @@ export class AutomaticTransactionGenerator { if (connectorStatus == null) { return } + // Abort the previous controller (if any) first to unblock any + // lingering interruptibleSleep from an earlier invocation on this + // connector. + this.connectorAbortControllers.get(connectorId)?.abort() + const abortController = new AbortController() + this.connectorAbortControllers.set(connectorId, abortController) logger.info( `${this.logPrefix( connectorId @@ -297,76 +319,91 @@ export class AutomaticTransactionGenerator { (connectorStatus.stopDate?.getTime() ?? 0) - (connectorStatus.startDate?.getTime() ?? 0) )}` ) - while (this.connectorsStatus.get(connectorId)?.start === true) { - await this.waitChargingStationAvailable(connectorId) - await this.waitConnectorAvailable(connectorId) - await this.waitRunningTransactionStopped(connectorId) - if (!this.canStartConnector(connectorId)) { - this.stopConnector(connectorId) - break - } - const wait = secondsToMilliseconds( - randomInt( - this.chargingStation.getAutomaticTransactionGeneratorConfiguration() - ?.minDelayBetweenTwoTransactions ?? 0, - (this.chargingStation.getAutomaticTransactionGeneratorConfiguration() - ?.maxDelayBetweenTwoTransactions ?? 0) + 1 - ) - ) - logger.info( - `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Waiting for ${formatDurationMilliSeconds(wait)}` - ) - await sleep(wait) - const start = secureRandom() - if ( - start < - (this.chargingStation.getAutomaticTransactionGeneratorConfiguration()?.probabilityOfStart ?? - 0) + try { + while ( + this.connectorsStatus.get(connectorId)?.start === true && + !abortController.signal.aborted ) { - connectorStatus.skippedConsecutiveTransactions = 0 - // Start transaction - const startResponse = await this.startTransaction(connectorId) - if (startResponse?.accepted === true) { - // Wait until end of transaction - const waitTrxEnd = secondsToMilliseconds( - randomInt( - this.chargingStation.getAutomaticTransactionGeneratorConfiguration()?.minDuration ?? - 0, - (this.chargingStation.getAutomaticTransactionGeneratorConfiguration()?.maxDuration ?? - 0) + 1 - ) + await this.waitChargingStationAvailable(connectorId, abortController.signal) + await this.waitConnectorAvailable(connectorId, abortController.signal) + await this.waitRunningTransactionStopped(connectorId, abortController.signal) + if (!this.canStartConnector(connectorId)) { + this.stopConnector(connectorId) + break + } + const wait = secondsToMilliseconds( + randomInt( + this.chargingStation.getAutomaticTransactionGeneratorConfiguration() + ?.minDelayBetweenTwoTransactions ?? 0, + (this.chargingStation.getAutomaticTransactionGeneratorConfiguration() + ?.maxDelayBetweenTwoTransactions ?? 0) + 1 ) + ) + logger.info( + `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Waiting for ${formatDurationMilliSeconds(wait)}` + ) + await interruptibleSleep(wait, abortController.signal) + const start = secureRandom() + if ( + start < + (this.chargingStation.getAutomaticTransactionGeneratorConfiguration() + ?.probabilityOfStart ?? 0) + ) { + connectorStatus.skippedConsecutiveTransactions = 0 + const startResponse = await this.startTransaction(connectorId) + if (startResponse?.accepted === true) { + const waitTrxEnd = secondsToMilliseconds( + randomInt( + this.chargingStation.getAutomaticTransactionGeneratorConfiguration()?.minDuration ?? + 0, + (this.chargingStation.getAutomaticTransactionGeneratorConfiguration() + ?.maxDuration ?? 0) + 1 + ) + ) + logger.info( + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Transaction started with id ${this.chargingStation + .getConnectorStatus(connectorId) + ?.transactionId?.toString()} and will stop in ${formatDurationMilliSeconds(waitTrxEnd)}` + ) + await interruptibleSleep(waitTrxEnd, abortController.signal) + await this.stopTransaction(connectorId) + } + } else { + ++connectorStatus.skippedConsecutiveTransactions + ++connectorStatus.skippedTransactions logger.info( - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Transaction started with id ${this.chargingStation - .getConnectorStatus(connectorId) - ?.transactionId?.toString()} and will stop in ${formatDurationMilliSeconds(waitTrxEnd)}` + `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Skipped consecutively ${connectorStatus.skippedConsecutiveTransactions.toString()}/${connectorStatus.skippedTransactions.toString()} transaction(s)` ) - await sleep(waitTrxEnd) - await this.stopTransaction(connectorId) } - } else { - ++connectorStatus.skippedConsecutiveTransactions - ++connectorStatus.skippedTransactions + connectorStatus.lastRunDate = new Date() + } + } finally { + // Gate all post-loop bookkeeping on ownership of the current + // controller entry: a concurrent stopConnector→startConnector may + // have already installed a successor and reset connectorStatus. + const isOwner = this.connectorAbortControllers.get(connectorId) === abortController + if (isOwner) { + this.connectorAbortControllers.delete(connectorId) + connectorStatus.stoppedDate = new Date() logger.info( - `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Skipped consecutively ${connectorStatus.skippedConsecutiveTransactions.toString()}/${connectorStatus.skippedTransactions.toString()} transaction(s)` + `${this.logPrefix( + connectorId + )} ${moduleName}.internalStartConnector: Stopped on connector and lasted for ${formatDurationMilliSeconds( + connectorStatus.stoppedDate.getTime() - (connectorStatus.startDate?.getTime() ?? 0) + )}` + ) + logger.debug( + `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Stopped with connector status: %j`, + connectorStatus + ) + this.chargingStation.emitChargingStationEvent(ChargingStationEvents.updated) + } else { + logger.debug( + `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Exited displaced loop — a successor controller is already installed` ) } - connectorStatus.lastRunDate = new Date() } - connectorStatus.stoppedDate = new Date() - logger.info( - `${this.logPrefix( - connectorId - )} ${moduleName}.internalStartConnector: Stopped on connector and lasted for ${formatDurationMilliSeconds( - connectorStatus.stoppedDate.getTime() - (connectorStatus.startDate?.getTime() ?? 0) - )}` - ) - logger.debug( - `${this.logPrefix(connectorId)} ${moduleName}.internalStartConnector: Stopped with connector status: %j`, - connectorStatus - ) - this.chargingStation.emitChargingStationEvent(ChargingStationEvents.updated) } private readonly logPrefix = (connectorId?: number): string => { @@ -508,9 +545,55 @@ export class AutomaticTransactionGenerator { return result } - private async waitChargingStationAvailable (connectorId: number): Promise { + /** + * Validates min/max invariants on the ATG configuration so a + * mis-configured template cannot kill the run loop with a + * `randomInt(min, max + 1)` RangeError at the first iteration. + * Memoized per `start()` cycle; reset by `stop()`. Returns `true` + * when no configuration is present (absence is handled elsewhere + * with defaulting to 0). + * @returns `true` when configuration is absent or its min/max + * invariants hold; `false` when a violation was found and logged. + */ + private validateConfiguration (): boolean { + if (this.configurationValidationResult !== undefined) { + return this.configurationValidationResult + } + const config = this.chargingStation.getAutomaticTransactionGeneratorConfiguration() + if (config == null) { + this.configurationValidationResult = true + return true + } + const { + maxDelayBetweenTwoTransactions, + maxDuration, + minDelayBetweenTwoTransactions, + minDuration, + } = config + if (!isValidRandomIntBounds(minDelayBetweenTwoTransactions, maxDelayBetweenTwoTransactions)) { + logger.error( + `${this.logPrefix()} ${moduleName}.validateConfiguration: invalid bounds minDelayBetweenTwoTransactions=${minDelayBetweenTwoTransactions.toString()}, maxDelayBetweenTwoTransactions=${maxDelayBetweenTwoTransactions.toString()} — aborting connector startup` + ) + this.configurationValidationResult = false + return false + } + if (!isValidRandomIntBounds(minDuration, maxDuration)) { + logger.error( + `${this.logPrefix()} ${moduleName}.validateConfiguration: invalid bounds minDuration=${minDuration.toString()}, maxDuration=${maxDuration.toString()} — aborting connector startup` + ) + this.configurationValidationResult = false + return false + } + this.configurationValidationResult = true + return true + } + + private async waitChargingStationAvailable ( + connectorId: number, + signal: AbortSignal + ): Promise { let logged = false - while (!this.chargingStation.isChargingStationAvailable()) { + while (!this.chargingStation.isChargingStationAvailable() && !signal.aborted) { if (!logged) { logger.info( `${this.logPrefix( @@ -519,13 +602,13 @@ export class AutomaticTransactionGenerator { ) logged = true } - await sleep(Constants.DEFAULT_ATG_WAIT_TIME_MS) + await interruptibleSleep(Constants.DEFAULT_ATG_WAIT_TIME_MS, signal) } } - private async waitConnectorAvailable (connectorId: number): Promise { + private async waitConnectorAvailable (connectorId: number, signal: AbortSignal): Promise { let logged = false - while (!this.chargingStation.isConnectorAvailable(connectorId)) { + while (!this.chargingStation.isConnectorAvailable(connectorId) && !signal.aborted) { if (!logged) { logger.info( `${this.logPrefix( @@ -534,22 +617,28 @@ export class AutomaticTransactionGenerator { ) logged = true } - await sleep(Constants.DEFAULT_ATG_WAIT_TIME_MS) + await interruptibleSleep(Constants.DEFAULT_ATG_WAIT_TIME_MS, signal) } } - private async waitRunningTransactionStopped (connectorId: number): Promise { - const connectorStatus = this.chargingStation.getConnectorStatus(connectorId) + private async waitRunningTransactionStopped ( + connectorId: number, + signal: AbortSignal + ): Promise { let logged = false - while (connectorStatus?.transactionStarted === true) { + while ( + this.chargingStation.getConnectorStatus(connectorId)?.transactionStarted === true && + !signal.aborted + ) { if (!logged) { + const transactionId = this.chargingStation.getConnectorStatus(connectorId)?.transactionId logger.info( // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - `${this.logPrefix(connectorId)} ${moduleName}.waitRunningTransactionStopped: Transaction loop waiting for started transaction ${connectorStatus.transactionId?.toString()} on connector ${connectorId.toString()} to be stopped` + `${this.logPrefix(connectorId)} ${moduleName}.waitRunningTransactionStopped: Transaction loop waiting for started transaction ${transactionId?.toString()} on connector ${connectorId.toString()} to be stopped` ) logged = true } - await sleep(Constants.DEFAULT_ATG_WAIT_TIME_MS) + await interruptibleSleep(Constants.DEFAULT_ATG_WAIT_TIME_MS, signal) } } } diff --git a/src/charging-station/ChargingStation.ts b/src/charging-station/ChargingStation.ts index 47371d50..95234b40 100644 --- a/src/charging-station/ChargingStation.ts +++ b/src/charging-station/ChargingStation.ts @@ -63,7 +63,7 @@ import { SupervisionUrlDistribution, SupportedFeatureProfiles, VendorParametersKey, - type Voltage, + Voltage, WebSocketCloseEventStatusCode, type WSError, type WsOptions, @@ -987,7 +987,6 @@ export class ChargingStation extends EventEmitter { options ) - // Handle WebSocket message this.wsConnection.on('message', data => { this.onMessage(data).catch((error: unknown) => logger.error( @@ -996,11 +995,8 @@ export class ChargingStation extends EventEmitter { ) ) }) - // Handle WebSocket error this.wsConnection.on('error', this.onError.bind(this)) - // Handle WebSocket close this.wsConnection.on('close', this.onClose.bind(this)) - // Handle WebSocket open this.wsConnection.on('open', () => { this.onOpen().catch((error: unknown) => logger.error( @@ -1009,9 +1005,7 @@ export class ChargingStation extends EventEmitter { ) ) }) - // Handle WebSocket ping this.wsConnection.on('ping', this.onPing.bind(this)) - // Handle WebSocket pong this.wsConnection.on('pong', this.onPong.bind(this)) } @@ -1076,17 +1070,13 @@ export class ChargingStation extends EventEmitter { /** Restarts the periodic heartbeat to the central server. */ public restartHeartbeat (): void { - // Stop heartbeat this.stopHeartbeat() - // Start heartbeat this.startHeartbeat() } /** Restarts the WebSocket ping interval. */ public restartWebSocketPing (): void { - // Stop WebSocket ping this.stopWebSocketPing() - // Start WebSocket ping this.startWebSocketPing() } @@ -1159,7 +1149,6 @@ export class ChargingStation extends EventEmitter { this.idTagsCache.deleteIdTags(idTagsFile) } OCPPAuthServiceFactory.clearInstance(this) - // Initialize this.initialize() // Restart the ATG const ATGStarted = this.automaticTransactionGenerator?.started @@ -1684,6 +1673,28 @@ export class ChargingStation extends EventEmitter { stationInfo.templateName = buildTemplateName(this.templateFile) createSerialNumber(stationTemplate, stationInfo) stationInfo.voltageOut = this.getVoltageOut(stationInfo) + if ( + this.getCurrentOutType(stationInfo) === CurrentType.AC && + (stationInfo.voltageOut === Voltage.VOLTAGE_400 || + stationInfo.voltageOut === Voltage.VOLTAGE_800) + ) { + const voltageOut = stationInfo.voltageOut + const derivedInfo = + this.getNumberOfPhases(stationInfo) === 3 + ? ` In 3-phase Y systems the derived line-to-line = sqrt(3) * ${voltageOut.toString()} = ${( + Math.sqrt(3) * voltageOut + ).toFixed(0)} V.` + : '' + const suggestion = + voltageOut === Voltage.VOLTAGE_400 + ? ` If you intended 400 V line-to-line, set voltageOut=${Voltage.VOLTAGE_230.toString()} V (standard L-N enum value for EU 3-phase).` + : ` No standard L-N enum value exists for ${voltageOut.toString()} V line-to-line (~${( + voltageOut / Math.sqrt(3) + ).toFixed(0)} V L-N); verify template voltageOut intent.` + logger.warn( + `${this.logPrefix()} ${moduleName}.getStationInfoFromTemplate: voltageOut=${voltageOut.toString()} V matches a line-to-line nominal value; the simulator treats voltageOut as line-to-neutral (phase voltage).${derivedInfo}${suggestion}` + ) + } if (isNotEmptyArray(stationTemplate.power)) { const powerArrayRandomIndex = Math.floor(secureRandom() * stationTemplate.power.length) stationInfo.maximumPower = @@ -1799,7 +1810,6 @@ export class ChargingStation extends EventEmitter { request )}` ) - // Process the message await this.ocppIncomingRequestService.incomingRequestHandler( this, messageId, @@ -2326,11 +2336,8 @@ export class ChargingStation extends EventEmitter { } private internalStopMessageSequence (): void { - // Stop WebSocket ping this.stopWebSocketPing() - // Stop heartbeat this.stopHeartbeat() - // Stop the ATG if (this.automaticTransactionGenerator?.started === true) { this.stopAutomaticTransactionGenerator() } @@ -2386,7 +2393,6 @@ export class ChargingStation extends EventEmitter { request = JSON.parse(data.toString()) as ErrorResponse | IncomingRequest | Response if (Array.isArray(request)) { ;[messageType] = request - // Check the type of message switch (messageType) { // Error Message case MessageType.CALL_ERROR_MESSAGE: @@ -2473,13 +2479,11 @@ export class ChargingStation extends EventEmitter { errorCallback(ocppError, false) } } else { - // Remove the request from the cache in case of error at response handling this.requests.delete(messageId) } break case MessageType.CALL_MESSAGE: ;[, , commandName] = request as IncomingRequest - // Send error await this.ocppRequestService.sendError(this, messageId, ocppError, commandName) break } @@ -2519,7 +2523,6 @@ export class ChargingStation extends EventEmitter { ) let registrationRetryCount = 0 if (!this.inAcceptedState()) { - // Send BootNotification do { await this.ocppRequestService.requestHandler< BootNotificationRequest, @@ -2832,15 +2835,12 @@ export class ChargingStation extends EventEmitter { skipBufferingOnError: true, }) } - // Start WebSocket ping if (this.wsPingSetInterval == null) { this.startWebSocketPing() } - // Start heartbeat if (this.heartbeatSetInterval == null) { this.startHeartbeat() } - // Initialize connectors status for (const { connectorId, connectorStatus, evseId } of this.iterateConnectors(true)) { await sendAndSetConnectorStatus(this, { connectorId, @@ -2858,7 +2858,6 @@ export class ChargingStation extends EventEmitter { this.stationInfo.firmwareStatus = FirmwareStatus.Installed } - // Start the ATG if (this.getAutomaticTransactionGeneratorConfiguration()?.enable === true) { this.startAutomaticTransactionGenerator(undefined, ATGStopAbsoluteDuration) } @@ -2906,7 +2905,6 @@ export class ChargingStation extends EventEmitter { stopTransactions?: boolean ): Promise { this.internalStopMessageSequence() - // Stop ongoing transactions stopTransactions && (await stopRunningTransactions(this, reason)) for (const { connectorId, connectorStatus, evseId } of this.iterateConnectors(true)) { await sendAndSetConnectorStatus(this, { diff --git a/src/charging-station/HelpersChargingProfile.ts b/src/charging-station/HelpersChargingProfile.ts index 962fe0b5..da5c5c15 100644 --- a/src/charging-station/HelpersChargingProfile.ts +++ b/src/charging-station/HelpersChargingProfile.ts @@ -335,7 +335,6 @@ const getChargingProfilesLimit = ( if (!canProceedChargingProfile(chargingProfile, currentDate, chargingStation.logPrefix())) { continue } - // Check if the charging profile is active if ( isWithinInterval(currentDate, { end: addSeconds(chargingSchedule.startSchedule, chargingSchedule.duration), @@ -360,7 +359,6 @@ const getChargingProfilesLimit = ( chargingSchedule.chargingSchedulePeriod.sort(chargingSchedulePeriodCompareFn) } - // Check if the first schedule period startPeriod property is equal to 0 if (chargingSchedule.chargingSchedulePeriod[0].startPeriod !== 0) { logger.error( @@ -368,7 +366,6 @@ const getChargingProfilesLimit = ( ) continue } - // Handle only one schedule period if (chargingSchedule.chargingSchedulePeriod.length === 1) { const chargingProfilesLimit: ChargingProfilesLimit = { @@ -401,7 +398,8 @@ const getChargingProfilesLimit = ( logger.debug(debugLogMsg, chargingProfilesLimit) return chargingProfilesLimit } - // Handle the last schedule period within the charging profile duration + // Last period: last in array OR next period would exceed the + // charging profile duration. if ( index === chargingSchedule.chargingSchedulePeriod.length - 1 || (index < chargingSchedule.chargingSchedulePeriod.length - 1 && diff --git a/src/charging-station/meter-values/CoherentSession.ts b/src/charging-station/meter-values/CoherentSession.ts index e8a77dc0..7b301ddf 100644 --- a/src/charging-station/meter-values/CoherentSession.ts +++ b/src/charging-station/meter-values/CoherentSession.ts @@ -100,7 +100,7 @@ export const createCoherentSession = ( (voltageOutNominal === Voltage.VOLTAGE_400 || voltageOutNominal === Voltage.VOLTAGE_800) ) { logger.warn( - `${context.logPrefix()} ${moduleName}.createCoherentSession: AC voltageOut=${voltageOutNominal.toString()}V is treated as line-to-neutral (phase voltage) by ACElectricUtils. If this value is meant as line-to-line, coherent power/current will be physically implausible.` + `${context.logPrefix()} ${moduleName}.createCoherentSession: voltageOut=${voltageOutNominal.toString()} V matches a line-to-line nominal value; the simulator treats voltageOut as line-to-neutral (phase voltage). If you intended this as line-to-line, coherent power/current will be physically implausible. Set voltageOut to the L-N equivalent in the station template to resolve.` ) } diff --git a/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts b/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts index 95921fbb..68407e4f 100644 --- a/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts +++ b/src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts @@ -117,6 +117,7 @@ import { isEmpty, isNotEmptyArray, isNotEmptyString, + isValidRandomIntBounds, logger, sleep, truncateId, @@ -1836,6 +1837,12 @@ export class OCPP16IncomingRequestService extends OCPPIncomingRequestService { if (!checkChargingStationState(chargingStation, chargingStation.logPrefix())) { return } + if (!isValidRandomIntBounds(minDelay, maxDelay)) { + logger.error( + `${chargingStation.logPrefix()} ${moduleName}.updateFirmwareSimulation: invalid bounds minDelay=${minDelay.toString()}, maxDelay=${maxDelay.toString()} — aborting firmware update simulation` + ) + return + } for (const { connectorId, connectorStatus } of chargingStation.iterateConnectors(true)) { if (connectorStatus.transactionStarted === false) { await sendAndSetConnectorStatus(chargingStation, { diff --git a/src/charging-station/ocpp/2.0/OCPP20CertSigningRetryManager.ts b/src/charging-station/ocpp/2.0/OCPP20CertSigningRetryManager.ts index 71549a5d..75603981 100644 --- a/src/charging-station/ocpp/2.0/OCPP20CertSigningRetryManager.ts +++ b/src/charging-station/ocpp/2.0/OCPP20CertSigningRetryManager.ts @@ -41,7 +41,7 @@ export class OCPP20CertSigningRetryManager { } this.retryCount = 0 logger.debug( - `${this.chargingStation.logPrefix()} ${moduleName}.cancelRetryTimer: Retry timer cancelled` + `${this.chargingStation.logPrefix()} ${moduleName}.cancelRetryTimer: Retry timer canceled` ) } diff --git a/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts b/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts index c8af668c..27778cb1 100644 --- a/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts +++ b/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts @@ -143,12 +143,12 @@ import { generateUUID, getErrorMessage, handleIncomingRequestError, + interruptibleSleep, isEmpty, isNotEmptyArray, isNotEmptyString, logger, promiseWithTimeout, - sleep, truncateId, validateUUID, } from '../../../utils/index.js' @@ -258,6 +258,9 @@ const buildRejectedResponse = ( interface OCPP20StationState { activeFirmwareUpdateAbortController?: AbortController activeFirmwareUpdateRequestId?: number + activeLogUploadAbortController?: AbortController + activeLogUploadRequestId?: number + activeLogUploadStatus?: UploadLogStatusEnumType certSigningRetryManager?: OCPP20CertSigningRetryManager isDrainingSecurityEvents: boolean preInoperativeConnectorStatuses: Map @@ -437,7 +440,10 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { request: OCPP20GetLogRequest, response: OCPP20GetLogResponse ) => { - if (response.status === LogStatusEnumType.Accepted) { + if ( + response.status === LogStatusEnumType.Accepted || + response.status === LogStatusEnumType.AcceptedCanceled + ) { this.simulateLogUploadLifecycle(chargingStation, request.requestId).catch( (error: unknown) => { logger.error( @@ -561,10 +567,12 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { .catch(errorHandler) break case MessageTriggerEnumType.FirmwareStatusNotification: { - const firmwareStatus = this.hasFirmwareUpdateInProgress(chargingStation) - ? (chargingStation.stationInfo?.firmwareStatus as OCPP20FirmwareStatusEnumType) - : OCPP20FirmwareStatusEnumType.Idle - const stationState = this.getStationState(chargingStation) + const stationState = this.stationsState.get(chargingStation) + const requestId = stationState?.activeFirmwareUpdateRequestId + const firmwareStatus = + requestId != null && this.hasFirmwareUpdateInProgress(chargingStation) + ? (chargingStation.stationInfo?.firmwareStatus as OCPP20FirmwareStatusEnumType) + : OCPP20FirmwareStatusEnumType.Idle chargingStation.ocppRequestService .requestHandler< OCPP20FirmwareStatusNotificationRequest, @@ -572,7 +580,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { >( chargingStation, OCPP20RequestCommand.FIRMWARE_STATUS_NOTIFICATION, - { requestId: stationState.activeFirmwareUpdateRequestId, status: firmwareStatus }, + { requestId, status: firmwareStatus }, { skipBufferingOnError: true, triggerMessage: true } ) .catch(errorHandler) @@ -588,7 +596,13 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { ) .catch(errorHandler) break - case MessageTriggerEnumType.LogStatusNotification: + case MessageTriggerEnumType.LogStatusNotification: { + const stationState = this.stationsState.get(chargingStation) + const requestId = stationState?.activeLogUploadRequestId + const logStatus = + requestId != null + ? (stationState?.activeLogUploadStatus ?? UploadLogStatusEnumType.Uploading) + : UploadLogStatusEnumType.Idle chargingStation.ocppRequestService .requestHandler< OCPP20LogStatusNotificationRequest, @@ -596,11 +610,12 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { >( chargingStation, OCPP20RequestCommand.LOG_STATUS_NOTIFICATION, - { status: UploadLogStatusEnumType.Idle }, + { requestId, status: logStatus }, { skipBufferingOnError: true, triggerMessage: true } ) .catch(errorHandler) break + } case MessageTriggerEnumType.MeterValues: this.triggerMeterValues(chargingStation, evse, errorHandler) break @@ -615,7 +630,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { public getCertSigningRetryManager ( chargingStation: ChargingStation ): OCPP20CertSigningRetryManager { - const state = this.getStationState(chargingStation) + const state = this.getOrCreateStationState(chargingStation) state.certSigningRetryManager ??= new OCPP20CertSigningRetryManager(chargingStation) return state.certSigningRetryManager } @@ -774,6 +789,10 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { const stationState = this.stationsState.get(chargingStation) if (stationState != null) { stationState.activeFirmwareUpdateAbortController?.abort() + stationState.activeLogUploadAbortController?.abort() + stationState.certSigningRetryManager?.cancelRetryTimer() + this.resetActiveFirmwareUpdateState(stationState) + this.resetActiveLogUploadState(stationState) this.stationsState.delete(chargingStation) } try { @@ -1282,10 +1301,30 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { } private clearActiveFirmwareUpdate (chargingStation: ChargingStation, requestId: number): void { - const stationState = this.getStationState(chargingStation) + const stationState = this.stationsState.get(chargingStation) + if (stationState == null) { + return + } if (stationState.activeFirmwareUpdateRequestId === requestId) { - stationState.activeFirmwareUpdateAbortController = undefined - stationState.activeFirmwareUpdateRequestId = undefined + this.resetActiveFirmwareUpdateState(stationState) + } else if (stationState.activeFirmwareUpdateRequestId != null) { + logger.debug( + `${chargingStation.logPrefix()} ${moduleName}.clearActiveFirmwareUpdate: Ignoring completion for superseded requestId ${requestId.toString()} (active: ${stationState.activeFirmwareUpdateRequestId.toString()})` + ) + } + } + + private clearActiveLogUpload (chargingStation: ChargingStation, requestId: number): void { + const stationState = this.stationsState.get(chargingStation) + if (stationState == null) { + return + } + if (stationState.activeLogUploadRequestId === requestId) { + this.resetActiveLogUploadState(stationState) + } else if (stationState.activeLogUploadRequestId != null) { + logger.debug( + `${chargingStation.logPrefix()} ${moduleName}.clearActiveLogUpload: Ignoring completion for superseded requestId ${requestId.toString()} (active: ${stationState.activeLogUploadRequestId.toString()})` + ) } } @@ -1372,20 +1411,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { .catch(errorHandler) } - private getRestoredConnectorStatus ( - chargingStation: ChargingStation, - connectorId: number - ): OCPP20ConnectorStatusEnumType { - const stationState = this.getStationState(chargingStation) - const saved = stationState.preInoperativeConnectorStatuses.get(connectorId) - if (saved != null) { - stationState.preInoperativeConnectorStatuses.delete(connectorId) - return saved - } - return OCPP20ConnectorStatusEnumType.Available - } - - private getStationState (chargingStation: ChargingStation): OCPP20StationState { + private getOrCreateStationState (chargingStation: ChargingStation): OCPP20StationState { let state = this.stationsState.get(chargingStation) if (state == null) { state = { @@ -1399,6 +1425,19 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { return state } + private getRestoredConnectorStatus ( + chargingStation: ChargingStation, + connectorId: number + ): OCPP20ConnectorStatusEnumType { + const stationState = this.getOrCreateStationState(chargingStation) + const saved = stationState.preInoperativeConnectorStatuses.get(connectorId) + if (saved != null) { + stationState.preInoperativeConnectorStatuses.delete(connectorId) + return saved + } + return OCPP20ConnectorStatusEnumType.Available + } + private handleConnectorChangeAvailability ( chargingStation: ChargingStation, evseId: number, @@ -1970,7 +2009,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { } } - const stationState = this.getStationState(chargingStation) + const stationState = this.getOrCreateStationState(chargingStation) const cached = stationState.reportDataCache.get(commandPayload.requestId) const reportData = cached ?? this.buildReportData(chargingStation, commandPayload.reportBase) if (!cached && isNotEmptyArray(reportData)) { @@ -2063,12 +2102,13 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { /** * Handles OCPP 2.0.1 GetLog request from central system. - * Accepts the log upload request and simulates the log upload lifecycle - * by sending LogStatusNotification messages through a state machine: - * Uploading → Uploaded + * When a prior upload is in progress, cancels it per N01.FR.12/FR.20 + * and returns AcceptedCanceled; otherwise simulates an Uploading → Uploaded + * lifecycle via LogStatusNotification messages. * @param chargingStation - The charging station instance processing the request * @param commandPayload - GetLog request payload with log type, requestId, and log parameters - * @returns GetLogResponse with Accepted status and simulated filename + * @returns GetLogResponse with `Accepted` (no prior upload) or `AcceptedCanceled` + * (superseded prior upload) status and simulated filename. */ private handleRequestGetLog ( chargingStation: ChargingStation, @@ -2080,6 +2120,30 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { `${chargingStation.logPrefix()} ${moduleName}.handleRequestGetLog: Received GetLog request with requestId ${requestId.toString()} for logType '${logType}'` ) + const stationState = this.stationsState.get(chargingStation) + if (stationState?.activeLogUploadRequestId != null) { + const previousRequestId = stationState.activeLogUploadRequestId + stationState.activeLogUploadAbortController?.abort() + this.resetActiveLogUploadState(stationState) + this.sendLogStatusNotification( + chargingStation, + UploadLogStatusEnumType.AcceptedCanceled, + previousRequestId + ).catch((error: unknown) => { + logger.error( + `${chargingStation.logPrefix()} ${moduleName}.handleRequestGetLog: Failed to send AcceptedCanceled for superseded requestId ${previousRequestId.toString()}:`, + error + ) + }) + logger.info( + `${chargingStation.logPrefix()} ${moduleName}.handleRequestGetLog: Canceled previous log upload (requestId ${previousRequestId.toString()})` + ) + return { + filename: 'simulator-log.txt', + status: LogStatusEnumType.AcceptedCanceled, + } + } + return { filename: 'simulator-log.txt', status: LogStatusEnumType.Accepted, @@ -3085,12 +3149,11 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { } // Cancel any in-progress firmware update; respond with AcceptedCanceled so the new request starts fresh - const stationState = this.getStationState(chargingStation) + const stationState = this.getOrCreateStationState(chargingStation) if (stationState.activeFirmwareUpdateAbortController != null) { const previousRequestId = stationState.activeFirmwareUpdateRequestId stationState.activeFirmwareUpdateAbortController.abort() - stationState.activeFirmwareUpdateAbortController = undefined - stationState.activeFirmwareUpdateRequestId = undefined + this.resetActiveFirmwareUpdateState(stationState) logger.info( `${chargingStation.logPrefix()} ${moduleName}.handleRequestUpdateFirmware: Canceled previous firmware update (requestId ${String(previousRequestId)})` ) @@ -3135,7 +3198,9 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { /** * Checks if firmware update is in progress per OCPP 2.0.1 Errata idle definition. * @param chargingStation - The charging station instance - * @returns true if firmware update is in progress (Downloading, Downloaded, or Installing) + * @returns `true` when {@link OCPP20FirmwareStatusEnumType} is any non-terminal value: + * Downloading, Downloaded, Installing, DownloadScheduled, DownloadPaused, + * InstallScheduled, InstallRebooting, or SignatureVerified. */ private hasFirmwareUpdateInProgress (chargingStation: ChargingStation): boolean { const firmwareStatus = chargingStation.stationInfo?.firmwareStatus @@ -3246,6 +3311,17 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { } } + private resetActiveFirmwareUpdateState (stationState: OCPP20StationState): void { + stationState.activeFirmwareUpdateAbortController = undefined + stationState.activeFirmwareUpdateRequestId = undefined + } + + private resetActiveLogUploadState (stationState: OCPP20StationState): void { + stationState.activeLogUploadAbortController = undefined + stationState.activeLogUploadRequestId = undefined + stationState.activeLogUploadStatus = undefined + } + /** * Reset connector status on start transaction error * @param chargingStation - The charging station instance @@ -3272,7 +3348,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { * @param evseId - Optional EVSE ID to scope the save; if omitted, saves all EVSEs */ private savePreInoperativeStatuses (chargingStation: ChargingStation, evseId?: number): void { - const stationState = this.getStationState(chargingStation) + const stationState = this.getOrCreateStationState(chargingStation) const evseIds = evseId != null && evseId > 0 ? [evseId] @@ -3521,7 +3597,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { response: OCPP20GetBaseReportResponse ): Promise { const { reportBase, requestId } = request - const stationState = this.getStationState(chargingStation) + const stationState = this.getOrCreateStationState(chargingStation) const cached = stationState.reportDataCache.get(requestId) const reportData = cached ?? this.buildReportData(chargingStation, reportBase) @@ -3565,7 +3641,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { } private sendQueuedSecurityEvents (chargingStation: ChargingStation): void { - const stationState = this.getStationState(chargingStation) + const stationState = this.getOrCreateStationState(chargingStation) if ( stationState.isDrainingSecurityEvents || !chargingStation.isWebSocketConnectionOpened() || @@ -3670,7 +3746,7 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { logger.info( `${chargingStation.logPrefix()} ${moduleName}.sendSecurityEventNotification: [SecurityEvent] type=${type}${techInfo != null ? `, techInfo=${techInfo}` : ''}` ) - this.getStationState(chargingStation).securityEventQueue.push({ + this.getOrCreateStationState(chargingStation).securityEventQueue.push({ timestamp: new Date(), type, ...(techInfo !== undefined && { techInfo }), @@ -3697,189 +3773,197 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { ): Promise { const { installDateTime, location, retrieveDateTime, signature } = firmware - // Store the abort controller so a subsequent UpdateFirmware can cancel this in-progress update + // Store the abort controller so a subsequent UpdateFirmware can abort this in-progress update const abortController = new AbortController() - const stationState = this.getStationState(chargingStation) + const stationState = this.getOrCreateStationState(chargingStation) stationState.activeFirmwareUpdateAbortController = abortController stationState.activeFirmwareUpdateRequestId = requestId const checkAborted = (): boolean => abortController.signal.aborted - // Delay the download until retrieveDateTime; inform the CSMS via DownloadScheduled first - const now = Date.now() - const retrieveTime = convertToDate(retrieveDateTime)?.getTime() ?? now - if (retrieveTime > now) { - await this.sendFirmwareStatusNotification( - chargingStation, - OCPP20FirmwareStatusEnumType.DownloadScheduled, - requestId - ) - await sleep(retrieveTime - now) - if (checkAborted()) return - } - - await this.sendFirmwareStatusNotification( - chargingStation, - OCPP20FirmwareStatusEnumType.Downloading, - requestId - ) - - await sleep(OCPP20Constants.FIRMWARE_STATUS_DELAY_MS) - if (checkAborted()) return - - // Empty or malformed firmware location: simulate the L01.FR.30 download retries, then emit DownloadFailed and stop - if (isEmpty(location) || !this.isValidFirmwareLocation(location)) { - // L01.FR.30: Simulate download retries before reporting DownloadFailed - const maxRetries = retries ?? 0 - const retryDelayMs = secondsToMilliseconds(retryInterval ?? 0) - for (let attempt = 1; attempt <= maxRetries; attempt++) { - logger.warn( - `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Download failed for requestId ${requestId.toString()} - invalid location '${location}' (attempt ${attempt.toString()}/${maxRetries.toString()}, retrying in ${retryInterval?.toString() ?? '0'}s)` - ) - await sleep(retryDelayMs) - if (checkAborted()) return + try { + // Delay the download until retrieveDateTime; inform the CSMS via DownloadScheduled first + const now = Date.now() + const retrieveTime = convertToDate(retrieveDateTime)?.getTime() ?? now + if (retrieveTime > now) { await this.sendFirmwareStatusNotification( chargingStation, - OCPP20FirmwareStatusEnumType.Downloading, + OCPP20FirmwareStatusEnumType.DownloadScheduled, requestId ) - await sleep(OCPP20Constants.FIRMWARE_STATUS_DELAY_MS) + await interruptibleSleep(retrieveTime - now, abortController.signal) if (checkAborted()) return } + await this.sendFirmwareStatusNotification( chargingStation, - OCPP20FirmwareStatusEnumType.DownloadFailed, + OCPP20FirmwareStatusEnumType.Downloading, requestId ) - logger.warn( - `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Download failed for requestId ${requestId.toString()} - invalid location '${location}'${maxRetries > 0 ? ` (exhausted ${maxRetries.toString()} retries)` : ''}` - ) - this.clearActiveFirmwareUpdate(chargingStation, requestId) - return - } - - await this.sendFirmwareStatusNotification( - chargingStation, - OCPP20FirmwareStatusEnumType.Downloaded, - requestId - ) - if (signature != null) { - await sleep(OCPP20Constants.FIRMWARE_VERIFY_DELAY_MS) + await interruptibleSleep(OCPP20Constants.FIRMWARE_STATUS_DELAY_MS, abortController.signal) if (checkAborted()) return - // L01.FR.04: Simulate signature verification - const simulateFailure = OCPP20ServiceUtils.readVariableAsBoolean( - chargingStation, - OCPP20ComponentName.FirmwareCtrlr, - OCPP20VendorVariableName.SimulateSignatureVerificationFailure, - false - ) - - if (simulateFailure) { - // L01.FR.03: InvalidSignature + SecurityEventNotification + // Empty or malformed firmware location: simulate the L01.FR.30 download retries, then emit DownloadFailed and stop + if (isEmpty(location) || !this.isValidFirmwareLocation(location)) { + // L01.FR.30: Simulate download retries before reporting DownloadFailed + const maxRetries = retries ?? 0 + const retryDelayMs = secondsToMilliseconds(retryInterval ?? 0) + for (let attempt = 1; attempt <= maxRetries; attempt++) { + logger.warn( + `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Download failed for requestId ${requestId.toString()} - invalid location '${location}' (attempt ${attempt.toString()}/${maxRetries.toString()}, retrying in ${retryInterval?.toString() ?? '0'}s)` + ) + await interruptibleSleep(retryDelayMs, abortController.signal) + if (checkAborted()) return + await this.sendFirmwareStatusNotification( + chargingStation, + OCPP20FirmwareStatusEnumType.Downloading, + requestId + ) + await interruptibleSleep(OCPP20Constants.FIRMWARE_STATUS_DELAY_MS, abortController.signal) + if (checkAborted()) return + } await this.sendFirmwareStatusNotification( chargingStation, - OCPP20FirmwareStatusEnumType.InvalidSignature, + OCPP20FirmwareStatusEnumType.DownloadFailed, requestId ) - this.sendSecurityEventNotification( - chargingStation, - 'InvalidFirmwareSignature', - `Firmware signature verification failed for requestId ${requestId.toString()}` - ) logger.warn( - `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Firmware signature verification failed for requestId ${requestId.toString()} (simulated)` + `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Download failed for requestId ${requestId.toString()} - invalid location '${location}'${maxRetries > 0 ? ` (exhausted ${maxRetries.toString()} retries)` : ''}` ) - this.clearActiveFirmwareUpdate(chargingStation, requestId) return } await this.sendFirmwareStatusNotification( chargingStation, - OCPP20FirmwareStatusEnumType.SignatureVerified, + OCPP20FirmwareStatusEnumType.Downloaded, requestId ) - } - // Delay the install until installDateTime; inform the CSMS via InstallScheduled first - if (installDateTime != null) { - const currentTime = Date.now() - const installTime = convertToDate(installDateTime)?.getTime() ?? currentTime - if (installTime > currentTime) { + if (signature != null) { + await interruptibleSleep(OCPP20Constants.FIRMWARE_VERIFY_DELAY_MS, abortController.signal) + if (checkAborted()) return + + // L01.FR.04: Simulate signature verification + const simulateFailure = OCPP20ServiceUtils.readVariableAsBoolean( + chargingStation, + OCPP20ComponentName.FirmwareCtrlr, + OCPP20VendorVariableName.SimulateSignatureVerificationFailure, + false + ) + + if (simulateFailure) { + // L01.FR.03: InvalidSignature + SecurityEventNotification + await this.sendFirmwareStatusNotification( + chargingStation, + OCPP20FirmwareStatusEnumType.InvalidSignature, + requestId + ) + this.sendSecurityEventNotification( + chargingStation, + 'InvalidFirmwareSignature', + `Firmware signature verification failed for requestId ${requestId.toString()}` + ) + logger.warn( + `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Firmware signature verification failed for requestId ${requestId.toString()} (simulated)` + ) + return + } + await this.sendFirmwareStatusNotification( chargingStation, - OCPP20FirmwareStatusEnumType.InstallScheduled, + OCPP20FirmwareStatusEnumType.SignatureVerified, requestId ) - await sleep(installTime - currentTime) - if (checkAborted()) return } - } - // L01.FR.06: Wait for active transactions to end before installing - // L01.FR.07: Set idle connectors to Unavailable when AllowNewSessionsPendingFirmwareUpdate is false/absent - const hasActiveTransactionsBeforeInstall = chargingStation - .iterateEvses(true) - .some(({ evseStatus }) => this.hasEvseActiveTransactions(evseStatus)) - if (hasActiveTransactionsBeforeInstall) { - const allowNewSessions = OCPP20ServiceUtils.readVariableAsBoolean( - chargingStation, - OCPP20ComponentName.ChargingStation, - 'AllowNewSessionsPendingFirmwareUpdate', - false - ) - while ( - !checkAborted() && - chargingStation - .iterateEvses(true) - .some(({ evseStatus }) => this.hasEvseActiveTransactions(evseStatus)) - ) { - // L01.FR.07: Set newly-available EVSE to Unavailable on each iteration - if (!allowNewSessions) { - for (const { evseId, evseStatus } of chargingStation.iterateEvses(true)) { - if (!this.hasEvseActiveTransactions(evseStatus)) { - this.sendEvseStatusNotifications( - chargingStation, - evseId, - OCPP20ConnectorStatusEnumType.Unavailable - ) + // Delay the install until installDateTime; inform the CSMS via InstallScheduled first + if (installDateTime != null) { + const currentTime = Date.now() + const installTime = convertToDate(installDateTime)?.getTime() ?? currentTime + if (installTime > currentTime) { + await this.sendFirmwareStatusNotification( + chargingStation, + OCPP20FirmwareStatusEnumType.InstallScheduled, + requestId + ) + await interruptibleSleep(installTime - currentTime, abortController.signal) + if (checkAborted()) return + } + } + + // L01.FR.06: Wait for active transactions to end before installing + // L01.FR.07: Set idle connectors to Unavailable when AllowNewSessionsPendingFirmwareUpdate is false/absent + const hasActiveTransactionsBeforeInstall = chargingStation + .iterateEvses(true) + .some(({ evseStatus }) => this.hasEvseActiveTransactions(evseStatus)) + if (hasActiveTransactionsBeforeInstall) { + const allowNewSessions = OCPP20ServiceUtils.readVariableAsBoolean( + chargingStation, + OCPP20ComponentName.ChargingStation, + 'AllowNewSessionsPendingFirmwareUpdate', + false + ) + while ( + !checkAborted() && + chargingStation + .iterateEvses(true) + .some(({ evseStatus }) => this.hasEvseActiveTransactions(evseStatus)) + ) { + // L01.FR.07: Set newly-available EVSE to Unavailable on each iteration + if (!allowNewSessions) { + for (const { evseId, evseStatus } of chargingStation.iterateEvses(true)) { + if (!this.hasEvseActiveTransactions(evseStatus)) { + this.sendEvseStatusNotifications( + chargingStation, + evseId, + OCPP20ConnectorStatusEnumType.Unavailable + ) + } } } + logger.debug( + `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Waiting for active transactions to end before installing (L01.FR.06)` + ) + await interruptibleSleep( + OCPP20Constants.FIRMWARE_INSTALL_DELAY_MS, + abortController.signal + ) } - logger.debug( - `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Waiting for active transactions to end before installing (L01.FR.06)` - ) - await sleep(OCPP20Constants.FIRMWARE_INSTALL_DELAY_MS) } - } - if (checkAborted()) return + if (checkAborted()) return - await this.sendFirmwareStatusNotification( - chargingStation, - OCPP20FirmwareStatusEnumType.Installing, - requestId - ) + await this.sendFirmwareStatusNotification( + chargingStation, + OCPP20FirmwareStatusEnumType.Installing, + requestId + ) - await sleep(OCPP20Constants.RESET_DELAY_MS) - if (checkAborted()) return - await this.sendFirmwareStatusNotification( - chargingStation, - OCPP20FirmwareStatusEnumType.Installed, - requestId - ) + await interruptibleSleep(OCPP20Constants.RESET_DELAY_MS, abortController.signal) + if (checkAborted()) return + await this.sendFirmwareStatusNotification( + chargingStation, + OCPP20FirmwareStatusEnumType.Installed, + requestId + ) - // Send SecurityEventNotification after a successful firmware update - this.sendSecurityEventNotification( - chargingStation, - 'FirmwareUpdated', - `Firmware update completed for requestId ${requestId.toString()}` - ) + // Send SecurityEventNotification after a successful firmware update + this.sendSecurityEventNotification( + chargingStation, + 'FirmwareUpdated', + `Firmware update completed for requestId ${requestId.toString()}` + ) - this.clearActiveFirmwareUpdate(chargingStation, requestId) - logger.info( - `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Firmware update simulation completed for requestId ${requestId.toString()}` - ) + logger.info( + `${chargingStation.logPrefix()} ${moduleName}.simulateFirmwareUpdateLifecycle: Firmware update simulation completed for requestId ${requestId.toString()}` + ) + } finally { + // Guarantee cleanup on every exit path (happy, abort, throw): without + // this, a thrown await would leave activeFirmwareUpdateAbortController + // set and any subsequent UpdateFirmware.req would abort a non-existent + // in-progress update while the station stays stuck. + this.clearActiveFirmwareUpdate(chargingStation, requestId) + } } /** @@ -3892,22 +3976,34 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { chargingStation: ChargingStation, requestId: number ): Promise { - await this.sendLogStatusNotification( - chargingStation, - UploadLogStatusEnumType.Uploading, - requestId - ) + const stationState = this.getOrCreateStationState(chargingStation) + const abortController = new AbortController() + stationState.activeLogUploadAbortController = abortController + stationState.activeLogUploadRequestId = requestId + stationState.activeLogUploadStatus = UploadLogStatusEnumType.Uploading + try { + await this.sendLogStatusNotification( + chargingStation, + UploadLogStatusEnumType.Uploading, + requestId + ) - await sleep(OCPP20Constants.LOG_UPLOAD_STEP_DELAY_MS) - await this.sendLogStatusNotification( - chargingStation, - UploadLogStatusEnumType.Uploaded, - requestId - ) + await interruptibleSleep(OCPP20Constants.LOG_UPLOAD_STEP_DELAY_MS, abortController.signal) + if (!abortController.signal.aborted && stationState.activeLogUploadRequestId === requestId) { + stationState.activeLogUploadStatus = UploadLogStatusEnumType.Uploaded + await this.sendLogStatusNotification( + chargingStation, + UploadLogStatusEnumType.Uploaded, + requestId + ) - logger.info( - `${chargingStation.logPrefix()} ${moduleName}.simulateLogUploadLifecycle: Log upload simulation completed for requestId ${requestId.toString()}` - ) + logger.info( + `${chargingStation.logPrefix()} ${moduleName}.simulateLogUploadLifecycle: Log upload simulation completed for requestId ${requestId.toString()}` + ) + } + } finally { + this.clearActiveLogUpload(chargingStation, requestId) + } } /** diff --git a/src/charging-station/ocpp/OCPPIncomingRequestService.ts b/src/charging-station/ocpp/OCPPIncomingRequestService.ts index 93c29087..06904d96 100644 --- a/src/charging-station/ocpp/OCPPIncomingRequestService.ts +++ b/src/charging-station/ocpp/OCPPIncomingRequestService.ts @@ -99,7 +99,6 @@ export abstract class OCPPIncomingRequestService extends EventEmitter { response = incomingRequestHandler(chargingStation, commandPayload) as ResType } } catch (error) { - // Log logger.error( `${chargingStation.logPrefix()} ${this.moduleName}.incomingRequestHandler: Handle incoming request error:`, error @@ -123,7 +122,6 @@ export abstract class OCPPIncomingRequestService extends EventEmitter { commandPayload ) } - // Send the built response await chargingStation.ocppRequestService.sendResponse( chargingStation, messageId, @@ -136,8 +134,18 @@ export abstract class OCPPIncomingRequestService extends EventEmitter { } } + /** + * Stops the incoming-request service for the given charging station. + * @param chargingStation - Target charging station. + */ public abstract stop (chargingStation: ChargingStation): void + /** + * Whether the given incoming-request command is supported for this station. + * @param chargingStation - Target charging station. + * @param commandName - OCPP incoming-request command name. + * @returns `true` when the command is supported. + */ protected abstract isIncomingRequestCommandSupported ( chargingStation: ChargingStation, commandName: IncomingRequestCommand @@ -155,7 +163,7 @@ export abstract class OCPPIncomingRequestService extends EventEmitter { * @param chargingStation - The charging station instance processing the request * @param commandName - OCPP command name to validate against * @param payload - JSON payload to validate - * @returns True if payload validation succeeds, false otherwise + * @returns `true` when payload validation succeeds; `false` otherwise. */ // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters protected validateIncomingRequestPayload( diff --git a/src/charging-station/ocpp/OCPPRequestService.ts b/src/charging-station/ocpp/OCPPRequestService.ts index e30b5f35..79329988 100644 --- a/src/charging-station/ocpp/OCPPRequestService.ts +++ b/src/charging-station/ocpp/OCPPRequestService.ts @@ -76,6 +76,14 @@ export abstract class OCPPRequestService { return OCPPRequestService.instances.get(this) as T } + /** + * Sends an OCPP request and awaits its response. + * @param chargingStation - Target charging station. + * @param commandName - OCPP request command name. + * @param commandParams - Optional request payload. + * @param params - Optional request behavior parameters. + * @returns Response payload from the Central System. + */ // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters public abstract requestHandler( chargingStation: ChargingStation, @@ -91,7 +99,6 @@ export abstract class OCPPRequestService { commandName: IncomingRequestCommand | RequestCommand ): Promise { try { - // Send error message return await this.internalSendMessage( chargingStation, messageId, @@ -117,7 +124,6 @@ export abstract class OCPPRequestService { commandName: IncomingRequestCommand ): Promise { try { - // Send response message return await this.internalSendMessage( chargingStation, messageId, @@ -208,7 +214,7 @@ export abstract class OCPPRequestService { * @param chargingStation - The charging station instance sending the request * @param commandName - OCPP command name to validate against * @param payload - JSON payload to validate - * @returns True if payload validation succeeds, false otherwise + * @returns `true` when payload validation succeeds; `false` otherwise. */ // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters protected validateRequestPayload( @@ -256,7 +262,6 @@ export abstract class OCPPRequestService { } // Request case MessageType.CALL_MESSAGE: - // Build request this.validateRequestPayload(chargingStation, commandName, messagePayload as JsonType) messageToSend = JSON.stringify([ messageType, @@ -267,7 +272,6 @@ export abstract class OCPPRequestService { break // Response case MessageType.CALL_RESULT_MESSAGE: - // Build response this.validateIncomingRequestResponsePayload( chargingStation, commandName, @@ -308,7 +312,6 @@ export abstract class OCPPRequestService { ) { // eslint-disable-next-line @typescript-eslint/no-this-alias -- stable outer-this reference captured for nested Promise executor and its response-handler closures const self = this - // Send a message through wsConnection return await new Promise((resolve, reject: (reason?: unknown) => void) => { /** * Function that will receive the request's response @@ -322,7 +325,6 @@ export abstract class OCPPRequestService { MessageType.CALL_RESULT_MESSAGE ) } - // Handle the request's response self.ocppResponseService .responseHandler( chargingStation, @@ -383,7 +385,6 @@ export abstract class OCPPRequestService { params.skipBufferingOnError === true && messageType === MessageType.CALL_MESSAGE ) { - // Remove request from the cache chargingStation.requests.delete(messageId) } reject(ocppError) @@ -399,7 +400,6 @@ export abstract class OCPPRequestService { messageType, commandName ) - // Check if wsConnection opened if (chargingStation.isWebSocketConnectionOpened()) { const beginId = PerformanceStatistics.beginMeasure(commandName) const sendTimeout = setTimeout(() => { diff --git a/src/charging-station/ocpp/OCPPResponseService.ts b/src/charging-station/ocpp/OCPPResponseService.ts index 224226ec..34d6c0ea 100644 --- a/src/charging-station/ocpp/OCPPResponseService.ts +++ b/src/charging-station/ocpp/OCPPResponseService.ts @@ -112,6 +112,12 @@ export abstract class OCPPResponseService { } } + /** + * Whether the given request command is supported for this station. + * @param chargingStation - Target charging station. + * @param commandName - OCPP request command name. + * @returns `true` when the command is supported. + */ protected abstract isRequestCommandSupported ( chargingStation: ChargingStation, commandName: RequestCommand @@ -133,7 +139,7 @@ export abstract class OCPPResponseService { * @param chargingStation - The charging station instance receiving the response * @param commandName - OCPP command name to validate against * @param payload - JSON response payload to validate - * @returns True if payload validation succeeds, false otherwise + * @returns `true` when payload validation succeeds; `false` otherwise. */ // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters protected validateResponsePayload( diff --git a/src/charging-station/ocpp/OCPPServiceUtils.ts b/src/charging-station/ocpp/OCPPServiceUtils.ts index 85159883..a7fad987 100644 --- a/src/charging-station/ocpp/OCPPServiceUtils.ts +++ b/src/charging-station/ocpp/OCPPServiceUtils.ts @@ -61,6 +61,7 @@ import { handleFileException, isNotEmptyArray, isNotEmptyString, + isValidRandomIntBounds, JSONStringify, logger, logPrefix, @@ -170,7 +171,7 @@ export const ajvErrorsToErrorType = (errors: ErrorObject[] | null | undefined): * @param validate - Ajv validation function for the command * @param context - Description of the validation context (e.g. 'request', 'response') * @param clonePayload - Whether to clone payload and convert dates before validation - * @returns True if payload validation succeeds, false otherwise + * @returns `true` when payload validation succeeds; `false` otherwise. */ // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters export const validatePayload = ( @@ -262,12 +263,20 @@ const buildSocMeasurandValue = ( const socMaximumValue = Constants.SOC_MAXIMUM_PERCENT const socMinimumValue = socSampledValueTemplate.minimumValue ?? 0 - const socSampledValueTemplateValue = isNotEmptyString(socSampledValueTemplate.value) - ? getRandomFloatFluctuatedRounded( + let socSampledValueTemplateValue: number + if (isNotEmptyString(socSampledValueTemplate.value)) { + socSampledValueTemplateValue = getRandomFloatFluctuatedRounded( convertToInt(socSampledValueTemplate.value), socSampledValueTemplate.fluctuationPercent ?? Constants.DEFAULT_FLUCTUATION_PERCENT ) - : randomInt(socMinimumValue, socMaximumValue + 1) + } else if (isValidRandomIntBounds(socMinimumValue, socMaximumValue)) { + socSampledValueTemplateValue = randomInt(socMinimumValue, socMaximumValue + 1) + } else { + logger.warn( + `${chargingStation.logPrefix()} ${moduleName}.buildSocMeasurandValue: invalid SoC bounds socMinimumValue=${socMinimumValue.toString()}, socMaximumValue=${socMaximumValue.toString()} — skipping SoC measurand` + ) + return null + } return { template: socSampledValueTemplate, @@ -1066,6 +1075,15 @@ const createVersionedSampledValueDispatcher = ( const warnedInvalidMeasurands = new WeakMap>() const KNOWN_MEASURANDS: ReadonlySet = new Set(Object.values(MeterValueMeasurand)) +const getOrCreateWarnedMeasurands = (chargingStation: ChargingStation): Set => { + let warned = warnedInvalidMeasurands.get(chargingStation) + if (warned == null) { + warned = new Set() + warnedInvalidMeasurands.set(chargingStation, warned) + } + return warned +} + /** * Resolves the set of measurands enabled by the configured OCPP variable. * @@ -1118,11 +1136,7 @@ const resolveEnabledMeasurands = ( enabled.add(trimmed as MeterValueMeasurand) continue } - let warned = warnedInvalidMeasurands.get(chargingStation) - if (warned == null) { - warned = new Set() - warnedInvalidMeasurands.set(chargingStation, warned) - } + const warned = getOrCreateWarnedMeasurands(chargingStation) if (!warned.has(trimmed)) { warned.add(trimmed) logger.warn( diff --git a/src/charging-station/ocpp/auth/services/OCPPAuthServiceImpl.ts b/src/charging-station/ocpp/auth/services/OCPPAuthServiceImpl.ts index 7fd3d4fa..11d0f1d8 100644 --- a/src/charging-station/ocpp/auth/services/OCPPAuthServiceImpl.ts +++ b/src/charging-station/ocpp/auth/services/OCPPAuthServiceImpl.ts @@ -63,7 +63,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { this.strategies = new Map() this.strategyPriority = ['local', 'remote', 'certificate'] - // Initialize metrics tracking this.metrics = { cacheHits: 0, cacheMisses: 0, @@ -76,7 +75,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { totalResponseTime: 0, } - // Initialize default configuration this.config = this.createDefaultConfiguration() // Note: Adapter and strategies will be initialized async via initialize() @@ -91,7 +89,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { const startTime = Date.now() let lastError: Error | undefined - // Update request metrics this.metrics.totalRequests++ logger.debug( @@ -132,7 +129,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { const duration = Date.now() - startTime - // Update metrics based on result this.updateMetricsForResult(result, strategyName, duration) logger.info( @@ -173,7 +169,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { const duration = Date.now() - startTime const errorMessage = lastError?.message ?? 'All authentication strategies failed' - // Update failure metrics this.metrics.failedAuth++ this.metrics.totalResponseTime += duration @@ -436,7 +431,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { * @returns True if at least one strategy can handle the identifier type, false otherwise */ public isSupported (identifier: Identifier): boolean { - // Create a minimal request to check applicability const testRequest: AuthRequest = { allowOffline: false, connectorId: 1, @@ -461,7 +455,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { return false } - // Check if adapter reports remote availability if (this.adapter) { try { if (this.adapter.isRemoteAvailable()) { @@ -538,13 +531,10 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { * @throws {OCPPError} If configuration validation fails */ public updateConfiguration (config: Partial): void { - // Merge new config with existing const newConfiguration = { ...this.config, ...config } - // Validate merged configuration AuthConfigValidator.validate(newConfiguration) - // Apply validated configuration this.config = newConfiguration logger.info( @@ -667,7 +657,6 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { ].includes(error.code) } - // Check for specific error patterns that indicate critical issues const criticalPatterns = [ 'SECURITY_VIOLATION', 'CERTIFICATE_EXPIRED', @@ -691,21 +680,18 @@ export class OCPPAuthServiceImpl implements OCPPAuthService { ): void { this.metrics.totalResponseTime += duration - // Track successful vs failed authentication if (result.status === AuthorizationStatus.ACCEPTED) { this.metrics.successfulAuth++ } else { this.metrics.failedAuth++ } - // Track strategy usage if (strategyName === 'local') { this.metrics.localAuthCount++ } else if (strategyName === 'remote') { this.metrics.remoteAuthCount++ } - // Track cache hits/misses based on method if (result.method === AuthenticationMethod.CACHE) { this.metrics.cacheHits++ } else if ( diff --git a/src/charging-station/ui-server/AbstractUIServer.ts b/src/charging-station/ui-server/AbstractUIServer.ts index 6fbd884a..ea44d4b6 100644 --- a/src/charging-station/ui-server/AbstractUIServer.ts +++ b/src/charging-station/ui-server/AbstractUIServer.ts @@ -312,8 +312,16 @@ export abstract class AbstractUIServer { ?.requestHandler(request, { origin: UIRequestOrigin.INTERNAL }) as Promise) } + /** + * Sends a UI protocol request to the connected UI client. + * @param request - Protocol request to send. + */ public abstract sendRequest (request: ProtocolRequest): void + /** + * Sends a UI protocol response to the connected UI client. + * @param response - Protocol response to send. + */ public abstract sendResponse (response: ProtocolResponse): void public setChargingStationData (hashId: string, data: ChargingStationData): boolean { diff --git a/src/performance/storage/Storage.ts b/src/performance/storage/Storage.ts index 22b91990..b3cac497 100644 --- a/src/performance/storage/Storage.ts +++ b/src/performance/storage/Storage.ts @@ -24,14 +24,27 @@ export abstract class Storage { this.logPrefix = logPrefix } + /** + * Closes the storage, releasing any underlying handles. + * @returns Promise that resolves once closed, or `void` for synchronous implementations. + */ public abstract close (): Promise | void public getPerformanceStatistics (): IterableIterator { return Storage.performanceStatistics.values() } + /** + * Opens the storage, acquiring any underlying handles. + * @returns Promise that resolves once open, or `void` for synchronous implementations. + */ public abstract open (): Promise | void + /** + * Persists a performance statistics snapshot. + * @param performanceStatistics - Statistics snapshot to persist. + * @returns Promise that resolves once the snapshot is stored, or `void` for synchronous implementations. + */ public abstract storePerformanceStatistics ( performanceStatistics: Statistics ): Promise | void diff --git a/src/utils/Constants.ts b/src/utils/Constants.ts index 760a6e77..2de368ee 100644 --- a/src/utils/Constants.ts +++ b/src/utils/Constants.ts @@ -134,7 +134,7 @@ export class Constants { static readonly ENV_SIMULATOR_COLD_START = 'SIMULATOR_COLD_START' - static readonly MAX_RANDOM_INTEGER = 281_474_976_710_655 // 2^48 - 1 (randomInit() limit) + static readonly MAX_RANDOM_INTEGER = 281_474_976_710_655 // 2^48 - 1 (randomInt() limit) // Node.js setInterval/setTimeout maximum safe delay value (2^31-1 ms ≈ 24.8 days) // Values exceeding this limit cause Node.js to reset the delay to 1ms diff --git a/src/utils/Utils.ts b/src/utils/Utils.ts index cd1361f6..144a97a4 100644 --- a/src/utils/Utils.ts +++ b/src/utils/Utils.ts @@ -163,6 +163,34 @@ export const sleep = async (milliSeconds: number): Promise => { }) } +/** + * Sleeps for the specified duration or resolves early when the signal + * aborts. Both the `setTimeout` handle and the `'abort'` listener are + * cleaned up on whichever path resolves the promise first so the caller + * cannot leak a pending `Timeout` or a stale listener. Resolves (does + * not reject) on abort — callers observe the abort by rechecking their + * own loop condition after the promise settles. + * @param milliSeconds - Duration to sleep in milliseconds. + * @param signal - `AbortSignal` that resolves the promise early on abort. + * @returns Promise that resolves when the timer fires or the signal aborts. + */ +export const interruptibleSleep = (milliSeconds: number, signal: AbortSignal): Promise => + new Promise(resolve => { + if (signal.aborted) { + resolve() + return + } + const timeout = setTimeout(() => { + signal.removeEventListener('abort', onAbort) + resolve() + }, milliSeconds) + const onAbort = (): void => { + clearTimeout(timeout) + resolve() + } + signal.addEventListener('abort', onAbort, { once: true }) + }) + /** * Races a promise against a timeout. Resolves/rejects with the promise result * if it settles before the deadline, otherwise rejects with a timeout error. @@ -231,6 +259,23 @@ export const isValidDate = (date: Date | number | undefined): date is Date | num return false } +/** + * Predicate for a min/max pair intended as input to + * `randomInt(minValue, maxValue + 1)` (from `node:crypto`). Rejects + * `NaN`, `Infinity`, non-integer floats, negative values, and ranges + * where `maxValue - minValue >= 2^48 - 1` — all of which would cause + * `randomInt` to throw `RangeError`. + * @param minValue - Lower bound (inclusive; must be a safe integer >= 0). + * @param maxValue - Upper bound (inclusive; must be a safe integer >= minValue). + * @returns `true` when the bounds are safe; `false` otherwise. + */ +export const isValidRandomIntBounds = (minValue: number, maxValue: number): boolean => + Number.isSafeInteger(minValue) && + Number.isSafeInteger(maxValue) && + minValue >= 0 && + minValue <= maxValue && + maxValue - minValue < 2 ** 48 - 1 + export const convertToDate = ( value: Date | null | number | string | undefined ): Date | undefined => { diff --git a/src/utils/index.ts b/src/utils/index.ts index 96c38f5d..17d02bfb 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -85,6 +85,7 @@ export { getWebSocketCloseEventStatusString, has, insertAt, + interruptibleSleep, isArraySorted, isAsyncFunction, isCFEnvironment, @@ -93,6 +94,7 @@ export { isNotEmptyArray, isNotEmptyString, isValidDate, + isValidRandomIntBounds, JSONStringify, logPrefix, mergeDeepRight, -- 2.53.0