Merge 721cf75626 into 851743adcf

strix/interface/cloud/source_upload.py

@@ -127,6 +127,21 @@ _ARCHIVE_MAGIC_PREFIXES = (
 )
 
 
+def _stat_identity(info: os.stat_result) -> tuple[int, ...]:
+    """Fields that must not change between selecting a file and archiving it.
+
+    Both sides of the comparison must come from `fstat` on an open handle.
+    What `st_ctime_ns` means depends on how a file is stat'ed: on POSIX a path
+    stat and `fstat` both report the inode-change time, but on Windows a path
+    stat reports the creation time while `fstat` reports the last metadata
+    change. Pinning files with a path stat therefore rejected every file
+    modified after it was created (#1258). With handle stats on both sides the
+    two agree, and `st_ctime_ns` stays a tamper signal on every platform: a
+    chmod or a rename that leaves size and mtime alone still moves it.
+    """
+    return (info.st_dev, info.st_ino, info.st_size, info.st_mtime_ns, info.st_ctime_ns)
+
+
 @dataclass(frozen=True)
 class SelectedFile:
     path: Path
@@ -137,6 +152,10 @@ class SelectedFile:
     mtime_ns: int
     ctime_ns: int
 
+    @property
+    def identity(self) -> tuple[int, ...]:
+        return (self.device, self.inode, self.size, self.mtime_ns, self.ctime_ns)
+
 
 @dataclass(frozen=True)
 class SourceManifest:
@@ -302,10 +321,21 @@ def select_source(
         if not stat.S_ISREG(info.st_mode):
             excluded["symlink_or_non_file"] += 1
             continue
-        if not include_archives and _has_archive_magic(path):
+        # Pin the file with the same kind of stat _write_archive checks it with.
+        handle_info, header = _inspect_regular_file(
+            path, archive_name, read_header=not include_archives
+        )
+        if not stat.S_ISREG(handle_info.st_mode) or (
+            handle_info.st_dev,
+            handle_info.st_ino,
+        ) != (info.st_dev, info.st_ino):
+            raise http.CloudError(
+                f"{archive_name} changed while the source was being selected; retry."
+            )
+        if not include_archives and _is_archive_header(header):
             excluded["nested_archive"] += 1
             continue
-        if info.st_size > MAX_FILE_BYTES:
+        if handle_info.st_size > MAX_FILE_BYTES:
             raise http.CloudError(
                 f"{archive_name} is larger than the 25 MB per-file limit; exclude it explicitly."
             )
@@ -313,14 +343,14 @@ def select_source(
             SelectedFile(
                 path=path,
                 archive_name=archive_name,
-                size=info.st_size,
-                device=info.st_dev,
-                inode=info.st_ino,
-                mtime_ns=info.st_mtime_ns,
-                ctime_ns=info.st_ctime_ns,
+                size=handle_info.st_size,
+                device=handle_info.st_dev,
+                inode=handle_info.st_ino,
+                mtime_ns=handle_info.st_mtime_ns,
+                ctime_ns=handle_info.st_ctime_ns,
             )
         )
-        total_bytes += info.st_size
+        total_bytes += handle_info.st_size
         if len(selected) > MAX_FILES:
             raise http.CloudError(
                 f"source contains more than {MAX_FILES:,} files; narrow --source or add exclusions."
@@ -597,14 +627,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 +650,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 "
@@ -653,12 +672,34 @@ def _has_archive_magic(path: Path) -> bool:
     try:
         descriptor = os.open(path, flags)
         with os.fdopen(descriptor, "rb") as stream:
-            header = stream.read(512)
+            header = stream.read(_ARCHIVE_HEADER_BYTES)
     except OSError:
         return False
+    return _is_archive_header(header)
+
+
+_ARCHIVE_HEADER_BYTES = 512
+
+
+def _is_archive_header(header: bytes) -> bool:
     return header.startswith(_ARCHIVE_MAGIC_PREFIXES) or header[257:262] == b"ustar"
 
 
+def _inspect_regular_file(
+    path: Path, archive_name: str, *, read_header: bool
+) -> tuple[os.stat_result, bytes]:
+    """Stat a file through its own handle, reading its header too if asked."""
+    flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)
+    try:
+        descriptor = os.open(path, flags)
+        with os.fdopen(descriptor, "rb") as stream:
+            info = os.fstat(stream.fileno())
+            header = stream.read(_ARCHIVE_HEADER_BYTES) if read_header else b""
+    except OSError as exc:
+        raise http.CloudError(f"could not safely read {archive_name}: {exc}") from exc
+    return info, header
+
+
 def _load_ignore_patterns(source: Path) -> list[str]:
     path = source / ".strixignore"
     raw_text = _read_ignore_file(path)

tests/test_cloud_source_upload.py

@@ -6,6 +6,7 @@ import json
 import os
 import shutil
 import subprocess
+import time
 import zipfile
 from typing import TYPE_CHECKING, Any
 
@@ -696,3 +697,95 @@ 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 _StatWith:
+    """A stat result with some of its fields replaced."""
+
+    def __init__(self, real: Any, **fields: int) -> None:
+        self._real = real
+        self.__dict__.update(fields)
+
+    def __getattr__(self, name: str) -> Any:
+        return getattr(self._real, name)
+
+
+def test_archive_accepts_a_path_stat_with_a_different_ctime(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Selection must not pin files with what a path stat calls `st_ctime_ns`.
+
+    On Windows a path stat reports the creation time there and `fstat` the last
+    metadata change, so for a file modified after it was created the two
+    differ for good (#1258). This reproduces that on any platform.
+    """
+    (tmp_path / "app.py").write_bytes(b"safe")
+    real_lstat = source_upload.Path.lstat
+
+    def lstat_reporting_creation_time(self: Path) -> Any:
+        info = real_lstat(self)
+        return _StatWith(info, st_ctime_ns=info.st_ctime_ns - 7 * 60 * 1_000_000_000)
+
+    monkeypatch.setattr(source_upload.Path, "lstat", lstat_reporting_creation_time)
+    manifest = source_upload.select_source(tmp_path)
+
+    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="only Windows reports different ctimes")
+def test_archive_accepts_a_windows_file_modified_after_creation(tmp_path: Path) -> None:
+    source_path = tmp_path / "app.py"
+    source_path.write_bytes(b"safe")
+    time.sleep(0.1)
+    with source_path.open("ab") as stream:
+        stream.write(b" and sound")
+    with source_path.open("rb") as stream:
+        # the situation from #1258: the path and the handle disagree on ctime
+        assert os.fstat(stream.fileno()).st_ctime_ns != source_path.lstat().st_ctime_ns
+    manifest = source_upload.select_source(tmp_path)
+
+    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 and sound"
+
+
+def test_archive_rejects_a_metadata_only_change_after_selection(tmp_path: Path) -> None:
+    """A chmod leaves size and mtime alone but moves the handle ctime, on Windows too."""
+    source_path = tmp_path / "app.py"
+    source_path.write_bytes(b"safe")
+    manifest = source_upload.select_source(tmp_path)
+    time.sleep(0.1)  # step past the filesystem's timestamp resolution
+    source_path.chmod(0o444)
+    try:
+        with pytest.raises(
+            http.CloudError, match="changed while the source archive was being built"
+        ):
+            source_upload._write_archive(tmp_path / "source.zip", manifest.files)
+    finally:
+        source_path.chmod(0o644)
+
+
+def test_archive_rejects_a_metadata_change_during_the_read(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    (tmp_path / "app.py").write_bytes(b"safe")
+    manifest = source_upload.select_source(tmp_path)
+    real_fstat = os.fstat
+    calls = 0
+
+    def fstat_changing_after_the_first_call(fileno: int) -> Any:
+        nonlocal calls
+        calls += 1
+        info = real_fstat(fileno)
+        if calls == 1:
+            return info
+        return _StatWith(info, st_ctime_ns=info.st_ctime_ns + 1)
+
+    monkeypatch.setattr(source_upload.os, "fstat", fstat_changing_after_the_first_call)
+
+    with pytest.raises(http.CloudError, match="changed while the source archive was being built"):
+        source_upload._write_archive(tmp_path / "source.zip", manifest.files)