Cordy/cairn-desktop

Fork 0

P2-7: Transfer concurrency, retry with backoff, partial-write safety #27

New issue

Closed

opened 2026-09-10 17:42:20 +00:00 by Cordy · 1 comment

Cordy commented

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

Owner

Depends on P2-6.

Goal

Make execution robust and reasonably fast without turning a flaky network into corruption or a denial-of-service against the user's own server.

Files

Create: internal/sync/execute.go, internal/sync/retry.go, internal/sync/execute_test.go
Modify: internal/sync/engine.go

1. Bounded concurrency

Run transfers through a worker pool, default 4, configurable.

Ordering still holds. Order (Task 8) guarantees parent-before-child and child-before-parent-delete. Concurrency must not break that. The safe approach: execute in dependency layers — group operations by Depth, run each layer concurrently, and only start the next layer once the previous has finished. Never run a create and a delete concurrently.

2. Retry with backoff — and knowing when not to

Retry only what can succeed on a second attempt:

Condition	Retry?
Network timeout, connection reset, 5xx	Yes — exponential backoff, jitter, cap ~5 attempts
`423 Locked`	Yes — someone else is writing; back off longer
`507 Quota exceeded`	No — surface it. Retrying cannot help and hammers the server
`401 Unauthorized`	No — the app password was revoked. Stop and ask for re-auth
`403 Forbidden`	No — a permission problem retrying will not fix

Getting this table wrong is how a client ends up sending thousands of futile requests to a server that has already said no.

3. Partial-write safety

A file being written by another application must not be uploaded mid-write.

Before reading a file for upload, check stability: stat it, wait a short interval (~2 s), stat again. If size or mtime changed, skip this pass and pick it up next time. Uploading a half-written file produces a valid-looking sync of corrupt content, and the state DB then records it as correct.

Steps

Write failing tests:
- a fake remote failing twice then succeeding ⇒ the operation completes and exactly 3 attempts were made
- a 507 ⇒ exactly one attempt, surfaced as a Skip
- a 401 ⇒ exactly one attempt, no retry
- layered ordering: with concurrency 4, a parent create is never observed after its child
- a file whose size changes between the two stats is skipped, not uploaded
Run; confirm failure.
Implement.
Run the full suite, including TestConvergence with -race.
Commit: git commit -s -m "feat(sync): bounded concurrency, selective retry, partial-write guard"

Acceptance criteria

Non-retryable errors are attempted exactly once (asserted by counting).
Ordering invariants from Task 9 hold under concurrency.
go test -race is clean.

Depends on P2-6. ## Goal Make execution robust and reasonably fast without turning a flaky network into corruption or a denial-of-service against the user's own server. ## Files - Create: `internal/sync/execute.go`, `internal/sync/retry.go`, `internal/sync/execute_test.go` - Modify: `internal/sync/engine.go` ## 1. Bounded concurrency Run transfers through a worker pool, default **4**, configurable. **Ordering still holds.** `Order` (Task 8) guarantees parent-before-child and child-before-parent-delete. Concurrency must not break that. The safe approach: execute in **dependency layers** — group operations by `Depth`, run each layer concurrently, and only start the next layer once the previous has finished. Never run a create and a delete concurrently. ## 2. Retry with backoff — and knowing when not to Retry only what can succeed on a second attempt: | Condition | Retry? | |---|---| | Network timeout, connection reset, 5xx | **Yes** — exponential backoff, jitter, cap ~5 attempts | | `423 Locked` | **Yes** — someone else is writing; back off longer | | `507 Quota exceeded` | **No** — surface it. Retrying cannot help and hammers the server | | `401 Unauthorized` | **No** — the app password was revoked. Stop and ask for re-auth | | `403 Forbidden` | **No** — a permission problem retrying will not fix | Getting this table wrong is how a client ends up sending thousands of futile requests to a server that has already said no. ## 3. Partial-write safety A file being written by another application must not be uploaded mid-write. Before reading a file for upload, check **stability**: stat it, wait a short interval (~2 s), stat again. If size or mtime changed, skip this pass and pick it up next time. Uploading a half-written file produces a valid-looking sync of corrupt content, and the state DB then records it as correct. ## Steps - [ ] Write failing tests: - a fake remote failing twice then succeeding ⇒ the operation completes and exactly 3 attempts were made - a 507 ⇒ exactly **one** attempt, surfaced as a `Skip` - a 401 ⇒ exactly one attempt, no retry - layered ordering: with concurrency 4, a parent create is never observed after its child - a file whose size changes between the two stats is skipped, not uploaded - [ ] Run; confirm failure. - [ ] Implement. - [ ] Run the full suite, including `TestConvergence` with `-race`. - [ ] Commit: `git commit -s -m "feat(sync): bounded concurrency, selective retry, partial-write guard"` ## Acceptance criteria - Non-retryable errors are attempted exactly once (asserted by counting). - Ordering invariants from Task 9 hold under concurrency. - `go test -race` is clean.

Cordy added this to the phase-2-integration milestone

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

Cordy added the

engine

data-integrity

labels

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

Cordy referenced this issue

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

P2-4: Resumable upload via tus #24

Cordy referenced this issue

2026-09-11 07:59:23 +00:00

P2-5: Real OS filesystem — vfs.FS with per-platform FileID #25

Cordy commented

2026-09-11 11:02:01 +00:00

Author

Owner

Done

2849548b83 — feat(sync): bounded concurrency, selective retry, partial-write guard
23ca0d9487 — fix(sync): re-check before PUT, settle once per pass, brake retries

What was built

Worker pool executing dependency layers by Depth; never a create and a delete concurrently.
Error classification in internal/remote (ErrTransient for 5xx/timeouts/resets, ErrLocked for 423; 507/401/403 not retried); internal/sync decides only via errors.Is.
Exponential backoff with jitter, cap ~5 attempts, longer backoff for 423, plus a per-pass brake after 8 consecutive give-ups (F5 ruling).
Partial-write guard: stat–wait(~2s)–stat, run once per pass via settle(), with put/conflict comparing against the scan's own stat.
Fix round 1 closed two data-integrity windows the review found: the server re-check now runs immediately before the PUT (F1), and a conflict's retried download re-checks the local file before every attempt so a save made during backoff is never lost (F2).

Tests

The issue's five listed tests, plus regression tests for F1/F2/F3/F5, each RED at the base commit and GREEN after the fix; 15 mutants killed, 0 survived.
Full suite green under CGO_ENABLED=0, and under CGO_ENABLED=1 -race including TestConvergence/TestConvergenceOnATestClock (50/50 seeds each, 0 data races) — P2-R7.
Forgejo CI run #25 on 23ca0d9, linux/arm64, green. Coverage total: 90.8%.

Acceptance criteria

Non-retryable errors (507/401/403) attempted exactly once — TestSyncNeverRetriesWhatAnotherAttemptCannotFix, TestSyncStopsWhenTheServerRefusesTheCredentials.
Task 9 ordering invariants hold under concurrency — TestSyncRunsOperationsConcurrentlyInOrdersLayers (pairwise start/end ordering and the exact concurrency bound at 0, 4 and 2).
go test -race is clean — full suite plus TestConvergence under -race, 0 warnings.

Rulings

P2-R1: 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.
P2-R7: -race runs locally with CGO_ENABLED=1 as a test tool only; CI stays CGO_ENABLED=0 without -race.
P2-R8: retry classification lives in internal/remote; internal/sync decides only via errors.Is (no net/net-http import); sleeps are injected for tests.
Task 27 Ruling (F5): a per-pass brake — after 8 consecutive give-ups on ErrTransient, stop retrying for the rest of the pass; a success resets the count; 423 unaffected.
Task 27 Ruling (CV4): live cairnd (bd006ef) under concurrency 4 partly exercised — concurrent MKCOL and a real 500 (read-only folder) behaved as classified; a real 423 and a mid-pass server stop are carried to #29.

Deferred

F4 (Minor, review): after a 401 halt, operations already in flight (a conflict's remaining steps, settleMove's put) still send one more request before stopping; not in this round's fix list.
CV5/P2-R6: partial-write detection on Windows, while a writer holds the file open, remains untested until #19.
Residual timing windows inherent to any re-check design (a save landing inside the settle wait, or between put's Open and the PUT) are named in the code's docs and left for the final review.

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/2849548b83345eeff02730ac6cbe7067e2a7489a — feat(sync): bounded concurrency, selective retry, partial-write guard - http://192.168.10.245/Cordy/cairn-desktop/commit/23ca0d948756c49ee460e68c313460cbe42b7e0a — fix(sync): re-check before PUT, settle once per pass, brake retries **What was built** - Worker pool executing dependency layers by `Depth`; never a create and a delete concurrently. - Error classification in internal/remote (`ErrTransient` for 5xx/timeouts/resets, `ErrLocked` for 423; 507/401/403 not retried); internal/sync decides only via `errors.Is`. - Exponential backoff with jitter, cap ~5 attempts, longer backoff for 423, plus a per-pass brake after 8 consecutive give-ups (F5 ruling). - Partial-write guard: stat–wait(~2s)–stat, run once per pass via `settle()`, with `put`/`conflict` comparing against the scan's own stat. - Fix round 1 closed two data-integrity windows the review found: the server re-check now runs immediately before the PUT (F1), and a conflict's retried download re-checks the local file before every attempt so a save made during backoff is never lost (F2). **Tests** - The issue's five listed tests, plus regression tests for F1/F2/F3/F5, each RED at the base commit and GREEN after the fix; 15 mutants killed, 0 survived. - Full suite green under `CGO_ENABLED=0`, and under `CGO_ENABLED=1 -race` including `TestConvergence`/`TestConvergenceOnATestClock` (50/50 seeds each, 0 data races) — P2-R7. - Forgejo CI run [#25](http://192.168.10.245/Cordy/cairn-desktop/actions/runs/25) on `23ca0d9`, linux/arm64, **green**. Coverage total: 90.8%. **Acceptance criteria** - Non-retryable errors (507/401/403) attempted exactly once — `TestSyncNeverRetriesWhatAnotherAttemptCannotFix`, `TestSyncStopsWhenTheServerRefusesTheCredentials`. - Task 9 ordering invariants hold under concurrency — `TestSyncRunsOperationsConcurrentlyInOrdersLayers` (pairwise start/end ordering and the exact concurrency bound at 0, 4 and 2). - `go test -race` is clean — full suite plus `TestConvergence` under `-race`, 0 warnings. **Rulings** - P2-R1: 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. - P2-R7: `-race` runs locally with `CGO_ENABLED=1` as a test tool only; CI stays `CGO_ENABLED=0` without `-race`. - P2-R8: retry classification lives in internal/remote; internal/sync decides only via `errors.Is` (no net/net-http import); sleeps are injected for tests. - Task 27 Ruling (F5): a per-pass brake — after 8 consecutive give-ups on `ErrTransient`, stop retrying for the rest of the pass; a success resets the count; 423 unaffected. - Task 27 Ruling (CV4): live cairnd (bd006ef) under concurrency 4 partly exercised — concurrent MKCOL and a real 500 (read-only folder) behaved as classified; a real 423 and a mid-pass server stop are carried to #29. **Deferred** - F4 (Minor, review): after a 401 halt, operations already in flight (a conflict's remaining steps, `settleMove`'s put) still send one more request before stopping; not in this round's fix list. - CV5/P2-R6: partial-write detection on Windows, while a writer holds the file open, remains untested until #19. - Residual timing windows inherent to any re-check design (a save landing inside the settle wait, or between `put`'s Open and the PUT) are named in the code's docs and left for the final review. _Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI._

Cordy closed this issue

2026-09-11 11:02:06 +00:00