From 16f72ccad7ef6c987829358021caf092738b4ac4 Mon Sep 17 00:00:00 2001 From: Artificium Date: Tue, 28 Jul 2026 01:38:47 +0000 Subject: [PATCH] =?UTF-8?q?fix(query):=20give=20retrieval=20a=20total=20or?= =?UTF-8?q?der=20=E2=80=94=20results=20were=20nondeterministic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running the same benchmark twice against an unchanged corpus produced different scores (Recall@3 moved 2.4pp with nothing changed). Cause: every retrieval path ordered by relevance alone, and relevance ties constantly — measured on the real corpus, 20 retrieved rows shared just 6 distinct ts_rank_cd values, with ties at the very top. `ORDER BY rank DESC` with no second key lets Postgres return tied rows in any order, and it does. This is a correctness problem before it is a measurement one. The same query could return different memories on consecutive calls; a caller paging with LIMIT could see a row twice and miss another entirely; and no improvement could be distinguished from noise, which would have made the upcoming full benchmark run unreproducible. Adds `id DESC` as the tie-break on all six ordering sites: warm-tier keyword, code, trigram and semantic, plus both shared-pool arms. Newest -first is the useful semantic — when relevance cannot separate two memories, prefer the more recent — and it is pinned by a test so a future edit cannot silently invert it. After: three consecutive evaluations returned byte-identical retrieved- id sequences, not merely equal aggregate scores. Note this changes measured numbers slightly, because the previous ones were partly luck: R@3 on the stratified 42-question sample had been oscillating between 81.0% and 83.3%, and settles at 81.0%. tests/retrieval-determinism.test.ts covers repeat-query stability, the newest-first tie order, and page/limit consistency. The fixture needs rows that tie on rank while differing in their first 100 characters, since query() collapses same-prefix results; ts_rank_cd applies no length normalisation, so an unmatched leading marker leaves rank untouched — verified before relying on it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011xCqQo49d3CEbn6oEvb3Ru --- package.json | 3 +- src/memory-manager.ts | 23 +++-- tests/retrieval-determinism.test.ts | 126 ++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 tests/retrieval-determinism.test.ts diff --git a/package.json b/package.json index 0f786e5..25bd9ce 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "test:dreams-compat": "node --import tsx/esm --test tests/dreams-compat.test.ts", "test:dreams-anthropic": "node --import tsx/esm --test tests/dreams-anthropic.test.ts", "test:dreams-bridge": "node --import tsx/esm --test tests/dreams-bridge.test.ts", - "test": "node --import tsx/esm --test --test-concurrency=1 tests/integration.test.ts tests/llm-paths.test.ts tests/http-api.test.ts tests/cache.test.ts tests/embedding-migration.test.ts tests/outcome-revision.test.ts tests/reflection-revision.test.ts tests/selective-forgetting.test.ts tests/multi-device.test.ts tests/dream-runs.test.ts tests/dreams-compat.test.ts tests/dreams-anthropic.test.ts tests/dreams-bridge.test.ts tests/sentiment-tagging.test.ts tests/adaptive-sleep.test.ts tests/epistemic-confidence.test.ts tests/explainable-memory.test.ts tests/causal-graph.test.ts tests/abstractions.test.ts tests/bootstrap.test.ts tests/contested-conflicts.test.ts tests/benchmark-metrics.test.ts tests/cache-degradation.test.ts", + "test": "node --import tsx/esm --test --test-concurrency=1 tests/integration.test.ts tests/llm-paths.test.ts tests/http-api.test.ts tests/cache.test.ts tests/embedding-migration.test.ts tests/outcome-revision.test.ts tests/reflection-revision.test.ts tests/selective-forgetting.test.ts tests/multi-device.test.ts tests/dream-runs.test.ts tests/dreams-compat.test.ts tests/dreams-anthropic.test.ts tests/dreams-bridge.test.ts tests/sentiment-tagging.test.ts tests/adaptive-sleep.test.ts tests/epistemic-confidence.test.ts tests/explainable-memory.test.ts tests/causal-graph.test.ts tests/abstractions.test.ts tests/bootstrap.test.ts tests/contested-conflicts.test.ts tests/benchmark-metrics.test.ts tests/cache-degradation.test.ts tests/retrieval-determinism.test.ts", "test:multi-device": "node --import tsx/esm --test tests/multi-device.test.ts", "test:sentiment-tagging": "node --import tsx/esm --test tests/sentiment-tagging.test.ts", "test:adaptive-sleep": "node --import tsx/esm --test tests/adaptive-sleep.test.ts", @@ -57,6 +57,7 @@ "test:contested-conflicts": "node --import tsx/esm --test tests/contested-conflicts.test.ts", "test:benchmark-metrics": "node --import tsx/esm --test tests/benchmark-metrics.test.ts", "test:cache-degradation": "node --import tsx/esm --test tests/cache-degradation.test.ts", + "test:retrieval-determinism": "node --import tsx/esm --test tests/retrieval-determinism.test.ts", "benchmark:longmemeval": "node --import tsx/esm benchmarks/longmemeval/run.ts", "benchmark:download": "node --import tsx/esm benchmarks/longmemeval/download.ts", "benchmark:ingest": "node --import tsx/esm benchmarks/longmemeval/ingest.ts", diff --git a/src/memory-manager.ts b/src/memory-manager.ts index 06d095e..dd561a8 100644 --- a/src/memory-manager.ts +++ b/src/memory-manager.ts @@ -728,11 +728,11 @@ Ranking (numbers only):`; ? `SELECT id, content, summary, metadata, published_at as consolidated_at, source_agent_id, hop_count, base_confidence, importance, ts_rank_cd(content_tsv, plainto_tsquery('english', $2)) * importance AS rank FROM shared_memories WHERE pool_id = $1 AND content_tsv @@ plainto_tsquery('english', $2) - ORDER BY rank DESC LIMIT $3` + ORDER BY rank DESC, id DESC LIMIT $3` : `SELECT id, content, summary, metadata, published_at as consolidated_at, source_agent_id, hop_count, base_confidence, importance, (1 - (embedding <=> $2::${await this.vcast()})) * importance AS rank FROM shared_memories WHERE pool_id = $1 AND embedding IS NOT NULL - ORDER BY embedding <=> $2::${await this.vcast()} LIMIT $3`, + ORDER BY embedding <=> $2::${await this.vcast()}, id DESC LIMIT $3`, mode === 'keyword' || mode === 'code' ? [pool.pool_id, searchText, Math.min(resolvedLimit, 10)] : [pool.pool_id, `[${(await this.embedder.embed(searchText)).join(',')}]`, Math.min(resolvedLimit, 10)], @@ -994,7 +994,14 @@ Ranking (numbers only):`; AND w.namespace = $3 AND w.content_tsv @@ q.tsq ${timeFilter} - ORDER BY rank DESC + -- Deterministic tie-break. ts_rank_cd produces heavy ties (measured: 20 + -- retrieved rows sharing only 6 distinct ranks), and without a stable + -- second key Postgres may return tied rows in any order — the same + -- query gave different results run to run, which makes retrieval + -- irreproducible for callers and unmeasurable for the benchmark. + -- Newest-first among equals is the useful semantic: when relevance + -- cannot distinguish two memories, prefer the more recent one. + ORDER BY rank DESC, w.id DESC LIMIT $${limitIdx}`, params, ); @@ -1034,7 +1041,8 @@ Ranking (numbers only):`; AND namespace = $3 AND content_code_tsv @@ plainto_tsquery('simple', $2) ${timeFilter} - ORDER BY rank DESC + -- Stable tie-break; see queryKeyword. + ORDER BY rank DESC, id DESC LIMIT $${limitIdx}`, params, ); @@ -1071,7 +1079,8 @@ Ranking (numbers only):`; AND namespace = $4 AND content ILIKE $3 ${timeFilter} - ORDER BY rank DESC + -- Stable tie-break; see queryKeyword. + ORDER BY rank DESC, id DESC LIMIT $${limitIdx}`, params, ); @@ -1111,7 +1120,9 @@ Ranking (numbers only):`; AND namespace = $3 AND embedding IS NOT NULL ${timeFilter} - ORDER BY embedding <=> $2::${await this.vcast()} + -- Stable tie-break; see queryKeyword. Vector distances tie less often + -- than lexical ranks, but halfvec quantization makes it possible. + ORDER BY embedding <=> $2::${await this.vcast()}, id DESC LIMIT $${limitIdx}`, params, ); diff --git a/tests/retrieval-determinism.test.ts b/tests/retrieval-determinism.test.ts new file mode 100644 index 0000000..0c8779b --- /dev/null +++ b/tests/retrieval-determinism.test.ts @@ -0,0 +1,126 @@ +// MemForge — retrieval determinism tests +// +// Ranking functions tie constantly: measured on a real corpus, 20 retrieved +// rows shared only 6 distinct ts_rank_cd values. With `ORDER BY rank DESC` +// and no second key, Postgres is free to return tied rows in any order, so +// the same query against unchanged data returned different results run to +// run — observed as a 2.4pp swing in benchmark Recall@3 with nothing changed. +// +// That is a correctness problem before it is a measurement problem: callers +// paging or caching results, and anyone trying to tell an improvement from +// noise, both need a total order. +// +// Run: node --import tsx/esm --test tests/retrieval-determinism.test.ts +// Requires: DATABASE_URL + +import { describe, it, before, after } from 'node:test'; +import assert from 'node:assert/strict'; +import { Pool } from 'pg'; + +const { MemoryManager } = await import('../src/memory-manager.js'); +const { NoOpEmbeddingProvider } = await import('../src/embedding.js'); +const { closePool } = await import('../src/db.js'); + +const DATABASE_URL = process.env['DATABASE_URL']; +if (!DATABASE_URL) { + console.error('[test] DATABASE_URL is required — set it to a test database'); + process.exit(1); +} + +const TEST_AGENT = 'test-agent-retrieval-determinism'; +const pool = new Pool({ connectionString: DATABASE_URL }); + +const manager = new MemoryManager({ + databaseUrl: DATABASE_URL, + consolidationBatchSize: 500, + consolidationThreshold: 1, + autoRegisterAgents: true, + consolidationMode: 'concat', + temporalDecayRate: 0, + embeddingProvider: new NoOpEmbeddingProvider(), + llmProvider: null, + sleepCycle: { + tokenBudget: 100_000, + evictionThreshold: 0.05, + revisionThreshold: 0.4, + includeReflection: false, + weights: { recency: 0.25, frequency: 0.20, centrality: 0.20, reflection: 0.15, stability: 0.20 }, + }, +}); + +async function cleanup(): Promise { + await pool.query(`DELETE FROM retrieval_log WHERE agent_id = $1`, [TEST_AGENT]); + await pool.query(`DELETE FROM knowledge_gaps WHERE agent_id = $1`, [TEST_AGENT]); + await pool.query(`DELETE FROM warm_tier WHERE agent_id = $1`, [TEST_AGENT]); + await pool.query(`DELETE FROM hot_tier WHERE agent_id = $1`, [TEST_AGENT]); + await pool.query(`DELETE FROM agents WHERE id = $1`, [TEST_AGENT]); +} + +describe('retrieval ordering is deterministic under rank ties', () => { + before(async () => { + await cleanup(); + await pool.query(`INSERT INTO agents (id) VALUES ($1) ON CONFLICT DO NOTHING`, [TEST_AGENT]); + + // Rows must tie on rank while differing in their opening text: query() + // collapses results sharing their first 100 characters ("prevents similar + // memories filling all top-k slots"), so wholly identical rows would be + // deduplicated to one and never exercise the ordering path. + // + // ts_rank_cd uses no length normalisation by default, so a distinct + // leading marker that the query does not match leaves rank untouched — + // verified: 6 such rows produce exactly 1 distinct rank. + for (let i = 0; i < 12; i++) { + await pool.query( + `INSERT INTO warm_tier (agent_id, content, content_hash, importance) + VALUES ($1, $2, $3, 0.5)`, + [ + TEST_AGENT, + `marker${i} distinct opening words here to defeat the dedup prefix check, followed by quarterly planning probe`, + `det-${i}`, + ], + ); + } + }); + after(async () => { + await cleanup(); + await pool.end(); + await closePool(); + }); + + it('returns the same ids in the same order across repeated identical queries', async () => { + const runs: string[][] = []; + for (let i = 0; i < 5; i++) { + const results = await manager.query(TEST_AGENT, { q: 'quarterly planning probe', limit: 10 }); + runs.push(results.map((r) => String(r.id))); + } + + assert.equal(runs[0]!.length, 10, 'fixture must produce a full page of rank-tied rows'); + for (let i = 1; i < runs.length; i++) { + assert.deepEqual(runs[i], runs[0], `run ${i + 1} returned a different order than run 1`); + } + }); + + it('orders tied rows newest-first', async () => { + // The documented tie semantic: when relevance cannot separate two + // memories, prefer the more recent. Pinned so a future ORDER BY edit + // cannot silently invert it. + const results = await manager.query(TEST_AGENT, { q: 'quarterly planning probe', limit: 10 }); + const ids = results.map((r) => BigInt(r.id)); + + const descending = [...ids].sort((a, b) => (a > b ? -1 : a < b ? 1 : 0)); + assert.deepEqual(ids, descending, 'tied rows must come back in descending id order'); + }); + + it('keeps paging stable — a second page does not repeat the first', async () => { + // Without a total order, LIMIT/paging over tied rows can return the same + // row on consecutive pages and drop others entirely. + const page = await manager.query(TEST_AGENT, { q: 'quarterly planning probe', limit: 5 }); + const wider = await manager.query(TEST_AGENT, { q: 'quarterly planning probe', limit: 10 }); + + assert.deepEqual( + wider.slice(0, 5).map((r) => String(r.id)), + page.map((r) => String(r.id)), + 'the first 5 of a 10-row query must equal the 5-row query', + ); + }); +});