Repository navigation
fix(background-uploader): stop sending upload events to Flutter engines that no longer exist - #9381
Conversation
…es that no longer exist The Darwin plugin never called registrar.publish(instance), and Flutter delivers detachFromEngine(for:) only to a plugin that published itself (FlutterPlugin.h), so BackgroundUploadCoordinator.detach had never run. The coordinator is process-wide and outlives every engine, so it kept each torn-down engine's method channel and fanned every upload event to it for the rest of the process - a "Communicating on a dead channel" warning per progress tick. Same defect and fix as divine_video_player under #9342. detach only drops the channel captured at init from the coordinator's list. It neither resolves registrar.messenger(), which reads nil inside -[FlutterEngine dealloc], nor sends on the channel, which would hit the destroyed shell, so it is safe where the engine delivers it. A RunnerTests XCTest registers the plugin with a fake registrar and pins both that register publishes the instance serving the channel and that detachFromEngine never reaches the engine.
This comment has been minimized.
This comment has been minimized.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Reviewed 02243f9a5eb57b4c9d02301ddbfa654af1e9aa28 (2 files, +154/-0).
The one-line fix is right. FlutterPlugin.h:231-243 says outright that detachFromEngineForRegistrar: is @optional and "You will only receive this method if you registered your plugin instance with the FlutterEngine via -[FlutterPluginRegistry publish:]". Without registrar.publish(instance) the hook below it was dead code, exactly as the description says.
Submitting this as a comment rather than an approval for two reasons: the two items under Blockers need your call, and nothing on this PR compiles the changed Swift (see Validation at the bottom).
Blockers
1. dealloc is one step later than the window that actually crashes
From FlutterEngine.mm:
destroyContext(573-583) runs_shell.reset()and leaves the engine object alive.resetChannelson the line above it nils only the engine's own framework channels (589-604) — a plugin channel is untouched.sendOnChannel:message:binaryReply:(1329-1350) guards withNSAssert(_shell && _shell->IsSetup(), ...), which is compiled out in release, then unconditionally runs_shell->GetPlatformView()->DispatchPlatformMessage(...)on line 1347.- The plugin detach block is the first thing in
dealloc(350-360).
So between destroyContext and dealloc the channel is still fully wired and a send dereferences a null shell. detachFromEngine fires only after that window has closed. What it does remove is the Communicating on a dead channel. warning, which FlutterBinaryMessengerRelay.mm:22-37 emits only once parent is nil — i.e. genuinely post-teardown, and harmless. That matches the symptom in the description, but it means the crash-bearing half of "engines that no longer exist" is still open: a progress tick landing in that window behaves exactly as before.
Closing it needs a signal at destroyContext/FlutterEngineWillDealloc time, or channel storage keyed weakly / by messenger — not a dealloc-time list removal. Happy to be told that's deliberate scope and the warning was the whole target, but the doc comment currently reads as though the crash case is covered.
2. detach now mutates a main-queue-confined array from dealloc
channels is documented at BackgroundUploaderPlugin.swift:31-32 as "Mutated and read only on the main queue," and every other access honours it — sendEvent wraps its for channel in self.channels in DispatchQueue.main.async, and enqueue / cancel / activeTaskIds / urlSessionDidFinishEvents all hop to main.
detachFromEngine is the only entry point with no hop, and it now runs on whatever thread drops the engine's last strong reference. A removeAll(where:) racing the main-queue for loop is an unsynchronised Array read/write. Before this PR the path was dead, so the invariant held; activating it is what introduces the exposure. Worth either hopping to main or stating why the release thread is guaranteed to be the platform thread for every engine in the app (including the notification-isolate one).
Correctness follow-ups
3. The method-call handler is never cleared, so the plugin/channel cycle survives. addMethodCallDelegate installs a handler that strongly captures instance, and instance holds channel. Android's onDetachedFromEngine (BackgroundUploaderPlugin.kt:30-32) does both channel.setMethodCallHandler(null) and instances.remove(this); the Darwin side does only the list removal, so a plugin + channel + relay is stranded per engine teardown, each one still live in the app-wide lifecycle delegate array. Note the remedy needs care rather than a one-liner: during the detach block _binaryMessenger.parent has not been nil'd yet (that happens further down dealloc, after the plugin enumeration), so a cleanup call there still reaches the engine.
4. activeForegroundSessionIds is left behind. beginForegroundSession inserts an id that only endForegroundSession removes; the beginBackgroundTask expiration handler touches backgroundTasks, not the id set. If the engine dies mid-publish — the scenario this PR is about — Dart never sends the end, finishBackgroundEventsIfReady's guard activeForegroundSessionIds.isEmpty fails for the rest of the process, and every later background wake falls through to the 25-second watchdog instead of completing promptly. detachFromEngine is the one signal that this engine's Dart can never call endForegroundSession.
5. register(with:) is not idempotent for one engine. publish: assigns into pluginPublications[pluginKey], so a second registration on the same engine replaces the publication while init has already appended a second channel — the first instance's channel then never gets detached, which is the leak being fixed. DivineVideoPlayerPlugin.register treats this as real (it opens with PlayerRegistry.shared.disposeForEngine(messenger) and the hot-restart comment). Keying channels by ObjectIdentifier(messenger) would make re-registration replace rather than accumulate.
6. Async FlutterResult replies still land on the dying channel. enqueue / cancel / activeTaskIds hand their result block into session.getAllTasks { ... }, which is asynchronous. A call issued just before teardown resolves afterwards and replies through the same messenger, hitting the same null-shell window as item 1. Clearing channels does not drop those captured blocks.
7. On macOS the publish call is inert. FlutterPluginMacOS.h in the pinned 3.47.2 declares no detachFromEngineForRegistrar: at all (zero occurrences), so publish: there retains the instance but no hook is ever delivered — while macos/Flutter/GeneratedPluginRegistrant.swift does register this plugin. Nothing in the code records that the guarantee is iOS-only, so detachFromEngine's "Runs inside -[FlutterEngine dealloc]" reads as a cross-platform promise it isn't.
Test and CI coverage
8. Both tests pass with an empty detachFromEngine body. testRegisterPublishesTheInstanceServingTheChannel never calls detach, and testDetachFromEngineNeverReachesTheEngine asserts only messengerResolutions == 1 and sentChannels == [] — both hold for a no-op. Nothing asserts the channel left BackgroundUploadCoordinator.shared, and nothing can: the coordinator is private at file scope, so even @testable import cannot see it. Making it (or an attachedChannelCount) internal and asserting 1 → 0 across the detach would pin the actual behaviour.
9. No job builds or runs RunnerTests. RunnerTests appears nowhere in .github/workflows/, codemagic.yaml, mobile/scripts/ or mobile/mise.toml. Codemagic's ios-build / ios-simulator-build are events: [], and e2e-smoke-ios builds the Runner target only. So the regression guard this PR adds has never executed, and a future change that drops registrar.publish reintroduces the bug with every check green. This is the one item I'd most like a plan for, even if it lands separately.
10. The ObjC selector Flutter actually dispatches is never exercised. dealloc calls the hook only behind respondsToSelector:@selector(detachFromEngineForRegistrar:); both tests invoke plugin.detachFromEngine(for:) through Swift static dispatch, which works whether or not the member reaches the ObjC runtime. One line covers it: XCTAssertTrue(plugin.responds(to: NSSelectorFromString("detachFromEngineForRegistrar:"))).
11. setUp opens the app's real background URLSession inside the test host. register → init → attach → _ = session builds URLSessionConfiguration.background(withIdentifier: "co.openvine.background_uploader.session") — the same identifier the shipping app uses, in an app-hosted test bundle, so nsurlsessiond can re-attach tasks from an earlier real run and fan a delegate event into whichever fake messenger is current. Nothing calls invalidateAndCancel, so it outlives the suite.
Smaller items
12. The doc comment above detachFromEngine contradicts itself and the engine: it says registrar.messenger() "resolves through a weak engine reference that already reads nil during dealloc" and that a send "dereferences the destroyed shell." Neither half is right at that point — dealloc nils _binaryMessenger.parent and registrar.flutterEngine only after the plugin enumeration, so during the hook both are still live and a send would take the real path. The conclusion (don't touch the engine here) is correct; the stated reason isn't, and it's copied into RunnerTests.swift twice, including as an assertion message.
13. tearDown's (registrar.published as? BackgroundUploaderPlugin)?.detachFromEngine(...) silently skips cleanup when the cast fails — including in the exact regression case the suite exists to catch — leaving the channel in the process-wide coordinator. tearDownWithError() + XCTUnwrap makes the skip loud.
14. FakePluginRegistrar.valuePublished(byPlugin:) ignores its pluginKey and returns whatever was published last; the real one is a keyed lookup returning nil for an unregistered key.
15. FakeTextureRegistry, applicationDelegates and nextConnection are written but never asserted on — roughly a third of the fake, in a file nothing compiles. applicationDelegates in particular is a real part of the registration contract this suite claims to pin.
Validation
Read-only review. Claims above about engine behaviour were checked against the pinned Flutter 3.47.2 sources on disk — FlutterPlugin.h, FlutterPluginMacOS.h, FlutterEngine.mm (dealloc, destroyContext, resetChannels, sendOnChannel:message:binaryReply:) and FlutterBinaryMessengerRelay.mm — and the plugin claims against this commit's own source.
Not done, and not doable from this PR: compiling or running the changed Swift. All 30 checks here are Dart/web, per item 9, so neither the plugin change nor the new tests have been built anywhere. That gap is why this is a comment rather than an approval.
…pin what detach does Review follow-ups on #9381. detachFromEngine now ends the foreground sessions the dead engine's Dart opened: that isolate can never send endForegroundSession, and one open session keeps finishBackgroundEventsIfReady from releasing every later background-URLSession wake until the 25 s watchdog. The coordinator records the owning channel per session for that. The hook hops to the main queue when dealloc runs elsewhere, since the coordinator's state is main-queue-confined; attach and detach now assert that with dispatchPrecondition. A "diagnostics" method call exposes the attached-channel and open-session counts, so the XCTests assert the 1 -> 0 deltas across detach instead of only what detach must not do; teardown resolves the plugin through the method-call delegate so it still cleans up when publishing regresses. The doc comments now say the hook is iOS-only, that it does not cover the destroyContext-to-dealloc window, and give the accurate reason nothing may go through the registrar: weak references to a deallocating object read nil.
This comment has been minimized.
This comment has been minimized.
|
@realmeylisdev thanks — every claim checked against the pinned engine sources ( Fixed
Declined, with evidence
Escalated
|
…S code Nothing in CI compiled or ran mobile/ios/RunnerTests: every Codemagic iOS workflow is manual and Mobile CI is Dart-only, so a regression in any of the Swift those tests pin — plugin registration and engine teardown, the camera's media-session scoping, the Nostr bridge attestation, the ProofMode vendor gate — shipped with every check green (#9381 review, item 9). Mobile CI gains an "iOS Native Tests" job on macos-15 with Xcode 26 (the image defaults to 16; divine_camera needs the iOS 26 SDK). It generates the iOS project with `flutter build ios --config-only`, repairs the Swift package floor the way Codemagic does, picks the newest available iPhone simulator by runtime, and runs `xcodebuild test -only-testing:RunnerTests`, uploading the xcresult on failure. Building the whole Runner target also means a pod or Swift-package resolution break fails on the PR instead of on Codemagic after merge. The job follows a new `ios_native` scope — mobile/ios, any package's ios/ or darwin/ directory, and mobile/pubspec.lock — rather than `app` or `ios`, because the XCTests never run Dart and a macOS minute costs ten Linux minutes. The Mobile CI gate requires it when that scope is set and accepts a skip otherwise. The timing budget is provisional until the first run is measured.
FakePluginRegistrar.valuePublished(byPlugin:) returned whatever was published last, whatever key it was asked for. The real -[FlutterEngineBaseRegistrar valuePublishedByPlugin:] is a lookup in the engine-wide pluginPublications dictionary, so it answers nil for a key that published nothing. Nothing calls it today, since teardown resolves the plugin through the method-call delegate instead, so this is a latent trap rather than a live bug: the first test to assert a publication through it would pass against behaviour Flutter does not provide. Key it to the registrar's own plugin key, and pin that with an assertion, since no CI job compiles this file. Verified on an iPhone 17e simulator: the suite is 5 of 5 green, and reverting the lookup to the single-slot form fails the new assertion.
Mobile PR PreviewPreview refreshed for Last refresh:
|
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Re-review of 23afd08472ded37e7294198c1dfb0f5c62d245f1.
Every finding from my review of 02243f9a has a disposition below. Eight are verified fixed, two I am withdrawing as wrong, four are open and yours, one I cannot touch.
I pushed one commit to this branch; it is described under What I pushed below.
Verified fixed
| Finding | Evidence |
|---|---|
| Tests passed with an empty detach body | testDetachFromEngineReleasesThisEnginesChannel asserts attached - 1, which a no-op body fails. The diagnostics counter is a better seam than the internal coordinator I suggested — it works through the public method-call path, which an app-hosted XCTest can use without @testable reaching into a package module. |
| Foreground sessions stranded | detach now closes the dead engine's sessions via foregroundSessionOwners, with testDetachFromEngineClosesTheSessionsItsDartCanNoLongerEnd covering it. Iterating keys.sorted() while endForegroundSession mutates the dictionary is safe because sorted() materialises an array first. |
channels mutated off the main queue |
The Thread.isMainThread hop plus dispatchPrecondition on attach/detach, with testDetachFromEngineOffTheMainThreadStillReleasesTheChannel. The ordering argument in that test's comment is right: the fulfil is enqueued after the hop detach enqueued, so FIFO on the main queue observes the detach first. |
| Doc comment contradicted itself | Replaced, and the replacement is correct — FlutterEngineBaseRegistrar.flutterEngine is @property(nonatomic, weak) (FlutterEngine.mm:96), so ARC reads it nil once dealloc has begun and registrar.messenger() hands Swift a nil. That is a better reason than the one I gave: I had argued from the explicit registrar.flutterEngine = nil further down dealloc, which happens after the plugin enumeration and so would not have been the mechanism. |
| macOS inertness undocumented | Both register and the hook now say iOS-only. |
| ObjC selector never exercised | plugin.responds(to: NSSelectorFromString("detachFromEngineForRegistrar:")). |
tearDown skipped detach silently |
tearDownWithError + XCTUnwrap, resolved through the method-call delegate so it still cleans up if publishing regresses. |
| Unasserted fake scaffolding | applicationDelegates is now asserted. See the withdrawal below for the rest of that finding. |
Withdrawn — my findings were wrong
The plugin/channel retain cycle does not exist. I claimed addMethodCallDelegate creates a cycle that detachFromEngine should break with setMethodCallHandler(nil), citing Android's onDetachedFromEngine as parity. FlutterMethodChannel's ivars are _messenger, _name, _codec, _connection and _taskQueue (FlutterChannels.mm:241-247) — the channel stores no handler. setMethodCallHandler: hands the wrapped block to [_messenger setMessageHandlerOnChannel:...], so the block lives in the engine's handler table and the ownership is engine → block → plugin → channel, a chain the engine's own dealloc releases. There is nothing to break, and adding the call would only emit a "Communicating on a dead channel" warning on every teardown. No change is owed here, and the Android difference is a platform difference rather than a gap.
Most of the "unused fake scaffolding" finding. textures() is a FlutterPluginRegistrar requirement, so FakeTextureRegistry has to exist, and nextConnection is what setMessageHandlerOnChannel must return. Only the applicationDelegates half was real, and it is fixed.
Open, and yours
1. The destroyContext-to-dealloc window. The new doc comment names this as uncovered, which is the right call for this PR's scope — I am recording it as an accepted limitation, not asking for it here. For whoever picks it up: destroyContext (FlutterEngine.mm:573-583) resets the shell with the engine alive, resetChannels above it nils only the engine's own framework channels, and sendOnChannel:message:binaryReply: dereferences _shell unconditionally at line 1347 once the NSAssert compiles out. Worth its own issue if it is not already tracked under #9342.
2. register(with:) is still not idempotent for one engine. A second registration on the same engine appends a second channel while publish: replaces the publication, so only the last instance receives the hook and the first channel stays in channels for the process lifetime. DivineVideoPlayerPlugin.register treats this as real with its disposeForEngine(messenger) pre-clean. Keying channels by ObjectIdentifier(messenger) would fix it, but that reshapes the coordinator's storage, so it is your call rather than something I should push.
3. Async FlutterResult replies still outlive the engine. enqueue, cancel and activeTaskIds hand their result block into session.getAllTasks { ... } (lines 136, 148, 157), which resolves after teardown and replies through the same messenger. detachFromEngine clears channels but nothing drops those captured blocks.
4. The tests open the app's real background URLSession. attach's _ = session builds URLSessionConfiguration.background(withIdentifier: "co.openvine.background_uploader.session") — the shipping identifier — inside an app-hosted test bundle, and nothing calls invalidateAndCancel. nsurlsessiond can re-attach tasks from an earlier real run on the same simulator and fan a delegate event into a fake messenger. Fixing it needs an injectable identifier, which is new surface, so flagging rather than pushing.
Open, and not mine to fix
No CI job builds or runs RunnerTests. Still true at this head: RunnerTests appears in no workflow, mobile/scripts/ entry or mise task, Codemagic's ios-build and ios-simulator-build are events: [], and e2e-smoke-ios builds the Runner target only. Wiring it up is a GitHub Actions change, which PR_REVIEW_TEAMS.md reserves for @divinevideo/platform, so I have left it alone. It is the highest-leverage follow-up on this PR: without it, the four tests added here have no mechanical guard, and the next Flutter bump that changes a FlutterPluginRegistrar requirement breaks FakePluginRegistrar with every check green.
Two things I checked because the follow-up newly introduces them
Both are fine; recording the evidence so nobody has to re-derive it.
dispatchPrecondition(.onQueue(.main))inattachis a hard trap in release builds, so an off-main registration would crash rather than race. Both of this app's registration paths are main-queue:AppDelegate.didInitializeImplicitFlutterEngineruns at launch, and the notification-action isolate registers insidedispatch_async(dispatch_get_main_queue(), ...)influtter_local_notifications21.0.0 (FlutterEngineManager.m:64-75), which is the callbackAppDelegateinstalls viasetPluginRegistrantCallback.Thread.isMainThreadguarding a.onQueue(.main)precondition looked like a mismatch — main thread inside asynconto another queue satisfies the first and, I assumed, not the second. It satisfies both: libdispatch treats execution on the main thread as being on the main queue, verified by running the case directly.
What I pushed
One commit, 23afd084, on top of 2877358e: test(background-uploader): key the fake registrar's publication lookup.
FakePluginRegistrar.valuePublished(byPlugin:) returned published for every key it was asked for. The real -[FlutterEngineBaseRegistrar valuePublishedByPlugin:] is a lookup in the engine-wide pluginPublications dictionary, so it answers nil for a key that published nothing. Nothing calls the fake's copy today, since teardown resolves the plugin through the method-call delegate, so this is a latent trap rather than a live bug — but the first test to assert a publication through it would pass against behaviour Flutter does not provide. It is now keyed to the registrar's own plugin key, with an assertion pinning that.
@hm21 please review that commit, and revert it if you would rather it went another way — nothing else on the branch was touched. The approval below covers 23afd084, my own commit included; @NotThatKindOfDrLiz's review request is still open and now points at that head, so a second pair of eyes on it is already queued and I have not duplicated the request.
Validation
No CI job builds this file, so I ran it: xcodebuild test -only-testing:RunnerTests/BackgroundUploaderEngineTeardownTests on an iPhone 17e simulator, Xcode 27.0, Flutter 3.47.2.
- At
23afd084:** TEST SUCCEEDED **, 5 of 5 —testRegisterPublishesTheInstanceServingTheChannel,testDetachFromEngineReleasesThisEnginesChannel,testDetachFromEngineClosesTheSessionsItsDartCanNoLongerEnd,testDetachFromEngineOffTheMainThreadStillReleasesTheChannel,testDetachFromEngineNeverGoesThroughTheRegistrar. The four tests this PR adds do pass on a real simulator, which nothing on the PR itself establishes. - Mutation check on my own commit: reverting the lookup to the single-slot form gives
** TEST FAILED **ontestRegisterPublishesTheInstanceServingTheChannel, so the new assertion fails for the reason it claims.
I did not mutation-check your four tests individually; I read them and ran them.
GitHub checks on 23afd084: 28 pass, 1 skipping (Publish daily service-test result, which skips on pull requests). None of them compile the Swift in this PR — that is the gap above, not a clean bill.
Engine claims above were read from the pinned Flutter 3.47.2 sources on disk — FlutterEngine.mm, FlutterChannels.mm, FlutterPlugin.h, FlutterPluginMacOS.h — and from flutter_local_notifications 21.0.0's iOS sources.
…S code Nothing in CI compiled or ran mobile/ios/RunnerTests: every Codemagic iOS workflow is manual and Mobile CI is Dart-only, so a regression in any of the Swift those tests pin — plugin registration and engine teardown, the camera's media-session scoping, the Nostr bridge attestation, the ProofMode vendor gate — shipped with every check green (#9381 review, item 9). Mobile CI gains an "iOS Native Tests" job on macos-15 with Xcode 26 (the image defaults to 16; divine_camera needs the iOS 26 SDK). It generates the iOS project with `flutter build ios --config-only`, repairs the Swift package floor the way Codemagic does, picks the newest available iPhone simulator by runtime, and runs `xcodebuild test -only-testing:RunnerTests`, uploading the xcresult on failure. Building the whole Runner target also means a pod or Swift-package resolution break fails on the PR instead of on Codemagic after merge. The job follows a new `ios_native` scope — mobile/ios, any package's ios/ or darwin/ directory, and mobile/pubspec.lock — rather than `app` or `ios`, because the XCTests never run Dart and a macOS minute costs ten Linux minutes. The Mobile CI gate requires it when that scope is set and accepts a skip otherwise. The timing budget is provisional until the first run is measured.
`NostrBridgeAttestationPlugin` held its `FlutterPluginRegistry` in a `private let`, and on iOS that registry *is* the `FlutterEngine` — `FlutterImplicitEngineBridgeImpl.pluginRegistry` returns `_engine`. The plugin itself lives in a process-lifetime `shared` static, so that one reference pinned the engine for the life of the process. `-[FlutterEngine dealloc]` is the only non-test place Flutter dispatches `detachFromEngineForRegistrar:`, so the pin silently disabled `detachFromEngine(for:)` for every plugin in the app — including `BackgroundUploaderPlugin`, whose hook #9381 had just made reachable. The consequence is worse than a stale-channel warning. The messenger relay's `parent` is weak and nils only at dealloc, so a pinned engine keeps forwarding, while the view controller's dealloc has already run `destroyContext` and dropped the shell (the implicit engine's `allowHeadlessExecution` is NO). The send then reaches `sendOnChannel:`, whose shell assert is present in the release engine binary, raising `NSInternalInconsistencyException: Sending a message before the FlutterEngine has been run` — the signature #4400 tracked. Weak lets the engine dealloc, so the hook fires and the residual destroyContext-to-dealloc window degrades to the benign dead-channel log instead of a crash. Both call sites already treat a missing WKWebView as recoverable, so they take that path when the registry is gone. Closes #9393
`NostrBridgeAttestationPlugin` held its `FlutterPluginRegistry` in a `private let`, and on iOS that registry *is* the `FlutterEngine` — `FlutterImplicitEngineBridgeImpl.pluginRegistry` returns `_engine`. The plugin itself lives in a process-lifetime `shared` static, so that one reference pinned the engine for the life of the process. `-[FlutterEngine dealloc]` is the only non-test place Flutter dispatches `detachFromEngineForRegistrar:`, so the pin silently disabled `detachFromEngine(for:)` for every plugin in the app — including `BackgroundUploaderPlugin`, whose hook #9381 had just made reachable. The consequence is worse than a stale-channel warning. The messenger relay's `parent` is weak and nils only at dealloc, so a pinned engine keeps forwarding, while the view controller's dealloc has already run `destroyContext` and dropped the shell (the implicit engine's `allowHeadlessExecution` is NO). The send then reaches `sendOnChannel:`, whose shell assert is present in the release engine binary, raising `NSInternalInconsistencyException: Sending a message before the FlutterEngine has been run` — the signature #4400 tracked. Weak lets the engine dealloc, so the hook fires and the residual destroyContext-to-dealloc window degrades to the benign dead-channel log instead of a crash. Both call sites already treat a missing WKWebView as recoverable, so they take that path when the registry is gone. Closes #9393
…S code (divinevideo#9388) * ci(mobile): run the native iOS XCTests on pull requests that touch iOS code Nothing in CI compiled or ran mobile/ios/RunnerTests: every Codemagic iOS workflow is manual and Mobile CI is Dart-only, so a regression in any of the Swift those tests pin — plugin registration and engine teardown, the camera's media-session scoping, the Nostr bridge attestation, the ProofMode vendor gate — shipped with every check green (divinevideo#9381 review, item 9). Mobile CI gains an "iOS Native Tests" job on macos-15 with Xcode 26 (the image defaults to 16; divine_camera needs the iOS 26 SDK). It generates the iOS project with `flutter build ios --config-only`, repairs the Swift package floor the way Codemagic does, picks the newest available iPhone simulator by runtime, and runs `xcodebuild test -only-testing:RunnerTests`, uploading the xcresult on failure. Building the whole Runner target also means a pod or Swift-package resolution break fails on the PR instead of on Codemagic after merge. The job follows a new `ios_native` scope — mobile/ios, any package's ios/ or darwin/ directory, and mobile/pubspec.lock — rather than `app` or `ios`, because the XCTests never run Dart and a macOS minute costs ten Linux minutes. The Mobile CI gate requires it when that scope is set and accepts a skip otherwise. The timing budget is provisional until the first run is measured. * ci(mobile): gate the iOS job on the pubspecs and scripts it uses The ios_native scope used mobile/pubspec.lock as its proxy for plugin registration, but a pub workspace member is never written to that lockfile (no 'source: path' entries, no package names). Editing a plugin's own pubspec, or adding an iOS plugin to mobile/pubspec.yaml, rewrites GeneratedPluginRegistrant.swift and the generated Swift package with the gate skipped and green. ensure_ios_swift_package_floor.rb and pre_build_ios.sh both execute inside the job, the latter as the Runner scheme's BuildAction pre-action, so a change to either could not re-verify the job that runs it. Same rule as check_gradle_wrapper_checksum.sh in the native arm (divinevideo#7201). * ci(mobile): keep package example apps out of the iOS scope A shell case glob's `*` spans `/`, so mobile/packages/*/ios/* matched mobile/packages/divine_camera/example/ios/Runner/Info.plist and scheduled a 23-minute macOS job for a file the Runner target never compiles. * test(ci): assert ios_native in the exhaustive scope table allScopes drove the "turns on exactly its own scopes" loop but never listed ios_native, so the new scope was unasserted across all twelve rows, and "a mobile_ci.yaml change runs every scope" omitted it despite its name. The containsAll at the top of the file only checks that the key is present. Widening the ios_native case to mobile/lib/* would have left the file green. That is the failure mode the comment above allScopes was written for, after a non-exhaustive predecessor let three detector blocks be deleted. Adds rows for a plugin pubspec, a script the job runs, and a package example app; all three fail against the previous single-arm case. * ci(mobile): skip the iOS job on a goldens regeneration dispatch workflow_dispatch hits the detector's fall_open path, so ios_native is true and this job ran for 23 minutes — about 230 Linux-equivalent minutes — after which the Mobile CI gate exits 1 unconditionally because UPDATE_GOLDENS is true. AGENTS.md documents that dispatch as the only sanctioned way to regenerate golden references, so every regeneration paid for a verdict that was thrown away. * ci(mobile): correct the Swift package floor comment, drop --debug The comment said Flutter skips raising the deployment target when its `xcodebuild -showBuildSettings` probe exceeds 60 s. It does not: an empty build-settings result makes ios/mac.dart print "No Xcode build settings have been found" and return XcodeBuildResult(success: false), which fails the command this step already checks. updateMinimumDeployment is reached under --config-only, and run 35596239558 logged "already targets iOS 16.0" — so Flutter had done it and the repair was a no-op. The path that really is silent is the replaceFirst of Flutter's default platform string, which no-ops on a manifest that no longer carries it. The repair stays as insurance; only its stated reason was wrong. build_ios.dart sets defaultBuildMode to debug whenever the environment is the simulator and rejects a non-debug simulator build, so --debug only added something to keep in sync with the scheme's Debug configuration. * ci(mobile): stop a beta simulator runtime from aborting the jq lookup A runtime key such as SimRuntime.iOS-18-0-beta1 makes map(tonumber) fail the whole program with "Invalid numeric literal ... while parsing 'beta1'", and jq exits 5. Because the result is a plain command substitution under bash -e, the shell exits right there, so the `if [ -z "$udid" ]` branch that exists to print ::error::No available iPhone simulator and dump the device list never runs — the step just fails with nothing useful in the log. tonumber? // 0 keeps a malformed segment sortable instead of fatal, and still selects the newest runtime when a beta and a release runtime are both present. * ci(mobile): keep the simulator diagnostic from failing its own step `xcrun simctl list devices available | grep "$udid"` was the last command of the step, and GitHub runs `run:` blocks under bash -e, so grep's status was the step's. A simctl listing that stops echoing the UDID verbatim would block the PR even though the udid had already been written to $GITHUB_OUTPUT on the line above. * ci(mobile): let xcodebuild own the step status, reuse the SwiftPM clone Under `set -o pipefail` the pipeline's status came from grep, so the step had two wrong outcomes: xcodebuild succeeding while printing no matching line failed a required gate, and xcodebuild failing the same way reported grep's 1 instead of its own 65. The step deliberately floats to the newest Xcode 26 on the image while hard-coding one output shape, so a point release that moves `xcodebuild test` to the Swift Testing summary would have reddened every iOS PR with an essentially empty log. Capturing PIPESTATUS[0] fixes both, and "Testing failed:" and "The following build commands failed:" now survive the filter. Flutter always passes -clonedSourcePackagesDirPath and --config-only has already prefetched the whole graph (2.1 GB locally), so without the flag Xcode re-resolved and re-cloned it into DerivedData inside the measured build step. * docs(agents): fix the local iOS XCTest recipe and scope list The recipe used a bare `flutter build ios --config-only`, which configures a release-for-device build because build_ios.dart derives defaultBuildMode from the environment type, while the next command targets a Simulator destination. It also omitted the Swift package floor repair the CI job performs. The scope list named only mobile/ios, a package's ios/ or darwin/ directory and pubspec.lock, which is narrower than what actually schedules the job. * docs(rules): list what actually schedules the iOS Native Tests job The checklist named three triggers. The job also runs on a pubspec that decides plugin registration, on the workflow and detector themselves, and on every fail-open path — so a reviewer watching checks on a large refactor would see an unexplained 23-minute macOS job. * ci(mobile): survive a dropped sqlite3 native-asset download in the iOS job package:sqlite3's build hook fetches libsqlite3mc.arm64.ios_sim.dylib from GitHub Releases in one unretried GET, inside the Xcode build phase, so a truncated transfer fails the whole 15-minute build with a hash mismatch — run 35612736422 died exactly that way on a rebase that changed nothing under mobile/. The Linux shards already carry the fix for the host dylib (divinevideo#7197): keep the hook's shared download directory in the Actions cache. This job now restores that cache on the macOS key before the build and saves it after a green run outside the merge queue, which cannot save. As a second line, xcodebuild is retried once, and only when the log shows the Flutter build phase failed building native assets: the hook re-hashes the file it left behind and fetches again, DerivedData keeps the second build incremental, and a test failure or any other build error is never retried. --------- Co-authored-by: Meylis <meylis@divine.video>
…inevideo#9395) * fix(ios): stop the attestation plugin pinning the Flutter engine `NostrBridgeAttestationPlugin` held its `FlutterPluginRegistry` in a `private let`, and on iOS that registry *is* the `FlutterEngine` — `FlutterImplicitEngineBridgeImpl.pluginRegistry` returns `_engine`. The plugin itself lives in a process-lifetime `shared` static, so that one reference pinned the engine for the life of the process. `-[FlutterEngine dealloc]` is the only non-test place Flutter dispatches `detachFromEngineForRegistrar:`, so the pin silently disabled `detachFromEngine(for:)` for every plugin in the app — including `BackgroundUploaderPlugin`, whose hook divinevideo#9381 had just made reachable. The consequence is worse than a stale-channel warning. The messenger relay's `parent` is weak and nils only at dealloc, so a pinned engine keeps forwarding, while the view controller's dealloc has already run `destroyContext` and dropped the shell (the implicit engine's `allowHeadlessExecution` is NO). The send then reaches `sendOnChannel:`, whose shell assert is present in the release engine binary, raising `NSInternalInconsistencyException: Sending a message before the FlutterEngine has been run` — the signature divinevideo#4400 tracked. Weak lets the engine dealloc, so the hook fires and the residual destroyContext-to-dealloc window degrades to the benign dead-channel log instead of a crash. Both call sites already treat a missing WKWebView as recoverable, so they take that path when the registry is gone. Closes divinevideo#9393 * docs(ios): drop the engine-pin example this change makes false --------- Co-authored-by: Daniel Cadenas <dcadenas@gmail.com>
2 participants
Description
On iOS the background-upload plugin never found out when its Flutter engine went away. The upload coordinator is one process-wide object that outlives every engine: each engine's registration hands it a method channel, and the only thing that takes that channel back is Flutter's
detachFromEnginehook — which Flutter delivers solely to plugins that registered themselves viaregistrar.publish(documented inFlutterPlugin.h;-[FlutterEngine dealloc]iteratespluginPublications). This plugin never published itself, so the hook had never run. After every engine teardown (a scene reconnect under the UIScene lifecycle, for instance) the coordinator kept the dead engine's channel and fanned every upload progress tick and terminal event to it for the rest of the process, each one loggingCommunicating on a dead channel.The fix is the one-line
registrar.publish(instance)— the same defect and the same fix asdivine_video_playerunder #9342.detachFromEngineruns inside-[FlutterEngine dealloc], whereregistrar.messenger()resolves through a weak engine reference that already reads nil and any send on the channel would hit the destroyed shell. The existingdetachonly removes the channel captured at init from the coordinator's list, touching neither, so it is safe exactly where the engine delivers it. No user-visible behaviour change on a live engine.What this deliberately does not change: the window between
destroyContext(scene disconnect / app termination — shell gone, engine object still alive) and dealloc. An upload completing inside it would still tripsendOnChannel:'s shell assert (Sending a message before the FlutterEngine has been run). Crashlytics shows zero uploader events in that crash family over the last 90 days — the family is real for the video player and audio plugins and is being handled under #9342 — so this PR stays on delivering the hook. macOS is unaffected in practice: itsFlutterPluginprotocol has nodetachFromEngineForRegistrar:in Flutter 3.47.2, and the publish call is harmless there.Related Issue: Closes #none (relates to #9342)
Out of Scope
destroyContextwindow above). Zero observed events; same shape as the fix(video-player): iOS crashes delivering a video frame after the Flutter engine is torn down #9342 fix if it ever shows up.DivineCameraPlugin.logSink(3 crashes / 3 users in 90 days) — different package, separate PR.FakePluginRegistrar/FakeBinaryMessengerinRunnerTests.swiftuse the same names as the fakes the fix(video-player): iOS crashes delivering a video frame after the Flutter engine is torn down #9342 branch adds, and sit at the same spot in the file, so whichever lands second gets a textual conflict to reconcile into one shared fake rather than two same-namedprivateclasses.Verification
cd mobile/ios && xcodebuild test -workspace Runner.xcworkspace -scheme Runner -destination 'platform=iOS Simulator,name=iPhone 17 Pro' -only-testing:RunnerTests— 29/29 passed (27 existing + 2 new inBackgroundUploaderEngineTeardownTests).registrar.publishfails both; makingdetachFromEnginere-resolveregistrar.messenger()(the pre-fix(video-player): iOS crashes delivering a video frame after the Flutter engine is torn down #9342 video-player shape) fails only the detach test.a804b261645ef8c13eb3d5c44a5c2fb0340c5539(FlutterEngine.mmdealloc/destroyContext/sendOnChannel:,FlutterBinaryMessengerRelay.mm) and the shippedFlutterPlugin.h/FlutterPluginMacOS.hheaders.enqueue→ progress and terminalonUploadEventover thebackground_uploaderchannel → publish); no uploader errors and noCommunicating on a dead channelline in the console.Type of Change