add mtime logic to file difference checking - #14
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70ba11a506
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
vertix
left a comment
There was a problem hiding this comment.
Overall this looks good, but there's one question about reliability of the current approach.
Some other things
- Add a normal PR description
- Look into the recent PRs and do the proper minor release in this PR. I.e. update releasenotes, pyproject.toml etc. This should be a new release
| def _compute_sync_diff(source: Iterator[FileInfo], target: Iterator[FileInfo]) -> tuple[list[FileInfo], list[FileInfo]]: | ||
| """Return ``(to_copy, to_delete)`` to make ``target`` mirror ``source``. | ||
|
|
||
| A file is copied when it is missing on the target, differs in size, or |
There was a problem hiding this comment.
make the docstring oneliner, too verbose now
the last sentence is not needed, just drop it, i mean
The
mtime check is what catches an in-place edit that
leaves the byte count unchanged (a rewritten checkpoint, a fixed-shape
array, a same-length text edit); size alone cannot see it.
| # file can therefore look "newer" than its S3 twin by a fraction of a second. | ||
| # Treat a source as newer only when it is ahead by more than this margin, so | ||
| # a freshly synced tree does not re-transfer on every tick. | ||
| _MTIME_TOLERANCE_SECONDS = 2.0 |
There was a problem hiding this comment.
How do we know that 2 seconds is enough? what if the connection lags, or smth like this. Is there a more "hard" method? For example, can we use an extra meta information to store the local data, and fallback to the "default" LastModified only when there's no our meta?
There was a problem hiding this comment.
You are right, the download from s3 direction is OK, since it assigns the Last-Modified info from s3 to a local file.
However, the upload direction is broken and prone to many bugs since we don't control Last-Modified metadata on the s3 side.
Claude's possible solution is:
The other way to match them is to change the local file instead. After an upload, read back LastModified (one extra request per uploaded file) and set the local file's mtime to it. It works, but I wouldn't do it
There was a problem hiding this comment.
Also, this might be concerning:
Upgrade note
Files downloaded by pos3 ≤ 0.3.1 have their download time as mtime, which is newer than their S3 LastModified. The first upload() / sync() of such a tree after upgrading will re-upload it once. After that, syncs are incremental again. This is also in the changelog.
Bump version to 0.3.2 in pyproject.toml and uv.lock, and move the mtime change-detection entries from [Unreleased] to [0.3.2]. Also correct the documented tolerance (2 s, matching _MTIME_TOLERANCE_SECONDS, not 1 s) and add an upgrade note: trees downloaded by <= 0.3.1 carry download-time mtimes, so their first upload/sync after upgrading re-uploads once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Change detection used to compare only presence and size. A file rewritten in place with the same byte count (an overwritten checkpoint, a fixed-shape array, a same-length text edit) was silently skipped by
upload,download,sync, and the background interval loop.This PR adds modification time to the diff and ships it as v0.3.2.
What changed
FileInfo.mtime: new optional field (POSIX timestamp)._scan_localfills it fromst_mtime, and_scan_s3fills it from the object'sLastModified. It isNonefor directories or when the backend doesn't report a time._compute_sync_diff: a file is now copied when it is_MTIME_TOLERANCE_SECONDS(2 s).If either side has no mtime, that entry falls back to size-only comparison, as before.
Downloads keep the S3 timestamp (like rsync
-t): afterdownload_file, the local copy's mtime is set to the object'sLastModifiedviaos.utime. Without this, a fresh download would carry "now" as its mtime, and the nextsync()would re-upload the whole tree it had just pulled. If stamping fails, a warning is logged and the download still succeeds.README: the change-detection bullet now says "size/presence/timestamp".
Because every path goes through
_compute_sync_diff, the behavior applies todownload,upload,sync, interval uploads,plan_download/plan_upload, and the CLI--dry-run.Why a 2 s tolerance
S3
LastModifiedhas whole-second resolution, while local filesystems keep sub-second mtimes. Right after an upload, the local file can look a fraction of a second "newer" than its S3 copy. The margin stops a freshly synced tree from re-transferring on every tick. The comparison is strict: a source ahead by exactly the tolerance does not count as newer.Release (v0.3.2)
pyproject.tomlanduv.lockbumped to0.3.2. The lock was also stale at0.3.0.CHANGELOG.md: the[Unreleased]section became[0.3.2] - 2026-09-10. The documented tolerance was corrected from 1 s to 2 s to match the constant.Files downloaded by pos3 ≤ 0.3.1 have their download time as mtime, which is newer than their S3
LastModified. The firstupload()/sync()of such a tree after upgrading will re-upload it once. After that, syncs are incremental again. This is also in the changelog.Tests
TestMtimeComparisonintests/test_s3.pycovers:_scan_localand_scan_s3;sync()not re-uploading what it just downloaded;plan_uploadreporting a same-size newer file.Tolerance-sensitive tests are written in terms of
_MTIME_TOLERANCE_SECONDS, so re-tuning the constant doesn't break them.After merge
Publish a GitHub release tagged
v0.3.2frommain.release.ymlbuilds and pushes to PyPI, and it checks that the tag matchesproject.version.🤖 Generated with Claude Code