From 6fab876a5e74adac1ec7720c23698b8dc8a6d756 Mon Sep 17 00:00:00 2001 From: Brian Nguyen Date: Tue, 18 Aug 2026 11:49:34 -0500 Subject: [PATCH 1/3] [TRTLLM-14628][fix] Do not trust just-written mtimes in sync_tree copy-back sync_tree skipped a file when its size and mtime matched the destination. Inode timestamps come from a coarse clock (one timer tick), so a source rewritten shortly after being copied can still report the mtime the copy recorded. The comparison then called it unchanged and left the stale copy in place, which is what makes test_incremental_converges_to_fresh_copy fail intermittently (~4% of pre-merge runs since #17538): pkg/sub/mod.py is rewritten to a same-size body a few milliseconds after the populate, so on a host whose timestamp tick is coarser than that gap the incremental sync keeps the old body. A size+mtime match is now trusted only once the source mtime has aged past a two-second window, and any copy made inside that window is recorded with a deliberately shifted mtime so the next sync re-copies it instead of trusting the match. The cold populate paths (tar pipeline and copytree) apply the same shift, since both copy source mtimes verbatim. Files that have settled are still skipped, so the incremental behaviour this path exists for is unchanged; the cost is one extra copy of files written immediately before a sync. Adds a deterministic regression test that pins the source mtime to the copied value (which fails without this change) plus one that pins the skip of a settled file, so a fix that degraded into a full recopy would be caught. Signed-off-by: Brian Nguyen --- scripts/build_wheel.py | 65 ++++++++++++++++++- .../scripts/test_build_wheel_copy_back.py | 48 ++++++++++++++ 2 files changed, 110 insertions(+), 3 deletions(-) diff --git a/scripts/build_wheel.py b/scripts/build_wheel.py index 51602d91a99b..80d32a2ee488 100755 --- a/scripts/build_wheel.py +++ b/scripts/build_wheel.py @@ -21,6 +21,7 @@ import sys import sysconfig import tempfile +import time import warnings from argparse import ArgumentParser, ArgumentTypeError from contextlib import contextmanager @@ -614,6 +615,36 @@ def _tar_pipe_copy(src: Path, dst: Path) -> bool: return producer.returncode == 0 and consumer.returncode == 0 +# How recently a source file must have been written for its mtime to be +# untrustworthy as a change marker. Inode timestamps come from a coarse clock +# (one timer tick on Linux) and some filesystems store whole seconds, so a file +# rewritten shortly after being copied can still report the mtime the copy +# recorded. A size+mtime comparison would then call it unchanged and leave a +# stale copy behind. Two seconds covers a whole-second-granularity destination +# (which can make an mtime look up to a second older than it is) on top of the +# tick granularity of the source. +_MTIME_RACE_WINDOW = 2.0 + + +def _demote_racy_mtime(dst_file, src_stat, now): + """Break the mtime match for a copy whose source was just written. + + A copy normally records the source's mtime so the next sync can skip it. + That is only sound once the source mtime has aged out of the window above; + before that the source can change again without its mtime moving. Backdating + the copy makes the next sync's comparison mismatch, so the file is re-copied + instead of silently kept stale. It costs one extra copy of files written + right before a sync, and converges: the re-copy records the real mtime. + """ + if src_stat is None or src_stat.st_mtime <= now - _MTIME_RACE_WINDOW: + return + try: + os.utime(dst_file, + (src_stat.st_atime, src_stat.st_mtime - _MTIME_RACE_WINDOW)) + except OSError: + pass + + def sync_tree(src, dst, exclude: Sequence[str] = ()): """Mirror the src directory into dst, touching only what changed. @@ -623,17 +654,37 @@ def sync_tree(src, dst, exclude: Sequence[str] = ()): destination (which may be a slow network filesystem). A missing dst is populated via a streamed tar pipeline instead of per-file copies. Symlinks are dereferenced like copytree(symlinks=False); mtimes are - preserved so the next sync can compare against them. exclude lists - fnmatch patterns for entry names to skip. + preserved so the next sync can compare against them, except for sources + written within _MTIME_RACE_WINDOW of the copy, whose mtimes cannot yet + prove the content settled. exclude lists fnmatch patterns for entry names + to skip. """ import fnmatch src = Path(src).resolve() dst = Path(dst) + now = time.time() def excluded(name): return any(fnmatch.fnmatch(name, pat) for pat in exclude) + def demote_racy_mtimes(): + # A cold populate (tar or copytree) copies source mtimes verbatim, so + # apply the same guard the incremental path applies per file. Walk the + # source rather than the freshly written destination: the source is + # local and warm, and only the few racy entries need a write. + for root, dirs, files in os.walk(src, followlinks=True): + dirs[:] = [d for d in dirs if not excluded(d)] + rel = Path(root).relative_to(src) + for name in files: + if excluded(name): + continue + try: + src_stat = (Path(root) / name).stat() + except OSError: + continue + _demote_racy_mtime(dst / rel / name, src_stat, now) + if dst.is_symlink(): dst.unlink() elif dst.exists() and src == dst.resolve(): @@ -641,11 +692,13 @@ def excluded(name): if not dst.exists(): if not exclude and _tar_pipe_copy(src, dst): + demote_racy_mtimes() return copytree(src, dst, symlinks=False, ignore=shutil.ignore_patterns(*exclude) if exclude else None) + demote_racy_mtimes() return for root, dirs, files in os.walk(src, followlinks=True): @@ -667,11 +720,16 @@ def excluded(name): for name in files: src_file = Path(root) / name dst_file = dst_root / name + src_stat = None try: src_stat = src_file.stat() dst_stat = dst_file.stat() + # Trust the match only once the source mtime has aged past the + # race window; a just-written source can be rewritten again + # without the mtime moving, which would strand a stale copy. if (src_stat.st_size == dst_stat.st_size - and abs(src_stat.st_mtime - dst_stat.st_mtime) < 1e-3): + and abs(src_stat.st_mtime - dst_stat.st_mtime) < 1e-3 + and src_stat.st_mtime <= now - _MTIME_RACE_WINDOW): continue except OSError: pass @@ -679,6 +737,7 @@ def excluded(name): rmtree(dst_file) # copy2: mtime must survive for the next sync's comparison. shutil.copy2(src_file, dst_file) + _demote_racy_mtime(dst_file, src_stat, now) def stage_python_package(project_dir: Path, staging_dir: Path) -> None: diff --git a/tests/unittest/scripts/test_build_wheel_copy_back.py b/tests/unittest/scripts/test_build_wheel_copy_back.py index 8d1449405911..1a32df8f52ce 100644 --- a/tests/unittest/scripts/test_build_wheel_copy_back.py +++ b/tests/unittest/scripts/test_build_wheel_copy_back.py @@ -30,6 +30,7 @@ import os import shutil import stat +import time from pathlib import Path import pytest @@ -183,6 +184,53 @@ def test_warm_noop_is_stable(sync_tree, tmp_path): assert_trees_equal(new_dir, old_dir) +def test_rewrite_without_mtime_change_is_not_skipped(sync_tree, tmp_path): + """A same-size rewrite that leaves the source mtime untouched must copy. + + Inode timestamps come from a coarse clock, so a file rewritten shortly + after being copied can still report the mtime the copy recorded, and a + size+mtime comparison would call it unchanged. utime pins the mtime to the + copied value, making that collision deterministic instead of a race. + """ + src = tmp_path / "src" + src.mkdir() + mod = src / "mod.py" + mod.write_bytes(b"def f():\n return 42\n") + new_dir = tmp_path / "new" + sync_tree(src, new_dir) + + copied = mod.stat() + mod.write_bytes(b"def f():\n return 99\n") # same size + os.utime(mod, (copied.st_atime, copied.st_mtime)) + sync_tree(src, new_dir) + + assert (new_dir / "mod.py").read_bytes() == b"def f():\n return 99\n" + + +def test_settled_file_is_not_recopied(sync_tree, tmp_path): + """A file whose mtime has aged out of the race window is left alone. + + Pins the incremental behaviour itself: the destination is diverged behind + sync_tree's back, so surviving the re-sync proves the file was skipped + rather than rewritten. + """ + src = tmp_path / "src" + src.mkdir() + aged = src / "aged.bin" + aged.write_bytes(b"aged\n") + old = time.time() - 3600 + os.utime(aged, (old, old)) + new_dir = tmp_path / "new" + sync_tree(src, new_dir) + + dst_file = new_dir / "aged.bin" + dst_file.write_bytes(b"kept\n") # same size as the source + os.utime(dst_file, (old, old)) + sync_tree(src, new_dir) + + assert dst_file.read_bytes() == b"kept\n" + + def test_same_src_dst_is_noop(sync_tree, tmp_path): """sync_tree onto itself must not wipe the tree. From 28d95183510f336c24ba67c9147ac9305f5b1fa2 Mon Sep 17 00:00:00 2001 From: Brian Nguyen Date: Tue, 18 Aug 2026 13:31:23 -0500 Subject: [PATCH 2/3] [TRTLLM-14628][test] Exercise both cold-populate backends in copy-back tests The mtime-collision tests populated the destination via sync_tree, whose cold populate uses _tar_pipe_copy when tar is on PATH and falls back to copytree otherwise, so each run only covered whichever backend the environment selected. Add a parametrized cold_backend fixture that runs the two mtime tests once per backend (monkeypatching _tar_pipe_copy off for the copytree case, skipping the tar case when tar is absent so it never aliases to copytree). Signed-off-by: Brian Nguyen --- .../scripts/test_build_wheel_copy_back.py | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/tests/unittest/scripts/test_build_wheel_copy_back.py b/tests/unittest/scripts/test_build_wheel_copy_back.py index 1a32df8f52ce..95b6e1c00991 100644 --- a/tests/unittest/scripts/test_build_wheel_copy_back.py +++ b/tests/unittest/scripts/test_build_wheel_copy_back.py @@ -42,11 +42,34 @@ @pytest.fixture(scope="module") -def sync_tree(): +def build_wheel_module(): spec = importlib.util.spec_from_file_location("build_wheel", SCRIPT_PATH) module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) - return module.sync_tree + return module + + +@pytest.fixture +def sync_tree(build_wheel_module): + return build_wheel_module.sync_tree + + +@pytest.fixture(params=["tar", "copytree"]) +def cold_backend(request, build_wheel_module, monkeypatch): + """Force sync_tree's cold populate down each backend in turn. + + A missing destination is filled by _tar_pipe_copy when tar is on PATH and + by copytree otherwise. Both preserve mtimes, so the mtime-collision tests + that consume this fixture must hold on either; without pinning the backend + a run only covers whichever one the environment happens to select. The tar + case skips when tar is absent so it can never silently alias to copytree. + """ + if request.param == "tar": + if shutil.which("tar") is None: + pytest.skip("tar unavailable; cannot exercise the tar cold path") + else: + monkeypatch.setattr(build_wheel_module, "_tar_pipe_copy", lambda src, dst: False) + return request.param def old_copy(src, dst, exclude=()): @@ -184,13 +207,14 @@ def test_warm_noop_is_stable(sync_tree, tmp_path): assert_trees_equal(new_dir, old_dir) -def test_rewrite_without_mtime_change_is_not_skipped(sync_tree, tmp_path): +def test_rewrite_without_mtime_change_is_not_skipped(sync_tree, cold_backend, tmp_path): """A same-size rewrite that leaves the source mtime untouched must copy. Inode timestamps come from a coarse clock, so a file rewritten shortly after being copied can still report the mtime the copy recorded, and a size+mtime comparison would call it unchanged. utime pins the mtime to the copied value, making that collision deterministic instead of a race. + cold_backend runs this once per cold-populate path (tar and copytree). """ src = tmp_path / "src" src.mkdir() @@ -207,12 +231,13 @@ def test_rewrite_without_mtime_change_is_not_skipped(sync_tree, tmp_path): assert (new_dir / "mod.py").read_bytes() == b"def f():\n return 99\n" -def test_settled_file_is_not_recopied(sync_tree, tmp_path): +def test_settled_file_is_not_recopied(sync_tree, cold_backend, tmp_path): """A file whose mtime has aged out of the race window is left alone. Pins the incremental behaviour itself: the destination is diverged behind sync_tree's back, so surviving the re-sync proves the file was skipped - rather than rewritten. + rather than rewritten. cold_backend runs this once per cold-populate path + (tar and copytree). """ src = tmp_path / "src" src.mkdir() From d8d350c8820005c36ca579668435db6fa12347a6 Mon Sep 17 00:00:00 2001 From: Brian Nguyen Date: Wed, 19 Aug 2026 10:53:21 -0500 Subject: [PATCH 3/3] [TRTLLM-14628][chore] Add type annotations to sync_tree copy-back helpers and tests Signed-off-by: Brian Nguyen --- scripts/build_wheel.py | 9 +++++---- tests/unittest/scripts/test_build_wheel_copy_back.py | 9 +++++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/scripts/build_wheel.py b/scripts/build_wheel.py index 80d32a2ee488..e546b2e12ca8 100755 --- a/scripts/build_wheel.py +++ b/scripts/build_wheel.py @@ -626,7 +626,8 @@ def _tar_pipe_copy(src: Path, dst: Path) -> bool: _MTIME_RACE_WINDOW = 2.0 -def _demote_racy_mtime(dst_file, src_stat, now): +def _demote_racy_mtime(dst_file: Path, src_stat: Optional[os.stat_result], + now: float) -> None: """Break the mtime match for a copy whose source was just written. A copy normally records the source's mtime so the next sync can skip it. @@ -645,7 +646,7 @@ def _demote_racy_mtime(dst_file, src_stat, now): pass -def sync_tree(src, dst, exclude: Sequence[str] = ()): +def sync_tree(src: Path, dst: Path, exclude: Sequence[str] = ()) -> None: """Mirror the src directory into dst, touching only what changed. Replaces the rmtree+copytree pattern for artifact copy-back: files are @@ -665,10 +666,10 @@ def sync_tree(src, dst, exclude: Sequence[str] = ()): dst = Path(dst) now = time.time() - def excluded(name): + def excluded(name: str) -> bool: return any(fnmatch.fnmatch(name, pat) for pat in exclude) - def demote_racy_mtimes(): + def demote_racy_mtimes() -> None: # A cold populate (tar or copytree) copies source mtimes verbatim, so # apply the same guard the incremental path applies per file. Walk the # source rather than the freshly written destination: the source is diff --git a/tests/unittest/scripts/test_build_wheel_copy_back.py b/tests/unittest/scripts/test_build_wheel_copy_back.py index 95b6e1c00991..d69999d06628 100644 --- a/tests/unittest/scripts/test_build_wheel_copy_back.py +++ b/tests/unittest/scripts/test_build_wheel_copy_back.py @@ -32,6 +32,7 @@ import stat import time from pathlib import Path +from typing import Callable import pytest @@ -207,7 +208,9 @@ def test_warm_noop_is_stable(sync_tree, tmp_path): assert_trees_equal(new_dir, old_dir) -def test_rewrite_without_mtime_change_is_not_skipped(sync_tree, cold_backend, tmp_path): +def test_rewrite_without_mtime_change_is_not_skipped( + sync_tree: Callable[..., None], cold_backend: str, tmp_path: Path +) -> None: """A same-size rewrite that leaves the source mtime untouched must copy. Inode timestamps come from a coarse clock, so a file rewritten shortly @@ -231,7 +234,9 @@ def test_rewrite_without_mtime_change_is_not_skipped(sync_tree, cold_backend, tm assert (new_dir / "mod.py").read_bytes() == b"def f():\n return 99\n" -def test_settled_file_is_not_recopied(sync_tree, cold_backend, tmp_path): +def test_settled_file_is_not_recopied( + sync_tree: Callable[..., None], cold_backend: str, tmp_path: Path +) -> None: """A file whose mtime has aged out of the race window is left alone. Pins the incremental behaviour itself: the destination is diverged behind