From be2422ed960c699499dd5d65e997d8a9862acb73 Mon Sep 17 00:00:00 2001 From: Toon Willems Date: Thu, 31 Oct 2024 15:52:03 +0100 Subject: [PATCH] feat(InvoiceError) - Add InvoiceError (#2763) ## Description When invoices fails to generate it's hard to understand what's going wrong. We're now automatically retrying invoices that are stuck in generating state. We will also create InvoiceError objects for invoices that fail to generate and finalize. --- app/jobs/bill_subscription_job.rb | 8 +-- ...ry_generating_subscription_invoices_job.rb | 26 ++++++++ app/models/clickhouse/events_raw.rb | 11 ++-- app/models/invoice_error.rb | 27 ++++++++ clock.rb | 5 ++ .../20241031102231_create_invoice_errors.rb | 14 +++++ db/schema.rb | 11 +++- spec/clockwork_spec.rb | 21 +++++++ spec/jobs/bill_subscription_job_spec.rb | 22 ++++++- ...nerating_subscription_invoices_job_spec.rb | 63 +++++++++++++++++++ spec/models/invoice_error_spec.rb | 42 +++++++++++++ 11 files changed, 238 insertions(+), 12 deletions(-) create mode 100644 app/jobs/clock/retry_generating_subscription_invoices_job.rb create mode 100644 app/models/invoice_error.rb create mode 100644 db/migrate/20241031102231_create_invoice_errors.rb create mode 100644 spec/jobs/clock/retry_generating_subscription_invoices_job_spec.rb create mode 100644 spec/models/invoice_error_spec.rb diff --git a/app/jobs/bill_subscription_job.rb b/app/jobs/bill_subscription_job.rb index 33a8ff482f1..8b500947407 100644 --- a/app/jobs/bill_subscription_job.rb +++ b/app/jobs/bill_subscription_job.rb @@ -19,10 +19,10 @@ def perform(subscriptions, timestamp, invoicing_reason:, invoice: nil, skip_char return if tax_error?(result) # If the invoice was passed as an argument, it means the job was already retried (see end of function) - result.raise_if_error! if invoice - - # If the invoice is in a retryable state, we'll re-enqueue the job manually, otherwise the job fails - result.raise_if_error! unless result.invoice&.generating? + if invoice || !result.invoice&.generating? + InvoiceError.create_for(invoice: result.invoice, error: result.error) + return result.raise_if_error! + end # On billing day, we'll retry the job further in the future because the system is typically under heavy load is_billing_date = invoicing_reason.to_sym == :subscription_periodic diff --git a/app/jobs/clock/retry_generating_subscription_invoices_job.rb b/app/jobs/clock/retry_generating_subscription_invoices_job.rb new file mode 100644 index 00000000000..51ab54b19e5 --- /dev/null +++ b/app/jobs/clock/retry_generating_subscription_invoices_job.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Clock + class RetryGeneratingSubscriptionInvoicesJob < ApplicationJob + include SentryCronConcern + + queue_as 'clock' + + THRESHOLD = -> { 1.day.ago } + + def perform + Invoice.subscription.generating.where.not(id: InvoiceError.select(:id)).where('created_at < ?', THRESHOLD.call).find_each do |invoice| + next unless invoice.invoice_subscriptions.any? + invoicing_reasons = invoice.invoice_subscriptions.pluck(:invoicing_reason).uniq + invoicing_reason = (invoicing_reasons.size == 1) ? invoicing_reasons.first : :upgrading + BillSubscriptionJob.perform_later( + subscriptions: invoice.subscriptions.to_a, + timestamp: invoice.invoice_subscriptions.first.timestamp, + invoicing_reason:, + invoice:, + skip_charges: invoice.skip_charges + ) + end + end + end +end diff --git a/app/models/clickhouse/events_raw.rb b/app/models/clickhouse/events_raw.rb index ca229a240cd..4673fbe9733 100644 --- a/app/models/clickhouse/events_raw.rb +++ b/app/models/clickhouse/events_raw.rb @@ -50,13 +50,12 @@ def organization # # Table name: events_raw # -# code :string not null, primary key -# ingested_at :datetime not null +# code :string not null # precise_total_amount_cents :decimal(40, 15) # properties :string not null -# timestamp :datetime not null, primary key +# timestamp :datetime not null # external_customer_id :string not null -# external_subscription_id :string not null, primary key -# organization_id :string not null, primary key -# transaction_id :string not null, primary key +# external_subscription_id :string not null +# organization_id :string not null +# transaction_id :string not null # diff --git a/app/models/invoice_error.rb b/app/models/invoice_error.rb new file mode 100644 index 00000000000..35bf30fd1fe --- /dev/null +++ b/app/models/invoice_error.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class InvoiceError < ApplicationRecord + # NOTE! Invoice errors will have the same id as the invoice they belong to. + def self.create_for(invoice:, error:) + return unless invoice + + create(id: invoice.id, + backtrace: error.backtrace, + error: error.inspect.to_json, + invoice: invoice.to_json(except: :file), + subscriptions: invoice.subscriptions.to_json) + end +end + +# == Schema Information +# +# Table name: invoice_errors +# +# id :uuid not null, primary key +# backtrace :text +# error :json +# invoice :json +# subscriptions :json +# created_at :datetime not null +# updated_at :datetime not null +# diff --git a/clock.rb b/clock.rb index f32116ad8f7..b9e47fa27f0 100644 --- a/clock.rb +++ b/clock.rb @@ -58,6 +58,11 @@ module Clockwork .set(sentry: {"slug" => 'lago_bill_customers', "cron" => '10 */1 * * *'}) .perform_later end + every(1.hour, 'schedule:retry_generating_subscription_invoices', at: '*:30') do + Clock::RetryGeneratingSubscriptionInvoicesJob + .set(sentry: {"slug" => 'lago_retry_invoices', "cron" => '30 */1 * * *'}) + .perform_later + end every(1.hour, 'schedule:finalize_invoices', at: '*:20') do Clock::FinalizeInvoicesJob diff --git a/db/migrate/20241031102231_create_invoice_errors.rb b/db/migrate/20241031102231_create_invoice_errors.rb new file mode 100644 index 00000000000..163c01e43ce --- /dev/null +++ b/db/migrate/20241031102231_create_invoice_errors.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateInvoiceErrors < ActiveRecord::Migration[7.1] + def change + create_table :invoice_errors, id: :uuid do |t| + t.text :backtrace + t.json :invoice + t.json :subscriptions + t.json :error + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index d1000861bef..569b3e6d5be 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[7.1].define(version: 2024_10_25_081408) do +ActiveRecord::Schema[7.1].define(version: 2024_10_31_102231) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -779,6 +779,15 @@ t.index ["token"], name: "index_invites_on_token", unique: true end + create_table "invoice_errors", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.text "backtrace" + t.json "invoice" + t.json "subscriptions" + t.json "error" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "invoice_metadata", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "invoice_id", null: false t.string "key", null: false diff --git a/spec/clockwork_spec.rb b/spec/clockwork_spec.rb index 170604c2e3b..74efa5e1e73 100644 --- a/spec/clockwork_spec.rb +++ b/spec/clockwork_spec.rb @@ -115,6 +115,27 @@ end end + describe 'schedule:retry_generating_subscription_invoices' do + let(:job) { 'schedule:retry_generating_subscription_invoices' } + let(:start_time) { Time.zone.parse('1 Apr 2022 00:01:00') } + let(:end_time) { Time.zone.parse('1 Apr 2022 01:01:00') } + + it 'enqueues a Clock::RetryGeneratingSubscriptionInvoiceJob' do + Clockwork::Test.run( + file: clock_file, + start_time:, + end_time:, + tick_speed: 1.second + ) + + expect(Clockwork::Test).to be_ran_job(job) + expect(Clockwork::Test.times_run(job)).to eq(1) + + Clockwork::Test.block_for(job).call + expect(Clock::RetryGeneratingSubscriptionInvoicesJob).to have_been_enqueued + end + end + describe 'schedule:compute_daily_usage' do let(:job) { 'schedule:compute_daily_usage' } let(:start_time) { Time.zone.parse('1 Apr 2022 00:01:00') } diff --git a/spec/jobs/bill_subscription_job_spec.rb b/spec/jobs/bill_subscription_job_spec.rb index e9d99abfed0..81ef36a04f3 100644 --- a/spec/jobs/bill_subscription_job_spec.rb +++ b/spec/jobs/bill_subscription_job_spec.rb @@ -24,7 +24,9 @@ context 'when result is a failure' do let(:result) do - BaseService::Result.new.single_validation_failure!(error_code: 'error') + result = BaseService::Result.new + result.invoice = invoice + result.single_validation_failure!(error_code: 'error') end it 'raises an error' do @@ -57,6 +59,15 @@ expect(Invoices::SubscriptionService).to have_received(:call) end + + it 'creates an InvoiceError' do + expect do + described_class.perform_now(subscriptions, timestamp, invoicing_reason:, invoice:) + end.to raise_error(BaseService::FailedResult) + + expect(InvoiceError.all.size).to eq(1) + expect(InvoiceError.first.id).to eq(invoice.id) + end end context 'when a generating invoice is attached to the result' do @@ -86,6 +97,15 @@ expect(Invoices::SubscriptionService).to have_received(:call) end + + it 'creates an InvoiceError' do + expect do + described_class.perform_now(subscriptions, timestamp, invoicing_reason:) + end.to raise_error(BaseService::FailedResult) + + expect(InvoiceError.all.size).to eq(1) + expect(InvoiceError.first.id).to eq(result_invoice.id) + end end end end diff --git a/spec/jobs/clock/retry_generating_subscription_invoices_job_spec.rb b/spec/jobs/clock/retry_generating_subscription_invoices_job_spec.rb new file mode 100644 index 00000000000..3a0591d50b8 --- /dev/null +++ b/spec/jobs/clock/retry_generating_subscription_invoices_job_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Clock::RetryGeneratingSubscriptionInvoicesJob, job: true do + subject { described_class } + + describe '.perform' do + let(:old_generating_invoice) { create(:invoice, :generating, created_at: 5.days.ago) } + + before do + old_generating_invoice + end + + it "does not enqueue a BillSubscriptionJob for this invoice (missing subscriptions)" do + expect do + described_class.perform_now + end.not_to have_enqueued_job(BillSubscriptionJob) + end + + context "with an actual invoice that should be retried" do + let(:old_generating_invoice) { create(:invoice, :subscription, created_at: 5.days.ago) } + + before do + old_generating_invoice.update(status: :generating) + end + + it "does enqueue a BillSubscriptionJob for this invoice " do + expect do + described_class.perform_now + end.to have_enqueued_job(BillSubscriptionJob) + end + + context "with an existing invoice error" do + let(:invoice_error) { InvoiceError.create(id: old_generating_invoice.id) } + + before do + invoice_error + end + + it "does not enqueue a BillSubscriptionJob for this invoice" do + expect do + described_class.perform_now + end.not_to have_enqueued_job(BillSubscriptionJob) + end + end + end + + context "with an addon" do + let(:old_generating_invoice) { create(:invoice, :add_on, created_at: 5.days.ago) } + + before do + old_generating_invoice.update(status: :generating) + end + + it "does not enqueue a BillSubscriptionJob for this invoice" do + expect do + described_class.perform_now + end.not_to have_enqueued_job(BillSubscriptionJob) + end + end + end +end diff --git a/spec/models/invoice_error_spec.rb b/spec/models/invoice_error_spec.rb new file mode 100644 index 00000000000..2b234c763b5 --- /dev/null +++ b/spec/models/invoice_error_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe InvoiceError, type: :model do + let(:invoice) { create(:invoice, :generating) } + let(:result) { BaseService::Result.new } + let(:error) { BaseService::ValidationFailure.new(result, messages: messages) } + let(:messages) { ["message1", "message2"] } + + let(:error_with_backtrace) do + error = OpenStruct.new + error.backtrace = "backtrace" + error + end + + describe ".create_for" do + it "does nothing if the invoice is nil" do + expect(described_class.create_for(invoice: nil, error:)).to eq(nil) + end + + it "creates an invoice error with the same id as the invoice" do + invoice_error = described_class.create_for(invoice:, error:) + expect(invoice_error.id).to eq(invoice.id) + end + + it "stores the error in the error field" do + invoice_error = described_class.create_for(invoice:, error:) + expect(invoice_error.error).to eq(error.inspect.to_json) + end + + it "stores the backtrace in the backtrace field" do + invoice_error = described_class.create_for(invoice:, error: error_with_backtrace) + expect(invoice_error.backtrace).to eq("backtrace") + end + + it "stores the subscriptions in the subscriptions field" do + invoice_error = described_class.create_for(invoice:, error:) + expect(invoice_error.subscriptions).to eq("[]") + end + end +end