From ae1eae78e7bdd5de14fcaf8de126809d7651225d Mon Sep 17 00:00:00 2001 From: jakeaturner Date: Tue, 29 Sep 2026 17:13:51 +0000 Subject: [PATCH] fix(kb): stop orphan sweep from purging an empty but present scan root Boot runs ensureDirectoryExists() on the zim root, so a separate volume that failed to mount leaves an empty mountpoint behind. The scan walks it without error and reports it as scanned. One upload in kb_uploads keeps the overall scan non-empty, so the next manual sync purged the vectors for every ZIM under that root. decideOrphans() now decides per scanned root and returns { orphans, withheld }. A root keeps its indexed sources when: - it holds no embeddable files (kiwix-library.xml does not count) - the purge would remove more than 50% of its indexed sources and at least 5 of them, which points to a wrong or stale mount rather than hand deletions scanAndSyncStorage() logs each withheld root and appends a note to the sync result message, so the user sees a likely unmounted drive instead of losing hours of embeddings. Replaces the unit test that asserted the old empty-root purge with regression coverage for #1378 and both guards. Closes #1378 --- admin/app/services/rag_service.ts | 63 +++++++---- admin/app/utils/kb_orphan_decision.ts | 108 ++++++++++++++++--- admin/tests/unit/kb_orphan_decision.spec.ts | 111 ++++++++++++++++---- 3 files changed, 222 insertions(+), 60 deletions(-) diff --git a/admin/app/services/rag_service.ts b/admin/app/services/rag_service.ts index c6e1be91..7995c41c 100644 --- a/admin/app/services/rag_service.ts +++ b/admin/app/services/rag_service.ts @@ -22,11 +22,11 @@ import { OllamaService } from './ollama_service.js' import { SERVICE_NAMES } from '../../constants/service_names.js' import { removeStopwords } from 'stopword' import { randomUUID } from 'node:crypto' -import { join, resolve, sep } from 'node:path' +import { join, relative, resolve, sep } from 'node:path' import KVStore from '#models/kv_store' import KbIngestState from '#models/kb_ingest_state' import { decideScanAction, type IngestPolicy } from '../utils/kb_ingest_decision.js' -import { decideOrphans, filterOrphanCandidates } from '../utils/kb_orphan_decision.js' +import { decideOrphans } from '../utils/kb_orphan_decision.js' import { decideContentReindex, type ReindexOutcome } from '../utils/content_reindex_decision.js' import KbRatioRegistry from '#models/kb_ratio_registry' import { decideWarnings } from '../utils/kb_warning_decision.js' @@ -2097,9 +2097,11 @@ export class RagService { * 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. + * cannot afford to confuse the two. "This root wasn't there" means we know + * nothing about it and must not touch it. A root that was there but empty + * is not much better evidence: boot creates the zim directory, so an + * unmounted volume shows up as an empty one (#1378). decideOrphans() + * therefore also refuses to purge under a walked root with no files. * * 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 @@ -2388,24 +2390,29 @@ export class RagService { // leftover from ZimService.delete() (which never touched Qdrant) or // from reconcileReplacedContentFile's qdrant_not_running no-op. Running // this in sync (rather than only in the delete path) also self-heals - // installs already in this state. decideOrphans no-ops when - // embeddableFiles came back empty, so a filesystem hiccup can't be - // misread as "every file was deleted." + // installs already in this state. // - // 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) + // Confined to `scannedRoots`, the roots the scan above actually walked, + // not the roots it meant to walk. Nomad's own bundled docs (README.md + + // docs/) live outside these roots, so they're left alone without being + // named here, and a root missing at scan time contributes nothing + // (#1050). Within each walked root, decideOrphans also withholds the + // purge when the root holds no embeddable files (an unmounted volume + // leaves an empty mountpoint behind, #1378) or when it would remove most + // of the root at once. Withheld roots are reported back to the operator + // rather than silently skipped. + const { orphans, withheld } = decideOrphans( + [...sourcesInQdrant], + embeddableFiles, + scannedRoots + ) + for (const w of withheld) { + logger.warn( + `[RAG] Withheld purge of ${w.count} indexed source(s) under ${w.root} (${w.reason}); the directory may be unmounted or pointing at the wrong location` + ) + } let orphansPurged = 0 - if (orphans && orphans.length > 0) { + if (orphans.length > 0) { logger.info( `[RAG] Found ${orphans.length} orphaned source(s) with no corresponding file on disk` ) @@ -2478,10 +2485,20 @@ export class RagService { `[RAG] Scan results (policy=${policy}): ${filesToEmbed.length} to embed, ${backfilled} backfilled, ${createdRows} new pending, ${createdPending} waiting on user, ${skipped} skipped` ) + const withheldNote = withheld + .map( + (w) => + `; left ${w.count} indexed source${w.count !== 1 ? 's' : ''} under ${relative(process.cwd(), w.root)} untouched because ${ + w.reason === 'empty_root' + ? 'that folder has no files (is the drive mounted?)' + : 'removing them would clear most of that folder (is it pointing at the right drive?)' + }` + ) + .join('') const orphanNote = - orphansPurged > 0 + (orphansPurged > 0 ? `; purged ${orphansPurged} orphaned source${orphansPurged !== 1 ? 's' : ''}` - : '' + : '') + withheldNote if (filesToEmbed.length === 0) { return { diff --git a/admin/app/utils/kb_orphan_decision.ts b/admin/app/utils/kb_orphan_decision.ts index 20655b9f..e03b5ed6 100644 --- a/admin/app/utils/kb_orphan_decision.ts +++ b/admin/app/utils/kb_orphan_decision.ts @@ -1,5 +1,43 @@ import { sep } from 'node:path' +/** + * Past this share of a root's indexed sources, a single sync refuses to purge + * that root at all. Losing most of a root at once is far likelier to mean the + * root points somewhere wrong (a different disk, a stale copy, a half-finished + * data-path move per #1050) than that the user deleted most of their library + * by hand. Real deletions through the UI purge their own vectors immediately + * (ZimService.delete, deleteFileBySource), so the sweep only ever mops up + * stragglers and has no business removing the bulk of a root. + */ +export const ORPHAN_PURGE_MAX_FRACTION = 0.5 + +/** + * The mass-removal guard only engages once this many sources would go. Below + * it, the fraction is too noisy to mean anything (1 of 2 is 50%) and the cost + * of a wrong purge is a few minutes of re-embedding, not hours. + */ +export const ORPHAN_PURGE_MIN_GUARDED = 5 + +export type WithheldOrphans = { + /** Scan root whose orphans were left in place. */ + root: string + /** How many indexed sources under it would have been purged. */ + count: number + /** + * `empty_root`: the root was walked but held no embeddable files. + * `mass_removal`: the purge would remove more than ORPHAN_PURGE_MAX_FRACTION + * of the root's indexed sources. + */ + reason: 'empty_root' | 'mass_removal' +} + +export type OrphanDecision = { + /** Sources safe to purge. */ + orphans: string[] + /** Roots whose missing sources were held back, for the operator to see. */ + withheld: WithheldOrphans[] +} + /** * Decision for the reverse sweep in `RagService.scanAndSyncStorage`. * @@ -12,26 +50,64 @@ import { sep } from 'node:path' * `reconcileReplacedContentFile`'s `qdrant_not_running` no-op therefore never * got reaped. * - * Guarded so a transient failure can't be misread as "every file was - * deleted": if the disk scan came back empty, we return `null` (do nothing) - * rather than treating every indexed source as an orphan. An empty - * `embeddableFiles` list is indistinguishable from a filesystem hiccup, and - * the blast radius of wrongly deleting a healthy knowledge base outweighs the - * cost of skipping a sweep for one cycle. + * Decided per scanned root, because the roots fail independently. Two guards + * apply to each, and either one withholds the whole root for this cycle: + * + * 1. A root with zero embeddable files keeps its sources (#1378). Walking a + * directory successfully does not prove it holds the content: boot runs + * ensureDirectoryExists() on the zim root, so a separate volume that + * failed to mount leaves a real, empty mountpoint behind (often holding a + * freshly regenerated kiwix-library.xml, which is why this counts + * embeddable files rather than directory entries). An empty root is + * indistinguishable from an unmounted one, and a single upload in + * kb_uploads is enough to make the overall scan non-empty. + * + * 2. A purge that would remove more than ORPHAN_PURGE_MAX_FRACTION of a + * root's indexed sources (and at least ORPHAN_PURGE_MIN_GUARDED of them) is + * withheld. This catches the root pointing at the wrong place while still + * holding a few files, which guard 1 can't see. + * + * Both guards trade a missed cleanup for safety: re-embedding a wrongly purged + * library takes hours to days, while a stale source costs nothing until the + * next sync or an explicit reset. */ export function decideOrphans( sourcesInQdrant: string[], - embeddableFiles: string[] -): string[] | null { - if (embeddableFiles.length === 0) return null - + embeddableFiles: string[], + scannedRoots: string[] +): OrphanDecision { const onDisk = new Set(embeddableFiles) - return sourcesInQdrant.filter((source) => !onDisk.has(source)) + const orphans: string[] = [] + const withheld: WithheldOrphans[] = [] + + for (const root of scannedRoots) { + const indexed = filterOrphanCandidates(sourcesInQdrant, [root]) + const missing = indexed.filter((source) => !onDisk.has(source)) + if (missing.length === 0) continue + + if (filterOrphanCandidates(embeddableFiles, [root]).length === 0) { + withheld.push({ root, count: missing.length, reason: 'empty_root' }) + continue + } + + if ( + missing.length >= ORPHAN_PURGE_MIN_GUARDED && + missing.length / indexed.length > ORPHAN_PURGE_MAX_FRACTION + ) { + withheld.push({ root, count: missing.length, reason: 'mass_removal' }) + continue + } + + orphans.push(...missing) + } + + return { orphans, withheld } } /** - * Narrows Qdrant sources down to the ones decideOrphans() can actually make an - * informed call about: sources under a root the disk scan genuinely walked. + * Narrows paths down to the ones under a root the disk scan genuinely walked. + * decideOrphans() applies it per root, both to Qdrant sources and to the files + * found on disk. * * `scannedRoots` is deliberately the roots that were *walked*, not the roots * that were *configured*. Two different failure modes collapse into that one @@ -45,9 +121,9 @@ export function decideOrphans( * 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. + * kb_uploads. A root we couldn't read tells us nothing about what belongs + * under it. (A root that exists but is empty is the same problem one step + * removed; decideOrphans' per-root guard handles that, see #1378.) * * Passing an empty `scannedRoots` therefore yields no candidates, which is the * correct reading of "we couldn't see any of the storage." diff --git a/admin/tests/unit/kb_orphan_decision.spec.ts b/admin/tests/unit/kb_orphan_decision.spec.ts index 2301c46a..bb379d9e 100644 --- a/admin/tests/unit/kb_orphan_decision.spec.ts +++ b/admin/tests/unit/kb_orphan_decision.spec.ts @@ -20,28 +20,39 @@ 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([], [zim('a.zim')]), []) + assert.deepEqual(decideOrphans([], [zim('a.zim')], SCAN_ROOTS), { orphans: [], withheld: [] }) }) test('every Qdrant source still has a file on disk → no orphans', () => { - assert.deepEqual(decideOrphans([zim('a.zim'), zim('b.zim')], [zim('a.zim'), zim('b.zim')]), []) + assert.deepEqual( + decideOrphans([zim('a.zim'), zim('b.zim')], [zim('a.zim'), zim('b.zim')], SCAN_ROOTS), + { orphans: [], withheld: [] } + ) }) test('a source with no matching file on disk is an orphan', () => { - assert.deepEqual(decideOrphans([zim('a.zim'), zim('gone.zim')], [zim('a.zim')]), [ - zim('gone.zim'), - ]) + assert.deepEqual(decideOrphans([zim('a.zim'), zim('gone.zim')], [zim('a.zim')], SCAN_ROOTS), { + orphans: [zim('gone.zim')], + withheld: [], + }) }) -test('every Qdrant source is orphaned when none remain on disk (but disk scan was non-empty)', () => { - assert.deepEqual(decideOrphans([zim('gone1.zim'), zim('gone2.zim')], [zim('unrelated.zim')]), [ - zim('gone1.zim'), - zim('gone2.zim'), - ]) +test('sources outside every scanned root are never orphans (e.g. bundled docs)', () => { + const readme = join('/data', 'README.md') + assert.deepEqual(decideOrphans([readme, zim('a.zim')], [zim('a.zim')], SCAN_ROOTS), { + orphans: [], + withheld: [], + }) }) -test('empty disk scan is treated as a transient failure, not "everything was deleted"', () => { - assert.equal(decideOrphans([zim('a.zim'), zim('b.zim')], []), null) +test('empty disk scan withholds every root instead of purging everything', () => { + assert.deepEqual(decideOrphans([upload('a.pdf'), zim('a.zim')], [], SCAN_ROOTS), { + orphans: [], + withheld: [ + { root: KB_UPLOADS_ROOT, count: 1, reason: 'empty_root' }, + { root: ZIM_ROOT, count: 1, reason: 'empty_root' }, + ], + }) }) test('filterOrphanCandidates keeps sources under the kb_uploads or zim scan roots', () => { @@ -80,7 +91,10 @@ test('a root that was not scanned contributes no candidates, even though the sca 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')]), []) + assert.deepEqual(decideOrphans(sourcesInQdrant, [upload('a.pdf')], [KB_UPLOADS_ROOT]), { + orphans: [], + withheld: [], + }) }) test('no roots scanned at all yields no candidates', () => { @@ -89,12 +103,67 @@ test('no roots scanned at all yields no candidates', () => { 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')]) +test('a walked root with no embeddable files keeps its sources (#1378)', () => { + // The unmounted-volume shape: boot's ensureDirectoryExists() recreates the + // zim mountpoint as an empty directory, so the scan walks it without error + // and reports it as scanned. kb_uploads keeps the overall scan non-empty. + // Treating "walked and empty" as "everything under it was deleted" would + // purge every ZIM in the index. + const sourcesInQdrant = [upload('a.pdf'), zim('wikipedia_en_all_maxi.zim'), zim('gutenberg.zim')] + assert.deepEqual(decideOrphans(sourcesInQdrant, [upload('a.pdf')], SCAN_ROOTS), { + orphans: [], + withheld: [{ root: ZIM_ROOT, count: 2, reason: 'empty_root' }], + }) +}) + +test('a root holding only non-embeddable files still counts as empty', () => { + // Kiwix regenerates kiwix-library.xml in the empty mountpoint, so the + // directory has entries. Only embeddable files are evidence the content is + // there, and _discoverKbFilesWithRoots() drops the XML before this point, so + // the zim root arrives with nothing under it. + const embeddable = [upload('a.pdf')] + assert.deepEqual(decideOrphans([zim('a.zim')], embeddable, SCAN_ROOTS).withheld, [ + { root: ZIM_ROOT, count: 1, reason: 'empty_root' }, + ]) +}) + +test('an empty kb_uploads root keeps its sources while zim is swept normally', () => { + const zims = ['a', 'b', 'c'].map((n) => zim(`${n}.zim`)) + assert.deepEqual( + decideOrphans([upload('a.pdf'), ...zims, zim('gone.zim')], zims, SCAN_ROOTS), + { + orphans: [zim('gone.zim')], + withheld: [{ root: KB_UPLOADS_ROOT, count: 1, reason: 'empty_root' }], + } + ) +}) + +test('removing most of a root in one sync is withheld as a likely wrong mount', () => { + // The root is present and non-empty, but it holds 2 of 10 indexed ZIMs: + // far likelier a different disk or stale copy than 8 hand-deletions. + const indexed = Array.from({ length: 10 }, (_, i) => zim(`z${i}.zim`)) + const onDisk = indexed.slice(0, 2) + assert.deepEqual(decideOrphans(indexed, onDisk, SCAN_ROOTS), { + orphans: [], + withheld: [{ root: ZIM_ROOT, count: 8, reason: 'mass_removal' }], + }) +}) + +test('removing up to half a root is still purged', () => { + const indexed = Array.from({ length: 10 }, (_, i) => zim(`z${i}.zim`)) + const onDisk = indexed.slice(0, 5) + assert.deepEqual(decideOrphans(indexed, onDisk, SCAN_ROOTS), { + orphans: indexed.slice(5), + withheld: [], + }) +}) + +test('the mass-removal guard does not engage below the minimum count', () => { + // 4 of 5 missing is 80%, but only 4 sources: too few for the ratio to mean + // anything, and cheap to re-embed if wrong. + const indexed = Array.from({ length: 5 }, (_, i) => zim(`z${i}.zim`)) + assert.deepEqual(decideOrphans(indexed, indexed.slice(0, 1), SCAN_ROOTS), { + orphans: indexed.slice(1), + withheld: [], + }) })