Cordy/cairn-desktop

Fork 0

P2-6: ETag-first change detection and lazy hashing #26

New issue

Closed

opened 2026-09-10 17:42:02 +00:00 by Cordy · 3 comments

Cordy commented

2026-09-10 17:42:02 +00:00

Owner

Depends on P2-3 and P2-5.

The problem this fixes

Task 12 built the engine to hash both sides on every pass. For the remote, that means downloading every file on every sync — correct, but unusable against a real server.

Goal

Decide what changed using cheap signals, and only hash when the cheap signals are ambiguous — without weakening the guarantee that ambiguity never causes a delete.

Files

Modify: internal/sync/engine.go
Create: internal/sync/detect.go, internal/sync/detect_test.go

The rules

Remote side — ETag is authoritative. The Remote contract (Task 6) guarantees the ETag changes whenever content changes. So:

remote.ETag == last.RemoteETag → unchanged. Do not download. Do not hash. Leave Side.Hash empty.
ETag differs → changed. Still do not hash eagerly; a changed remote generally means download, which reveals the content anyway.

Local side — mtime plus size is the cheap signal.

mtime == last.LocalMtime && size == last.LocalSize → assume unchanged, reuse last.ContentHash. No read.
Either differs → hash the file to find out whether content genuinely changed. mtime alone is not trustworthy: clock skew, restored backups and some editors rewrite mtime without changing content, and the reverse (same mtime, changed content) happens with fast successive edits.

Hash only when the decision needs it. Decide needs a hash on both sides only to distinguish "converged independently" from "genuine conflict" — the both-changed case. Compute lazily at that point, not up front.

The trap to avoid

Do not let this optimisation reintroduce a delete-on-ambiguity path. sameContent still requires both hashes to be known and equal. If a hash is unavailable, the outcome must be a conflict copy or a transfer — never a delete. Task 8's TestDecideNeverDeletesOnUnknownHash must still pass untouched.

Steps

Write failing tests:
- unchanged ETag ⇒ zero calls to Remote.Get during a pass (instrument a counting Remote wrapper)
- unchanged mtime+size ⇒ zero local file reads
- changed mtime but identical content ⇒ resolves to OpUpdateState, not an upload
- both sides changed ⇒ hashes are computed and a conflict is correctly identified
- a whole no-op pass over 100 files performs zero content reads on either side
Run; confirm failure.
Implement.
Re-run the entire phase-1 suite, especially TestConvergence. It must still pass with 50 seeds. If it does not, the optimisation is wrong — fix it, do not weaken the test.
Commit: git commit -s -m "perf(sync): ETag-first change detection with lazy hashing"

Acceptance criteria

A no-op sync over an unchanged tree reads no file content, locally or remotely.
TestConvergence still passes at 50 seeds.
TestDecideNeverDeletesOnUnknownHash is unmodified and passing.

Depends on P2-3 and P2-5. ## The problem this fixes Task 12 built the engine to hash **both** sides on every pass. For the remote, that means **downloading every file on every sync** — correct, but unusable against a real server. ## Goal Decide what changed using cheap signals, and only hash when the cheap signals are ambiguous — without weakening the guarantee that ambiguity never causes a delete. ## Files - Modify: `internal/sync/engine.go` - Create: `internal/sync/detect.go`, `internal/sync/detect_test.go` ## The rules **Remote side — ETag is authoritative.** The `Remote` contract (Task 6) guarantees the ETag changes whenever content changes. So: - `remote.ETag == last.RemoteETag` → unchanged. Do not download. Do not hash. Leave `Side.Hash` empty. - ETag differs → changed. Still do not hash eagerly; a changed remote generally means download, which reveals the content anyway. **Local side — mtime plus size is the cheap signal.** - `mtime == last.LocalMtime && size == last.LocalSize` → assume unchanged, reuse `last.ContentHash`. No read. - Either differs → hash the file to find out whether content genuinely changed. mtime alone is not trustworthy: clock skew, restored backups and some editors rewrite mtime without changing content, and the reverse (same mtime, changed content) happens with fast successive edits. **Hash only when the decision needs it.** `Decide` needs a hash on both sides only to distinguish "converged independently" from "genuine conflict" — the both-changed case. Compute lazily at that point, not up front. ## The trap to avoid Do **not** let this optimisation reintroduce a delete-on-ambiguity path. `sameContent` still requires both hashes to be **known and equal**. If a hash is unavailable, the outcome must be a conflict copy or a transfer — never a delete. Task 8's `TestDecideNeverDeletesOnUnknownHash` must still pass untouched. ## Steps - [ ] Write failing tests: - unchanged ETag ⇒ zero calls to `Remote.Get` during a pass (instrument a counting `Remote` wrapper) - unchanged mtime+size ⇒ zero local file reads - changed mtime but identical content ⇒ resolves to `OpUpdateState`, not an upload - both sides changed ⇒ hashes are computed and a conflict is correctly identified - a whole no-op pass over 100 files performs **zero** content reads on either side - [ ] Run; confirm failure. - [ ] Implement. - [ ] **Re-run the entire phase-1 suite, especially `TestConvergence`.** It must still pass with 50 seeds. If it does not, the optimisation is wrong — fix it, do not weaken the test. - [ ] Commit: `git commit -s -m "perf(sync): ETag-first change detection with lazy hashing"` ## Acceptance criteria - A no-op sync over an unchanged tree reads no file content, locally or remotely. - `TestConvergence` still passes at 50 seeds. - `TestDecideNeverDeletesOnUnknownHash` is unmodified and passing.

Cordy added this to the phase-2-integration milestone

2026-09-10 17:42:02 +00:00

Cordy added the

engine

data-integrity

labels

2026-09-10 17:42:02 +00:00

Cordy commented

2026-09-10 18:08:15 +00:00

Author

Owner

Amendment — 2026-09-10: files-on-demand is now in scope

Virtual files moved to a planned phase (phase-4-virtual-files). This task is now load-bearing
for that feature, not merely a performance optimisation.

The rule to add

A placeholder can never have been modified locally. You cannot edit a file whose bytes are
not on disk — the OS hydrates it first, which makes it a normal file. So:

if !local.Hydrated {
    // Placeholder: content is authoritative on the server.
    // Treat as locally unchanged. Never hash it.
    side.Hash = last.ContentHash   // i.e. "unchanged"
}

Three consequences, each worth a test:

Never hash a file with Hydrated == false. Hashing forces a download. A sync pass that
hashes every placeholder hydrates the entire tree — the exact opposite of what
files-on-demand is for. This is the single most destructive mistake available in this task.
Never let a placeholder produce an upload. If its metadata looks odd, treat it as
unchanged rather than uploading content you would have to fetch first.
A remote change still wins normally. ETag differs → download → the file becomes
hydrated, or the placeholder is simply updated in place, depending on the provider.

Why this lands here rather than in phase 4

Decide (Task 8) stays pure and unchanged — the caller decides what Side.Hash to present.
That is exactly the lazy-hashing responsibility this issue already owns for the
mtime-and-size-unchanged case. Placeholders are the same pattern with a different signal.

Keeping the branch here means phase 4 adds OS integration only, with no changes to the engine's
decision logic. That is what makes the phase-4 work bounded.

Also

Add the placeholder case to this issue's test list: a sync pass over a tree of placeholders
must perform zero content reads and produce zero operations.

## Amendment — 2026-09-10: files-on-demand is now in scope Virtual files moved to a planned phase (`phase-4-virtual-files`). **This task is now load-bearing for that feature, not merely a performance optimisation.** ### The rule to add A **placeholder can never have been modified locally.** You cannot edit a file whose bytes are not on disk — the OS hydrates it first, which makes it a normal file. So: ``` if !local.Hydrated { // Placeholder: content is authoritative on the server. // Treat as locally unchanged. Never hash it. side.Hash = last.ContentHash // i.e. "unchanged" } ``` Three consequences, each worth a test: 1. **Never hash a file with `Hydrated == false`.** Hashing forces a download. A sync pass that hashes every placeholder hydrates the entire tree — the exact opposite of what files-on-demand is for. This is the single most destructive mistake available in this task. 2. **Never let a placeholder produce an upload.** If its metadata looks odd, treat it as unchanged rather than uploading content you would have to fetch first. 3. **A remote change still wins normally.** ETag differs → download → the file becomes hydrated, or the placeholder is simply updated in place, depending on the provider. ### Why this lands here rather than in phase 4 `Decide` (Task 8) stays pure and unchanged — the caller decides what `Side.Hash` to present. That is exactly the lazy-hashing responsibility this issue already owns for the mtime-and-size-unchanged case. Placeholders are the same pattern with a different signal. Keeping the branch here means phase 4 adds OS integration only, with no changes to the engine's decision logic. That is what makes the phase-4 work bounded. ### Also Add the placeholder case to this issue's test list: a sync pass over a tree of placeholders must perform **zero** content reads and produce **zero** operations.

Cordy referenced this issue

2026-09-10 18:08:49 +00:00

P4-1: Placeholder abstraction and pin model (no OS code) #44

Cordy commented

2026-09-11 01:50:39 +00:00

Author

Owner

Note — 2026-09-11: where the placeholder rule lands (final review X5, carry-forward to #44)

The files-on-demand amendment above states the rule. This note records where phase 1 left the
code, so the rule lands in the right place.

Engine.observe in internal/sync/engine.go is the change-detection layer that spec §4
refers to. Today it ignores vfs.FileInfo.Hydrated and hashes every local file. That is
harmless in phase 1, because MemFS always reports Hydrated: true.
The ETag-first rewrite of observe here must never open a file with Hydrated == false. It
presents such a file as locally unchanged by reusing the row's ContentHash.
Decide in this issue or in #44 what a placeholder with no state row presents as. It
should not arise, since a placeholder is created from a synced server entry, but it must not
be hashed either.
The re-check before destructive operations, added in commit
699ec0a (fix(sync): re-check a copy against the scan before replacing or removing it), compares a
local file's size, mtime and FileID only. It deliberately ignores Hydrated, because
hydration is not a modification (#31 amendment). Keep it that way.

## Note — 2026-09-11: where the placeholder rule lands (final review X5, carry-forward to #44) The files-on-demand amendment above states the rule. This note records where phase 1 left the code, so the rule lands in the right place. - `Engine.observe` in `internal/sync/engine.go` is the change-detection layer that spec §4 refers to. Today it ignores `vfs.FileInfo.Hydrated` and hashes every local file. That is harmless in phase 1, because `MemFS` always reports `Hydrated: true`. - The ETag-first rewrite of `observe` here must never open a file with `Hydrated == false`. It presents such a file as locally unchanged by reusing the row's `ContentHash`. - Decide in this issue or in #44 what a placeholder with **no** state row presents as. It should not arise, since a placeholder is created from a synced server entry, but it must not be hashed either. - The re-check before destructive operations, added in commit 699ec0a (`fix(sync): re-check a copy against the scan before replacing or removing it`), compares a local file's size, mtime and FileID only. It deliberately ignores `Hydrated`, because hydration is not a modification (#31 amendment). Keep it that way.

Cordy referenced this issue from a commit

2026-09-11 01:55:32 +00:00

docs: CLAUDE.md lists the phase-1 hand-off amendments on #22, #23, #25, #26

Cordy referenced this issue

2026-09-11 06:40:24 +00:00

P2-4: Resumable upload via tus #24

Cordy commented

2026-09-11 09:27:27 +00:00

Author

Owner

Done

602b80ac38 — perf(sync): ETag-first change detection with lazy hashing
81e9446ecb — fix(sync): set a row's HashedAt back 2 s so coarse mtimes never vouch for an edit

What was built

detect.go: ETag-first remote check (no Get when the ETag is unchanged) and an mtime+size local shortcut (no read when unchanged, reusing the row's hash); the server copy is hashed only on OpConflict.
Files-on-demand placeholder rule: a file with Hydrated == false is never opened or hashed; put/conflict refuse to upload one (Skip); a renamed placeholder becomes one MOVE instead of losing its server copy.
A touched-but-identical file resolves to OpUpdateState, never an upload; a racy re-read is refreshed silently so it isn't re-read forever.
Fix round 1: a 2 s mtimeSlack margin on HashedAt so FAT's 2 s truncation and coarse kernel clocks can't hide a same-size in-place edit; TestConvergence's body became a shared runner plus TestConvergenceOnATestClock, since the wall-clock property test alone vouched only 2 times in 1611 calls.

Tests

Full suite green: internal/remote, state, sync, vfs (CGO_ENABLED=0 go test ./..., plus -race locally per P2-R7).
TestConvergence at 50 seeds (500 once, clean); TestDecideNeverDeletesOnUnknownHash untouched; 11+ mutations killed (report's tables).
Independently reproduced by the controller off-repo: a live cairnd 5-pass smoke run, and the FAT32 data-loss repro plus its fix on a real FAT32 image (progress.md CV3/CV4).
CI: Forgejo Actions run #24 (id 2906) for 81e9446, linux/arm64 — green. Coverage total: 90.3%.

Acceptance criteria

No-op sync reads no content, either side → TestSyncNoOpPassReadsNothing (0 Open, 0 Get over 100 files).
TestConvergence passes at 50 seeds → yes (500 once).
TestDecideNeverDeletesOnUnknownHash unmodified and passing → yes, decide.go untouched.
Unchanged ETag ⇒ zero Get → TestSyncNeverDownloadsAServerCopyWhoseETagIsUnchanged.
Unchanged mtime+size ⇒ zero local reads → TestSyncNeverReadsALocalFileWhoseMtimeAndSizeAreUnchanged.
Changed mtime, same content ⇒ OpUpdateState, not upload → TestSyncTouchedFileWithTheSameContentIsAStateUpdate.
Both changed ⇒ hashed, conflict identified → TestSyncReadsBothCopiesOnlyWhereBothChanged.
Amendment: placeholder never hashed/opened/uploaded, remote change still wins → TestSyncNeverReadsAPlaceholder and its subtests; a tree of placeholders → zero reads, zero ops.

Rulings

P2-R1 (owner): push to main after a clean review; close only once every Actions run for the pushed commit is green; comment + close, never edit issue bodies; commit subject = the issue's message, -s, Co-Authored-By trailer.
Task 26 Ruling (CV4): the racy-second guard was reproduced as unsound on a real FAT32 volume; fixed with the 2 s mtimeSlack margin (review F1). Residual, for the record:

The mtime/size shortcut trusts a row only when its recorded mtime lies more than 2 s before the pass that read it. Untested: network shares whose clock lags this machine by more than ~2 s; explicit back-dating (touch -r, rsync -t --inplace); Windows NTFS timestamps of a file a writer still holds open (until #19).

Deferred

F3: no migration for hashed_at (state.go:54) — CREATE TABLE IF NOT EXISTS leaves an older table without the column.
F4: a renamed placeholder with an unknown FileID still loses its server copy (rename.go:44) — carried into #44.
F5: describes duplicates vouches' FileID check with plain == instead of knownEqual (detect.go:150).
TestConvergence itself now vouches 0/1578 times (its wall-clock writes fall inside the 2 s margin); the shortcut is exercised only by TestConvergenceOnATestClock, by design.

Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI.

**Done** - http://192.168.10.245/Cordy/cairn-desktop/commit/602b80ac38a89b73fc7021444591c234aee6a574 — perf(sync): ETag-first change detection with lazy hashing - http://192.168.10.245/Cordy/cairn-desktop/commit/81e9446ecbfcdd96158ddab48556762d97cf76c8 — fix(sync): set a row's HashedAt back 2 s so coarse mtimes never vouch for an edit **What was built** - `detect.go`: ETag-first remote check (no `Get` when the ETag is unchanged) and an mtime+size local shortcut (no read when unchanged, reusing the row's hash); the server copy is hashed only on `OpConflict`. - Files-on-demand placeholder rule: a file with `Hydrated == false` is never opened or hashed; `put`/`conflict` refuse to upload one (Skip); a renamed placeholder becomes one MOVE instead of losing its server copy. - A touched-but-identical file resolves to `OpUpdateState`, never an upload; a racy re-read is refreshed silently so it isn't re-read forever. - Fix round 1: a 2 s `mtimeSlack` margin on `HashedAt` so FAT's 2 s truncation and coarse kernel clocks can't hide a same-size in-place edit; `TestConvergence`'s body became a shared runner plus `TestConvergenceOnATestClock`, since the wall-clock property test alone vouched only 2 times in 1611 calls. **Tests** - Full suite green: `internal/remote`, `state`, `sync`, `vfs` (`CGO_ENABLED=0 go test ./...`, plus `-race` locally per P2-R7). - `TestConvergence` at 50 seeds (500 once, clean); `TestDecideNeverDeletesOnUnknownHash` untouched; 11+ mutations killed (report's tables). - Independently reproduced by the controller off-repo: a live cairnd 5-pass smoke run, and the FAT32 data-loss repro plus its fix on a real FAT32 image (progress.md CV3/CV4). - CI: Forgejo Actions run **#24** (id 2906) for `81e9446`, linux/arm64 — green. Coverage `total: 90.3%`. **Acceptance criteria** - No-op sync reads no content, either side → `TestSyncNoOpPassReadsNothing` (0 Open, 0 Get over 100 files). - `TestConvergence` passes at 50 seeds → yes (500 once). - `TestDecideNeverDeletesOnUnknownHash` unmodified and passing → yes, `decide.go` untouched. - Unchanged ETag ⇒ zero `Get` → `TestSyncNeverDownloadsAServerCopyWhoseETagIsUnchanged`. - Unchanged mtime+size ⇒ zero local reads → `TestSyncNeverReadsALocalFileWhoseMtimeAndSizeAreUnchanged`. - Changed mtime, same content ⇒ `OpUpdateState`, not upload → `TestSyncTouchedFileWithTheSameContentIsAStateUpdate`. - Both changed ⇒ hashed, conflict identified → `TestSyncReadsBothCopiesOnlyWhereBothChanged`. - Amendment: placeholder never hashed/opened/uploaded, remote change still wins → `TestSyncNeverReadsAPlaceholder` and its subtests; a tree of placeholders → zero reads, zero ops. **Rulings** - P2-R1 (owner): push to main after a clean review; close only once every Actions run for the pushed commit is green; comment + close, never edit issue bodies; commit subject = the issue's message, `-s`, Co-Authored-By trailer. - Task 26 Ruling (CV4): the racy-second guard was reproduced as unsound on a real FAT32 volume; fixed with the 2 s `mtimeSlack` margin (review F1). Residual, for the record: > The mtime/size shortcut trusts a row only when its recorded mtime lies more than 2 s before the pass that read it. Untested: network shares whose clock lags this machine by more than ~2 s; explicit back-dating (`touch -r`, `rsync -t --inplace`); Windows NTFS timestamps of a file a writer still holds open (until #19). **Deferred** - F3: no migration for `hashed_at` (`state.go:54`) — `CREATE TABLE IF NOT EXISTS` leaves an older table without the column. - F4: a renamed placeholder with an unknown FileID still loses its server copy (`rename.go:44`) — carried into #44. - F5: `describes` duplicates `vouches`' FileID check with plain `==` instead of `knownEqual` (`detect.go:150`). - `TestConvergence` itself now vouches 0/1578 times (its wall-clock writes fall inside the 2 s margin); the shortcut is exercised only by `TestConvergenceOnATestClock`, by design. _Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI._

Cordy closed this issue

2026-09-11 09:27:31 +00:00

Cordy referenced this issue

2026-09-11 12:02:31 +00:00

P2-8: cmd/cairnsync — headless CLI #28

Cordy referenced this issue from a commit

2026-09-11 20:45:36 +00:00

fix(sync): read a vouched local file before deleting or replacing it

Cordy referenced this issue

2026-09-11 20:59:05 +00:00

P4-1: Placeholder abstraction and pin model (no OS code) #44