md2link
Sign in

In-App Ad Flow Feature Implementation

DraftApr 22, 2026

In-App Ad Flow Feature Implementation

Date: 2026-04-22 Severity: Medium (post-review bugs would have been High) Component: com.apero.monetize.d7.inapp — InAppAdManager, Pool, Loader, Reloader, RemoteConfig, LayoutSelector Status: Resolved

What Happened

Full implementation of the In-App Ad Flow feature: partner-facing InAppAdManager with string-keyed touchpoints, waterfall loader (native_full_2ID → inter_preparing), 3 selectable layouts via remote config, and lifecycle reload with a 3s min-age guard.

Cycle was brainstorm → 5-phase plan → implement → adversarial code review → fix → commit. Commits: 7ce607a 66907e2 4fa142c b1a8706 f881858 2904dc1

The Brutal Truth

The initial implementation passed all individual phase specs but contained 6 correctness bugs — none caught during implementation, all caught by code review. The structure looked right. It wasn't. That gap between "looks structured" and "actually correct" is the recurring failure mode here.

Technical Details

Six bugs surfaced during adversarial review:

  1. HashMap race in InAppAdPool — per-touchpoint mutex protected intra-touchpoint calls but concurrent cross-touchpoint writes to the underlying HashMap could corrupt. Fixed: ConcurrentHashMap.

  2. Firebase keys never synced — RemoteFOTemplate1Configuration.sync() only writes pre-registered typed RemoteKeys objects to SharedPreferences. Dynamic per-touchpoint keys (na_fu_in_home_2ID, etc.) were generated but never registered, so sync() silently skipped them. The feature would have run on hardcoded defaults forever. Fixed: added saveInAppKeys(touchpoints) iterating all 5 key patterns per touchpoint before sync().

  3. Pool mutex bypass in show() — the load-and-insert path on pool miss called loader.load + pool.put directly, bypassing putIfAbsent deduplication. Concurrent show() calls on the same touchpoint could double-load. Fixed: unified through putIfAbsent.

  4. ApInterstitialAd branch missed onAdShown(tp) — the 3s reload guard only started on the native branch. Interstitial fallback ads never triggered lifecycle reload after dismiss. Fixed: onAdShown call added before forceShowInterstitial.

  5. native2IdEnabled flag never read — InAppRemoteConfig.native2IdEnabled(tp) was defined and wired but the loader never consulted it. Dual-ID native load was always attempted regardless of the remote flag. Fixed: loader gates native path on the flag.

  6. initOnce TOCTOU race — @Volatile var initialized + check-then-write is not atomic. Double observer registration was possible under concurrent init() calls. Fixed: AtomicBoolean.compareAndSet.

What We Tried

Initial implementation followed the 5-phase plan without a systemic correctness pass between phases. Each phase was reviewed in isolation for spec compliance. That is insufficient.

Root Cause Analysis

Phase-by-phase implementation without a cross-cutting correctness review. Concurrency, flag enforcement, and Firebase sync behavior are not visible when reviewing individual component specs — they only surface when someone asks "what happens when two threads hit this simultaneously" or "does this config value ever actually reach the user."

The Firebase key bug is the worst of the six. It required knowing an internal SDK invariant (sync() only writes registered keys) that is not documented anywhere in the codebase. Found only because the reviewer asked "how do dynamic keys reach SharedPreferences?" — the answer turned out to be "they don't."

Lessons Learned

  • Run adversarial review before committing, not after. The current cycle caught bugs at the right gate but the mental model of "review = polish" still needs to become "review = correctness blocker."
  • Concurrency bugs don't announce themselves. Any mutable shared structure needs explicit reasoning about cross-thread access, not just per-caller reasoning.
  • SDK internal invariants must be validated by reading source, not by assumption. sync() behavior was assumed to be "write all keys." It wasn't.
  • Flag wiring needs an end-to-end trace, not just a presence check. Defined + exposed + unused is indistinguishable from a compile-time perspective.

Next Steps

  • Confirm tracking event IDs with PM against Confluence Google Sheet — current strings are brainstorm-derived, not validated.
  • Add click-event hook on the interstitial path (AperoAdCallback has no click surface today).
  • Investigate whether saveInAppKeys() in initOnce() races Firebase fetch/activate at cold start — likely fine given pre-existing pattern but worth a timed test.

Open Threads

  • APERO_API_KEY hardcoded in app/src/main/java/com/apero/flow/d7/AppApplication.kt — pre-existing since 33bd8c4, not introduced this session. Real credential exposure. Flag to owner for rotation and env-var migration before any public repo access.
  • clicked event only tracked on native path; no click hook available for inter path via AperoAdCallback.
  • saveInAppKeys may execute before Firebase remote config fetch completes at cold start — matches pre-existing sync pattern but not tested under network delay.
  • InAppAdLoader hardcodes R.layout.fo_native_fullscreen as default; LayoutSelector overrides via customActivityClass so it works, but the layout ID is not parameterized.
  • Plans directory is .gitignore'd — integration guide and phase docs are local only. Decision pending on whether plans should be version-controlled.