Changing archive download process to output html file, rather than capturing stdout.

modules/archive/__init__.py

@@ -9,7 +9,7 @@ from selenium import webdriver
 from sqlalchemy import not_
 from sqlalchemy.orm import Session
 
-from wrolpi.cmd import SINGLE_FILE_BIN, CHROMIUM, READABILITY_BIN
+from wrolpi.cmd import READABILITY_BIN, SINGLE_FILE_BIN, CHROMIUM
 from wrolpi.common import logger, register_modeler, register_refresh_cleanup, limit_concurrent, split_lines_by_length, \
     slow_logger, html_screenshot, get_title_from_html
 from wrolpi.db import optional_session, get_db_session
@@ -71,33 +71,41 @@ class ArchiveDownloader(Downloader, ABC):
 
     async def do_singlefile(self, download: Download) -> Tuple[bytes, dict]:
         """Create a Singlefile from the archive's URL."""
-        cmd = ('/usr/bin/nice', '-n15',  # Nice above map importing, but very low priority.
-               str(SINGLE_FILE_BIN),
-               download.url,
-               '--browser-executable-path', CHROMIUM,
-               '--browser-args', '["--no-sandbox"]',
-               '--dump-content',
-               '--user-agent', USER_AGENT,
-               )
-        return_code, logs, stdout = await self.process_runner(
-            download.id,
-            download.url,
-            cmd,
-            pathlib.Path('/home/wrolpi'),
-        )
-        if return_code != 0:
-            raise RuntimeError(f'Archive singlefile exited with {return_code}')
+        with tempfile.NamedTemporaryFile(suffix='.html', delete=False) as output:
+            output = pathlib.Path(output.name)
+            cmd = ('/usr/bin/nice', '-n15',  # Nice above map importing, but very low priority.
+                   str(SINGLE_FILE_BIN),
+                   '--browser-executable-path', CHROMIUM,
+                   '--browser-args', '["--no-sandbox"]',
+                   '--user-agent', USER_AGENT,
+                   download.url,
+                   output,
+                   )
+            return_code, logs, stdout = await self.process_runner(
+                download.id,
+                download.url,
+                cmd,
+                pathlib.Path('/home/wrolpi'),
+            )
 
-        if not stdout or SINGLEFILE_HEADER.encode() not in stdout[:1000]:
-            if logs and (stderr := logs.get('stderr')):
-                e = ChildProcessError(stderr.decode()[:1000])
-                raise RuntimeError(f'Singlefile created was invalid: {download.url}') from e
-            if stdout:
-                e = ChildProcessError(stdout.decode()[:1000])
-                raise RuntimeError(f'Singlefile created was invalid: {download.url}') from e
-            raise RuntimeError(f'Singlefile created was invalid: {download.url}')
+            stdout = stdout.decode()
+            stderr = logs['stderr'].decode()
+            log_output = stderr or stdout or 'No stderr or stdout!'
 
-        return stdout, logs
+            if return_code != 0:
+                e = ChildProcessError(log_output[:1000])
+                raise RuntimeError(f'Archive singlefile exited with {return_code}') from e
+
+            content = output.read_bytes() if output.is_file() else ''
+            if not content:
+                e = ChildProcessError(log_output[:1000])
+                raise RuntimeError(f'Singlefile created was empty: {download.url}') from e
+
+            if SINGLEFILE_HEADER.encode() not in content:
+                e = ChildProcessError(log_output[:1000])
+                raise RuntimeError(f'Singlefile created was invalid: {download.url}') from e
+
+        return content, logs
 
     async def do_readability(self, download: Download, html: bytes) -> dict:
         """Extract the readability dict from the provided HTML."""

wrolpi/common.py

@@ -440,6 +440,7 @@ class ConfigFile:
         @param overwrite: Will overwrite the config file even if it was not imported successfully.
         """
         from wrolpi.api_utils import api_app
+        from wrolpi.events import Events
 
         file = file or self.get_file()
         if self.file_name not in file.name:
@@ -449,6 +450,7 @@ class ConfigFile:
 
         if file.exists() and overwrite is False:
             if not self.successful_import:
+                Events.send_config_save_failed(f'Failed to save config: {rel_path}')
                 raise RuntimeError(f'Refusing to save config because it was never successfully imported! {rel_path}')
             version = self.read_config_file(file).get('version')
             if version and version > self.version:
@@ -489,7 +491,6 @@ class ConfigFile:
                 message = f'Failed to save config: {rel_path}'
                 logger_.critical(message, exc_info=e)
                 if send_events:
-                    from wrolpi.events import Events
                     Events.send_config_save_failed(message)
                 raise e
 

wrolpi/downloader.py

@@ -296,7 +296,7 @@ class Downloader:
         Global timeout takes precedence over the timeout argument, unless it is 0.  (Smaller global timeout wins)
         """
         str_cmd = " ".join([str(i) for i in cmd])
-        logger.debug(f'{self} launching download process with args: {str_cmd}')
+        logger.info(f'{self} launching download process with args: {str_cmd}')
         start = now()
         proc = await asyncio.create_subprocess_exec(*cmd,
                                                     stdout=asyncio.subprocess.PIPE,
@@ -310,7 +310,7 @@ class Downloader:
         timeout = WROLPI_CONFIG.download_timeout or timeout or self.timeout
         logger.debug(f'{self} launched download process {pid=} {timeout=} for {url}')
 
-        stdout, stderr = None, None
+        stdout, stderr = b'', b''
         try:
             while True:
                 task = asyncio.Task(proc.communicate())
@@ -346,8 +346,10 @@ class Downloader:
         finally:
             # Output all logs from the process.
             # TODO is there a way to stream this output while the process is running?
-            logger.debug(f'Download exited with {proc.returncode}')
-            logs = {'stdout': stdout or '', 'stderr': stderr or ''}
+            stdout_len = len(stdout)
+            stderr_len = len(stderr)
+            logger.debug(f'Download exited with {proc.returncode} and stdout={stdout_len} stderr={stderr_len}')
+            logs = {'stdout': stdout, 'stderr': stderr}
 
         return proc.returncode, logs, stdout
 

wrolpi/test/test_downloader.py

@@ -1,5 +1,7 @@
 import asyncio
 import json
+import pathlib
+import tempfile
 from abc import ABC
 from datetime import datetime
 from http import HTTPStatus
@@ -297,6 +299,50 @@ async def test_process_runner_timeout(async_client, test_session, test_directory
     assert 2 < elapsed.total_seconds() < 4
 
 
+GOOD_SCRIPT = '''#! /usr/bin/env bash
+echo "standard output"
+echo "standard error" >&2
+'''
+
+BAD_SCRIPT = '''#! /usr/bin/env bash
+echo "new standard output"
+echo "new standard error" >&2
+exit 123
+'''
+
+
+@pytest.mark.asyncio
+async def test_process_runner_output(async_client, test_directory, test_downloader):
+    with tempfile.TemporaryDirectory() as directory:
+        directory = pathlib.Path(directory)
+        script = directory / 'echo_script.sh'
+        script.write_text(GOOD_SCRIPT)
+        cmd = ('/bin/bash', script)
+        return_code, logs, stdout = await test_downloader.process_runner(
+            1,
+            'https://example.com',
+            cmd,
+            directory,
+            timeout=1
+        )
+        assert return_code == 0
+        assert logs['stdout'] == b'standard output\n'
+        assert logs['stderr'] == b'standard error\n'
+
+        script.write_text(BAD_SCRIPT)
+        cmd = ('/bin/bash', script)
+        return_code, logs, stdout = await test_downloader.process_runner(
+            1,
+            'https://example.com',
+            cmd,
+            directory,
+            timeout=1
+        )
+        assert return_code == 123
+        assert logs['stdout'] == b'new standard output\n'
+        assert logs['stderr'] == b'new standard error\n'
+
+
 @pytest.mark.asyncio
 async def test_crud_download(async_client, test_session, test_download_manager, test_downloader):
     """Test the ways that Downloads can be created."""