Making rule engine execution order more deterministic

src/main/clojure/clara/rules/compiler.clj

@@ -1499,9 +1499,12 @@
                                ;; Internal system types always use Clojure's type mechanism.
                                (type fact)
                                ;; All other types defer to the provided function.
-                               (fact-type-fn fact))))]
+                               (fact-type-fn fact))))
+
+        alpha-roots (get merged-rules :alpha-roots)]
+
     (fn [facts]
-      (for [[fact-type facts] (platform/tuned-group-by fact-grouping-fn facts)]
+      (for [[fact-type facts] (platform/group-by-seq fact-grouping-fn facts)]
 
         (if-let [alpha-nodes (get @alpha-map fact-type)]
 
@@ -1511,15 +1514,16 @@
           ;; The alpha nodes weren't cached for the type, so get them now.
           (let [ancestors (conj (ancestors-fn fact-type) fact-type)
 
-                ;; Get all alpha nodes for all ancestors.
-                new-nodes (distinct
-                           (reduce
-                            (fn [coll ancestor]
-                              (concat
-                               coll
-                               (get-in merged-rules [:alpha-roots ancestor])))
-                            []
-                            ancestors))]
+                ;; Get all alpha nodes for all ancestors.  Keep them sorted to maintain
+                ;; deterministic ordering of fact propagation across the network.
+                ;; Alpha nodes do not have a :node-id of their own right now, so sort
+                ;; by the :node-id of their :children.
+                new-nodes (sort-by #(mapv :node-id (:children %))
+                                   (into []
+                                         (comp (map #(get alpha-roots %))
+                                               cat
+                                               (distinct))
+                                         ancestors))]
 
             (swap! alpha-map assoc fact-type new-nodes)
             [new-nodes facts]))))))

src/main/clojure/clara/rules/engine.cljc

@@ -104,7 +104,7 @@
     (if (pos? (count join-keys))
 
       ;; Group by the join keys for the activation.
-      (doseq [[join-bindings item-group] (platform/tuned-group-by #(select-keys (:bindings %) join-keys) items)]
+      (doseq [[join-bindings item-group] (platform/group-by-seq #(select-keys (:bindings %) join-keys) items)]
         (propagate-fn node
                       join-bindings
                       item-group
@@ -805,7 +805,7 @@
   IAccumRightActivate
   (pre-reduce [node elements]
     ;; Return a seq tuples with the form [binding-group facts-from-group-elements].
-    (for [[bindings element-group] (platform/tuned-group-by :bindings elements)]
+    (for [[bindings element-group] (platform/group-by-seq :bindings elements)]
       [bindings (mapv :fact element-group)]))
 
   (right-activate-reduced [node join-bindings fact-seq memory transport listener]
@@ -948,7 +948,7 @@
     (doseq [:let [convert-return-fn (:convert-return-fn accumulator)
                   matched-tokens (mem/get-tokens memory node join-bindings)
                   has-matches? (seq matched-tokens)]
-            [bindings elements] (platform/tuned-group-by :bindings elements)
+            [bindings elements] (platform/group-by-seq :bindings elements)
 
             :let [previous (mem/get-accum-reduced memory node join-bindings bindings)
                   has-previous? (not= :clara.rules.memory/no-accum-reduced previous)
@@ -1130,7 +1130,7 @@
     ;; Return a map of bindings to the candidate facts that match them. This accumulator
     ;; depends on the values from parent facts, so we defer actually running the accumulator
     ;; until we have a token.
-    (for [[bindings element-group] (platform/tuned-group-by :bindings elements)]
+    (for [[bindings element-group] (platform/group-by-seq :bindings elements)]
       [bindings (map :fact element-group)]))
 
   (right-activate-reduced [node join-bindings binding-candidates-seq memory transport listener]
@@ -1220,7 +1220,7 @@
 
     (doseq [:let [convert-return-fn (:convert-return-fn accumulator)
                   matched-tokens (mem/get-tokens memory node join-bindings)]
-            [bindings elements] (platform/tuned-group-by :bindings elements)
+            [bindings elements] (platform/group-by-seq :bindings elements)
             :let [previous-candidates (mem/get-accum-reduced memory node join-bindings bindings)]
 
             ;; No need to retract anything if there was no previous item.

src/main/clojure/clara/rules/platform.cljc

@@ -7,6 +7,37 @@
   [^String description]
   (throw #?(:clj (IllegalArgumentException. description) :cljs (js/Error. description))))
 
+#?(:clj
+   (defn group-by-seq
+     "Groups the items of the given coll by f to each item.  Returns a seq of tuples of the form 
+      [f-val xs] where xs are items from the coll and f-val is the result of applying f to any of 
+      those xs.  Each x in xs has the same value (f x).  xs will be in the same order as they were
+      found in coll.
+      The behavior is similar to calling `(seq (group-by f coll))` However, the returned seq will
+      always have consistent ordering from process to process.  The ordering is insertion order
+      as new (f x) values are found traversing the given coll collection in its seq order.  The
+      returned order is made consistent to ensure that relevant places within the rules engine that
+      use this grouping logic have deterministic behavior across different processes."
+     [f coll]
+     (let [^java.util.Map m (reduce (fn [^java.util.Map m x]
+                                      (let [k (f x)
+                                            xs (or (.get m k)
+                                                   (transient []))]
+                                        (.put m k (conj! xs x)))
+                                      m)
+                                    (java.util.LinkedHashMap.)
+                                    coll)
+           it (.iterator (.entrySet m))]
+       ;; Explicitly iterate over a Java iterator in order to avoid running into issues as
+       ;; discussed in http://dev.clojure.org/jira/browse/CLJ-1738
+       (loop [coll (transient [])]
+         (if (.hasNext it)
+           (let [^java.util.Map$Entry e (.next it)]
+             (recur (conj! coll [(.getKey e) (persistent! (.getValue e))])))
+           (persistent! coll)))))
+   :cljs
+   (def group-by-seq (comp seq clojure.core/group-by)))
+
 #?(:clj
     (defn tuned-group-by
       "Equivalent of the built-in group-by, but tuned for when there are many values per key."

src/test/clojure/clara/test_rules.clj

@@ -4419,6 +4419,47 @@
 
     (reset! fire-order [])))
 
+(deftest test-rule-order-respected-by-batched-inserts
+  (let [qholder (atom [])
+
+        r1 (dsl/parse-rule [[Temperature (= ?t temperature)]]
+                           (insert! (->Cold ?t)))
+        r2 (dsl/parse-rule [[Temperature (= ?t temperature)]]
+                           (insert! (->Hot ?t)))
+
+        ;; Make two "alpha roots" that the 2 rules above insertions will need to propagate to.
+        q1 (dsl/parse-query [] [[?c <- Cold  (swap! qholder conj :cold)]])
+        q2 (dsl/parse-query [] [[?h <- Hot (swap! qholder conj :hot)]])
+
+        order1 (mk-session [r1 r2 q1 q2] :cache false)
+        order2 (mk-session [r2 r1 q1 q2] :cache false)
+
+        run-session (fn [s]
+                      (let [s (-> s
+                                  (insert (->Temperature 10 "MCI")) 
+                                  fire-rules)]
+                        [(-> s (query q1) frequencies)
+                         (-> s (query q2) frequencies)]))
+
+        [res11 res12] (run-session order1)
+        holder1 @qholder
+        _ (reset! qholder [])
+
+        [res21 res22] (run-session order2)
+        holder2 @qholder
+        _ (reset! qholder [])]
+
+    ;; Sanity check that the query matches what is expected.
+    (is (= (frequencies [{:?c (->Cold 10)}])
+           res11
+           res21))
+    (is (= (frequencies [{:?h (->Hot 10)}])
+           res12
+           res22))
+
+    (is (= [:cold :hot] holder1))
+    (is (= [:hot :cold] holder2))))
+
 (deftest test-force-multiple-transient-transitions-activation-memory
   ;; The objective of this test is to verify that activation memory works
   ;; properly after going through persistent/transient shifts, including shifts