Repository navigation
feat(dart-worker): onStopped hook on cancel — onStoppedId + cancelGrace (#75) - #78
Merged
Merged
Conversation
…ancelGrace (#75) Follow-up to #67 / discussion #66. `isTaskCancelled()` worked but was poll-only: every callback had to remember to check it. @devroble's point that cancellation shouldn't be re-implemented by each app is fair, so this adds the push hook. What it is, precisely: a notification plus a bounded cleanup window, then an optional teardown. It does NOT preempt a running `await` — Dart has no API for that, so the same caveat the prior art carries applies here too. The shape matches flutter_workmanager's `onTaskStopped` (their 0.10.1), which converged on the same answer as the grace-period idea already floated in the discussion. API: initialize(onStoppedHandlers: {...}) // own registry; handler returns void DartWorker(onStoppedId:, cancelGrace:) `cancelGrace` doubles as the handler's budget and the teardown opt-in. Default null = notify-only, byte-for-byte the v1.8.x behaviour plus a notification, so no existing task changes behaviour on upgrade. `Duration.zero` tears down as soon as the handler returns. Assumption worth flagging: the discussion asked devroble whether they want (a) notify-for-cleanup or (b) unconditional kill, and that answer is still outstanding. The mechanism is the same either way, so this ships answer-agnostic — (a) is the default and (b) is `cancelGrace: Duration.zero`. Flipping the default later is one line. Wired on BOTH Dart inbound paths, deliberately: the headless `_callbackDispatcher` resolves the handler by *handle* (that isolate never ran initialize(), so it has no registry to resolve an id against), and the main-isolate `method_channel.dart` resolves by *id*. #40 shipped the progress fix for only one of these and left iOS broken until #41. iOS hangs the notifier off DartTaskCancellationRegistry rather than editing each `markCancelled` call site, so explicit cancel, cancelByTag, cancelAll, notification-driven cancel and BGTask expiration all fire it for free. The notifier is removed as it is taken, so a cancel racing an expiration cannot notify twice. Known limitation, documented in the API docs, CHANGELOG and at the dispose site: teardown is not per-task. The engine is shared across concurrent DartWorkers and every dispose is gated on activeTaskCount because tearing it down while another task holds the methodChannel is a JNI crash on freed memory. Kill-everything or kill-nothing; when a sibling is in flight the cancelled task is left running and we say so loudly instead of aborting unrelated work. `NonCancellable` around the Android notify is load-bearing, not defensive: that coroutine is already cancelled, so a plain suspending call would abort at its first suspension point and the handler would never run. Two incidental fixes the guards caught: - setStoppedExecutor defaults to a no-op, not UnimplementedError — initialize() calls it unconditionally and throwing broke every pre-#75 platform fake, for a capability that is opt-in per task. - notifyDartTaskStopped exempted in cancellation_rethrow_invariant_test with the reason (runs only inside NonCancellable; a rethrow would abort the very notification it exists to deliver). Tests: 16 unit tests, of which the inbound-channel group is the actual bridge guard — proven red by breaking cancelGraceMs forwarding and __taskId forwarding separately, then green again. Serialization round-trips alone would not catch a dropped bridge forward (issue #30 rule). Plus two issue_75 device tests: one using a callback that deliberately never polls (so only a real push can make it pass), one asserting the teardown actually freezes an uncooperative callback. flutter analyze clean; 1246 unit tests green; Android Kotlin and example iOS build clean. Claude-Session: https://claude.ai/code/session_01CzHcizphPhUUeDiAr5VnnT
…ice (#75) Found by actually running the issue_75 device tests on a Pixel 6 Pro, which the first commit had not done. Both failed. **The real bug.** `dispose()` is a `suspend fun` whose body is `initializationMutex.withLock { withContext(Dispatchers.Main) { engine?.destroy() } }`. The teardown call sits in the `wasCancelled` branch, where the coroutine has already been cancelled — so that inner `withContext` threw JobCancellationException before `engine.destroy()` ever ran. dispose()'s own `catch` swallowed it, logged "Error destroying engine (expected if already detached)", and then nulled the engine field regardless. Net effect: the native engine leaked AND the Dart isolate kept running, while the `catch (_: Exception)` at the call site hid everything. Logcat showed only that one misleading line; the test caught it because the counter kept climbing from 10 to 25 after cancel. I had already wrapped the *notify* in NonCancellable for exactly this reason and still missed the dispose call two lines below it — it reads as a plain synchronous call at the call site. Comment at the site now spells out why, because this is precisely the shape a future cleanup commit would undo. **Second failure, a test bug not a product bug.** Both tests asserted the callback had started after a fixed 600ms wait. A cold Flutter engine boot is 500-1000ms by itself, so that precondition passed only when an earlier test had already warmed the engine and failed when the test ran in isolation. Replaced with a bounded `_waitForFile` poll. Verified after the fix: - Pixel 6 Pro, Android 17: both issue_75 tests green; the whole Cancellation group green (issue_66, issue_72, issue_75) — no regression. - iOS 26 simulator: the onStopped test green, with `dit_on_stopped: fired for taskId=dit_issue_75_on_stopped_...` in the log confirming the hook fires on iOS with the right taskId; the teardown test correctly skips (iOS foreground runs on the host app's main engine, which must never be disposed). - 1246 unit tests green, flutter analyze clean. Claude-Session: https://claude.ai/code/session_01CzHcizphPhUUeDiAr5VnnT
The notification half is genuinely cross-platform and device-verified on both. The teardown half is not, and the first pass claimed parity it doesn't have. On iOS nothing is disposed on cancel: - foreground/simulator runs the callback on the host app's own Flutter engine, which must never be disposed — that would kill the app; - the killed-app headless engine COULD be torn down (its dispose() is synchronous, so it has none of the suspend-cancellation hazard Android's has), but iOS has no in-flight task counter to gate it the way Android's activeTaskCount does. Disposing ungated risks tearing the engine out from under a sibling callback, so it is left as follow-up rather than shipped unsafe. Corrected in the three places a user or a future maintainer would look: the cancelGrace dartdoc, the CHANGELOG entry, and the device test's skip reason — which previously implied the test merely couldn't run on iOS, rather than that the behaviour isn't there. Also verified while checking this: - no path in executeDartWorkerViaMethodChannel leaves a running callback with a cleared stop notifier — both early failure returns (missing callbackId, missing callbackHandle) happen before either registerStopNotifier call, and the defer only fires at function exit. - Task Chains, Tags and Concurrent tasks groups green on a Pixel 6 Pro after the FlutterEngineManager and iOS registry edits. One Task Chains failure on the first run did not reproduce across two further full-group runs and passes in isolation — the suite's known cross-test flakiness, and unreachable from this change on Android anyway (the Kotlin registry is untouched, and the wasCancelled branch only diverges when onStoppedHandle/cancelGraceMs are set, which no chain test sets). Claude-Session: https://claude.ai/code/session_01CzHcizphPhUUeDiAr5VnnT
README: the cancellation section only documented polling. Adds the push hook alongside it, with both caveats stated rather than buried — it is a notification and not preemption, and cancelGrace's teardown is Android-only. Example app: new "Stop Handler (#75)" page. The two worker callbacks behind it are deliberately identical bar one line — one polls isTaskCancelled(), the other never does — because that is the distinction the feature turns on: the handler fires for both, but only the cooperative one actually stops on iOS. A third button runs the stubborn callback with cancelGrace: Duration.zero so the Android teardown path is reachable from the UI too. Verified beyond "it compiles": launched the example app on an iOS 18 simulator and confirmed a clean start with onStoppedHandlers registered. That is the real risk in this change — PluginUtilities.getCallbackHandle returns null for anything that is not a top-level or static function, and initialize() throws StateError on that, which would break app startup rather than merely the demo. No StateError, no handle-resolution failure in the log. flutter analyze clean, 1246 unit tests green, pragma placement guard green (three new @pragma('vm:entry-point') callbacks in the demo page). Note: the Pixel 6 Pro dropped off wireless ADB partway through, so this commit's device check is the iOS simulator only. The Android paths were already device-verified in 0ab4018.
…#75) Closes the parity gap the previous commit documented. iOS notified but disposed nothing; now it tears the headless engine down like Android does. The blocker was said to be "iOS has no in-flight task counter". Reading the code first changed the shape of the fix: callbackQueue is an AsyncQueue, a strict serialiser, so the headless engine runs exactly one Dart callback at a time. But a counter is still needed, for a subtler reason than concurrency — teardown is not instantaneous. The cancelled task's Swift Task is cancelled immediately, which frees the AsyncQueue slot, while the engine is only disposed once the stop handler replies or its budget elapses. A queued task can start inside that window, and disposing then would tear the engine out from under it. So activeCallbackCount gates the dispose exactly as Android's activeTaskCount does. Checked before writing any of it that disposing cannot wedge the queue: the cancelled execution settles through _ContinuationBox + withTaskCancellationHandler (so CancellationError unwinds and releases the slot) rather than waiting on a channel reply that will never arrive. Without that the dispose would have stalled every subsequent headless task behind the 5-minute callback timeout. notifyDartTaskStopped is no longer fire-and-forget: it takes the invokeMethod reply and races it against the budget with a one-shot flag, so teardown happens on handler-return or budget-expiry, whichever comes first — the same rule Android applies. cancelGrace == nil stays notify-only and disposes nothing. disposeAfterCancelIfIdle does its check-and-dispose inside a single queue.sync so nothing can start between the two, and calls _disposeInternal() rather than _dispose() — the latter re-enters queue.sync and would deadlock. **Not device-verified, and the docs say so.** The headless path only runs on a killed-app BGTask launch on physical hardware; a simulator cannot reproduce it, so this ships compiled and reviewed but unexercised. The foreground/simulator path is deliberately excluded from teardown at the source — it runs on the host app's own engine and disposing that would kill the app — so the iOS device test still skips, now with that as the stated reason rather than "not implemented". Docs corrected in all four places the previous commit had marked Android-only: cancelGrace dartdoc, CHANGELOG, README, device test skip reason. iOS simulator: issue_75 notification test still green, no regression. Android untouched this round (no Kotlin diff; lib/ diff is comment-only), so its earlier device verification stands. flutter analyze clean, 1246 unit tests green.
…s migrator (plugin stays 14.0)
… iOS now has it on the headless path (#75)
This was referenced Sep 23, 2026
Merged
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.
Closes #75 (follow-up to discussion #66 / #67).
Cancellation used to be poll-only through
isTaskCancelled(). This PR adds a push notification:DartWorker(onStoppedId:, cancelGrace:)together withinitialize(onStoppedHandlers: {...}).cancel(),cancelAll()andcancelByTag(), and when the OS reclaims the work (WorkManager stopping the worker on Android, BGTask expiration on iOS)._callbackDispatcher(looks the handler up by handle) and the main-isolate method channel (looks it up by id). Issue fix: restore DartWorker progress events and TaskStore status (#38, #39) #40 fixed only one of these two paths and left iOS broken.cancelGraceis the time budget for the handler.Duration.zero: tear the engine down as soon as the handler returns.Where teardown applies, and what was verified
dispose()is a suspend fun, so calling it on the already-cancelled coroutine silently did nothing; it now runs underNonCancellable.BGTaskSchedulerpath: teardown implemented. It compiles, but it is not device-verified, because a simulator can't reach that path. The CHANGELOG says so.Tests
test/unit/issue_75_stop_handler_test.dartissue_75_*inexample/integration_test/device_integration_test.dartcancellation_rethrow_invariant_testexemption fornotifyDartTaskStoppedOut of scope
stopReasonparity. It needsWorkerEnvironment.stopReasonin kmpworkmanager core.Rebased onto main after #77. Formatted with Flutter 3.47.5, and the example's iOS target raised to 15.0 by Flutter's own migrator. Checked locally on 3.47.5:
flutter test test/unit/: 1249 passed