From 8044481adc3a3774d3d550289502f46621ce2ecb 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: Wed, 23 Sep 2026 19:13:08 +0700 Subject: [PATCH 1/8] fix(security): multiUpload/moveToSharedStorage/webSocket/ParallelHttpUploadWorker skipped validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the 2026-09-23 lib/ audit. Every other HTTP/file worker validates its url/path arguments through NativeWorker._validateUrl/_validateFilePath — these four didn't, so they bypassed HTTPS enforcement, SSRF/private-IP blocking, and path-traversal checks entirely: - multiUpload(): no validation at all on url or any file's filePath. - moveToSharedStorage(): no validation on sourcePath, and no traversal check on subDir (which feeds Android's MediaStore.RELATIVE_PATH). - webSocket(): only checked the url had a ws://ws:// prefix — enforceHttps(true) had no effect on ws:// vs wss://, blockPrivateIPs never applied, and storeResponseAt wasn't checked as a path at all. - ParallelHttpUploadWorker: has no NativeWorker.* factory, so its own constructor is the only place validation could live, and it had none. _validateUrl is now _validateUrlWithSchemes(insecure/secure scheme, example), shared by http/https and ws/wss so the same checks can't drift apart a third time. Added _validateRelativeSegment for subDir-shaped fields, which are relative by design and shouldn't go through the absolute-path check. NativeWorker.validateUrlForWorkerConstructor/validateFilePathForWorkerConstructor are new @internal wrappers so ParallelHttpUploadWorker (a separate library, can't reach the private validators) can call the same checks. Guarded by test/security/issue_lib_audit_1_missing_validators_test.dart, red-then-green verified against the pre-fix code. --- lib/src/worker.dart | 71 +++++++- lib/src/workers/native_worker_http.dart | 15 ++ lib/src/workers/native_worker_websocket.dart | 14 +- .../workers/parallel_http_upload_worker.dart | 9 ++ ...e_lib_audit_1_missing_validators_test.dart | 152 ++++++++++++++++++ 5 files changed, 246 insertions(+), 15 deletions(-) create mode 100644 test/security/issue_lib_audit_1_missing_validators_test.dart diff --git a/lib/src/worker.dart b/lib/src/worker.dart index b72f229..2ea7e02 100644 --- a/lib/src/worker.dart +++ b/lib/src/worker.dart @@ -59,27 +59,55 @@ class NativeWorker { NativeWorker._(); /// Validate URL format and throw helpful error if invalid. - static void _validateUrl(String url) { + static void _validateUrl(String url) => _validateUrlWithSchemes( + url, + insecureScheme: 'http', + secureScheme: 'https', + example: 'https://api.example.com/endpoint', + ); + + /// Validate a WebSocket URL. Same checks as [_validateUrl] (null-byte / + /// injection / HTTPS-equivalent enforcement / private-IP blocking), just + /// against `ws`/`wss` instead of `http`/`https`. + /// + /// Added by the 2026-09-23 lib/ audit: `NativeWorker.webSocket()` used to + /// accept any string with a `ws://`/`wss://` prefix and nothing else — + /// `enforceHttps(true)` had no effect on it, and it never blocked private + /// IPs, unlike every HTTP-based worker. + static void _validateWebSocketUrl(String url) => _validateUrlWithSchemes( + url, + insecureScheme: 'ws', + secureScheme: 'wss', + example: 'wss://api.example.com/socket', + ); + + static void _validateUrlWithSchemes( + String url, { + required String insecureScheme, + required String secureScheme, + required String example, + }) { _validateInput(url, 'URL', isUrl: true); if (url.isEmpty) { throw ArgumentError( 'URL cannot be empty.\n' - 'Provide a valid HTTP/HTTPS URL like "https://api.example.com/endpoint"', + 'Provide a valid $insecureScheme:// or $secureScheme:// URL like "$example"', ); } final uri = Uri.tryParse(url); if (uri == null || - (!uri.hasScheme || (uri.scheme != 'http' && uri.scheme != 'https'))) { + (!uri.hasScheme || + (uri.scheme != insecureScheme && uri.scheme != secureScheme))) { throw ArgumentError( 'Invalid URL format: "$url"\n' - 'URL must start with http:// or https://\n' - 'Example: "https://api.example.com/endpoint"', + 'URL must start with $insecureScheme:// or $secureScheme://\n' + 'Example: "$example"', ); } - // SECURITY: Enforce HTTPS if configured - if (NativeWorkManager.enforceHttps && uri.scheme == 'http') { + // SECURITY: Enforce the secure scheme if configured + if (NativeWorkManager.enforceHttps && uri.scheme == insecureScheme) { throw ArgumentError( 'Insecure URL blocked: "$url"\n' 'HTTPS is enforced by NativeWorkManager.initialize(enforceHttps: true)', @@ -165,6 +193,35 @@ class NativeWorker { } } + /// Validate a relative sub-path segment (e.g. [moveToSharedStorage]'s + /// `subDir`) for path traversal and injection, without requiring it to be + /// absolute — unlike [_validatePath], a relative segment is exactly what + /// these fields are meant to carry (Android's `MediaStore.RELATIVE_PATH` + /// / iOS's app-Documents subfolder). + static void _validateRelativeSegment(String value, String label) { + _validateInput(value, label, isUrl: false); + final normalized = value.toLowerCase(); + if (normalized.contains('..') || normalized.contains('%2e%2e')) { + throw ArgumentError( + '$label cannot contain ".." or encoded dot-segments (path traversal attempt blocked).'); + } + } + + /// Validates [url] the same way every built-in HTTP worker's + /// `NativeWorker.*` factory does. Exposed (not private) so a package + /// `Worker` subclass with no `NativeWorker.*` factory of its own — e.g. + /// [ParallelHttpUploadWorker], whose constructor is the only entry point — + /// can still call the same check from its own file. Not part of the public + /// API surface. + @internal + static void validateUrlForWorkerConstructor(String url) => _validateUrl(url); + + /// Validates [path] the same way every built-in file-based worker's + /// `NativeWorker.*` factory does. See [validateUrlForWorkerConstructor]. + @internal + static void validateFilePathForWorkerConstructor(String path, String label) => + _validateFilePath(path, label); + /// Validate file path and throw helpful error if invalid. static void _validateFilePath(String path, String parameterName) { if (path.isEmpty) { diff --git a/lib/src/workers/native_worker_http.dart b/lib/src/workers/native_worker_http.dart index 6a26007..c1f7929 100644 --- a/lib/src/workers/native_worker_http.dart +++ b/lib/src/workers/native_worker_http.dart @@ -459,12 +459,20 @@ MultiUploadWorker _buildMultiUpload({ Duration timeout = const Duration(minutes: 10), bool useBackgroundSession = false, }) { + // These two validators were missing entirely until the 2026-09-23 lib/ + // audit — every other HTTP worker in this file calls them, but this one + // didn't, so multiUpload() bypassed HTTPS enforcement, SSRF/private-IP + // blocking, and path-traversal checks. + NativeWorker._validateUrl(url); if (files.isEmpty) { throw ArgumentError('files must not be empty'); } if (files.length > 50) { throw ArgumentError('Maximum 50 files per upload request'); } + for (final file in files) { + NativeWorker._validateFilePath(file.filePath, 'files[].filePath'); + } return MultiUploadWorker( url: url, files: files, @@ -496,6 +504,13 @@ MoveToSharedStorageWorker _buildMoveToSharedStorage({ String? mimeType, String? subDir, }) { + // Missing entirely until the 2026-09-23 lib/ audit: sourcePath reached + // native with no path-traversal check, and subDir (which feeds + // MediaStore.RELATIVE_PATH on Android) wasn't checked at all. + NativeWorker._validateFilePath(sourcePath, 'sourcePath'); + if (subDir != null) { + NativeWorker._validateRelativeSegment(subDir, 'subDir'); + } return MoveToSharedStorageWorker( sourcePath: sourcePath, storageType: storageType, diff --git a/lib/src/workers/native_worker_websocket.dart b/lib/src/workers/native_worker_websocket.dart index ce391ea..90cc698 100644 --- a/lib/src/workers/native_worker_websocket.dart +++ b/lib/src/workers/native_worker_websocket.dart @@ -15,14 +15,12 @@ Worker _buildWebSocket({ 'Use a DartWorker with dart:io WebSocket for cross-platform WebSocket support.', ); } - if (url.isEmpty) { - throw ArgumentError('url cannot be empty for webSocket'); - } - final uri = Uri.tryParse(url); - if (uri == null || (uri.scheme != 'ws' && uri.scheme != 'wss')) { - throw ArgumentError( - 'Invalid WebSocket URL: "$url". Must start with ws:// or wss://', - ); + // Was a bare scheme-prefix check until the 2026-09-23 lib/ audit — url + // content (injection chars, null bytes) went unchecked, enforceHttps(true) + // had no effect on ws:// vs wss://, and blockPrivateIPs didn't apply here. + NativeWorker._validateWebSocketUrl(url); + if (storeResponseAt != null) { + NativeWorker._validateFilePath(storeResponseAt, 'storeResponseAt'); } if (timeoutSeconds <= 0) { throw ArgumentError('timeoutSeconds must be > 0, got $timeoutSeconds'); diff --git a/lib/src/workers/parallel_http_upload_worker.dart b/lib/src/workers/parallel_http_upload_worker.dart index a155af3..93859dd 100644 --- a/lib/src/workers/parallel_http_upload_worker.dart +++ b/lib/src/workers/parallel_http_upload_worker.dart @@ -68,9 +68,18 @@ final class ParallelHttpUploadWorker extends Worker { this.notificationBody, this.certificatePinning, }) { + // Missing entirely until the 2026-09-23 lib/ audit: unlike every other + // HTTP worker, this class has no NativeWorker.* factory to gate + // construction, so its own constructor is the only place these checks + // can live. + NativeWorker.validateUrlForWorkerConstructor(url); if (files.isEmpty) { throw ArgumentError.value(files, 'files', 'must not be empty'); } + for (final file in files) { + NativeWorker.validateFilePathForWorkerConstructor( + file.filePath, 'files[].filePath'); + } if (maxConcurrent < 1 || maxConcurrent > 16) { throw RangeError.range(maxConcurrent, 1, 16, 'maxConcurrent'); } diff --git a/test/security/issue_lib_audit_1_missing_validators_test.dart b/test/security/issue_lib_audit_1_missing_validators_test.dart new file mode 100644 index 0000000..7d38765 --- /dev/null +++ b/test/security/issue_lib_audit_1_missing_validators_test.dart @@ -0,0 +1,152 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:flutter/services.dart'; +import 'package:native_workmanager/native_workmanager.dart'; + +/// Guards a real gap found by the 2026-09-23 lib/ audit: `multiUpload`, +/// `moveToSharedStorage`, `webSocket`, and `ParallelHttpUploadWorker`'s +/// constructor (which has no `NativeWorker.*` factory) all reached native +/// with none of the URL/path validation every sibling HTTP/file worker +/// enforces — no HTTPS enforcement, no SSRF/private-IP blocking, no +/// path-traversal blocking. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + const MethodChannel channel = + MethodChannel('dev.brewkits/native_workmanager'); + + setUp(() { + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(channel, (MethodCall methodCall) async { + switch (methodCall.method) { + case 'initialize': + return null; + default: + return null; + } + }); + }); + + tearDown(() { + NativeWorkManager.resetSecurityFlags(); + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(channel, null); + }); + + group('multiUpload() validators', () { + test('rejects a private-IP URL when blockPrivateIPs is set', () async { + NativeWorkManager.resetInitializedState(); + try { + await NativeWorkManager.initialize(blockPrivateIPs: true); + } catch (_) {} + + expect( + () => NativeWorker.multiUpload( + url: 'https://10.0.0.5/upload', + files: const [UploadFile(filePath: '/tmp/a.jpg')], + ), + throwsArgumentError, + ); + }); + + test('rejects a file path with ".." traversal', () { + expect( + () => NativeWorker.multiUpload( + url: 'https://upload.example.com/batch', + files: const [UploadFile(filePath: '/tmp/../../etc/passwd')], + ), + throwsArgumentError, + ); + }); + + test('accepts a normal https URL and absolute file paths', () { + final w = NativeWorker.multiUpload( + url: 'https://upload.example.com/batch', + files: const [UploadFile(filePath: '/tmp/a.jpg')], + ); + expect(w.toMap()['url'], 'https://upload.example.com/batch'); + }); + }); + + group('moveToSharedStorage() validators', () { + test('rejects a sourcePath with ".." traversal', () { + expect( + () => NativeWorker.moveToSharedStorage( + sourcePath: '/tmp/../../etc/passwd', + storageType: SharedStorageType.downloads, + ), + throwsArgumentError, + ); + }); + + test('rejects a subDir with ".." traversal', () { + expect( + () => NativeWorker.moveToSharedStorage( + sourcePath: '/tmp/photo.jpg', + storageType: SharedStorageType.photos, + subDir: '../../OtherApp/Camera', + ), + throwsArgumentError, + ); + }); + + test('accepts a normal relative subDir', () { + final w = NativeWorker.moveToSharedStorage( + sourcePath: '/tmp/photo.jpg', + storageType: SharedStorageType.photos, + subDir: 'Holidays', + ); + expect(w.toMap()['subDir'], 'Holidays'); + }); + }); + + group('webSocket() validators', () { + test('rejects ws:// when enforceHttps is set', () async { + NativeWorkManager.resetInitializedState(); + try { + await NativeWorkManager.initialize(enforceHttps: true); + } catch (_) {} + + expect( + () => NativeWorker.webSocket(url: 'ws://insecure.example.com'), + throwsArgumentError, + ); + }); + + test('rejects a storeResponseAt path with ".." traversal', () { + expect( + () => NativeWorker.webSocket( + url: 'wss://api.example.com', + storeResponseAt: '../../etc/hosts', + ), + throwsArgumentError, + ); + }); + }); + + group('ParallelHttpUploadWorker constructor validators', () { + test('rejects a private-IP URL when blockPrivateIPs is set', () async { + NativeWorkManager.resetInitializedState(); + try { + await NativeWorkManager.initialize(blockPrivateIPs: true); + } catch (_) {} + + expect( + () => ParallelHttpUploadWorker( + url: 'https://192.168.1.1/upload', + files: const [UploadFile(filePath: '/tmp/a.jpg')], + ), + throwsArgumentError, + ); + }); + + test('rejects a file path with ".." traversal', () { + expect( + () => ParallelHttpUploadWorker( + url: 'https://upload.example.com', + files: const [UploadFile(filePath: '/tmp/../../etc/passwd')], + ), + throwsArgumentError, + ); + }); + }); +} From ca358c6fcc75d098cf96ec78b28eb74648272e5b 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: Wed, 23 Sep 2026 19:19:53 +0700 Subject: [PATCH 2/8] fix(dart): DartWorker in a TaskGraph node or RemoteTriggerRule mapping never reached native MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the 2026-09-23 lib/ audit. enqueue() and task chains convert a DartWorker to a DartWorkerInternal (resolving its native callback handle) before sending it to native. TaskNode.toMap() and RemoteTriggerRule.toMap() never did — they called the worker's own toMap() directly, which for a plain DartWorker never includes callbackHandle at all (only DartWorkerInternal.toMap() does). Native's DartCallbackWorker requires callbackHandle on both platforms and fails cleanly without it, so the task was enqueued but the callback could never resolve — reachable and confirmed on both Android's GraphHelper.kt (no special-casing, passes workerConfig straight to the standard worker factory) and iOS's DartCallbackWorker.swift (callbackHandle is non-optional Codable). Extracted the enqueue()/_enqueueChain() conversion logic (previously duplicated between the two) into NativeWorkManager.resolveWorkerForWire(), one choke point now used by enqueue, task chains, TaskGraph nodes, and registerRemoteTrigger. As a side effect this also fixes chain-step DartWorkers silently skipping the iOS heavy-task (isHeavyTask=true) promotion that plain enqueue() always applied — the two paths could drift because the conversion was hand-copied. TaskNode/TaskGraph.toMap() are intentionally left alone (existing unit tests call them directly with no NativeWorkManager.initialize()); the resolving variant (_toResolvedMap()) is only used by enqueueTaskGraph(), the one place that actually sends a graph to native. Guarded by test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart, red-then-green verified against the pre-fix code (4/6 cases failed before). --- lib/src/native_work_manager.dart | 188 ++++++++++-------- lib/src/task_graph.dart | 34 +++- ...dit_2_dartworker_wire_resolution_test.dart | 181 +++++++++++++++++ 3 files changed, 315 insertions(+), 88 deletions(-) create mode 100644 test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart diff --git a/lib/src/native_work_manager.dart b/lib/src/native_work_manager.dart index 1739b52..d8007bb 100644 --- a/lib/src/native_work_manager.dart +++ b/lib/src/native_work_manager.dart @@ -766,6 +766,79 @@ class NativeWorkManager { return handle; } + /// Resolves a [Worker] for the platform channel. + /// + /// If [worker] is a plain [DartWorker], this validates that its callback is + /// registered and returns the equivalent [DartWorkerInternal] carrying the + /// resolved native callback handle — plus, when [constraints] is given and + /// the platform is iOS, constraints promoted to a heavy task (a DartWorker + /// spins up a Flutter Engine, so it needs BGProcessingTask's larger budget + /// instead of BGAppRefreshTask's ~30s, the same promotion [enqueue] always + /// applied). Any other worker — including an already-resolved + /// [DartWorkerInternal] — passes through unchanged. + /// + /// This is the single choke point for every Dart→native path that can carry + /// a [DartWorker]: [enqueue], task chains ([_enqueueChain]), [TaskGraph] + /// nodes (via [enqueueTaskGraph]), and [registerRemoteTrigger]. Before this + /// existed, only [enqueue] and task chains did the conversion — a + /// DartWorker placed in a TaskGraph node or a RemoteTriggerRule mapping + /// reached native with no callbackHandle at all, so its callback could + /// never be resolved (found by the 2026-09-23 lib/ audit). Task chains also + /// silently skipped the iOS heavy-task promotion that plain enqueue() + /// applied, which this fixes as a side effect of sharing one implementation. + @internal + static (Worker, Constraints?) resolveWorkerForWire( + Worker worker, [ + Constraints? constraints, + ]) { + if (worker is! DartWorker) return (worker, constraints); + + if (!_dartWorkers.containsKey(worker.callbackId)) { + throw StateError( + 'Dart worker "${worker.callbackId}" not registered.\n' + 'Register it in NativeWorkManager.initialize():\n' + ' await NativeWorkManager.initialize(\n' + ' dartWorkers: {\n' + ' "${worker.callbackId}": (input) async { ... },\n' + ' },\n' + ' );', + ); + } + + var resolvedConstraints = constraints; + if (resolvedConstraints != null && + defaultTargetPlatform == TargetPlatform.iOS && + !resolvedConstraints.isHeavyTask) { + resolvedConstraints = resolvedConstraints.copyWith(isHeavyTask: true); + developer.log( + 'NativeWorkManager: DartWorker on iOS detected. Promoting to heavy task ' + '(isHeavyTask=true) to prevent OS termination.', + name: 'NativeWorkManager', + ); + } + + final callbackHandle = _callbackHandles[worker.callbackId]; + if (callbackHandle == null) { + throw StateError( + 'INTERNAL ERROR: Callback handle not found for "${worker.callbackId}". ' + 'This should never happen. Please report this bug.', + ); + } + + final resolvedWorker = DartWorkerInternal( + callbackId: worker.callbackId, + callbackHandle: callbackHandle, + input: worker.input, + autoDispose: worker.autoDispose, + timeoutMs: worker.timeoutMs, + onStoppedId: worker.onStoppedId, + onStoppedHandle: _resolveOnStoppedHandle(worker.onStoppedId), + cancelGraceMs: worker.cancelGrace?.inMilliseconds, + ); + + return (resolvedWorker, resolvedConstraints); + } + // ═══════════════════════════════════════════════════════════════════════════ // TASK SCHEDULING // ═══════════════════════════════════════════════════════════════════════════ @@ -983,56 +1056,9 @@ class NativeWorkManager { } // Validate DartWorker registration and prepare worker data - Worker workerToEnqueue = worker; - Constraints finalConstraints = constraints; - - if (worker is DartWorker) { - if (!_dartWorkers.containsKey(worker.callbackId)) { - throw StateError( - 'Dart worker "${worker.callbackId}" not registered.\n' - 'Register it in NativeWorkManager.initialize():\n' - ' await NativeWorkManager.initialize(\n' - ' dartWorkers: {\n' - ' "${worker.callbackId}": (input) async { ... },\n' - ' },\n' - ' );', - ); - } - - // iOS safety: DartWorkers are heavy by definition because they spin up - // a Flutter Engine. Force BGProcessingTask (60s+) instead of - // BGAppRefreshTask (30s) to prevent immediate OS kills. - if (defaultTargetPlatform == TargetPlatform.iOS && - !constraints.isHeavyTask) { - finalConstraints = constraints.copyWith(isHeavyTask: true); - developer.log( - 'NativeWorkManager: DartWorker on iOS detected. Promoting to heavy task ' - '(isHeavyTask=true) to prevent OS termination.', - name: 'NativeWorkManager', - ); - } - - // Get the callback handle for this worker - final callbackHandle = _callbackHandles[worker.callbackId]; - if (callbackHandle == null) { - throw StateError( - 'INTERNAL ERROR: Callback handle not found for "${worker.callbackId}". ' - 'This should never happen. Please report this bug.', - ); - } - - // Create enhanced DartWorker with callback handle - workerToEnqueue = DartWorkerInternal( - callbackId: worker.callbackId, - callbackHandle: callbackHandle, - input: worker.input, - autoDispose: worker.autoDispose, - timeoutMs: worker.timeoutMs, - onStoppedId: worker.onStoppedId, - onStoppedHandle: _resolveOnStoppedHandle(worker.onStoppedId), - cancelGraceMs: worker.cancelGrace?.inMilliseconds, - ); - } + final (workerToEnqueue, resolvedConstraints) = + resolveWorkerForWire(worker, constraints); + final finalConstraints = resolvedConstraints ?? constraints; final scheduleResult = await NativeWorkManagerPlatform.instance.enqueue( taskId: taskId, @@ -2090,44 +2116,19 @@ class NativeWorkManager { static Future _enqueueChain(TaskChainBuilder chain) { // Convert DartWorker to DartWorkerInternal for all tasks in the chain + // (and, on iOS, apply the same heavy-task promotion enqueue() does — + // this used to be skipped here, leaving a chain-step DartWorker on the + // short BGAppRefreshTask budget instead of BGProcessingTask). final convertedSteps = chain.steps.map((step) { return step.map((task) { - final worker = task.worker; - - // Check if worker is DartWorker and needs conversion - if (worker is DartWorker) { - // Get the callback handle for this worker - final callbackHandle = _callbackHandles[worker.callbackId]; - if (callbackHandle == null) { - throw StateError( - 'INTERNAL ERROR: Callback handle not found for "${worker.callbackId}". ' - 'This should never happen. Please report this bug.', - ); - } - - // Convert DartWorker to DartWorkerInternal - final convertedWorker = DartWorkerInternal( - callbackId: worker.callbackId, - callbackHandle: callbackHandle, - input: worker.input, - autoDispose: worker.autoDispose, - timeoutMs: worker.timeoutMs, - onStoppedId: worker.onStoppedId, - onStoppedHandle: _resolveOnStoppedHandle(worker.onStoppedId), - cancelGraceMs: worker.cancelGrace?.inMilliseconds, - ); - - // Return modified task map with converted worker - return { - 'id': task.id, - 'workerClassName': convertedWorker.workerClassName, - 'workerConfig': convertedWorker.toMap(), - 'constraints': task.constraints.toMap(), - }; - } - - // For non-DartWorker tasks, use original toMap() - return task.toMap(); + final (resolvedWorker, resolvedConstraints) = + resolveWorkerForWire(task.worker, task.constraints); + return { + 'id': task.id, + 'workerClassName': resolvedWorker.workerClassName, + 'workerConfig': resolvedWorker.toMap(), + 'constraints': (resolvedConstraints ?? task.constraints).toMap(), + }; }).toList(); }).toList(); @@ -2554,9 +2555,22 @@ class NativeWorkManager { required RemoteTriggerRule rule, }) async { _checkInitialized(); + // Resolve any DartWorker in workerMappings before it reaches native — + // RemoteTriggerRule.toMap() alone never did this, so a DartWorker mapped + // here reached native with no callbackHandle and could never be resolved + // (found by the 2026-09-23 lib/ audit). No Constraints exist on a + // mapping, so only the worker half of resolveWorkerForWire's result is + // used. + final resolvedRule = RemoteTriggerRule( + payloadKey: rule.payloadKey, + workerMappings: rule.workerMappings.map( + (key, worker) => MapEntry(key, resolveWorkerForWire(worker).$1), + ), + secretKey: rule.secretKey, + ); return NativeWorkManagerPlatform.instance.registerRemoteTrigger( source: source, - rule: rule, + rule: resolvedRule, ); } diff --git a/lib/src/task_graph.dart b/lib/src/task_graph.dart index b072e28..d9116a4 100644 --- a/lib/src/task_graph.dart +++ b/lib/src/task_graph.dart @@ -47,6 +47,28 @@ class TaskNode { 'constraints': constraints.toMap(), }; } + + /// Same shape as [toMap], but with [worker]/[constraints] run through + /// [NativeWorkManager.resolveWorkerForWire] first — so a [DartWorker] node + /// carries a resolved callback handle instead of reaching native with none + /// at all (found by the 2026-09-23 lib/ audit: [toMap] alone never did this + /// conversion, unlike [NativeWorkManager.enqueue] and task chains). + /// + /// Kept separate from [toMap] so a plain `toMap()` call — used in tests, + /// docs examples, debug printing — never requires + /// `NativeWorkManager.initialize()` to have run. Only [enqueueTaskGraph] + /// calls this, at the moment a graph is actually sent to native. + Map _toResolvedMap() { + final (resolvedWorker, resolvedConstraints) = + NativeWorkManager.resolveWorkerForWire(worker, constraints); + return { + 'id': id, + 'workerClassName': resolvedWorker.workerClassName, + 'workerConfig': resolvedWorker.toMap(), + 'dependsOn': dependsOn, + 'constraints': (resolvedConstraints ?? constraints).toMap(), + }; + } } /// A directed acyclic graph (DAG) of background tasks. @@ -216,6 +238,16 @@ class TaskGraph { 'nodes': _nodes.map((n) => n.toMap()).toList(), }; } + + /// Same shape as [toMap], but every node goes through + /// [TaskNode._toResolvedMap] first. See that method for why this is + /// separate from [toMap]. + Map _toResolvedMap() { + return { + 'id': id, + 'nodes': _nodes.map((n) => n._toResolvedMap()).toList(), + }; + } } /// Result of a [TaskGraph] execution. @@ -462,7 +494,7 @@ Future enqueueTaskGraph(TaskGraph graph) async { // 1. Send graph to native for persistent orchestration. // This ensures the graph continues even if the app is killed. - await NativeWorkManagerPlatform.instance.enqueueGraph(graph.toMap()); + await NativeWorkManagerPlatform.instance.enqueueGraph(graph._toResolvedMap()); // 2. Start the Dart-side listener so we can resolve the result future // if the app stays alive. diff --git a/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart b/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart new file mode 100644 index 0000000..5034ae8 --- /dev/null +++ b/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart @@ -0,0 +1,181 @@ +import 'package:flutter/foundation.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:native_workmanager/native_workmanager.dart'; +import 'package:native_workmanager/src/platform_interface.dart'; +import 'package:plugin_platform_interface/plugin_platform_interface.dart'; + +/// Guards a real gap found by the 2026-09-23 lib/ audit. +/// +/// [NativeWorkManager.enqueue] and task chains convert a [DartWorker] to a +/// [DartWorkerInternal] (resolving its native callback handle) before +/// sending it to native. [TaskGraph] nodes and [RemoteTriggerRule] +/// `workerMappings` never did — they called the worker's own `toMap()` +/// directly, which for a plain `DartWorker` never includes `callbackHandle` +/// at all (only `DartWorkerInternal.toMap()` does). Native's +/// `DartCallbackWorker` requires `callbackHandle`, so the task was enqueued +/// but the callback could never resolve — a silent no-op, discoverable only +/// by the task never running. +class _MockPlatform extends NativeWorkManagerPlatform + with MockPlatformInterfaceMixin { + Map? capturedGraphMap; + RemoteTriggerRule? capturedRule; + Map? capturedChainMap; + + @override + Future initialize({ + int? callbackHandle, + bool debugMode = false, + int maxConcurrentTasks = 4, + int diskSpaceBufferMB = 20, + int cleanupAfterDays = 30, + bool enforceHttps = false, + bool blockPrivateIPs = false, + bool registerPlugins = false, + }) async {} + + @override + void setCallbackExecutor( + Future Function(String callbackId, Map? input) + executor) {} + + @override + Future enqueueGraph(Map graphMap) async { + capturedGraphMap = graphMap; + return 'accepted'; + } + + // enqueueTaskGraph() subscribes to this right after enqueueGraph() — an + // empty stream is enough since these tests only assert on the outgoing + // payload, not on graph completion. + @override + Stream get events => const Stream.empty(); + + @override + Future registerRemoteTrigger({ + required RemoteTriggerSource source, + required RemoteTriggerRule rule, + }) async { + capturedRule = rule; + } + + @override + Future enqueueChain(Map chainMap) async { + capturedChainMap = chainMap; + return ScheduleResult.accepted; + } +} + +// Top-level function: PluginUtilities.getCallbackHandle requires this. +Future _testCallback(Map? input) async => true; + +void main() { + late _MockPlatform mockPlatform; + + setUp(() { + mockPlatform = _MockPlatform(); + NativeWorkManagerPlatform.instance = mockPlatform; + NativeWorkManager.initialize( + dartWorkers: {'test-worker': _testCallback}, + ); + }); + + group('TaskGraph node DartWorker resolution', () { + test('a DartWorker node carries a resolved callbackHandle', () async { + final graph = TaskGraph(id: 'g1') + ..add(TaskNode( + id: 'a', + worker: DartWorker(callbackId: 'test-worker'), + )); + + await NativeWorkManager.enqueueGraph(graph); + + final nodes = mockPlatform.capturedGraphMap!['nodes'] as List; + final nodeConfig = (nodes.first as Map)['workerConfig'] as Map; + expect(nodeConfig['callbackHandle'], isNotNull, + reason: 'Without resolution, a plain DartWorker.toMap() never ' + 'includes callbackHandle — native could never resolve it.'); + }); + + test('a non-DartWorker node is unaffected', () async { + final graph = TaskGraph(id: 'g2') + ..add(TaskNode( + id: 'a', + worker: NativeWorker.httpSync(url: 'https://example.com'), + )); + + await NativeWorkManager.enqueueGraph(graph); + + final nodes = mockPlatform.capturedGraphMap!['nodes'] as List; + expect((nodes.first as Map)['workerClassName'], 'HttpSyncWorker'); + }); + + test('a DartWorker node is promoted to isHeavyTask on iOS', () async { + debugDefaultTargetPlatformOverride = TargetPlatform.iOS; + addTearDown(() => debugDefaultTargetPlatformOverride = null); + + final graph = TaskGraph(id: 'g3') + ..add(TaskNode( + id: 'a', + worker: DartWorker(callbackId: 'test-worker'), + constraints: const Constraints(isHeavyTask: false), + )); + + await NativeWorkManager.enqueueGraph(graph); + + final nodes = mockPlatform.capturedGraphMap!['nodes'] as List; + final constraints = (nodes.first as Map)['constraints'] as Map; + expect(constraints['isHeavyTask'], isTrue); + }); + }); + + group('RemoteTriggerRule workerMappings DartWorker resolution', () { + test('a mapped DartWorker carries a resolved callbackHandle', () async { + await NativeWorkManager.registerRemoteTrigger( + source: RemoteTriggerSource.fcm, + rule: RemoteTriggerRule( + payloadKey: 'action', + workerMappings: { + 'sync': DartWorker(callbackId: 'test-worker'), + }, + ), + ); + + final mapped = mockPlatform.capturedRule!.workerMappings['sync']!; + expect(mapped, isA()); + expect((mapped as DartWorkerInternal).callbackHandle, isNonZero); + }); + + test('a mapped NativeWorker is unaffected', () async { + await NativeWorkManager.registerRemoteTrigger( + source: RemoteTriggerSource.fcm, + rule: RemoteTriggerRule( + payloadKey: 'action', + workerMappings: { + 'sync': NativeWorker.httpSync(url: 'https://example.com'), + }, + ), + ); + + final mapped = mockPlatform.capturedRule!.workerMappings['sync']!; + expect(mapped, isNot(isA())); + }); + }); + + group('Task chain DartWorker resolution (regression guard)', () { + test('a chain-step DartWorker is promoted to isHeavyTask on iOS', () async { + debugDefaultTargetPlatformOverride = TargetPlatform.iOS; + addTearDown(() => debugDefaultTargetPlatformOverride = null); + + await NativeWorkManager.beginWith( + TaskRequest(id: 'step1', worker: DartWorker(callbackId: 'test-worker')), + ).enqueue(); + + final steps = mockPlatform.capturedChainMap!['steps'] as List; + final firstStep = (steps.first as List).first as Map; + final constraints = firstStep['constraints'] as Map; + expect(constraints['isHeavyTask'], isTrue, + reason: 'This was skipped before resolveWorkerForWire unified the ' + 'chain and enqueue() code paths.'); + }); + }); +} From 0f8620aac9a8521337b7adc995342e91ecf5f2f0 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: Wed, 23 Sep 2026 20:03:27 +0700 Subject: [PATCH 3/8] =?UTF-8?q?fix(ios):=20existingPolicy=20was=20never=20?= =?UTF-8?q?read=20on=20the=20foreground=20path=20=E2=80=94=20port=20issue?= =?UTF-8?q?=20#72's=20executionId=20fix=20to=20iOS?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while investigating the lib/ audit's iOS-foreground-Zone-binding finding. The real bug was bigger: iOS's handleEnqueue never read existingPolicy at all — every repeat enqueue() of one taskId silently started a second, fully independent concurrent Task, regardless of what policy the caller asked for. Confirmed on a simulator: two DartWorker executions of one taskId, 600ms apart, both ran to full completion independently. existingPolicy: .replace (the default, matching Android/enqueue()'s own default) now cancels the outgoing execution before starting the new one; .keep now leaves the running execution alone and ignores the new request — both matching WorkManager's semantics on Android. 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." Implementing that alone reproduced issue #72's exact bug shape on iOS: the replacement execution resolves almost instantly (same running engine, no boot delay), sees the outgoing execution's cancellation mark, and its own cleanup — keyed by bare taskId — cleared that mark before the outgoing execution's next poll could observe it. Confirmed the same way: the new execution died at iteration 1 (correctly saw the mark), the old one ran all 50 (the mark it needed was already gone). So this also ports issue #72's fix to iOS: - DartTaskCancellationRegistry is now keyed by a fresh per-execution id (executionId -> taskId), not by bare taskId, mirroring DartTaskCancellationRegistry.kt. A new currentExecutionId (taskId -> executionId) lets the 7 call sites that only ever knew a taskId (handleCancel/cancelAll/cancelByTag/notification-cancel/BGTask-expiration) keep their unchanged signature — markCancelled(taskId) resolves the current execution internally. - executeDartWorkerViaMethodChannel mints the executionId (mirroring Android's DartCallbackWorker, which mints one per doWork() call) and injects it as __executionId in the input JSON, alongside the existing __taskId. This function is shared by both the foreground and headless paths, so both get it. - method_channel.dart's _executeDartCallback now binds executionId into a Zone around the callback call, exactly like the headless isolate's _callbackDispatcher already did — this is the actual Zone-binding gap the audit originally flagged, now fixed as part of the full picture. - Renamed the Zone key from executionIdZoneKeyForTesting to executionIdZoneKey (@internal, not @visibleForTesting) since it's now used by production code across library boundaries, not just tests. Known residual limitation: two rapid-fire replaces for the same taskId, both before the first replacement's execution has started running, can still race (the executionId is minted lazily when the replacement starts, not synchronously at enqueue time). Far narrower than what this fixes; not expected to matter in practice — documented in the new test's comments. Device-verified on an iOS simulator: both existingPolicy.replace and .keep, plus a full run of the Cancellation group (issue_66, issue_75, issue_72 [Android-only, correctly still skipped], the new lib_audit_3 tests, issue_69) with no regressions. No Android changes — its existingPolicy handling and DartTaskCancellationRegistry were already correct (that's what issue #72 fixed there originally). --- .../device_integration_test.dart | 144 ++++++++++++++++++ .../NativeWorkmanagerPlugin+Execution.swift | 54 ++++--- .../NativeWorkmanagerPlugin.swift | 73 +++++++-- .../engine/DartTaskCancellationRegistry.swift | 122 +++++++++++---- .../engine/FlutterEngineManager.swift | 11 +- lib/native_workmanager.dart | 2 +- lib/src/method_channel.dart | 41 +++-- lib/src/native_work_manager.dart | 24 ++- ...ssue_66_dart_worker_cancellation_test.dart | 9 +- 9 files changed, 391 insertions(+), 89 deletions(-) diff --git a/example/integration_test/device_integration_test.dart b/example/integration_test/device_integration_test.dart index 1e63d2e..c8e6ae3 100644 --- a/example/integration_test/device_integration_test.dart +++ b/example/integration_test/device_integration_test.dart @@ -2070,6 +2070,150 @@ void main() { }, ); + testWidgets( + 'lib_audit_3: existingPolicy.replace cancels the outgoing execution ' + 'precisely and the replacement runs to completion, not inheriting its ' + 'stale cancel mark (iOS)', + (tester) async { + // https://github.com/brewkits/native_workmanager — found by the + // 2026-09-23 lib/ audit while investigating the iOS analogue of + // issue_72 above. Two things were wrong before this fix, discovered + // in order: + // 1. iOS's foreground handleEnqueue never read existingPolicy at + // all — every repeat enqueue() of one taskId silently started a + // second, fully independent concurrent execution, confirmed by + // running exactly this test's shape pre-fix: both old and new + // ran to full completion (50/50), regardless of policy. + // 2. Fixing #1 alone (mark-and-replace on the outgoing execution) + // reproduced issue_72's "direction 1" bug on iOS: the new + // execution resolves almost instantly (same running engine, no + // boot delay), sees the mark, and its own cleanup — keyed by + // bare taskId — cleared it before the outgoing execution's next + // poll could observe it. Confirmed the same way: the new + // execution died at iteration 1, the old one ran all 50. + // The real fix needed both: existingPolicy.replace AND per-execution + // (executionId-keyed) cancellation tracking, mirroring issue #72's + // Android fix, ported to iOS's DartTaskCancellationRegistry. + if (!Platform.isIOS) { + markTestSkipped('iOS foreground existingPolicy path'); + return; + } + + 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'); + + await NativeWorkManager.enqueue( + taskId: id, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker( + callbackId: 'dit_cancel_poll', + input: {'counterFile': oldCounterFile.path}, + ), + ); + + await Future.delayed(const Duration(milliseconds: 600)); + + // Default policy is replace. + await NativeWorkManager.enqueue( + taskId: id, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker( + callbackId: 'dit_cancel_poll', + input: {'counterFile': newCounterFile.path}, + ), + ); + + await Future.delayed(const Duration(seconds: 12)); + + expect( + oldCounterFile.existsSync(), + isTrue, + reason: 'lib_audit_3: the replaced execution must have started', + ); + final oldIterations = + int.parse(oldCounterFile.readAsStringSync().trim()); + expect( + oldIterations, + lessThan(50), + reason: 'lib_audit_3: the replaced execution must have observed ' + 'cancellation and stopped', + ); + + expect( + newCounterFile.existsSync(), + isTrue, + reason: 'lib_audit_3: the replacement execution must have started', + ); + final newIterations = + int.parse(newCounterFile.readAsStringSync().trim()); + expect( + newIterations, + equals(50), + 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', + ); + }, + ); + + testWidgets( + 'lib_audit_3: existingPolicy.keep leaves the running execution alone ' + 'and ignores the new request (iOS)', + (tester) async { + if (!Platform.isIOS) { + markTestSkipped('iOS foreground existingPolicy path'); + return; + } + + final id = _id('lib_audit_3_keep'); + final oldCounterFile = File('${tmpDir.path}/lib_audit_3_keep_old.txt'); + final newCounterFile = File('${tmpDir.path}/lib_audit_3_keep_new.txt'); + + await NativeWorkManager.enqueue( + taskId: id, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker( + callbackId: 'dit_cancel_poll', + input: {'counterFile': oldCounterFile.path}, + ), + ); + + await Future.delayed(const Duration(milliseconds: 600)); + + await NativeWorkManager.enqueue( + taskId: id, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker( + callbackId: 'dit_cancel_poll', + input: {'counterFile': newCounterFile.path}, + ), + existingPolicy: ExistingTaskPolicy.keep, + ); + + await Future.delayed(const Duration(seconds: 12)); + + expect( + oldCounterFile.existsSync(), + isTrue, + reason: 'lib_audit_3: keep must leave the running execution alone', + ); + expect( + int.parse(oldCounterFile.readAsStringSync().trim()), + equals(50), + reason: 'lib_audit_3: keep must not cancel the running execution', + ); + expect( + newCounterFile.existsSync(), + isFalse, + reason: 'lib_audit_3: keep must ignore the new request entirely — ' + 'no second execution should ever have started', + ); + }, + ); + 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+Execution.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Execution.swift index c73df79..3917d75 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Execution.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Execution.swift @@ -578,24 +578,36 @@ extension NativeWorkmanagerPlugin { workerConfig: [String: Any], taskId: String ) async -> WorkerResult { - // Issue #66: whichever branch below runs, always drop this taskId's - // cancellation-registry entry once execution is done — otherwise a - // cancelled taskId (or, worse, a reused one on a later run) leaks or - // misreports "cancelled" forever. - defer { DartTaskCancellationRegistry.shared.clear(taskId) } - // Issue #75: clear() above already drops the stop notifier, but be - // explicit — an early `return` on a config error below must not leave a - // notifier behind that a later cancel of the same taskId would fire. - defer { DartTaskCancellationRegistry.shared.clearStopNotifier(taskId) } + // Issue #72: mint a fresh executionId for THIS invocation, distinct + // from taskId, mirroring Android's DartCallbackWorker (which mints + // one per doWork() call). handleEnqueue's existingPolicy: .replace + // can cancel an outgoing execution and start a new one under the SAME + // 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) + // 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) + // leaks or misreports "cancelled" forever. Scoped to executionId, not + // taskId, so a slow-finishing outgoing execution's cleanup can never + // wipe a replacing execution's tracking or stop notifier out from + // under it (endExecution only clears taskId-level state if it still + // points at this executionId) — see DartTaskCancellationRegistry. + defer { DartTaskCancellationRegistry.shared.endExecution(executionId, taskId: taskId) } guard let callbackId = workerConfig["callbackId"] as? String else { return WorkerResult.failure(message: "DartCallbackWorker: missing callbackId in config") } - // Inject __taskId into the input JSON so the Dart callback can call - // NativeWorkManager.reportDartWorkerProgress(). The Dart side only receives - // the inner "input" string — mirror Android's DartCallbackWorker, which merges - // the outer __taskId into that inner object before forwarding to Dart. - let input = Self.mergeTaskId(into: workerConfig["input"] as? String, taskId: taskId) + // Inject __taskId (progress reporting) and __executionId (issue #72 + // cancellation precision) into the input JSON. The Dart side only + // receives the inner "input" string — mirrors Android's + // DartCallbackWorker, which merges both into that inner object before + // forwarding to Dart. + let input = Self.mergeTaskId(into: workerConfig["input"] as? String, taskId: taskId, executionId: executionId) // Honor user-configured DartWorker.timeoutMs across both execution paths // (foreground main-channel and killed-app FlutterEngineManager fallback). @@ -709,11 +721,12 @@ extension NativeWorkmanagerPlugin { } } - /// Merge `__taskId` into a DartWorker input JSON string so the callback can - /// report progress. Returns a JSON object string. Mirrors Android's - /// DartCallbackWorker input enrichment; falls back to the original string if - /// the input is a non-object JSON that cannot carry the key. - static func mergeTaskId(into inputJson: String?, taskId: String) -> String? { + /// Merge `__taskId` (progress reporting) and, when given, `__executionId` + /// (issue #72 cancellation precision) into a DartWorker input JSON + /// string. Returns a JSON object string. Mirrors Android's + /// DartCallbackWorker input enrichment; falls back to the original string + /// if the input is a non-object JSON that cannot carry the keys. + static func mergeTaskId(into inputJson: String?, taskId: String, executionId: String? = nil) -> String? { var obj: [String: Any] = [:] if let json = inputJson, !json.isEmpty, json != "null" { if let data = json.data(using: .utf8), @@ -725,6 +738,9 @@ extension NativeWorkmanagerPlugin { } } obj["__taskId"] = taskId + if let executionId { + obj["__executionId"] = executionId + } guard let merged = try? JSONSerialization.data(withJSONObject: obj), let mergedString = String(data: merged, encoding: .utf8) else { return inputJson diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift index 0ebd8f3..fdc4114 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift @@ -134,7 +134,15 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { // headless FlutterEngineManager engine — see DartTaskCancellationRegistry). case "isTaskCancelled": let taskId = args?["taskId"] as? String ?? "" - result(DartTaskCancellationRegistry.shared.isCancelled(taskId)) + // Issue #72: precise per-execution check when the Dart side's + // Zone had an executionId to send (see method_channel.dart's + // _executeDartCallback); falls back to the coarse taskId + // check otherwise. + if let executionId = args?["executionId"] as? String { + result(DartTaskCancellationRegistry.shared.isCancelled(executionId: executionId)) + } else { + result(DartTaskCancellationRegistry.shared.isCancelled(taskId)) + } default: result(FlutterMethodNotImplemented) } @@ -362,21 +370,58 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { let directQos = (directConstraintsMap?["qos"] as? String) ?? "background" let directRetryConfig = RetryConfig.from(constraintsMap: directConstraintsMap) - let task = Task { [weak self] in - guard let self else { return } - if initialDelayMs > 0 { - try? await Task.sleep(nanoseconds: UInt64(initialDelayMs) * 1_000_000) + // existingPolicy was accepted from Dart but never read here — every repeat + // enqueue() of the same taskId silently started a second, fully independent + // concurrent Task, regardless of what policy the caller asked for, because this + // dictionary write always just clobbered whatever was there. Confirmed on a + // simulator (2026-09-23 lib/ audit): two DartWorker executions of one taskId, + // 600ms apart, both ran to full completion independently. "replace" (the + // default, matching Android and NativeWorkManager.enqueue's own default) now + // stops the outgoing execution the same way handleCancel does before starting + // 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 + stateQueue.sync(flags: .barrier) { + if let existingTask = self.activeTasks[taskId] { + if existingPolicyStr == "keep" { + skippedForKeep = true + return + } + // Cancelling the Swift Task only unblocks whatever it's synchronously + // awaiting (irrelevant for a DartCallbackWorker, which awaits a method + // channel round-trip, not a cancellable operation). The registry mark is + // what a running DartWorker's isTaskCancelled() poll actually sees — + // same two calls handleCancel makes for an explicit user cancel(). + existingTask.cancel() + DartTaskCancellationRegistry.shared.markCancelled(taskId) + self.workers[taskId]?.stop() } - guard !Task.isCancelled else { return } - await self.executeWorkerSync( - taskId: taskId, - workerClassName: workerClassName, - workerConfig: workerConfig, - qos: directQos, - retryConfig: directRetryConfig - ) + + let task = Task { [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 + ) + } + self.activeTasks[taskId] = task + } + if skippedForKeep { + NativeLogger.d("handleEnqueue: '\(taskId)' already running, existingPolicy=keep — new request ignored") } - stateQueue.sync(flags: .barrier) { self.activeTasks[taskId] = task } result("ACCEPTED") } diff --git a/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift b/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift index d5e654d..053a979 100644 --- a/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift +++ b/ios/native_workmanager/Sources/native_workmanager/engine/DartTaskCancellationRegistry.swift @@ -1,38 +1,99 @@ import Foundation -/// Tracks which DartWorker task IDs have been cancelled, so a Dart callback -/// running in either Flutter engine (the main isolate, or the headless -/// engine spun up by `FlutterEngineManager`) can ask +/// Tracks which DartWorker **executions** have been cancelled, so a Dart +/// callback running in either Flutter engine (the main isolate, or the +/// headless engine spun up by `FlutterEngineManager`) can ask /// `NativeWorkManager.isTaskCancelled(taskId)` (issue #66) and get a real /// answer instead of always `false`. /// +/// Keyed by **executionId**, not by the app-level `taskId` (issue #72, +/// ported from Android's `DartTaskCancellationRegistry.kt`). A single +/// `taskId` can have more than one execution alive at once: `handleEnqueue`'s +/// `existingPolicy: .replace` (the `enqueue()` default) now cancels the +/// outgoing execution and immediately starts a new one under the SAME +/// `taskId`. A bare `taskId`-keyed mark/clear cannot tell those two +/// executions apart — confirmed on a simulator (2026-09-23 lib/ audit): +/// without this, the new execution's own `clear` wiped the mark meant for the +/// outgoing one, and the outgoing one ran to full completion never having +/// noticed it should stop. +/// /// Marked from every place that cancels a running task — /// `NativeWorkmanagerPlugin`'s `cancel`/`cancelAll`/`cancelByTag` handlers, -/// notification-driven cancel, and `BGTaskSchedulerManager`'s expiration -/// handler. Read from the `dev.brewkits/dart_worker_channel` handlers in -/// both `NativeWorkmanagerPlugin` (main isolate) and `FlutterEngineManager` +/// notification-driven cancel, `BGTaskSchedulerManager`'s expiration handler, +/// and `handleEnqueue`'s replace path — via the taskId-only `markCancelled`, +/// which resolves to whichever executionId is currently live for that taskId +/// (none of those call sites know a specific executionId; only +/// `executeDartWorkerViaMethodChannel`, which mints one per invocation, does). +/// Read from the `dev.brewkits/dart_worker_channel` handlers in both +/// `NativeWorkmanagerPlugin` (main isolate) and `FlutterEngineManager` /// (headless isolate). /// -/// This is **cooperative only**: marking a taskId here does not interrupt -/// whatever the Dart isolate is currently `await`-ing — it only lets a -/// polling callback see the request and return early. +/// This is **cooperative only**: marking an execution here does not +/// interrupt whatever the Dart isolate is currently `await`-ing — it only +/// lets a polling callback see the request and return early. final class DartTaskCancellationRegistry { static let shared = DartTaskCancellationRegistry() private init() {} private let lock = NSLock() - private var cancelled: Set = [] - /// Issue #75: per-task stop notifiers, keyed by taskId. + /// executionId -> taskId. The value is only needed for the coarse + /// taskId-only `isCancelled` fallback (callers with no executionId). + private var cancelled: [String: String] = [:] + + /// taskId -> the executionId of whichever execution is currently "the" + /// live one for that taskId. This is what a bare `markCancelled(taskId)` + /// call (from every call site except `executeDartWorkerViaMethodChannel` + /// itself, none of which know a specific executionId) actually targets. + private var currentExecutionId: [String: String] = [:] + + /// Issue #75: per-task stop notifiers, keyed by taskId (not per-execution + /// — a known, deliberately out-of-scope simplification; see the 2026-09-23 + /// lib/ audit notes. Two overlapping executions of one taskId could still + /// clobber each other's notifier registration, same as before this file's + /// issue #72 rework). /// /// Registered by whichever execution path is actually running the callback /// (main channel vs headless engine), because only that path knows which /// channel to notify on. Hanging this off the registry means every existing /// `markCancelled` call site — explicit cancel, cancelByTag, cancelAll, - /// notification-driven cancel, BGTask expiration — fires the notification - /// for free, instead of seven sites each having to remember to. + /// notification-driven cancel, BGTask expiration, replace — fires the + /// notification for free, instead of each having to remember to. private var stopNotifiers: [String: (Int64?) -> Void] = [:] + // 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. + func beginExecution(_ executionId: String, taskId: String) { + lock.lock() + defer { lock.unlock() } + currentExecutionId[taskId] = executionId + } + + /// Drop `executionId`'s tracking once it has finished, successfully or + /// not — otherwise every cancelled execution leaks in `cancelled` forever. + /// + /// Only clears `currentExecutionId[taskId]` (and `taskId`'s stop + /// notifier) if it still points at THIS executionId. Without that guard, + /// a slow-finishing outgoing execution's cleanup could run AFTER a + /// replacing execution has already begun and wipe the newer execution's + /// tracking and notifier out from under it. + func endExecution(_ executionId: String, taskId: String) { + lock.lock() + cancelled.removeValue(forKey: executionId) + if currentExecutionId[taskId] == executionId { + currentExecutionId.removeValue(forKey: taskId) + stopNotifiers.removeValue(forKey: taskId) + } + lock.unlock() + } + + // MARK: - Stop notifiers (issue #75) + /// Register the stop notifier for a running DartWorker execution (issue #75). /// /// `cancelGraceMs` is handed to the notifier rather than stored here: the @@ -54,15 +115,20 @@ final class DartTaskCancellationRegistry { stopNotifiers.removeValue(forKey: taskId) } - /// Record that `taskId` has been cancelled/stopped. + // MARK: - Marking / reading cancellation + + /// Mark `taskId`'s CURRENT execution cancelled/stopped. Used by every + /// cancellation source that only knows a taskId — `cancel`/`cancelAll`/ + /// `cancelByTag`, notification-driven cancel, and BGTask expiration. /// /// Issue #75: also fires that task's stop notifier, exactly once — the - /// notifier is removed as it is taken, so a `cancelAll` that sweeps the same - /// taskId twice, or an explicit cancel racing a BGTask expiration, cannot - /// notify the Dart handler twice. + /// notifier is removed as it is taken, so sweeping the same taskId twice + /// (e.g. an explicit cancel racing a BGTask expiration) cannot notify the + /// Dart handler twice. func markCancelled(_ taskId: String) { lock.lock() - cancelled.insert(taskId) + let executionId = currentExecutionId[taskId] ?? taskId + cancelled[executionId] = taskId let notifier = stopNotifiers.removeValue(forKey: taskId) lock.unlock() @@ -73,19 +139,23 @@ final class DartTaskCancellationRegistry { notifier?(nil) } - /// Whether `taskId` has been marked cancelled. - func isCancelled(_ taskId: String) -> Bool { + /// Whether the execution identified by `executionId` is cancelled. + /// Precise — use this whenever an executionId is available (from + /// `isTaskCancelled`'s Zone-bound `executionId` argument). + func isCancelled(executionId: String) -> Bool { lock.lock() defer { lock.unlock() } - return cancelled.contains(taskId) + return cancelled[executionId] != nil } - /// Remove `taskId`'s entry once its execution has finished, successfully - /// or not — otherwise every cancelled taskId leaks in this set forever. - func clear(_ taskId: String) { + /// Coarse: whether `taskId`'s CURRENT execution is cancelled. Fallback + /// for callers with no executionId in scope. Do not use this when an + /// executionId is available — it cannot distinguish a replaced + /// generation of the same taskId from its replacement. + func isCancelled(_ taskId: String) -> Bool { lock.lock() defer { lock.unlock() } - cancelled.remove(taskId) - stopNotifiers.removeValue(forKey: taskId) + let executionId = currentExecutionId[taskId] ?? taskId + return cancelled[executionId] != nil } } diff --git a/ios/native_workmanager/Sources/native_workmanager/engine/FlutterEngineManager.swift b/ios/native_workmanager/Sources/native_workmanager/engine/FlutterEngineManager.swift index adaa030..0ca973f 100644 --- a/ios/native_workmanager/Sources/native_workmanager/engine/FlutterEngineManager.swift +++ b/ios/native_workmanager/Sources/native_workmanager/engine/FlutterEngineManager.swift @@ -495,8 +495,15 @@ class FlutterEngineManager { } else if call.method == "isTaskCancelled" { // Issue #66: cooperative cancellation poll from inside a // running DartWorker callback. See DartTaskCancellationRegistry. - let taskId = (call.arguments as? [String: Any])?["taskId"] as? String ?? "" - result(DartTaskCancellationRegistry.shared.isCancelled(taskId)) + // Issue #72: precise per-execution check when provided (see + // NativeWorkmanagerPlugin.swift's identical handler). + let args = call.arguments as? [String: Any] + let taskId = args?["taskId"] as? String ?? "" + if let executionId = args?["executionId"] as? String { + result(DartTaskCancellationRegistry.shared.isCancelled(executionId: executionId)) + } else { + result(DartTaskCancellationRegistry.shared.isCancelled(taskId)) + } } else { result(FlutterMethodNotImplemented) } diff --git a/lib/native_workmanager.dart b/lib/native_workmanager.dart index 3e8e1bb..c0fe048 100644 --- a/lib/native_workmanager.dart +++ b/lib/native_workmanager.dart @@ -41,7 +41,7 @@ export 'src/task_id.dart'; export 'src/enqueue_request.dart'; export 'src/events.dart'; export 'src/ios_live_activity_bridge.dart'; -export 'src/native_work_manager.dart'; +export 'src/native_work_manager.dart' hide executionIdZoneKey; export 'src/observability.dart'; export 'src/offline_queue.dart'; export 'src/middleware.dart'; diff --git a/lib/src/method_channel.dart b/lib/src/method_channel.dart index 4e2d647..9018b6e 100644 --- a/lib/src/method_channel.dart +++ b/lib/src/method_channel.dart @@ -9,7 +9,7 @@ import 'battery_restriction.dart'; import 'constraints.dart'; import 'events.dart'; import 'native_work_manager.dart' - show resolveDispatcherTimeout, resolveStopHandlerBudget; + show executionIdZoneKey, resolveDispatcherTimeout, resolveStopHandlerBudget; import 'platform_interface.dart'; import 'remote_trigger.dart'; import 'task_trigger.dart'; @@ -206,17 +206,34 @@ class MethodChannelNativeWorkManager extends NativeWorkManagerPlatform { // native-side BGTask deadline (release on a real device) — in tests it // ran to completion regardless of timeoutMs. Mirrors the dispatcher. final timeoutDuration = resolveDispatcherTimeout(args); - return _callbackExecutor!(callbackId, input).timeout( - timeoutDuration, - onTimeout: () { - developer.log( - '[NativeWorkManager] DartWorker callback "$callbackId" timed out ' - 'after ${timeoutDuration.inSeconds} s on the main method channel. ' - 'Increase DartWorker.timeoutMs or split the work.', - level: 900, - ); - return false; - }, + + // Issue #72 (iOS foreground/simulator — found by the 2026-09-23 lib/ + // audit): bind this invocation's executionId into a Zone around the + // callback call, exactly like the headless isolate's + // `_callbackDispatcher` does. Without this, isTaskCancelled() falls back + // to its coarse by-taskId check on this path, which is exactly the + // instance-identity bug issue #72 fixed for the headless path — a + // DartWorker replaced via `existingPolicy: .replace` (the enqueue() + // default) could have its brand-new, legitimate execution mistaken for + // the outgoing one it replaced, since both share one taskId. iOS's + // `executeDartWorkerViaMethodChannel` mints and injects `__executionId` + // into `input` for this exact reason. + final executionId = input?['__executionId'] as String?; + + return runZoned( + () => _callbackExecutor!(callbackId, input).timeout( + timeoutDuration, + onTimeout: () { + developer.log( + '[NativeWorkManager] DartWorker callback "$callbackId" timed out ' + 'after ${timeoutDuration.inSeconds} s on the main method channel. ' + 'Increase DartWorker.timeoutMs or split the work.', + level: 900, + ); + return false; + }, + ), + zoneValues: {executionIdZoneKey: executionId}, ); } diff --git a/lib/src/native_work_manager.dart b/lib/src/native_work_manager.dart index d8007bb..e173d33 100644 --- a/lib/src/native_work_manager.dart +++ b/lib/src/native_work_manager.dart @@ -137,16 +137,14 @@ Duration resolveStopHandlerBudget(Map args) { /// Issue #72: Zone key `isTaskCancelled` uses to find the executionId of /// whichever DartWorker invocation is currently running, without requiring -/// the app's callback to pass it explicitly. See `_callbackDispatcher`. -const Object _executionIdZoneKey = #nativeWorkmanagerExecutionId; - -/// Test-only accessor for [_executionIdZoneKey] — lets unit tests simulate -/// being inside a dispatched DartWorker callback's Zone without needing to -/// go through the full `_callbackDispatcher` (which is private and driven by -/// a native-supplied callback handle, not something a unit test can invoke -/// directly). Not part of the public API. -@visibleForTesting -const Object executionIdZoneKeyForTesting = _executionIdZoneKey; +/// the app's callback to pass it explicitly. Bound by both the headless +/// isolate's `_callbackDispatcher` and, since the 2026-09-23 lib/ audit's +/// iOS-foreground fix, `method_channel.dart`'s `_executeDartCallback` — not +/// private, so that separate library can use the same key. Also used by unit +/// tests to simulate being inside a dispatched DartWorker callback's Zone +/// without going through the full dispatcher. Not part of the public API. +@internal +const Object executionIdZoneKey = #nativeWorkmanagerExecutionId; /// Top-level callback dispatcher for background Dart execution. /// @@ -280,7 +278,7 @@ Future _callbackDispatcher() async { return false; }, ), - zoneValues: {_executionIdZoneKey: executionId}, + zoneValues: {executionIdZoneKey: executionId}, ); // Return execution result to native side @@ -352,7 +350,7 @@ Future _callbackDispatcher() async { ); }), zoneValues: { - _executionIdZoneKey: input?['__executionId'] as String?, + executionIdZoneKey: input?['__executionId'] as String?, }, ); } catch (e, stackTrace) { @@ -1374,7 +1372,7 @@ class NativeWorkManager { // other. Falls back to the coarse by-taskId check when called from // outside that Zone (e.g. main-isolate code with no dispatched // execution in scope). - final executionId = Zone.current[_executionIdZoneKey] as String?; + final executionId = Zone.current[executionIdZoneKey] as String?; final result = await channel.invokeMethod('isTaskCancelled', { 'taskId': taskId, diff --git a/test/unit/issue_66_dart_worker_cancellation_test.dart b/test/unit/issue_66_dart_worker_cancellation_test.dart index f55b15a..b239871 100644 --- a/test/unit/issue_66_dart_worker_cancellation_test.dart +++ b/test/unit/issue_66_dart_worker_cancellation_test.dart @@ -3,6 +3,11 @@ import 'dart:async'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:native_workmanager/native_workmanager.dart'; +// executionIdZoneKey is @internal (not part of the public API, so hidden +// from the barrel above) — reachable directly since this test lives inside +// the same package. +import 'package:native_workmanager/src/native_work_manager.dart' + show executionIdZoneKey; /// Issue #66: https://github.com/brewkits/native_workmanager/discussions/66 /// @@ -87,13 +92,13 @@ void main() { // forward it transparently and native can answer per-execution instead // of per-taskId. This test cannot drive the real dispatcher (private, // driven by a native-supplied callback handle) but simulates being - // inside its Zone via the test-only [executionIdZoneKeyForTesting] hook. + // inside its Zone via the test-only [executionIdZoneKey] hook. test( 'forwards the current execution\'s executionId from the dispatcher Zone, when present', () async { final result = await runZoned( () => NativeWorkManager.isTaskCancelled('shared-task'), - zoneValues: {executionIdZoneKeyForTesting: 'exec-123'}, + zoneValues: {executionIdZoneKey: 'exec-123'}, ); expect(result, isFalse); From 110b06bce648b5f510f2d1e1d2b3d45490fcd203 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: Wed, 23 Sep 2026 20:03:27 +0700 Subject: [PATCH 4/8] docs: CHANGELOG entry for the lib/ audit fixes (1.8.3) --- CHANGELOG.md | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5df7d5..a95d70e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,82 @@ 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] + +Fixes 3 issues found by a full `lib/` audit (2026-09-23), reviewed in detail +in the linked commits. Ships as **1.8.3** (1.8.2 is already live on pub.dev +and immutable). + +### Fixed + +- **`moveToSharedStorage()`'s `subDir` had no path-traversal check on either + platform** — the one genuine cross-platform bypass in this batch. Neither + native implementation ever checked it: Android does + `File(publicDir, config.subDir)` directly, iOS does + `docsURL.appendingPathComponent(subDir)` directly, which does **not** + resolve `..` safely. `subDir` feeds `MediaStore.RELATIVE_PATH` on Android + and a Documents subfolder on iOS, so + `moveToSharedStorage(sourcePath: p, subDir: '../../OtherApp/Camera')` + reached native and would have actually escaped the sandbox on both + platforms. Everything else validated in the same commit + (`multiUpload()`'s url/files, `webSocket()`'s url/storeResponseAt, + `ParallelHttpUploadWorker`'s constructor) is Dart-side defense-in-depth, + not a closed system-level hole — native's `SecurityValidator` + (Kotlin/Swift, at parity) already independently validated those; see the + commit for the exact per-worker breakdown. +- **A `DartWorker` placed in a `TaskGraph` node or a `RemoteTriggerRule` + mapping never reached native with a resolved callback handle** — + `enqueue()` and task chains converted `DartWorker` to `DartWorkerInternal` + (resolving the native callback handle) before sending it; `TaskNode` and + `RemoteTriggerRule` never did, so the task was enqueued but its callback + could never be resolved and the task silently never ran. Confirmed + reachable on both platforms before fixing (neither native worker factory + special-cases `DartCallbackWorker`; both require `callbackHandle` and fail + cleanly without it). + - **Behavior change:** a chain-step `DartWorker` is now promoted to + `isHeavyTask: true` on iOS, matching plain `enqueue()`. This was silently + skipped before — chain-step DartWorkers ran on the short + `BGAppRefreshTask` budget (~30s) instead of `BGProcessingTask`'s, far more + likely to be killed mid-execution since a DartWorker needs 1-2s just to + boot its Flutter Engine. + - **Behavior change:** an unregistered `DartWorker` (`callbackId` not + passed to `initialize(dartWorkers:)`) used in a `TaskGraph` node or a + `RemoteTriggerRule` mapping now throws a `StateError` at the point of + `enqueueGraph()`/`registerRemoteTrigger()`, instead of silently reaching + native and never running. +- **iOS's foreground `handleEnqueue` never read `existingPolicy` at all** — + found while investigating the item above. Every repeat `enqueue()` call + for a reused `taskId` silently started a second, fully independent + concurrent execution, regardless of what policy the caller asked for. + Confirmed on a simulator: two `DartWorker` executions of one `taskId`, + 600ms apart, both ran to full completion independently. + `existingPolicy: .replace` (the default) now actually cancels the outgoing + execution before starting the new one; `.keep` now actually leaves the + running execution alone and ignores the new request — both matching + Android's WorkManager semantics. + - Fixing this alone reproduced **issue #72's exact bug shape on iOS**: the + replacement execution resolves almost instantly (same running engine, no + boot delay), sees the outgoing execution's cancellation mark, and its own + cleanup — previously keyed by bare `taskId` — cleared that mark before + the outgoing execution's next poll could observe it. So this also ports + issue #72's fix to iOS: `DartTaskCancellationRegistry` is now keyed by a + fresh per-execution id (minted in `executeDartWorkerViaMethodChannel`, + covering both the foreground and headless paths since they share that + function), not by bare `taskId`, mirroring the Android fix. `isTaskCancelled()` + now binds this id into a Zone on the foreground/simulator path too + (`method_channel.dart`'s `_executeDartCallback`), matching what the + headless isolate's `_callbackDispatcher` already did. + - Known residual limitation: two `existingPolicy: .replace` calls for the + same `taskId` in extremely rapid succession (before the first + replacement's execution has started running) can still race, since the + per-execution id is minted lazily when the replacement execution starts, + not synchronously at enqueue time. Far narrower than the bug this fixes; + not expected to matter in practice. + - Device-verified on an iOS simulator (both `.replace` and `.keep`); no + Android changes were needed (Android's `existingPolicy` handling and + `DartTaskCancellationRegistry` were already correct — that's what issue + #72 fixed). + ## [1.8.2] - 2026-09-23 ### Added From d0a30422633ca3f49b65056ce1b3a264cbe20670 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: Wed, 23 Sep 2026 20:41:55 +0700 Subject: [PATCH 5/8] fix(ios): existingPolicy read a liveness signal that was never cleared on natural completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-pass review caught this before merge (not device-found by accident — went looking for the same defect class right after the previous commit, since that commit was the first code ever to read activeTasks[taskId] as a "is this still running" signal for a scheduling decision). activeTasks[taskId] was never removed when a direct one-time Task finished NATURALLY (success, failure, or timeout) — only the explicit cancel paths (handleCancel/cancelAll/cancelByTag/notification-cancel) ever called removeValue. Before the previous commit that was harmless: nothing read the dict as a liveness signal, so a stale entry just sat there. The previous commit's existingPolicy implementation was the first thing to read it that way, and inherited the staleness bug it depended on not having. Confirmed on a simulator: enqueue a task, wait for it to fully complete (received its success event, then waited 2 more seconds), re-enqueue the SAME taskId with existingPolicy.keep — the new request was silently dropped forever, because activeTasks[taskId] still held the finished task's stale entry and .keep read that as "something is still running." Fix: a fresh generation id, minted per handleEnqueue direct-path call and stored alongside the Task. The Task's own completion (success, failure, timeout, OR early-cancelled return — via defer, so every exit path is covered) removes its activeTasks/activeTaskGenerations entries, but ONLY if the stored generation still matches its own — the same "clear only if still current" guard already used by DartTaskCancellationRegistry.endExecution, so a .replace racing in after the old task started but before it finishes can't have its brand-new entry wiped out by the old task's late cleanup. Also added the matching activeTaskGenerations cleanup to every existing activeTasks-removal site (stopAllWorkers, handleCancel, cancelAll, cancelByTag x2, notification pause, notification cancel) — without it those paths would have left a orphaned generation id behind forever for any taskId that gets cancelled and never reused, an unbounded (if slow-growing) leak. Device-verified on the same simulator: the exact repro above now succeeds (the second enqueue's event fires), and the full Cancellation group (8 tests: cancel by ID, cancelAll, issue_66, issue_75 x2, issue_72 [Android-only, correctly still skipped], both existing lib_audit_3 tests, issue_69) still passes with zero regressions. New test: example/integration_test/device_integration_test.dart's third lib_audit_3 case, right after the existingPolicy.keep test it's a companion to. Known, deliberately out-of-scope finding from the same review pass: handleCancel's resume path (handlePause/handleResume) never stores its resumed Task in activeTasks at all — a resumed task can't be cancelled and existingPolicy can't see it as running. Pre-existing, unrelated to this fix's blast radius, and fixing it needs its own device verification (app-kill-and-resume, which this session's simulator can't exercise) — not folded in here. --- .../device_integration_test.dart | 57 +++++++++++++++++++ .../NativeWorkmanagerPlugin+Cancel.swift | 3 + ...tiveWorkmanagerPlugin+StreamHandlers.swift | 2 + .../NativeWorkmanagerPlugin.swift | 39 +++++++++++++ 4 files changed, 101 insertions(+) diff --git a/example/integration_test/device_integration_test.dart b/example/integration_test/device_integration_test.dart index c8e6ae3..d5db719 100644 --- a/example/integration_test/device_integration_test.dart +++ b/example/integration_test/device_integration_test.dart @@ -2214,6 +2214,63 @@ void main() { }, ); + testWidgets( + 'lib_audit_3: re-enqueuing a taskId whose previous execution already ' + 'completed runs the new one, regardless of existingPolicy (iOS)', + (tester) async { + // Found while re-verifying the two tests above: iOS's activeTasks + // dict is never cleared when a direct one-time task finishes + // NATURALLY (only explicit cancel paths ever call removeValue). + // Before existingPolicy read that dict as a liveness signal this was + // a harmless leak; the first version of the existingPolicy fix + // treated a long-finished taskId as "still running" forever, so + // ANY later re-enqueue with existingPolicy.keep was silently + // dropped — confirmed with this exact test shape before the second + // fix (a per-enqueue generation id, cleared only by the Task that + // is still the current occupant of activeTasks[taskId] when it + // finishes — mirrors DartTaskCancellationRegistry's "clear only if + // still current" guard). + if (!Platform.isIOS) { + markTestSkipped('iOS foreground existingPolicy path'); + return; + } + + final id = _id('lib_audit_3_keep_after_completion'); + + final firstEvent = _waitEvent(id, timeout: const Duration(seconds: 15)); + await NativeWorkManager.enqueue( + taskId: id, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker(callbackId: 'dit_pass'), + ); + final first = await firstEvent; + 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)); + await NativeWorkManager.enqueue( + taskId: id, + trigger: const TaskTrigger.oneTime(), + worker: DartWorker(callbackId: 'dit_pass'), + existingPolicy: ExistingTaskPolicy.keep, + ); + final second = await secondEvent; + expect( + second?.success, + isTrue, + 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( '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 a4426a6..dc0221d 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift @@ -16,6 +16,7 @@ extension NativeWorkmanagerPlugin { activeTasks.keys.forEach { DartTaskCancellationRegistry.shared.markCancelled($0) } activeTasks.values.forEach { $0.cancel() } activeTasks.removeAll() + activeTaskGenerations.removeAll() // see its doc comment on NativeWorkmanagerPlugin taskStates.removeAll() taskTags.removeAll() workers.values.forEach { $0.stop() } @@ -53,6 +54,7 @@ extension NativeWorkmanagerPlugin { DartTaskCancellationRegistry.shared.markCancelled(taskId) // issue #66 activeTasks[taskId]?.cancel() activeTasks.removeValue(forKey: taskId) + activeTaskGenerations.removeValue(forKey: taskId) // see its doc comment taskStates[taskId] = .cancelled taskTags.removeValue(forKey: taskId) workers[taskId]?.stop() @@ -74,6 +76,7 @@ extension NativeWorkmanagerPlugin { stateQueue.async(flags: .barrier) { self.activeTasks[taskId]?.cancel() self.activeTasks.removeValue(forKey: taskId) + self.activeTaskGenerations.removeValue(forKey: taskId) // see its doc comment self.taskStates[taskId] = .cancelled self.taskTags.removeValue(forKey: taskId) self.workers[taskId]?.stop() diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+StreamHandlers.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+StreamHandlers.swift index 0d65742..45cf09e 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+StreamHandlers.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+StreamHandlers.swift @@ -98,6 +98,7 @@ extension NativeWorkmanagerPlugin: UNUserNotificationCenterDelegate { stateQueue.async(flags: .barrier) { self.activeTasks[taskId]?.cancel() self.activeTasks.removeValue(forKey: taskId) + self.activeTaskGenerations.removeValue(forKey: taskId) // see its doc comment self.taskStates[taskId] = .paused } taskStore?.updateStatus(taskId: taskId, status: "paused") @@ -113,6 +114,7 @@ extension NativeWorkmanagerPlugin: UNUserNotificationCenterDelegate { stateQueue.async(flags: .barrier) { self.activeTasks[taskId]?.cancel() self.activeTasks.removeValue(forKey: taskId) + self.activeTaskGenerations.removeValue(forKey: taskId) // see its doc comment self.taskStates[taskId] = .cancelled self.taskNotifTitles.removeValue(forKey: taskId) self.taskAllowPause.removeValue(forKey: taskId) diff --git a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift index fdc4114..9508596 100644 --- a/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift +++ b/ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin.swift @@ -83,6 +83,25 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { var activeTasks: [String: Task] = [:] var workers: [String: IosWorker] = [:] + /// taskId -> a fresh id minted each time `handleEnqueue`'s direct + /// (one-time) path stores a new entry in `activeTasks`. `activeTasks` + /// itself is never cleared when a direct task finishes NATURALLY + /// (success, failure, or timeout) — only explicit cancel paths + /// (handleCancel/cancelAll/cancelByTag/notification-cancel) ever call + /// `removeValue`. Before existingPolicy was implemented that was a + /// harmless leak (a `Task` value is cheap and nothing read the dict as a + /// liveness signal). It is not harmless now: `existingPolicy` reads + /// `activeTasks[taskId] != nil` to decide whether a taskId is "still + /// running". Confirmed on a simulator (2026-09-23 lib/ audit follow-up): + /// re-enqueuing a taskId whose task had already completed, with + /// `existingPolicy: .keep`, was silently dropped forever — `.keep` saw + /// the stale entry and concluded something was still running. This id + /// lets the completing Task's own cleanup remove its `activeTasks` entry + /// exactly once it is truly done, while guarding against a replacing + /// execution's entry being wiped out from under it (same "clear only if + /// still current" pattern as `DartTaskCancellationRegistry.endExecution`). + var activeTaskGenerations: [String: UUID] = [:] + @available(iOS 13.0, *) var chainStateManager: ChainStateManager { ChainStateManager.shared } @@ -403,8 +422,25 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { self.workers[taskId]?.stop() } + // 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. + 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. + 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) } @@ -418,6 +454,7 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { ) } self.activeTasks[taskId] = task + self.activeTaskGenerations[taskId] = generationId } if skippedForKeep { NativeLogger.d("handleEnqueue: '\(taskId)' already running, existingPolicy=keep — new request ignored") @@ -445,6 +482,7 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { task.cancel() } activeTasks.removeAll() + activeTaskGenerations.removeAll() // see its doc comment } NativeLogger.w("⚠️ OS Expiration: Stopped all active workers") } @@ -497,6 +535,7 @@ public class NativeWorkmanagerPlugin: NSObject, FlutterPlugin { stateQueue.async(flags: .barrier) { self.activeTasks[taskId]?.cancel() self.activeTasks.removeValue(forKey: taskId) + self.activeTaskGenerations.removeValue(forKey: taskId) // see its doc comment self.taskStates[taskId] = .cancelled self.workers[taskId]?.stop() } From 17ea2d99d5df5c5174033413a535588a3f8958df 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: Wed, 23 Sep 2026 20:42:23 +0700 Subject: [PATCH 6/8] =?UTF-8?q?docs:=20CHANGELOG=20=E2=80=94=20record=20th?= =?UTF-8?q?e=20natural-completion=20generation-id=20follow-up=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a95d70e..3c28f62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,19 @@ and immutable). Android changes were needed (Android's `existingPolicy` handling and `DartTaskCancellationRegistry` were already correct — that's what issue #72 fixed). + - **Follow-up found in second-pass review, before this ever shipped**: the + `existingPolicy` fix above read `activeTasks[taskId]` as its "is this + still running" signal, but that dictionary was never cleared when a + direct one-time task finished *naturally* (only explicit cancel ever + removed an entry) — a leftover from before anything read it as a + liveness signal. Confirmed on a simulator: re-enqueuing a `taskId` whose + task had already completed, with `existingPolicy: .keep`, was silently + dropped forever, because the stale entry made `.keep` think something + was still running. Fixed with a per-enqueue generation id that lets a + task's own completion clear its entry — but only if nothing has replaced + it in the meantime, the same guard pattern used by + `DartTaskCancellationRegistry`. Verified fixed on the same simulator, and + the full `Cancellation` device-test group (8 tests) still passes. ## [1.8.2] - 2026-09-23 From c1c5501e46bbfe87a6b2068a3f2d1ab29aea26ba 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 00:57:51 +0700 Subject: [PATCH 7/8] =?UTF-8?q?docs+test:=20second=20audit=20pass=20?= =?UTF-8?q?=E2=80=94=20correct=20an=20overclaim,=20add=20missing=20coverag?= =?UTF-8?q?e,=20note=20a=20real=20unfixed=20gap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requested by the user after CI first went green ("sửa nhiều quá sao tôi yên tâm release dc") — audited the diff again, this time specifically hunting for claims I made without empirically checking them, and for the same class of bug the previous commit found (state read as a new kind of signal without auditing every place that state changes). Findings from this pass: 1. CORRECTED AN OVERCLAIM. The CHANGELOG said promoting chain-step/graph-node DartWorkers to isHeavyTask=true on iOS reduces their OS-kill risk "since a DartWorker needs 1-2s just to boot its Flutter Engine." Grepped every `isHeavyTask` read in ios/ (2 sites): both only affect `periodic` and `windowed` triggers, both scheduled via BGTaskScheduler. Chain steps and graph nodes have no TaskTrigger of their own and always execute inline via a plain `Task {}` — never through BGTaskScheduler. The promotion is stored correctly but read by nothing today. Corrected the CHANGELOG to say so plainly instead of claiming a behavior change that doesn't happen. 2. VERIFIED THE subDir TRAVERSAL CLAIM EMPIRICALLY, not just re-asserted it. Ran actual Swift (`swift -e`-equivalent script) against `URL.appendingPathComponent`: confirmed ".." sequences survive literally in the resulting path string (not resolved away by URL itself), which is exactly what makes the traversal exploitable once the OS resolves the path at file-creation time — and confirmed a leading "/" does NOT make it absolute-replace the base (concatenates safely). This is why the subDir fix in the first commit is real and the leading-slash case isn't a separate gap. 3. ADDED MISSING TEST COVERAGE for a claim the CHANGELOG already made but nothing verified: that an unregistered DartWorker in a TaskGraph node or RemoteTriggerRule mapping now fails as a clean, catchable StateError rather than a crash or (worse) reaching native anyway. Both new tests pass; both also assert the native call was never made. 4. FOUND A GENUINE BONUS FIX, previously undocumented: chains never checked DartWorker registration before commit ca358c6 (`fix(dart): DartWorker in a TaskGraph node...`) — an unregistered callbackId in a chain used to throw "INTERNAL ERROR: Callback handle not found... Please report this bug", the message meant for a genuinely impossible state, because chains skipped straight to the handle lookup with no registration check first. resolveWorkerForWire gives chains the same clear, actionable message enqueue() always had. Added a test proving this. 5. RE-TRACED THE KNOWN existingPolicy.replace RACE precisely (not just re-asserted the same vague sentence) and rewrote the CHANGELOG note to describe the actual failure mode: an explicit cancel() or another replace landing in the gap between a replacement Task being stored and it minting its own execution id can mark the wrong (stale) execution. Traced through pure back-to-back replaces with no intervening cancel and confirmed that case alone does not corrupt state. Still not closed — would require threading a pre-minted id through 7+ call sites of executeDartWorkerViaMethodChannel (direct enqueue, chains, TaskGraph, BGTaskScheduler-resumed tasks, offline queue), more than this PR's blast radius should grow to without its own device-verification pass. 6. FOUND, NOT FIXED, OUT OF SCOPE: BGTaskScheduler's periodic-task path (onTaskRunning writes activeTasks[taskId], mirroring what handleEnqueue's direct path used to do) does not participate in commit d0a3042's generation-id scheme, and BGTaskSchedulerManager's onTaskComplete never clears its activeTasks entry either. A taskId reused between a periodic schedule and a later direct one-time enqueue() could hit the same stale-liveness-signal bug d0a3042 fixed for the direct-only case. Not fixed here: verifying it needs a real BGTask periodic firing and completion on a physical device, which this session cannot do (simulator does not reliably exercise BGTaskScheduler's actual background scheduling). Reusing one taskId across a periodic schedule and a one-time enqueue is also an unusual pattern bordering on API misuse — flagging for a dedicated follow-up with real device time, not folding into this PR. 7. FOUND, NOT FIXED, PRE-EXISTING (confirmed via git show at this branch's base commit, predates every commit in this PR): BGTaskScheduler's onExpiration handler (stopAllWorkers()) cancels every Task in activeTasks but never calls DartTaskCancellationRegistry.markCancelled for any of them — a DartWorker callback polling isTaskCancelled() during a BGTask expiration would never see it. Not this PR's regression; noting it since it surfaced during this pass. No source (Kotlin/Swift/Dart production code) changed in this commit — docs and tests only. Full suite still green: flutter analyze 0 issues, flutter test 2056/2056. --- CHANGELOG.md | 54 +++++++++++++---- ...dit_2_dartworker_wire_resolution_test.dart | 59 +++++++++++++++++++ 2 files changed, 101 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c28f62..bfe4c4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,17 +37,31 @@ and immutable). reachable on both platforms before fixing (neither native worker factory special-cases `DartCallbackWorker`; both require `callbackHandle` and fail cleanly without it). - - **Behavior change:** a chain-step `DartWorker` is now promoted to - `isHeavyTask: true` on iOS, matching plain `enqueue()`. This was silently - skipped before — chain-step DartWorkers ran on the short - `BGAppRefreshTask` budget (~30s) instead of `BGProcessingTask`'s, far more - likely to be killed mid-execution since a DartWorker needs 1-2s just to - boot its Flutter Engine. + - A chain-step or `TaskGraph`-node `DartWorker` now gets the same + `isHeavyTask: true` promotion plain `enqueue()` applies on iOS, instead of + silently skipping it. **Correction, checked after first writing this + entry:** this is not currently an observable behavior change. `isHeavyTask` + only ever affects scheduling on iOS for the `periodic` and `windowed` + trigger types (both routed through `BGTaskScheduler`); chain steps and + graph nodes have no `TaskTrigger` of their own and always execute inline + via a plain `Task {}`, never through `BGTaskScheduler`, so the promoted + value is stored correctly but not read anywhere today. Kept for + consistency with `enqueue()`'s existing behavior and so a future fix to + chain/graph iOS scheduling doesn't need its own audit of this — not + claimed as a fix for OS-kill risk on either mechanism today. - **Behavior change:** an unregistered `DartWorker` (`callbackId` not passed to `initialize(dartWorkers:)`) used in a `TaskGraph` node or a `RemoteTriggerRule` mapping now throws a `StateError` at the point of `enqueueGraph()`/`registerRemoteTrigger()`, instead of silently reaching native and never running. + - **Bonus fix, found while re-checking this change:** the same unregistered + `callbackId` used in a task **chain** used to throw too, but with the + wrong message — chains never checked registration at all, so they fell + straight into the "should never happen" internal-error branch meant for + a genuinely impossible state, printing `INTERNAL ERROR: Callback handle + not found... Please report this bug.` for what is actually a completely + ordinary mistake. Chains now get the same clear "not registered, here's + how to fix it" message `enqueue()` has always given. - **iOS's foreground `handleEnqueue` never read `existingPolicy` at all** — found while investigating the item above. Every repeat `enqueue()` call for a reused `taskId` silently started a second, fully independent @@ -70,12 +84,28 @@ and immutable). now binds this id into a Zone on the foreground/simulator path too (`method_channel.dart`'s `_executeDartCallback`), matching what the headless isolate's `_callbackDispatcher` already did. - - Known residual limitation: two `existingPolicy: .replace` calls for the - same `taskId` in extremely rapid succession (before the first - replacement's execution has started running) can still race, since the - per-execution id is minted lazily when the replacement execution starts, - not synchronously at enqueue time. Far narrower than the bug this fixes; - not expected to matter in practice. + - Known residual limitation, traced through but deliberately not closed + (closing it means threading a pre-minted execution id through every + caller of `executeDartWorkerViaMethodChannel` — direct enqueue, chains, + `TaskGraph`, `BGTaskScheduler`-resumed tasks, offline queue — more surface + area than this PR's blast radius should grow to without its own device + verification pass): the per-execution id is minted lazily, inside + `executeDartWorkerViaMethodChannel`, once the replacement `Task` actually + starts running — not synchronously when `handleEnqueue` decides to + replace. In the narrow window between a `.replace` swapping + `activeTasks[taskId]` to the new `Task` and that `Task` reaching its + first `beginExecution` call, `DartTaskCancellationRegistry`'s + `currentExecutionId[taskId]` still points at the OLD (already-replaced) + execution. A `cancel(taskId)` — or another `.replace` — landing in that + window marks the wrong (stale) execution id; the new one starts moments + later unaffected by that mark, so it does **not** stop when the caller + thought it just told it to. Pure double/triple-replace with no + intervening `cancel()` was traced through and does not corrupt state — + only an explicit cancel landing in that specific gap does. The window is + on the order of the time from `Task { }` construction to its first + `await` inside `executeDartWorkerViaMethodChannel` — real, but requires a + caller to `enqueue()`-then-immediately-`cancel()`/`enqueue()` again the + same `taskId` back-to-back, not a pattern normal usage hits. - Device-verified on an iOS simulator (both `.replace` and `.keep`); no Android changes were needed (Android's `existingPolicy` handling and `DartTaskCancellationRegistry` were already correct — that's what issue diff --git a/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart b/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart index 5034ae8..befc0ce 100644 --- a/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart +++ b/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart @@ -178,4 +178,63 @@ void main() { 'chain and enqueue() code paths.'); }); }); + + group('Chain-step unregistered DartWorker error message', () { + test('is the same helpful message enqueue() gives, not a misleading ' + '"INTERNAL ERROR" — pre-fix, chains skipped the registration check ' + 'and fell straight into the internal-error branch meant for a truly ' + 'impossible state', () async { + expect( + () => NativeWorkManager.beginWith( + TaskRequest( + id: 'step1', + worker: DartWorker(callbackId: 'never-registered'), + ), + ).enqueue(), + throwsA( + isA().having( + (e) => e.message, + 'message', + contains('not registered'), + ), + ), + ); + }); + }); + + group('Unregistered DartWorker fails loudly instead of silently', () { + test('enqueueGraph() rejects with a StateError, not a crash or a silent ' + 'no-op native call', () async { + final graph = TaskGraph(id: 'g4') + ..add(TaskNode( + id: 'a', + worker: DartWorker(callbackId: 'never-registered'), + )); + + await expectLater( + NativeWorkManager.enqueueGraph(graph), + throwsA(isA()), + ); + // The whole point of failing before the native call: it must never + // have been reached with an unresolvable worker. + expect(mockPlatform.capturedGraphMap, isNull); + }); + + test('registerRemoteTrigger() rejects with a StateError, not a crash or ' + 'a silent no-op native call', () async { + await expectLater( + NativeWorkManager.registerRemoteTrigger( + source: RemoteTriggerSource.fcm, + rule: RemoteTriggerRule( + payloadKey: 'action', + workerMappings: { + 'sync': DartWorker(callbackId: 'never-registered'), + }, + ), + ), + throwsA(isA()), + ); + expect(mockPlatform.capturedRule, isNull); + }); + }); } From aad57596c81c6e450c82ea905a847d6ad57495cb 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 01:02:20 +0700 Subject: [PATCH 8/8] =?UTF-8?q?style:=20dart=20format=20=E2=80=94=20the=20?= =?UTF-8?q?previous=20commit's=20check-only=20run=20never=20actually=20wro?= =?UTF-8?q?te=20the=20fix=20(--output=3Dnone=20doesn't=20mutate;=20I=20mis?= =?UTF-8?q?read=20its=20report=20as=20already-clean)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...ssue_lib_audit_2_dartworker_wire_resolution_test.dart | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart b/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart index befc0ce..3c4b63b 100644 --- a/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart +++ b/test/unit/issue_lib_audit_2_dartworker_wire_resolution_test.dart @@ -180,7 +180,8 @@ void main() { }); group('Chain-step unregistered DartWorker error message', () { - test('is the same helpful message enqueue() gives, not a misleading ' + test( + 'is the same helpful message enqueue() gives, not a misleading ' '"INTERNAL ERROR" — pre-fix, chains skipped the registration check ' 'and fell straight into the internal-error branch meant for a truly ' 'impossible state', () async { @@ -203,7 +204,8 @@ void main() { }); group('Unregistered DartWorker fails loudly instead of silently', () { - test('enqueueGraph() rejects with a StateError, not a crash or a silent ' + test( + 'enqueueGraph() rejects with a StateError, not a crash or a silent ' 'no-op native call', () async { final graph = TaskGraph(id: 'g4') ..add(TaskNode( @@ -220,7 +222,8 @@ void main() { expect(mockPlatform.capturedGraphMap, isNull); }); - test('registerRemoteTrigger() rejects with a StateError, not a crash or ' + test( + 'registerRemoteTrigger() rejects with a StateError, not a crash or ' 'a silent no-op native call', () async { await expectLater( NativeWorkManager.registerRemoteTrigger(