From 6cbb1032dd9e5e56b5dd7eadad16dd50971b0c45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 10:05:51 +0200 Subject: [PATCH 01/11] fix: move cache and singleton state onto class instance --- src/NtpTimeSync.ts | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/NtpTimeSync.ts b/src/NtpTimeSync.ts index 159693c..c4d8c71 100644 --- a/src/NtpTimeSync.ts +++ b/src/NtpTimeSync.ts @@ -5,15 +5,6 @@ import { NtpPacket, NtpPacketParser } from "ntp-packet-parser"; import { NtpTimeResult } from "./NtpTimeResult.js"; import { RecursivePartial } from "./RecursivePartial.js"; -let singleton: NtpTimeSync | undefined; -let lastPoll: number | undefined; -let lastResult: - | undefined - | { - offset: number; - precision: number; - }; - export interface NtpTimeSyncConstructorOptions { servers: string[]; sampleCount: number; @@ -78,8 +69,17 @@ interface SampleData { } export class NtpTimeSync { + private static singleton: NtpTimeSync | undefined; + private options: NtpTimeSyncOptions; private samples: SampleData[] = []; + private lastPoll: number | undefined; + private lastResult: + | undefined + | { + offset: number; + precision: number; + }; constructor(options: RecursivePartial = {}) { const serverConfig = options.servers || NtpTimeSyncDefaultOptions.servers; @@ -143,11 +143,11 @@ export class NtpTimeSync { * Returns a singleton */ static getInstance(options: RecursivePartial = {}): NtpTimeSync { - if (!singleton) { - singleton = new NtpTimeSync(options); + if (!NtpTimeSync.singleton) { + NtpTimeSync.singleton = new NtpTimeSync(options); } - return singleton; + return NtpTimeSync.singleton; } private async collectSamples(numSamples: number) { @@ -234,17 +234,17 @@ export class NtpTimeSync { async getTime(force = false): Promise { if ( !force && - lastPoll && - lastResult && - Date.now() - lastPoll < Math.pow(2, this.options.ntpDefaults.minPoll) * 1000 + this.lastPoll && + this.lastResult && + Date.now() - this.lastPoll < Math.pow(2, this.options.ntpDefaults.minPoll) * 1000 ) { let date = new Date(); - date.setUTCMilliseconds(date.getUTCMilliseconds() + lastResult.offset); + date.setUTCMilliseconds(date.getUTCMilliseconds() + this.lastResult.offset); return { now: date, - offset: lastResult.offset, - precision: lastResult.precision, + offset: this.lastResult.offset, + precision: this.lastResult.precision, }; } @@ -259,11 +259,11 @@ export class NtpTimeSync { const precision = NtpTimeSync.stdDev(this.samples.map((sample) => sample.offset)); - lastResult = { + this.lastResult = { offset: offset, precision: precision, }; - lastPoll = Date.now(); + this.lastPoll = Date.now(); let date = new Date(); date.setUTCMilliseconds(date.getUTCMilliseconds() + offset); From 1d208513f8d03f8534d144f5420e219e1937e9b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 10:10:15 +0200 Subject: [PATCH 02/11] ci: verify published artifact with npm pack, publint, and attw Add pre-publish artifact verification to catch broken files/exports, wrong entry paths, or incorrect types resolution before 0.6.0 ships. Runs as a dedicated verify-package job on Node 22 LTS (artifact check, not runtime check) that needs the build matrix to succeed first. - npm pack --dry-run: confirm tarball manifest is producible - publint: lint package.json exports/main/module/types/files - @arethetypeswrong/cli: verify TS consumers (node16/bundler) resolve working types for both CJS and ESM entrypoints Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/nodejs.yml | 22 ++++++++++++++++++++++ package.json | 8 +++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index 2aa3b8f..bb96d1a 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -37,6 +37,28 @@ jobs: - name: Run examples run: node examples/example.js + verify-package: + runs-on: ubuntu-latest + needs: build + steps: + - uses: actions/checkout@v5 + - name: Use Node.js 22 (LTS) + uses: actions/setup-node@v5 + with: + node-version: '22' + check-latest: true + - name: Cache Node.js modules + id: yarn-cache + uses: actions/cache@v5 + with: + path: '**/node_modules' + key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}-22 + - name: Install dependencies + if: steps.yarn-cache.outputs.cache-hit != 'true' + run: yarn install + - name: Verify published artifact (pack, publint, attw) + run: yarn verify:package + deno: runs-on: ubuntu-latest strategy: diff --git a/package.json b/package.json index 944de65..8f3475d 100644 --- a/package.json +++ b/package.json @@ -24,8 +24,10 @@ "ntp-packet-parser": "^0.6.0" }, "devDependencies": { + "@arethetypeswrong/cli": "0.18.2", "@types/node": "^20 || ^22 || ^24 || ^25", "prettier": "3.8.3", + "publint": "0.3.18", "ts-node": "10.9.2", "typescript": "6.0.3" }, @@ -40,7 +42,11 @@ "test:integration:deno": "deno run --allow-read test/integration/deno.ts && deno run --allow-read test/integration/deno-esm.ts", "test:integration": "yarn test:integration:cjs && yarn test:integration:esm", "prettier": "prettier --write src/**/*.ts", - "prettier:lint": "prettier --list-different src/**/*.ts" + "prettier:lint": "prettier --list-different src/**/*.ts", + "verify:pack": "npm pack --dry-run", + "verify:publint": "publint", + "verify:attw": "attw --pack .", + "verify:package": "yarn build && yarn verify:pack && yarn verify:publint && yarn verify:attw" }, "keywords": [ "ntp", From 6b2b18f4a52b0bb9d791e90f4d19a8dea13b6249 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 11:24:54 +0200 Subject: [PATCH 03/11] perf: rewrite createPacket with Buffer.writeBigUInt64BE Replace Array(48) + pad()/parseInt()/substr() chain with Buffer.alloc(48) plus direct byte assignment and buf.writeBigUInt64BE for the origin and transmit timestamps. Drops the now-unused pad() helper. The new implementation splits the NTP 64-bit timestamp into whole seconds and fractional seconds before combining via BigInt so the fractional portion no longer loses low-order bits to the Number mantissa during the legacy (seconds * 2**32) float round-trip. --- src/NtpTimeSync.ts | 58 ++++++++++++---------------------------------- 1 file changed, 15 insertions(+), 43 deletions(-) diff --git a/src/NtpTimeSync.ts b/src/NtpTimeSync.ts index c4d8c71..5bd6dd6 100644 --- a/src/NtpTimeSync.ts +++ b/src/NtpTimeSync.ts @@ -286,14 +286,6 @@ export class NtpTimeSync { return now; } - private static pad(string: string, length: number, char = "0", side: "left" | "right" = "left") { - if (side === "left") { - return char.repeat(length).substring(0, length - string.length) + string; - } - - return string + char.repeat(length).substring(0, length - string.length); - } - /** * @param {Integer} leapIndicator, defaults to 3 (unsynchronized) * @param {Integer} ntpVersion, defaults to `options.ntpDefaults.version` @@ -303,46 +295,26 @@ export class NtpTimeSync { private createPacket(leapIndicator = 3, ntpVersion: number | undefined = undefined, mode = 3): Buffer { ntpVersion = ntpVersion || this.options.ntpDefaults.version; - // generate NTP packet - let ntpData = new Array(48).fill(0); + const buf = Buffer.alloc(48); - ntpData[0] = - // Leap indicator (= 3, unsynchronized) - NtpTimeSync.pad((leapIndicator >>> 0).toString(2), 2) + - // NTP version (= 4) - NtpTimeSync.pad((ntpVersion >>> 0).toString(2), 3) + - // client mode (= 3) - NtpTimeSync.pad((mode >>> 0).toString(2), 3); + // Leap indicator (2 bits) | NTP version (3 bits) | mode (3 bits) + buf[0] = ((leapIndicator & 0x3) << 6) | ((ntpVersion & 0x7) << 3) | (mode & 0x7); - ntpData[0] = parseInt(ntpData[0], 2); + // origin timestamp: seconds since 1900 epoch in upper 32 bits, + // fractional seconds (scaled by 2^32) in lower 32 bits + const baseTimeMs = new Date().getTime() - this.options.ntpDefaults.referenceDate.getTime(); + const seconds = Math.trunc(baseTimeMs / 1000); + const fractional = Math.trunc(((baseTimeMs % 1000) / 1000) * 2 ** 32); + const mask32 = BigInt("0xffffffff"); + const shift32 = BigInt(32); + const ntpTimestamp = ((BigInt(seconds) & mask32) << shift32) | (BigInt(fractional) & mask32); // origin timestamp - const baseTime = new Date().getTime() - this.options.ntpDefaults.referenceDate.getTime(); - const seconds = baseTime / 1000; - let ntpTimestamp = (seconds * Math.pow(2, 32)).toString(2); - ntpTimestamp = NtpTimeSync.pad(ntpTimestamp, 64); - - // origin timestamp - ntpData[24] = parseInt(ntpTimestamp.substr(0, 8), 2); - ntpData[25] = parseInt(ntpTimestamp.substr(8, 8), 2); - ntpData[26] = parseInt(ntpTimestamp.substr(16, 8), 2); - ntpData[27] = parseInt(ntpTimestamp.substr(24, 8), 2); - ntpData[28] = parseInt(ntpTimestamp.substr(32, 8), 2); - ntpData[29] = parseInt(ntpTimestamp.substr(40, 8), 2); - ntpData[30] = parseInt(ntpTimestamp.substr(48, 8), 2); - ntpData[31] = parseInt(ntpTimestamp.substr(56, 8), 2); - + buf.writeBigUInt64BE(ntpTimestamp, 24); // transmit timestamp - ntpData[40] = parseInt(ntpTimestamp.substr(0, 8), 2); - ntpData[41] = parseInt(ntpTimestamp.substr(8, 8), 2); - ntpData[42] = parseInt(ntpTimestamp.substr(16, 8), 2); - ntpData[43] = parseInt(ntpTimestamp.substr(24, 8), 2); - ntpData[44] = parseInt(ntpTimestamp.substr(32, 8), 2); - ntpData[45] = parseInt(ntpTimestamp.substr(40, 8), 2); - ntpData[46] = parseInt(ntpTimestamp.substr(48, 8), 2); - ntpData[47] = parseInt(ntpTimestamp.substr(56, 8), 2); - - return Buffer.from(ntpData); + buf.writeBigUInt64BE(ntpTimestamp, 40); + + return buf; } private static cleanup(client: dgram.Socket) { From 460208387987babe5fcb8220f907116ff0c5a37b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 11:25:30 +0200 Subject: [PATCH 04/11] fix: narrow NTP reply field validation in acceptResponse acceptResponse previously validated only originTimestamp, leaving transmitTimestamp, receiveTimestamp, and precision as non-null-asserted in collectSamples. Add explicit undefined checks for those fields in acceptResponse (throwing the same Format error pattern as existing checks) so malformed replies are rejected before they become samples, and drop the ! assertions in collectSamples. A guard in the forEach provides type narrowing that is now backed by the validator. --- src/NtpTimeSync.ts | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/src/NtpTimeSync.ts b/src/NtpTimeSync.ts index 5bd6dd6..b031ee0 100644 --- a/src/NtpTimeSync.ts +++ b/src/NtpTimeSync.ts @@ -186,10 +186,20 @@ export class NtpTimeSync { // filter erroneous responses, use valid ones as samples let samples: SampleData[] = []; ntpResults.forEach((data) => { - const transmitTimestamp = data.transmitTimestamp!; - const receiveTimestamp = data.receiveTimestamp!; - const originTimestamp = data.originTimestamp!; - const precision = data.precision!; + const transmitTimestamp = data.transmitTimestamp; + const receiveTimestamp = data.receiveTimestamp; + const originTimestamp = data.originTimestamp; + const precision = data.precision; + + // acceptResponse has already validated these fields; narrow for the type system + if ( + transmitTimestamp === undefined || + receiveTimestamp === undefined || + originTimestamp === undefined || + precision === undefined + ) { + return; + } const offsetSign = transmitTimestamp.getTime() > data.destinationTimestamp.getTime() ? 1 : -1; @@ -430,6 +440,19 @@ export class NtpTimeSync { if (data.originTimestamp === undefined || data.originTimestamp.getTime() > new Date().getTime()) { throw new Error("Format error: Origin timestamp is from the future"); } + + /* + * Verify remaining fields required for sample computation + */ + if (data.transmitTimestamp === undefined) { + throw new Error("Format error: Missing transmit timestamp"); + } + if (data.receiveTimestamp === undefined) { + throw new Error("Format error: Missing receive timestamp"); + } + if (data.precision === undefined) { + throw new Error("Format error: Missing precision"); + } } /** From f8377c39f2885a84608b2baa5cd2011d33c33b93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 11:25:54 +0200 Subject: [PATCH 05/11] fix: register message listener before send to avoid missed-reply race The previous ordering registered client.once("message") inside the send() callback, so an NTP reply that arrived after the packet was on the wire but before Node fired the send callback would hit no listener and be silently dropped; the caller would then wait the full replyTimeout before rejecting. Attach the message listener up front and keep hasFinished/timeoutHandler guards so the timeout, send errors, and socket errors still short-circuit the promise correctly. --- src/NtpTimeSync.ts | 57 ++++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/src/NtpTimeSync.ts b/src/NtpTimeSync.ts index b031ee0..fbb0098 100644 --- a/src/NtpTimeSync.ts +++ b/src/NtpTimeSync.ts @@ -363,42 +363,45 @@ export class NtpTimeSync { errorCallback(new Error("Timeout waiting for NTP response.")); }, this.options.replyTimeout); - client.send(this.createPacket(), port, server, (err: Error | null) => { + // Register the message listener BEFORE sending the packet so we never + // miss an unusually fast reply that arrives between send() completing + // and the send callback firing. + client.once("message", (msg: Buffer) => { if (hasFinished) { return; } - if (err) { - errorCallback(err); + clearTimeout(timeoutHandler); + timeoutHandler = undefined; + client.close(); + + let parsed: Partial; + try { + parsed = NtpPacketParser.parse(msg); + } catch (err) { + hasFinished = true; + reject(err); return; } - client.once("message", function (msg: Buffer) { - if (hasFinished) { - return; - } + const result: NtpReceivedPacket = { + ...parsed, + destinationTimestamp: new Date(), + }; - clearTimeout(timeoutHandler); - timeoutHandler = undefined; - client.close(); - - let parsed: Partial; - try { - parsed = NtpPacketParser.parse(msg); - } catch (err) { - hasFinished = true; - reject(err); - return; - } - - const result: NtpReceivedPacket = { - ...parsed, - destinationTimestamp: new Date(), - }; + hasFinished = true; + resolve(result); + }); - hasFinished = true; - resolve(result); - }); + client.send(this.createPacket(), port, server, (err: Error | null) => { + if (hasFinished) { + return; + } + + if (err) { + errorCallback(err); + return; + } }); }); } From fdc1924e477c67db77bc924b6772e7bf8995a2c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 11:26:13 +0200 Subject: [PATCH 06/11] fix: increment retry when round yields no new samples The retry counter only advanced when ntpResults was still empty, so any partial success that produced fewer than numSamples usable packets (e.g. one server responding, the rest persistently timing out) could spin the do/while loop indefinitely because retry stayed at 0. Snapshot the result count before each round and bump retry when the post-round count is unchanged, keeping the existing retry < 3 cap. --- src/NtpTimeSync.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/NtpTimeSync.ts b/src/NtpTimeSync.ts index fbb0098..8412ab0 100644 --- a/src/NtpTimeSync.ts +++ b/src/NtpTimeSync.ts @@ -167,6 +167,8 @@ export class NtpTimeSync { ); }); + const prevResultCount = ntpResults.length; + // wait for NTP responses to arrive ntpResults = ntpResults .concat(await Promise.all(timePromises.map((p) => p.catch((e) => e)))) @@ -174,7 +176,9 @@ export class NtpTimeSync { return !(result instanceof Error); }); - if (ntpResults.length === 0) { + // count a retry whenever a full round produced no new usable samples; + // otherwise partial progress could loop forever against a slow server set + if (ntpResults.length === prevResultCount) { retry++; } } while (ntpResults.length < numSamples && retry < 3); From 893ed711c2dddec19cb9706dab8ea45d9a32b4a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 11:26:39 +0200 Subject: [PATCH 07/11] fix: harden socket and listener cleanup on error paths Two related leaks: (1) dgram.send can throw synchronously when the socket is in a bad state, and the throw was not caught, so the timeout handler, UDP socket, and registered listeners all leaked and the surrounding promise was never rejected; wrap the send call so we clear the timeout, run NtpTimeSync.cleanup, and reject exactly once. (2) NtpTimeSync.cleanup only called client.close(), leaving "message" and "error" listeners attached on an abandoned socket where a late event could still fire resolve/reject or prevent the process from exiting; call client.removeAllListeners() before close(). --- src/NtpTimeSync.ts | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/NtpTimeSync.ts b/src/NtpTimeSync.ts index 8412ab0..933127a 100644 --- a/src/NtpTimeSync.ts +++ b/src/NtpTimeSync.ts @@ -332,6 +332,14 @@ export class NtpTimeSync { } private static cleanup(client: dgram.Socket) { + try { + // Drop all listeners first so late-arriving error/message events on an + // already-abandoned socket cannot trigger resolve/reject a second time + // or keep the event loop alive. + client.removeAllListeners(); + } catch (e) { + // ignore, as we just want to cleanup + } try { client.close(); } catch (e) { @@ -397,16 +405,31 @@ export class NtpTimeSync { resolve(result); }); - client.send(this.createPacket(), port, server, (err: Error | null) => { - if (hasFinished) { - return; - } + try { + client.send(this.createPacket(), port, server, (err: Error | null) => { + if (hasFinished) { + return; + } - if (err) { - errorCallback(err); - return; + if (err) { + errorCallback(err); + return; + } + }); + } catch (err) { + // dgram.send can throw synchronously (e.g. when the packet cannot be + // constructed or the socket is already in an unusable state) - make + // sure we still tear the socket down and reject the pending promise. + if (timeoutHandler !== undefined) { + clearTimeout(timeoutHandler); + timeoutHandler = undefined; } - }); + NtpTimeSync.cleanup(client); + if (!hasFinished) { + hasFinished = true; + reject(err); + } + } }); } From 5c1032e993de73a97347f1db28008572f708fb0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 11:29:30 +0200 Subject: [PATCH 08/11] fix: lock down public API with explicit exports and frozen defaults --- src/NtpTimeSync.ts | 24 ++++++++++++++++++++++-- src/index.ts | 9 +++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/NtpTimeSync.ts b/src/NtpTimeSync.ts index 933127a..768242e 100644 --- a/src/NtpTimeSync.ts +++ b/src/NtpTimeSync.ts @@ -31,7 +31,27 @@ export interface NtpTimeSyncOptions extends Omit; } -export const NtpTimeSyncDefaultOptions = { +// Recursively freeze an object tree so consumers cannot mutate shared defaults. +// Arrays are frozen along with each of their entries. +const deepFreeze = (value: T): T => { + if (value === null || typeof value !== "object" || Object.isFrozen(value)) { + return value; + } + + if (Array.isArray(value)) { + for (const entry of value) { + deepFreeze(entry); + } + } else { + for (const key of Object.keys(value as object)) { + deepFreeze((value as Record)[key]); + } + } + + return Object.freeze(value); +}; + +export const NtpTimeSyncDefaultOptions = deepFreeze({ // list of NTP time servers, optionally including a port (defaults to options.ntpDefaults.port = 123) servers: ["0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org", "3.pool.ntp.org"], @@ -55,7 +75,7 @@ export const NtpTimeSyncDefaultOptions = { precision: -18, referenceDate: new Date("Jan 01 1900 GMT"), }, -}; +}); interface NtpReceivedPacket extends Partial { destinationTimestamp: Date; diff --git a/src/index.ts b/src/index.ts index 3fe158d..f6e02db 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,2 +1,7 @@ -export * from "./NtpTimeResult.js"; -export * from "./NtpTimeSync.js"; +export type { NtpTimeResult } from "./NtpTimeResult.js"; +export { + NtpTimeSync, + NtpTimeSyncDefaultOptions, + type NtpTimeSyncConstructorOptions, + type NtpTimeSyncOptions, +} from "./NtpTimeSync.js"; From 40910719851461716bf34291f83f5234481625df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 11:38:01 +0200 Subject: [PATCH 09/11] test: add unit tests for protocol logic and recent fixes --- package.json | 1 + scripts/run-unit-tests.mjs | 53 +++++++++ test/unit/acceptResponse.test.ts | 87 +++++++++++++++ test/unit/collectSamples-retry.test.ts | 97 ++++++++++++++++ test/unit/createPacket.test.ts | 105 ++++++++++++++++++ test/unit/helpers/ntpFixture.ts | 146 +++++++++++++++++++++++++ test/unit/instance-isolation.test.ts | 83 ++++++++++++++ test/unit/samples-math.test.ts | 111 +++++++++++++++++++ test/unit/socket-cleanup.test.ts | 102 +++++++++++++++++ 9 files changed, 785 insertions(+) create mode 100644 scripts/run-unit-tests.mjs create mode 100644 test/unit/acceptResponse.test.ts create mode 100644 test/unit/collectSamples-retry.test.ts create mode 100644 test/unit/createPacket.test.ts create mode 100644 test/unit/helpers/ntpFixture.ts create mode 100644 test/unit/instance-isolation.test.ts create mode 100644 test/unit/samples-math.test.ts create mode 100644 test/unit/socket-cleanup.test.ts diff --git a/package.json b/package.json index 8f3475d..e8b38fe 100644 --- a/package.json +++ b/package.json @@ -41,6 +41,7 @@ "test:integration:esm": "node test/integration/esm.mjs", "test:integration:deno": "deno run --allow-read test/integration/deno.ts && deno run --allow-read test/integration/deno-esm.ts", "test:integration": "yarn test:integration:cjs && yarn test:integration:esm", + "test:unit": "node scripts/run-unit-tests.mjs", "prettier": "prettier --write src/**/*.ts", "prettier:lint": "prettier --list-different src/**/*.ts", "verify:pack": "npm pack --dry-run", diff --git a/scripts/run-unit-tests.mjs b/scripts/run-unit-tests.mjs new file mode 100644 index 0000000..5ec6db5 --- /dev/null +++ b/scripts/run-unit-tests.mjs @@ -0,0 +1,53 @@ +// Runner for the unit test suite. +// +// Discovers every test/unit/**/*.test.ts file and spawns `node --test` with +// ts-node's transpile-only CJS hook registered. Uses TS_NODE_COMPILER_OPTIONS +// to force CommonJS compilation with bundler resolution so that the source's +// `.js`-suffixed ESM-style imports resolve correctly under Yarn PnP without +// pulling in a PnP ESM loader. + +import { readdirSync, statSync } from "node:fs"; +import { join, relative } from "node:path"; +import { spawnSync } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +const projectRoot = fileURLToPath(new URL("..", import.meta.url)); +const testRoot = join(projectRoot, "test", "unit"); + +function collectTestFiles(dir) { + const out = []; + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + const st = statSync(full); + if (st.isDirectory()) { + out.push(...collectTestFiles(full)); + } else if (st.isFile() && entry.endsWith(".test.ts")) { + out.push(relative(projectRoot, full)); + } + } + return out; +} + +const testFiles = collectTestFiles(testRoot).sort(); +if (testFiles.length === 0) { + console.error("No test files found under test/unit/"); + process.exit(1); +} + +const result = spawnSync( + process.execPath, + ["--test", "--require", "ts-node/register/transpile-only", ...testFiles], + { + stdio: "inherit", + cwd: projectRoot, + env: { + ...process.env, + TS_NODE_COMPILER_OPTIONS: JSON.stringify({ + module: "commonjs", + moduleResolution: "bundler", + }), + }, + } +); + +process.exit(result.status ?? 1); diff --git a/test/unit/acceptResponse.test.ts b/test/unit/acceptResponse.test.ts new file mode 100644 index 0000000..af8deb8 --- /dev/null +++ b/test/unit/acceptResponse.test.ts @@ -0,0 +1,87 @@ +// Regression tests for acceptResponse's field validation. +// The method is private; we call it through a TypeScript cast to keep the +// test decoupled from the src/ visibility model (no production change). + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { NtpTimeSync } from "../../src/NtpTimeSync"; + +type AcceptResponseFn = (data: Record) => void; + +function getAcceptResponse(sync: NtpTimeSync): AcceptResponseFn { + const fn = (sync as unknown as { acceptResponse: AcceptResponseFn }).acceptResponse; + assert.equal(typeof fn, "function", "acceptResponse is present on the instance"); + return fn.bind(sync) as AcceptResponseFn; +} + +function baseValidPacket(): Record { + const now = new Date(); + // rootDelay / rootDispersion are Dates relative to the 1900 NTP epoch once + // parsed; use the epoch itself so (rootDelay - epoch)/1000 = 0. + const epoch1900 = new Date("Jan 01 1900 GMT"); + return { + version: 4, + leapIndicator: 0, + stratum: 2, + rootDelay: epoch1900, + rootDispersion: epoch1900, + originTimestamp: new Date(now.getTime() - 10), + receiveTimestamp: now, + transmitTimestamp: now, + precision: -18, + }; +} + +test("acceptResponse accepts a schema-valid packet with all required fields", () => { + const sync = new NtpTimeSync(); + const accept = getAcceptResponse(sync); + assert.doesNotThrow(() => accept(baseValidPacket())); +}); + +test("acceptResponse rejects packet missing originTimestamp", () => { + const sync = new NtpTimeSync(); + const accept = getAcceptResponse(sync); + const pkt = baseValidPacket(); + delete pkt.originTimestamp; + assert.throws(() => accept(pkt), /origin timestamp/i); +}); + +test("acceptResponse rejects packet missing transmitTimestamp", () => { + const sync = new NtpTimeSync(); + const accept = getAcceptResponse(sync); + const pkt = baseValidPacket(); + delete pkt.transmitTimestamp; + assert.throws(() => accept(pkt), /missing transmit timestamp/i); +}); + +test("acceptResponse rejects packet missing receiveTimestamp", () => { + const sync = new NtpTimeSync(); + const accept = getAcceptResponse(sync); + const pkt = baseValidPacket(); + delete pkt.receiveTimestamp; + assert.throws(() => accept(pkt), /missing receive timestamp/i); +}); + +test("acceptResponse rejects packet missing precision", () => { + const sync = new NtpTimeSync(); + const accept = getAcceptResponse(sync); + const pkt = baseValidPacket(); + delete pkt.precision; + assert.throws(() => accept(pkt), /missing precision/i); +}); + +test("acceptResponse rejects packet with originTimestamp in the future", () => { + const sync = new NtpTimeSync(); + const accept = getAcceptResponse(sync); + const pkt = baseValidPacket(); + pkt.originTimestamp = new Date(Date.now() + 60_000); + assert.throws(() => accept(pkt), /from the future/i); +}); + +test("acceptResponse rejects unsynchronized stratum (leapIndicator=3)", () => { + const sync = new NtpTimeSync(); + const accept = getAcceptResponse(sync); + const pkt = baseValidPacket(); + pkt.leapIndicator = 3; + assert.throws(() => accept(pkt), /unsynchronized/i); +}); diff --git a/test/unit/collectSamples-retry.test.ts b/test/unit/collectSamples-retry.test.ts new file mode 100644 index 0000000..8c525f5 --- /dev/null +++ b/test/unit/collectSamples-retry.test.ts @@ -0,0 +1,97 @@ +// Regression tests for collectSamples' retry accounting. +// The loop must increment `retry` on no-progress rounds and bail after 3 +// such rounds, preventing an infinite loop against a server set that never +// produces enough valid samples. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { NtpTimeSync } from "../../src/NtpTimeSync"; +import { buildNtpResponse, startFakeNtpServer } from "./helpers/ntpFixture"; + +test("collectSamples bails out after 3 no-progress retry rounds when no samples arrive", async () => { + // Server never replies; every round produces zero new samples → retry++ + // on each iteration. With replyTimeout=200ms and retry limit=3, total + // wall time is bounded near 4 * 200ms = ~800ms. + const server = await startFakeNtpServer(() => null); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 4, + replyTimeout: 150, + }); + + const start = Date.now(); + await assert.rejects( + () => sync.getTime(true), + /Unable to get any NTP response/i, + "getTime must reject with a connection error after retries exhaust" + ); + const elapsed = Date.now() - start; + + // 4 rounds * 150ms timeout = 600ms base. Allow generous safety margin + // but ensure we didn't loop unbounded. + assert.ok(elapsed < 3000, `must bail out quickly, took ${elapsed}ms`); + assert.ok(elapsed >= 400, `should observe at least a few timeouts, took ${elapsed}ms`); + } finally { + await server.close(); + } +}); + +test("collectSamples terminates when partial samples never reach target count", async () => { + // Server responds to every request, but we ask for more samples than we + // can produce in a single round. Because every round produces the SAME + // new sample count, ntpResults grows → retry stays at 0 for progress + // rounds and increments when the round produces duplicates / no growth. + // With 1 server + sampleCount=5, each round adds exactly 1 sample → no + // progress after round 1 because ntpResults.length keeps increasing... + // actually it IS progress. So to force termination we use sampleCount + // HIGHER than achievable and verify the implementation doesn't spin + // forever. + let requestCount = 0; + const server = await startFakeNtpServer(() => { + requestCount++; + // Return a malformed packet (wrong length) so the parser throws and + // acceptResponse never gets a valid sample. This keeps ntpResults + // length at 0, triggering the no-progress retry path. + return Buffer.alloc(10); // invalid length, parser will reject + }); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 3, + replyTimeout: 150, + }); + + const start = Date.now(); + await assert.rejects( + () => sync.getTime(true), + /Unable to get any NTP response/i, + "must reject when parser rejects every response" + ); + const elapsed = Date.now() - start; + + // 4 rounds should have occurred before bailing. + assert.ok(requestCount >= 3, `server should have received ≥3 requests, got ${requestCount}`); + assert.ok(elapsed < 3000, `bounded termination time, got ${elapsed}ms`); + } finally { + await server.close(); + } +}); + +test("collectSamples produces a result when sampleCount is satisfied in one round", async () => { + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [ + `127.0.0.1:${server.port}`, + `127.0.0.1:${server.port}`, + ], + sampleCount: 2, + replyTimeout: 500, + }); + const result = await sync.getTime(true); + assert.ok(Number.isFinite(result.offset), "offset produced when targets met in one round"); + } finally { + await server.close(); + } +}); diff --git a/test/unit/createPacket.test.ts b/test/unit/createPacket.test.ts new file mode 100644 index 0000000..ec673e0 --- /dev/null +++ b/test/unit/createPacket.test.ts @@ -0,0 +1,105 @@ +// Regression tests for the NTP v4 packet layout produced by NtpTimeSync. +// The packet builder is private; we observe it indirectly by sending a real +// getTime() call against an in-process UDP server that captures the datagram. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { NtpTimeSync } from "../../src/NtpTimeSync"; +import { buildNtpResponse, startFakeNtpServer } from "./helpers/ntpFixture"; + +const NTP_EPOCH_OFFSET_SECONDS = 2208988800; + +function readNtpTimestampAsDate(buf: Buffer, offset: number): Date { + const value = buf.readBigUInt64BE(offset); + const mask32 = BigInt("0xffffffff"); + const shift32 = BigInt(32); + const seconds = Number(value >> shift32); + const fractional = Number(value & mask32); + const unixMs = (seconds - NTP_EPOCH_OFFSET_SECONDS) * 1000 + (fractional / 2 ** 32) * 1000; + return new Date(unixMs); +} + +test("createPacket produces a 48-byte NTP v4 client packet", async () => { + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + await sync.getTime(true); + + assert.ok(server.receivedPackets.length >= 1, "server received at least one packet"); + const packet = server.receivedPackets[0]; + assert.equal(packet.length, 48, "packet length is 48 bytes"); + + // Byte 0: LI (2) | VN (3) | Mode (3) + // Default leapIndicator = 3 (unsynchronized), version = 4, mode = 3 (client) + const byte0 = packet[0]; + const leapIndicator = (byte0 >> 6) & 0x3; + const version = (byte0 >> 3) & 0x7; + const mode = byte0 & 0x7; + assert.equal(leapIndicator, 3, "leap indicator bits 6-7 encode default value 3"); + assert.equal(version, 4, "version bits 3-5 encode NTP v4"); + assert.equal(mode, 3, "mode bits 0-2 encode client (3)"); + } finally { + await server.close(); + } +}); + +test("createPacket writes a BE 64-bit timestamp at offset 24 that matches offset 40", async () => { + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + const sentAt = Date.now(); + await sync.getTime(true); + const receivedAt = Date.now(); + + const packet = server.receivedPackets[0]; + + const originRaw = packet.readBigUInt64BE(24); + const transmitRaw = packet.readBigUInt64BE(40); + assert.equal(originRaw, transmitRaw, "origin (24) and transmit (40) timestamps are written with the same value"); + + const decoded = readNtpTimestampAsDate(packet, 24); + const decodedMs = decoded.getTime(); + // Allow generous envelope for wall-clock jitter; timestamps must fall + // between the pre-call time and post-call time plus safety margin. + assert.ok( + decodedMs >= sentAt - 5 && decodedMs <= receivedAt + 50, + `decoded timestamp ${decodedMs} should be between ${sentAt - 5} and ${receivedAt + 50}` + ); + } finally { + await server.close(); + } +}); + +test("createPacket round-trips an NTP timestamp through big-endian write/read", async () => { + // Independently verify the big-endian encoding logic against a known Date. + // This re-implements the src-side math to lock the byte layout in place. + const knownDate = new Date("2024-06-15T12:34:56.789Z"); + const baseMs = knownDate.getTime() - new Date("Jan 01 1900 GMT").getTime(); + const seconds = Math.trunc(baseMs / 1000); + const fractional = Math.trunc(((baseMs % 1000) / 1000) * 2 ** 32); + const mask32 = BigInt("0xffffffff"); + const shift32 = BigInt(32); + const ntpTimestamp = ((BigInt(seconds) & mask32) << shift32) | (BigInt(fractional) & mask32); + + const buf = Buffer.alloc(48); + buf.writeBigUInt64BE(ntpTimestamp, 24); + buf.writeBigUInt64BE(ntpTimestamp, 40); + + // High 32 bits stored at offset 24 (big-endian) should decode back to seconds. + const decodedSeconds = buf.readUInt32BE(24); + const decodedFraction = buf.readUInt32BE(28); + assert.equal(decodedSeconds, seconds, "seconds word stored big-endian at offset 24"); + assert.equal(decodedFraction, fractional, "fraction word stored big-endian at offset 28"); + + // Same for offset 40. + assert.equal(buf.readUInt32BE(40), seconds); + assert.equal(buf.readUInt32BE(44), fractional); +}); diff --git a/test/unit/helpers/ntpFixture.ts b/test/unit/helpers/ntpFixture.ts new file mode 100644 index 0000000..ac4936a --- /dev/null +++ b/test/unit/helpers/ntpFixture.ts @@ -0,0 +1,146 @@ +// Helpers for building synthetic NTP response packets and running an in-process +// UDP server. Kept deterministic and offline so the unit suite can complete +// in well under a second without any external network access. + +import * as dgram from "node:dgram"; +import { AddressInfo } from "node:net"; + +const NTP_EPOCH_OFFSET_SECONDS = 2208988800; // 1970-01-01 in NTP seconds-since-1900 + +export interface BuildPacketOptions { + leapIndicator?: number; + version?: number; + mode?: number; + stratum?: number; + poll?: number; + /** Raw byte value (0-255). -18 would be written as 0xee (two's complement). */ + precisionByte?: number; + /** Root delay in seconds (added to NTP epoch when written). */ + rootDelaySeconds?: number; + rootDispersionSeconds?: number; + referenceId?: number; + referenceTimestamp?: Date; + originTimestamp?: Date; + receiveTimestamp?: Date; + transmitTimestamp?: Date; + /** + * When true, zero out the 64-bit slot for the field. This lets tests + * produce packets whose parsed timestamps come back as 1900-01-01 to + * validate acceptResponse missing-field handling while still keeping a + * schema-correct 48-byte buffer. + */ + blankReceiveTimestamp?: boolean; + blankTransmitTimestamp?: boolean; + blankOriginTimestamp?: boolean; +} + +function writeNtpTimestamp(buf: Buffer, offset: number, date: Date): void { + // Convert a JS Date to an NTP 64-bit timestamp and write big-endian. + const unixMs = date.getTime(); + const totalSeconds = unixMs / 1000 + NTP_EPOCH_OFFSET_SECONDS; + const seconds = Math.trunc(totalSeconds); + const fractional = Math.trunc((totalSeconds - seconds) * 2 ** 32); + const mask32 = BigInt("0xffffffff"); + const shift32 = BigInt(32); + const value = ((BigInt(seconds) & mask32) << shift32) | (BigInt(fractional) & mask32); + buf.writeBigUInt64BE(value, offset); +} + +function writeSecondsSinceEpoch(buf: Buffer, offset: number, seconds: number): void { + // rootDelay/rootDispersion use a 32-bit "seconds since NTP epoch" field + // that ntp-packet-parser converts into a Date relative to 1900-01-01. + const scaled = Math.trunc(seconds); + buf.writeUInt32BE(scaled, offset); +} + +/** + * Build a 48-byte NTP response packet. Defaults produce a schema-valid, sane + * packet that acceptResponse should accept. + */ +export function buildNtpResponse(opts: BuildPacketOptions = {}): Buffer { + const now = new Date(); + const buf = Buffer.alloc(48); + + const leapIndicator = opts.leapIndicator ?? 0; + const version = opts.version ?? 4; + const mode = opts.mode ?? 4; // server + buf[0] = ((leapIndicator & 0x3) << 6) | ((version & 0x7) << 3) | (mode & 0x7); + + buf[1] = opts.stratum ?? 2; + buf[2] = opts.poll ?? 4; + // precision byte: default 0xee == -18 two's complement, interpreted by + // ntp-packet-parser as the raw unsigned byte 238. acceptResponse only + // checks "!== undefined", so any non-missing byte is accepted. + buf[3] = opts.precisionByte ?? 0xee; + + writeSecondsSinceEpoch(buf, 4, opts.rootDelaySeconds ?? 0); + writeSecondsSinceEpoch(buf, 8, opts.rootDispersionSeconds ?? 0); + buf.writeUInt32BE(opts.referenceId ?? 0, 12); + + // reference timestamp (offset 16) + writeNtpTimestamp(buf, 16, opts.referenceTimestamp ?? now); + // origin timestamp (offset 24) + if (!opts.blankOriginTimestamp) { + writeNtpTimestamp(buf, 24, opts.originTimestamp ?? new Date(now.getTime() - 1)); + } + // receive timestamp (offset 32) + if (!opts.blankReceiveTimestamp) { + writeNtpTimestamp(buf, 32, opts.receiveTimestamp ?? now); + } + // transmit timestamp (offset 40) + if (!opts.blankTransmitTimestamp) { + writeNtpTimestamp(buf, 40, opts.transmitTimestamp ?? now); + } + + return buf; +} + +export interface FakeNtpServer { + port: number; + receivedPackets: Buffer[]; + close(): Promise; +} + +/** + * Start a UDP server on 127.0.0.1:0 that echoes a synthetic reply for every + * inbound NTP request. The reply is produced by `buildReply(requestBuffer)`. + * Returning `null` suppresses the reply (tests can simulate drop / timeout). + */ +export async function startFakeNtpServer( + buildReply: (request: Buffer) => Buffer | null +): Promise { + const socket = dgram.createSocket("udp4"); + const received: Buffer[] = []; + + socket.on("message", (msg: Buffer, rinfo) => { + received.push(Buffer.from(msg)); + const reply = buildReply(msg); + if (reply) { + socket.send(reply, 0, reply.length, rinfo.port, rinfo.address); + } + }); + + await new Promise((resolve, reject) => { + socket.once("error", reject); + socket.bind(0, "127.0.0.1", () => { + socket.removeListener("error", reject); + resolve(); + }); + }); + + const addr = socket.address() as AddressInfo; + + return { + port: addr.port, + receivedPackets: received, + close: () => + new Promise((resolve) => { + socket.removeAllListeners(); + try { + socket.close(() => resolve()); + } catch { + resolve(); + } + }), + }; +} diff --git a/test/unit/instance-isolation.test.ts b/test/unit/instance-isolation.test.ts new file mode 100644 index 0000000..fb3743c --- /dev/null +++ b/test/unit/instance-isolation.test.ts @@ -0,0 +1,83 @@ +// Regression test for the module-level-state → instance-state refactor. +// Two independent NtpTimeSync instances with different server lists must +// maintain independent caches. This test fails against the old code where +// lastResult / samples lived at module scope. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { NtpTimeSync } from "../../src/NtpTimeSync"; +import { buildNtpResponse, startFakeNtpServer } from "./helpers/ntpFixture"; + +test("two NtpTimeSync instances with different servers keep independent caches", async () => { + // Server A always reports a time 1 second BEHIND wall clock. + const serverA = await startFakeNtpServer(() => { + const skewed = new Date(Date.now() - 1000); + return buildNtpResponse({ + receiveTimestamp: skewed, + transmitTimestamp: skewed, + originTimestamp: new Date(Date.now() - 5), + }); + }); + // Server B always reports a time 1 second AHEAD of wall clock. + const serverB = await startFakeNtpServer(() => { + const skewed = new Date(Date.now() + 1000); + return buildNtpResponse({ + receiveTimestamp: skewed, + transmitTimestamp: skewed, + originTimestamp: new Date(Date.now() - 5), + }); + }); + + try { + const syncA = new NtpTimeSync({ + servers: [`127.0.0.1:${serverA.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + const syncB = new NtpTimeSync({ + servers: [`127.0.0.1:${serverB.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + + const resultA = await syncA.getTime(true); + const resultB = await syncB.getTime(true); + + // Offsets must reflect each server's intentional skew, not be cross- + // contaminated by the other instance. + assert.ok(resultA.offset < -500, `A should report negative offset (~-1000ms), got ${resultA.offset}`); + assert.ok(resultB.offset > 500, `B should report positive offset (~+1000ms), got ${resultB.offset}`); + + // Re-read cache; non-forced calls must return each instance's own value. + const cachedA = await syncA.getTime(false); + const cachedB = await syncB.getTime(false); + assert.equal(cachedA.offset, resultA.offset, "A's cache survives B's invocation"); + assert.equal(cachedB.offset, resultB.offset, "B's cache survives A's invocation"); + + // And they are distinct. + assert.notEqual(cachedA.offset, cachedB.offset, "A and B have different cached offsets"); + + // Internal state inspection (via TS cast): samples arrays must be + // separate Array instances, not a shared module-level variable. + const samplesA = (syncA as unknown as { samples: unknown[] }).samples; + const samplesB = (syncB as unknown as { samples: unknown[] }).samples; + assert.notEqual(samplesA, samplesB, "samples arrays are separate objects per instance"); + } finally { + await serverA.close(); + await serverB.close(); + } +}); + +test("instance options are not shared between constructions", () => { + const a = new NtpTimeSync({ servers: ["a.example"], sampleCount: 2 }); + const b = new NtpTimeSync({ servers: ["b.example"], sampleCount: 7 }); + + const optsA = (a as unknown as { options: { servers: Array<{ host: string }>; sampleCount: number } }).options; + const optsB = (b as unknown as { options: { servers: Array<{ host: string }>; sampleCount: number } }).options; + + assert.equal(optsA.servers[0].host, "a.example"); + assert.equal(optsB.servers[0].host, "b.example"); + assert.equal(optsA.sampleCount, 2); + assert.equal(optsB.sampleCount, 7); + assert.notEqual(optsA, optsB, "options objects are distinct"); +}); diff --git a/test/unit/samples-math.test.ts b/test/unit/samples-math.test.ts new file mode 100644 index 0000000..90c664a --- /dev/null +++ b/test/unit/samples-math.test.ts @@ -0,0 +1,111 @@ +// Regression tests for the offset/delay/dispersion math in collectSamples. +// The math lives in a private method; we validate it through the public +// getTime() path against a deterministic fake server. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { NtpTimeSync } from "../../src/NtpTimeSync"; +import { buildNtpResponse, startFakeNtpServer } from "./helpers/ntpFixture"; + +test("getTime returns a result with offset and precision fields", async () => { + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + const result = await sync.getTime(true); + assert.ok(result.now instanceof Date, "result.now is a Date"); + assert.equal(typeof result.offset, "number", "result.offset is numeric"); + assert.equal(typeof result.precision, "number", "result.precision is numeric"); + assert.ok(Number.isFinite(result.offset), "offset is finite"); + assert.ok(result.precision >= 0, "precision (stddev) is non-negative"); + } finally { + await server.close(); + } +}); + +test("getTime with receive=transmit=now from fake server yields near-zero offset", async () => { + // When a server's receive and transmit timestamps match the client's + // destination time, the offset formula ((|T2-T1| + |T3-T4|) / 2) must + // be small — bounded by RTT and wall-clock jitter. + const server = await startFakeNtpServer((req) => { + // Echo the request timestamp back as origin; use current time for + // receive/transmit to minimize apparent offset. + const now = new Date(); + return buildNtpResponse({ + receiveTimestamp: now, + transmitTimestamp: now, + originTimestamp: new Date(Date.now() - 1), + }); + }); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + const result = await sync.getTime(true); + // Loopback RTT should be <100ms; offset magnitude must stay bounded. + assert.ok( + Math.abs(result.offset) < 500, + `|offset| should be under 500ms against a local fake, got ${result.offset}` + ); + } finally { + await server.close(); + } +}); + +test("getTime caches result across invocations within minPoll window", async () => { + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + await sync.getTime(true); + const firstPacketCount = server.receivedPackets.length; + + // Non-forced call within minPoll window should hit the cache. + await sync.getTime(false); + const secondPacketCount = server.receivedPackets.length; + + assert.equal( + secondPacketCount, + firstPacketCount, + "cached getTime must not send another packet" + ); + + // Forced call bypasses the cache. + await sync.getTime(true); + assert.ok( + server.receivedPackets.length > secondPacketCount, + "force=true must trigger a new network round" + ); + } finally { + await server.close(); + } +}); + +test("getTime with 3 samples averages offsets across responses", async () => { + // Three back-to-back identical responses must produce a stable offset. + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [ + `127.0.0.1:${server.port}`, + `127.0.0.1:${server.port}`, + `127.0.0.1:${server.port}`, + ], + sampleCount: 3, + replyTimeout: 500, + }); + const result = await sync.getTime(true); + assert.ok(Number.isFinite(result.offset)); + assert.ok(result.precision >= 0); + } finally { + await server.close(); + } +}); diff --git a/test/unit/socket-cleanup.test.ts b/test/unit/socket-cleanup.test.ts new file mode 100644 index 0000000..70ca906 --- /dev/null +++ b/test/unit/socket-cleanup.test.ts @@ -0,0 +1,102 @@ +// Regression tests for socket lifecycle: cleanup uses removeAllListeners + +// close, and a synchronous throw from createPacket/send must not leak a +// dgram socket. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import * as dgram from "node:dgram"; +import { NtpTimeSync } from "../../src/NtpTimeSync"; +import { startFakeNtpServer, buildNtpResponse } from "./helpers/ntpFixture"; + +interface ActiveHandle { + constructor?: { name?: string }; +} + +function countUdpHandles(): number { + // Use Node's debug API to count active UDP socket handles. This is + // deprecated/private but stable across LTS versions and suits a + // regression check that cleanup() actually releases the handle. + const getActiveHandles = (process as unknown as { _getActiveHandles?: () => ActiveHandle[] })._getActiveHandles; + if (typeof getActiveHandles !== "function") return -1; + return getActiveHandles + .call(process) + .filter((h) => h?.constructor?.name === "UDP" || h?.constructor?.name === "Socket") + .length; +} + +test("socket handles do not accumulate across repeated getNetworkTime calls", async () => { + // Fix #6 guards against socket leaks. A leak would show up as a steadily + // growing active-handle count across many iterations. Run enough rounds + // to notice any retained-per-call handle. + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + + // Warm-up to stabilize the baseline + await sync.getTime(true); + // Give Node one tick to close sockets from the warm-up round + await new Promise((r) => setImmediate(r)); + const baseline = countUdpHandles(); + + for (let i = 0; i < 10; i++) { + await sync.getTime(true); + } + await new Promise((r) => setImmediate(r)); + + const after = countUdpHandles(); + // Tolerate +1 for the fake server's own socket variations; any larger + // delta implies a leaked client socket per iteration. + assert.ok( + after - baseline <= 1, + `UDP handle count grew from ${baseline} to ${after}, suggesting a socket leak` + ); + } finally { + await server.close(); + } +}); + +test("getNetworkTime rejects and cleans up when sending to an unreachable port", async () => { + // Closed loopback port → kernel sends ICMP unreachable → socket errors. + // This exercises the error-callback path where cleanup must remove all + // listeners and close the socket before rejecting. + const sync = new NtpTimeSync({ + servers: ["127.0.0.1:1"], + sampleCount: 1, + replyTimeout: 300, + }); + + await assert.rejects(() => sync.getNetworkTime("127.0.0.1", 1)); + // If cleanup failed, a UDP handle would linger past a tick. + await new Promise((r) => setImmediate(r)); + // No assertion on count because other tests may have warmed handles up; + // we just assert the rejection path completes without hanging. +}); + +test("reply that arrives before send-callback completes is still captured", async () => { + // This indirectly verifies fix #4: the message listener must be wired + // up BEFORE client.send() is called. Our fake server responds + // synchronously as soon as it receives a packet, which stresses that + // ordering — if the message listener were registered after send, a + // very fast reply could slip through the gap. Against a loopback UDP + // server the round-trip is microseconds, so this tightens the race + // window. + const server = await startFakeNtpServer(() => buildNtpResponse()); + try { + const sync = new NtpTimeSync({ + servers: [`127.0.0.1:${server.port}`], + sampleCount: 1, + replyTimeout: 500, + }); + // Run several iterations to exercise the race window repeatedly. + for (let i = 0; i < 5; i++) { + const result = await sync.getTime(true); + assert.ok(result.now instanceof Date, `iteration ${i}: message was captured`); + } + } finally { + await server.close(); + } +}); From 8f40049c0bacbe15ba1c28a6d2fa04ea1f7ab359 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 12:24:16 +0200 Subject: [PATCH 10/11] chore: prefix repository.url with git+ for publint compliance --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e8b38fe..91da806 100644 --- a/package.json +++ b/package.json @@ -60,7 +60,7 @@ "author": "Laurens Stötzel", "repository": { "type": "git", - "url": "https://github.com/buffcode/ntp-time-sync.git" + "url": "git+https://github.com/buffcode/ntp-time-sync.git" }, "files": [ "dist" From 00c02b0d16c5d657906eee2532beec3add70ed6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Wed, 22 Apr 2026 14:58:30 +0200 Subject: [PATCH 11/11] ci: key node_modules cache on package.json instead of gitignored yarn.lock The cache key used hashFiles('**/yarn.lock'), but yarn.lock is gitignored and thus absent from CI checkouts. hashFiles returned an empty hash, collapsing the key to a constant (e.g. Linux-modules--22) that never invalidated across commits. This caused verify-package to restore a stale node_modules from before commit 1d20851 and skip install (cache-hit guard), so newly added devDependencies publint and @arethetypeswrong/cli were missing: /bin/sh: 1: publint: not found (exit 127). Switch all three jobs (build, verify-package, deno) to key on package.json, which is tracked and changes when deps change. --- .github/workflows/nodejs.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index bb96d1a..3e84f5a 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -24,7 +24,7 @@ jobs: uses: actions/cache@v5 with: path: '**/node_modules' - key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}-${{ matrix.node-version }} + key: ${{ runner.os }}-modules-${{ hashFiles('**/package.json') }}-${{ matrix.node-version }} - name: Install dependencies if: steps.yarn-cache.outputs.cache-hit != 'true' run: yarn install @@ -52,7 +52,7 @@ jobs: uses: actions/cache@v5 with: path: '**/node_modules' - key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}-22 + key: ${{ runner.os }}-modules-${{ hashFiles('**/package.json') }}-22 - name: Install dependencies if: steps.yarn-cache.outputs.cache-hit != 'true' run: yarn install @@ -76,7 +76,7 @@ jobs: uses: actions/cache@v5 with: path: '**/node_modules' - key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}-${{ matrix.node-version }}-deno + key: ${{ runner.os }}-modules-${{ hashFiles('**/package.json') }}-${{ matrix.node-version }}-deno - name: Install dependencies if: steps.yarn-cache.outputs.cache-hit != 'true' run: yarn install