fix(webui): keep modern station-card Delete action aligned on the footer row (#2083)
The modern station-card footer laid the 5 action buttons in a non-wrapping
inline-flex group beside the Delete button, inside a wrapping footer. Once the
group's max-content width exceeded the card, the footer broke the line between
the group and Delete, dropping Delete onto its own row where margin-left:auto
pinned it bottom-right (orphaned) instead of aligned with the other actions.
Make the footer a single flex line (flex-wrap: nowrap) and let the action
group wrap its buttons internally (flex-wrap: wrap) so it reflows across rows
on narrow cards while Delete stays on the same row, pinned right. align-self:
flex-start top-aligns Delete with the first action row when the group wraps.
Presentational modern-skin CSS only; no markup or handler change.
fix(simulator): seed autoRegister and ocppProtocol defaults into stationInfo (#2082)
* fix(simulator): seed autoRegister and ocppProtocol defaults into stationInfo
autoRegister and ocppProtocol had no default, so a template omitting them
left stationInfo carrying undefined for both. Raw consumers of stationInfo
— the UI data payload (buildChargingStationDataPayload) and the persisted
configuration — therefore received undefined, so the Web UI station details
showed an empty placeholder for Auto Register and OCPP Protocol.
Both are static defaults (no derivation), so seed them in DEFAULT_STATION_INFO
(autoRegister: false, ocppProtocol: OCPPProtocol.JSON) alongside currentOutType
and ocppVersion. getStationInfo applies them via
mergeDeepRight(DEFAULT_STATION_INFO, stationInfo), so an explicit template/file
value or option still wins (idempotent, no clobber) and a persisted config
predating the fields is backfilled on reload. Export OCPPProtocol from the
types barrel to keep Constants.ts importing types from a single source, as
OCPPVersion already does.
Runtime-neutral: ocppProtocol has no runtime read; the autoRegister === true
guards are unaffected by false vs undefined; the getHeartbeatInterval warn
guarded by === false is unreachable in the initialized pipeline because
initializeOcppConfiguration always seeds the HeartbeatInterval key first.
* refactor(test): extract persisted-config resolution into realStation helpers
The persisted configuration file/dir resolution from a real-station template
was inlined and duplicated across three suites (AutoRegisterOcppProtocol,
NumberOfPhases, ResetIdentity). Extract persistedConfigurationDir and
resolvePersistedConfigurationFile into StationHelpers.realStation.ts (single
source of truth for the temp-dir layout) and migrate all three call sites.
resolvePersistedConfigurationFile throws a descriptive error when no config
exists yet, replacing the silent `?? ''` fallback (which degraded into EISDIR).
Group the helper by concern (temp-dir lifecycle / construction / persisted-config
resolution) with section headers, and document the two new helpers in
TEST_STYLE_GUIDE.md.
* docs(test): harmonize helper-table param placeholders in TEST_STYLE_GUIDE
test(simulator): drop manual test counters from worker broadcast-channel test (#2081)
The group-header comments carried hand-maintained (N tests) counters that rot
as tests are added or removed; the test runner already reports the count.
Remove them, keeping the descriptive group headers.
fix(webui): polish the station details view (panel placement + section/key-value separation) (#2076)
CSS/Vue-only polish of the #993 Show details view: classic panel-placement fix + section/key-value separation; modern section panels, promoted titles and tighter symmetric row separators; not-found escape; toggle-navigation test coverage.
chore(ui-server): remove deprecated HTTP UI protocol Insomnia collection (#2070)
The HTTP UI transport (UIHttpServer / ApplicationProtocol.HTTP) is
deprecated in favor of the MCP transport and will be removed. Drop its
Insomnia requests collection and the README pointer to it; the WebSocket
collection and the deprecation notice are kept.
fix(ui-server): reconcile evses wire type so /metrics works on OCPP 2.0.x (#2048)
* fix(ui-server): reconcile evses wire type so /metrics works on OCPP 2.0.x
The worker->main producer `buildEvseEntries` emits `evseStatus.connectors`
as a `ConnectorEntry[]` array, but `ChargingStationData.evses` was typed
with the in-memory `EvseStatus` (connectors: Map), hidden by an
`as unknown as EvseEntry[]` cast. On a live scrape the metrics consumer
`iterateConnectors` tuple-destructured each array element as a Map entry,
throwing `TypeError: ... is not iterable` inside prom-client's
`Registry.metrics()` -> HTTP 500 for every OCPP 2.0.x station.
`countConnectors` also read `.size` on the array -> NaN. OCPP 1.6 was
unaffected because its `data.connectors` path already yields ConnectorEntry
objects without destructuring.
Introduce dedicated wire types `EvseEntryData`/`EvseStatusData`
(connectors: ConnectorEntry[]), type `ChargingStationData.evses` and
`buildEvseEntries` with them, and drop the cast so the compiler enforces
the array shape end to end. `iterateConnectors` now yields the entry
directly and `countConnectors` uses `.length`. The array shape matches what
the Web UI and CLI already consume over the JSON wire; a Map would not
survive JSON serialization. The in-memory `EvseStatus` (connectors: Map)
is unchanged.
Add a regression test feeding the actual `buildEvseEntries` output through
the /metrics scrape (fails pre-fix with 500/NaN, passes post-fix) plus
zero-id and empty-evses edge cases, and migrate the existing EVSE-mode test
off its synthetic Map fixture that masked the defect.
Closes #2046
* docs(types): document EvseEntryData/EvseStatusData wire projection
Add JSDoc on the wire types introduced for #2046:
- EvseEntryData: anchor its relation to the in-memory EvseEntry (Map) and to
the structurally-identical ui-common EvseEntry (duplicated, no cross-package
re-export), removing the naming ambiguity.
- EvseStatusData: document that MeterValues is intentionally omitted (the
producer never emits it, no UI-facing consumer reads it off the wire),
resolving the PR review comment on the omission.
Doc-only; no type or runtime change.
* test(ui-server): harden #2046 metrics regression guards
Address review findings on the #2046 regression tests:
- Convert the "empty evses" test to a populated EVSE with an empty connectors
array. The zero-EVSE case short-circuits `reduce` (returns 0 even pre-fix),
so it guarded nothing; a populated EVSE with `connectors: []` is the case
that pre-fix reads `.size` on an array -> undefined -> NaN. Verified: fails
pre-fix (connectors_total Nan), passes post-fix (0).
- Type the buildEvseEntries stub as `Pick<ChargingStation, 'iterateEvses'>`
instead of `as unknown as`, so `iterateEvses`/`evseStatus` stay type-checked
(resolves the PR review comment on the stub cast).
- Use `satisfies` instead of `as` on the migrated EVSE-mode fixture, so a
Map/array shape drift is caught at compile time (the unchecked-cast class
that masked the original bug).
* refactor(types): make EvseStatusData an immutable wire snapshot
Mark EvseStatusData.availability and connectors readonly (and connectors a
readonly ConnectorEntry[]), matching the readonly EvseEntryData wrapper. The
wire projection is a produced-once snapshot never mutated by any consumer
(countConnectors reads .length, iterateConnectors iterates read-only), so
immutability is the accurate contract and removes the wrapper/payload
readonly asymmetry. In-memory EvseStatus (mutable Map) is unchanged.
test(ocpp-server): add OCPP 1.6 mock server and extend reservation coverage (#2047)
* test(ocpp-server): add OCPP 1.6 mock server and extend reservation coverage
- Add server16.py: standalone OCPP 1.6 mock server mirroring server.py
architecture (AuthConfig, ServerConfig, ChargePoint, on_connect, main)
- Add test_server16.py: 59 tests covering handlers, outgoing commands,
signed meter values, timer, on_connect and main (~96% branch coverage)
- Add _send_reserve_now/_send_cancel_reservation to server.py (OCPP 2.0.1)
with matching ServerConfig fields and DEFAULT_RESERVATION_* constants
- Extend test_server.py: tests for ReserveNow/CancelReservation commands,
entries in EXPECTED_OUTGOING_COMMANDS and FAILURE_PATH_CASES, strengthen
timestamp assertions (fromisoformat + tzinfo check)
- Update pyproject.toml: server16 run task, typecheck and coverage source
- Add tools/http_broadcast_timing.py and tools/reused_ws_request_id.py:
ad-hoc diagnostic scripts for UI-server fixes #2037 and #2033
* chore(serena): update project knowledge base
* revert: remove tools/ from tracked files (kept locally)
* fix(ocpp-server): address review findings M1-M3 and m1-m8
M1 - test_boot_status_sequence_parsed: reuse _patch_main (add
**args_overrides), capture ServerConfig via side_effect, assert
boot_sequence == (pending, accepted)
M2 - DRY: extract check_positive_number to _common.py; update
typecheck task, coverage source, and mypy override to include it
M3 - README: rename to 'OCPP Mock Servers', add Running the Servers
section, add full OCPP 1.6 server documentation (20 CLI flags,
supported commands, transaction tracking)
m1 - remove dead DEFAULT_VENDOR_ID constant (server16.py)
m2 - align --reservation-id flag (server.py: --reserve-id ->
--reservation-id) and fix coupled Namespace in test_server.py
m3 - _call_and_log: log response.status on failure (server.py)
m4 - pyproject.toml: add test_server16 to mypy disallow_untyped_defs
override
m5 - add type hints to on_start_transaction, on_stop_transaction,
on_status_notification handlers (server16.py)
m6 - add test_windows_handler_schedules_via_call_soon_threadsafe
(test_server16.py)
m7 - document intentional 1.6 command scope in _COMMAND_HANDLERS
m8 - remove dead AuthMode.offline member (server.py)
INFO - boot_index: clarify shared-across-all-stations behavior
INFO - magic number 128: add Unix convention comment
INFO - argparse: 'OCPP2 Server' -> 'OCPP 2.0.1 Server' (server.py)
INFO - _parse_commands: raw_entry/entry variable naming (server.py)
Coverage tests: test_custom_auth_config, token vide x2,
test_meter_values_with_signed_meter_value (test_server16.py)
Quality gates: 335 passed, format/lint/typecheck clean, 94.78%
* fix(ocpp-server): address post-review findings N1-N6
N1/M2 - server16.py: import check_positive_number from _common, remove
local duplicate (567-576); _common.py now has a single consumer
N1/m1 - server16.py: remove dead DEFAULT_VENDOR_ID constant
N1/m5 - server16.py: add type hints to on_start_transaction
(connector_id, meter_start: int; id_tag: str), on_stop_transaction
(meter_stop, transaction_id: int), on_status_notification
(connector_id: int; error_code, status: str)
N1/m7 - server16.py: document intentional subset scope above
_COMMAND_HANDLERS with runtime behavior note
N1/INFO - server16.py: add Unix 128+signal comment before sys.exit;
expand boot_index one-liner to full NOTE (shared across ALL stations,
not per-station)
N2 - test_server.py: extend _patch_main with **args_overrides (same
pattern as test_server16.py); add test_boot_status_sequence_parsed in
TestMainGracefulShutdown — covers server.py:1134-1149
N3 - test_server16.py: add test_boot_status_single_value — covers
server16.py:786-787 (elif --boot-status single value branch)
N4 - pyproject.toml: update description to cover both 1.6 and 2.0.1
N5 - README.md: remove duplicated sentence at line 38
N6 - test_server16.py: fix RuntimeWarning (coroutine never awaited)
in test_commands_sequence_scheduled_on_connect — mock_cp.send_commands
was AsyncMock, its coroutine was passed to mocked create_task unrun;
replace with MagicMock() to prevent the orphan coroutine
Quality gates: 337 passed (0 warnings), format/lint/typecheck clean,
coverage 95.95% (+1.17pp)
* test(charging-station): relax template file count assertion
Replace strictEqual(15) with ok(count >= 15) so adding new station
templates does not require updating the hardcoded count.
* fix(ocpp-server): address round-3 review findings
MAJOR-1 - README.md 2.0.1: add ReserveNow and CancelReservation to
Outgoing Commands list; add --reservation-id, --reserve-id-token,
--reserve-evse-id flags to Command-specific options; add example command
MAJOR-2a - test_server.py: add caplog assertions to 4 signed MeterValue
tests (started/updated/ended/meter_values) — verify _log_signed_meter_values
actually logs, not just that the handler returns correct type
MAJOR-2b - test_server.py: rewrite test_unsupported_command_logs_warning
with caplog + assert 'not supported' in log output
MAJOR-2c - test_server.py: align 7 empty-response handlers on equality
pattern (== EmptyResult()) on top of isinstance — matches test_server16
MINOR-1 - _common.py: add generic parse_commands(str, type[ActionT])
factored from the two identical _parse_commands; both servers now use
a one-line wrapper; remove import math from server16/server (now in
_common only); coverage 96.76% (+0.81pp)
MINOR-2 - server.py: add intentional-subset scope comment above
_COMMAND_HANDLERS (mirrors server16.py:451-453)
MINOR-3 - server.py:342: (kwargs.get('evse') or {}).get('id', 0)
prevents AttributeError when payload carries evse=null explicitly;
add regression test test_transaction_event_started_null_evse_defaults_to_zero
MINOR-4 - test_server.py: port 3 missing tests from test_server16:
- test_unexpected_error_is_caught (covers server.py:782-783)
- test_commands_sequence_scheduled_on_connect (covers server.py:866)
- test_boot_status_single_value via main() (covers server.py:1151)
MINOR-5 - README.md 1.6: enumerate valid --trigger-message types
(StatusNotification, BootNotification, Heartbeat, MeterValues,
FirmwareStatusNotification, DiagnosticsStatusNotification,
LogStatusNotification, SignChargePointCertificate)
Quality gates: 341 passed (0 warnings), format/lint/typecheck clean,
coverage 96.76% (+0.81pp from 95.95%)
* fix(ocpp-server): address round-4 review findings
MAJOR-A - README.md 2.0.1: replace invalid --trigger-message value
SignCertificate with the 11 exact MessageTriggerEnumType members
(SignChargingStationCertificate/SignV2GCertificate/SignCombinedCertificate
+ TransactionEvent + PublishFirmwareStatusNotification); the 1.6 list was
fixed in round-3, this is its 2.0.1 counterpart
MAJOR-B - test_server.py: add TestLogSignedMeterValues (3 tests) adapted
to 2.0.1 semantics (signed_meter_value field, not format=SignedData);
covers the previously-uncovered unsigned branch server.py:97->95
MINOR-C - test_server.py + test_server16.py: 6 error-handling tests
(timeout/ocpp_error/unexpected) upgraded from swallowing-only to caplog
assertions on level ERROR + message substring
MINOR-D - DRY: extract negotiate_subprotocol + install_signal_handlers_and_wait
to _common.py (version-agnostic lifecycle, logger passed explicitly to
preserve observability); both servers use them; remove import signal from
both servers; retarget 4 signal.signal patches to _common. Instance methods
(_call_and_log/_send_command/...) and DEFAULT_RESERVATION_* constants left
in place (OCPP coupling / locality cost > DRY gain for a test mock)
MINOR-E - _common.py to 100%: add non-numeric-delay and empty-entry tests
for _parse_commands in both suites
INFO MORT-1 - remove unreachable dead code (if not boot_sequence) in both
servers' main()
INFO-D - server.py: handle_connection_closed(self) -> None annotation
Quality gates: 348 passed (0 warnings), format/lint/typecheck clean,
coverage 97.51% (+0.75pp); _common.py 100%
* test(ocpp-server): close round-5 parity + config findings
MINOR parity - test_server.py: assert len(config.charge_points) == 0 on
both on_connect rejection tests (missing subprotocol / protocol mismatch),
mirroring test_server16.py; guards against a regression registering a
ChargePoint on a rejected connection in 2.0.1
MINOR parity - test_server.py: strengthen Heartbeat test_returns_current_time
with a freshness window (delta < 60s) + import timezone, mirroring the 1.6
suite (was isinstance + tzinfo only)
INFO I4 - pyproject.toml: mypy python_version 3.14 -> 3.12 to match the
requires-python floor (>=3.12); mypy stays clean (verified live)
INFO I5 - README.md: add the --boot-status shorthand sentence to the 1.6
section for parity with 2.0.1 (identical behavior)
Deliberately NOT changed (accepted debt, evidence-based):
- DRY mixin for send_command/send_commands/handle_connection_closed: moving
them to _common would emit lifecycle logs under _common instead of
server/server16 (silent observability regression) for ~24 lines in a mock
- mypy test override kept: real guard (test files carry untyped defs)
- docstrings on _common helpers: 0/4 documented, keeping style consistent
Quality gates: 348 passed, format/lint/typecheck clean (python 3.12),
coverage 97.51%
* test(ocpp-server): close round-6 parity + hygiene findings
MINOR-1 - test_server16.py: add test_parent_id_tag_absent_by_default,
mirroring 2.0.1 test_authorize_no_enrichment_by_default; asserts
parent_id_tag is absent from id_tag_info under the default AuthConfig
(closes the last inter-suite rigor asymmetry)
MINOR-2 - server.py: harmonize _call_and_log docstring with server16.py
(both now '...based on its status')
MINOR-3 - test_server.py: add test_parse_set_variable_specs_skips_empty_entries
and test_parse_get_variable_specs_invalid_no_dot, covering the two
previously-uncovered branches of _parse_variable_specs (empty-entry skip,
require_value=False missing-dot); server.py 875/885 now covered
I1 - server.py + server16.py: derive --auth-mode choices/default from the
local AuthMode StrEnum ([mode.value for mode in AuthMode] / AuthMode.normal.value)
instead of string literals (AGENTS.md: avoid string literals when an
enumeration exists); order/content identical (verified live), no behavior change
Quality gates: 351 passed, format/lint/typecheck clean (python 3.12),
coverage 97.88% (server.py 98%)
* test(ocpp-server): close round-7 style + DRY findings
- test_server.py: fix stale docstring (20 -> 24 _send_* methods)
- server.py/server16.py: --auth-mode type=str -> type=AuthMode to match
the sibling enum-typed CLI args (--boot-status, --reset-type, ...); keep
choices for unchanged help/UX
- server.py/server16.py: extract DEFAULT_WHITELIST/DEFAULT_BLACKLIST module
constants (single source of truth; removes __init__/argparse duplication)
- test_server.py/test_server16.py: TestHandlerCoverage now asserts @on
registration (_on_action) so an undecorated handler is caught
* test(ocpp-server): close round-8 findings (harmonize auth-mode, DRY, handler routing)
- _common.py: host DEFAULT_WHITELIST/DEFAULT_BLACKLIST as the single source
of truth; server.py and server16.py now import them (removes cross-file
literal duplication)
- server.py/server16.py: drop now-dead choices=list(AuthMode) from --auth-mode
and move valid values into the help text, matching the sibling enum-typed
args (--boot-status, --reset-type, ...); uniform invalid-value error message
- server.py/server16.py: mode=args.auth_mode (args value is already an AuthMode
via type=AuthMode; drop redundant re-wrap)
- test_server.py/test_server16.py: TestHandlerCoverage now asserts each handler
is registered for the CORRECT OCPP action (_on_action == expected), not merely
decorated with some @on
* test(ocpp-server): address bot review comments on server16
- test_server16.py: use supported CSMS->CS commands (TriggerMessage/Reset)
instead of ClearCache/Heartbeat in the send_commands sequencing and
_parse_commands tests, so the fixtures reflect commands actually in
_COMMAND_HANDLERS rather than unsupported outgoing actions
- server.py/server16.py: replace the empty `except KeyboardInterrupt: pass`
with contextlib.suppress(KeyboardInterrupt) (clears the empty-except finding
without a comment; both servers for parity)
- server.py/server16.py: _resolve_auth_status returns default_status via an
unconditional trailing return instead of a wildcard `case _`, making the
control flow unambiguous to static analysis (both servers for parity)
fix(ui-server): defer HTTP broadcast responses until worker replies aggregate (#2028) (#2037)
The deprecated HTTP UI transport emitted a synthetic `success` the instant the
request handler resolved `undefined` (the broadcast-pending signal), without
awaiting worker replies. PR #2020's Set-based aggregation therefore covered the
WebSocket and MCP transports but not HTTP: a fan-out command reported
`200`/`success` before any worker acted, and the aggregated
`hashIdsFailed`/`responsesFailed` never reached the client.
Align HTTP with the WebSocket and MCP transports (issue #2028 option 1): only
synchronous, non-broadcast procedures respond inline. A broadcast keeps its
already-registered response handler open so the later aggregated `sendResponse`
writes the real status and `hashIdsSucceeded`/`hashIdsFailed`, reusing the
existing 60s safety-net timeout (`UI_SERVER_BROADCAST_CHANNEL_REQUEST_TIMEOUT_MS`)
so a never-arriving aggregation yields a bounded failure with no hung socket.
Client-disconnect cleanup and server-side uuid minting are unchanged; no new
timeout or aggregation path is introduced.
This is a UI SRPC transport-layer fix only; no OCPP PDU/message format changes,
and no change to WebSocket/MCP behavior or the shared aggregation semantics.
feat(worker): terminate a deleted station's worker once it hosts zero elements (#2027) (#2038)
* feat(worker): terminate a deleted station's worker once it hosts zero elements (#2027)
Add an element-granular removeElement primitive to the worker abstraction and
wire it into the station-delete path so a deleted charging station's hosting
worker thread is terminated once it no longer hosts any station, without
disrupting sibling stations that share the worker (elementsPerWorker > 1).
- WorkerAbstract: new abstract removeElement(elementKey: PropertyKey).
- WorkerSet: internal PropertyKey -> WorkerSetElement map fed by an injected
generic elementKey selector; decrement numberOfWorkerElements and reuse a
factored terminateWorker helper (shared with stop()) to terminate only at
zero elements with no in-flight addition; purge the map on element removal.
- WorkerFixedPool/WorkerDynamicPool: documented no-op (poolifier exposes no safe
single-element eviction without destroying the worker and its siblings).
- Bootstrap: pass stationInfo => stationInfo.hashId; call removeElement from
workerEventDeleted.
Set-vs-pool decision: element-granular termination is workerSet-only; pool
worker threads are a bounded, reused resource and are not reclaimed per delete.
Cleanup/timeout contract: termination reuses the existing worker exit handler
for pending-promise rejection and set/map cleanup; any in-flight broadcast
request still resolves via the existing 60s aggregation timeout backstop.
Part A (#2031 cancellable reset) and bulk stop() are untouched.
* fix(worker): harden element-granular termination against concurrency races
Cross-validated review of the removeElement primitive surfaced two reachable
defects and one latent robustness issue, now fixed:
- Phantom-count leak on re-adding the same element key: numberOfWorkerElements
was incremented unconditionally, so a duplicate/re-added key inflated the
count and the worker was never terminated. Count is now key-aware (distinct
keys per worker); factored into trackAddedWorkerElement.
- New addition routed onto a terminating worker: getWorkerSetElement could
select a worker parked in terminateWorker, losing the message and rejecting
an unrelated add. Terminating workers are now flagged and excluded from
selection.
- getWorkerSetElementByWorker matched by threadId, which collapses to -1 after
terminate(); switched to worker object identity.
Tests: add coverage for the in-flight-sibling gate, drain-to-last-sibling,
add-during-terminate, same-key re-add, and unknown-key no-op; consolidate the
silent worker fixture into echoWorker via a `hold` flag. Mutation-verified.
* fix(worker): make element termination robust and tighten its types
Second cross-validated review round hardened the removeElement primitive:
- terminateWorker no longer awaits a re-attached 'exit' listener (redundant,
since worker.terminate() fulfils once the worker has exited, and it could
deadlock if 'exit' fired first). It now catches a rejected terminate()
(surfaced via the error event, not rethrown so the removal still succeeds)
and performs pending-promise rejection + set/map cleanup in a finally, closing
the zombie-on-reject leak, the orphaned-pending edge, and the stop-concurrent-
exit deadlock in one place.
- Drop the dead, semantically fictional migration branch in
trackAddedWorkerElement: duplicate live element keys cannot occur (identities
are deduplicated upstream), so a key is counted once per worker and a same-key
re-add is a no-op.
- Make WorkerSetElement.terminating a required boolean (initialized false) and
narrow the element key from PropertyKey to string, matching the sole caller
(hashId) and avoiding non-serializable keys.
Tests: add reject-path coverage (removeElement resolves and cleans up when
terminate() rejects; an in-flight addition is still rejected on stop when
terminate() rejects). Mutation-verified. All project gates green.
* test(worker): align WorkerSet internals view element key type to string
Coherence with the production narrowing of the element key from PropertyKey
to string; test-only, no runtime change.
* docs(worker): drop the worker-termination note from the README
refactor: code-quality cleanup — utils usage, logic consolidation, export hygiene (#2036)
* refactor(ui-server): use isEmpty for outstanding-hashid checks
* refactor(ui-server): extract releaseRequest for broadcast request release
* refactor(ocpp): lift requestHandler into base template method
* refactor(utils): add isOCPP20x helper and route version predicates
* refactor(charging-station): single-source the request-statistics gate
* refactor(utils): use isNotEmptyString/isNotEmptyArray predicates
* refactor: drop dead exports and redundant re-export
* refactor(auth): rename AuthorizationStatus enum to AuthResultStatus
* refactor(ocpp): drop now-redundant logRequestHandlerError module-name param
* docs(charging-station): document recordRequestStatistic public method
* refactor(ocpp): route VariableMetadata mutability/persistence checks through predicates
* refactor(ocpp): unexport in-file-only parseJsonSchemaFile
fix(charging-station): cancel a pending reset when the station is deleted (#2027) (#2031)
reset() stopped the station, slept resetTime, then re-initialized and
reconnected without checking whether the station had been deleted during
the sleep. A station deleted mid-reset was resurrected and reconnected to
the CSMS (the "zombie" reconnect that triggers #2017).
Add an instance AbortController tripped by delete(); reset() now awaits
interruptibleSleep(resetTime, signal) and rechecks the aborted state
before re-initializing, bailing out cleanly on abort. The dispose signal
is kept distinct from started/stopping, which reset() itself toggles via
stop() and therefore cannot be used to detect deletion.
The OCPP Reset response stays Accepted and the handler still invokes
reset() fire-and-forget; only the reset/delete lifecycle changes.
Part B (element-granular worker-thread termination) remains open.
fix(ui-server): make UI broadcast-channel commands complete reliably (#2018) (#2020)
* fix(ui-server): time out broadcast-channel requests so UI commands cannot hang
A UI control command (start/stop/delete a station) is dispatched over the
worker broadcast channel and the UI service waits for a fixed number of
worker responses, sampled at send time. If a targeted worker never replies
-- e.g. the station is deleted while the command is in flight -- that count
is never reached, so the request is never completed or released and the
client waits forever. Read commands keep working, which masks the wedge.
Arm a per-request safety-net timeout when a broadcast-channel request is
sent. On expiry the request is completed with a failure (reporting the
charging stations that did reply successfully) and both the request and
response aggregation state are released, so the caller gets an answer
instead of hanging. The timeout is cleared on normal completion, on a
dispatch failure, and on service stop.
Closes #2018.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* fix(ui-server): complete broadcast requests by target set and reconcile on station deletion (#2018)
Broadcast-channel request completion was gated on a frozen integer count
sampled at send time, blind to which stations were targeted. When the set
of stations changed mid-flight the aggregator could not reconcile the
drift, producing failure modes on top of the hang the safety-net timeout
already backstops:
- Hang until timeout when a targeted station is deleted mid-flight (the
count is never reached).
- Late double-completion: after release the count reads 0, so a late reply
re-enters and 1 >= 0 re-completes the request.
- False success: an empty explicit hashIds array degraded to a
broadcast-to-all instead of failing.
Track the outstanding responders as a Set of target hashIds and make the
Set the single source of truth for completion:
1. Snapshot the resolved targets (validated explicit hashIds, or the live
station set for a broadcast) into the request context; complete when the
Set empties. AbstractUIServer.deleteChargingStationData fans out
AbstractUIService.reconcileDeletedStation, which drops the departed
hashId from every in-flight request and completes any that empty with
the truthful aggregated payload. A DELETE_CHARGING_STATIONS request is
left untouched by reconciliation: its targets self-delete yet still post
their command reply on a separate transport, so that reply (not the
racing `deleted` event) is the completion source; a worker that dies
mid-delete is covered by the timeout.
2. Drop untracked responses (unknown/released request, or a hashId that is
not an outstanding responder) in the response handler, closing the late
double-completion re-entry. A reply without a hashId for a still-tracked
request is dropped and logged distinctly so the resulting timeout is
diagnosable.
3. Reject unknown and empty explicit targets instead of degrading to a
broadcast-to-all.
The 60 s safety-net timeout stays as a pure backstop for genuine worker
crash/deadlock; normal and reconciled completion clear its timer. The
completion accessor is named getBroadcastChannelOutstandingResponseCount to
reflect that it returns the responses still outstanding, not a frozen total.
Duplicate-identity false success and orphan-worker termination are not
addressed here (a hashId-keyed set cannot disambiguate two workers sharing
an identity) and are deferred to follow-up changes. The deprecated HTTP
transport bypasses aggregation and is likewise out of scope.
Tests cover reconcile-on-delete, the untracked-response guard, unknown and
empty target rejection, the DELETE self-target ordering (reconcile-first
completes via the reply; no reply falls back to the timeout), reconcile
that does not empty the set, two concurrent requests reconciled by one
deletion, a hashId-less reply, and the previously untested timeout
behaviors (partial-success payload, dispatch-failure and stop() timer
clearing, and the completion-race guard).
Closes #2018.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
---------
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Jérôme Benoit <jerome.benoit@sap.com>
fix(ocpp): reject charging profiles by EVSE existence and isolate non-persistent station configuration (#2022, #2024) (#2025)
* fix(ocpp): reject charging profiles by EVSE existence, not EVSE count (#2022)
OCPP 2.0 validateChargingProfile() guarded evseId with a count comparison
(evseId > getNumberOfEvses()). getNumberOfEvses() counts EVSEs (excluding
EVSE 0) and is not a maximum id, so a station with non-contiguous EVSE ids
{0,1,3} false-rejected a valid profile for EVSE 3 (3 > 2). Use the existing
hasEvse() existence check instead, mirroring the OCPP 1.6 hasConnector twin,
and align the message to "EVSE does not exist".
RequestStartTransaction carries the TxProfile (F01.FR.08-10 / F02.FR.16-18);
existence-based rejection mirrors K01.FR.28 (conformance test TC_K_14_CS).
Test: a charging profile for the existing, non-contiguous EVSE 3 is accepted
(a count-based guard would reject it, 3 > 2). Mutation-verified: reverting the
guard makes the test fail.
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
* fix(charging-station): clone template OCPP configuration to isolate non-persistent stations (#2024)
getOcppConfigurationFromTemplate() returned the SharedLRUCache-cached
template's Configuration by reference. For non-persistent stations
(ocppPersistentConfiguration=false) this aliased the shared configurationKey
array, so an in-place setConfigurationKeyValue() mutation polluted the cached
template. Clone the Configuration, mirroring the existing connector/EVSE clone
pattern (clone(undefined) === undefined).
Test: a non-persistent station's Configuration is an independent copy of the
cached template, and mutating a key on it leaves that cached template unchanged.
Mutation-verified: reverting the clone makes the test fail. Retires the
now-redundant SharedLRUCache-clearing afterEach workaround in
ChargingStation-ResetIdentity.test.ts (file still passes).
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
---------
Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>
fix(charging-station): retain creation options so a reset keeps station identity (#2019)
* fix(charging-station): retain creation options so a reset keeps station identity
On an OCPP Reset (and on a template-file reload) the station was
re-initialized via initialize() with no arguments, dropping the
creation-time options. The restarted station reverted to the template
defaults, losing its fixed identity (reappearing as CS-BASIC-000N with a
new hashId) and its configured supervision URL; the original station
never came back.
Retain the creation options on the instance and re-pass them in reset()
and in the template-watcher reload. setSupervisionUrl() also updates the
retained snapshot so a runtime supervision-URL change survives a reboot
(but not a full simulator restart, which reconstructs the worker from the
original options). The snapshot is in-memory only: it is never persisted
and never written to the template.
Closes #2017.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* fix(charging-station): clone the retained creation options
setSupervisionUrl updates the retained options in place so a later reset
re-applies the current supervision URL. Clone them at construction so that
in-place update never mutates the shared worker data passed from the worker
thread.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* test(charging-station): cover identity retention across a reset
A non-persistent station keeps its configured identity across a reset only
when the creation options are re-applied; a persistent one restores it from
its saved configuration without them. This locks the behavior and scopes the
options fix to the non-persistent case.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* refactor(charging-station): re-apply retained options on reset only when non-persistent
Address review feedback on the identity-retention fix:
- Rename the retained-options field to creationOptions (clearer; no longer
shadows the options parameters).
- Re-apply the creation options on reset/template-reload only for a
non-persistent station, via the reinitializeOptions getter. A persistent
station restores from its saved config, which stays the source of truth, so
re-applying them would needlessly override it (and collapse an OCPP-config
station's supervision URL). The unconditional setSupervisionUrl mirror is kept,
with a sharpened comment explaining why it must run for both branches.
- Cover the reset -> initialize wiring (both persistence modes) and the
setSupervisionUrl-across-reset retention with unit tests.
No change to the fixed behavior: a reset still keeps the configured identity.
* refactor(charging-station): factor setSupervisionUrl credential updates into a helper
Deduplicate the supervisionUser/supervisionPassword null-checks that were
applied twice (to stationInfo and to the retained creation options) into a
single applyCredentials() closure. No behavior change.
* test(charging-station): cover OCPP-config supervision URL retention across reset
- Add a regression test for the non-persistent supervisionUrlOcppConfiguration
path: after setSupervisionUrl(), a real reset() must still dial the new URL
(the branch the setSupervisionUrl mirror comment reasons about, previously
untested).
- Prefix all test titles with "should" per tests/TEST_STYLE_GUIDE.md.
- Clear the real SharedLRUCache singleton in afterEach so a cached template
cannot bleed between tests.
- Support optional template field overrides in the makeTemplate helper.
* refactor(charging-station): rename reinitialization getter and fix its mirror comment
- Rename the reinitializeOptions getter to reinitializationOptions (noun form,
consistent with the wsConnectionUrl/hasEvses getters).
- Correct the setSupervisionUrl mirror comment: the retained-options mirror is
load-bearing on a cache-cold reinitialization (template reload), not on a plain
warm-cache reset() (whose in-place OCPP-key cache mutation already carries the
URL). No behavior change.
* test(charging-station): add a genuine OCPP-config supervision-URL reload guard
The previous OCPP-config test drove a warm-cache reset() and passed even with the
retained-options mirror deleted (a false guard): the cached template still carried
the mutated OCPP key. Add a cache-cold reload test (clear SharedLRUCache, then
reinitialize) that re-seeds the key from configuredSupervisionUrl and therefore
fails if the mirror is removed. Relabel the warm-cache reset test and correct its
comment to reflect that it exercises the cache-mutation path, not the mirror.
* docs(charging-station): tighten the setSupervisionUrl mirror comment
Condense the retained-options mirror comment (~11 -> 8 lines) while preserving
every load-bearing detail: it runs for both branches, a cache-cold reload
re-seeds the OCPP key from configuredSupervisionUrl, a warm reset() survives via
the cached template mutation, and the mirror is in-memory only.
---------
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Jérôme Benoit <jerome.benoit@sap.com>
fix(charging-station): re-dial after a server-initiated connection close (#2021)
* fix(charging-station): re-dial after a server-initiated connection close
When the CSMS or a proxy closed a station's WebSocket with a normal
(clean) close, the station stayed "started but not connected" instead of
reconnecting. onClose only re-dialed on abnormal close codes and treated
clean closes as terminal, but a clean close cannot be told apart by code
from the station's own closeWSConnection() (the UI disconnect action,
which must stay down). Real charge-point hardware re-dials after losing
the connection regardless of the close code.
Mark only explicitly-requested closes (the UI disconnect) as terminal via
a byRequest flag on closeWSConnection(), and reconnect on any close the
station did not request while it is still started -- clean or abnormal.
This also fixes certificate rotation, which closes the socket to force a
re-dial with the new certificate and previously never reconnected.
Closes #2016.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* refactor(charging-station): pass closeWSConnection intent via an options object
Match the paired openWSConnection signature and make the call site
self-documenting: closeWSConnection({ byRequest: true }) rather than a bare
positional boolean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* test(charging-station): cover the onClose reconnect decision
Drive onClose directly with a spied reconnect: the station re-dials after a
server-initiated normal close while started, and stays disconnected after an
operator-requested close. Removes a stale test that only asserted the socket
reached CLOSED, which held regardless of the reconnect decision.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
* docs(charging-station): harmonize reconnect terminology and tighten close comments
- Use the codebase's "reconnect" term consistently (drop "re-dial"), and
"server-initiated"/"requested" to match the PR title and the byRequest option.
- Tighten the closeWSConnection JSDoc and onClose comments; align the JSDoc
@param style (no trailing period) with the sibling openWSConnection.
- Prefix the reconnect test titles with "should" per tests/TEST_STYLE_GUIDE.md.
No behavior change.
---------
Signed-off-by: Daniel <7558512+DerGenaue@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Jérôme Benoit <jerome.benoit@sap.com>
refactor(ocpp): use iterateConnectors() in OCPP 1.6 incoming request service (#2015)
Closes #2014
style: codebase consistency sweep — JSDoc @returns backticks + != null idiom (#2009)
Behavior-preserving codebase style-consistency sweep. Closes #1966.
- Backtick 49 JSDoc @returns boolean literals across 17 files (lowercase True/False).
- Align !== undefined -> != null on number|undefined guards in OCPP20IncomingRequestService.ts (13 tokens); conditional-spread inclusion guards preserved as !== undefined.
- Extend != null idiom to OCPP20VariableManager min/max ternaries + presence guards and the negated EVSE guard.
- Fix OCPP20AuthAdapter.validateConfiguration @returns (synchronous boolean; was falsely 'Promise resolving to').
Gap C (String(previousRequestId) -> .toString()) dropped: premise false — operand is number|undefined, .toString() would fail typecheck (TS18048); String() is the only behavior-preserving form.
All gates green (format/typecheck/lint/build/test, fail=0). Reviewed across multiple multi-agent rounds with cross-validation.
refactor(ocpp): remove as-unknown-as double-casts from request/response dispatch (#2003)
Give each builder its precise input contract instead of laundering casts
through phantom generics or JsonType:
- buildRequestPayload drops its unsound <Request> generic and returns honest
JsonType; each dispatch branch narrows with a single cast at a genuine
JsonType boundary.
- Builder inputs (StatusNotificationOptions, OCPP20TransactionEventOptions,
SignCertificateOptions) are JsonObject subtypes; StatusNotification build
validates connectorStatus via the shared isOCPP20ConnectorStatus guard.
- Handler bridges (toRequestHandler/toResponseHandler) use a single cast that
preserves the handler's async identity, fixing an unawaited-handler
regression from a plain wrapper defeating isAsyncFunction.
- CAT-C string-to-enum narrowing becomes a type guard / typed Record.
Closes #1968
refactor(utils): swap getRandomFloat(Rounded) params to (min, max) for consistency (#1998)
Swap getRandomFloat and getRandomFloatRounded params from (max, min[, scale]) to (min, max[, scale]) for consistency with the sibling bounds helpers randomInt (node:crypto) and isValidRandomIntBounds; migrate all 11 production + 5 test call sites; harmonize JSDoc across the random-float family.
Bundles a correctness fix: getRandomFloat now rejects a non-finite interval width (max - min), closing a latent overflow where finite endpoints such as (-MAX_VALUE, MAX_VALUE) returned Infinity/NaN, silently violating the documented [min, max] contract.
Internal API refactor: package.json exports only ./dist/start.js, so getRandomFloat is app-internal and this is not a semver-major public break.
Closes #1967
test(ocpp): cover OCPP 2.0.1 log/firmware lifecycle supersession and cleanup paths (#1997)
fix(ocpp): guard OCPP 1.6 UpdateFirmware deferred-timer schedule against sealed state writes (#1994)
The .on(UPDATE_FIRMWARE, ...) listener's deferred branch obtains state via
getOrCreateStationState() and then writes stationState.deferredFirmwareUpdateTimer
= setTimeout(...). After PR #1992 sealed the base-plumbing behavior,
getOrCreateStationState() returns the stopped state instead of lazy-initializing
a fresh entry once stop() has marked stationsState.get(cs)?.stopped === true. A
late UPDATE_FIRMWARE dispatch after ChargingStation.stop() runs
ocppIncomingRequestService.stop(this) at ChargingStation.ts:1273 (before
this.started = false at :1280) therefore writes a fresh Timeout onto the sealed
state.
The callback body is safe (unref'd, checkChargingStationState gate), but the
write is inconsistent with the OCPP 2.0.1 GUARD sites shipped in PR #1992
(getCertSigningRetryManager, sendSecurityEventNotification,
simulateFirmwareUpdateLifecycle, simulateLogUploadLifecycle) and leaves a Timeout
closure-referenced entry pending in Node's queue until GC reclaims the
ChargingStation.
Insert a stopped GUARD immediately after getOrCreateStationState, before
cancelDeferredFirmwareUpdate — mirroring the exact syntax of the OCPP 2.0.1
GUARD sites (bare 'return' after 'if (stationState.stopped === true) {'). The
GUARD short-circuits the schedule branch before any write reaches the sealed
state.
Add a regression test covering the post-stop() deferred-schedule path in
tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts.
Closes #1993
Touchpoint:
- src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts:705 — stopped GUARD
References:
- PR #1992 — source of the 'stopped' sentinel + OCPP 2.0.1 GUARD sites
- PR #1984 — source of 'deferredFirmwareUpdateTimer'
This is purely additive; no observable OCPP behavior change (the pre-existing
fire-time checkChargingStationState gate inside the setTimeout callback remains
as defense-in-depth).
fix(ocpp): guard post-stop() handler dispatch against WeakMap resurrection (#1992)
Closes #1970
OCPP 2.0.1 handlers dispatched from queued microtasks, unref'd retry
timers, or EventEmitter-dispatched `.on(...)` listeners could run
AFTER `stop()` and resurrect the `stationsState` WeakMap entry via
`getOrCreateStationState` lazy-init. `sendQueuedSecurityEvents` also
scheduled a retry `setTimeout(...)` whose handle was never stored, so
`stop()` had no way to cancel a pending retry, and the recursive
retry callback re-entered `sendQueuedSecurityEvents` — resurrecting
state.
Three coordinated parts.
Part 1 — base plumbing (`OCPPIncomingRequestService.ts`):
- Generic bound widened to
`TStationState extends { stopped?: boolean } = { stopped?: boolean }`.
- `stop()` template: after `resetStationState`, marks
`stationState.stopped = true` instead of `stationsState.delete(cs)`.
Deletion would re-enable resurrection via `getOrCreateStationState`
lazy-init on any late dispatch. Keeping the sealed entry lets the
getter return the sealed state and raw `.get()` null-guarded callers
observe the marker and drop. The WeakMap entry is naturally
collected when the ChargingStation reference is dropped.
- `getOrCreateStationState` returns the sealed stopped state when
`state?.stopped === true`; no fresh entry is lazy-init'd post-stop.
- Both concrete state interfaces (`OCPP16StationState`,
`OCPP20StationState`) get an alphabetized `stopped?: boolean` field.
Part 2 — OCPP 2.0.1 call-site audit (11 sites):
Two sites converted to raw `stationsState.get(cs)` + `stopped === true`
null-guard, silent-drop:
- `sendNotifyReportRequest` — dispatched from
`.on(GET_BASE_REPORT).catch(...)` microtask; state usually created
by sibling `handleRequestGetBaseReport` but external `emit(...)`
paths and post-stop dispatch are covered by the null-guard.
- `sendQueuedSecurityEvents` — dispatched from unref'd retry
`setTimeout`; guard is the last line of defense against a
fired-before-cancel race.
Four sites use `getOrCreateStationState` + explicit `stopped` check
(state may not exist yet on the pre-stop happy path):
- `sendSecurityEventNotification` — lifecycle-entry on the
invalid-cert-on-first-request edge case
(`handleRequestCertificateSigned` X.509-invalid branch,
`handleRequestUpdateFirmware` invalid-PEM branch).
- `getCertSigningRetryManager` — public accessor; return type widened
to `OCPP20CertSigningRetryManager | undefined`; two callers
(`handleRequestCertificateSigned`, `OCPP20ResponseService`) updated
with `?.` to tolerate the undefined return. Without this guard, a
late `handleResponseSignCertificate` dispatch (a SignCertificate
response can arrive between `ocppIncomingRequestService.stop(cs)`
and `closeWSConnection()` in `ChargingStation.stop()`) would
materialize a fresh manager on the sealed state and schedule a
non-`.unref()`'d retry `setTimeout` holding the ChargingStation
reference.
- `simulateFirmwareUpdateLifecycle` — dispatched from
`.on(UPDATE_FIRMWARE).catch(...)` microtask; guards against driving
the full async lifecycle on a sealed state (which would send OCPP
requests through a closed WebSocket and clobber the abort-controller
fields).
- `simulateLogUploadLifecycle` — symmetric,
`.on(GET_LOG).catch(...)`.
Five sites kept unchanged (Part 1 guard covers them):
`getRestoredConnectorStatus`, `handleRequestGetBaseReport`,
`handleRequestUpdateFirmware`, `savePreInoperativeStatuses`,
`sendFirmwareStatusNotification`.
Part 3 — retry timer discipline in `sendQueuedSecurityEvents`:
- Store the retry `setTimeout` handle on
`OCPP20StationState.securityEventRetryTimer`.
- Cancel any previously scheduled retry before scheduling a new one.
- Self-clear the handle before the recursive
`sendQueuedSecurityEvents` call inside the callback.
- New `cancelSecurityEventRetryTimer` helper is called from
`resetStationState` (after abort-controller aborts and cert-signing
retry cancel, before the two `resetActive*State` helpers).
- `.unref()` preserved.
Files touched (7 modified + 1 new):
- `src/charging-station/ocpp/OCPPIncomingRequestService.ts`
- `src/charging-station/ocpp/1.6/OCPP16IncomingRequestService.ts`
- `src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts`
- `src/charging-station/ocpp/2.0/OCPP20ResponseService.ts`
- `tests/charging-station/ocpp/OCPPIncomingRequestService-StationState.test.ts`
- `tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-Firmware.test.ts`
- `tests/charging-station/ocpp/1.6/OCPP16IncomingRequestService-GetDiagnostics.test.ts`
- `tests/charging-station/ocpp/2.0/OCPP20IncomingRequestService-PostStopResurrection.test.ts`
(new)
Design rationale — silent-drop over `StationStoppedError` throw:
matches the "post-stop silently drop" semantic and avoids catch-block
sprawl at every handler.
Design rationale — return `undefined` from `getCertSigningRetryManager`
over convert-with-explicit-null-manager: two callers already handle
optional chaining cleanly; return-type widening is the minimum change.
Verification: pnpm format / typecheck / lint pass; pnpm test 3029
pass, 0 fail; pnpm build exit 0. No `stationsState.delete` remains in
`src/`; `securityEventRetryTimer` lives in exactly three logical
places (interface field, retry-site store + self-clear, cancel helper
called from `resetStationState`).
References: #1983 (shared base plumbing), #1984 (OCPP 1.6
`deferredFirmwareUpdateTimer` timer discipline).
fix(ocpp): cross-check OCPP 1.6 lifecycle flags in trigger, re-entry, and simulation entry (#1987)
The OCPP 1.6 TriggerMessage handler cases for DiagnosticsStatusNotification and
FirmwareStatusNotification read stationInfo.diagnosticsStatus and
stationInfo.firmwareStatus directly. When an exception, abort, or lifecycle
drift left stationInfo.*Status at a stale non-terminal value, the trigger
faithfully reported that stale value to the CSMS instead of Idle.
The identical anti-pattern was present in handleRequestUpdateFirmware's re-entry
guard (which suppressed legitimate new UpdateFirmware.req when
stationInfo.firmwareStatus was stuck at Downloading / Downloaded / Installing)
and in the base-class event dispatcher's unconditional fan-out of
UPDATE_FIRMWARE emits (which spawned duplicate concurrent
updateFirmwareSimulation invocations emitting duplicate progress notifications
to the CSMS).
Track lifecycle progress with two per-station boolean flags on
OCPP16StationState (diagnosticsUploadInProgress, firmwareUpdateInProgress), set
on lifecycle entry and cleared in a finally block on every exit path (happy,
return, throw). The flags are consumed at four sites: the two TriggerMessage
cases, the handleRequestUpdateFirmware re-entry guard, and the
updateFirmwareSimulation entry guard.
OCPP 1.6 GetDiagnostics.req and UpdateFirmware.req core-profile payloads do not
carry a requestId (unlike OCPP 2.0.1 GetLog and UpdateFirmware), so lifecycle
progress is tracked with boolean flags rather than requestId-presence
sentinels.
Satisfies OCPP 1.6 SHALL clauses in section 4.4 (Diagnostics) and section 4.5
(Firmware), and the Idle enumeration definitions in section 7.24 and
section 7.25.
Closes #1973
fix(ocpp): drive FirmwareStatusNotification trigger from last-sent notification (L01.FR.26) (#1981)
TriggerMessage(FirmwareStatusNotification) was gated on
`activeFirmwareUpdateRequestId != null && hasFirmwareUpdateInProgress()`,
falling back to Idle otherwise. Since `hasFirmwareUpdateInProgress` returns
false for every terminal status (`DownloadFailed`, `InvalidSignature`,
`InstallationFailed`, `InstallVerificationFailed`, `Installed`), the
station returned Idle after every non-Installed terminal, violating OCPP
2.0.1 L01.FR.26 (SHALL, mirrored by L02.FR.17).
Persist the last-sent `(requestId, status)` on `OCPP20StationState` and
drive the trigger from it. `stationInfo.firmwareStatus` is retained
because `hasFirmwareUpdateInProgress` and its 3 consumers (reset
rejection, isChargingStationIdle, isEvseIdle) depend on its
"in-progress predicate" semantics — deliberate dual-source, not
conflation.
Closes #1979
fix(ocpp): reset stationInfo.firmwareStatus on abort/exception in simulateFirmwareUpdateLifecycle (#1976)
simulateFirmwareUpdateLifecycle wrote stationInfo.firmwareStatus at every
status transition via sendFirmwareStatusNotification, but its finally block
only cleared the per-station WeakMap state (activeFirmwareUpdateAbortController,
activeFirmwareUpdateRequestId). On exception or abort (supersession, stop()),
stationInfo.firmwareStatus stayed at the last-emitted non-terminal value
(e.g. 'Downloading'), leaving a latent data-model split any future
consumer that does not cross-check activeFirmwareUpdateRequestId would
observe.
Track the current lifecycle stage inside the lifecycle scope and, in the
existing finally, reset stationInfo.firmwareStatus in-place when the
lifecycle did not reach Installed. Per OCPP 2.0.1 FirmwareStatusEnumType,
Idle SHALL only be used in TriggerMessage-triggered notifications, so as
a persistent local state Idle is only valid before install starts:
- clean download-phase abort -> Idle
- clean install-phase abort -> InstallationFailed
- exception during download stage -> DownloadFailed
- exception during install stage -> InstallationFailed
Per OCPP 2.0.1 L01.FR.24 Note the terminal *Failed FirmwareStatusNotification
on supersession is optional; L02.FR.15 Note omits any such clause. Emitting
no FirmwareStatusNotification from finally is safe under both profiles.
The field is reset in-place; no FirmwareStatusNotification is emitted
from finally, so the simulator does not race the caller driving the
supersession.
Stage transitions are placed only after successful advancement (after the
Installing / Installed notification awaits resolve), never on error/abort
branches, so the terminal-value selection in finally is unambiguous. The
finally guards mirror clearActiveFirmwareUpdate's requestId-supersession
pattern to avoid clobbering a superseder's live status, and preserve any
explicit terminal already emitted from inside try (DownloadFailed /
InvalidSignature / InstallationFailed).
Stage->terminal mapping uses `Record<Exclude<FirmwareStage, 'installed'>,
OCPP20FirmwareStatusEnumType>` with `as const satisfies` for
compile-time exhaustiveness over the failure-eligible stages. Naming
convention (SCREAMING_SNAKE_CASE) matches CoherentMeterValueBuilder's
PHASE_FAMILY/PHASE_RANK pattern for private module-scope Record lookup
tables.
Add OCPP20VariableManager.getInstance().resetRuntimeOverrides() to the
shared standardCleanup test helper so per-station variable overrides do
not leak between tests (all mock stations share a hardcoded hashId).
Add 8 tests covering: supersession mid-download -> Idle, supersession
mid-install -> InstallationFailed, throw during download -> DownloadFailed,
throw during install -> InstallationFailed, happy path preserved -> Installed,
supersession-with-emit race (T2 launched via constructor listener) ->
Downloading preserved, InvalidSignature preserved, retries-exhausted
DownloadFailed preserved.
Closes #1969
fix(atg): invalidate configurationValidationResult on ChargingStationEvents.updated (#1977)
Subscribe to ChargingStationEvents.updated in AutomaticTransactionGenerator's
constructor so in-flight configuration mutations that reach any charging
station mutation emit cannot leave a stale memoized validation decision
behind. stop() still clears the cache as a redundant safety net.
deleteInstance unsubscribes the handler before removing the entry so the
retired instance is not retained via the charging station's listener array
across a getInstance/deleteInstance churn cycle.
Fixes #1965.
refactor: audit backlog cleanup — JSDoc gaps, physics warning, ATG robustness, comment sweep (#1962)
* 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: <non-Idle> } — 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<ChargingStation,
Set<string>>). 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).
repositories / e-mobility-charging-stations-simulator.git / log
