Merge pull request #2 from BackendStack21/fix/top-5-improvements

fix: harden ring, add exports map, batch addNodes, numeric hashes (v1.1.1)

Author: jkyberneees

README.md

@@ -32,11 +32,17 @@ import ConsistentHash from "fast-hashring";
 const ch = new ConsistentHash({ virtualNodes: 100 });
 ch.addNode("server1");
 ch.addNode("server2");
+// Or add multiple nodes at once (ring is sorted once, not per node):
+ch.addNodes(["server3", "server4"]);
 
 const node = ch.getNode("my-key");
 console.log(`Key is assigned to node: ${node}`);
 ```
 
+Notes:
+- `virtualNodes` must be a positive integer (validated in the constructor).
+- `getNode` returns `null` when the ring is empty (no nodes added, or all removed).
+
 ### Jump Consistent Hash (no ring, O(1) memory)
 
 Jump Consistent Hash maps a string key to a stable index in [0, N). This is useful when you just need a bucket index (e.g., shard number) without maintaining a ring of nodes.

consistent-hash.fix.test.js

@@ -0,0 +1,141 @@
+import { describe, test, expect } from 'bun:test'
+import ConsistentHash from './consistent-hash.js'
+
+// ── Item 1: constructor validates virtualNodes ──────────────────────────────
+describe('ConsistentHash constructor validation', () => {
+  test('throws on virtualNodes = 0', () => {
+    expect(() => new ConsistentHash({ virtualNodes: 0 })).toThrow()
+  })
+
+  test('throws on negative virtualNodes', () => {
+    expect(() => new ConsistentHash({ virtualNodes: -5 })).toThrow()
+  })
+
+  test('throws on fractional virtualNodes', () => {
+    expect(() => new ConsistentHash({ virtualNodes: 10.5 })).toThrow()
+  })
+
+  test('throws on non-numeric virtualNodes', () => {
+    expect(() => new ConsistentHash({ virtualNodes: '100' })).toThrow()
+  })
+
+  test('accepts a positive integer', () => {
+    const ch = new ConsistentHash({ virtualNodes: 4 })
+    expect(ch.virtualNodes).toBe(4)
+  })
+
+  test('default is 100', () => {
+    const ch = new ConsistentHash()
+    expect(ch.virtualNodes).toBe(100)
+  })
+})
+
+// ── Item 2: ring uses 32-bit numeric hashes (numeric compare, no strings) ───
+describe('numeric ring representation', () => {
+  test('ring entries are 32-bit numbers, not hex strings', () => {
+    const ch = new ConsistentHash({ virtualNodes: 8 })
+    ch.addNode('a')
+    expect(ch.ring.length).toBe(8)
+    for (const h of ch.ring) {
+      expect(typeof h).toBe('number')
+      expect(Number.isInteger(h)).toBe(true)
+      expect(h).toBeGreaterThanOrEqual(0)
+      expect(h).toBeLessThanOrEqual(0xffffffff)
+    }
+  })
+
+  test('ring is sorted numerically ascending', () => {
+    const ch = new ConsistentHash({ virtualNodes: 32 })
+    for (const n of ['a', 'b', 'c']) ch.addNode(n)
+    for (let i = 1; i < ch.ring.length; i++) {
+      expect(ch.ring[i - 1] <= ch.ring[i]).toBe(true)
+    }
+  })
+})
+
+// ── Item 4: batch addNodes sorts once ────────────────────────────────────────
+describe('addNodes batch API', () => {
+  test('addNodes accepts a list and registers all nodes', () => {
+    const ch = new ConsistentHash({ virtualNodes: 16 })
+    ch.addNodes(['a', 'b', 'c'])
+    expect(ch.size()).toBe(3)
+    expect(ch.getNode('key-1')).toBeDefined()
+  })
+
+  test('addNodes rejects an empty or invalid list', () => {
+    const ch = new ConsistentHash({ virtualNodes: 4 })
+    expect(() => ch.addNodes([])).toThrow()
+    expect(() => ch.addNodes('a')).toThrow()
+  })
+
+  test('addNodes rejects duplicate nodes within the batch', () => {
+    const ch = new ConsistentHash({ virtualNodes: 4 })
+    expect(() => ch.addNodes(['a', 'a'])).toThrow()
+  })
+})
+
+// ── Item 5: ring invariants ──────────────────────────────────────────────────
+describe('ring invariants', () => {
+  const keys = Array.from({ length: 2000 }, (_, i) => `key-${i}`)
+
+  test('getNode returns null on an empty ring', () => {
+    const ch = new ConsistentHash({ virtualNodes: 8 })
+    expect(ch.getNode('any-key')).toBeNull()
+  })
+
+  test('getNode output is deterministic', () => {
+    const ch = new ConsistentHash({ virtualNodes: 16 })
+    ch.addNode('a')
+    ch.addNode('b')
+    for (const k of keys) {
+      expect(ch.getNode(k)).toBe(ch.getNode(k))
+    }
+  })
+
+  test('monotonicity: removing a node only remaps its keys', () => {
+    const ch = new ConsistentHash({ virtualNodes: 64 })
+    ch.addNodes(['a', 'b', 'c'])
+    const before = new Map(keys.map((k) => [k, ch.getNode(k)]))
+
+    ch.removeNode('b')
+    const moved = keys.filter((k) => ch.getNode(k) !== before.get(k))
+    // Every moved key must now land on a surviving node.
+    for (const k of moved) {
+      expect(['a', 'c']).toContain(ch.getNode(k))
+    }
+    // Minimal disruption: removed node owned ~1/3 of keys; moved set must be
+    // exactly the set previously owned by 'b' (ring + virtual nodes guarantee).
+    const movedFraction = moved.length / keys.length
+    expect(movedFraction).toBeGreaterThan(0.2)
+    expect(movedFraction).toBeLessThan(0.45)
+  })
+
+  test('monotonicity: adding a node only remaps a minority of keys', () => {
+    const ch = new ConsistentHash({ virtualNodes: 64 })
+    ch.addNodes(['a', 'b'])
+    const before = new Map(keys.map((k) => [k, ch.getNode(k)]))
+    ch.addNode('c')
+    const moved = keys.filter((k) => ch.getNode(k) !== before.get(k)).length / keys.length
+    expect(moved).toBeGreaterThan(0.1) // should win ~1/3 of keys
+    expect(moved).toBeLessThan(0.5)
+  })
+
+  test('only node removal leaves an empty ring and getNode returns null', () => {
+    const ch = new ConsistentHash({ virtualNodes: 8 })
+    ch.addNode('a')
+    ch.removeNode('a')
+    expect(ch.size()).toBe(0)
+    expect(ch.getNode('x')).toBeNull()
+  })
+
+  test('distribution across nodes is balanced within 25%', () => {
+    const ch = new ConsistentHash({ virtualNodes: 100 })
+    ch.addNodes(['a', 'b', 'c'])
+    const counts = { a: 0, b: 0, c: 0 }
+    for (const k of keys) counts[ch.getNode(k)]++
+    const avg = keys.length / 3
+    for (const c of Object.values(counts)) {
+      expect(Math.abs(c - avg)).toBeLessThanOrEqual(avg * 0.25)
+    }
+  })
+})

consistent-hash.js

@@ -1,43 +1,49 @@
-import crypto from 'crypto'
+import crypto from 'node:crypto'
 
 /**
  * ConsistentHash provides an implementation of consistent hashing using virtual nodes
- * for improved key distribution. It maintains a ring structure of hashed virtual node values,
- * mapping them to their corresponding real nodes.
+ * for improved key distribution. It maintains a ring structure of 32-bit hashed virtual
+ * node values, mapping them to their corresponding real nodes.
  *
  * This module is designed to be published on NPM and used as a standalone library.
  *
  * Example usage:
- *   import ConsistentHash from 'consistent-hash'
+ *   import ConsistentHash from 'fast-hashring'
  *   const ch = new ConsistentHash({ virtualNodes: 100 })
  *   ch.addNode('server1')
  *   const node = ch.getNode('my-key')
  *
  * @module consistent-hash
  */
+const UINT32_MAX = 0xffffffff
+
 class ConsistentHash {
   /**
    * Create a new ConsistentHash instance.
    *
    * @param {Object} [options={}] - Configuration options.
-   * @param {number} [options.virtualNodes=100] - Number of virtual nodes per real node.
+   * @param {number} [options.virtualNodes=100] - Number of virtual nodes per real node (positive integer).
    */
   constructor(options = {}) {
-    this.virtualNodes = options.virtualNodes || 100;
-    this.ring = []; // Sorted array of virtual node hashes.
+    const virtualNodes = options.virtualNodes ?? 100;
+    if (!Number.isInteger(virtualNodes) || virtualNodes < 1) {
+      throw new Error('virtualNodes must be a positive integer');
+    }
+    this.virtualNodes = virtualNodes;
+    this.ring = []; // Sorted array of 32-bit virtual node hashes.
     this.nodes = new Set(); // Set of real node identifiers.
-    this.virtualToReal = new Map(); // Maps virtual node hash to real node.
+    this.virtualToReal = new Map(); // Maps 32-bit virtual node hash to real node.
   }
 
   /**
-   * Generate an MD5 hash for a given key.
+   * Generate a 32-bit hash for a given key (first 4 bytes of MD5).
    *
    * @private
    * @param {string} key - The key to hash.
-   * @returns {string} The hexadecimal hash.
+   * @returns {number} The unsigned 32-bit hash.
    */
   _hash(key) {
-    return crypto.createHash('md5').update(key).digest('hex');
+    return crypto.createHash('md5').update(key).digest().readUInt32BE(0);
   }
 
   /**
@@ -57,13 +63,41 @@ class ConsistentHash {
 
     // Create virtual nodes for the real node.
     for (let i = 0; i < this.virtualNodes; i++) {
-      const virtualNode = `${node}-vn-${i}`;
-      const hash = this._hash(virtualNode);
+      const hash = this._hash(`${node}-vn-${i}`);
       this.ring.push(hash);
       this.virtualToReal.set(hash, node);
     }
-    // Keep the ring sorted for efficient binary search.
-    this.ring.sort();
+    // Keep the ring sorted for efficient binary search (numeric comparator).
+    this.ring.sort((a, b) => a - b);
+  }
+
+  /**
+   * Add multiple nodes to the hash ring, sorting the ring only once.
+   *
+   * @param {string[]} nodes - The node identifiers.
+   * @throws {Error} If nodes is not a non-empty array, or any node is invalid/duplicated.
+   */
+  addNodes(nodes) {
+    if (!Array.isArray(nodes) || nodes.length === 0) {
+      throw new Error('Nodes must be a non-empty array');
+    }
+    for (const node of nodes) {
+      if (!node || typeof node !== 'string') {
+        throw new Error('Node must be a non-empty string');
+      }
+      if (this.nodes.has(node) || nodes.indexOf(node) !== nodes.lastIndexOf(node)) {
+        throw new Error('Node already exists');
+      }
+      this.nodes.add(node);
+    }
+    for (const node of nodes) {
+      for (let i = 0; i < this.virtualNodes; i++) {
+        const hash = this._hash(`${node}-vn-${i}`);
+        this.ring.push(hash);
+        this.virtualToReal.set(hash, node);
+      }
+    }
+    this.ring.sort((a, b) => a - b);
   }
 
   /**
@@ -79,13 +113,14 @@ class ConsistentHash {
     this.nodes.delete(node);
 
     // Remove all virtual nodes associated with the given node.
+    const removed = new Set();
     for (let i = 0; i < this.virtualNodes; i++) {
-      const virtualNode = `${node}-vn-${i}`;
-      const hash = this._hash(virtualNode);
+      const hash = this._hash(`${node}-vn-${i}`);
       this.virtualToReal.delete(hash);
+      removed.add(hash);
     }
-    // Rebuild the ring with remaining virtual nodes.
-    this.ring = this.ring.filter((hash) => this.virtualToReal.has(hash));
+    // Rebuild the ring without the removed virtual nodes.
+    this.ring = this.ring.filter((hash) => !removed.has(hash));
   }
 
   /**
@@ -104,19 +139,20 @@ class ConsistentHash {
       return null;
     }
     const hash = this._hash(key);
+    const ring = this.ring;
 
     // Binary search to find the first virtual node hash greater than or equal to the key hash.
-    let low = 0, high = this.ring.length;
+    let low = 0, high = ring.length;
     while (low < high) {
-      const mid = Math.floor((low + high) / 2);
-      if (this.ring[mid] < hash) {
+      const mid = (low + high) >>> 1;
+      if (ring[mid] < hash) {
         low = mid + 1;
       } else {
         high = mid;
       }
     }
-    const index = low % this.ring.length; // Wrap around if needed.
-    return this.virtualToReal.get(this.ring[index]);
+    const index = low % ring.length; // Wrap around if needed.
+    return this.virtualToReal.get(ring[index]);
   }
 
   /**

jump-hash.js

@@ -1,4 +1,4 @@
-import crypto from "crypto";
+import crypto from "node:crypto";
 
 /**
  * JumpConsistentHash maps string keys to an integer index in [0, N)

package.json

@@ -1,9 +1,19 @@
 {
   "name": "fast-hashring",
-  "version": "1.1.0",
-  "description": "A library for consistent hashing.",
+  "version": "1.1.1",
+  "description": "Fast consistent hashing: classic ring with virtual nodes + Jump Consistent Hash (O(1) memory).",
   "main": "consistent-hash.js",
   "type": "module",
+  "exports": {
+    ".": {
+      "types": "./index.d.ts",
+      "import": "./consistent-hash.js"
+    },
+    "./jump-hash.js": {
+      "types": "./jump-hash.d.ts",
+      "import": "./jump-hash.js"
+    }
+  },
   "scripts": {
     "test": "bun test --coverage"
   },

package.test.js

@@ -0,0 +1,32 @@
+import { describe, test, expect } from 'bun:test'
+import fs from 'node:fs'
+import path from 'node:path'
+import { fileURLToPath } from 'node:url'
+
+const __dirname = path.dirname(fileURLToPath(import.meta.url))
+const pkg = JSON.parse(fs.readFileSync(path.join(__dirname, 'package.json'), 'utf8'))
+
+// Item 3: package must expose an exports map covering the documented subpath
+// import `fast-hashring/jump-hash.js` (README), with types.
+describe('package exports map', () => {
+  test('defines "." export with default, import and types', () => {
+    expect(pkg.exports).toBeDefined()
+    expect(pkg.exports['.']).toBeDefined()
+    expect(pkg.exports['.'].import).toBe('./consistent-hash.js')
+    expect(pkg.exports['.'].types).toBeDefined()
+  })
+
+  test('defines "./jump-hash.js" subpath export (documented in README)', () => {
+    expect(pkg.exports['./jump-hash.js']).toBeDefined()
+    expect(pkg.exports['./jump-hash.js'].import).toBe('./jump-hash.js')
+    expect(pkg.exports['./jump-hash.js'].types).toBeDefined()
+  })
+
+  test('uses node: protocol for builtin imports in sources', () => {
+    for (const f of ['consistent-hash.js', 'jump-hash.js']) {
+      const src = fs.readFileSync(path.join(__dirname, f), 'utf8')
+      expect(src).not.toMatch(/from ['"]crypto['"]/)
+      expect(src).toMatch(/from ['"]node:crypto['"]/)
+    }
+  })
+})