mirror of
https://github.com/zarzet/SpotiFLAC-Mobile.git
synced 2026-07-25 21:40:57 +02:00
fix(native-worker): keep batches and retries intact across seam races
- Verification challenge no longer abandons the batch: batch mates the worker cancel would mark skipped are requeued and fenced from later snapshot applications, and the adoption loop restarts the queue for requeued items when the run ends - Pause vs cancel race: a cancelled result with no pause flag set waits up to 1.5s for the pause intent to land on the main looper before being classified as a permanent skip - Stale Go cancel flag: new ResetDownloadCancel export (wired through Android and iOS bridges) drops a pre-registered cancel with no active download; retryItem/retryAllFailed call it so the first retry of a cancelled-before-start item no longer aborts instantly - Reconcile-loop errors now cancel the native worker and clear the run id before marking items failed, instead of leaving the service downloading a batch the UI already wrote off
This commit is contained in:
@@ -755,6 +755,27 @@ class DownloadService : Service() {
|
||||
writeNativeAlbumReplayGainIfComplete()
|
||||
} else {
|
||||
val errorType = result.optString("error_type")
|
||||
if (errorType == "cancelled" &&
|
||||
!nativeWorkerPaused &&
|
||||
!nativeWorkerCancelRequested &&
|
||||
generation == nativeWorkerGeneration
|
||||
) {
|
||||
// A pause from Dart cancels the in-flight Go
|
||||
// download directly but delivers the pause flag
|
||||
// via a startService intent through the main
|
||||
// looper; the download can unwind first. Give the
|
||||
// flag a moment to settle before classifying this
|
||||
// cancellation as a permanent skip.
|
||||
var waitedMs = 0L
|
||||
while (waitedMs < 1500 &&
|
||||
!nativeWorkerPaused &&
|
||||
!nativeWorkerCancelRequested &&
|
||||
generation == nativeWorkerGeneration
|
||||
) {
|
||||
delay(100)
|
||||
waitedMs += 100
|
||||
}
|
||||
}
|
||||
if (errorType == "cancelled" && nativeWorkerPaused && !nativeWorkerCancelRequested) {
|
||||
updateNativeWorkerItem(request.itemId) {
|
||||
it.status = "queued"
|
||||
|
||||
@@ -2308,6 +2308,13 @@ class MainActivity: FlutterFragmentActivity() {
|
||||
}
|
||||
result.success(null)
|
||||
}
|
||||
"resetDownloadCancel" -> {
|
||||
val itemId = call.argument<String>("item_id") ?: ""
|
||||
withContext(Dispatchers.IO) {
|
||||
Gobackend.resetDownloadCancel(itemId)
|
||||
}
|
||||
result.success(null)
|
||||
}
|
||||
"setDownloadDirectory" -> {
|
||||
val path = call.argument<String>("path") ?: ""
|
||||
withContext(Dispatchers.IO) {
|
||||
|
||||
@@ -91,6 +91,22 @@ func isDownloadCancelled(itemID string) bool {
|
||||
return canceled
|
||||
}
|
||||
|
||||
// resetDownloadCancel removes a cancellation entry that has no active
|
||||
// download attached (refs <= 0). Such entries exist to catch an item that is
|
||||
// just about to start, but if the item never starts the flag lingers and the
|
||||
// next explicit retry would consume it and abort immediately.
|
||||
func resetDownloadCancel(itemID string) {
|
||||
if itemID == "" {
|
||||
return
|
||||
}
|
||||
|
||||
cancelMu.Lock()
|
||||
if entry, ok := cancelMap[itemID]; ok && entry.refs <= 0 {
|
||||
delete(cancelMap, itemID)
|
||||
}
|
||||
cancelMu.Unlock()
|
||||
}
|
||||
|
||||
func clearDownloadCancel(itemID string) {
|
||||
if itemID == "" {
|
||||
return
|
||||
|
||||
@@ -419,6 +419,13 @@ func CancelDownload(itemID string) {
|
||||
cancelDownload(itemID)
|
||||
}
|
||||
|
||||
// ResetDownloadCancel drops a pre-registered cancellation flag for an item
|
||||
// with no active download, so a user-initiated retry does not consume a stale
|
||||
// cancel and abort instantly. Entries with live references are left alone.
|
||||
func ResetDownloadCancel(itemID string) {
|
||||
resetDownloadCancel(itemID)
|
||||
}
|
||||
|
||||
func CleanupConnections() {
|
||||
CloseIdleConnections()
|
||||
}
|
||||
|
||||
@@ -267,6 +267,35 @@ func TestFileDownloadFailureLeavesNoFinalFile(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestResetDownloadCancelDropsStaleFlag(t *testing.T) {
|
||||
const itemID = "reset-cancel-item"
|
||||
|
||||
// A cancel issued while nothing is downloading pre-registers a flag that
|
||||
// the next attempt would consume and abort; reset must drop it.
|
||||
cancelDownload(itemID)
|
||||
if !isDownloadCancelled(itemID) {
|
||||
t.Fatal("expected pre-registered cancel flag")
|
||||
}
|
||||
resetDownloadCancel(itemID)
|
||||
if isDownloadCancelled(itemID) {
|
||||
t.Fatal("expected stale cancel flag to be dropped")
|
||||
}
|
||||
|
||||
// A cancel attached to an active download must survive reset.
|
||||
ctx := initDownloadCancel(itemID)
|
||||
cancelDownload(itemID)
|
||||
resetDownloadCancel(itemID)
|
||||
if !isDownloadCancelled(itemID) {
|
||||
t.Fatal("expected active cancel to survive reset")
|
||||
}
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
default:
|
||||
t.Fatal("expected cancelled context")
|
||||
}
|
||||
clearDownloadCancel(itemID)
|
||||
}
|
||||
|
||||
func TestParseExtensionTrackValueExplicit(t *testing.T) {
|
||||
vm := goja.New()
|
||||
|
||||
|
||||
@@ -431,7 +431,13 @@ import Gobackend
|
||||
let itemId = args["item_id"] as! String
|
||||
GobackendCancelDownload(itemId)
|
||||
return nil
|
||||
|
||||
|
||||
case "resetDownloadCancel":
|
||||
let args = call.arguments as! [String: Any]
|
||||
let itemId = args["item_id"] as! String
|
||||
GobackendResetDownloadCancel(itemId)
|
||||
return nil
|
||||
|
||||
case "setDownloadDirectory":
|
||||
let args = call.arguments as! [String: Any]
|
||||
let path = args["path"] as! String
|
||||
|
||||
@@ -2566,6 +2566,14 @@ class DownloadQueueNotifier extends Notifier<DownloadQueueState> {
|
||||
}
|
||||
|
||||
_log.i('Retrying item: ${item.track.name} (id: $id)');
|
||||
// A cancel issued while the item never started leaves a pre-registered
|
||||
// flag in the Go backend that the next attempt would consume and abort
|
||||
// instantly; the user asked for a retry, so drop it first.
|
||||
unawaited(
|
||||
PlatformBridge.resetDownloadCancel(id).catchError((Object e) {
|
||||
_log.w('Failed to reset cancel flag for $id: $e');
|
||||
}),
|
||||
);
|
||||
_locallyCancelledItemIds.remove(id);
|
||||
_verificationRetriedItemIds.removeWhere(
|
||||
(retryKey) => retryKey == id || retryKey.startsWith('$id::'),
|
||||
@@ -2619,6 +2627,13 @@ class DownloadQueueNotifier extends Notifier<DownloadQueueState> {
|
||||
}
|
||||
|
||||
_log.i('Retrying ${failedIds.length} failed download(s)');
|
||||
for (final id in failedIds) {
|
||||
unawaited(
|
||||
PlatformBridge.resetDownloadCancel(id).catchError((Object e) {
|
||||
_log.w('Failed to reset cancel flag for $id: $e');
|
||||
}),
|
||||
);
|
||||
}
|
||||
_locallyCancelledItemIds.removeAll(failedIds);
|
||||
_pausePendingItemIds.removeAll(failedIds);
|
||||
|
||||
@@ -4055,6 +4070,14 @@ class DownloadQueueNotifier extends Notifier<DownloadQueueState> {
|
||||
);
|
||||
if (snapshot['is_running'] != true) {
|
||||
await _clearNativeWorkerRunId(runId);
|
||||
// Items may have been requeued during reconciliation (e.g. batch
|
||||
// mates of a verification challenge); hand them to the queue.
|
||||
if (!state.isPaused &&
|
||||
state.items.any(
|
||||
(item) => item.status == DownloadStatus.queued,
|
||||
)) {
|
||||
Future.microtask(() => _processQueue());
|
||||
}
|
||||
break;
|
||||
}
|
||||
await Future<void>.delayed(const Duration(seconds: 1));
|
||||
@@ -4174,6 +4197,16 @@ class DownloadQueueNotifier extends Notifier<DownloadQueueState> {
|
||||
return false;
|
||||
}
|
||||
_log.e('Android native worker failed: $e', e, stack);
|
||||
// The native worker keeps downloading on its own; without cancelling
|
||||
// it here the batch we just marked failed would continue in the
|
||||
// background, its completions never reconciled, and the Dart queue
|
||||
// could even restart alongside it.
|
||||
try {
|
||||
await PlatformBridge.cancelNativeDownloadWorker();
|
||||
} catch (cancelError) {
|
||||
_log.w('Failed to cancel native worker after error: $cancelError');
|
||||
}
|
||||
await _clearNativeWorkerRunId(runId);
|
||||
for (final item in queuedItems) {
|
||||
final current = _findItemById(item.id);
|
||||
if (current == null ||
|
||||
@@ -4543,11 +4576,36 @@ class DownloadQueueNotifier extends Notifier<DownloadQueueState> {
|
||||
_log.i(
|
||||
'Android native worker requires verification for ${current.track.name}; switching back to interactive queue',
|
||||
);
|
||||
// Cancelling the native worker marks every remaining batch item
|
||||
// "skipped" on the native side. Those items did nothing wrong —
|
||||
// remember them now and requeue them below so the batch resumes
|
||||
// after the challenge instead of being abandoned.
|
||||
final pendingBatchIds = <String>[];
|
||||
for (final pendingId in contexts.keys) {
|
||||
if (pendingId == itemId || reconciledIds.contains(pendingId)) {
|
||||
continue;
|
||||
}
|
||||
final pending = _findItemById(pendingId);
|
||||
if (pending == null) continue;
|
||||
if (pending.status == DownloadStatus.queued ||
|
||||
pending.status == DownloadStatus.downloading ||
|
||||
pending.status == DownloadStatus.finalizing) {
|
||||
pendingBatchIds.add(pendingId);
|
||||
}
|
||||
}
|
||||
try {
|
||||
await PlatformBridge.cancelNativeDownloadWorker();
|
||||
} catch (e) {
|
||||
_log.w('Failed to cancel native worker before verification: $e');
|
||||
}
|
||||
for (final pendingId in pendingBatchIds) {
|
||||
reconciledIds.add(pendingId);
|
||||
updateItemStatus(
|
||||
pendingId,
|
||||
DownloadStatus.queued,
|
||||
progress: 0.0,
|
||||
);
|
||||
}
|
||||
await _handleVerificationRequiredDownload(
|
||||
current,
|
||||
errorMsg,
|
||||
|
||||
@@ -541,6 +541,12 @@ class PlatformBridge {
|
||||
await _channel.invokeMethod('cancelDownload', {'item_id': itemId});
|
||||
}
|
||||
|
||||
/// Drops a stale pre-registered cancel flag for an item with no active
|
||||
/// download, so a user-initiated retry does not abort instantly.
|
||||
static Future<void> resetDownloadCancel(String itemId) async {
|
||||
await _channel.invokeMethod('resetDownloadCancel', {'item_id': itemId});
|
||||
}
|
||||
|
||||
static Future<void> setDownloadDirectory(String path) async {
|
||||
await _channel.invokeMethod('setDownloadDirectory', {'path': path});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user