ID validation

[Breaking change]

This patch adds `auto_increment_increment` validation to the following
scenarios:
 * Connection initalization
 * Generating one ID
 * Generating multiple IDs

These changes should have no noticeable impact on performance and are
thread safe.

The intent is to prevent data corruption when a server is misconfigured
and in the event that one is found, it's removed from the pool of
available alloc servers allowing creation to continue with the valid
alloc servers.

Validation during initialization is done by comparing the client
configuration and with what's set on each server. If the server is
misconfigured, an error is reported via the `notifier` and it's left out
of the pool.

Validation during creation happens by comparing each pair of identifiers
and ensuring they've been incremented correctly by the configured amount.

If the `auto_increment_increment` changes while the clients are using
the server, the alloc server is removed from the list of servers and
an error is reported via the `notifier`.

An InvalidIncrementException is raised if all servers are misconfigured.

The changeset is considered a breaking change as any clients upgrading
will see an increment exception if their `increment_by` was set
incorrectly, this previously would have gone unnoticed.

[squashing] Add more detail to what the spec is doing

Addressing this comment

> Could we please expand a bit on this comment? Maybe tell which IDs are
> created now, and which IDs would be created after calling
> updated_increment once or twice.

> It feels like I have to read a lot of code to understand this test in
> its current form. I could do that now, but I know that I may well be
> to lazy to do so when looking at this test a year or two in the future

Asserting the IDs could lead to spurious failures since it'll come from
one of the two servers.

[squashing] Rename max_size to max_window_size

max_size is misleading, one may presume that it's the maximum amount of
IDs allowed to be allocated for the server. If they passed 100000 in, it
would have a negative performance hit

[squashing] Rename allocation methods

The method names were inconsistent, the method checks the allocation,
comparing it with the previous. It doesn't check the increment, except
in `validate_connection_increment` which remains unchanged

[squashing] Cleanup allocator validation

The previous implementation was a mess, it was cleaned up in the follow-
up PR, bringing it strait into this PR.

The InvalidIncrementException message has also been improved, when
the ID returned is an unexpected value, we'll query the database before
closing the connection to get the set `auto_increment_increment` value.
E.g

GlobalUid::InvalidIncrementException Configured: '5', Found: '42' on
  'global_uid_test_id_server_1'. Recently allocated IDs: [43, 85]

Also, while re-reading the code I realised that the previous `alert`
implementation would log the error twice when exceptions aren't
suppressed

Changelog.md

@@ -6,6 +6,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Added
+- [Breaking change] ID Validation, ensure the ID coming back has been incremented using the configured `auto_increment_increment`. (https://github.com/zendesk/global_uid/pull/63)
+
 ### Removed
 - Removed the `dry_run` option (https://github.com/zendesk/global_uid/pull/64)
 - Removed `GlobalUid::ServerVariables` module (https://github.com/zendesk/global_uid/pull/66)

README.md

@@ -49,14 +49,15 @@ The `increment_by` value configured here does not dictate the value on your allo
 
 Here's a complete list of the options you can use:
 
-| Name                  | Default                                    | Description                                                                                                |
-| --------------------- | ------------------------------------------ | ---------------------------------------------------------------------------------------------------------- |
-| `:disabled`           | `false`                                    | Disable GlobalUid entirely                                                                                 |
-| `:connection_timeout` | 3 seconds                                  | Timeout for connecting to a global UID server                                                              |
-| `:query_timeout`      | 10 seconds                                 | Timeout for retrieving a global UID from a server before we move on to the next server                     |
-| `:connection_retry`   | 10 minutes                                 | After failing to connect or query a UID server, how long before we retry                                   |
-| `:notifier`           | A proc calling `ActiveRecord::Base.logger` | This proc is called with two parameters upon UID server failure -- an exception and a message              |
-| `:increment_by`       | 5                                          | Used to validate number of ID servers, preventing connections if there are more servers than the given increment |
+| Name                             | Default                                    | Description                                                                                                  |
+| -------------------------------- | ------------------------------------------ | ------------------------------------------------------------------------------------------------------------ |
+| `:disabled`                      | `false`                                    | Disable GlobalUid entirely                                                                                   |
+| `:connection_timeout`            | 3 seconds                                  | Timeout for connecting to a global UID server                                                                |
+| `:query_timeout`                 | 10 seconds                                 | Timeout for retrieving a global UID from a server before we move on to the next server                       |
+| `:connection_retry`              | 10 minutes                                 | After failing to connect or query a UID server, how long before we retry                                     |
+| `:notifier`                      | A proc calling `ActiveRecord::Base.logger` | This proc is called with two parameters upon UID server failure -- an exception and a message                |
+| `:increment_by`                  | 5                                          | Used for validation, compared with the value on the alloc servers to prevent allocation of duplicate IDs     |
+| `:suppress_increment_exceptions` | `false`                                    | Suppress configuration validation, allowing updates to `auto_increment_increment` while alloc servers in use |
 
 ### Migration
 

lib/global_uid.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 require "global_uid/base"
+require "global_uid/allocator"
 require "global_uid/active_record_extension"
 require "global_uid/has_and_belongs_to_many_builder_extension"
 require "global_uid/migration_extension"
@@ -9,6 +10,8 @@ module GlobalUid
   class NoServersAvailableException < StandardError ; end
   class ConnectionTimeoutException < StandardError ; end
   class TimeoutException < StandardError ; end
+  class InvalidIncrementException < StandardError ; end
+  class InvalidIncrementPreventionException < StandardError ; end
 end
 
 ActiveRecord::Base.send(:include, GlobalUid::ActiveRecordExtension)

lib/global_uid/allocator.rb

@@ -0,0 +1,66 @@
+module GlobalUid
+  class Allocator
+    attr_reader :allocations, :max_window_size, :incrementing_by, :connection
+
+    def initialize(max_window_size: 5, incrementing_by:, connection:)
+      @allocations = []
+      @max_window_size = max_window_size
+      @incrementing_by = incrementing_by
+      @connection = connection
+      validate_connection_increment
+    end
+
+    def allocate_one(table)
+      identifier = connection.insert("REPLACE INTO #{table} (stub) VALUES ('a')")
+      allocate(identifier)
+    end
+
+    def allocate_many(table, count:)
+      increment_by = validate_connection_increment
+
+      start_id = connection.insert("REPLACE INTO #{table} (stub) VALUES " + (["('a')"] * count).join(','))
+      identifiers = start_id.step(start_id + (count - 1) * increment_by, increment_by).to_a
+      identifiers.each { |identifier| allocate(identifier) }
+      identifiers
+    end
+
+    private
+
+    def allocate(identifier)
+      allocations.shift if allocations.size >= max_window_size
+      allocations << identifier
+
+      if !valid_allocation?
+        db_increment = connection.select_value("SELECT @@auto_increment_increment")
+        message = "Configured: '#{incrementing_by}', Found: '#{db_increment}' on '#{connection.current_database}'. Recently allocated IDs: #{allocations}"
+        alert(InvalidIncrementException.new(message))
+      end
+
+      identifier
+    end
+
+    def valid_allocation?
+      allocations[1..-1].all? do |identifier|
+        (identifier - allocations[0]) % incrementing_by == 0
+      end
+    end
+
+    def validate_connection_increment
+      db_increment = connection.select_value("SELECT @@auto_increment_increment")
+
+      if db_increment != incrementing_by
+        alert(InvalidIncrementPreventionException.new("Configured: '#{incrementing_by}', Found: '#{db_increment}' on '#{connection.current_database}'"))
+      end
+
+      db_increment
+    end
+
+    def alert(exception)
+      if GlobalUid::Base.global_uid_options[:suppress_increment_exceptions]
+        GlobalUid::Base.notify(exception, exception.message)
+      else
+        raise exception
+      end
+    end
+  end
+end

lib/global_uid/base.rb

@@ -11,9 +11,10 @@ class Base
       :connection_retry     => 10.minutes,
       :notifier             => Proc.new { |exception, message| ActiveRecord::Base.logger.error("GlobalUID error: #{exception.class} #{message}") },
       :query_timeout        => 10,
-      :increment_by         => 5,  # This will define the maximum number of servers that you can have
+      :increment_by         => 5, # This will define the maximum number of servers that you can have
       :disabled             => false,
-      :per_process_affinity => true
+      :per_process_affinity => true,
+      :suppress_increment_exceptions => false
     }
 
     def self.servers
@@ -94,6 +95,7 @@ def self.disconnect!
 
     def self.setup_connections!
       connection_timeout = self.global_uid_options[:connection_timeout]
+      increment_by = self.global_uid_options[:increment_by]
 
       if self.servers.nil?
         self.servers = init_server_info
@@ -107,9 +109,18 @@ def self.setup_connections!
         if info[:new?] || ( info[:retry_at] && Time.now > info[:retry_at] )
           info[:new?] = false
 
-          connection = new_connection(info[:name], connection_timeout)
-          info[:cx]  = connection
-          info[:retry_at] = Time.now + self.global_uid_options[:connection_retry] if connection.nil?
+          begin
+            connection = new_connection(info[:name], connection_timeout)
+            info[:cx] = connection
+            if connection.nil?
+              info[:retry_at] = Time.now + self.global_uid_options[:connection_retry]
+            else
+              info[:allocator] = Allocator.new(incrementing_by: increment_by, connection: connection)
+            end
+          rescue InvalidIncrementPreventionException => e
+            notify e, "#{e.message}"
+            info[:cx] = nil
+          end
         end
       end
 
@@ -160,9 +171,9 @@ def self.get_connections
 
     def self.get_uid_for_class(klass)
       with_connections do |connection|
+        server = self.servers.find { |s| connection.current_database.include?(s[:name]) }
         Timeout.timeout(self.global_uid_options[:query_timeout], TimeoutException) do
-          id = connection.insert("REPLACE INTO #{klass.global_uid_table} (stub) VALUES ('a')")
-          return id
+          return server[:allocator].allocate_one(klass.global_uid_table)
         end
       end
       raise NoServersAvailableException, "All global UID servers are gone!"
@@ -171,10 +182,9 @@ def self.get_uid_for_class(klass)
     def self.get_many_uids_for_class(klass, count)
       return [] unless count > 0
       with_connections do |connection|
+        server = self.servers.find { |s| connection.current_database.include?(s[:name]) }
         Timeout.timeout(self.global_uid_options[:query_timeout], TimeoutException) do
-          increment_by = connection.select_value("SELECT @@auto_increment_increment")
-          start_id = connection.insert("REPLACE INTO #{klass.global_uid_table} (stub) VALUES " + (["('a')"] * count).join(','))
-          return start_id.step(start_id + (count-1) * increment_by, increment_by).to_a
+          return server[:allocator].allocate_many(klass.global_uid_table, count: count)
         end
       end
       raise NoServersAvailableException, "All global UID servers are gone!"