billing errors and warnings, stripe and orb webhook, river worker for async background jobs #5440
Conversation
pjain1
force-pushed
the
payment_webhook
branch
from
7f13627
to
65f2670
Compare
pjain1
changed the title
clean up
pjain1
changed the title
proto/rill/admin/v1/api.proto
Outdated
| message BillingErrorMetadata { | ||
| oneof metadata { | ||
| BillingErrorMetadataNoPaymentMethod no_payment_method = 1; | ||
| BillingErrorMetadataNoBillableAddress no_billable_address = 2; | ||
| BillingErrorMetadataInvoicePaymentFailed invoice_payment_failed = 3; | ||
| BillingErrorMetadataTrialEnded trial_ended = 4; | ||
| BillingErrorMetadataSubscriptionCancelled subscription_cancelled = 5; | ||
| } | ||
| } | ||
|
|
||
| message BillingErrorMetadataNoPaymentMethod {} | ||
|
|
||
| message BillingErrorMetadataNoBillableAddress {} | ||
|
|
||
| message BillingErrorMetadataInvoicePaymentFailed { | ||
| repeated InvoicePaymentFailedMeta invoices = 1; | ||
| } | ||
|
|
||
| message InvoicePaymentFailedMeta { | ||
| string invoice_id = 1; | ||
| string invoice_number = 2; | ||
| string invoice_url = 3; | ||
| string amount_due = 4; | ||
| string currency = 5; | ||
| google.protobuf.Timestamp due_date = 6; | ||
| } | ||
|
|
||
| message BillingErrorMetadataTrialEnded { | ||
| google.protobuf.Timestamp grace_period_end_date = 1; | ||
| } | ||
|
|
||
| message BillingErrorMetadataSubscriptionCancelled { | ||
| google.protobuf.Timestamp end_date = 1; | ||
| } |
There was a problem hiding this comment.
Just wondering from a simplicity point of view whether this info could be simplified to a string message? Or does the CLI/frontend need it destructured like this?
There was a problem hiding this comment.
Earlier I had string message but found that this to be would be better for UI if they want to create call to action. For example failed invoice meta contains url that can directly take them to the failed invoice page and they can pay. Or use some of the end dates to perform something.
There was a problem hiding this comment.
For a CTA, couldn't that be managed with either just an error code and message, or message and a cta_url? Or does the frontend need deeper understanding of the error?
There was a problem hiding this comment.
Still feel its better to have it like this. UI can choose to create their own message. Another example on how these fields can be used is trial ending soon metadata contains trial end date or sub cancel error metadata contains sub end date so the UI can choose to display some banner or pop up on the end date or sometime before end date etc. Don't think it will be complicated for UI to use these to create flows as per product requirement since its typed metadata instead of json blobs.
| if s.webhookSecret == "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Isn't it a panic risk to mount a nil handler? Maybe safer to return a handler that does nothing
There was a problem hiding this comment.
// WebhookHandlerFunc returns a http.HandlerFunc that can be used to handle incoming webhooks from the payment provider. Return nil if you don't want to register any webhook handlers.
Its documented and returning a handler that just does nothing returns nil, would send http ok and webhook events will be just lost. Instead a handler like this needs to be mounted
return func(w http.ResponseWriter, r *http.Request) error {
return httputil.Errorf(http.StatusServiceUnavailable, "webhook secret not set")
}
Let me know if this should be done
admin/jobs/river/river.go
Outdated
| InvoiceID: invoiceID, | ||
| GracePeriodEndDate: gracePeriodEndDate, | ||
| }, &river.InsertOpts{ | ||
| ScheduledAt: gracePeriodEndDate.AddDate(0, 0, 1).Add(1 * time.Hour), // end of grace period date + 1 hour buffer |
There was a problem hiding this comment.
Unfortunately I think we need to reassess the use of ScheduledAt. Having jobs scheduled far in the future makes them much harder to maintain because you have to think about backwards compatibility and job migrations. There's also issues of jobs going stale that needs to be cancelled or correctly ignored upon invocation.
For example, if you look at Temporal, which is a more advanced background job system, you'll see they devote a lot of docs to discussing versioning and compatibility issues for these kinds of issues, and have some advanced features for dealing with them.
Looking at the use of ScheduledAt, it seems like these cases could relatively easily be moved into a cron job? For example, for this InvoicePaymentFailedGracePeriodCheck check, you could have an payment_failed_grace_period_ends_on timestamp in the orgs table, and have a cron job that grabs all rows where it is in the past and processes them. It is effectively the same approach, but makes the job itself stateless and easy to change.
There was a problem hiding this comment.
ok I see, makes sense. May be we can have cron jobs for all the billing errors/warning that requires some processing in future. When they wake up (mostly past every midnight) they look for all the orgs with that billing error and checks if its action time and takes action. We will need to maintain a flag if the billing error is processed or not to prevent reprocessing.
The problem you mentioned about backwards compatibility and job migrations applies somewhat for immediate scheduled jobs as well, for this we can just make sure that worker does graceful shutdown during upgrades and allows current running jobs to finish.
Fixes #5467, #5466 and https://github.com/rilldata/rill-private-issues/issues/527