fix(nfc): stop leaking sessions that wedge polling with a 408 - #2556
Open
davidcreated wants to merge 2 commits into
Open
fix(nfc): stop leaking sessions that wedge polling with a 408#2556davidcreated wants to merge 2 commits into
davidcreated wants to merge 2 commits into
Conversation
Closes SatoshiPortal#2552. flutter_nfc_kit keeps its session state in process-global fields (pollingTimeoutTask, tagTechnology, ndefTechnology) and its finish handler is the only code that cancels the poll timer, closes the tag and disables Android reader mode — pollTag never disables reader mode on a successful poll. Two paths polled without finishing: NfcScannerWidget on both its success and error paths, and BroadcastSignedTxCubit.onNfcScanned which read NDEF records and never finished. Reader mode therefore stayed registered and the next poll waited for a tag-discovery event that cannot fire for a card already held, so it failed with 408 Polling tag timeout and kept failing until the app restarted. Three more failure modes fell out of the same root cause: a finish() from a disposing widget could land after a new poll started and cancel its timer and reader mode, leaving a spinner that never resolved and never errored; the static entry points had no re-entrancy guard, so a double tap opened overlapping sessions; and an abandoned poll's platform reply was never delivered, leaving a dangling Dart future with no timeout. NFC now goes through one locator-registered NfcSession singleton that serializes every operation behind a Lock, runs finish() in a finally on every path, and guards cancel() with a generation counter so a late cancel cannot touch a later operation. Each step has a Dart-side watchdog, because the platform timer is exactly what a stale finish cancels. FlutterNfcKit is now named in exactly one file. Errors become a sealed NfcFailure family mapped from platform codes (408/406/405/404/503/500/409/400) with iOS message fallbacks, translated in a presentation extension. The nfcError key that printed raw PlatformException text into a snackbar is deleted so it cannot come back. A timeout now renders in place with a working Try Again button; the old retry button only appeared after a success, so it was unreachable after a failure. ScanNfcPage previously passed no onError at all and swallowed every poll failure. The PushTx path stops reading NDEF records in the cubit and parsing them with a regex over record.toString(); pushTxUriFromNdefRecords handles it as a pure function. AbsoluteUriRecord is deliberately not supported — ndef 0.4.0 has uri and decodedType delegating to each other, so touching either recurses until the stack overflows. Poll timeout is explicit at 15s instead of the plugin's 20s default, and the scan UI is now the same on both platforms so an iOS timeout has a reachable retry instead of an after-the-fact snackbar. Covered by unit tests that need no hardware: finish() is asserted on every failure injection point, the overlap/timeout/self-heal sequence is driven with fake_async, and the iOS tag-lost retry, failure mapping, translation exhaustiveness, parser and cubit paths each have their own. (cherry picked from commit 13d60a21cdd4412826a238fcf365557c4ebfadaa)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2552.
Root cause
flutter_nfc_kitkeeps its session state in process-global fields —pollingTimeoutTask,tagTechnology,ndefTechnologyall live on the plugin'scompanion object.pollTagarms a 20 s timer that disables reader mode and returnserror("408", "Polling tag timeout"), registers Android reader mode, and never disables reader mode on a successful poll. The"finish"handler is the only code that cancels the timer, closes the tag and disables reader mode.Two paths polled without ever finishing:
NfcScannerWidget._scan— nofinish()on the error path, and none on the success path either (onlydispose()did it).BroadcastSignedTxCubit.onNfcScanned— calledreadNDEFRecords()and never finished at all.So after an aborted or PushTx scan, reader mode stayed registered and the tag stayed connected. The next
pollre-registered a callback waiting for a tag-discovery event, which cannot fire for a card that never left the field — 20 s later,408, and every subsequent attempt failed the same way until the app restarted. That is the reported "works a few times, then always fails".Three more failure modes fall out of the same root cause:
"finish"cancelspollingTimeoutTaskunconditionally, so afinish()fired by a disposing widget (unawaited(_finishNfcSession())) could land after a new poll started and kill that poll's timer and its reader mode. That one does not even 408 — the platform reply is never delivered, so the spinner runs forever.MethodChannelreply is never delivered, leaving a dangling Dart future with no timeout on our side.On iOS the same missing
finish()is worse:pollreturns406whensession != nil, and the plugin'stagReaderSession(_:didInvalidateWithError:)returns early onguard result != nilbefore clearingsession. One missedfinish()wedges every later scan at406permanently, until the app is force-quit. That is an upstream plugin bug; this PR cannot fix it, only make missing afinish()structurally hard.What changed
One owner for the NFC session. New
lib/core/nfc/:data/nfc_kit_datasource.dartFlutterNfcKit. Also the seam the tests inject.data/nfc_session_impl.dartLock, runsfinish()in afinallyon every path, guardscancel()with a generation counter, watchdogs each step.data/nfc_error_mapper.dartPlatformException→ typed failure, code-first with iOS message fallbacks.domain/nfc_failure.dartNfcFailurefamily (AGENTS.md rule #11), Flutter-free.domain/nfc_session.dartResult<T, NfcFailure>.presentation/nfc_failure_l10n.darttoTranslated(BuildContext); returnsnullfor a cancelled session so "show nothing on cancel" is enforced by the type.nfc_locator.dartNfcSessionas a lazy singleton — oneLockper process, to match the plugin's process-global state. AregisterFactoryhere would silently restore the bug.finish()cancels — the platform 408 cannot be relied on. Poll 20 s on Android / 75 s on iOS (above the OS's own 60 s invalidation), transfer 30 s,finish3 s so a hungfinishdegrades to a logged warning instead of holding the lock. Exactly one value leaves the lock body, so there is no double-completion path.elapsethem exactly.nfcErroris deleted. It wascontext.loc.nfcError(error.toString()), i.e.NFC error: PlatformException(408, Polling tag timeout, null, null)in a snackbar. Removing the key makes that leak un-reintroducible; raw detail now goes tolog.warningonly, andlogMessagenever reaches the UI.nfcTimeouttext, working "Try Again". The old retry button only rendered when_tag != null, i.e. only after a success, so it was unreachable after a failure.ScanNfcPagepassed noonErrorat all and silently swallowed every poll failure; it now renders the typed failure.NfcScanFlowdriver plus a presentationalNfcScanView;NfcScannerWidgetis deleted. A widget owningpoll+finishwhile the bottom sheet also ownedpoll+finishover one process-global state was the structural cause of the divergence.NfcBottomSheet.showReadNfc/showWriteNfckeep their exact signatures, so the three call sites are untouched.Platform.isIOSfrom the UI layer.onNfcScanned(NFCTag)→onPushTxPayload(String), dropping therecords.first.toString()+RegExp('uri=([^ ]+)')hack and the direct datasource call from a cubit (AGENTS.md rule Sync in Isolate #2).pushTxUriFromNdefRecordshandles it as a pure, tested function.New l10n keys:
nfcTimeout,nfcBusy,nfcUnsupportedTag,nfcScanInstructions(en only; other locales follow the usualchore(l10n)pass). Added viatools/arb.dart, not by hand.Testing
65 new/extended tests, none needing a Coldcard:
finish()is always called — table-driven over every failure injection point (poll 408, poll 409, tag removed mid-read, unparseable payload, write error, NDEF-unsupported tag), each asserting the mapped failure andverify(finish).called(1). Plus the negative: NFC off / no NFC hardware →verifyNever(poll)andverifyNever(finish), because no session was opened.fake_async, a second scan during an in-flight poll returnsNfcBusyFailurewithpollcalled exactly once (overlapping polls are unreachable, so the stale-timer cascade cannot happen), the first then times out via the watchdog, and a following scan succeeds. The reported symptom asserted in reverse: the session self-heals instead of staying wedged.finishcannot deadlock the next scan;cancel()ends an in-flight scan and closes once, is a no-op when idle, and cannot touch a later operation.logMessage), parser, and the cubit's PushTx validation paths.Verified on device.
All CI gates run locally on this branch, on top of current
develop:make analyze(--fatal-warnings --fatal-infos)make bull-ui-checkmake fix-checkmake format-checkmake unit-testBelt-and-braces greps:
FlutterNfcKitappears in exactly one file, andnfcErrorappears nowhere inlib/orlocalization/.Notes for reviewers
AbsoluteUriRecordis deliberately unsupported inpushTxUriFromNdefRecords. Inndef0.4.0,AbsoluteUriRecord.urireturnsdecodedTypeanddecodedTypereturnsuri, so touching either recurses until the stack overflows. The Coldcard PushTx tag is a well-known URI record, and the previous regex-over-toString()approach would have hit the same recursion, so this is parity, not a regression.readIso15693: true,readIso18092: false, 14443A/B left at the plugin defaults). Narrowing to ISO-15693-only would want hardware QA on both Mk4 and Q; not worth bundling that risk here.flutter_nfc_kitbugs — nodisableReaderModeafter a successful poll, and the iOS invalidation-ordering guard — are worked around, not fixed. Worth filing upstream.transferWatchdog = 30 sis an estimate for large-PSBT writes over ISO-15693. It is a public constant precisely so it is easy to tune if a big PSBT ever trips it.log.warning-only: one line per operation with elapsed ms and mapped failure class, one when a watchdog trips ("platform never replied" — the smoking gun for the dangling-future mode), one whenfinishfails orcancelis skipped by the generation guard. No payload, PSBT, tx, NDEF bytes or tag id is ever logged, since logs are user-shareable.