fix(ios): cancellation-registry improvements — from small to large - #83
Merged
Merged
Conversation
…llation registry 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.
vietnguyentuan2019
changed the title
… run a task twice concurrently 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 "<executionId> <iteration>" 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.
…ks entry on natural completion 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.
…row — reproduces with 5 DartWorkers in flight 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<String> 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.
vietnguyentuan2019
marked this pull request as ready for review
September 24, 2026 13:45
vietnguyentuan2019
added a commit
that referenced
this pull request
Sep 24, 2026
…efore tagging User's explicit call: land all 4 iOS cancellation-registry follow-up fixes in 1.8.3 rather than a separate release, since 1.8.3 had not been tagged or published yet. This just reshapes the CHANGELOG to match — merges the standalone [Unreleased] section into [1.8.3]'s own entry, and corrects the original executionId-gap bullet's "narrow... not a pattern normal usage hits" claim in place rather than leaving it to contradict the corrected bullet below it. No code changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft PR to track the improvement ideas raised after the lib/ audit (#81/#82) — see that PR's discussion for full context. Working from small/low-risk to large/architectural, one commit at a time, each with its own verification.
Root cause these all trace back to
Answering "is taskId X currently running, and which execution" requires checking 7 separate dictionaries, written from 4 uncoordinated call sites, each with its own clear-up discipline:
NativeWorkmanagerPlugin:activeTasks,activeTaskGenerations,taskStates,workersDartTaskCancellationRegistry:cancelled,currentExecutionId,stopNotifiersThis fragmentation is what produced 3 confirmed real bugs (and 2 more found-but-deferred) during the lib/ audit in one session. The items below range from closing individual gaps in the current structure (low risk, mergeable independently) to actually fixing the fragmentation (a real architectural change, its own review + device time).
Done so far
stopAllWorkers()(BGTask expiration) never calledDartTaskCancellationRegistry.markCancelled, unlike every other cancel path. Fixed. Verified: full test suite green, Cancellation device-test group (8/8) re-run on both simulator and a physical iPhone. The actual expiration path itself couldn't be end-to-end verified — no harness in this repo has ever exercised real BGTask expiration timing.handleResumenever registered its Task inactiveTasks— fixed. Turned out bigger than framed: a paused-then-resumed non-background-session task could run TWICE concurrently, sincehandlePause's only real stop mechanism is a no-op for anything that isn't a background-session download. Extracted a sharedreplaceActiveTaskhelper (used by both this andhandleEnqueue) instead of hand-copying the cancel+register+generation-cleanup sequence a third time. New device testlib_audit_4, red-then-green verified viagit stash, regression-checked on simulator + a physical Pixel 6 Pro.BGTaskScheduler's periodic path (onTaskRunning) never cleared itsactiveTasksentry on natural completion — fixed, via an observerTaskawaitingrunningTask.value(sinceBGTaskSchedulerManagerowns the Task itself, unlike the two fixes above). Could not verify against a REAL background launch: re-ran the repo's existing periodic-firing test in isolation and confirmed it hits its own 2-minute fallback — BGTaskScheduler genuinely won't grant a launch inside an ordinary test session, on simulator OR real hardware. Firebase Test Lab would hit the identical OS-level constraint, so it was skipped rather than spending budget on it for no verification gain. Full regression suite green on iOS simulator + a physical Pixel 6 Pro.Planned (from the brainstorm, not started)
.replacedecides to swapactiveTasks[taskId], instead of lazily insideexecuteDartWorkerViaMethodChannel— closes the narrow residual race documented in the v1.8.3 CHANGELOG.TaskExecutionRegistry(single lock, one record per taskId, one lifecycle, used by all 4 write sites).NSLock/DispatchQueue.sync(flags: .barrier)synchronization to Swift actors.activeTasks[taskId] != nilliveness signal with an explicit typed state (pending / running(executionId) / completed / cancelled).Not requesting review yet — still a draft while more items land.