Inconsistent behavior of dependent: :destroy_async for thehas_many association

[smt116]

Steps to reproduce

1. Generate a new application:

   rails new AsyncDestroyTestApp --skip-action-mailer --skip-action-mailbox --skip-action-text --skip-active-storage --skip-action-cable --skip-sprockets --skip-spring --skip-listen --skip-javascript --skip-turbolinks --skip-jbuilder --skip-test --skip-system-test --skip-bootsnap
   

2. Apply the following diff (add models):

   diff --git a/app/models/book.rb b/app/models/book.rb
   new file mode 100644
   index 0000000..f1e2a65
   --- /dev/null
   +++ b/app/models/book.rb
   @@ -0,0 +1,3 @@
   +class Book < ApplicationRecord
   +  has_many :tags, dependent: :destroy_async
   +end
   diff --git a/app/models/tag.rb b/app/models/tag.rb
   new file mode 100644
   index 0000000..7b23605
   --- /dev/null
   +++ b/app/models/tag.rb
   @@ -0,0 +1,2 @@
   +class Tag < ApplicationRecord
   +end
   diff --git a/db/migrate/20210609112034_add_books_and_tags.rb b/db/migrate/20210609112034_add_books_and_tags.rb
   new file mode 100644
   index 0000000..baed2c0
   --- /dev/null
   +++ b/db/migrate/20210609112034_add_books_and_tags.rb
   @@ -0,0 +1,8 @@
   +class AddBooksAndTags < ActiveRecord::Migration[6.1]
   +  def change
   +    create_table :books
   +    create_table :tags do |t|
   +      t.references :book, foreign_key: true, null: true
   +    end
   +  end
   +end
   diff --git a/db/schema.rb b/db/schema.rb
   new file mode 100644
   index 0000000..fca4c69
   --- /dev/null
   +++ b/db/schema.rb
   @@ -0,0 +1,24 @@
   +# This file is auto-generated from the current state of the database. Instead
   +# of editing this file, please use the migrations feature of Active Record to
   +# incrementally modify your database, and then regenerate this schema definition.
   +#
   +# This file is the source Rails uses to define your schema when running `bin/rails
   +# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to
   +# be faster and is potentially less error prone than running all of your
   +# migrations from scratch. Old migrations may fail to apply correctly if those
   +# migrations use external dependencies or application code.
   +#
   +# It's strongly recommended that you check this file into your version control system.
   +
   +ActiveRecord::Schema.define(version: 2021_06_09_112034) do
   +
   +  create_table "books", force: :cascade do |t|
   +  end
   +
   +  create_table "tags", force: :cascade do |t|
   +    t.integer "book_id"
   +    t.index ["book_id"], name: "index_tags_on_book_id"
   +  end
   +
   +  add_foreign_key "tags", "books"
   +end

Expected behavior

Consistent behavior when removing records asynchronously via passing a new list to the has_many association (replacing). When you delete an object using parent.has_many_attr.first.destroy!, Active Record will update the child's foreign key to nil and schedule a job to delete an associated record from the database. However, when you will replace the "has many" association and delete the child (using parent.update!(has_many_attr: [])) it will only update the foreign key. It won't actually cleanup them from the database:

irb(main):001:0> book = Book.create!(tags: [Tag.new, Tag.new])
   (2.4ms)  SELECT sqlite_version(*)
  TRANSACTION (0.1ms)  begin transaction
  Book Create (1.4ms)  INSERT INTO "books" DEFAULT VALUES
  Tag Create (1.2ms)  INSERT INTO "tags" ("book_id") VALUES (?)  [["book_id", 1]]
  Tag Create (0.2ms)  INSERT INTO "tags" ("book_id") VALUES (?)  [["book_id", 1]]
  TRANSACTION (2.1ms)  commit transaction
=> #<Book:0x00007fcfa24c0c60 id: 1>
irb(main):002:0> Tag.all
  Tag Load (0.3ms)  SELECT "tags".* FROM "tags"
=> [#<Tag:0x00007fcfa258a420 id: 1, book_id: 1>, #<Tag:0x00007fcfa258a2b8 id: 2, book_id: 1>]
irb(main):003:0> book.tags.last.destroy!
  TRANSACTION (0.1ms)  begin transaction
  Tag Destroy (0.8ms)  DELETE FROM "tags" WHERE "tags"."id" = ?  [["id", 2]]
  TRANSACTION (1.5ms)  commit transaction
=> #<Tag:0x00007fcfa24c31b8 id: 2, book_id: 1>
irb(main):004:0> Tag.all
  Tag Load (0.2ms)  SELECT "tags".* FROM "tags"
=> [#<Tag:0x00007fcfa26c37b0 id: 1, book_id: 1>]
irb(main):005:0> book.update!(tags: [])
  TRANSACTION (0.1ms)  begin transaction
  Tag Update All (1.0ms)  UPDATE "tags" SET "book_id" = ? WHERE "tags"."book_id" = ? AND "tags"."id" IN (?, ?)  [["book_id", nil], ["book_id", 1], [nil, 1], [nil, 2]]
  TRANSACTION (1.5ms)  commit transaction
=> true
irb(main):006:0> Tag.all
  Tag Load (0.2ms)  SELECT "tags".* FROM "tags"
=> [#<Tag:0x00007fcfa25288b0 id: 1, book_id: nil>]
irb(main):007:0> book.update!(tags: [Tag.new])
  TRANSACTION (0.1ms)  begin transaction
  Tag Create (1.2ms)  INSERT INTO "tags" ("book_id") VALUES (?)  [["book_id", 1]]
  TRANSACTION (9.4ms)  commit transaction
=> true
irb(main):008:0> Tag.all
  Tag Load (0.3ms)  SELECT "tags".* FROM "tags"
=> [#<Tag:0x00007fcfa2e5b3d8 id: 1, book_id: nil>, #<Tag:0x00007fcfa2e5b310 id: 3, book_id: 1>]

Note that the tag with id = 1 is still there while Active Record should clean it up after calling book.update!(tags: []).

Actual behavior

Inconsistent behavior when replacing "has many" association (deleting some records asynchronously). Associated records are updated with the null value for the foreign key when replacing the association as the whole, but there are no delete jobs scheduled.

Note that using dependant: :destroy works as expected:

irb(main):001:0> book = Book.create!(tags: [Tag.new, Tag.new])
   (1.2ms)  SELECT sqlite_version(*)
  TRANSACTION (0.1ms)  begin transaction
  Book Create (1.3ms)  INSERT INTO "books" DEFAULT VALUES
  Tag Create (0.4ms)  INSERT INTO "tags" ("book_id") VALUES (?)  [["book_id", 1]]
  Tag Create (0.3ms)  INSERT INTO "tags" ("book_id") VALUES (?)  [["book_id", 1]]
  TRANSACTION (1.6ms)  commit transaction
=> #<Book:0x00007fd0d6be2e78 id: 1>
irb(main):002:0> book.update!(tags: [])
  TRANSACTION (0.1ms)  begin transaction
  Tag Destroy (1.3ms)  DELETE FROM "tags" WHERE "tags"."id" = ?  [["id", 1]]
  Tag Destroy (0.4ms)  DELETE FROM "tags" WHERE "tags"."id" = ?  [["id", 2]]
  TRANSACTION (10.0ms)  commit transaction
=> true
irb(main):003:0> Tag.all
  Tag Load (0.3ms)  SELECT "tags".* FROM "tags"
=> []
irb(main):004:0>

System configuration

Rails version: 6.1.3.2

Ruby version: 3.0.1

[ghiculescu]

Thanks for the detailed bug report. Would you be interested in helping to contribute a fix? A huge start would be to add a failing test that replicates the bug: https://github.com/rails/rails/blob/main/activerecord/test/activejob/destroy_association_async_test.rb would be a good place for it to live.

I imagine the bug is somewhere in here https://github.com/rails/rails/blob/main/activerecord/lib/active_record/associations/has_many_association.rb#L10 but we can confirm that once we have a failing test.

If you haven't done it before, https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#contributing-to-the-rails-code runs you through setting up a rails dev environment so you can run the tests locally and make a PR!

[smt116]

@ghiculescu I will prepare a PR with a fix.

[smt116]

I need a little help with this. I'm not sure what is the best way of recognizing that a given "has many through" record is not shared with any other "owner", and thus, can be deleted (see smt116@e535360#diff-80da0f0295c07d53f1ae0aea7b9b77b0563fc5482b32fd23e94dea5835783c39R220 and smt116@e535360#diff-723088e0463253081f6039eafcc905dc2a31d8fad496647cf6c9244b5bbec80bR94-R115).

/cc @ghiculescu

I've submitted a draft for this. It will be easier to discuss things with a context. See #42452

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

[smt116]

This is still relevant. The PR awaits code review (see #42452).

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

[smt116]

@rails-bot This is still relevant and the PR is pending.

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

[smt116]

@rails-bot This is still relevant.