Update log collection logic in close()

src/ffpuppet/core.py

@@ -535,8 +535,7 @@ def close(self, force_close: bool = False) -> None:
             # this can raise but shouldn't, we want to know if a timeout happens
             self._proc.wait(timeout=30)
 
-        # set reason code
-        exit_code: Optional[int] = None
+        # check state of browser processes and set the close reason
         if any(self._crashreports(skip_benign=True)):
             r_code = Reason.ALERT
             # Wait a moment for processes to exit automatically.
@@ -568,12 +567,11 @@ def close(self, force_close: bool = False) -> None:
             # have a crash report... this should be revisited when time allows
             # https://bugzil.la/1370520
             # Ignore -9 to avoid false positives due to system OOM killer
-            exit_code = self._proc.poll()
+            exit_code: Optional[int] = self._proc.poll()
+            assert exit_code is not None
             r_code = Reason.ALERT
             LOG.warning(
-                "Browser exit code: %r (%X), no crash reports found",
-                exit_code,
-                exit_code,
+                "No crash reports found, exit code: %d (%X)", exit_code, exit_code
             )
         else:
             r_code = Reason.EXITED
@@ -582,48 +580,38 @@ def close(self, force_close: bool = False) -> None:
         self._terminate()
 
         # collect crash reports and logs
-        if not force_close:
-            if self._logs.closed:  # pragma: no cover
-                # This should not happen while everything is working as expected.
-                # This is here to prevent additional unexpected issues.
-                # Since this should never happen in normal operation this assert
-                # will help verify that.
-                # If '_proc' is not None this is the first call to close()
-                # in this situation the PuppetLogger should still be available.
-                assert self._proc is None, "PuppetLogger is closed!"
-            else:
-                LOG.debug("reviewing %d check(s)", len(self._checks))
-                for check in self._checks:
-                    if check.message is not None:
-                        r_code = Reason.WORKER
-                        check.dump_log(
-                            dst_fp=self._logs.add_log(f"ffp_worker_{check.name}")
-                        )
-                # collect logs (excluding minidumps)
-                for log_path in self._crashreports(skip_md=True, skip_benign=False):
-                    self._logs.add_log(log_path.name, log_path.open("rb"))
-                assert self.profile is not None
-                assert self.profile.path is not None
-                assert self._bin_path is not None
-                # check for minidumps and process them if possible
-                md_path = self.profile.path / "minidumps"
-                if any(md_path.glob("*.dmp")):
-                    # check for local build symbols
-                    sym_path = self._bin_path.parent / "crashreporter-symbols"
-                    if not sym_path.is_dir():
-                        # use packaged symbols
-                        sym_path = self._bin_path / "symbols"
-                    process_minidumps(
-                        md_path,
-                        sym_path,
-                        self._logs.add_log,
-                        working_path=self._working_path,
+        if not force_close and not self._logs.closed:
+            LOG.debug("reviewing %d check(s)", len(self._checks))
+            for check in self._checks:
+                if check.message is not None:
+                    r_code = Reason.WORKER
+                    check.dump_log(
+                        dst_fp=self._logs.add_log(f"ffp_worker_{check.name}")
                     )
-                stderr_fp = self._logs.get_fp("stderr")
-                if stderr_fp:
-                    if exit_code is not None:
-                        stderr_fp.write(f"[ffpuppet] Exit code: {exit_code}\n".encode())
-                    stderr_fp.write(f"[ffpuppet] Reason code: {r_code.name}\n".encode())
+            # collect logs (excluding minidumps)
+            for log_path in self._crashreports(skip_md=True, skip_benign=False):
+                self._logs.add_log(log_path.name, log_path.open("rb"))
+            assert self.profile is not None
+            assert self.profile.path is not None
+            assert self._bin_path is not None
+            # check for minidumps and process them if possible
+            md_path = self.profile.path / "minidumps"
+            if any(md_path.glob("*.dmp")):
+                # check for local build symbols
+                sym_path = self._bin_path.parent / "crashreporter-symbols"
+                if not sym_path.is_dir():
+                    # use packaged symbols
+                    sym_path = self._bin_path / "symbols"
+                process_minidumps(
+                    md_path,
+                    sym_path,
+                    self._logs.add_log,
+                    working_path=self._working_path,
+                )
+            stderr_fp = self._logs.get_fp("stderr")
+            if stderr_fp:
+                stderr_fp.write(f"[ffpuppet] Exit code: {self._proc.poll()}\n".encode())
+                stderr_fp.write(f"[ffpuppet] Reason code: {r_code.name}\n".encode())
 
         # reset remaining to closed state
         try:

src/ffpuppet/test_ffpuppet.py

@@ -998,7 +998,7 @@ def get_processes(self):
     with StubbedProc() as ffp:
         ffp.launch()
         proc = ffp._proc
-        ffp._proc.poll.side_effect = (None, 0, 0)
+        ffp._proc.poll.side_effect = (None, 0, 0, 0)
         ffp.close()
         assert ffp._proc is None
         assert ffp._logs.closed