From be6c4dc182b118fc982680fd9eff08e17cef67af Mon Sep 17 00:00:00 2001 From: David Snelling Date: Thu, 9 Apr 2026 16:26:38 -0700 Subject: [PATCH] fix: correct orderBy sort for timestamp fields via centralized field resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Muse reported that find({ orderBy: 'createdAt' }) silently returned the wrong order: getFieldValueForEntity read noun.metadata[field] for timestamp fields, but getNoun() destructures standard fields to the top level, so the read always returned undefined and the sort collapsed to insertion order. The fix introduces a single source of truth for reading fields off an entity. STANDARD_ENTITY_FIELDS + resolveEntityField() in coreTypes.ts encode the "standard fields top-level, custom fields nested" contract in one place. getFieldValueForEntity now uses the helper and routes through a named BUCKETED_INDEX_FIELDS set instead of a hardcoded timestamp if-chain — filtered sort on createdAt/updatedAt now works. Unfiltered orderBy is explicitly rejected with a clear error pointing callers at the right pattern. A scalable, unfiltered-sort-capable time-ordered segment index is tracked as a separate follow-up. --- src/brainy.ts | 15 ++ src/coreTypes.ts | 52 +++++ src/utils/metadataIndex.ts | 82 +++++--- tests/integration/orderby-sort-bug.test.ts | 223 +++++++++++++++++++++ 4 files changed, 343 insertions(+), 29 deletions(-) create mode 100644 tests/integration/orderby-sort-bug.test.ts diff --git a/src/brainy.ts b/src/brainy.ts index 68aa17ae..5c8936dc 100644 --- a/src/brainy.ts +++ b/src/brainy.ts @@ -2166,6 +2166,21 @@ export class Brainy implements BrainyInterface { const limit = params.limit || 20 const offset = params.offset || 0 + // orderBy without any filter is not supported — it would require + // O(N) work across every entity in storage. Consumers must supply + // a filter (`type`, `where`, or `excludeVFS: true`) so the sort + // runs over a bounded, roaring-bitmap-filtered set. The long-term + // fix is a dedicated time-ordered segment index (Track 2). + if (params.orderBy) { + throw new Error( + `find({ orderBy: '${params.orderBy}' }) requires a filter. ` + + `Add 'type', 'where', or 'excludeVFS: true' so the sort runs ` + + `over a bounded set. Unfiltered sort over all entities is not ` + + `scalable with the current index and is tracked as a dedicated ` + + `time-ordered segment index (Track 2).` + ) + } + // ExcludeVFS helper - exclude VFS infrastructure entities // VFS files/folders have vfsType set, extracted entities do NOT let filter: any = {} diff --git a/src/coreTypes.ts b/src/coreTypes.ts index 08135bb1..f8a67144 100644 --- a/src/coreTypes.ts +++ b/src/coreTypes.ts @@ -233,6 +233,58 @@ export interface HNSWNounWithMetadata { metadata?: Record } +/** + * Standard top-level fields on HNSWNounWithMetadata. + * + * Single source of truth for the entity shape contract: fields listed here + * live at the top level of the entity; any other field is a custom user + * field and lives in `entity.metadata`. + * + * Keep this set in lockstep with the HNSWNounWithMetadata interface above. + * Adding a new top-level field to the interface? Add it here too, or + * `resolveEntityField` will look for it in the wrong place. + */ +export const STANDARD_ENTITY_FIELDS: ReadonlySet = new Set([ + 'id', + 'vector', + 'connections', + 'level', + 'type', + 'confidence', + 'weight', + 'createdAt', + 'updatedAt', + 'service', + 'createdBy', + 'data' +]) + +/** + * Resolve a field value off an entity by name. + * + * Encodes the HNSWNounWithMetadata shape contract in one place: standard + * fields live at the top level, custom user fields live in `entity.metadata`. + * Use this helper anywhere code needs to read a field by name (sorting, + * filtering, aggregation) instead of reaching into the entity directly. + * + * @param entity - The entity to read from + * @param field - The field name to resolve + * @returns The field value, or undefined if not present + * + * @example + * resolveEntityField(noun, 'createdAt') // reads noun.createdAt (top-level) + * resolveEntityField(noun, 'customTag') // reads noun.metadata?.customTag + */ +export function resolveEntityField( + entity: HNSWNounWithMetadata, + field: string +): unknown { + if (STANDARD_ENTITY_FIELDS.has(field)) { + return (entity as unknown as Record)[field] + } + return entity.metadata?.[field] +} + /** * Combined verb structure for transport/API boundaries * diff --git a/src/utils/metadataIndex.ts b/src/utils/metadataIndex.ts index f408a999..3fb31db6 100644 --- a/src/utils/metadataIndex.ts +++ b/src/utils/metadataIndex.ts @@ -4,7 +4,7 @@ * Automatically updates indexes when data changes */ -import { StorageAdapter } from '../coreTypes.js' +import { StorageAdapter, resolveEntityField } from '../coreTypes.js' import { MetadataIndexCache, MetadataIndexCacheConfig } from './metadataIndexCache.js' import { prodLog } from './logger.js' import { getGlobalCache, UnifiedCache } from './unifiedCache.js' @@ -28,6 +28,21 @@ import { EntityIdMapper } from './entityIdMapper.js' import { RoaringBitmap32, roaringLibraryInitialize } from './roaring/index.js' import { FieldTypeInference, FieldType } from './fieldTypeInference.js' +/** + * Fields whose values are stored in the sparse index as BUCKETED values + * (rounded to a coarser granularity to keep the index compact). Sorting + * and any precision-sensitive comparison on these fields must bypass the + * index and read the actual value directly from entity storage. + * + * Currently only timestamps are bucketed — they round to 1-minute windows + * via `Math.floor(ts / 60000) * 60000` in the chunking layer. If any new + * bucketed field is added (e.g. a compressed float), add it here too. + */ +const BUCKETED_INDEX_FIELDS: ReadonlySet = new Set([ + 'createdAt', + 'updatedAt' +]) + export interface MetadataIndexEntry { field: string value: string | number | boolean @@ -2216,6 +2231,12 @@ export class MetadataIndexManager { order: 'asc' | 'desc' = 'asc' ): Promise { // 1. Get filtered IDs using existing roaring bitmap implementation (fast!) + // + // NOTE: This method REQUIRES a non-empty filter. Unfiltered sort over all + // entities is not supported here because it would require O(N) storage reads + // on bucketed fields (timestamps), which does not scale. The proper solution + // is a dedicated time-ordered segment index — tracked as Track 2. + // Callers should enforce a filter (type / where / excludeVFS) before calling. const filteredIds = await this.getIdsForFilter(filter) if (filteredIds.length === 0) { @@ -2254,17 +2275,24 @@ export class MetadataIndexManager { /** * Get field value for a specific entity (helper for sorted queries) * - * **IMPORTANT**: For timestamp fields (createdAt, updatedAt), this loads - * the ACTUAL value from entity metadata, NOT the bucketed index value. - * This is required because timestamp bucketing (1-minute precision) loses - * precision needed for accurate sorting. + * Three-path lookup: * - * For non-timestamp fields, loads from the chunked sparse index without - * loading the full entity. This is critical for production-scale sorting. + * 1. **Bucketed fields** (timestamps) — the sparse index stores values + * rounded to 1-minute buckets to keep the index compact for range + * queries. That bucketing loses precision, so sorting must read the + * actual value directly from entity storage. + * + * 2. **Custom fields with no sparse index** — VFS fields like `modified` + * and `accessed`, plus any user custom field whose sparse index was + * never built. Resolved from entity storage via `resolveEntityField`, + * which knows the top-level-vs-metadata shape contract. + * + * 3. **Indexed fields** — strings, enums, and low-cardinality ints live + * in the sparse roaring index. O(chunks) lookup, typically 1-10 chunks. * * **Performance**: - * - Timestamp fields: O(1) metadata load from storage (cached) - * - Other fields: O(chunks) roaring bitmap lookup (typically 1-10 chunks) + * - Paths 1 & 2: O(1) entity load from storage (cached) + * - Path 3: O(chunks) roaring bitmap lookup * * @param entityId - Entity UUID to get field value for * @param field - Field name to retrieve (e.g., 'createdAt', 'title') @@ -2273,43 +2301,39 @@ export class MetadataIndexManager { * @public (called from brainy.ts for sorted queries) */ async getFieldValueForEntity(entityId: string, field: string): Promise { - // For timestamp fields, load ACTUAL value from entity metadata - // (index has bucketed values which lose precision for sorting) - if (field === 'createdAt' || field === 'updatedAt' || field === 'accessed' || field === 'modified') { - try { - const noun = await this.storage.getNoun(entityId) - if (noun && noun.metadata) { - return noun.metadata[field] - } - } catch (err) { - // If metadata load fails, fall back to index (bucketed value) - console.warn(`[MetadataIndex] Failed to load ${field} from metadata for ${entityId}, using bucketed value`) - } + // Path 1: Bucketed fields need the actual value from storage. + if (BUCKETED_INDEX_FIELDS.has(field)) { + const noun = await this.storage.getNoun(entityId) + return noun ? resolveEntityField(noun, field) : undefined } - // For non-timestamp fields, use the sparse index (no bucketing issues) + // Path 3 precondition: entity must be in the id mapper for bitmap lookup. const intId = this.idMapper.getInt(entityId) if (intId === undefined) { return undefined } - // Load sparse index for this field (cached via UnifiedCache) + // Load sparse index for this field (cached via UnifiedCache). const sparseIndex = await this.loadSparseIndex(field) + + // Path 2: No sparse index exists — fall back to entity storage. + // Covers VFS custom fields (modified, accessed) and user fields not + // yet indexed. resolveEntityField handles the shape contract. if (!sparseIndex) { - return undefined + const noun = await this.storage.getNoun(entityId) + return noun ? resolveEntityField(noun, field) : undefined } - // Search through chunks to find which value this entity has - // Typically 1-10 chunks per field, so this is fast + // Path 3: Search sparse index chunks for this entity's value. + // Typically 1-10 chunks per field, so this is fast. for (const chunkId of sparseIndex.getAllChunkIds()) { const chunk = await this.chunkManager.loadChunk(field, chunkId) if (!chunk) continue - // Check each value's roaring bitmap for our entity ID - // Roaring bitmap .has() is O(1) with SIMD optimization + // Check each value's roaring bitmap for our entity ID. + // Roaring bitmap .has() is O(1) with SIMD optimization. for (const [value, bitmap] of chunk.entries) { if (bitmap.has(intId)) { - // Found it! Denormalize the value (no bucketing for non-timestamps) return this.denormalizeValue(value, field) } } diff --git a/tests/integration/orderby-sort-bug.test.ts b/tests/integration/orderby-sort-bug.test.ts new file mode 100644 index 00000000..76128c28 --- /dev/null +++ b/tests/integration/orderby-sort-bug.test.ts @@ -0,0 +1,223 @@ +/** + * @module orderby-sort-bug + * @description Regression tests for the `find({ orderBy: ... })` sort bug. + * + * Bug: `getFieldValueForEntity` read `noun.metadata[field]` for timestamp + * fields, but `getNoun()` destructures standard fields to the top level. + * Every entity returned `undefined` for the sort key, stable sort preserved + * insertion order, and `order: 'desc'` behaved like `order: 'asc'`. + * + * Fix: centralized `resolveEntityField` helper + `BUCKETED_INDEX_FIELDS` + * set in coreTypes.ts, used by getFieldValueForEntity. + * + * Reported by Muse team 2026-04-09 (handoff action BR-ORDERBY-TS). + * + * NOTE: These tests cover FILTERED sort, which is the only supported path. + * Unfiltered `find({ orderBy })` is explicitly rejected until the dedicated + * time-ordered segment index ships (Track 2). + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { Brainy } from '../../src/brainy' +import { + resolveEntityField, + STANDARD_ENTITY_FIELDS, + type HNSWNounWithMetadata +} from '../../src/coreTypes' +import { NounType } from '../../src/types/graphTypes' + +describe('find({ orderBy }) sort bug regression', () => { + let brain: Brainy + + beforeEach(async () => { + brain = new Brainy({ storage: { type: 'memory' }, silent: true }) + await brain.init() + }) + + afterEach(async () => { + await brain.close() + }) + + /** + * Muse's real use case: filtered sort of chat sessions. + * Before the fix, this returned the OLDEST entity instead of the newest + * because getFieldValueForEntity was reading createdAt from the wrong + * location on the entity. + */ + it('orderBy createdAt desc with filter returns newest matching entity', async () => { + // Add 4 entities 20ms apart so createdAt values are distinct. + const id1 = await brain.add({ data: 'first', type: NounType.Concept }) + await new Promise((r) => setTimeout(r, 20)) + await brain.add({ data: 'second', type: NounType.Concept }) + await new Promise((r) => setTimeout(r, 20)) + await brain.add({ data: 'third', type: NounType.Concept }) + await new Promise((r) => setTimeout(r, 20)) + const id4 = await brain.add({ data: 'fourth', type: NounType.Concept }) + + const results = await brain.find({ + type: NounType.Concept, + orderBy: 'createdAt', + order: 'desc', + limit: 1 + }) + + expect(results).toHaveLength(1) + // Must be the last-inserted entity, not the first. + expect(results[0].id).toBe(id4) + expect(results[0].id).not.toBe(id1) + }) + + it('orderBy createdAt asc with filter returns oldest matching entity', async () => { + const id1 = await brain.add({ data: 'first', type: NounType.Concept }) + await new Promise((r) => setTimeout(r, 20)) + await brain.add({ data: 'second', type: NounType.Concept }) + await new Promise((r) => setTimeout(r, 20)) + await brain.add({ data: 'third', type: NounType.Concept }) + + const results = await brain.find({ + type: NounType.Concept, + orderBy: 'createdAt', + order: 'asc', + limit: 1 + }) + + expect(results).toHaveLength(1) + expect(results[0].id).toBe(id1) + }) + + it('orderBy createdAt desc with filter returns all matching entities in newest-first order', async () => { + const ids: string[] = [] + for (let i = 0; i < 5; i++) { + ids.push(await brain.add({ data: `item-${i}`, type: NounType.Concept })) + await new Promise((r) => setTimeout(r, 20)) + } + + const results = await brain.find({ + type: NounType.Concept, + orderBy: 'createdAt', + order: 'desc' + }) + + expect(results).toHaveLength(5) + expect(results.map((r) => r.id)).toEqual([...ids].reverse()) + }) + + it('orderBy updatedAt desc with filter returns most-recently-updated matching entity', async () => { + const id1 = await brain.add({ data: 'first', type: NounType.Concept }) + await new Promise((r) => setTimeout(r, 20)) + await brain.add({ data: 'second', type: NounType.Concept }) + await new Promise((r) => setTimeout(r, 20)) + await brain.add({ data: 'third', type: NounType.Concept }) + + // Touch id1 so it becomes the most-recently-updated. + await new Promise((r) => setTimeout(r, 20)) + await brain.update({ id: id1, data: 'first-updated' }) + + const results = await brain.find({ + type: NounType.Concept, + orderBy: 'updatedAt', + order: 'desc', + limit: 1 + }) + + expect(results).toHaveLength(1) + expect(results[0].id).toBe(id1) + }) + + /** + * Unfiltered sort is explicitly rejected with a clear error pointing + * callers at the right pattern. This guards against silent N-scans + * on production workloads. + */ + it('orderBy without any filter throws a helpful error', async () => { + await brain.add({ data: 'anything', type: NounType.Concept }) + + await expect( + brain.find({ orderBy: 'createdAt', order: 'desc', limit: 1 }) + ).rejects.toThrow(/requires a filter/) + }) +}) + +describe('resolveEntityField helper', () => { + const entity: HNSWNounWithMetadata = { + id: 'abc', + vector: [0.1, 0.2], + connections: new Map(), + level: 0, + type: NounType.Concept, + createdAt: 1700000000000, + updatedAt: 1700000060000, + confidence: 0.9, + weight: 1, + service: 'test', + data: { title: 'Hello' }, + metadata: { + customTag: 'green', + priority: 5, + modified: 1700000120000 // VFS custom field, lives in metadata + } + } + + it('reads standard fields from top level', () => { + expect(resolveEntityField(entity, 'createdAt')).toBe(1700000000000) + expect(resolveEntityField(entity, 'updatedAt')).toBe(1700000060000) + expect(resolveEntityField(entity, 'type')).toBe(NounType.Concept) + expect(resolveEntityField(entity, 'confidence')).toBe(0.9) + expect(resolveEntityField(entity, 'weight')).toBe(1) + expect(resolveEntityField(entity, 'service')).toBe('test') + expect(resolveEntityField(entity, 'id')).toBe('abc') + }) + + it('reads custom fields from metadata', () => { + expect(resolveEntityField(entity, 'customTag')).toBe('green') + expect(resolveEntityField(entity, 'priority')).toBe(5) + }) + + it('reads VFS custom fields (modified, accessed) from metadata', () => { + // VFS stores `modified` as a custom field, not top-level. + expect(resolveEntityField(entity, 'modified')).toBe(1700000120000) + }) + + it('returns undefined for unknown fields', () => { + expect(resolveEntityField(entity, 'nonexistent')).toBeUndefined() + }) + + it('returns undefined for custom fields when metadata is absent', () => { + const noMetadata: HNSWNounWithMetadata = { ...entity, metadata: undefined } + expect(resolveEntityField(noMetadata, 'customTag')).toBeUndefined() + }) + + it('does not look in metadata for standard fields', () => { + // If a standard field is absent top-level, resolver returns undefined + // rather than silently falling through to metadata. This prevents + // misuse from masking bugs. + const withShadowedField: HNSWNounWithMetadata = { + ...entity, + // @ts-expect-error intentionally clobbering for the test + createdAt: undefined, + metadata: { createdAt: 9999 } + } + expect(resolveEntityField(withShadowedField, 'createdAt')).toBeUndefined() + }) + + it('STANDARD_ENTITY_FIELDS covers every declared top-level field', () => { + // Guards against the resolver and the interface drifting out of sync. + const expected = [ + 'id', + 'vector', + 'connections', + 'level', + 'type', + 'confidence', + 'weight', + 'createdAt', + 'updatedAt', + 'service', + 'createdBy', + 'data' + ] + for (const field of expected) { + expect(STANDARD_ENTITY_FIELDS.has(field)).toBe(true) + } + }) +})