Guard snapshot hash reuse against unsettled mtime buckets

[in-jun]

The scan hash-reuse shortcut trusted a cached hash on any exact size+mtime match, with no allowance for filesystem mtime granularity. On coarse-mtime backends (FAT SD cards at 2 s, SMB servers that truncate to whole seconds) two same-size edits can share one mtime bucket, so the second edit keeps the stale hash, gets classified UNCHANGED, and never propagates. That path was also untested: the fake honored the hint but nothing pinned the reuse condition or asserted reuse actually happens.

• SnapshotBuilder now reuses a hint hash only once the entry's mtime has settled past a granularity window of the scan clock; nearer than that (or against an unknown clock, or a future-dated mtime) it re-hashes. Local scans pass device time; remote scans pass server time and disable reuse when the server clock is unknown rather than trust a device clock that may run ahead.
• Reuse also requires the hint itself to have been recorded after its own mtime settled. A same-size edit that lands in the bucket after a racy hint was written looks settled to every later scan, so a per-side racy flag rides on the recorded stat (git's racily-clean idea), persisted in the ancestor table via migration 2->3, and a flagged hint is re-hashed once before it is ever trusted.
• Add SnapshotBuilderTest pinning reuse vs. re-hash across settled/racy/size/mtime/absent-hint/unknown-clock/future-mtime cases, and give the in-memory fake a scan hash counter so SyncExecutorTest asserts an unchanged file is served from the hint while a changed one is re-hashed.

Fixes #51
Fixes #44

[in-jun]

Thanks for taking this on — the #44 side is fully covered and the surrounding safety choices are right. But I don't think the #51 path is actually closed, and I'd like to sort that out before we merge.

The guard measures now - mtime > granularity against the current scan clock. The miss #51 describes, though, is a same-size edit that lands in the mtime bucket after the hint was recorded. Once that stale (size, mtime, hash) is committed to the ancestor, every later scan sees the mtime comfortably in the past, reads "settled", and reuses the stale hash — permanently, which is exactly the failure the issue warns about.

Concrete timeline (2 s bucket):

• 12:00:00.4 file rewritten to C1 (size S, mtime truncates to 12:00:00)
• 12:00:00.6 a sync scans, hashes C1, commits ancestor (S, 12:00:00, H1)
• 12:00:01.3 file rewritten to C2, same size S, mtime still 12:00:00
• 12:15:00 next sync: now - mtime is ~15 min > 2 s → settled → size+mtime match → reuse H1. C2 never propagates, and stays that way until a later edit happens to shift size or mtime.

The granularity window that matters is the one around when the hint was captured, not the one around the current scan. Anchoring to now only catches the case where the recovering scan itself falls inside the window (a prompt re-scan right after the racy edit); a scan a window or more later — the normal cadence, and the one that leaves the loss permanent — reads the same stale pair as settled. The doc comment's framing ("a same-size edit could still land unseen in the same bucket") is forward-looking from now, but the edit that gets missed is one that already landed since the hint was written.

The usual way to close this fully is to anchor the check to the hint's own capture time (git's racy-clean compares the entry mtime against the index/snapshot timestamp, not the wall clock): record the scan time alongside the ancestor stat and treat a hash as reusable only once its recorded mtime was already older than that capture time by the granularity — equivalently, mark any entry scanned while its mtime is within the window of that scan as non-reusable, so the following pass re-hashes it once. Either keeps a single durable timestamp per record and closes the persistent-stale case.

Everything else looks good to me: the fake now honors the hint with a hash counter, SnapshotBuilderTest pins the reuse/re-hash matrix cleanly, the executor test asserts reuse on an unchanged file, and disabling reuse on an unknown server clock plus re-hashing future-dated mtimes are both the safe direction. No regressions or scope creep — the remaining gap is just the anchor point of the settle check.

[in-jun]

Good catch on the #51 path — you're right that the now-anchored guard only recovers the race when the recovering scan itself falls inside the window, which the normal cadence never does. I reproduced it against build() exactly as described: once the stale (size, mtime, H1) is committed, later scans read the mtime as long-settled and reuse H1 permanently.

Fixed by anchoring the second half of the reuse test to the hint's own capture rather than the current clock (git's racily-clean approach):

• FileMeta now carries a per-side racyMtime flag. A stat observed while its mtime was still inside the granularity window is flagged; a flagged hint is never reused on a stat match, so it is re-hashed once and only then (bucket settled) becomes reusable.
• SnapshotBuilder.build flags each output entry when its mtime is within the window at scan time, and reuse now requires: stat match, settled at this scan, and the hint settled at its own capture (!prev.racyMtime). The scan clock alone only proves the bucket is closed for edits made before this scan, never for one made during the window the hint was born in.
• The flag persists per side in the ancestor table (migration 2->3). Existing rows default to settled to avoid a full re-hash on upgrade; every row written from here on carries the real flag.

Added a SnapshotBuilderTest case pinning the exact miss — a stat that looks settled against the clock but whose hint was recorded racy re-hashes — plus one asserting the output flag is set within the window and cleared once settled. writtenMeta stays reusable (unflagged), so the #44 reuse assertions in SyncExecutorTest still hold and pushed files aren't needlessly re-hashed. Full core-sync suite and the app unit tests pass.

[in-jun]

The rework closes the path I flagged before, and the coverage around it is convincing. The persisted racyMtime flag survives from the scan that recorded a hint into the next scan that reads it back, so the miss where a same-size edit lands in the mtime bucket after a hint was committed is now caught: SnapshotBuilder.build gates reuse on !prev.racyMtime in addition to the settle window, MIGRATION_2_3 carries the flag on both sides, and SnapshotBuilderTest pins exactly that case ("re-hashes a settled stat match when the hint was recorded while racy"). For files committed through the scan path — steady-state Converged, the local side of a push, the remote side of a pull — the flag rides the scanned FileMeta all the way to the ancestor. That part is solid.

One gap remains, on the write-commit path. writtenMeta (SyncExecutor.kt:211) builds the destination-side meta as FileMeta(written.size, written.mtimeMillis, source.contentHash), leaving racyMtime at its default of false. That value is the freshly-persisted stamp — writeAtomic returns tmp.lastModified() locally and info.lastWriteTime over SMB — so on a coarse-mtime backend it sits inside the granularity window at the moment we write it, yet it's recorded as settled. writtenMeta is the destination commit for every transfer: the remote side of a push (line 99), the local side of a pull (104), and all four conflict writes (153, 154, 164, 172). So a just-written file's destination side reverts to the now-anchored-only guard that the racy flag was introduced to replace, which reopens the same #51 window there.

Concrete repro (2s bucket, remote side of a push):

1. 12:00:00.6 — push writes the remote file; SMB truncates its mtime to 12:00:00; committed as (S, 12:00:00, H1, racy=false).
2. 12:00:01.3 — another writer on the server rewrites that file to C2, same size, mtime still 12:00:00.
3. 12:15:00 — next sync: the remote scan sees mtime settled (~15 min > 2s), size+mtime match the hint, and prev.racyMtime is false, so H1 is reused and C2 never propagates.

refreshHints can't heal it for the same reason as before: the reused hash equals the record hash, so l.contentHash == hash and it skips. The only scan that recovers is one that happens to fall inside the window right after the racy write — exactly the fragility the flag was meant to remove.

The fix mirrors what the scan path already does: writtenMeta should flag the destination racy when the write's mtime is within the window. Threading the scan clock and granularity through and computing it the way SnapshotBuilder does is the faithful option; since a freshly-written file is by definition freshly stamped, unconditionally setting racyMtime = true there is a simpler equivalent — it costs one re-hash on the next settled scan and then clears. Either way, a small executor-level test that pushes a file and then asserts a same-size edit in the same bucket is re-hashed on the next pass would pin it; SnapshotBuilderTest exercises the builder in isolation, which is why this slipped through.

[in-jun]

Reviewed against #51 and #44. This correctly closes the miss and the test gap, and I built both modules plus ran the core-sync suite green.

The fix holds where it matters. The settle guard is anchored to the hint's own capture rather than the current scan clock, which is the subtle part: a same-size edit that lands in the bucket after a racy hint was recorded looks settled to every later scan, so without the persisted per-side flag its stale hash would be reused forever. The racyMtime flag threads cleanly from SnapshotBuilder through the scan snapshot into the Push/Pull actions and the committed ancestor, so the flagship in-place same-size rewrite is re-hashed on the next pass. Disabling reuse entirely when the SMB offset is unknown (rather than trusting a device clock that may run ahead of the server) is the right call.

No reconciliation regression: racyMtime joins FileMeta's equals, but classification still keys on sameContentAs (hash only), so the added field can't perturb the engine. refreshHints now also re-records a flipped flag, which is cheap and self-healing. Migration 2->3 matches the entity column definition and is wired into DatabaseModule; defaulting existing rows to settled is a reasonable one-time upgrade tradeoff and it's documented in the migration.

The tests are the right ones — SnapshotBuilderTest pins reuse vs. re-hash across settled/racy/size/mtime/absent/unknown-clock/future-mtime, and the counter on the fake lets SyncExecutorTest actually assert an unchanged file is served from the hint while a changed one is re-hashed. That's exactly the pathway #44 flagged as untested.

One narrow follow-up, not blocking: writtenMeta records the just-written destination side with racyMtime = false even though that stat's mtime is fresh. A concurrent external same-size edit that lands in that bucket could still be trusted on a later settled scan. It's a genuine multi-writer edge rather than the in-place-rewrite case #51 targets, and it's strictly better than the prior no-guard behavior, so I'd leave it for a separate change if it's worth chasing at all.

Looks good to me."