Follow Plugin 👨

Thanks for the report. I’ll investigate.

3 Likes

Thanks a million, it’s really unfortunate because otherwise, this plugin is straight up fantastic.

3 Likes

OK I found a bug that caused some PostAlert jobs to fail, causing perpetual retries and I believe the observed user experience of intermittent spam.

That probably caused notifications to pop up with every retry. I could see dozens of attempts for each failed post notification in Production. The problem was, some work was getting done anyway but the Job was getting marked as failed.

Please upgrade this plugin asap (please do not peform a full rebuild if possible if using other Pavilion plugins in case of compatibility issues with other plugins and core, as we are outside the support period).

Once you’ve applied the update the failed jobs should get retried and they will now leave the retry queue, hopefully never to return! :gun: :cowboy_hat_face:

As the retried jobs tick out successfully, there will be a last round of notification spam.

@b481 hopefully this addresses your issue fully

6 Likes

Has this been fixed or this is still a bug in this plugin, if this is fixed i can try this, else i will not install it as it will create orphan notifications and break my users UI if i uninstall the plugin, @angus

2 Likes

This is not a bug. If you uninstall a plugin there is no code to run. Use the script provided for time being.

We may provide admin UI to delete the notifications within the plugin some time in the future so you can do that before uninstalling the plugin.

Btw please do not @ people, especially at the weekend. That’s bad etiquette.

3 Likes

This plugins has many bugs.

  • when disable the plugin all profile pages show 404 page.
  • after delete disable, notifications not showing.
    • Javascript gives this error in console: Uncaught TypeError: Cannot read property ‘dasherize’ of undefined
  • when plugin is active old notificatians appears again and again.

The code and the discourse system are at last version.

No, it does not.

This bug was fixed as notified above. Please update to latest tests-passed and the latest plugin version and provide detailed steps to reproduce.

This is noted in OP and discussed in the Topic. There is a script to run if you remove or disable the plugin. Please read this before posting.

So the only unresolved one I wasn’t aware of is this:

I’ll take a look

2 Likes

Please let me know if that resolves it for you.

2 Likes

I think this fix introduced some new issue or did not fully fix the nil-indexing issue.

We’re seeing tens of thousands of errors related to this on our 400k-user Discourse forum:

Job exception: undefined method `push' for nil:NilClass

We’re also seeing this one thrown much less often, also by this plugin:

Job exception: Couldn't find User with 'id'=xxx

image

We have this version of the plugin installed (the commit before your disabled setting profile fix): GitHub - paviliondev/discourse-follow at d20a90663465d13ecab1c338206bc115fbb3181f

As far as I see, the latest version (the 1 newer commit) doesn’t change anything about the code that is causing errors below.

Stack trace of “undefined method push” error:

/var/www/discourse/plugins/discourse-follow/plugin.rb:165:in `block in author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `each'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `reduce'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:148:in `after_save_post'
/var/www/discourse/plugins/post-approval/plugin.rb:162:in `after_save_post'
/var/www/discourse/app/jobs/regular/post_alert.rb:11:in `execute'
/var/www/discourse/app/jobs/base.rb:232:in `block (2 levels) in perform'
rails_multisite-2.5.0/lib/rails_multisite/connection_management.rb:76:in `with_connection'
/var/www/discourse/app/jobs/base.rb:221:in `block in perform'
/var/www/discourse/app/jobs/base.rb:217:in `each'
/var/www/discourse/app/jobs/base.rb:217:in `perform'
sidekiq-6.1.2/lib/sidekiq/processor.rb:196:in `execute_job'
sidekiq-6.1.2/lib/sidekiq/processor.rb:164:in `block (2 levels) in process'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:138:in `block in invoke'
/var/www/discourse/lib/sidekiq/pausable.rb:138:in `call'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:143:in `invoke'
sidekiq-6.1.2/lib/sidekiq/processor.rb:163:in `block in process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:136:in `block (6 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:111:in `local'
sidekiq-6.1.2/lib/sidekiq/processor.rb:135:in `block (5 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq.rb:38:in `block in <module:Sidekiq>'
sidekiq-6.1.2/lib/sidekiq/processor.rb:131:in `block (4 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:257:in `stats'
sidekiq-6.1.2/lib/sidekiq/processor.rb:126:in `block (3 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:13:in `call'
sidekiq-6.1.2/lib/sidekiq/processor.rb:125:in `block (2 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:78:in `global'
sidekiq-6.1.2/lib/sidekiq/processor.rb:124:in `block in dispatch'
sidekiq-6.1.2/lib/sidekiq/logger.rb:10:in `with'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:33:in `prepare'
sidekiq-6.1.2/lib/sidekiq/processor.rb:123:in `dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:162:in `process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:78:in `process_one'
sidekiq-6.1.2/lib/sidekiq/processor.rb:68:in `run'
sidekiq-6.1.2/lib/sidekiq/util.rb:15:in `watchdog'
sidekiq-6.1.2/lib/sidekiq/util.rb:24:in `block in safe_thread'

Stack trace of “couldn’t find user” error:

activerecord-6.0.3.3/lib/active_record/core.rb:177:in `find'
/var/www/discourse/plugins/discourse-follow/plugin.rb:162:in `block in author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `each'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `reduce'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:148:in `after_save_post'
/var/www/discourse/plugins/post-approval/plugin.rb:162:in `after_save_post'
/var/www/discourse/app/jobs/regular/post_alert.rb:11:in `execute'
/var/www/discourse/app/jobs/base.rb:232:in `block (2 levels) in perform'
rails_multisite-2.5.0/lib/rails_multisite/connection_management.rb:76:in `with_connection'
/var/www/discourse/app/jobs/base.rb:221:in `block in perform'
/var/www/discourse/app/jobs/base.rb:217:in `each'
/var/www/discourse/app/jobs/base.rb:217:in `perform'
sidekiq-6.1.2/lib/sidekiq/processor.rb:196:in `execute_job'
sidekiq-6.1.2/lib/sidekiq/processor.rb:164:in `block (2 levels) in process'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:138:in `block in invoke'
/var/www/discourse/lib/sidekiq/pausable.rb:138:in `call'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:143:in `invoke'
sidekiq-6.1.2/lib/sidekiq/processor.rb:163:in `block in process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:136:in `block (6 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:111:in `local'
sidekiq-6.1.2/lib/sidekiq/processor.rb:135:in `block (5 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq.rb:38:in `block in <module:Sidekiq>'
sidekiq-6.1.2/lib/sidekiq/processor.rb:131:in `block (4 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:257:in `stats'
sidekiq-6.1.2/lib/sidekiq/processor.rb:126:in `block (3 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:13:in `call'
sidekiq-6.1.2/lib/sidekiq/processor.rb:125:in `block (2 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:78:in `global'
sidekiq-6.1.2/lib/sidekiq/processor.rb:124:in `block in dispatch'
sidekiq-6.1.2/lib/sidekiq/logger.rb:10:in `with'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:33:in `prepare'
sidekiq-6.1.2/lib/sidekiq/processor.rb:123:in `dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:162:in `process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:78:in `process_one'
sidekiq-6.1.2/lib/sidekiq/processor.rb:68:in `run'
sidekiq-6.1.2/lib/sidekiq/util.rb:15:in `watchdog'
sidekiq-6.1.2/lib/sidekiq/util.rb:24:in `block in safe_thread'

This is causing significant disruption of our distributed forum deployment because it is retrying PostAlert jobs all the time and sending many duplicate notifications to users (because the job only partially fails, some users keep getting re-notified for the same posts).

2 Likes

I can’t support older commits :smiley:

Please update to latest and give it a whirl.

I’m not seeing any such issue.

That said, something might be going on here:

    def author_replied_followers(post)
      User.find(post.user_id).followers.reduce([]) do |users, user_id|
        user = User.find(user_id).notify_me_when_followed_replies ? User.find(user_id) : nil
        following = user ? user.following.select { |data| data[0] == post.user_id } : nil
        if following && following.last.to_i == Follow::Notification.levels[:watching]
          users.push(user)
        end
      end
    end
1 Like

My bad – just checked and we are running the latest version since a week, and the issues are still occurring (with either version, which makes sense because the latest commit doesn’t touch this code and only adds some enabled checks).

So the issue occurs with latest.

2 Likes

Cool, I’ll take a look.

It’s clear to me already user can be set to nil yet later on this is used without a check for nil, which doesn’t look right.

EDIT: no, its not that - I jumped the gun - looks like users is nil …

2 Likes

From a quick inspection it looks like the reduce isn’t set up properly – it is meant to return the updated value, but right now it only returns non-nil when it adds a user (passes all the checks/if).

If it doesn’t pass that if statement it will set nil as the reduced value, which subsequently errors on the next iteration of the run.

This demonstrates what happens:

[1, 2, 3].reduce(0) { |sum, n|
    if n == 3
        sum + n
    end
}

iterations:

  1. sum = 0, n = 1, returns 1
  2. sum = 1, n = 2, returns 3
  3. sum = 3, n = 3, doesn’t pass the if so returns nil
  4. sum = nil, n = 4, throws
undefined method `+' for nil:NilClass (NoMethodError)

I think the fix is something like:

    def author_replied_followers(post)
      User.find(post.user_id).followers.reduce([]) do |users, user_id|
        user = User.find(user_id).notify_me_when_followed_replies ? User.find(user_id) : nil
        following = user ? user.following.select { |data| data[0] == post.user_id } : nil
        if following && following.last.to_i == Follow::Notification.levels[:watching]
+          return users.push(user)
        end
+       users
      end
    end

(or with an else that returns users)

Please sanity check of course :slightly_smiling_face:

1 Like

I’m not sure.

What angus was doing here was starting with an empty set, and pushing objects to it. That should be legal?

The users object is initialised as [ ], so is NOT nil.

1 Like

The lambda you pass to reduce has to return the updated value. The current one returns nothing if the if-statement isn’t passed. (users.push will return the updated array, it is not actually updating the users value)

2 Likes

Yep, that looks right, thanks. The initial value is irrelevant if it is lost somewhere during a single iteration.

2 Likes

Pushed this. Testing. Feel free to join me!

2 Likes

Thanks, just a note but this code block is starting to look a bit unmaintainable (espec. line 162). Maybe it makes sense to extract the logic whether to include a user to a helper method, and reduce some of the code dupe? (e.g. on User.find(user_id))

def should_notify_follower(user, post)
	user.notify_me_when_followed_replies &&
		following = user.following.select { |data| data[0] == post.user_id } &&
		following.last.to_i == Follow::Notification.levels[:watching]
end

def author_replied_followers(post)
  User.find(post.user_id).followers.reduce([]) do |users, user_id|
	user = User.find(user_id)
	if user && should_notify_follower(user, post)
		users.push(user)
	else
		users
	end
  end
end

(Have not gotten a chance to test above – just an idea. I’ll test it later.)

1 Like

Hold off from testing. It’s not fully resolved. Find can and will still fail. I’ll revert with another iteration.

Agree it could do with some rationalisation. But fix is more urgent.

2 Likes