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 <noreply@anthropic.com>
This commit is contained in:
itzzdev09 2026-09-12 12:35:19 +05:30
parent 95e085eb6c
commit 3b63de2ad4
2 changed files with 85 additions and 13 deletions

View file

@ -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 "

View file

@ -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)