Skip to content

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

Open
fonkamloic wants to merge 2 commits into
mainfrom
fix/deferred-medium-batch-20260826
Open

Clear deferred-medium backlog: single-flight status write + example mounted guards#34
fonkamloic wants to merge 2 commits into
mainfrom
fix/deferred-medium-batch-20260826

Conversation

@fonkamloic

Copy link
Copy Markdown
Contributor

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. 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.

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

checkAndInstall's contract is that every false return leaves its reason
in CodePush.status. The single-flight early return (a second check while
one is already running) was the one false return that wrote nothing, so
a losing caller surfacing status after false rendered the in-flight
check's foreign progress state as its own result. It now writes
'A check is already running' before returning.

The write is transient by design: the in-flight check keeps overwriting
status as it progresses, and ValueNotifier notifications are synchronous,
so the 'Patch active' edge is always delivered before any overwrite.

Adds a regression test in the single-flight group.

Fixes #32
The overlay re-keys the app subtree when a patch activates, which
disposes the demo 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 bails out first when
the State is no longer mounted. 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.

Fixes #33
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown

🔴 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant