From 8ee4f308a47ea613a97493b8afbe1aed02f80036 Mon Sep 17 00:00:00 2001
From: Ben Busby <noreply+git@benbusby.com>
Date: Wed, 10 Nov 2021 12:19:37 -0700
Subject: [PATCH] Prevent same instance from being selected twice in a row

Introduces a new db key "<service>-previous" to track which instance was
last selected for a particular service. This allows for filtering the
list of available instances to exclude the instance that was last
picked, to ensure a (slightly) more even distribution of traffic.
There's still the possiblity of the following scenario, however:

:service instances > 2

/:service request #1 -> instance #1
/:service request #2 -> instance #2
/:service request #3 -> instance #1
/:service request #4 -> instance #2

where there are many ignored instances for a particular service. One
possible solution would be to implement the "<service>-previous" value
to be a list, rather than a single value, and push to that list until
only one element is left in the original "instance" array after
filtering, and then delete the "<service>-previous" key.
---
 config/config.exs     |  3 ++-
 lib/farside.ex        | 29 ++++++++++++++++++++++++++---
 test/farside_test.exs | 15 ++++++++++++++-
 update.exs            |  6 +++---
 4 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/config/config.exs b/config/config.exs
index 3b9dfe6..c522a9f 100644
--- a/config/config.exs
+++ b/config/config.exs
@@ -2,8 +2,9 @@ import Config
 
 config :farside,
   redis_conn: "redis://localhost:6379",
-  fallback_str: "-fallback",
   update_file: ".update-results",
   service_prefix: "service-",
+  fallback_suffix: "-fallback",
+  previous_suffix: "-previous",
   services_json: "services.json",
   index: "index.eex"
diff --git a/lib/farside.ex b/lib/farside.ex
index 0595e33..ea574fa 100644
--- a/lib/farside.ex
+++ b/lib/farside.ex
@@ -1,6 +1,7 @@
 defmodule Farside do
   @service_prefix Application.fetch_env!(:farside, :service_prefix)
-  @fallback_str Application.fetch_env!(:farside, :fallback_str)
+  @fallback_suffix Application.fetch_env!(:farside, :fallback_suffix)
+  @previous_suffix Application.fetch_env!(:farside, :previous_suffix)
 
   def get_services_map do
     {:ok, service_list} = Redix.command(:redix, ["KEYS", "#{@service_prefix}*"])
@@ -41,12 +42,34 @@ defmodule Farside do
     # or fall back to the default one
     instance =
       if Enum.count(instances) > 0 do
-        Enum.random(instances)
+        if Enum.count(instances) == 1 do
+          # If there's only one instance, just return that one...
+          List.first(instances)
+        else
+          # ...otherwise pick a random one from the list, ensuring
+          # that the same instance is never picked twice in a row.
+          {:ok, previous} =
+            Redix.command(
+              :redix,
+              ["GET", "#{service}#{@previous_suffix}"]
+            )
+
+          instance =
+            Enum.filter(instances, &(&1 != previous))
+            |> Enum.random()
+
+          Redix.command(
+            :redix,
+            ["SET", "#{service}#{@previous_suffix}", instance]
+          )
+
+          instance
+        end
       else
         {:ok, result} =
           Redix.command(
             :redix,
-            ["GET", "#{service}#{@fallback_str}"]
+            ["GET", "#{service}#{@fallback_suffix}"]
           )
 
         result
diff --git a/test/farside_test.exs b/test/farside_test.exs
index 6347691..786c382 100644
--- a/test/farside_test.exs
+++ b/test/farside_test.exs
@@ -42,15 +42,28 @@ defmodule FarsideTest do
     IO.puts("")
 
     Enum.map(service_names, fn service_name ->
-      IO.puts("/#{service_name}")
 
       conn =
         :get
         |> conn("/#{service_name}", "")
         |> Router.call(@opts)
 
+      first_redirect = elem(List.last(conn.resp_headers), 1)
+      IO.puts("    /#{service_name} (#1) -- #{first_redirect}")
       assert conn.state == :set
       assert conn.status == 302
+
+
+      conn =
+        :get
+        |> conn("/#{service_name}", "")
+        |> Router.call(@opts)
+
+      second_redirect = elem(List.last(conn.resp_headers), 1)
+      IO.puts("    /#{service_name} (#2) -- #{second_redirect}")
+      assert conn.state == :set
+      assert conn.status == 302
+      assert first_redirect != second_redirect
     end)
   end
 end
diff --git a/update.exs b/update.exs
index 7452e3d..e0c367a 100644
--- a/update.exs
+++ b/update.exs
@@ -1,5 +1,5 @@
 defmodule Instances do
-  @fallback_str Application.fetch_env!(:farside, :fallback_str)
+  @fallback_suffix Application.fetch_env!(:farside, :fallback_suffix)
   @update_file Application.fetch_env!(:farside, :update_file)
   @services_json Application.fetch_env!(:farside, :services_json)
   @service_prefix Application.fetch_env!(:farside, :service_prefix)
@@ -59,13 +59,13 @@ defmodule Instances do
     if Enum.count(instances) > 0 do
       Redix.command(:redix, [
         "SET",
-        "#{service.type}#{@fallback_str}",
+        "#{service.type}#{@fallback_suffix}",
         Enum.random(instances)
       ])
     else
       Redix.command(:redix, [
         "SET",
-        "#{service.type}#{@fallback_str}",
+        "#{service.type}#{@fallback_suffix}",
         service.fallback
       ])
     end