From e7a2bb9f187ad74d3983e25467eff65a812d0c81 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 6 Jul 2026 07:39:25 +0900 Subject: [PATCH] fix: keep attribute preservation best-effort when os.utime fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit preserve_file_attributes documents best-effort semantics ("failures are ignored so a completed conversion is not lost"), and guards chmod and the xattr copy accordingly. The two os.utime calls that restore nanosecond atime/mtime were unguarded, so an OSError (read-only destination, a filesystem without timestamp support) propagated up through convert_file and discarded an otherwise completed conversion — and skipped the macOS creation-time restore. Extract the utime restore into a _restore_timestamps helper that swallows OSError, matching the surrounding best-effort guards. Add a regression test that patches os.utime to raise and asserts the copy neither raises nor skips the creation-time step. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01CJRVbDrp1vGYkJgNHMGPpG --- media_shrinker.py | 17 +++++++++++++++-- tests/test_media_shrinker.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/media_shrinker.py b/media_shrinker.py index d72a8d5..7f21fbf 100644 --- a/media_shrinker.py +++ b/media_shrinker.py @@ -700,12 +700,12 @@ def preserve_file_attributes( _copy_extended_attributes(source, dest) - os.utime(dest, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns)) + _restore_timestamps(source_stat, dest) resolved_setfile = setfile_path if setfile_path is not None else _get_setfile_path() if resolved_setfile: _copy_macos_creation_time(source_stat, dest, resolved_setfile) - os.utime(dest, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns)) + _restore_timestamps(source_stat, dest) def convert_file( @@ -1630,6 +1630,19 @@ def _resolve_collision(path: Path, *, overwrite: bool) -> Path: raise FileExistsError(f"Could not find free output path for {path}") +def _restore_timestamps(source_stat: os.stat_result, dest: Path) -> None: + """Best-effort copy of nanosecond atime/mtime from source_stat onto dest. + + A read-only destination or a filesystem lacking timestamp support can make + os.utime raise OSError; that is non-critical metadata, so the failure is + swallowed to keep attribute preservation best-effort. + """ + try: + os.utime(dest, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns)) + except OSError: + pass + + def _copy_extended_attributes(source: Path, dest: Path) -> None: """Copy extended attributes from source to dest if supported by OS.""" if not all(hasattr(os, attr) for attr in ("listxattr", "getxattr", "setxattr")): diff --git a/tests/test_media_shrinker.py b/tests/test_media_shrinker.py index 0ab2397..9495186 100644 --- a/tests/test_media_shrinker.py +++ b/tests/test_media_shrinker.py @@ -1914,3 +1914,32 @@ class MockStat: with patch("media_shrinker._copy_macos_creation_time"): with patch("media_shrinker._get_setfile_path", return_value="/bin/echo"): preserve_file_attributes(src, dest) + + def test_preserve_file_attributes_ignores_utime_error(self) -> None: + """os.utime failure must not abort best-effort metadata copy. + + A read-only or timestamp-unsupporting destination filesystem can make + os.utime raise OSError. The documented contract is best-effort, so the + completed conversion must not be lost and the macOS creation-time step + must still run. + """ + from media_shrinker import preserve_file_attributes + + with tempfile.TemporaryDirectory() as tmp: + src = Path(tmp) / "src.txt" + src.write_text("hello") + dest = Path(tmp) / "dest.txt" + dest.write_text("world") + with patch( + "media_shrinker.os.utime", side_effect=OSError("read-only fs") + ): + with patch( + "media_shrinker._copy_macos_creation_time" + ) as mock_creation: + with patch( + "media_shrinker._get_setfile_path", return_value="/bin/echo" + ): + # Must not raise despite os.utime failing. + preserve_file_attributes(src, dest) + # Best-effort continues to the creation-time step after utime fails. + mock_creation.assert_called_once()