Skip to content

Fix missing ticker.Stop() in pollForSettingChanges#1148

Merged
brandur merged 1 commit intoriverqueue:masterfrom
GiGurra:fix/poll-settings-ticker-leak
Feb 15, 2026
Merged

Fix missing ticker.Stop() in pollForSettingChanges#1148
brandur merged 1 commit intoriverqueue:masterfrom
GiGurra:fix/poll-settings-ticker-leak

Conversation

@GiGurra
Copy link
Contributor

@GiGurra GiGurra commented Feb 13, 2026

Summary

  • pollForSettingChanges creates a time.NewTicker but never calls Stop() on it when the context is cancelled, leaking the ticker's internal runtime timer
  • Other functions in the same file (heartbeatLogLoop, reportQueueStatusLoop) correctly use defer ticker.Stop() — this aligns with that pattern
  • Note: pollForSettingChanges only runs when no notifier is configured (poll-only mode), so this only affects non-LISTEN/NOTIFY setups

Test plan

  • Existing TestProducer_PollOnly exercises this code path
  • Existing test suite passes

🤖 Generated with Claude Code

The pollForSettingChanges function creates a ticker but never calls
Stop() on it when the context is cancelled, leaking the ticker's
internal runtime timer.

Other functions in the same file (heartbeatLogLoop, reportQueueStatusLoop)
correctly use defer ticker.Stop(). This aligns with that pattern.

Note: pollForSettingChanges only runs when no notifier is configured
(poll-only mode), so this only affects non-LISTEN/NOTIFY setups.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@brandur
Copy link
Contributor

brandur commented Feb 14, 2026

@GiGurra Can you add yourself to the CLA repo? https://github.com/riverqueue/rivercla

@GiGurra
Copy link
Contributor Author

GiGurra commented Feb 15, 2026

@GiGurra Can you add yourself to the CLA repo? https://github.com/riverqueue/rivercla

Alright, something like this, right?
riverqueue/rivercla#20

@brandur
Copy link
Contributor

brandur commented Feb 15, 2026

@GiGurra Yep, that'll do it.

Copy link
Contributor

@brandur brandur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GiGurra Thanks! This looks good.

Just heads up: we don't have a clear policy on AI-generated contributions right now, but we may have to consider adding a little bit of friction to the process because if they become large in number (e.g. what projects like Ghostty are seeing right now), they could easily overwhelm our review bandwidth.

These changes largely look pretty legitimate though, so no worries for now. In case you do send more, would you mind compacting the commit messages a bit more? The summary section is okay, but I don't think we need the list of checkboxes.

@brandur brandur merged commit f4c871e into riverqueue:master Feb 15, 2026
11 checks passed
@GiGurra
Copy link
Contributor Author

GiGurra commented Feb 15, 2026

@GiGurra Thanks! This looks good.

Just heads up: we don't have a clear policy on AI-generated contributions right now, but we may have to consider adding a little bit of friction to the process because if they become large in number (e.g. what projects like Ghostty are seeing right now), they could easily overwhelm our review bandwidth.

These changes largely look pretty legitimate though, so no worries for now. In case you do send more, would you mind compacting the commit messages a bit more? The summary section is okay, but I don't think we need the list of checkboxes.

Yes, no problem. I'll make sure to remove them in the future :) (I must admit I was a bit lazy in these PRs with regards to tweaking the auto generated descriptions and commit messages).

Full disclosure, I was quite lazy overall in these PRs, but I hope they can be of some use. Background: I had seen some odd performance degradations over time at work (where we use river). I wanted, initially just for fun, to let opus 4.6 have a look at the river source code, to see if it could find any obvious places for improvements (and potential culprits for our performance degradations). 3 of these 4 PRs are mostly just small things, but the jitter one could potentially be what we need to solve our problems at work.

brandur added a commit that referenced this pull request Feb 15, 2026
Adds changelog entries for #1147, #1148, and #1149.

Also make a couple small tweaks: one to alphabetize a newly added
property, and one to move a success case test to above the error case,
which is a little more conventional.
@brandur
Copy link
Contributor

brandur commented Feb 15, 2026

Yes, no problem. I'll make sure to remove them in the future :) (I must admit I was a bit lazy in these PRs with regards to tweaking the auto generated descriptions and commit messages).

Awesome, thanks! And actually, I just noticed that the checkboxes are in the pull request description but not the commit messages, so actually, that's okay if you want to keep them going like that. (Mainly, just don't want them adding noise to Git history.)

Full disclosure, I was quite lazy overall in these PRs, but I hope they can be of some use. Background: I had seen some odd performance degradations over time at work (where we use river). I wanted, initially just for fun, to let opus 4.6 have a look at the river source code, to see if it could find any obvious places for improvements (and potential culprits for our performance degradations). 3 of these 4 PRs are mostly just small things, but the jitter one could potentially be what we need to solve our problems at work.

Nice! Yep, very cool.

brandur added a commit that referenced this pull request Feb 15, 2026
Adds changelog entries for #1147, #1148, and #1149.

Also make a couple small tweaks: one to alphabetize a newly added
property, and one to move a success case test to above the error case,
which is a little more conventional.
brandur added a commit that referenced this pull request Feb 15, 2026
Adds changelog entries for #1147, #1148, #1149, and #1150.

Also make a couple small tweaks: one to alphabetize a newly added
property, and one to move a success case test to above the error case,
which is a little more conventional.
brandur added a commit that referenced this pull request Feb 15, 2026
#1151)

Adds changelog entries for #1147, #1148, #1149, and #1150.

Also make a couple small tweaks: one to alphabetize a newly added
property, and one to move a success case test to above the error case,
which is a little more conventional.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants