Handle duplicate child key errors more gracefully.

formula/src/main/java/com/instacart/formula/FormulaPlugins.kt

@@ -18,4 +18,12 @@ object FormulaPlugins {
             else -> ListInspector(listOf(global, local))
         }
     }
+
+    fun onDuplicateChildKey(
+        parentFormulaType: Class<*>,
+        childFormulaType: Class<*>,
+        key: Any,
+    ) {
+       plugin?.onDuplicateChildKey(parentFormulaType, childFormulaType, key)
+    }
 }

formula/src/main/java/com/instacart/formula/Plugin.kt

@@ -12,4 +12,13 @@ interface Plugin {
     fun inspector(type: KClass<*>): Inspector? {
         return null
     }
+
+    /**
+     * Notified when there is a duplicate child key detected.
+     */
+    fun onDuplicateChildKey(
+        parentType: Class<*>,
+        childFormulaType: Class<*>,
+        key: Any,
+    ) = Unit
 }

formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt

@@ -1,7 +1,9 @@
 package com.instacart.formula.internal
 
+import com.instacart.formula.FormulaPlugins
 import com.instacart.formula.IFormula
 import com.instacart.formula.Inspector
+import java.lang.IllegalStateException
 
 /**
  * Keeps track of child formula managers.
@@ -11,14 +13,19 @@ internal class ChildrenManager(
     private val inspector: Inspector?,
 ) {
     private var children: SingleRequestMap<Any, FormulaManager<*, *>>? = null
+    private var indexes: MutableMap<Any, Int>? = null
     private var pendingRemoval: MutableList<FormulaManager<*, *>>? = null
 
+    private var duplicateKeyLogs: MutableSet<Any>? = null
+
     /**
      * After evaluation, we iterate over detached child formulas, mark them as terminated
      * and add them to [pendingRemoval] list. The work to clean them up will be performed
      * in post evaluation, which will call [terminateChildren] function.
      */
     fun prepareForPostEvaluation() {
+        indexes?.clear()
+
         children?.clearUnrequested {
             pendingRemoval = pendingRemoval ?: mutableListOf()
             it.markAsTerminated()
@@ -51,20 +58,82 @@ internal class ChildrenManager(
         formula: IFormula<ChildInput, ChildOutput>,
         input: ChildInput,
     ): FormulaManager<ChildInput, ChildOutput> {
+        val childHolder = childFormulaHolder(key, formula, input)
+        return if (childHolder.requested) {
+            val logs = duplicateKeyLogs ?: run {
+                val newSet = mutableSetOf<Any>()
+                duplicateKeyLogs = newSet
+                newSet
+            }
+
+            /**
+             * Since child key was already requested, we use a fallback mechanism that
+             * uses index increment to handle collisions. This is better than crashing, but
+             * can lead to subtle bugs when content shifts. We notify the global listener
+             * so it can be logged and fixed by defining explicit key.
+             */
+            if (logs.add(key)) {
+                FormulaPlugins.onDuplicateChildKey(
+                    parentFormulaType = delegate.loggingType.java,
+                    childFormulaType = formula.type().java,
+                    key = key,
+                )
+            }
+
+            if (key is IndexedKey) {
+                // This should never happen, but added as safety
+                throw IllegalStateException("Key already indexed (and still duplicate).")
+            }
+
+            val index = nextIndex(key)
+            val indexedKey = IndexedKey(key, index)
+            findOrInitChild(indexedKey, formula, input)
+        } else {
+            childHolder.requestAccess {
+                "There already is a child with same key: $key. Override [Formula.key] function."
+            }
+        }
+    }
+
+    private fun <ChildInput, ChildOutput>  childFormulaHolder(
+        key: Any,
+        formula: IFormula<ChildInput, ChildOutput>,
+        input: ChildInput,
+    ): SingleRequestHolder<FormulaManager<ChildInput, ChildOutput>> {
         @Suppress("UNCHECKED_CAST")
         val children = children ?: run {
             val initialized: SingleRequestMap<Any, FormulaManager<*, *>> = LinkedHashMap()
             this.children = initialized
             initialized
         }
 
-        return children
-            .findOrInit(key) {
-                val implementation = formula.implementation()
-                FormulaManagerImpl(delegate, implementation, input, loggingType = formula::class, inspector = inspector)
-            }
-            .requestAccess {
-                "There already is a child with same key: $key. Override [Formula.key] function."
-            } as FormulaManager<ChildInput, ChildOutput>
+        val childFormulaHolder = children.findOrInit(key) {
+            val implementation = formula.implementation()
+            FormulaManagerImpl(
+                delegate,
+                implementation,
+                input,
+                loggingType = formula::class,
+                inspector = inspector
+            )
+        }
+        @Suppress("UNCHECKED_CAST")
+        return childFormulaHolder as SingleRequestHolder<FormulaManager<ChildInput, ChildOutput>>
+    }
+
+    /**
+     * Function which returns next index for a given key. It will
+     * mutate the [indexes] map.
+     */
+    private fun nextIndex(key: Any): Int {
+        val indexes = indexes ?: run {
+            val initialized = mutableMapOf<Any, Int>()
+            this.indexes = initialized
+            initialized
+        }
+
+        val index = indexes.getOrElse(key) { 0 } + 1
+        indexes[key] = index
+        return index
     }
 }

formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt

@@ -21,7 +21,7 @@ internal class FormulaManagerImpl<Input, State, Output>(
     private val delegate: ManagerDelegate,
     private val formula: Formula<Input, State, Output>,
     initialInput: Input,
-    private val loggingType: KClass<*>,
+    internal val loggingType: KClass<*>,
     private val listeners: Listeners = Listeners(),
     private val inspector: Inspector?,
 ) : FormulaManager<Input, Output>, ManagerDelegate {

formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt

@@ -4,6 +4,7 @@ import com.google.common.truth.Truth
 import com.google.common.truth.Truth.assertThat
 import com.instacart.formula.actions.EmptyAction
 import com.instacart.formula.internal.ClearPluginsRule
+import com.instacart.formula.internal.FormulaKey
 import com.instacart.formula.internal.TestInspector
 import com.instacart.formula.internal.Try
 import com.instacart.formula.rxjava3.RxAction
@@ -29,6 +30,7 @@ import com.instacart.formula.subjects.FromObservableWithInputFormula
 import com.instacart.formula.subjects.HasChildFormula
 import com.instacart.formula.subjects.MultiChildIndirectStateChangeRobot
 import com.instacart.formula.subjects.InputChangeWhileFormulaRunningRobot
+import com.instacart.formula.subjects.KeyFormula
 import com.instacart.formula.subjects.KeyUsingListFormula
 import com.instacart.formula.subjects.MessageFormula
 import com.instacart.formula.subjects.MixingCallbackUseWithKeyUse
@@ -1099,16 +1101,35 @@ class FormulaRuntimeTest(val runtime: TestableRuntime, val name: String) {
             }
     }
 
-    @Test fun `adding duplicate child throws an exception`() {
+    @Test fun `adding duplicate child logs global event`() {
+        val duplicateKeys = mutableListOf<Any>()
+
+        FormulaPlugins.setPlugin(object : Plugin {
+            override fun onDuplicateChildKey(
+                parentType: Class<*>,
+                childFormulaType: Class<*>,
+                key: Any
+            ) {
+                duplicateKeys.add(key)
+            }
+        })
+
         val result = Try {
             val formula = DynamicParentFormula()
             runtime.test(formula, Unit)
                 .output { addChild(TestKey("1")) }
                 .output { addChild(TestKey("1")) }
         }
 
+        // No errors
         val error = result.errorOrNull()?.cause
-        assertThat(error).isInstanceOf(IllegalStateException::class.java)
+        assertThat(error).isNull()
+
+        // Should log only once
+        assertThat(duplicateKeys).hasSize(1)
+        assertThat(duplicateKeys).containsExactly(
+            FormulaKey(null, KeyFormula::class.java, TestKey("1"))
+        )
     }
 
     @Test