fix ws killpill

[HugoCasa]

Important

Introduce a kill switch for websocket triggers to enable graceful shutdown and improve trigger management.

• Websocket Triggers:
  • Add killpill_rx to start_websockets() in websocket_triggers.rs for graceful shutdown.
  • Modify listen_to_websocket() to handle killpill_rx for stopping connections.
  • Introduce listen_to_unlistened_websockets() for efficient trigger management.
• Server Initialization:
  • Update run_server() in lib.rs to pass ws_killpill_rx to start_websockets().
• Query Changes:
  • Minor formatting change in SQL query in query-5303cb9dd5903aa4791ef8e5e5881a50a832e65c8c9632e2e12cd9c2747f2fc7.json.

^{This description was created by for 74f9379. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to ee081c0 in 32 seconds

More details

• Looked at 312 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. backend/windmill-api/src/websocket_triggers.rs:350

• Draft comment:
  Be cautious with killpill_rx.resubscribe() here. If maybe_listen_to_websocket is called multiple times, it could lead to multiple subscriptions, which might not be the intended behavior.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new function listen_to_unlistened_websockets which is called both at the start and in the loop of start_websockets. This is a good refactoring for code reuse. However, the killpill_rx is resubscribed in maybe_listen_to_websocket which might lead to multiple subscriptions if the function is called multiple times. This could be a potential issue if not handled properly.

2. backend/windmill-api/src/websocket_triggers.rs:368

• Draft comment:
  Consider whether biased; is necessary here. It prioritizes the first branch, which might not be the intended behavior if equal priority is desired for all branches.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The tokio::select! macro is used to handle multiple asynchronous operations. However, the biased; keyword is used, which prioritizes the first branch. This might not be the intended behavior if equal priority is desired.

3. backend/windmill-api/src/websocket_triggers.rs:537

• Draft comment:
  Consider whether biased; is necessary here. It prioritizes the first branch, which might not be the intended behavior if equal priority is desired for all branches.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The tokio::select! macro is used to handle multiple asynchronous operations. However, the biased; keyword is used, which prioritizes the first branch. This might not be the intended behavior if equal priority is desired.

Workflow ID: wflow_FsmCoqI2QcXBJSX0

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	9c3f1ec
Status:	⚡️  Build in progress...

View logs

[ellipsis-dev]

👍 Looks good to me! Incremental review on 74f9379 in 9 seconds

More details

• Looked at 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. backend/windmill-api/src/websocket_triggers.rs:595

• Draft comment:
  Typo fixed in the error message: 'cloesd' corrected to 'closed'.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The PR description mentions a fix for a typo in the error message. The typo is indeed present in the original code and has been corrected in the PR.

Workflow ID: wflow_E0UXqWWJTH8Zb3tQ

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.