From 32bb35e286f263cc0b85fafb3f330b4813ed60aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguye=CC=82=CC=83n=20Tua=CC=82=CC=81n=20Vie=CC=A3=CC=82t?= Date: Thu, 24 Sep 2026 07:24:15 +0700 Subject: [PATCH 1/5] fix(ios): stopAllWorkers() (BGTask expiration) never marked the cancellation registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improvement-ideas item #1 (smallest, lowest-risk of the list) from the post-audit brainstorm. Pre-existing gap, confirmed via `git show` at this branch's base — predates every commit in the lib/ audit PR, not a regression from it. handleCancel/cancelAll/cancelByTag all call DartTaskCancellationRegistry.shared.markCancelled(taskId) before cancelling the Swift Task, so a DartWorker callback cooperatively polling isTaskCancelled() finds out. stopAllWorkers() — called when the OS actually reclaims a BGTask's time (onExpiration) — only ever called .cancel() on the Task, never touched the registry. A DartWorker running past its BGTask's window, polling isTaskCancelled() the way #66/#72 tell it to, would never see the reclaim. Verification: `flutter analyze` 0 issues, full `flutter test` suite green, and the Cancellation device-test group (8 tests, all passing) re-run on both the iOS simulator and a physical iPhone 14 Pro Max (Linkpinky) after this change — confirms it didn't regress anything already covered. What could NOT be verified end-to-end, and why: this specific function only runs on a REAL BGTask time-limit expiration (BGTaskScheduler's onExpiration callback), which requires either waiting out an actual OS-scheduled BGProcessingTask window (unpredictable, iOS decides when to run it at all) or an attached-debugger `_simulateExpirationForTaskWithIdentifier:` call, neither available in this session. Checked: no test in this repo's history has ever exercised stopAllWorkers()/onExpiration (confirmed via grep across example/integration_test/) — this isn't a verification step I skipped, it's one nobody has built the harness for yet. --- .../native_workmanager/NativeWorkmanagerPlugin.swift | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift index 9508596..9c30e47 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift @@ -478,7 +478,15 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { internal func stopAllWorkers() { stateQueue.sync(flags: .barrier) { - for (_, task) in activeTasks { + for (taskId, task) in activeTasks { + // Pre-existing gap, found during the 2026-09-23 lib/ audit: this + // used to cancel the Swift Task without ever marking the + // registry, so a DartWorker callback cooperatively polling + // isTaskCancelled() during a real OS-triggered BGTask + // expiration would never find out — the same information + // handleCancel/cancelAll/cancelByTag already give an + // explicitly-cancelled task. + DartTaskCancellationRegistry.shared.markCancelled(taskId) task.cancel() } activeTasks.removeAll() From 18859a1dd84645236490711a6b7c2b1f90236b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguye=CC=82=CC=83n=20Tua=CC=82=CC=81n=20Vie=CC=A3=CC=82t?= Date: Thu, 24 Sep 2026 07:32:30 +0700 Subject: [PATCH 2/5] chore: retrigger PR title validation after renaming the PR From 96d4b1bd7c376611ff67b17c345562ba641223fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguye=CC=82=CC=83n=20Tua=CC=82=CC=81n=20Vie=CC=A3=CC=82t?= Date: Thu, 24 Sep 2026 11:41:41 +0700 Subject: [PATCH 3/5] =?UTF-8?q?fix(ios):=20handleResume=20never=20register?= =?UTF-8?q?ed=20its=20Task=20=E2=80=94=20pause+resume=20could=20run=20a=20?= =?UTF-8?q?task=20twice=20concurrently?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improvement-ideas item #2. Investigating turned up a bigger bug than the brainstorm framing ("existingPolicy can't see a resumed task as running"): handlePause's ONLY actual stop mechanism is BackgroundSessionManager.pause(), which looks taskId up in the background URL session's OWN task list. For a plain DartWorker — or ANY non-background-session task — that lookup finds nothing, calls back false, and NOTHING about the real running execution is touched. handlePause still unconditionally relabels the task "paused" regardless of that result. handleResume then built a brand-new, entirely untracked `Task { }` to re-run the persisted config under the same taskId — never checking activeTasks first, never registering itself there either. handleResume also has no gate on the task's actual status: it re-executes ANY taskId with a non-empty persisted workerClassName, paused or not, completed or not, HttpDownloadWorker or not — "only HttpDownloadWorker supports pause/resume" turned out to be a documented convention the native code never actually enforces. Net effect: pause()-then-resume() on an ordinary (non-background-session) task could run it TWICE concurrently — the original, never actually paused, and the freshly resumed one — both writing to the same output, both reporting completion for the same taskId. Extracted the exact "cancel whatever's registered for taskId, then register a fresh execution with a generation-guarded cleanup" sequence out of handleEnqueue's existingPolicy handling into a shared `replaceActiveTask` helper, rather than hand-copying it a second time — duplicating this exact logic once already is how the previous two fixes in this branch happened (stopAllWorkers missing a markCancelled call, the natural-completion generation bug). handleResume now routes through it, so a still-running task is cancelled before the resumed execution starts, exactly like existingPolicy: .replace already does for a repeat enqueue(). New test: lib_audit_4 in device_integration_test.dart. Needed a new DartWorker callback (dit_pause_resume_log) — a plain counter file couldn't distinguish "one execution ran serially" from "two executions raced," since resume() reuses the same persisted input (same output path) as the original. The new callback appends " " lines instead, so the test can group by execution and assert the outgoing one was cut short while the resumed one ran to completion. Red-then-green, on an iOS simulator: stashed this commit's Swift changes (keeping the new test) and reran lib_audit_4 — both executions logged all 50 iterations each, confirming the double-execution bug the test is meant to catch. Restored the fix, reran: outgoing execution stopped early, resumed execution completed, distinct executionIds, exactly as expected. Then rechecked for regressions: the full Cancellation group (9 tests, was 8) green on the simulator, and separately the 6 Android-applicable tests in that group green on a physical Pixel 6 Pro (issue_66, issue_75 x2, issue_72 — the Android original of the bug shape ported to iOS in this branch's earlier commits — plus the 5 iOS-only tests correctly skipping). Could not verify on a physical iPhone this session — the iPhone 14 Pro Max used earlier for real-hardware confirmation disconnected mid-session and did not reconnect; an iPhone 6s Plus made available instead turned out to be permanently incompatible with the installed Xcode 27 for on-device run/debug (SDK-support boundary, not a config issue — see the pause note in android_device_test_harness.md). flutter analyze: 0 issues. flutter test: 2056/2056 passing. --- .../device_integration_test.dart | 213 +++++++++++++++--- .../NativeWorkmanagerPlugin+Cancel.swift | 13 +- .../NativeWorkmanagerPlugin.swift | 87 ++++--- 3 files changed, 258 insertions(+), 55 deletions(-) diff --git a/example/integration_test/device_integration_test.dart b/example/integration_test/device_integration_test.dart index d5db719..486266b 100644 --- a/example/integration_test/device_integration_test.dart +++ b/example/integration_test/device_integration_test.dart @@ -278,6 +278,33 @@ Future _ditStopNoPoll(Map? input) async { return true; } +/// Issue lib_audit_4 (iOS pause()/resume() double-execution check, +/// 2026-09-24 improvement pass): appends " " lines to +/// [logFile] so the test can tell one execution running serially (a short +/// prefix from the cancelled-out outgoing execution, then a fresh 1..50 run +/// from the resumed one) apart from two executions racing concurrently +/// (interleaved executionIds within the same time window). Polls +/// isTaskCancelled() like dit_cancel_poll — needed so the OUTGOING execution +/// has any way to notice replaceActiveTask cancelled it. +@pragma('vm:entry-point') +Future _ditPauseResumeLog(Map? input) async { + final taskId = input?['__taskId'] as String?; + final executionId = input?['__executionId'] as String? ?? 'unknown'; + final logFile = input?['logFile'] as String?; + for (var i = 1; i <= 50; i++) { + if (logFile != null) { + File( + logFile, + ).writeAsStringSync('$executionId $i\n', mode: FileMode.append); + } + if (taskId != null && await NativeWorkManager.isTaskCancelled(taskId)) { + return false; + } + await Future.delayed(const Duration(milliseconds: 200)); + } + return true; +} + /// Wait (bounded) for a DartWorker callback to create [file]. /// /// Issue #75: a fixed delay is not enough to know a callback has started — a @@ -352,13 +379,12 @@ void main() { 'dit_retry_counter': _ditRetryCounter, 'dit_cancel_poll': _ditCancelPoll, 'dit_stop_no_poll': _ditStopNoPoll, + 'dit_pause_resume_log': _ditPauseResumeLog, 'workflow_finalizer': _workflowFinalizer, }, // Issue #75: stop handlers live in their own registry — a // DartWorkerStoppedCallback returns void, so it cannot share dartWorkers. - onStoppedHandlers: { - 'dit_on_stopped': _ditOnStopped, - }, + onStoppedHandlers: {'dit_on_stopped': _ditOnStopped}, ); // Cancel any leftover tasks from previous runs. @@ -1867,7 +1893,8 @@ void main() { expect( started, isTrue, - reason: 'issue_75: the callback must have started before being ' + reason: + 'issue_75: the callback must have started before being ' 'cancelled, or this test proves nothing', ); @@ -1879,14 +1906,16 @@ void main() { expect( markerFile.existsSync(), isTrue, - reason: 'issue_75: the onStopped handler never ran. Either the native ' + reason: + 'issue_75: the onStopped handler never ran. Either the native ' 'side did not invoke onTaskStopped/onDartTaskStopped, or the ' 'handle/id never crossed the bridge.', ); expect( markerFile.readAsStringSync().trim(), equals(id), - reason: 'issue_75: the handler ran but did not receive __taskId, so a ' + reason: + 'issue_75: the handler ran but did not receive __taskId, so a ' 'shared handler could not tell which task stopped', ); }, @@ -1928,14 +1957,18 @@ void main() { worker: DartWorker( callbackId: 'dit_stop_no_poll', onStoppedId: 'dit_on_stopped', - cancelGrace: Duration.zero, // tear down as soon as the handler returns + cancelGrace: + Duration.zero, // tear down as soon as the handler returns autoDispose: true, input: {'counterFile': counterFile.path}, ), ); - expect(await _waitForFile(counterFile), isTrue, - reason: 'issue_75: the callback must have started before cancel'); + expect( + await _waitForFile(counterFile), + isTrue, + reason: 'issue_75: the callback must have started before cancel', + ); await NativeWorkManager.cancel(taskId: id); // Past the grace, the engine should be gone and the counter frozen. @@ -1948,14 +1981,16 @@ void main() { expect( later, equals(afterTeardown), - reason: 'issue_75: the counter is still climbing after cancelGrace ' + reason: + 'issue_75: the counter is still climbing after cancelGrace ' 'elapsed — the engine was never torn down, so an uncooperative ' 'callback still runs to completion', ); expect( later, lessThan(45), - reason: 'issue_75: the callback got essentially all 50 iterations, ' + reason: + 'issue_75: the callback got essentially all 50 iterations, ' 'meaning teardown never happened', ); }, @@ -2100,10 +2135,12 @@ void main() { } final id = _id('lib_audit_3_replace'); - final oldCounterFile = - File('${tmpDir.path}/lib_audit_3_replace_old.txt'); - final newCounterFile = - File('${tmpDir.path}/lib_audit_3_replace_new.txt'); + final oldCounterFile = File( + '${tmpDir.path}/lib_audit_3_replace_old.txt', + ); + final newCounterFile = File( + '${tmpDir.path}/lib_audit_3_replace_new.txt', + ); await NativeWorkManager.enqueue( taskId: id, @@ -2133,12 +2170,14 @@ void main() { isTrue, reason: 'lib_audit_3: the replaced execution must have started', ); - final oldIterations = - int.parse(oldCounterFile.readAsStringSync().trim()); + final oldIterations = int.parse( + oldCounterFile.readAsStringSync().trim(), + ); expect( oldIterations, lessThan(50), - reason: 'lib_audit_3: the replaced execution must have observed ' + reason: + 'lib_audit_3: the replaced execution must have observed ' 'cancellation and stopped', ); @@ -2147,12 +2186,14 @@ void main() { isTrue, reason: 'lib_audit_3: the replacement execution must have started', ); - final newIterations = - int.parse(newCounterFile.readAsStringSync().trim()); + final newIterations = int.parse( + newCounterFile.readAsStringSync().trim(), + ); expect( newIterations, equals(50), - reason: 'lib_audit_3: the replacement must run to completion — a ' + reason: + 'lib_audit_3: the replacement must run to completion — a ' 'lower count means it inherited the replaced execution\'s ' 'stale cancellation mark and self-aborted', ); @@ -2208,7 +2249,8 @@ void main() { expect( newCounterFile.existsSync(), isFalse, - reason: 'lib_audit_3: keep must ignore the new request entirely — ' + reason: + 'lib_audit_3: keep must ignore the new request entirely — ' 'no second execution should ever have started', ); }, @@ -2244,16 +2286,21 @@ void main() { worker: DartWorker(callbackId: 'dit_pass'), ); final first = await firstEvent; - expect(first?.success, isTrue, - reason: 'lib_audit_3: the first execution must complete'); + expect( + first?.success, + isTrue, + reason: 'lib_audit_3: the first execution must complete', + ); // Give the natural-completion cleanup a moment to run before // re-enqueuing, so this genuinely exercises the "already finished, // not just finishing" case. await Future.delayed(const Duration(seconds: 2)); - final secondEvent = - _waitEvent(id, timeout: const Duration(seconds: 15)); + final secondEvent = _waitEvent( + id, + timeout: const Duration(seconds: 15), + ); await NativeWorkManager.enqueue( taskId: id, trigger: const TaskTrigger.oneTime(), @@ -2264,13 +2311,125 @@ void main() { expect( second?.success, isTrue, - reason: 'lib_audit_3: a taskId reused after its previous execution ' + reason: + 'lib_audit_3: a taskId reused after its previous execution ' 'already completed must run the new request — keep must not ' 'mistake a long-finished task for one still running', ); }, ); + testWidgets( + 'lib_audit_4: pausing a non-background-session task then resuming it ' + 'does not leave the outgoing execution running as an untracked zombie ' + 'alongside the resumed one (iOS)', + (tester) async { + // Found in the 2026-09-24 improvement pass while fixing item #2 on + // the post-audit list ("handleResume doesn't register in + // activeTasks"). The real bug turned out bigger than that framing: + // handlePause()'s only actual stop mechanism is + // BackgroundSessionManager.pause(), which looks the taskId up in the + // background URL session's OWN task list — for a plain DartWorker (or + // any non-background-session task) that lookup finds nothing, calls + // back false, and NOTHING about the real running execution is + // touched. handlePause still unconditionally relabels the task + // "paused" regardless. handleResume then built a brand-new, entirely + // untracked Task to re-run the persisted config under the same + // taskId — so pause()-then-resume() on an ordinary task could run + // TWO concurrent executions: the original, never actually paused, + // and the resumed one. + // + // Fixed by routing handleResume through the same replaceActiveTask + // helper existingPolicy: .replace uses — it cancels+marks whatever's + // still registered for the taskId before starting the resumed + // execution, exactly like a repeat enqueue() does. + if (!Platform.isIOS) { + markTestSkipped('iOS pause()/resume() path'); + return; + } + + final id = _id('lib_audit_4_pause_resume'); + final logFile = File('${tmpDir.path}/lib_audit_4_log.txt'); + if (logFile.existsSync()) logFile.deleteSync(); + + await NativeWorkManager.enqueue( + taskId: id, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker( + callbackId: 'dit_pause_resume_log', + input: {'logFile': logFile.path}, + ), + ); + + // Let the original execution actually start and log a few iterations. + await Future.delayed(const Duration(milliseconds: 600)); + + // DartWorker isn't a background-session download, so pause() cannot + // actually stop the underlying execution — it only relabels state. + await NativeWorkManager.pause(taskId: id); + await NativeWorkManager.resume(taskId: id); + + // Long enough for a full uncancelled 50-iteration run (~10s) to + // finish, whether that's the resumed execution behaving correctly or + // a zombie original proving the bug is back. + await Future.delayed(const Duration(seconds: 12)); + + expect( + logFile.existsSync(), + isTrue, + reason: + 'lib_audit_4: the original execution must have started ' + 'logging before pause/resume were called', + ); + + final lines = logFile + .readAsLinesSync() + .where((l) => l.trim().isNotEmpty) + .map((l) => l.split(' ')) + .toList(); + final byExecution = >{}; + for (final parts in lines) { + final execId = parts[0]; + final iter = int.parse(parts[1]); + (byExecution[execId] ??= []).add(iter); + } + + expect( + byExecution.length, + equals(2), + reason: + 'lib_audit_4: expected exactly two executions (the ' + 'original and the resumed one) to have logged anything at ' + 'all — got ${byExecution.length}: ${byExecution.keys}', + ); + + final maxIterations = byExecution.values.map( + (v) => v.reduce((a, b) => a > b ? a : b), + ); + final sorted = maxIterations.toList()..sort(); + final outgoingMax = sorted.first; + final resumedMax = sorted.last; + + expect( + outgoingMax, + lessThan(30), + reason: + 'lib_audit_4: the outgoing (pre-resume) execution must ' + 'have been cancelled shortly after resume() started a new ' + 'one — reaching anywhere near 50 means it kept running ' + 'unimpeded as an untracked zombie, the exact double-execution ' + 'this fix closes. Per-execution max iterations: $byExecution', + ); + expect( + resumedMax, + equals(50), + reason: + 'lib_audit_4: the resumed execution must run to ' + 'completion. Per-execution max iterations: $byExecution', + ); + }, + ); + testWidgets( 'issue_69: cancelling a background-session download actually aborts the transfer (iOS)', (tester) async { diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift index dc0221d..f005acf 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift @@ -175,7 +175,18 @@ extension NativeWorkmanagerPlugin { } taskStore?.updateStatus(taskId: taskId, status: "pending") stateQueue.async(flags: .barrier) { self.taskStates[taskId] = .pending } - Task { [weak self] in + // Found in the 2026-09-24 iOS improvement pass: this used to be a bare, + // untracked `Task { }` — never registered in activeTasks, so a + // resumed task could neither be cancel()'d nor be seen by + // existingPolicy, AND (the more serious half) handlePause() never + // actually stops anything for a task BackgroundSessionManager + // doesn't recognize as a real download — so pausing then resuming a + // plain (non-background-session) task could run it TWICE + // concurrently: the original, never-actually-paused execution, and + // this fresh one. replaceActiveTask cancels whatever's still + // registered for taskId first, exactly like existingPolicy: .replace + // does in handleEnqueue, then registers this execution the same way. + replaceActiveTask(taskId: taskId) { [weak self] in await self?.executeWorkerSync( taskId: taskId, workerClassName: record.workerClassName, diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift index 9c30e47..a4bd91c 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift @@ -400,16 +400,64 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { // the new one; "keep" leaves the running execution alone and ignores the new // request, matching WorkManager's ExistingWorkPolicy.KEEP on Android, which also // always reports the enqueue as accepted regardless of whether it was a no-op. - // - // This whole check-decide-store sequence is one atomic stateQueue block so two - // overlapping handleEnqueue calls for the same taskId can't both see "nothing - // running yet" and both proceed. let existingPolicyStr = (args["existingPolicy"] as? String)?.lowercased() ?? "replace" var skippedForKeep = false + replaceActiveTask( + taskId: taskId, + skipIfAlreadyRunning: existingPolicyStr == "keep", + onSkipped: { skippedForKeep = true } + ) { [weak self] in + guard let self else { return } + if initialDelayMs > 0 { + try? await Task.sleep(nanoseconds: UInt64(initialDelayMs) * 1_000_000) + } + guard !Task.isCancelled else { return } + await self.executeWorkerSync( + taskId: taskId, + workerClassName: workerClassName, + workerConfig: workerConfig, + qos: directQos, + retryConfig: directRetryConfig + ) + } + if skippedForKeep { + NativeLogger.d("handleEnqueue: '\(taskId)' already running, existingPolicy=keep — new request ignored") + } + + result("ACCEPTED") + } + + /// Cancels whatever is currently registered for `taskId` in `activeTasks` and + /// registers a fresh execution, atomically — the "check-decide-store" sequence + /// `handleEnqueue`'s `existingPolicy` handling and `handleResume` both need, pulled + /// into one place after duplicating it once already produced a gap (`handleResume` + /// used to build its own untracked `Task {}`, found in the 2026-09-24 iOS + /// improvement pass: a paused-then-resumed NON-background-session task could run + /// TWO concurrent executions, because `handlePause` never actually stops anything + /// for a task `BackgroundSessionManager` doesn't recognize as a real download, and + /// the resumed `Task` was never registered in `activeTasks` for anything to check + /// against). + /// + /// The whole thing runs inside one `stateQueue` barrier block so two overlapping + /// calls for the same `taskId` (an enqueue racing a resume, a resume racing another + /// resume, ...) can't both see "nothing running yet" and both proceed. + /// + /// - Parameters: + /// - skipIfAlreadyRunning: `true` for `existingPolicy: .keep` — leaves a running + /// execution alone and calls `onSkipped` instead of starting `work`. + /// - work: the body to run as the new tracked `Task`. Checking `Task.isCancelled` + /// inside `work` (e.g. after an initial delay) is the caller's job, same as + /// before this was extracted. + func replaceActiveTask( + taskId: String, + skipIfAlreadyRunning: Bool = false, + onSkipped: (() -> Void)? = nil, + work: @escaping () async -> Void + ) { stateQueue.sync(flags: .barrier) { if let existingTask = self.activeTasks[taskId] { - if existingPolicyStr == "keep" { - skippedForKeep = true + if skipIfAlreadyRunning { + onSkipped?() return } // Cancelling the Swift Task only unblocks whatever it's synchronously @@ -425,42 +473,27 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { // See activeTaskGenerations' doc comment: this id is what lets the // Task below tell, once IT finishes, whether it is still the // current occupant of activeTasks[taskId] — a naturally-completing - // task must remove its own entry so a later enqueue() doesn't - // mistake a long-finished taskId for one still running. + // task must remove its own entry so a later call doesn't mistake a + // long-finished taskId for one still running. let generationId = UUID() let task = Task { [weak self] in guard let self else { return } defer { self.stateQueue.sync(flags: .barrier) { // Only clear if nothing has replaced us in the meantime — - // a .replace enqueue() racing in after we started but - // before we finish must not have its brand-new entry - // wiped out by our own late cleanup. + // a call racing in after we started but before we finish + // must not have its brand-new entry wiped out by our own + // late cleanup. guard self.activeTaskGenerations[taskId] == generationId else { return } self.activeTasks.removeValue(forKey: taskId) self.activeTaskGenerations.removeValue(forKey: taskId) } } - if initialDelayMs > 0 { - try? await Task.sleep(nanoseconds: UInt64(initialDelayMs) * 1_000_000) - } - guard !Task.isCancelled else { return } - await self.executeWorkerSync( - taskId: taskId, - workerClassName: workerClassName, - workerConfig: workerConfig, - qos: directQos, - retryConfig: directRetryConfig - ) + await work() } self.activeTasks[taskId] = task self.activeTaskGenerations[taskId] = generationId } - if skippedForKeep { - NativeLogger.d("handleEnqueue: '\(taskId)' already running, existingPolicy=keep — new request ignored") - } - - result("ACCEPTED") } @available(iOS 13.0, *) From 87e233592aff76c1e29d1828512e0521f57a3649 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguye=CC=82=CC=83n=20Tua=CC=82=CC=81n=20Vie=CC=A3=CC=82t?= Date: Thu, 24 Sep 2026 12:33:25 +0700 Subject: [PATCH 4/5] fix(ios): BGTaskScheduler's periodic path never cleared its activeTasks entry on natural completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improvement-ideas item #3. Same bug shape as the previous two commits in this branch: onTaskRunning's closure stored the OS-triggered periodic/ refresh Task in activeTasks with no generation tracking, and nothing ever removed the entry when the task finished NATURALLY (success or failure) — only expiration self-heals, via stopAllWorkers()'s activeTasks.removeAll(). Left uncorrected, a periodic taskId that already ran once and finished would look "still running" forever to existingPolicy or an explicit cancel(taskId) — the same stale-liveness bug already fixed for the direct- enqueue and resume paths, reachable here if an app reuses a taskId string between a periodic schedule and a one-time enqueue. Couldn't reuse replaceActiveTask directly: BGTaskSchedulerManager creates and owns the Task itself (driving the actual BGProcessingTask/ BGAppRefreshTask lifecycle), so this closure can't wrap its body the way handleEnqueue/handleResume wrap their own `Task { }`. Instead: mint a generation id when registering, and spawn a small observer Task that awaits `runningTask.value` (suspends until the task's closure returns; Never means it can't throw) and then runs the identical "clear only if still current" cleanup used everywhere else in this branch. Verification: this needs a REAL BGTaskScheduler launch to exercise the code path end to end, which turned out unverifiable in this environment for reasons worth recording precisely, not just "couldn't test it": - Re-ran the repo's existing periodic-firing device test ("Trigger Types > periodic – first execution fires") on the iOS simulator in isolation before writing this fix. It ran the full 2-minute timeout and fell into its already-existing fallback path — confirming BGTaskScheduler does not grant a real background launch within an ordinary test session on the simulator, consistent with it being an OS-timed decision, not something an app (or a test) can request on demand. - This is not a simulator-only limitation. A real device — including one rented for a few minutes on Firebase Test Lab — faces the identical constraint: the OS decides when to grant a background execution window, and a short test session cannot force that. The one thing that reliably forces a real launch is Apple's private `_simulateLaunchForTaskWithIdentifier:` selector via an attached debugger or an in-process call to it — this plugin has no bridge exposing that today, and building one is new production-adjacent surface that deserves its own review, not something to add as a side effect of verifying this fix. - What WAS verified: full regression pass, since this change only adds bookkeeping around an already-running Task and touches no scheduling logic. Cancellation group (9/9), Trigger Types group (4/4, including the above periodic test taking its normal fallback path unchanged), and Issue #36 BGTask registration test (1/1) — all on the iOS simulator. Cancellation group's 6 Android-applicable tests also re-run green on a physical Pixel 6 Pro. flutter analyze: 0 issues. flutter test: 2056/2056. Still could not get the iPhone 14 Pro Max (Linkpinky) back online this session to add a physical-iOS data point beyond what item 1/2's commits already captured. --- .../NativeWorkmanagerPlugin.swift | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift index a4bd91c..7117d2c 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift @@ -192,8 +192,38 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { BGTaskSchedulerManager.shared.onTaskRunning = { [weak instance] taskId, runningTask in // Track OS-triggered running tasks so NativeWorkManager.cancel(taskId) can // cancel the Swift Task via cooperative cancellation. - instance?.stateQueue.sync(flags: .barrier) { - instance?.activeTasks[taskId] = runningTask + guard let instance else { return } + // Improvement pass, 2026-09-24: this used to just store the Task with no + // generation tracking, so — same bug shape as handleEnqueue/handleResume + // before they were fixed — a periodic/refresh task that finishes NATURALLY + // (not via expiration) never had its activeTasks entry cleared, leaving a + // stale "still running" signal for that taskId forever (existingPolicy + // could misread it, cancel(taskId) would try to cancel an already-finished + // Task). Expiration already self-heals via stopAllWorkers(), which clears + // everything — this is specifically for the non-expiring completion path. + // + // BGTaskSchedulerManager creates and owns `runningTask` itself (it's what + // actually drives the BGProcessingTask/BGAppRefreshTask lifecycle), so this + // closure can't wrap its body in a defer the way handleEnqueue/handleResume + // wrap their own `Task { }` via replaceActiveTask. Instead, observe its + // completion from the outside: `Task.value` suspends until the + // task's closure returns — success, failure, or an early cancelled return — + // then run the exact same "clear only if still current" cleanup as + // everywhere else, guarding against a taskId that got replaced in the + // meantime. + let generationId = UUID() + instance.stateQueue.sync(flags: .barrier) { + instance.activeTasks[taskId] = runningTask + instance.activeTaskGenerations[taskId] = generationId + } + Task { [weak instance] in + _ = await runningTask.value + guard let instance else { return } + instance.stateQueue.sync(flags: .barrier) { + guard instance.activeTaskGenerations[taskId] == generationId else { return } + instance.activeTasks.removeValue(forKey: taskId) + instance.activeTaskGenerations.removeValue(forKey: taskId) + } } } BackgroundSessionManager.shared.richProgressDelegate = { [weak instance] _, dict in From 1956586728369d9e6681bcd050c063587b0097b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguye=CC=82=CC=83n=20Tua=CC=82=CC=81n=20Vie=CC=A3=CC=82t?= Date: Thu, 24 Sep 2026 16:59:16 +0700 Subject: [PATCH 5/5] =?UTF-8?q?fix(ios):=20DartCallbackWorker=20executionI?= =?UTF-8?q?d=20registration=20gap=20was=20not=20narrow=20=E2=80=94=20repro?= =?UTF-8?q?duces=20with=205=20DartWorkers=20in=20flight?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improvement-ideas item #4, scoped going in as closing a "narrow race" in v1.8.3's CHANGELOG. It isn't narrow. handleEnqueue/handleResume used to mint+register a DartCallbackWorker's executionId LAZILY, inside executeDartWorkerViaMethodChannel. A task that has passed the outer Task's `guard !Task.isCancelled` but is then parked inside ConcurrencyLimiter.acquire() (default cap: 4 concurrent) is past the only Swift-level cancellation check on this path — acquire()'s withCheckedContinuation has no cancellation handler, so Task.cancel() does not release it early, it only resumes once a slot frees — but it hasn't reached executeDartWorkerViaMethodChannel yet, so it had no registry entry either. A cancel(taskId) landing in that window fell back to marking the bare taskId. Once a slot freed and the real executionId was minted+registered, that bare-taskId mark was orphaned under the wrong key: the freshly-registered execution polled isTaskCancelled() against its OWN executionId, found nothing, and ran on uncancelled. Reproduces with only 5 DartWorkers ever in flight — no artificial delay or back-to-back enqueue/cancel needed. This is a regression from the issue #72 executionId port to iOS (0f8620a), not a pre-existing gap: before that port, the registry was a bare Set of taskIds, so a cancel() landing at any point — including while parked — was visible at the very next poll. Porting to per-execution keys (needed to fix #72's own bug: two executions of one taskId clobbering each other's mark) reintroduced this as a side effect. 0f8620a is already on main (pubspec says 1.8.3) but that version has not been tagged or published to pub.dev, so this is still fixable before release — flagging for a decision on whether it lands in 1.8.3 or ships separately. Fix: replaceActiveTask now optionally accepts a pre-minted dartExecutionId and, when given one, calls beginExecution for it synchronously inside the same barrier block that cancels/replaces whatever's currently registered for taskId — before enqueue()'s or resume()'s Future even resolves in Dart. handleEnqueue's direct path and handleResume now mint this id (only for DartCallbackWorker) and thread it through executeWorkerSync -> _executeWorker -> executeDartWorkerViaMethodChannel via a new preMintedExecutionId parameter, restricted to attempt 1 of executeWorkerSync's retry loop (a retry is a fresh execution nothing external raced against, so it mints its own lazily like before — reusing the attempt-1 id across retries would be actively wrong, since attempt 1's own cleanup clears the registry entry when it finishes). Scope of what this actually closes: attempt 1 of handleEnqueue's direct path and handleResume only. Chains, TaskGraph, BGTaskScheduler's periodic path, the offline queue, and retry attempts 2+ still call executeDartWorkerViaMethodChannel without a pre-minted id and still have this exact gap. Tracked as a follow-up on PR #83, not fixed here. iOS-only: Android's DartCallbackWorker mints its executionId inside doWork(), which WorkManager only invokes once it has already decided to run the request — a WorkRequest cancelled while still queued never reaches doWork() at all, so there's no equivalent in-process concurrency-limiter gap to reproduce there. New device test (lib_audit_5) skips on Android with a stated reason, matching the existing issue_72 pattern. Also fixes a leak found before committing (advisor review caught it): the pre-minted executionId is registered in replaceActiveTask, but only executeDartWorkerViaMethodChannel's own defer ended it. Any exit before reaching that function — handleEnqueue's guard !Task.isCancelled after the initialDelay sleep, executeWorkerSync's per-attempt top-of-loop guard, or self being nil — never called endExecution, leaking the registry entry and leaving currentExecutionId[taskId] pointing at a dead id forever. replaceActiveTask's Task now unconditionally defers endExecution for its pre-minted id; idempotent alongside executeDartWorkerViaMethodChannel's own defer. Verification: red-then-green on an iOS simulator (stashed the Swift changes, confirmed lib_audit_5 fails with B running 15/50 iterations uncancelled, restored, confirmed it passes at iteration 1). Full Cancellation group (10/10) green after. flutter analyze: 0 issues (both packages). dart format: clean. flutter test test/unit/: 1258/1258. ./scripts/run_all_tests.sh: all 6 suites green. No real device (Android or iOS) was available this session — no adb device attached, and the iPhone used earlier in this branch's other commits did not reconnect — so this has only been verified on the iOS simulator plus the full host-only test suite. CHANGELOG.md corrects the "narrow window... not a pattern normal usage hits" characterization in the existing 1.8.3 entry, which undersold this. --- CHANGELOG.md | 50 ++++++ .../device_integration_test.dart | 166 +++++++++++++++++- .../NativeWorkmanagerPlugin+Cancel.swift | 10 +- .../NativeWorkmanagerPlugin+Execution.swift | 59 ++++++- .../NativeWorkmanagerPlugin.swift | 65 ++++++- .../engine/DartTaskCancellationRegistry.swift | 15 +- 6 files changed, 345 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5753766..f59cb63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,56 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +iOS cancellation-registry improvement pass (branch +`ios-cancellation-registry-improvements`, PR #83, still a draft — not +released). Items 1–3 fix `activeTasks`/registry bookkeeping bugs found while +reviewing the 1.8.3 `existingPolicy`/executionId work for follow-ups +(`stopAllWorkers()` never marking the cancellation registry, `handleResume` +never registering its `Task` — pause+resume could run a task twice +concurrently, `BGTaskScheduler`'s periodic path never clearing its +`activeTasks` entry on natural completion). See their commits for full +detail. + +### Fixed + +- **Item 4, and a correction to 1.8.3's own CHANGELOG entry below**: that + entry describes the `executeDartWorkerViaMethodChannel` executionId + registration gap as "narrow" and "not a pattern normal usage hits." That + undersold it. The gap is real and reproduces under ordinary usage — a + `cancel(taskId)` landing while a `DartCallbackWorker` is parked inside + `ConcurrencyLimiter.acquire()` (the default cap is 4 concurrent tasks, so + a 5th enqueued DartWorker parks immediately) falls back to marking the + bare `taskId`, an orphaned entry once the task's own executionId + registers moments later once a slot frees — the task runs on, uncancelled. + Reproduced with only 5 DartWorkers ever in flight; no artificial delay or + back-to-back enqueue/cancel needed. `replaceActiveTask` now mints the + executionId and registers it synchronously, in the same barrier block + that accepts the enqueue — before the `enqueue()`/`resume()` Future even + resolves in Dart. + - Scope: this closes the gap for attempt 1 of `handleEnqueue`'s direct + (one-time) path and `handleResume` only — the two callers that now + pre-mint. Chains, `TaskGraph`, `BGTaskScheduler`'s periodic path, the + offline queue, and retry attempts 2+ within a single `enqueue()` call + still register lazily inside `executeDartWorkerViaMethodChannel` and + still have this exact gap. Tracked as a follow-up on PR #83. + - Android has no equivalent: `DartCallbackWorker` mints its executionId + inside `doWork()`, which WorkManager only invokes once it has already + decided to run the request — a request cancelled while still queued + never reaches `doWork()` at all. iOS-only, per `lib_audit_5` in + `device_integration_test.dart` (red-then-green verified on an iOS + simulator; no real device available this session). + - Also fixes a leak this introduced during development (caught before + committing): the pre-minted executionId is registered in + `replaceActiveTask`, but only `executeDartWorkerViaMethodChannel`'s own + `defer` ended it — any exit before reaching that function (the + `guard !Task.isCancelled` checks in `handleEnqueue`'s closure or + `executeWorkerSync`'s retry loop, or `self` being nil) never cleaned up + the registry entry. `replaceActiveTask`'s `Task` now unconditionally + calls `endExecution` for its pre-minted id in its own `defer`, + idempotent alongside `executeDartWorkerViaMethodChannel`'s. + ## [1.8.3] - 2026-09-24 Fixes 3 issues found by a full `lib/` audit (2026-09-23), reviewed in detail diff --git a/example/integration_test/device_integration_test.dart b/example/integration_test/device_integration_test.dart index 486266b..9634fd0 100644 --- a/example/integration_test/device_integration_test.dart +++ b/example/integration_test/device_integration_test.dart @@ -279,7 +279,7 @@ Future _ditStopNoPoll(Map? input) async { } /// Issue lib_audit_4 (iOS pause()/resume() double-execution check, -/// 2026-09-24 improvement pass): appends " " lines to +/// 2026-09-24 improvement pass): appends ` ` lines to /// [logFile] so the test can tell one execution running serially (a short /// prefix from the cancelled-out outgoing execution, then a fresh 1..50 run /// from the resumed one) apart from two executions racing concurrently @@ -2430,6 +2430,170 @@ void main() { }, ); + testWidgets('lib_audit_5: cancel() landing while parked on the concurrency ' + 'limiter resolves against the enqueued execution, not a stale ' + 'pre-registration gap (iOS)', (tester) async { + // 2026-09-24 improvement pass, item 4 of PR #83's "small to large" + // list — this turned out NOT to be the narrow, sub-microsecond race it + // was scoped as going in. handleEnqueue used to mint+register a + // DartCallbackWorker's executionId LAZILY, inside + // executeDartWorkerViaMethodChannel. A task that has passed + // handleEnqueue's `guard !Task.isCancelled` but is then parked inside + // `ConcurrencyLimiter.acquire()` (max 4 concurrent — see + // NativeWorkmanagerPlugin's `concurrencyLimiter`) is now PAST the only + // Swift-level cancellation check on this path (`acquire()`'s + // `withCheckedContinuation` has no cancellation handler, so + // `Task.cancel()` does not release it early — it only resumes once a + // slot frees) — but it hasn't reached executeDartWorkerViaMethodChannel + // yet, so pre-fix it had no registry entry either. A cancel(taskId) + // landing in that window fell back to marking the bare taskId as + // cancelled. Once a slot freed and the real executionId was finally + // minted+registered, that bare-taskId mark was an orphaned entry under + // the wrong key — the freshly-registered execution polled + // isTaskCancelled() against ITS OWN executionId, found nothing, and ran + // on uncancelled. This is ordinary usage, not a rare timing accident — + // it reproduces with only 5 DartWorkers ever in flight (the default + // `maxConcurrentTasks` is 4), confirmed first RED (15/50 iterations run + // uncancelled) then GREEN against this exact test. + // + // Regression, not a pre-existing gap: before the issue #72 executionId + // port to iOS (0f8620a, already on main / pubspec 1.8.3, but that + // version has NOT been tagged or published to pub.dev — see + // `git tag -l` / CHANGELOG), the registry was a bare `Set` of + // taskIds with no executionId concept, so a cancel() landing at any + // point — including while parked here — was visible at the very next + // poll regardless of when the callback actually started. Porting to + // per-execution keys (needed to fix issue #72's own bug: two + // executions of one taskId clobbering each other's mark) reintroduced + // this as a side effect, because the new registration point is deep + // inside executeDartWorkerViaMethodChannel rather than at "decided to + // run". + // + // (A cancel landing during a plain initialDelay does NOT reproduce + // this: `activeTasks[taskId]?.cancel()` + the `guard + // !Task.isCancelled` right after the delay's `Task.sleep` already + // stops it before it ever reaches the Dart callback, in both the + // pre-fix and post-fix code — that path is covered by the existing + // 'cancel by ID' test above.) + // + // Fix, and its actual scope: replaceActiveTask now mints the + // executionId and registers it (beginExecution) synchronously, inside + // the same barrier block that decides to run the enqueue — before + // enqueue()'s Future even resolves in Dart — so a cancel() landing at + // any point afterward, including while parked on the limiter, resolves + // against the correct execution. This closes the gap ONLY for attempt + // 1 of handleEnqueue's direct (one-time) path and handleResume — the + // two callers that now pre-mint. Chains (executeChain/resumeChain), + // TaskGraph, BGTaskScheduler's periodic path, the offline queue, and + // retry attempts 2+ within executeWorkerSync's own loop all still call + // executeDartWorkerViaMethodChannel without a pre-minted id, so they + // still register lazily and still have this exact gap. Tracked as a + // follow-up item on PR #83, not fixed by this commit. + // + // iOS-only: Android's DartCallbackWorker mints its executionId inside + // doWork(), which Android WorkManager only ever invokes once it has + // actually decided to run the request — a WorkRequest cancelled while + // still queued (WorkManager's own scheduler, not an in-process + // limiter) never reaches doWork() at all, so there is no equivalent + // gap to reproduce there. + // + // Reproduced here by saturating the (default max 4) limiter with + // blocker tasks, enqueuing the task under test so it parks on + // acquire(), cancelling it while parked, then freeing a slot. + if (!Platform.isIOS) { + markTestSkipped( + 'lib_audit_5: Android WorkManager gates a cancelled-while-queued ' + 'request at the OS level, before doWork() (and executionId ' + 'minting) ever runs — this in-process ConcurrencyLimiter gap is ' + 'iOS-only.', + ); + return; + } + final blockerIds = List.generate(4, (i) => _id('lib_audit_5_blocker_$i')); + final blockerFiles = [ + for (final blockerId in blockerIds) + File('${tmpDir.path}/${blockerId}_counter.txt'), + ]; + for (var i = 0; i < blockerIds.length; i++) { + await NativeWorkManager.enqueue( + taskId: blockerIds[i], + trigger: const TaskTrigger.oneTime(), + worker: DartWorker( + callbackId: 'dit_cancel_poll', + input: {'counterFile': blockerFiles[i].path}, + ), + ); + } + + // Give the 4 blockers time to each grab a limiter slot and start + // polling — proves all 4 slots are occupied before B is enqueued. + await Future.delayed(const Duration(milliseconds: 500)); + for (var i = 0; i < blockerFiles.length; i++) { + expect( + blockerFiles[i].existsSync(), + isTrue, + reason: + 'lib_audit_5: blocker $i must have started and be holding ' + 'a concurrency slot', + ); + } + + final bId = _id('lib_audit_5_b'); + final bCounterFile = File('${tmpDir.path}/lib_audit_5_b_counter.txt'); + await NativeWorkManager.enqueue( + taskId: bId, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker( + callbackId: 'dit_cancel_poll', + input: {'counterFile': bCounterFile.path}, + ), + ); + + // Give B's Task time to run past handleEnqueue's `guard + // !Task.isCancelled` and park inside ConcurrencyLimiter.acquire() — + // no I/O on that path, so this is a generous margin. + await Future.delayed(const Duration(milliseconds: 300)); + expect( + bCounterFile.existsSync(), + isFalse, + reason: + 'lib_audit_5: B must still be parked on the saturated ' + 'limiter, not yet running — if this file exists a slot was ' + 'free and the test does not exercise the intended window', + ); + + // Cancel B while it is parked — this is the race window. + await NativeWorkManager.cancel(taskId: bId); + + // Now free a slot: cancel the 4 blockers so their next poll (within + // 200ms) observes cancellation and releases the limiter, waking B. + for (final blockerId in blockerIds) { + await NativeWorkManager.cancel(taskId: blockerId); + } + + // Give B time to resume, run its method-channel round trip, write + // iteration 1, and poll — well short of the 10s it would take to + // run all 50 iterations if the cancellation was never observed. + await Future.delayed(const Duration(seconds: 3)); + + expect( + bCounterFile.existsSync(), + isTrue, + reason: 'lib_audit_5: B must have started once a slot freed up', + ); + final iterations = int.parse(bCounterFile.readAsStringSync().trim()); + expect( + iterations, + lessThan(5), + reason: + 'lib_audit_5: B was cancelled before it ever got a ' + 'concurrency slot — it must stop at its very first ' + 'isTaskCancelled() poll. A count this high means the cancel ' + 'landed in the pre-fix gap and was silently dropped, letting ' + 'B run on uncancelled. Iterations observed: $iterations', + ); + }); + testWidgets( 'issue_69: cancelling a background-session download actually aborts the transfer (iOS)', (tester) async { diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift index f005acf..c4fc451 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift @@ -186,12 +186,18 @@ extension NativeWorkmanagerPlugin { // this fresh one. replaceActiveTask cancels whatever's still // registered for taskId first, exactly like existingPolicy: .replace // does in handleEnqueue, then registers this execution the same way. - replaceActiveTask(taskId: taskId) { [weak self] in + // 2026-09-24: pre-mint dartExecutionId the same way handleEnqueue does — + // see replaceActiveTask's doc comment for why this must happen atomically + // with cancelling the outgoing execution, not lazily inside + // executeDartWorkerViaMethodChannel. + let dartExecutionId = record.workerClassName == "DartCallbackWorker" ? UUID().uuidString : nil + replaceActiveTask(taskId: taskId, dartExecutionId: dartExecutionId) { [weak self] preMintedExecutionId in await self?.executeWorkerSync( taskId: taskId, workerClassName: record.workerClassName, workerConfig: workerConfig, - qos: "background" + qos: "background", + preMintedExecutionId: preMintedExecutionId ) } } else { diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Execution.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Execution.swift index 3917d75..00903e6 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Execution.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Execution.swift @@ -423,7 +423,8 @@ extension NativeWorkmanagerPlugin { workerClassName: String, workerConfig: [String: Any], qos: String = "background", - retryConfig: RetryConfig = .noRetry + retryConfig: RetryConfig = .noRetry, + preMintedExecutionId: String? = nil ) async -> WorkerResult { let workerStartTime = Date() emitTaskStarted(taskId: taskId, workerType: workerClassName) @@ -437,12 +438,25 @@ extension NativeWorkmanagerPlugin { return .failure(message: "Cancelled before attempt \(attempt)/\(totalAttempts)") } await concurrencyLimiter.acquire() + // preMintedExecutionId (see replaceActiveTask) names ONE specific + // execution — the one that atomically replaced whatever was + // previously registered for taskId at enqueue/resume time. It + // only applies to attempt 1: a retry is a fresh execution that + // nothing external raced against, so it mints its own id lazily + // like any other caller of _executeWorker. Reusing the same + // preminted id across retries would be actively wrong — attempt + // 1's `defer { endExecution(...) }` already clears + // currentExecutionId[taskId] when it finishes, so attempt 2 + // would run with an id that's no longer registered as "current" + // and a cancel() landing during attempt 2 would resolve against + // the wrong bucket. lastResult = await _executeWorker( taskId: taskId, workerClassName: workerClassName, workerConfig: workerConfig, qos: qos, - shouldEmitEvent: false + shouldEmitEvent: false, + preMintedExecutionId: attempt == 1 ? preMintedExecutionId : nil ) await concurrencyLimiter.release() @@ -491,7 +505,8 @@ extension NativeWorkmanagerPlugin { workerClassName: String, workerConfig: [String: Any], qos: String = "background", - shouldEmitEvent: Bool = false + shouldEmitEvent: Bool = false, + preMintedExecutionId: String? = nil ) async -> WorkerResult { NativeLogger.d("Executing task '\(taskId)' in chain with QoS: \(qos)...") @@ -499,7 +514,11 @@ extension NativeWorkmanagerPlugin { // Using an unstructured Task {} inside withCheckedContinuation caused scheduling issues // on iOS 15 when two DartCallbackWorker tasks ran in parallel inside withTaskGroup. if workerClassName == "DartCallbackWorker" { - return await executeDartWorkerViaMethodChannel(workerConfig: workerConfig, taskId: taskId) + return await executeDartWorkerViaMethodChannel( + workerConfig: workerConfig, + taskId: taskId, + preMintedExecutionId: preMintedExecutionId + ) } let qosClass = mapQoS(qos) @@ -576,7 +595,8 @@ extension NativeWorkmanagerPlugin { func executeDartWorkerViaMethodChannel( workerConfig: [String: Any], - taskId: String + taskId: String, + preMintedExecutionId: String? = nil ) async -> WorkerResult { // Issue #72: mint a fresh executionId for THIS invocation, distinct // from taskId, mirroring Android's DartCallbackWorker (which mints @@ -585,10 +605,31 @@ extension NativeWorkmanagerPlugin { // taskId — a bare taskId-keyed cancellation mark cannot tell the two // apart, so this is what lets isTaskCancelled() resolve against the // specific execution polling it rather than whichever one happens to - // share its taskId. Registered immediately so a replace racing in - // concurrently always has something to resolve to. - let executionId = UUID().uuidString - DartTaskCancellationRegistry.shared.beginExecution(executionId, taskId: taskId) + // share its taskId. + // + // 2026-09-24 improvement pass: minting AND registering it here was + // NOT the narrow, sub-microsecond race it was originally scoped as — + // a cancel() landing after handleEnqueue accepts a task but before + // this line finally runs (e.g. while the task is parked inside + // ConcurrencyLimiter.acquire(), waiting for one of the default 4 + // concurrent slots) fell back to marking the bare taskId, an + // orphaned entry under the wrong key once this line's fresh + // executionId registers — reproduces with only 5 DartWorkers ever in + // flight, confirmed red-then-green by `lib_audit_5` in + // device_integration_test.dart. replaceActiveTask now closes that gap + // for its two callers (handleEnqueue, handleResume) by minting the id + // and calling beginExecution SYNCHRONOUSLY, in the same atomic block + // that cancels the outgoing execution — preMintedExecutionId is that + // id, arriving already registered. Every other caller of this + // function (chains, TaskGraph, BGTaskScheduler's periodic path, the + // offline queue, and retry attempts 2+ of executeWorkerSync's own + // loop) still doesn't have one to give, so this mints+registers its + // own exactly as before — those paths still have the same gap, + // tracked as a follow-up on PR #83. + let executionId = preMintedExecutionId ?? UUID().uuidString + if preMintedExecutionId == nil { + DartTaskCancellationRegistry.shared.beginExecution(executionId, taskId: taskId) + } // Issue #66/#72: whichever branch below runs, always drop this // execution's cancellation-registry entry once it's done — otherwise // a cancelled execution (or, worse, a reused taskId on a later run) diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift index 7117d2c..b0588ce 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift @@ -432,11 +432,18 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { // always reports the enqueue as accepted regardless of whether it was a no-op. let existingPolicyStr = (args["existingPolicy"] as? String)?.lowercased() ?? "replace" var skippedForKeep = false + // Minted here (not lazily inside executeDartWorkerViaMethodChannel) only for + // DartCallbackWorker — see replaceActiveTask's doc comment. Any other worker + // class has no cancellation-registry entry to pre-register, and doing it + // anyway would leak: only executeDartWorkerViaMethodChannel's `defer` calls + // endExecution. + let dartExecutionId = workerClassName == "DartCallbackWorker" ? UUID().uuidString : nil replaceActiveTask( taskId: taskId, skipIfAlreadyRunning: existingPolicyStr == "keep", + dartExecutionId: dartExecutionId, onSkipped: { skippedForKeep = true } - ) { [weak self] in + ) { [weak self] preMintedExecutionId in guard let self else { return } if initialDelayMs > 0 { try? await Task.sleep(nanoseconds: UInt64(initialDelayMs) * 1_000_000) @@ -447,7 +454,8 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { workerClassName: workerClassName, workerConfig: workerConfig, qos: directQos, - retryConfig: directRetryConfig + retryConfig: directRetryConfig, + preMintedExecutionId: preMintedExecutionId ) } if skippedForKeep { @@ -481,8 +489,9 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { func replaceActiveTask( taskId: String, skipIfAlreadyRunning: Bool = false, + dartExecutionId: String? = nil, onSkipped: (() -> Void)? = nil, - work: @escaping () async -> Void + work: @escaping (String?) async -> Void ) { stateQueue.sync(flags: .barrier) { if let existingTask = self.activeTasks[taskId] { @@ -500,6 +509,28 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { self.workers[taskId]?.stop() } + // 2026-09-24: if the caller already minted an executionId for the + // INCOMING execution (dartExecutionId — handleEnqueue/handleResume + // do this only when workerClassName == "DartCallbackWorker"), + // register it as taskId's current execution in this SAME + // barrier-protected block that just cancelled the outgoing one. + // Closes a real, easily-reproduced gap (not the narrow race it + // was originally scoped as): without this, there was a window + // between "decided to run" and "executeDartWorkerViaMethodChannel + // actually mints+registers its own id" — most commonly the task + // sitting parked in ConcurrencyLimiter.acquire() once the default + // 4 concurrent slots are full — where an explicit cancel(taskId) + // landing in that gap would resolve against whichever execution + // DartTaskCancellationRegistry knew about yet (the just-cancelled + // outgoing one on a replace, or nothing at all on a first-time + // enqueue — see markCancelled's taskId fallback), not this + // incoming one. See lib_audit_5 in device_integration_test.dart + // for the red-then-green repro (5 DartWorkers in flight, no + // artificial delay needed). + if let dartExecutionId { + DartTaskCancellationRegistry.shared.beginExecution(dartExecutionId, taskId: taskId) + } + // See activeTaskGenerations' doc comment: this id is what lets the // Task below tell, once IT finishes, whether it is still the // current occupant of activeTasks[taskId] — a naturally-completing @@ -507,6 +538,32 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { // long-finished taskId for one still running. let generationId = UUID() let task = Task { [weak self] in + // Unconditional and outside the `guard let self` / generation + // checks below: whichever exit path `work` takes must still + // drop this execution's registry entry, or it leaks (and + // `currentExecutionId[taskId]` is left pointing at a dead id + // forever). This matters because `work` CAN bail out WITHOUT + // ever reaching executeDartWorkerViaMethodChannel's own + // `defer { endExecution(...) }` — but NOT via + // ConcurrencyLimiter.acquire(): a task parked there is NOT + // cancellation-aware and DOES eventually proceed once a slot + // frees (confirmed by lib_audit_5 — that's exactly why a + // pre-minted id is needed there in the first place; see + // executeDartWorkerViaMethodChannel's own comment). The real + // bail-out paths this defer exists for are: handleEnqueue's + // closure's `guard !Task.isCancelled` (checked once, after + // the initialDelay `Task.sleep`, before `work` is ever + // called), executeWorkerSync's per-attempt top-of-loop + // `guard !Task.isCancelled`, and `self` being nil below. + // endExecution is idempotent (guarded on + // currentExecutionId[taskId] still pointing at this id), so + // whichever of this defer or executeDartWorkerViaMethodChannel's + // own defer runs first makes the other a no-op. + defer { + if let dartExecutionId { + DartTaskCancellationRegistry.shared.endExecution(dartExecutionId, taskId: taskId) + } + } guard let self else { return } defer { self.stateQueue.sync(flags: .barrier) { @@ -519,7 +576,7 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { self.activeTaskGenerations.removeValue(forKey: taskId) } } - await work() + await work(dartExecutionId) } self.activeTasks[taskId] = task self.activeTaskGenerations[taskId] = generationId diff --git a/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift b/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift index 053a979..6130401 100644 --- a/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift +++ b/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift @@ -64,10 +64,17 @@ final class DartTaskCancellationRegistry { // MARK: - Per-execution lifecycle (issue #72) /// Record that `executionId` is now the live execution for `taskId`. - /// Called once, right after `executeDartWorkerViaMethodChannel` mints an - /// executionId for a fresh invocation — before the Dart callback starts, - /// so a `markCancelled(taskId)` racing in concurrently always has - /// something to resolve to. + /// + /// Called from one of two places: (1) `executeDartWorkerViaMethodChannel`, + /// right after minting an executionId for a fresh invocation it wasn't + /// handed one for (chains, TaskGraph, BGTaskScheduler's periodic path, + /// the offline queue), or (2) `replaceActiveTask` (2026-09-24), which + /// mints and registers it synchronously — in the SAME barrier block that + /// decides to replace whatever's currently running — for its two callers + /// (`handleEnqueue`'s direct path, `handleResume`), so a + /// `markCancelled(taskId)` racing in immediately after has something + /// current to resolve to instead of a stale outgoing execution's id. + /// Either way, this must run before the Dart callback starts polling. func beginExecution(_ executionId: String, taskId: String) { lock.lock() defer { lock.unlock() }