From 366db256f4159c91881d77d953c374111d39911e Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 6 Sep 2026 15:09:50 +0200 Subject: [PATCH] add_warning: store exceptions given as args as text, not the exception For a BackupWarning, the args of the warning include the caught BackupError exception. Storing that in the global warnings list kept the exception - and thus its traceback and the frames (with all their locals) of the code that failed - alive for the rest of the borg run, one per warning: for a failed file in borg extract, that is the extract_item frame including the chunk data that was being written. borg extract of 300 files of 2.5 MB whose writes all fail peaked at 876 MiB RSS instead of 409 MiB, and on a full disk every remaining file adds another chunk. Store the exception's message text instead, which is what formatting the warning message with the exception gives anyway. Add a regression test (the exception wrapped by a BackupWarning must be freed after print_warning_instance()) and a unit test for the get_ec() warnings logic. Co-Authored-By: Claude Fable 5.1 --- src/borg/helpers/__init__.py | 5 +++ .../testsuite/archiver/return_codes_test.py | 31 ++++++++++++++++++- src/borg/testsuite/helpers/__init__test.py | 23 +++++++++++++- 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/borg/helpers/__init__.py b/src/borg/helpers/__init__.py index b3d5216d51..daeab865b0 100644 --- a/src/borg/helpers/__init__.py +++ b/src/borg/helpers/__init__.py @@ -95,6 +95,11 @@ def add_warning(msg, *args, **kwargs): assert isinstance(warning_code, int) warning_type = kwargs.get("wt", "percent") assert warning_type in ("percent", "curly") + # Store exceptions given as args (e.g. the BackupError wrapped by a BackupWarning) as their message text: + # an exception references its traceback and thus the frames (with all their locals, e.g. the chunk data + # that was being written) of the code that failed - keeping that for every warning, for the whole borg + # run, would leak memory. The text is what formatting the message with the exception gives anyway. + args = tuple(str(arg) if isinstance(arg, BaseException) else arg for arg in args) _warnings_list.append(warning_info(warning_code, msg, args, warning_type)) diff --git a/src/borg/testsuite/archiver/return_codes_test.py b/src/borg/testsuite/archiver/return_codes_test.py index 6e22ff4937..21ca05b341 100644 --- a/src/borg/testsuite/archiver/return_codes_test.py +++ b/src/borg/testsuite/archiver/return_codes_test.py @@ -1,7 +1,13 @@ +import errno +import gc import os +import weakref +from ...archiver import Archiver from ...constants import * # NOQA -from ...helpers import IncludePatternNeverMatchedWarning +from ...helpers import IncludePatternNeverMatchedWarning, BackupError, BackupOSError, BackupWarning +from ...helpers import get_reset_ec, init_ec_warnings, modern_ec +from ...logger import setup_logging from ...repository import Repository from . import cmd, changedir, generate_archiver_tests # NOQA @@ -33,3 +39,26 @@ def test_exit_codes(archivers, request, monkeypatch): cmd(archiver, "create", "archive", "input", fork=True, exit_code=EXIT_ERROR) monkeypatch.setenv("BORG_EXIT_CODES", "modern") cmd(archiver, "create", "archive", "input", fork=True, exit_code=Repository.DoesNotExist.exit_mcode) + + +def test_print_warning_instance_does_not_retain_exception(): + """The warnings bookkeeping for the final exit code must not keep the wrapped exception alive. + + An exception references its traceback and thus the frames (with all their locals, e.g. the chunk + data that was being written) of the code that failed - keeping that per warning would leak memory. + """ + setup_logging() + init_ec_warnings() + archiver = Archiver() + try: + raise BackupOSError("write", OSError(errno.ENOSPC, "No space left on device")) + except BackupError as exc: + exc_ref = weakref.ref(exc) + archiver.print_warning_instance(BackupWarning("input/file", exc)) + # "except ... as exc" unbinds exc when its block ends, so the exception is only still alive if the + # warnings bookkeeping references it. CPython frees it right away (refcounting), PyPy only when + # its GC runs, so collect explicitly before looking at the weakref. + gc.collect() + assert exc_ref() is None + # the warning was recorded for the exit code, though. + assert get_reset_ec() == (BackupOSError.exit_mcode if modern_ec else EXIT_WARNING) diff --git a/src/borg/testsuite/helpers/__init__test.py b/src/borg/testsuite/helpers/__init__test.py index 4c01179293..24fe10fd11 100644 --- a/src/borg/testsuite/helpers/__init__test.py +++ b/src/borg/testsuite/helpers/__init__test.py @@ -1,7 +1,7 @@ import pytest from ...constants import * # NOQA -from ...helpers import classify_ec, max_ec +from ...helpers import classify_ec, max_ec, add_warning, get_ec, get_reset_ec, init_ec_warnings @pytest.mark.parametrize( @@ -62,3 +62,24 @@ def test_ec_invalid(): ) def test_max_ec(ec1, ec2, ec_max): assert max_ec(ec1, ec2) == ec_max + + +def test_get_ec_warnings(): + init_ec_warnings() + # no warnings: the exit code set via set_ec (or given to get_ec) is returned as is. + assert get_ec() == EXIT_SUCCESS + # only warnings of one kind: the exit code is that specific warning code. + add_warning("some warning", wc=EXIT_WARNING_BASE + 1) + add_warning("some warning", wc=EXIT_WARNING_BASE + 1) + assert get_ec() == EXIT_WARNING_BASE + 1 + # warnings of different kinds: the exit code is the generic warning code. + add_warning("another warning", wc=EXIT_WARNING_BASE + 2) + assert get_ec() == EXIT_WARNING + # an error is more severe than any warning. + assert get_ec(EXIT_ERROR) == EXIT_ERROR + # get_reset_ec returns the exit code and then starts over (no exit code, no warnings). + assert get_reset_ec() == EXIT_ERROR + assert get_ec() == EXIT_SUCCESS + add_warning("some warning", wc=EXIT_WARNING_BASE + 1) + assert get_reset_ec() == EXIT_WARNING_BASE + 1 + assert get_ec() == EXIT_SUCCESS