feat(ios): report the ABI-mismatch reload skip to telemetry

[fonkamloic]

Follow-up tabled on #19, approved for this batch.

Devices that skip the cold-start reload because the installed patch was built for a different engine ABI sat on baseline invisibly. The skip now posts a client-error report — kind: incompatible_reload, stored vs live fingerprints — so fleet-wide 'engine changed under installed patches' is observable.

• Fire-and-forget (unawaited): the boot path never waits on telemetry; the helper swallows every error.
• Latched once per process: init() can legally run more than once (re-init on resume) and each run re-enters the reload — without the latch every re-init would re-POST the same stranding event. (Adversarial-review finding; the original comment claimed per-launch bounding that re-inits would have broken.)
• No server change: the client-error endpoint already stores structured kind/reason reports; _reportIncompatibleBaseline gained an optional kind param defaulting to the existing value, all current callers unchanged.

Suite 134/134.

[claude]

🔴 Critical

None. The change is additive and failure-soft: the new kind param (lib/src/code_push.dart:856) defaults to the existing incompatible_baseline, so the three existing callers (:532, :553, :588) are unchanged — no public-API or server-contract break — and the report is fired unawaited with all errors swallowed, so the boot path can neither block nor crash on it.

🟠 Medium

• Boot-time-offline devices lose the report permanently (lib/src/code_push.dart:1880-1893). The latch is set before the fire-and-forget POST resolves:

  if (_reportedIncompatibleReload) return;
  _reportedIncompatibleReload = true;       // set unconditionally
  unawaited(_reportIncompatibleBaseline(...)); // return value discarded

  If the first reload happens while the device is offline, the POST fails silently but the latch stays true for the rest of the process, so no later re-init retries. This diverges from the sibling dedup a few lines up (:588-599), which gates its marker on a successful round trip (if (reported) _reportedBaselineMismatches.add(...)) precisely so offline devices retry. Net effect: the "device sits on baseline invisibly" gap this PR sets out to close stays open for devices that boot offline. Consider awaiting the result and only latching on true — the helper already returns transport success/failure.
• No test coverage for the new behavior. The analogous baseline-mismatch dedup is tested end-to-end (test/baseline_hash_android_test.dart:152-173 counts telemetry POSTs and asserts a second check does not re-POST), but neither the incompatible_reload kind nor the reload latch is exercised. The reload path is gated behind if (!Platform.isIOS ...) return; (:1827) with no host-testable seam, so a regression in the latch or the emitted kind/fingerprints would pass the suite silently. A debug seam over the gate branch (like the existing debugDecideReloadGate) would make the latch and payload assertable.

🟡 Low

• The latch is a process-global bool, whereas the sibling path keys dedup on the specific mismatch pair (_reportedBaselineMismatches). Keying on the stored-vs-live ABI pair would be consistent and would still allow a report if the engine changes a second time within one process — minor, given how rare in-process engine changes are.

🟢 Positives

• Backward-compatible by construction: the optional kind defaulting to the old value means zero changes to existing callers and no server-side change, exactly as the description claims.
• Correct failure-soft placement: the latch check sits after the idempotent _iosResetBootCounter and status.value update (:1871-1873), so dedup only suppresses the telemetry POST — never the actual baseline-recovery behavior.
• Fingerprint semantics match the other callers: expected = the ABI the patch was built for (stored), actual = the live engine ABI — consistent and correct.
• Good use of unawaited (already imported) plus the helper's blanket error swallow keeps the cold-start path off telemetry's critical path.

[fonkamloic]

Triage round 1

• 🟠 Offline boot loses the report — FIXED in 0a2958e: extracted into debugReportIncompatibleReload, latched on DELIVERY — set optimistically (no duplicate in-flight posts), cleared again when the POST never completed an HTTP round trip, so a later re-init retries. Mirrors the sibling baseline-mismatch dedup's delivered-only recording.
• 🟠 No test coverage — FIXED in the same commit: 3 tests against a loopback capture server assert the payload shape (kind: incompatible_reload, stored/live fingerprints), the delivered-latch, and the offline-retry.
• 🟡 Pair-keyed dedup — REFUTED: an engine ABI cannot change within a running process (the fingerprint comes from the loaded engine binary itself), so there is no second distinct pair to report before the next cold start; a process-scoped bool is exactly sufficient. Documented on the helper.

[claude]

🔴 Critical

None. The change is additive and failure-soft. The new kind param defaults to 'incompatible_baseline' (lib/src/code_push.dart:893), so the three existing callers (:569, :590, :625) and the server contract are unchanged. The report is fired unawaited from the reload path (:1906-1918) through a helper that swallows every error, so telemetry can neither block nor crash cold start.

I also confirmed the two Mediums from the prior automated review are genuinely fixed in 0a2958e, not just claimed:

• Offline-boot retry works: debugReportIncompatibleReload (:199-209) sets the latch optimistically, then if (!delivered) _reportedIncompatibleReload = false; clears it on transport failure. _reportIncompatibleBaseline returns true on any completed round trip and false only on transport failure (:882-928), so an offline boot retries on a later re-init. Mirrors the sibling _reportedBaselineMismatches gate (:624-636).
• Payload/latch coverage is now exercised by test/ios_reload_telemetry_test.dart against a loopback server (kind, fingerprints, delivered-latch, offline-retry).

🟠 Medium

• The reload→report wiring itself is still untested (lib/src/code_push.dart:1902-1918). The new tests call debugReportIncompatibleReload directly, which validates the helper, but nothing exercises _iosReloadInstalledPatch's skipIncompatibleAbi branch. A regression that swapped the arguments — e.g. passing liveAbi as storedAbi, or reading the wrong key for patchId — would ship green, since the emitted expected/actual fingerprints are only asserted against literals the test itself supplies. debugDecideReloadGate is already a host-testable seam; a test that drives the gate to skipIncompatibleAbi and asserts the resulting POST would close this. Reasonable to track as a follow-up rather than block.

🟡 Low

• debug-prefixed, @visibleForTesting method on the production path (:189-209, called at :1907). debugReportIncompatibleReload is annotated @visibleForTesting yet is the only production trigger for the report. Same-file use avoids an analyzer warning, but the name and annotation both signal "test-only seam," which is misleading for a method the shipping reload path depends on. Consider a private _reportIncompatibleReload holding the latch logic that both production and a thin @visibleForTesting wrapper call — matching how the sibling _reportIncompatibleBaseline is structured.

🟢 Positives

• Backward-compatible by construction: the optional kind defaulting to the old literal means zero changes to existing callers and no server-side change.
• Delivery-latch semantics are correct and match the sibling baseline-mismatch dedup — latched on a completed round trip (any status), cleared only on transport failure — so a persistently-refusing endpoint isn't turned into a per-boot beacon while genuinely-offline devices still retry.
• Failure-soft placement is right: the report sits after _iosResetBootCounter and the status.value update (:1901-1904), so dedup only ever suppresses the telemetry POST, never the baseline-recovery behavior.
• The check-then-set at the top of the latch (:200-201) is synchronous with no await between, so concurrent re-entrant reloads can't double-POST.
• Fingerprint direction is consistent with the other callers: expected = stored ABI the patch was built for, actual = live engine ABI. The gate guarantees storedAbi is non-null and non-unknown before skipIncompatibleAbi, so the report always carries a real expected fingerprint.

[fonkamloic]

Round 2 triage: 🟠 reload→report wiring test — TABLED as the reviewer suggests (tracked into the follow-up bundle; the gate decision and the report payload/latch are each covered, the untested residue is the argument plumbing between two tested seams). 🟡 naming — FIXED in 85e63bf: production trigger is now private _reportIncompatibleReload; the debug method is a thin test wrapper, matching the sibling reporter's structure.

[claude]

Reviewed the current head (85e63bf) against the two prior review rounds. I re-read the full _iosReloadInstalledPatch (lib/src/code_push.dart:1878-1942), the helper, and the new test rather than trusting the triage notes. The prior Mediums (offline-retry, payload coverage) and the naming Low are genuinely fixed in the tree, not just claimed.

🔴 Critical

None. The change is additive and failure-soft. The new kind param defaults to 'incompatible_baseline' (lib/src/code_push.dart:911), so the three existing callers (:587, :608, and the sibling at :625+) and the server contract are unchanged. The report is fired unawaited from the reload path (:1933) through a helper that swallows every error (:942-945), so telemetry can neither block nor crash cold start. I verified the argument plumbing by hand: the report's storedAbi/liveAbi at :1938-1939 are the same values fed to the gate at :1914-1915 (info?['engine_abi']?.toString() and the probed liveAbi), so expected=stored / actual=live is correct — not swapped.

🟠 Medium

None that block. The one open item — the skipIncompatibleAbi branch's wiring is exercised only indirectly (tests call debugReportIncompatibleReload directly, not through the gate) — was already tabled by author/reviewer agreement into the follow-up bundle. I'm not re-raising it: I read both sides of the plumbing and confirmed the arguments are correct, so the untested residue is genuinely just the two-line hand-off between two already-tested seams. Reasonable to track.

🟡 Low

• Dead-port retry test is theoretically flaky (test/ios_reload_telemetry_test.dart:68-72). It binds a server, records the port, closes it, then POSTs expecting connection-refused. The port could be reused by another process between close and connect. Extremely unlikely on a loopback CI runner, but a deterministic stand-in (an HttpOverrides that throws, or an unroutable address) would remove the last drop of nondeterminism.
• HttpOverrides.global = null is set at file scope in main() (test/ios_reload_telemetry_test.dart:16). Fine because each Dart test file runs in its own isolate, but a short comment on why it is safe to null the binding's stub globally (rather than scoping it per-test) would help the next maintainer who copies this pattern.

🟢 Positives

• Backward-compatible by construction: the optional kind defaulting to the old literal means zero changes to existing callers and no server-side change.
• Delivery-latch semantics are correct and mirror the sibling baseline-mismatch dedup — latched on any completed HTTP round trip, cleared only on transport failure (lib/src/code_push.dart:206). A persistently-refusing endpoint isn't turned into a per-boot beacon, while genuinely-offline devices still retry on a later re-init. The offline-retry path is now covered end-to-end against a loopback server.
• The check-then-set at the top of the latch (:196-197) is synchronous with no await between, so concurrent re-entrant reloads can't double-POST.
• Failure-soft placement is right: the report sits after _iosResetBootCounter and the status.value update (:1926-1928), so dedup only ever suppresses the telemetry POST — never the baseline-recovery behavior.
• The Low naming feedback from round 2 was actually applied: the production trigger is the private _reportIncompatibleReload and debug… is a thin @visibleForTesting wrapper, matching the sibling reporter's structure.