Cordy/cairn-desktop

Fork 0

Task 12: The engine cycle #12

New issue

Closed

opened 2026-09-10 17:16:49 +00:00 by Cordy · 1 comment

Cordy commented

2026-09-10 17:16:49 +00:00

Owner

Depends on Task 11. Wires everything into one pass.

Files

Create: internal/sync/engine.go, internal/sync/engine_test.go

Produces

type Engine struct {
	FS       vfs.FS
	Remote   remote.Remote
	State    *state.Store
	Now      func() time.Time // injected so conflict names are deterministic in tests
	Platform Platform
}

type Skip struct {
	Path   string
	Reason string
}

type Result struct {
	Applied   []Operation
	Skipped   []Skip
	Conflicts []string
}

func (e *Engine) SyncOnce() (Result, error)

The cycle

FS.Walk() and Remote.List(); load State.All().
Build the union of all known paths (local ∪ remote ∪ state).
Per path: skip illegal names (Task 4), skip directories, build Side values, call Decide.
DetectRenames over the resulting operations (Task 11).
Order them (Task 8).
Execute each, recording state only after the operation is confirmed.

Rules

An error on one path is a Skip, not a returned error. Only a failure that makes further progress impossible (a failed Walk or List) returns an error. One bad file must not stop the folder.
record() runs after confirmation, so a crash mid-sync resumes rather than corrupts.
Conflict handling: copy the local file to ConflictName(path, e.Now()), download the remote to the canonical path, upload the conflict copy, and append to Result.Conflicts. Both versions survive.

Steps

Write the failing tests:
- new local file → uploaded
- new remote file → downloaded
- second SyncOnce is a no-op (len(Applied) == 0) — proves idempotence
- local delete → propagates to remote
- divergent edits → one conflict, remote version at the canonical path, local version at the conflict name
- an illegal name on Windows → Skipped, and no error returned

func TestSyncConflictKeepsBothVersions(t *testing.T) {
	e, fs, rem := newEngine(t)
	fs.Write("a.txt", strings.NewReader("original"), time.Now())
	e.SyncOnce()

	fs.Write("a.txt", strings.NewReader("local edit"), time.Now())
	rem.Put("a.txt", strings.NewReader("remote edit"), time.Now())

	res, err := e.SyncOnce()
	if err != nil {
		t.Fatal(err)
	}
	if len(res.Conflicts) != 1 {
		t.Fatalf("expected 1 conflict, got %+v", res.Conflicts)
	}
	if got := readLocal(t, fs, "a.txt"); got != "remote edit" {
		t.Errorf("canonical path should hold the remote version, got %q", got)
	}
	if got := readLocal(t, fs, ConflictName("a.txt", e.Now())); got != "local edit" {
		t.Errorf("conflict copy should hold the local version, got %q", got)
	}
}

Write a newEngine(t) helper returning (*Engine, *vfs.MemFS, *remote.MemRemote) with Now pinned to a fixed time and an in-memory state store.

Run them, confirm they fail.
Implement. Note: computing the remote hash by downloading content on every pass is correct but naive — that is fine here. Plan 2 replaces it with ETag-first comparison. Do not optimise it now.
Run tests, confirm they pass — including the arch guard, which must still be green.
Commit: git commit -s -m "feat(sync): engine cycle — scan, decide, order, execute, record"

Acceptance criteria

All six behaviours above have a named test.
A second consecutive SyncOnce applies zero operations.
SyncOnce returns an error only when Walk or List fails.
internal/sync still imports nothing forbidden.

Depends on Task 11. Wires everything into one pass. ## Files - Create: `internal/sync/engine.go`, `internal/sync/engine_test.go` ## Produces ```go type Engine struct { FS vfs.FS Remote remote.Remote State *state.Store Now func() time.Time // injected so conflict names are deterministic in tests Platform Platform } type Skip struct { Path string Reason string } type Result struct { Applied []Operation Skipped []Skip Conflicts []string } func (e *Engine) SyncOnce() (Result, error) ``` ## The cycle 1. `FS.Walk()` and `Remote.List()`; load `State.All()`. 2. Build the union of all known paths (local ∪ remote ∪ state). 3. Per path: skip illegal names (Task 4), skip directories, build `Side` values, call `Decide`. 4. `DetectRenames` over the resulting operations (Task 11). 5. `Order` them (Task 8). 6. Execute each, recording state **only after** the operation is confirmed. ## Rules - **An error on one path is a `Skip`, not a returned error.** Only a failure that makes further progress impossible (a failed `Walk` or `List`) returns an error. One bad file must not stop the folder. - **`record()` runs after confirmation**, so a crash mid-sync resumes rather than corrupts. - **Conflict handling:** copy the local file to `ConflictName(path, e.Now())`, download the remote to the canonical path, upload the conflict copy, and append to `Result.Conflicts`. Both versions survive. ## Steps - [ ] **Write the failing tests:** - new local file → uploaded - new remote file → downloaded - **second `SyncOnce` is a no-op** (`len(Applied) == 0`) — proves idempotence - local delete → propagates to remote - divergent edits → one conflict, remote version at the canonical path, local version at the conflict name - an illegal name on Windows → `Skipped`, and **no error returned** ```go func TestSyncConflictKeepsBothVersions(t *testing.T) { e, fs, rem := newEngine(t) fs.Write("a.txt", strings.NewReader("original"), time.Now()) e.SyncOnce() fs.Write("a.txt", strings.NewReader("local edit"), time.Now()) rem.Put("a.txt", strings.NewReader("remote edit"), time.Now()) res, err := e.SyncOnce() if err != nil { t.Fatal(err) } if len(res.Conflicts) != 1 { t.Fatalf("expected 1 conflict, got %+v", res.Conflicts) } if got := readLocal(t, fs, "a.txt"); got != "remote edit" { t.Errorf("canonical path should hold the remote version, got %q", got) } if got := readLocal(t, fs, ConflictName("a.txt", e.Now())); got != "local edit" { t.Errorf("conflict copy should hold the local version, got %q", got) } } ``` Write a `newEngine(t)` helper returning `(*Engine, *vfs.MemFS, *remote.MemRemote)` with `Now` pinned to a fixed time and an in-memory state store. - [ ] **Run them, confirm they fail.** - [ ] **Implement.** Note: computing the remote hash by downloading content on every pass is correct but naive — that is fine here. Plan 2 replaces it with ETag-first comparison. Do not optimise it now. - [ ] **Run tests, confirm they pass** — including the arch guard, which must still be green. - [ ] **Commit:** `git commit -s -m "feat(sync): engine cycle — scan, decide, order, execute, record"` ## Acceptance criteria - All six behaviours above have a named test. - A second consecutive `SyncOnce` applies zero operations. - `SyncOnce` returns an error only when `Walk` or `List` fails. - `internal/sync` still imports nothing forbidden.

Cordy added this to the phase-1-engine milestone

2026-09-10 17:16:49 +00:00

Cordy added the

data-integrity

engine

labels

2026-09-10 17:16:49 +00:00

Cordy referenced this issue

2026-09-10 20:37:25 +00:00

Task 4: Platform-illegal filename detection #4

Cordy referenced this issue

2026-09-10 22:23:12 +00:00

Task 8: The decision function — three-way truth table #8

Cordy referenced this issue

2026-09-10 23:04:08 +00:00

Task 11: Rename detection via FileID #11

Cordy commented

2026-09-10 23:47:01 +00:00

Author

Owner

Done

What was built

internal/sync/engine.go: Engine/Skip/Result/SyncOnce running Walk+List+State.All → union of paths → per-path Decide → DetectRenames → Order → execute-then-record.
One SHA-256/hex hash function for both sides; naive per-pass remote hashing as the issue specifies.
Conflict handling (copy local → ConflictName, download remote to canonical, upload copy, record in Result.Conflicts) with collision numbering that never overwrites an existing local/remote/state path.
Move execution per Task 11 F4, fixed in round 1 to record the scanned pre-move metadata rather than a post-move Stat (closes a window where a concurrent write could be hidden).

Tests

16 engine tests including the 6 named behaviours, TestSyncSecondPassIsNoOp, TestSyncConflictCopyNeverOverwritesExistingPath, TestSyncMoveNeverHidesAConcurrentWrite, TestSyncReturnsErrorWhenScanFails (walk/list/state), plus the arch guard.
Mutation probes A–I (report) all fail exactly the test they target.
CI run #12 on linux/arm64 — green (go vet + go test -count=1 ./..., all 4 packages ok).

Acceptance criteria

All six behaviours have a named test — met (see Tests).
A second consecutive SyncOnce applies zero operations — met, TestSyncSecondPassIsNoOp.
SyncOnce returns an error only when Walk or List fails — met as read (State.All failure also errors, since without state rows every path degrades to a two-way compare); now covered by a state subtest.
internal/sync imports nothing forbidden — met, TestEngineHasNoForbiddenImports green.

Rulings

R2: MemFS/MemRemote wrap io/fs.ErrNotExist for a missing path; the engine treats it on delete as already done.
R3: one SHA-256/hex hash function for both sides; an ETag is compared only with a stored ETag, never a content hash.
R4: a conflict copy never overwrites an existing local/remote/state path; a taken ConflictName gets a number inside the parenthetical.
R12: commit subjects are the issue's exact text, git commit -s, with the Co-Authored-By trailer.
F3 (fix round 1): settleMove records the prior row's remote metadata — what the scan proved before the MOVE — never a post-move Remote.Stat, so a concurrent write is never hidden as "synced."
CV4: WebDAV Move/settleMove semantics (overwrite-onto-existing, ETag stability across MOVE, missing parent collections) ruled per-semantic; binding on #23 that Client.Move creates missing ancestor collections before sending MOVE.
CV5: #13's convergence/no-byte-loss property tests must include local renames in the operation mix; #12 already satisfies the Task 9 F2 execute-every-op carry-forward.

Deferred

engine.go:487 — settleMove's Put-failure branch drops the Remote.Stat error/not-found result from the Skip reason.
engine.go:361-363 — a failed download inside a conflict leaves the copy unreported in Result.Conflicts; next pass re-conflicts it.
engine_test.go:410 — coverage gaps on rare data-integrity branches (state-only R4 row, failed conflict-copy upload).
engine.go:396 — nil Engine.Now panics on the first conflict; default to time.Now or document the requirement.
engine.go:311-314 — the Stat/not-found-to-ErrNotExist idiom is duplicated three times; a small helper would remove the drift risk.
F1 spec reading — the controller still has to ratify that "error only on Walk/List" also covers a failed State.All.

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

**Done** - [feat(sync): engine cycle — scan, decide, order, execute, record](http://192.168.10.245/Cordy/cairn-desktop/commit/53b17240630e5e825cc5974c38f13cd5b9ebd97a) - [fix(sync): record the scanned remote metadata after a move, not a post-move Stat](http://192.168.10.245/Cordy/cairn-desktop/commit/37799e717532b177f5a07d00e9892204278ce6ee) - [test(sync): SyncOnce returns an error when the state store cannot be loaded](http://192.168.10.245/Cordy/cairn-desktop/commit/412d5177f02e033043601d8ce1810119ac418110) **What was built** - `internal/sync/engine.go`: `Engine`/`Skip`/`Result`/`SyncOnce` running Walk+List+State.All → union of paths → per-path Decide → DetectRenames → Order → execute-then-record. - One SHA-256/hex hash function for both sides; naive per-pass remote hashing as the issue specifies. - Conflict handling (copy local → ConflictName, download remote to canonical, upload copy, record in `Result.Conflicts`) with collision numbering that never overwrites an existing local/remote/state path. - Move execution per Task 11 F4, fixed in round 1 to record the scanned pre-move metadata rather than a post-move `Stat` (closes a window where a concurrent write could be hidden). **Tests** - 16 engine tests including the 6 named behaviours, `TestSyncSecondPassIsNoOp`, `TestSyncConflictCopyNeverOverwritesExistingPath`, `TestSyncMoveNeverHidesAConcurrentWrite`, `TestSyncReturnsErrorWhenScanFails` (walk/list/state), plus the arch guard. - Mutation probes A–I (report) all fail exactly the test they target. - CI run [#12](http://192.168.10.245/Cordy/cairn-desktop/actions/runs/12) on `linux/arm64` — green (`go vet` + `go test -count=1 ./...`, all 4 packages ok). **Acceptance criteria** - All six behaviours have a named test — met (see Tests). - A second consecutive `SyncOnce` applies zero operations — met, `TestSyncSecondPassIsNoOp`. - `SyncOnce` returns an error only when `Walk` or `List` fails — met as read (State.All failure also errors, since without state rows every path degrades to a two-way compare); now covered by a `state` subtest. - `internal/sync` imports nothing forbidden — met, `TestEngineHasNoForbiddenImports` green. **Rulings** - R2: MemFS/MemRemote wrap `io/fs.ErrNotExist` for a missing path; the engine treats it on delete as already done. - R3: one SHA-256/hex hash function for both sides; an ETag is compared only with a stored ETag, never a content hash. - R4: a conflict copy never overwrites an existing local/remote/state path; a taken `ConflictName` gets a number inside the parenthetical. - R12: commit subjects are the issue's exact text, `git commit -s`, with the `Co-Authored-By` trailer. - F3 (fix round 1): `settleMove` records the prior row's remote metadata — what the scan proved before the MOVE — never a post-move `Remote.Stat`, so a concurrent write is never hidden as "synced." - CV4: WebDAV Move/settleMove semantics (overwrite-onto-existing, ETag stability across MOVE, missing parent collections) ruled per-semantic; binding on #23 that `Client.Move` creates missing ancestor collections before sending MOVE. - CV5: #13's convergence/no-byte-loss property tests must include local renames in the operation mix; #12 already satisfies the Task 9 F2 execute-every-op carry-forward. **Deferred** - `engine.go:487` — settleMove's Put-failure branch drops the `Remote.Stat` error/not-found result from the Skip reason. - `engine.go:361-363` — a failed download inside a conflict leaves the copy unreported in `Result.Conflicts`; next pass re-conflicts it. - `engine_test.go:410` — coverage gaps on rare data-integrity branches (state-only R4 row, failed conflict-copy upload). - `engine.go:396` — nil `Engine.Now` panics on the first conflict; default to `time.Now` or document the requirement. - `engine.go:311-314` — the Stat/not-found-to-ErrNotExist idiom is duplicated three times; a small helper would remove the drift risk. - F1 spec reading — the controller still has to ratify that "error only on Walk/List" also covers a failed `State.All`. _Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI._

Cordy closed this issue

2026-09-10 23:47:06 +00:00