github-mirror/project-nomad
1
0
Fork 0
mirror of https://github.com/Crosstalk-Solutions/project-nomad.git synced 2026-10-17 05:14:19 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity Graph

fix(kb): confine the orphan sweep to roots the scan actually walked

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Chris Sherwood 2026-08-29 14:59:25 -07:00 committed by Jake Turner
parent d4f0f078c5
commit 52d12a222e
3 changed files with 141 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
admin/app/services/rag_service.ts
View File

@ -2082,8 +2082,35 @@ export class RagService {
* type" and retry on every sync.
*/
private async _discoverKbFiles(): Promise<string[]> {
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) {

46
admin/app/utils/kb_orphan_decision.ts
View File

@ -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 `<root>-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)))
}

94
admin/tests/unit/kb_orphan_decision.spec.ts
View File

@ -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), [])
// '<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([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')])
})