From a02fe251df7ea7316f51850fe603e7e5ac4431e2 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 14 Mar 2016 15:18:52 +0100
Subject: [PATCH 1/4] Allow project housekeeping only once an hour

---
 app/controllers/projects_controller.rb        |  4 +-
 app/services/projects/housekeeping_service.rb | 14 +++++++
 .../projects/housekeeping_service_spec.rb     | 40 +++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 spec/services/projects/housekeeping_service_spec.rb

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index c70add86a20..2a3dc5c79f7 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -170,10 +170,10 @@ class ProjectsController < ApplicationController
   end
 
   def housekeeping
-    ::Projects::HousekeepingService.new(@project).execute
+    message = ::Projects::HousekeepingService.new(@project).execute
 
     respond_to do |format|
-      flash[:notice] = "Housekeeping successfully started."
+      flash[:notice] = message
       format.html { redirect_to project_path(@project) }
     end
   end
diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb
index 0db85ac2142..11be5b1badf 100644
--- a/app/services/projects/housekeeping_service.rb
+++ b/app/services/projects/housekeeping_service.rb
@@ -9,12 +9,26 @@ module Projects
   class HousekeepingService < BaseService
     include Gitlab::ShellAdapter
 
+    LEASE_TIMEOUT = 3600
+
     def initialize(project)
       @project = project
     end
 
     def execute
+      if !try_obtain_lease
+        return "Housekeeping was already triggered in the past #{LEASE_TIMEOUT / 60} minutes"
+      end
+      
       GitlabShellWorker.perform_async(:gc, @project.path_with_namespace)
+      return "Housekeeping successfully started"
+    end
+
+    private
+
+    def try_obtain_lease
+      lease = ::Gitlab::ExclusiveLease.new("project_housekeeping:#{@project.id}", timeout: LEASE_TIMEOUT)
+      lease.try_obtain
     end
   end
 end
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
new file mode 100644
index 00000000000..7cddeb5c354
--- /dev/null
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -0,0 +1,40 @@
+require 'spec_helper'
+
+describe Projects::HousekeepingService do
+  subject { Projects::HousekeepingService.new(project) }
+  let(:project) { create :project }
+
+  describe :execute do
+    before do
+      project.pushes_since_gc = 3
+      project.save!
+    end
+
+    it 'enqueues a sidekiq job' do
+      expect(subject).to receive(:try_obtain_lease).and_return(true)
+      expect(GitlabShellWorker).to receive(:perform_async).with(:gc, project.path_with_namespace)
+
+      expect(subject.execute).to include('successfully started')
+      expect(project.pushes_since_gc).to eq(0)
+    end
+
+    it 'does not enqueue a job when no lease can be obtained' do
+      expect(subject).to receive(:try_obtain_lease).and_return(false)
+      expect(GitlabShellWorker).not_to receive(:perform_async)
+
+      expect(subject.execute).to include('already triggered')
+      expect(project.pushes_since_gc).to eq(3)
+    end
+  end
+
+  describe :needed? do
+    it 'when the count is low enough' do
+      expect(subject.needed?).to eq(false)
+    end
+
+    it 'when the count is high enough' do
+      allow(project).to receive(:pushes_since_gc).and_return(10)
+      expect(subject.needed?).to eq(true)
+    end
+  end
+end
\ No newline at end of file

From 021d53c96d308df7c721984435442993357a3414 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 14 Mar 2016 16:49:24 +0100
Subject: [PATCH 2/4] Run 'git gc' every 10 pushes

---
 app/services/git_push_service.rb              |  8 ++++++
 app/services/projects/housekeeping_service.rb | 16 +++++++++--
 ...0314143402_projects_add_pushes_since_gc.rb |  5 ++++
 db/schema.rb                                  |  3 ++-
 spec/services/git_push_service_spec.rb        | 27 +++++++++++++++++++
 .../projects/housekeeping_service_spec.rb     | 10 ++++++-
 6 files changed, 65 insertions(+), 4 deletions(-)
 create mode 100644 db/migrate/20160314143402_projects_add_pushes_since_gc.rb

diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index bd31a617747..e50cbdfb602 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -49,6 +49,8 @@ class GitPushService < BaseService
     # Update merge requests that may be affected by this push. A new branch
     # could cause the last commit of a merge request to change.
     update_merge_requests
+
+    perform_housekeeping
   end
 
   def update_main_language
@@ -73,6 +75,12 @@ class GitPushService < BaseService
     ProjectCacheWorker.perform_async(@project.id)
   end
 
+  def perform_housekeeping
+    housekeeping = Projects::HousekeepingService.new(@project)
+    housekeeping.increment!
+    housekeeping.execute if housekeeping.needed?
+  end
+
   def process_default_branch
     @push_commits = project.repository.commits(params[:newrev])
 
diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb
index 11be5b1badf..83bdedf7a8d 100644
--- a/app/services/projects/housekeeping_service.rb
+++ b/app/services/projects/housekeeping_service.rb
@@ -19,9 +19,21 @@ module Projects
       if !try_obtain_lease
         return "Housekeeping was already triggered in the past #{LEASE_TIMEOUT / 60} minutes"
       end
-      
+
       GitlabShellWorker.perform_async(:gc, @project.path_with_namespace)
-      return "Housekeeping successfully started"
+      @project.pushes_since_gc = 0
+      @project.save!
+
+      "Housekeeping successfully started"
+    end
+
+    def needed?
+      @project.pushes_since_gc >= 10
+    end
+
+    def increment!
+      @project.pushes_since_gc += 1
+      @project.save!
     end
 
     private
diff --git a/db/migrate/20160314143402_projects_add_pushes_since_gc.rb b/db/migrate/20160314143402_projects_add_pushes_since_gc.rb
new file mode 100644
index 00000000000..5d30a38bc99
--- /dev/null
+++ b/db/migrate/20160314143402_projects_add_pushes_since_gc.rb
@@ -0,0 +1,5 @@
+class ProjectsAddPushesSinceGc < ActiveRecord::Migration
+  def change
+    add_column :projects, :pushes_since_gc, :integer, default: 0
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 3ac6203632d..cf3f8245b38 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160309140734) do
+ActiveRecord::Schema.define(version: 20160314143402) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -711,6 +711,7 @@ ActiveRecord::Schema.define(version: 20160309140734) do
     t.boolean  "pending_delete",         default: false
     t.boolean  "public_builds",          default: true,     null: false
     t.string   "main_language"
+    t.integer  "pushes_since_gc",        default: 0
   end
 
   add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index a7e2e1b1792..ebf3ec1f5fd 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -401,6 +401,33 @@ describe GitPushService, services: true do
     end
   end
 
+  describe "housekeeping" do
+    let(:housekeeping) { instance_double('Projects::HousekeepingService', increment!: nil, needed?: false) }
+
+    before do
+      allow(Projects::HousekeepingService).to receive(:new).and_return(housekeeping)
+    end
+
+    it 'does not perform housekeeping when not needed' do
+      expect(housekeeping).not_to receive(:execute)
+
+      execute_service(project, user, @oldrev, @newrev, @ref)
+    end
+
+    it 'performs housekeeping when needed' do
+      expect(housekeeping).to receive(:needed?).and_return(true)
+      expect(housekeeping).to receive(:execute)
+
+      execute_service(project, user, @oldrev, @newrev, @ref)
+    end
+
+    it 'increments the push counter' do
+      expect(housekeeping).to receive(:increment!)
+ 
+      execute_service(project, user, @oldrev, @newrev, @ref)
+    end
+  end
+
   def execute_service(project, user, oldrev, newrev, ref)
     service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref )
     service.execute
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index 7cddeb5c354..32552d882aa 100644
--- a/spec/services/projects/housekeeping_service_spec.rb
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -37,4 +37,12 @@ describe Projects::HousekeepingService do
       expect(subject.needed?).to eq(true)
     end
   end
-end
\ No newline at end of file
+
+  describe :increment! do
+    it 'increments the pushes_since_gc counter' do
+      expect(project.pushes_since_gc).to eq(0)
+      subject.increment!
+      expect(project.pushes_since_gc).to eq(1)
+    end
+  end
+end

From 0beae70efaafc361cf15c13231bdc5ed6de8569f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 14 Mar 2016 18:46:38 +0100
Subject: [PATCH 3/4] Use strings instead of symbols

---
 spec/services/projects/housekeeping_service_spec.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index 32552d882aa..4c3577149f9 100644
--- a/spec/services/projects/housekeeping_service_spec.rb
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -4,7 +4,7 @@ describe Projects::HousekeepingService do
   subject { Projects::HousekeepingService.new(project) }
   let(:project) { create :project }
 
-  describe :execute do
+  describe 'execute' do
     before do
       project.pushes_since_gc = 3
       project.save!
@@ -27,7 +27,7 @@ describe Projects::HousekeepingService do
     end
   end
 
-  describe :needed? do
+  describe 'needed?' do
     it 'when the count is low enough' do
       expect(subject.needed?).to eq(false)
     end
@@ -38,7 +38,7 @@ describe Projects::HousekeepingService do
     end
   end
 
-  describe :increment! do
+  describe 'increment!' do
     it 'increments the pushes_since_gc counter' do
       expect(project.pushes_since_gc).to eq(0)
       subject.increment!

From 30b36c92c386e93b432166fb6f9dd973882a6d82 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Tue, 15 Mar 2016 11:03:43 +0100
Subject: [PATCH 4/4] Use an exception to pass messages

---
 app/controllers/projects_controller.rb        | 15 ++++++++----
 app/services/git_push_service.rb              |  1 +
 app/services/projects/housekeeping_service.rb | 19 ++++++++-------
 spec/services/git_push_service_spec.rb        | 24 ++++++++++++++-----
 .../projects/housekeeping_service_spec.rb     |  6 ++---
 5 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 2a3dc5c79f7..36f37221c58 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -170,12 +170,17 @@ class ProjectsController < ApplicationController
   end
 
   def housekeeping
-    message = ::Projects::HousekeepingService.new(@project).execute
+    ::Projects::HousekeepingService.new(@project).execute
 
-    respond_to do |format|
-      flash[:notice] = message
-      format.html { redirect_to project_path(@project) }
-    end
+    redirect_to(
+      project_path(@project),
+      notice: "Housekeeping successfully started"
+    )
+  rescue ::Projects::HousekeepingService::LeaseTaken => ex
+    redirect_to(
+      edit_project_path(@project),
+      alert: ex.to_s
+    )
   end
 
   def toggle_star
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index e50cbdfb602..4313de0ccab 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -79,6 +79,7 @@ class GitPushService < BaseService
     housekeeping = Projects::HousekeepingService.new(@project)
     housekeeping.increment!
     housekeeping.execute if housekeeping.needed?
+  rescue Projects::HousekeepingService::LeaseTaken
   end
 
   def process_default_branch
diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb
index 83bdedf7a8d..bccd67d3dbf 100644
--- a/app/services/projects/housekeeping_service.rb
+++ b/app/services/projects/housekeeping_service.rb
@@ -11,20 +11,22 @@ module Projects
 
     LEASE_TIMEOUT = 3600
 
+    class LeaseTaken < StandardError
+      def to_s
+        "Somebody already triggered housekeeping for this project in the past #{LEASE_TIMEOUT / 60} minutes"
+      end
+    end
+
     def initialize(project)
       @project = project
     end
 
     def execute
-      if !try_obtain_lease
-        return "Housekeeping was already triggered in the past #{LEASE_TIMEOUT / 60} minutes"
-      end
+      raise LeaseTaken if !try_obtain_lease
 
       GitlabShellWorker.perform_async(:gc, @project.path_with_namespace)
-      @project.pushes_since_gc = 0
-      @project.save!
-
-      "Housekeeping successfully started"
+    ensure
+      @project.update_column(:pushes_since_gc, 0)
     end
 
     def needed?
@@ -32,8 +34,7 @@ module Projects
     end
 
     def increment!
-      @project.pushes_since_gc += 1
-      @project.save!
+      @project.increment!(:pushes_since_gc)
     end
 
     private
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index ebf3ec1f5fd..145bc937560 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -402,7 +402,7 @@ describe GitPushService, services: true do
   end
 
   describe "housekeeping" do
-    let(:housekeeping) { instance_double('Projects::HousekeepingService', increment!: nil, needed?: false) }
+    let(:housekeeping) { Projects::HousekeepingService.new(project) }
 
     before do
       allow(Projects::HousekeepingService).to receive(:new).and_return(housekeeping)
@@ -414,16 +414,28 @@ describe GitPushService, services: true do
       execute_service(project, user, @oldrev, @newrev, @ref)
     end
 
-    it 'performs housekeeping when needed' do
-      expect(housekeeping).to receive(:needed?).and_return(true)
-      expect(housekeeping).to receive(:execute)
+    context 'when housekeeping is needed' do
+      before do
+        allow(housekeeping).to receive(:needed?).and_return(true)
+      end
 
-      execute_service(project, user, @oldrev, @newrev, @ref)
+      it 'performs housekeeping' do
+        expect(housekeeping).to receive(:execute)
+
+        execute_service(project, user, @oldrev, @newrev, @ref)
+      end
+
+      it 'does not raise an exception' do
+        allow(housekeeping).to receive(:try_obtain_lease).and_return(false)
+
+        execute_service(project, user, @oldrev, @newrev, @ref)
+      end
     end
 
+
     it 'increments the push counter' do
       expect(housekeeping).to receive(:increment!)
- 
+
       execute_service(project, user, @oldrev, @newrev, @ref)
     end
   end
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index 4c3577149f9..93bf1b81fbe 100644
--- a/spec/services/projects/housekeeping_service_spec.rb
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -14,7 +14,7 @@ describe Projects::HousekeepingService do
       expect(subject).to receive(:try_obtain_lease).and_return(true)
       expect(GitlabShellWorker).to receive(:perform_async).with(:gc, project.path_with_namespace)
 
-      expect(subject.execute).to include('successfully started')
+      subject.execute
       expect(project.pushes_since_gc).to eq(0)
     end
 
@@ -22,8 +22,8 @@ describe Projects::HousekeepingService do
       expect(subject).to receive(:try_obtain_lease).and_return(false)
       expect(GitlabShellWorker).not_to receive(:perform_async)
 
-      expect(subject.execute).to include('already triggered')
-      expect(project.pushes_since_gc).to eq(3)
+      expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken)
+      expect(project.pushes_since_gc).to eq(0)
     end
   end