Skip to content

Conversation

@rosa
Copy link
Member

@rosa rosa commented Nov 24, 2025

Motivation / Background

Today, while looking into some 500 errors from GET /rails/active_storage/representations/redirect/ requests, I noticed an unexpected behaviour in our logs. We were computing a variant for a previewable blob (for example, a video or a PDF, whose variants belong to their preview image), and then, after having uploaded it and stored it, we were trying to load it again from the database, failing to find it, then downloading the preview once more and processing the variant again, and finally failing to create the variant record, because it already existed. Turns out, the reason we couldn't find it the second time we tried to load it was that this was happening in the replica, whereas the new variant is created against the primary, by manually connecting to the writer:

@record =
ActiveRecord::Base.connected_to(role: ActiveRecord.writing_role) do
blob.variant_records.create_or_find_by!(variation_digest: variation.digest) do |record|
record.image.attach(image)
end
end

This PR avoids loading the record that was created within the request once more just to get the redirect URL. See below for more details.

Detail

Even though we track and memoize the just-created @record in ActiveStorage::VariantWithRecord when we call processed (which calls process), we always instantiate a new object when we call ActiveStorage::Blob#variant.

def variant(transformations)
if variable?
variant_class.new(self, ActiveStorage::Variation.wrap(transformations).default_to(default_variant_transformations))

If we only call this once, that's fine, but when processing the variant via GET /rails/active_storage/representations/redirect, we call this twice: one when setting the representation and processing it:

def set_representation
@representation = @blob.representation(params[:variation_key]).processed

and another one when calling @representation.url for the redirect:

redirect_to @representation.url(disposition: params[:disposition]), allow_other_host: true

which calls ActiveStorage::Preview#url, which ultimately calls ActiveStorage::VariantWithRecord.processed again, but on a new instance of ActiveStorage::VariantWithRecord. This checks whether the variant record exists (the one we just created in the previous call to processed), but does so against the replica, which is unlikely to find it if there's any delay, since these calls are very close together.

With this change, we make sure we reuse the variant instance we just calculated for the preview image.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

In particular, when using a primary-replica DB setup, where variants are
created synchronously in a GET request, manually connecting to the
writer. In this case, we might try to load the variant record we just
created in the primary from the replica, not find it due to replication
lag, then process it again (downloading the original preview and
everything), and fail to insert it because it's already there.

Even though we track and memoize the just created `@record` in
`ActiveStorage::VariantWithRecord` when we call `processed`,
we instatiate a new object when we call `ActiveStorage::Blob#variant`.
If we only call this once that's fine, but when processing the variant via
`GET /rails/active_storage/representations/redirect`, we call this
twice: one when setting the representation and processing it, as
```ruby
@representation = @blob.representation(params[:variation_key]).processed
```
and another one when calling `@representation.url` for the redirect,
which calls `ActiveStorage::Preview#url`, which ultimately ends up
calling `ActiveStorage::VariantWithRecord.processed` again, but over a
new instance of `ActiveStorage::VariantWithRecord`. This checks whether
the variant record exists (the one we just created in the previous call
to `processed`), but does so against the replica, which is likely to not
find it if there's any delay (as these calls are very close together).

With this change, we make sure we reuse the variant instance we just
calculated for the preview image.
@jeremy jeremy added this to the 8.2.0 milestone Nov 24, 2025
@jeremy jeremy merged commit 3f888f4 into rails:main Nov 24, 2025
3 of 4 checks passed
yahonda added a commit to yahonda/rails that referenced this pull request Nov 25, 2025
This commit addresses the `Lint/UselessAssignment: Useless assignment to variable - sql.`

```ruby
$ bundle exec rubocop test/controllers/representations/redirect_controller_test.rb
Inspecting 1 file
W

Offenses:

test/controllers/representations/redirect_controller_test.rb:96:7: W: [Correctable] Lint/UselessAssignment: Useless assignment to variable - sql.
      sql = event.payload[:sql]
      ^^^

1 file inspected, 1 offense detected, 1 offense autocorrectable
$
```

This sql is not used, removing this line should pass the test.

Follow up rails#56225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants