From 7f1bad009b1562de45e9c461c846a1dbdef9c607 Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:43:44 +0100 Subject: [PATCH] refactor(#14): Address PR feedback --- README.md | 4 +- docker-compose.yml | 2 +- src/lib/components/NetworkDiagram.svelte | 16 ++-- src/lib/shared/networkPersistenceCore.mjs | 105 +++++++++++++++++++--- tests/networkPersistenceCore.test.mjs | 17 ++++ 5 files changed, 125 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index ae1c8e7..d8297b9 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,7 @@ If you prefer compose: services: open-network-diagram: image: jcreek23/open-network-diagram:latest - user: '${OND_UID:-1000}:${OND_GID:-1000}' + user: '${OND_UID:?Set OND_UID to your host user ID}:${OND_GID:?Set OND_GID to your host group ID}' ports: - '8080:3000' volumes: @@ -144,7 +144,7 @@ mkdir -p data OND_UID="$(id -u)" OND_GID="$(id -g)" docker compose up -d ``` -`OND_UID` and `OND_GID` are used only by Compose to select the container process identity; they are not application environment variables. The defaults suit common Linux installations. Docker Desktop users can remove the `user` line if their file-sharing configuration requires the image's built-in user. +`OND_UID` and `OND_GID` are required Compose interpolation values supplied by the command above; they are not application environment variables. Docker Desktop users can remove the `user` line if their file-sharing configuration requires the image's built-in user. ### Using a Docker Named Volume diff --git a/docker-compose.yml b/docker-compose.yml index c94d90b..9c344d8 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -4,7 +4,7 @@ services: context: . dockerfile: Dockerfile image: open-network-diagram:local - user: '${OND_UID:-1000}:${OND_GID:-1000}' + user: '${OND_UID:?Set OND_UID to your host user ID}:${OND_GID:?Set OND_GID to your host group ID}' ports: - '8080:3000' volumes: diff --git a/src/lib/components/NetworkDiagram.svelte b/src/lib/components/NetworkDiagram.svelte index f13df0e..b7c38bf 100644 --- a/src/lib/components/NetworkDiagram.svelte +++ b/src/lib/components/NetworkDiagram.svelte @@ -301,6 +301,9 @@ let subnetEditorOpen = false; let showRackManager = false; + $: totalEntities = networkData.machines.length + networkData.devices.length; + $: emptyWritableDataset = writable && totalEntities === 0; + // Writable empty datasets use a session-only dismissal so onboarding // returns after reload until the user adds a real entity. Sample-data // dismissal remains persisted for returning demo users. @@ -310,7 +313,7 @@ let emptyFirstRunDismissedThisSession = false; function dismissFirstRun() { - if (writable && totalEntities === 0) { + if (emptyWritableDataset) { emptyFirstRunDismissedThisSession = true; return; } @@ -328,7 +331,6 @@ } } - $: totalEntities = networkData.machines.length + networkData.devices.length; $: allExampleNames = totalEntities > 0 && [ @@ -338,7 +340,7 @@ $: showFirstRun = hasLoadedInitialData && !isLoadingData && - ((writable && totalEntities === 0 && !emptyFirstRunDismissedThisSession) || + ((emptyWritableDataset && !emptyFirstRunDismissedThisSession) || (!firstRunDismissed && totalEntities <= 4 && allExampleNames)); let copiedIp: string | null = null; let copiedIpTimer: ReturnType | null = null; @@ -704,10 +706,12 @@ } function resolveStaticFallbackNotice(reason: string | null): string { - if (!reason) { + const normalizedReason = reason?.trim(); + if (!normalizedReason) { return 'Read-only: API unavailable; using bundled static data.'; } - return `Read-only: ${reason} Using bundled static data.`; + const separator = /[.!?]$/.test(normalizedReason) ? ' ' : '. '; + return `Read-only: ${normalizedReason}${separator}Using bundled static data.`; } // Nodes are invisible hit targets (interaction, layout, edge anchoring); @@ -2527,7 +2531,7 @@ isSampleData={allExampleNames} {writable} hasMachines={networkData.machines.length > 0} - storageReady={writable && totalEntities === 0} + storageReady={emptyWritableDataset} on:addmachine={addMachine} on:connectport={openFirstMachineEditor} on:openipam={() => (showIpamPanel = true)} diff --git a/src/lib/shared/networkPersistenceCore.mjs b/src/lib/shared/networkPersistenceCore.mjs index 2e8d7a2..95b9111 100644 --- a/src/lib/shared/networkPersistenceCore.mjs +++ b/src/lib/shared/networkPersistenceCore.mjs @@ -19,7 +19,7 @@ import path from 'node:path'; * @typedef {{ status: 'ready'; data: NetworkData; source: string; updatedAt: string } | { status: 'missing' | 'blank'; source: string; updatedAt: string | null }} NetworkFileReadState * @typedef {NetworkFileReadResult & { initialized: boolean }} NetworkFileBootstrapResult * @typedef {{ writable: boolean; reason: string | null }} WritableState - * @typedef {{ createBackup?: boolean }} WriteNetworkFileOptions + * @typedef {{ createBackup?: boolean; beforeCommit?: () => Promise }} WriteNetworkFileOptions */ const NETWORK_READ_ONLY = process.env.NETWORK_READ_ONLY === 'true'; @@ -29,6 +29,12 @@ const EMPTY_NETWORK_DATA = Object.freeze({ machines: Object.freeze([]), devices: Object.freeze([]) }); +/** @type {Map>} */ +const fileWriteOperations = new Map(); +/** @type {Map>} */ +const initializationFlights = new Map(); + +class InitializationSupersededError extends Error {} /** * @returns {string} @@ -193,21 +199,18 @@ export async function trimBackups(backupDirectory, baseName, keep = 5) { /** * @param {NetworkData} data + * @param {string} source * @param {WriteNetworkFileOptions} [options] * @returns {Promise} */ -export async function writeNetworkFile(data, options) { +async function writeNetworkFileUnlocked(data, source, options) { if (!isWriteEnabled()) { throw new Error('Writes disabled by NETWORK_READ_ONLY=true.'); } - const source = resolveDataFilePath(); const backupDirectory = resolveBackupDirectory(); const directory = path.dirname(source); await mkdir(directory, { recursive: true }); - if (options?.createBackup ?? true) { - await createBackupIfPresent(source, backupDirectory); - } const temporaryPath = `${source}.tmp-${Date.now()}-${process.pid}`; const jsonPayload = `${JSON.stringify(data, null, '\t')}\n`; @@ -219,10 +222,14 @@ export async function writeNetworkFile(data, options) { await fileHandle.close(); } try { + await options?.beforeCommit?.(); + if (options?.createBackup ?? true) { + await createBackupIfPresent(source, backupDirectory); + } await rename(temporaryPath, source); - } catch (renameError) { + } catch (error) { await unlink(temporaryPath).catch(() => undefined); - throw renameError; + throw error; } await trimBackups(backupDirectory, path.basename(source), 5); @@ -234,6 +241,36 @@ export async function writeNetworkFile(data, options) { }; } +/** + * @template T + * @param {string} source + * @param {() => Promise} operation + * @returns {Promise} + */ +async function withFileWriteLock(source, operation) { + const previousOperation = fileWriteOperations.get(source) ?? Promise.resolve(); + const currentOperation = previousOperation.catch(() => undefined).then(operation); + fileWriteOperations.set(source, currentOperation); + + try { + return await currentOperation; + } finally { + if (fileWriteOperations.get(source) === currentOperation) { + fileWriteOperations.delete(source); + } + } +} + +/** + * @param {NetworkData} data + * @param {WriteNetworkFileOptions} [options] + * @returns {Promise} + */ +export function writeNetworkFile(data, options) { + const source = resolveDataFilePath(); + return withFileWriteLock(source, () => writeNetworkFileUnlocked(data, source, options)); +} + /** * @returns {Promise} */ @@ -302,7 +339,7 @@ export async function checkWritableState() { * * @returns {Promise} */ -export async function readOrInitializeNetworkFile() { +async function initializeNetworkFile() { const state = await readNetworkFileState(); if (state.status === 'ready') { return { @@ -323,7 +360,34 @@ export async function readOrInitializeNetworkFile() { machines: [...EMPTY_NETWORK_DATA.machines], devices: [...EMPTY_NETWORK_DATA.devices] }; - const metadata = await writeNetworkFile(data, { createBackup: false }); + let metadata; + try { + metadata = await withFileWriteLock(state.source, () => + writeNetworkFileUnlocked(data, state.source, { + createBackup: false, + beforeCommit: async () => { + const currentState = await readNetworkFileState(); + if (currentState.status === 'ready') { + throw new InitializationSupersededError(); + } + } + }) + ); + } catch (error) { + if (!(error instanceof InitializationSupersededError)) { + throw error; + } + const currentState = await readNetworkFileState(); + if (currentState.status !== 'ready') { + throw new Error(`Network data file "${currentState.source}" is ${currentState.status}.`); + } + return { + data: currentState.data, + source: currentState.source, + updatedAt: currentState.updatedAt, + initialized: false + }; + } return { data, source: metadata.source, @@ -331,3 +395,24 @@ export async function readOrInitializeNetworkFile() { initialized: true }; } + +/** + * @returns {Promise} + */ +export function readOrInitializeNetworkFile() { + const source = resolveDataFilePath(); + const existingFlight = initializationFlights.get(source); + if (existingFlight) { + return existingFlight; + } + + const flight = initializeNetworkFile(); + initializationFlights.set(source, flight); + const clearFlight = () => { + if (initializationFlights.get(source) === flight) { + initializationFlights.delete(source); + } + }; + flight.then(clearFlight, clearFlight); + return flight; +} diff --git a/tests/networkPersistenceCore.test.mjs b/tests/networkPersistenceCore.test.mjs index f500d0d..bf64268 100644 --- a/tests/networkPersistenceCore.test.mjs +++ b/tests/networkPersistenceCore.test.mjs @@ -89,6 +89,23 @@ test('does not rewrite an existing valid empty network', async () => { }); }); +test('does not overwrite real data written while a missing file is being initialized', async () => { + await withPersistenceEnvironment(async ({ persistence, source }) => { + const realData = { + machines: [{ machineName: 'Router' }], + devices: [] + }; + + const bootstrap = persistence.readOrInitializeNetworkFile(); + const write = persistence.writeNetworkFile(realData, { createBackup: false }); + const [bootstrapResult] = await Promise.all([bootstrap, write]); + + assert.equal(bootstrapResult.initialized, false); + assert.deepEqual(bootstrapResult.data, realData); + assert.deepEqual(JSON.parse(await readFile(source, 'utf8')), realData); + }); +}); + test('does not replace malformed or schema-invalid non-empty JSON', async () => { await withPersistenceEnvironment(async ({ persistence, source }) => { await persistence.writeNetworkFile({ machines: [], devices: [] }, { createBackup: false });