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) +})