Skip to content

cannon: Set warp timeout to default 30s from > 1 hour#4994

Merged
akshaymankar merged 8 commits intodevelopfrom
cannon-ping-freq
Feb 4, 2026
Merged

cannon: Set warp timeout to default 30s from > 1 hour#4994
akshaymankar merged 8 commits intodevelopfrom
cannon-ping-freq

Conversation

@akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Jan 29, 2026

Reduce ping interval to 15s, this is also what most webapps use.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@akshaymankar akshaymankar requested review from a team as code owners January 29, 2026 15:38
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 29, 2026
Reduce ping interval to 20s, this is also what most webapps use.
Copy link
Contributor

@supersven supersven left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@akshaymankar akshaymankar merged commit a704c85 into develop Feb 4, 2026
10 checks passed
@akshaymankar akshaymankar deleted the cannon-ping-freq branch February 4, 2026 10:23
@marcoconti83
Copy link
Member

The PR description says 20 seconds, but PR title and the changelog file you edited says 30.
The code says 20 :)

Set idle timeout on HTTP connections to 30s No newline at end of file
Set idle timeout on HTTP connections to 30s. Set ping interval to 15s in cannon,
missing two pings will cause the connection to close. This also removes ability
for the client to control the ping interval. (#4992, ##) No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

In this contex, is "the client" anothe backend service, or the frontend? If the latter, we need to double check that no frontend client sets the interval to 30s+, otherwise they will keep be disconnected every 30s. I'm not saying they should do that, I'm saying it will impact them (and they should be fixed, but then should we roll this out)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its the frontend. The webapp doesn't even have a way to use this feature since the browser APIs don't allow sending pings. I remember asking about this few months ago, but we can check again.

The websocket doesn't break if they try to use this feature, they will just not be able to control how often the backend sends pings.

Copy link
Member

Choose a reason for hiding this comment

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

So it's for clients to ask the backend to send a ping (backend->client) at an interval of their chosing? I had assumed the other way around. Ok this makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, my wording could definitely be better here. Sorry about the confusion.

@akshaymankar
Copy link
Member Author

The PR description says 20 seconds, but PR title and the changelog file you edited says 30. The code says 20 :)

Those configs are unused, initially I was going to make all this very extensible, but then I realized this is just a way for us to cause ourselves problems. So I removed it, then I forgot to delete these config options. I will create another PR to clean this up.

We're using defaultPingPongOptions from the websockets library: https://hackage.haskell.org/package/websockets-0.13.0.0/docs/Network-WebSockets-Connection-PingPong.html#v:defaultPingPongOptions

So the change log is correct. I will update the PR description to match.

akshaymankar added a commit that referenced this pull request Feb 9, 2026
This option was never exposed in the helm chart config, so noone ever configured
it. The code using this was removed in
#4994
@akshaymankar akshaymankar mentioned this pull request Feb 9, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants