From 52d12a222e997f628cad935c5211366e2b338be3 Mon Sep 17 00:00:00 2001 From: Chris Sherwood Date: Sat, 29 Aug 2026 14:59:25 -0700 Subject: [PATCH] fix(kb): confine the orphan sweep to roots the scan actually walked decideOrphans guards on the disk scan coming back empty, but the failure mode it needs to defend against is per-root. _discoverKbFiles() walks kb_uploads and zim in a loop and skips a missing root rather than failing, because a fresh install legitimately has no kb_uploads yet. So if the zim root is relocated, renamed, or not yet mounted (#1050), the scan still returns a non-empty list from kb_uploads. The empty-scan guard never fires, filterOrphanCandidates has no way to tell a root that scanned clean from one that was never there, and every ZIM in the index is classified as an orphan and purged in a single batch. On a Wikipedia-scale knowledge base that is millions of vectors and a multi-hour re-embed. It is reachable from the UI: scanAndSyncStorage is POST /rag/sync, so the button pressed when the knowledge base looks wrong is the button that empties it. _discoverKbFilesWithRoots() now reports which roots were walked, and filterOrphanCandidates allowlists against that list instead of the configured roots. A root that wasn't there contributes no candidates; a root that scanned clean and held no files still yields its orphans, so the legitimate purge path is unchanged. The empty-scan guard becomes the degenerate case of the same rule rather than a separate one. Also builds the unit-test paths with join() instead of POSIX literals. Written as '/data/...' the filterOrphanCandidates cases pass on Linux CI and fail on Windows, where sep is backslash and no source ever matches a prefix, which is exactly where they would be run while changing this. Co-Authored-By: Claude Opus 5 (1M context) --- admin/app/services/rag_service.ts | 53 ++++++++++-- admin/app/utils/kb_orphan_decision.ts | 46 ++++++---- admin/tests/unit/kb_orphan_decision.spec.ts | 94 +++++++++++++++------ 3 files changed, 141 insertions(+), 52 deletions(-) diff --git a/admin/app/services/rag_service.ts b/admin/app/services/rag_service.ts index 38c08e15..5ad4ebc7 100644 --- a/admin/app/services/rag_service.ts +++ b/admin/app/services/rag_service.ts @@ -2082,8 +2082,35 @@ export class RagService { * type" and retry on every sync. */ private async _discoverKbFiles(): Promise { + return (await this._discoverKbFilesWithRoots()).files + } + + /** + * As _discoverKbFiles(), but also reports which roots were actually walked. + * + * A root that isn't there is skipped rather than fatal, because a fresh + * install legitimately has no kb_uploads until the first upload. That makes + * an absent root indistinguishable from a present-but-empty one by looking + * at the file list alone — and the orphan sweep in scanAndSyncStorage() + * cannot afford to confuse the two. "This root scanned clean and had no + * files" means everything indexed under it is an orphan; "this root wasn't + * there" means we know nothing about it and must not touch it. + * + * That distinction is the whole reason this variant exists. If the zim root + * is missing, renamed, or not yet mounted (see #1050 — relocating the data + * path is easy to get subtly wrong), the scan still returns a non-empty + * list from kb_uploads. A guard that only checks "did the scan come back + * empty" therefore passes, and every ZIM in the index gets purged in one + * batch. Reporting the roots lets the sweep confine itself to ground it + * actually stood on. + */ + private async _discoverKbFilesWithRoots(): Promise<{ + files: string[] + scannedRoots: string[] + }> { const { kbUploadsPath: KB_UPLOADS_PATH, zimPath: ZIM_PATH } = this._kbScanRoots() const filesInStorage: string[] = [] + const scannedRoots: string[] = [] for (const [label, dirPath] of [ [RagService.UPLOADS_STORAGE_PATH, KB_UPLOADS_PATH] as const, @@ -2094,6 +2121,7 @@ export class RagService { contents.forEach((entry) => { if (entry.type === 'file') filesInStorage.push(entry.key) }) + scannedRoots.push(dirPath) logger.debug(`[RAG] Found ${contents.length} files in ${label}`) } catch (error) { if (error.code === 'ENOENT') { @@ -2104,7 +2132,10 @@ export class RagService { } } - return filesInStorage.filter((f) => determineFileType(f) !== 'unknown') + return { + files: filesInStorage.filter((f) => determineFileType(f) !== 'unknown'), + scannedRoots, + } } /** @@ -2308,7 +2339,7 @@ export class RagService { logger.error('[RAG] Error during Nomad docs discovery in sync process:', error) }) - const filesInStorage = await this._discoverKbFiles() + const { files: filesInStorage, scannedRoots } = await this._discoverKbFilesWithRoots() logger.info(`[RAG] Found ${filesInStorage.length} embeddable files in storage`) await this._ensureCollection( @@ -2356,13 +2387,17 @@ export class RagService { // embeddableFiles came back empty, so a filesystem hiccup can't be // misread as "every file was deleted." // - // Allowlisted (via filterOrphanCandidates) to sources under the same - // two roots _discoverKbFiles() just scanned to build embeddableFiles — - // the only roots this sweep can make an informed orphan/not-orphan - // call about. Nomad's own bundled docs (README.md + docs/) are - // embedded by discoverNomadDocs() above from outside those roots, so - // this naturally leaves them alone without needing to name them here. - const orphanCandidates = filterOrphanCandidates([...sourcesInQdrant], this._kbScanRoots()) + // Allowlisted (via filterOrphanCandidates) to `scannedRoots` — the roots + // the scan above actually walked, not the roots it meant to walk. Two + // separate things are excluded by that one rule. Nomad's own bundled + // docs (README.md + docs/) are embedded by discoverNomadDocs() from + // outside these roots, so they're left alone without being named here. + // And a root that wasn't present at scan time contributes no candidates + // at all, because a missing root is skipped rather than fatal: without + // this, a relocated or unmounted zim directory (#1050) would leave the + // scan non-empty via kb_uploads, sail past decideOrphans' empty-scan + // guard, and purge every ZIM in the index in a single batch. + const orphanCandidates = filterOrphanCandidates([...sourcesInQdrant], scannedRoots) const orphans = decideOrphans(orphanCandidates, embeddableFiles) let orphansPurged = 0 if (orphans && orphans.length > 0) { diff --git a/admin/app/utils/kb_orphan_decision.ts b/admin/app/utils/kb_orphan_decision.ts index 222a7286..20655b9f 100644 --- a/admin/app/utils/kb_orphan_decision.ts +++ b/admin/app/utils/kb_orphan_decision.ts @@ -19,7 +19,10 @@ import { sep } from 'node:path' * the blast radius of wrongly deleting a healthy knowledge base outweighs the * cost of skipping a sweep for one cycle. */ -export function decideOrphans(sourcesInQdrant: string[], embeddableFiles: string[]): string[] | null { +export function decideOrphans( + sourcesInQdrant: string[], + embeddableFiles: string[] +): string[] | null { if (embeddableFiles.length === 0) return null const onDisk = new Set(embeddableFiles) @@ -27,22 +30,35 @@ export function decideOrphans(sourcesInQdrant: string[], embeddableFiles: string } /** - * Narrows Qdrant sources down to the ones decideOrphans() can actually make - * an informed call about: sources under the same roots `_discoverKbFiles()` - * scanned to build `embeddableFiles` (kb_uploads, zim). Everything else — - * Nomad's own bundled docs (README.md + docs/), or any future embedding - * source root — is left alone here rather than denylisted by name, so a new - * source root added outside kb_uploads/zim doesn't get treated as an orphan - * and purged the first time it appears (see issue #1170's docs-collision - * near-miss for exactly that failure mode). + * Narrows Qdrant sources down to the ones decideOrphans() can actually make an + * informed call about: sources under a root the disk scan genuinely walked. + * + * `scannedRoots` is deliberately the roots that were *walked*, not the roots + * that were *configured*. Two different failure modes collapse into that one + * rule: + * + * 1. A source root outside the scan entirely — Nomad's own bundled docs + * (README.md + docs/), or any root added in future. Allowlisting rather + * than denylisting those by name means a new root doesn't get treated as + * orphaned the first time it appears (see #1170's docs-collision near-miss). + * + * 2. A configured root that wasn't there at scan time. `_discoverKbFiles()` + * skips a missing root rather than failing, so a relocated or not-yet- + * mounted zim directory (#1050) still leaves the scan non-empty via + * kb_uploads. decideOrphans' empty-scan guard doesn't fire, and without + * this filter every ZIM in the index would be purged in one batch. A root + * we couldn't read tells us nothing about what belongs under it. + * + * Passing an empty `scannedRoots` therefore yields no candidates, which is the + * correct reading of "we couldn't see any of the storage." */ export function filterOrphanCandidates( sourcesInQdrant: string[], - scanRoots: { kbUploadsPath: string; zimPath: string } + scannedRoots: string[] ): string[] { - const kbUploadsPrefix = scanRoots.kbUploadsPath + sep - const zimPrefix = scanRoots.zimPath + sep - return sourcesInQdrant.filter( - (source) => source.startsWith(kbUploadsPrefix) || source.startsWith(zimPrefix) - ) + // The trailing separator is what makes this a subpath test rather than a + // naive string prefix match, so `-backup/x.zim` doesn't qualify. + const prefixes = scannedRoots.map((root) => root + sep) + if (prefixes.length === 0) return [] + return sourcesInQdrant.filter((source) => prefixes.some((prefix) => source.startsWith(prefix))) } diff --git a/admin/tests/unit/kb_orphan_decision.spec.ts b/admin/tests/unit/kb_orphan_decision.spec.ts index f934a52c..2301c46a 100644 --- a/admin/tests/unit/kb_orphan_decision.spec.ts +++ b/admin/tests/unit/kb_orphan_decision.spec.ts @@ -1,62 +1,100 @@ import * as assert from 'node:assert/strict' import { test } from 'node:test' +import { join } from 'node:path' import { decideOrphans, filterOrphanCandidates } from '../../app/utils/kb_orphan_decision.js' -const SCAN_ROOTS = { kbUploadsPath: '/data/storage/kb_uploads', zimPath: '/data/storage/zim' } +// Paths are built with join() rather than written as POSIX literals so these +// cases exercise the same separator filterOrphanCandidates does. Written as +// '/data/...' string literals they silently pass on Linux CI and fail on a +// Windows dev box, which makes the suite useless exactly where someone is +// most likely to be running it while changing this file. +const STORAGE = join('/data', 'storage') +const KB_UPLOADS_ROOT = join(STORAGE, 'kb_uploads') +const ZIM_ROOT = join(STORAGE, 'zim') + +/** Both roots present and walked — the healthy case. */ +const SCAN_ROOTS = [KB_UPLOADS_ROOT, ZIM_ROOT] + +const upload = (name: string) => join(KB_UPLOADS_ROOT, name) +const zim = (name: string) => join(ZIM_ROOT, name) test('no sources in Qdrant → no orphans', () => { - assert.deepEqual(decideOrphans([], ['/storage/zim/a.zim']), []) + assert.deepEqual(decideOrphans([], [zim('a.zim')]), []) }) test('every Qdrant source still has a file on disk → no orphans', () => { - assert.deepEqual( - decideOrphans(['/storage/zim/a.zim', '/storage/zim/b.zim'], ['/storage/zim/a.zim', '/storage/zim/b.zim']), - [] - ) + assert.deepEqual(decideOrphans([zim('a.zim'), zim('b.zim')], [zim('a.zim'), zim('b.zim')]), []) }) test('a source with no matching file on disk is an orphan', () => { - assert.deepEqual( - decideOrphans(['/storage/zim/a.zim', '/storage/zim/gone.zim'], ['/storage/zim/a.zim']), - ['/storage/zim/gone.zim'] - ) + assert.deepEqual(decideOrphans([zim('a.zim'), zim('gone.zim')], [zim('a.zim')]), [ + zim('gone.zim'), + ]) }) test('every Qdrant source is orphaned when none remain on disk (but disk scan was non-empty)', () => { - assert.deepEqual( - decideOrphans(['/storage/zim/gone1.zim', '/storage/zim/gone2.zim'], ['/storage/zim/unrelated.zim']), - ['/storage/zim/gone1.zim', '/storage/zim/gone2.zim'] - ) + assert.deepEqual(decideOrphans([zim('gone1.zim'), zim('gone2.zim')], [zim('unrelated.zim')]), [ + zim('gone1.zim'), + zim('gone2.zim'), + ]) }) test('empty disk scan is treated as a transient failure, not "everything was deleted"', () => { - assert.equal(decideOrphans(['/storage/zim/a.zim', '/storage/zim/b.zim'], []), null) + assert.equal(decideOrphans([zim('a.zim'), zim('b.zim')], []), null) }) test('filterOrphanCandidates keeps sources under the kb_uploads or zim scan roots', () => { - assert.deepEqual( - filterOrphanCandidates( - ['/data/storage/kb_uploads/a.pdf', '/data/storage/zim/b.zim'], - SCAN_ROOTS - ), - ['/data/storage/kb_uploads/a.pdf', '/data/storage/zim/b.zim'] - ) + assert.deepEqual(filterOrphanCandidates([upload('a.pdf'), zim('b.zim')], SCAN_ROOTS), [ + upload('a.pdf'), + zim('b.zim'), + ]) }) test('filterOrphanCandidates excludes sources outside the scanned roots (e.g. bundled docs)', () => { assert.deepEqual( filterOrphanCandidates( - ['/data/README.md', '/data/docs/guide.md', '/data/storage/zim/b.zim'], + [join('/data', 'README.md'), join('/data', 'docs', 'guide.md'), zim('b.zim')], SCAN_ROOTS ), - ['/data/storage/zim/b.zim'] + [zim('b.zim')] ) }) test('filterOrphanCandidates does not match a sibling directory that merely shares a root as a string prefix', () => { - // '/data/storage/zim-backup/...' must not pass just because it starts with - // the same characters as zimPath — the trailing separator is what makes - // this a real subpath check rather than a naive string prefix match. - assert.deepEqual(filterOrphanCandidates(['/data/storage/zim-backup/x.zim'], SCAN_ROOTS), []) + // '/zim-backup/...' must not pass just because it starts with the + // same characters as zimPath — the trailing separator is what makes this a + // real subpath check rather than a naive string prefix match. + assert.deepEqual(filterOrphanCandidates([join(STORAGE, 'zim-backup', 'x.zim')], SCAN_ROOTS), []) +}) + +test('a root that was not scanned contributes no candidates, even though the scan found files', () => { + // The #1050 shape: the zim root is relocated or not yet mounted, so + // _discoverKbFiles() skips it (ENOENT is not fatal) and reports only + // kb_uploads as scanned. The file list is still non-empty from kb_uploads, + // so decideOrphans' empty-scan guard does NOT fire. Without confining + // candidates to roots actually walked, every indexed ZIM would be + // classified as an orphan and purged in one batch. + const sourcesInQdrant = [upload('a.pdf'), zim('wikipedia_en_all_maxi.zim'), zim('gutenberg.zim')] + const candidates = filterOrphanCandidates(sourcesInQdrant, [KB_UPLOADS_ROOT]) + assert.deepEqual(candidates, [upload('a.pdf')]) + + // End to end: the ZIMs survive despite having no backing file in the scan. + assert.deepEqual(decideOrphans(candidates, [upload('a.pdf')]), []) +}) + +test('no roots scanned at all yields no candidates', () => { + // "We could not read any of the storage" must never read as "nothing is on + // disk, so purge everything." + assert.deepEqual(filterOrphanCandidates([zim('a.zim')], []), []) +}) + +test('a scanned-but-empty root still yields orphans for what it contains', () => { + // The counterpart to the case above, and the reason the distinction matters: + // zim scanned clean and legitimately holds no files, so its indexed sources + // really are orphaned and should be purged. A missing root and an empty one + // must not behave the same way. + const candidates = filterOrphanCandidates([zim('gone.zim')], SCAN_ROOTS) + assert.deepEqual(candidates, [zim('gone.zim')]) + assert.deepEqual(decideOrphans(candidates, [upload('a.pdf')]), [zim('gone.zim')]) })