fix(sync): hold accept decisions until folder paths have settled (#56) - #59
Conversation
On cold start the auto-accept pass raced the folder-path reconcile: the scenePhase-active and onboarding-completion triggers fire reconcileFolderPaths without awaiting it, while ContentView's onChange(of: pendingFolders, initial: true) runs the accept pass as soon as the pending list is decoded. The pass then derives its occupied-path overlap set from pre-reconcile folder paths — stale exactly after an iOS container move — so a share could take the very path an existing folder was about to be rebased onto: the #45 overlap through a timing window. The manual accept paths, including the #54 merge confirmation, judged the same stale set. #53 closed this ordering gap for the reconnect flow only; the cold-start trigger still raced. Per decision record 008 (added here): accept decisions only run on settled paths. SyncthingManager tracks settledness in PathSettlement, a pure generation-tokened ledger — unsettled while a reconcile is in flight or before one has completed for the current engine generation. The automatic pass parks silently and re-fires from an onChange once paths settle; a manual tap gets a transient 'try again in a moment' (002: explain, never silently refuse). No data is ever moved. Why this is safe: - Fail-closed: an abandoned reconcile (engine died mid-run) and a stale outcome from before an engine stop/restart can only hold accepts, never unlock them (generation token; pinned in PathSettlementTests, including the recovery path reset -> fresh reconcile -> settled). - No hold without release: every path that attaches ContentView also fires a reconcile — the onboarding-completion handler now reconciles even when onboarding itself already started the engine (previously such a launch ran with no reconcile at all). The repeat reconcile is a no-op on already-correct paths: reconcile() skips every folder whose canonical desired path equals the stored one, and the #45 pause shield pauses each folder at most once via its sidecar. - No failure is recorded while held, so the automatic re-fire is never blocked; the #53 retry pass routes through the same gate and is now additionally protected against a concurrently running cold-start reconcile. Verified here (Linux): design-token lint green, all four Localizable.strings key counts identical (614), duplicate-key check clean (the 'The folder \"' hit is the documented false positive). Swift suite not runnable on Linux — PathSettlementTests and the full plan are pending the macOS test pass. No go/ changes, so no xcframework rebuild is required.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📜 Recent review details
|
| Direction | Path |
|---|---|
| Server → iPhone | vaultsync-notify spots outgoing changes → Cloud Relay silent push → VaultSync wakes and pulls. |
| iPhone → Server | iOS doesn't guarantee timely background execution for local edits. The reliable path is to open VaultSync and let embedded Syncthing run in the foreground — a Shortcuts automation can do that automatically whenever you leave Obsidian. |
Cloud Relay is a server → iPhone acceleration path, not a guarantee of symmetric real-time background sync.
Connection paths & iOS network privacy
How peers are reached, fastest f...
Files:
docs/decisions/008-accepts-only-on-settled-paths.md
docs/decisions/**
⚙️ CodeRabbit configuration file
docs/decisions/**: # 001 — Two Syncthing folders never overlap on disk (three enforcement layers)Context: A server sharing more than one vault could hand the second share the same local folder as the first (1.6.0–1.7.0) or a subfolder inside an existing vault (1.7.1). Overlapping folders sync each other's content as their own — deleting the stray copy on any peer would have deleted the inner vault everywhere (
#45).Decision: The no-overlap invariant (equal, nested, or containing paths) is enforced in three independent layers, each with its own tests: the Go hard floor (
AcceptPendingFolder/AddFolderreject withfolderPathOverlapError), the Swift mapping (VaultManager.resolveSharePathreturnsnilrather than an overlapping path), and the launch shield (PathCollisionGuardpauses already-overlapping folders exactly once).Why: The single-layer version failed twice — both
#45bugs lived in the Swift mapping. The Go floor backstops future mapping bugs, the Swift layer turns a hard engine error into user guidance, and only the shield catches damage that predates the fix.Rejected alternative: Enforcing only in the Swift mapping — the proven failure mode; a bug there would silently re-open the hole with no backstop.
Links:
#45, PR#47(same-folder merge, 1.7.1), PR#51(nesting, 1.7.2).
docs/decisions/**: # 002 — Recovery from data damage is never automaticContext: When VaultSync detects existing damage (two vaults merged into one folder, or one nested inside another,
#45), an automatic repair would have to move, rename, delete, or re-accept user data whose true state only the user knows — after a merge, the app cannot tell which files belong to which vault.Decision: Recovery is always: pause the affected folders (exactly once — a folder the user deliberately resumes is never re-paused), explain the problem in a critical issue with concrete recovery steps, and let the user act.
Why: Sync propagates every local act...
Files:
docs/decisions/008-accepts-only-on-settled-paths.md
🧠 Learnings (1)
📚 Learning: 2026-06-10T18:47:10.724Z
Learnt from: psimaker
Repo: psimaker/vaultsync PR: 38
File: ios/VaultSync/Views/ContentView.swift:605-611
Timestamp: 2026-06-10T18:47:10.724Z
Learning: In the SwiftUI codebase under ios/VaultSync, do not flag missing localization for SwiftUI string literals used as Text("…") or DisclosureGroup("…") titles/labels. In SwiftUI, these string literals are treated as LocalizedStringKey and resolve via the app’s Localizable.strings automatically—so they only need attention if the corresponding key is actually missing. Only require an explicit localization helper (e.g., L10n.tr(…)) when the string is not being passed through SwiftUI’s LocalizedStringKey path (e.g., plain String values provided to non-SwiftUI APIs).
Applied to files:
ios/VaultSync/App/VaultSyncApp.swiftios/VaultSync/Services/PathSettlement.swiftios/VaultSync/Views/ContentView.swiftios/VaultSync/Services/SyncthingManager.swift
🪛 LanguageTool
CHANGELOG.md
[style] ~20-~20: This expression tends to be overused. Try opting for a more concise alternative.
Context: ...during the window asks you to try again in a moment. Nothing is ever moved automatically. L...
(IN_A_MOMENT)
🔇 Additional comments (13)
ios/VaultSync/de.lproj/Localizable.strings (1)
329-329: LGTM!ios/VaultSync/en.lproj/Localizable.strings (1)
329-329: LGTM!ios/VaultSync/es.lproj/Localizable.strings (1)
329-329: LGTM!ios/VaultSync/zh-Hans.lproj/Localizable.strings (1)
329-329: LGTM!CHANGELOG.md (1)
20-20: LGTM!ios/VaultSyncTests/PathSettlementTests.swift (1)
1-106: LGTM!ios/VaultSync/App/VaultSyncApp.swift (1)
128-134: LGTM!ios/VaultSync/Views/ContentView.swift (1)
198-206: LGTM!Also applies to: 344-349, 368-374, 401-415
ios/VaultSync/Services/PathSettlement.swift (2)
1-32: LGTM!Also applies to: 56-63
33-54: 🗄️ Data Integrity & Integration
reconcileAbandonedshould not clearcompletedThisGeneration— if any reconcile in the current generation has completed,settledshould become true onceinFlightreaches zero; an abandoned sibling pass must not undo that state.> Likely an incorrect or invalid review comment.docs/decisions/008-accepts-only-on-settled-paths.md (1)
1-12: LGTM!ios/VaultSync/Services/SyncthingManager.swift (2)
684-727: 🎯 Functional CorrectnessCorrect token-before-task ordering; relies on the
PathSettlementfix noted separately.Minting the token synchronously before the detached task starts (Line 690) correctly closes the original poll-vs-hold race. The engine-died-mid-wait path and the
stop()/resetForRestart()generation resets are consistent with the intended contract. Note this file is the call site that makes concurrentreconcileFolderPathsinvocations plausible (per the PR's own onboarding-completion fix triggering a reconcile even while one is already running) — see thecompletedThisGenerationdowngrade-on-abandon issue flagged inPathSettlement.swift, which this wiring would otherwise surface in production.
93-96: LGTM!Also applies to: 730-753, 936-956
📝 Walkthrough
Walkthrough
Introduces a PathSettlement state machine in SyncthingManager that tracks reconcile generation/completion, gating automatic and manual share acceptance in ContentView until folder-path reconciliation settles. Fixes a cold-start reconcile gap in VaultSyncApp, adds a decision record, localized strings, changelog entry, and unit tests.
Changes
Path settlement gating
| Layer / File(s) | Summary |
|---|---|
PathSettlement contract and decision record ios/VaultSync/Services/PathSettlement.swift, docs/decisions/008-accepts-only-on-settled-paths.md |
New PathSettlement type tracks generation token, in-flight reconciles, and completion state; settled is true only after a matching completion with no in-flight reconcile. Decision doc documents the fail-closed rule and rejected alternatives (timeout fallback, accept-then-repair). |
SyncthingManager reconcile lifecycle wiring ios/VaultSync/Services/SyncthingManager.swift |
Adds pathSettlement property; reconcileFolderPaths generates a token via reconcileBegan(), marks abandonment on engine death, completion on success via new markReconcile(token:completed:) helper; stop() and resetForRestart() reset settlement state. |
Cold-start reconcile gap fix ios/VaultSync/App/VaultSyncApp.swift |
Adds an else branch to onChange(of: hasCompletedOnboarding) so reconcileFolderPaths runs even when Syncthing is already running. |
Accept flow gating in ContentView ios/VaultSync/Views/ContentView.swift |
New onChange on pathSettlement.settled re-triggers auto-accept; autoAcceptPendingShares, manualAcceptPendingShare, and performPendingShareAccept early-return when unsettled, surfacing a "try again" alert only for manual actions. |
Localization, changelog, and tests ios/VaultSync/{en,de,es,zh-Hans}.lproj/Localizable.strings, CHANGELOG.md, ios/VaultSyncTests/PathSettlementTests.swift |
Adds the "vault locations still being checked" message in four locales, a changelog entry, and tests covering cold-start, overlapping reconciles, abandonment, reset, and stale-token behavior. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Possibly related PRs
- psimaker/vaultsync#32: Both PRs touch SyncthingManager's
reconcileFolderPathsflow — this PR gates share-acceptance on a newPathSettlementstate, while the other introduces the underlying reconciler mechanism it depends on.
Poem
A vault path shifts, a race once lurked,
NowPathSettlementkeeps things checked and worked. 🐇
Accept holds tight till reconcile's done,
Manual taps get a "try again, hon."
Four languages whisper the same delay—
Stale paths banished, settled to stay. ✨
🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title is concise, conventional-commit style, and accurately summarizes the main fix for settling folder paths before accept decisions. |
| Linked Issues check | ✅ Passed | The PR implements the settled-path gate, retries automatic accept after reconciliation, shows manual retry feedback, and documents the rule as requested. |
| Out of Scope Changes check | ✅ Passed | The changes stay within the cold-start accept-race fix and supporting docs, tests, and localizations, with no clear unrelated scope creep. |
| No Private Note Leakage | ✅ Passed | Added path-settlement gating and retry messaging only; no new logs, analytics, network calls, or secret/path/bookmark disclosures were introduced. |
| Bounded Ios Background Work | ✅ Passed | PASS: This PR only adds path-settlement gating and short reconciles; it doesn't alter BGTask/beginBackgroundTask handling, and existing background handlers still have expiration cleanup. |
| Bridge Contract Compatibility | ✅ Passed | PR only changes iOS accept-gating files; no go/bridge or SyncBridgeService surface changed, and bridge tests/conventions remain untouched. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/issue-56-cold-start-accept-race
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
1 participant
Cold-start auto-accept raced the folder-path reconcile: accept decisions derived their occupied-path set from pre-reconcile folder paths — stale exactly after an iOS container move (#45 overlap through a timing window). Per decision record 008 (added here), every accept decision — automatic, manual, and the #54 merge confirmation — now runs only on settled paths: held automatically and re-fired once the reconcile completes; manual taps get a transient retry message (002). Fail-closed via a generation-tokened ledger (PathSettlement, exhaustively unit-tested); the onboarding-completion path now reconciles even when onboarding already started the engine, so a hold always has a release. No data is ever moved. Swift suite not runnable on the authoring machine — macOS CI run pending. Closes #56.
VaultSync now waits for folder-path reconciliation to finish before running any accept logic, preventing cold-start decisions from using stale occupied-path data.
PathSettlementgate to track whether paths are settled and to ignore stale reconcile completions across restarts.