diff --git a/CHANGELOG.md b/CHANGELOG.md index 14bbb23..7f2ad5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## Unreleased -- Documentation fixes only; no API or behavior changes. +- Overlapping `checkAndInstall` calls now report their own reason + ("A check is already running") in `CodePush.status` instead of surfacing the + other check's state. - `CodePushOverlay.bannerBuilder`: return a widget to show no banner (`const SizedBox.shrink()`) — it never accepted `null`. Keep side effects out of the builder; wire the handed `onRestart`/`onDismiss` to your UI instead. diff --git a/README.md b/README.md index 0debc28..406707b 100644 --- a/README.md +++ b/README.md @@ -245,7 +245,18 @@ CodePush.status.addListener(() { ``` Values include: `init`, `Checking server...`, `Downloading patch...`, -`Patch active`, `No update (204)`, `Restart to apply`, etc. +`Patch active`, `No update (204)`, `Restart to apply`, +`A check is already running`, etc. + +One ordering guarantee holds across those transitions: when `checkAndInstall` +returns `false`, the reason is that check's **final** `status` write, so +reading the value immediately after the `await` reliably explains the result. +A concurrent (losing) call stamps `A check is already running` without +disturbing the active check's terminal message. The guarantee is about which +transition a check ends on — the value still does not stay put afterwards. +The one exception is the iOS post-download load path, where the guard is +released before the load completes: its writes are best-effort ordering, +and a concurrent full check may overwrite them. Don't use this as an app-facing "a patch loaded" signal. `CodePushOverlay` latches the `Patch active` edge internally, but on that transition it re-keys diff --git a/example/lib/main.dart b/example/lib/main.dart index d291a6a..31c02cd 100644 --- a/example/lib/main.dart +++ b/example/lib/main.dart @@ -58,6 +58,11 @@ class _CodePushDemoState extends State { Future _loadStatus() async { final patched = await CodePush.isPatched; final patch = await CodePush.currentPatch; + // Guard every setState that follows an await: the overlay re-keys the + // app subtree when a patch activates (see the build-method note below), + // which disposes this State mid-await — a late setState would then + // throw in debug builds. + if (!mounted) return; setState(() { _isPatched = patched; _currentPatch = patch; @@ -72,9 +77,15 @@ class _CodePushDemoState extends State { appId: 'your-app-id', releaseVersion: '1.0.0+1', onUpdateReady: () { + // checkAndInstall invokes this synchronously, before the await + // below resumes — but the overlay can re-key this subtree while + // the check is in flight, so the callback can land on a + // disposed State. Hence the mounted guard. + if (!mounted) return; setState(() => _status = 'Patch installed! Restart to apply.'); }, ); + if (!mounted) return; if (!installed) { // `false` is not just "no update" — checkAndInstall also returns false // for "a check is already running", a download failure, a hash @@ -86,6 +97,7 @@ class _CodePushDemoState extends State { } await _loadStatus(); } on CodePushException catch (e) { + if (!mounted) return; setState(() => _status = 'Error: ${e.message}'); } } @@ -94,15 +106,18 @@ class _CodePushDemoState extends State { setState(() => _status = 'Rolling back...'); try { await CodePush.rollback(); + if (!mounted) return; setState(() => _status = 'Rolled back. Restart to revert.'); await _loadStatus(); } on CodePushException catch (e) { + if (!mounted) return; setState(() => _status = 'Rollback failed: ${e.message}'); } catch (e) { // The iOS Dart-side rollback deletes the resident patch file, which can // throw a FileSystemException (not a CodePushException) if it is already // gone or unreadable — catch it so the button never leaves an exception // unhandled. + if (!mounted) return; setState(() => _status = 'Rollback failed: $e'); } } diff --git a/lib/src/code_push.dart b/lib/src/code_push.dart index 91db55b..9d879ce 100644 --- a/lib/src/code_push.dart +++ b/lib/src/code_push.dart @@ -163,6 +163,12 @@ abstract final class CodePush { /// `'Patch already installed'` at the next check, and again on every resume), /// so it is a fleeting TRANSITION, not a level you can poll. /// + /// One ordering guarantee holds across those transitions: when + /// [checkAndInstall] returns `false`, its reason was that check's final + /// write, so reading the value right after the `await` is reliable — the + /// guarantee is about WHICH transition a check ends on, not about the + /// value staying put afterwards. + /// /// Do NOT use it as an app-facing "a patch loaded" signal. [CodePushOverlay] /// latches the `'Patch active'` edge internally, but on that same transition /// it re-keys its child subtree — disposing any latch a widget under it holds @@ -442,6 +448,16 @@ abstract final class CodePush { /// Checks the server for updates, downloads and installs if available. /// /// Returns `true` if a patch was installed (restart needed). + /// + /// Status contract: every `false` return leaves its reason as the FINAL + /// [status] write of that check, so reading [status] immediately after the + /// `await` reliably explains the result (a later check will overwrite it). + /// A concurrent call loses the single-flight guard: it stamps + /// `'A check is already running'` and returns `false` without disturbing + /// the active check's terminal message — except on the iOS post-download + /// load path, where the guard is released before the load completes, so + /// its writes are best-effort ordering rather than a guarantee (a + /// concurrent full check may overwrite them). static Future checkAndInstall({ required String serverUrl, required String appId, @@ -456,7 +472,18 @@ abstract final class CodePush { // is deliberately global (not per appId/channel): apps use a single // config, and a losing caller's `false` means "another check is // already running", not "no update". - if (_checkInFlight) return false; + // + // Write [status] before this early return too — checkAndInstall's + // contract is that every `false` return leaves its reason in [status], + // and without this write a losing caller would surface the OTHER + // check's in-flight state as if it were its own result. The in-flight + // check keeps overwriting [status] as it progresses, so this write is + // transient and cannot mask a 'Patch active' edge (notifications are + // synchronous, so that edge has already been delivered). + if (_checkInFlight) { + status.value = 'A check is already running'; + return false; + } _checkInFlight = true; try { print('[CP] checkAndInstall start'); @@ -507,7 +534,6 @@ abstract final class CodePush { // any installed patch, so the fleet returns to the store baseline // within one check interval of the switch being flipped. if (data['ota_disabled'] == true) { - status.value = 'OTA disabled by server'; try { // Unconditional: the rollback knows where each platform keeps // the patch (engine on Android/desktop, file removal on iOS @@ -520,7 +546,12 @@ abstract final class CodePush { status.value = 'OTA disabled by server — patch removed (restart to apply)'; } catch (_) { - // Best-effort: a clean device has nothing to revert. + // Best-effort: a clean device has nothing to revert. The + // status is written AFTER the awaited rollback attempt (in + // both branches): a concurrent check() during that await + // stamps 'A check is already running', and the kill-switch + // signal must be what stands when we return. + status.value = 'OTA disabled by server'; } return false; } @@ -607,7 +638,6 @@ abstract final class CodePush { ); if (actualEngineFingerprint == null) { - status.value = 'Incompatible baseline: engine has no code push support'; await _reportIncompatibleBaseline( serverUrl: serverUrl, appId: appId, @@ -617,6 +647,12 @@ abstract final class CodePush { expectedFingerprint: expectedEngineFingerprint, actualFingerprint: null, ); + // Terminal status is written AFTER the awaited report: a + // concurrent check() during that POST stamps 'A check is + // already running', and this actionable message must be what + // stands when we return. It's an error state, not progress, so + // writing it last loses nothing. + status.value = 'Incompatible baseline: engine has no code push support'; return false; } @@ -627,8 +663,6 @@ abstract final class CodePush { if (expectedEngineFingerprint != null && actualEngineFingerprint != 'unknown' && expectedEngineFingerprint != actualEngineFingerprint) { - status.value = 'Incompatible baseline: engine ABI mismatch ' - '($actualEngineFingerprint vs $expectedEngineFingerprint)'; await _reportIncompatibleBaseline( serverUrl: serverUrl, appId: appId, @@ -637,6 +671,11 @@ abstract final class CodePush { expectedFingerprint: expectedEngineFingerprint, actualFingerprint: actualEngineFingerprint, ); + // After the await, so a concurrent check()'s guard write can't + // overwrite this terminal status (see the null-fingerprint + // branch above). + status.value = 'Incompatible baseline: engine ABI mismatch ' + '($actualEngineFingerprint vs $expectedEngineFingerprint)'; return false; } @@ -1906,15 +1945,22 @@ abstract final class CodePush { 'bytes=${container.length}', ); } - status.value = 'Patch format is unexpected — rolling back. ' - 'Upgrade flutter_compile to the latest version and ' - 'rebuild the patch.'; await _iosImmediateRollback( serverUrl: serverUrl, appId: appId, patchId: patchId, errorMessage: 'Patch format mismatch — rejected before load', ); + // Written after the awaited rollback (file deletes + telemetry + // POST) so this upgrade hint is the last write this method + // makes. The single-flight guard is not held here (the install + // path returns this future without awaiting it, and cold-boot / + // reload callers never take the guard), so a concurrent full + // check can still overwrite it later — best-effort ordering, + // not a guarantee. + status.value = 'Patch format is unexpected — rolling back. ' + 'Upgrade flutter_compile to the latest version and ' + 'rebuild the patch.'; return false; } @@ -1993,7 +2039,6 @@ abstract final class CodePush { ); } print('[CP] MODULE LOAD THREW ($origin) — $e'); - status.value = 'Module error: $e — rolling back patch'; // Real load failure. Delete immediately instead of waiting for // the three-strike auto-rollback. await _iosImmediateRollback( @@ -2002,6 +2047,11 @@ abstract final class CodePush { patchId: patchId, errorMessage: 'Patch load threw $e — deleted immediately', ); + // Terminal status goes after the awaited rollback so it is the + // last write this method makes. The single-flight guard is not + // held on this path, so a concurrent full check can still + // overwrite it later — best-effort ordering, not a guarantee. + status.value = 'Module error: $e — rolling back patch'; return false; } } diff --git a/test/hardening_test.dart b/test/hardening_test.dart index 20e2b68..b699e0f 100644 --- a/test/hardening_test.dart +++ b/test/hardening_test.dart @@ -23,6 +23,8 @@ void main() { Duration offerDelay = Duration.zero; String? offeredHash; String? offeredUrlOverride; + String? offeredEngineFingerprint; + Future Function()? onTelemetry; void serve() { server.listen((HttpRequest req) async { @@ -36,9 +38,14 @@ void main() { 'patch_available': true, 'patch_id': 'p1', if (offeredHash != null) 'patch_hash': offeredHash, + if (offeredEngineFingerprint != null) + 'engine_fingerprint': offeredEngineFingerprint, 'patch_url': offeredUrlOverride ?? 'http://127.0.0.1:${server.port}/patch', })); + } else if (req.uri.path.endsWith('/telemetry/client-error')) { + if (onTelemetry != null) await onTelemetry!(); + req.response.statusCode = HttpStatus.ok; } else { downloads++; req.response.statusCode = HttpStatus.ok; @@ -55,6 +62,8 @@ void main() { offerDelay = Duration.zero; offeredHash = null; offeredUrlOverride = null; + offeredEngineFingerprint = null; + onTelemetry = null; CodePush.debugResetBaselineHashCache(); TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger .setMockMethodCallHandler(engineChannel, (MethodCall call) async { @@ -169,6 +178,65 @@ void main() { expect(updateChecks, 1, reason: 'losers return before the network'); expect(downloads, 1); }); + + test('a losing overlapped check writes its own reason to status', + () async { + // Contract: checkAndInstall leaves its reason in CodePush.status + // before EVERY false return — including the single-flight early + // return, which previously left the other check's in-flight state + // in place (so a caller surfacing status after `false` rendered a + // foreign operation's progress as its own result). + offeredHash = patchHash; + offerDelay = const Duration(milliseconds: 300); + serve(); + + // Both calls in the same synchronous turn: the winner latches the + // guard before its first await, and the loser's status write also + // lands before ITS first await — so no wall-clock sequencing is + // needed (a delay could even over-sleep past the offer window and + // turn the "loser" into a real second check). + final winner = check(); + final loser = check(); + expect(CodePush.status.value, 'A check is already running'); + expect(await loser, isFalse); + + await winner; + }); + + test( + 'incompatible-baseline terminal status survives a concurrent check ' + 'during the telemetry POST', () async { + // Regression guard for the reordered terminal writes: the engine + // ABI-mismatch branch reports telemetry (awaited) BEFORE writing + // its terminal status. A concurrent check() arriving inside that + // await stamps 'A check is already running'; the terminal write + // must land AFTER the await so the actionable message is what + // stands when the winner returns. Moving that write back up next + // to its branch condition turns this test red. + offeredHash = patchHash; + offeredEngineFingerprint = 'flutter-9.9.9'; // mocked engine: 'abi' + Future? loser; + String? statusDuringReport; + onTelemetry = () async { + // The winner is parked on the awaited telemetry POST with the + // single-flight guard held — exactly the reordered-write window. + loser ??= check(); + statusDuringReport = CodePush.status.value; + }; + serve(); + + expect(await check(), isFalse); + expect(loser, isNotNull, reason: 'the telemetry hook must have fired'); + expect(await loser, isFalse); + expect(statusDuringReport, 'A check is already running', + reason: 'the loser must have stamped its status inside the ' + 'window, or this test is not exercising the race'); + expect( + CodePush.status.value, + startsWith('Incompatible baseline: engine ABI mismatch'), + ); + expect(downloads, 0, reason: 'mismatch is refused before download'); + }); }); group('download size cap', () { diff --git a/test/ota_controls_test.dart b/test/ota_controls_test.dart index 50bff4f..0c94f3e 100644 --- a/test/ota_controls_test.dart +++ b/test/ota_controls_test.dart @@ -78,6 +78,43 @@ void main() { expect(CodePush.status.value, isNot(contains('patch removed'))); }); + test( + 'clean device: kill-switch status survives a concurrent check ' + 'during the revert attempt', () async { + serveOtaDisabled(); + Future? loser; + String? statusDuringRevert; + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(engineChannel, (MethodCall call) async { + if (call.method == 'CodePush.rollback') { + // The winner is parked on the revert await with the + // single-flight guard held — exactly the window where a + // concurrent check stamps its loser status. The kill-switch + // branch must rewrite AFTER this await so the fleet-wide + // signal is what stands when the winner returns. + loser ??= check(); + // Captured (not expect()ed) here: a TestFailure thrown inside + // this handler is swallowed by the kill-switch catch around + // the rollback await; the body assertion below fails loudly. + statusDuringRevert = CodePush.status.value; + return false; // Unpatched device: the revert attempt no-ops. + } + return null; + }); + + final installed = await check(); + + expect(installed, isFalse); + expect(loser, isNotNull); + expect(statusDuringRevert, contains('already running'), + reason: 'the loser must actually have stamped its status inside ' + 'the revert window — otherwise this test is vacuous'); + expect(await loser, isFalse); + expect(CodePush.status.value, contains('OTA disabled')); + expect(CodePush.status.value, isNot(contains('patch removed'))); + expect(CodePush.status.value, isNot(contains('already running'))); + }); + test('patched device: reverts to the store baseline', () async { serveOtaDisabled(); final engineCalls = [];