From 3b63de2ad404b45dfa9fc3d1095817a929941dad Mon Sep 17 00:00:00 2001 From: itzzdev09 Date: Sat, 12 Sep 2026 12:35:19 +0530 Subject: [PATCH] fix(cloud): stop comparing st_ctime_ns on Windows when archiving sources `_write_archive` pins each file to the stat it was selected with and rejects the upload if anything moved. On Windows that check fires on unchanged files: a path `stat` can be served from cached directory metadata while `fstat` reads the handle directly, and NTFS settles that metadata asynchronously, so `st_ctime_ns` drifts by roughly 1-30 ms with nothing having touched the file. Repeated runs of the packaging tests failed a different, shifting set of cases every time. `st_ctime_ns` earns its place on POSIX, where it is the inode-change time and moves for a chmod or rename that leaves size and mtime alone. It buys nothing on Windows, where it does not hold still. Drop it from the comparison there and keep device, inode, size, and mtime, which still catch replacement and truncation, alongside the existing post-read check for a file that grew during the read. Both comparisons now go through one `_stat_identity` helper instead of two copies of a six-clause condition. Fixes #1258 Co-Authored-By: Claude Opus 5 --- strix/interface/cloud/source_upload.py | 42 +++++++++++++------ tests/test_cloud_source_upload.py | 56 ++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 13 deletions(-) diff --git a/strix/interface/cloud/source_upload.py b/strix/interface/cloud/source_upload.py index 780a84569..81dfa6486 100644 --- a/strix/interface/cloud/source_upload.py +++ b/strix/interface/cloud/source_upload.py @@ -127,6 +127,26 @@ _ARCHIVE_MAGIC_PREFIXES = ( ) +# Whether `st_ctime_ns` belongs in the identity a source file is pinned to. +# +# On POSIX it is the inode-change time and a real tamper signal: a chmod or a +# rename that leaves size and mtime untouched still moves it. On Windows the +# value does not hold still for a file that was written moments ago. A path +# `stat` can be served from cached directory metadata while `fstat` reads the +# handle directly, and NTFS settles that metadata asynchronously, so the two +# disagree by roughly 1-30 ms with nothing having touched the file. Comparing +# it there rejects unchanged sources instead of catching tampering (#1258). +_IDENTITY_INCLUDES_CTIME = os.name != "nt" + + +def _stat_identity(info: os.stat_result) -> tuple[int, ...]: + """Fields that must not change between selecting a file and archiving it.""" + identity: tuple[int, ...] = (info.st_dev, info.st_ino, info.st_size, info.st_mtime_ns) + if _IDENTITY_INCLUDES_CTIME: + identity = (*identity, info.st_ctime_ns) + return identity + + @dataclass(frozen=True) class SelectedFile: path: Path @@ -137,6 +157,13 @@ class SelectedFile: mtime_ns: int ctime_ns: int + @property + def identity(self) -> tuple[int, ...]: + identity: tuple[int, ...] = (self.device, self.inode, self.size, self.mtime_ns) + if _IDENTITY_INCLUDES_CTIME: + identity = (*identity, self.ctime_ns) + return identity + @dataclass(frozen=True) class SourceManifest: @@ -597,14 +624,7 @@ def _write_archive(destination: Path, files: tuple[SelectedFile, ...]) -> None: raise http.CloudError(f"could not safely read {item.archive_name}: {exc}") from exc with os.fdopen(descriptor, "rb") as source_file: current = os.fstat(source_file.fileno()) - if ( - not stat.S_ISREG(current.st_mode) - or current.st_size != item.size - or current.st_dev != item.device - or current.st_ino != item.inode - or current.st_mtime_ns != item.mtime_ns - or current.st_ctime_ns != item.ctime_ns - ): + if not stat.S_ISREG(current.st_mode) or _stat_identity(current) != item.identity: raise http.CloudError( f"{item.archive_name} changed while the source archive was being built; " "retry." @@ -627,11 +647,7 @@ def _write_archive(destination: Path, files: tuple[SelectedFile, ...]) -> None: if ( source_file.read(1) or not stat.S_ISREG(final.st_mode) - or final.st_size != item.size - or final.st_dev != item.device - or final.st_ino != item.inode - or final.st_mtime_ns != item.mtime_ns - or final.st_ctime_ns != item.ctime_ns + or _stat_identity(final) != item.identity ): raise http.CloudError( f"{item.archive_name} changed while the source archive was being " diff --git a/tests/test_cloud_source_upload.py b/tests/test_cloud_source_upload.py index dcd40baa8..8ad650527 100644 --- a/tests/test_cloud_source_upload.py +++ b/tests/test_cloud_source_upload.py @@ -696,3 +696,59 @@ def test_oversize_archive_names_largest_files_and_exclude_guidance( assert "--exclude" in raised.value.next_step assert "--dry-run --show-files" in raised.value.next_step assert not list(tmp_path.glob("strix-source-*.zip")) + + +class _CtimeDriftedStat: + """A stat result whose `st_ctime_ns` has moved and nothing else.""" + + def __init__(self, real: os.stat_result, ctime_ns: int) -> None: + self._real = real + self.st_ctime_ns = ctime_ns + + def __getattr__(self, name: str) -> Any: + return getattr(self._real, name) + + +def _drift_ctime(monkeypatch: pytest.MonkeyPatch, delta_ns: int) -> None: + """Make every `fstat` in the archiver report a later ctime than selection saw.""" + real_fstat = os.fstat + + def drifted(fileno: int) -> Any: + info = real_fstat(fileno) + return _CtimeDriftedStat(info, info.st_ctime_ns + delta_ns) + + monkeypatch.setattr(source_upload.os, "fstat", drifted) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows is where ctime drifts on its own") +def test_archive_survives_a_windows_ctime_only_drift( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """An unchanged file must still archive when only `st_ctime_ns` moved. + + Windows serves a path `stat` from cached directory metadata and `fstat` + from the handle, and NTFS settles that metadata asynchronously, so the two + disagree for a short window after a write with nothing having touched the + file. That is not tampering and must not fail the upload. + """ + (tmp_path / "app.py").write_bytes(b"safe") + manifest = source_upload.select_source(tmp_path) + _drift_ctime(monkeypatch, 30_000_000) # 30 ms, the worst drift observed + + source_upload._write_archive(tmp_path / "source.zip", manifest.files) + + with zipfile.ZipFile(tmp_path / "source.zip") as archive: + assert archive.read("app.py") == b"safe" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX ctime is a real tamper signal") +def test_archive_still_rejects_a_posix_ctime_change( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """On POSIX `st_ctime_ns` is inode-change time, so a move in it is a change.""" + (tmp_path / "app.py").write_bytes(b"safe") + manifest = source_upload.select_source(tmp_path) + _drift_ctime(monkeypatch, 30_000_000) + + with pytest.raises(http.CloudError, match="changed while the source archive was being built"): + source_upload._write_archive(tmp_path / "source.zip", manifest.files)