Fixing bug where not all archives were modeled.

modules/archive/__init__.py

@@ -10,7 +10,7 @@ from sqlalchemy.orm import Session
 from wrolpi.cmd import SINGLE_FILE_BIN, CHROMIUM
 from wrolpi.collections import Collection
 from wrolpi.common import logger, register_modeler, register_refresh_cleanup, limit_concurrent, split_lines_by_length, \
-    slow_logger, get_title_from_html
+    slow_logger, get_title_from_html, TRACE_LEVEL
 from wrolpi.db import get_db_session
 from wrolpi.downloader import Downloader, Download, DownloadResult
 from wrolpi.errors import UnrecoverableDownloadError
@@ -144,6 +144,7 @@ def model_archive(session: Session, file_group: FileGroup) -> Archive:
         raise InvalidArchive('FileGroup does not contain any html files')
 
     # All Archives have a Singlefile.
+    singlefile_path = None
     for file in html_paths:
         try:
             if is_singlefile_file(file):
@@ -153,7 +154,8 @@ def model_archive(session: Session, file_group: FileGroup) -> Archive:
             if PYTEST:
                 raise
             logger.debug(f'Cannot check is_singlefile_file of {repr(file)}', exc_info=e)
-    else:
+
+    if singlefile_path is None:
         logger.debug(f'No Archive singlefile found in {file_group}')
         raise InvalidArchive('FileGroup does not contain a singlefile')
 
@@ -260,7 +262,6 @@ async def archive_modeler():
 
             processed = 0
             for processed, (file_group, archive) in enumerate(results):
-
                 with slow_logger(1, f'Modeling archive took %(elapsed)s seconds: {file_group}',
                                  logger__=logger):
                     if archive:
@@ -269,7 +270,7 @@ async def archive_modeler():
                             archive.validate()
                             # Successfully validated, mark as indexed
                             file_group.indexed = True
-                        except Exception:
+                        except Exception as e:
                             logger.error(f'Unable to validate Archive {archive_id}')
                             # Don't mark as indexed - will retry later
                             if PYTEST:
@@ -291,12 +292,12 @@ async def archive_modeler():
 
             session.commit()
 
-            if processed < 20:
-                # Did not reach limit, do not query again.
+            if processed < 19:
+                # Did not reach limit (enumerate is 0-indexed, so 19 = 20 items), do not query again.
+                if logger.isEnabledFor(TRACE_LEVEL):
+                    logger.trace(f'archive_modeler: DONE (processed {processed + 1} files)')
                 break
 
-            logger.debug(f'Modeled {processed} Archives')
-
         # Sleep to catch cancel.
         await asyncio.sleep(0)
 

modules/archive/test/test_archive_modeler_loop.py

@@ -0,0 +1,107 @@
+"""Test for archive_modeler loop bug.
+
+The bug: archive_modeler uses enumerate() which is 0-indexed, but checks
+`if processed < 20` to determine if more batches exist. After processing
+exactly 20 items, processed=19, so 19 < 20 is True and the loop exits early.
+
+This means archive_modeler only ever processes ONE batch of 20 files,
+leaving all other archives unprocessed.
+"""
+import datetime
+import pathlib
+
+import pytest
+import pytz
+
+from modules.archive import archive_modeler
+from modules.archive.lib import archive_strftime
+from modules.archive.models import Archive
+from wrolpi.files.models import FileGroup
+
+
+def create_singlefile_content(url: str, title: str) -> str:
+    """Create minimal singlefile HTML content with embedded URL."""
+    return f'''<!DOCTYPE html><html><!--
+ Page saved with SingleFile
+ url: {url}
+ saved date: Mon May 16 2022 23:51:35 GMT+0000 (Coordinated Universal Time)
+--><head><meta charset="utf-8">
+<meta name="generator" content="SingleFile">
+<title>{title}</title>
+</head>
+<body><h1>{title}</h1><p>Content</p></body>
+</html>'''
+
+
+@pytest.mark.asyncio
+async def test_archive_modeler_processes_more_than_20_files(async_client, test_session, test_directory,
+                                                             make_files_structure):
+    """
+    Test that archive_modeler processes MORE than 20 files.
+
+    This test creates 25 singlefile archives and verifies that archive_modeler
+    processes all of them, not just the first batch of 20.
+
+    The bug: enumerate() is 0-indexed, so after processing 20 items,
+    processed=19. The check `if processed < 20` is True (19 < 20), causing
+    the loop to exit after just one batch.
+    """
+    domain = 'example.com'
+    archive_dir = test_directory / 'archives' / domain
+    archive_dir.mkdir(parents=True)
+
+    # Create 25 singlefile archives (more than one batch of 20)
+    num_archives = 25
+    files_structure = {}
+
+    for i in range(num_archives):
+        # Use different days to avoid hour overflow
+        timestamp = archive_strftime(
+            datetime.datetime(2000, 1, 1 + i, 0, 0, 0).astimezone(pytz.UTC)
+        )
+        title = f'Article {i}'
+        url = f'https://{domain}/article-{i}'
+
+        singlefile_content = create_singlefile_content(url, title)
+        html_path = str(archive_dir / f'{timestamp}_{title}.html')
+        files_structure[html_path] = singlefile_content
+
+    # Create all files
+    files = make_files_structure(files_structure)
+
+    # Create FileGroups for each file (simulating what refresh does)
+    file_groups = []
+    for file_path in files:
+        path = pathlib.Path(file_path)
+        if path.suffix == '.html':
+            fg = FileGroup.from_paths(test_session, path)
+            file_groups.append(fg)
+
+    test_session.commit()
+
+    # Verify we have the expected number of unindexed HTML FileGroups
+    unindexed_count = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.mimetype == 'text/html'
+    ).count()
+    assert unindexed_count == num_archives, f"Expected {num_archives} unindexed HTML files, got {unindexed_count}"
+
+    # Run the archive_modeler
+    await archive_modeler()
+
+    # Count how many Archives were created
+    archive_count = test_session.query(Archive).count()
+
+    # THE BUG: Only 20 archives created instead of 25
+    # After fixing, all 25 should be created
+    assert archive_count == num_archives, \
+        f"archive_modeler should process ALL {num_archives} files, but only processed {archive_count}. " \
+        f"This is the enumerate off-by-one bug!"
+
+    # Also verify all FileGroups are now indexed
+    still_unindexed = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.mimetype == 'text/html'
+    ).count()
+    assert still_unindexed == 0, \
+        f"All HTML FileGroups should be indexed, but {still_unindexed} remain unindexed"

modules/videos/test/test_video_modeler_loop.py

@@ -0,0 +1,71 @@
+"""Test for video_modeler loop to ensure it processes more than one batch.
+
+This test ensures the video_modeler can process more than VIDEO_PROCESSING_LIMIT (20)
+files. If the loop logic has an off-by-one error (like using enumerate() with wrong
+comparison), the modeler would only process one batch and exit early.
+"""
+import shutil
+
+import pytest
+
+from modules.videos import video_modeler, VIDEO_PROCESSING_LIMIT
+from modules.videos.models import Video
+from wrolpi.files.models import FileGroup
+from wrolpi.vars import PROJECT_DIR
+
+
+@pytest.mark.asyncio
+async def test_video_modeler_processes_more_than_batch_limit(async_client, test_session, test_directory):
+    """
+    Test that video_modeler processes MORE than VIDEO_PROCESSING_LIMIT (20) files.
+
+    This test creates 25 video files and verifies that video_modeler
+    processes all of them, not just the first batch of 20.
+
+    This catches off-by-one bugs in the loop logic (e.g., using enumerate()
+    which is 0-indexed but comparing against the limit incorrectly).
+    """
+    video_dir = test_directory / 'videos'
+    video_dir.mkdir(parents=True)
+
+    # Create more videos than the batch limit
+    num_videos = VIDEO_PROCESSING_LIMIT + 5  # 25 videos
+    video_paths = []
+
+    for i in range(num_videos):
+        video_path = video_dir / f'test_video_{i:03d}.mp4'
+        shutil.copy(PROJECT_DIR / 'test/big_buck_bunny_720p_1mb.mp4', video_path)
+        video_paths.append(video_path)
+
+    # Create FileGroups for each video file (simulating what refresh does)
+    for video_path in video_paths:
+        fg = FileGroup.from_paths(test_session, video_path)
+        assert fg.mimetype.startswith('video/')
+
+    test_session.commit()
+
+    # Verify we have the expected number of unindexed video FileGroups
+    unindexed_count = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.mimetype.like('video/%'),
+    ).count()
+    assert unindexed_count == num_videos, f"Expected {num_videos} unindexed video files, got {unindexed_count}"
+
+    # Run the video_modeler
+    await video_modeler()
+
+    # Count how many Videos were created
+    video_count = test_session.query(Video).count()
+
+    # All videos should be created
+    assert video_count == num_videos, \
+        f"video_modeler should process ALL {num_videos} files, but only processed {video_count}. " \
+        f"This may be an off-by-one bug in the loop logic!"
+
+    # Also verify all FileGroups are now indexed
+    still_unindexed = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.mimetype.like('video/%'),
+    ).count()
+    assert still_unindexed == 0, \
+        f"All video FileGroups should be indexed, but {still_unindexed} remain unindexed"

modules/zim/test/test_zim_modeler_loop.py

@@ -0,0 +1,75 @@
+"""Test for zim_modeler loop to ensure it processes more than one batch.
+
+This test ensures the zim_modeler can process more than its batch limit (10)
+files. If the loop logic has an off-by-one error (like using enumerate() with wrong
+comparison), the modeler would only process one batch and exit early.
+"""
+import shutil
+
+import pytest
+
+from modules.zim.lib import zim_modeler
+from modules.zim.models import Zim
+from wrolpi.files.models import FileGroup
+from wrolpi.vars import PROJECT_DIR
+
+# The zim_modeler uses a hardcoded limit of 10
+ZIM_PROCESSING_LIMIT = 10
+
+
+@pytest.mark.asyncio
+async def test_zim_modeler_processes_more_than_batch_limit(async_client, test_session, test_directory):
+    """
+    Test that zim_modeler processes MORE than the batch limit (10) files.
+
+    This test creates 15 zim files and verifies that zim_modeler
+    processes all of them, not just the first batch of 10.
+
+    This catches off-by-one bugs in the loop logic (e.g., using enumerate()
+    which is 0-indexed but comparing against the limit incorrectly).
+    """
+    zim_dir = test_directory / 'zims'
+    zim_dir.mkdir(parents=True)
+
+    # Create more zim files than the batch limit
+    num_zims = ZIM_PROCESSING_LIMIT + 5  # 15 zims
+    zim_paths = []
+
+    for i in range(num_zims):
+        zim_path = zim_dir / f'test_zim_{i:03d}.zim'
+        shutil.copy(PROJECT_DIR / 'test/zim.zim', zim_path)
+        zim_paths.append(zim_path)
+
+    # Create FileGroups for each zim file (simulating what refresh does)
+    for zim_path in zim_paths:
+        fg = FileGroup.from_paths(test_session, zim_path)
+        # Zim files don't have a standard mimetype, they're identified by suffix
+        assert fg.primary_path.suffix == '.zim'
+
+    test_session.commit()
+
+    # Verify we have the expected number of unindexed zim FileGroups
+    unindexed_count = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.primary_path.ilike('%.zim'),
+    ).count()
+    assert unindexed_count == num_zims, f"Expected {num_zims} unindexed zim files, got {unindexed_count}"
+
+    # Run the zim_modeler
+    await zim_modeler()
+
+    # Count how many Zims were created
+    zim_count = test_session.query(Zim).count()
+
+    # All zims should be created
+    assert zim_count == num_zims, \
+        f"zim_modeler should process ALL {num_zims} files, but only processed {zim_count}. " \
+        f"This may be an off-by-one bug in the loop logic!"
+
+    # Also verify all FileGroups are now indexed
+    still_unindexed = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.primary_path.ilike('%.zim'),
+    ).count()
+    assert still_unindexed == 0, \
+        f"All zim FileGroups should be indexed, but {still_unindexed} remain unindexed"

wrolpi/files/test/test_ebook_modeler_loop.py

@@ -0,0 +1,74 @@
+"""Test for ebook_modeler loop to ensure it processes more than one batch.
+
+This test ensures the ebook_modeler can process more than its batch limit (10)
+files. If the loop logic has an off-by-one error (like using enumerate() with wrong
+comparison), the modeler would only process one batch and exit early.
+"""
+import shutil
+
+import pytest
+
+from wrolpi.files.ebooks import ebook_modeler, EBook
+from wrolpi.files.models import FileGroup
+from wrolpi.vars import PROJECT_DIR
+
+# The ebook_modeler uses a hardcoded limit of 10
+EBOOK_PROCESSING_LIMIT = 10
+
+
+@pytest.mark.asyncio
+async def test_ebook_modeler_processes_more_than_batch_limit(async_client, test_session, test_directory):
+    """
+    Test that ebook_modeler processes MORE than the batch limit (10) files.
+
+    This test creates 15 ebook files and verifies that ebook_modeler
+    processes all of them, not just the first batch of 10.
+
+    This catches off-by-one bugs in the loop logic (e.g., using enumerate()
+    which is 0-indexed but comparing against the limit incorrectly).
+    """
+    ebook_dir = test_directory / 'ebooks'
+    ebook_dir.mkdir(parents=True)
+
+    # Create more ebooks than the batch limit
+    num_ebooks = EBOOK_PROCESSING_LIMIT + 5  # 15 ebooks
+    ebook_paths = []
+
+    for i in range(num_ebooks):
+        ebook_path = ebook_dir / f'test_ebook_{i:03d}.epub'
+        shutil.copy(PROJECT_DIR / 'test/ebook example.epub', ebook_path)
+        ebook_paths.append(ebook_path)
+
+    # Create FileGroups for each ebook file (simulating what refresh does)
+    for ebook_path in ebook_paths:
+        fg = FileGroup.from_paths(test_session, ebook_path)
+        # EPUB mimetype might be 'application/epub+zip' or 'application/epub'
+        assert fg.mimetype.startswith('application/epub')
+
+    test_session.commit()
+
+    # Verify we have the expected number of unindexed ebook FileGroups
+    unindexed_count = test_session.query(FileGroup).filter(
+        FileGroup.indexed == False,
+        FileGroup.mimetype == 'application/epub+zip',
+    ).count()
+    assert unindexed_count == num_ebooks, f"Expected {num_ebooks} unindexed ebook files, got {unindexed_count}"
+
+    # Run the ebook_modeler
+    await ebook_modeler()
+
+    # Count how many EBooks were created
+    ebook_count = test_session.query(EBook).count()
+
+    # All ebooks should be created
+    assert ebook_count == num_ebooks, \
+        f"ebook_modeler should process ALL {num_ebooks} files, but only processed {ebook_count}. " \
+        f"This may be an off-by-one bug in the loop logic!"
+
+    # Also verify all FileGroups are now indexed
+    still_unindexed = test_session.query(FileGroup).filter(
+        FileGroup.indexed == False,
+        FileGroup.mimetype == 'application/epub+zip',
+    ).count()
+    assert still_unindexed == 0, \
+        f"All ebook FileGroups should be indexed, but {still_unindexed} remain unindexed"

wrolpi/files/test/test_pdf_modeler_loop.py

@@ -0,0 +1,73 @@
+"""Test for pdf_modeler loop to ensure it processes more than one batch.
+
+This test ensures the pdf_modeler can process more than PDF_PROCESSING_LIMIT (10)
+files. If the loop logic has an off-by-one error (like using enumerate() with wrong
+comparison), the modeler would only process one batch and exit early.
+"""
+import shutil
+
+import pytest
+
+from wrolpi.files.pdfs import pdf_modeler, PDF_PROCESSING_LIMIT
+from wrolpi.files.models import FileGroup
+from wrolpi.vars import PROJECT_DIR
+
+
+@pytest.mark.asyncio
+async def test_pdf_modeler_processes_more_than_batch_limit(async_client, test_session, test_directory):
+    """
+    Test that pdf_modeler processes MORE than PDF_PROCESSING_LIMIT (10) files.
+
+    This test creates 15 PDF files and verifies that pdf_modeler
+    processes all of them, not just the first batch of 10.
+
+    This catches off-by-one bugs in the loop logic (e.g., using enumerate()
+    which is 0-indexed but comparing against the limit incorrectly).
+    """
+    pdf_dir = test_directory / 'pdfs'
+    pdf_dir.mkdir(parents=True)
+
+    # Create more PDFs than the batch limit
+    num_pdfs = PDF_PROCESSING_LIMIT + 5  # 15 PDFs
+    pdf_paths = []
+
+    for i in range(num_pdfs):
+        pdf_path = pdf_dir / f'test_pdf_{i:03d}.pdf'
+        shutil.copy(PROJECT_DIR / 'test/pdf example.pdf', pdf_path)
+        pdf_paths.append(pdf_path)
+
+    # Create FileGroups for each PDF file (simulating what refresh does)
+    for pdf_path in pdf_paths:
+        fg = FileGroup.from_paths(test_session, pdf_path)
+        assert fg.mimetype == 'application/pdf'
+
+    test_session.commit()
+
+    # Verify we have the expected number of unindexed PDF FileGroups
+    unindexed_count = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.mimetype == 'application/pdf',
+    ).count()
+    assert unindexed_count == num_pdfs, f"Expected {num_pdfs} unindexed PDF files, got {unindexed_count}"
+
+    # Run the pdf_modeler
+    await pdf_modeler()
+
+    # Count how many FileGroups are now indexed as PDFs
+    indexed_pdf_count = test_session.query(FileGroup).filter(
+        FileGroup.indexed == True,
+        FileGroup.model == 'pdf',
+    ).count()
+
+    # All PDFs should be indexed
+    assert indexed_pdf_count == num_pdfs, \
+        f"pdf_modeler should process ALL {num_pdfs} files, but only processed {indexed_pdf_count}. " \
+        f"This may be an off-by-one bug in the loop logic!"
+
+    # Also verify no FileGroups remain unindexed
+    still_unindexed = test_session.query(FileGroup).filter(
+        FileGroup.indexed != True,
+        FileGroup.mimetype == 'application/pdf',
+    ).count()
+    assert still_unindexed == 0, \
+        f"All PDF FileGroups should be indexed, but {still_unindexed} remain unindexed"