From: Jérôme Benoit Date: Tue, 26 May 2026 00:19:25 +0000 (+0200) Subject: fix(utils): make file persistence atomic across writers (#1871) X-Git-Tag: cli@v4.8.0~17 X-Git-Url: https://git.piment-noir.org/?a=commitdiff_plain;h=550824fa7da7dd829fb3155321aa924bf9681116;p=e-mobility-charging-stations-simulator.git fix(utils): make file persistence atomic across writers (#1871) * feat(utils): add atomic file write primitive and migrate call sites Add `atomicWriteFile` and `atomicWriteFileSync` to `src/utils/FileUtils.ts`, implementing the canonical write-then-rename pattern with optional fsync durability and best-effort temp file cleanup on failure. Errors are funnelled through the existing `handleFileException` helper so callers stay aligned with the project-wide file error reporting contract. Migrate the five non-atomic disk writes uncovered by the audit: - `BootstrapStateUtils.writeStateFile` replaces its inline tmp+rename with the new primitive (single source of truth, gains fsync durability). - `ChargingStation.saveConfiguration` replaces `writeFileSync` so charging station OCPP configuration JSON cannot be torn by a crash mid-write. - `JsonFileStorage.storePerformanceStatistics` drops the persistent `openSync('w')` file descriptor design (which truncated the file at byte zero on every sample) and uses the atomic primitive instead. Also fixes the previous fire-and-forget `runExclusive(...).catch()` pattern by awaiting the lock. - `OCPP20CertificateManager` writes installed PEM certificates atomically. Add `FileType.Certificate` so PEM writes can flow through `handleFileException` with an accurate file type label. Concurrent writers to the same path must still be serialized externally (typically via `AsyncLock`); the primitive does not implement an internal queue, matching how every existing call site already locks. The `createWriteStream` diagnostics archive in OCPP 1.6 `GetDiagnostics` is intentionally left as-is since the file is ephemeral (FTP-uploaded then discarded). Targets Node >= 22 so `writeFile`/`writeFileSync` natively expose the `flush: true` option used for the fsync step. * fix(utils): align atomic write call sites on a single error path Address review feedback on PR #1871. The atomic write primitive logs and re-throws by default via `handleFileException`. Three call sites kept their pre-migration outer error wrappers, producing double handling: - `ChargingStation.saveConfiguration`: the `.catch` handler attached to the fire-and-forget `AsyncLock.runExclusive(...).catch(...)` chain re-threw inside the catch callback, leaking an unhandled promise rejection on every config write failure (disk full, EACCES, EROFS, ...). Pass `{ throwError: false }` to that `handleFileException` call so the rejection is fully absorbed. The retry semantics are preserved: when `atomicWriteFileSync` throws, the `endMeasure` and `configurationFileHash` updates inside the lock callback are skipped, so the next `saveConfiguration` invocation will retry the write. - `JsonFileStorage.storePerformanceStatistics`: drop the redundant outer `try/catch` and pass `{ throwError: false }` to the primitive, matching the `BootstrapStateUtils.writeStateFile` template. Failures now produce a single error log instead of one error log followed by a second warn-level log. - `OCPP20CertificateManager.storeCertificate`: replace the empty `logPrefix` with a string carrying `stationHashId` and the module origin so the new error log carries actionable context. The outer `try/catch` in `storeCertificate` only stringifies the error into the structured `{ success: false, error }` result and does not call `handleFileException`, so there is no double handling here — only the missing context to fix. * refactor(utils): consolidate atomic write options and address audit findings - FileUtils: include threadId in temp filename for cross-worker uniqueness; fold errorParams into AtomicWriteOptions (single trailing options bag); document SIGKILL leak, durability scope, and per-field defaults; drop redundant 'utf8' as BufferEncoding cast. - BootstrapStateUtils.writeStateFile: drop the undefined placeholder. - ChargingStation.saveConfiguration: route fs failures (already logged at error by handleFileException) to debug, and surface non-fs failures from the lock body at error level via a typeof-guarded ErrnoException check. - OCPP20CertificateManager.storeCertificate: take a required logPrefix from the caller (replaces the synthetic prefix); drop the redundant manual mkdir+pathExists block (atomicWriteFile's default ensureDir handles it); write certificates with mode 0o600; document the per-path serialization rationale (paths keyed by certificate serial number). Both OCPP20IncomingRequestService callers updated to pass chargingStation.logPrefix(); the manager unit tests are migrated. - JsonFileStorage.storePerformanceStatistics: migrate to AtomicWriteOptions with errorParams; set flush: false on this hot telemetry path (durability across crashes is not required for performance records); add 5 focused unit tests covering happy path, overwrite, Map serialization, runtime storage directory removal, and parallel writes. - FileUtils tests: tighten assert.throws/rejects with { code: 'ENOENT' } and add a deterministic test that asserts the destination remains intact and the temp file is cleaned up when the rename step fails. * fix(performance-storage): decode file URIs into native filesystem paths JsonFileStorage parsed the storage URI via new URL(uri) and used .pathname directly as a filesystem path. On Windows, file: URLs yield WHATWG-formatted pathnames such as '/C:/Users/...' which mkdirSync interprets relative to the current drive, producing corrupt paths like 'D:\\C:\\Users\\...' and breaking JsonFileStorage on windows-latest CI. Use fileURLToPath() (the standard Node.js inverse of pathToFileURL) to decode file: URIs into the native path on every platform. Non-file schemes (typically jsonfile:./relative-path) keep .pathname semantics for backward compatibility with existing user configurations. The new tests in tests/performance/storage/JsonFileStorage.test.ts exercise this code path with pathToFileURL(absolutePath), which now round-trips correctly across POSIX and Windows. --- diff --git a/src/charging-station/BootstrapStateUtils.ts b/src/charging-station/BootstrapStateUtils.ts index 9b2ff8d5..e33b7686 100644 --- a/src/charging-station/BootstrapStateUtils.ts +++ b/src/charging-station/BootstrapStateUtils.ts @@ -1,15 +1,7 @@ // Partial Copyright Jerome Benoit. 2021-2025. All Rights Reserved. -import { - existsSync, - mkdirSync, - readdirSync, - readFileSync, - renameSync, - rmSync, - writeFileSync, -} from 'node:fs' -import { basename, dirname, join } from 'node:path' +import { existsSync, readdirSync, readFileSync, renameSync, rmSync } from 'node:fs' +import { basename, join } from 'node:path' import type { TemplateStatistics } from '../types/index.js' @@ -17,6 +9,7 @@ import { FileType } from '../types/index.js' import { AsyncLock, AsyncLockType, + atomicWriteFileSync, ensureError, formatLogPrefix, handleFileException, @@ -148,29 +141,16 @@ export const writeStateFile = async ( logPrefixFn?: () => string ): Promise => { await AsyncLock.runExclusive(AsyncLockType.simulatorState, () => { - const tmpFile = `${stateFilePath}.tmp` - try { - mkdirSync(dirname(stateFilePath), { recursive: true }) - const stateData: SimulatorStateFile = { - started, - version: STATE_FILE_VERSION, - } - writeFileSync(tmpFile, JSON.stringify(stateData, undefined, 2), 'utf8') - renameSync(tmpFile, stateFilePath) - } catch (error) { - // Best-effort tmp cleanup; ignore secondary failure to surface the original error. - try { - rmSync(tmpFile, { force: true }) - } catch { - // Ignore - } - handleFileException( - stateFilePath, - FileType.SimulatorState, - ensureError(error), - logPrefixFn?.() ?? '', - { throwError: false } - ) + const stateData: SimulatorStateFile = { + started, + version: STATE_FILE_VERSION, } + atomicWriteFileSync( + stateFilePath, + JSON.stringify(stateData, undefined, 2), + FileType.SimulatorState, + logPrefixFn?.() ?? '', + { errorParams: { throwError: false } } + ) }) } diff --git a/src/charging-station/ChargingStation.ts b/src/charging-station/ChargingStation.ts index 69e46b37..7c7090e0 100644 --- a/src/charging-station/ChargingStation.ts +++ b/src/charging-station/ChargingStation.ts @@ -3,7 +3,7 @@ import { millisecondsToSeconds, secondsToMilliseconds } from 'date-fns' import { hash, randomInt } from 'node:crypto' import { EventEmitter } from 'node:events' -import { existsSync, type FSWatcher, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, type FSWatcher, mkdirSync, readFileSync, rmSync } from 'node:fs' import { dirname, join } from 'node:path' import { URL } from 'node:url' import { parentPort } from 'node:worker_threads' @@ -69,6 +69,7 @@ import { ACElectricUtils, AsyncLock, AsyncLockType, + atomicWriteFileSync, buildAddedMessage, buildChargingStationAutomaticTransactionGeneratorConfiguration, buildConnectorsStatus, @@ -2589,22 +2590,37 @@ export class ChargingStation extends EventEmitter { configurationData.configurationHash = configurationHash const measureId = `${FileType.ChargingStationConfiguration} write` const beginId = PerformanceStatistics.beginMeasure(measureId) - writeFileSync( + atomicWriteFileSync( this.configurationFile, JSON.stringify(configurationData, undefined, 2), - 'utf8' + FileType.ChargingStationConfiguration, + this.logPrefix() ) PerformanceStatistics.endMeasure(measureId, beginId) this.sharedLRUCache.deleteChargingStationConfiguration(this.configurationFileHash) this.sharedLRUCache.setChargingStationConfiguration(configurationData) this.configurationFileHash = configurationHash }).catch((error: unknown) => { - handleFileException( - this.configurationFile, - FileType.ChargingStationConfiguration, - ensureError(error), - this.logPrefix() - ) + // File-system failures are already logged at error level by the atomic + // write via handleFileException; absorb them here at debug level. Other + // failures inside the lock body (JSON serialization, cache mutation, ...) + // would otherwise go unobserved, so log them at error level. + const isErrnoException = + typeof error === 'object' && + error !== null && + 'code' in error && + typeof (error as NodeJS.ErrnoException).code === 'string' + if (isErrnoException) { + logger.debug( + `${this.logPrefix()} ${moduleName}.saveConfiguration: configuration save rejected:`, + error + ) + } else { + logger.error( + `${this.logPrefix()} ${moduleName}.saveConfiguration: unexpected error inside configuration save lock:`, + ensureError(error) + ) + } }) } else { logger.debug( diff --git a/src/charging-station/ocpp/2.0/OCPP20CertificateManager.ts b/src/charging-station/ocpp/2.0/OCPP20CertificateManager.ts index 689ef983..72702640 100644 --- a/src/charging-station/ocpp/2.0/OCPP20CertificateManager.ts +++ b/src/charging-station/ocpp/2.0/OCPP20CertificateManager.ts @@ -1,7 +1,7 @@ // Copyright Jerome Benoit. 2021-2025. All Rights Reserved. import { hash, X509Certificate } from 'node:crypto' -import { mkdir, readdir, readFile, realpath, rm, stat, writeFile } from 'node:fs/promises' +import { readdir, readFile, realpath, rm, stat } from 'node:fs/promises' import { join, resolve, sep } from 'node:path' import type { ChargingStation } from '../../index.js' @@ -12,11 +12,18 @@ import { type CertificateHashDataType, CertificateSigningUseEnumType, DeleteCertificateStatusEnumType, + FileType, GetCertificateIdUseEnumType, HashAlgorithmEnumType, InstallCertificateUseEnumType, } from '../../../types/index.js' -import { convertToDate, getErrorMessage, isEmpty, isNotEmptyArray } from '../../../utils/index.js' +import { + atomicWriteFile, + convertToDate, + getErrorMessage, + isEmpty, + isNotEmptyArray, +} from '../../../utils/index.js' import { extractDerIssuer } from './Asn1DerUtils.js' /** @@ -62,7 +69,8 @@ export interface OCPP20CertificateManagerInterface { storeCertificate( stationHashId: string, certType: CertificateSigningUseEnumType | InstallCertificateUseEnumType, - pemData: string + pemData: string, + logPrefix: string ): Promise | StoreCertificateResult validateCertificateFormat(pemData: unknown): boolean validateCertificateX509(pem: string): ValidateCertificateX509Result @@ -377,12 +385,14 @@ export class OCPP20CertificateManager { * @param certType - Certificate type for storage (InstallCertificateUseEnumType for root certificates * or CertificateSigningUseEnumType for signed leaf certificates) * @param pemData - PEM-encoded certificate data + * @param logPrefix - Caller-supplied log prefix used for atomic write error logging * @returns Storage result with success status and file path or error */ public async storeCertificate ( stationHashId: string, certType: CertificateSigningUseEnumType | InstallCertificateUseEnumType, - pemData: string + pemData: string, + logPrefix: string ): Promise { if (!this.validateCertificateFormat(pemData)) { return { @@ -407,12 +417,16 @@ export class OCPP20CertificateManager { await this.validateCertificatePath(filePath, OCPP20CertificateManager.BASE_CERT_PATH) - const dirPath = resolve(filePath, '..') - if (!(await this.pathExists(dirPath))) { - await mkdir(dirPath, { recursive: true }) - } - - await writeFile(filePath, pemData, 'utf8') + // Per-path serialization is implicit: the destination path is keyed by the + // certificate serial number, so concurrent calls writing byte-identical PEMs + // collapse to equivalent writes, and certificates with distinct serials use + // distinct paths. No external AsyncLock is required for these cases. Concurrent + // calls writing different PEM bytes to the same serial (rare in practice) are + // last-writer-wins. Parent directory creation is delegated to atomicWriteFile + // via its default `ensureDir: true`. + await atomicWriteFile(filePath, pemData, FileType.Certificate, logPrefix, { + mode: 0o600, + }) return { filePath, diff --git a/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts b/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts index 6e063f93..b3052af2 100644 --- a/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts +++ b/src/charging-station/ocpp/2.0/OCPP20IncomingRequestService.ts @@ -1542,7 +1542,8 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { const result = chargingStation.certificateManager.storeCertificate( chargingStation.stationInfo?.hashId ?? '', certificateType ?? CertificateSigningUseEnumType.ChargingStationCertificate, - certificateChain + certificateChain, + chargingStation.logPrefix() ) const storeResult = result instanceof Promise ? await result : result @@ -2073,7 +2074,8 @@ export class OCPP20IncomingRequestService extends OCPPIncomingRequestService { const rawResult = chargingStation.certificateManager.storeCertificate( chargingStation.stationInfo?.hashId ?? '', certificateType, - certificate + certificate, + chargingStation.logPrefix() ) const resultPromise: Promise = rawResult instanceof Promise diff --git a/src/performance/storage/JsonFileStorage.ts b/src/performance/storage/JsonFileStorage.ts index 3d1fa0d5..a81c1c6a 100644 --- a/src/performance/storage/JsonFileStorage.ts +++ b/src/performance/storage/JsonFileStorage.ts @@ -1,12 +1,12 @@ // Copyright Jerome Benoit. 2021-2025. All Rights Reserved. -import { closeSync, openSync, writeSync } from 'node:fs' +import { fileURLToPath } from 'node:url' -import { BaseError } from '../../exception/index.js' import { FileType, MapStringifyFormat, type Statistics } from '../../types/index.js' import { AsyncLock, AsyncLockType, + atomicWriteFileSync, ensureError, handleFileException, JSONStringify, @@ -14,36 +14,24 @@ import { import { Storage } from './Storage.js' export class JsonFileStorage extends Storage { - private fd?: number - constructor (storageUri: string, logPrefix: string) { super(storageUri, logPrefix) - this.dbName = this.storageUri.pathname + // Decode `file:` URIs into a native filesystem path; `URL.pathname` would yield + // `/C:/...` on Windows which is not usable as-is. Other schemes (typically a relative + // path passed as `jsonfile:./...`) keep `pathname` semantics for backward compatibility. + this.dbName = + this.storageUri.protocol === 'file:' + ? fileURLToPath(this.storageUri) + : this.storageUri.pathname } public close (): void { this.clearPerformanceStatistics() - try { - if (this.fd != null) { - closeSync(this.fd) - delete this.fd - } - } catch (error) { - handleFileException( - this.dbName, - FileType.PerformanceRecords, - ensureError(error), - this.logPrefix - ) - } } public open (): void { try { - if (this.fd == null) { - this.ensureDBDirectory() - this.fd = openSync(this.dbName, 'w') - } + this.ensureDBDirectory() } catch (error) { handleFileException( this.dbName, @@ -54,32 +42,19 @@ export class JsonFileStorage extends Storage { } } - public storePerformanceStatistics (performanceStatistics: Statistics): void { + public async storePerformanceStatistics (performanceStatistics: Statistics): Promise { this.setPerformanceStatistics(performanceStatistics) - const fd = this.checkPerformanceRecordsFile() - AsyncLock.runExclusive(AsyncLockType.performance, () => { - writeSync( - fd, - JSONStringify([...this.getPerformanceStatistics()], 2, MapStringifyFormat.object), - 0, - 'utf8' - ) - }).catch((error: unknown) => { - handleFileException( + await AsyncLock.runExclusive(AsyncLockType.performance, () => { + // Performance records are observability data; skip the per-sample `mkdir` (the + // directory is created by `open()`) and `fsync` (durability across crashes is + // not required for telemetry) to keep the hot path cheap. + atomicWriteFileSync( this.dbName, + JSONStringify([...this.getPerformanceStatistics()], 2, MapStringifyFormat.object), FileType.PerformanceRecords, - ensureError(error), - this.logPrefix + this.logPrefix, + { ensureDir: false, errorParams: { throwError: false }, flush: false } ) }) } - - private checkPerformanceRecordsFile (): number { - if (this.fd == null) { - throw new BaseError( - `${this.logPrefix} Performance records '${this.dbName}' file descriptor not found` - ) - } - return this.fd - } } diff --git a/src/types/FileType.ts b/src/types/FileType.ts index b38814f8..27963a8e 100644 --- a/src/types/FileType.ts +++ b/src/types/FileType.ts @@ -1,5 +1,6 @@ export enum FileType { Authorization = 'authorization', + Certificate = 'certificate', ChargingStationConfiguration = 'charging station configuration', ChargingStationTemplate = 'charging station template', Configuration = 'configuration', diff --git a/src/utils/FileUtils.ts b/src/utils/FileUtils.ts index 756931ea..33d06c78 100644 --- a/src/utils/FileUtils.ts +++ b/src/utils/FileUtils.ts @@ -1,6 +1,21 @@ -import { type FSWatcher, watch, type WatchListener } from 'node:fs' +// Copyright Jerome Benoit. 2021-2025. All Rights Reserved. -import type { FileType } from '../types/index.js' +import { + type FSWatcher, + mkdirSync, + renameSync, + rmSync, + watch, + type WatchListener, + type WriteFileOptions, + writeFileSync, +} from 'node:fs' +import { mkdir, rename, rm, writeFile } from 'node:fs/promises' +import { dirname } from 'node:path' +import { pid } from 'node:process' +import { threadId } from 'node:worker_threads' + +import type { EmptyObject, FileType, HandleErrorParams } from '../types/index.js' import { ensureError, handleFileException } from './ErrorUtils.js' import { logger } from './Logger.js' @@ -8,6 +23,42 @@ import { isNotEmptyString } from './Utils.js' const moduleName = 'FileUtils' +const DEFAULT_FILE_MODE = 0o666 + +let tmpInvocationCounter = 0 + +export interface AtomicWriteOptions { + /** + * Character encoding when `data` is a string. Defaults to `'utf8'`. + */ + encoding?: BufferEncoding + /** + * Whether to call `mkdir(dirname(file), { recursive: true })` before writing. + * Defaults to `true`. + */ + ensureDir?: boolean + /** + * Error handling parameters forwarded to {@link handleFileException}. Defaults + * to `{ throwError: true, consoleOut: false }` (log at error level and rethrow). + */ + errorParams?: HandleErrorParams + /** + * Whether to flush (`fsync`) the temp file to the storage device before renaming. + * Defaults to `true`. + */ + flush?: boolean + /** + * File mode applied at temp file creation; the OS umask is applied on top. The + * destination inherits the temp file's mode after rename. Defaults to `0o666`. + */ + mode?: number +} + +const buildTmpPath = (file: string): string => { + tmpInvocationCounter += 1 + return `${file}.${pid.toString()}.${threadId.toString()}.${tmpInvocationCounter.toString()}.tmp` +} + export const watchJsonFile = ( file: string, fileType: FileType, @@ -28,3 +79,121 @@ export const watchJsonFile = ( ) } } + +/** + * Asynchronously writes `data` to `file` atomically using a write-then-rename strategy. + * + * The data is first written to a unique temporary file in the same directory as `file`, + * optionally flushed to disk via `fsync`, then renamed to `file`. The rename step is + * atomic at the filesystem level, so a concurrent reader observes either the previous + * file content or the complete new content, never a partially written file. + * + * Temporary file name encodes `pid`, `threadId` (0 in the main thread, non-zero per + * worker thread), and a per-thread monotonic counter. This guarantees uniqueness across + * processes and worker threads of the same process. + * + * Concurrent writers to the same `file` must be serialized externally (typically via + * `AsyncLock.runExclusive`); this primitive does not queue, deduplicate, or order + * concurrent calls. The `AsyncLock` instances in the project are per-thread, so when a + * given destination can be written from multiple threads the caller must additionally + * partition paths or coordinate across threads. + * + * Durability: when `flush` is `true` (default) the temporary file is fsync'd before + * `rename`. The parent directory entry is not separately fsync'd, so a kernel-level + * crash between `rename` and the directory inode flush may, on some filesystems, + * revert the rename. This is acceptable for the simulator's persistence needs (config + * files, simulator state, performance records, certificates) but is not full POSIX D + * durability. + * + * On `SIGKILL`, OOM kill, or power loss between `writeFile` and `rename`, the + * temporary `....tmp` artifact may remain on disk; it is inert + * and safe to delete manually. Normal failure paths clean it up best-effort. + * + * On error, the temporary file is removed best-effort and the failure is forwarded to + * {@link handleFileException} using `fileType`, `logPrefix`, and `options.errorParams`. + * @param file - Destination file path. + * @param data - Content to write. + * @param fileType - File type used for error logging. + * @param logPrefix - Caller-supplied log prefix used for error logging. + * @param options - Atomic write options. + * @returns A promise that resolves once the rename has completed. + * @throws {Error} When the write fails and `options.errorParams.throwError !== false` + * (the default). The thrown error is the underlying `NodeJS.ErrnoException` + * re-thrown by {@link handleFileException} after logging. + */ +export const atomicWriteFile = async ( + file: string, + data: NodeJS.ArrayBufferView | string, + fileType: FileType, + logPrefix: string, + options?: AtomicWriteOptions +): Promise => { + const { + encoding = 'utf8', + ensureDir = true, + errorParams, + flush = true, + mode = DEFAULT_FILE_MODE, + } = options ?? {} + const tmpFile = buildTmpPath(file) + try { + if (ensureDir) { + await mkdir(dirname(file), { recursive: true }) + } + await writeFile(tmpFile, data, { encoding, flush, mode }) + await rename(tmpFile, file) + } catch (error) { + try { + await rm(tmpFile, { force: true }) + } catch { + // Ignore secondary cleanup failure to surface the original error. + } + handleFileException(file, fileType, ensureError(error), logPrefix, errorParams) + } +} + +/** + * Synchronous variant of {@link atomicWriteFile}. + * + * Same algorithm and contract as the asynchronous variant. Useful for shutdown paths + * and other synchronous code where awaiting is not possible. + * @param file - Destination file path. + * @param data - Content to write. + * @param fileType - File type used for error logging. + * @param logPrefix - Caller-supplied log prefix used for error logging. + * @param options - Atomic write options. + * @throws {Error} When the write fails and `options.errorParams.throwError !== false` + * (the default). The thrown error is the underlying `NodeJS.ErrnoException` + * re-thrown by {@link handleFileException} after logging. + */ +export const atomicWriteFileSync = ( + file: string, + data: NodeJS.ArrayBufferView | string, + fileType: FileType, + logPrefix: string, + options?: AtomicWriteOptions +): void => { + const { + encoding = 'utf8', + ensureDir = true, + errorParams, + flush = true, + mode = DEFAULT_FILE_MODE, + } = options ?? {} + const tmpFile = buildTmpPath(file) + try { + if (ensureDir) { + mkdirSync(dirname(file), { recursive: true }) + } + const writeOptions: WriteFileOptions = { encoding, flush, mode } + writeFileSync(tmpFile, data, writeOptions) + renameSync(tmpFile, file) + } catch (error) { + try { + rmSync(tmpFile, { force: true }) + } catch { + // Ignore secondary cleanup failure to surface the original error. + } + handleFileException(file, fileType, ensureError(error), logPrefix, errorParams) + } +} diff --git a/src/utils/index.ts b/src/utils/index.ts index ace35ae4..0840b634 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -19,7 +19,12 @@ export { handleUncaughtException, handleUnhandledRejection, } from './ErrorUtils.js' -export { watchJsonFile } from './FileUtils.js' +export { + atomicWriteFile, + atomicWriteFileSync, + type AtomicWriteOptions, + watchJsonFile, +} from './FileUtils.js' export { logger } from './Logger.js' export { buildAddedMessage, diff --git a/tests/charging-station/ocpp/2.0/OCPP20CertificateManager.test.ts b/tests/charging-station/ocpp/2.0/OCPP20CertificateManager.test.ts index b6a0aad9..06b58a21 100644 --- a/tests/charging-station/ocpp/2.0/OCPP20CertificateManager.test.ts +++ b/tests/charging-station/ocpp/2.0/OCPP20CertificateManager.test.ts @@ -57,7 +57,8 @@ await describe('I02-I04 - ISO15118 Certificate Management', async () => { const result = await manager.storeCertificate( TEST_CHARGING_STATION_HASH_ID, TEST_CERT_TYPE, - VALID_PEM_CERTIFICATE_EXTENDED + VALID_PEM_CERTIFICATE_EXTENDED, + 'test |' ) assert.notStrictEqual(result, undefined) @@ -74,7 +75,8 @@ await describe('I02-I04 - ISO15118 Certificate Management', async () => { const result = await manager.storeCertificate( TEST_CHARGING_STATION_HASH_ID, TEST_CERT_TYPE, - INVALID_PEM_CERTIFICATE_MISSING_MARKERS + INVALID_PEM_CERTIFICATE_MISSING_MARKERS, + 'test |' ) assert.notStrictEqual(result, undefined) @@ -89,7 +91,8 @@ await describe('I02-I04 - ISO15118 Certificate Management', async () => { const result = await manager.storeCertificate( TEST_CHARGING_STATION_HASH_ID, TEST_CERT_TYPE, - EMPTY_PEM_CERTIFICATE + EMPTY_PEM_CERTIFICATE, + 'test |' ) assert.notStrictEqual(result, undefined) @@ -101,7 +104,8 @@ await describe('I02-I04 - ISO15118 Certificate Management', async () => { const result = await manager.storeCertificate( TEST_CHARGING_STATION_HASH_ID, InstallCertificateUseEnumType.V2GRootCertificate, - VALID_PEM_CERTIFICATE_EXTENDED + VALID_PEM_CERTIFICATE_EXTENDED, + 'test |' ) assert.notStrictEqual(result, undefined) @@ -411,12 +415,14 @@ await describe('I02-I04 - ISO15118 Certificate Management', async () => { manager.storeCertificate( TEST_CHARGING_STATION_HASH_ID, InstallCertificateUseEnumType.CSMSRootCertificate, - VALID_PEM_CERTIFICATE_EXTENDED + VALID_PEM_CERTIFICATE_EXTENDED, + 'test |' ), manager.storeCertificate( TEST_CHARGING_STATION_HASH_ID, InstallCertificateUseEnumType.V2GRootCertificate, - VALID_PEM_CERTIFICATE_EXTENDED + VALID_PEM_CERTIFICATE_EXTENDED, + 'test |' ), manager.getInstalledCertificates(TEST_CHARGING_STATION_HASH_ID), ]) @@ -433,7 +439,8 @@ await describe('I02-I04 - ISO15118 Certificate Management', async () => { const result = await manager.storeCertificate( TEST_CHARGING_STATION_HASH_ID, TEST_CERT_TYPE, - longChain + longChain, + 'test |' ) assert.notStrictEqual(result, undefined) diff --git a/tests/performance/storage/JsonFileStorage.test.ts b/tests/performance/storage/JsonFileStorage.test.ts new file mode 100644 index 00000000..89fd2790 --- /dev/null +++ b/tests/performance/storage/JsonFileStorage.test.ts @@ -0,0 +1,99 @@ +/** + * @file Tests for JsonFileStorage + * @description Unit tests for the JSON file performance storage backend. + */ +import assert from 'node:assert/strict' +import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, it } from 'node:test' +import { pathToFileURL } from 'node:url' + +import { JsonFileStorage } from '../../../src/performance/storage/JsonFileStorage.js' +import { logger } from '../../../src/utils/index.js' +import { createLoggerMocks, standardCleanup } from '../../helpers/TestLifecycleHelpers.js' +import { buildTestStatistics } from './StorageTestHelpers.js' + +const LOG_PREFIX = 'JsonFileStorage-test |' + +const buildStorageUri = (filePath: string): string => pathToFileURL(filePath).toString() + +await describe('JsonFileStorage', async () => { + let tmpDir: string + let dbPath: string + let storage: JsonFileStorage + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'json-file-storage-test-')) + dbPath = join(tmpDir, 'perf.json') + storage = new JsonFileStorage(buildStorageUri(dbPath), LOG_PREFIX) + storage.open() + }) + + afterEach(() => { + storage.close() + standardCleanup() + rmSync(tmpDir, { force: true, recursive: true }) + }) + + await it('should write performance statistics atomically and leave no temp artifact behind', async () => { + const stats = buildTestStatistics('cs-1') + + await storage.storePerformanceStatistics(stats) + + assert.strictEqual(existsSync(dbPath), true) + const written = JSON.parse(readFileSync(dbPath, 'utf8')) as { id: string }[] + assert.strictEqual(Array.isArray(written), true) + assert.strictEqual(written.length, 1) + assert.strictEqual(written[0].id, 'cs-1') + assert.deepStrictEqual( + readdirSync(tmpDir).filter(name => name.endsWith('.tmp')), + [] + ) + }) + + await it('should overwrite the records file with the latest snapshot on each store call', async () => { + await storage.storePerformanceStatistics(buildTestStatistics('cs-1')) + await storage.storePerformanceStatistics(buildTestStatistics('cs-2')) + + const written = JSON.parse(readFileSync(dbPath, 'utf8')) as { id: string }[] + const ids = written.map(entry => entry.id).sort() + assert.deepStrictEqual(ids, ['cs-1', 'cs-2']) + }) + + await it('should serialize the statisticsData Map via MapStringifyFormat.object', async () => { + const stats = buildTestStatistics('cs-1', 'station-with-map') + + await storage.storePerformanceStatistics(stats) + + const written = JSON.parse(readFileSync(dbPath, 'utf8')) as { + statisticsData: Record + }[] + assert.ok(typeof written[0].statisticsData === 'object', 'statisticsData must be an object') + assert.strictEqual(written[0].statisticsData.Heartbeat.requestCount, 100) + }) + + await it('should log a warning and not throw when the storage directory is removed at runtime', async t => { + const { warnMock } = createLoggerMocks(t, logger) + rmSync(tmpDir, { force: true, recursive: true }) + + await assert.doesNotReject(storage.storePerformanceStatistics(buildTestStatistics('cs-1'))) + + assert.strictEqual(existsSync(dbPath), false) + assert.strictEqual(warnMock.mock.calls.length, 1) + }) + + await it('should reflect every parallel writer in the final snapshot when serialized via AsyncLock', async () => { + const stations = Array.from({ length: 4 }, (_, i) => buildTestStatistics(`cs-${i.toString()}`)) + + await Promise.all(stations.map(async stats => storage.storePerformanceStatistics(stats))) + + const written = JSON.parse(readFileSync(dbPath, 'utf8')) as { id: string }[] + const ids = written.map(entry => entry.id).sort() + assert.deepStrictEqual(ids, ['cs-0', 'cs-1', 'cs-2', 'cs-3']) + assert.deepStrictEqual( + readdirSync(tmpDir).filter(name => name.endsWith('.tmp')), + [] + ) + }) +}) diff --git a/tests/utils/FileUtils.test.ts b/tests/utils/FileUtils.test.ts index 519d95c2..fb0d6923 100644 --- a/tests/utils/FileUtils.test.ts +++ b/tests/utils/FileUtils.test.ts @@ -1,95 +1,293 @@ /** * @file Tests for FileUtils - * @description Unit tests for file watching utility functions + * @description Unit tests for file watching and atomic file write utility functions. */ import assert from 'node:assert/strict' -import { mkdtempSync, rmSync, type WatchListener, writeFileSync } from 'node:fs' +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + statSync, + type WatchListener, + writeFileSync, +} from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, describe, it } from 'node:test' +import { afterEach, beforeEach, describe, it } from 'node:test' import { FileType } from '../../src/types/index.js' -import { watchJsonFile } from '../../src/utils/FileUtils.js' +import { atomicWriteFile, atomicWriteFileSync, watchJsonFile } from '../../src/utils/FileUtils.js' import { logger } from '../../src/utils/index.js' import { createLoggerMocks, standardCleanup } from '../helpers/TestLifecycleHelpers.js' +const LOG_PREFIX = 'FileUtils-test |' + const noop: WatchListener = () => { /* intentionally empty */ } +const listTmpArtifacts = (dir: string, baseName: string): string[] => + readdirSync(dir).filter(name => name.startsWith(`${baseName}.`) && name.endsWith('.tmp')) + await describe('FileUtils', async () => { + let tmpDir: string + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'fileutils-test-')) + }) + afterEach(() => { standardCleanup() + rmSync(tmpDir, { force: true, recursive: true }) }) - await it('should return undefined and log info for empty file path', t => { - const infoMock = t.mock.method(logger, 'info') + await describe('watchJsonFile', async () => { + await it('should return undefined and log info for empty file path', t => { + const infoMock = t.mock.method(logger, 'info') - const result = watchJsonFile('', FileType.Authorization, 'test prefix |', noop) + const result = watchJsonFile('', FileType.Authorization, 'test prefix |', noop) - assert.strictEqual(result, undefined) - assert.strictEqual(infoMock.mock.calls.length, 1) - }) + assert.strictEqual(result, undefined) + assert.strictEqual(infoMock.mock.calls.length, 1) + }) - await it('should include file type and log prefix in info log message for empty path', t => { - const infoMock = t.mock.method(logger, 'info') + await it('should include file type and log prefix in info log message for empty path', t => { + const infoMock = t.mock.method(logger, 'info') - watchJsonFile('', FileType.ChargingStationConfiguration, 'CS-001 |', noop) + watchJsonFile('', FileType.ChargingStationConfiguration, 'CS-001 |', noop) - assert.strictEqual(infoMock.mock.calls.length, 1) - const logMessage = infoMock.mock.calls[0].arguments[0] as unknown as string - assert.ok(logMessage.includes(FileType.ChargingStationConfiguration)) - assert.ok(logMessage.includes('CS-001 |')) - }) + assert.strictEqual(infoMock.mock.calls.length, 1) + const logMessage = infoMock.mock.calls[0].arguments[0] as unknown as string + assert.ok(logMessage.includes(FileType.ChargingStationConfiguration)) + assert.ok(logMessage.includes('CS-001 |')) + }) - await it('should handle watch error and return undefined for nonexistent file', t => { - const { warnMock } = createLoggerMocks(t, logger) + await it('should handle watch error and return undefined for nonexistent file', t => { + const { warnMock } = createLoggerMocks(t, logger) - const result = watchJsonFile( - '/nonexistent/path/to/file.json', - FileType.Authorization, - 'test prefix |', - noop - ) + const result = watchJsonFile( + '/nonexistent/path/to/file.json', + FileType.Authorization, + 'test prefix |', + noop + ) - assert.strictEqual(result, undefined) - assert.strictEqual(warnMock.mock.calls.length, 1) - }) + assert.strictEqual(result, undefined) + assert.strictEqual(warnMock.mock.calls.length, 1) + }) - await it('should return FSWatcher for valid file path', () => { - const tmpDir = mkdtempSync(join(tmpdir(), 'fileutils-test-')) - const tmpFile = join(tmpDir, 'test.json') - writeFileSync(tmpFile, '{}') + await it('should return FSWatcher for valid file path', () => { + const target = join(tmpDir, 'test.json') + writeFileSync(target, '{}') - try { - const result = watchJsonFile(tmpFile, FileType.Authorization, 'test |', noop) + const result = watchJsonFile(target, FileType.Authorization, 'test |', noop) assert.notStrictEqual(result, undefined) result?.close() - } finally { - rmSync(tmpDir, { recursive: true }) - } - }) + }) - await it('should call watch with file and listener arguments', () => { - const tmpDir = mkdtempSync(join(tmpdir(), 'fileutils-test-')) - const tmpFile = join(tmpDir, 'test.json') - writeFileSync(tmpFile, '{}') + await it('should call watch with file and listener arguments', () => { + const target = join(tmpDir, 'test.json') + writeFileSync(target, '{}') - try { let receivedEvent = false const listener: WatchListener = () => { receivedEvent = true } - const result = watchJsonFile(tmpFile, FileType.Authorization, 'test |', listener) + const result = watchJsonFile(target, FileType.Authorization, 'test |', listener) assert.notStrictEqual(result, undefined) assert.strictEqual(typeof result?.close, 'function') result?.close() assert.strictEqual(receivedEvent, false) - } finally { - rmSync(tmpDir, { recursive: true }) - } + }) + }) + + await describe('atomicWriteFileSync', async () => { + await it('should write string content atomically and leave no temp file behind', () => { + const target = join(tmpDir, 'output.json') + + atomicWriteFileSync(target, '{"ok":true}', FileType.SimulatorState, LOG_PREFIX) + + assert.strictEqual(readFileSync(target, 'utf8'), '{"ok":true}') + assert.deepStrictEqual(listTmpArtifacts(tmpDir, 'output.json'), []) + }) + + await it('should write Uint8Array content atomically', () => { + const target = join(tmpDir, 'binary.bin') + const data = new Uint8Array([0x00, 0x01, 0x02, 0x03]) + + atomicWriteFileSync(target, data, FileType.PerformanceRecords, LOG_PREFIX) + + assert.deepStrictEqual(new Uint8Array(readFileSync(target)), data) + }) + + await it('should overwrite an existing file atomically', () => { + const target = join(tmpDir, 'existing.json') + writeFileSync(target, 'old content', 'utf8') + + atomicWriteFileSync(target, 'new content', FileType.SimulatorState, LOG_PREFIX) + + assert.strictEqual(readFileSync(target, 'utf8'), 'new content') + }) + + await it('should create missing parent directories when ensureDir is enabled (default)', () => { + const target = join(tmpDir, 'nested', 'deep', 'output.json') + + atomicWriteFileSync(target, '{}', FileType.SimulatorState, LOG_PREFIX) + + assert.strictEqual(existsSync(target), true) + assert.strictEqual(readFileSync(target, 'utf8'), '{}') + }) + + await it('should fail and clean up the temp file when ensureDir is disabled and the parent does not exist', t => { + const { errorMock } = createLoggerMocks(t, logger) + const parent = join(tmpDir, 'missing-parent') + const target = join(parent, 'output.json') + + assert.throws( + () => { + atomicWriteFileSync(target, '{}', FileType.SimulatorState, LOG_PREFIX, { + ensureDir: false, + }) + }, + { code: 'ENOENT' } + ) + + assert.strictEqual(existsSync(target), false) + assert.strictEqual(existsSync(parent), false) + assert.strictEqual(errorMock.mock.calls.length, 1) + }) + + await it('should not throw when errorParams.throwError is false', t => { + const { errorMock, warnMock } = createLoggerMocks(t, logger) + const target = join(tmpDir, 'missing-parent', 'output.json') + + assert.doesNotThrow(() => { + atomicWriteFileSync(target, '{}', FileType.SimulatorState, LOG_PREFIX, { + ensureDir: false, + errorParams: { throwError: false }, + }) + }) + + assert.strictEqual(existsSync(target), false) + assert.strictEqual(warnMock.mock.calls.length, 1) + assert.strictEqual(errorMock.mock.calls.length, 0) + }) + + await it('should support a custom encoding', () => { + const target = join(tmpDir, 'latin1.txt') + + atomicWriteFileSync(target, 'café', FileType.Configuration, LOG_PREFIX, { + encoding: 'latin1', + }) + + assert.strictEqual(readFileSync(target, 'latin1'), 'café') + }) + }) + + await describe('atomicWriteFile', async () => { + await it('should write string content atomically and leave no temp file behind', async () => { + const target = join(tmpDir, 'output.json') + + await atomicWriteFile(target, '{"ok":true}', FileType.SimulatorState, LOG_PREFIX) + + assert.strictEqual(readFileSync(target, 'utf8'), '{"ok":true}') + assert.deepStrictEqual(listTmpArtifacts(tmpDir, 'output.json'), []) + }) + + await it('should overwrite an existing file atomically', async () => { + const target = join(tmpDir, 'existing.json') + writeFileSync(target, 'old content', 'utf8') + + await atomicWriteFile(target, 'new content', FileType.SimulatorState, LOG_PREFIX) + + assert.strictEqual(readFileSync(target, 'utf8'), 'new content') + }) + + await it('should create missing parent directories when ensureDir is enabled (default)', async () => { + const target = join(tmpDir, 'nested', 'deep', 'output.json') + + await atomicWriteFile(target, '{}', FileType.SimulatorState, LOG_PREFIX) + + assert.strictEqual(existsSync(target), true) + assert.strictEqual(readFileSync(target, 'utf8'), '{}') + }) + + await it('should fail and clean up the temp file when ensureDir is disabled and the parent does not exist', async t => { + const { errorMock } = createLoggerMocks(t, logger) + const parent = join(tmpDir, 'missing-parent') + const target = join(parent, 'output.json') + + await assert.rejects( + atomicWriteFile(target, '{}', FileType.SimulatorState, LOG_PREFIX, { + ensureDir: false, + }), + { code: 'ENOENT' } + ) + + assert.strictEqual(existsSync(target), false) + assert.strictEqual(existsSync(parent), false) + assert.strictEqual(errorMock.mock.calls.length, 1) + }) + + await it('should not throw when errorParams.throwError is false', async t => { + const { errorMock, warnMock } = createLoggerMocks(t, logger) + const target = join(tmpDir, 'missing-parent', 'output.json') + + await assert.doesNotReject( + atomicWriteFile(target, '{}', FileType.SimulatorState, LOG_PREFIX, { + ensureDir: false, + errorParams: { throwError: false }, + }) + ) + + assert.strictEqual(existsSync(target), false) + assert.strictEqual(warnMock.mock.calls.length, 1) + assert.strictEqual(errorMock.mock.calls.length, 0) + }) + + await it('should support concurrent writes to distinct paths without temp-name collisions', async () => { + const targets = Array.from({ length: 8 }, (_, i) => + join(tmpDir, `concurrent-${i.toString()}.txt`) + ) + + await Promise.all( + targets.map(async (target, i) => + atomicWriteFile(target, `payload-${i.toString()}`, FileType.SimulatorState, LOG_PREFIX) + ) + ) + + for (const [i, target] of targets.entries()) { + assert.strictEqual(readFileSync(target, 'utf8'), `payload-${i.toString()}`) + } + assert.deepStrictEqual( + readdirSync(tmpDir).filter(name => name.endsWith('.tmp')), + [] + ) + }) + + await it('should leave the destination intact and clean up the temp file when rename fails', async () => { + const target = join(tmpDir, 'preserved-dir') + mkdirSync(target) + mkdirSync(join(target, 'child')) + + await assert.rejects( + atomicWriteFile(target, 'NEW', FileType.SimulatorState, LOG_PREFIX), + (err: NodeJS.ErrnoException) => + err.code === 'EISDIR' || err.code === 'ENOTEMPTY' || err.code === 'EPERM' + ) + + assert.ok(statSync(target).isDirectory(), 'target directory must remain intact') + assert.deepStrictEqual( + readdirSync(tmpDir).filter(name => name.endsWith('.tmp')), + [], + 'temp file should be cleaned up after rename failure' + ) + }) }) })