feat(submission): offload background operations from finalise transaction

app/jobs/course/assessment/submission/publish_task_completion_job.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+# Pushes a submission's task status to Cikgo once the transaction that changed it has committed.
+#
+# The push is a synchronous HTTP call with a five second timeout, so it is kept off the request: a
+# slow or unavailable Cikgo must not delay a response, hold locks on a submission that is already
+# saved, or fail a request for work that has been committed.
+#
+# Unlike the inline +publish_task_completion+, which logs and swallows in production, a failure here
+# is allowed to propagate. That is the point of running it as a job: Sidekiq retries it and Rollbar
+# records it, rather than the error disappearing into the log.
+class Course::Assessment::Submission::PublishTaskCompletionJob < ApplicationJob
+  rescue_from(ActiveJob::DeserializationError) do |_|
+    # The submission was deleted before the job ran; there is no status left to publish.
+  end
+
+  def perform(submission)
+    instance = Course.unscoped { submission.assessment.course.instance }
+
+    ActsAsTenant.with_tenant(instance) do
+      # Re-read rather than trusting the state at enqueue time. A later transition may already have
+      # been pushed, and the status Cikgo should end up with is the current one either way — which
+      # also makes two pushes racing each other harmless.
+      next unless submission.should_publish_task_completion?
+
+      submission.publish_task_completion!
+    end
+  end
+end

app/jobs/course/lesson_plan/personalized_timeline_update_job.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+# Recomputes one course user's personalised timeline.
+#
+# Enqueued after a submission is finalised, rather than run inside the finalise transaction: the
+# recomputation walks every lesson plan item in the course and writes a personal time per shiftable
+# item, which is by far the largest unit of work in that transaction and the only part that contends
+# with +CoursewidePersonalizedTimelineUpdateJob+ for the same rows.
+#
+# The timeline is derived state. If this job never succeeds the student's timeline is one submission
+# stale but still valid, and the next recomputation for them — their next submission, an instructor
+# pressing "Recompute all times", or a coursewide run — brings it forward.
+class Course::LessonPlan::PersonalizedTimelineUpdateJob < ApplicationJob
+  include Course::LessonPlan::PersonalizationConcern
+
+  queue_as :lowest
+
+  rescue_from(ActiveJob::DeserializationError) do |_|
+    # The course user was removed from the course before the job ran; there is no timeline to update.
+  end
+
+  def perform(course_user)
+    instance = Course.unscoped { course_user.course.instance }
+
+    ActsAsTenant.with_tenant(instance) do
+      update_personalized_timeline_for_user(course_user)
+    end
+  end
+end

app/models/concerns/course/assessment/submission/cikgo_task_completion_concern.rb

@@ -10,18 +10,27 @@ module Course::Assessment::Submission::CikgoTaskCompletionConcern
   extend ActiveSupport::Concern
 
   included do
-    after_save :publish_task_completion, if: -> { should_publish_task_completion? && saved_change_to_workflow_state? }
+    after_save :enqueue_publish_task_completion,
+               if: -> { should_publish_task_completion? && saved_change_to_workflow_state? }
   end
 
+  # Pushes the status, absorbing failures the way the controller's +edit+ hook needs: logged, and
+  # fatal only outside production so a broken Cikgo integration is noticed in development.
   def publish_task_completion
+    publish_task_completion!
+  rescue StandardError => e
+    Rails.logger.error("Cikgo: Cannot publish task completion for submission #{id}: #{e}")
+    raise e unless Rails.env.production?
+  end
+
+  # Pushes the status and lets failures propagate, for callers that can act on them —
+  # +PublishTaskCompletionJob+, where Sidekiq retries and Rollbar records.
+  def publish_task_completion!
     Cikgo::ResourcesService.mark_task!(status, lesson_plan_item, {
       user_id: creator_id_on_cikgo,
       url: submission_url,
       score: grade&.to_i
     })
-  rescue StandardError => e
-    Rails.logger.error("Cikgo: Cannot publish task completion for submission #{id}: #{e}")
-    raise e unless Rails.env.production?
   end
 
   def should_publish_task_completion?
@@ -31,6 +40,14 @@ def should_publish_task_completion?
 
   private
 
+  # Hands the push to a job once the transaction that changed the status has committed, so the
+  # request neither waits on Cikgo nor fails because of it. See +PublishTaskCompletionJob+.
+  def enqueue_publish_task_completion
+    ActiveRecord.after_all_transactions_commit do
+      Course::Assessment::Submission::PublishTaskCompletionJob.perform_later(self)
+    end
+  end
+
   delegate :edit_course_assessment_submission_url, to: 'Rails.application.routes.url_helpers'
 
   def status

app/models/concerns/course/assessment/submission/notification_concern.rb

@@ -27,6 +27,14 @@ def send_submit_notification
     return if assessment.autograded?
     return unless course_user.student?
 
-    Course::AssessmentNotifier.assessment_submitted(creator, course_user, self)
+    # Building the activity walks the submitter's groups to find managers and subtracts the
+    # unsubscribed ones, then writes the activity and its notifications. None of that needs to be in
+    # the transaction that commits the student's work: an unsent notification is recoverable
+    # attention, a rolled back submission is lost work. It stays on the request rather than becoming
+    # a job of its own, since its real work is enqueuing mail delivery jobs — already deferred to
+    # after commit by Notifier::Base::ActivityWrapper.
+    ActiveRecord.after_all_transactions_commit do
+      Course::AssessmentNotifier.assessment_submitted(creator, course_user, self)
+    end
   end
 end

app/models/concerns/course/assessment/submission/workflow_event_concern.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 module Course::Assessment::Submission::WorkflowEventConcern
   extend ActiveSupport::Concern
-  include Course::LessonPlan::PersonalizationConcern
   include Course::Assessment::Submission::CikgoTaskCompletionConcern
 
   included do
@@ -30,7 +29,7 @@ def finalise(_ = nil)
     # NB: We are not recomputing on unsubmission because unsubmit is not done by the student
     #     It will recompute again when resubmission occurs. This also prevents the timings for
     #     the unsubmitted item from changing e.g. from other submissions that the student has done.
-    update_personalized_timeline_for_user(course_user)
+    enqueue_personalized_timeline_update
   end
 
   # Handles the marking of a submission.
@@ -104,6 +103,21 @@ def resubmit_programming
 
   private
 
+  # Recomputes the submitter's personalised timeline out of band.
+  #
+  # The recomputation reads every lesson plan item in the course and writes a personal time per
+  # shiftable item, so it is kept out of the transaction that commits the student's work: a failure
+  # there used to roll the whole finalise back (see
+  # docs/personal_time_duplicate_incident_summary.md). Enqueued after commit so the job also reads a
+  # committed submission — +submitted_items+ is meant to include this one.
+  def enqueue_personalized_timeline_update
+    submitter = course_user
+
+    ActiveRecord.after_all_transactions_commit do
+      Course::LessonPlan::PersonalizedTimelineUpdateJob.perform_later(submitter)
+    end
+  end
+
   # finalise event (from attempting) - Assign 0 points as there are no questions.
   def assign_zero_experience_points
     return unless assessment.questions.empty?

spec/controllers/concerns/course/lesson_plan/personalization_concern_spec.rb

@@ -197,9 +197,15 @@ def submit_assessment(assessment)
 
     context 'when the course has many course users' do
       let!(:course_users) do
-        create_list(:course_user, 3, course: course, timeline_algorithm: 'fomo').each do |course_user|
+        users = create_list(:course_user, 3, course: course, timeline_algorithm: 'fomo')
+        users.each do |course_user|
           create(:course_assessment_submission, assessment: assessment, creator: course_user.user).tap(&:finalise!)
         end
+        # Each finalise enqueues a PersonalizedTimelineUpdateJob for its submitter. Settle them
+        # before the example runs, so the coursewide recomputation under test is not racing three
+        # per-user recomputations over the same rows.
+        wait_for_enqueued_jobs
+        users
       end
 
       it 'shifts the item for every course user' do

spec/jobs/course/assessment/submission/publish_task_completion_job_spec.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+RSpec.describe Course::Assessment::Submission::PublishTaskCompletionJob do
+  let(:instance) { Instance.default }
+  with_tenant(:instance) do
+    let(:course) { create(:course) }
+    let(:assessment) { create(:assessment, course: course) }
+    let(:course_student) { create(:course_student, course: course) }
+    let(:submission) do
+      create(:submission, :attempting, assessment: assessment,
+                                       creator: course_student.user, course_user: course_student)
+    end
+    subject { Course::Assessment::Submission::PublishTaskCompletionJob }
+
+    it 'can be queued' do
+      expect { subject.perform_later(submission) }.to have_enqueued_job(subject).exactly(:once)
+    end
+
+    it 'pushes the current status' do
+      allow(submission).to receive(:should_publish_task_completion?).and_return(true)
+      expect(submission).to receive(:publish_task_completion!)
+
+      subject.perform_now(submission)
+    end
+
+    # Re-read at run time rather than trusted from enqueue time, so a superseded push is dropped.
+    it 'does not push when the submission no longer qualifies' do
+      allow(submission).to receive(:should_publish_task_completion?).and_return(false)
+      expect(submission).not_to receive(:publish_task_completion!)
+
+      subject.perform_now(submission)
+    end
+
+    it 'lets a failed push propagate, so Sidekiq retries and Rollbar records it' do
+      allow(submission).to receive(:should_publish_task_completion?).and_return(true)
+      allow(submission).to receive(:publish_task_completion!).and_raise(StandardError, 'cikgo down')
+
+      expect { subject.perform_now(submission) }.to raise_error(StandardError, 'cikgo down')
+    end
+  end
+end

spec/jobs/course/lesson_plan/personalized_timeline_update_job_spec.rb

@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+RSpec.describe Course::LessonPlan::PersonalizedTimelineUpdateJob do
+  let(:instance) { Instance.default }
+  with_tenant(:instance) do
+    let(:course) { create(:course) }
+    let!(:submitted_assessment) do
+      create(:assessment, course: course, start_at: 2.days.ago, end_at: 3.days.from_now, published: true)
+    end
+    let!(:upcoming_assessment) do
+      create(:assessment, course: course, start_at: 5.days.from_now, end_at: 10.days.from_now, published: true)
+    end
+    let(:timeline_algorithm) { 'fomo' }
+    let(:course_user) { create(:course_user, course: course, timeline_algorithm: timeline_algorithm) }
+    let(:submission) do
+      create(:course_assessment_submission, assessment: submitted_assessment, creator: course_user.user)
+    end
+    subject { Course::LessonPlan::PersonalizedTimelineUpdateJob }
+
+    it 'can be queued' do
+      expect { subject.perform_later(course_user) }.to have_enqueued_job(subject).exactly(:once)
+    end
+
+    it 'is enqueued when a submission is finalised' do
+      expect { submission.finalise! }.to have_enqueued_job(subject).exactly(:once)
+    end
+
+    context 'when the course user is on a personalized timeline' do
+      it 'shifts the timeline for the course user', :sidekiq_same_thread do
+        submission.finalise!
+        submission.save!
+        expect(course_user.personal_times).to be_empty
+
+        perform_sidekiq_jobs { subject.perform_later(course_user) }
+
+        # The submitted item is never shifted, so only the upcoming one gets a personal time.
+        expect(course_user.personal_times.count).to eq(1)
+        expect(course_user.personal_times.first.lesson_plan_item_id).
+          to eq(upcoming_assessment.lesson_plan_item.id)
+      end
+
+      it 'is safe to run more than once', :sidekiq_same_thread do
+        submission.finalise!
+        submission.save!
+
+        perform_sidekiq_jobs { subject.perform_later(course_user) }
+        first_run = course_user.personal_times.pluck(:lesson_plan_item_id, :start_at, :end_at)
+
+        perform_sidekiq_jobs { subject.perform_later(course_user) }
+
+        # Idempotent, and — importantly for the offload — it never accumulates rows: the timeline is
+        # recomputed from current state rather than appended to.
+        expect(course_user.personal_times.reload.pluck(:lesson_plan_item_id, :start_at, :end_at)).
+          to match_array(first_run)
+      end
+    end
+
+    context 'when the course user is on the fixed timeline' do
+      let(:timeline_algorithm) { 'fixed' }
+
+      it 'creates no personal times', :sidekiq_same_thread do
+        submission.finalise!
+        submission.save!
+
+        perform_sidekiq_jobs { subject.perform_later(course_user) }
+
+        expect(course_user.personal_times).to be_empty
+      end
+    end
+  end
+end

spec/models/course/assessment/submission_spec.rb

@@ -344,6 +344,81 @@
         end
       end
 
+      # The finalise transaction commits the student's work, so nothing derived from it — the
+      # personalised timeline, the notification, the Cikgo push — may be able to roll it back. The
+      # first two also leave the request entirely, as jobs, because they call out to a third party
+      # or walk the whole course.
+      describe 'work deferred past the finalise transaction' do
+        # Created up front so that only the finalise itself is observed: creating a submission
+        # notifies too, and would otherwise land inside the transaction under test.
+        before { submission }
+
+        # The notification is only sent when the student submits for themselves, so the stamper has
+        # to be the creator — as it is on every path that reaches here in production.
+        def finalise
+          User.with_stamper(submission.creator) { submission.update!('finalise' => 'true') }
+        end
+
+        def rolled_back_finalise
+          User.with_stamper(submission.creator) do
+            ActiveRecord::Base.transaction do
+              submission.update!('finalise' => 'true')
+              raise ActiveRecord::Rollback
+            end
+          end
+        end
+
+        with_active_job_queue_adapter(:test) do
+          context 'when the course is pushed to Cikgo' do
+            before { allow(submission).to receive(:should_publish_task_completion?).and_return(true) }
+
+            it 'enqueues the Cikgo push instead of calling out during the request' do
+              expect(submission).not_to receive(:publish_task_completion!)
+
+              expect { finalise }.
+                to have_enqueued_job(Course::Assessment::Submission::PublishTaskCompletionJob).
+                exactly(:once)
+            end
+
+            it 'does not enqueue the push when the finalise is rolled back' do
+              expect { rolled_back_finalise }.
+                not_to have_enqueued_job(Course::Assessment::Submission::PublishTaskCompletionJob)
+            end
+          end
+
+          it 'enqueues the personalised timeline recomputation rather than running it inline' do
+            expect { finalise }.
+              to have_enqueued_job(Course::LessonPlan::PersonalizedTimelineUpdateJob).exactly(:once)
+          end
+
+          it 'does not enqueue the recomputation when the finalise is rolled back' do
+            expect { rolled_back_finalise }.
+              not_to have_enqueued_job(Course::LessonPlan::PersonalizedTimelineUpdateJob)
+          end
+        end
+
+        # Asserted through the activity record rather than the notifier, which is reached through
+        # Notifier::Base.method_missing and so cannot be a verified double. The notification stays on
+        # the request — its own work is enqueuing mail delivery — so this only checks that it is out
+        # of the transaction.
+        it 'writes the submission activity only once the transaction has committed' do
+          activities = Activity.count
+
+          User.with_stamper(submission.creator) do
+            ActiveRecord::Base.transaction do
+              submission.update!('finalise' => 'true')
+              expect(Activity.count).to eq(activities)
+            end
+          end
+
+          expect(Activity.count).to eq(activities + 1)
+        end
+
+        it 'does not write the activity when the finalise is rolled back' do
+          expect { rolled_back_finalise }.not_to change(Activity, :count)
+        end
+      end
+
       context 'when one of the answers is finalised' do
         before do
           answer = submission.answers.sample

spec/support/active_job.rb

@@ -122,6 +122,17 @@ def clear_enqueued_jobs
     ActiveJob::Base.queue_adapter.clear_enqueued_jobs
   end
 
+  # Blocks until every job enqueued so far has finished.
+  #
+  # Unlike +wait_for_job+, this does not skip the example. Use it when setup enqueues work whose
+  # effects the example depends on — the background thread adapter otherwise runs those jobs
+  # concurrently with the example body, and only joins them once the example is over.
+  def wait_for_enqueued_jobs
+    return unless ActiveJob::Base.queue_adapter.is_a?(ActiveJob::QueueAdapters::BackgroundThreadAdapter)
+
+    ActiveJob::Base.queue_adapter.wait_for_jobs
+  end
+
   # Polls until at least +count+ emails have been delivered, or the Capybara wait time elapses. Under
   # the :sidekiq_separate_thread harness a mail job enqueued by the Capybara server thread is delivered
   # out-of-band by the worker thread, so feature specs must wait for it rather than assert immediately.