Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: Gotify version check makes webook initialization unreliable #215

Closed
2 tasks done
megatwig opened this issue Oct 27, 2024 · 3 comments · Fixed by #216
Closed
2 tasks done

bug: Gotify version check makes webook initialization unreliable #215

megatwig opened this issue Oct 27, 2024 · 3 comments · Fixed by #216
Labels
bug Something isn't working fix waiting for release Fix has been committed and waiting for a release to be cut

Comments

@megatwig
Copy link

Please check existing knowledge before opening an issue

Describe the Bug

Gotify webhooks are initialized by calling the /version endpoint, and the webhook is only used if this call succeeds.

This check is never retried, so a transient error with your Gotify instance/your internet connection could mean no webhook notifications until multi-scrobbler is restarted. This could also happen if e.g. Gotify and multi-scrobbler are both running as docker containers on the same host, and multi-scrobbler starts up before gotify is ready.

const resp = await request.get(`${url}/version`);
this.logger.verbose(`Initialized. Found Server version ${resp.body.version}`);
this.initialized = true;

imo webhooks should be set up unconditionally - the only benefit I can see for failing on this version check is to avoid potentially sending out failing requests for the rest of the lifetime of the application, which is unlikely to cause major issues.

Platform

Docker

Versions

  • multi-scrobbler 0.8.6 on Docker

Logs

No response

Additional Context

nb: my true motivation for this is actually that I use Pushbits as a notification server which has a Gotify-compatible API but no /version endpoint. I filed an issue with them as it's not really compatible if it's missing API methods, but I'm not sure it's receiving active feature development

@megatwig megatwig added the bug Something isn't working label Oct 27, 2024
@FoxxMD
Copy link
Owner

FoxxMD commented Oct 28, 2024

Thanks for the detailed report. The intention behind checking version was mainly to let the user know if they have the URL for the endpoint correct. Since notifications aren't mission critical you are right it should send regardless and the check should instead just be a logged warning.

FoxxMD added a commit that referenced this issue Oct 28, 2024
* Notifications are not mission critical so its okay if they fail on init because service outage may be transient. Always init notification services and always attempt to push (we catch failures anyway)
* Parse more info from notification URL and test simple reachability of host/port to make troubleshooting easier

Fixes #215
@FoxxMD
Copy link
Owner

FoxxMD commented Oct 28, 2024

Please try the docker image foxxmd/multi-scrobbler:pr-216 and let me know if this fixes things for you.

@FoxxMD FoxxMD closed this as completed in 65012d4 Dec 10, 2024
FoxxMD added a commit that referenced this issue Dec 10, 2024
feat(notifications): Make init more granular and always complete init
@FoxxMD
Copy link
Owner

FoxxMD commented Dec 10, 2024

Available in edge docker image. Will comment when a release is cut that includes it.

@FoxxMD FoxxMD added the fix waiting for release Fix has been committed and waiting for a release to be cut label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix waiting for release Fix has been committed and waiting for a release to be cut
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants