Clear deferred-medium backlog: single-flight status write + example mounted guards

[fonkamloic]

Clears the two open deferred-medium issues on this repo. Each was a Medium review finding deferred at merge time under the deferred-medium rule and is now being fixed.

Fixes #32 — checkAndInstall's single-flight early return now writes its reason to status

checkAndInstall's documented contract is that every false return leaves its reason in CodePush.status. The single-flight early return (a second check arriving while one is in flight) was the one false return that wrote nothing, so a losing caller that surfaced status after false rendered the in-flight check's progress state as if it were its own result. It now writes 'A check is already running' before returning, which also makes the example's "writes status before every false return" comment accurate (all 18 false-return sites verified).

The write is safe against the 'Patch active' transition: notifications are synchronous, so that edge is always delivered before any overwrite, and the in-flight check keeps overwriting status as it progresses. The loser's stamp can transiently replace the winner's in-progress message until the winner's next write — that is the accepted tradeoff of the contract fix; the four terminal error statuses that were followed by a best-effort await are now written after that await so the actionable message is the last write those paths make. A regression test is added in the single-flight group.

Fixes #33 — example guards every setState-after-await with a mounted check

The overlay re-keys the app subtree when a patch activates, which disposes the demo page's State mid-await; a setState landing after that throws in debug builds. Every setState that follows an await in _loadStatus, _manualCheck (including the async onUpdateReady callback and the catch branch), and _rollback (both catch branches) now returns early when the State is unmounted. Pre-await synchronous setState calls need no guard and are unchanged; the early returns skip only UI updates on a disposed State — no side effects are lost.

Review rounds

Round 2 corrected the CHANGELOG entry to state only what is guaranteed (the overlapping call reports its own reason, rather than 'never overwrites'), and reworded two code comments that overstated the terminal-status ordering as a guarantee on paths where the single-flight guard is not held.

Checks

• flutter analyze: no new issues (6 pre-existing avoid_print infos in lib/src/code_push.dart; example package clean)
• flutter test: 153/153 pass, including the new regression test (now deterministic — no wall-clock dependency)

[claude]

🔴 Critical

None. No crash path, no new network/file-I/O surface, and no source- or behaviour-breaking change to the public API (status is documented as a free-form transition string, so adding a value is additive).

🟠 Medium

The new loser write makes the "every false return leaves its own reason in status" contract breakable in the other direction — lib/src/code_push.dart:468

Before this PR, nothing outside the in-flight check could write status while _checkInFlight was latched, so a winner's terminal status was safe once written. The loser write removes that guarantee, and there are four sites where the winner writes its terminal status and then awaits before returning false, never rewriting it:

• lib/src/code_push.dart:620-632 — 'Incompatible baseline: engine has no code push support' → await _reportIncompatibleBaseline(...) → return false
• lib/src/code_push.dart:641-654 — 'Incompatible baseline: engine ABI mismatch (...)' → await _reportIncompatibleBaseline(...) → return false
• lib/src/code_push.dart:1920-1928 (_iosLoadPayload) — 'Patch format is unexpected — rolling back. Upgrade flutter_compile...' → await _iosImmediateRollback(...) → return false
• lib/src/code_push.dart:2007-2015 (_iosLoadPayload) — 'Module error: ... — rolling back patch' → await _iosImmediateRollback(...) → return false

Those awaits are not short: _reportIncompatibleBaseline (lib/src/code_push.dart:947) does an HTTP POST with two 5s timeouts, and _iosImmediateRollback (lib/src/code_push.dart:2190) does file deletes, a platform-channel call, and a telemetry POST — and neither rewrites status.

Concretely: an iOS device is offered a patch whose container header doesn't match. _iosLoadPayload writes the actionable 'Patch format is unexpected — upgrade flutter_compile' and starts the rollback + telemetry POST. The user backgrounds/foregrounds the app during that window, _CodePushOverlayState.didChangeAppLifecycleState (lib/src/code_push.dart:2578) calls checkAndInstall, which now stamps 'A check is already running'. The original check then returns false, and the example renders No new patch installed: A check is already running instead of the one message that tells the developer what to do. The PR trades a loser reading a foreign status for a winner reporting a foreign status — on exactly the unhappy paths where the message is most actionable.

The surgical fix is to move the status.value = ... at those four sites to after the awaited best-effort call. They are all error states rather than progress, so nothing is lost by writing them last.

Worth naming the root cause too: "every false return leaves its reason in status" isn't expressible with one process-global ValueNotifier shared by the periodic timer, the resume handler, and manual callers — and status's own doc (lib/src/code_push.dart:160-163) plus the README (README.md:236-238) both say it is a transition, "not a level you can poll". If the reason is meant to be part of checkAndInstall's contract, returning it (a small result object or enum) rather than having callers read a global is the durable answer.

🟡 Low

• test/hardening_test.dart:180-192 — the new regression test depends on wall-clock timing. It relies on Future.delayed(50ms) landing inside the winner's 300ms offerDelay. But _checkInFlight is latched synchronously before the first await in checkAndInstall, so the delay isn't needed at all — and if a loaded CI machine over-sleeps past 300ms, the second check() becomes a real check and lands on 'Patch already installed' instead. Deterministic version, since the new write also happens before the first await:

  final winner = check();
  final loser = check();     // same turn: the guard is already latched
  expect(CodePush.status.value, 'A check is already running');
  expect(await loser, isFalse);
  await winner;

  The sibling test at test/hardening_test.dart:157 already avoids wall-clock sequencing this way with Future.wait.

• example/lib/main.dart:80 — the comment is backwards. onUpdateReady?.call() is invoked synchronously inside checkAndInstall (lib/src/code_push.dart:818, :867), i.e. strictly before the await below resumes, not "long after" it. The guard is still worth having — a manual check in flight when _iosReloadInstalledPatch latches 'Patch active' gets its State re-keyed away — but the stated reason misleads a reader reasoning about ordering. Something like "the overlay can re-key this subtree while this check is in flight, so the callback can land on a disposed State" would be accurate.

• CHANGELOG.md:1-3 — the ## Unreleased section still reads "Documentation fixes only; no API or behavior changes", which this PR makes untrue: status now takes a new value on a path that previously wrote nothing, and consumers who log or branch on status will see it. Worth a line under Unreleased.

🟢 Positives

• The single-flight fix is minimal and correctly placed. The write lands before the first await, so it can't race the guard latch, and ValueNotifier's if (_value == newValue) return suppression can't swallow a subsequent 'Patch active' notification (the loser's string is never equal to it). The ordering argument in the code comment holds up.
• The mounted audit in the example is complete rather than spot-fixed: every setState reachable after an await in _loadStatus, _manualCheck (including the callback and the catch) and _rollback (both catch branches) is guarded, pre-await ones are correctly left alone, and the early returns skip only UI work — no side effect is dropped. Covering both catch branches in _rollback is the kind of thing that usually gets missed.
• New behaviour ships with a regression test rather than a bare assertion, and the test's comment explains the failure it guards against.
• Comments explain why (the re-key mechanism, the 'Patch active' ordering argument) rather than restating the code, consistent with the density of the surrounding file.

_{Static review — flutter test / flutter analyze were not run in this environment, so the PR's 153/153 claim is unverified here.}

[fonkamloic]

Round 1 addressed in 9f546e2:

Medium (fixed). All four cited sites (engine-absent baseline guard, engine ABI mismatch, and the two _iosLoadPayload paths) now write their terminal status after the awaited best-effort call, so a concurrent check's "A check is already running" write during the telemetry POST / rollback window can no longer replace the actionable message. Audited every other status.value write in the file for the same write-await-return-false shape: the remaining sites either return immediately with no intervening await, use unawaited fire-and-forget telemetry, or sit outside the single-flight guard on a path where a concurrent real check repaints status regardless — no additional sites needed the fix.

Lows (all fixed).

• Test is now deterministic: both check() calls run in one synchronous turn (the guard latches and the loser's status write lands before each call's first await), status is asserted immediately, then both futures are awaited. The wall-clock Future.delayed is gone.
• example/lib/main.dart comment corrected: the callback runs synchronously inside checkAndInstall, before the await resumes; the guard exists because the overlay can re-key the subtree while the check is in flight, so the callback can land on a disposed State.
• CHANGELOG Unreleased no longer claims doc-only; it now carries one line noting that a concurrent check no longer overwrites the active check's status message.

Result-object suggestion: acknowledged, deliberately not taken here. status is documented as a transition, and returning a richer result from checkAndInstall is an API design change — worth its own discussion, out of scope for this fix batch.

flutter analyze: clean (6 pre-existing avoid_print infos). flutter test: 153/153.

[claude]

Round 2 — re-reviewed against 9f546e2. The four cited write-then-await-then-return false sites are correctly reordered, the test is now deterministic, and the example comment is accurate. One site of the same class was missed by the audit.

🔴 Critical

None. No native/platform-channel change, no new network or file-I/O surface, no source-breaking change to the public API. The reordered status.value writes at lib/src/code_push.dart:635, :657, :1938, :2030 all sit after best-effort helpers (_reportIncompatibleBaseline :954, _iosImmediateRollback :2205) that are internally try/caught on every step and cannot throw, so moving the write after the await does not introduce a path where the status is skipped entirely.

🟠 Medium

The OTA kill-switch branch is the same write-await-return false shape and was not reordered — lib/src/code_push.dart:521

The Round 1 response says every other status.value write was audited for this shape and that the remainder "either return immediately with no intervening await, use unawaited fire-and-forget telemetry, or sit outside the single-flight guard." :520-537 is none of those — it is inside the guard, has an intervening await, and returns false:

520  if (data['ota_disabled'] == true) {
521    status.value = 'OTA disabled by server';
522    try {
530      await _rollbackInternal(quarantine: false);      // <-- async gap
531      status.value =
532          'OTA disabled by server — patch removed (restart to apply)';
533    } catch (_) {
534      // Best-effort: a clean device has nothing to revert.
535    }
536    return false;
537  }

On the success path :531 rewrites the status after the await, so it is safe. On the throw path the catch at :533 writes nothing, so the only status this branch ever wrote is the pre-await one at :521 — exactly the window the rest of the PR closes.

The throw path is the common, healthy case, not an exotic one. _rollbackInternal (:1720) throws CodePushException('No active patch to roll back.') at :1735 whenever the patch file is absent, and the comment at :526 says so explicitly ("throws harmlessly when the device is already clean"). test/ota_controls_test.dart:62-79 covers precisely this device ("clean device: stops quietly, revert attempt no-ops").

Failure scenario: an unpatched device, server flips the OTA kill switch. The periodic-timer checkAndInstall writes 'OTA disabled by server' and awaits _rollbackInternal, which does a CodePush.rollback platform-channel round trip (:1722) and then _getPatchDir() (:1730) — two real async gaps — before throwing. During that window the user foregrounds the app, _CodePushOverlayState.didChangeAppLifecycleState (:2585) calls checkAndInstall, which now stamps 'A check is already running'. The catch swallows, :536 returns false, and the caller reads A check is already running instead of the kill-switch reason — the fleet-wide signal an operator most needs to see in the debug bar right after flipping the switch.

Surgical fix, same shape as the four already applied — write the terminal status after the await on both outcomes:

if (data['ota_disabled'] == true) {
  try {
    await _rollbackInternal(quarantine: false);
    status.value =
        'OTA disabled by server — patch removed (restart to apply)';
  } catch (_) {
    // Best-effort: a clean device has nothing to revert.
    status.value = 'OTA disabled by server';
  }
  return false;
}

test/ota_controls_test.dart:77-78 (contains('OTA disabled'), isNot(contains('patch removed'))) still passes unchanged.

This also makes the CHANGELOG line accurate: as it stands, "a concurrent checkAndInstall call no longer overwrites the active check's status message" (CHANGELOG.md:2-4) is true at four sites and false at this one.

🟡 Low

• The reordering — the substantive half of this PR — ships with no test (lib/src/code_push.dart:635, :657, :1938, :2030). The one-line loser write got a regression test; the four moves that make it safe are protected only by comments. Moving a status.value back up next to its branch condition is exactly the kind of readability "cleanup" a later refactor makes, and nothing would fail. One test against the cheapest site would cover the class: serve an offer whose engine_fingerprint mismatches, have the telemetry handler fire a second check() before responding, then assert the winner returns false with status.value starting 'Incompatible baseline: engine ABI mismatch'. The existing harness already supports this shape — test/hardening_test.dart:27-47 gates responses on an offerDelay, so a handler-side hook is a small addition.

• The contract this PR enforces is not in the public docs, and the nearest doc contradicts it. "Every false return leaves its reason in status" now lives in an example comment (example/lib/main.dart:88-94) and a test name (test/hardening_test.dart:173), but checkAndInstall's own dartdoc (lib/src/code_push.dart:442-444) is three lines about the return value and says nothing about status, while status's dartdoc (:161-170) and README.md:238-240 both say it is "a fleeting TRANSITION, not a level you can poll" and "don't use this as an app-facing signal". A reader following the public docs would conclude that reading status after false is unsupported. Worth a line on checkAndInstall stating the contract, and adding A check is already running to the values list at README.md:246-247.

🟢 Positives

• All four Round 1 sites are fixed as cited, and each carries a comment explaining why the write sits after the await — the ordering is load-bearing and non-obvious, and without the comment the next reader would move it back. That is the right defense for a constraint no test enforces.
• The determinism fix on the test is the better version of the suggestion rather than a literal transcription: both check() calls in one synchronous turn, status asserted before any await, and a comment (test/hardening_test.dart:181-185) recording why the wall-clock delay was wrong instead of just deleting it.
• The ordering argument in the guard comment (lib/src/code_push.dart:460-467) holds under scrutiny. _onModuleLoaded (:2570-2574) latches on status.value == 'Patch active', and ValueNotifier dispatch is synchronous, so the moduleResult → status write pair at :1992-1993 always delivers the latch before any later loser write can overwrite it. The new write also cannot be swallowed by ValueNotifier's equality suppression, since its string never equals a neighbouring one.
• The mounted audit in the example is complete rather than spot-fixed, and the corrected callback comment (example/lib/main.dart:80-84) now states both the true ordering (synchronous, before the await resumes) and the real reason the guard is still needed (subtree re-key). Getting the reason right matters more than the guard here, since the guard would otherwise read as cargo cult.
• The result-object suggestion was declined with a stated reason and scoped out rather than silently dropped — the right call for an API change in a fix batch.

_{Static review — no Flutter toolchain in this environment, so the 153/153 and flutter analyze claims are unverified here. Line numbers are from the PR merge head.}

[claude]

Round 3 — re-reviewed against the current merge head. The CHANGELOG rewording and the two "best-effort ordering, not a guarantee" comment corrections from the last round landed. The Round 2 Medium did not, and I confirmed it is still present at HEAD; re-raising with the verification below rather than restating it.

🔴 Critical

None. No native or platform-channel change, so Dart↔Android/iOS parity is untouched. No new network or file-I/O surface, no change to download verification or URL scheme gating. No source-breaking change: status is documented free-form (lib/src/code_push.dart:161-170, README.md:237-240), so a new value is additive. The four reordered writes (:635, :657, :1941, :2034) sit after _reportIncompatibleBaseline (:954-996) and _iosImmediateRollback (:2209-2270), both of which try/catch every step and swallow — I read both in full, and neither has a path that escapes, so moving the write after the await cannot skip it. Nothing here can crash or brick a host app.

🟠 Medium

The OTA kill-switch catch branch is the same write-await-return false shape and is still unfixed — lib/src/code_push.dart:520-537

This was raised last round and has not been addressed or refuted — no reply, and the code is unchanged at HEAD:

520  if (data['ota_disabled'] == true) {
521    status.value = 'OTA disabled by server';
522    try {
530      await _rollbackInternal(quarantine: false);   // <-- async gap
531      status.value =
532          'OTA disabled by server — patch removed (restart to apply)';
533    } catch (_) {
534      // Best-effort: a clean device has nothing to revert.
535    }
536    return false;
537  }

The success path rewrites at :531 after the await, so it is safe. The catch path writes nothing, so the only status this branch ever leaves is the pre-await one at :521 — precisely the window :635/:657/:1941/:2034 were moved to close.

Why this matters more than an exotic edge: the catch path is the majority of the fleet, not a corner case. _rollbackInternal (:1720) throws CodePushException('No active patch to roll back.') at :1735 whenever the patch file is absent, and the engine early-return at :1726 only fires when the native rollback reports true. test/ota_controls_test.dart:62-79 is exactly this device — its mock returns false for CodePush.rollback and the test is named "clean device: stops quietly, revert attempt no-ops". Every unpatched device in the fleet takes this branch when the switch is flipped.

Failure scenario: unpatched device, operator flips the kill switch. The periodic-timer checkAndInstall writes 'OTA disabled by server' and awaits _rollbackInternal, which does a CodePush.rollback channel round trip (:1722) then await _getPatchDir() (:1730) — two real async gaps — before throwing. The user foregrounds the app in that window; _CodePushOverlayState.didChangeAppLifecycleState (:2591-2602) calls checkAndInstall, which now stamps 'A check is already running' (:468). The catch swallows, :536 returns false, and the caller (example/lib/main.dart:95-96, or the debug bar at :2618) renders No new patch installed: A check is already running instead of the fleet-wide signal an operator most needs right after flipping the switch. Before this PR that write did not exist, so this branch is a regression the PR introduces — the one place it does not also fix.

Same three-line shape as the four already applied:

if (data['ota_disabled'] == true) {
  try {
    await _rollbackInternal(quarantine: false);
    status.value =
        'OTA disabled by server — patch removed (restart to apply)';
  } catch (_) {
    // Best-effort: a clean device has nothing to revert.
    status.value = 'OTA disabled by server';
  }
  return false;
}

test/ota_controls_test.dart:77-78 (contains('OTA disabled'), isNot(contains('patch removed'))) passes unchanged.

One open Medium is mergeable under the deferred-medium rule, but filing a deferred-medium issue for a three-line fix on a PR whose stated purpose is clearing the deferred-medium backlog seems like the wrong trade — cheaper to just land it here.

🟡 Low

• The reordering ships with no test — lib/src/code_push.dart:635, :657, :1941, :2034. The one-line loser write got a regression test; the four moves that make it safe are defended only by comments. Moving a status.value back up next to its branch condition is exactly the tidy-up a later refactor makes, and nothing would go red. One test over the cheapest site covers the class: offer a mismatching engine_fingerprint, have the telemetry handler fire a second check() before responding, assert the winner returns false with status.value starting 'Incompatible baseline: engine ABI mismatch'. test/hardening_test.dart:27-48 already gates responses on offerDelay, so a handler-side hook is a small addition.

• The new test asserts the status but not that single-flight still holds — test/hardening_test.dart:173-195. It checks status.value and await loser == isFalse, but not updateChecks or the winner's result. A regression where the loser stamps the status and proceeds to the network would still pass. expect(updateChecks, 1) and expect(await winner, isTrue) after :194 close that for two lines — the sibling test at :160-171 already asserts updateChecks.

• The contract is enforced in code but absent from the public docs, and the nearest doc reads the other way. "Every false return leaves its reason in status" now lives in an example comment (example/lib/main.dart:90-94) and a test name (test/hardening_test.dart:173), while checkAndInstall's own dartdoc (lib/src/code_push.dart:442-444) says nothing about status, and both status's dartdoc (:161-164) and README.md:238-240 say it is "a fleeting TRANSITION, not a level you can poll". A reader following the published docs would conclude that reading status after false is unsupported. A line on checkAndInstall stating the contract, plus A check is already running in the values list at README.md:247-248, would make the two agree.

🟢 Positives

• All four Round 1 sites are correctly reordered, and each carries a comment explaining why the write sits after the await. The ordering is load-bearing and invisible; without those comments the next reader moves it back. That is the right defense for a constraint no test enforces.
• The Round 2 follow-up commit narrowed two of those comments to "best-effort ordering, not a guarantee" on the _iosLoadPayload paths where the single-flight guard genuinely is not held (:850 returns the future without awaiting it, so the finally at :883 releases the guard while the load is still running). Correcting a comment down from a guarantee to a best-effort, unprompted, is the harder direction to get right.
• The single-flight write at :468 is correctly placed. It lands before the first await, so it cannot race the latch at :471; it cannot be swallowed by ValueNotifier's equality suppression (its string never equals a neighbour); and it cannot spoof the overlay's latch, which gates on status.value == 'Patch active' (:2576) — the moduleResult→status pair at :1995-1996 dispatches synchronously, so that edge is always delivered before any later loser write.
• The determinism fix on the new test is the better version of the suggestion rather than a transcription: both check() calls in one synchronous turn, status asserted before any await, and a comment (test/hardening_test.dart:184-188) recording why the wall-clock delay was wrong instead of just deleting it.
• The mounted audit in the example is genuinely complete, not spot-fixed. example/lib/main.dart is the only Dart file in the example package, and every setState reachable after an await — _loadStatus:65, _manualCheck:84/:88/:100, _rollback:109/:113/:120 — is guarded, including both catch branches in _rollback, which is the one that usually gets missed. Pre-await synchronous calls at :73 and :106 are correctly left alone, and every early return skips only UI work, so no side effect is dropped.
• The corrected callback comment (example/lib/main.dart:80-83) now states both the true ordering (synchronous, before the await resumes — :827, :876) and the real reason the guard is still needed (subtree re-key at :2612). Getting the reason right matters more than the guard, which would otherwise read as cargo cult.
• The result-object suggestion was declined with a stated reason and scoped out rather than silently dropped — the right call for an API change inside a fix batch.

_{Static review — no Flutter toolchain in this environment, so the 153/153 and flutter analyze claims are unverified here. Line numbers are from the PR merge head.}

[fonkamloic]

Round-3 Medium fixed in 040614b: the OTA kill-switch branch no longer writes its status before the awaited revert attempt — both post-await branches now write it (success keeps 'OTA disabled by server — patch removed (restart to apply)', the catch path writes 'OTA disabled by server'), so a concurrent check's 'A check is already running' can no longer stand as the final status on unpatched devices. Added a deterministic regression test that fires a concurrent check from inside the revert's async gap (fails on the previous code, passes now). Re-audited every remaining status.value write for the same write-await-return shape: no further sites. flutter analyze clean (6 known avoid_print infos), flutter test 154/154.

[claude]

Round 4 — re-reviewed against the current merge head (040614b). The Round-3 Medium is fixed as described, and I verified it independently rather than taking the reply at face value.

🔴 Critical

None.

• No native or platform-channel change in the diff (android/, ios/ untouched), so Dart↔Android/iOS parity is unaffected.
• No new network or file-I/O surface: no change to _httpGet/_httpGetBytes, the download hash verification (lib/src/code_push.dart:723-729), the download size cap, or the https/loopback gating at :484-488 and :557-566.
• No source-breaking change. The only public-API-visible effect is a new CodePush.status string; status is documented free-form and transitional (lib/src/code_push.dart:161-171, README.md:236-238), so an additional value is additive.
• Failure-soft holds for every reordered write. _reportIncompatibleBaseline (:958-1000) wraps its whole body in try/catch (_) { return false; }, and _iosImmediateRollback (:2213-2280) try/catches each of its four steps independently. I read both in full — neither has a path that escapes, so moving the status.value write to after the await cannot skip it.

🟠 Medium

None.

Round-3 finding verified fixed — lib/src/code_push.dart:520-540. The kill-switch branch no longer writes 'OTA disabled by server' before the awaited revert; both outcomes write after it (:530-531 on success, :538 in the catch). The regression test at test/ota_controls_test.dart:81-108 is a real one: it fires the concurrent check() from inside the CodePush.rollback mock, i.e. while the winner is parked on the _rollbackInternal await with _checkInFlight still latched (:471, released only by the finally at :887) — so the loser genuinely takes the :467 early return, and expect(..., isNot(contains('already running'))) at :107 fails on the pre-fix code.

I re-ran the audit for the same write-await-return false shape rather than trusting the "no further sites" claim, and agree there are none left:

• All 18 false returns in checkAndInstall write status first (:469, :487, :511, :540, :545, :553, :565, :589, :608, :640, :663, :710, :715, :727, :777, :787, :824, :885), and each terminal one that sits behind an await now writes after it.
• _iosLoadPayload's two false returns (:1948, :2039) are both post-await.
• The remaining status writers return void and use fire-and-forget telemetry (:2101-2116, :2132-2134, :2149, :2175, :2181), so the shape doesn't apply.
• The store-install gate (:316-331) is the one structural lookalike, but it never starts the update flow (:332 returns before _startUpdateFlow), so there is no in-process check to lose to.

🟡 Low

• Four of the five reordered writes still ship with no test — lib/src/code_push.dart:639, :661, :1945, :2038. The kill-switch reorder got the regression test it needed; the incompatible-baseline and _iosLoadPayload moves are defended only by comments. grep over test/ finds no assertion on 'Incompatible baseline', 'Module error', or 'Patch format is unexpected' at all, so moving those writes back up next to their branch condition — exactly the tidy-up a later refactor makes — turns nothing red. One test over the cheapest site covers the class: offer a mismatching engine_fingerprint, fire a second check() from the telemetry handler, assert status.value starts 'Incompatible baseline: engine ABI mismatch'. test/hardening_test.dart:27-49 already gates responses on offerDelay, so the hook is small.

• The new test can go vacuous without failing — test/ota_controls_test.dart:94. expect(loser, isNotNull) at :103 is a good vacuity guard for "the handler fired", but nothing pins "the loser actually stamped its status". If the :468 write were later made conditional or removed, the mock would still fire, loser would still be non-null, and all four assertions would still pass — the test would silently stop exercising the race it was written for. One line inside the handler, right after loser ??= check(), closes it: expect(CodePush.status.value, contains('already running'));.

• The contract this PR enforces is still absent from the public docs, and the nearest doc reads the other way (raised in Rounds 2 and 3; not addressed and not refuted, so re-flagging once). "Every false return leaves its reason in status" lives only in an example comment (example/lib/main.dart:90-94) and a test name (test/hardening_test.dart:173). checkAndInstall's own dartdoc (lib/src/code_push.dart:442-444) is three lines about the return value and says nothing about status, while status's dartdoc (:161-164) and README.md:237-238 both say it is "a fleeting TRANSITION, not a level you can poll". A reader following the published docs would conclude that the pattern the example now demonstrates is unsupported. A line on checkAndInstall stating the contract, plus A check is already running in the values list at README.md:247-248, would make the two agree.

🟢 Positives

• The Round-3 fix is the minimal correct shape and covers both outcomes. Writing the fallback in the catch — rather than only hoisting the success write — is the half that actually matters here, since _rollbackInternal throws CodePushException('No active patch to roll back.') at :1740 on every unpatched device, which is the majority of the fleet when the switch is flipped.
• The regression test picks the right injection point. Hooking the CodePush.rollback mock puts the concurrent check inside the real async gap while the guard is held, instead of approximating it with a wall-clock delay — deterministic, and it genuinely fails on the old code. expect(loser, isNotNull) guarding against a silently-never-fired handler is the kind of check that usually gets skipped.
• Existing coverage was preserved rather than rewritten around: test/ota_controls_test.dart:76-78 and :127 still assert the pre-existing clean-device and patched-device messages unchanged, so the fix is demonstrably a status ordering change and not a message change.
• Each reordered write carries a comment explaining why it sits after the await. The ordering is load-bearing and invisible; without those comments the next reader moves it back. That is the right defense for a constraint no test enforces.
• Two of those comments (:1938-1944, :2034-2037) were narrowed to "best-effort ordering, not a guarantee" on the _iosLoadPayload paths where the guard genuinely is not held — :854 returns the future without awaiting it, so the finally at :887 releases _checkInFlight while the load is still running. Correcting a comment down from a guarantee, unprompted, is the harder direction to get right.
• The :468 write is correctly placed and cannot cause collateral damage: it lands before the first await so it can't race the latch at :471; it can't be swallowed by ValueNotifier equality suppression (its string never equals a neighbour); and it can't spoof the overlay's latch, which gates on status.value == 'Patch active' (:2580) — the moduleResult→status pair at :1999-2000 dispatches synchronously, so that edge is always delivered before any later loser write.
• The mounted audit in the example is genuinely complete, not spot-fixed. example/lib/main.dart is the only Dart file in the example package, and every setState reachable after an await — :66, :85, :95, :101, :110, :114, :121 — is guarded, including both catch branches in _rollback, which is the one that usually gets missed. The pre-await synchronous calls at :73 and :106 are correctly left alone, and every early return skips only UI work, so no side effect is dropped.
• The CHANGELOG line (CHANGELOG.md:2-5) is now accurate at all five sites, which it was not in Round 3.

_{Static review — no Flutter toolchain in this environment, so the 154/154 and flutter analyze claims are unverified here. Line numbers are from the PR merge head.}

[fonkamloic]

All three Round-4 Lows addressed in c8ed9b2:

• Low 1 (no test on the reordered writes): new regression test in test/hardening_test.dart at the cheapest site per the recipe — a mismatching engine_fingerprint offer, with a second check() fired from the telemetry handler while the winner is parked on the awaited POST; asserts the terminal status starts with Incompatible baseline: engine ABI mismatch and that the loser stamped A check is already running inside the window. Verified red on the pre-fix ordering (moving the write back above the await fails it with exactly the predicted Actual: 'A check is already running').
• Low 2 (vacuity at test/ota_controls_test.dart:94): the loser-stamp assertion is added, but as a capture-in-handler + assert-in-body rather than the inline expect. Empirically verified first: a deliberately failing expect inside the rollback mock is swallowed silently (the TestFailure propagates out of the mock handler into the kill-switch branch's own catch (_) around the rollback await), so the inline form would itself be vacuous. The captured form fails loudly; a comment in the test explains why.
• Low 3 (docs): checkAndInstall's dartdoc now states the contract (every false return leaves its reason as that check's final status write; a concurrent loser stamps A check is already running without disturbing the active check's terminal message); README's status values list includes A check is already running; and both status's dartdoc and the README reconcile the "fleeting transition, not a level" wording with the enforced guarantee — which transition a check ends on, not a pollable level.

Verification: flutter analyze clean (only the 6 known avoid_print infos), flutter test 155/155 (154 + the new test).

[claude]

Round 5 — re-reviewed against the current merge head (c8ed9b2). All three Round-4 Lows landed, and I verified each independently rather than taking the reply at face value. One new finding, and it comes from the doc work itself.

🔴 Critical

None.

• No native or platform-channel change in the diff (android/, ios/ untouched), so Dart↔Android/iOS parity is unaffected.
• No new network or file-I/O surface. _httpGet/_httpGetBytes, the download hash verification (lib/src/code_push.dart:736-742), the size cap, and the https/loopback gating at :497-501 and :570-579 are all unchanged.
• No source-breaking change. The only public-API-visible effect is a new CodePush.status string plus dartdoc/README text; status is documented free-form, so an additional value is additive.
• Failure-soft holds for all five reordered writes. _reportIncompatibleBaseline (:971-1013) wraps its whole body in try/catch (_) { return false; }, and _iosImmediateRollback (:2226-2290) try/catches each of its four steps independently — I read both in full, and neither has an escaping path, so moving a status.value write to after the await cannot skip it.
• I re-walked all 18 false returns in checkAndInstall (:482, :500, :524, :553, :558, :566, :578, :602, :621, :653, :676, :723, :728, :740, :790, :800, :837, :898) — every one writes status, and every one sitting behind an await now writes after it.

🟠 Medium

The new public contract is stated unconditionally, but the PR's own comments document it as best-effort on the iOS install path — lib/src/code_push.dart:452-457, README.md:251-256

The new dartdoc says, without qualification:

Status contract: every false return leaves its reason as the FINAL [status] write of that check … 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.

Both halves are false on the iOS post-download load path, and the same commit says so eleven hundred lines further down:

1951  // Written after the awaited rollback (file deletes + telemetry
1952  // POST) so this upgrade hint is the last write this method
1953  // makes. The single-flight guard is not held here (the install
1954  // path returns this future without awaiting it, ...), so a
1955  // concurrent full check can still overwrite it later —
1956  // best-effort ordering, not a guarantee.

That comment (and its twin at :2047-2050) is correct, and it describes a checkAndInstall false return: :867 is a bare return _iosLoadPayload(...) with no await, so the finally at :899-901 clears _checkInFlight as soon as the load hits its first suspension — the returned future is chained after the finally runs. This is the same mechanism behind the standard "use return await inside a try block" guidance. So on that path the concurrent caller does not "lose the guard and stamp"; it takes the guard and runs a full check.

Failure scenario. iOS, first patch of the session, codePushLoadModule throws. _iosLoadPayload's catch (:2031) awaits _iosImmediateRollback, which does file deletes (:2256-2261), an engine channel round trip (:2270), and a telemetry POST (:2284) — a multi-second window on a flaky network. _checkInFlight is already false. The periodic timer or didChangeAppLifecycleState fires a check; it sails past :480, writes 'Checking server...' at :487, then 'Downloading patch...' at :718. The first call writes 'Module error: … — rolling back patch' at :2051 and returns false; the second keeps writing. example/lib/main.dart:96 then renders No new patch installed: Downloading patch... — a foreign operation's progress presented as this call's result, on the one platform where the actionable message (module error, format mismatch → upgrade flutter_compile) matters most. That is exactly the bug this PR fixes at the other five sites, now documented as impossible.

In-scope fix is one clause on each doc block — e.g. "…except the iOS post-download load, where the guard is released before the load completes (best-effort ordering there)." That makes the public docs agree with :1951-1957 and :2047-2050 instead of contradicting them.

Separately, and not a request for this PR: :867 returning un-awaited also means the guard is genuinely not held across the iOS load, so a second check can download and re-enter _iosLoadPayload while the first load is in flight — the precise scenario the guard's own rationale at :465-471 says it exists to prevent ("on iOS a second load of the same payload throws — whose rollback would then revert the copy the first call just loaded successfully"). return await at :867 would close both, but it changes when the guard releases and deserves its own PR with its own tests. Flagging it so the connection is on record.

🟡 Low

• The README now teaches string-matching on a status literal — README.md:247-256. Before this PR the values list was illustrative and the surrounding text steered readers away from consuming status programmatically. It now names A check is already running and documents reading status right after await as the supported way to explain a false. Apps will string-compare it, and the next reword becomes a silent breaking change. Either expose it as a static const on CodePush (which the docs can then reference), or add a line saying the strings are human-facing and not a stable API.

• The contract is documented only for false returns — lib/src/code_push.dart:452-457. The true side is the stronger guarantee and is worth the half sentence: both 'Restart to apply' writes (:843-845 iOS persist-and-restart, :892-894 Android) are followed only by a synchronous onUpdateReady?.call() and the synchronous finally, so nothing can interleave. Stating just the false half leaves a reader to assume the true half is unspecified, when it is the case that holds unconditionally on both platforms.

🟢 Positives

• The new hardening_test.dart regression test is the real thing, not a shape. Firing the loser from inside the /telemetry/client-error handler (:46-48, :198-215) parks the winner on the awaited POST with _checkInFlight still held — genuinely the reordered-write window at :663-675, not an approximation. expect(loser, isNotNull) guards against a never-fired hook, expect(statusDuringReport, 'A check is already running') proves the race was actually exercised, and expect(downloads, 0) pins that the mismatch is still refused pre-download.
• The vacuity fix chose correctness over the literal suggestion. Round 4 asked for an inline expect inside the rollback mock; the author checked first, found that a TestFailure there is swallowed by the kill-switch catch (_) at :545, and used capture-in-handler + assert-in-body instead (test/ota_controls_test.dart:96-99, :109-111) — with a comment recording why. The inline form would itself have been vacuous. The same pattern is then applied consistently in the new hardening test.
• The doc reconciliation resolves a real contradiction rather than papering over it. status's dartdoc (:166-170) and README.md:251-256 now separate "which transition a check ends on" from "a level you can poll", so the example's status-after-false pattern (example/lib/main.dart:90-96) is no longer contradicted by the very docs a reader would consult. Modulo the Medium above, the distinction drawn is the right one.
• Existing coverage was preserved, not rewritten around. test/ota_controls_test.dart:76-78 still asserts the pre-existing clean-device message unchanged, so the kill-switch change remains demonstrably an ordering fix and not a message change; the new test is additive alongside it.
• Each reordered write carries a comment explaining why it sits after the await, and two of them (:1951-1957, :2047-2050) were narrowed to "best-effort ordering, not a guarantee" on exactly the paths where the guard genuinely isn't held. Correcting a comment down from a guarantee, unprompted, is the harder direction to get right — and it is what let me pin the Medium above.
• The mounted audit in the example is complete, not spot-fixed. example/lib/main.dart is the package's only Dart file, and every setState reachable after an await — :66, :85, :88, :100, :109, :113, :120 — is guarded, including both catch branches in _rollback, which is the one usually missed. The pre-await synchronous calls at :73 and :106 are correctly left alone, and every early return skips only UI work (_loadStatus has no side effects), so nothing is dropped.
• The new loser write at :481 still can't cause collateral damage. It lands before the first await; it can't be swallowed by ValueNotifier equality suppression; and it can't spoof the overlay, whose only string comparison in the whole package is the one-directional == 'Patch active' latch at :2593, delivered synchronously by the moduleResult→status pair at :2012-2013.
• The CHANGELOG line (CHANGELOG.md:3-5) is accurate — it claims only that an overlapping call reports its own reason, which is what the code now does.

_{Static review — no Flutter toolchain in this environment, so the 155/155 and flutter analyze claims are unverified here. Line numbers are from the PR merge head (c8ed9b2).}

[fonkamloic]

Round-5 Medium addressed in 6c8f800: both doc blocks now carry the iOS post-download qualifier (guard released before the load completes → best-effort ordering there), matching the code comments at the two load-path sites. The separate return await guard-release observation is filed as its own issue rather than folded in here, per the review's own scoping.

[claude]

Round 6 — re-reviewed against the current merge head (6c8f800). The Round-5 Medium is fixed as described, and I verified it independently rather than taking the reply at face value. No new Critical or Medium; the three Lows below are documentation/scope items, one of which is pre-existing and explicitly not a gate on this PR.

🔴 Critical

None.

• No native or platform-channel change in the diff (android/, ios/ untouched), so Dart↔Android/iOS parity is unaffected.
• No new network or file-I/O surface. _httpGet/_httpGetBytes, the download hash verification (lib/src/code_push.dart:739-743), the size cap, and the https/loopback gating at :496-504 and :571-581 are unchanged.
• No source-breaking change. The public-API-visible effect is one new CodePush.status string plus dartdoc/README text; status is documented free-form, so an additional value is additive.
• Failure-soft holds for all five reordered writes. _reportIncompatibleBaseline (:974-1016) wraps its whole body in try/catch (_) { return false; }, and _iosImmediateRollback (:2229-2297) try/catches each of its four steps independently — I read both in full; neither has an escaping path, so moving a status.value write after the await cannot skip it.
• I re-walked every false return in checkAndInstall (:485, :503, :527, :556, :561, :569, :581, :605, :624, :656, :679, :726, :731, :743, :793, :803, :843, :901). Each writes status, and each one sitting behind an await writes after it. The catch at :899-901 and the finally at :902-904 are the last two frames, and the catch writes 'Error: $e' immediately before returning, so the contract holds on the throw path too.

🟠 Medium

None.

Round-5 Medium verified fixed — lib/src/code_push.dart:452-460, README.md:257-259. Both doc blocks now carry the iOS post-download qualifier, and it matches the code. I confirmed the underlying mechanism rather than just the wording: :870 is a bare return _iosLoadPayload(...) with no await, so the finally at :902-904 clears _checkInFlight at the load's first suspension, and the two load-path comments (:1954-1960, :2050-2053) correctly describe their writes as best-effort. The qualifier's placement is right — it attaches to the concurrent-caller sentence and reaches back over the "FINAL write" sentence via "its writes."

The return await observation from Round 5 was filed as its own issue per that review's own scoping, so it is not re-raised here.

🟡 Low

• status's own dartdoc still states the guarantee unqualified — lib/src/code_push.dart:166-170. This is the same doc set the Round-5 Medium was about, one block wider than the two that were cited. checkAndInstall's dartdoc (:452-460) and README.md:251-259 now both carry "except on the iOS post-download load path"; status's does not — it says the reason "was that check's final write, so reading the value right after the await is reliable," full stop. status is the member a reader lands on first when they want to know what the notifier promises, so it is the block most likely to be read alone. A trailing "— see [checkAndInstall] for the one iOS exception" closes it. The same applies to the example's comment at example/lib/main.dart:90-94 ("which checkAndInstall writes before every false return"), though an example carries less weight than a dartdoc.

• The README now teaches string-matching on a status literal with no stable-API statement (raised in Round 5, not addressed and not refuted, so re-flagging once) — README.md:247-259. Before this PR the values list was illustrative and the surrounding prose steered readers away from consuming status programmatically. It now names A check is already running and documents reading status right after the await as the supported way to explain a false. Apps will string-compare it, and the next reword becomes a silent breaking change for them. Either expose it as a static const on CodePush that the docs reference, or add one line saying the strings are human-facing and not a stable API. The tests already pin the literal in two places (test/hardening_test.dart:196, test/ota_controls_test.dart:106), so a static const would cost nothing to adopt.

• Out of scope for this PR, pre-existing — flagging for a separate issue, not as a gate: CodePushOverlay.didChangeAppLifecycleState bypasses disableOnPlayStoreInstalls — lib/src/code_push.dart:2610-2624. The flag is consulted in exactly one place (:315, inside init's async chain), and that branch returns at :338 before _startUpdateFlow, so no periodic timer starts. But the overlay's resume handler calls CodePush.checkAndInstall directly at :2614 with no store-install check, and checkAndInstall never consults the flag itself (grep for disableOnPlayStoreInstalls hits only :284, :295, :315, :2422, :2435, :2588). So on a Play-Store-installed Android build with disableOnPlayStoreInstalls: true, the first app resume runs a full check → download → install, against CodePushConfig's documented promise that those installs "only ever change through store updates" (:2431-2434). The asymmetry — the overlay forwards the flag to init at :2588 but not to its own resume check — reads as an oversight rather than a design choice. No test covers it: the four disableOnPlayStoreInstalls tests (test/ota_controls_test.dart:179, 212, 247, 265) all exercise init, none the resume path. Untouched by this diff, so it should not block this merge.

🟢 Positives

• The Round-5 doc fix is the minimal correct edit and lands in both places. It would have been easy to soften the guarantee into vagueness; instead the qualifier names the exact path, the exact mechanism (guard released before the load completes), and the exact consequence (a concurrent full check may overwrite). That makes the public docs agree with :1954-1960 and :2050-2053 instead of contradicting them, and it is falsifiable — a reader can check it against the code.
• The separate return await observation was filed rather than folded in. That is the right call: changing when _checkInFlight releases alters the concurrency shape of the iOS install path and deserves its own tests, not a drive-by in a docs commit.
• The two regression tests added over Rounds 3–4 both inject at the real async gap, not an approximation. test/ota_controls_test.dart:81-113 fires the loser from inside the CodePush.rollback mock (winner parked on _rollbackInternal with the guard held); test/hardening_test.dart:200-241 fires it from inside the /telemetry/client-error handler (winner parked on the awaited POST). Both are deterministic, both go red on the pre-fix ordering, and both carry a vacuity guard (expect(loser, isNotNull)) plus an in-window status capture that proves the race actually ran.
• The vacuity fix chose correctness over the literal suggestion. Round 4 asked for an inline expect inside the rollback mock; the author checked first, found a TestFailure there is swallowed by the kill-switch catch (_) at :548, and used capture-in-handler + assert-in-body instead — with a comment recording why (test/ota_controls_test.dart:93-96). The inline form would itself have been vacuous. The same pattern is then applied consistently in the hardening test.
• The new /telemetry/client-error branch in the test server (test/hardening_test.dart:46-49) is split out of the catch-all, so telemetry POSTs no longer inflate downloads — which is what lets expect(downloads, 0, reason: 'mismatch is refused before download') at :240 mean what it says.
• Existing coverage was preserved, not rewritten around. test/ota_controls_test.dart:61-78 still asserts the pre-existing clean-device message unchanged, so the kill-switch change remains demonstrably an ordering fix and not a message change; the new test sits alongside it.
• Each reordered write carries a comment explaining why it sits after the await. The ordering is load-bearing and invisible; without those comments the next reader moves it back. Two of them (:1954-1960, :2050-2053) were narrowed to "best-effort ordering, not a guarantee" on exactly the paths where the guard isn't held — correcting a comment down from a guarantee, unprompted, is the harder direction to get right, and it is what made the Round-5 Medium findable.
• The new loser write at :484 still can't cause collateral damage. It lands before the first await, so it can't race the latch at :487; it can't be swallowed by ValueNotifier equality suppression; and it can't spoof the overlay, whose only string comparison in the package is the one-directional == 'Patch active' latch at :2596, delivered synchronously by the moduleResult→status pair at :2015-2016.
• The mounted audit in the example is complete, not spot-fixed. example/lib/main.dart is the package's only Dart file, and every setState reachable after an await — :66, :85, :88, :95, :109, :113, :120 — is guarded, including both catch branches in _rollback, which is the one usually missed. The pre-await synchronous calls at :73 and :106 are correctly left alone, and every early return skips only UI work (_loadStatus has no side effects), so nothing is dropped.
• The CHANGELOG line (CHANGELOG.md:3-5) claims only what the code does — that an overlapping call reports its own reason — rather than the stronger "never overwrites" it started as.

_{Static review — no Flutter toolchain in this environment, so the 155/155 and flutter analyze claims are unverified here. Line numbers are from the PR merge head (6c8f800).}

[fonkamloic]

Round-6 clean (no Critical, no Medium; the three Lows are documentation/scope items the review itself marks as non-gating — tabled on record here). Merging under the standard gate.