I’ve been looking into consistency problem with association counter caches where the counter cache value in the database is not consistent with the actual number of records in the association. What I’ve found is that it is from a concurrency issue where two process try to destroy the same record at the same time. Here is the pseudo SQL that is sent to the database when two process are deleting at the same time:
process_1 -> SELECT * FROM table WHERE id = 1
process_2 -> SELECT * FROM table WHERE id = 1
process_1 -> BEGIN
process_2 -> BEGIN
process_1 -> UPDATE parent_table SET counter_cache = COALESCE(counter_cache, 0) - 1 WHERE id = 1
process_1 -> DELETE FROM table WHERE id = 1
process_1 -> COMMIT
process_2 -> UPDATE parent_table SET counter_cache = COALESCE(counter_cache, 0) - 1 WHERE id = 1
process_2 -> DELETE FROM table WHERE id = 1
process_2 -> COMMIT
What happens is process_1 updates the counter cache and deletes the record. Process_2 simply updates the counter cache because the record is already deleted by the time it tries to delete it.
This is a pretty complicated issue and it touches more than just this one test case. The problem being that all the before and after destroy callbacks will be called regardless of if the record is actually destroyed. In the particular case of the counter caches, I think it could be fixed by moving the callback from a before_destroy to an after_destroy and adding a check in ActiveRecord to only call after destroy callbacks if a row was actually removed from the table.
In general I think it would be correct to make this general behavior so that after_destroy callbacks are not called if no record was deleted. However, that could affect quite a few things inside application code which could potentially leave objects in an inconsistent state because an expected callback was not called. I think the pending upgrade to Rails 4.0 might be a good time to introduce such behavior since it’s a major upgrade and as such people should not be expecting applications to work 100% without some alterations. This does not touch on the issue of before_destroy callbacks which would not be able to check the status of the delete operation. This could be handled with documentation stating that this is a known issue.
Another solution that would have less effect on current applications (but also leave them more vulnerable to being in an inconsistent state) would be to provide some sort of flag within the record that after_destroy callbacks could check if they are persisting data or interacting with external systems. Something like “row_deleted?” so that callbacks could be defined as:
after_destroy :my_callback, :if => :row_deleted?