Overlay hardening: supersede-race fix, widget coverage, patch-active constant - #36
Conversation
…am (#31, #29) #31 — `'Patch active'` was app-facing API written as a bare literal in three places and compared in a fourth, so an edit to the text could silently break every caller latching on it. Promote it to `CodePush.statusPatchActive` and route all four sites plus the docs through the constant. The string itself is unchanged. #29 — `CodePushOverlay.initState` drives the live update cycle (server check, patch-directory file I/O) and the update-ready signal is private to it, so the documented `bannerBuilder` contract had no way to be asserted on a host test runner. Add `debugUpdateCycleOverride`, a `@visibleForTesting` seam that takes over the three calls the overlay makes on its own behalf (init, resume re-check, dispose) so a test can fire the overlay's own `onUpdateReady` directly. When it is unset the overlay behaves exactly as before. Also folds the four duplicated update-ready/dismiss closures into `_markUpdateReady` / `_dismissBanner`; the dismiss path now carries the `mounted` guard the ready path already had, since a custom banner may call the handed `onDismiss` after unmount. No update-flow, platform-channel or wire-format change.
#31) Fourteen `testWidgets`/`test` rows against the seam from the previous commit, pinning what the docs already promise: - a custom `bannerBuilder`'s widget is what renders once an update is ready, and it replaces the default banner rather than adding to it; - the handed `onDismiss` removes the banner, and the slot comes back for the next update; - the documented "show no banner" recipe — returning `const SizedBox.shrink()` — renders nothing hit-testable (note the banner slot is a left/right-anchored `Positioned`, so the box is stretched to full width: zero HEIGHT is what makes it inert); - the default banner renders and its LATER button dismisses; - config resolution: explicit `config:` wins, `CodePush.lastConfig` is the fallback, and neither is an actionable `StateError`; - the debug bar tracks `CodePush.status` and retires on the `statusPatchActive` edge, which latches and does not un-latch. Plus two rows for #31: the constant keeps its app-facing wire value, and it is the only place in the library source where the literal appears outside doc-comment prose — so a future status-text edit cannot move one writer without the others. Closes #29. Closes #31.
Two `CodePush.init` calls leave two live update chains: the epoch guard covers every async gap before the flow starts, but the flow's own continuation does not carry the epoch, so the FIRST chain wins the single-flight guard. An app that calls `init(onUpdateReady:)` in `main()` and then wraps `runApp` in `CodePushOverlay` therefore gets its own callback, not the overlay's banner. The test tells the two sessions apart by `app_id` on the /updates query against a loopback server, and asserts only the newest session checks. Verified to reproduce: without the fix the captured request carries the SUPERSEDED session's app_id. It is committed SKIPPED. The fix belongs in the update flow itself (threading the captured epoch into the `_startUpdateFlow` continuation and bailing before the iOS reload, the launch timer, the quarantine and rollback-report side effects, and the first check), which is out of scope for this widget-layer branch and needs review of its interaction with the three-strike gate. Removing the `skip:` is the acceptance criterion. Refs #30.
…ersedes an in-flight one The crash-protection .then continuation ran to completion for a superseded session, taking the single-flight guard with the stale serverUrl/appId and starving the live session's first check. Thread the init epoch into _startUpdateFlow and re-check it at the continuation entry and after every await (the same discipline init's store-install branch documents). A superseded or disposed session now also never starts the launch-success timer (gate 5). Un-skips test/init_supersede_test.dart — the reproduction harness is the acceptance criterion.
🔴 CriticalNone. Nothing here can crash or brick a host app, and the public API changes are purely additive ( 🟠 Medium1. The epoch guard runs after The guard is the first statement inside Failure scenario: iOS, a good patch installed, app calls 2. The epoch does not survive into
Failure scenario: app calls 🟡 Low
🟢 Positives
|
…eview Lows - M1: crash protection (iOS boot counter) now runs at most once per process via a latch - two inits per launch no longer double-increment the three-strike counter (test hook: debugResetCrashProtectionLatch). - M2: checkAndInstall is epoch-aware at its two network chokepoints, so a chain superseded mid-check cannot download or install with stale parameters; a LIVE caller that lost the single-flight guard to a STALE in-flight check re-arms once when the guard clears (same-epoch overlaps keep the pinned quiet-loser semantics). - Lows: harness doc-comment brought in line with reality; harness sync is now completer-based (no fixed sleep); the overlay captures its update-cycle mode in initState so resume/dispose cannot disagree with a mid-lifetime seam change; the constant scan covers every lib source file and strips trailing comments; CHANGELOG gains the two user-facing entries. Suite 170/170.
|
Round-1 response: both Mediums fixed (crash-protection per-process latch; epoch-aware checkAndInstall + stale-loss re-arm — scoped to live-vs-stale losses so the pinned quiet-loser semantics for same-session overlaps are unchanged). Lows 1/2/3/5/6 fixed. Low 4 (seam call-kind discriminator + resume-branch coverage) tabled as a tracked follow-up: it is test-infrastructure ergonomics with no production surface; noting here per the gate. |
|
Round-2 review. The round-1 Mediums and Lows 1/2/3/5/6 are genuinely fixed — I re-read the re-arm, the crash-protection latch, the 🔴 CriticalNone. Nothing added here can crash or brick a host app: every new epoch check is a plain 🟠 Medium1. The supersede window is closed at the two network chokepoints but not across the install, so a stale chain can still fire the stale The post-download guard at Failure scenario: The re-arm cannot heal this by construction, because the second check legitimately has nothing to offer. A fix needs the notification to be re-deliverable: e.g. a process-level "an install this session is pending restart" latch that the already-installed branch consults and re-announces to the current caller's 2. Both round-1 concurrency fixes ship with zero test coverage —
That leaves the two riskiest additions in the PR — process-global mutable statics with epoch-dependent branches, and a per-process latch on a rollback counter — resting on review rather than on a gate. The re-arm in particular is testable with the same loopback harness Merge gate: two open Mediums. Per policy this may merge only if each unfixed one is filed as its own issue labelled 🟡 Low
🟢 Positives
|
…cy coverage - M1: the install window no longer leaks the notification to a superseded session. Both install tails set a per-process pending-restart latch and fire onUpdateReady only for a current session; the already-installed branch re-announces to a live caller while an install from this session still awaits its restart (the re-arm's second check now produces the banner instead of a silent 'Patch already installed'). Latch cleared on rollback and on the server-withdrawal convergence branch. - M2: both round-1 mechanisms now have direct coverage — a held-open loopback response proves a mid-flight supersede re-arms exactly one live check (no stampede), and a run counter proves the boot-counter latch admits one crash-protection run per process across inits. - Low: removed the tautological epoch conjunct from the re-arm guard. Suite 172/172.
|
Round-3 review. The two round-2 Mediums are addressed and I verified each: the install-window door is closed end-to-end (stale chain installs → One new Critical below, which is a side effect of the round-2 Medium-1 fix. 🔴 Critical1. The latch is set on install and cleared only by rollback ( Both automatic entry points pass
Failure scenario A (overlay): Android, patch installs, banner appears, user taps LATER → Failure scenario B (app-supplied callback, the public-API break): The #30 door only needs the notification to survive once, to the first live caller that lands on that branch. Clearing the latch after re-announcing ( 🟠 Medium1. The re-arm regression test passes whether or not the re-arm runs — Synchronization between the live Making it discriminating is cheap: after the 300ms window and before releasing, assert 2.
🟡 Low
🟢 Positives
Merge gate: 1 Critical → blocks. The two Mediums are moot until the Critical is resolved, since the fix and its coverage land together. |
…r both mechanisms
- CRITICAL: the pending-restart latch was a sticky level - every
periodic/resume check re-fired onUpdateReady until restart. The
announcement is now edge-triggered per session (announced-epoch
marker): one delivery per session, whether at install time or via the
already-installed re-announce; rollback and the convergence branch
reset it.
- The re-arm test now PROVES the re-arm ran: it requires the live chain
to observably lose the guard ('A check is already running') while the
stale request is still held, closing the pass-without-re-arm hole.
- The latch itself is tested end to end through the already-installed
branch (staged on-disk identity + matching offer): silent without a
pending install, exactly one announcement per session, no re-fire on
later checks, one more for a genuinely new session, silent after the
rollback-path clear.
Suite 173/173.
|
Round-4 review. The round-3 Critical is fixed and I traced it end to end: 🔴 CriticalNone. Every new epoch check is a plain 🟠 Medium1. The pending-restart announcement is one token per SESSION but delivered to one CALLER — All three sites stamp Failure scenario: an app uses The null-callback half is a plain bug in any configuration and is cheap to close: only stamp the marker when there is a callback to stamp it for. The multi-caller half needs "who has been told" to be per-listener rather than per-epoch — e.g. a level the overlay can read rather than an edge only one caller can consume. 2. Before this PR In the package's own example: Merge gate: two open Mediums. Per policy this may merge only if each unfixed one is first filed as its own issue labelled 🟡 Low
🟢 Positives
|
…rn contract - A caller passing no callback no longer consumes the session's one pending-restart announcement - the next caller that can hear it gets it. Multi-listener UIs are pointed at status/moduleResult in the contract docs. - The 'onUpdateReady can fire on a false return' behavior is now part of the documented contract (checkAndInstall doc, README, example comment).
|
Round-5 review. The round-4 work is in — 🔴 CriticalNone. Every new epoch check is a plain 🟠 Medium1. The two install sites still stamp the session's announcement token when Round 4 flagged that all three sites stamp Failure scenario: an app mounts The multi-caller half is also still open, and the new docs' mitigation does not apply to the package's own widget: 2. The shipped example still demonstrates the contradiction its own new comment warns against — Round-4 Medium 2 said Merge gate: two open Mediums. Per policy this may merge only if each unfixed one is first filed as its own issue labelled 🟡 Low
🟢 Positives
|
… example follows its own advice Both install tails now stamp the session's announcement token only when a callback actually receives it, matching the re-announce site's rule from round 4. The example tracks whether onUpdateReady fired and no longer clobbers the restart prompt with the false-return status.
|
Round-6 review. Round-5's two Mediums are fixed and I verified both: the producing install tails now stamp the announcement token only when a callback hears it ( What is left is the half of round-4/5 Medium 1 that was never addressed — the token is per-SESSION but consumed by one CALLER — plus its coverage gap. Neither has been refuted in-thread. Round-5 Lows are re-listed compactly, not re-argued. 🔴 CriticalNone. Every new epoch check is a plain 🟠 Medium1. The session's announcement token is consumed by whichever caller installs first — and the shipped example is the configuration that loses it. Both producing sites now require a non-null callback before stamping, but they stamp for their own caller and there is no second delivery.
Reproducible with the example as shipped: Also worth having in view: re-delivery depends on the server still offering the same patch — 2. The round-5 token rule ships with no test.
That is the third consecutive round in which this one mechanism changed semantics (r3 sticky level, r4 unconditional stamp, r5 null-callback stamp), each caught by reading rather than by a gate. The fixture exists at Merge gate: two open Mediums. Per policy this may merge only if each unfixed one is first filed as its own issue labelled 🟡 Low
🟢 Positives
|
Structural exit from the one-token-many-listeners class: 'Restart to apply' is now a named constant (statusRestartToApply), and the overlay latches the restart-pending edge from CodePush.status - every listener sees the status write, so the banner appears no matter which caller's callback consumed the session announcement (the shipped example's manual button being the case that lost it). The round-5 token rule is pinned end-to-end through a REAL install tail (mocked engine persists the payload): a callback-less install leaves the token, the next callback-passing check receives it.
|
Round-7 review. Both round-6 Mediums are addressed and verified in the code: the overlay now latches the restart edge off 🔴 CriticalNone. Every new epoch check is a plain 🟠 Medium1. The round-6 fix ships with no test —
2. The status latch is edge-only: an overlay that mounts after the install never sees it —
Failure scenario: Merge gate: two open Mediums. Per policy this may merge only if each unfixed one is first filed as its own issue labelled 🟡 Low
🟢 Positives
|
initState now evaluates the current notifier values once (exactly as the listener would), so an overlay mounting AFTER an install or a patch activation is not blind to a level the edge-only listeners will never fire for. Both shapes pinned: the mid-life status edge shows the banner, and a late-mounting overlay shows it too.
|
Round-8 review. Both round-7 Mediums landed and I verified them: the mount catch-up is real ( 🔴 CriticalNone. Every epoch check is a plain 🟠 Medium1. The overlay now has two independent deliveries of the same restart signal, and the second one undoes a dismiss — Round 6 routed the restart-pending signal through Concrete sequence, all on Android:
That contradicts the dismiss contract Round 7 checked dismiss semantics and concluded they hold, but only over the status writers (same-value Cleanest fix: now that the overlay latches the level, stop having it consume the edge. Drop Merge gate: one open Medium — under the two-Medium ceiling, so this does not block on count. It still wants a fix or a 🟡 Low
🟢 Positives
|
The status latch is now episode-scoped: while the restart-pending level stands, a dismissal stands too - later notifier events (moduleResult, status rewrites) cannot re-latch and undo it. The episode resets when the status leaves the level, so a genuinely new install (which transitions through other statuses) shows a fresh banner. Pinned: dismissed-stays-dismissed under a second delivery, and a new episode re-offers.
|
Round-9 review. The round-8 fix landed and I verified the half it covers: 🔴 CriticalNone. Every epoch check is a plain 🟠 Medium1. The dismissed banner still returns — through the callback, not the notifier. The round-8 fix is scoped to Sequence, Android:
Fix, unchanged from round 8: drop The matching test row is still missing — Merge gate: one open Medium — under the two-Medium ceiling, so it does not block on count. It still wants a fix or a 🟡 Low
🟢 Positives
|
_markUpdateReady itself now enforces one-banner-per-restart-episode, so the update-cycle callback (a resume check's re-announce) can no longer undo a dismissal the notifier-side guard honored. A fresh install transitions the status, resetting the episode, so new updates still re-offer through either path. Pinned both ways.
|
Round-10 review. The round-9 guard reads a status value the real call site has already overwritten, so the round-9 finding still reproduces. Round-9 Lows are re-listed compactly, not re-argued. 🔴 CriticalNone. Every epoch check is a plain 🟠 Medium1. The dismissed banner still returns — the round-9 guard cannot fire at the site it was written for.
if (CodePush.status.value == CodePush.statusRestartToApply) { // 2841
if (_restartLevelHandled && !_updateReady) return; // 2842
_restartLevelHandled = true;
}
if (mounted) setState(() => _updateReady = true); // 2845The already-installed re-announce writes Sequence, Android:
PR-introduced: on I could not find a reachable path where the Why the new test misses it — Fix: let the episode survive the intermediate status writes instead of re-reading the level at callback time — record the dismissal for the current episode and end the episode only on a real level re-entry, not on any status leaving the level. Dropping Merge gate: one open Medium — under the two-Medium ceiling, so it does not block on count. It still wants a fix or a 🟡 LowAll carried over, re-verified at current line numbers.
🟢 Positives
|
… status strings The round-9 guard was dead code - the re-announce site overwrites the status before the callback fires, so a status-keyed episode rule can never see the level at the moment it matters. Episodes are now the SDK's own truth: isRestartPending + a monotonic installSeq bumped at each install commit. The overlay records which episode was dismissed; that dismissal stands across BOTH delivery paths and any status churn, and only a LATER install (a new episode) re-offers. The status-string latch is gone; every notifier event re-evaluates the pending level, so a late-mounting overlay shows the banner even while the status reads a transient - a gap the old latch had. Tests now drive the production ordering the round-10 review specified (status overwritten before the callback), plus the transient-status mount case.
|
Round-10 response: taken at the root rather than another patch on the guard — you were right that the status channel cannot anchor episodes (busy channel; the guard was dead code). Episodes are now the SDK's own truth: |
|
Round-11 review. The round-10 Medium is fixed — I traced the mechanism rather than the description, and it no longer reproduces on the production ordering. Rounds 7–10 Lows are re-listed compactly, not re-argued; three are new. 🔴 CriticalNone.
🟠 MediumNone. The round-10 finding does not reproduce. I re-traced its exact sequence on Android against the new anchor:
🟡 Low
🟢 Positives
|
1 participant
Fixes #30, fixes #29, fixes #31.
init(e.g.CodePushOverlay's) did not supersede an in-flight one — the crash-protection continuation completed for the stale session, took the single-flight guard with the supersededserverUrl/appId, and starved the live session's first check. The init epoch now threads into_startUpdateFlowand is re-checked at the continuation entry and after every await (the disciplineinit's store-install branch already documents). A superseded/disposed session also never starts the launch-success timer. Proven by a loopback-server reproduction harness that failed pre-fix and is now un-skipped (170/170 green).CodePushOverlaywidget-test coverage via a@visibleForTestingupdate-cycle seam (null ⇒ byte-identical production behavior): 14 cases pinning the bannerBuilder contract, dismiss/re-offer semantics, config resolution, and status latching.'Patch active'promoted toCodePush.statusPatchActive(value unchanged — no wire break), all writers routed through it, enforced by a source-scan test.Authorship note: #30's update-flow change was implemented under the reviewer lane per the workspace's work/review split; #29/#31 by the implementation lane.