From d710ab07907e39b9e8049d639b9a31898743cae1 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Mon, 6 Jan 2025 15:27:40 -0800 Subject: [PATCH] backups: Properly cleanup after downloading an archive Closes: #2354. - Process spawned for raw IO should be collected using wait(). Closing the specially created pipe early causes an exception in the child process when it tries to write to stderr the JSON result of executed method. Tests: - Monitor the output of 'journalctl -f'. Download and archive. Without the patch, an error is printed as described in the bug. With the but no such error is printed. Downloaded file is the same in both cases. - Writing a log message in cleanup_func shows that the process has been waited for and the FDs have been closed. Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/modules/backups/repository.py | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/plinth/modules/backups/repository.py b/plinth/modules/backups/repository.py index c642e2f04..f2e8680f8 100644 --- a/plinth/modules/backups/repository.py +++ b/plinth/modules/backups/repository.py @@ -196,6 +196,9 @@ class BaseBorgRepository(abc.ABC): """Override to call read() instead of readline().""" chunk = self.read(io.DEFAULT_BUFFER_SIZE) if not chunk: + if getattr(self, 'cleanup_func'): + self.cleanup_func() + raise StopIteration return chunk @@ -204,12 +207,27 @@ class BaseBorgRepository(abc.ABC): self._get_archive_path(archive_name), self._get_encryption_passpharse(), _raw_output=True) - os.close(read_fd) # Don't use the pipe for communication, just stdout + # Write the method request with args to the process proc.stdin.write(input_) proc.stdin.close() - proc.stderr.close() # writing to stderr in child will cause SIGPIPE - return BufferedReader(proc.stdout) + def _cleanup_func(): + """After the process has been read from, cleanup the process.""" + try: + if proc.stdout: + proc.stdout.close() + + if proc.stderr: + proc.stderr.close() + + proc.wait(30) + os.close(read_fd) + except Exception: + logger.exception('Closing process failed after download') + + reader = BufferedReader(proc.stdout) + reader.cleanup_func = _cleanup_func + return reader def _get_archive_path(self, archive_name): """Return full borg path for an archive."""