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

feat(Threads): Refresh Threads access tokens automatically. #629

Merged
merged 13 commits into from
Nov 14, 2024

Conversation

elisa-a-v
Copy link
Contributor

@elisa-a-v elisa-a-v commented Nov 5, 2024

Resolves #627

Long-lived tokens need to be at least 24 hours old to be refreshed so upon creation of a new Threads channel, a post_save signal enqueues a job to refresh the token in 2 days. After that, we enqueue a new job in as many seconds as the expires_in value from the token refresh response.

bc/channel/tasks.py Outdated Show resolved Hide resolved
bc/channel/tasks.py Outdated Show resolved Hide resolved
bc/channel/tasks.py Outdated Show resolved Hide resolved
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks generally OK, but I think we need something else to trigger expiration because this creates an ongoing chain of refreshes that's bound to break eventually. If just one refresh fails for whatever reason (say threads is down?), we'll never refresh again and we'll be sad.

Could you check the token's expiration every time you do a post and trigger a refresh if you're getting close to the deadline? You could get fancy and put a semaphore in redis to prevent two or more simultaneous refresh tasks, but I'm not sure it matters. Ideally we're sending one or two posts per hour, say, and so you get:

  1. New post
  2. Refresh task queued
  3. Refresh task completed
  4. Next post

You could have two or more posts at the same time, I suppose, which would lead to:

  1. New post
  2. First refresh task queued
  3. Next post
  4. Second refresh task queued
  5. First refresh complete
  6. Second refresh complete

That's fine too, right?

@elisa-a-v
Copy link
Contributor Author

We did consider an approach like the one you're suggesting but the only way to get the tokens' expiration is via token retrieval, so we have no way of getting the expiration from the API when publishing posts.

@ERosendo and I were discussing some alternatives to achieve something similar by storing the expiration date somewhere (so we can check that every time we do a post), either in our database, or on redis, and they both have their pros and cons:

  1. Storing in database:
    • Pro: we can tweak the script just a little bit to add the expiration date for the admin to create the channel using that right away.
    • Con: we need to either add a column to the Channel model (which would only be used by Threads channels, so not ideal), or maybe create a new model with a FK to the channel (which is an entire model just for this, so I guess also not ideal? not too bad imo but def not very nice)
  2. Storing in redis:
    • Pro: no need to change the database schema.
    • Cons:
      • More volatile? we'd have to handle persistence.
      • More changes required: we would probably need to run the script as a command within django instead of a standalone python script.

Another alternative entirely could be to keep the chain of refreshes, and add redundancy by using a lazy refresh on post if we get an authentication error from Threads API. Of course this also has its pros and cons:

  • Pro: minimal changes? no need to store anything new.
  • Cons:
    • more API calls could potentially be an issue, although the limit seems to be >48.000 API calls in a 24h period so I think we're fine.
    • Better error handling would be needed.
    • UX could be affected if posts take too long to be posted on Threads, but I'm not sure it would be too bad? Probably less than a few minutes long difference, which sounds reasonable to me for the purposes of a social media bot, but you tell me.
    • The race condition seems even worse with this one? Not entirely sure why

I personally like the redis option because it seems more elegant and it's something I'd like to learn about, but the database option with a new model seems like the safest bet? It's probably best for persistence and reliability. The redundancy alternative is not my favorite but it seems simpler to implement at first glance.

What do you think?

@mlissner
Copy link
Member

mlissner commented Nov 7, 2024

Redis is plenty persistent for something like this, and you could have a rule that says, "If the key is missing, just refresh the key." If you do that, I think you'd be good to go without having to fiddle with the models.

We do have the redis CLI installed and the redis Python module is available, so you wouldn't need to convert it to a Django command to just make/check a key.

… logic

- Eliminated chained enqueued tasks used for token refreshing in Threads integration.
- Moved token validation and refresh logic into the Channel model's validate_access_token method.
- Adjusted ThreadsAPI methods to handle token expiration and refreshing internally.
- Implemented Redis-based locking mechanism to prevent concurrent token refreshes.
@elisa-a-v
Copy link
Contributor Author

Ok so instead of chainging tasks, we're now checking token validity before posting by getting the expiration from redis. If no expiration date is stored, or the expiration date is in less than 2 days, then we try to refresh the token.

This is done with a new protocol that extends the BaseAPIConnector to add a new method for token validation, which we now call from a Channel model method before attempting to post.

@elisa-a-v elisa-a-v self-assigned this Nov 11, 2024
bc/channel/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ERosendo ERosendo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Just a few minor tweaks before merging.

@ERosendo ERosendo self-requested a review November 14, 2024 18:25
@mlissner mlissner merged commit e206fd4 into main Nov 14, 2024
10 checks passed
@mlissner mlissner deleted the threads-refresh-access-token branch November 14, 2024 22:06
@mlissner
Copy link
Member

Lovely, merged!

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

Successfully merging this pull request may close these issues.

Implement refresh mechanism for Threads access tokens
3 participants