Fix Git Sync race between startup pull and auto-sync export
syncOnStartup() (pull+import, fired at launch) and autoSync() (export+push, debounced off chat activity) ran as fully independent, uncoordinated Tasks with no mutual exclusion. A user launching the app and chatting right away could hit autoSync's export mid-pull, leaving a freshly-written untracked file that the pull then refuses to merge over — the same failure class as the earlier folders.json bug, now much more likely to surface widely since folders.json/notes.json are brand new for every existing sync repo. Adds a shared isSyncing guard across all three entry points (syncOnStartup skips if busy, autoSync waits for a clear slot, syncNow throws .syncInProgress) and moves Sync Now's pull/import/export/push orchestration out of SettingsView into GitSyncService.syncNow(), where the guard can actually protect it.
This commit is contained in:
@@ -39,6 +39,7 @@ enum SyncError: LocalizedError {
|
||||
case repoNotCloned
|
||||
case secretsDetected([String])
|
||||
case parseError(String)
|
||||
case syncInProgress
|
||||
|
||||
var errorDescription: String? {
|
||||
switch self {
|
||||
@@ -56,6 +57,8 @@ enum SyncError: LocalizedError {
|
||||
return "Secrets detected in conversations: \(secrets.joined(separator: ", ")). Remove before syncing."
|
||||
case .parseError(let message):
|
||||
return "Failed to parse conversation: \(message)"
|
||||
case .syncInProgress:
|
||||
return "A sync is already in progress. Try again in a moment."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -630,6 +630,20 @@ class GitSyncService {
|
||||
return
|
||||
}
|
||||
|
||||
// Guard against racing autoSync()/syncNow() on the same working tree — this method and
|
||||
// autoSync() are both fired from independent, uncoordinated Tasks (this one at app launch,
|
||||
// autoSync() debounced off chat activity), so without this a pull here could run while
|
||||
// autoSync() is mid-export, leaving a freshly-written untracked file (folders.json,
|
||||
// notes.json) that the pull then refuses to merge over: "untracked working tree files
|
||||
// would be overwritten by merge." Skipping outright (not waiting) is fine here since
|
||||
// startup sync is a one-time best-effort fetch, not something the user is blocked on.
|
||||
guard !isSyncing else {
|
||||
log.debug("Skipping startup sync (another sync already in progress)")
|
||||
return
|
||||
}
|
||||
isSyncing = true
|
||||
defer { isSyncing = false }
|
||||
|
||||
log.info("Running startup sync (pull + import)...")
|
||||
|
||||
do {
|
||||
@@ -668,6 +682,12 @@ class GitSyncService {
|
||||
|
||||
// Schedule new sync with 5 second delay
|
||||
pendingSyncTask = Task {
|
||||
// Tracks whether *this* task is the one holding isSyncing, so the catch block below
|
||||
// only ever releases a lock it actually acquired — without this, a cancellation while
|
||||
// still waiting in the loop below (i.e. before this task owns the lock at all) would
|
||||
// incorrectly clear isSyncing out from under whichever other sync is still running.
|
||||
var acquiredLock = false
|
||||
|
||||
do {
|
||||
// Wait for debounce period
|
||||
try await Task.sleep(for: .seconds(5))
|
||||
@@ -675,11 +695,23 @@ class GitSyncService {
|
||||
// Check if cancelled during sleep
|
||||
guard !Task.isCancelled else { return }
|
||||
|
||||
// Wait for any other sync (startup pull, manual Sync Now) already in flight to
|
||||
// finish rather than racing it on the same working tree — see syncOnStartup()'s
|
||||
// guard for what goes wrong otherwise. Waiting (not skipping) here, since
|
||||
// auto-sync is how local changes actually reach the remote; silently dropping this
|
||||
// round could leave a push pending indefinitely if nothing else triggers autoSync
|
||||
// again soon.
|
||||
while await MainActor.run(body: { isSyncing }) {
|
||||
guard !Task.isCancelled else { return }
|
||||
try await Task.sleep(for: .milliseconds(500))
|
||||
}
|
||||
|
||||
// Set syncing state
|
||||
await MainActor.run {
|
||||
isSyncing = true
|
||||
lastSyncError = nil
|
||||
}
|
||||
acquiredLock = true
|
||||
|
||||
log.info("Auto-sync starting (export + push)...")
|
||||
|
||||
@@ -694,15 +726,18 @@ class GitSyncService {
|
||||
isSyncing = false
|
||||
syncStatus.lastSyncTime = Date()
|
||||
}
|
||||
acquiredLock = false
|
||||
|
||||
log.info("Auto-sync completed successfully")
|
||||
|
||||
} catch {
|
||||
// Error
|
||||
// Error — only release the lock if this task actually acquired it
|
||||
if acquiredLock {
|
||||
await MainActor.run {
|
||||
isSyncing = false
|
||||
lastSyncError = error.localizedDescription
|
||||
}
|
||||
}
|
||||
|
||||
log.error("Auto-sync failed: \(error.localizedDescription)")
|
||||
}
|
||||
@@ -712,6 +747,27 @@ class GitSyncService {
|
||||
await pendingSyncTask?.value
|
||||
}
|
||||
|
||||
/// Manual full sync (the Settings → Sync "Sync Now" button): pull → import → export → push,
|
||||
/// in that order so the working tree is fully merged before Confab writes its own files back
|
||||
/// out (see exportAllConversations's ordering note). Throws `.syncInProgress` rather than
|
||||
/// racing autoSync()/syncOnStartup() if either is already running on the same working tree —
|
||||
/// same class of bug as the "untracked working tree files" failure those two guard against.
|
||||
func syncNow() async throws -> (imported: Int, skipped: Int) {
|
||||
guard !isSyncing else {
|
||||
throw SyncError.syncInProgress
|
||||
}
|
||||
isSyncing = true
|
||||
defer { isSyncing = false }
|
||||
|
||||
try await pull()
|
||||
let result = try await importAllConversations()
|
||||
try await exportAllConversations()
|
||||
try await push()
|
||||
|
||||
await updateStatus()
|
||||
return (result.imported, result.skipped)
|
||||
}
|
||||
|
||||
// MARK: - Secret Scanning
|
||||
|
||||
/// Scan for API keys and secrets in conversations
|
||||
|
||||
@@ -3039,31 +3039,13 @@ It's better to admit "I need more information" or "I cannot do that" than to fak
|
||||
|
||||
private func syncNow() async {
|
||||
isSyncing = true
|
||||
syncTestResult = nil
|
||||
syncTestResult = "Syncing..."
|
||||
|
||||
do {
|
||||
// Step 1: Pull from remote first, into a working tree that hasn't been touched by
|
||||
// this round's export yet. Exporting before pulling can write files (e.g.
|
||||
// folders.json, which every machine writes to the same path) that git then refuses
|
||||
// to merge over: "untracked working tree files would be overwritten by merge".
|
||||
syncTestResult = "Pulling changes..."
|
||||
try await gitSync.pull()
|
||||
|
||||
// Step 2: Import any new/updated conversations and folders from what was just pulled
|
||||
syncTestResult = "Importing conversations..."
|
||||
let result = try await gitSync.importAllConversations()
|
||||
|
||||
// Step 3: Export — now safe, re-derives working tree files from the local DB state,
|
||||
// which already reflects whatever was just imported plus this machine's own changes
|
||||
syncTestResult = "Exporting conversations..."
|
||||
try await gitSync.exportAllConversations()
|
||||
|
||||
// Step 4: Push to remote
|
||||
syncTestResult = "Pushing changes..."
|
||||
try await gitSync.push()
|
||||
|
||||
// Success
|
||||
await gitSync.updateStatus()
|
||||
// Orchestration (pull → import → export → push) and the guard against racing
|
||||
// autoSync()/syncOnStartup() on the same working tree both live in GitSyncService now
|
||||
// — see its syncNow() for why.
|
||||
let result = try await gitSync.syncNow()
|
||||
syncTestResult = "✓ Sync complete: \(result.imported) imported, \(result.skipped) skipped"
|
||||
} catch {
|
||||
syncTestResult = "✗ Sync failed: \(error.localizedDescription)"
|
||||
|
||||
Reference in New Issue
Block a user