From 05deffab1f5f4c97f150dd7d1af283f4b7f38659 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Fri, 30 Oct 2026 03:07:51 +0100 Subject: [PATCH 1/2] create: do not archive an atime we caused ourselves, fixes #6194 borg has to open() a fs item before it can fstat() it. On platforms without O_NOATIME support (or when we are not allowed to use it), that open() may already have updated the atime - and then we would archive that atime instead of the one the item had before borg touched it. stat_update_check() now returns a stat result look-alike that has the atime from the stat we did before opening the item, if the atime changed between the two stat calls. If it did not change (e.g. O_NOATIME worked), the fd-based stat result is returned as before. (cherry picked from commit 51cc27e79558f0e74192e1a9513bf741bce59a4c) Co-Authored-By: Claude Opus 5.5 --- src/borg/archive.py | 28 ++++++++++++++++ src/borg/testsuite/archive.py | 58 ++++++++++++++++++++++++++++++++-- src/borg/testsuite/archiver.py | 45 ++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 2 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 6312f80e40..48ccaf1243 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -229,6 +229,28 @@ def backup_io_iter(iterator): yield item +class StatOrigAtime: + """ + An os.stat_result look-alike: all attributes come from *st*, only the + access time comes from *st_orig*. + + Rationale: borg needs to open() a fs item before it can fstat() it. If the + platform does not support O_NOATIME (or we are not allowed to use it), that + open() might have already updated the atime - we do not want to archive such + an atime, but the one the item had before borg touched it. + """ + + __slots__ = ('_st', 'st_atime', 'st_atime_ns') + + def __init__(self, st, st_orig): + self._st = st + self.st_atime = st_orig.st_atime + self.st_atime_ns = st_orig.st_atime_ns + + def __getattr__(self, name): + return getattr(self._st, name) + + def stat_update_check(st_old, st_curr): """ This checks for race conditions between the first filename-based stat() @@ -251,6 +273,12 @@ def stat_update_check(st_old, st_curr): if st_old.st_ino != st_curr.st_ino: # in this case, the hardlinks-related code in create_helper has the wrong inode - abort! raise BackupRaceConditionError('file inode changed (race condition), skipping file') + if st_old.st_atime_ns != st_curr.st_atime_ns: + # the atime was updated in between the 2 stat calls - most likely by us, because we + # had to open the item and O_NOATIME was not available / not usable, see #6194. + # in the (rare) case that somebody else accessed the item at just that moment, we + # lose that atime update, but that is much less of an issue than archiving our own. + return StatOrigAtime(st_curr, st_old) # looks ok, we are still dealing with the same thing - return current stat: return st_curr diff --git a/src/borg/testsuite/archive.py b/src/borg/testsuite/archive.py index 8b2463b870..a98dc2518b 100644 --- a/src/borg/testsuite/archive.py +++ b/src/borg/testsuite/archive.py @@ -7,10 +7,11 @@ import pytest -from . import BaseTestCase +from . import BaseTestCase, is_utime_fully_supported from ..crypto.key import PlaintextKey from ..archive import Archive, CacheChunkBuffer, RobustUnpacker, valid_msgpacked_dict, ITEM_KEYS, Statistics -from ..archive import BackupOSError, backup_io, backup_io_iter, get_item_uid_gid, is_automount +from ..archive import BackupOSError, BackupRaceConditionError, backup_io, backup_io_iter, get_item_uid_gid +from ..archive import is_automount, stat_update_check from ..helpers import Manifest from ..helpers import msgpack from ..item import Item, ArchiveItem @@ -306,6 +307,59 @@ def __next__(self): assert False, 'StopIteration handled incorrectly' +def _stat_with_atime(path, atime_ns, mtime_ns=234567890000000000): + os.utime(path, ns=(atime_ns, mtime_ns)) + return os.stat(path) + + +@pytest.mark.skipif(not is_utime_fully_supported(), reason='cannot properly setup and execute test without utime') +def test_stat_update_check_atime_updated(tmpdir): + path = str(tmpdir.join('file')) + with open(path, 'wb') as f: + f.write(b'12345') + st_old = _stat_with_atime(path, 123456789000000000) + st_curr = _stat_with_atime(path, 987654321000000000) + st = stat_update_check(st_old, st_curr) + # the atime is the one from before we (usually: by opening the file) touched it, see #6194: + assert st.st_atime_ns == st_old.st_atime_ns + assert st.st_atime == st_old.st_atime + # everything else comes from the current stat: + assert st.st_mode == st_curr.st_mode + assert st.st_ino == st_curr.st_ino + assert st.st_size == st_curr.st_size + assert st.st_mtime_ns == st_curr.st_mtime_ns + # optional attributes must be present (or absent) just like on a real stat result: + assert hasattr(st, 'st_birthtime') == hasattr(st_curr, 'st_birthtime') + with pytest.raises(AttributeError): + st.st_does_not_exist + + +@pytest.mark.skipif(not is_utime_fully_supported(), reason='cannot properly setup and execute test without utime') +def test_stat_update_check_atime_unchanged(tmpdir): + path = str(tmpdir.join('file')) + with open(path, 'wb') as f: + f.write(b'12345') + st_old = _stat_with_atime(path, 123456789000000000) + st_curr = os.stat(path) + assert st_old.st_atime_ns == st_curr.st_atime_ns + # nothing to fix up, so we get the current stat result as is: + assert stat_update_check(st_old, st_curr) is st_curr + + +def test_stat_update_check_race_conditions(tmpdir): + file_path = str(tmpdir.join('file')) + with open(file_path, 'wb'): + pass + other_path = str(tmpdir.join('other_file')) + with open(other_path, 'wb'): + pass + st_file, st_other, st_dir = os.stat(file_path), os.stat(other_path), os.stat(str(tmpdir)) + with pytest.raises(BackupRaceConditionError): # file type changed + stat_update_check(st_file, st_dir) + with pytest.raises(BackupRaceConditionError): # inode changed + stat_update_check(st_file, st_other) + + @pytest.mark.parametrize("mounted", [False, True, None]) def test_is_automount(tmp_path, monkeypatch, mounted): import borg.archive as archive_module diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index ea40c1f7b6..ea543766d5 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -31,6 +31,7 @@ import borg import borg.helpers.errors from .. import xattr, helpers, platform +from .. import archive as archive_module from ..archive import Archive, ChunkBuffer from ..archiver import Archiver, parse_storage_quota, PURE_PYTHON_MSGPACK_WARNING from ..cache import Cache, LocalCache @@ -761,6 +762,46 @@ def has_noatime(some_file): # it touched the input file's atime while backing it up assert same_ts_ns(sto.st_atime_ns, atime * 1e9) + @pytest.mark.skipif(not is_utime_fully_supported(), reason='cannot properly setup and execute test without utime') + def test_atime_open_updates_atime(self): + # simulate a platform where already the open() updates the atime (no O_NOATIME support, + # e.g. cygwin) - borg must archive the atime the item had before borg opened it, see #6194. + atime, mtime = 123456780, 234567890 + + def updates_atime(some_file): + os.utime(some_file, (atime, mtime)) + with open(some_file, 'rb') as f: + f.read(1) + return os.stat(some_file).st_atime_ns != atime * 10**9 + + real_os_open = archive_module.os_open + + def os_open_updating_atime(**kwargs): + # open without O_NOATIME (as on a platform not having it), so that reading + # updates the atime - and read right away, as if the open() had done that. + fd = real_os_open(**dict(kwargs, noatime=False)) + if fd is not None: + try: + os.pread(fd, 1, 0) # a read updates the atime (but not the ctime) + except OSError: + pass # not seekable / not readable (fifo, directory, ...) + return fd + + self.create_test_files() + if not updates_atime('input/file1'): + pytest.skip('filesystem does not update the atime when reading') + os.utime('input/file1', (atime, mtime)) + self.cmd('init', '--encryption=repokey', self.repository_location) + with patch.object(archive_module, 'os_open', os_open_updating_atime): + self.cmd('create', '--atime', self.repository_location + '::test', 'input') + # make sure the simulation worked: the open() did update the input file's atime. + assert os.stat('input/file1').st_atime_ns != atime * 10**9 + with changedir('output'): + self.cmd('extract', self.repository_location + '::test') + sto = os.stat('output/input/file1') + assert same_ts_ns(sto.st_mtime_ns, mtime * 10**9) + assert same_ts_ns(sto.st_atime_ns, atime * 10**9) + @pytest.mark.skipif(not is_utime_fully_supported(), reason='cannot properly setup and execute test without utime') @pytest.mark.skipif(not is_birthtime_fully_supported(), reason='cannot properly setup and execute test without birthtime') def test_birthtime(self): @@ -4649,6 +4690,10 @@ def test_extract_xattrs_errors(self): def test_create_exclude_dataless(self): pass + @unittest.skip('patches objects') + def test_atime_open_updates_atime(self): + pass + @unittest.skip('patches objects') def test_create_automount(self): pass From a70ccd1649d9f268cff0007c8cc1a7a90ca2fa89 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Fri, 30 Oct 2026 03:07:55 +0100 Subject: [PATCH 2/2] tests: test_atime: do not create a hardlink create_test_files creates input/hardlink as a hard link to input/file1, so both share one inode and anything touching one of them also changes the atime of the other. That is not what test_atime is about, but it made the test fail on cygwin: At extraction time, borg creates the hard link via os.link(). On cygwin, os.link() updates the atime of the file it links to (at least if that file has the x bit set, as input/file1 has here). So it overwrote the atime borg had correctly restored for input/file1 just before and the test then saw "now" instead of the expected atime. Also, while backing up, borg has to open() input/file1 and that updates the atime of the shared inode. When borg then gets to input/hardlink, that already changed atime is what the stat before the open sees, so stat_update_check() can not recognize it as one we caused ourselves and archives it. The test does not look at input/hardlink, so this did not make it fail, but it is another reason why hardlinks and atime testing do not mix well. On cygwin, test_atime failed reliably before this and passes now. (cherry picked from commit 00fd6676b68291695f704dd0680b32e0c58480ed) Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Opus 5.5 --- src/borg/testsuite/archiver.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index ea543766d5..cea28adb30 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -743,7 +743,10 @@ def has_noatime(some_file): noatime_used = flags_noatime != flags_normal return noatime_used and same_ts_ns(atime_before, atime_after) - self.create_test_files() + # No hardlinks: input/hardlink would share the inode with input/file1, so anything + # touching one of them also changes the atime of the other. That has nothing to do with + # what is tested here, it only makes the results hard to interpret. + self.create_test_files(create_hardlinks=False) atime, mtime = 123456780, 234567890 have_noatime = has_noatime('input/file1') os.utime('input/file1', (atime, mtime))