Get underlying error when conversion fails

[apyrgio]

When we get an early EOF from the converter process, we should
immediately get the exit code of that process, to find out the actual
underlying error. Currently, the exception we raise masks the underlying
error.

Raise a ConverterProcException, that in turns makes our error handling
code read the exit code of the spawned process, and converts it to a
helpful error message.

Fixes #714

[deeplow]

I agree with doing away with the InterruptedConversionException and removing the proc from ConverterProcException as it did feel odd when I made it store a proc.

However, it feels a bit weird to me that Document would be concerned with any implementation details. It seems like we're overloading the usage of Document. Unless there is some other argument that I am not considering, I would advocate for keeping any proc-related logic within the isolation_process submodule.

So here are the changes I propose, done on top of your PR. Let me know if you'd agree.

         try:
+            conversion_proc = self.start_doc_to_pixels_proc()
             with tempfile.TemporaryDirectory() as t:
                 Path(f"{t}/pixels").mkdir()
-                self.doc_to_pixels(document, t)
+                self.doc_to_pixels(document, t, conversion_proc)
                 # TODO: validate convert to pixels output
                 self.pixels_to_pdf(document, t, ocr_lang)
             document.mark_as_safe()
             if document.archive_after_conversion:
                 document.archive()
-        except errors.ConverterProcException as e:
-            exception = self.get_proc_exception(document.conversion_proc)
+        except errors.ConverterProcException:
+            exception = self.get_proc_exception(conversion_proc)

and also

@@ -91,11 +93,10 @@ class IsolationProvider(ABC):
             self.print_progress(document, True, str(e), 0)
             document.mark_as_failed()
 
-    def doc_to_pixels(self, document: Document, tempdir: str) -> None:
+    def doc_to_pixels(self, document: Document, tempdir: str, p: subprocess.Popen) -> subprocess.Popen:
         percentage = 0.0
         with open(document.input_filename, "rb") as f:
-            p = self.start_doc_to_pixels_proc()
-            document.conversion_proc = p
+            # document.conversion_proc = p
             try:
                 assert p.stdin is not None
                 p.stdin.write(f.read())
@@ -150,6 +151,7 @@ class IsolationProvider(ABC):
             log.info(
                 f"Conversion output (doc to pixels)\n{self.sanitize_conversion_str(untrusted_log)}"
             )
+        return p

[apyrgio]

I have updated the PR to use an approach like you suggested. Because the change is significant, I have force-pushed the changes, instead of having FIXUP commits.

[deeplow]

I have updated the PR to use an approach like you suggested. Because the change is significant, I have force-pushed the changes, instead of having FIXUP commits.

Great! Thanks for implementing it! I have only left one comment and then it's ready to merge.

[deeplow]

Ready to merge once the lints have been fixed.

[apyrgio]

I've tested this branch on Qubes, and added some extra commits which fix some Qubes-related issues I discovered, plus:

1. An improvement for the Git commit message lint
2. A fix in our lints, due to a previous bad merge.

[deeplow]

LGTM. I took some extra time to approve because this was failing on my Qubes, but it turns out the issue also impacted main. So I am now looking into that now.

Feel free to merge (after squashing the fixups).