Document Rails 4.0 bidirectional dependent: :destroy recursion

rails-upgrade/CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog
 
 ## Unreleased
+- Documented the Rails 4.0 bidirectional `dependent: :destroy` recursion in `version-guides/upgrade-3.2-to-4.0.md` (new MEDIUM entry 14, existing LOW entries renumbered 15-25) and added a matching `BELONGS_TO_DEPENDENT` detection pattern to `rails-40-patterns.yml` with fixture expectations. Rails 3.2 registered `belongs_to ..., dependent: :destroy` as an after_destroy (`belongs_to.rb`'s belongs_to-specific `configure_dependency`); Rails 4.0 dropped that special case and registers all three macros as before_destroy from the shared `association.rb` builder. Two models that each cascade at the other therefore terminate on 3.2 and recurse forever on 4.0 — `SystemStackError` in tests, a hang that times out into a 5xx in a request. Silent on upgrade: no deprecation warning, boots fine, and only fires on the delete path. Fixed upstream in Rails 5.0 by rails/rails#18548, not backported. The recommended fix moves the non-driven cascade to an explicit `after_destroy` (restoring the 3.2 ordering) rather than deleting one `dependent:`, which stops the loop but silently orphans rows; the entry also records the two caveats that come with it (soft-delete default scopes, and the loss of the cascade's ability to veto the destroy).
 - `bin/validate-patterns` now accepts an optional `kind:` field on every pattern entry. Allowed values: `breaking`, `deprecation`, `migration`, `optional`. The validator rejects unknown values to guard against typos. The field is optional during the issue #53 rollout and becomes required once every pattern file has been classified. Documented the rubric in `CLAUDE.md` under "Assigning `kind:`".
 - `bin/validate-patterns` now **requires** the `kind:` field on every pattern entry (issue #53 rollout, sub-issue #67). All 12 pattern files have been classified across the preceding 12 sub-issues. Updated `CLAUDE.md` to list `kind:` among the required per-pattern fields and to remove the rollout caveat.
 - Renamed the top-level pattern-file key `breaking_changes:` → `upgrade_findings:` (issue #53 rollout, sub-issue #68). The old name was a misnomer — most entries are deprecations, migrations, or opt-in features rather than hard-breaking changes. Touched all 12 `rails-*-patterns.yml` files, `bin/validate-patterns` (the `TOP_LEVEL_KEYS` constant and the lookup variable in `validate`), and `rails-upgrade/workflows/direct-detection-workflow.md` (5 references). The validator now rejects files that still use the old key.

rails-upgrade/detection-scripts/patterns/rails-40-patterns.expectations.yml

@@ -458,3 +458,25 @@ RACK_LOCK_MIDDLEWARE:
     - '    config.middleware.use Rack::Lock'
     # commented-out line is inert
     - '    # config.middleware.insert_before Rack::Lock'
+
+BELONGS_TO_DEPENDENT:
+  match:
+    - "  belongs_to :document, dependent: :destroy"
+    - "  belongs_to :content, polymorphic: true, dependent: :destroy"
+    - "  belongs_to :document, foreign_key: :resource_id, dependent: :destroy"
+    # 3.2-era hash-rocket syntax is common in the code being upgraded.
+    - "  belongs_to :document, :dependent => :destroy"
+    # Commented-out declarations are flagged on purpose: a commented cascade is
+    # still worth a look when tracing a cycle, and dropping it is a one-line dismissal.
+    - "  # belongs_to :document, dependent: :destroy"
+  no_match:
+    # dependent: :delete deletes without running callbacks, so it cannot close a
+    # cycle. :destroy and :delete are the only options belongs_to accepts (4.0
+    # builder/belongs_to.rb valid_dependent_options), so :destroy is the full set.
+    - "  belongs_to :brand, dependent: :delete"
+    # Only belongs_to moved phase in 4.0; has_one/has_many were already before_destroy.
+    - "  has_one :attachment, dependent: :destroy"
+    - "  has_many :comments, dependent: :destroy"
+    # A belongs_to without a cascade is unaffected.
+    - "  belongs_to :creator, class_name: \"Person\""
+    - "  belongs_to :organization"

rails-upgrade/detection-scripts/patterns/rails-40-patterns.yml

@@ -549,6 +549,17 @@ upgrade_findings:
       fix: "Form A — bare `Model.scoped` (no args). Pick per call-site by looking at what comes after. A1 (terminal, no further chaining): `Model.scoped` → `Model.all`. On Rails 3.2 `.all` materializes to an Array; on 4.0+ it returns a Relation. Iteration via `.each`/`.map` works on both. A2 (chained, followed by `.joins`/`.where`/`.merge`): `Model.scoped` → `Model.where(nil)`. `where(nil)` returns a Relation with no conditions on both 3.2 and 4.0+, preserving chaining. Example: `scope :foo, -> { scoped.joins(:bar) }` → `scope :foo, -> { where(nil).joins(:bar) }`. Form B — `Model.scoped(hash)`. Break the hash into chained methods: `conditions:` → `.where`, `include:` → `.includes`, `order:` → `.order`, `joins:` → `.joins`, `select:` → `.select`, `limit:` → `.limit`, `offset:` → `.offset`, `group:` → `.group`, `having:` → `.having`, `readonly: true` → `.readonly(true)`. All chains work on Rails 3.2 and 4.0+. False-positive watch: a local variable, symbol, or string literally named `scoped` (rare) — `\\b` boundaries already rule out `unscoped` / `default_scoped` / `scoped_ids` / `x_scoped`, so the residual noise is only a bare `scoped` token that is not the AR method"
       variable_name: "CLASS_SCOPED"
 
+    - name: "belongs_to with dependent: (bidirectional destroy cycle)"
+      kind: "breaking"
+      pattern: "belongs_to\\s+:[a-z_]+.*dependent\\s*(?::|=>)\\s*:destroy"
+      exclude: ""
+      search_paths:
+        - "app/models/"
+        - "lib/"
+      explanation: "Rails 3.2 registers `belongs_to ..., dependent: :destroy` as an after_destroy (a belongs_to-specific `configure_dependency` in activerecord-3.2.x/lib/active_record/associations/builder/belongs_to.rb, `model.after_destroy method_name`). Rails 4.0 drops that special case and registers all three macros from the shared builder as a before_destroy (activerecord-4.0.x/lib/active_record/associations/builder/association.rb, `model.before_destroy \"#{macro}_dependent_for_#{name}\"`). has_many and has_one were before_destroy on both versions; only belongs_to moved. Any two models that each declare `dependent: :destroy` at the other therefore terminate on 3.2 and recurse forever on 4.0: on 3.2 the belongs_to side fired after its own row was deleted so the reciprocal found nothing, while on 4.0 both fire with both rows still present and a.destroy -> b.destroy -> a.destroy never ends. Each lap reloads from the database, so nothing detects the repetition — SystemStackError in tests, and in a request a hang that times out into a 5xx. Silent on upgrade: no deprecation warning, the app boots, and it only fires on the delete path. Fixed in Rails 5.0 by rails/rails#18548, not backported. This pattern flags one side only; a regex cannot see the pair, so treat each hit as `check the target model for a reciprocal cascade`. It matches both `dependent: :destroy` and the 3.2-era `:dependent => :destroy`, and deliberately does not skip commented-out lines — a commented cascade is still useful when tracing a pair, and it is a one-line dismissal. `dependent: :delete` is excluded on purpose: it deletes without running callbacks, so it cannot close a cycle, and `:destroy` / `:delete` are the only two values belongs_to accepts (activerecord-4.0.x builder/belongs_to.rb `valid_dependent_options`), so `:destroy` is the complete set of cycle-capable values. One remaining limit: the match is single-line, so a declaration wrapped across lines (`belongs_to :document,` newline `dependent: :destroy`) is missed. To enumerate cycles across a whole app, and to catch the wrapped declarations this regex misses, walk reflections instead: for every association with a cascading `dependent:`, resolve the target (for a polymorphic belongs_to, through the models declaring the matching `as:`) and look for an edge pointing back"
+      fix: "Only a reciprocal pair is a bug — a lone `belongs_to ..., dependent: :destroy` is fine, so check the target model first for a has_one/has_many pointing back with `dependent: :destroy`. When there is a pair, move the cascade the app does NOT drive to an explicit after_destroy, which restores the 3.2 ordering: `has_one :attachment, dependent: :destroy` becomes `has_one :attachment` plus `after_destroy :destroy_attachment` with `def destroy_attachment; attachment.destroy if attachment; end`. By then the row is gone, so the reciprocal re-reads, resolves to nil, and stops after one bounce. Prefer this over simply deleting one `dependent:`, which stops the loop but silently orphans rows. Three caveats: that termination assumes the reciprocal association is re-read from the database — with `inverse_of:` on the pair (available in 4.0; only the automatic detection arrived in 4.1) or an already-loaded target, `attachment.document` returns the in-memory destroyed record instead of nil, its before_destroy fires again, and the loop survives the fix, since 4.0 has no re-entrancy guard (Rails 5.0 added `@_destroy_callback_already_called`). Verify the pair does not declare `inverse_of:`, or make the callback re-entrant with an instance flag: `def destroy_attachment; return if @_destroying_attachment; @_destroying_attachment = true; attachment.destroy if attachment; end`. A `destroyed?` / `frozen?` check does not work here — neither flag is set until the destroy completes, so the second pass still sees a live-looking record. Also: if the model soft-deletes, `gone` means excluded by the default scope rather than deleted — verify that terminates for your soft-delete implementation; and the cleanup can no longer veto the destroy, since as a before_destroy a failed cascade halted everything and returned false, whereas from after_destroy the row is already deleted, so a failure leaves an orphan and destroy still reports success. Raise from the callback if the all-or-nothing behavior matters"
+      variable_name: "BELONGS_TO_DEPENDENT"
+
   low_priority:
     - name: "request.env.merge! in tests"
       kind: "optional"