fix(ios): rollback keeps in-memory module state true to the VM — and sticks

[fonkamloic]

Follow-up tabled on #17/#19 (the rollback() _moduleLoaded lie), approved for this batch.

The bug

A loaded module cannot be unloaded from the running VM, but the Dart-side rollback (the OTA kill switch's path, and app-called rollback()) cleared _moduleLoaded and the loaded identity while cleaning the disk — claiming the VM was back on baseline when it wasn't. A same-session install after a rollback then took the fresh-load path; the second loadModule into the still-occupied VM threw, and the failure handler quarantined the NEW patch — a healthy update lost until the server shipped a different one.

The fix

The flags now keep telling the truth (rollback()'s doc has always promised 'takes effect on next cold restart'): disk cleaned, module stays marked resident, later offers route through the persist-don't-load branch. moduleResult still clears as the app-facing kill-switch signal; status reports Patch removed — active until restart.

Hardening from adversarial review of the draft

• A public rollback() must STICK. Without a marker, the next check silently re-persisted the still-offered patch via the running-module match — undoing the API call minutes later. rollback() now quarantines the removed patch (auto-cleared when the server offers a different one). The kill switch uses the non-quarantining internal path: there the server stopped offering, and a re-enable must resume service with the SAME patch.
• Kill-switch bounce honesty. Re-enable-with-same-patch converges via persist-silently, but moduleResult was reverted and can't be restored in-session — 'restart changes nothing' is false there. A _contentRevertedThisSession latch routes that case to the Restart to apply banner + onUpdateReady.

Known-remaining (pre-existing, tabled): the in-flight double-load race (a load awaited in one path while another path passes its _moduleLoaded check) exists on main with identical windows; this branch strictly shrinks load opportunities. Proper fix is a load-serialization gate — its own change.

Testing

137/137 — 3 new tests for the Dart-side rollback (quarantine written from patch_info, no marker without metadata, clean-device throw). The resident-module behavior itself is platform-gated and needs a device cycle.

[claude]

🔴 Critical

None. The change preserves the public rollback() signature (Future<void>), keeps moduleResult cleared as the app-facing kill-switch signal, and only mutates internal statics. The update/reload paths remain failure-soft (every new disk/branch operation is inside try/catch or a _moduleLoaded guard), so nothing here can crash or brick the host app. The core insight — a loaded VM module cannot be unloaded, so _moduleLoaded/_loadedPatch* must keep telling the truth after a disk rollback — is correct, and it genuinely closes the "healthy same-session install gets quarantined by a second loadModule throw" hole.

🟠 Medium

• New session-latch behavior is untested (lib/src/code_push.dart:716-717, 1616-1641). The three added tests only cover marker writing on the Dart-side rollback. The two behaviors this PR actually hinges on — (a) keeping _moduleLoaded/_loadedPatch* resident after rollback, and (b) _contentRevertedThisSession flipping a persistSilently convergence into a Restart to apply + onUpdateReady — have no off-device coverage. Unlike the load/reload decisions (debugDecideLoadedSessionOffer, debugDecideReloadGate), the latch gate is inlined in checkAndInstall with no @visibleForTesting seam, so the "kill-switch bounce re-offers the resident patch" path can only be verified on a device. The author flags this as platform-gated, which is fair, but extracting the persistSilently && !_contentRevertedThisSession decision into a pure helper (mirroring the existing debug* decisions) would make the one branch this whole PR exists to add regression-safe.

🟡 Low

• Quarantine-sticks behavior is iOS-only, but the doc now reads cross-platform (lib/src/code_push.dart:1565-1571, 1583). On Android/desktop the engine-side rollback returns true at line 1583 and _rollbackInternal returns before the quarantine block ever runs, so a deliberate rollback() there never writes rolled_back_patch; the server can re-deliver the same patch on the next check (the installed_patch_identity guard is gated on the local patch file, which the engine manages, not the Dart patch dir). This is pre-existing (the early return predates this PR), but the new docstring "A deliberate rollback also QUARANTINES the removed patch" now overclaims for non-iOS. A one-line "(iOS Dart-side path)" qualifier would keep it honest.
• _contentRevertedThisSession isn't reset on the hard-reset path (lib/src/code_push.dart:2078-2081). That reset clears _moduleLoaded/_loadedPatch* but leaves the latch true. It's harmless today because the latch is only read inside the if (_moduleLoaded) branch (unreachable once _moduleLoaded is false) and gets cleared on the next successful _iosLoadPayload. Worth resetting alongside the others just so the invariant "latch implies resident revert" holds locally.

🟢 Positives

• The "don't clear _moduleLoaded on a disk rollback because the VM module can't actually be unloaded" reasoning is the right call and is documented thoroughly at the point of change (1616-1641) — future readers will understand why the flags stay set.
• Splitting the public rollback() (quarantine) from the kill-switch _rollbackInternal(quarantine: false) is the correct distinction: a deliberate rollback must stick, whereas a server-withdrawn patch must be resumable with the same bytes on re-enable. The _contentRevertedThisSession latch correctly makes the re-enable-with-same-patch bounce honest about needing a restart rather than silently converging.
• The quarantine marker is recorded before deleting patch_info.json (1595-1611), avoiding the ordering trap where the identity would already be gone.
• Marker writing, patch_info reads, and file deletes are all wrapped so a rollback can't throw on partial/corrupt on-disk state, and identity is read via the shared _identityMatches (id-or-hash, empty-as-absent) so it stays consistent with the existing already-installed/quarantine checks. The three tests also correctly cover the no-metadata and clean-device edge cases, not just the happy path.

[fonkamloic]

Triage round 1

• 🟠 Untested latch/resident behaviors — FIXED in 22fdb14: debugPersistOutcomeShowsRestart extracts the silent-vs-banner rule (3 tests), and a debugModuleLoadedForTesting window makes the resident-flag invariant assertable — a new test proves rollback() keeps the flag, sets the honest status, and still clears moduleResult. Suite 141/141.
• 🟡 Cross-platform doc overclaim — FIXED: qualified to the iOS Dart-side path (Android's engine rollback keeps its own bookkeeping via the breadcrumb).
• 🟡 Latch not reset on the hard-reset path — FIXED: _contentRevertedThisSession clears alongside the other flags, keeping 'latch implies resident revert' local.

[claude]

Reviewed the current head. This is a focused change to the iOS Dart-side rollback path: public rollback() now quarantines the removed patch, while the OTA kill switch routes through a new _rollbackInternal(quarantine: false) that does not, and the resident-module flags stay truthful after a disk rollback. I read the whole _rollbackInternal / checkAndInstall loaded-session path and confirmed the core reasoning holds. (Note: an earlier self-review here flagged the missing @visibleForTesting seam and the un-reset latch on the hard-reset path, both now addressed at debugPersistOutcomeShowsRestart and code_push.dart:2118, so I am not re-raising them.)

🔴 Critical

None. The public rollback() signature (Future<void>) is preserved, so no API break. Every new disk/marker operation is inside try/catch (code_push.dart:1634-1645) or a _moduleLoaded guard, and the loaded-session persist branch sits inside the checkAndInstall try/catch/finally (642-787), so nothing here can throw out of the update path or brick the host app. The central insight (a loaded VM module cannot be unloaded, so _moduleLoaded/_loadedPatch* must keep telling the truth after a disk rollback, code_push.dart:1651-1675) is correct and closes the double-loadModule quarantine hole.

🟠 Medium

• The quarantine: false branch, the whole reason for the split, has no test (lib/src/code_push.dart:443, 1612-1646). The new test/ios_rollback_quarantine_test.dart only exercises the public rollback() (quarantine: true, marker written). The kill-switch contract this PR introduces is the opposite: _rollbackInternal(quarantine: false) must NOT write rolled_back_patch, so a later re-enable can resume with the same bytes. The test/ota_controls_test.dart patched-device case returns true from the engine rollback (ota_controls_test.dart:87), so it never reaches the Dart-side quarantine block and never touches a marker. Nothing asserts the no-marker behavior. Worth a test: ota_disabled + engine CodePush.rollback returns false + a patch file present on disk, then assert the patch file is removed and rolled_back_patch is NOT written. That is the one branch the PR exists to add, and it is currently unverified off-device.

🟡 Low

• _rollbackInternal deletes only the current _patchFilename, unlike _iosImmediateRollback (lib/src/code_push.dart:1647 vs 2095). The immediate-rollback path deletes both patch.bytecode and patch.vmcode to be safe across SDK upgrades; the rollback path deletes only the current-platform name. A legacy-named leftover from an older SDK would survive a rollback(). It is inert (the load path only reads the current name), so this is minor, but the two deletion paths reading differently is a latent inconsistency.
• Android deliberate-rollback quarantine is asserted by the docstring but unverifiable in this repo (lib/src/code_push.dart:1599-1605, 1618). On Android the engine returns true and _rollbackInternal returns before the quarantine block, so stick-iness relies entirely on the engine writing a rollback breadcrumb that _quarantineFromBreadcrumb later promotes. The engine lives outside this repo (the Kotlin plugin only handles getAppLibHash/getInstallerSource). If the engine writes that breadcrumb only on the three-strike auto-rollback and not on a deliberate CodePush.rollback, a deliberate rollback() on Android would not stick and the server could re-deliver the same patch. Worth a one-line confirmation from the native side, since the new doc now claims parity.

🟢 Positives

• The keep-_moduleLoaded-set-on-a-disk-rollback reasoning is the right call and is documented thoroughly at the point of change (code_push.dart:1651-1675), so future readers will understand why the flags stay set.
• Splitting deliberate rollback() (quarantine) from the kill-switch _rollbackInternal(quarantine: false) is the correct distinction, and the _contentRevertedThisSession latch (1667-1675, gate at 716-733) makes a same-session re-enable-with-same-patch bounce honest about needing a restart instead of silently converging.
• The quarantine identity is recorded before patch_info.json is deleted (1631-1646), avoiding the ordering trap, and the marker is written from patch_info via the same shape as the existing _iosImmediateRollback path so the two quarantine writers stay consistent.
• All new marker writes, reads, and deletes are wrapped so a rollback cannot throw on partial/corrupt on-disk state, and the PR adds the @visibleForTesting seams (debugPersistOutcomeShowsRestart, debugModuleLoadedForTesting) with tests covering the no-metadata and clean-device edge cases, not just the happy path.

[fonkamloic]

Round 2 triage: 🟠 the quarantine: false branch untested — FIXED in 621f12e with exactly the prescribed test: ota_disabled served from a loopback server + engine CodePush.rollback returning false + a patch on disk → asserts the patch file is removed AND rolled_back_patch is NOT written. Suite 142/142. (#21 merged meanwhile; will refresh this branch if the squash conflicts.)

[claude]

🔴 Critical

None. The public rollback() signature (Future<void>) is preserved — no API break. Every new disk/marker operation runs inside try/catch (lib/src/code_push.dart:1690-1706) or the if (_moduleLoaded) guard (1727-1735), and the loaded-session persist branch sits inside checkAndInstall's outer try/catch, so nothing added here can throw out of the update path or brick the host app. I re-traced the two contracts the split turns on and both hold:

• Kill-switch re-enable resumes with the same patch. _rollbackInternal(quarantine: false) deletes patch.bytecode/patch_info.json but leaves installed_patch_identity.json in place, which would let a stale identity block re-delivery — except _isPatchAlreadyInstalled is gated on the patch file still existing (1101), so after the file is deleted a re-offer of p1 is downloaded and re-persisted rather than short-circuited. Contract intact.
• Deliberate rollback() sticks. The quarantine marker is written from patch_info.json before it is deleted (1690-1709), via the same patch_id/patch_hash shape as _iosImmediateRollback/_iosCheckAndAutoRollback (2294-2300), and _isPatchQuarantined clears it automatically once a different patch is offered (1074-1076). The _contentRevertedThisSession latch only alters behavior in the persistSilently case (offer == resident module), reachable only via the non-quarantining kill-switch path — exactly where a restart genuinely restores content — so it never fires a spurious banner.

The central insight (a loaded VM module cannot be unloaded, so _moduleLoaded/_loadedPatch* must stay truthful after a disk rollback, 1711-1735) is correct and closes the double-loadModule quarantine hole.

🟠 Medium

None. The prior rounds' test gaps are closed: debugPersistOutcomeShowsRestart (3 tests) covers the silent-vs-banner rule, debugModuleLoadedForTesting proves the resident-flag invariant, and test/ios_rollback_quarantine_test.dart:87-120 drives the quarantine: false kill-switch branch end-to-end against a loopback server (patch removed, no marker written). Edge cases (no metadata, clean-device throw) are covered, not just the happy path.

🟡 Low

• _rollbackInternal deletes only the current _patchFilename, unlike the auto-rollback paths (lib/src/code_push.dart:1707 vs 2304-2310). On iOS _patchFilename is patch.bytecode (124-125); _iosCheckAndAutoRollback defensively deletes both patch.bytecode and the legacy patch.vmcode to cover a device upgraded from 0.1.10, but rollback()/kill-switch delete only the current name. A legacy leftover would survive a deliberate rollback(). It is inert (the load path reads only _patchFilename), so this is a latent inconsistency rather than a bug — raised in a prior round and still open; worth a one-line unify.
• installed_patch_identity.json outlives _rollbackInternal. Harmless today thanks to the patch-file gate above, but the identity file surviving a rollback that removed its patch is the kind of coupling that breaks quietly if that gate ever moves. A comment at the delete site noting the reliance would help.

🟢 Positives

• The keep-_moduleLoaded-set-on-a-disk-rollback reasoning is the right call and is documented thoroughly at the point of change (1711-1735), so future readers understand why the flags stay set.
• Splitting deliberate rollback() (quarantine) from the kill-switch _rollbackInternal(quarantine: false) is the correct distinction, and the _contentRevertedThisSession latch makes a same-session re-enable-with-same-patch bounce honest about needing a restart instead of silently converging.
• Quarantine identity is recorded before patch_info.json is deleted (1690-1709), avoiding the ordering trap, and it reuses the exact marker shape as the existing quarantine writers so all three stay consistent.
• All new marker writes/reads/deletes are wrapped so a rollback cannot throw on partial or corrupt on-disk state, and the PR adds proper @visibleForTesting seams with off-device coverage rather than leaving the new branch device-only.

[fonkamloic]

Final round triage: both 🟡 Lows FIXED in e3c2001 — the deliberate-rollback delete now sweeps both filenames (mirroring the auto-rollback paths), and the identity-file survival is documented at the delete site with its gate reliance. Merging on green.

[claude]

🔴 Critical

None. The public rollback() signature (Future<void>) is preserved, so no API break. The split into rollback() (quarantine: true) and the kill-switch _rollbackInternal(quarantine: false) is correct, and both contracts hold on a re-trace:

• Kill-switch re-enable resumes with the same patch. _rollbackInternal(quarantine: false) writes no rolled_back_patch marker, and although installed_patch_identity.json survives the delete, _isPatchAlreadyInstalled is gated on the patch file still existing (lib/src/code_push.dart:1101), so after the bytes are gone a re-offer is re-downloaded rather than short-circuited.
• Deliberate rollback() sticks. The quarantine identity is captured from patch_info.json before it is deleted (1690-1706) and later blocks re-delivery via _isPatchQuarantined (1064-1084), which auto-clears once a different patch is offered.

Every new disk/marker operation is inside try/catch (1694-1705) or the if (_moduleLoaded) guard (1739-1747), and the loaded-session persist branch sits inside checkAndInstall's outer try/catch, so nothing added here can throw out of the update path or brick the host app. The central insight — a loaded VM module cannot be unloaded, so _moduleLoaded/_loadedPatch* must stay truthful after a disk rollback (1723-1747) — is correct and closes the double-loadModule quarantine hole.

🟠 Medium

None. The test gaps flagged in earlier rounds are closed: debugPersistOutcomeShowsRestart (3 tests) covers the silent-vs-banner rule, debugModuleLoadedForTesting proves the resident-flag invariant survives a disk rollback, and test/ios_rollback_quarantine_test.dart drives both the quarantining rollback() and the non-quarantining kill-switch branch (loopback server, engine CodePush.rollback -> false, patch removed, no marker) end-to-end, plus the no-metadata and clean-device edge cases.

🟡 Low

• The legacy-file sweep is defeated by its own existence guard (lib/src/code_push.dart:1686-1688 vs 1711-1714). Commit e3c2001 made the delete sweep both patch.bytecode and patch.vmcode so "a device upgraded from an old SDK can't keep a stale artifact." But the early-return guard at 1687 only checks _patchFilename (the current name — patch.bytecode on iOS). So on the exact scenario the sweep targets — a device carrying only the legacy patch.vmcode and no current-name file — rollback() throws No active patch to roll back at 1688 and the loop never runs, leaving the legacy file behind. This is inert (the iOS load path reads only _patchFilename, so a legacy leftover is never loaded), so it's a Low, but the guard and the sweep now read the two filenames inconsistently, which undercuts the stated goal of the sweep. Gating the guard on either name existing would make it do what its comment claims.
• installed_patch_identity.json surviving _rollbackInternal is load-bearing but coupled to a gate elsewhere. The delete-site comment (1717-1721) correctly documents the reliance on the _isPatchAlreadyInstalled file-exists gate at 1101; keeping that comment in sync if the gate ever moves is the only maintenance risk. Noted, not blocking.

🟢 Positives

• The keep-_moduleLoaded-set-on-a-disk-rollback reasoning is the right call and is documented thoroughly at the point of change (1723-1747), so future readers understand why the flags stay set.
• Splitting deliberate rollback() (quarantine) from the kill-switch path is the correct distinction, and the _contentRevertedThisSession latch makes a same-session re-enable-with-same-patch bounce honest about needing a restart instead of silently converging — while only ever firing in the persistSilently case reachable via the kill-switch, so it never raises a spurious banner.
• Quarantine identity is recorded before patch_info.json is deleted, avoiding the ordering trap, and reuses the exact {patch_id, patch_hash} marker shape as the existing quarantine writers (_iosCheckAndAutoRollback) so all three stay consistent, with matching gated on id-OR-hash via the shared _identityMatches.
• All new marker writes/reads/deletes are wrapped so a rollback cannot throw on partial or corrupt on-disk state, and the PR adds proper @visibleForTesting seams with off-device coverage of the new branch rather than leaving it device-only.