diff --git a/modules/archive/__init__.py b/modules/archive/__init__.py index 0868a79d..51cfb510 100644 --- a/modules/archive/__init__.py +++ b/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) diff --git a/modules/archive/test/test_archive_modeler_loop.py b/modules/archive/test/test_archive_modeler_loop.py new file mode 100644 index 00000000..99184b71 --- /dev/null +++ b/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'''
+ +Content
+''' + + +@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" diff --git a/modules/videos/test/test_video_modeler_loop.py b/modules/videos/test/test_video_modeler_loop.py new file mode 100644 index 00000000..6f3a0934 --- /dev/null +++ b/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" diff --git a/modules/zim/test/test_zim_modeler_loop.py b/modules/zim/test/test_zim_modeler_loop.py new file mode 100644 index 00000000..5a54de38 --- /dev/null +++ b/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" diff --git a/wrolpi/files/test/test_ebook_modeler_loop.py b/wrolpi/files/test/test_ebook_modeler_loop.py new file mode 100644 index 00000000..5e52d7c5 --- /dev/null +++ b/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" diff --git a/wrolpi/files/test/test_pdf_modeler_loop.py b/wrolpi/files/test/test_pdf_modeler_loop.py new file mode 100644 index 00000000..03c5c050 --- /dev/null +++ b/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"