FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix based on Coverity and Sonar audits (part 11) by stelfrag · Pull Request #22339 · netdata/netdata · GitHub

Fix based on Coverity and Sonar audits (part 11) - #22339

Merged
stelfrag merged 3 commits into
netdata:masterfrom
stelfrag:cov_fix_202604_part11
May 7, 2026
Merged

Fix based on Coverity and Sonar audits (part 11)#22339
stelfrag merged 3 commits into
netdata:masterfrom
stelfrag:cov_fix_202604_part11

Conversation

Copy link
Copy Markdown
Collaborator

ktsaou added 3 commits April 30, 2026 10:27
Sonar python:S7497: connect_with_backoff() caught CancelledError during
the reconnect-delay wait but swallowed it with `pass`. A parent-driven
cancellation (e.g., process shutdown) could be lost and the outer
`while True` reconnect loop would continue.

Replace `pass` with `raise` to propagate the cancellation. The
preceding lines already perform local cleanup (cancelling helper
tasks, clearing retry_event), so no additional cleanup is needed
before re-raising.

The similar handler at line 312 is a different pattern (cancel-then-
await on a child task this code just cancelled); it intentionally
suppresses the expected CancelledError from the awaited task and is
addressed in a separate finding.
…agates

Sonar python:S7497: the cleanup loop after asyncio.wait() cancelled
each pending task and awaited it inside `try/except CancelledError:
pass`. The handler swallowed the expected CancelledError from the
just-cancelled child, but it would also swallow a CancelledError from
the OUTER `connect_with_backoff` coroutine if the parent arrived
mid-await -- breaking shutdown propagation.

Cancel all pending tasks first, then drain them via
`asyncio.gather(..., return_exceptions=True)`. Per-child CancelledError
is collected as a result and ignored; non-cancellation exceptions are
re-raised. If `connect_with_backoff` itself is cancelled while awaiting
the gather, gather propagates the cancellation unconditionally.

Side-benefit: the original sequential loop would skip cancelling later
tasks if an earlier task raised non-CancelledError. The gather form
cancels all up front, so cleanup is complete regardless of outcome.
Cancel both helper tasks and clear the retry event before re-raising asyncio.CancelledError so reconnect-delay cancellation does not leave dangling state behind.

Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

cubic-dev-ai Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@stelfrag I have started the AI code review. It will take a few minutes to complete.

stelfrag marked this pull request as ready for review May 1, 2026 07:26
Copilot AI review requested due to automatic review settings May 1, 2026 07:26
stelfrag marked this pull request as draft May 1, 2026 07:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

Improves asyncio cancellation handling in the MCP stdio-python bridge to avoid leaving background tasks running across reconnect/shutdown paths.

Changes:

  • Propagate asyncio.CancelledError during reconnect backoff after cancelling in-flight delay/wakeup tasks.
  • Cancel and drain pending stdin/websocket processor tasks using asyncio.gather(..., return_exceptions=True) and re-raise non-cancellation exceptions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

stelfrag marked this pull request as ready for review May 4, 2026 08:28

thiagoftsm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Algorithm received good improvements. LGTM!

stelfrag merged commit 735549f into netdata:master May 7, 2026
160 of 161 checks passed
stelfrag deleted the cov_fix_202604_part11 branch May 7, 2026 03:52
stelfrag mentioned this pull request Jun 22, 2026
Ferroin pushed a commit that referenced this pull request Jul 15, 2026
* nd-mcp: re-raise asyncio.CancelledError in reconnect-delay handler

Sonar python:S7497: connect_with_backoff() caught CancelledError during
the reconnect-delay wait but swallowed it with `pass`. A parent-driven
cancellation (e.g., process shutdown) could be lost and the outer
`while True` reconnect loop would continue.

Replace `pass` with `raise` to propagate the cancellation. The
preceding lines already perform local cleanup (cancelling helper
tasks, clearing retry_event), so no additional cleanup is needed
before re-raising.

The similar handler at line 312 is a different pattern (cancel-then-
await on a child task this code just cancelled); it intentionally
suppresses the expected CancelledError from the awaited task and is
addressed in a separate finding.

* nd-mcp: drain cancelled tasks via gather() so outer cancellation propagates

Sonar python:S7497: the cleanup loop after asyncio.wait() cancelled
each pending task and awaited it inside `try/except CancelledError:
pass`. The handler swallowed the expected CancelledError from the
just-cancelled child, but it would also swallow a CancelledError from
the OUTER `connect_with_backoff` coroutine if the parent arrived
mid-await -- breaking shutdown propagation.

Cancel all pending tasks first, then drain them via
`asyncio.gather(..., return_exceptions=True)`. Per-child CancelledError
is collected as a result and ignored; non-cancellation exceptions are
re-raised. If `connect_with_backoff` itself is cancelled while awaiting
the gather, gather propagates the cancellation unconditionally.

Side-benefit: the original sequential loop would skip cancelling later
tasks if an earlier task raised non-CancelledError. The gather form
cancels all up front, so cleanup is complete regardless of outcome.

* nd-mcp: address PR-review findings

Cancel both helper tasks and clear the retry event before re-raising asyncio.CancelledError so reconnect-delay cancellation does not leave dangling state behind.

---------

Co-authored-by: Costa Tsaousis <costa@netdata.cloud>
(cherry picked from commit 735549f)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL