Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix expirein used instead of keepuntil #1788

Merged
merged 2 commits into from
Jan 3, 2025
Merged

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Jan 2, 2025

Description

We've been using expirein for reminder, delete and boost jobs far into the future. We thought expirein > startafter would make sure they don't disappear before they get run. However, expirein is for how long a job can stay in the active state:

Expiration options

  • expireInSeconds, number
    How many seconds a job may be in active state before it is failed because of expiration. Must be >=1

-- https://timgit.github.io/pg-boss/#/./api/jobs

The docs aren't clear, but I assume keepuntil is called "Retention options" in the docs and they state this:

Retention options

  • retentionSeconds, number
    How many seconds a job may be in created or retry state before it's archived. Must be >=1

So we want to use keepuntil instead of expirein for reminder, delete and boost jobs.

Our defaults via \d pgboss.job:

  • expirein: 15 minutes
  • keepuntil: 14 days

TODO:

  • reschedule reminders

since the jobs are just for the push notifications, I decided to not reschedule them to avoid duplicate push notifications for past or future reminders

  • reschedule deletes

won't do in this PR because I need an up-to-date db dump from prod after this was deployed

  • fix existing active boost jobs by updating keepuntil

added migration

Additional Context

We could add a constraint on pgboss.job to make sure that keepuntil > startafter since I think any such case is a bug.

Checklist

Are your changes backwards compatible? Please answer below:

  • some delete jobs have been lost (see TODO)
  • some reminders might not send push notifications but that's not a big deal
  • fixed existing expireBoost via update query in migrations

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

5. tested that reminders, deletes, expire boost, checkInvoice jobs are inserted without error.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

n/a

Did you introduce any new environment variables? If so, call them out explicitly here:

no

@ekzyis ekzyis added the bug label Jan 2, 2025
@ekzyis ekzyis marked this pull request as draft January 2, 2025 23:48
@ekzyis ekzyis force-pushed the fix-jobs-expirein-vs-keepuntil branch from ff0e1d1 to 5a0674b Compare January 3, 2025 00:04
@ekzyis ekzyis marked this pull request as ready for review January 3, 2025 01:52
@huumn huumn merged commit 077727d into master Jan 3, 2025
6 checks passed
@huumn huumn deleted the fix-jobs-expirein-vs-keepuntil branch January 3, 2025 14:52
@huumn
Copy link
Member

huumn commented Jan 3, 2025

The migration here doesn't work. The shadow db doesn't know about pgboss. I've removed it in the cowboy credit branch.

@ekzyis
Copy link
Member Author

ekzyis commented Jan 3, 2025

Oh, then I must have forgotten to run it with Prisma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants