Remote kills are absolute (#112)

* Remote kills are absolute

* Add ADR about absolute remote kills

Author: jmileham

app/models/app_remote_kill.rb

@@ -13,28 +13,14 @@ class AppRemoteKill < ActiveRecord::Base
   validate :fixed_version_must_be_greater_than_first_bad_version
   validate :must_not_overlap_existing
 
-  scope :affecting, ->(app_build, override: false, overridden_at: nil) do
+  scope :affecting, ->(app_build) do
     where(
       arel_table[:app_id].eq(app_build.app_id)
       .and(arel_table[:first_bad_version].lteq(app_build.version))
       .and(arel_fixed_version_is_null_or_greater_than(app_build.version))
-      .and(
-        Arel::Nodes::Grouping.new(
-          arel_is_false(override)
-          .or(arel_table[:updated_at].gt(overridden_at))
-        )
-      )
     )
   end
 
-  class << self
-    private
-
-    def arel_is_false(value_or_arel)
-      Arel::Nodes::Grouping.new(Arel::Nodes::False.new).eq(value_or_arel)
-    end
-  end
-
   scope :overlapping, ->(other) do
     where(app_id: other.app_id, split_id: other.split_id)
       .where.not(id: other.id)

app/models/assignment.rb

@@ -47,7 +47,7 @@ class Assignment < ActiveRecord::Base
 
   scope :excluding_remote_kills_for, ->(app_build) do
     joins(:split).where(
-      Split.arel_excluding_remote_kills_for(app_build, override: arel_table[:force], overridden_at: arel_table[:updated_at])
+      Split.arel_excluding_remote_kills_for(app_build)
     )
   end
 

app/models/split.rb

@@ -71,8 +71,8 @@ def self.arel_excluding_incomplete_features_for(app_build)
       .readonly
   end
 
-  def self.arel_excluding_remote_kills_for(app_build, override: false, overridden_at: nil)
-    AppRemoteKill.select(1).affecting(app_build, override: override, overridden_at: overridden_at).arel
+  def self.arel_excluding_remote_kills_for(app_build)
+    AppRemoteKill.select(1).affecting(app_build).arel
       .where(AppRemoteKill.arel_table[:split_id].eq(arel_table[:id])).exists.not
   end
 

doc/arch/adr-001.md

@@ -1,5 +1,9 @@
 # ADR 001: AppRemoteKill Will Loosely Couple to Override Variant
 
+## Status
+
+Accepted
+
 ## Context
 
 Splits are accessible to any app in the same ecosystem, but not always
@@ -30,10 +34,6 @@ At this time, we will still strongly couple app_remote_kills to split
 existence, however. This may change in the future if it proves to cause
 operational issues
 
-## Status
-
-Accepted
-
 ## Consequences
 
 The only remaining way to ship an AppRemoteKill that doesn't apply

doc/arch/adr-002.md

@@ -0,0 +1,58 @@
+# ADR 002: Remote kills are absolute
+
+## Status
+
+Accepted
+
+## Context
+
+When rapidly iterating on API for prerelease features, we will commonly
+want to make breaking API changes without revving the API endpoints and
+maintaining the previous versions of the endpoints indefinitely for
+versions of the app that will never be seen by customers.
+
+In that context, remote kills stand to be an incredibly useful tool.
+When you decide to make a breaking API change, you simply create/update
+a remote kill for the app from version `0` through the first client
+version conforming to the new contract. Then, instead of getting crash
+bugs on dev builds, the feature simply turns itself off, and testers
+will have to upgrade in order to see the feature again.
+
+This stands to eliminate false bug reports of crash bugs from
+internal/alpha testers who simply need to upgrade.
+
+But if we want to rely on this as a safety mechanism, we need to make
+sure that chrome extension assignment overrides don't have the
+opportunity to win-out over a remote kill, because they are too blunt of
+a tool. As an alpha tester, you might see that the feature went away and
+perform another override in the Chrome extension. If that happens,
+you'd be back in crash bug land.
+
+## Decision
+
+On TestTrack server, remote kills will be absolute and won't be
+overridden by force assignments regardless of the relative recency of
+the override and the remote kill.
+
+## Consequences
+
+As a result, it may be somewhat more difficult for app developers to
+"see around" an open-ended remote kill that was dropped because of a bug
+as they work on the bugfix.
+
+A hypothetical solution is to bump the app version on their branch and
+then mark the remote_kill's fixed_version as that version, so that as
+they develop the fix, their local builds will allow them to see their
+work. Then merging and releasing the branch will cause the fix to be
+available to consumers of that build.
+
+This might introduce version number linearization problems unless we
+hold all other merges until the bugfix is released, which may or may not
+be an issue the dev team can stomach.
+
+As an alternative, we could make the forthcoming fake TestTrack server
+being developed into the Rails client less draconian, and allow
+assignment overrides to take precedence over remote kills in that
+context only. Then the remote_kill fixed_version could be stamped once
+the release version of the bugfix is known, after the developer has
+already locally tested the fix.

spec/models/app_remote_kill_spec.rb

@@ -188,26 +188,6 @@
         expect(described_class.affecting(app_build)).to include(existing_remote_kill)
       end
 
-      it "doesn't return remote kills with newer overrides" do
-        app_build = app.define_build(version: "1.0", built_at: nil)
-
-        expect(described_class.affecting(app_build, override: true, overridden_at: 1.hour.from_now)).not_to include(existing_remote_kill)
-      end
-
-      it "does return remote kills with older overrides" do
-        app_build = app.define_build(version: "1.0", built_at: nil)
-
-        expect(described_class.affecting(app_build, override: true, overridden_at: 1.hour.ago)).to include(existing_remote_kill)
-      end
-
-      it "accepts arel expressions for override and overridden_at and behaves the same" do
-        app_build = app.define_build(version: "1.0", built_at: nil)
-        override = Arel::Nodes::True.new
-        overridden_at = Arel::Nodes.build_quoted(1.hour.ago)
-
-        expect(described_class.affecting(app_build, override: override, overridden_at: overridden_at)).to include(existing_remote_kill)
-      end
-
       it "doesn't return remote kills starting after app version" do
         app_build = app.define_build(version: "0.9", built_at: nil)
 

spec/models/assignment_spec.rb

@@ -270,33 +270,13 @@
       expect(described_class.excluding_remote_kills_for(app_build)).to include(assignment)
     end
 
-    it "doesn't return assignments for overlapping remote kills" do
-      split = FactoryBot.create(:split)
-      assignment = FactoryBot.create(:assignment, split: split)
-      app = FactoryBot.create(:app)
-      FactoryBot.create(:app_remote_kill, split: split, app: app, first_bad_version: "0.9", fixed_version: "1.2")
-      app_build = app.define_build(built_at: Time.zone.now, version: "1.0.0")
-
-      expect(described_class.excluding_remote_kills_for(app_build)).not_to include(assignment)
-    end
-
-    it "returns assignments for overlapping older remote kills when forced" do
+    it "doesn't return assignments for overlapping remote kills, even if forced and more recent" do
       split = FactoryBot.create(:split)
       app = FactoryBot.create(:app)
       FactoryBot.create(:app_remote_kill, split: split, app: app, first_bad_version: "0.9", fixed_version: "1.2")
       assignment = FactoryBot.create(:assignment, split: split, force: true)
       app_build = app.define_build(built_at: Time.zone.now, version: "1.0.0")
 
-      expect(described_class.excluding_remote_kills_for(app_build)).to include(assignment)
-    end
-
-    it "doesn't return forced assignments older than a remote kill" do
-      split = FactoryBot.create(:split)
-      assignment = FactoryBot.create(:assignment, split: split, force: true)
-      app = FactoryBot.create(:app)
-      FactoryBot.create(:app_remote_kill, split: split, app: app, first_bad_version: "0.9", fixed_version: "1.2")
-      app_build = app.define_build(built_at: Time.zone.now, version: "1.0.0")
-
       expect(described_class.excluding_remote_kills_for(app_build)).not_to include(assignment)
     end
 

spec/models/split_spec.rb

@@ -361,21 +361,12 @@
       expect(Split.where(Split.arel_excluding_remote_kills_for(app_build))).not_to include(split)
     end
 
-    it "is backed by AppRemoteKill.affecting with default override args" do
+    it "is backed by AppRemoteKill.affecting" do
       allow(AppRemoteKill).to receive(:affecting).and_call_original
 
       Split.arel_excluding_remote_kills_for(app_build)
 
-      expect(AppRemoteKill).to have_received(:affecting).with(app_build, override: false, overridden_at: nil)
-    end
-
-    it "is backed by AppRemoteKill.affecting with explicit override args" do
-      allow(AppRemoteKill).to receive(:affecting).and_call_original
-      t = Time.zone.now
-
-      Split.arel_excluding_remote_kills_for(app_build, override: true, overridden_at: t)
-
-      expect(AppRemoteKill).to have_received(:affecting).with(app_build, override: true, overridden_at: t)
+      expect(AppRemoteKill).to have_received(:affecting).with(app_build)
     end
   end