diff --git a/.gitignore b/.gitignore index 063233c..4326e70 100644 --- a/.gitignore +++ b/.gitignore @@ -30,3 +30,5 @@ # Ignore master key for decrypting credentials and more. /config/master.key + +*.DS_Store diff --git a/.tool-versions b/.tool-versions new file mode 100644 index 0000000..773d7a5 --- /dev/null +++ b/.tool-versions @@ -0,0 +1 @@ +ruby 3.3.9 diff --git a/app/controllers/certificate_quantities_controller.rb b/app/controllers/certificate_quantities_controller.rb index 648ba0c..e0bf735 100644 --- a/app/controllers/certificate_quantities_controller.rb +++ b/app/controllers/certificate_quantities_controller.rb @@ -60,7 +60,7 @@ def transfer @certificate_quantity.update(account: account) elsif organization_id organization = Organization.find(organization_id) - @certificate_quantity.update(status: "intransit", to_organization: organization) + @certificate_quantity.update(status: "intransit", to_organization: organization, transfer_initiated_at: Time.zone.now) end render "show" @@ -74,7 +74,7 @@ def cancel_transfer return head :unprocessable_entity end - @certificate_quantity.update(status: "active", to_organization: nil) + @certificate_quantity.update(status: "active", to_organization: nil, transfer_initiated_at: nil) render "show" end @@ -87,7 +87,7 @@ def accept_transfer return head :unprocessable_entity end - @certificate_quantity.update(status: "active", to_organization: nil, account: current_user.organization.default_account) + @certificate_quantity.update(status: "active", to_organization: nil, transfer_initiated_at: nil, account: current_user.organization.default_account) render "show" end diff --git a/app/jobs/cancel_stale_transfers_job.rb b/app/jobs/cancel_stale_transfers_job.rb new file mode 100644 index 0000000..2f83271 --- /dev/null +++ b/app/jobs/cancel_stale_transfers_job.rb @@ -0,0 +1,22 @@ +class CancelStaleTransfersJob < ApplicationJob + queue_as :default + + # Prevents multiple instances from running simultaneously, allow if a job is still running after 60 minutes + limits_concurrency to: 1, key: -> { "cancel_stale_transfers" }, duration: 60.minutes + + def perform + stale_certificate_quantities.find_each do |certificate_quantity| + certificate_quantity.cancel_transfer! + end + end + + private + + def stale_certificate_quantities + CertificateQuantity.where(status: "intransit").where("transfer_initiated_at < ?", stale_cutoff) + end + + def stale_cutoff + 24.hours.ago + end +end diff --git a/app/models/certificate_quantity.rb b/app/models/certificate_quantity.rb index 5aec70d..2fb0125 100644 --- a/app/models/certificate_quantity.rb +++ b/app/models/certificate_quantity.rb @@ -11,4 +11,31 @@ def split(quantity) def retire update(status: "retired") end + + def cancel_transfer! + log_transfer_cancellation + update!(status: "active", to_organization: nil, transfer_initiated_at: nil) + end + + private + + def log_transfer_cancellation + Rails.logger.info { { + msg: "Transfer timeout, cancelling transfer for certificate quantity ID: #{id}.", + id: id, + to_organization: to_organization + } } + + # Not implemented, log to Transactions table. + # The above log might be in a service object that both logs and creates the Transaction object + # so it's not this model's responsibility to know how to do this. + # + # Transaction.create!( + # :auto_cancel_stale_transfer, + # certificate_quantity: certificate_quantity.id, + # from_status: "intransit", + # to_status: "active", + # to_organization: certificate_quantity.to_organization + # ) + end end diff --git a/config/recurring.yml b/config/recurring.yml index b4207f9..22f5db4 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -13,3 +13,8 @@ production: clear_solid_queue_finished_jobs: command: "SolidQueue::Job.clear_finished_in_batches(sleep_between_batches: 0.3)" schedule: every hour at minute 12 + + cancel_stale_transfers: + class: CancelStaleTransfersJob + schedule: every 5 minutes + \ No newline at end of file diff --git a/db/migrate/20250914010837_add_transfer_initiated_at_to_certificate_quantities.rb b/db/migrate/20250914010837_add_transfer_initiated_at_to_certificate_quantities.rb new file mode 100644 index 0000000..3d1d871 --- /dev/null +++ b/db/migrate/20250914010837_add_transfer_initiated_at_to_certificate_quantities.rb @@ -0,0 +1,6 @@ +class AddTransferInitiatedAtToCertificateQuantities < ActiveRecord::Migration[8.0] + def change + add_column :certificate_quantities, :transfer_initiated_at, :datetime + add_index :certificate_quantities, :transfer_initiated_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 495163c..29e108e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2025_08_01_025252) do +ActiveRecord::Schema[8.0].define(version: 2025_09_14_010837) do create_table "accounts", force: :cascade do |t| t.string "name" t.integer "organization_id" @@ -23,9 +23,11 @@ t.integer "account_id" t.string "status" t.integer "to_organization_id" + t.datetime "transfer_initiated_at" t.index ["account_id"], name: "index_certificate_quantities_on_account_id" t.index ["certificate_id"], name: "index_certificate_quantities_on_certificate_id" t.index ["to_organization_id"], name: "index_certificate_quantities_on_to_organization_id" + t.index ["transfer_initiated_at"], name: "index_certificate_quantities_on_transfer_initiated_at" end create_table "certificates", force: :cascade do |t| diff --git a/exercise_3_discussion.md b/exercise_3_discussion.md new file mode 100644 index 0000000..94b19a4 --- /dev/null +++ b/exercise_3_discussion.md @@ -0,0 +1,27 @@ +# Exercise 3 Implementation + +## Assumptions +1. A stale transfer is considered any transfer that is still status: `intransit` for > 24 hours. +2. The number of stale transfers in a given day is < 1M +3. There is not a particular time of day we want to cancel `intransit` stale transfers en masse, we want to cancel ASAP after 24 hours of time in transit. +4. There are not going to be model level validations added to the currently sparse validations on `CertificateQuantity` that would cause an automated update to fail. + +## Discussion + +I added a new column `certificate_quantities.transfer_initiated_at`. If exercise 1 were implemented, +this might be a simpler column name like `status_updated_at` but for this exercise I figured let's be precise. +This column must be updated when a transfer is initiated, accepted, or cancelled. + +We're kicking off a recurring SolidQueue job `CancelStaleTransfersJob` every 5 minutes to catch any `CertificateQuantities` in transit that have +gone stale in the past 5 minutes. We've limited the concurrency to 1 so even if this job runs long, it shouldn't overlap +with another recurring job run. It will allow another job to run if one job is stuck for > 1 hr. + +## Extensions + +### We want this to cancel intransit transfers immediately at our 24h threshold +We could enqueue a job to cancel the transfer at 24h exactly after the transfer was initiated, and if the CertificateQuantity is no longer `intransit`, the job is a noop. + +### We have millions of rows of transfers to cancel and this is always taking > 5 min +I would first try implementing a fanout strategy, where the periodic job `FindStaleTransfersJob` enqueues n other jobs `CancelStaleTransferJob`. `CancelStaleTransferJob` would take an argument for a `certificate_quantity_id`. Each `CancelStaleTransferJob` job would be responsible for +canceling that stale transfer. If the state has changed and the certificate_quantity is no longer `intransit`, it would noop and return early. +We would enqueue the jobs using bulk enqueuing to reduce roundtrips to the database: https://guides.rubyonrails.org/active_job_basics.html#bulk-enqueuing diff --git a/spec/jobs/cancel_stale_transfers_job_spec.rb b/spec/jobs/cancel_stale_transfers_job_spec.rb new file mode 100644 index 0000000..4ffc8b1 --- /dev/null +++ b/spec/jobs/cancel_stale_transfers_job_spec.rb @@ -0,0 +1,50 @@ +require "rails_helper" + +RSpec.describe "CancelStaleTransfersJob", type: :job do + def create_certificate_quantity + generation = create(:generation) + certificate = generation.certificate + certificate_quantity = certificate.certificate_quantities.first + certificate_quantity.update!( + status: "intransit", + to_organization: create(:organization), + transfer_initiated_at: 25.hours.ago + ) + certificate_quantity + end + + it "resets intransit certificate quantities in transit for over 24 hours" do + certificate_quantity = create_certificate_quantity + + CancelStaleTransfersJob.perform_now + + certificate_quantity.reload + expect(certificate_quantity.status).to eq("active") + expect(certificate_quantity.transfer_initiated_at).to be_nil + expect(certificate_quantity.to_organization).to be_nil + end + + it "does not reset intransit certificate quantities in transit for under 24 hours" do + certificate_quantity = create_certificate_quantity + certificate_quantity.update!(transfer_initiated_at: 23.hours.ago) + + CancelStaleTransfersJob.perform_now + + certificate_quantity.reload + expect(certificate_quantity.status).to eq("intransit") + expect(certificate_quantity.transfer_initiated_at).to be_present + expect(certificate_quantity.to_organization).to be_present + end + + it "does not reset active certificate quantities" do + certificate_quantity = create_certificate_quantity + certificate_quantity.update!(transfer_initiated_at: nil, status: "active", to_organization: nil) + + CancelStaleTransfersJob.perform_now + + certificate_quantity.reload + expect(certificate_quantity.status).to eq("active") + expect(certificate_quantity.transfer_initiated_at).to be_nil + expect(certificate_quantity.to_organization).to be_nil + end +end diff --git a/spec/models/certificate_quantity_spec.rb b/spec/models/certificate_quantity_spec.rb new file mode 100644 index 0000000..4adcdce --- /dev/null +++ b/spec/models/certificate_quantity_spec.rb @@ -0,0 +1,36 @@ +require 'rails_helper' + +describe "CertificateQuantity" do + describe "#cancel_transfer!" do + let(:other_organization) { create(:organization) } + let(:generation) { create(:generation, quantity: 5, generator: create(:generator)) } + let(:certificate_quantity) { generation.certificate.certificate_quantities.first } + + before do + certificate_quantity.update!( + status: "intransit", + to_organization: other_organization, + transfer_initiated_at: 1.hour.ago + ) + certificate_quantity.cancel_transfer! + end + + it "resets the status to active" do + expect(certificate_quantity.reload.status).to eq("active") + end + + it "clears the to_organization" do + expect(certificate_quantity.reload.to_organization).to be_nil + end + + it "clears the transfer_initiated_at timestamp" do + expect(certificate_quantity.reload.transfer_initiated_at).to be_nil + end + + it "logs the transfer cancellation" do + expect(Rails.logger).to receive(:info) + + certificate_quantity.cancel_transfer! + end + end +end diff --git a/spec/requests/certificate_quantities_request_spec.rb b/spec/requests/certificate_quantities_request_spec.rb index ee3670d..9108327 100644 --- a/spec/requests/certificate_quantities_request_spec.rb +++ b/spec/requests/certificate_quantities_request_spec.rb @@ -157,6 +157,10 @@ def certificate_quantity_json(certificate_quantity) it 'modifies the to_organization' do expect(certificate_quantity.reload.to_organization).to eq(other_organization) end + + it 'sets the transfer_initiated_at timestamp' do + expect(certificate_quantity.reload.transfer_initiated_at).to be_within(1.minute).of(Time.zone.now) + end end context "when passing invalid parameters" do @@ -204,7 +208,7 @@ def certificate_quantity_json(certificate_quantity) context 'cancel_transfer' do context "when cancelling a transfer that is associated with the user's organization" do before do - certificate_quantity.update(status: 'intransit') + certificate_quantity.update(status: 'intransit', to_organization: other_organization, transfer_initiated_at: 1.hour.ago) put "/certificate_quantities/#{certificate_quantity.id}/cancel_transfer", headers: headers end @@ -224,6 +228,10 @@ def certificate_quantity_json(certificate_quantity) it 'clears out the to_organization' do expect(certificate_quantity.reload.to_organization).to be_nil end + + it 'clears out the transfer_initiated_at timestamp' do + expect(certificate_quantity.reload.transfer_initiated_at).to be_nil + end end context 'when the status is not intransit' do @@ -236,7 +244,7 @@ def certificate_quantity_json(certificate_quantity) context "when canceling a transfer of a certificate to another organization that is associated with the user's organization" do before do - certificate_quantity.update(status: 'intransit', to_organization: other_organization) + certificate_quantity.update(status: 'intransit', to_organization: other_organization, transfer_initiated_at: 1.hour.ago) put "/certificate_quantities/#{certificate_quantity.id}/cancel_transfer", headers: headers end @@ -256,11 +264,15 @@ def certificate_quantity_json(certificate_quantity) it 'resets the to_organization' do expect(certificate_quantity.reload.to_organization).to be_nil end + + it 'resets the transfer_initiated_at timestamp' do + expect(certificate_quantity.reload.transfer_initiated_at).to be_nil + end end context "when transferring a certificate that is not associated with the user's organization" do before do - certificate_quantity.update(status: 'intransit', to_organization: other_organization) + certificate_quantity.update(status: 'intransit', to_organization: other_organization, transfer_initiated_at: 1.hour.ago) put "/certificate_quantities/#{other_certificate_quantity.id}/transfer", headers: headers end @@ -275,6 +287,10 @@ def certificate_quantity_json(certificate_quantity) it 'leaves the to_organization' do expect(certificate_quantity.reload.to_organization).to eq(other_organization) end + + it 'leaves the transfer_initiated_at timestamp' do + expect(certificate_quantity.reload.transfer_initiated_at).to be_present + end end end @@ -306,6 +322,10 @@ def certificate_quantity_json(certificate_quantity) expect(certificate_quantity.reload.to_organization).to be_nil end + it 'clears out the transfer_initiated_at timestamp' do + expect(certificate_quantity.reload.transfer_initiated_at).to be_nil + end + it 'sets the account to the organizations default account' do expect(certificate_quantity.reload.account).to eq(other_organization.default_account) end @@ -326,7 +346,7 @@ def certificate_quantity_json(certificate_quantity) context "when transferring a certificate that is not associated with the user's organization" do before do - certificate_quantity.update(status: 'intransit', to_organization: other_organization) + certificate_quantity.update(status: 'intransit', to_organization: other_organization, transfer_initiated_at: 1.hour.ago) put "/certificate_quantities/#{other_certificate_quantity.id}/transfer", headers: headers end @@ -341,6 +361,10 @@ def certificate_quantity_json(certificate_quantity) it 'leaves the to_organization' do expect(certificate_quantity.reload.to_organization).to eq(other_organization) end + + it 'leaves the transfer_initiated_at timestamp' do + expect(certificate_quantity.reload.transfer_initiated_at).to be_present + end end end