From ab31e208d1937d6d04dfd29aff82c3e662df979b Mon Sep 17 00:00:00 2001 From: rcorvus <5025458+rcorvus@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:56:14 -0700 Subject: [PATCH] fix(rag): don't abandon a ZIM after one sparse batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batch continuation gated on articles that produced chunks rather than articles the extractor consumed. Articles whose text is empty after HTML cleaning — redirect stubs, category listings, media and PDF wrappers — return no chunks and so no documentIds, which made a normal sparse batch look like the end of the archive. Ingestion stopped after the first batch and the rest of the file was silently skipped. Medicine LibreTexts embedded 16 chunks out of 23,171 articles: its first 50 entries are navigation pages, only 10 of which had text, so 10 >= 50 was false and no continuation was ever dispatched. Measured against the same install, WikiMed reached 28% of its articles, Libre Pathology 31%, MedlinePlus 84%, and CDC Travelers' Health 2.7%. The same value was also returned as `articlesProcessed`, which EmbedFileJob uses to advance the next batch offset. Advancing by the content-bearing count re-read the overlap on every sparse batch, and a batch yielding no text at all would have advanced by zero and re-run the same window indefinitely. Both now use the extractor's consumed-article count, which it already computed and logged but discarded. The end-of-archive signal is the iterator running dry (articlesProcessed < batchSize); archive.articleCount cannot serve as a bound because iterByPath() legitimately yields more article entries than that figure for some archives. The log line now reports both counts (content-bearing/consumed) so a sparse batch is visible without re-deriving it. --- admin/app/services/rag_service.ts | 37 ++++++++----- admin/app/services/zim_extraction_service.ts | 4 +- admin/app/utils/zim_batch_decision.ts | 41 +++++++++++++++ admin/tests/unit/zim_batch_decision.spec.ts | 55 ++++++++++++++++++++ 4 files changed, 123 insertions(+), 14 deletions(-) create mode 100644 admin/app/utils/zim_batch_decision.ts create mode 100644 admin/tests/unit/zim_batch_decision.spec.ts diff --git a/admin/app/services/rag_service.ts b/admin/app/services/rag_service.ts index 858b58a..0e90849 100644 --- a/admin/app/services/rag_service.ts +++ b/admin/app/services/rag_service.ts @@ -37,6 +37,7 @@ const EXCLUDE_EVAL_FILTER = { import type { KbIngestStateValue } from '../../types/kb_ingest_state.js' import { ZIMExtractionService } from './zim_extraction_service.js' import { ZIM_BATCH_SIZE } from '../../constants/zim_extraction.js' +import { hasMoreArticleBatches } from '../utils/zim_batch_decision.js' import { EMBEDDING_MODEL_NAME } from '../../constants/ollama.js' import { ProcessAndEmbedFileResponse, ProcessZIMFileResponse, RAGResult, RerankedRAGResult } from '../../types/rag.js' @@ -556,10 +557,14 @@ export class RagService { `[RAG] Extracting ZIM content (batch: offset=${startOffset}, size=${ZIM_BATCH_SIZE})` ) - const { chunks: zimChunks, totalArticles } = await zimExtractionService.extractZIMContent( - filepath, - { startOffset, batchSize: ZIM_BATCH_SIZE } - ) + const { + chunks: zimChunks, + totalArticles, + articlesProcessed, + } = await zimExtractionService.extractZIMContent(filepath, { + startOffset, + batchSize: ZIM_BATCH_SIZE, + }) logger.info( `[RAG] Extracted ${zimChunks.length} chunks from ZIM file with enhanced metadata (file totalArticles=${totalArticles})` @@ -610,15 +615,19 @@ export class RagService { } } - // Count unique articles processed in this batch. hasMoreBatches gates on the article - // count — zimChunks.length counts section-level chunks (multiple per article under the - // 'structured' strategy), so comparing it to ZIM_BATCH_SIZE (an article limit) caps - // processing at the first batch for any real archive. - const articlesInBatch = new Set(zimChunks.map((c) => c.documentId)).size - const hasMoreBatches = articlesInBatch >= ZIM_BATCH_SIZE + // Gate the continuation on articles the extractor CONSUMED, not on articles that + // produced chunks. Articles whose text is empty after cleaning (redirect stubs, + // category pages, media/PDF wrappers) contribute no documentIds, so a chunk-derived + // count reads a normal sparse batch as end-of-archive and silently abandons the rest + // of the file. See `zim_batch_decision.ts` for the full rationale. + const articlesWithContent = new Set(zimChunks.map((c) => c.documentId)).size + const hasMoreBatches = hasMoreArticleBatches({ + articlesProcessed, + batchSize: ZIM_BATCH_SIZE, + }) logger.info( - `[RAG] Successfully embedded ${totalChunks} total chunks from ${articlesInBatch} articles (hasMore: ${hasMoreBatches})` + `[RAG] Successfully embedded ${totalChunks} total chunks from ${articlesWithContent}/${articlesProcessed} articles (hasMore: ${hasMoreBatches})` ) // Only delete the file when: @@ -640,7 +649,11 @@ export class RagService { : 'ZIM file processed and embedded successfully with enhanced metadata.', chunks: totalChunks, hasMoreBatches, - articlesProcessed: articlesInBatch, + // MUST be the consumed count: EmbedFileJob advances the next batch offset by this + // value. Reporting the content-bearing count instead re-reads the overlap on every + // sparse batch, and a batch that yields no text at all would advance the offset by + // zero and re-run the same window forever. + articlesProcessed, totalArticles, } } diff --git a/admin/app/services/zim_extraction_service.ts b/admin/app/services/zim_extraction_service.ts index 072098c..ec30788 100644 --- a/admin/app/services/zim_extraction_service.ts +++ b/admin/app/services/zim_extraction_service.ts @@ -44,7 +44,7 @@ export class ZIMExtractionService { async extractZIMContent( filePath: string, opts: ExtractZIMContentOptions = {} - ): Promise<{ chunks: ZIMContentChunk[]; totalArticles: number }> { + ): Promise<{ chunks: ZIMContentChunk[]; totalArticles: number; articlesProcessed: number }> { try { logger.info(`[ZIMExtractionService]: Processing ZIM file at path: ${filePath}`) @@ -165,7 +165,7 @@ export class ZIMExtractionService { textPreview: c.text.substring(0, 100) }))) logger.debug("Total structured sections extracted:", toReturn.length) - return { chunks: toReturn, totalArticles: archive.articleCount } + return { chunks: toReturn, totalArticles: archive.articleCount, articlesProcessed } } catch (error) { logger.error('Error processing ZIM file:', error) throw error diff --git a/admin/app/utils/zim_batch_decision.ts b/admin/app/utils/zim_batch_decision.ts new file mode 100644 index 0000000..c6ad5da --- /dev/null +++ b/admin/app/utils/zim_batch_decision.ts @@ -0,0 +1,41 @@ +/** + * Decision for whether a batched ZIM ingestion should dispatch a continuation. + * + * This is the pure, I/O-free core of the batch loop in + * `RagService.processZIMFile` (mirrors `decideScanAction` in + * `kb_ingest_decision.ts`). + * + * The signal MUST be the number of articles the extractor *consumed*, never the + * number that produced chunks. An article yields zero chunks whenever its text + * is empty after HTML cleaning — redirect stubs, category listings, image/video + * wrappers, PDF containers. Those are perfectly normal ZIM entries, but they are + * invisible to any count derived from the returned chunks. + * + * Gating on chunk-derived counts silently truncates ingestion the first time a + * window of `batchSize` articles happens to contain mostly empty ones: the + * continuation is never dispatched and every remaining article in the archive is + * skipped. That is not a rare edge case — it is the norm for scraped-site and + * media-heavy archives, where the opening entries are usually navigation pages. + * + * `articlesProcessed < batchSize` means the extractor's iterator ran dry, which + * is the only reliable end-of-archive signal available: `archive.articleCount` + * cannot serve as an upper bound, because `iterByPath()` legitimately yields + * more article entries than that figure for some archives. + * + * A full final batch costs one extra dispatch that extracts nothing and stops. + * That is deliberate: over-running by a single empty batch is cheap, while + * stopping one batch early loses the remainder of the archive. + */ +export interface ZimBatchProgress { + /** Articles the extractor consumed in this batch (not articles that produced chunks). */ + articlesProcessed: number + /** The article ceiling requested for this batch. */ + batchSize: number +} + +export function hasMoreArticleBatches({ + articlesProcessed, + batchSize, +}: ZimBatchProgress): boolean { + return articlesProcessed >= batchSize +} diff --git a/admin/tests/unit/zim_batch_decision.spec.ts b/admin/tests/unit/zim_batch_decision.spec.ts new file mode 100644 index 0000000..66c0959 --- /dev/null +++ b/admin/tests/unit/zim_batch_decision.spec.ts @@ -0,0 +1,55 @@ +import * as assert from 'node:assert/strict' +import { test } from 'node:test' + +import { hasMoreArticleBatches } from '../../app/utils/zim_batch_decision.js' + +const BATCH = 50 + +test('continues while the extractor keeps filling the batch', () => { + assert.equal(hasMoreArticleBatches({ articlesProcessed: BATCH, batchSize: BATCH }), true) +}) + +test('stops once the extractor runs out of article entries', () => { + assert.equal(hasMoreArticleBatches({ articlesProcessed: 12, batchSize: BATCH }), false) +}) + +test('stops on an archive smaller than one batch', () => { + assert.equal(hasMoreArticleBatches({ articlesProcessed: 1, batchSize: BATCH }), false) +}) + +test('stops on an exhausted iterator that yielded nothing', () => { + assert.equal(hasMoreArticleBatches({ articlesProcessed: 0, batchSize: BATCH }), false) +}) + +/** + * Regression: Medicine LibreTexts (23,171 articles) embedded only 16 chunks. + * + * Its first 50 article entries are navigation and category pages; only 10 of + * them produced any text. The previous gate counted articles that produced + * chunks, so it evaluated `10 >= 50` -> false, never dispatched a continuation, + * and abandoned the remaining 23,121 articles. A full batch was consumed, so the + * ingestion must continue regardless of how little text came out of it. + */ +test('continues through a full batch that produced almost no text', () => { + const articlesWithContent = 10 + assert.equal( + hasMoreArticleBatches({ articlesProcessed: BATCH, batchSize: BATCH }), + true, + 'a full batch must continue even when most of its articles are empty' + ) + assert.equal( + articlesWithContent >= BATCH, + false, + 'the old chunk-derived gate stopped here — this is the bug being fixed' + ) +}) + +/** + * Regression: a media archive (Canadian Prepper, 92 articles / 187 media files) + * yields zero text chunks, but its articles still have to be walked to the end + * rather than abandoned after the first batch. + */ +test('walks a zero-text archive to the end instead of stopping at batch one', () => { + assert.equal(hasMoreArticleBatches({ articlesProcessed: BATCH, batchSize: BATCH }), true) + assert.equal(hasMoreArticleBatches({ articlesProcessed: 42, batchSize: BATCH }), false) +})