From fb377e0b8b98744d1d3bc292018640142c732299 Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 21 Aug 2026 16:36:40 -0400 Subject: [PATCH] fix(plugin): never read an empty peer list as "this node owns every URL"; v0.50.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `util/residency.js` captured the cluster node list once, at module load, from `server.nodes`. That value is derived state, not configuration: harper-pro initialises it to `[]` at module scope and only fills it when `subscribeToNodeUpdates` runs a full `hdb_nodes` scan, so anything evaluated before that scan sees no peers on a healthy multi-node cluster — and that ordering repeats for every worker start, not only the first. With no peers, rendezvous hashing makes the local node the owner of every URL. `setResidencyById` then reported self for every key, so Harper stored every RenderSchedule row locally instead of routing it to its owner — silently and permanently, because nothing deletes a schedule row from a node that does not own it, and the non-owner's own reschedule routes away from the row it just claimed. One unlucky evaluation therefore poisoned a worker for its whole lifetime; this has been observed in production affecting a multi-percent share of a corpus. The list is now read per call. A list with no usable peer in it means "not known yet", whether it is empty or populated only by nameless descriptors: the last list that did contain a peer wins over it, while any list that contains one is adopted at once so a genuine membership change still takes effect. A node that has never seen a peer warns and owns everything — correct for a single-node deployment, a misconfiguration on a cluster — rather than throwing, which would make Harper drop the record instead of misplacing it. Note that the later `hdb_nodes` rebuilds are NOT windows of their own: the scan is a plain synchronous `for...of`, and the per-update path filters and pushes before its first await, so both are atomic to any observer. They matter only as evidence that this list is rebuilt from a table rather than fixed at boot, which is why capturing it once is wrong in principle. Also fixes two smaller faults on the same path: - A nameless node descriptor (which `knownNodes.ts` can push after a decode miss) entered the ring as `undefined`, and returning it read as a residency excluding every node, so Harper stored the record nowhere at all. - `peer.js`'s `isKnownNode` guard read the same stale snapshot and rejected legitimate peers, breaking the explainer's cross-node fetch. It reads through the new accessor and heals with it. The hash and the mapping are untouched. Duplicate names are now collapsed, which cannot move a key because HRW takes the max over the names present; a test pins that, and a four-node mapping fixture, so no refactor can silently re-shard a running keyspace. Co-Authored-By: Claude Opus 5 --- package-lock.json | 2 +- packages/plugin/README.md | 15 ++ packages/plugin/package.json | 2 +- packages/plugin/src/util/peer.js | 4 +- packages/plugin/src/util/residency.js | 81 +++++++++- packages/plugin/test/residency.test.js | 205 +++++++++++++++++++++++++ 6 files changed, 303 insertions(+), 6 deletions(-) create mode 100644 packages/plugin/test/residency.test.js diff --git a/package-lock.json b/package-lock.json index 392263e..3b1eac1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8570,7 +8570,7 @@ }, "packages/plugin": { "name": "@harperfast/prerender", - "version": "0.49.0", + "version": "0.50.1", "license": "Apache-2.0", "dependencies": { "fast-xml-parser": "^5.0.9", diff --git a/packages/plugin/README.md b/packages/plugin/README.md index cc50b38..b35928c 100644 --- a/packages/plugin/README.md +++ b/packages/plugin/README.md @@ -830,6 +830,21 @@ land where no claim will look. Such a row is recovered only on the next floor re (`queue.claimFloor.resetInterval`), and until then the URL silently does not render. Use `POST /prerender_admin/revalidate` — or the button — which writes through the funnel. +### Where residency comes from + +Ownership is rendezvous-hashed over the cluster's node names, read from `server.nodes` on every +call. It is read per call, and never snapshotted, because `server.nodes` is not a stable list: +Harper initialises it empty and fills it asynchronously from `hdb_nodes`, replaces it while +rebuilding after a subscription restart, and briefly drops a node while applying an update to it. + +**An empty peer list is treated as "not known yet", never as "this node is alone."** With no +peers, rendezvous hashing makes the local node the owner of every URL — so every schedule row +would be stored locally rather than routed, where nothing claims it correctly and nothing removes +it. The last non-empty list therefore wins over an empty one, while any non-empty list is adopted +at once so a real membership change still takes effect. A node that has never seen a peer logs a +warning and owns everything, which is correct for a single-node deployment and a misconfiguration +on a cluster. + ### Schedule repair: the half-written target A `RenderTarget` and its `RenderSchedule` row live in **separate databases**, so creating a diff --git a/packages/plugin/package.json b/packages/plugin/package.json index 77ef186..a42bd30 100644 --- a/packages/plugin/package.json +++ b/packages/plugin/package.json @@ -1,6 +1,6 @@ { "name": "@harperfast/prerender", - "version": "0.50.0", + "version": "0.50.1", "type": "module", "description": "Configurable Harper plugin for prerendering pages for bots and crawlers", "license": "Apache-2.0", diff --git a/packages/plugin/src/util/peer.js b/packages/plugin/src/util/peer.js index 0b65eb1..08095fe 100644 --- a/packages/plugin/src/util/peer.js +++ b/packages/plugin/src/util/peer.js @@ -23,7 +23,7 @@ */ import { config } from '../config.js'; -import { nodes } from './residency.js'; +import { getNodes } from './residency.js'; // Peers are reached over TLS: the Harper HTTP port serves TLS in every real deployment, and // the node certificates chain to a publicly-trusted CA, so standard validation applies (no @@ -57,7 +57,7 @@ export const peerOrigin = (hostname) => { * not weaken the guard — membership in the known set is still required. */ export const isKnownNode = (hostname) => - typeof hostname === 'string' && nodes.some((node) => node.toLowerCase() === hostname.toLowerCase()); + typeof hostname === 'string' && getNodes().some((node) => node.toLowerCase() === hostname.toLowerCase()); /** * The subset of the caller's headers to forward. Only credentials, nothing else — the peer diff --git a/packages/plugin/src/util/residency.js b/packages/plugin/src/util/residency.js index ebfd8db..f0cf405 100644 --- a/packages/plugin/src/util/residency.js +++ b/packages/plugin/src/util/residency.js @@ -1,12 +1,89 @@ import { fnv1a32 } from './hash.js'; -export const nodes = [server.hostname, ...(server.nodes?.map(({ name }) => name) ?? [])].sort(); - /** * Rendezvous (HRW) hashing: deterministically picks the node responsible for a * given URL, so every node agrees on the owner without coordination. + * + * THE NODE LIST MUST BE READ LAZILY, AND AN EMPTY PEER LIST IS NEVER AN ANSWER. + * + * `server.nodes` is derived state, not configuration. harper-pro initialises it to `[]` at + * module scope and only fills it when `subscribeToNodeUpdates` runs a full `hdb_nodes` scan + * (`replication/knownNodes.ts`), so anything evaluated before that scan sees no peers on a + * healthy multi-node cluster — and that ordering repeats for every worker start, not only the + * first. Later rebuilds do empty and refill the array, but synchronously (the scan is a plain + * `for...of`, and the per-update path filters and pushes before its first await), so they are + * atomic to any observer and are NOT a window of their own. What they establish is that this + * list is rebuilt from a table rather than fixed at boot, which is why capturing it once is + * wrong in principle and not merely unlucky. + * + * With no peers, HRW makes this node the owner of every URL. `RenderSchedule.setResidencyById` + * then reports self for every key, so Harper stores every schedule row LOCALLY instead of + * routing it to its owner (`core/resources/Table.ts`, which omits the local record only when + * the computed residency excludes `server.hostname`). That is silent and permanent: nothing + * deletes a schedule row from a node that does not own it, the owner never gets the row, and + * the non-owner re-claims and re-renders it forever because its own reschedule routes away. + * Capturing the list at module load made one unlucky evaluation poison a worker for its whole + * lifetime, which has been observed in production as a multi-percent share of a corpus. + * + * So: the last non-empty list wins over an empty one, and any non-empty list is adopted + * immediately (a genuine membership change must still take effect). Before any peer has ever + * been seen, self-only is the only answer available — and the correct one for a single-node + * deployment, which is why this warns rather than throws. */ + +// Cache identity AND length: `knownNodes.ts` both reassigns `server.nodes` (filter-then-push) +// and pushes onto the existing array, so neither check alone sees every change. Steady state is +// two comparisons, which matters because the reconcile and orphan sweeps call this once per row +// over the whole corpus. It would miss a same-length rename mutated into the array in place; +// `knownNodes.ts` has no such path, and every mutation it does perform changes one or the other. +let cachedFrom; +let cachedLength = -1; +let resolvedNodes = []; +let selfOnly; +let warnedPeerless = false; + +function currentNodes() { + const peers = server.nodes; + const length = peers?.length ?? 0; + + if (length !== 0 && (peers !== cachedFrom || length !== cachedLength)) { + cachedFrom = peers; + cachedLength = length; + // A decode-miss descriptor can reach `server.nodes` without a name. Left in, it sorts + // into the ring as `undefined` and — when it wins the hash — makes this function return + // undefined, which Harper reads as a residency that excludes every node and stores the + // record nowhere at all. + const names = peers.map((node) => node?.name).filter(Boolean); + // Latch only a list that actually contains a peer: a populated `server.nodes` that + // yields no usable name is "not known yet" for the same reason an empty one is, and + // must not be allowed to overwrite a good list with a self-only one. + if (names.length) resolvedNodes = [...new Set([server.hostname, ...names])].sort(); + } + + if (resolvedNodes.length) return resolvedNodes; + + if (!warnedPeerless) { + warnedPeerless = true; + // `globalThis.logger`, not a bare `logger`: optional chaining does not guard an + // UNDECLARED identifier, and a residency throw makes Harper drop the record — the + // diagnostic must not be able to break the decision it reports on. + globalThis.logger?.warn?.( + `[prerender] residency has never seen a peer, so ${server.hostname} maps to itself for every URL. ` + + `Schedule rows written now are stored locally instead of on their owner, and nothing removes ` + + `them later. Expected only on a single-node deployment; on a cluster it means this process ` + + `started before hdb_nodes was populated.` + ); + } + + // Cached like the peered list: a single-node deployment takes this branch on every call. + return (selfOnly ??= [server.hostname]); +} + +/** The cluster's known node names, self included. Sorted, deduplicated, never empty. */ +export const getNodes = () => currentNodes(); + export function getResidencyByUrl(url) { + const nodes = currentNodes(); let bestIdx = 0; let bestScore = -1; diff --git a/packages/plugin/test/residency.test.js b/packages/plugin/test/residency.test.js new file mode 100644 index 0000000..927b791 --- /dev/null +++ b/packages/plugin/test/residency.test.js @@ -0,0 +1,205 @@ +import { test, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; + +/** + * Residency is the shard map for the whole RenderSchedule keyspace, and a wrong answer is + * silent: the row is stored on a node that does not own it, the owner never gets it, and + * nothing deletes it. The properties pinned here are therefore about WHERE the node list comes + * from — an empty `server.nodes` must never read as "this node owns everything" — plus the + * mapping itself, so no refactor can re-shard a live cluster. + */ + +const A = 'node-a.example.com'; +const B = 'node-b.example.com'; +const C = 'node-c.example.com'; +const SELF = 'node-d.example.com'; +const ALL = [A, B, C, SELF].sort(); + +let warnings; +let freshCounter = 0; + +const setServer = (hostname, peerNames) => { + globalThis.server = { hostname, nodes: peerNames === null ? undefined : peerNames.map((name) => ({ name })) }; +}; + +// Each test gets its own module instance so "never seen a peer" and the warn-once latch are +// testable; every OTHER test proves the module needs no reload to pick a change up. +const freshResidency = () => import(`../src/util/residency.js?fresh=${freshCounter++}`); + +beforeEach(() => { + warnings = []; + globalThis.logger = { warn: (m) => warnings.push(String(m)), error: () => {}, trace: () => {}, debug: () => {} }; +}); + +afterEach(() => { + delete globalThis.server; + delete globalThis.logger; +}); + +test('a peer list that appears AFTER module load is picked up without a reload', async () => { + setServer(SELF, []); + const { getResidencyByUrl, getNodes } = await freshResidency(); + + // The load-time snapshot bug: with no peers, self owns everything. + assert.equal(getResidencyByUrl('https://example.com/a'), SELF); + assert.deepEqual(getNodes(), [SELF]); + + globalThis.server.nodes = [A, B, C].map((name) => ({ name })); + + assert.deepEqual(getNodes(), ALL); +}); + +test('every node computes the SAME owner from the same membership', async () => { + const url = 'https://example.com/catalog?page=2'; + const verdicts = []; + for (const self of ALL) { + setServer( + self, + ALL.filter((h) => h !== self) + ); + const { getResidencyByUrl } = await freshResidency(); + verdicts.push(getResidencyByUrl(url)); + } + assert.equal(new Set(verdicts).size, 1, 'nodes disagreed on the owner'); +}); + +test('the keyspace mapping is unchanged', async () => { + // A CHANGE-DETECTOR ON PURPOSE. These pairs are computed by the current FNV-1a + HRW pair, + // and one key is pinned per node so the assertion also proves the ring still spreads rather + // than collapsing onto self. Any edit that moves one of them re-shards every RenderSchedule + // row on every running cluster — the hazard util/hash.js already warns about for the mixing + // constant. If one of these fails, that is the finding; do not update the expectation. + setServer(SELF, [A, B, C]); + const { getResidencyByUrl } = await freshResidency(); + assert.equal(getResidencyByUrl('https://example.com/catalog?page=0'), A); + assert.equal(getResidencyByUrl('https://example.com/catalog?page=2'), B); + assert.equal(getResidencyByUrl('https://example.com/catalog?page=8'), C); + assert.equal(getResidencyByUrl('https://example.com/catalog?page=3'), SELF); +}); + +test('self appearing in server.nodes does not re-shard the keyspace', async () => { + // server.nodes is meant to hold PEERS only, but self has leaked into it before + // (harper-pro#489). Deduplicating must not move a single key: HRW takes the max score over + // the NAMES present, so a repeated name cannot change which name wins. + setServer(SELF, [A, B, C]); + const clean = await freshResidency(); + setServer(SELF, [A, B, C, SELF]); + const withSelf = await freshResidency(); + + assert.deepEqual(withSelf.getNodes(), clean.getNodes()); + for (let i = 0; i < 500; i++) { + const url = `https://example.com/p/${i}`; + assert.equal(withSelf.getResidencyByUrl(url), clean.getResidencyByUrl(url), url); + } +}); + +test('a transient empty peer list does not move ownership', async () => { + setServer(SELF, [A, B, C]); + const { getResidencyByUrl, getNodes } = await freshResidency(); + const url = 'https://example.com/catalog?page=2'; + assert.equal(getResidencyByUrl(url), B); + + // knownNodes.ts assigns a fresh empty array while rebuilding from hdb_nodes. + globalThis.server.nodes = []; + assert.equal(getResidencyByUrl(url), B, 'ownership moved to self during a rebuild window'); + assert.deepEqual(getNodes(), ALL); + + globalThis.server.nodes = undefined; + assert.equal(getResidencyByUrl(url), B, 'ownership moved to self when server.nodes went away'); +}); + +test('a genuine membership change IS adopted', async () => { + setServer(SELF, [A, B, C]); + const { getNodes } = await freshResidency(); + getNodes(); + + // One node retired: still a non-empty list, so it must take effect. + globalThis.server.nodes = [B, C].map((name) => ({ name })); + assert.deepEqual(getNodes(), [B, C, SELF].sort()); + assert.ok(!getNodes().includes(A)); +}); + +test('a push onto the existing array is seen (same identity, new length)', async () => { + setServer(SELF, [A]); + const { getNodes } = await freshResidency(); + assert.deepEqual(getNodes(), [SELF, A].sort()); + + // knownNodes.ts pushes onto server.nodes rather than reassigning it. + globalThis.server.nodes.push({ name: C }); + assert.deepEqual(getNodes(), [A, C, SELF].sort()); +}); + +test('a nameless node descriptor is never returned as an owner', async () => { + // knownNodes.ts can push a reconstructed descriptor with no name. Left in the ring it wins + // the hash for some keys, and returning undefined makes Harper store the row nowhere. + setServer(SELF, [A, B, C]); + globalThis.server.nodes.push({ name: undefined }, {}, null); + const { getResidencyByUrl, getNodes } = await freshResidency(); + + assert.deepEqual(getNodes(), ALL); + for (let i = 0; i < 300; i++) { + const owner = getResidencyByUrl(`https://example.com/p/${i}`); + assert.ok(getNodes().includes(owner), `key ${i} resolved to ${owner}`); + } +}); + +test('a populated peer list with no usable names is treated as peerless', async () => { + // A decode miss can put descriptors in server.nodes that carry no name. A list of nothing + // but those is "not known yet", not "this node is alone" — it must warn, and it must not + // overwrite a good list with a self-only one. + setServer(SELF, []); + globalThis.server.nodes = [{ name: undefined }, {}, null]; + const { getResidencyByUrl, getNodes } = await freshResidency(); + + assert.deepEqual(getNodes(), [SELF]); + assert.equal(warnings.filter((w) => w.includes('never seen a peer')).length, 1); + + globalThis.server.nodes = [A, B, C].map((name) => ({ name })); + const url = 'https://example.com/catalog?page=2'; + assert.equal(getResidencyByUrl(url), B); + + // ...and a later all-nameless list must not clobber the good one. + globalThis.server.nodes = [{ name: undefined }, {}]; + assert.equal(getResidencyByUrl(url), B); + assert.deepEqual(getNodes(), ALL); +}); + +test('a peerless residency decision warns exactly once', async () => { + setServer(SELF, []); + const { getResidencyByUrl } = await freshResidency(); + for (let i = 0; i < 5; i++) getResidencyByUrl(`https://example.com/${i}`); + + const peerless = warnings.filter((w) => w.includes('never seen a peer')); + assert.equal(peerless.length, 1); + assert.match(peerless[0], /stored locally instead of on their owner/); +}); + +test('a single-node deployment resolves to itself without repeated warnings', async () => { + setServer(SELF, null); + const { getResidencyByUrl, getNodes } = await freshResidency(); + assert.equal(getResidencyByUrl('https://example.com/a'), SELF); + assert.deepEqual(getNodes(), [SELF]); + assert.equal(warnings.filter((w) => w.includes('never seen a peer')).length, 1); + // This branch is taken on EVERY call here, so it must not allocate either. + assert.equal(getNodes(), getNodes()); +}); + +test('an unchanged peer list is not rebuilt per call', async () => { + // The reconcile and orphan sweeps call this once per row across the whole corpus, so the + // steady-state path must not allocate. + setServer(SELF, [A, B, C]); + const { getNodes } = await freshResidency(); + assert.equal(getNodes(), getNodes(), 'the node list was rebuilt for an unchanged peer list'); +}); + +test('isKnownNode accepts a peer that joined after module load', async () => { + // The same stale snapshot made peer.js reject legitimate peers, breaking the explainer's + // cross-node fetch. It reads through the same accessor, so it heals with it. + globalThis.server = { hostname: SELF, nodes: [], config: { http: { port: 9925, securePort: 9926 } } }; + const peer = await import(`../src/util/peer.js?fresh=${freshCounter++}`); + assert.equal(peer.isKnownNode(C), false); + + globalThis.server.nodes = [{ name: C }]; + assert.equal(peer.isKnownNode(C), true); + assert.equal(peer.isKnownNode('evil.example.com'), false); +});