Skip to content

fix(cloud): pin selected sources with handle stats so Windows ctime compares like with like - #1300

Open
itzzdev09 wants to merge 2 commits into
usestrix:mainfrom
itzzdev09:fix/windows-source-archive-ctime
Open

itzzdev09 wants to merge 2 commits into
usestrix:mainfrom
itzzdev09:fix/windows-source-archive-ctime

Conversation

@itzzdev09

@itzzdev09 itzzdev09 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #1258

Root cause

select_source pins every file with path.lstat(), and _write_archive checks it again with os.fstat() on the open handle. On POSIX both calls fill st_ctime_ns with the inode-change time. On Windows they don't: CPython fills it from different Win32 calls. A path stat reports the file's creation time, while fstat reports its last metadata change.

So the guard compares two different timestamps. They only agree for a file that hasn't changed since it was created. That explains both symptoms in this issue:

  • The intermittent test failures. The tests write a file and archive it within milliseconds, so the creation and change times are sometimes equal and sometimes a tick apart. I originally put this down to NTFS settling metadata asynchronously. That was wrong: it's the two calls meaning different things.
  • The permanent failure @Kotsur69 hit on a real checkout. lstat gave the NTFS creation time, and fstat gave a value equal to st_mtime_ns, the last change. Any file edited after it was created fails every time.

Measured on Windows 11 with Python 3.12, on 3,000 freshly written files (a third or half of them appended to after creation):

comparison disagreements
lstat ctime vs fstat ctime, as main compares them 1,091 / 3,000
fstat vs fstat on the same handle, before and after reading 0 / 3,000
fstat vs fstat from two separate handles, opened 0.5 s apart 0 / 3,000

On Windows, fstat's ctime does move on a chmod and on a rename away and back, exactly like POSIX ctime, while lstat's doesn't.

The fix: compare like with like

Instead of leaving st_ctime_ns out on Windows, selection now pins each file with the same call the archiver checks it with:

  • select_source still uses lstat to reject symlinks and non-regular files, then opens the file once and takes fstat from that handle. It checks that the handle's (st_dev, st_ino) match the lstat, so a file swapped in between is refused. The archive-magic check reads its header from the same handle instead of opening the file a second time.
  • _stat_identity is a single definition for every platform: dev, inode, size, mtime and ctime. There's no longer a platform flag.
  • _write_archive is unchanged. Its before-read and after-read fstat now compare against a pin of the same kind.

What that keeps compared with dropping ctime on Windows: a metadata-only change is still caught there, just as on POSIX. That covers a chmod or attribute change, or a rename away and back, that leaves size and mtime untouched, whether it happens between selection and archiving or during the read. With ctime excluded on Windows, such a file would be uploaded as if nothing had changed.

Behaviour note: because selection now opens each regular file, a file that can be stat'ed but not opened fails with the same could not safely read ... error that archiving already raised, just earlier.

Tests

Four tests replace the two earlier ones:

test platforms fails on main fails with ctime dropped on Windows
test_archive_accepts_a_path_stat_with_a_different_ctime: lstat reports a different ctime than fstat, reproducing the Windows case anywhere all
test_archive_accepts_a_windows_file_modified_after_creation: a real file appended to after creation, with the lstat/fstat disagreement asserted first Windows
test_archive_rejects_a_metadata_only_change_after_selection: a real chmod between selection and archiving all
test_archive_rejects_a_metadata_change_during_the_read: fstat ctime moves between the before and after checks all

The first two fail against main's source_upload.py. The last two fail against the previous version of this PR, which left ctime out on Windows. All four pass here.

Flakiness, running tests/test_cloud_source_upload.py five times on Windows:

main:         2, 6, 9, 2, 4 failed
this branch:  0, 0, 0, 0, 0 failed   (34 passed, 1 skipped each run)

On the full suite on Windows, all 28 tests that fail on this branch also fail on main. They're Windows-specific test assumptions in other modules (19 of them in test_threat_model_tool.py), which #1297 addresses. main additionally failed 5 tests in test_cloud_source_upload.py on that run, and those pass here. The pinned ruff 0.15.20 check and format are clean. mypy reports nothing new in source_upload.py for --platform linux, darwin or win32.

`_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 usestrix#1258

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge with no actionable correctness, security, or repository-rule violations identified.

Summary

  • Preserves device, inode, size, and mtime checks on every platform.
  • Applies the same identity logic before and after archive reads.
  • Adds platform-specific regression tests for Windows ctime drift and POSIX ctime changes.

Reviews (1) · Last reviewed commit: "fix(cloud): stop comparing st_ctime_ns o..."

…g ctime

On Windows a path stat reports the creation time as st_ctime_ns while fstat on a handle reports the last metadata change, so pinning files with lstat and checking them with fstat compared two different timestamps and rejected every file modified after it was created. The previous commit excluded st_ctime_ns on Windows, which also stopped a chmod or rename from being detected there.

Selection now takes fstat from the file's own handle, after checking it is the same file lstat saw, and reads the archive header from that handle. Both sides of the comparison mean the same thing on every platform, so st_ctime_ns is part of the identity everywhere again.
@itzzdev09 itzzdev09 changed the title fix(cloud): stop comparing st_ctime_ns on Windows when archiving sources fix(cloud): pin selected sources with handle stats so Windows ctime compares like with like Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Cloud packaging intermittently rejects unchanged Windows files

1 participant