From 875e87d1bcab33169de9b83f4debe681217deee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 1 Nov 2026 13:40:50 +0100 Subject: [PATCH] [DeckSync] Address auto-sync review: disconnect, backfill and modal conflicts - Fail the run when the connection drops while a sync is running, and clear the active conflict, so the queue is not sent on a dead client and a re-login as another account never receives this profile's uploads. - Backfill last_modified from upload_time in the 37 -> 38 migration and drop the "last_modified is null" arm from the sync-check query, so rows that predate the column stop reporting as changed on every poll. - Skip conflicts during auto runs and report how many were skipped, instead of opening the modal conflict dialog from a timer or a file-watcher event. --- .../interface/deck_sync/deck_sync_manager.cpp | 30 ++++++++++++++++++- .../interface/deck_sync/deck_sync_manager.h | 3 ++ .../migrations/servatrice_0040_to_0041.sql | 6 ++++ servatrice/src/serversocketinterface.cpp | 5 ++-- 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 6a1bba5bf..eefaf96e6 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -109,6 +109,14 @@ DeckSyncManager::DeckSyncManager(AbstractClient *_client, ColorIdentityProvider if (status == StatusDisconnected) { offline = true; debounceTimer->stop(); + // A run cannot survive the session it was started on: the in-flight + // command comes back RespNotConnected, the remaining queue would be + // sent on a dead client, and a re-login as another account would + // receive this profile's uploads and deletes. + if (syncing) { + activeConflict = ActiveConflict(); + failSync(tr("Disconnected.")); + } } }, Qt::UniqueConnection); @@ -201,7 +209,7 @@ void DeckSyncManager::onDebounceTimeout() return; } if (canAutoRun()) { - syncNow(); + startSync(true); } } @@ -233,10 +241,16 @@ bool DeckSyncManager::isSyncing() const } void DeckSyncManager::syncNow() +{ + startSync(false); +} + +void DeckSyncManager::startSync(bool _autoRun) { if (syncing) { return; } + autoRun = _autoRun; const DeckSyncProfile runProfile = buildProfile(); if (client == nullptr || !runProfile.isValid() || deckPath.isEmpty()) { emit syncFinished(SyncSummary{}, tr("Not connected to a server.")); @@ -253,6 +267,7 @@ void DeckSyncManager::syncNow() downloadedCount = 0; unchangedCount = 0; skippedCount = 0; + conflictCount = 0; pendingUploads.clear(); pendingDownloads.clear(); pendingDeletes.clear(); @@ -746,6 +761,16 @@ void DeckSyncManager::queueConflict(const QString &relativePath, void DeckSyncManager::handleConflictDownload(const QString &relativePath, const QString &serverDeckString) { const ConflictDownload ctx = pendingConflicts.take(relativePath); + if (autoRun) { + // An auto run has no user in front of it: opening the modal conflict + // dialog from a timer or a file-watcher event would pop up mid-game. + // Leave the path untouched so the next manual sync classifies it again + // and asks. + ++conflictCount; + ++skippedCount; + processNext(); + return; + } if (sessionDefault == DeckSyncConflictKeepMine) { applyUpload(relativePath, ctx.serverPath, ctx.deckId, ctx.guid); processNext(); @@ -1245,6 +1270,9 @@ void DeckSyncManager::finishSync() .arg(summary.downloaded) .arg(summary.unchanged) .arg(summary.skipped)); + if (conflictCount > 0) { + emit statusMessage(tr("%1 conflicting decks skipped. Press Sync now to resolve them.").arg(conflictCount)); + } emit syncFinished(summary, QString()); } diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index 98c9301ac..7d3c6358d 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -159,6 +159,7 @@ private: qint64 lastModifiedSecs = 0; }; + void startSync(bool autoRun); void failSync(const QString &error); void finishSync(); [[nodiscard]] SyncSummary currentSummary() const; @@ -240,6 +241,8 @@ private: int downloadedCount = 0; int unchangedCount = 0; int skippedCount = 0; + int conflictCount = 0; + bool autoRun = false; }; #endif // DECK_SYNC_MANAGER_H diff --git a/servatrice/migrations/servatrice_0040_to_0041.sql b/servatrice/migrations/servatrice_0040_to_0041.sql index 83edf871f..aed483d04 100644 --- a/servatrice/migrations/servatrice_0040_to_0041.sql +++ b/servatrice/migrations/servatrice_0040_to_0041.sql @@ -16,6 +16,12 @@ ALTER TABLE `cockatrice_decklist_files` ADD COLUMN `last_modified` datetime NULL AFTER `hash`, ADD COLUMN `deleted` tinyint(1) NOT NULL DEFAULT 0 AFTER `last_modified`; +-- Rows created before this migration only get a last_modified when a client +-- re-uploads them, so every sync check would keep reporting them as changed. +-- Backfill from the upload time once, here, instead of carrying a +-- "last_modified is null" branch in the poll query forever. +UPDATE `cockatrice_decklist_files` SET `last_modified` = `upload_time` WHERE `last_modified` IS NULL; + CREATE TABLE IF NOT EXISTS `cockatrice_deck_versions` ( `id` int(7) unsigned zerofill NOT NULL auto_increment, `deck_id` int(7) unsigned zerofill NOT NULL, diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 50a544538..51c5b8d92 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -904,10 +904,11 @@ bool AbstractServerSocketInterface::syncCheckHelper(int userId, qint64 sinceSecs // their files linger as tombstones, so walking the live folder tree would // miss deletes pointing at nonexistent folders. The client only checks // changed_files_size(), so folder paths are not reconstructed here. + // last_modified is backfilled by the 37 -> 38 migration, so a plain range + // check is enough. QSqlQuery *query = sqlInterface->prepareQuery("select id, guid, hash, last_modified, deleted from " "{prefix}_decklist_files where id_user = :id_user " - "and (last_modified is null or last_modified >= " - "FROM_UNIXTIME(:since))"); + "and last_modified >= FROM_UNIXTIME(:since)"); query->bindValue(":id_user", userId); query->bindValue(":since", sinceSecs); if (!sqlInterface->execSqlQuery(query)) {