From e62052250c3daaf5ea818b7bfb22ff9c1475bd0f Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 22 Aug 2026 06:12:39 -0600 Subject: [PATCH 1/8] Deflake unit-test cluster: seed HNSW routing graph, signal-based txn/subscription waits, quarantine 2 unreproduced flakes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes three flaky unit tests with demonstrated root causes and quarantines two that could not be root-caused this round (each linked to a filed issue): - HNSW greedy-routing test: graph levels came from unseeded Math.random, and greedy-vs-full-ef equality is only statistically true across random graphs. Adds a 'random' test seam to HierarchicalNavigableSmallWorld (also clamps the entry-point level to MAX_LEVEL, matching the other assignment site) and seeds the test's graph. 0/120 contended runs post-fix (was ~2.5-5%). - Txn expiration test: fixed 50ms window raced a 40ms sleep plus real DB work plus two 20ms expiry ticks. Now waits on the actual signals and proves expiry landed before the slow get() settled. 0/160 pinned runs (was 2/80). - Subscription replay (updates to passed keys): still used collect()'s quiet-period timer, the race its sibling tests were already migrated off; now uses the same waitFor-final-values pattern. - risk-query integration suite: skipped on win32 (#2273 — deploy_component hangs after npm pack on Windows CI) and readiness-poll fetches now carry AbortSignal.timeout so one hung fetch cannot burn undici's 300s default. - MQTT non-clean-session test: skipped (#2274 — silent 20s hang on CI, 0/30 contended local repro attempts). Refs #1655 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0142195pzooHeNejheNfsPDZ --- DESIGN.md | 6 +++++ .../components/risk-query.test.ts | 13 +++++++--- .../HierarchicalNavigableSmallWorld.ts | 8 ++++-- unitTests/apiTests/mqtt-test.mjs | 6 ++++- .../resources/subscriptionReplay.test.js | 19 +++++++++++++- unitTests/resources/txn-tracking.test.js | 25 ++++++++++++++----- unitTests/resources/vectorIndex.test.js | 11 ++++++++ 7 files changed, 75 insertions(+), 13 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index be585ba295..f85e950b21 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1094,6 +1094,12 @@ graphs being identical, though equal metrics do not prove it. It is the expected the upper layers are sparse enough that a greedy walk reaches the same entry point, which is why standard HNSW descends this way. +Greedy-equals-full is statistical, not per-graph: rare level layouts route to a different layer-0 +entry point and displace the tail of the top-k (~2-3% of random 600-node graphs in the unit test's +corpus). Tests that assert exact result-set equality across search strategies must therefore pin the +graph: level assignment draws from the instance's `random` property (a test seam defaulting to +`Math.random`), which the routing test replaces with a seeded PRNG. + ## `efConstruction` and the search-`ef` ceiling both auto-scale with the graph The connection-building pass selects each node's stored edges from a candidate list of diff --git a/integrationTests/components/risk-query.test.ts b/integrationTests/components/risk-query.test.ts index 45676b19d5..84a0c2778c 100644 --- a/integrationTests/components/risk-query.test.ts +++ b/integrationTests/components/risk-query.test.ts @@ -13,7 +13,12 @@ const __dirname = dirname(fileURLToPath(import.meta.url)); import { startHarper, teardownHarper, sendOperation, type ContextWithHarper } from '@harperfast/integration-testing'; -suite('Component: risk-query', (ctx: ContextWithHarper) => { +// Quarantined on Windows: deploy_component (restart:true) hangs server-side after npm pack, which +// stalls before() on undici's 300s headers timeout and cancels every child test. See +// https://github.com/HarperFast/harper/issues/2273 — remove the skip when the deploy hang is fixed. +const skipSuite = process.platform === 'win32'; + +suite('Component: risk-query', { skip: skipSuite }, (ctx: ContextWithHarper) => { before(async () => { await startHarper(ctx); @@ -27,11 +32,13 @@ suite('Component: risk-query', (ctx: ContextWithHarper) => { strictEqual(body.message, 'Successfully deployed: risk-query, restarting Harper'); ok(typeof body.deployment_id === 'string', `expected deployment_id in deploy response, got ${body.deployment_id}`); - // Poll until the component is ready + // Poll until the component is ready. Each probe carries its own abort timeout: without it, a + // connection that opens but never sends headers holds fetch for undici's 300s default and + // blows past the deadline check (issue #2273's client-side half). const deadline = Date.now() + 30_000; while (true) { try { - const check = await fetch(`${ctx.harper.httpURL}/RisqTable/`); + const check = await fetch(`${ctx.harper.httpURL}/RisqTable/`, { signal: AbortSignal.timeout(5_000) }); if (check.status === 200) break; } catch { // server not yet accepting connections diff --git a/resources/indexes/HierarchicalNavigableSmallWorld.ts b/resources/indexes/HierarchicalNavigableSmallWorld.ts index 1a49067257..6ee044285b 100644 --- a/resources/indexes/HierarchicalNavigableSmallWorld.ts +++ b/resources/indexes/HierarchicalNavigableSmallWorld.ts @@ -209,6 +209,10 @@ export class HierarchicalNavigableSmallWorld { // a value of 1 is extremely aggressive. optimizeRouting = 0.5; nodesVisitedCount = 0; + // Test seam: source of randomness for level assignment. Tests that assert exact result-set + // equality across search strategies replace this with a seeded PRNG so the graph shape is + // reproducible; greedy routing legitimately diverges on rare unlucky graphs otherwise. + random: () => number = Math.random; // Visit-budget multiplier for predicate-aware traversal (#1241). Under-filled filtered searches // stop after the resolved budget ef * filterExpansion visits; automatic search ef contributes at // most AUTO_EF_MAX, while explicit schema/query ef remains authoritative. A filter that fills its @@ -337,7 +341,7 @@ export class HierarchicalNavigableSmallWorld { const storedScale = q ? q.scale : undefined; let entryPoint = entryPointId && this.safeGetSync(entryPointId, options); if (entryPoint == null) { - const level = Math.floor(-Math.log(Math.random()) * this.mL); + const level = Math.min(Math.floor(-Math.log(this.random()) * this.mL), MAX_LEVEL); const node = { vector: storedVector, scale: storedScale, @@ -358,7 +362,7 @@ export class HierarchicalNavigableSmallWorld { } // Generate random level for this new element - const level = oldNode.level ?? Math.min(Math.floor(-Math.log(Math.random()) * this.mL), MAX_LEVEL); + const level = oldNode.level ?? Math.min(Math.floor(-Math.log(this.random()) * this.mL), MAX_LEVEL); let currentLevel = entryPoint.level; if (level > currentLevel) { // if we are at a higher level, make this the new entry point diff --git a/unitTests/apiTests/mqtt-test.mjs b/unitTests/apiTests/mqtt-test.mjs index e81b31dc33..b097d21ad7 100644 --- a/unitTests/apiTests/mqtt-test.mjs +++ b/unitTests/apiTests/mqtt-test.mjs @@ -890,7 +890,11 @@ describe('test MQTT connections and commands', function () { const granted = await subscribeAllowingSubackError(clientV5, '+/SimpleRecord/test'); assert.equal(granted[0].qos, 0x8f); // assert that the subscription was rejected }); - it('subscribe with QoS=1 and reconnect with non-clean session', async function () { + // Quarantined: hung silently to its 20s timeout on CI main (lmdb pass, Node 26) with no + // server-side log output; not reproduced in 30 contended local runs. Evidence, hypotheses, + // and the reinstatement path (bounded per-step waits) are in + // https://github.com/HarperFast/harper/issues/2274 — unskip once the hang is localized. + it.skip('subscribe with QoS=1 and reconnect with non-clean session', async function () { this.timeout(20000); // needs more than the suite-level 10 s on loaded runners // this first connection is a tear down to remove any previous durable session with this id let client = await connectAsync(mqttUrl, { diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index 1c9fec9a1a..8c57d6643c 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -418,8 +418,25 @@ describe('Subscription replay', () => { await CurrentStateTable.put(7000 + i, { name: 'pp_updated' + i }); } })(); - const events = await collect(subscription, 250); + // Attach a listener and wait for every key's final value rather than using collect()'s + // quiet-period timer — on a loaded runner the subscription can go quiet longer than the + // window while queued updates are still in flight (the same race the two tests above + // already moved off of). The timeout path falls through so the per-key asserts below + // report exactly which key was stale. + const events = []; + subscription.on('data', (e) => events.push(e)); await concurrentWrites; + await waitFor( + () => { + const finals = new Map(); + for (const e of events) finals.set(e.id, e); + for (let i = 0; i < 50; i++) { + if (finals.get(7000 + i)?.value?.name !== 'pp_updated' + i) return false; + } + return true; + }, + { timeout: 5000 } + ).catch(() => {}); subscription.return?.(); // every key 7000..7049 must end up at the updated value as its final delivery diff --git a/unitTests/resources/txn-tracking.test.js b/unitTests/resources/txn-tracking.test.js index 394da8af31..c03e2081f1 100644 --- a/unitTests/resources/txn-tracking.test.js +++ b/unitTests/resources/txn-tracking.test.js @@ -73,14 +73,27 @@ describe('Txn Expiration', () => { assert.equal(lastTxn.startedFrom.method, 'get'); assert.equal(lastTxn.timeout, 20); } - await Promise.race([delay(50), result]); - assert(performedDBInteractions); + // Wait on the actual signals instead of racing a fixed 50ms window: under CI contention + // the 40ms sleep inside get() overruns the window before the follow-up read/write lands, + // and the expiry sweep needs two ~20ms monitor ticks that can also land late. The 500ms + // tail inside get() keeps `result` pending, so observing expiry before `result` settles + // still proves the txn was expired mid-flight rather than removed by normal completion. + let resultSettled = false; + result.then( + () => (resultSettled = true), + () => (resultSettled = true) + ); + await waitFor(() => performedDBInteractions, { message: 'read/write after expiry never completed' }); // Check the specific txn we started was expired and removed. Counting against // existingTxns is unreliable: other tests' transactions can expire concurrently and - // shift the count underneath us during the 50ms window. - assert.ok( - !trackedTxns.has(lastTxn), - 'expected the slow transaction to have been expired and removed from trackedTxns' + // shift the count underneath us. + const outcome = await waitFor(() => (!trackedTxns.has(lastTxn) ? 'expired' : resultSettled && 'settled'), { + message: 'the slow transaction was neither expired nor completed', + }); + assert.equal( + outcome, + 'expired', + 'expected the slow transaction to have been expired and removed from trackedTxns before get() completed' ); }); after(function () { diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index c87ca1843b..4340b28add 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -948,6 +948,17 @@ describe('HNSW greedy routing above layer 0 (ROUTING_EF)', () => { { name: 'vector', indexed: { type: 'HNSW', distance: 'cosine' }, type: 'Array' }, ], }); + // Seed level assignment (mulberry32) so the graph is identical every run. The greedy-vs-full + // equality below is only statistically true over random graphs — rare level layouts + // legitimately route to a different entry point and change the top-10 tail (flaked ~2-3% of + // runs on CI). Pinning the graph keeps the assertion exact without weakening it. + let seedState = 0x9e3779b9; + T.indices.vector.customIndex.random = () => { + seedState = (seedState + 0x6d2b79f5) | 0; + let t = Math.imul(seedState ^ (seedState >>> 15), 1 | seedState); + t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t; + return ((t ^ (t >>> 14)) >>> 0) / 4294967296; + }; for (let i = 0; i < N; i++) { const a = (i / N) * Math.PI * 2; const b = ((i * 7) % N) / N; From 2f7ab2531045a2e49c8e0ecaa9e11cd489f36ca3 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 22 Aug 2026 06:25:57 -0600 Subject: [PATCH 2/8] Address pre-push review: drain slow txn get(), convert remaining quiet-period waits, narrow MQTT skip to lmdb, test the entry-point clamp - txn-tracking: await the slow get() (asserting the abort surfaces on rocksdb) so its 500ms tail cannot bleed into the next describe's expiration settings and re-pathed test DB. - subscriptionReplay: convert the two remaining quiet-period/fixed-delay waits ('rapid updates' and 'subscribe while writes are in flight') to the same waitFor pattern; trim history-narrating comments. - mqtt-test: quarantine the non-clean-session test on lmdb only (where the hang was observed) so rocksdb keeps the durable-session coverage. - vectorIndex: regression test pinning the empty-index entry-point level clamp (random() === 0 previously meant an infinite loop). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0142195pzooHeNejheNfsPDZ --- unitTests/apiTests/mqtt-test.mjs | 195 +++++++++--------- .../resources/subscriptionReplay.test.js | 35 +++- unitTests/resources/txn-tracking.test.js | 15 +- unitTests/resources/vectorIndex.test.js | 37 ++++ 4 files changed, 175 insertions(+), 107 deletions(-) diff --git a/unitTests/apiTests/mqtt-test.mjs b/unitTests/apiTests/mqtt-test.mjs index b097d21ad7..317526ebff 100644 --- a/unitTests/apiTests/mqtt-test.mjs +++ b/unitTests/apiTests/mqtt-test.mjs @@ -890,112 +890,117 @@ describe('test MQTT connections and commands', function () { const granted = await subscribeAllowingSubackError(clientV5, '+/SimpleRecord/test'); assert.equal(granted[0].qos, 0x8f); // assert that the subscription was rejected }); - // Quarantined: hung silently to its 20s timeout on CI main (lmdb pass, Node 26) with no - // server-side log output; not reproduced in 30 contended local runs. Evidence, hypotheses, - // and the reinstatement path (bounded per-step waits) are in - // https://github.com/HarperFast/harper/issues/2274 — unskip once the hang is localized. - it.skip('subscribe with QoS=1 and reconnect with non-clean session', async function () { - this.timeout(20000); // needs more than the suite-level 10 s on loaded runners - // this first connection is a tear down to remove any previous durable session with this id - let client = await connectAsync(mqttUrl, { - clean: true, - clientId: 'test-client1', - protocolVersion: 4, - }); - await endDurableSession(client, 'test-client1'); - client = await connectAsync(mqttUrl, { - clean: false, - clientId: 'test-client1', - protocolVersion: 4, - }); - await client.subscribeAsync(['SimpleRecord/41', 'SimpleRecord/42'], { qos: 1 }); - await endDurableSession(client, 'test-client1'); - client = await connectAsync(mqttUrl, { - clean: false, - clientId: 'test-client1', - protocolVersion: 4, - }); - await new Promise((resolve) => { - // Wait for the broker to finish processing (and durably persisting) our ack of this - // message, not just for the client to have sent it — see `session.acknowledge()`. - const acknowledged = waitForMqttSessionEvent('acknowledged', 'test-client1'); - client.on('message', (topic, payload) => { - JSON.parse(payload); - resolve(acknowledged); + // Quarantined on lmdb only: hung silently to its 20s timeout on CI main (lmdb pass, Node 26) + // with no server-side log output; not reproduced in 30 contended local runs. The rocksdb pass + // keeps this durable-session coverage. Evidence, hypotheses, and the reinstatement path + // (bounded per-step waits) are in https://github.com/HarperFast/harper/issues/2274. + (process.env.HARPER_STORAGE_ENGINE === 'lmdb' ? it.skip : it)( + 'subscribe with QoS=1 and reconnect with non-clean session', + async function () { + this.timeout(20000); // needs more than the suite-level 10 s on loaded runners + // this first connection is a tear down to remove any previous durable session with this id + let client = await connectAsync(mqttUrl, { + clean: true, + clientId: 'test-client1', + protocolVersion: 4, }); + await endDurableSession(client, 'test-client1'); + client = await connectAsync(mqttUrl, { + clean: false, + clientId: 'test-client1', + protocolVersion: 4, + }); + await client.subscribeAsync(['SimpleRecord/41', 'SimpleRecord/42'], { qos: 1 }); + await endDurableSession(client, 'test-client1'); + client = await connectAsync(mqttUrl, { + clean: false, + clientId: 'test-client1', + protocolVersion: 4, + }); + await new Promise((resolve) => { + // Wait for the broker to finish processing (and durably persisting) our ack of this + // message, not just for the client to have sent it — see `session.acknowledge()`. + const acknowledged = waitForMqttSessionEvent('acknowledged', 'test-client1'); + client.on('message', (topic, payload) => { + JSON.parse(payload); + resolve(acknowledged); + }); - client.publish( + client.publish( + 'SimpleRecord/41', + JSON.stringify({ + name: 'This is a test of durable session with subscriptions restarting', + }), + { + qos: 1, + } + ); + }); + await endDurableSession(client, 'test-client1'); + await clientV5.publishAsync( 'SimpleRecord/41', JSON.stringify({ - name: 'This is a test of durable session with subscriptions restarting', + name: 'This is a test of publishing to a disconnected durable session', }), { qos: 1, } ); - }); - await endDurableSession(client, 'test-client1'); - await clientV5.publishAsync( - 'SimpleRecord/41', - JSON.stringify({ - name: 'This is a test of publishing to a disconnected durable session', - }), - { - qos: 1, - } - ); - await clientV5.publishAsync( - 'SimpleRecord/42', - JSON.stringify({ - name: 'This is a test of publishing to a disconnected durable session 2', - }), - { - qos: 1, - } - ); - await clientV5.publishAsync( - 'SimpleRecord/42', - JSON.stringify({ - name: 'This is a test of publishing to a disconnected durable session 3', - }), - { - qos: 1, - } - ); - let messages = []; - client = await connectWithMessageListener( - mqttUrl, - { - clean: false, - clientId: 'test-client1', - protocolVersion: 5, - properties: { - sessionExpiryInterval: 3600, + await clientV5.publishAsync( + 'SimpleRecord/42', + JSON.stringify({ + name: 'This is a test of publishing to a disconnected durable session 2', + }), + { + qos: 1, + } + ); + await clientV5.publishAsync( + 'SimpleRecord/42', + JSON.stringify({ + name: 'This is a test of publishing to a disconnected durable session 3', + }), + { + qos: 1, + } + ); + let messages = []; + client = await connectWithMessageListener( + mqttUrl, + { + clean: false, + clientId: 'test-client1', + protocolVersion: 5, + properties: { + sessionExpiryInterval: 3600, + }, }, - }, - (topic, message) => { - messages.push(message.toString()); - } - ); - await new Promise((resolve, reject) => { - const interval = setInterval(() => { - if (messages.length === 3) { - clearInterval(interval); - resolve(); + (topic, message) => { + messages.push(message.toString()); } - }, 1); - setTimeout(() => { - clearInterval(interval); - reject( - new Error(`Expected 3 queued messages to be delivered to reconnected durable session, got ${messages.length}`) - ); - }, 15000); - }); - await delay(50); - await client.endAsync(); - if (messages.length !== 3) console.error('Incorrect messages', { messages }); - assert(messages.length === 3); - }); + ); + await new Promise((resolve, reject) => { + const interval = setInterval(() => { + if (messages.length === 3) { + clearInterval(interval); + resolve(); + } + }, 1); + setTimeout(() => { + clearInterval(interval); + reject( + new Error( + `Expected 3 queued messages to be delivered to reconnected durable session, got ${messages.length}` + ) + ); + }, 15000); + }); + await delay(50); + await client.endAsync(); + if (messages.length !== 3) console.error('Incorrect messages', { messages }); + assert(messages.length === 3); + } + ); it('subscribe with QoS=2', async function () { // this first connection is a tear down to remove any previous durable session with this id let client = await connectAsync(mqttUrl, { diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index 8c57d6643c..f1b3a9c61c 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -381,7 +381,19 @@ describe('Subscription replay', () => { const events = []; subscription.on('data', (e) => events.push(e)); await concurrentWrites; - await delay(200); + // wait for every hammered key's final (round-2) value instead of a fixed 200ms — the + // queued deliveries can trail the commits on a loaded runner + await waitFor( + () => { + const lastSeen = new Map(); + for (const e of events) lastSeen.set(e.id, e); + for (let i = 100; i < 200; i++) { + if (lastSeen.get(6000 + i)?.value?.name !== `rc_v2_${i}`) return false; + } + return true; + }, + { timeout: 5000 } + ).catch(() => {}); subscription.return?.(); // every key in 6000..6199 must appear at least once @@ -418,11 +430,9 @@ describe('Subscription replay', () => { await CurrentStateTable.put(7000 + i, { name: 'pp_updated' + i }); } })(); - // Attach a listener and wait for every key's final value rather than using collect()'s - // quiet-period timer — on a loaded runner the subscription can go quiet longer than the - // window while queued updates are still in flight (the same race the two tests above - // already moved off of). The timeout path falls through so the per-key asserts below - // report exactly which key was stale. + // Queued deliveries can trail the commits by more than any quiet window on a loaded + // runner, so wait for every key's final value. The timeout falls through so the per-key + // asserts below report exactly which key was stale. const events = []; subscription.on('data', (e) => events.push(e)); await concurrentWrites; @@ -567,8 +577,19 @@ describe('Subscription replay', () => { inFlight.push(CurrentStateTable.put(14000 + i, { name: 'inflight' + i })); } const subscription = await CurrentStateTable.subscribe({ isCollection: true }); - const events = await collect(subscription, 250); + // collect()'s quiet window can expire while the 200 puts are still committing; wait for + // the commits, then for every key's delivery. + const events = []; + subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); + await waitFor( + () => { + const seen = new Set(events.map((e) => e.id)); + for (let i = 0; i < 200; i++) if (!seen.has(14000 + i)) return false; + return true; + }, + { timeout: 5000 } + ).catch(() => {}); subscription.return?.(); // every in-flight key must be delivered at least once (duplicates allowed under diff --git a/unitTests/resources/txn-tracking.test.js b/unitTests/resources/txn-tracking.test.js index c03e2081f1..c81063d59f 100644 --- a/unitTests/resources/txn-tracking.test.js +++ b/unitTests/resources/txn-tracking.test.js @@ -73,11 +73,8 @@ describe('Txn Expiration', () => { assert.equal(lastTxn.startedFrom.method, 'get'); assert.equal(lastTxn.timeout, 20); } - // Wait on the actual signals instead of racing a fixed 50ms window: under CI contention - // the 40ms sleep inside get() overruns the window before the follow-up read/write lands, - // and the expiry sweep needs two ~20ms monitor ticks that can also land late. The 500ms - // tail inside get() keeps `result` pending, so observing expiry before `result` settles - // still proves the txn was expired mid-flight rather than removed by normal completion. + // The 500ms tail inside get() keeps `result` pending, so observing expiry before `result` + // settles proves the txn was expired mid-flight rather than removed by normal completion. let resultSettled = false; result.then( () => (resultSettled = true), @@ -95,6 +92,14 @@ describe('Txn Expiration', () => { 'expired', 'expected the slow transaction to have been expired and removed from trackedTxns before get() completed' ); + // Drain the slow get() so its 500ms tail and final read cannot run into the next + // describe's expiration settings and freshly re-pathed test DB. On rocksdb the aborted + // outer transaction must also surface to the caller. + if (SlowResource.primaryStore instanceof RocksDatabase) { + await assert.rejects(result, /aborted after exceeding the maximum open-transaction time/); + } else { + await result.catch(() => {}); + } }); after(function () { setTxnExpiration(30000); diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 4340b28add..ba13ec03c0 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1025,6 +1025,43 @@ describe('HNSW greedy routing above layer 0 (ROUTING_EF)', () => { }); }); +describe('HNSW entry-point level clamp', () => { + if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return; + let T; + before(() => { + setupTestDBPath(); + setMainIsWorker(true); + T = table({ + table: 'HNSWClampTest', + database: 'test', + attributes: [ + { name: 'id', isPrimaryKey: true }, + { name: 'vector', indexed: { type: 'HNSW', distance: 'cosine' }, type: 'Array' }, + ], + }); + }); + after(() => { + T.dropTable(); + }); + + it('caps the first node of an empty index at MAX_LEVEL even when random() returns 0', async () => { + const customIndex = T.indices.vector.customIndex; + // -Math.log(0) is Infinity; unclamped, the entry-point path would loop forever + // initializing per-level connection arrays. + customIndex.random = () => 0; + try { + await T.put(1, { vector: [1, 0, 0] }); + } finally { + customIndex.random = Math.random; + } + let entryLevel; + for (const { value } of customIndex.indexStore.getRange({ start: 0, end: Infinity })) { + if (value?.level !== undefined) entryLevel = value.level; + } + assert.strictEqual(entryLevel, 10, `expected the entry point level to be clamped to MAX_LEVEL, got ${entryLevel}`); + }); +}); + describe('HNSW limit above the resolved search ef', () => { if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return; let T; From b52d72f4ea48c5af07c8ab31476be928d5585be1 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 22 Aug 2026 06:33:18 -0600 Subject: [PATCH 3/8] Address review round 2: finite clamp-test draw, retire remaining collect() quiet windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - vectorIndex: drive the clamp test with Number.MIN_VALUE (finite ~268 level) instead of 0 — a clamp regression now fails in milliseconds rather than wedging the runner in a synchronous infinite loop. - subscriptionReplay: convert the last three collect() quiet-window waits; the two duplicate-detection tests could previously pass vacuously when the window expired before in-flight deliveries. The non-collection test waits for the final version only, since rapid same-record versions legitimately coalesce. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0142195pzooHeNejheNfsPDZ --- .../resources/subscriptionReplay.test.js | 22 +++++++++++++++---- unitTests/resources/vectorIndex.test.js | 10 +++++---- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index f1b3a9c61c..291bb88982 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -577,8 +577,6 @@ describe('Subscription replay', () => { inFlight.push(CurrentStateTable.put(14000 + i, { name: 'inflight' + i })); } const subscription = await CurrentStateTable.subscribe({ isCollection: true }); - // collect()'s quiet window can expire while the 200 puts are still committing; wait for - // the commits, then for every key's delivery. const events = []; subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); @@ -646,8 +644,19 @@ describe('Subscription replay', () => { inFlight.push(CountTable.put(17000 + i, { name: 'count_race_inflight' + i })); } const subscription = await CountTable.subscribe({ previousCount: 10, isCollection: true }); - const events = await collect(subscription, 250); + // The duplicate check below is only sound once every in-flight write's delivery has had + // the chance to arrive — collect()'s quiet window returning early made it vacuous. + const events = []; + subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); + await waitFor( + () => { + const seen = new Set(events.map((e) => e.id)); + for (let i = 0; i < 30; i++) if (!seen.has(17000 + i)) return false; + return true; + }, + { timeout: 5000 } + ).catch(() => {}); subscription.return?.(); // the regression we want to catch: a record landing in BOTH history (from cursor's @@ -669,8 +678,13 @@ describe('Subscription replay', () => { inFlight.push(RecordTable.put(15000, { name: 'inflight_v' + i })); } const subscription = await RecordTable.subscribe({ id: 15000, startTime: startTime - 1 }); - const events = await collect(subscription, 250); + // Same soundness requirement as the count test above, but rapid same-record versions can + // legitimately coalesce, so the only guaranteed delivery is the final version — wait for + // it (any cursor/listener duplicate of an earlier version travels with its original). + const events = []; + subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); + await waitFor(() => events.some((e) => e.value?.name === 'inflight_v49'), { timeout: 5000 }).catch(() => {}); subscription.return?.(); const pairs = events.map((e) => `${e.id}:${e.version}`); diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index ba13ec03c0..4109be97c0 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1044,11 +1044,13 @@ describe('HNSW entry-point level clamp', () => { T.dropTable(); }); - it('caps the first node of an empty index at MAX_LEVEL even when random() returns 0', async () => { + it('caps the first node of an empty index at MAX_LEVEL', async () => { const customIndex = T.indices.vector.customIndex; - // -Math.log(0) is Infinity; unclamped, the entry-point path would loop forever - // initializing per-level connection arrays. - customIndex.random = () => 0; + // Number.MIN_VALUE draws an unclamped level of ~268 (-ln(5e-324) * mL). A finite draw keeps + // a clamp regression a fast assertion failure — random() === 0 (Infinity) would instead wedge + // the runner in the synchronous per-level init loop, which is worse than the flake this + // branch removes. + customIndex.random = () => Number.MIN_VALUE; try { await T.put(1, { vector: [1, 0, 0] }); } finally { From f96c4ef8b6713af5b138ebdc42b1ff85f24ac668 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 22 Aug 2026 06:36:42 -0600 Subject: [PATCH 4/8] Add additive settle before duplicate checks in converted subscription tests A duplicate can trail the last expected delivery; the 100ms settle after the positive wait can only surface more events, never lose them. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0142195pzooHeNejheNfsPDZ --- unitTests/resources/subscriptionReplay.test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index 291bb88982..0ed8a3d892 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -657,6 +657,9 @@ describe('Subscription replay', () => { }, { timeout: 5000 } ).catch(() => {}); + // additive settle: a duplicate trailing the last expected delivery can still land; this + // can only catch more, never lose events + await delay(100); subscription.return?.(); // the regression we want to catch: a record landing in BOTH history (from cursor's @@ -685,6 +688,8 @@ describe('Subscription replay', () => { subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); await waitFor(() => events.some((e) => e.value?.name === 'inflight_v49'), { timeout: 5000 }).catch(() => {}); + // additive settle so a duplicate trailing the final version can still surface + await delay(100); subscription.return?.(); const pairs = events.map((e) => `${e.id}:${e.version}`); From 54b706e9f7f84c3de0a53f728a01bb89a0fe01ee Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 20:37:00 -0600 Subject: [PATCH 5/8] Address review round 3: sentinel waits, sweep routing seeds, restore lmdb txn expiration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two in-flight duplicate-detection tests could pass vacuously: their `waitFor` swallowed its timeout and the only assertion left was "no duplicate (id,version)", which is trivially true over a partial set. Run alone, the count test timed out on every run (5.3s, green). Waiting for all 30 in-flight ids is not the fix — one that commits before the cursor's snapshot and falls outside `previousCount` is legitimately never delivered (17000-17002, measured). Both now write a record after the in-flight writes settle and wait for it, with the timeout terminal: deliveries follow commit order, so the sentinel's arrival bounds them. The burst test still used `collect()`'s quiet window while asserting all 600 ids arrived, and still flaked (1 failure in 32 lmdb runs). Converted, along with the three other tests that write after subscribe and then assert completeness, so the quiet window is left only where a quiet window is the right tool. The routing test pinned one graph, sampling a statistical property once: over 40 arbitrary seeds, 4 produce a graph that legitimately routes to a different entry point. It now sweeps eight fixed seeds, each asserted exactly — deterministic, unlike a divergence-rate bound, which at this rate would itself flake. Txn expiration teardown restored only the rocksdb global, so on the lmdb pass a 20ms open-transaction limit leaked into every later test in the process. Refs #1655 Co-Authored-By: Claude Opus --- DESIGN.md | 5 +- .../resources/subscriptionReplay.test.js | 106 +++++++++++---- unitTests/resources/txn-tracking.test.js | 3 + unitTests/resources/vectorIndex.test.js | 122 +++++++++--------- 4 files changed, 150 insertions(+), 86 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index f85e950b21..bcf8c139e0 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1098,7 +1098,10 @@ Greedy-equals-full is statistical, not per-graph: rare level layouts route to a entry point and displace the tail of the top-k (~2-3% of random 600-node graphs in the unit test's corpus). Tests that assert exact result-set equality across search strategies must therefore pin the graph: level assignment draws from the instance's `random` property (a test seam defaulting to -`Math.random`), which the routing test replaces with a seeded PRNG. +`Math.random`), which the routing test replaces with a seeded PRNG. One pinned graph samples the +property once, so that test sweeps a fixed list of seeds, each verified non-divergent when the list +was written — a seed that starts diverging after an intentional index change is a re-pin, not +necessarily a routing regression. ## `efConstruction` and the search-`ef` ceiling both auto-scale with the graph diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index 0ed8a3d892..2f15ddadf5 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -131,18 +131,29 @@ describe('Subscription replay', () => { await StartTimeTable.put(1000 + i, { name: 'pre' + i }); } const subscription = await StartTimeTable.subscribe({ startTime: startTime - 1, isCollection: true }); + const events = []; + subscription.on('data', (e) => events.push(e)); // fire writes concurrently with the replay loop const concurrentWrites = (async () => { for (let i = 0; i < 30; i++) { await StartTimeTable.put(2000 + i, { name: 'post' + i }); } })(); - const events = await collect(subscription, 100); await concurrentWrites; - // All writes have committed; wait for any pending subscription events to flush - // before closing. Without this, the last write's event can be queued but not - // yet delivered when return() closes the stream. - await delay(200); + // Wait for every expected id rather than for the stream to go quiet: under contention + // the gap between two deliveries can exceed any fixed window, and the per-id asserts + // below then name whichever one went missing. + await waitFor( + () => { + const ids = new Set(events.map((e) => e.id)); + for (let i = 0; i < 150; i++) if (!ids.has(1000 + i)) return false; + for (let i = 0; i < 30; i++) if (!ids.has(2000 + i)) return false; + return true; + }, + { timeout: 5000 } + ).catch(() => {}); + // additive settle: a duplicate trailing the last expected delivery can still land + await delay(100); subscription.return?.(); const ids = new Set(events.map((e) => e.id)); @@ -188,15 +199,25 @@ describe('Subscription replay', () => { await CountTable.put(3000 + i, { name: 'count_pre' + i }); } const subscription = await CountTable.subscribe({ previousCount: 5, isCollection: true }); + const events = []; + subscription.on('data', (e) => events.push(e)); // fire writes after subscribe returns but while replay loop is still running const concurrentWrites = (async () => { for (let i = 0; i < 20; i++) { await CountTable.put(4000 + i, { name: 'count_post' + i }); } })(); - const events = await collect(subscription, 150); await concurrentWrites; - await delay(200); + await waitFor( + () => { + const ids = new Set(events.map((e) => e.id)); + for (let i = 0; i < 20; i++) if (!ids.has(4000 + i)) return false; + return true; + }, + { timeout: 5000 } + ).catch(() => {}); + // additive settle: a duplicate trailing the last expected delivery can still land + await delay(100); subscription.return?.(); // concurrent writes should arrive @@ -245,15 +266,27 @@ describe('Subscription replay', () => { await CurrentStateTable.put(5000 + i, { name: 'dedupe_pre' + i }); } const subscription = await CurrentStateTable.subscribe({ isCollection: true }); + const events = []; + subscription.on('data', (e) => events.push(e)); // concurrent writes that may be observed by both cursor (snapshot:false) and the listener const concurrentWrites = (async () => { for (let i = 0; i < 30; i++) { await CurrentStateTable.put(5000 + i, { name: 'dedupe_updated' + i }); } })(); - const events = await collect(subscription, 150); await concurrentWrites; - await delay(200); + await waitFor( + () => { + const lastByKey = new Map(); + for (const e of events) lastByKey.set(e.id, e); + for (let i = 0; i < 30; i++) { + if (lastByKey.get(5000 + i)?.value?.name !== 'dedupe_updated' + i) return false; + } + return true; + }, + { timeout: 5000 } + ).catch(() => {}); + await delay(100); subscription.return?.(); // every updated key MUST be delivered with its final value at least once, even if @@ -644,21 +677,21 @@ describe('Subscription replay', () => { inFlight.push(CountTable.put(17000 + i, { name: 'count_race_inflight' + i })); } const subscription = await CountTable.subscribe({ previousCount: 10, isCollection: true }); - // The duplicate check below is only sound once every in-flight write's delivery has had - // the chance to arrive — collect()'s quiet window returning early made it vacuous. const events = []; subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); - await waitFor( - () => { - const seen = new Set(events.map((e) => e.id)); - for (let i = 0; i < 30; i++) if (!seen.has(17000 + i)) return false; - return true; - }, - { timeout: 5000 } - ).catch(() => {}); - // additive settle: a duplicate trailing the last expected delivery can still land; this - // can only catch more, never lose events + // The duplicate check below is only sound once every delivery that is coming has come. + // It cannot wait for all 30 in-flight ids: one that commits before the cursor's snapshot + // and falls outside previousCount is legitimately never delivered (running this test + // alone, 17000-17002 never arrive). Deliveries follow commit order, so a record written + // after the in-flight writes settle bounds them — once it arrives, they have all arrived. + await CountTable.put(17999, { name: 'count_race_sentinel' }); + await waitFor(() => events.some((e) => e.id === 17999), { + timeout: 5000, + message: 'sentinel written after the in-flight writes was never delivered', + }); + // additive settle: a duplicate trailing the sentinel can still land; this can only + // catch more, never lose events await delay(100); subscription.return?.(); @@ -681,14 +714,19 @@ describe('Subscription replay', () => { inFlight.push(RecordTable.put(15000, { name: 'inflight_v' + i })); } const subscription = await RecordTable.subscribe({ id: 15000, startTime: startTime - 1 }); - // Same soundness requirement as the count test above, but rapid same-record versions can - // legitimately coalesce, so the only guaranteed delivery is the final version — wait for - // it (any cursor/listener duplicate of an earlier version travels with its original). const events = []; subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); - await waitFor(() => events.some((e) => e.value?.name === 'inflight_v49'), { timeout: 5000 }).catch(() => {}); - // additive settle so a duplicate trailing the final version can still surface + // Same soundness requirement as the count test above. Rapid same-record versions can + // legitimately coalesce, so no in-flight version is guaranteed to be delivered on its + // own; a version written after they all settle is (any cursor/listener duplicate of an + // earlier version travels with its original, so it precedes the sentinel). + await RecordTable.put(15000, { name: 'inflight_sentinel' }); + await waitFor(() => events.some((e) => e.value?.name === 'inflight_sentinel'), { + timeout: 5000, + message: 'sentinel version written after the in-flight writes was never delivered', + }); + // additive settle so a duplicate trailing the sentinel can still surface await delay(100); subscription.return?.(); @@ -1035,7 +1073,21 @@ describe('Subscription replay', () => { for (let i = 0; i < N; i++) { await T.put(20000 + i, { name: 'burst' + i }); } - const events = await collect(subscription, 300); + const events = []; + subscription.on('data', (e) => events.push(e)); + // Every write is committed and awaited before this point, so all N must be delivered — + // wait for that rather than for a quiet window, which a mid-drain stall longer than the + // window ends early (measured: 1 failure in 32 lmdb runs, "missing 7 of 600"). + await waitFor( + () => { + const seen = new Set(events.map((e) => e.id)); + for (let i = 0; i < N; i++) if (!seen.has(20000 + i)) return false; + return true; + }, + { timeout: 10000 } + ).catch(() => {}); + // additive settle: a duplicate trailing the last delivery can still land + await delay(100); subscription.return?.(); const ids = new Set(events.map((e) => e.id)); diff --git a/unitTests/resources/txn-tracking.test.js b/unitTests/resources/txn-tracking.test.js index c81063d59f..f61a90d041 100644 --- a/unitTests/resources/txn-tracking.test.js +++ b/unitTests/resources/txn-tracking.test.js @@ -102,7 +102,10 @@ describe('Txn Expiration', () => { } }); after(function () { + // the 20ms limit above went to whichever engine is active; both module globals are + // process-wide, so both are restored rather than leaking 20ms into the rest of the run setTxnExpiration(30000); + setLMDBTxnExpiration(30000); }); }); diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 4109be97c0..18432cf34c 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -934,25 +934,37 @@ describe('HNSW construction ef auto-scale (#2180)', () => { describe('HNSW greedy routing above layer 0 (ROUTING_EF)', () => { if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return; - let T; const N = 600; + // Each graph's level assignment is pinned with a seeded PRNG (mulberry32) so a run is + // reproducible: greedy-vs-full equality is only statistically true over random graphs — ~2-3% + // of random 600-node graphs legitimately route to a different entry point and change the + // top-10 tail, which is what flaked on CI. One pinned graph samples that property once, so the + // assertion sweeps several: all of these are non-divergent at this head (measured over 40 + // arbitrary seeds, 4 diverged, so a divergent seed here after an intentional index change is a + // re-pin rather than necessarily a regression — see DESIGN.md). + const SEEDS = [0x9e3779b9, 0x85ebca6b, 0xc2b2ae35, 0x27d4eb2f, 0x165667b1, 0x7feb352d, 0x846ca68b, 0xff51afd7]; + const targets = [ + [1, 0, 0, 0], + [0, 1, 0.5, 0.2], + [-0.6, 0.3, 0.9, 0.4], + [0.2, -0.8, 0.1, 0.7], + ]; - before(async () => { + before(() => { setupTestDBPath(); setMainIsWorker(true); - T = table({ - table: 'HNSWRoutingTest', + }); + + async function buildGraph(seed) { + const T = table({ + table: 'HNSWRoutingTest' + (seed >>> 0).toString(16), database: 'test', attributes: [ { name: 'id', isPrimaryKey: true }, { name: 'vector', indexed: { type: 'HNSW', distance: 'cosine' }, type: 'Array' }, ], }); - // Seed level assignment (mulberry32) so the graph is identical every run. The greedy-vs-full - // equality below is only statistically true over random graphs — rare level layouts - // legitimately route to a different entry point and change the top-10 tail (flaked ~2-3% of - // runs on CI). Pinning the graph keeps the assertion exact without weakening it. - let seedState = 0x9e3779b9; + let seedState = seed; T.indices.vector.customIndex.random = () => { seedState = (seedState + 0x6d2b79f5) | 0; let t = Math.imul(seedState ^ (seedState >>> 15), 1 | seedState); @@ -964,63 +976,57 @@ describe('HNSW greedy routing above layer 0 (ROUTING_EF)', () => { const b = ((i * 7) % N) / N; await T.put(i, { vector: [Math.cos(a), Math.sin(a), b, (i % 11) / 11] }); } - }); + return T; + } - after(() => { - T.dropTable(); - }); + async function topTenIds(T, target) { + return ( + await fromAsync( + T.search({ sort: { attribute: 'vector', target, distance: 'cosine' }, select: ['id'], limit: 10 }) + ) + ) + .map((r) => r.id) + .join(','); + } // Layers above 0 only supply the next layer's entry point, so descending greedily must not cost // accuracy. Compare against the same graph searched with the full ef at every layer — the // pre-change behaviour — rather than against a fixed expectation. it('returns the same neighbours as searching every layer at the full ef', async () => { - const customIndex = T.indices.vector.customIndex; - const originalSearchLayer = customIndex.searchLayer; - const targets = [ - [1, 0, 0, 0], - [0, 1, 0.5, 0.2], - [-0.6, 0.3, 0.9, 0.4], - [0.2, -0.8, 0.1, 0.7], - ]; - - const greedy = []; - for (const target of targets) { - greedy.push( - ( - await fromAsync( - T.search({ sort: { attribute: 'vector', target, distance: 'cosine' }, select: ['id'], limit: 10 }) - ) - ) - .map((r) => r.id) - .join(',') - ); - } + for (const seed of SEEDS) { + const label = '0x' + (seed >>> 0).toString(16); + const T = await buildGraph(seed); + try { + const customIndex = T.indices.vector.customIndex; + const greedy = []; + for (const target of targets) { + greedy.push(await topTenIds(T, target)); + } - // Every layer at the ef layer 0 actually resolves to — what search() passed down before greedy - // descent. Read it from a real query rather than efConstructionSearch, which is only the - // pre-change value when a schema configures one; this index takes the auto-scaled path. - const resolvedLayer0Ef = await captureLayer0Ef(T, { limit: 10 }); - assert(resolvedLayer0Ef > 1, `expected an auto-scaled layer-0 ef, got ${resolvedLayer0Ef}`); - customIndex.searchLayer = function (v, epId, ep, ef, level, ...rest) { - return originalSearchLayer.call(this, v, epId, ep, level > 0 ? resolvedLayer0Ef : ef, level, ...rest); - }; - try { - for (let i = 0; i < targets.length; i++) { - const full = ( - await fromAsync( - T.search({ - sort: { attribute: 'vector', target: targets[i], distance: 'cosine' }, - select: ['id'], - limit: 10, - }) - ) - ) - .map((r) => r.id) - .join(','); - assert.strictEqual(greedy[i], full, `greedy descent changed the result set for target ${i}`); + // Every layer at the ef layer 0 actually resolves to — what search() passed down before + // greedy descent. Read it from a real query rather than efConstructionSearch, which is + // only the pre-change value when a schema configures one; this index takes the + // auto-scaled path. + const resolvedLayer0Ef = await captureLayer0Ef(T, { limit: 10 }); + assert(resolvedLayer0Ef > 1, `expected an auto-scaled layer-0 ef, got ${resolvedLayer0Ef} (seed ${label})`); + const originalSearchLayer = customIndex.searchLayer; + customIndex.searchLayer = function (v, epId, ep, ef, level, ...rest) { + return originalSearchLayer.call(this, v, epId, ep, level > 0 ? resolvedLayer0Ef : ef, level, ...rest); + }; + try { + for (let i = 0; i < targets.length; i++) { + assert.strictEqual( + greedy[i], + await topTenIds(T, targets[i]), + `greedy descent changed the result set for target ${i} (seed ${label})` + ); + } + } finally { + customIndex.searchLayer = originalSearchLayer; + } + } finally { + T.dropTable(); } - } finally { - customIndex.searchLayer = originalSearchLayer; } }); }); From 4c6727b77bdef28685222fd59f0446b4e05eded4 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 20:52:50 -0600 Subject: [PATCH 6/8] Address pre-push review: await the per-seed table drop, trim repeated comments dropTable() is async and drains in-flight writes before dropping column families; unawaited in the seed loop it would race the next graph's 600 puts and turn a LOCK_TIMEOUT rejection into an unhandled rejection. The sentinel comments now name why the cursor cannot deliver the sentinel (its history/entry is captured before subscribe() resolves) rather than only asserting commit order, which was the review's open question. Co-Authored-By: Claude Opus --- unitTests/resources/subscriptionReplay.test.js | 13 +++++++------ unitTests/resources/txn-tracking.test.js | 4 ++-- unitTests/resources/vectorIndex.test.js | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index 2f15ddadf5..18b324e640 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -216,7 +216,6 @@ describe('Subscription replay', () => { }, { timeout: 5000 } ).catch(() => {}); - // additive settle: a duplicate trailing the last expected delivery can still land await delay(100); subscription.return?.(); @@ -683,8 +682,10 @@ describe('Subscription replay', () => { // The duplicate check below is only sound once every delivery that is coming has come. // It cannot wait for all 30 in-flight ids: one that commits before the cursor's snapshot // and falls outside previousCount is legitimately never delivered (running this test - // alone, 17000-17002 never arrive). Deliveries follow commit order, so a record written - // after the in-flight writes settle bounds them — once it arrives, they have all arrived. + // alone, 17000-17002 never arrive). A record written after they settle bounds them + // instead: the previousCount cursor collects its history from a snapshot taken before + // subscribe() resolves, so it cannot deliver this record, and the queue that will is + // drained in commit order. await CountTable.put(17999, { name: 'count_race_sentinel' }); await waitFor(() => events.some((e) => e.id === 17999), { timeout: 5000, @@ -719,8 +720,9 @@ describe('Subscription replay', () => { await Promise.all(inFlight); // Same soundness requirement as the count test above. Rapid same-record versions can // legitimately coalesce, so no in-flight version is guaranteed to be delivered on its - // own; a version written after they all settle is (any cursor/listener duplicate of an - // earlier version travels with its original, so it precedes the sentinel). + // own; a version written after they all settle is. The cursor captures this record's + // entry before subscribe() resolves, so it cannot deliver that later version — it + // arrives through the commit-ordered queue, after every in-flight delivery. await RecordTable.put(15000, { name: 'inflight_sentinel' }); await waitFor(() => events.some((e) => e.value?.name === 'inflight_sentinel'), { timeout: 5000, @@ -1086,7 +1088,6 @@ describe('Subscription replay', () => { }, { timeout: 10000 } ).catch(() => {}); - // additive settle: a duplicate trailing the last delivery can still land await delay(100); subscription.return?.(); diff --git a/unitTests/resources/txn-tracking.test.js b/unitTests/resources/txn-tracking.test.js index f61a90d041..6b6e55242f 100644 --- a/unitTests/resources/txn-tracking.test.js +++ b/unitTests/resources/txn-tracking.test.js @@ -102,8 +102,8 @@ describe('Txn Expiration', () => { } }); after(function () { - // the 20ms limit above went to whichever engine is active; both module globals are - // process-wide, so both are restored rather than leaking 20ms into the rest of the run + // both expiration globals are process-wide, and the 20ms above went to whichever engine + // is active, so restoring only one leaks it into every later test on the other pass setTxnExpiration(30000); setLMDBTxnExpiration(30000); }); diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 18432cf34c..cc550a56d1 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1025,7 +1025,7 @@ describe('HNSW greedy routing above layer 0 (ROUTING_EF)', () => { customIndex.searchLayer = originalSearchLayer; } } finally { - T.dropTable(); + await T.dropTable(); } } }); From 11f62278ff77e9413a68d79b6a3102a3fdf54c03 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 22:50:52 -0600 Subject: [PATCH 7/8] Give the fresh-DB in-flight wait an explicit timeout so a lost write names its id The FIRST-subscription-on-fresh-DB completeness wait took waitFor's bare 2000ms default with no catch, so a genuinely lost delivery died inside waitFor with "Timed out after 2000ms waiting for condition" and never reached the per-id assert that names it. Matches the timeout+catch shape every other converted wait in this file already uses. Co-Authored-By: Claude Opus --- unitTests/resources/subscriptionReplay.test.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index 18b324e640..4ef8ef8489 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -372,12 +372,16 @@ describe('Subscription replay', () => { await Promise.all(inFlight); // Wait for every in-flight write to be delivered rather than guessing a fixed // duration — the fixed wait raced loaded runners and was the source of the - // intermittent "missing id" failures. - await waitFor(() => { - const seen = new Set(events.map((e) => e.id)); - for (let i = 0; i < 200; i++) if (!seen.has(20000 + i)) return false; - return true; - }); + // intermittent "missing id" failures. The timeout falls through so the per-id + // asserts below name whichever write was lost instead of reporting a bare timeout. + await waitFor( + () => { + const seen = new Set(events.map((e) => e.id)); + for (let i = 0; i < 200; i++) if (!seen.has(20000 + i)) return false; + return true; + }, + { timeout: 5000 } + ).catch(() => {}); subscription.return?.(); const ids = new Set(events.map((e) => e.id)); From d894e40ef64a8b4fe07541d62f1cd5c6984870fc Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 00:38:45 -0600 Subject: [PATCH 8/8] Link the fresh-DB in-flight flake to the product defect it exposes The FIRST-subscription-on-fresh-DB wait fails as `missing in-flight id N` when the startTime replay branch drops a live event committed after its audit cursor terminated. Filed as #2311 (P1, epic #1651) with the code trace and a forced repro; point the test comment at it so a future failure here is read as the product bug rather than a timing threshold. Refs #2311 Co-Authored-By: Claude Opus --- unitTests/resources/subscriptionReplay.test.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/unitTests/resources/subscriptionReplay.test.js b/unitTests/resources/subscriptionReplay.test.js index 4ef8ef8489..317acf3475 100644 --- a/unitTests/resources/subscriptionReplay.test.js +++ b/unitTests/resources/subscriptionReplay.test.js @@ -370,10 +370,11 @@ describe('Subscription replay', () => { const events = []; subscription.on('data', (e) => events.push(e)); await Promise.all(inFlight); - // Wait for every in-flight write to be delivered rather than guessing a fixed - // duration — the fixed wait raced loaded runners and was the source of the - // intermittent "missing id" failures. The timeout falls through so the per-id - // asserts below name whichever write was lost instead of reporting a bare timeout. + // Wait on the deliveries themselves rather than a fixed duration, and let the timeout + // fall through so the per-id asserts below name whichever write was lost. A + // `missing in-flight id N` failure here is not a test-timing problem: it is the lost + // delivery in https://github.com/HarperFast/harper/issues/2311 — the startTime replay + // branch drops live events committed after its audit cursor has terminated. await waitFor( () => { const seen = new Set(events.map((e) => e.id));